Re: [Freeipa-devel] [PATCH] 0008 Refactor test_sudocmdgroup_plugin, create SudoCmdGroupTracker

[Filip Skola]

Hi,

fixed. To be honest, I left that +1char longer lines there on purpose. IMHO it 
brings better readability and pep8 *.py | wc -l in test_xmlrpc dir returns an 
overwhelming number anyway. But yeah, some of these weren't really 
necessary...so I changed them all :)

This patch is dependent on 0007-3 patch.

Filip

- Original Message -
> NACK.
> 
> 
> [root@master2 test_xmlrpc]# pep8 test_sudocmdgroup_plugin.py
> test_sudocmdgroup_plugin.py:26:80: E501 line too long (80 > 79 characters)
> test_sudocmdgroup_plugin.py:70:80: E501 line too long (80 > 79 characters)
> test_sudocmdgroup_plugin.py:76:80: E501 line too long (80 > 79 characters)
> test_sudocmdgroup_plugin.py:84:80: E501 line too long (80 > 79 characters)
> test_sudocmdgroup_plugin.py:90:80: E501 line too long (80 > 79 characters)
> test_sudocmdgroup_plugin.py:98:80: E501 line too long (80 > 79 characters)
> test_sudocmdgroup_plugin.py:104:80: E501 line too long (80 > 79 characters)
> test_sudocmdgroup_plugin.py:166:80: E501 line too long (80 > 79 characters)
> test_sudocmdgroup_plugin.py:180:80: E501 line too long (80 > 79 characters)
> test_sudocmdgroup_plugin.py:186:80: E501 line too long (84 > 79 characters)
> [root@master2 test_xmlrpc]# pep8 tracker/sudocmdgroup_plugin.py
> tracker/sudocmdgroup_plugin.py:36:80: E501 line too long (82 > 79 characters)
> tracker/sudocmdgroup_plugin.py:42:80: E501 line too long (82 > 79 characters)
> tracker/sudocmdgroup_plugin.py:46:80: E501 line too long (85 > 79 characters)
> tracker/sudocmdgroup_plugin.py:55:80: E501 line too long (82 > 79 characters)
> tracker/sudocmdgroup_plugin.py:64:80: E501 line too long (82 > 79 characters)
> 
> 
> 
> - Original Message -
> > From: "Filip Skola" 
> > To: "Aleš Mareček" 
> > Cc: freeipa-devel@redhat.com, "Milan Kubík" 
> > Sent: Monday, February 22, 2016 3:41:36 PM
> > Subject: Re: [Freeipa-devel] [PATCH] 0008 Refactor
> > test_sudocmdgroup_plugin, create SudoCmdGroupTracker
> > 
> > Hi,
> > 
> > the test has been updated so it now uses the SudoCmdTracker (from the
> > previous patch).
> > 
> > Filip
> > 
> > - Original Message -
> > > NACK.
> > > 
> > > "create_sudocmd" and "delete_sudocmd" should be imported from Tracker,
> > > not
> > > from the previous test (sudocmd_plugin).
> > > 
> > >   - alich -
> > > 
> > > - Original Message -
> > > > From: "Filip Skola" 
> > > > To: freeipa-devel@redhat.com
> > > > Sent: Thursday, January 28, 2016 12:49:17 PM
> > > > Subject: [Freeipa-devel] [PATCH] 0008 Refactor
> > > > test_sudocmdgroup_plugin,
> > > > create SudoCmdGroupTracker
> > > > 
> > > > Hi,
> > > > 
> > > > sending the next sudo patch. This one depends on the previous one
> > > > (sudocmd_plugin).
> > > > 
> > > > Filip
> > > > 
> > > > --
> > > > 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
> > > 
> > 
> 
From 0fe52e2a9718a0aae5542e8bedc8d312dbc6e360 Mon Sep 17 00:00:00 2001
From: Filip Skola 
Date: Mon, 22 Feb 2016 14:21:56 +0100
Subject: [PATCH] Refactor test_sudocmdgroup_plugin

---
 ipatests/test_xmlrpc/test_sudocmdgroup_plugin.py   | 860 +
 .../test_xmlrpc/tracker/sudocmdgroup_plugin.py | 226 ++
 2 files changed, 415 insertions(+), 671 deletions(-)
 create mode 100644 ipatests/test_xmlrpc/tracker/sudocmdgroup_plugin.py

diff --git a/ipatests/test_xmlrpc/test_sudocmdgroup_plugin.py b/ipatests/test_xmlrpc/test_sudocmdgroup_plugin.py
index c72ba2f7a25d5b3c4817c7e17346a8ff5a324760..3f5879c4d90ed9872b9287344583892405bebff3 100644
--- a/ipatests/test_xmlrpc/test_sudocmdgroup_plugin.py
+++ b/ipatests/test_xmlrpc/test_sudocmdgroup_plugin.py
@@ -20,678 +20,196 @@
 Test the `ipalib/plugins/sudocmdgroup.py` module.
 """
 
-from ipalib import api, errors
-from ipatests.test_xmlrpc import objectclasses
-from ipatests.test_xmlrpc.xmlrpc_test import (Declarative, fuzzy_uuid,
-  fuzzy_sudocmddn)
-from ipapython.dn import DN
-import pytest
-
-sudocmdgroup1 = u'testsudocmdgroup1'
-sudocmdgroup2 = u'testsudocmdgroup2'
-sudocmd1 = u'/usr/bin/sudotestcmd1'
-sudocmd1_camelcase = u'/usr/bin/sudoTestCmd1'
-sudocmd_plus = u'/bin/ls -l /lost+found/*'
-
-def create_command(sudocmd):
-return dict(
-desc='Create %r' % sudocmd,
-command=(
-'sudocmd_add', [], dict(sudocmd=sudocmd,
-description=u'Test sudo command')
-),
-expected=dict(
-value=sudocmd,
-summary=u'Added Sudo Command "%s"' % sudocmd,
-result=dict(
-objectclass=objectclasses.sudocmd,
-sudocmd=[sudocmd],
-ipauniqueid=[fuzzy_uuid], description=[u'Test sudo command'],
-dn=fuzzy_sudocmddn,
-),
-),
+from ipalib import errors
+from ipatests.test_xmlrpc.xmlrpc_test import XMLRPC_test, raises_exact
+f

Re: [Freeipa-devel] [PATCH] 0007 Refactor test_sudocmd_plugin

[Filip Skola]

Hi,

these problems have been fixed.

F.

- Original Message -
> NACK.
> Some little changes still required:
>  * fixing the pep8 errors
>  * fixing the wrong comment
> 
> [root@master2 freeipa]# pep8 ipatests/test_xmlrpc/test_sudocmd_plugin.py
> ipatests/test_xmlrpc/test_sudocmd_plugin.py:94:80: E501 line too long (87 >
> 79 characters)
> ipatests/test_xmlrpc/test_sudocmd_plugin.py:97:80: E501 line too long (87 >
> 79 characters)
> ipatests/test_xmlrpc/test_sudocmd_plugin.py:134:80: E501 line too long (80 >
> 79 characters)
>   
> [root@master2 freeipa]# pep8 ipatests/test_xmlrpc/tracker/sudocmd_plugin.py
> ipatests/test_xmlrpc/tracker/sudocmd_plugin.py:14:80: E501 line too long (81
> > 79 characters)
> 
> [root@master2 freeipa]# grep 'Class for'
> ipatests/test_xmlrpc/tracker/sudocmd_plugin.py
> """ Class for host plugin like tests """
> 
> 
> - Original Message -
> > From: "Filip Skola" 
> > To: "Aleš Mareček" 
> > Cc: freeipa-devel@redhat.com, "Milan Kubík" 
> > Sent: Monday, February 22, 2016 1:59:43 PM
> > Subject: Re: [Freeipa-devel] [PATCH] 0007 Refactor test_sudocmd_plugin
> > 
> > Hi,
> > 
> > sudocmd tracker has been created.
> > 
> > Filip
> > 
> > - Original Message -
> > > NACK.
> > > 
> > > "create_sudocmd" and "delete_sudocmd" should be placed in Tracker. So
> > > this
> > > patch should create the tracker as well.
> > > 
> > > - Original Message -
> > > > From: "Filip Skola" 
> > > > To: freeipa-devel@redhat.com
> > > > Sent: Monday, January 25, 2016 3:57:25 PM
> > > > Subject: [Freeipa-devel] [PATCH] 0007 Refactor test_sudocmd_plugin
> > > > 
> > > > Hello,
> > > > 
> > > > attaching refactored sudocmd_plugin.
> > > > 
> > > > Filip
> > > > --
> > > > 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
> > > 
> > 
> 
From 1da1bdc46eb96b966e060b0ad83a3edebcc9c37b Mon Sep 17 00:00:00 2001
From: Filip Skola 
Date: Mon, 18 Jan 2016 13:56:44 +0100
Subject: [PATCH] Refactor test_sudocmd_plugin

---
 ipatests/test_xmlrpc/test_sudocmd_plugin.py| 448 +
 ipatests/test_xmlrpc/tracker/sudocmd_plugin.py | 113 +++
 2 files changed, 269 insertions(+), 292 deletions(-)
 create mode 100644 ipatests/test_xmlrpc/tracker/sudocmd_plugin.py

diff --git a/ipatests/test_xmlrpc/test_sudocmd_plugin.py b/ipatests/test_xmlrpc/test_sudocmd_plugin.py
index 2056118ba763be45e78ddf6643059e32d7680af8..7ffe7a1d0497ad9c755ccdbe057fee1a13523f20 100644
--- a/ipatests/test_xmlrpc/test_sudocmd_plugin.py
+++ b/ipatests/test_xmlrpc/test_sudocmd_plugin.py
@@ -21,309 +21,173 @@
 Test the `ipalib/plugins/sudocmd.py` module.
 """
 
-from ipalib import errors
-from ipatests.test_xmlrpc.xmlrpc_test import (Declarative, fuzzy_sudocmddn,
-fuzzy_uuid)
-from ipatests.test_xmlrpc import objectclasses
+from ipalib import api, errors
+from ipatests.util import assert_deepequal
+from ipatests.test_xmlrpc.xmlrpc_test import (XMLRPC_test, raises_exact)
+from ipatests.test_xmlrpc.tracker.sudocmd_plugin import SudoCmdTracker
 import pytest
 
-sudocmd1 = u'/usr/bin/sudotestcmd1'
-sudocmd1_camelcase = u'/usr/bin/sudoTestCmd1'
 
