Re: [Freeipa-devel] [PATCH 0113] properly add ACIs to custodia container during IPA upgrade

[David Kupka]

On 10/12/15 10:14, Martin Babinsky wrote:

On 12/08/2015 10:45 AM, Martin Babinsky wrote:

fixes https://fedorahosted.org/freeipa/ticket/5524





Attaching updated patch with simpler fix suggested by Jan.




Thanks for the patch. 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] 0001 Refactor test_user_plugin

[Aleš Mareček]

There is some issue with "test_rename_to_too_long_login" test. It fails but 
actually this is false positive because it is possible to create login upto 255 
characters. I don't know why test mentions 32 characters without any other 
modified setup.
NACK for now.
 - alich -


- Original Message -
> From: "Aleš Mareček" 
> To: "Filip Škola" 
> Cc: freeipa-devel@redhat.com, "Milan Kubík" 
> Sent: Thursday, December 10, 2015 4:11:47 PM
> Subject: Re: [Freeipa-devel] [PATCH] 0001 Refactor test_user_plugin
> 
> Ah, sorry, haven't realized there had been devel list attached.
> Ok, there is some problem with \r\n in the patch.
> Filip, please take a look at it...
> Thanks...
>  - alich -
> 
> - Original Message -
> > From: "Filip Škola" 
> > To: "Aleš Mareček" 
> > Cc: freeipa-devel@redhat.com, "Milan Kubík" 
> > Sent: Thursday, December 10, 2015 11:29:52 AM
> > Subject: Re: [Freeipa-devel] [PATCH] 0001 Refactor test_user_plugin
> > 
> > Hi,
> > 
> > this if fixed. Also issues with test_stageuser_plugin caused by
> > UserTracker changes should be fixed here.
> > 
> > Filip
> > 
> > 
> > On Mon, 7 Dec 2015 09:29:31 -0500 (EST)
> > Aleš Mareček  wrote:
> > 
> > > NACK.
> > > 
> > > $ ./make-lint
> > > * Module ipatests.test_xmlrpc.test_user_plugin
> > > ipatests/test_xmlrpc/test_user_plugin.py:42:
> > > [E0611(no-name-in-module), ] No name 'ldaptracker' in module
> > > 'ipatests.test_xmlrpc')
> > > 
> > > $ grep ldaptracker ipatests/test_xmlrpc/test_user_plugin.py
> > > from ipatests.test_xmlrpc.ldaptracker import Tracker
> > > $ ls ipatests/test_xmlrpc/ldaptracker*
> > > ls: cannot access ipatests/test_xmlrpc/ldaptracker*: No such file or
> > > directory
> > > 
> > > 
> > > - Original Message -
> > > > From: "Filip Škola" 
> > > > To: "Milan Kubík" 
> > > > Cc: freeipa-devel@redhat.com
> > > > Sent: Thursday, December 3, 2015 5:38:43 PM
> > > > Subject: Re: [Freeipa-devel] [PATCH] 0001 Refactor test_user_plugin
> > > > 
> > > > Hi,
> > > > 
> > > > sending corrected version.
> > > > 
> > > > F.
> > > > 
> > > > On Thu, 12 Nov 2015 14:03:19 +0100
> > > > Milan Kubík  wrote:
> > > > 
> > > > > On 11/10/2015 12:13 PM, Filip Škola wrote:
> > > > > > Hi,
> > > > > >
> > > > > > fixed.
> > > > > >
> > > > > > F.
> > > > > >
> > > > > > On Tue, 10 Nov 2015 10:52:45 +0100
> > > > > > Milan Kubík  wrote:
> > > > > >
> > > > > >> On 11/09/2015 04:35 PM, Filip Škola wrote:
> > > > > >>> Another patch was applied in the meantime.
> > > > > >>>
> > > > > >>> Attaching an updated version.
> > > > > >>>
> > > > > >>> F.
> > > > > >>>
> > > > > >>> On Mon, 9 Nov 2015 13:35:02 +0100
> > > > > >>> Milan Kubík  wrote:
> > > > > >>>
> > > > >  On 11/06/2015 11:32 AM, Filip Škola wrote:
> > > > >  Hi,
> > > > >  the patch doesn't apply.
> > > > > 
> > > > > >> Please fix this.
> > > > > >>
> > > > > >>   ipatests/test_xmlrpc/test_user_plugin.py:1419:
> > > > > >> [E0602(undefined-variable),
> > > > > >> TestDeniedBindWithExpiredPrincipal.teardown_class] Undefined
> > > > > >> variable 'user1')
> > > > > >>
> > > > > >> Also, use the version numbers for your changed patches.
> > > > > >>
> > > > > >
> > > > > >
> > > > > Thanks for the patch. Several issues:
> > > > > 
> > > > > 1. Use dict.items instead of dict.iteritems, for python3
> > > > > compatibility
> > > > > 
> > > > > 2. What is the purpose of TestPrepare class? The 'purge' methods
> > > > > do not call any ipa commands.
> > > > > Tracker.make_fixture should be used to make the Tracked resources
> > > > > clean themselves up when they're out of scope.
> > > > > 
> > > > > 3. Why reference the resources by hardcoded name if they have a
> > > > > fixture representation?
> > > > > 
> > > > > 4. Rewrite {create,delete}_test_group to a fixture. You may want
> > > > > to use different scope (or not).
> > > > > 
> > > > > 5. In `def atest_rename_to_invalid_login(self, user):` - use
> > > > > pytest.skipif decorator and provide a reason if you must,
> > > > > do not obfuscate method name in order not to run it.
> > > > > 
> > > > > 
> > > > 
> > > > 
> > > > --
> > > > 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
> > 
> > 
> 

-- 
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 0113] properly add ACIs to custodia container during IPA upgrade

[Tomas Babej]

On 12/11/2015 10:37 AM, David Kupka wrote:
> On 10/12/15 10:14, Martin Babinsky wrote:
>> On 12/08/2015 10:45 AM, Martin Babinsky wrote:
>>> fixes https://fedorahosted.org/freeipa/ticket/5524
>>>
>>>
>>>
>>
>> Attaching updated patch with simpler fix suggested by Jan.
>>
>>
>>
> Thanks for the patch. Works for me, ACK.
> 

I was also finally able to reproduce it on a clear machine.

Pushed to master: a66a2c5160dbc23cdeec55d17422812028939e16

-- 
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 0113] properly add ACIs to custodia container during IPA upgrade

[Tomas Babej]

On 12/11/2015 12:50 PM, Tomas Babej wrote:
> 
> 
> On 12/11/2015 10:37 AM, David Kupka wrote:
>> On 10/12/15 10:14, Martin Babinsky wrote:
>>> On 12/08/2015 10:45 AM, Martin Babinsky wrote:
 fixes https://fedorahosted.org/freeipa/ticket/5524



>>>
>>> Attaching updated patch with simpler fix suggested by Jan.
>>>
>>>
>>>
>> Thanks for the patch. Works for me, ACK.
>>
> 
> I was also finally able to reproduce it on a clear machine.
> 
> Pushed to master: a66a2c5160dbc23cdeec55d17422812028939e16
> 

Martin actually pushed this 30 minutes ago, actual commit hash is
e130d35687a05cb3d2dd8708b76e7745e337c0c0.

-- 
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 0058, 0064] dns: do not add (forward)zone if it is already resolvable.

[David Kupka]

On 10/12/15 18:10, Petr Spacek wrote:

On 10.12.2015 17:31, David Kupka wrote:

On 09/12/15 18:55, Petr Spacek wrote:

On 9.12.2015 13:37, David Kupka wrote:

On 08/12/15 15:24, Petr Spacek wrote:

On 8.12.2015 12:19, David Kupka wrote:

On 08/12/15 08:56, Petr Spacek wrote:

On 7.12.2015 14:41, David Kupka wrote:

+def is_host_resolvable(fqdn):
+if not isinstance(fqdn, DNSName):
+fqdn = DNSName(fqdn)
+for rdtype in (rdatatype.A, rdatatype.):
+try:
+resolver.query(fqdn.make_absolute(), rdtype)
+except DNSException:
+continue
+else:
+return True
+
+return False



NACK, you are re-introducing duplicate function which was removed in
498471e4aed1367b72cd74d15811d0584a6ee268.

Please amend the patch ASAP to use new verify_host_resolvable() function
so I
can test it and get it into 4.3.


Updated patches attached.


Hmm, my review script screams:

Detected base branch:   origin/master
Checks will be made against following base commit: 848912a add missing
/ipaplatform/constants.py to .gitignore
Writing API to API.txt
./ipalib/plugins/dns.py:2127:9: E124 closing bracket does not match visual
indentation
./ipalib/plugins/dns.py:2711:13: E128 continuation line under-indented for
visual indent
./ipalib/plugins/dns.py:2713:9: E124 closing bracket does not match visual
indentation
./ipalib/plugins/dns.py:2716:13: E128 continuation line under-indented for
visual indent
./ipapython/ipautil.py:928:1: E302 expected 2 blank lines, found 1
./ipapython/ipautil.py:948:17: E128 continuation line under-indented for
visual indent
./ipapython/ipautil.py:956:1: E302 expected 2 blank lines, found 1
./ipapython/ipautil.py:997:80: E501 line too long (83 > 79 characters)
./ipapython/ipautil.py:998:80: E501 line too long (83 > 79 characters)
./ipaserver/install/bindinstance.py:49:9: E128 continuation line
under-indented for visual indent
./ipaserver/install/bindinstance.py:292:80: E501 line too long (80 > 79
characters)
./ipaserver/install/bindinstance.py:438:19: E128 continuation line
under-indented for visual indent
./ipaserver/install/server/common.py:271:63: E261 at least two spaces before
inline comment
./ipaserver/install/server/common.py:271:80: E501 line too long (90 > 79
characters)
ERROR: PEP8 --diff failed, continuing ...
ERROR: API.txt was changed without a change in VERSION, continuing ...
Summary of detected errors:
ERROR: PEP8 --diff failed
ERROR: API.txt was changed without a change in VERSION

Please fix it :-)

Make make this more pleasant for you, you can use (and review) my attached
patch. It adds 'review' target to Makefile, it will do the same checks as
I do.



Unfortunately your tool does not work for me (output w/ -o xtrace attached).
Anyway I have incremented API version and fixed most of the pep8 errors except
for E124 twice in ipalib/plugins/dns.py as it seems to be convention in the
file and E501 twice in ipapython/ipautil.py because IMO breaking string
constant into multiple lines does not help readability.

Updated patches also attached.


We are almost there, but NACK for now.

1) Following attempt to install IPA explodes. Please note that
dom-245.idm.lab.eng.brq.redhat.com DNS domain is delegated to the IPA server
before installation is started, so it gives 'timeout' or possibly SERVFAIL.

# ipa-server-install --ds-password=root4lab --admin-password=root4lab
--setup-dns --forwarder=10.38.5.26 --no-reverse --allow-zone-overlap
--domain=dom-245.idm.lab.eng.brq.redhat.com
--realm=DOM-245.IDM.LAB.ENG.BRQ.REDHAT.COM
[...]

