Re: [Freeipa-devel] [PATCHES] changes in preparation of replica promotion work
On Thu, 2015-08-27 at 11:05 +0200, Jan Cholasta wrote: On 27.8.2015 07:56, Jan Cholasta wrote: On 25.8.2015 20:43, Simo Sorce wrote: On Wed, 2015-08-05 at 11:24 -0400, Simo Sorce wrote: On Wed, 2015-08-05 at 08:20 +0200, Jan Cholasta wrote: Hi, Dne 31.7.2015 v 12:46 Simo Sorce napsal(a): I've been carrying these patches in my tree for a while, I think it is time to put them in master as they stand on their own. Simo. Patch 530: ACK Patch 531: ACK Patch 532: The methods should be static methods: @staticmethod def setOption(name, value): ... Care to explain why ? @staticmethod is not used anywhere else in that file. Rebased patches on master, made requested change +1 more patch. Simo. Patch 532: ACK Patch 533: ACK Pushed to master: f57b687241fbc92d1138507210e87e9de465c507 Honza Actually, there is a problem with patch 531: SASL mapping are added only on replica. The attached patch fixes it. This will break the promotion code, which needs to add the real sasl mappings later in the process. Can you leave the step in the non-common part of the setup for both server and replica installs ? Simo. -- Simo Sorce * Red Hat, Inc * New York -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCHES 478-479] cert renewal: KRA agent certificate update
On 27.8.2015 10:34, Alexander Bokovoy wrote: On Thu, 27 Aug 2015, Jan Cholasta wrote: Hi, the attached patches fix https://fedorahosted.org/freeipa/ticket/5253. ACK to both patches. Thanks. Pushed to: master: e9a76c3d126367f72e353919ecbff45bed3abaaf ipa-4-2: cea66362621f6ab6219a689c2da4025c37f496bb -- Jan Cholasta -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCHES] 0696-0710 More modernization
On 27.8.2015 12:17, Petr Viktorin wrote: On 08/27/2015 09:07 AM, Jan Cholasta wrote: On 26.8.2015 12:15, Petr Viktorin wrote: On 08/26/2015 09:47 AM, Jan Cholasta wrote: On 25.8.2015 15:05, Petr Viktorin wrote: On 08/25/2015 12:39 PM, Christian Heimes wrote: On 2015-08-24 17:31, Petr Viktorin wrote: 0701.2-Use-Python3-compatible-dict-method-names NACK Why are you replacing iteritems() with items() instead of using six.iteritems()? It looks cleaner, and it will be easier to clean up after six is dropped. Also, the performance difference is negligible if the whole thing is iterated over. (On small dicts, which are the majority of what iteritems was used on, items() is actually a bit faster on my machine.) Right, for small dicts the speed difference is negligible and favors the items() over iteritems(). For medium sized and large dicts the iterators are faster and consume less memory. I'm preferring iterator methods everywhere because I don't have to worry about dict sizes. 0710.2-Modernize-use-of-range NACK Please use six.moves.range. It defaults to xrange() in Python 2. Why? What is the benefit of xrange in these situations? Like with iteritems in 0701, when iterating over the whole thing, the performance difference is negligible. I don't think a few microseconds outside of tight loops are worth the verbosity. It's the same reasoning as in 0701. As long as you have a small range, it doesn't make a difference. For large ranges the additional memory usage can be problematic. In all above cases the iterator methods and generator functions are a safer choice. A malicious user can abuse the non-iterative methods for DoS attacks. As long as the input can't be controlled by a user and the range/dict/set/list is small, the non-iterative methods are fine. You have to verify every location, though. Keep in mind that for dicts, all the data is in memory already (in the dict). Are you worried about DoS from an extra list of references to existing objects? I'm usually too busy with other stuff (aka lazy) to verify these preconditions... I changed one questionable use of range. The other ones are: ipalib/plugins/dns.py: for i in range(len(relative_record_name) (max. ~255, verified by DNSName) ipaserver/install/dnskeysyncinstance.py: for _ in range(0, 16)) (16) ipaserver/install/ipa_otptoken_import.py: for i in range(1, blocks + 1): (about 7) ipaserver/install/ipa_otptoken_import.py: for k in range(mac.digest_size): (16) ipatests/data.py: for d in range(256)) (256) Plus tests, where it's rarely above 10. I have just pushed Michael's python-krbV removal patch, which conflicts with your patches. Could you please rebase them on top of current master? Sure, here they are. Patches 696-699: ACK Patch 700: There are some error messages which contain basestring. Do we want to fix these as well? No, I think it's okay as a term. When Python 2 is dropped it can be shortened to str. Patch 701: Since we are using python-six, shouldn't six.PY2 be used instead of sys.version_info (3, 0)? Right, it looks a bit better. Patches 702-709: ACK Patch 710: There are some xranges in ipapython/p11helper.py and ipatests/test_xmlrpc/test_add_remove_cert_cmd.py. Thanks, fixed. Patch 711: ACK Thanks, ACK. Christian, if you don't have any objections, I will push the patches. -- Jan Cholasta -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCHES] changes in preparation of replica promotion work
On Thu, 2015-08-27 at 16:02 +0200, Jan Cholasta wrote: On 27.8.2015 14:34, Simo Sorce wrote: On Thu, 2015-08-27 at 11:05 +0200, Jan Cholasta wrote: On 27.8.2015 07:56, Jan Cholasta wrote: On 25.8.2015 20:43, Simo Sorce wrote: On Wed, 2015-08-05 at 11:24 -0400, Simo Sorce wrote: On Wed, 2015-08-05 at 08:20 +0200, Jan Cholasta wrote: Hi, Dne 31.7.2015 v 12:46 Simo Sorce napsal(a): I've been carrying these patches in my tree for a while, I think it is time to put them in master as they stand on their own. Simo. Patch 530: ACK Patch 531: ACK Patch 532: The methods should be static methods: @staticmethod def setOption(name, value): ... Care to explain why ? @staticmethod is not used anywhere else in that file. Rebased patches on master, made requested change +1 more patch. Simo. Patch 532: ACK Patch 533: ACK Pushed to master: f57b687241fbc92d1138507210e87e9de465c507 Honza Actually, there is a problem with patch 531: SASL mapping are added only on replica. The attached patch fixes it. This will break the promotion code, which needs to add the real sasl mappings later in the process. Can you leave the step in the non-common part of the setup for both server and replica installs ? OK, here you go. LGTM Simo. -- Simo Sorce * Red Hat, Inc * New York -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCHES] changes in preparation of replica promotion work
On 27.8.2015 16:02, Simo Sorce wrote: On Thu, 2015-08-27 at 16:02 +0200, Jan Cholasta wrote: On 27.8.2015 14:34, Simo Sorce wrote: On Thu, 2015-08-27 at 11:05 +0200, Jan Cholasta wrote: On 27.8.2015 07:56, Jan Cholasta wrote: On 25.8.2015 20:43, Simo Sorce wrote: On Wed, 2015-08-05 at 11:24 -0400, Simo Sorce wrote: On Wed, 2015-08-05 at 08:20 +0200, Jan Cholasta wrote: Hi, Dne 31.7.2015 v 12:46 Simo Sorce napsal(a): I've been carrying these patches in my tree for a while, I think it is time to put them in master as they stand on their own. Simo. Patch 530: ACK Patch 531: ACK Patch 532: The methods should be static methods: @staticmethod def setOption(name, value): ... Care to explain why ? @staticmethod is not used anywhere else in that file. Rebased patches on master, made requested change +1 more patch. Simo. Patch 532: ACK Patch 533: ACK Pushed to master: f57b687241fbc92d1138507210e87e9de465c507 Honza Actually, there is a problem with patch 531: SASL mapping are added only on replica. The attached patch fixes it. This will break the promotion code, which needs to add the real sasl mappings later in the process. Can you leave the step in the non-common part of the setup for both server and replica installs ? OK, here you go. LGTM Simo. Pushed to master: 0914cb663e6ea72628776e79d93f20bf979c7b68 -- Jan Cholasta -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCHES] changes in preparation of replica promotion work
On 27.8.2015 14:34, Simo Sorce wrote: On Thu, 2015-08-27 at 11:05 +0200, Jan Cholasta wrote: On 27.8.2015 07:56, Jan Cholasta wrote: On 25.8.2015 20:43, Simo Sorce wrote: On Wed, 2015-08-05 at 11:24 -0400, Simo Sorce wrote: On Wed, 2015-08-05 at 08:20 +0200, Jan Cholasta wrote: Hi, Dne 31.7.2015 v 12:46 Simo Sorce napsal(a): I've been carrying these patches in my tree for a while, I think it is time to put them in master as they stand on their own. Simo. Patch 530: ACK Patch 531: ACK Patch 532: The methods should be static methods: @staticmethod def setOption(name, value): ... Care to explain why ? @staticmethod is not used anywhere else in that file. Rebased patches on master, made requested change +1 more patch. Simo. Patch 532: ACK Patch 533: ACK Pushed to master: f57b687241fbc92d1138507210e87e9de465c507 Honza Actually, there is a problem with patch 531: SASL mapping are added only on replica. The attached patch fixes it. This will break the promotion code, which needs to add the real sasl mappings later in the process. Can you leave the step in the non-common part of the setup for both server and replica installs ? OK, here you go. -- Jan Cholasta From c6a0b0e8b97605b24efb3d6a7272df604b3df3f8 Mon Sep 17 00:00:00 2001 From: Jan Cholasta jchol...@redhat.com Date: Thu, 27 Aug 2015 10:52:57 +0200 Subject: [PATCH] install: Fix SASL mappings not added in ipa-server-install --- ipaserver/install/dsinstance.py | 1 + 1 file changed, 1 insertion(+) diff --git a/ipaserver/install/dsinstance.py b/ipaserver/install/dsinstance.py index 8320569..819b6cc 100644 --- a/ipaserver/install/dsinstance.py +++ b/ipaserver/install/dsinstance.py @@ -300,6 +300,7 @@ class DsInstance(service.Service): self.__common_setup() +self.step(adding sasl mappings to the directory, self.__configure_sasl_mappings) self.step(adding default layout, self.__add_default_layout) self.step(adding delegation layout, self.__add_delegation_layout) self.step(creating container for managed entries, self.__managed_entries) -- 2.4.3 -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH] First part of integration tests for Topology Plugin
Hi, I am sorry I have missed that. Fixed. The test fails now due to this bug: https://fedorahosted.org/freeipa/ticket/5222 The test output is attached together with the updated patch On 08/26/2015 05:53 PM, Martin Basti wrote: On 08/26/2015 05:42 PM, Martin Basti wrote: On 08/26/2015 02:53 PM, Oleg Fayans wrote: Hi, No more short links :) On 08/26/2015 11:50 AM, Tomas Babej wrote: On 08/26/2015 11:44 AM, Oleg Fayans wrote: Hi Martin, On 08/20/2015 11:18 AM, Martin Basti wrote: On 08/20/2015 10:26 AM, Martin Basti wrote: On 08/19/2015 04:17 PM, Martin Basti wrote: I got this: https://paste.fedoraproject.org/256746/43999380/ FYI replica install failure. (I will retest it, but I'm pretty sure that it was clean VM, test for some reason install client first) File /usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py, line 295, in decorated func(installer) File /usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py, line 319, in install_check sys.exit(IPA client is already configured on this system.\n 2015-08-19T14:14:15Z DEBUG The ipa-replica-install command failed, exception: SystemExit: IPA client is already configured on this system. Please uninstall it first before configuring the replica, using 'ipa-client-install --uninstall'. 2015-08-19T14:14:15Z ERROR IPA client is already configured on this system. Please uninstall it first before configuring the replica, using 'ipa-client-install --uninstall'. Confirm I got same error. Fixed. It was a regex error. On 08/19/2015 09:00 AM, Oleg Fayans wrote: Hi Martin, As discussed, here is a new version with pep8-related fixes On 08/14/2015 10:44 AM, Oleg Fayans wrote: Hi Martin, Already noticed that. Implemented the named groups as Tomas advised. Added the third test for http://www.freeipa.org/page/V4/Manage_replication_topology/Test_plan#Test_case:_Removal_of_a_topology_segment_is_allowed_only_if_there_is_at_least_one_more_segment_connecting_the_given_replica On 08/13/2015 05:06 PM, Martin Basti wrote: On 08/11/2015 03:36 PM, Oleg Fayans wrote: Hi Martin, On 08/11/2015 02:02 PM, Martin Basti wrote: NACK, comments inline. On 11/08/15 13:25, Oleg Fayans wrote: Hi Martin, Thanks for the review! On 08/10/2015 07:08 PM, Martin Basti wrote: Thank you for patch, I have a few nitpicks: 1) On 10/08/15 13:05, Oleg Fayans wrote: +def create_segment(master, leftnode, rightnode): +create_segment(master, leftnode, rightnode) Why do you add the name of method in docstring? My bad, fixed. still there +tokenize_topologies(command_output) +takes an output of `ipa topologysegment-find` and returns an array of Fixed, sorry. 2) +def create_segment(master, leftnode, rightnode): +create_segment(master, leftnode, rightnode) +creates a topology segment. The first argument is a node to run the command on +The first 3 arguments should be objects of class Host +Returns a hash object containing segment's name, leftnode, rightnode information + I would prefer to add assert there instead of just document that a Host object is needed assert(isinstance(master, Host)) ... Fixed. Created a decorator that checks the type of arguments This does not scale well. If we will want to add new argument that is not host object, you need change it again. Agreed. Modified the decorator so that you can specify a slice of arguments to be checked and a class to compare to. This does scale :) This might be used as function with specified variables that have to be host objects 3) +def destroy_segment(master, segment_name): + +destroy_segment(master, segment_name) +Destroys topology segment. First argument should be object of class Host Instead of description of params as first, second etc., you may use following: +def destroy_segment(master, segment_name): + +Destroys topology segment. +:param master: reference to master object of class Host +:param segment: name fo segment and eventually this in other methods +:returns: Lorem ipsum sit dolor mit amet +:raises NotFound: if segment is not found Fixed 4) cls.replicas[:len(cls.replicas) - 1], I suggest cls.replicas[:-1] In [2]: a = [1, 2, 3, 4, 5] In [3]: a[:-1] Out[3]: [1, 2, 3, 4] Fixed 5) Why re.findall() and then you just use the first result? 'leftnode': self.leftnode_re.findall(i)[0] Isn't just re.search() enough? leftnode_re.search(value).group(1) in fact leftnode_re.findall(string)[0] and leftnode_re.search(string).group(1), Are equally bad from the readability point of view. The first one is even shorter a bit, so why change? :) It depends on point of view, because when I reviewed it yesterday my brain raises exception that you are trying to add multiple values to single value attribute, because you used findall, I expected that you need multiple values, and then I saw that index [0] at the end, and I was pretty
Re: [Freeipa-devel] [PATCH] First part of integration tests for Topology Plugin
Hi Martin, My bad, forgot to do git add. On 08/27/2015 06:27 PM, Martin Basti wrote: On 08/27/2015 05:41 PM, Oleg Fayans wrote: Hi, I am sorry I have missed that. Fixed. The test fails now due to this bug: https://fedorahosted.org/freeipa/ticket/5222 The test output is attached together with the updated patch On 08/26/2015 05:53 PM, Martin Basti wrote: On 08/26/2015 05:42 PM, Martin Basti wrote: On 08/26/2015 02:53 PM, Oleg Fayans wrote: Hi, No more short links :) On 08/26/2015 11:50 AM, Tomas Babej wrote: On 08/26/2015 11:44 AM, Oleg Fayans wrote: Hi Martin, On 08/20/2015 11:18 AM, Martin Basti wrote: On 08/20/2015 10:26 AM, Martin Basti wrote: On 08/19/2015 04:17 PM, Martin Basti wrote: I got this: https://paste.fedoraproject.org/256746/43999380/ FYI replica install failure. (I will retest it, but I'm pretty sure that it was clean VM, test for some reason install client first) File /usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py, line 295, in decorated func(installer) File /usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py, line 319, in install_check sys.exit(IPA client is already configured on this system.\n 2015-08-19T14:14:15Z DEBUG The ipa-replica-install command failed, exception: SystemExit: IPA client is already configured on this system. Please uninstall it first before configuring the replica, using 'ipa-client-install --uninstall'. 2015-08-19T14:14:15Z ERROR IPA client is already configured on this system. Please uninstall it first before configuring the replica, using 'ipa-client-install --uninstall'. Confirm I got same error. Fixed. It was a regex error. On 08/19/2015 09:00 AM, Oleg Fayans wrote: Hi Martin, As discussed, here is a new version with pep8-related fixes On 08/14/2015 10:44 AM, Oleg Fayans wrote: Hi Martin, Already noticed that. Implemented the named groups as Tomas advised. Added the third test for http://www.freeipa.org/page/V4/Manage_replication_topology/Test_plan#Test_case:_Removal_of_a_topology_segment_is_allowed_only_if_there_is_at_least_one_more_segment_connecting_the_given_replica On 08/13/2015 05:06 PM, Martin Basti wrote: On 08/11/2015 03:36 PM, Oleg Fayans wrote: Hi Martin, On 08/11/2015 02:02 PM, Martin Basti wrote: NACK, comments inline. On 11/08/15 13:25, Oleg Fayans wrote: Hi Martin, Thanks for the review! On 08/10/2015 07:08 PM, Martin Basti wrote: Thank you for patch, I have a few nitpicks: 1) On 10/08/15 13:05, Oleg Fayans wrote: +def create_segment(master, leftnode, rightnode): +create_segment(master, leftnode, rightnode) Why do you add the name of method in docstring? My bad, fixed. still there + tokenize_topologies(command_output) +takes an output of `ipa topologysegment-find` and returns an array of Fixed, sorry. 2) +def create_segment(master, leftnode, rightnode): +create_segment(master, leftnode, rightnode) +creates a topology segment. The first argument is a node to run the command on +The first 3 arguments should be objects of class Host +Returns a hash object containing segment's name, leftnode, rightnode information + I would prefer to add assert there instead of just document that a Host object is needed assert(isinstance(master, Host)) ... Fixed. Created a decorator that checks the type of arguments This does not scale well. If we will want to add new argument that is not host object, you need change it again. Agreed. Modified the decorator so that you can specify a slice of arguments to be checked and a class to compare to. This does scale :) This might be used as function with specified variables that have to be host objects 3) +def destroy_segment(master, segment_name): + +destroy_segment(master, segment_name) +Destroys topology segment. First argument should be object of class Host Instead of description of params as first, second etc., you may use following: +def destroy_segment(master, segment_name): + +Destroys topology segment. +:param master: reference to master object of class Host +:param segment: name fo segment and eventually this in other methods +:returns: Lorem ipsum sit dolor mit amet +:raises NotFound: if segment is not found Fixed 4) cls.replicas[:len(cls.replicas) - 1], I suggest cls.replicas[:-1] In [2]: a = [1, 2, 3, 4, 5] In [3]: a[:-1] Out[3]: [1, 2, 3, 4] Fixed 5) Why re.findall() and then you just use the first result? 'leftnode': self.leftnode_re.findall(i)[0] Isn't just re.search() enough? leftnode_re.search(value).group(1) in fact leftnode_re.findall(string)[0] and leftnode_re.search(string).group(1), Are equally bad from the readability point of view. The first one is even shorter a bit, so why change? :) It depends on point of view, because when I reviewed it yesterday my brain raises exception that you are trying to add multiple values to single value attribute,
Re: [Freeipa-devel] [PATCH] First part of integration tests for Topology Plugin
On 08/27/2015 05:41 PM, Oleg Fayans wrote: Hi, I am sorry I have missed that. Fixed. The test fails now due to this bug: https://fedorahosted.org/freeipa/ticket/5222 The test output is attached together with the updated patch On 08/26/2015 05:53 PM, Martin Basti wrote: On 08/26/2015 05:42 PM, Martin Basti wrote: On 08/26/2015 02:53 PM, Oleg Fayans wrote: Hi, No more short links :) On 08/26/2015 11:50 AM, Tomas Babej wrote: On 08/26/2015 11:44 AM, Oleg Fayans wrote: Hi Martin, On 08/20/2015 11:18 AM, Martin Basti wrote: On 08/20/2015 10:26 AM, Martin Basti wrote: On 08/19/2015 04:17 PM, Martin Basti wrote: I got this: https://paste.fedoraproject.org/256746/43999380/ FYI replica install failure. (I will retest it, but I'm pretty sure that it was clean VM, test for some reason install client first) File /usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py, line 295, in decorated func(installer) File /usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py, line 319, in install_check sys.exit(IPA client is already configured on this system.\n 2015-08-19T14:14:15Z DEBUG The ipa-replica-install command failed, exception: SystemExit: IPA client is already configured on this system. Please uninstall it first before configuring the replica, using 'ipa-client-install --uninstall'. 2015-08-19T14:14:15Z ERROR IPA client is already configured on this system. Please uninstall it first before configuring the replica, using 'ipa-client-install --uninstall'. Confirm I got same error. Fixed. It was a regex error. On 08/19/2015 09:00 AM, Oleg Fayans wrote: Hi Martin, As discussed, here is a new version with pep8-related fixes On 08/14/2015 10:44 AM, Oleg Fayans wrote: Hi Martin, Already noticed that. Implemented the named groups as Tomas advised. Added the third test for http://www.freeipa.org/page/V4/Manage_replication_topology/Test_plan#Test_case:_Removal_of_a_topology_segment_is_allowed_only_if_there_is_at_least_one_more_segment_connecting_the_given_replica On 08/13/2015 05:06 PM, Martin Basti wrote: On 08/11/2015 03:36 PM, Oleg Fayans wrote: Hi Martin, On 08/11/2015 02:02 PM, Martin Basti wrote: NACK, comments inline. On 11/08/15 13:25, Oleg Fayans wrote: Hi Martin, Thanks for the review! On 08/10/2015 07:08 PM, Martin Basti wrote: Thank you for patch, I have a few nitpicks: 1) On 10/08/15 13:05, Oleg Fayans wrote: +def create_segment(master, leftnode, rightnode): +create_segment(master, leftnode, rightnode) Why do you add the name of method in docstring? My bad, fixed. still there + tokenize_topologies(command_output) +takes an output of `ipa topologysegment-find` and returns an array of Fixed, sorry. 2) +def create_segment(master, leftnode, rightnode): +create_segment(master, leftnode, rightnode) +creates a topology segment. The first argument is a node to run the command on +The first 3 arguments should be objects of class Host +Returns a hash object containing segment's name, leftnode, rightnode information + I would prefer to add assert there instead of just document that a Host object is needed assert(isinstance(master, Host)) ... Fixed. Created a decorator that checks the type of arguments This does not scale well. If we will want to add new argument that is not host object, you need change it again. Agreed. Modified the decorator so that you can specify a slice of arguments to be checked and a class to compare to. This does scale :) This might be used as function with specified variables that have to be host objects 3) +def destroy_segment(master, segment_name): + +destroy_segment(master, segment_name) +Destroys topology segment. First argument should be object of class Host Instead of description of params as first, second etc., you may use following: +def destroy_segment(master, segment_name): + +Destroys topology segment. +:param master: reference to master object of class Host +:param segment: name fo segment and eventually this in other methods +:returns: Lorem ipsum sit dolor mit amet +:raises NotFound: if segment is not found Fixed 4) cls.replicas[:len(cls.replicas) - 1], I suggest cls.replicas[:-1] In [2]: a = [1, 2, 3, 4, 5] In [3]: a[:-1] Out[3]: [1, 2, 3, 4] Fixed 5) Why re.findall() and then you just use the first result? 'leftnode': self.leftnode_re.findall(i)[0] Isn't just re.search() enough? leftnode_re.search(value).group(1) in fact leftnode_re.findall(string)[0] and leftnode_re.search(string).group(1), Are equally bad from the readability point of view. The first one is even shorter a bit, so why change? :) It depends on point of view, because when I reviewed it yesterday my brain raises exception that you are trying to add multiple values to single value attribute, because you used findall, I expected that you need multiple values, and
[Freeipa-devel] [PATCH] 377 Using LDAPI to setup CA and KRA agents.
The CA and KRA installation code has been modified to use LDAPI
to create the CA and KRA agents directly in the CA and KRA
database. This way it's no longer necessary to use the Directory
Manager password or CA and KRA admin certificate.
https://fedorahosted.org/freeipa/ticket/5257
--
Endi S. Dewata
From 45af6d4f9a8ebc9bbd2856d7bf3af48520996dad Mon Sep 17 00:00:00 2001
From: Endi S. Dewata edew...@redhat.com
Date: Thu, 27 Aug 2015 06:44:29 +0200
Subject: [PATCH] Using LDAPI to setup CA and KRA agents.
The CA and KRA installation code has been modified to use LDAPI
to create the CA and KRA agents directly in the CA and KRA
database. This way it's no longer necessary to use the Directory
Manager password or CA and KRA admin certificate.
https://fedorahosted.org/freeipa/ticket/5257
---
ipaplatform/base/paths.py| 2 -
ipaserver/install/cainstance.py | 49 ++---
ipaserver/install/krainstance.py | 113 +++
3 files changed, 72 insertions(+), 92 deletions(-)
diff --git a/ipaplatform/base/paths.py b/ipaplatform/base/paths.py
index
5c8f25d6ef85fab2b9b30a660cd1c0360dbe9931..0dd3c7fda3020264a1ace8f2d13557cfddf18c2d
100644
--- a/ipaplatform/base/paths.py
+++ b/ipaplatform/base/paths.py
@@ -343,8 +343,6 @@ class BasePathNamespace(object):
SLAPD_INSTANCE_SOCKET_TEMPLATE = /var/run/slapd-%s.socket
ALL_SLAPD_INSTANCE_SOCKETS = /var/run/slapd-*.socket
ADMIN_CERT_PATH = '/root/.dogtag/pki-tomcat/ca_admin.cert'
-KRA_NSSDB_PASSWORD_FILE = /root/.dogtag/pki-tomcat/kra/password.conf
-KRA_PKCS12_PASSWORD_FILE =
/root/.dogtag/pki-tomcat/kra/pkcs12_password.conf
ENTROPY_AVAIL = '/proc/sys/kernel/random/entropy_avail'
LDIF2DB = '/usr/sbin/ldif2db'
DB2LDIF = '/usr/sbin/db2ldif'
diff --git a/ipaserver/install/cainstance.py b/ipaserver/install/cainstance.py
index
ecd9300036353426097d929918be974cbbb5c69d..bec39419363f1ade0130465d3b70e1c5540b6006
100644
--- a/ipaserver/install/cainstance.py
+++ b/ipaserver/install/cainstance.py
@@ -904,17 +904,26 @@ class CAInstance(DogtagInstance):
self.configure_agent_renewal()
def __configure_ra(self):
-# Create an RA user in the CA LDAP server and add that user to
-# the appropriate groups so it can issue certificates without
-# manual intervention.
-conn = ipaldap.IPAdmin(self.fqdn, self.ds_port)
-conn.do_simple_bind(DN(('cn', 'Directory Manager')), self.dm_password)
+
+Create CA agent, assign a certificate, and add the user to
+the appropriate groups for accessing CA services.
+
-decoded = base64.b64decode(self.ra_cert)
+# get ipaCert certificate
+cert_data = base64.b64decode(self.ra_cert)
+cert = x509.load_certificate(cert_data, x509.DER)
-entry_dn = DN(('uid', ipara), ('ou', 'People'), self.basedn)
-entry = conn.make_entry(
-entry_dn,
+# connect to CA database
+server_id = installutils.realm_to_serverid(api.env.realm)
+dogtag_uri = 'ldapi://%%2fvar%%2frun%%2fslapd-%s.socket' % server_id
+conn = ldap2.ldap2(api, ldap_uri=dogtag_uri)
+if not conn.isconnected():
+conn.connect(autobind=True)
+
+# create ipara user with ipaCert certificate
+user_dn = DN(('uid', ipara), ('ou', 'People'), self.basedn)
+conn.create(
+dn=user_dn,
objectClass=['top', 'person', 'organizationalPerson',
'inetOrgPerson', 'cmsuser'],
uid=[ipara],
@@ -922,23 +931,23 @@ class CAInstance(DogtagInstance):
cn=[ipara],
usertype=[agentType],
userstate=[1],
-userCertificate=[decoded],
+userCertificate=[cert_data],
description=['2;%s;%s;%s' % (
-str(self.requestId),
+cert.serial_number,
DN(('CN', 'Certificate Authority'), self.subject_base),
DN(('CN', 'IPA RA'), self.subject_base))])
-conn.add_entry(entry)
+# add ipara user to Certificate Manager Agents group
+group_dn = DN(('cn', 'Certificate Manager Agents'), ('ou', 'groups'),
+self.basedn)
+conn.add_entry_to_group(user_dn, group_dn, 'uniqueMember')
-dn = DN(('cn', 'Certificate Manager Agents'), ('ou', 'groups'),
self.basedn)
-modlist = [(0, 'uniqueMember', '%s' % entry_dn)]
-conn.modify_s(dn, modlist)
+# add ipara user to Registration Manager Agents group
+group_dn = DN(('cn', 'Registration Manager Agents'), ('ou', 'groups'),
+self.basedn)
+conn.add_entry_to_group(user_dn, group_dn, 'uniqueMember')
-dn = DN(('cn', 'Registration Manager Agents'), ('ou', 'groups'),
self.basedn)
-modlist = [(0, 'uniqueMember', '%s' % entry_dn)]
-conn.modify_s(dn, modlist)
-
-conn.unbind()
+conn.disconnect()
def
Re: [Freeipa-devel] [PATCH 0066] ipactl: Do not start/stop/restart single service multiple times
On 26/08/15 17:49, Tomas Babej wrote: On 08/26/2015 03:16 PM, David Kupka wrote: https://fedorahosted.org/freeipa/ticket/5248 +def deduplicate(lst): +new_lst = [] +s = set(lst) +for i in lst: +if i in s: +s.remove(i) +new_lst.append(i) + +return new_lst + Imho, this method deserves a docstring or at least a comment. It is not entrirely clear from the name, that its job is to remove the duplicates while preserving the order of the entries. You're right, line or two could not hurt. Patch attached. Anyway, deduplication can be implemented in a more readable way: from collections import OrderedDict sample_list = [3,2,1,2,1,5,3] OrderedDict.fromkeys(sample_list).keys() [3, 2, 1, 5] I know about this approach and it does not seem much more readable to me. I just chosen few more lines instead of an import. Tomas -- David Kupka From 8f8a97747968ae9b69b1f7d0b9deaab730feba4c Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Thu, 27 Aug 2015 07:34:02 +0200 Subject: [PATCH] comment: Add Documentation string to deduplicate function --- install/tools/ipactl | 3 +++ 1 file changed, 3 insertions(+) diff --git a/install/tools/ipactl b/install/tools/ipactl index 5782d4c424fb98c08e55614c71f3abaa6e776a68..379a8d91b2419a9708919ac9713352fcc242e79f 100755 --- a/install/tools/ipactl +++ b/install/tools/ipactl @@ -46,6 +46,9 @@ def check_IPA_configuration(): (see man pages of ipa-server-install for help), 6) def deduplicate(lst): +Remove duplicates and preserve order. +Returns copy of list with preserved order and removed duplicates. + new_lst = [] s = set(lst) for i in lst: -- 2.4.3 -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH] 0004 Fix user tracker to reflect new user-del message
On 08/26/2015 04:29 PM, Lenka Doudova wrote: Fix for user tracker in ipatests/test_xmlrpc/test_user_plugin.py so that it reflects recently changed message of user-del command. Lenka Thank you, ACK Pushed to: master: a78e75120990341434e92b370cd3386fd1f8fed3 ipa-4-2: f5dcb03a1c003557371be52597aba7900b0ac345 -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] ipa-client-install domain autodiscovery - try _kerberos first?
On Thu, 27 Aug 2015, Petr Spacek wrote: Hello, while investigating a problem reported on ipa-users, I found out that check_domain() method in ipaclient/ipadiscovery.py checks _ldap._tcp SRV record first. This seems to be based on assumption that IPA client is in the same DNS sub-tree as the main IPA domain. IMHO it would be better to find _kerberos TXT record in client's domain (or its parent domains) and then check _ldap._tcp SRV records in domain pointed to by _kerberos record. Do you agree? Am I missing something? Yes, you should be using _kerberos TXT record first and then follow domain derived from the realm. However, this would not work for non-FQDN REALMs (for example, Kerberos realm IPA). In that case you would have KDC explicitly defined in the krb5.conf, though. Side note: ipadiscovery.py could be re-used in ipa-server-install as mechanism to detect attempts to install IPA into a DNS domain which is already occupied by another IPA, or AD, or something else. Correct and makes sense. -- / Alexander Bokovoy -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCHES] changes in preparation of replica promotion work
On 27/08/15 11:05, Jan Cholasta wrote: On 27.8.2015 07:56, Jan Cholasta wrote: On 25.8.2015 20:43, Simo Sorce wrote: On Wed, 2015-08-05 at 11:24 -0400, Simo Sorce wrote: On Wed, 2015-08-05 at 08:20 +0200, Jan Cholasta wrote: Hi, Dne 31.7.2015 v 12:46 Simo Sorce napsal(a): I've been carrying these patches in my tree for a while, I think it is time to put them in master as they stand on their own. Simo. Patch 530: ACK Patch 531: ACK Patch 532: The methods should be static methods: @staticmethod def setOption(name, value): ... Care to explain why ? @staticmethod is not used anywhere else in that file. Rebased patches on master, made requested change +1 more patch. Simo. Patch 532: ACK Patch 533: ACK Pushed to master: f57b687241fbc92d1138507210e87e9de465c507 Honza Actually, there is a problem with patch 531: SASL mapping are added only on replica. The attached patch fixes it. Works for me, ACK. -- David Kupka -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCHES] changes in preparation of replica promotion work
On 27.8.2015 07:56, Jan Cholasta wrote: On 25.8.2015 20:43, Simo Sorce wrote: On Wed, 2015-08-05 at 11:24 -0400, Simo Sorce wrote: On Wed, 2015-08-05 at 08:20 +0200, Jan Cholasta wrote: Hi, Dne 31.7.2015 v 12:46 Simo Sorce napsal(a): I've been carrying these patches in my tree for a while, I think it is time to put them in master as they stand on their own. Simo. Patch 530: ACK Patch 531: ACK Patch 532: The methods should be static methods: @staticmethod def setOption(name, value): ... Care to explain why ? @staticmethod is not used anywhere else in that file. Rebased patches on master, made requested change +1 more patch. Simo. Patch 532: ACK Patch 533: ACK Pushed to master: f57b687241fbc92d1138507210e87e9de465c507 Honza Actually, there is a problem with patch 531: SASL mapping are added only on replica. The attached patch fixes it. -- Jan Cholasta From 75d16dfc519c457eead2126bf53087dc971674c6 Mon Sep 17 00:00:00 2001 From: Jan Cholasta jchol...@redhat.com Date: Thu, 27 Aug 2015 10:52:57 +0200 Subject: [PATCH] install: Fix SASL mappings not added in ipa-server-install --- ipaserver/install/dsinstance.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ipaserver/install/dsinstance.py b/ipaserver/install/dsinstance.py index 8320569..dd67915 100644 --- a/ipaserver/install/dsinstance.py +++ b/ipaserver/install/dsinstance.py @@ -253,6 +253,7 @@ class DsInstance(service.Service): self.step(configure autobind for root, self.__root_autobind) self.step(configure new location for managed entries, self.__repoint_managed_entries) self.step(configure dirsrv ccache, self.configure_dirsrv_ccache) +self.step(adding sasl mappings to the directory, self.__configure_sasl_mappings) self.step(enable SASL mapping fallback, self.__enable_sasl_mapping_fallback) self.step(restarting directory server, self.__restart_instance) @@ -354,7 +355,6 @@ class DsInstance(service.Service): self.__common_setup(True) self.step(setting up initial replication, self.__setup_replica) -self.step(adding sasl mappings to the directory, self.__configure_sasl_mappings) self.step(updating schema, self.__update_schema) # See LDIFs for automember configuration during replica install self.step(setting Auto Member configuration, self.__add_replica_automember_config) -- 2.4.3 -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] ipa-devel repos on jdennis.fedorapeople.org
On 15.7.2015 09:44, Jan Pazdziora wrote: On Tue, Jul 14, 2015 at 12:49:23PM -0400, John Dennis wrote: On 07/14/2015 12:03 PM, Petr Spacek wrote: Hello, Is anyone using repos https://jdennis.fedorapeople.org/ipa-devel/ ? AFAIK nobody in Brno is seriously using it but I'm not sure about people outside the Brno. Could we use COPR instead and get out of builder business? Upcoming lab maintenance window could be a good time to do that. I would love to get out of the builder business and I suspect Nalin would as well [1]. The question came up in our Monday meeting as well. Nobody seem to know if anyone was using these builds and why we weren't using COPR. The The Fedora infra admins should be able to provide HTTP logs for the repo, if you needs some numbers about potential usage. That is a good idea! I got logs from Fedora admins and as far as I can tell, in the last month there were only 7 RPM downloads and nothing else. The 7 hits I found was for /ipa-devel/rhel/6/x86_64/os/sssd-1.13.1-0.20150813T1121Zgit137d5dd.el6.i686.rpm and other packages from the same version. I did not find any hits for IPA packages at all. All the remaining traffic (except the 7 RPM hits) was from repo data refreshes: - 83 % is RHEL 5 repodata - 13 % is RHEL 6 repodata - remaining ~ 4 % of noise is Fedora repodata It seems to me that we can get out the builder business completely and decommission ipa-devel and replace it with COPR. Do you agree? John? Nathaniel? Stephen? Anyone? :-) -- Petr^2 Spacek -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] ipa-devel repos on jdennis.fedorapeople.org
On Thu, 27 Aug 2015, Petr Spacek wrote: On 15.7.2015 09:44, Jan Pazdziora wrote: On Tue, Jul 14, 2015 at 12:49:23PM -0400, John Dennis wrote: On 07/14/2015 12:03 PM, Petr Spacek wrote: Hello, Is anyone using repos https://jdennis.fedorapeople.org/ipa-devel/ ? AFAIK nobody in Brno is seriously using it but I'm not sure about people outside the Brno. Could we use COPR instead and get out of builder business? Upcoming lab maintenance window could be a good time to do that. I would love to get out of the builder business and I suspect Nalin would as well [1]. The question came up in our Monday meeting as well. Nobody seem to know if anyone was using these builds and why we weren't using COPR. The The Fedora infra admins should be able to provide HTTP logs for the repo, if you needs some numbers about potential usage. That is a good idea! I got logs from Fedora admins and as far as I can tell, in the last month there were only 7 RPM downloads and nothing else. The 7 hits I found was for /ipa-devel/rhel/6/x86_64/os/sssd-1.13.1-0.20150813T1121Zgit137d5dd.el6.i686.rpm and other packages from the same version. I did not find any hits for IPA packages at all. All the remaining traffic (except the 7 RPM hits) was from repo data refreshes: - 83 % is RHEL 5 repodata - 13 % is RHEL 6 repodata - remaining ~ 4 % of noise is Fedora repodata It seems to me that we can get out the builder business completely and decommission ipa-devel and replace it with COPR. Do you agree? John? Nathaniel? Stephen? Anyone? :-) Yes, I think we can decommission this repository thanks to COPR infrastructure. -- / Alexander Bokovoy -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCHES] 0696-0710 More modernization
On 26.8.2015 12:15, Petr Viktorin wrote: On 08/26/2015 09:47 AM, Jan Cholasta wrote: On 25.8.2015 15:05, Petr Viktorin wrote: On 08/25/2015 12:39 PM, Christian Heimes wrote: On 2015-08-24 17:31, Petr Viktorin wrote: 0701.2-Use-Python3-compatible-dict-method-names NACK Why are you replacing iteritems() with items() instead of using six.iteritems()? It looks cleaner, and it will be easier to clean up after six is dropped. Also, the performance difference is negligible if the whole thing is iterated over. (On small dicts, which are the majority of what iteritems was used on, items() is actually a bit faster on my machine.) Right, for small dicts the speed difference is negligible and favors the items() over iteritems(). For medium sized and large dicts the iterators are faster and consume less memory. I'm preferring iterator methods everywhere because I don't have to worry about dict sizes. 0710.2-Modernize-use-of-range NACK Please use six.moves.range. It defaults to xrange() in Python 2. Why? What is the benefit of xrange in these situations? Like with iteritems in 0701, when iterating over the whole thing, the performance difference is negligible. I don't think a few microseconds outside of tight loops are worth the verbosity. It's the same reasoning as in 0701. As long as you have a small range, it doesn't make a difference. For large ranges the additional memory usage can be problematic. In all above cases the iterator methods and generator functions are a safer choice. A malicious user can abuse the non-iterative methods for DoS attacks. As long as the input can't be controlled by a user and the range/dict/set/list is small, the non-iterative methods are fine. You have to verify every location, though. Keep in mind that for dicts, all the data is in memory already (in the dict). Are you worried about DoS from an extra list of references to existing objects? I'm usually too busy with other stuff (aka lazy) to verify these preconditions... I changed one questionable use of range. The other ones are: ipalib/plugins/dns.py: for i in range(len(relative_record_name) (max. ~255, verified by DNSName) ipaserver/install/dnskeysyncinstance.py: for _ in range(0, 16)) (16) ipaserver/install/ipa_otptoken_import.py: for i in range(1, blocks + 1): (about 7) ipaserver/install/ipa_otptoken_import.py: for k in range(mac.digest_size): (16) ipatests/data.py: for d in range(256)) (256) Plus tests, where it's rarely above 10. I have just pushed Michael's python-krbV removal patch, which conflicts with your patches. Could you please rebase them on top of current master? Sure, here they are. Patches 696-699: ACK Patch 700: There are some error messages which contain basestring. Do we want to fix these as well? Patch 701: Since we are using python-six, shouldn't six.PY2 be used instead of sys.version_info (3, 0)? Patches 702-709: ACK Patch 710: There are some xranges in ipapython/p11helper.py and ipatests/test_xmlrpc/test_add_remove_cert_cmd.py. Patch 711: ACK -- Jan Cholasta -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCHES 478-479] cert renewal: KRA agent certificate update
On Thu, 27 Aug 2015, Jan Cholasta wrote: Hi, the attached patches fix https://fedorahosted.org/freeipa/ticket/5253. ACK to both patches. -- / Alexander Bokovoy -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] ipa-devel repos on jdennis.fedorapeople.org
On 08/27/2015 04:27 AM, Petr Spacek wrote: On 15.7.2015 09:44, Jan Pazdziora wrote: On Tue, Jul 14, 2015 at 12:49:23PM -0400, John Dennis wrote: On 07/14/2015 12:03 PM, Petr Spacek wrote: Hello, Is anyone using repos https://jdennis.fedorapeople.org/ipa-devel/ ? AFAIK nobody in Brno is seriously using it but I'm not sure about people outside the Brno. Could we use COPR instead and get out of builder business? Upcoming lab maintenance window could be a good time to do that. I would love to get out of the builder business and I suspect Nalin would as well [1]. The question came up in our Monday meeting as well. Nobody seem to know if anyone was using these builds and why we weren't using COPR. The The Fedora infra admins should be able to provide HTTP logs for the repo, if you needs some numbers about potential usage. That is a good idea! I got logs from Fedora admins and as far as I can tell, in the last month there were only 7 RPM downloads and nothing else. The 7 hits I found was for /ipa-devel/rhel/6/x86_64/os/sssd-1.13.1-0.20150813T1121Zgit137d5dd.el6.i686.rpm and other packages from the same version. I did not find any hits for IPA packages at all. All the remaining traffic (except the 7 RPM hits) was from repo data refreshes: - 83 % is RHEL 5 repodata - 13 % is RHEL 6 repodata - remaining ~ 4 % of noise is Fedora repodata It seems to me that we can get out the builder business completely and decommission ipa-devel and replace it with COPR. Do you agree? John? Nathaniel? Stephen? Anyone? :-) Yes, I agree. Do we have a cut off date when I can stop the service? -- John -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