-sudorule1 = u'test_sudorule1'
+@pytest.fixture(scope='class')
+def sudocmd1(request):
+tracker = SudoCmdTracker(command=u'/usr/bin/sudotestcmd1',
+ description=u'Test sudo command 1')
+return tracker.make_fixture(request)
 
 
-@pytest.mark.tier1
-class test_sudocmd(Declarative):
-
-cleanup_commands = [
-('sudocmd_del', [sudocmd1], {}),
-('sudocmd_del', [sudocmd1_camelcase], {}),
-('sudorule_del', [sudorule1], {}),
-]
-
-tests = [
-
-dict(
-desc='Try to retrieve non-existent %r' % sudocmd1,
-command=('sudocmd_show', [sudocmd1], {}),
-expected=errors.NotFound(
-reason=u'%s: sudo command not found' % sudocmd1),
-),
-
-
-dict(
-desc='Try to update non-existent %r' % sudocmd1,
-command=('sudocmd_mod', [sudocmd1], dict(description=u'Nope')),
-expected=errors.NotFound(
-reason=u'%s: sudo command not found' % sudocmd1),
-),
-
-
-dict(
-desc='Try to delete non-existent %r' % sudocmd1,
-command=('sudocmd_del', [sudocmd1], {}),
-expected=errors.NotFound(
-reason=u'%s: sudo command not found' % sudocmd1),
-),
-
-
-dict(
-desc='Create %r' % sudocmd1,
-command=('sudocmd_add', [sudocmd1],
-dict(
-description=u'Test sudo command 1',
-),
-),
-expected=dict(
-value=sudocmd1,
-summary=u'Added Sudo Command "%s"' % sudocmd1,
-result=dict(
-dn=fuzzy_sudocmddn,
-sudo

Re: [Freeipa-devel] [patch 0034] ipatests: extend permission plugin test with new expected output

[Martin Basti]

On 24.02.2016 08:34, Milan Kubík wrote:

On 02/18/2016 03:52 PM, Milan Kubík wrote:

On 02/15/2016 04:59 PM, Milan Kubík wrote:

Patch attached. Applies on ipa-4-3 as well.




Updated version of patch fixes test_old_permission_plugin as well.

--
Milan Kubik



Review bump.

--
Milan Kubik



NACK

[mbasti@dhcp129-96 freeipa-devel]$ git show -U0 | pep8 --diff
./ipatests/test_xmlrpc/test_old_permission_plugin.py:528:80: E501 line 
too long (95 > 79 characters)
./ipatests/test_xmlrpc/test_old_permission_plugin.py:586:1: E101 
indentation contains mixed spaces and tabs
./ipatests/test_xmlrpc/test_old_permission_plugin.py:586:1: W191 
indentation contains tabs
./ipatests/test_xmlrpc/test_old_permission_plugin.py:587:1: E101 
indentation contains mixed spaces and tabs
./ipatests/test_xmlrpc/test_old_permission_plugin.py:587:80: E501 line 
too long (95 > 79 characters)
./ipatests/test_xmlrpc/test_old_permission_plugin.py:591:1: E101 
indentation contains mixed spaces and tabs
./ipatests/test_xmlrpc/test_old_permission_plugin.py:591:1: W191 
indentation contains tabs
./ipatests/test_xmlrpc/test_permission_plugin.py:821:80: E501 line too 
long (99 > 79 characters)
./ipatests/test_xmlrpc/test_permission_plugin.py:884:80: E501 line too 
long (99 > 79 characters)


-- 
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

[Freeipa-devel] [PATCH 0422] CI: allow customized DS install test to run under different domain levels

[Martin Basti]

https://fedorahosted.org/freeipa/ticket/5606

Patch attached.
From 1fe93ac24694dc490ddc497b1cbd493cd38e8117 Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Wed, 24 Feb 2016 17:45:55 +0100
Subject: [PATCH] CI: allow customized DS install test to work with domain
 levels

Test will use tasks methods instead of custom commands to be able work
with domain levels.

https://fedorahosted.org/freeipa/ticket/5606
---
 ipatests/test_integration/tasks.py |  9 +++--
 .../test_customized_ds_config_install.py   | 23 ++
 2 files changed, 13 insertions(+), 19 deletions(-)

diff --git a/ipatests/test_integration/tasks.py b/ipatests/test_integration/tasks.py
index 60e9e82391077ce6b997d0ed4ad4f2ff19f43d1e..672b09e4aa98bcf2cdd0c8574aaa538f33100c2f 100644
--- a/ipatests/test_integration/tasks.py
+++ b/ipatests/test_integration/tasks.py
@@ -262,7 +262,7 @@ def enable_replication_debugging(host):
  stdin_text=logging_ldif)
 
 
-def install_master(host, setup_dns=True, setup_kra=False):
+def install_master(host, setup_dns=True, setup_kra=False, extra_args=()):
 host.collect_log(paths.IPASERVER_INSTALL_LOG)
 host.collect_log(paths.IPACLIENT_INSTALL_LOG)
 inst = host.domain.realm.replace('.', '-')
@@ -288,6 +288,8 @@ def install_master(host, setup_dns=True, setup_kra=False):
 '--auto-reverse'
 ])
 
+args.extend(extra_args)
+
 host.run_command(args)
 enable_replication_debugging(host)
 setup_sssd_debugging(host)
@@ -345,7 +347,7 @@ def replica_prepare(master, replica):
 
 
 def install_replica(master, replica, setup_ca=True, setup_dns=False,
-setup_kra=False):
+setup_kra=False, extra_args=()):
 replica.collect_log(paths.IPAREPLICA_INSTALL_LOG)
 replica.collect_log(paths.IPAREPLICA_CONNCHECK_LOG)
 allow_sync_ptr(master)
@@ -363,6 +365,9 @@ def install_replica(master, replica, setup_ca=True, setup_dns=False,
 ])
 if master_authoritative_for_client_domain(master, replica):
 args.extend(['--ip-address', replica.ip])
+
+args.extend(extra_args)
+
 if domainlevel(master) == DOMAIN_LEVEL_0:
 # prepare the replica file on master and put it to replica, AKA "old way"
 replica_prepare(master, replica)
diff --git a/ipatests/test_integration/test_customized_ds_config_install.py b/ipatests/test_integration/test_customized_ds_config_install.py
index 0d8c836d9c511965d4158044a4bcecfdd7774800..b0ee8f73e1bf7ae0d859280642c01bbea93f87d5 100644
--- a/ipatests/test_integration/test_customized_ds_config_install.py
+++ b/ipatests/test_integration/test_customized_ds_config_install.py
@@ -59,14 +59,9 @@ class TestCustomInstallMaster(IntegrationTest):
 cls.master.put_file_contents(CONFIG_LDIF_PATH, DIRSRV_CONFIG_MODS)
 
 def test_customized_ds_install_master(self):
-args = [
-'ipa-server-install', '-U',
-'-r', self.master.domain.name,
-'-p', self.master.config.dirman_password,
-'-a', self.master.config.admin_password,
-'--dirsrv-config-file', CONFIG_LDIF_PATH,
-]
-self.master.run_command(args)
+tasks.install_master(self.master, setup_dns=False, extra_args=[
+'--dirsrv-config-file', CONFIG_LDIF_PATH
+])
 
 
 class TestCustomInstallReplica(IntegrationTest):
@@ -83,12 +78,6 @@ class TestCustomInstallReplica(IntegrationTest):
 tasks.install_master(cls.master)
 
 def test_customized_ds_install_replica(self):
-tasks.replica_prepare(self.master, self.replicas[0])
-replica_filename = tasks.get_replica_filename(self.replicas[0])
-args = ['ipa-replica-install', '-U',
-'-p', self.replicas[0].config.dirman_password,
-'-w', self.replicas[0].config.admin_password,
-'--ip-address', self.replicas[0].ip,
-'--dirsrv-config-file', CONFIG_LDIF_PATH,
-replica_filename]
-self.replicas[0].run_command(args)
+tasks.install_replica(
+self.master, self.replicas[0], setup_ca=False,
+extra_args=['--dirsrv-config-file', CONFIG_LDIF_PATH])
-- 
2.5.0

-- 
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

[Freeipa-devel] [PATCH 0424] Pylint: add missing attributes of exceptions to definition in pylint plugin

[Martin Basti]

Pylint is not able to handle IPA errors objects, because attributes are 
added into objects dynamically, and pylint 1.5 reports them as no-member 
errors.


https://fedorahosted.org/freeipa/ticket/5615

Patch attached.
From 2e70066d10e8d15c3989a8c1c7583cf83c471f38 Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Wed, 24 Feb 2016 18:48:29 +0100
Subject: [PATCH] Pylint: add missing attributes of errors to definitions

Attributes are added to IPA error objects dynamicaly and pylint is not
able to handle it itself. Add missing attributes to definitions in
pylint plugin.

https://fedorahosted.org/freeipa/ticket/5615
---
 pylint_plugins.py | 36 
 1 file changed, 36 insertions(+)

diff --git a/pylint_plugins.py b/pylint_plugins.py
index f64c426fdc6344d718322b092f106fa00a98b470..c3042f4a4597bef748a786ca0664760f67ebeb6b 100644
--- a/pylint_plugins.py
+++ b/pylint_plugins.py
@@ -96,6 +96,39 @@ ipa_class_members = {
 'startup_traceback',
 'verbose'
 ] + LOGGING_ATTRS,
+'ipalib.errors.ACIError': [
+'info',
+],
+'ipalib.errors.ConversionError': [
+'error',
+],
+'ipalib.errors.DatabaseError': [
+'desc',
+],
+'ipalib.errors.NetworkError': [
+'error',
+],
+'ipalib.errors.NotFound': [
+'reason',
+],
+'ipalib.errors.PublicError': [
+'msg',
+'strerror',
+],
+'ipalib.errors.SingleMatchExpected': [
+'found',
+],
+'ipalib.errors.SkipPluginModule': [
+'reason',
+],
+'ipalib.errors.ValidationError': [
+'error',
+],
+'ipalib.messages.PublicMessage': [
+'msg',
+'strerror',
+'type',
+],
 'ipalib.parameters.Param': [
 'cli_name',
 'cli_short_name',
@@ -162,6 +195,9 @@ ipa_class_members = {
 'ipalib.session.AuthManager': LOGGING_ATTRS,
 'ipalib.session.SessionAuthManager': LOGGING_ATTRS,
 'ipalib.session.SessionManager': LOGGING_ATTRS,
+'ipalib.util.ForwarderValidationError': [
+'msg',
+],
 'ipaserver.install.ldapupdate.LDAPUpdate': LOGGING_ATTRS,
 'ipaserver.rpcserver.KerberosSession': [
 fake_api,
-- 
2.5.0

-- 
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

[Freeipa-devel] [PATCH 0423] fix duplicated except

[Martin Basti]

During my playing with pylint, I fixed this issue which allows us to 
enable additional check in pylint (the nice one).


Patch attached, it should go only to master.
From 547d41f5835e1dd3a4dcf644948ef104cc50c5dc Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Fri, 12 Feb 2016 17:30:10 +0100
Subject: [PATCH] Remove duplicated except

Both exceptions have been catched before, so it is bacically dead code
---
 ipalib/backend.py   | 5 -
 ipalib/plugins/automount.py | 3 ---
 pylintrc| 1 -
 3 files changed, 9 deletions(-)

diff --git a/ipalib/backend.py b/ipalib/backend.py
index 342e17ec5010b546cfb97063cbcddf570c85d70b..1bfb3e401dc0551bed06c4337f05c9f6cfad896e 100644
--- a/ipalib/backend.py
+++ b/ipalib/backend.py
@@ -143,11 +143,6 @@ class Executioner(Backend):
 'non-public: %s: %s', e.__class__.__name__, str(e)
 )
 error = InternalError()
-except Exception as e:
-self.exception(
-'unhandled exception: %s: %s', e.__class__.__name__, str(e)
-)
-error = InternalError()
 destroy_context()
 if error is None:
 return result
diff --git a/ipalib/plugins/automount.py b/ipalib/plugins/automount.py
index d673ef9979f2f0e909dd90c436024efb407e1462..7dc00224e845e12a699ea263abbb0edd45ea4bc0 100644
--- a/ipalib/plugins/automount.py
+++ b/ipalib/plugins/automount.py
@@ -491,9 +491,6 @@ class automountlocation_import(LDAPQuery):
 raise errors.DuplicateEntry(
 message=_('map %(map)s already exists') % dict(
 map=am[1]))
-except errors.DuplicateEntry:
-# This means the same map is used on several mount points.
-pass
 
 # Now iterate over the map files and add the keys. To handle
 # continuation lines I'll make a pass through it to skip comments
diff --git a/pylintrc b/pylintrc
index c6f7f457d4b9b8edf6040be2a96380bb9ed526b5..1003e49217ec9761ea5e17d3e965376a0cca1a32 100644
--- a/pylintrc
+++ b/pylintrc
@@ -74,7 +74,6 @@ disable=
 unsubscriptable-object,
 unsupported-membership-test,
 not-an-iterable,
-duplicate-except,
 singleton-comparison,
 misplaced-comparison-constant,
 consider-using-enumerate,
-- 
2.5.0

-- 
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

[Freeipa-devel] [PATCH] revert temporal changes in the vault CI test

[Martin Basti]

Workaround for #5538 works, tests can be restored to original state

https://fedorahosted.org/freeipa/ticket/5538

Pushed to:
master: a14d68749397a52537594da890fb23e994dd04e2
ipa-4-3: 390f6342952b63d6d27648096d70bc3fdd81dc78

--
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] 0017 configure DNA shared config entry to allow connection with GSSAPI

[thierry bordaz]

On 01/21/2016 05:04 PM, Martin Babinsky wrote:

On 01/21/2016 01:37 PM, thierry bordaz wrote:





Hi Thierry,

I have couple of comments to your patch:

1.)
there is a number of PEP8 errors in the patch 
(http://paste.fedoraproject.org/313246/33893701), please fix them.


See http://www.freeipa.org/page/Python_Coding_Style for our 
conventions used in Python code.


2.)
+DNA_BIND_METHOD   = "dnaRemoteBindMethod"
+DNA_CONN_PROTOCOL = "dnaRemoteConnProtocol"
+DNA_PLUGIN_DN = 'cn=Distributed Numeric Assignment 
Plugin,cn=plugins,cn=config'

+dna_config_base   = 'cn=Posix IDs,%s' % DNA_PLUGIN_DN

Uppercase names are usually reserved for module-level constants. OTOH, 
local variables should be lowercase. Also you can instantiate 
dna_config_base directly as DN, using 2-member tuples, i. e:


"""
dna_config_base = DN(('cn', 'posix IDs'), ('cn', 'Distributed Numeric 
Assignment Plugin'), ('cn', 'plugins'), ('cn', 'config'))

"""

When passing DN object to the formatting functions/operators, it is 
automatically converted to string so no need to hold string and DN 
object separately. This is done in other places (see function/methods 
in replication.py).


3.)

+for i in range(len(entries)) :
+
+mod = []
+if entries[i].single_value.get(DNA_BIND_METHOD) != method:
+mod.append((ldap.MOD_REPLACE, DNA_BIND_METHOD, method))
+
+if entries[i].single_value.get(DNA_CONN_PROTOCOL) != 
protocol:
+mod.append((ldap.MOD_REPLACE, DNA_CONN_PROTOCOL, 
protocol))



please use idiomatic Python when handling list of entries, e.g.:

"""
for entry in entries:
   mod = []
   if entry.single_value.get(DNA_BIND_METHOD) != method:
   ...
"""

4.) I think that this method should in DSInstance class since it deals 
with directory server configuration. Service is a parent object of all 
other service installers/configurators and should contain only methods 
common to more children.


5.) Since the method is called from every installer, it could be 
beneficial to call it in DSInstance.__common_post_setup() as a part of 
Directory server installation. Is there any reason why this is not the 
case?


6.)

+while attempt != MAX_WAIT:
+try:
+entries = conn.get_entries(sharedcfgdn, 
scope=ldap.SCOPE_ONELEVEL, filter='dnaHostname=%s' % self.fqdn)

+break
+except errors.NotFound:
+root_logger.debug("So far enable not find DNA shared 
config entry for dnaHostname=%s under %s. Retry in 2sec" % (self.fqdn, 
sharedcfgdn))

+attempt = attempt + 1
+time.sleep(2)
+continue
+
+# safety checking
+# there is no return, if there are several entries, as a 
workaround of #5510

+if len(entries) != 1:

I am quite afraid what would happen if the server does not return any 
entries until 30 s timeout. The code will then continue to the 
condition which can potentially test an uninitialized variable and 
blow up with 'NameError'. This should be handled more robustly, e. g. 
raise an exception when a timeout is reached and no entries were 
returned.


7.)

+if len(mod) > 0:

A Pythonic way to test for non-empty container is

"""
if mods:
   # do stuff
"""

since an empty list/dict/set evaluates to False and non-empty 
containers are True.



8.)

+entry = conn.get_entry(entries[i].dn)
+if entry.single_value.get(DNA_BIND_METHOD) != method:
+root_logger.error("Fail to set SASL/GSSAPI bind 
method to %s" % (entries[i].dn))
+if entry.single_value.get(DNA_CONN_PROTOCOL) != 
protocol:
+root_logger.error("Fail to set LDAP protocol to 
%s" % (entries[i].dn))


rather than re-fetching the modified entry and testing what happened, 
you can just catch an exception raised by unsuccessfull mod and log an 
error like this:


"""
try:
   conn.modify_s(entry.dn, mod)
except Exception as e:
   root_logger.error("Failed to modify entry {}: {}".format(entry, e))
"""

as a matter of fact, if the modify_s operation would fail for some 
reason, an ldap exception would be raised and you checks would not 
even be executed.


9.)
The debug message on line 219 should read "Unable to find DNA shared 
config entry for dnaHostname=%s so far. Retry in 2 sec.". The errors 
at the end of the method should have "Failed" instead of "Fail".



Hi Martin,

Finally tested... here is the updated patch. Thanks for you patience


thanks
thierry
From 391e597b4de93897fb496cc06552cf9e434af3d5 Mon Sep 17 00:00:00 2001
From: Thierry Bordaz 
Date: Mon, 8 Feb 2016 16:14:58 +0100
Subject: [PATCH] configure DNA plugin shared config entries to allow
 connection with GSSAPI

https://fedorahosted.org/freeipa/ticket/4026

When a replica needs to extend its DNA range, it selects the remote replica with the
larger available range. If there is no replica agreemen

Re: [Freeipa-devel] IPA client realm/domain autodiscovery improvements

[Sumit Bose]

On Wed, Feb 24, 2016 at 04:08:14PM +0100, David Kupka wrote:
> On 24/02/16 15:55, Sumit Bose wrote:
> >On Wed, Feb 24, 2016 at 03:30:40PM +0100, Martin Babinsky wrote:
> >>On 02/24/2016 03:20 PM, Sumit Bose wrote:
> >>>On Wed, Feb 24, 2016 at 01:31:55PM +0100, Petr Vobornik wrote:
> On 02/16/2016 02:23 PM, Martin Babinsky wrote:
> >Hi list,
> >
> >WARNING: huge brain dump ahead.
> >
> >During investigation of https://fedorahosted.org/freeipa/ticket/4305 me
> >and Petr Spaced (CC'ed) came to a conclusion that the IPA realm
> >autodiscovery code used by ipa-client-install is so convoluted, complex
> >and hard to understand that the cost of fixing a bug/adding a behavioral
> >change (there are some other tickets dealing with ipadiscovery, e.g. see
> >https://fedorahosted.org/freeipa/ticket/5270
> >https://fedorahosted.org/freeipa/ticket/3912 ) can potentially be higher
> >that a more large-scale rewrite of the module and related codebase.
> >
> >Since we plan to do some general refactoring work anyway, I would like
> >to discuss the possible course of action we may take to tackle this
> >issue. I would like to present the following options we have been
> >discussing so far:
> >
> >1.) Do a substantial rewrite of existing code 
> >("ipaclient/ipadiscovery.py")
> >
> >We may take the existing IPADiscovery class and try rewrite it in order
> >to get a more concise and deterministic code, which will:
> >
> >* rely more on python-dns and answers provided by resolver (e.g. we are
> >directly parsing resolv.conf for available domains when we can ask the
> >resolver to get domains for us)
> >* be more pythonic (replace error codes with thrown exceptions, clean up
> >numerous C-isms etc.)
> >* not try to outsmart user when server/realm/domain is specified
> >beforehand. Currently, even if you specify all three options, there is
> >still some DNS discovery performed, hence bug #4305. I think that one
> >you specify server(s), not magic should be performed and the discovery
> >process should be reduced to checking whether they are IPA servers and
> >pull all other info (like realm) from them.
> >
> >This may be a considerable effort requiring some time to implement and
> >get right, but IMHO still comparable to the time spent iteratively
> >placing 'if' statements into the existing code and hoping to not break
> >anything. I would even argue it is not worth the effort because we can
> >
> >2.) Use realmd dbus interface to do DNS discovery
> >
> >I have attached aquick and dirty script I have slapped together to
> >leverage 'org.freedesktop.realmd.Discover' interface to do IPA realm
> >discovery for us. This has a lot of appeal to me since we are leveraging
> >existing codebase for DNS discovery and will have to handle only cases
> >when the user manually specifies a list of IPA servers for the client.
> >
> >This however pulls in realmd as a (potentially circular) dependency for
> >ipa client, and when we find bug in the discovery code, we will have to
> >poke upstream (Stef or Sumit I think) to fix it.
> >
> >So from the long-term point of view, Jan Cholasta's (CC'ed) suggestion 
> >to:
> >
> >3.) Split out IPA discovery portion of realmd to a separate C library
> >shared between IPA and realmd
> >
> >may be best. Both projects will have shared codebase (maintained either
> >by us or realmd devs), which can be reused also by other people to
> >create their own discovery/enrollment solution. This would however
> >require sustained and concerted efforts of both teams to create the
> >library and possibly rewrite realmd to accommodate this change.
> >
> >There may be some other options viable for us, if so please mention them
> >in a reply. Also please correct any piece of information I got wrong.
> >
> >TL;DR: IPA realm/domain discovery is a mess, please suggest a way to fix
> >it.
> >
> 
> #3 sounds good from long term perspective.
> 
> In short term, i.e., #4305, we should skip discovery when --on-master is
> used.
> >>>
> >>>I would prefer #2. Because as seen from the patch it is already working
> >>>and can easily be used from python. I think this was also one of the
> >>>reasons why Stef used a DBus service for this instead of a C library
> >>>which then needs various bindings for python, Java, ruby, Go you name
> >>>it.
> >>>
> >>
> >>This approach is also my favorite. However, my (and several of my
> >>colleagues) concern is that
> >>https://bugzilla.redhat.com/show_bug.cgi?id=1271551 will break it in
> >>kickstart environment. I don't know how much of an issue is this, I guess it
> >>completely precludes automatic enrollment during machine provisioning.
> >
> >realmd has the --dbus-peer option. So I guess it mi

Re: [Freeipa-devel] IPA client realm/domain autodiscovery improvements

[David Kupka]

On 24/02/16 15:55, Sumit Bose wrote:

On Wed, Feb 24, 2016 at 03:30:40PM +0100, Martin Babinsky wrote:

On 02/24/2016 03:20 PM, Sumit Bose wrote:

On Wed, Feb 24, 2016 at 01:31:55PM +0100, Petr Vobornik wrote:

On 02/16/2016 02:23 PM, Martin Babinsky wrote:

Hi list,

WARNING: huge brain dump ahead.

During investigation of https://fedorahosted.org/freeipa/ticket/4305 me
and Petr Spaced (CC'ed) came to a conclusion that the IPA realm
autodiscovery code used by ipa-client-install is so convoluted, complex
and hard to understand that the cost of fixing a bug/adding a behavioral
change (there are some other tickets dealing with ipadiscovery, e.g. see
https://fedorahosted.org/freeipa/ticket/5270
https://fedorahosted.org/freeipa/ticket/3912 ) can potentially be higher
that a more large-scale rewrite of the module and related codebase.

Since we plan to do some general refactoring work anyway, I would like
to discuss the possible course of action we may take to tackle this
issue. I would like to present the following options we have been
discussing so far:

1.) Do a substantial rewrite of existing code ("ipaclient/ipadiscovery.py")

We may take the existing IPADiscovery class and try rewrite it in order
to get a more concise and deterministic code, which will:

* rely more on python-dns and answers provided by resolver (e.g. we are
directly parsing resolv.conf for available domains when we can ask the
resolver to get domains for us)
* be more pythonic (replace error codes with thrown exceptions, clean up
numerous C-isms etc.)
* not try to outsmart user when server/realm/domain is specified
beforehand. Currently, even if you specify all three options, there is
still some DNS discovery performed, hence bug #4305. I think that one
you specify server(s), not magic should be performed and the discovery
process should be reduced to checking whether they are IPA servers and
pull all other info (like realm) from them.

This may be a considerable effort requiring some time to implement and
get right, but IMHO still comparable to the time spent iteratively
placing 'if' statements into the existing code and hoping to not break
anything. I would even argue it is not worth the effort because we can

2.) Use realmd dbus interface to do DNS discovery

I have attached aquick and dirty script I have slapped together to
leverage 'org.freedesktop.realmd.Discover' interface to do IPA realm
discovery for us. This has a lot of appeal to me since we are leveraging
existing codebase for DNS discovery and will have to handle only cases
when the user manually specifies a list of IPA servers for the client.

This however pulls in realmd as a (potentially circular) dependency for
ipa client, and when we find bug in the discovery code, we will have to
poke upstream (Stef or Sumit I think) to fix it.

So from the long-term point of view, Jan Cholasta's (CC'ed) suggestion to:

3.) Split out IPA discovery portion of realmd to a separate C library
shared between IPA and realmd

may be best. Both projects will have shared codebase (maintained either
by us or realmd devs), which can be reused also by other people to
create their own discovery/enrollment solution. This would however
require sustained and concerted efforts of both teams to create the
library and possibly rewrite realmd to accommodate this change.

There may be some other options viable for us, if so please mention them
in a reply. Also please correct any piece of information I got wrong.

TL;DR: IPA realm/domain discovery is a mess, please suggest a way to fix
it.



#3 sounds good from long term perspective.

In short term, i.e., #4305, we should skip discovery when --on-master is
used.


I would prefer #2. Because as seen from the patch it is already working
and can easily be used from python. I think this was also one of the
reasons why Stef used a DBus service for this instead of a C library
which then needs various bindings for python, Java, ruby, Go you name
it.



This approach is also my favorite. However, my (and several of my
colleagues) concern is that
https://bugzilla.redhat.com/show_bug.cgi?id=1271551 will break it in
kickstart environment. I don't know how much of an issue is this, I guess it
completely precludes automatic enrollment during machine provisioning.


realmd has the --dbus-peer option. So I guess it might be possible that
realmd can be started by ipa-client-install directly in this case and
allow communication over a socket pair. I'm not sure how hard or easy
this would be in python.

Stef, do you have some pointers how to use dbus-peer from python?

bye,
Sumit



We have already done something similar with certmonger. You can look 
into ipapython/certmonger.py






About the concerns. rpm-wise realmd has no dependencies on its
underlying tools, it will tell which packages must be installed to do
the join or will use packagekit to install them on its own. So it is
safe to add a realmd dependency to the ipa-client package. And as long
as you only use the

Re: [Freeipa-devel] IPA client realm/domain autodiscovery improvements

[Martin Babinsky]

On 02/24/2016 01:31 PM, Petr Vobornik wrote:

On 02/16/2016 02:23 PM, Martin Babinsky wrote:

Hi list,

WARNING: huge brain dump ahead.