Configuring DNS (named)
[1/11]: generating rndc key file
[2/11]: adding DNS container
[3/11]: setting up our zone
[error] InvocationError: DNS check for domain
dom-245.idm.lab.eng.brq.redhat.com. failed: The DNS operation timed out after
30.000453949 seconds. Please make sure that the domain is properly delegated
to this IPA server.
ipa.ipapython.install.cli.install_tool(Server): ERRORDNS check for domain
dom-245.idm.lab.eng.brq.redhat.com. failed: The DNS operation timed out after
30.000453949 seconds. Please make sure that the domain is properly delegated
to this IPA server.
ipa.ipapython.install.cli.install_tool(Server): ERRORThe
ipa-server-install command failed. See /var/log/ipaserver-install.log for more
information

2015-12-09T17:15:37Z DEBUG   File
"/usr/lib/python2.7/site-packages/ipapython/admintool.py", line 171, in execute
  return_value = self.run()
File "/usr/lib/python2.7/site-packages/ipapython/install/cli.py", line 318,
in run
  cfgr.run()
File "/usr/lib/python2.7/site-packages/ipapython/install/core.py", line 310,
in run
  self.execute()
File "/usr/lib/python2.7/site-packages/ipapython/install/core.py", line 332,
in execute
  for nothing in self._executor():
File "/usr/lib/python2.7/site-packages/ipapython/install/core.py", line 372,
in __runner
  self._handle_exception(exc_info)
File 

Re: [Freeipa-devel] [PATCH 0373] Upgrade: Fix IPA version comparison

[Martin Basti]

On 11.12.2015 09:36, Martin Kosek wrote:

On 12/10/2015 05:09 PM, Martin Basti wrote:



On 10.12.2015 15:49, Tomas Babej wrote:


On 12/10/2015 11:23 AM, Martin Basti wrote:


On 10.12.2015 09:13, Lukas Slebodnik wrote:

On (09/12/15 19:22), Martin Basti wrote:

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

Patch attached.
>From 8ef93485d61e8732166fb0c5b6c4559209740f3e Mon Sep 17 00:00:00 
2001

From: Martin Basti 
Date: Wed, 9 Dec 2015 18:53:35 +0100
Subject: [PATCH] Fix version comparison

Use RPM library to compare vendor versions of IPA for redhat 
platform


https://fedorahosted.org/freeipa/ticket/5535
---
freeipa.spec.in |  2 ++
ipaplatform/redhat/tasks.py | 19 +++
2 files changed, 21 insertions(+)

diff --git a/freeipa.spec.in b/freeipa.spec.in
index
9f82b3695fb10c4db65cc31278364b3b34e26098..09feba7b8324f5e645da3e8010de86b6c3ee5ab9 



100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -166,6 +166,8 @@ Requires: %{etc_systemd_dir}
Requires: gzip
Requires: python-gssapi >= 1.1.0
Requires: custodia
+Requires: rpm-python
+Requires: rpmdevtools

Could you explain why do you need the 2nd package?
It does not contains any python modules
and I cannot see usage of any binary in this patch

LS

Thanks for this catch, it is actually located in yum package, I rather
copy stringToVersion function from there to IPA, to avoid 
dependency hell


Updated patch attached.



Looking good. The __cmp__ function, however, is not available in Python
3. As we will eventually support python3 in RHEL as well, maybe we
should make sure even platform-dependent parts are python3 compatible?
For the future's sake.

Tomas


Thanks,

python 3 compatible patch attached.


I wonder why we have to add so much code here and reimplement RPM 
version checking if it is already provided by rpmdevtools:


~~~
$ /usr/bin/rpmdev-vercmp ipa-4.2.0-15.el7 4.2.0-15.el7_2.3; echo $?
WARNING: hyphen in release1: 4.2.0-15.el7

rpmdev-vercmp  
rpmdev-vercmp  
rpmdev-vercmp # with no arguments, prompt

Exit status is 0 if the EVR's are equal, 11 if EVR1 is newer, and 12 
if EVR2

is newer.  Other exit statuses indicate problems.

ipa-4.2.0-15.el7 < 4.2.0-15.el7_2.3
12
~~~

which could be directly called from __eq__ or __lt__, since we are in 
platform specific code anyway already.


Martin
I do not like the idea of calling external tool, but I have no other 
objections except my personal feeling.


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 0395] replicainstall: Make sure the enrollment state is preserved

[Tomas Babej]

On 12/10/2015 02:22 PM, Tomas Babej wrote:
> 
> 
> On 12/10/2015 02:18 PM, Tomas Babej wrote:
>> Hi,
>>
>> During the promote_check phase, the subsequent checks after the machine
>> is enrolled may cause the installation to abort, hence leaving it
>> enrolled even though it might not have been prior to the execution of
>> the ipa-replica-install command.
>>
>> Make sure that ipa-client-install --uninstall is called on the machine
>> that has not been enrolled before in case of failure during the
>> promote_check phase.
>>
>> https://fedorahosted.org/freeipa/ticket/5529
>>
> 
> Self-NACK, installer is redundant for uninstall_client.
> 
> Updated patch attached.
> 
> Tomas
> 
> 
> 

Attaching rebased version.
From faaf1da99a8b04ff546d8df38e1c7e65177b35d7 Mon Sep 17 00:00:00 2001
From: Tomas Babej 
Date: Thu, 10 Dec 2015 14:10:18 +0100
Subject: [PATCH] replicainstall: Make sure the enrollment state is preserved

During the promote_check phase, the subsequent checks after the machine
is enrolled may cause the installation to abort, hence leaving it
enrolled even though it might not have been prior to the execution of
the ipa-replica-install command.

Make sure that ipa-client-install --uninstall is called on the machine
that has not been enrolled before in case of failure during the
promote_check phase.

https://fedorahosted.org/freeipa/ticket/5529
---
 ipaserver/install/server/replicainstall.py | 32 ++
 1 file changed, 32 insertions(+)

diff --git a/ipaserver/install/server/replicainstall.py b/ipaserver/install/server/replicainstall.py
index 97f19c7fd4b156a3a3a9fd4b87d6ce297ad2983c..6733caad8b3b159cd3c354f6933dcce797fb94cf 100644
--- a/ipaserver/install/server/replicainstall.py
+++ b/ipaserver/install/server/replicainstall.py
@@ -385,6 +385,34 @@ def common_cleanup(func):
 return decorated
 
 
+def preserve_enrollment_state(func):
+"""
+Makes sure the machine is unenrollled if the decorated function
+failed.
+"""
+def decorated(installer):
+try:
+func(installer)
+except BaseException:
+if installer._enrollment_performed:
+uninstall_client()
+raise
+
+return decorated
+
+
+def uninstall_client():
+"""
+Attempts to unenroll the IPA client using the ipa-client-install utility.
+
+An unsuccessful attempt to uninstall is ignored (no exception raised).
+"""
+
+print("Removing client side components")
+ipautil.run([paths.IPA_CLIENT_INSTALL, "--unattended", "--uninstall"],
+raiseonerr=False)
+
+
 def promote_sssd(host_name):
 sssdconfig = SSSDConfig.SSSDConfig()
 sssdconfig.import_config()
@@ -790,6 +818,8 @@ def ensure_enrolled(installer):
 # Call client install script
 service.print_msg("Configuring client side components")
 try:
+installer._enrollment_performed = True
+
 args = [paths.IPA_CLIENT_INSTALL, "--unattended"]
 stdin = None
 
@@ -831,9 +861,11 @@ def ensure_enrolled(installer):
  "ipa-client-install returned: " + str(e))
 
 @common_cleanup
+@preserve_enrollment_state
 def promote_check(installer):
 options = installer
 
+installer._enrollment_performed = False
 installer._top_dir = tempfile.mkdtemp("ipa")
 
 tasks.check_selinux_status()
-- 
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 0388] tests: Add hostmask detection for sudo rules validating

[Lukas Slebodnik]

On (10/12/15 11:40), Tomas Babej wrote:
>On 12/10/2015 09:05 AM, Lukas Slebodnik wrote:
>> On (08/12/15 14:47), Tomas Babej wrote:
>>>
>>>
>>> On 12/03/2015 04:33 PM, Tomas Babej wrote:


 On 12/03/2015 04:26 PM, Aleš Mareček wrote:
> Hello,
>
> ACK for code
> NACK for the placing "get_client_ip_with_hostmask" function to 
> test_sudo.py (this function should be in some more general file)
>

 What place would you propose? The task.py is not a good place, as this
 is not really a task.

 Nevertheless, I'd rather have it moved when an use case outside
 test_sudo.py actually arises. Right now it would lead to unnecessary
 cluttering.

 Tomas

>>>
>>> I re-discovered ipatests.test_integration.util (two years after I
>>> created it :D) - which seemed ideal for this function.
>>>
>>> Updated patch attached.
>>>
>>> Tomas
>> 
>>>From 33552d6078d75ee99f9ec19ae143df5a61ba84a4 Mon Sep 17 00:00:00 2001
>>> From: Tomas Babej 
>>> Date: Wed, 2 Dec 2015 15:25:49 +0100
>>> Subject: [PATCH] tests: Add hostmask detection for sudo rules validating on
>>> hostmask
>>>
>>> IPA sudo tests worked under the assumption that the clients
>>> that are executing the sudo commands have their IPs assigned
>>> within 255.255.255.0 hostmask.
>>>
>>> Removes this (invalid) assumption and adds a
>>> dynamic detection of the hostmask of the IPA client.
>>>
>>> https://fedorahosted.org/freeipa/ticket/5501
>>> ---
>>> ipatests/test_integration/test_sudo.py | 33 
>>> +++--
>>> ipatests/test_integration/util.py  | 16 
>>> 2 files changed, 43 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/ipatests/test_integration/util.py 
>>> b/ipatests/test_integration/util.py
>>> index 
>>> 1a1bb3fcc923c9f2721f0a4c1cb7a1ba2ccc2dd8..187f39e80e84af0eb4938fb19ac3d3c7c2280ed9
>>>  100644
>>> --- a/ipatests/test_integration/util.py
>>> +++ b/ipatests/test_integration/util.py
>>> @@ -58,3 +58,19 @@ def run_repeatedly(host, command, assert_zero_rc=True, 
>>> test=None,
>>>  .format(cmd=' '.join(command),
>>>  times=timeout / time_step,
>>>  timeout=timeout))
>>> +
>>> +
>>> +def get_host_ip_with_hostmask(host):
>>> +"""
>>> +Detects the IP of the host including the hostmask.
>>> +
>>> +Returns None if the IP could not be detected.
>>> +"""
>>> +
>>> +ip = host.ip
>>> +result = host.run_command(['ip', 'addr'])
>>> +full_ip_regex = r'(?P%s/\d{1,2}) ' % re.escape(ip)
>>> +match = re.search(full_ip_regex, result.stdout_text)
>> ./make-lint 
>> * Module ipatests.test_integration.util
>> ipatests/test_integration/util.py:72: [E0602(undefined-variable), 
>> get_host_ip_with_hostmask] Undefined variable 're')
>> ipatests/test_integration/util.py:73: [E0602(undefined-variable), 
>> get_host_ip_with_hostmask] Undefined variable 're')
>> ===
>> Errors were found during the static code check.
>> If you are certain that any of the reported errors are false positives, 
>> please
>> mark them in the source code according to the pylint documentation.
>> ===
>> Makefile:124: recipe for target 'lint' failed
>> 
>> LS
>> 
>
>Nothing can break when moving a function, right? Missing import fixed.
>
>Tomas

>From c176ff1ab9ea1c56dc0c5d44bc490d925fad1497 Mon Sep 17 00:00:00 2001
>From: Tomas Babej 
>Date: Wed, 2 Dec 2015 15:25:49 +0100
>Subject: [PATCH] tests: Add hostmask detection for sudo rules validating on
> hostmask
>
>IPA sudo tests worked under the assumption that the clients
>that are executing the sudo commands have their IPs assigned
>within 255.255.255.0 hostmask.
>
>Removes this (invalid) assumption and adds a
>dynamic detection of the hostmask of the IPA client.
>
>https://fedorahosted.org/freeipa/ticket/5501
>---
> ipatests/test_integration/test_sudo.py | 32 ++--
> ipatests/test_integration/util.py  | 17 +
> 2 files changed, 43 insertions(+), 6 deletions(-)
>
Thank you very much.

ACK

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 0388] tests: Add hostmask detection for sudo rules validating

[Tomas Babej]

On 12/11/2015 02:01 PM, Lukas Slebodnik wrote:
> On (10/12/15 11:40), Tomas Babej wrote:
>> On 12/10/2015 09:05 AM, Lukas Slebodnik wrote:
>>> On (08/12/15 14:47), Tomas Babej wrote:


 On 12/03/2015 04:33 PM, Tomas Babej wrote:
>
>
> On 12/03/2015 04:26 PM, Aleš Mareček wrote:
>> Hello,
>>
>> ACK for code
>> NACK for the placing "get_client_ip_with_hostmask" function to 
>> test_sudo.py (this function should be in some more general file)
>>
>
> What place would you propose? The task.py is not a good place, as this
> is not really a task.
>
> Nevertheless, I'd rather have it moved when an use case outside
> test_sudo.py actually arises. Right now it would lead to unnecessary
> cluttering.
>
> Tomas
>

 I re-discovered ipatests.test_integration.util (two years after I
 created it :D) - which seemed ideal for this function.

 Updated patch attached.

 Tomas
>>>
>>> >From 33552d6078d75ee99f9ec19ae143df5a61ba84a4 Mon Sep 17 00:00:00 2001
 From: Tomas Babej 
 Date: Wed, 2 Dec 2015 15:25:49 +0100
 Subject: [PATCH] tests: Add hostmask detection for sudo rules validating on
 hostmask

 IPA sudo tests worked under the assumption that the clients
 that are executing the sudo commands have their IPs assigned
 within 255.255.255.0 hostmask.

 Removes this (invalid) assumption and adds a
 dynamic detection of the hostmask of the IPA client.

 https://fedorahosted.org/freeipa/ticket/5501
 ---
 ipatests/test_integration/test_sudo.py | 33 
 +++--
 ipatests/test_integration/util.py  | 16 
 2 files changed, 43 insertions(+), 6 deletions(-)

 diff --git a/ipatests/test_integration/util.py 
 b/ipatests/test_integration/util.py
 index 
 1a1bb3fcc923c9f2721f0a4c1cb7a1ba2ccc2dd8..187f39e80e84af0eb4938fb19ac3d3c7c2280ed9
  100644
 --- a/ipatests/test_integration/util.py
 +++ b/ipatests/test_integration/util.py
 @@ -58,3 +58,19 @@ def run_repeatedly(host, command, assert_zero_rc=True, 
 test=None,
  .format(cmd=' '.join(command),
  times=timeout / time_step,
  timeout=timeout))
 +
 +
 +def get_host_ip_with_hostmask(host):
 +"""
 +Detects the IP of the host including the hostmask.
 +
 +Returns None if the IP could not be detected.
 +"""
 +
 +ip = host.ip
 +result = host.run_command(['ip', 'addr'])
 +full_ip_regex = r'(?P%s/\d{1,2}) ' % re.escape(ip)
 +match = re.search(full_ip_regex, result.stdout_text)
>>> ./make-lint 
>>> * Module ipatests.test_integration.util
>>> ipatests/test_integration/util.py:72: [E0602(undefined-variable), 
>>> get_host_ip_with_hostmask] Undefined variable 're')
>>> ipatests/test_integration/util.py:73: [E0602(undefined-variable), 
>>> get_host_ip_with_hostmask] Undefined variable 're')
>>> ===
>>> Errors were found during the static code check.
>>> If you are certain that any of the reported errors are false positives, 
>>> please
>>> mark them in the source code according to the pylint documentation.
>>> ===
>>> Makefile:124: recipe for target 'lint' failed
>>>
>>> LS
>>>
>>
>> Nothing can break when moving a function, right? Missing import fixed.
>>
>> Tomas
> 
>>From c176ff1ab9ea1c56dc0c5d44bc490d925fad1497 Mon Sep 17 00:00:00 2001
>> From: Tomas Babej 
>> Date: Wed, 2 Dec 2015 15:25:49 +0100
>> Subject: [PATCH] tests: Add hostmask detection for sudo rules validating on
>> hostmask
>>
>> IPA sudo tests worked under the assumption that the clients
>> that are executing the sudo commands have their IPs assigned
>> within 255.255.255.0 hostmask.
>>
>> Removes this (invalid) assumption and adds a
>> dynamic detection of the hostmask of the IPA client.
>>
>> https://fedorahosted.org/freeipa/ticket/5501
>> ---
>> ipatests/test_integration/test_sudo.py | 32 ++--
>> ipatests/test_integration/util.py  | 17 +
>> 2 files changed, 43 insertions(+), 6 deletions(-)
>>
> Thank you very much.
> 
> ACK
> 
> LS
> 

Pushed to:
master: a02f83ff9c6a1920cedebee69dc6857c3521f161
ipa-4-2: f676b122848f7d843e200484ac06568d71f2f4ef

-- 
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 0377] CI: fix test_vault installation on domain level 1

[Oleg Fayans]

ACK

On 12/11/2015 02:45 PM, Martin Basti wrote:
> Patch attached.

-- 
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.

-- 
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 0377] CI: fix test_vault installation on domain level 1

[Martin Basti]

On 11.12.2015 15:08, Oleg Fayans wrote:

ACK

On 12/11/2015 02:45 PM, Martin Basti wrote:

Patch attached.

Pushed to master: 1e0f1f5197ecf8f20ae4cba29af2a2162096c4f6

--
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 0069] Add 'review' target for make

[Jan Cholasta]

Hi,

On 10.12.2015 18:04, Petr Spacek wrote:

On 9.12.2015 15:30, Petr Spacek wrote:

Hello,

this patch automates some of sanity checks proposed by Petr Vobornik.

'make review' should be used in root of clean Git tree which has patches under
review applied.

Magic in review.sh attempts to detect nearest remote branch which can be used
as diff base for review. Please see review.sh for further details.


And here is the patch! :-)


Nice, but I would rather see this in ipatool 
(). Or is there any obvious 
benefit in having this in freeipa itself that I'm missing?


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 0069] Add 'review' target for make

[Lukas Slebodnik]

On (11/12/15 09:46), Petr Spacek wrote:
>On 11.12.2015 09:22, Lukas Slebodnik wrote:
>> On (10/12/15 18:04), Petr Spacek wrote:
>>> On 9.12.2015 15:30, Petr Spacek wrote:
 Hello,

 this patch automates some of sanity checks proposed by Petr Vobornik.

 'make review' should be used in root of clean Git tree which has patches 
 under
 review applied.

>> +1 for automatisation
>> -1 for name.
>> 
>> Your script does not review[1] anything. Code review(peer review) is a job
>> for humans. What do you think about alternative
>> names: "review-helper", "sanity-check" ...
>
>I like short name but I respect your point, so I've updated the script to 
>print:
>"No problems detected using lint, pep8, ./make{api,aci}, and VERSION" at the 
>end.
>Now it should be clear what it did, so reviewer knows what steps were already
>done and what can be skipped during further manual review.
>
"review-helper", "sanity-check" are not long.

long would be
"aotomated-part-of-sanity-checks-from-FreeIPA-developers's-check-list"

And pdf document
https://pvoborni.fedorapeople.org/FreeIPAdeveloperschecklist.pdf
already mentioned "Sanity checks"

Please consider to not use confusing name.
Neither as make target nor as name of script.

BTW I cannot see sanity check for spec file.
If it will be added to FreeIPAdeveloperschecklist.pdf
than you might add it to the sript as well.

>Attached patch also adds 'assert-clean-tree' target which is run before lint
>target, so no time is wasted if the tree is not clean. Parallel build will
>execute and terminate both targets at the same time, but it saves time in both
>cases, the output is just not so nice.
>
>And IPA cannot be built in parallel anyway :-)
>


 Magic in review.sh attempts to detect nearest remote branch which can be 
 used
 as diff base for review. Please see review.sh for further details.