During investigation of https://fedorahosted.org/freeipa/ticket/4305 me
and Petr Spaced (CC'ed) came to a conclusion that the IPA realm
autodiscovery code used by ipa-client-install is so convoluted, complex
and hard to understand that the cost of fixing a bug/adding a behavioral
change (there are some other tickets dealing with ipadiscovery, e.g. see
https://fedorahosted.org/freeipa/ticket/5270
https://fedorahosted.org/freeipa/ticket/3912 ) can potentially be higher
that a more large-scale rewrite of the module and related codebase.

Since we plan to do some general refactoring work anyway, I would like
to discuss the possible course of action we may take to tackle this
issue. I would like to present the following options we have been
discussing so far:

1.) Do a substantial rewrite of existing code
("ipaclient/ipadiscovery.py")

We may take the existing IPADiscovery class and try rewrite it in order
to get a more concise and deterministic code, which will:

* rely more on python-dns and answers provided by resolver (e.g. we are
directly parsing resolv.conf for available domains when we can ask the
resolver to get domains for us)
* be more pythonic (replace error codes with thrown exceptions, clean up
numerous C-isms etc.)
* not try to outsmart user when server/realm/domain is specified
beforehand. Currently, even if you specify all three options, there is
still some DNS discovery performed, hence bug #4305. I think that one
you specify server(s), not magic should be performed and the discovery
process should be reduced to checking whether they are IPA servers and
pull all other info (like realm) from them.

This may be a considerable effort requiring some time to implement and
get right, but IMHO still comparable to the time spent iteratively
placing 'if' statements into the existing code and hoping to not break
anything. I would even argue it is not worth the effort because we can

2.) Use realmd dbus interface to do DNS discovery

I have attached aquick and dirty script I have slapped together to
leverage 'org.freedesktop.realmd.Discover' interface to do IPA realm
discovery for us. This has a lot of appeal to me since we are leveraging
existing codebase for DNS discovery and will have to handle only cases
when the user manually specifies a list of IPA servers for the client.

This however pulls in realmd as a (potentially circular) dependency for
ipa client, and when we find bug in the discovery code, we will have to
poke upstream (Stef or Sumit I think) to fix it.

So from the long-term point of view, Jan Cholasta's (CC'ed) suggestion
to:

3.) Split out IPA discovery portion of realmd to a separate C library
shared between IPA and realmd

may be best. Both projects will have shared codebase (maintained either
by us or realmd devs), which can be reused also by other people to
create their own discovery/enrollment solution. This would however
require sustained and concerted efforts of both teams to create the
library and possibly rewrite realmd to accommodate this change.

There may be some other options viable for us, if so please mention them
in a reply. Also please correct any piece of information I got wrong.

TL;DR: IPA realm/domain discovery is a mess, please suggest a way to fix
it.



#3 sounds good from long term perspective.

In short term, i.e., #4305, we should skip discovery when --on-master is
used.


That was implemented already here 
https://www.redhat.com/archives/freeipa-devel/2015-October/msg00125.html 
along with some minor refactoring, but Petr Spacek perma-nacked the 
patchset.


Alternative approach for #4305 would be to disable search for Kerberos 
KDC when list of servers, domain and realm is specified, since it always 
runs regardless of what was forced during discovery and is the only 
thing that causes #4305.


--
Martin^3 Babinsky

--
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] [REVIEW] Intial stab towards Authentication Indicators

[Nathaniel McCallum]

On Sun, 2016-02-21 at 20:50 -0500, Simo Sorce wrote:
> On Sun, 2016-02-21 at 20:20 -0500, Nathaniel McCallum wrote:
> > 
> > https://github.com/npmccallum/freeipa/pull/1
> > 
> > The above (pseudo) pull request contains four patches against
> > FreeIPA
> > to enable the insertion of Authentication Indicators into Kerberos
> > tickets. The basic flow looks like this.
> > 
> > First, we patch ipa-pwd-extop to return a control indicating what
> > authentication method succeeded resulting in a successful bind.
> > 
> > Second, we patch ipa-otpd to check the returned control to ensure
> > that
> > the bind resulted from an otp validation.
> > 
> > Third, we patch ipa-kdb to enable the KDC to return either the
> > encrypted timestamp or encrypted challenge preauth mechanism when
> > the
> > user is configured for optional 2FA logins. Clients can then decide
> > whether to do 1FA or 2FA login (for kinit, sane behavior already
> > exists).
> > 
> > Forth, we patch ipa-kdb again to insert hard-coded authentication
> > indicators for either OTP or RADIUS.
> > 
> > Some explanation is required for the first two patches. Currently,
> > it
> > is possible to do a 1FA through the otp preauthentication mechanism
> > if
> > the user is configured for doing optional 2FA. However, because we
> > want
> > to insert an authentication indicator in this code path, we need to
> > guarantee that a request going through the otp preauth mechanism
> > actually validates an OTP. This is the purpose of the control.
> > 
> > Items still on the TODO list:
> > 
> >   * Authentication Indicator enforcement
> > - Upstream libkrb5 needs to grow funcs for reading indicators
> > - Schema change to add indicators multi-value attr to services
> > - ipa-kdb needs to implement check_policy_tgs()
> > 
> > 
> >   * SSSD needs to learn to handle optional 2FA
> > 
> > I will write up a project page for all of this tomorrow. But this
> > small
> > code basically amounts to my brainstorming. It is not ready for
> > merge,
> > just basic review.
> > 
> It looks mostly ok, however the LDAP control part needs to be done as
> a
> request/response pair.
> A client that wishes to know what kind of authentication happened
> should
> send a request control, and only in that case , the server will send
> the
> associated reply control with the requested information.

I just pushed a new version of the control (now merged into a single
patch): https://github.com/npmccallum/freeipa/commit/a78191ee5d31e1de39
f28eb637f66199da7e9225

In this version the client sends a critical control with no content
indicating that the server must validate an OTP. If the LDAP server
doesn't support the control (for whatever reason), bind will fail. If
the LDAP server doesn't validate an OTP (for whatever reason), bind
will fail.

This approach is simpler and doesn't require a request/response control
pair.

Nathaniel

-- 
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 realm/domain autodiscovery improvements

[Sumit Bose]

On Wed, Feb 24, 2016 at 03:30:40PM +0100, Martin Babinsky wrote:
> On 02/24/2016 03:20 PM, Sumit Bose wrote:
> >On Wed, Feb 24, 2016 at 01:31:55PM +0100, Petr Vobornik wrote:
> >>On 02/16/2016 02:23 PM, Martin Babinsky wrote:
> >>>Hi list,
> >>>
> >>>WARNING: huge brain dump ahead.
> >>>
> >>>During investigation of https://fedorahosted.org/freeipa/ticket/4305 me
> >>>and Petr Spaced (CC'ed) came to a conclusion that the IPA realm
> >>>autodiscovery code used by ipa-client-install is so convoluted, complex
> >>>and hard to understand that the cost of fixing a bug/adding a behavioral
> >>>change (there are some other tickets dealing with ipadiscovery, e.g. see
> >>>https://fedorahosted.org/freeipa/ticket/5270
> >>>https://fedorahosted.org/freeipa/ticket/3912 ) can potentially be higher
> >>>that a more large-scale rewrite of the module and related codebase.
> >>>
> >>>Since we plan to do some general refactoring work anyway, I would like
> >>>to discuss the possible course of action we may take to tackle this
> >>>issue. I would like to present the following options we have been
> >>>discussing so far:
> >>>
> >>>1.) Do a substantial rewrite of existing code ("ipaclient/ipadiscovery.py")
> >>>
> >>>We may take the existing IPADiscovery class and try rewrite it in order
> >>>to get a more concise and deterministic code, which will:
> >>>
> >>>* rely more on python-dns and answers provided by resolver (e.g. we are
> >>>directly parsing resolv.conf for available domains when we can ask the
> >>>resolver to get domains for us)
> >>>* be more pythonic (replace error codes with thrown exceptions, clean up
> >>>numerous C-isms etc.)
> >>>* not try to outsmart user when server/realm/domain is specified
> >>>beforehand. Currently, even if you specify all three options, there is
> >>>still some DNS discovery performed, hence bug #4305. I think that one
> >>>you specify server(s), not magic should be performed and the discovery
> >>>process should be reduced to checking whether they are IPA servers and
> >>>pull all other info (like realm) from them.
> >>>
> >>>This may be a considerable effort requiring some time to implement and
> >>>get right, but IMHO still comparable to the time spent iteratively
> >>>placing 'if' statements into the existing code and hoping to not break
> >>>anything. I would even argue it is not worth the effort because we can
> >>>
> >>>2.) Use realmd dbus interface to do DNS discovery
> >>>
> >>>I have attached aquick and dirty script I have slapped together to
> >>>leverage 'org.freedesktop.realmd.Discover' interface to do IPA realm
> >>>discovery for us. This has a lot of appeal to me since we are leveraging
> >>>existing codebase for DNS discovery and will have to handle only cases
> >>>when the user manually specifies a list of IPA servers for the client.
> >>>
> >>>This however pulls in realmd as a (potentially circular) dependency for
> >>>ipa client, and when we find bug in the discovery code, we will have to
> >>>poke upstream (Stef or Sumit I think) to fix it.
> >>>
> >>>So from the long-term point of view, Jan Cholasta's (CC'ed) suggestion to:
> >>>
> >>>3.) Split out IPA discovery portion of realmd to a separate C library
> >>>shared between IPA and realmd
> >>>
> >>>may be best. Both projects will have shared codebase (maintained either
> >>>by us or realmd devs), which can be reused also by other people to
> >>>create their own discovery/enrollment solution. This would however
> >>>require sustained and concerted efforts of both teams to create the
> >>>library and possibly rewrite realmd to accommodate this change.
> >>>
> >>>There may be some other options viable for us, if so please mention them
> >>>in a reply. Also please correct any piece of information I got wrong.
> >>>
> >>>TL;DR: IPA realm/domain discovery is a mess, please suggest a way to fix
> >>>it.
> >>>
> >>
> >>#3 sounds good from long term perspective.
> >>
> >>In short term, i.e., #4305, we should skip discovery when --on-master is
> >>used.
> >
> >I would prefer #2. Because as seen from the patch it is already working
> >and can easily be used from python. I think this was also one of the
> >reasons why Stef used a DBus service for this instead of a C library
> >which then needs various bindings for python, Java, ruby, Go you name
> >it.
> >
> 
> This approach is also my favorite. However, my (and several of my
> colleagues) concern is that
> https://bugzilla.redhat.com/show_bug.cgi?id=1271551 will break it in
> kickstart environment. I don't know how much of an issue is this, I guess it
> completely precludes automatic enrollment during machine provisioning.

realmd has the --dbus-peer option. So I guess it might be possible that
realmd can be started by ipa-client-install directly in this case and
allow communication over a socket pair. I'm not sure how hard or easy
this would be in python.

Stef, do you have some pointers how to use dbus-peer from python?

bye,
Sumit

> 
> >About the concerns. rpm-wise real

Re: [Freeipa-devel] [PATCH 0420] Set BuildRequires to pylint 1.4

[Martin Basti]

On 24.02.2016 13:38, Lukas Slebodnik wrote:

On (24/02/16 11:06), Petr Vobornik wrote:

On 02/24/2016 09:50 AM, Lukas Slebodnik wrote:

On (23/02/16 14:23), Rob Crittenden wrote:

Lukas Slebodnik wrote:

On (23/02/16 17:09), Martin Basti wrote:

We cannot guarantee that versions older than 1.4 will work with freeipa code.

Patch attached.

>From a59e72a0b87231c0f2e0d737057550dd532feed7 Mon Sep 17 00:00:00 2001

From: Martin Basti 
Date: Tue, 23 Feb 2016 16:58:07 +0100
Subject: [PATCH] Set BuildRequires to pylint >= 1.4

We can guarantee that only pylint 1.4 and newer will work

https://fedorahosted.org/freeipa/ticket/5615
---
freeipa.spec.in | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/freeipa.spec.in b/freeipa.spec.in
index 
54a11bfc8cced643c19c29c5ada70bacf7540395..219c5ca2f13eaac14746ec4689ba611bbc6fc377
 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -76,7 +76,7 @@ BuildRequires:  python-netaddr
BuildRequires:  python-gssapi >= 1.1.2
BuildRequires:  python-rhsm
BuildRequires:  pyOpenSSL
-BuildRequires:  pylint >= 1.0
+BuildRequires:  pylint >= 1.4

I can build rpms even withour pylint and pylint is not executed
anywhere in spec file. (in other words, my patch was rejected)
Why does it need to be in BuildRequires?

pylint is part of the in-tree build process (make rpms). It is not
executed when building upstream packages.


It's not buildrequires becuase I can rebuild src.rpm
without it. It should not be there or it should be optional
to do not break developer workflow.

e.g. "%bcond_with extra_dependencies_for_pylint"

The upstream spec files is close to the fedora spec file
and pylint is istalled there even though it's not used.

Another use case is coverity scan.

LS


Basically I agree with Lukas but this patch is the way how we do build
upstream right now.

The proposed change would required more refactoring of build process and is
out of scope of this ticket. Feel free to open a ticket for it. It should not
block this patch.

I have a different opinion here.

The ticket #5615 says: "Prepare IPA for pylint 1.5.2"
It does not say anything about minimal requirement on pylint 1.4
even though all current version of fedora has pylint 1.4.

Older version of pylint (1.3.1) is available only in fedora 20.
Which is out of support for more than a year.
Patch is neither required for fedora nor for ticket #5615