>>>
>>> And here is the patch! :-)
>>>
>>> -- 
>>> Petr^2 Spacek
>> 
>> [1] https://en.wikipedia.org/wiki/Code_review
>> 
>>>From 52393bf2bb0ad74dbe37e496b1fd41a6ab22bd90 Mon Sep 17 00:00:00 2001
>>> From: Petr Spacek 
>>> Date: Tue, 8 Dec 2015 12:06:33 +0100
>>> Subject: [PATCH] Add 'review' target for make. It automates following tasks:
>>>
>>> - check if ACI.txt and API.txt are up-to-date
>>> - check if VERSION was changed if API was changed
>>> - pep8 --diff does not produce new errors when ran on diff from 
>>> origin/branch
>>> - make lint does not produce errors
>>> ---
>>> Makefile  |  4 
>>> review.sh | 64 
>>> +++
>>> 2 files changed, 68 insertions(+)
>>> create mode 100755 review.sh
>>>
>>> diff --git a/Makefile b/Makefile
>>> index 
>>> d2c37f1597a011af2bd9ef1a4f7ce87ac59620a3..b85b3a5d6362150dcebf16745414c3d416c9547f
>>>  100644
>>> --- a/Makefile
>>> +++ b/Makefile
>>> @@ -79,6 +79,10 @@ check: bootstrap-autogen server tests
>>> (cd $$subdir && $(MAKE) check) || exit 1; \
>>> done
>>>
>>> +# works only in Git tree
>>> +review: version-update
>>> +   ./review.sh
>>> +
>>> bootstrap-autogen: version-update client-autogen
>>> @echo "Building IPA $(IPA_VERSION)"
>>> cd asn1; if [ ! -e Makefile ]; then ../autogen.sh --prefix=/usr 
>>> --sysconfdir=/etc --localstatedir=/var --libdir=$(LIBDIR); fi
>>> diff --git a/review.sh b/review.sh
>>> new file mode 100755
>>> index 
>>> ..3a5114fbe88d3aba8e926e049500f5ac5f41a83e
>>> --- /dev/null
>>> +++ b/review.sh
>>> @@ -0,0 +1,64 @@
>>> +#!/bin/bash
>>> +is_tree_clean() {
>>> +   test "$(git status --porcelain "$@")" == ""
>>> +   return $?
>>> +}
>>> +
>>> +log_error() {
>>> +   echo "ERROR: ${1}, continuing ..."
>>> +   errs=(${errs[@]} "ERROR: ${1}\n")
>>> +}
>>> +
>>> +set -o errexit -o nounset #-o xtrace
>>> +
>>> +LOGFILE="$(mktemp --suff=.log)"
>>> +exec > >(tee -i ${LOGFILE})
>>> +exec 2>&1
>>> +
>>> +echo -n "make lint is running ... "
>>> +make --silent lint || log_error "make lint failed"
>>> +
>>> +# Go backwards in history until you find a remote branch from which current
>>> +# branch was created. This will be used as base for git diff.
>>> +PATCHCNT=0
>>> +BASEBRANCH=""
>> If base branch means the remote branch which was used for cloning the cerrent
>> git HEAD than you can simplify it a littl bit.
>> git rev-parse --symbolic-full-name @{upstream}
>
>Nice! I did not know this trick. Unfortunately it does not work for my
>use-case: I clone upstream repo, pick a branch to base review on, create my
>own branch from the upstream one, and then apply patches on top of that.
>
>Imagine that I'm going to run the tool on branch 'review-tool'. Git history
>tree looks like this:
>
>* d036f3e (HEAD -> review-tool) fixup! Add 'review' target for make.
>* c6d75b0 Makefile: disable parallel build
>| * 667d50b (dkupka-review) dns: Add --auto-reverse option.
>| * c85f176 dns: Check if domain already exists.
>| * a55cd76 dns: do not add (forward)zone if it is 

Re: [Freeipa-devel] [PATCH 0058, 0064] dns: do not add (forward)zone if it is already resolvable.

[Petr Spacek]

On 11.12.2015 12:35, David Kupka wrote:
> On 10/12/15 18:10, Petr Spacek wrote:
>> On 10.12.2015 17:31, David Kupka wrote:
>>> On 09/12/15 18:55, Petr Spacek wrote:
 On 9.12.2015 13:37, David Kupka wrote:
> On 08/12/15 15:24, Petr Spacek wrote:
>> On 8.12.2015 12:19, David Kupka wrote:
>>> On 08/12/15 08:56, Petr Spacek wrote:
 On 7.12.2015 14:41, David Kupka wrote:
> +def is_host_resolvable(fqdn):
> +if not isinstance(fqdn, DNSName):
> +fqdn = DNSName(fqdn)
> +for rdtype in (rdatatype.A, rdatatype.):
> +try:
> +resolver.query(fqdn.make_absolute(), rdtype)
> +except DNSException:
> +continue
> +else:
> +return True
> +
> +return False
>

 NACK, you are re-introducing duplicate function which was removed in
 498471e4aed1367b72cd74d15811d0584a6ee268.

 Please amend the patch ASAP to use new verify_host_resolvable() 
 function
 so I
 can test it and get it into 4.3.

>>> Updated patches attached.
>>
>> Hmm, my review script screams:
>>
>> Detected base branch:   origin/master
>> Checks will be made against following base commit: 848912a add missing
>> /ipaplatform/constants.py to .gitignore
>> Writing API to API.txt
>> ./ipalib/plugins/dns.py:2127:9: E124 closing bracket does not match 
>> visual
>> indentation
>> ./ipalib/plugins/dns.py:2711:13: E128 continuation line under-indented 
>> for
>> visual indent
>> ./ipalib/plugins/dns.py:2713:9: E124 closing bracket does not match 
>> visual
>> indentation
>> ./ipalib/plugins/dns.py:2716:13: E128 continuation line under-indented 
>> for
>> visual indent
>> ./ipapython/ipautil.py:928:1: E302 expected 2 blank lines, found 1
>> ./ipapython/ipautil.py:948:17: E128 continuation line under-indented for
>> visual indent
>> ./ipapython/ipautil.py:956:1: E302 expected 2 blank lines, found 1
>> ./ipapython/ipautil.py:997:80: E501 line too long (83 > 79 characters)
>> ./ipapython/ipautil.py:998:80: E501 line too long (83 > 79 characters)
>> ./ipaserver/install/bindinstance.py:49:9: E128 continuation line
>> under-indented for visual indent
>> ./ipaserver/install/bindinstance.py:292:80: E501 line too long (80 > 79
>> characters)
>> ./ipaserver/install/bindinstance.py:438:19: E128 continuation line
>> under-indented for visual indent
>> ./ipaserver/install/server/common.py:271:63: E261 at least two spaces
>> before
>> inline comment
>> ./ipaserver/install/server/common.py:271:80: E501 line too long (90 > 79
>> characters)
>> ERROR: PEP8 --diff failed, continuing ...
>> ERROR: API.txt was changed without a change in VERSION, continuing ...
>> Summary of detected errors:
>> ERROR: PEP8 --diff failed
>> ERROR: API.txt was changed without a change in VERSION
>>
>> Please fix it :-)
>>
>> Make make this more pleasant for you, you can use (and review) my 
>> attached
>> patch. It adds 'review' target to Makefile, it will do the same checks as
>> I do.
>>
>
> Unfortunately your tool does not work for me (output w/ -o xtrace 
> attached).
> Anyway I have incremented API version and fixed most of the pep8 errors
> except
> for E124 twice in ipalib/plugins/dns.py as it seems to be convention in 
> the
> file and E501 twice in ipapython/ipautil.py because IMO breaking string
> constant into multiple lines does not help readability.
>
> Updated patches also attached.

 We are almost there, but NACK for now.

 1) Following attempt to install IPA explodes. Please note that
 dom-245.idm.lab.eng.brq.redhat.com DNS domain is delegated to the IPA 
 server
 before installation is started, so it gives 'timeout' or possibly SERVFAIL.

 # ipa-server-install --ds-password=root4lab --admin-password=root4lab
 --setup-dns --forwarder=10.38.5.26 --no-reverse --allow-zone-overlap
 --domain=dom-245.idm.lab.eng.brq.redhat.com
 --realm=DOM-245.IDM.LAB.ENG.BRQ.REDHAT.COM
 [...]

 Configuring DNS (named)
 [1/11]: generating rndc key file
 [2/11]: adding DNS container
 [3/11]: setting up our zone
 [error] InvocationError: DNS check for domain
 dom-245.idm.lab.eng.brq.redhat.com. failed: The DNS operation timed out 
 after
 30.000453949 seconds. Please make sure that the domain is properly 
 delegated
 to this IPA server.
 ipa.ipapython.install.cli.install_tool(Server): ERRORDNS check for 
 domain
 dom-245.idm.lab.eng.brq.redhat.com. failed: The DNS operation timed out 
 after
 30.000453949 seconds. Please make sure that the domain is properly 
 

Re: [Freeipa-devel] [PATCHES 516-517] spec file: put Python modules into standalone packages

[Jan Cholasta]

On 11.12.2015 13:56, Petr Vobornik wrote:

On 12/10/2015 01:51 PM, Jan Cholasta wrote:

On 10.12.2015 11:32, Jan Cholasta wrote:

On 9.12.2015 20:51, Petr Vobornik wrote:

On 12/07/2015 04:21 PM, Jan Cholasta wrote:

Hi,

the attached patches partially fix
. This is done to allow
the addition of Python 3 packages, see
.




See commit messages for more information.

In order to test:
1. make rpms
2.






3. Test with both dnf and yum-deprecated.

Beware that when you run "yum-deprecated clean all", it does not
remove
cache for the on-disk repository created in step 2, you have to remove
the /var/cache/yum/$basearch/$releasever/$reponame directory manually.

Honza



Shouldn't freeipa-server-dns and freeipa-server-trust-add depend on
freeipa-server? They do not in this patch. IMO they should.


Fixed.



following updates work (all on f23, update from 4.2.3):
   dnf update
   dnf update freeipa-*
   yum-depracated update freeipa-*

for both client or server with all packages.

but when I tried to install only client "dnf install freeipa-client"
and
then following failed:
   dnf update freeipa-client

The difference was:
 Installing:
  freeipa-client-common  noarch
  freeipa-common noarch
  python2-ipaclient  noarch
  python2-ipalib x86_64
 Upgrading:
  freeipa-client

Works:
  Installing:
  freeipa-client-common  noarch
  freeipa-common noarch
  freeipa-python-compat  noarch
  replacing  freeipa-python.x86_64 4.2.3-1.1.fc23
  python2-ipaclient  noarch
  python2-ipalib x86_64
 Upgrading:
  freeipa-client


not sure if it is a problem, otherwise the patch looks OK.


Hmm, Fedora packaging guidelines are silent about this.

When you run "dnf update freeipa-client freeipa-python" to force
freeipa-python update it works fine.

I removed Provides added Conflicts on freeipa-python which seem to have
fixed it.


Actually I think it would be better to keep the Provides and do
Conflicts < 4.2.91 - fixed.

Also updated %alt_name dependencies to be in sync with %name
dependencies and added arch-specific python-ipalib Provides to
python2-ipalib.

Updated patches attached.



ACK from me.

What surprised me a bit is that I don't see any other responses here.

If somebody didn't ready the mail, here is the new package list:

freeipa-admintools
freeipa-client
freeipa-client-common
freeipa-common
freeipa-debuginfo
freeipa-python-compat
freeipa-server
freeipa-server-common
freeipa-server-dns
freeipa-server-trust-ad
python2-ipaclient
python2-ipalib
python2-ipaserver
python2-ipatests


Thanks for the review.

Pushed to master: e9baafb08f04f91168469d3d8f36fc792d7d62dc

--
Jan Cholasta

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCHES 523-525] replica install: add remote connection check over API

[Jan Cholasta]

On 11.12.2015 08:03, Jan Cholasta wrote:

On 11.12.2015 07:08, Jan Cholasta wrote:

On 10.12.2015 15:56, Martin Babinsky wrote:

On 12/10/2015 09:48 AM, Jan Cholasta wrote:

On 9.12.2015 16:38, Jan Cholasta wrote:

On 9.12.2015 14:52, Jan Cholasta wrote:

On 9.12.2015 10:02, Jan Cholasta wrote:

Hi,

the attached patches fix
.


Note that this needs selinux-policy fix to work, so put SELinux into
permissive mode for testing:
.


Updated patches attached.


I screwed up a change in patch 524 and accidentally included a chunk of
code in patch 525 that doesn't belong in it.

Updated patches attached.





Patches work as expected and I was not able to find any functional
problem.

I have a question about the naming of the oddjob helper script: the one
related to trusts is named 'com.redhat.idm.trust-fetch-domains', and the
conncheck runner is named 'org.freeipa.server.conncheck'. I don't want
to start another bikeshedding conversation but shouldn't we named them
in a consistent fashion (either rename the first one in separate patch
or rename the new helper to com.redhat.idm.server.conncheck)?

I understand that as an upstream, we should go with the 'org.freeipa.*'
convention, but having two helpers with different prefixes makes me sad.


If you look at the larger picture, org.freeipa is the consistent name.
It makes me sad as well, but mistakes should be corrected. This is
similar to how we use PEP8 in new code, but do not fix it in old code
just for the sake of fixing it.



That is a nitpick though, it does not affect the overall functionality
of the patches so ACK.


Thanks for the review. The current patch 523 breaks the trusts oddjob
with SELinux in enforcing mode, I will send an update which corrects
that, until bug 1289930 is fixed.


Updated patches attached.


Rebased on top of current master.

--
Jan Cholasta
From f0040a84412e03ab6d97d4a3ec8fc697f19df3ca Mon Sep 17 00:00:00 2001
From: Jan Cholasta 
Date: Wed, 9 Dec 2015 08:17:07 +0100
Subject: [PATCH 1/3] build: put oddjob scripts into separate directory

https://fedorahosted.org/freeipa/ticket/5497
---
 freeipa.spec.in| 4 
 install/oddjob/Makefile.am | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/freeipa.spec.in b/freeipa.spec.in
index aceb076..cc4c122 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -585,6 +585,9 @@ make client-install DESTDIR=%{buildroot}
 mkdir -p %{buildroot}%{_usr}/share/ipa
 
 %if ! %{ONLY_CLIENT}
+# FIXME: https://bugzilla.redhat.com/show_bug.cgi?id=1289930
+mv %{buildroot}%{_libexecdir}/ipa/oddjob/com.redhat.idm.trust-fetch-domains %{buildroot}%{_libexecdir}/ipa/com.redhat.idm.trust-fetch-domains
+
 # Remove .la files from libtool - we don't want to package
 # these files
 rm %{buildroot}/%{plugin_dir}/libipa_pwd_extop.la
@@ -905,6 +908,7 @@ fi
 %{_libexecdir}/ipa/ipa-dnskeysync-replica
 %{_libexecdir}/ipa/ipa-ods-exporter
 %{_libexecdir}/ipa/ipa-httpd-kdcproxy