However there is only pylint-1.3.1 in epel7
And it would cause issues there.

Summary: This patch does not solve anything.

LS

We can discard this patch, I do not insist on it, and I do not plan to 
spent time on it. In future we would add minimal dependency to pylint 
1.5.x to have better checks accessible.`


--
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 0413] fix permission: Read Replication Agreements

[Martin Basti]

On 24.02.2016 13:36, Jan Cholasta wrote:

On 24.2.2016 13:07, Martin Basti wrote:



On 24.02.2016 10:45, Jan Cholasta wrote:

On 23.2.2016 17:20, Martin Basti wrote:



On 22.02.2016 09:00, Jan Cholasta wrote:

Hi,

On 17.2.2016 14:49, Martin Basti wrote:

https://fedorahosted.org/freeipa/ticket/5631

Patch attached (for master, 4.3, 4.2)


1) All the replication agreement permission ACIs should be located in
the same entry. Currently "Read Replication Agreements" is in
"cn=config" and everything else in "cn=mapping tree,cn=config", so I
guess "cn=mapping tree,cn=config" makes more sense.


2) Instead of literal DN('cn=permissions,cn=pbac'), use
api.env.container_permissions.


3) IMO the removal of managed permission attributes could be a little
bit more robust. You should check that the original entry contains 
all

the required values before touching it (objectclass=ipapermissionv2,
ipapermissiontype=V2, ipapermissiontype=MANAGED) and remove only the
values that need to be removed, instead of just overwriting 
everything.



Honza


Updated patch attached.


The patch does not apply on ipa-4-2.


I will send it later.


Also this bit in replica-acis.ldif is redundant:

+
+dn: cn=mapping tree,cn=config
+changetype: modify
+add: aci

All related ACIs to replication are in both replica-acis.ldif and
20-aci.update.
I just do not want to mess it more than it is.


What I'm trying to say is that:

dn: cn=mapping tree,cn=config
changetype: modify
add: aci
aci: $ACI1

dn: cn=mapping tree,cn=config
changetype: modify
add: aci
aci: $ACI2

is the same as:

dn: cn=mapping tree,cn=config
changetype: modify
add: aci
aci: $ACI1
aci: $ACI2

. You actually have it right in 20-aci.update, but not in 
replica-acis.ldif.



I made it in that way to keep consistency in the replica-acis.ldif file.

Patch for 4-2 added


From bf05a9b458c82e4be4273f00610cdaa7569a5036 Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Thu, 4 Feb 2016 16:23:40 +0100
Subject: [PATCH] fix permission: Read Replication Agreements

This permission cannot be MANAGED permission because it is located in
nonreplicating part of the LDAP tree.

As side effect, the particular ACI has not been created on all replicas.

This commit makes Read Replication Agreements non managed permission and
also fix missing ACI on replicas.

https://fedorahosted.org/freeipa/ticket/5631
---
 ACI.txt|   2 -
 install/share/delegation.ldif  |   9 ++
 install/share/replica-acis.ldif|   5 +
 install/updates/20-aci.update  |   7 +-
 install/updates/90-post_upgrade_plugins.update |   1 +
 .../install/plugins/update_managed_permissions.py  | 133 +++--
 6 files changed, 93 insertions(+), 64 deletions(-)

diff --git a/ACI.txt b/ACI.txt
index 40fa822217eaee8d0966491b10cdf7e0739a87ce..fd1ba6725f2377a2506284c9477f99424e9d1e0a 100644
--- a/ACI.txt
+++ b/ACI.txt
@@ -388,8 +388,6 @@ dn: cn=Domain Level,cn=ipa,cn=etc,dc=ipa,dc=example
 aci: (targetattr = "createtimestamp || entryusn || ipadomainlevel || modifytimestamp || objectclass")(targetfilter = "(objectclass=ipadomainlevelconfig)")(version 3.0;acl "permission:System: Read Domain Level";allow (compare,read,search) userdn = "ldap:///all";;)
 dn: cn=masters,cn=ipa,cn=etc,dc=ipa,dc=example
 aci: (targetattr = "cn || createtimestamp || entryusn || ipaconfigstring || modifytimestamp || objectclass")(targetfilter = "(objectclass=nscontainer)")(version 3.0;acl "permission:System: Read IPA Masters";allow (compare,read,search) groupdn = "ldap:///cn=System: Read IPA Masters,cn=permissions,cn=pbac,dc=ipa,dc=example";)
-dn: cn=config
-aci: (targetattr = "cn || createtimestamp || description || entryusn || modifytimestamp || nsds50ruv || nsds5beginreplicarefresh || nsds5debugreplicatimeout || nsds5flags || nsds5replicaabortcleanruv || nsds5replicaautoreferral || nsds5replicabackoffmax || nsds5replicabackoffmin || nsds5replicabinddn || nsds5replicabindmethod || nsds5replicabusywaittime || nsds5replicachangecount || nsds5replicachangessentsincestartup || nsds5replicacleanruv || nsds5replicacleanruvnotified || nsds5replicacredentials || nsds5replicaenabled || nsds5replicahost || nsds5replicaid || nsds5replicalastinitend || nsds5replicalastinitstart || nsds5replicalastinitstatus || nsds5replicalastupdateend || nsds5replicalastupdatestart || nsds5replicalastupdatestatus || nsds5replicalegacyconsumer || nsds5replicaname || nsds5replicaport || nsds5replicaprotocoltimeout || nsds5replicapurgedelay || nsds5replicareferral || nsds5replicaroot || nsds5replicasessionpausetime || nsds5replicastripattrs || nsds5replicatedattributelist || nsds5replicatedattributelisttotal || nsds5replicatimeout || nsds5replicatombstonepurgeinterval || nsds5replicatransportinfo || nsds5replicatype || nsds5replicaupdateinprogress || nsds5replicaupdateschedule || nsds5task || nsds7directoryreplicasubtree || 

Re: [Freeipa-devel] IPA client realm/domain autodiscovery improvements

[Martin Babinsky]

On 02/24/2016 03:20 PM, Sumit Bose wrote:

On Wed, Feb 24, 2016 at 01:31:55PM +0100, Petr Vobornik wrote:

On 02/16/2016 02:23 PM, Martin Babinsky wrote:

Hi list,

WARNING: huge brain dump ahead.

During investigation of https://fedorahosted.org/freeipa/ticket/4305 me
and Petr Spaced (CC'ed) came to a conclusion that the IPA realm
autodiscovery code used by ipa-client-install is so convoluted, complex
and hard to understand that the cost of fixing a bug/adding a behavioral
change (there are some other tickets dealing with ipadiscovery, e.g. see
https://fedorahosted.org/freeipa/ticket/5270
https://fedorahosted.org/freeipa/ticket/3912 ) can potentially be higher
that a more large-scale rewrite of the module and related codebase.

Since we plan to do some general refactoring work anyway, I would like
to discuss the possible course of action we may take to tackle this
issue. I would like to present the following options we have been
discussing so far:

1.) Do a substantial rewrite of existing code ("ipaclient/ipadiscovery.py")

We may take the existing IPADiscovery class and try rewrite it in order
to get a more concise and deterministic code, which will:

* rely more on python-dns and answers provided by resolver (e.g. we are
directly parsing resolv.conf for available domains when we can ask the
resolver to get domains for us)
* be more pythonic (replace error codes with thrown exceptions, clean up
numerous C-isms etc.)
* not try to outsmart user when server/realm/domain is specified
beforehand. Currently, even if you specify all three options, there is
still some DNS discovery performed, hence bug #4305. I think that one
you specify server(s), not magic should be performed and the discovery
process should be reduced to checking whether they are IPA servers and
pull all other info (like realm) from them.

This may be a considerable effort requiring some time to implement and
get right, but IMHO still comparable to the time spent iteratively
placing 'if' statements into the existing code and hoping to not break
anything. I would even argue it is not worth the effort because we can

2.) Use realmd dbus interface to do DNS discovery

I have attached aquick and dirty script I have slapped together to
leverage 'org.freedesktop.realmd.Discover' interface to do IPA realm
discovery for us. This has a lot of appeal to me since we are leveraging
existing codebase for DNS discovery and will have to handle only cases
when the user manually specifies a list of IPA servers for the client.

This however pulls in realmd as a (potentially circular) dependency for
ipa client, and when we find bug in the discovery code, we will have to
poke upstream (Stef or Sumit I think) to fix it.

So from the long-term point of view, Jan Cholasta's (CC'ed) suggestion to:

3.) Split out IPA discovery portion of realmd to a separate C library
shared between IPA and realmd

may be best. Both projects will have shared codebase (maintained either
by us or realmd devs), which can be reused also by other people to
create their own discovery/enrollment solution. This would however
require sustained and concerted efforts of both teams to create the
library and possibly rewrite realmd to accommodate this change.

There may be some other options viable for us, if so please mention them
in a reply. Also please correct any piece of information I got wrong.

TL;DR: IPA realm/domain discovery is a mess, please suggest a way to fix
it.



#3 sounds good from long term perspective.

In short term, i.e., #4305, we should skip discovery when --on-master is
used.


I would prefer #2. Because as seen from the patch it is already working
and can easily be used from python. I think this was also one of the
reasons why Stef used a DBus service for this instead of a C library
which then needs various bindings for python, Java, ruby, Go you name
it.



This approach is also my favorite. However, my (and several of my 
colleagues) concern is that 
https://bugzilla.redhat.com/show_bug.cgi?id=1271551 will break it in 
kickstart environment. I don't know how much of an issue is this, I 
guess it completely precludes automatic enrollment during machine 
provisioning.



About the concerns. rpm-wise realmd has no dependencies on its
underlying tools, it will tell which packages must be installed to do
the join or will use packagekit to install them on its own. So it is
safe to add a realmd dependency to the ipa-client package. And as long
as you only use the Discover method realmd would not try to call
ipa-client-install, so there is no logical circle either. To avoid an
un-needed second discovery when ipa-client-install is not run from the
command line but from realmd directly ipa-client-install can skip the
realmd call if domain and realm are already given on the command line
because realmd will set both options when calling ipa-client-install.



And if there is a bug you have to poke the maintainer of the library or
realmd either way.



bye,
Sumit



--
Petr Voborn

Re: [Freeipa-devel] Locations design v2: LDAP schema & user interface

[Simo Sorce]

On Wed, 2016-02-24 at 10:00 +0100, Martin Kosek wrote:
> On 02/23/2016 06:59 PM, Petr Spacek wrote:
> > On 23.2.2016 18:14, Simo Sorce wrote:
> ...
> >> More seriously I think it is a great idea, but too premature to get all
> >> the way there now. We need to build schema and CLI that will allow us to
> >> get there without having to completely change interfaces if at all
> >> possible or minimizing any disruption in the tools.
> > 
> > Actually the backwards compatibility is the main worry which led to this 
> > idea
> > with links.
> > 
> > If we release first version of locations with custom priorities etc. we will
> > have support the schema (which will be different) and API (which will be 
> > later
> > unnecessary) forever.
> > 
> > If we skip this intermediate phase with hand-made configuration we can save
> > all the headache with upgrades to more automatic solution later on.
> > 
> > 
> > Maybe we should invert the order:
> > Start with locations + links with administrative metric and add 
> > hand-tweaking
> > capabilities later (if necessary).
> > 
> > IMHO locations + links with administrative metric will be easier to 
> > implement
> > than the first version.
> > 
> > Just thinking aloud ...
> 
> Makes sense to me, I would have the same worry as Petr, that we would break
> something if we decide moving to links based solution later.

Maybe I am missing something, but in order to generate the proper SRV
records we need priority and weights anyway, either by entering them
manually or by autogenerating them from some other piece of information
in the framework. So given this information is needed anyway why would
it become a problem to retain it in the future if we enable a tool the
simply autogenerates this information ?

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] IPA client realm/domain autodiscovery improvements

[Sumit Bose]

On Wed, Feb 24, 2016 at 01:31:55PM +0100, Petr Vobornik wrote:
> On 02/16/2016 02:23 PM, Martin Babinsky wrote:
> >Hi list,
> >
> >WARNING: huge brain dump ahead.
> >
> >During investigation of https://fedorahosted.org/freeipa/ticket/4305 me
> >and Petr Spaced (CC'ed) came to a conclusion that the IPA realm
> >autodiscovery code used by ipa-client-install is so convoluted, complex
> >and hard to understand that the cost of fixing a bug/adding a behavioral
> >change (there are some other tickets dealing with ipadiscovery, e.g. see
> >https://fedorahosted.org/freeipa/ticket/5270
> >https://fedorahosted.org/freeipa/ticket/3912 ) can potentially be higher
> >that a more large-scale rewrite of the module and related codebase.
> >
> >Since we plan to do some general refactoring work anyway, I would like
> >to discuss the possible course of action we may take to tackle this
> >issue. I would like to present the following options we have been
> >discussing so far:
> >
> >1.) Do a substantial rewrite of existing code ("ipaclient/ipadiscovery.py")
> >
> >We may take the existing IPADiscovery class and try rewrite it in order
> >to get a more concise and deterministic code, which will:
> >
> >* rely more on python-dns and answers provided by resolver (e.g. we are
> >directly parsing resolv.conf for available domains when we can ask the
> >resolver to get domains for us)
> >* be more pythonic (replace error codes with thrown exceptions, clean up
> >numerous C-isms etc.)
> >* not try to outsmart user when server/realm/domain is specified
> >beforehand. Currently, even if you specify all three options, there is
> >still some DNS discovery performed, hence bug #4305. I think that one
> >you specify server(s), not magic should be performed and the discovery
> >process should be reduced to checking whether they are IPA servers and
> >pull all other info (like realm) from them.
> >
> >This may be a considerable effort requiring some time to implement and
> >get right, but IMHO still comparable to the time spent iteratively
> >placing 'if' statements into the existing code and hoping to not break
> >anything. I would even argue it is not worth the effort because we can
> >
> >2.) Use realmd dbus interface to do DNS discovery
> >
> >I have attached aquick and dirty script I have slapped together to
> >leverage 'org.freedesktop.realmd.Discover' interface to do IPA realm
> >discovery for us. This has a lot of appeal to me since we are leveraging
> >existing codebase for DNS discovery and will have to handle only cases
> >when the user manually specifies a list of IPA servers for the client.
> >
> >This however pulls in realmd as a (potentially circular) dependency for
> >ipa client, and when we find bug in the discovery code, we will have to
> >poke upstream (Stef or Sumit I think) to fix it.
> >
> >So from the long-term point of view, Jan Cholasta's (CC'ed) suggestion to:
> >
> >3.) Split out IPA discovery portion of realmd to a separate C library
> >shared between IPA and realmd
> >
> >may be best. Both projects will have shared codebase (maintained either
> >by us or realmd devs), which can be reused also by other people to
> >create their own discovery/enrollment solution. This would however
> >require sustained and concerted efforts of both teams to create the
> >library and possibly rewrite realmd to accommodate this change.
> >
> >There may be some other options viable for us, if so please mention them
> >in a reply. Also please correct any piece of information I got wrong.
> >
> >TL;DR: IPA realm/domain discovery is a mess, please suggest a way to fix
> >it.
> >
> 
> #3 sounds good from long term perspective.
> 
> In short term, i.e., #4305, we should skip discovery when --on-master is
> used.

I would prefer #2. Because as seen from the patch it is already working
and can easily be used from python. I think this was also one of the
reasons why Stef used a DBus service for this instead of a C library
which then needs various bindings for python, Java, ruby, Go you name
it.

About the concerns. rpm-wise realmd has no dependencies on its
underlying tools, it will tell which packages must be installed to do
the join or will use packagekit to install them on its own. So it is
safe to add a realmd dependency to the ipa-client package. And as long
as you only use the Discover method realmd would not try to call
ipa-client-install, so there is no logical circle either. To avoid an
un-needed second discovery when ipa-client-install is not run from the
command line but from realmd directly ipa-client-install can skip the
realmd call if domain and realm are already given on the command line
because realmd will set both options when calling ipa-client-install.

And if there is a bug you have to poke the maintainer of the library or
realmd either way.



bye,
Sumit


> -- 
> Petr Vobornik
> 
> -- 
> Manage your subscription for the Freeipa-devel mailing list:
> https://www.redhat.com/mailman/listinfo/freeipa-devel
> Contribute to F

[Freeipa-devel] [PATCH 0421] Make PTR records check optional for IPA installation

[Martin Basti]

https://fedorahosted.org/freeipa/ticket/5686

Patch attached.
From 19a00a076101ff25ba0cd626bab8b5f8a6e6cb68 Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Wed, 24 Feb 2016 14:33:34 +0100
Subject: [PATCH] Make PTR records check optional for IPA installation

PTR records are not mandratory for IPA, result fo checks should be only
warning not hard error.

https://fedorahosted.org/freeipa/ticket/5686
---
 ipaserver/install/installutils.py | 17 +
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/ipaserver/install/installutils.py b/ipaserver/install/installutils.py
index 6c00c3482df4945b3a74fa7d656eb2d1d1347594..4489cd80b9640c33a4f8fdcf383659b80647bd25 100644
--- a/ipaserver/install/installutils.py
+++ b/ipaserver/install/installutils.py
@@ -203,15 +203,16 @@ def verify_fqdn(host_name, no_host_dns=False, local_hostname=True):
 revname = socket.gethostbyaddr(address)[0]
 except Exception as e:
 root_logger.debug('Check failed: %s', e)
-raise HostReverseLookupError(
+root_logger.error(
 "Unable to resolve the IP address %s to a host name, "
-"check /etc/hosts and DNS name resolution" % address)
-root_logger.debug('Found reverse name: %s', revname)
-if revname != host_name:
-raise HostReverseLookupError(
-"The host name %s does not match the value %s obtained "
-"by reverse lookup on IP address %s"
-% (host_name, revname, address))
+"check /etc/hosts and DNS name resolution", address)
+else:
+root_logger.debug('Found reverse name: %s', revname)
+if revname != host_name:
+root_logger.error(
+"The host name %s does not match the value %s obtained "
+"by reverse lookup on IP address %s", host_name, revname,
+address)
 verified.add(address)
 
 
-- 
2.5.0

-- 
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 0011] Move freeipa certmonger helpers to libexecdir.

[Rob Crittenden]

David Kupka wrote:
> On 23/02/16 16:41, Rob Crittenden wrote:
>> David Kupka wrote:
>>> On 23/02/16 10:14, Martin Kosek wrote:
 On 02/23/2016 09:47 AM, David Kupka wrote:
> On 22/02/16 16:15, Martin Kosek wrote:
>> On 02/22/2016 04:04 PM, Jan Cholasta wrote:
>>> On 22.2.2016 15:56, David Kupka wrote:
 On 22/02/16 07:28, Jan Cholasta wrote:
> On 18.2.2016 10:10, David Kupka wrote:
>> On 19/01/16 16:10, David Kupka wrote:
>>> On 19/01/16 14:38, Jan Cholasta wrote:
 On 19.1.2016 14:26, Martin Kosek wrote:
> On 01/19/2016 01:47 PM, David Kupka wrote:
>> I've polished the patch attached to #5586 by Timo Aaltonen.
>>
>> Thanks for the patch. I've fixed the path in specfile and
>> removed
>> unused import
>> but otherwise it works, ACK.
>>
>> https://fedorahosted.org/freeipa/ticket/5586
>
> Won't this break existing certmonger requests depending on
> the old
> path?

 It will, I don't see any upgrade code.

>
> # getcert list | grep '/usr/lib64/ipa/certmonger'
>pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad
>post-save command:
> /usr/lib64/ipa/certmonger/renew_ca_cert
> "auditSigningCert
> cert-pki-ca"
>pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad
>post-save command:
> /usr/lib64/ipa/certmonger/renew_ca_cert
> "ocspSigningCert
> cert-pki-ca"
>pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad
>post-save command:
> /usr/lib64/ipa/certmonger/renew_ca_cert
> "subsystemCert
> cert-pki-ca"
>pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad
>post-save command:
> /usr/lib64/ipa/certmonger/renew_ca_cert
> "caSigningCert
> cert-pki-ca"
>post-save command:
> /usr/lib64/ipa/certmonger/renew_ra_cert
>pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad
>post-save command:
> /usr/lib64/ipa/certmonger/renew_ca_cert
> "Server-Cert
> cert-pki-ca"
>post-save command:
> /usr/lib64/ipa/certmonger/restart_dirsrv
> RHEL72
>post-save command:
> /usr/lib64/ipa/certmonger/restart_httpd
>


>>>
>>> You're right it will break the upgrade. I haven't noticed that
>>> Server-Cert for DS and HTTPD are not handled by
>>> certificate_renewal_update (ipaserver.install.server.upgrade)
>>> where all
>>> the other trackings are stopped and then configured again
>>> with the
>>> paths.CERTMONGER_COMMAND_TEMPLATE already updated.
>>>
>>> Thanks for the catch.
>>>
>>
>> I've updated Timo's patch little more and added
>> start_tracking_certificates() for dsinstance and httpinstance.
>> Now the
>> upgrade works as expected.
>
> The way the patches are split is kind of weird and apparently
> confusing
> (see the other thread). IMO there should be 2 patches: the first
> should
> add the ability to change DS and HTTP certmonger config during
> upgrade
> (i.e. the start_tracking_certificates() methods and
> certificate_renewal_update() changes), the second should move the
> helpers (i.e. the actual move and certificate_renewal_update()
> version
> bump).
>
 Honza, do I understand it correctly that the code is OK but I
 did not
 split it to the patches correctly?
>>>
>>> Yes.
>>
>> Before acking or pushing, can you please explain for me how the
>> upgrade of
>> certmonger tracking requests work? I want to make sure this is
>> right, so please
>> bear with me:
>>
>> 1) How does it edit existing tracking requests with the new helper
>> paths?
>>
>> 2) Does it go and try to edit the requests on every upgrade? Or is
>> there some
>> check that requests were updated?
>>
>> Thanks,
>> Martin
>>
>
> Whole upgrade of renewal requests is done in
> ipaserver/install/server/upgrade.py in certificate_renewal_upgrade().
>
> First there is version of requests and if it's the same as in state
> file
> upgrade is skipped.
> Then every request is searched over certmonger's DBus interface and
> if at least
> one is not found it means that there was change in request
> configuration. All
>>

Re: [Freeipa-devel] [PATCH 0420] Set BuildRequires to pylint 1.4

[Lukas Slebodnik]

On (24/02/16 11:06), Petr Vobornik wrote:
>On 02/24/2016 09:50 AM, Lukas Slebodnik wrote:
>>On (23/02/16 14:23), Rob Crittenden wrote:
>>>Lukas Slebodnik wrote:
On (23/02/16 17:09), Martin Basti wrote:
>We cannot guarantee that versions older than 1.4 will work with freeipa 
>code.
>
>Patch attached.

>From a59e72a0b87231c0f2e0d737057550dd532feed7 Mon Sep 17 00:00:00 2001
>From: Martin Basti 
>Date: Tue, 23 Feb 2016 16:58:07 +0100
>Subject: [PATCH] Set BuildRequires to pylint >= 1.4
>
>We can guarantee that only pylint 1.4 and newer will work
>
>https://fedorahosted.org/freeipa/ticket/5615
>---
>freeipa.spec.in | 2 +-
>1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/freeipa.spec.in b/freeipa.spec.in
>index 
>54a11bfc8cced643c19c29c5ada70bacf7540395..219c5ca2f13eaac14746ec4689ba611bbc6fc377
> 100644
>--- a/freeipa.spec.in
>+++ b/freeipa.spec.in
>@@ -76,7 +76,7 @@ BuildRequires:  python-netaddr
>BuildRequires:  python-gssapi >= 1.1.2
>BuildRequires:  python-rhsm
>BuildRequires:  pyOpenSSL
>-BuildRequires:  pylint >= 1.0
>+BuildRequires:  pylint >= 1.4

I can build rpms even withour pylint and pylint is not executed
anywhere in spec file. (in other words, my patch was rejected)
Why does it need to be in BuildRequires?
>>>
>>>pylint is part of the in-tree build process (make rpms). It is not
>>>executed when building upstream packages.
>>>
>>It's not buildrequires becuase I can rebuild src.rpm
>>without it. It should not be there or it should be optional
>>to do not break developer workflow.
>>
>>e.g. "%bcond_with extra_dependencies_for_pylint"
>>
>>The upstream spec files is close to the fedora spec file
>>and pylint is istalled there even though it's not used.
>>
>>Another use case is coverity scan.
>>
>>LS
>>
>
>Basically I agree with Lukas but this patch is the way how we do build
>upstream right now.
>
>The proposed change would required more refactoring of build process and is
>out of scope of this ticket. Feel free to open a ticket for it. It should not
>block this patch.
I have a different opinion here.

The ticket #5615 says: "Prepare IPA for pylint 1.5.2"
It does not say anything about minimal requirement on pylint 1.4
even though all current version of fedora has pylint 1.4.

Older version of pylint (1.3.1) is available only in fedora 20.
Which is out of support for more than a year.
Patch is neither required for fedora nor for ticket #5615

However there is only pylint-1.3.1 in epel7
And it would cause issues there.

Summary: This patch does not solve anything.

LS

-- 
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 0413] fix permission: Read Replication Agreements

[Jan Cholasta]

On 24.2.2016 13:07, Martin Basti wrote:



On 24.02.2016 10:45, Jan Cholasta wrote:

On 23.2.2016 17:20, Martin Basti wrote:



On 22.02.2016 09:00, Jan Cholasta wrote:

Hi,

On 17.2.2016 14:49, Martin Basti wrote:

https://fedorahosted.org/freeipa/ticket/5631

Patch attached (for master, 4.3, 4.2)


1) All the replication agreement permission ACIs should be located in
the same entry. Currently "Read Replication Agreements" is in
"cn=config" and everything else in "cn=mapping tree,cn=config", so I
guess "cn=mapping tree,cn=config" makes more sense.


2) Instead of literal DN('cn=permissions,cn=pbac'), use
api.env.container_permissions.


3) IMO the removal of managed permission attributes could be a little
bit more robust. You should check that the original entry contains all
the required values before touching it (objectclass=ipapermissionv2,
ipapermissiontype=V2, ipapermissiontype=MANAGED) and remove only the
values that need to be removed, instead of just overwriting everything.


Honza


Updated patch attached.


The patch does not apply on ipa-4-2.


I will send it later.


Also this bit in replica-acis.ldif is redundant:

+
+dn: cn=mapping tree,cn=config
+changetype: modify
+add: aci

All related ACIs to replication are in both replica-acis.ldif and
20-aci.update.
I just do not want to mess it more than it is.


What I'm trying to say is that:

dn: cn=mapping tree,cn=config
changetype: modify
add: aci
aci: $ACI1

dn: cn=mapping tree,cn=config
changetype: modify
add: aci
aci: $ACI2

is the same as:

dn: cn=mapping tree,cn=config
changetype: modify
add: aci
aci: $ACI1
aci: $ACI2

. You actually have it right in 20-aci.update, but not in replica-acis.ldif.

--
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] IPA client realm/domain autodiscovery improvements

[Petr Vobornik]

On 02/16/2016 02:23 PM, Martin Babinsky wrote:

Hi list,

WARNING: huge brain dump ahead.

During investigation of https://fedorahosted.org/freeipa/ticket/4305 me
and Petr Spaced (CC'ed) came to a conclusion that the IPA realm
autodiscovery code used by ipa-client-install is so convoluted, complex
and hard to understand that the cost of fixing a bug/adding a behavioral
change (there are some other tickets dealing with ipadiscovery, e.g. see
https://fedorahosted.org/freeipa/ticket/5270
https://fedorahosted.org/freeipa/ticket/3912 ) can potentially be higher
that a more large-scale rewrite of the module and related codebase.

Since we plan to do some general refactoring work anyway, I would like
to discuss the possible course of action we may take to tackle this
issue. I would like to present the following options we have been
discussing so far:

1.) Do a substantial rewrite of existing code ("ipaclient/ipadiscovery.py")

We may take the existing IPADiscovery class and try rewrite it in order
to get a more concise and deterministic code, which will:

* rely more on python-dns and answers provided by resolver (e.g. we are
directly parsing resolv.conf for available domains when we can ask the
resolver to get domains for us)
* be more pythonic (replace error codes with thrown exceptions, clean up
numerous C-isms etc.)
* not try to outsmart user when server/realm/domain is specified
beforehand. Currently, even if you specify all three options, there is
still some DNS discovery performed, hence bug #4305. I think that one
you specify server(s), not magic should be performed and the discovery
process should be reduced to checking whether they are IPA servers and
pull all other info (like realm) from them.

This may be a considerable effort requiring some time to implement and
get right, but IMHO still comparable to the time spent iteratively
placing 'if' statements into the existing code and hoping to not break
anything. I would even argue it is not worth the effort because we can

2.) Use realmd dbus interface to do DNS discovery

I have attached aquick and dirty script I have slapped together to
leverage 'org.freedesktop.realmd.Discover' interface to do IPA realm
discovery for us. This has a lot of appeal to me since we are leveraging
existing codebase for DNS discovery and will have to handle only cases
when the user manually specifies a list of IPA servers for the client.

This however pulls in realmd as a (potentially circular) dependency for
ipa client, and when we find bug in the discovery code, we will have to
poke upstream (Stef or Sumit I think) to fix it.

So from the long-term point of view, Jan Cholasta's (CC'ed) suggestion to:

3.) Split out IPA discovery portion of realmd to a separate C library
shared between IPA and realmd

may be best. Both projects will have shared codebase (maintained either
by us or realmd devs), which can be reused also by other people to
create their own discovery/enrollment solution. This would however
require sustained and concerted efforts of both teams to create the
library and possibly rewrite realmd to accommodate this change.

There may be some other options viable for us, if so please mention them
in a reply. Also please correct any piece of information I got wrong.

TL;DR: IPA realm/domain discovery is a mess, please suggest a way to fix
it.



#3 sounds good from long term perspective.

In short term, i.e., #4305, we should skip discovery when --on-master is 
used.

--
Petr Vobornik

--
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 0413] fix permission: Read Replication Agreements

[Martin Basti]

On 24.02.2016 10:45, Jan Cholasta wrote:

On 23.2.2016 17:20, Martin Basti wrote:



On 22.02.2016 09:00, Jan Cholasta wrote:

Hi,

On 17.2.2016 14:49, Martin Basti wrote:

https://fedorahosted.org/freeipa/ticket/5631

Patch attached (for master, 4.3, 4.2)


1) All the replication agreement permission ACIs should be located in
the same entry. Currently "Read Replication Agreements" is in
"cn=config" and everything else in "cn=mapping tree,cn=config", so I
guess "cn=mapping tree,cn=config" makes more sense.


2) Instead of literal DN('cn=permissions,cn=pbac'), use
api.env.container_permissions.


3) IMO the removal of managed permission attributes could be a little
bit more robust. You should check that the original entry contains all
the required values before touching it (objectclass=ipapermissionv2,
ipapermissiontype=V2, ipapermissiontype=MANAGED) and remove only the
values that need to be removed, instead of just overwriting everything.


Honza


Updated patch attached.


The patch does not apply on ipa-4-2.


I will send it later.


Also this bit in replica-acis.ldif is redundant:

+
+dn: cn=mapping tree,cn=config
+changetype: modify
+add: aci
All related ACIs to replication are in both replica-acis.ldif and 
20-aci.update.

I just do not want to mess it more than it is.



Honza


Martin^2

--
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 0420] Set BuildRequires to pylint 1.4

[Petr Vobornik]

On 02/24/2016 09:50 AM, Lukas Slebodnik wrote:

On (23/02/16 14:23), Rob Crittenden wrote:

Lukas Slebodnik wrote:

On (23/02/16 17:09), Martin Basti wrote:

We cannot guarantee that versions older than 1.4 will work with freeipa code.

Patch attached.


>From a59e72a0b87231c0f2e0d737057550dd532feed7 Mon Sep 17 00:00:00 2001

From: Martin Basti 
Date: Tue, 23 Feb 2016 16:58:07 +0100
Subject: [PATCH] Set BuildRequires to pylint >= 1.4

We can guarantee that only pylint 1.4 and newer will work

https://fedorahosted.org/freeipa/ticket/5615
---
freeipa.spec.in | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/freeipa.spec.in b/freeipa.spec.in
index 
54a11bfc8cced643c19c29c5ada70bacf7540395..219c5ca2f13eaac14746ec4689ba611bbc6fc377
 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -76,7 +76,7 @@ BuildRequires:  python-netaddr
BuildRequires:  python-gssapi >= 1.1.2
BuildRequires:  python-rhsm
BuildRequires:  pyOpenSSL
-BuildRequires:  pylint >= 1.0
+BuildRequires:  pylint >= 1.4


I can build rpms even withour pylint and pylint is not executed
anywhere in spec file. (in other words, my patch was rejected)
Why does it need to be in BuildRequires?


pylint is part of the in-tree build process (make rpms). It is not
executed when building upstream packages.


It's not buildrequires becuase I can rebuild src.rpm
without it. It should not be there or it should be optional
to do not break developer workflow.

e.g. "%bcond_with extra_dependencies_for_pylint"

The upstream spec files is close to the fedora spec file
and pylint is istalled there even though it's not used.

Another use case is coverity scan.

LS



Basically I agree with Lukas but this patch is the way how we do build 
upstream right now.


The proposed change would required more refactoring of build process and 
is out of scope of this ticket. Feel free to open a ticket for it. It 
should not block this patch.

--
Petr Vobornik

--
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 546] client: stop using /etc/pki/nssdb

[Jan Cholasta]

On 24.2.2016 10:50, David Kupka wrote:

On 22/02/16 16:06, Jan Cholasta wrote:

Hi,

the attached patch fixes .

Honza




Works for me, ACK.


Thanks.

Pushed to:
master: 11592dde1b232a70f318e01f5271b38890090648
ipa-4-3: a3e8af3b4aae3dd59985b1065f530587015ae05c

--
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] [PATCH 546] client: stop using /etc/pki/nssdb

[David Kupka]

On 22/02/16 16:06, Jan Cholasta wrote:

Hi,

the attached patch fixes .

Honza




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] [PATCH 0413] fix permission: Read Replication Agreements

[Jan Cholasta]

On 23.2.2016 17:20, Martin Basti wrote:



On 22.02.2016 09:00, Jan Cholasta wrote:

Hi,

On 17.2.2016 14:49, Martin Basti wrote:

https://fedorahosted.org/freeipa/ticket/5631

Patch attached (for master, 4.3, 4.2)


1) All the replication agreement permission ACIs should be located in
the same entry. Currently "Read Replication Agreements" is in
"cn=config" and everything else in "cn=mapping tree,cn=config", so I
guess "cn=mapping tree,cn=config" makes more sense.


2) Instead of literal DN('cn=permissions,cn=pbac'), use
api.env.container_permissions.


3) IMO the removal of managed permission attributes could be a little
bit more robust. You should check that the original entry contains all
the required values before touching it (objectclass=ipapermissionv2,
ipapermissiontype=V2, ipapermissiontype=MANAGED) and remove only the
values that need to be removed, instead of just overwriting everything.


Honza


Updated patch attached.


The patch does not apply on ipa-4-2.

Also this bit in replica-acis.ldif is redundant:

+
+dn: cn=mapping tree,cn=config
+changetype: modify
+add: aci

Honza

--
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] [PATCH 0084-0086] CI: Add double circle topology

[Martin Basti]

On 24.02.2016 10:07, David Kupka wrote:

On 24/02/16 08:27, David Kupka wrote:

On 23/02/16 17:54, Martin Basti wrote:



On 23.02.2016 17:33, Martin Basti wrote:



On 23.02.2016 17:30, Martin Basti wrote:



On 18.02.2016 10:14, David Kupka wrote:

On 12/02/16 16:52, Martin Basti wrote:



On 12.02.2016 13:03, Milan Kubík wrote:

On 02/12/2016 10:59 AM, David Kupka wrote:

Sending one more topology test. This one creates a M groups
consisting N (N>=2) servers.
First two servers in each group are used to connect with nearest
four
groups and also with the other servers inside the group (when 
N>2).

Servers inside the group (not connecting to other groups) are
connected with each other.

The patch set needs freeipa-dkupka-8{1,2,3} applied.


ACK


I cannot apply patches, please rebase

[mbasti@dhcp129-96 freeipa-devel]$ git checkout master
Switched to branch 'master'
Your branch is up-to-date with 'origin/master'.
[mbasti@dhcp129-96 freeipa-devel]$ git am freeipa-dkupka-008* -3
Applying: CI: Add '2-connected' topology generator.
Applying: CI: Add simple replication test in 2-connected topology.
Applying: CI: Add test for 2-connected topology generator.
Applying: CI: Add double circle topology.
Applying: CI: Add replication test utilizing double-circle 
topology.

Applying: CI: Add test for double-circle topology generator.
error: invalid object 100644 
e12d141391840cc7f9150a178875393a96dd469b

for 'ipatests/test_integration/test_topologies.py'
fatal: git-write-tree: error building trees
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0006 CI: Add test for double-circle topology
generator.
The copy of the patch that failed is found in:
/home/mbasti/work/freeipa-devel/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am
--abort".


Martin^2


Git fails to apply patches because wrong version of
freeipa-dkupka-008{1,2,3} was pushed. Attached patches should fix 
it.



Sorry my bad.

ACK

Pushed to:
ipa-4-3: ffe3731ae7813fcc3246a83e37d62fc2754bb4ca
master: acdabba6ec0f68841f02c1e6ad65232de81bb505


New test:

Pushed to:
master: a1e582b33c42bcc8a708777afb975e7dc571ee3d
ipa-4-3: 2efa60637111e40a9ac9459d507d9f55a2ae301a


Revert?
IMO this will not work on python3

* Module ipatests.test_integration.test_topologies
ipatests/test_integration/test_topologies.py:124:
[W1638(range-builtin-not-iterating), test_topology_double_circle_topo]
range built-in referenced when not iterating)
* Module ipatests.test_integration.tasks
ipatests/test_integration/tasks.py:949: [W0110(deprecated-lambda),
double_circle_topo] map/filter on lambda could be replaced by
comprehension)
ipatests/test_integration/tasks.py:949:
[W1636(map-builtin-not-iterating), double_circle_topo] map built-in
referenced when not iterating)
ipatests/test_integration/tasks.py:949:
[W1637(zip-builtin-not-iterating), double_circle_topo] zip built-in
referenced when not iterating)



You can revert it and I will send fixed patches or you can just apply
attached patch.



Updated patch attached.


ACK

Pushed to:
master: 775ee77bcc091ba31fdd3e59f8d45d0b646a44a0
ipa-4-3: 05539761cd6f7bd6ce40b687b913be81eabf69e1

--
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 0084-0086] CI: Add double circle topology

[David Kupka]

On 24/02/16 08:27, David Kupka wrote:

On 23/02/16 17:54, Martin Basti wrote:



On 23.02.2016 17:33, Martin Basti wrote:



On 23.02.2016 17:30, Martin Basti wrote:



On 18.02.2016 10:14, David Kupka wrote:

On 12/02/16 16:52, Martin Basti wrote:



On 12.02.2016 13:03, Milan Kubík wrote:

On 02/12/2016 10:59 AM, David Kupka wrote:

Sending one more topology test. This one creates a M groups
consisting N (N>=2) servers.
First two servers in each group are used to connect with nearest
four
groups and also with the other servers inside the group (when N>2).
Servers inside the group (not connecting to other groups) are
connected with each other.

The patch set needs freeipa-dkupka-8{1,2,3} applied.


ACK


I cannot apply patches, please rebase

[mbasti@dhcp129-96 freeipa-devel]$ git checkout master
Switched to branch 'master'
Your branch is up-to-date with 'origin/master'.
[mbasti@dhcp129-96 freeipa-devel]$ git am freeipa-dkupka-008* -3
Applying: CI: Add '2-connected' topology generator.
Applying: CI: Add simple replication test in 2-connected topology.
Applying: CI: Add test for 2-connected topology generator.
Applying: CI: Add double circle topology.
Applying: CI: Add replication test utilizing double-circle topology.
Applying: CI: Add test for double-circle topology generator.
error: invalid object 100644 e12d141391840cc7f9150a178875393a96dd469b
for 'ipatests/test_integration/test_topologies.py'
fatal: git-write-tree: error building trees
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0006 CI: Add test for double-circle topology
generator.
The copy of the patch that failed is found in:
/home/mbasti/work/freeipa-devel/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am
--abort".


Martin^2


Git fails to apply patches because wrong version of
freeipa-dkupka-008{1,2,3} was pushed. Attached patches should fix it.


Sorry my bad.

ACK

Pushed to:
ipa-4-3: ffe3731ae7813fcc3246a83e37d62fc2754bb4ca
master: acdabba6ec0f68841f02c1e6ad65232de81bb505


New test:

Pushed to:
master: a1e582b33c42bcc8a708777afb975e7dc571ee3d
ipa-4-3: 2efa60637111e40a9ac9459d507d9f55a2ae301a


Revert?
IMO this will not work on python3

* Module ipatests.test_integration.test_topologies
ipatests/test_integration/test_topologies.py:124:
[W1638(range-builtin-not-iterating), test_topology_double_circle_topo]
range built-in referenced when not iterating)
* Module ipatests.test_integration.tasks
ipatests/test_integration/tasks.py:949: [W0110(deprecated-lambda),
double_circle_topo] map/filter on lambda could be replaced by
comprehension)
ipatests/test_integration/tasks.py:949:
[W1636(map-builtin-not-iterating), double_circle_topo] map built-in
referenced when not iterating)
ipatests/test_integration/tasks.py:949:
[W1637(zip-builtin-not-iterating), double_circle_topo] zip built-in
referenced when not iterating)



You can revert it and I will send fixed patches or you can just apply
attached patch.



Updated patch attached.

--
David Kupka
From 1e12eb7d207e5980fb3653d5818968b129f4a1c9 Mon Sep 17 00:00:00 2001
From: David Kupka 
Date: Wed, 24 Feb 2016 08:15:51 +0100
Subject: [PATCH] CI: Make double circle topology python3 compatible

---
 ipatests/test_integration/tasks.py   | 2 +-
 ipatests/test_integration/test_topologies.py | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/ipatests/test_integration/tasks.py b/ipatests/test_integration/tasks.py
index 9d9a78bb10b463016c646aa921dde722e882da93..60e9e82391077ce6b997d0ed4ad4f2ff19f43d1e 100644
--- a/ipatests/test_integration/tasks.py
+++ b/ipatests/test_integration/tasks.py
@@ -946,7 +946,7 @@ def double_circle_topo(master, replicas, site_size=6):
 
 # split servers into sites
 it = [iter(servers)] * site_size
-sites = map(lambda x: (x[0], x[1], x[2:]), zip(*it))
+sites = [(x[0], x[1], x[2:]) for x in zip(*it)]
 num_sites = len(sites)
 
 for i in range(num_sites):
diff --git a/ipatests/test_integration/test_topologies.py b/ipatests/test_integration/test_topologies.py
index a0a1b9d6222779a9ce67b2fa8a29052747eae8f9..4618b44fe3b6cd69149a5fb55b2b392c5383a5c2 100644
--- a/ipatests/test_integration/test_topologies.py
+++ b/ipatests/test_integration/test_topologies.py
@@ -121,7 +121,7 @@ def test_topology_two_connected():
 def test_topology_double_circle_topo():
 topo = tasks.get_topo('double-circle')
 assert topo == tasks.double_circle_topo
-assert list(topo('M', range(1, 30))) == [
+assert list(topo('M', list(range(1, 30 == [
 ('M', 1),
 (1, 6),
 (1, 12),
-- 
2.5.0

-- 
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] Locations design v2: LDAP schema & user interface

[Martin Kosek]

On 02/23/2016 06:59 PM, Petr Spacek wrote:
> On 23.2.2016 18:14, Simo Sorce wrote:
...
>> More seriously I think it is a great idea, but too premature to get all
>> the way there now. We need to build schema and CLI that will allow us to
>> get there without having to completely change interfaces if at all
>> possible or minimizing any disruption in the tools.
> 
> Actually the backwards compatibility is the main worry which led to this idea
> with links.
> 
> If we release first version of locations with custom priorities etc. we will
> have support the schema (which will be different) and API (which will be later
> unnecessary) forever.
> 
> If we skip this intermediate phase with hand-made configuration we can save
> all the headache with upgrades to more automatic solution later on.
> 
> 
> Maybe we should invert the order:
> Start with locations + links with administrative metric and add hand-tweaking
> capabilities later (if necessary).
> 
> IMHO locations + links with administrative metric will be easier to implement
> than the first version.
> 
> Just thinking aloud ...

Makes sense to me, I would have the same worry as Petr, that we would break
something if we decide moving to links based solution later.

-- 
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 0420] Set BuildRequires to pylint 1.4

[Lukas Slebodnik]

On (23/02/16 14:23), Rob Crittenden wrote:
>Lukas Slebodnik wrote:
>> On (23/02/16 17:09), Martin Basti wrote:
>>> We cannot guarantee that versions older than 1.4 will work with freeipa 
>>> code.
>>>
>>> Patch attached.
>> 
>>>From a59e72a0b87231c0f2e0d737057550dd532feed7 Mon Sep 17 00:00:00 2001
>>> From: Martin Basti 
>>> Date: Tue, 23 Feb 2016 16:58:07 +0100
>>> Subject: [PATCH] Set BuildRequires to pylint >= 1.4
>>>
>>> We can guarantee that only pylint 1.4 and newer will work
>>>
>>> https://fedorahosted.org/freeipa/ticket/5615
>>> ---
>>> freeipa.spec.in | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/freeipa.spec.in b/freeipa.spec.in
>>> index 
>>> 54a11bfc8cced643c19c29c5ada70bacf7540395..219c5ca2f13eaac14746ec4689ba611bbc6fc377
>>>  100644
>>> --- a/freeipa.spec.in
>>> +++ b/freeipa.spec.in
>>> @@ -76,7 +76,7 @@ BuildRequires:  python-netaddr
>>> BuildRequires:  python-gssapi >= 1.1.2
>>> BuildRequires:  python-rhsm
>>> BuildRequires:  pyOpenSSL
>>> -BuildRequires:  pylint >= 1.0
>>> +BuildRequires:  pylint >= 1.4
>> 
>> I can build rpms even withour pylint and pylint is not executed
>> anywhere in spec file. (in other words, my patch was rejected)
>> Why does it need to be in BuildRequires?
>
>pylint is part of the in-tree build process (make rpms). It is not
>executed when building upstream packages.
>
It's not buildrequires becuase I can rebuild src.rpm
without it. It should not be there or it should be optional
to do not break developer workflow.

e.g. "%bcond_with extra_dependencies_for_pylint"

The upstream spec files is close to the fedora spec file
and pylint is istalled there even though it's not used.

Another use case is coverity scan.

LS

-- 
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 547] cacert install: fix trust chain validation

[Jan Cholasta]

On 24.2.2016 09:13, Martin Babinsky wrote:

On 02/22/2016 06:30 PM, Jan Cholasta wrote:

Hi,

the attached patch fixes .

Honza




ACK.


Thanks.

Pushed to:
master: ef9134640795b736731bfbdb6fe0badb3e817552
ipa-4-3: 4fa8d3bca44b02b81783673dd14954b94ed49efa

--
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] [PATCH 0022-23] Coverity patches

[Jan Cholasta]

On 24.2.2016 08:46, Stanislav Laznicka wrote:

Reworded the commit messages so that they mention Coverity.

On 02/22/2016 07:18 AM, Jan Cholasta wrote:

On 2.2.2016 13:36, Stanislav Laznicka wrote:

On 02/01/2016 02:24 PM, Jan Cholasta wrote:

On 1.2.2016 12:11, Petr Spacek wrote:

On 1.2.2016 09:03, Jan Cholasta wrote:

Hi,

On 29.1.2016 15:49, Martin Basti wrote:



On 29.01.2016 15:49, Stanislav Laznicka wrote:

Reworded the commits so that they better reflect what's going on
in those.

On 01/29/2016 02:49 PM, Stanislav Laznicka wrote:

Hello,

I made some patches based on the Coverity report from 18.1.2016.

Cheers,
Standa




NACK, see my previous email


I don't think this deserves 9 patches, 1 would be sufficient enough.


I would rather have it is separate patches as these fixes are largely
not
related. It will make bisecting easier.


Most of the fixes are cosmetic, which makes them related, and the rest
are small isolated changes, so in reality it would hardly make
bisecting easier and only increase the overhead. In the past we
usually had put such fixes into a single patch and AFAIK nobody
complained so far.


Squeezed the changes into two new patches, then. One for the very
cosmetic changes, one for possible bugs.


OK.





Patch 0013:

1) I think this unreachable return is intentional, as indicated by
the comment:

-#we shouldn't get here
-return [UNKNOWN_ERROR]


I would use
assert False, "we shouldn't get here"
neither we nor Coverity are confused when we hit the code path one
day.

UNKNOWN_ERROR would pop up somewhere else and it will be harder to
find out
why the hell the code behaves as mad. Traceback will clearly indicate
that
there is a problem with the 'switch'.


Sure, my point is that returning None is no better than returning
UNKNOWN_ERROR.


Added assert as suggested. There should still be no way of getting to
it, though.


Petr^2 Spacek


2) How is this dead code?

-if options.mode == 'validate_pot' or options.mode ==
'validate_po':
+if options.mode in ('validate_pot', 'validate_po'):

-elif options.mode == 'create_test' or 'test_gettext':
+elif options.mode in ('create_test', 'test_gettext'):



Patch 0014-0015: LGTM


Actually scratch that, patch 0015 breaks correct code.


The dead code appears in the 'else' branch as the latter of these two
conditions always evaluates to True. The first condition change is just
a cosmetic one so that both of the conditions look the same.


OK.



Also removed the changes made in patch 0015.



Patch 0016: The original code is in fact correct.


Point taken, removed the change.


Patch 0017: This will break Python 3. The two branches are
performing the same
action, but with different data types.


This might undergo further investigation in the future as there is no
way how "bytes" instance could become an argument of this function (as
suggested). Not even the newest Python 3 patches from pviktori mention
this code.


OK. (This is not what Coverity was complaining about, though.)



Patch 0018: LGTM


Patch 0019: IMO the original code is better (None has a __class__
too, you know).


Made it more "Coverity friendly" yet nice enough modification.


Patch 0020: LGTM


It seems that there actually is a check that checks whether the input is
correct. It is called ad-hoc but that might be the test feature.
Therefore just added an assert so that Coverity does not complain.


OK.



Patch 0021: Please use the original error messages (there are no
requests
being added to D-Bus, but to certmonger).


Honza





Added error messages that reflect the situation better, then.


Could you please mention Coverity in the commit messages, so that it's
clear why are we doing these changes? Otherwise LGTM.



Thanks, ACK.

Pushed to:
master: d7efd8a33ab14a561d3af445e62bceb6f2f13fd1
ipa-4-3: d78a759569020eba08b90e35707e86778523ec58

--
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] [PATCH 547] cacert install: fix trust chain validation

[Martin Babinsky]

On 02/22/2016 06:30 PM, Jan Cholasta wrote:

Hi,

the attached patch fixes .

Honza




ACK.

--
Martin^3 Babinsky

--
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