+%dir %{_libexecdir}/ipa/oddjob
 %dir %{_libdir}/ipa/certmonger
 %attr(755,root,root) %{_libdir}/ipa/certmonger/*
 # NOTE: systemd specific section
diff --git a/install/oddjob/Makefile.am b/install/oddjob/Makefile.am
index 9dde10c..5cdaf2b 100644
--- a/install/oddjob/Makefile.am
+++ b/install/oddjob/Makefile.am
@@ -1,6 +1,6 @@
 NULL =
 
-oddjobdir = $(libexecdir)/ipa
+oddjobdir = $(libexecdir)/ipa/oddjob
 oddjobconfdir = $(sysconfdir)/oddjobd.conf.d
 dbusconfdir = $(sysconfdir)/dbus-1/system.d
 
-- 
2.4.3

From a51197977ad476f41fc965c1e94596e060faea3b Mon Sep 17 00:00:00 2001
From: Jan Cholasta 
Date: Wed, 9 Dec 2015 08:18:21 +0100
Subject: [PATCH 2/3] replica install: add remote connection check over API

Add server_conncheck command which calls ipa-replica-conncheck --replica
over oddjob.

https://fedorahosted.org/freeipa/ticket/5497
---
 API.txt|   8 ++
 VERSION|   4 +-
 freeipa.spec.in|   9 +-
 install/oddjob/Makefile.am |   3 +
 .../etc/dbus-1/system.d/org.freeipa.server.conf|  21 
 install/oddjob/etc/oddjobd.conf.d/ipa-server.conf  |  20 
 install/oddjob/org.freeipa.server.conncheck|   2 +
 install/tools/ipa-ca-install   |   6 +
 install/tools/ipa-replica-conncheck| 131 ++---
 install/updates/90-post_upgrade_plugins.update |   1 -
 ipalib/messages.py |  10 ++
 ipalib/plugins/server.py   |  70 ++-
 ipaserver/install/adtrustinstance.py   |  19 ---
 ipaserver/install/ca.py|   2 +-
 ipaserver/install/httpinstance.py  |  26 
 ipaserver/install/installutils.py  |  12 --
 ipaserver/install/plugins/adtrust.py   |  21 

Re: [Freeipa-devel] [PATCH 0018] Fixed install_ca and install_kra failures at domain level 0

[Petr Vobornik]

On 12/11/2015 05:28 PM, Oleg Fayans wrote:

HI Oleg,

could you prefix the commit message and mail subject with "tests: " or 
something similar to make clear that this is a fix in tests and not 
actual CA or KRA installation.

--
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] [PATCHES 529-530] ca install: use host credentials in domain level 1

[Martin Basti]

On 11.12.2015 15:00, Jan Cholasta wrote:

On 10.12.2015 09:51, Jan Cholasta wrote:

Hi,

the attached patches fix .

My patches 523-525 are required for this:
. 




Honza


Rebased patches attached.

Patch works for me, but can you provide explanations (and update commit 
message) why the ACI change is needed:


* why it is moved three ACIs from 'cn="$SUFFIX",cn=mapping 
tree,cn=config' to 'cn=mapping tree,cn=config'

* why you removed completely 'dn: cn=o\3Dipaca,cn=mapping tree,cn=config'

Martin

--
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 0018] Fixed install_ca and install_kra failures at domain level 0

[Oleg Fayans]

-- 
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
From 93b8e9fcbbba0db1f21924b46097c557c9cca358 Mon Sep 17 00:00:00 2001
From: Oleg Fayans 
Date: Fri, 11 Dec 2015 15:58:39 +0100
Subject: [PATCH] Fixed install_ca and install_kra under domain level 0

Also added ipa_backup, ipa_restore and replica_uninstall functions
---
 ipatests/test_integration/tasks.py | 28 
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/ipatests/test_integration/tasks.py b/ipatests/test_integration/tasks.py
index c3681fca952807ac6ebcca56ce961df2d3f33f0c..70071aaad8457e6be1cfaecec7da36bb9f31eaf1 100644
--- a/ipatests/test_integration/tasks.py
+++ b/ipatests/test_integration/tasks.py
@@ -932,9 +932,22 @@ def resolve_record(nameserver, query, rtype="SOA", retry=True, timeout=100):
 time.sleep(1)
 
 
+def ipa_backup(master):
+result = master.run_command(["ipa-backup"])
+myre = re.compile(".*Backed up to (?P.*?)\n.*")
+matched = myre.search(result.stdout_text + result.stderr_text)
+return matched.group("backup")
+
+
+def ipa_restore(master, backup_path):
+master.run_command(["ipa-restore", "-U",
+"-p", master.config.dirman_password,
+backup_path])
+
+
 def install_kra(host, domain_level=None, first_instance=False, raiseonerr=True):
-if not domain_level:
-   domain_level = domainlevel(host)
+if domain_level is None:
+domain_level = domainlevel(host)
 command = ["ipa-kra-install", "-U", "-p", host.config.dirman_password]
 if domain_level == DOMAIN_LEVEL_0 and not first_instance:
 replica_file = get_replica_filename(host)
@@ -943,8 +956,8 @@ def install_kra(host, domain_level=None, first_instance=False, raiseonerr=True):
 
 
 def install_ca(host, domain_level=None, first_instance=False, raiseonerr=True):
-if not domain_level:
-   domain_level = domainlevel(host)
+if domain_level is None:
+domain_level = domainlevel(host)
 command = ["ipa-ca-install", "-U", "-p", host.config.dirman_password,
"-P", 'admin', "-w", host.config.admin_password]
 if domain_level == DOMAIN_LEVEL_0 and not first_instance:
@@ -961,3 +974,10 @@ def install_dns(host, raiseonerr=True):
 "-U",
 ]
 return host.run_command(args, raiseonerr=raiseonerr)
+
+
+def uninstall_replica(master, replica):
+master.run_command(["ipa-replica-manage", "del", "--force",
+"-p", master.config.dirman_password,
+replica.hostname], raiseonerr=False)
+uninstall_master(replica)
-- 
2.4.3

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCHES 523-525] replica install: add remote connection check over API

[Martin Basti]

On 11.12.2015 15:40, Jan Cholasta wrote:

On 11.12.2015 08:03, Jan Cholasta wrote:

On 11.12.2015 07:08, Jan Cholasta wrote:

On 10.12.2015 15:56, Martin Babinsky wrote:

On 12/10/2015 09:48 AM, Jan Cholasta wrote:

On 9.12.2015 16:38, Jan Cholasta wrote:

On 9.12.2015 14:52, Jan Cholasta wrote:

On 9.12.2015 10:02, Jan Cholasta wrote:

Hi,

the attached patches fix
.


Note that this needs selinux-policy fix to work, so put SELinux 
into

permissive mode for testing:
.


Updated patches attached.


I screwed up a change in patch 524 and accidentally included a 
chunk of

code in patch 525 that doesn't belong in it.

Updated patches attached.





Patches work as expected and I was not able to find any functional
problem.

I have a question about the naming of the oddjob helper script: the 
one
related to trusts is named 'com.redhat.idm.trust-fetch-domains', 
and the

conncheck runner is named 'org.freeipa.server.conncheck'. I don't want
to start another bikeshedding conversation but shouldn't we named them
in a consistent fashion (either rename the first one in separate patch
or rename the new helper to com.redhat.idm.server.conncheck)?

I understand that as an upstream, we should go with the 
'org.freeipa.*'
convention, but having two helpers with different prefixes makes me 
sad.


If you look at the larger picture, org.freeipa is the consistent name.
It makes me sad as well, but mistakes should be corrected. This is
similar to how we use PEP8 in new code, but do not fix it in old code
just for the sake of fixing it.



That is a nitpick though, it does not affect the overall functionality
of the patches so ACK.


Thanks for the review. The current patch 523 breaks the trusts oddjob
with SELinux in enforcing mode, I will send an update which corrects
that, until bug 1289930 is fixed.


Updated patches attached.


Rebased on top of current master.




Just question, should be any kinited user allowed to run conncheck via rpc?

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 0391] replicainstall: Add check for domain if server is specified

[Petr Vobornik]

On 12/08/2015 02:00 PM, Simo Sorce wrote:

On Tue, 2015-12-08 at 13:34 +0100, Martin Kosek wrote:

On 12/08/2015 08:28 AM, Jan Cholasta wrote:

On 8.12.2015 08:23, Martin Kosek wrote:

On 12/08/2015 07:57 AM, Jan Cholasta wrote:

On 7.12.2015 16:43, Martin Kosek wrote:

On 12/07/2015 02:17 PM, Tomas Babej wrote:



On 12/04/2015 08:22 PM, Rob Crittenden wrote:

Martin Kosek wrote:

On 12/04/2015 07:17 PM, Tomas Babej wrote:

Hi,

Avoids failing in the later stages during the ipa-client-install
command.

Tomas


Is this change needed? Wouldn't it be better to update
ipa-client-install or ipa-replica-install to not require the --domain
option? I would hope that --domain can be figured out during
installation and not passed to ipa-replica-install manually by the admin.

I just think that calling
# ipa-replica-install --server=master.example.com
is better than
# ipa-replica-install --server=master.example.com --domain example.com
if possible.


IIRC this is for service discovery when using a specific server and not
LDAP. This is the domain used to search for the kerberos realm, for
example.

That isn't to say this isn't discoverable but it would require another
function in discovery to query what the IPA domain is from the given
master but it gets tricky if anonymous search is disabled, for example.

rob



Needed or not, this is the behaviour that ipa-client-install has now.
Adding a domain detection method would be a RFE for ipa-client-install
(and imho not something we should be adding at this point).

This patch only focuses on making the ipa-replica-install work more
smoothly.


I am just thinking that client promotion (ipa-replica-install) and
ipa-client-install are a bit different use cases. While ipa-client-install
should be typically run in auto-discovery and you thus do not use --server
option much, while with ipa-replica-install, you want to make sure you have
the
expected topology and should use --server all the time without gambling on it.

But I do not think it has to be there since 4.3 GA, can you please file a
ticket for this gap?


I would rather do it now, because the change from optional to mandatory is
backward incompatible. (We don't want to break users' scripts, right?)


I think it is the other way around - with the change I was suggesting
(autodetecting --domain option instead of always requesting it, as in Tomas'
patch which we can merge if my proposal is not doable for 4.3 GA).



"with ipa-replica-install, you want to make sure you have the expected topology
and should use --server all the time" sounds like you want to make --server
mandatory for ipa-replica-install, which should be done either before 4.3 GA or
never.


Ah, no, this is not what I meant. I was only discussing the --domain option in
this mail the the typical use cases for --server option in ipa-client-install
and ipa-replica-install.

If we can trust ipa-replica-install to do a good job in picking a server to
replica from, the --server can stay optional. Although I am on fence here,
being more explicit when creating topology may be helpful. CCing Simo, in case
he has some opinions on this.


Leave it optional, our first order of business is making things simple,
then adding optional knobs to let the admin with knowledge to tweak
things.

Simo.



ACK for original  patch

Pushed to master: c3c8651ac1bac794e32b3c01f7e4f6b487dcef08
--
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 0373] Upgrade: Fix IPA version comparison

[Martin Kosek]

On 12/10/2015 05:09 PM, Martin Basti wrote:



On 10.12.2015 15:49, Tomas Babej wrote:


On 12/10/2015 11:23 AM, Martin Basti wrote:


On 10.12.2015 09:13, Lukas Slebodnik wrote:

On (09/12/15 19:22), Martin Basti wrote:

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

Patch attached.

>From 8ef93485d61e8732166fb0c5b6c4559209740f3e Mon Sep 17 00:00:00 2001

From: Martin Basti 
Date: Wed, 9 Dec 2015 18:53:35 +0100
Subject: [PATCH] Fix version comparison

Use RPM library to compare vendor versions of IPA for redhat platform

https://fedorahosted.org/freeipa/ticket/5535
---
freeipa.spec.in |  2 ++
ipaplatform/redhat/tasks.py | 19 +++
2 files changed, 21 insertions(+)

diff --git a/freeipa.spec.in b/freeipa.spec.in
index
9f82b3695fb10c4db65cc31278364b3b34e26098..09feba7b8324f5e645da3e8010de86b6c3ee5ab9

100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -166,6 +166,8 @@ Requires: %{etc_systemd_dir}
Requires: gzip
Requires: python-gssapi >= 1.1.0
Requires: custodia
+Requires: rpm-python
+Requires: rpmdevtools

Could you explain why do you need the 2nd package?
It does not contains any python modules
and I cannot see usage of any binary in this patch

LS

Thanks for this catch, it is actually located in yum package, I rather
copy stringToVersion function from there to IPA, to avoid dependency hell

Updated patch attached.



Looking good. The __cmp__ function, however, is not available in Python
3. As we will eventually support python3 in RHEL as well, maybe we
should make sure even platform-dependent parts are python3 compatible?
For the future's sake.

Tomas


Thanks,

python 3 compatible patch attached.


I wonder why we have to add so much code here and reimplement RPM version 
checking if it is already provided by rpmdevtools:


~~~
$ /usr/bin/rpmdev-vercmp ipa-4.2.0-15.el7 4.2.0-15.el7_2.3; echo $?
WARNING: hyphen in release1: 4.2.0-15.el7

rpmdev-vercmp  
rpmdev-vercmp  
rpmdev-vercmp # with no arguments, prompt

Exit status is 0 if the EVR's are equal, 11 if EVR1 is newer, and 12 if EVR2
is newer.  Other exit statuses indicate problems.

ipa-4.2.0-15.el7 < 4.2.0-15.el7_2.3
12
~~~

which could be directly called from __eq__ or __lt__, since we are in platform 
specific code anyway already.


Martin

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCHES 523-525] replica install: add remote connection check over API

[Tomas Babej]

On 12/11/2015 03:40 PM, Jan Cholasta wrote:
> On 11.12.2015 08:03, Jan Cholasta wrote:
>> On 11.12.2015 07:08, Jan Cholasta wrote:
>>> On 10.12.2015 15:56, Martin Babinsky wrote:
 On 12/10/2015 09:48 AM, Jan Cholasta wrote:
> On 9.12.2015 16:38, Jan Cholasta wrote:
>> On 9.12.2015 14:52, Jan Cholasta wrote:
>>> On 9.12.2015 10:02, Jan Cholasta wrote:
 Hi,

 the attached patches fix
 .
>>>
>>> Note that this needs selinux-policy fix to work, so put SELinux into
>>> permissive mode for testing:
>>> .
>>
>> Updated patches attached.
>
> I screwed up a change in patch 524 and accidentally included a
> chunk of
> code in patch 525 that doesn't belong in it.
>
> Updated patches attached.
>
>
>

 Patches work as expected and I was not able to find any functional
 problem.

 I have a question about the naming of the oddjob helper script: the one
 related to trusts is named 'com.redhat.idm.trust-fetch-domains', and
 the
 conncheck runner is named 'org.freeipa.server.conncheck'. I don't want
 to start another bikeshedding conversation but shouldn't we named them
 in a consistent fashion (either rename the first one in separate patch
 or rename the new helper to com.redhat.idm.server.conncheck)?

 I understand that as an upstream, we should go with the 'org.freeipa.*'
 convention, but having two helpers with different prefixes makes me
 sad.
>>>
>>> If you look at the larger picture, org.freeipa is the consistent name.
>>> It makes me sad as well, but mistakes should be corrected. This is
>>> similar to how we use PEP8 in new code, but do not fix it in old code
>>> just for the sake of fixing it.
>>>

 That is a nitpick though, it does not affect the overall functionality
 of the patches so ACK.
>>>
>>> Thanks for the review. The current patch 523 breaks the trusts oddjob
>>> with SELinux in enforcing mode, I will send an update which corrects
>>> that, until bug 1289930 is fixed.
>>
>> Updated patches attached.
> 
> Rebased on top of current master.
> 
> 
> 

ACK from me too,
Pushed to master: 14a44ea47bf9a617019ebc91fbe272215c428d82

-- 
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 0066] Migrate wget references to curl

[Martin Basti]

ACK

Pushed to master: 5c9b9089b7b0d40b7e924177f99c2568aaa1b5b2

On 04.12.2015 22:55, Gabe Alford wrote:

My bad. Copy and paste error. Updated patch attached.

Thanks,

Gabe

On Fri, Dec 4, 2015 at 12:17 PM, Martin Basti > wrote:




On 01.12.2015 15:00, Gabe Alford wrote:

Hello,

Fix for https://fedorahosted.org/freeipa/ticket/5458

Thanks,

Gabe



Hello,

I haven't looked closer, but your patch is causing this:

Configuring certificate server (pki-tomcatd). Estimated time: 3
minutes 30 seconds
  [1/27]: creating certificate server user
  [2/27]: configuring certificate server instance
  [3/27]: stopping certificate server instance to update CS.cfg
  [4/27]: backing up CS.cfg
  [5/27]: disabling nonces
  [6/27]: set up CRL publishing
  [7/27]: enable PKIX certificate path discovery and validation
  [8/27]: starting certificate server instance
ipa.ipaserver.install.cainstance.CAInstance: CRITICAL Failed to
restart the Dogtag instance.See the installation log for details.
  [9/27]: creating RA agent certificate database
  [10/27]: importing CA chain to RA certificate database
  [11/27]: fixing RA database permissions
  [12/27]: setting up signing cert profile
  [13/27]: setting audit signing renewal to 2 years
  [14/27]: restarting certificate server
ipa.ipaserver.install.cainstance.CAInstance: CRITICAL Failed to
restart the Dogtag instance.See the installation log for details.
  [15/27]: requesting RA certificate from CA
  [16/27]: issuing RA agent certificate
  [17/27]: adding RA agent as a trusted user
  [18/27]: authorizing RA to modify profiles
  [19/27]: configure certmonger for renewals
  [20/27]: configure certificate renewals
  [21/27]: configure RA certificate renewal
  [22/27]: configure Server-Cert certificate renewal
  [23/27]: Configure HTTP to proxy connections
  [24/27]: restarting certificate server

ipa.ipaserver.install.cainstance.CAInstance: CRITICAL Failed to
restart the Dogtag instance.See the installation log for details.
  [25/27]: migrating certificate profiles to LDAP
  [26/27]: importing IPA certificate profiles
  [27/27]: adding default CA ACL


CA is operational and ready, but IPA installer is not able to
detect it correctly

2015-12-04T19:08:54Z DEBUG stderr=curl: option --connect-timeout
30: is unknown
curl: try 'curl --help' or 'curl --manual' for more information

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 0373] Upgrade: Fix IPA version comparison

[Tomas Babej]

On 12/11/2015 09:36 AM, Martin Kosek wrote:
> On 12/10/2015 05:09 PM, Martin Basti wrote:
>>
>>
>> On 10.12.2015 15:49, Tomas Babej wrote:
>>>
>>> On 12/10/2015 11:23 AM, Martin Basti wrote:

 On 10.12.2015 09:13, Lukas Slebodnik wrote:
> On (09/12/15 19:22), Martin Basti wrote:
>> https://fedorahosted.org/freeipa/ticket/5535
>>
>> Patch attached.
> >From 8ef93485d61e8732166fb0c5b6c4559209740f3e Mon Sep 17 00:00:00
> 2001
>> From: Martin Basti 
>> Date: Wed, 9 Dec 2015 18:53:35 +0100
>> Subject: [PATCH] Fix version comparison
>>
>> Use RPM library to compare vendor versions of IPA for redhat platform
>>
>> https://fedorahosted.org/freeipa/ticket/5535
>> ---
>> freeipa.spec.in |  2 ++
>> ipaplatform/redhat/tasks.py | 19 +++
>> 2 files changed, 21 insertions(+)
>>
>> diff --git a/freeipa.spec.in b/freeipa.spec.in
>> index
>> 9f82b3695fb10c4db65cc31278364b3b34e26098..09feba7b8324f5e645da3e8010de86b6c3ee5ab9
>>
>>
>> 100644
>> --- a/freeipa.spec.in
>> +++ b/freeipa.spec.in
>> @@ -166,6 +166,8 @@ Requires: %{etc_systemd_dir}
>> Requires: gzip
>> Requires: python-gssapi >= 1.1.0
>> Requires: custodia
>> +Requires: rpm-python
>> +Requires: rpmdevtools
> Could you explain why do you need the 2nd package?
> It does not contains any python modules
> and I cannot see usage of any binary in this patch
>
> LS
 Thanks for this catch, it is actually located in yum package, I rather
 copy stringToVersion function from there to IPA, to avoid dependency
 hell

 Updated patch attached.


>>> Looking good. The __cmp__ function, however, is not available in Python
>>> 3. As we will eventually support python3 in RHEL as well, maybe we
>>> should make sure even platform-dependent parts are python3 compatible?
>>> For the future's sake.
>>>
>>> Tomas
>>>
>> Thanks,
>>
>> python 3 compatible patch attached.
> 
> I wonder why we have to add so much code here and reimplement RPM
> version checking if it is already provided by rpmdevtools:
> 
> ~~~
> $ /usr/bin/rpmdev-vercmp ipa-4.2.0-15.el7 4.2.0-15.el7_2.3; echo $?
> WARNING: hyphen in release1: 4.2.0-15.el7
> 
> rpmdev-vercmp  
> rpmdev-vercmp  
> rpmdev-vercmp # with no arguments, prompt
> 
> Exit status is 0 if the EVR's are equal, 11 if EVR1 is newer, and 12 if
> EVR2
> is newer.  Other exit statuses indicate problems.
> 
> ipa-4.2.0-15.el7 < 4.2.0-15.el7_2.3
> 12
> ~~~
> 
> which could be directly called from __eq__ or __lt__, since we are in
> platform specific code anyway already.
> 
> Martin

Imho we should generally prefer reaching out to a Python library rather
launching a subprocess to compare the versions, it's unnecessary overhead.

That said, the main argument for the usage of rpm-python over
rpmdevtools I see is that rpm-python is very likely to be present on the
system (i.e. it is yum's own dependency), while rpmdevtools will not be
present by default.

>From the standpoint of trying to minimize the size of freeipa
installation (i.e recent wget -> curl migration), we should keep the
spirit here and do not introduce a dependency for a collection of
developer tools.

/2 cents

Tomas

-- 
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 0065] ipa-replica-install prints incorrect error message when replica is already installed

[Martin Basti]

ACK

Pushed to master: 12e7f71600e62eab9d48a13fba37d2f182c8bdee

On 09.12.2015 14:44, Gabe Alford wrote:

Fixed. Updated patch attached.

On Wed, Dec 9, 2015 at 2:37 AM, Martin Basti > wrote:


NACK

Patch contains syntax error, missing brace

ipaserver/install/server/replicainstall.py:850:
[E0001(syntax-error), ] invalid syntax)

Martin


On 09.12.2015 07:08, Jan Cholasta wrote:

LGTM

On 8.12.2015 17:04, Gabe Alford wrote:

Updated patch attached.

On Tue, Dec 8, 2015 at 8:27 AM, Martin Basti

>> wrote:



On 08.12.2015 16:26, Gabe Alford wrote:

Just to confirm:

if server is installed:
 Let's stop here and not do anything else

if domain level 0:
 check if client installed and stop here

Right?

yes




On Tue, Dec 8, 2015 at 8:20 AM, Jan Cholasta

>> wrote:

On 8.12.2015 16:17, Martin Basti wrote:



On 08.12.2015 16:14, Jan Cholasta wrote:

On 8.12.2015 16:09, Martin Basti wrote:



On 01.12.2015 14:57, Gabe Alford
wrote:

Sorry guys, I forgot to add a
meaningful
subject to this message.
Ignore the previous thread start.

-- Forwarded message
--
From: *Gabe Alford*

>


>
   

Re: [Freeipa-devel] [PATCH 0374-0375] Fix permissions on newly created directories

[Martin Basti]

On 10.12.2015 15:18, Martin Basti wrote:

Hello,

patch 0374 fixes the ticket, but I found more issues with directory 
permission, I fixed them in 0375


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

Patches attached.


Patches attached.
From ae0bcea3f6173bd6466d26a7d0cb2886029a10f6 Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Wed, 9 Dec 2015 12:12:22 +0100
Subject: [PATCH] DNS: fix file permissions

With non default umask named-pkcs11 cannot access the softhsm token storage

https://fedorahosted.org/freeipa/ticket/5520
---
 ipaserver/install/dnskeysyncinstance.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/ipaserver/install/dnskeysyncinstance.py b/ipaserver/install/dnskeysyncinstance.py
index b2ccc027469a352c815963abfd0c0a61dd37297f..f2a976eecd2c4f6de1e12c46969c6d5addd79e41 100644
--- a/ipaserver/install/dnskeysyncinstance.py
+++ b/ipaserver/install/dnskeysyncinstance.py
@@ -201,7 +201,8 @@ class DNSKeySyncInstance(service.Service):
 # create dnssec directory
 if not os.path.exists(paths.IPA_DNSSEC_DIR):
 self.logger.debug("Creating %s directory", paths.IPA_DNSSEC_DIR)
-os.mkdir(paths.IPA_DNSSEC_DIR, 0o770)
+os.mkdir(paths.IPA_DNSSEC_DIR)
+os.chmod(paths.IPA_DNSSEC_DIR, 0o770)
 # chown ods:named
 os.chown(paths.IPA_DNSSEC_DIR, self.ods_uid, self.named_gid)
 
@@ -218,6 +219,7 @@ class DNSKeySyncInstance(service.Service):
 named_fd.truncate(0)
 named_fd.write(softhsm_conf_txt)
 named_fd.close()
+os.chmod(paths.DNSSEC_SOFTHSM2_CONF, 0o644)
 
 # setting up named to use softhsm2
 if not self.fstore.has_file(paths.SYSCONFIG_NAMED):
-- 
2.5.0

From 57f7841185e6e25d12ca83a537d2cb7184854a23 Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Wed, 9 Dec 2015 13:40:04 +0100
Subject: [PATCH] Explicitly call chmod on newly created directories

Without calling os.chmod(), umask is effective and may cause that
directory is created with permission that causes failure.

This can be related to https://fedorahosted.org/freeipa/ticket/5520
---
 ipaplatform/base/services.py |  1 +
 ipaserver/install/cainstance.py  |  1 +
 ipaserver/install/ipa_backup.py  |  7 ---
 ipaserver/install/ipa_replica_prepare.py |  3 ++-
 ipaserver/install/ipa_restore.py | 10 ++
 5 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/ipaplatform/base/services.py b/ipaplatform/base/services.py
index da2f1011e34431664cd5c730668ae483b7bd0a1d..e6a0403b6edfb62a1d7f807fef93121718ba59f5 100644
--- a/ipaplatform/base/services.py
+++ b/ipaplatform/base/services.py
@@ -421,6 +421,7 @@ class SystemdService(PlatformService):
 try:
 if not ipautil.dir_exists(srv_tgt):
 os.mkdir(srv_tgt)
+os.mkdir(srv_tgt, 0o755)
 if os.path.exists(srv_lnk):
 # Remove old link
 os.unlink(srv_lnk)
diff --git a/ipaserver/install/cainstance.py b/ipaserver/install/cainstance.py
index 2ca718a7b6799b7daf825918517a54852746a84f..56ec3fe74e8d4adfe17f46a62f705021f6a81f75 100644
--- a/ipaserver/install/cainstance.py
+++ b/ipaserver/install/cainstance.py
@@ -794,6 +794,7 @@ class CAInstance(DogtagInstance):
 
 if not ipautil.dir_exists(self.ra_agent_db):
 os.mkdir(self.ra_agent_db)
+os.chmod(self.ra_agent_db, 0o755)
 
 # Create the password file for this db
 hex_str = binascii.hexlify(os.urandom(10))
diff --git a/ipaserver/install/ipa_backup.py b/ipaserver/install/ipa_backup.py
index 6d97ef13b383b9917fa70426a99463f8c14955e8..523cb9180f36a32f3d18547c9b5db86e913985d9 100644
--- a/ipaserver/install/ipa_backup.py
+++ b/ipaserver/install/ipa_backup.py
@@ -272,8 +272,8 @@ class Backup(admintool.AdminTool):
 os.chown(self.top_dir, pent.pw_uid, pent.pw_gid)
 os.chmod(self.top_dir, 0o750)
 self.dir = os.path.join(self.top_dir, "ipa")
-os.mkdir(self.dir, 0o750)
-
+os.mkdir(self.dir)
+os.chmod(self.dir, 0o750)
 os.chown(self.dir, pent.pw_uid, pent.pw_gid)
 
 self.header = os.path.join(self.top_dir, 'header')
@@ -585,7 +585,8 @@ class Backup(admintool.AdminTool):
 backup_dir = os.path.join(paths.IPA_BACKUP_DIR, time.strftime('ipa-full-%Y-%m-%d-%H-%M-%S'))
 filename = os.path.join(backup_dir, "ipa-full.tar")
 
-os.mkdir(backup_dir, 0o700)
+os.mkdir(backup_dir)
+os.chmod(backup_dir, 0o700)
 
 cwd = os.getcwd()
 os.chdir(self.dir)
diff --git a/ipaserver/install/ipa_replica_prepare.py b/ipaserver/install/ipa_replica_prepare.py
index c1bce693b37d26944339f0797b5c15b3da847215..cef0228ea87b8e0bc2c01cfe4b1589811c631c79 100644
--- a/ipaserver/install/ipa_replica_prepare.py
+++ b/ipaserver/install/ipa_replica_prepare.py
@@ -361,7 +361,8 @@ class 

[Freeipa-devel] [PATCH 0022] topology plugin prevents deletes but does not prevent moddn

[Ludwig Krispenz]

Ticket: https://fedorahosted.org/freeipa/ticket/5536

Patch attached.
>From 592c2cfece7c1f0860cacc72b642826d5b4a7791 Mon Sep 17 00:00:00 2001
From: Ludwig Krispenz 
Date: Fri, 11 Dec 2015 13:50:53 +0100
Subject: [PATCH] prevent moving of topology entries out of managed scope by
 modrdn operations

Ticket: https://fedorahosted.org/freeipa/ticket/5536
---
 daemons/ipa-slapi-plugins/topology/topology.h  |  1 +
 daemons/ipa-slapi-plugins/topology/topology_init.c |  2 +
 daemons/ipa-slapi-plugins/topology/topology_pre.c  | 53 ++
 3 files changed, 56 insertions(+)

diff --git a/daemons/ipa-slapi-plugins/topology/topology.h b/daemons/ipa-slapi-plugins/topology/topology.h
index d264ed9c1e3e903d7554963b843d1f98385ec47a..4ea2b368f279b56c330dc2067eb6f6aee36b1abb 100644
--- a/daemons/ipa-slapi-plugins/topology/topology.h
+++ b/daemons/ipa-slapi-plugins/topology/topology.h
@@ -211,6 +211,7 @@ int ipa_topo_post_del(Slapi_PBlock *pb);
 /* preop plugin functions */
 int ipa_topo_pre_add(Slapi_PBlock *pb);
 int ipa_topo_pre_mod(Slapi_PBlock *pb);
+int ipa_topo_pre_modrdn(Slapi_PBlock *pb);
 int ipa_topo_pre_del(Slapi_PBlock *pb);
 
 /* functions to modify agreements */
diff --git a/daemons/ipa-slapi-plugins/topology/topology_init.c b/daemons/ipa-slapi-plugins/topology/topology_init.c
index de53ad69ed636ef59b26e64d760d60b9da3a5dfd..02ff495e36b33e35abce361b61c1c2ba8871a5e8 100644
--- a/daemons/ipa-slapi-plugins/topology/topology_init.c
+++ b/daemons/ipa-slapi-plugins/topology/topology_init.c
@@ -90,6 +90,8 @@ ipa_topo_preop_init(Slapi_PBlock *pb)
 
 rc = slapi_pblock_set(pb, SLAPI_PLUGIN_BE_PRE_MODIFY_FN,
   (void *)ipa_topo_pre_mod);
+rc |= slapi_pblock_set(pb, SLAPI_PLUGIN_BE_PRE_MODRDN_FN,
+  (void *)ipa_topo_pre_modrdn);
 rc |= slapi_pblock_set(pb, SLAPI_PLUGIN_BE_PRE_ADD_FN,
   (void *)ipa_topo_pre_add);
 rc |= slapi_pblock_set(pb, SLAPI_PLUGIN_BE_PRE_DELETE_FN,
diff --git a/daemons/ipa-slapi-plugins/topology/topology_pre.c b/daemons/ipa-slapi-plugins/topology/topology_pre.c
index 1788c6d3e9d95543d905054d9d1f31c40dddc045..d0436bafcc52bf0b187fe08400c0a656e97cd4b4 100644
--- a/daemons/ipa-slapi-plugins/topology/topology_pre.c
+++ b/daemons/ipa-slapi-plugins/topology/topology_pre.c
@@ -402,6 +402,29 @@ ipa_topo_check_segment_updates(Slapi_PBlock *pb)
 }
 
 int
+ipa_topo_check_entry_move(Slapi_PBlock *pb)
+{
+int rc = 0;
+int entry_type = TOPO_IGNORE_ENTRY;
+Slapi_Entry *modrdn_entry;
+slapi_pblock_get(pb,SLAPI_MODRDN_TARGET_ENTRY,_entry);
+entry_type = ipa_topo_check_entry_type(modrdn_entry);
+switch (entry_type) {
+case TOPO_SEGMENT_ENTRY:
+case TOPO_CONFIG_ENTRY: {
+Slapi_DN *newsuperior = NULL;
+slapi_pblock_get(pb, SLAPI_MODRDN_NEWSUPERIOR_SDN, );
+if (newsuperior && slapi_sdn_get_dn(newsuperior)) rc = 1;
+break;
+}
+default:
+rc = 0;
+break;
+}
+return rc;
+}
+
+int
 ipa_topo_check_host_updates(Slapi_PBlock *pb)
 {
 int rc = 0;
@@ -605,3 +628,33 @@ ipa_topo_pre_del(Slapi_PBlock *pb)
 "<-- ipa_topo_pre_del\n");
 return result;
 }
+int
+ipa_topo_pre_modrdn(Slapi_PBlock *pb)
+{
+
+int result = SLAPI_PLUGIN_SUCCESS;
+
+slapi_log_error(SLAPI_LOG_PLUGIN, IPA_TOPO_PLUGIN_SUBSYSTEM,
+"--> ipa_topo_pre_modrdn\n");
+
+if (0 == ipa_topo_get_plugin_active()) {
+slapi_log_error(SLAPI_LOG_PLUGIN, IPA_TOPO_PLUGIN_SUBSYSTEM,
+"<-- ipa_topo_pre_modrdn - plugin not active\n");
+return 0;
+}
+
+if (ipa_topo_pre_ignore_op(pb)) return result;
+
+if (ipa_topo_check_entry_move(pb)){
+int rc = LDAP_UNWILLING_TO_PERFORM;
+char *errtxt;
+errtxt = slapi_ch_smprintf("Moving of a segment or config entry "
+   "to another subtree is not allowed.\n");
+slapi_pblock_set(pb, SLAPI_PB_RESULT_TEXT, errtxt);
+slapi_pblock_set(pb, SLAPI_RESULT_CODE, );
+result = SLAPI_PLUGIN_FAILURE;
+}
+
+return result;
+
+}
-- 
2.4.3

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0069] Add 'review' target for make

[Petr Spacek]

On 11.12.2015 09:22, Lukas Slebodnik wrote:
> On (10/12/15 18:04), Petr Spacek wrote:
>> On 9.12.2015 15:30, Petr Spacek wrote:
>>> Hello,
>>>
>>> this patch automates some of sanity checks proposed by Petr Vobornik.
>>>
>>> 'make review' should be used in root of clean Git tree which has patches 
>>> under
>>> review applied.
>>>
> +1 for automatisation
> -1 for name.
> 
> Your script does not review[1] anything. Code review(peer review) is a job
> for humans. What do you think about alternative
> names: "review-helper", "sanity-check" ...

I like short name but I respect your point, so I've updated the script to print:
"No problems detected using lint, pep8, ./make{api,aci}, and VERSION" at the 
end.
Now it should be clear what it did, so reviewer knows what steps were already
done and what can be skipped during further manual review.

Attached patch also adds 'assert-clean-tree' target which is run before lint
target, so no time is wasted if the tree is not clean. Parallel build will
execute and terminate both targets at the same time, but it saves time in both
cases, the output is just not so nice.

And IPA cannot be built in parallel anyway :-)

>>> Magic in review.sh attempts to detect nearest remote branch which can be 
>>> used
>>> as diff base for review. Please see review.sh for further details.
>>
>> And here is the patch! :-)
>>
>> -- 
>> Petr^2 Spacek
> 
> [1] https://en.wikipedia.org/wiki/Code_review
> 
>>From 52393bf2bb0ad74dbe37e496b1fd41a6ab22bd90 Mon Sep 17 00:00:00 2001
>> From: Petr Spacek 
>> Date: Tue, 8 Dec 2015 12:06:33 +0100
>> Subject: [PATCH] Add 'review' target for make. It automates following tasks:
>>
>> - check if ACI.txt and API.txt are up-to-date
>> - check if VERSION was changed if API was changed
>> - pep8 --diff does not produce new errors when ran on diff from origin/branch
>> - make lint does not produce errors
>> ---
>> Makefile  |  4 
>> review.sh | 64 
>> +++
>> 2 files changed, 68 insertions(+)
>> create mode 100755 review.sh
>>
>> diff --git a/Makefile b/Makefile
>> index 
>> d2c37f1597a011af2bd9ef1a4f7ce87ac59620a3..b85b3a5d6362150dcebf16745414c3d416c9547f
>>  100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -79,6 +79,10 @@ check: bootstrap-autogen server tests
>>  (cd $$subdir && $(MAKE) check) || exit 1; \
>>  done
>>
>> +# works only in Git tree
>> +review: version-update
>> +./review.sh
>> +
>> bootstrap-autogen: version-update client-autogen
>>  @echo "Building IPA $(IPA_VERSION)"
>>  cd asn1; if [ ! -e Makefile ]; then ../autogen.sh --prefix=/usr 
>> --sysconfdir=/etc --localstatedir=/var --libdir=$(LIBDIR); fi
>> diff --git a/review.sh b/review.sh
>> new file mode 100755
>> index 
>> ..3a5114fbe88d3aba8e926e049500f5ac5f41a83e
>> --- /dev/null
>> +++ b/review.sh
>> @@ -0,0 +1,64 @@
>> +#!/bin/bash
>> +is_tree_clean() {
>> +test "$(git status --porcelain "$@")" == ""
>> +return $?
>> +}
>> +
>> +log_error() {
>> +echo "ERROR: ${1}, continuing ..."
>> +errs=(${errs[@]} "ERROR: ${1}\n")
>> +}
>> +
>> +set -o errexit -o nounset #-o xtrace
>> +
>> +LOGFILE="$(mktemp --suff=.log)"
>> +exec > >(tee -i ${LOGFILE})
>> +exec 2>&1
>> +
>> +echo -n "make lint is running ... "
>> +make --silent lint || log_error "make lint failed"
>> +
>> +# Go backwards in history until you find a remote branch from which current
>> +# branch was created. This will be used as base for git diff.
>> +PATCHCNT=0
>> +BASEBRANCH=""
> If base branch means the remote branch which was used for cloning the cerrent
> git HEAD than you can simplify it a littl bit.
> git rev-parse --symbolic-full-name @{upstream}

Nice! I did not know this trick. Unfortunately it does not work for my
use-case: I clone upstream repo, pick a branch to base review on, create my
own branch from the upstream one, and then apply patches on top of that.

Imagine that I'm going to run the tool on branch 'review-tool'. Git history
tree looks like this:

* d036f3e (HEAD -> review-tool) fixup! Add 'review' target for make.
* c6d75b0 Makefile: disable parallel build
| * 667d50b (dkupka-review) dns: Add --auto-reverse option.
| * c85f176 dns: Check if domain already exists.
| * a55cd76 dns: do not add (forward)zone if it is already resolvable.
| * 0b0c529 (dns-no-forwarders) DNS: Make --no-forwarders option default.
|/
* d2e8470 Add 'review' target for make. It automates following tasks:
| * 101ffae (mbasti-review) Fix DNS tests: dns-resolve returns warning
| * 52393bf Add 'review' target for make. It automates following tasks:
|/
* 848912a (origin/master, master) add missing /ipaplatform/constants.py to
.gitignore

$ git rev-parse --symbolic-full-name @{upstream}
errors out like this:
fatal: no upstream configured for branch 'review-tool'

I definitely agree that the code above is just an ugly hack, but I would like
to keep current behavior so I do not need to