[Freeipa-devel] Testing FreeIPA 4.3 for GA

[Petr Vobornik]

Blocking patches for FreeIPA 4.3 were pushed, ipa-4-3 branch was 
created. Master branch is ready for 4.4 development.


A build is available for testing in my pvoborni/freeipa-4-3 COPR repo 
[1] until the official 4.3 repo is created. The repo contains only this 
build. The build is not pure upstream ipa-4-3 branch but rather a build 
which will go to Fedora rawhide and official 4.3 copr repo - Fedora 
downstream spec with the SELinux workaround applied [2][3].


Known issues:
* https://fedorahosted.org/freeipa/ticket/5469 - requires fix in PKI
* https://bugzilla.redhat.com/show_bug.cgi?id=1289930 - SELinux Policy 
update needed for connection check


I have started drafting release page [4].

[1] https://copr.fedoraproject.org/coprs/pvoborni/freeipa-4-3/
[2] https://fedorahosted.org/freeipa/ticket/5533
[3] http://www.redhat.com/archives/freeipa-devel/2015-December/msg00234.html
[4] http://www.freeipa.org/page/Releases/4.3.0
--
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 14.12.2015 07:53, Jan Cholasta wrote:

On 11.12.2015 17:24, Martin Basti wrote:



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'


So that they apply to all replication agreements.

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


I didn't, they were moved to cn=mapping tree,cn=config as well.

Updated patches attached.


ACK

--
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 531-532] server install: redirect ipa-client-install output to standard output

[Jan Cholasta]

On 14.12.2015 12:54, Tomas Babej wrote:



On 12/14/2015 12:52 PM, Jan Cholasta wrote:

Hi,

the attached patches fix .

Honza





Shouldn't skip_output be also marked as incompatible with redirect_output?


Yes, it should.

--
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 0074] spec file: Add dbus-python to BuildRequires

[Jan Cholasta]

On 14.12.2015 13:26, David Kupka wrote:

During work on ticket #5497 [0] the need for dbus-python in build time
was introduced but it was not added in spec file.

[0] https://fedorahosted.org/freeipa/ticket/5497


Thanks. Pushed to master under the one-liner rule: 
8b1002a18c42e8be45f0e03e7d3526179ef5b648


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] [PATCHES 523-525] replica install: add remote connection check over API

[Martin Basti]

On 14.12.2015 07:23, Jan Cholasta wrote:

On 11.12.2015 18:49, Tomas Babej wrote:



On 12/11/2015 05:37 PM, Martin Basti wrote:



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


I guess there's is little harm, any kinited user that was allowed to
access the machine could perform the conncheck even without these 
patches:


In the RPC check, the user must have the Replication Administrators 
privilege, which by default only admins have.


I tried to install replica with a regular user and conncheck passed.
Martin^2




# ipa-replica-conncheck --master master.ipa.test -p ran...@ipa.test -w
ratarata -a -r IPA.TEST
Check connection from replica to remote master 'master.ipa.test':
Directory Service: Unsecure port (389): OK
Directory Service: Secure port (636): OK
Kerberos KDC: TCP (88): OK
Kerberos Kpasswd: TCP (464): OK
HTTP Server: Unsecure port (80): OK
HTTP Server: Secure port (443): OK

The following list of ports use UDP protocol and would need to be
checked manually:
Kerberos KDC: UDP (88): SKIPPED
Kerberos Kpasswd: UDP (464): SKIPPED

Connection from replica to master is OK.
Start listening on required ports for remote master check
Get credentials to log in to remote master
Check SSH connection to remote master
Execute check on remote master
Check connection from master to remote replica 'replica.ipa.test':
Directory Service: Unsecure port (389): OK
Directory Service: Secure port (636): OK
Kerberos KDC: TCP (88): OK
Kerberos KDC: UDP (88): OK
Kerberos Kpasswd: TCP (464): OK
Kerberos Kpasswd: UDP (464): OK
HTTP Server: Unsecure port (80): OK
HTTP Server: Secure port (443): OK

Connection from master to replica is OK.






--
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 0376] KRA: add RA cert during replica promotion

[Martin Basti]

On 14.12.2015 11:46, David Kupka wrote:

On 14/12/15 11:00, David Kupka wrote:

On 10/12/15 19:40, Martin Basti wrote:

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

patch attached.



Hi,
thanks for the patch. It works but only when WAIT_AFTER_ARCHIVE is 
raised.

Patch attached.
IOW, your patch works for me, ACK. To let tests pass (and eventually 
fall on other issue when it appears) please include my patch.



Pushed to master: bf9a34f4cfc2c514ff53efea4ba56e2c0cb3033f

I also pushed your workaround for tests.
Pushed to:
master: 8112ac69ccf56dd98c5eb6e77ea131b4665bd1cf
ipa-4-2: ac0999ede0d665dee75446f660301fd53e69177b

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

[Freeipa-devel] [PATCH 0378] Tests: fix always true assertion

[Martin Basti]

Fixes:
/usr/lib/python2.7/site-packages/ipatests/test_cmdline/test_ipagetkeytab.py:116: 
SyntaxWarning: assertion is always true, perhaps remove parentheses?


Patch attached.
From 27fa8fb58e2163836e0e5784fee422af248a158c Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Mon, 14 Dec 2015 12:21:38 +0100
Subject: [PATCH] Tests: test_ipagetkeytab: fix assert that is always true

Fixes:
/usr/lib/python2.7/site-packages/ipatests/test_cmdline/test_ipagetkeytab.py:116:
SyntaxWarning: assertion is always true, perhaps remove parentheses?
---
 ipatests/test_cmdline/test_ipagetkeytab.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ipatests/test_cmdline/test_ipagetkeytab.py b/ipatests/test_cmdline/test_ipagetkeytab.py
index 2df9b6eb2c82fb7c4bf99ebd5177c575736eb90f..37f4e4a7aac4354610c0aae53f4c321548e253c8 100644
--- a/ipatests/test_cmdline/test_ipagetkeytab.py
+++ b/ipatests/test_cmdline/test_ipagetkeytab.py
@@ -113,8 +113,8 @@ class test_ipagetkeytab(cmdline_test):
 result = ipautil.run(new_args, None, capture_error=True)
 expected = 'Keytab successfully retrieved and stored in: %s\n' % (
 self.keytabname)
-assert (expected in result.error_output,
-'Success message not in output:\n%s' % result.error_output)
+assert expected in result.error_output, (
+'Success message not in output:\n%s' % result.error_output)
 except ipautil.CalledProcessError as e:
 assert (False)
 
-- 
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 0074] spec file: Add dbus-python to BuildRequires

[David Kupka]

During work on ticket #5497 [0] the need for dbus-python in build time 
was introduced but it was not added in spec file.


[0] https://fedorahosted.org/freeipa/ticket/5497
--
David Kupka
From 6d1f5532de420efbe5c5f251681b8e7496ecb065 Mon Sep 17 00:00:00 2001
From: David Kupka 
Date: Mon, 14 Dec 2015 12:16:33 +
Subject: [PATCH] spec file: Add dbus-python to BuildRequires

Commit 8d7f67e introduced the need for dbus-python during build time.

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

diff --git a/freeipa.spec.in b/freeipa.spec.in
index a074c37..d8d9f11 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -98,6 +98,7 @@ BuildRequires:  python-six
 BuildRequires:  python-jwcrypto
 BuildRequires:  custodia
 BuildRequires:  libini_config-devel >= 1.2.0
+BuildRequires:  dbus-python
 
 # Build dependencies for unit tests
 BuildRequires:  libcmocka-devel
-- 
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] 0748 Handle encoding for ipautil.run

[Jan Cholasta]

On 14.12.2015 10:49, Petr Viktorin wrote:

On 12/14/2015 10:29 AM, Jan Cholasta wrote:

On 9.12.2015 12:04, Petr Viktorin wrote:

On 12/04/2015 12:49 PM, Jan Cholasta wrote:

On 4.12.2015 12:43, Petr Viktorin wrote:

On 12/04/2015 12:15 PM, Jan Cholasta wrote:

On 4.12.2015 10:53, Petr Viktorin wrote:

On 12/04/2015 08:51 AM, Jan Cholasta wrote:

On 3.12.2015 15:42, Petr Viktorin wrote:

On 12/03/2015 10:55 AM, Jan Cholasta wrote:

[...]

If we do encode/decode in run(), I think there should be a way to
override the default encoding. My idea was to either accept/return
only
bytes and let the caller handle encoding themselves, or to handle
encoding in run(), but be able to override the defaults.

Given we handle encoding in run(), I would imagine it like this:

 def run(args, stdin=None, raiseonerr=True, nolog=(),
env=None,
 capture_stdout=False, skip_output=False, cwd=None,
 runas=None, timeout=None, suplementary_groups=[],
 capture_stderr=False, encode_stdout=False,
 encode_stderr=False, encoding=None):

i.e. nothing is captured by default, when stdout or stderr are
captured
they are returned as bytes by default, when stdout or stderr are
returned as text they are decoded using the locale encoding by
default,
or the encoding can be explicitly specified.

Do you think this is feasible?


Feasible, yes.
In the majority of cases where the output is needed, we want text
encoded with locale.getpreferredencoding(), so I'd rather make
that the
default rather than bytes.

Or we could make "encode_stdout" imply "capture_stdout", so you
wouldn't
have to always specify both. (It would be better named
"encoded_stdout"
in that case.)


I think it should be OK to decode by default. (IMO it would be
best to
reverse the logic and name it "raw_stdout", with False default.
Do we
need "raw_stderr" at all? I don't think we do.)


Actually, now that I think about it: if the result was a namedtuple
subclass, we can always set extra raw_stdout/raw_stderr attributes on
it. (Unless skip_output=True, of course.)

   result = run(['generate_binary_data'])
   use(result.raw_stdout)

   # and for backcompat,
   rc, _out, _err = result


Good idea.

Perhaps we should use the same names as CalledProcessError for the
attributes, i.e. "returncode", "output", and "error_output",
"raw_output", "raw_error_output" for the new attributes?


(Actually, since "returncode" is not "return_code", it should probably
be "returncode", "output", "erroroutput", "raw_output",
"raw_erroroutput".)



OK. It's also good to use different names than Popen's stdout/stderr,
which are file-like objects rather than strings. But then,
capture_stdout/capture_stderr should be
capture_output/capture_error_output.


Here is the new patch.
I also added output_log and error_log to the result to abstract the
following common pattern:

  result = run([...])
  if result.returncode != 0:
  stderr = result.error_output
  if six.PY3:
  error = stderr.encode(locale.getpreferredencoding(),
'replace')
  self.log.critical('...: %s', stderr)


1) certutil -L -r outputs raw DER cert, so this will explode in Python 3
when pem == False:

  else:
  args.append('-r')
  try:
-cert, err, returncode = self.run_certutil(args)
+result = self.run_certutil(args, capture_output=True)
  except ipautil.CalledProcessError:
  raise RuntimeError("Failed to get %s" % nickname)
-return cert
+return result.output

  def has_nickname(self, nickname):
  try:


2) There are some PEP8 transgressions:

$ git show -U0 | pep8 --diff
./ipaplatform/base/services.py:297:22: E127 continuation line
over-indented for visual indent
./ipapython/ipautil.py:279:1: E302 expected 2 blank lines, found 1
./ipapython/ipautil.py:280:40: E128 continuation line under-indented for
visual indent
./ipapython/ipautil.py:283:1: E302 expected 2 blank lines, found 1
./ipapython/ipautil.py:330:80: E501 line too long (80 > 79 characters)
./ipapython/ipautil.py:437:43: E127 continuation line over-indented for
visual indent
./ipapython/ipautil.py:442:43: E127 continuation line over-indented for
visual indent
./ipapython/kernel_keyring.py:57:11: E225 missing whitespace around
operator
./ipaserver/install/cainstance.py:626:80: E501 line too long (80 > 79
characters)
./ipaserver/install/ipa_backup.py:498:17: E128 continuation line
under-indented for visual indent
./ipaserver/install/ipa_backup.py:521:21: E122 continuation line missing
indentation or outdented
./ipaserver/install/ipa_backup.py:530:17: E122 continuation line missing
indentation or outdented
./ipaserver/install/ipa_backup.py:605:52: E225 missing whitespace around
operator
./ipaserver/install/ipa_backup.py:606:17: E122 continuation line missing
indentation or outdented
./ipaserver/install/krbinstance.py:303:80: E501 line too long (84 > 79

[Freeipa-devel] [PATCHES 531-532] server install: redirect ipa-client-install output to standard output

[Jan Cholasta]

Hi,

the attached patches fix .

Honza

--
Jan Cholasta
From 8ee7ad0e87b1df062fbf253170bfffcb7b3bfb85 Mon Sep 17 00:00:00 2001
From: Jan Cholasta 
Date: Mon, 14 Dec 2015 12:45:45 +0100
Subject: [PATCH 1/2] ipautil: allow redirecting command output to standard
 output in run()

https://fedorahosted.org/freeipa/ticket/5527
---
 ipapython/ipautil.py | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/ipapython/ipautil.py b/ipapython/ipautil.py
index 4480744..31dad34 100644
--- a/ipapython/ipautil.py
+++ b/ipapython/ipautil.py
@@ -286,7 +286,7 @@ class _RunResult(collections.namedtuple('_RunResult',
 def run(args, stdin=None, raiseonerr=True, nolog=(), env=None,
 capture_output=False, skip_output=False, cwd=None,
 runas=None, timeout=None, suplementary_groups=[],
-capture_error=False, encoding=None):
+capture_error=False, encoding=None, redirect_output=False):
 """
 Execute an external command.
 
@@ -323,6 +323,7 @@ def run(args, stdin=None, raiseonerr=True, nolog=(), env=None,
 :param encoding: For Python 3, the encoding to use for output,
 error_output, and (if it's not bytes) stdin.
 If None, the current encoding according to locale is used.
+:param redirect_output: Redirect (error) output to standard (error) output.
 
 :return: An object with these attributes:
 
@@ -362,6 +363,10 @@ def run(args, stdin=None, raiseonerr=True, nolog=(), env=None,
 raise ValueError('skip_output is incompatible with '
  'capture_output or capture_error')
 
+if redirect_output and (capture_output or capture_error):
+raise ValueError('redirect_output is incompatible with '
+ 'capture_output or capture_error')
+
 if env is None:
 # copy default env
 env = copy.deepcopy(os.environ)
@@ -370,6 +375,9 @@ def run(args, stdin=None, raiseonerr=True, nolog=(), env=None,
 p_in = subprocess.PIPE
 if skip_output:
 p_out = p_err = open(paths.DEV_NULL, 'w')
+elif redirect_output:
+p_out = sys.stdout
+p_err = sys.stderr
 else:
 p_out = subprocess.PIPE
 p_err = subprocess.PIPE
@@ -432,7 +440,7 @@ def run(args, stdin=None, raiseonerr=True, nolog=(), env=None,
 
 # The command and its output may include passwords that we don't want
 # to log. Replace those.
-if skip_output:
+if skip_output or redirect_output:
 output_log = None
 error_log = None
 else:
-- 
2.4.3

From d75712df78099d71d3cf245f96bac35c008c3beb Mon Sep 17 00:00:00 2001
From: Jan Cholasta 
Date: Mon, 14 Dec 2015 12:46:48 +0100
Subject: [PATCH 2/2] server install: redirect ipa-client-install output to
 standard output

https://fedorahosted.org/freeipa/ticket/5527
---
 ipaserver/install/server/install.py| 12 +---
 ipaserver/install/server/replicainstall.py | 14 ++
 2 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/ipaserver/install/server/install.py b/ipaserver/install/server/install.py
index 540ce20..1791832 100644
--- a/ipaserver/install/server/install.py
+++ b/ipaserver/install/server/install.py
@@ -991,10 +991,9 @@ def install(installer):
 args.append("--no-sshd")
 if options.mkhomedir:
 args.append("--mkhomedir")
-run(args)
+run(args, redirect_output=True)
 except Exception as e:
-sys.exit("Configuration of client side components failed!\n"
- "ipa-client-install returned: " + str(e))
+sys.exit("Configuration of client side components failed!")
 
 # Everything installed properly, activate ipa service.
 services.knownservices.ipa.enable()
@@ -1251,13 +1250,12 @@ def uninstall(installer):
 try:
 result = run([paths.IPA_CLIENT_INSTALL, "--on-master",
   "--unattended", "--uninstall"],
- raiseonerr=False, capture_output=True)
+ raiseonerr=False, redirect_output=True)
 if result.returncode not in [0, 2]:
-raise RuntimeError(result.output)
-except Exception as e:
+raise RuntimeError("Failed to configure the client")
+except Exception:
 rv = 1
 print("Uninstall of client side components failed!")
-print("ipa-client-install returned: " + str(e))
 
 sys.exit(rv)
 
diff --git a/ipaserver/install/server/replicainstall.py b/ipaserver/install/server/replicainstall.py
index 311f0e5..6d2589f 100644
--- a/ipaserver/install/server/replicainstall.py
+++ b/ipaserver/install/server/replicainstall.py
@@ -409,7 +409,7 @@ def uninstall_client():
 
 print("Removing client side components")
 ipautil.run([paths.IPA_CLIENT_INSTALL, "--unattended", "--uninstall"],
-raiseonerr=False)
+raiseonerr=False, redirect_output=True)
 
 
 def 

Re: [Freeipa-devel] [PATCH] 0748 Handle encoding for ipautil.run

[Jan Cholasta]

On 9.12.2015 12:04, Petr Viktorin wrote:

On 12/04/2015 12:49 PM, Jan Cholasta wrote:

On 4.12.2015 12:43, Petr Viktorin wrote:

On 12/04/2015 12:15 PM, Jan Cholasta wrote:

On 4.12.2015 10:53, Petr Viktorin wrote:

On 12/04/2015 08:51 AM, Jan Cholasta wrote:

On 3.12.2015 15:42, Petr Viktorin wrote:

On 12/03/2015 10:55 AM, Jan Cholasta wrote:

[...]

If we do encode/decode in run(), I think there should be a way to
override the default encoding. My idea was to either accept/return
only
bytes and let the caller handle encoding themselves, or to handle
encoding in run(), but be able to override the defaults.

Given we handle encoding in run(), I would imagine it like this:

def run(args, stdin=None, raiseonerr=True, nolog=(),
env=None,
capture_stdout=False, skip_output=False, cwd=None,
runas=None, timeout=None, suplementary_groups=[],
capture_stderr=False, encode_stdout=False,
encode_stderr=False, encoding=None):

i.e. nothing is captured by default, when stdout or stderr are
captured
they are returned as bytes by default, when stdout or stderr are
returned as text they are decoded using the locale encoding by
default,
or the encoding can be explicitly specified.

Do you think this is feasible?


Feasible, yes.
In the majority of cases where the output is needed, we want text
encoded with locale.getpreferredencoding(), so I'd rather make
that the
default rather than bytes.

Or we could make "encode_stdout" imply "capture_stdout", so you
wouldn't
have to always specify both. (It would be better named
"encoded_stdout"
in that case.)


I think it should be OK to decode by default. (IMO it would be best to
reverse the logic and name it "raw_stdout", with False default. Do we
need "raw_stderr" at all? I don't think we do.)


Actually, now that I think about it: if the result was a namedtuple
subclass, we can always set extra raw_stdout/raw_stderr attributes on
it. (Unless skip_output=True, of course.)

  result = run(['generate_binary_data'])
  use(result.raw_stdout)

  # and for backcompat,
  rc, _out, _err = result


Good idea.

Perhaps we should use the same names as CalledProcessError for the
attributes, i.e. "returncode", "output", and "error_output",
"raw_output", "raw_error_output" for the new attributes?


(Actually, since "returncode" is not "return_code", it should probably
be "returncode", "output", "erroroutput", "raw_output", "raw_erroroutput".)



OK. It's also good to use different names than Popen's stdout/stderr,
which are file-like objects rather than strings. But then,
capture_stdout/capture_stderr should be
capture_output/capture_error_output.


Here is the new patch.
I also added output_log and error_log to the result to abstract the
following common pattern:

 result = run([...])
 if result.returncode != 0:
 stderr = result.error_output
 if six.PY3:
 error = stderr.encode(locale.getpreferredencoding(), 'replace')
 self.log.critical('...: %s', stderr)


1) certutil -L -r outputs raw DER cert, so this will explode in Python 3 
when pem == False:


 else:
 args.append('-r')
 try:
-cert, err, returncode = self.run_certutil(args)
+result = self.run_certutil(args, capture_output=True)
 except ipautil.CalledProcessError:
 raise RuntimeError("Failed to get %s" % nickname)
-return cert
+return result.output

 def has_nickname(self, nickname):
 try:


2) There are some PEP8 transgressions:

$ git show -U0 | pep8 --diff
./ipaplatform/base/services.py:297:22: E127 continuation line 
over-indented for visual indent

./ipapython/ipautil.py:279:1: E302 expected 2 blank lines, found 1
./ipapython/ipautil.py:280:40: E128 continuation line under-indented for 
visual indent

./ipapython/ipautil.py:283:1: E302 expected 2 blank lines, found 1
./ipapython/ipautil.py:330:80: E501 line too long (80 > 79 characters)
./ipapython/ipautil.py:437:43: E127 continuation line over-indented for 
visual indent
./ipapython/ipautil.py:442:43: E127 continuation line over-indented for 
visual indent

./ipapython/kernel_keyring.py:57:11: E225 missing whitespace around operator
./ipaserver/install/cainstance.py:626:80: E501 line too long (80 > 79 
characters)
./ipaserver/install/ipa_backup.py:498:17: E128 continuation line 
under-indented for visual indent
./ipaserver/install/ipa_backup.py:521:21: E122 continuation line missing 
indentation or outdented
./ipaserver/install/ipa_backup.py:530:17: E122 continuation line missing 
indentation or outdented
./ipaserver/install/ipa_backup.py:605:52: E225 missing whitespace around 
operator
./ipaserver/install/ipa_backup.py:606:17: E122 continuation line missing 
indentation or outdented
./ipaserver/install/krbinstance.py:303:80: E501 line too long (84 > 79 
characters)
./ipatests/test_ipapython/test_keyring.py:97:33: E222 multiple spaces 
after operator

[Freeipa-devel] [PATCH 533] replica promotion: notify user about ignoring client enrollment options

[Jan Cholasta]

Hi,

the attached patch fixes .

Honza

--
Jan Cholasta
From 484f68304bd4748313c50be35b1667694e07f00b Mon Sep 17 00:00:00 2001
From: Jan Cholasta 
Date: Mon, 14 Dec 2015 13:30:33 +0100
Subject: [PATCH] replica promotion: notify user about ignoring client
 enrollment options

When IPA client is already installed, notify the user that the enrollment
options are ignored in ipa-replica-install.

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

diff --git a/ipaserver/install/server/replicainstall.py b/ipaserver/install/server/replicainstall.py
index 1d5b528..572af75 100644
--- a/ipaserver/install/server/replicainstall.py
+++ b/ipaserver/install/server/replicainstall.py
@@ -883,6 +883,11 @@ def promote_check(installer):
 client_fstore = sysrestore.FileStore(paths.IPA_CLIENT_SYSRESTORE)
 if not client_fstore.has_files():
 ensure_enrolled(installer)
+else:
+if (options.server or options.domain or options.password or
+options.keytab):
+print("IPA client is already configured on this system, ignoring "
+  "the --server, --domain, --password and --keytab options.")
 
 sstore = sysrestore.StateFile(paths.SYSRESTORE)
 
-- 
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

[Jan Cholasta]

On 14.12.2015 10:27, Martin Basti wrote:



On 14.12.2015 07:23, Jan Cholasta wrote:

On 11.12.2015 18:49, Tomas Babej wrote:



On 12/11/2015 05:37 PM, Martin Basti wrote:



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


I guess there's is little harm, any kinited user that was allowed to
access the machine could perform the conncheck even without these
patches:


In the RPC check, the user must have the Replication Administrators
privilege, which by default only admins have.


I tried to install replica with a regular user and conncheck passed.
Martin^2


See patch 525.

--
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] 0748 Handle encoding for ipautil.run

[Petr Viktorin]

On 12/14/2015 10:29 AM, Jan Cholasta wrote:
> On 9.12.2015 12:04, Petr Viktorin wrote:
>> On 12/04/2015 12:49 PM, Jan Cholasta wrote:
>>> On 4.12.2015 12:43, Petr Viktorin wrote:
 On 12/04/2015 12:15 PM, Jan Cholasta wrote:
> On 4.12.2015 10:53, Petr Viktorin wrote:
>> On 12/04/2015 08:51 AM, Jan Cholasta wrote:
>>> On 3.12.2015 15:42, Petr Viktorin wrote:
 On 12/03/2015 10:55 AM, Jan Cholasta wrote:
 [...]
> If we do encode/decode in run(), I think there should be a way to
> override the default encoding. My idea was to either accept/return
> only
> bytes and let the caller handle encoding themselves, or to handle
> encoding in run(), but be able to override the defaults.
>
> Given we handle encoding in run(), I would imagine it like this:
>
> def run(args, stdin=None, raiseonerr=True, nolog=(),
> env=None,
> capture_stdout=False, skip_output=False, cwd=None,
> runas=None, timeout=None, suplementary_groups=[],
> capture_stderr=False, encode_stdout=False,
> encode_stderr=False, encoding=None):
>
> i.e. nothing is captured by default, when stdout or stderr are
> captured
> they are returned as bytes by default, when stdout or stderr are
> returned as text they are decoded using the locale encoding by
> default,
> or the encoding can be explicitly specified.
>
> Do you think this is feasible?

 Feasible, yes.
 In the majority of cases where the output is needed, we want text
 encoded with locale.getpreferredencoding(), so I'd rather make
 that the
 default rather than bytes.

 Or we could make "encode_stdout" imply "capture_stdout", so you
 wouldn't
 have to always specify both. (It would be better named
 "encoded_stdout"
 in that case.)
>>>
>>> I think it should be OK to decode by default. (IMO it would be
>>> best to
>>> reverse the logic and name it "raw_stdout", with False default.
>>> Do we
>>> need "raw_stderr" at all? I don't think we do.)
>>
>> Actually, now that I think about it: if the result was a namedtuple
>> subclass, we can always set extra raw_stdout/raw_stderr attributes on
>> it. (Unless skip_output=True, of course.)
>>
>>   result = run(['generate_binary_data'])
>>   use(result.raw_stdout)
>>
>>   # and for backcompat,
>>   rc, _out, _err = result
>
> Good idea.
>
> Perhaps we should use the same names as CalledProcessError for the
> attributes, i.e. "returncode", "output", and "error_output",
> "raw_output", "raw_error_output" for the new attributes?
>>>
>>> (Actually, since "returncode" is not "return_code", it should probably
>>> be "returncode", "output", "erroroutput", "raw_output",
>>> "raw_erroroutput".)
>>>

 OK. It's also good to use different names than Popen's stdout/stderr,
 which are file-like objects rather than strings. But then,
 capture_stdout/capture_stderr should be
 capture_output/capture_error_output.
>>
>> Here is the new patch.
>> I also added output_log and error_log to the result to abstract the
>> following common pattern:
>>
>>  result = run([...])
>>  if result.returncode != 0:
>>  stderr = result.error_output
>>  if six.PY3:
>>  error = stderr.encode(locale.getpreferredencoding(),
>> 'replace')
>>  self.log.critical('...: %s', stderr)
> 
> 1) certutil -L -r outputs raw DER cert, so this will explode in Python 3
> when pem == False:
> 
>  else:
>  args.append('-r')
>  try:
> -cert, err, returncode = self.run_certutil(args)
> +result = self.run_certutil(args, capture_output=True)
>  except ipautil.CalledProcessError:
>  raise RuntimeError("Failed to get %s" % nickname)
> -return cert
> +return result.output
> 
>  def has_nickname(self, nickname):
>  try:
> 
> 
> 2) There are some PEP8 transgressions:
> 
> $ git show -U0 | pep8 --diff
> ./ipaplatform/base/services.py:297:22: E127 continuation line
> over-indented for visual indent
> ./ipapython/ipautil.py:279:1: E302 expected 2 blank lines, found 1
> ./ipapython/ipautil.py:280:40: E128 continuation line under-indented for
> visual indent
> ./ipapython/ipautil.py:283:1: E302 expected 2 blank lines, found 1
> ./ipapython/ipautil.py:330:80: E501 line too long (80 > 79 characters)
> ./ipapython/ipautil.py:437:43: E127 continuation line over-indented for
> visual indent
> ./ipapython/ipautil.py:442:43: E127 continuation line over-indented for
> visual indent
> ./ipapython/kernel_keyring.py:57:11: E225 missing whitespace around
> operator
> ./ipaserver/install/cainstance.py:626:80: 

Re: [Freeipa-devel] [PATCH 0376] KRA: add RA cert during replica promotion

[David Kupka]

On 10/12/15 19:40, Martin Basti wrote:

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

patch attached.



Hi,
thanks for the patch. It works but only when WAIT_AFTER_ARCHIVE is raised.
Patch attached.
--
David Kupka
From a209343652b8bedfcbca83c7eafc699e72c0a261 Mon Sep 17 00:00:00 2001
From: David Kupka 
Date: Mon, 14 Dec 2015 10:52:44 +0100
Subject: [PATCH] test: Temporarily increase timeout in vault test.

Remove this change when vault is fixed.
---
 ipatests/test_integration/test_vault.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ipatests/test_integration/test_vault.py b/ipatests/test_integration/test_vault.py
index 74b554eb283278940e6ed0a93596ed194eadadcb..3b717c9cdfda30dd230d31730328cd3aa4cbdd49 100644
--- a/ipatests/test_integration/test_vault.py
+++ b/ipatests/test_integration/test_vault.py
@@ -7,7 +7,7 @@ import time
 from ipatests.test_integration.base import IntegrationTest
 from ipatests.test_integration import tasks
 
-WAIT_AFTER_ARCHIVE = 30  # give some time to replication
+WAIT_AFTER_ARCHIVE = 90  # give some time to replication
 
 
 class TestInstallKRA(IntegrationTest):
-- 
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] [PATCHES 531-532] server install: redirect ipa-client-install output to standard output

[Tomas Babej]

On 12/14/2015 12:52 PM, Jan Cholasta wrote:
> Hi,
> 
> the attached patches fix .
> 
> Honza
> 
> 
> 

Shouldn't skip_output be also marked as incompatible with redirect_output?

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 0117] ipa-client-install: create a temporary directory for ccache files

[Jan Cholasta]

On 14.12.2015 18:51, Tomas Babej wrote:



On 12/14/2015 05:31 PM, Martin Babinsky wrote:

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


Works as expected, code-wise looks good.

Thanks for looking into this, ACK!

Pushed to master: 5886f87f974fa508047a21350c2e6e75a3001da6


It would probably be better if ipa-client-install used 
ipautil.private_ccache(), but I guess we can fix that later, once we 
start digging into ipa-client-install componentization / rectofaring.


--
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] Testing FreeIPA 4.3 for GA

[Jan Cholasta]

On 15.12.2015 07:24, Lukas Slebodnik wrote:

On (15/12/15 01:05), Petr Vobornik wrote:

Blocking patches for FreeIPA 4.3 were pushed, ipa-4-3 branch was created.
Master branch is ready for 4.4 development.

A build is available for testing in my pvoborni/freeipa-4-3 COPR repo [1]
until the official 4.3 repo is created. The repo contains only this build.
The build is not pure upstream ipa-4-3 branch but rather a build which will
go to Fedora rawhide and official 4.3 copr repo - Fedora downstream spec with
the SELinux workaround applied [2][3].

Known issues:
* https://fedorahosted.org/freeipa/ticket/5469 - requires fix in PKI
* https://bugzilla.redhat.com/show_bug.cgi?id=1289930 - SELinux Policy update
needed for connection check


I would suggest to do small change in packaging.

sh$ rpm -qp --requires ./freeipa-client-4.3.0-1.fc23.x86_64.rpm | grep pam
pam_krb5

pam_krb5 is not used by default. I will suggest to use weak dependencies here
If you do not want to remove it completely.
http://rpm.org/wiki/PackagerDocs/Dependencies#Weakdependencies


I would rather remove it completely, along with nss_ldap and 
pam-nss-ldapd support.


--
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 0365] Remove unused KRA code from ipa-server-install

[David Kupka]

On 14/12/15 16:54, Alexander Bokovoy wrote:

On Mon, 14 Dec 2015, David Kupka wrote:

On 14/12/15 15:05, Alexander Bokovoy wrote:

On Mon, 14 Dec 2015, David Kupka wrote:

On 30/11/15 16:31, Martin Basti wrote:

First instance of KRA should be installed only by ipa-kra-install

Patch attached.





Hi,
patch works, but I don't like the approach.

Do we really want to remove '--setup-kra' option from
ipa-server-install?
Why do we remove '--setup-kra' while keeping '--setup-dns'? Why do we
keep installing PKI CA when it can be also installed later?

Yes, we do want. Adding the option was an error in the first place. This
patch fixes the error.


I would love if 'ipa-server-install' installed only the bare minimum
needed and then other features was added using ipa-{feature}-install
but also I don't mind one almighty installer that can install all
possible combinations of features.
But this is neither of it. This just brings another inconsistency into
FreeIPA behavior.

We don't have --with-adtrust either. And we don't want it.


Ok, then are we aiming for 'ipa-server-install' that only installs the
bare minimum and everything else should be installed later?

Or, do we decide per feature if it's appropriate to include it in
'ipa-server-install'? What are the criteria in this case?

Per feature decision is a bit better description of an ad-hoc process we
have so far.

As we had this discussion with Simo today, let me quickly capture
arguments for both. We typically allow options in ipa-server-install for
features that can be present and used very early. Certificates are
issued early in the process, DNS records are critical to exist before
hosts can be added and can start using Kerberos and so on. However,
trust to AD cannot be established to 'just deployed' FreeIPA realm, you
need to pre-configure your and that remote realm.

Certificates were in particular important part of the story before we
added support for GSSAPI authentication between replicas (4.3 release is
going to have it). This meant we were constrained by the fact that some
entity had to issue certificates before we even create a replica.

I was arguing that having KRA would require us to have full fledged CA
as we depend on ability to issue certificates for KRA feature to be
useful as well. While standard CA is somewhat optional in the case only
end-point service certs are needed (Let's Encrypt is a solution there),
having KRA means use of some CA. So to me having KRA means we have CA
already, why not to simply merge the two setting into --setup-ca at all?

Separate installers also were used in past as a gap-bridging tool to
allow people to upgrade their old deployments and gain new
functionality. So separate installer has another function than just
deploying a service.

Having --with-kra option thus makes less sense. We can consider KRA
functionality to be in a default feature set at some point, that would
still require us to have a separate installer, ipa-kra-install, to allow
extending old deployments to provide new feature.



The fact that the DNS records need to exist for some (core) IPA 
functionality does not necessary mean that we need DNS server installed 
as a part of FreeIPA. We even support DNS-less install and user is then 
responsible for maintaining DNS records in server of his choice.
IOW 'ipa-server-install --setup-dns' results in the same as 
'ipa-server-install && ipa-dns-install'.
In case of trust, would it be really impossible to preconfigure the 
remote realm and then provide all necessary information to 
'ipa-server-install --setup-trust'? Or is it just the way we're doing it 
right now for some maybe really good reason?
So the only reason to add the '--setup-' or '--with-' 
to ipa-server-install I can see is user comfort and ease of 
installation. That's good reason for me but in that case I still do not 
understand why do not include options that can be easily included.

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

[Martin Basti]

On 14.12.2015 09:24, Martin Kosek wrote:

On 12/14/2015 07:21 AM, Jan Cholasta wrote:

On 11.12.2015 19:01, Tomas Babej wrote:


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.

I would not be too worried about miliseconds longer execution on a function
called during RPM upgrade.


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

+1, also a single function is hardly "much code".

Ok then. If you all want to add yet another N-V-R parsing function in the
FreeIPA code, I can live with it (with raised eyebrows though).


Rebased patch attached.
From c15bb5a71a8a1fb065b4ffdaee0de0554569ec9f 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 |  1 +
 ipaplatform/redhat/tasks.py | 53 +
 2 files changed, 54 insertions(+)

diff --git a/freeipa.spec.in b/freeipa.spec.in
index 09bd62a4f8ad7fd739d7422750bc0054a3afef9d..4002666df1952af59115da7f36386afc580f2ff4 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -204,6 +204,7 @@ Requires: python-pyasn1
 Requires: dbus-python
 Requires: python-dns >= 1.11.1
 Requires: python-kdcproxy >= 0.3
+Requires: rpm-python
 
 %description -n python2-ipaserver
 IPA is an integrated solution to provide centrally managed Identity (users,
diff --git a/ipaplatform/redhat/tasks.py b/ipaplatform/redhat/tasks.py
index 94d2cb4e906965a20bcfdd55f38854005091c26f..c90e55f28cd492d43a98e4e0ee0476a23db8a099 100644
--- a/ipaplatform/redhat/tasks.py
+++ b/ipaplatform/redhat/tasks.py
@@ -30,11 +30,13 @@ import stat
 import socket
 import sys
 import base64
+from functools import total_ordering
 
 from subprocess import CalledProcessError
 from nss.error import NSPRError
 from pyasn1.error import PyAsn1Error
 from six.moves import urllib
+import rpm
 
 from ipapython.ipa_log_manager import root_logger, log_mgr
 from ipapython import ipautil
@@ -47,6 +49,35 @@ from ipaplatform.redhat.authconfig import RedHatAuthConfig
 from 

[Freeipa-devel] [PATCH 0088] Don't error when find_base() fails if a base is not required

[Nathaniel McCallum]

We always have to call find_base() in order to force libldap to open
the socket. However, if no base is actually required then there is
no reason to error out if find_base() fails. This condition can arise
when anonymous binds are disabled.From 7cb7a7da4271101b7ad089d90716a27dd2041c0d Mon Sep 17 00:00:00 2001
From: Nathaniel McCallum 
Date: Mon, 14 Dec 2015 10:12:26 -0500
Subject: [PATCH] Don't error when find_base() fails if a base is not required

We always have to call find_base() in order to force libldap to open
the socket. However, if no base is actually required then there is
no reason to error out if find_base() fails. This condition can arise
when anonymous binds are disabled.
---
 daemons/ipa-otpd/main.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/daemons/ipa-otpd/main.c b/daemons/ipa-otpd/main.c
index a5d1f93ff06783d9139582aba51587f8f6641c29..aebc039bc05e5ddd04de2c0647a1cdb851a1b697 100644
--- a/daemons/ipa-otpd/main.c
+++ b/daemons/ipa-otpd/main.c
@@ -175,12 +175,13 @@ static krb5_error_code setup_ldap(const char *uri, krb5_boolean bind,
 
 /* Always find the base since this forces open the socket. */
 basetmp = find_base(ldp);
-if (basetmp == NULL)
-return ENOTCONN;
-if (base != NULL)
+if (base != NULL) {
+if (basetmp == NULL)
+return ENOTCONN;
 *base = basetmp;
-else
+} else {
 free(basetmp);
+}
 
 /* Set default timeout to just return immediately for async requests. */
 memset(, 0, sizeof(timeout));
-- 
2.6.4

-- 
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 14/12/15 15:25, David Kupka wrote:

On 14/12/15 14:52, David Kupka wrote:

On 11/12/15 15:00, Petr Spacek wrote:

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
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():
   

Re: [Freeipa-devel] [PATCH 0365] Remove unused KRA code from ipa-server-install

[Alexander Bokovoy]

On Mon, 14 Dec 2015, David Kupka wrote:

On 14/12/15 15:05, Alexander Bokovoy wrote:

On Mon, 14 Dec 2015, David Kupka wrote:

On 30/11/15 16:31, Martin Basti wrote:

First instance of KRA should be installed only by ipa-kra-install

Patch attached.





Hi,
patch works, but I don't like the approach.

Do we really want to remove '--setup-kra' option from ipa-server-install?
Why do we remove '--setup-kra' while keeping '--setup-dns'? Why do we
keep installing PKI CA when it can be also installed later?

Yes, we do want. Adding the option was an error in the first place. This
patch fixes the error.


I would love if 'ipa-server-install' installed only the bare minimum
needed and then other features was added using ipa-{feature}-install
but also I don't mind one almighty installer that can install all
possible combinations of features.
But this is neither of it. This just brings another inconsistency into
FreeIPA behavior.

We don't have --with-adtrust either. And we don't want it.

Ok, then are we aiming for 'ipa-server-install' that only installs the 
bare minimum and everything else should be installed later?


Or, do we decide per feature if it's appropriate to include it in 
'ipa-server-install'? What are the criteria in this case?

Per feature decision is a bit better description of an ad-hoc process we
have so far.

As we had this discussion with Simo today, let me quickly capture
arguments for both. We typically allow options in ipa-server-install for
features that can be present and used very early. Certificates are
issued early in the process, DNS records are critical to exist before
hosts can be added and can start using Kerberos and so on. However,
trust to AD cannot be established to 'just deployed' FreeIPA realm, you
need to pre-configure your and that remote realm.

Certificates were in particular important part of the story before we
added support for GSSAPI authentication between replicas (4.3 release is
going to have it). This meant we were constrained by the fact that some
entity had to issue certificates before we even create a replica.

I was arguing that having KRA would require us to have full fledged CA
as we depend on ability to issue certificates for KRA feature to be
useful as well. While standard CA is somewhat optional in the case only
end-point service certs are needed (Let's Encrypt is a solution there),
having KRA means use of some CA. So to me having KRA means we have CA
already, why not to simply merge the two setting into --setup-ca at all?

Separate installers also were used in past as a gap-bridging tool to
allow people to upgrade their old deployments and gain new
functionality. So separate installer has another function than just
deploying a service.

Having --with-kra option thus makes less sense. We can consider KRA
functionality to be in a default feature set at some point, that would
still require us to have a separate installer, ipa-kra-install, to allow
extending old deployments to provide new feature.

--
/ Alexander Bokovoy

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

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

[Petr Spacek]

On 14.12.2015 16:31, David Kupka wrote:
> On 14/12/15 15:25, David Kupka wrote:
>> On 14/12/15 14:52, David Kupka wrote:
>>> On 11/12/15 15:00, Petr Spacek wrote:
 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

[Freeipa-devel] certmonger everywhere

[Jan Cholasta]

Hi,

recently I and David discussed the direction of installers with regard 
to requesting certificates. Currently there are four (!) different ways 
of requesting certificates in the installer [1][2][3][4]. We would like 
to reduce it to one.


Since all the certificates are tracked by certmonger and certmonger 
already knows how to request certificates from Dogtag (and other CAs), 
we believe that all certificates should be requested using certmonger.


Taking our meditation further, we thought "Why not use certmonger for 
the cert-request command as well?" What is the benefit, do you ask?


 a) single code path for requesting certificates (seriously, the 
current state is ridiculous)


 b) use any CA supported by certmonger as the IPA CA (i.e. Let's 
Encrypt [5], once certmonger gains support for it)


 c) automate external CA install, using any CA supported by certmonger [6]

 d) support multiple different CAs at once (generalization of the 
Sub-CA feature)


 e) uniform configuration on clients (configure once, use forever, even 
for CA-less)


The idea is to store configuration for the different CAs in LDAP and 
have cert-request redirect requests to a proper CA helper according to 
that configuration. This would require a new certmonger D-Bus method to 
call a CA helper without associated certificate storage, but that should 
be rather easy to add. In return, it would be possible to do all of the 
above.


Note that this should not conflict with tighter integration with Dogtag 
(profiles, ACLs, etc.).


Comments are welcome.

Honza

[1] 

[2] 

[3] 

[4] 


[5] 
[6] 

--
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 0365] Remove unused KRA code from ipa-server-install

[Jan Cholasta]

On 14.12.2015 16:54, Alexander Bokovoy wrote:

On Mon, 14 Dec 2015, David Kupka wrote:

On 14/12/15 15:05, Alexander Bokovoy wrote:

On Mon, 14 Dec 2015, David Kupka wrote:

On 30/11/15 16:31, Martin Basti wrote:

First instance of KRA should be installed only by ipa-kra-install

Patch attached.





Hi,
patch works, but I don't like the approach.

Do we really want to remove '--setup-kra' option from
ipa-server-install?
Why do we remove '--setup-kra' while keeping '--setup-dns'? Why do we
keep installing PKI CA when it can be also installed later?

Yes, we do want. Adding the option was an error in the first place. This
patch fixes the error.


I would love if 'ipa-server-install' installed only the bare minimum
needed and then other features was added using ipa-{feature}-install
but also I don't mind one almighty installer that can install all
possible combinations of features.
But this is neither of it. This just brings another inconsistency into
FreeIPA behavior.

We don't have --with-adtrust either. And we don't want it.


Ok, then are we aiming for 'ipa-server-install' that only installs the
bare minimum and everything else should be installed later?

Or, do we decide per feature if it's appropriate to include it in
'ipa-server-install'? What are the criteria in this case?

Per feature decision is a bit better description of an ad-hoc process we
have so far.

As we had this discussion with Simo today, let me quickly capture
arguments for both. We typically allow options in ipa-server-install for
features that can be present and used very early. Certificates are
issued early in the process, DNS records are critical to exist before
hosts can be added and can start using Kerberos and so on. However,
trust to AD cannot be established to 'just deployed' FreeIPA realm, you
need to pre-configure your and that remote realm.

Certificates were in particular important part of the story before we
added support for GSSAPI authentication between replicas (4.3 release is
going to have it). This meant we were constrained by the fact that some
entity had to issue certificates before we even create a replica.

I was arguing that having KRA would require us to have full fledged CA
as we depend on ability to issue certificates for KRA feature to be
useful as well. While standard CA is somewhat optional in the case only
end-point service certs are needed (Let's Encrypt is a solution there),
having KRA means use of some CA. So to me having KRA means we have CA
already, why not to simply merge the two setting into --setup-ca at all?


Because KRA is a separate component that does something completely 
different than a CA and is configured separately even in Dogtag. The 
logic here is exactly the reverse - if you want to install KRA, you have 
to install (Dogtag) CA, not the other way around, so merging both into 
--setup-ca does not make sense.




Separate installers also were used in past as a gap-bridging tool to
allow people to upgrade their old deployments and gain new
functionality. So separate installer has another function than just
deploying a service.

Having --with-kra option thus makes less sense. We can consider KRA
functionality to be in a default feature set at some point, that would
still require us to have a separate installer, ipa-kra-install, to allow
extending old deployments to provide new feature.


We can always have both. With the new installer framework it is trivial 
to fold installers like this without code duplication. It is still work 
in progress though, so I would prefer not to add --setup-kra to 
ipa-server-install now (if ever).


--
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 0117] ipa-client-install: create a temporary directory for ccache files

[Martin Babinsky]

On 12/15/2015 07:19 AM, Jan Cholasta wrote:

On 14.12.2015 18:51, Tomas Babej wrote:



On 12/14/2015 05:31 PM, Martin Babinsky wrote:

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


Works as expected, code-wise looks good.

Thanks for looking into this, ACK!

Pushed to master: 5886f87f974fa508047a21350c2e6e75a3001da6


It would probably be better if ipa-client-install used
ipautil.private_ccache(), but I guess we can fix that later, once we
start digging into ipa-client-install componentization / rectofaring.

Yes since that would probably require heroic amounts of spaghetti 
untangling to get it right. Anyway we have any milestone for ipa-client 
refactoring? Is it a part of 4.4 installer redesign effort?


--
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] Testing FreeIPA 4.3 for GA

[Lukas Slebodnik]

On (15/12/15 01:05), Petr Vobornik wrote:
>Blocking patches for FreeIPA 4.3 were pushed, ipa-4-3 branch was created.
>Master branch is ready for 4.4 development.
>
>A build is available for testing in my pvoborni/freeipa-4-3 COPR repo [1]
>until the official 4.3 repo is created. The repo contains only this build.
>The build is not pure upstream ipa-4-3 branch but rather a build which will
>go to Fedora rawhide and official 4.3 copr repo - Fedora downstream spec with
>the SELinux workaround applied [2][3].
>
>Known issues:
>* https://fedorahosted.org/freeipa/ticket/5469 - requires fix in PKI
>* https://bugzilla.redhat.com/show_bug.cgi?id=1289930 - SELinux Policy update
>needed for connection check
>
I would suggest to do small change in packaging.

sh$ rpm -qp --requires ./freeipa-client-4.3.0-1.fc23.x86_64.rpm | grep pam
pam_krb5

pam_krb5 is not used by default. I will suggest to use weak dependencies here
If you do not want to remove it completely.
http://rpm.org/wiki/PackagerDocs/Dependencies#Weakdependencies


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] Testing FreeIPA 4.3 for GA

[Lukas Slebodnik]

On (15/12/15 01:05), Petr Vobornik wrote:
>Blocking patches for FreeIPA 4.3 were pushed, ipa-4-3 branch was created.
>Master branch is ready for 4.4 development.
>
>A build is available for testing in my pvoborni/freeipa-4-3 COPR repo [1]
>until the official 4.3 repo is created. The repo contains only this build.
>The build is not pure upstream ipa-4-3 branch but rather a build which will
>go to Fedora rawhide and official 4.3 copr repo - Fedora downstream spec with
>the SELinux workaround applied [2][3].
>
>Known issues:
>* https://fedorahosted.org/freeipa/ticket/5469 - requires fix in PKI
>* https://bugzilla.redhat.com/show_bug.cgi?id=1289930 - SELinux Policy update
>needed for connection check
>
>I have started drafting release page [4].
>
It looks like there is a bug in dnf, because it cannot install
freeipa-tests package; which was renamed to python2-ipatests.

[root@3a8359f9e1cd ~]# rpm -q freeipa-server
freeipa-server-4.3.0-1.fc23.x86_64


[root@3a8359f9e1cd ~]# dnf install -y --best freeipa-tests
Last metadata expiration check performed 0:09:23 ago on Tue Dec 15 07:16:26
2015.
Error: package freeipa-tests-4.2.3-1.1.fc23.x86_64 requires freeipa-client =
4.2.3-1.1.fc23, but none of the providers can be installed.
package freeipa-tests-4.2.2-1.fc23.x86_64 requires freeipa-python =
4.2.2-1.fc23, but none of the providers can be installed
(try to add '--allowerasing' to command line to replace conflicting packages)


[root@3a8359f9e1cd ~]# rpm -qp --provides
./python2-ipatests-4.3.0-1.fc23.noarch.rpm
freeipa-tests(x86-64) = 4.3.0-1.fc23
ipa-tests(x86-64) = 4.3.0
python-ipatests = 4.3.0-1.fc23
python2-ipatests = 4.3.0-1.fc23


FYI: It works with yum-deprecated.

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 0365] Remove unused KRA code from ipa-server-install

[David Kupka]

On 30/11/15 16:31, Martin Basti wrote:

First instance of KRA should be installed only by ipa-kra-install

Patch attached.





Hi,
patch works, but I don't like the approach.

Do we really want to remove '--setup-kra' option from ipa-server-install?
Why do we remove '--setup-kra' while keeping '--setup-dns'? Why do we 
keep installing PKI CA when it can be also installed later?


I would love if 'ipa-server-install' installed only the bare minimum 
needed and then other features was added using ipa-{feature}-install but 
also I don't mind one almighty installer that can install all possible 
combinations of features.
But this is neither of it. This just brings another inconsistency into 
FreeIPA behavior.


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

[Oleg Fayans]

Hi Martin,

On 12/11/2015 05:58 PM, Martin Basti wrote:
> 
> 
> On 11.12.2015 17:28, Oleg Fayans wrote:
>> +myre = re.compile(".*Backed up to (?P.*?)\n.*")
> 
> IMO this regexp is not good.
> 
> 1)
> please name it better than "myre"

Done

> 
> 2)
> initial '.*' is not needed because regexp does not start with '^' and
> you use search() later
> 
> 3)
> 
> trailing '.*' is not needed as well, because it does not end with '$'
> 
> 4)
> You can use re.MULTILINE that will parse string per lines
> 
> path_re = re.compile("^Backed up to (?P.*)$", re.MULTILINE)

Used it, thanks!

> 
> 5)
> +matched = myre.search(result.stdout_text + result.stderr_text)
> Why do you need search in both stderr and stdout?

Because of this bug: https://fedorahosted.org/freeipa/ticket/5484

> 
> Martin^2
> 
> 

-- 
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
From 1805ac791e061117405560dbca371f1666c1b9e2 Mon Sep 17 00:00:00 2001
From: Oleg Fayans 
Date: Mon, 14 Dec 2015 14:04:07 +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..70354919e6ed8b1f21b19e838a3083c89a1bdc6d 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"])
+path_re = re.compile("^Backed up to (?P.*)$", re.MULTILINE)
+matched = path_re.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] [PATCH] 0047 dogtaginstance: remove unused function 'check_inst'

[Tomas Babej]

On 12/14/2015 06:56 AM, Fraser Tweedale wrote:
> Just some drive-by cleanup of an unused function.
> 
> Cheers,
> Fraser
> 

ACK, thanks for the cleanup!

Pushed to master: 38861428e76c19107a03f07530e3724aee60a270

-- 
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 11/12/15 15:00, Petr Spacek wrote:

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

Re: [Freeipa-devel] [PATCH 0365] Remove unused KRA code from ipa-server-install

[Alexander Bokovoy]

On Mon, 14 Dec 2015, David Kupka wrote:

On 30/11/15 16:31, Martin Basti wrote:

First instance of KRA should be installed only by ipa-kra-install

Patch attached.





Hi,
patch works, but I don't like the approach.

Do we really want to remove '--setup-kra' option from ipa-server-install?
Why do we remove '--setup-kra' while keeping '--setup-dns'? Why do we 
keep installing PKI CA when it can be also installed later?

Yes, we do want. Adding the option was an error in the first place. This
patch fixes the error.

I would love if 'ipa-server-install' installed only the bare minimum 
needed and then other features was added using ipa-{feature}-install 
but also I don't mind one almighty installer that can install all 
possible combinations of features.
But this is neither of it. This just brings another inconsistency into 
FreeIPA behavior.

We don't have --with-adtrust either. And we don't want it.

--
/ Alexander Bokovoy

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

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

[David Kupka]

On 14/12/15 14:52, David Kupka wrote:

On 11/12/15 15:00, Petr Spacek wrote:

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

Re: [Freeipa-devel] [PATCH 0365] Remove unused KRA code from ipa-server-install

[David Kupka]

On 14/12/15 15:05, Alexander Bokovoy wrote:

On Mon, 14 Dec 2015, David Kupka wrote:

On 30/11/15 16:31, Martin Basti wrote:

First instance of KRA should be installed only by ipa-kra-install

Patch attached.





Hi,
patch works, but I don't like the approach.

Do we really want to remove '--setup-kra' option from ipa-server-install?
Why do we remove '--setup-kra' while keeping '--setup-dns'? Why do we
keep installing PKI CA when it can be also installed later?

Yes, we do want. Adding the option was an error in the first place. This
patch fixes the error.


I would love if 'ipa-server-install' installed only the bare minimum
needed and then other features was added using ipa-{feature}-install
but also I don't mind one almighty installer that can install all
possible combinations of features.
But this is neither of it. This just brings another inconsistency into
FreeIPA behavior.

We don't have --with-adtrust either. And we don't want it.

Ok, then are we aiming for 'ipa-server-install' that only installs the 
bare minimum and everything else should be installed later?


Or, do we decide per feature if it's appropriate to include it in 
'ipa-server-install'? What are the criteria in this case?


--
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 534] replica promotion: let ipa-client-install validate enrollment options

[Tomas Babej]

On 12/14/2015 03:37 PM, Jan Cholasta wrote:
> Hi,
> 
> the attached patch fixes .
> 
> Honza
> 
> 
> 

ACK, Pushed to master: 110e3dfc5401899ae0a54cc979ca0820e53cfa02

-- 
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 0116] CI tests: remove '-p' option from ipa-dns-install calls

[Milan Kubík]

On 12/10/2015 04:35 PM, Martin Babinsky wrote:

See commit message.


Works for me. ACK.


--
Milan Kubik

--
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 531-532] server install: redirect ipa-client-install output to standard output

[Tomas Babej]

On 12/14/2015 12:57 PM, Jan Cholasta wrote:
> On 14.12.2015 12:54, Tomas Babej wrote:
>>
>>
>> On 12/14/2015 12:52 PM, Jan Cholasta wrote:
>>> Hi,
>>>
>>> the attached patches fix .
>>>
>>> Honza
>>>
>>>
>>>
>>
>> Shouldn't skip_output be also marked as incompatible with
>> redirect_output?
> 
> Yes, it should.
> 

Otherwise the patch works fine, so conditional ACK here given that the
additional sanity check is implemented.

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 533] replica promotion: notify user about ignoring client enrollment options

[Tomas Babej]

On 12/14/2015 02:02 PM, Jan Cholasta wrote:
> On 14.12.2015 13:41, Jan Cholasta wrote:
>> Hi,
>>
>> the attached patch fixes .
> 
> Self-NACK, updated patch attached.
> 
> 
> 

ACK, works fine.

-- 
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 14.12.2015 10:15, Martin Basti wrote:



On 14.12.2015 07:53, Jan Cholasta wrote:

On 11.12.2015 17:24, Martin Basti wrote:



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'


So that they apply to all replication agreements.

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


I didn't, they were moved to cn=mapping tree,cn=config as well.

Updated patches attached.


ACK


Pushed to master: b248dfda3980244070f85a1968e76d37ad50de9c

--
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/14/2015 10:21 AM, Martin Basti wrote:
> 
> 
> On 14.12.2015 09:24, Martin Kosek wrote:
>> On 12/14/2015 07:21 AM, Jan Cholasta wrote:
>>> On 11.12.2015 19:01, Tomas Babej wrote:

 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.
>> I would not be too worried about miliseconds longer execution on a
>> function
>> called during RPM upgrade.
>>
 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
>>> +1, also a single function is hardly "much code".
>> Ok then. If you all want to add yet another N-V-R parsing function in the
>> FreeIPA code, I can live with it (with raised eyebrows though).
> 
> Rebased patch attached.

I tested the patch, and it works fine - so conditional ACK from me for
the current iteration of the patch, given developer consensus which was
not reached yet.

There's a split of opinions (external binary camp vs. copy camp),
so we need to decide if we both camps are OK with proceeding.

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 0116] CI tests: remove '-p' option from ipa-dns-install calls

[Martin Basti]

On 14.12.2015 15:40, Milan Kubík wrote:

On 12/10/2015 04:35 PM, Martin Babinsky wrote:

See commit message.


Works for me. ACK.



Pushed to master: c4b9b295d8184694c50c0d56051e0273445c98ec

--
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 0378] Tests: fix always true assertion

[Tomas Babej]

On 12/14/2015 12:24 PM, Martin Basti wrote:
> Fixes:
> /usr/lib/python2.7/site-packages/ipatests/test_cmdline/test_ipagetkeytab.py:116:
> SyntaxWarning: assertion is always true, perhaps remove parentheses?
> 
> Patch attached.
> 
> 

Nice catch. ACK.

Pushed to master: e1cb802d15d07f80b4812c51e25eb4f7ea48d7c5

-- 
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 531-532] server install: redirect ipa-client-install output to standard output

[Jan Cholasta]

On 14.12.2015 14:20, Tomas Babej wrote:



On 12/14/2015 12:57 PM, Jan Cholasta wrote:

On 14.12.2015 12:54, Tomas Babej wrote:



On 12/14/2015 12:52 PM, Jan Cholasta wrote:

Hi,

the attached patches fix .

Honza





Shouldn't skip_output be also marked as incompatible with
redirect_output?


Yes, it should.



Otherwise the patch works fine, so conditional ACK here given that the
additional sanity check is implemented.


Updated patches attched.

--
Jan Cholasta
From fa6e8f530b7e436e975c81f23503a78d32d5fe30 Mon Sep 17 00:00:00 2001
From: Jan Cholasta 
Date: Mon, 14 Dec 2015 12:45:45 +0100
Subject: [PATCH 1/2] ipautil: allow redirecting command output to standard
 output in run()

https://fedorahosted.org/freeipa/ticket/5527
---
 ipapython/ipautil.py | 15 +--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/ipapython/ipautil.py b/ipapython/ipautil.py
index 4480744..160706f 100644
--- a/ipapython/ipautil.py
+++ b/ipapython/ipautil.py
@@ -286,7 +286,7 @@ class _RunResult(collections.namedtuple('_RunResult',
 def run(args, stdin=None, raiseonerr=True, nolog=(), env=None,
 capture_output=False, skip_output=False, cwd=None,
 runas=None, timeout=None, suplementary_groups=[],
-capture_error=False, encoding=None):
+capture_error=False, encoding=None, redirect_output=False):
 """
 Execute an external command.
 
@@ -323,6 +323,7 @@ def run(args, stdin=None, raiseonerr=True, nolog=(), env=None,
 :param encoding: For Python 3, the encoding to use for output,
 error_output, and (if it's not bytes) stdin.
 If None, the current encoding according to locale is used.
+:param redirect_output: Redirect (error) output to standard (error) output.
 
 :return: An object with these attributes:
 
@@ -362,6 +363,13 @@ def run(args, stdin=None, raiseonerr=True, nolog=(), env=None,
 raise ValueError('skip_output is incompatible with '
  'capture_output or capture_error')
 
+if redirect_output and (capture_output or capture_error):
+raise ValueError('redirect_output is incompatible with '
+ 'capture_output or capture_error')
+
+if skip_output and redirect_output:
+raise ValueError('skip_output is incompatible with redirect_output')
+
 if env is None:
 # copy default env
 env = copy.deepcopy(os.environ)
@@ -370,6 +378,9 @@ def run(args, stdin=None, raiseonerr=True, nolog=(), env=None,
 p_in = subprocess.PIPE
 if skip_output:
 p_out = p_err = open(paths.DEV_NULL, 'w')
+elif redirect_output:
+p_out = sys.stdout
+p_err = sys.stderr
 else:
 p_out = subprocess.PIPE
 p_err = subprocess.PIPE
@@ -432,7 +443,7 @@ def run(args, stdin=None, raiseonerr=True, nolog=(), env=None,
 
 # The command and its output may include passwords that we don't want
 # to log. Replace those.
-if skip_output:
+if skip_output or redirect_output:
 output_log = None
 error_log = None
 else:
-- 
2.4.3

From 21b5b0391b3eafdf0baf4b80562a0571f995b171 Mon Sep 17 00:00:00 2001
From: Jan Cholasta 
Date: Mon, 14 Dec 2015 12:46:48 +0100
Subject: [PATCH 2/2] server install: redirect ipa-client-install output to
 standard output

https://fedorahosted.org/freeipa/ticket/5527
---
 ipaserver/install/server/install.py| 13 ++---
 ipaserver/install/server/replicainstall.py | 18 +-
 2 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/ipaserver/install/server/install.py b/ipaserver/install/server/install.py
index 540ce20..a07ca66 100644
--- a/ipaserver/install/server/install.py
+++ b/ipaserver/install/server/install.py
@@ -991,10 +991,10 @@ def install(installer):
 args.append("--no-sshd")
 if options.mkhomedir:
 args.append("--mkhomedir")
-run(args)
+run(args, redirect_output=True)
+print()
 except Exception as e:
-sys.exit("Configuration of client side components failed!\n"
- "ipa-client-install returned: " + str(e))
+sys.exit("Configuration of client side components failed!")
 
 # Everything installed properly, activate ipa service.
 services.knownservices.ipa.enable()
@@ -1251,13 +1251,12 @@ def uninstall(installer):
 try:
 result = run([paths.IPA_CLIENT_INSTALL, "--on-master",
   "--unattended", "--uninstall"],
- raiseonerr=False, capture_output=True)
+ raiseonerr=False, redirect_output=True)
 if result.returncode not in [0, 2]:
-raise RuntimeError(result.output)
-except Exception as e:
+raise RuntimeError("Failed to configure the client")
+except Exception:
 rv = 1
 print("Uninstall of client side components failed!")
-

Re: [Freeipa-devel] [PATCHES 531-532] server install: redirect ipa-client-install output to standard output

[Tomas Babej]

On 12/14/2015 02:41 PM, Jan Cholasta wrote:
> On 14.12.2015 14:20, Tomas Babej wrote:
>>
>>
>> On 12/14/2015 12:57 PM, Jan Cholasta wrote:
>>> On 14.12.2015 12:54, Tomas Babej wrote:


 On 12/14/2015 12:52 PM, Jan Cholasta wrote:
> Hi,
>
> the attached patches fix
> .
>
> Honza
>
>
>

 Shouldn't skip_output be also marked as incompatible with
 redirect_output?
>>>
>>> Yes, it should.
>>>
>>
>> Otherwise the patch works fine, so conditional ACK here given that the
>> additional sanity check is implemented.
> 
> Updated patches attched.
> 

Thanks, I just removed the empty space in the 'print ()' call before
pushing.

ACK, Pushed to master: c856401478ce2f4fdd9cd7192afd18704f78e2e6

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 0374-0375] Fix permissions on newly created directories

[Tomas Babej]

On 12/11/2015 07:19 PM, Martin Basti wrote:
> 
> 
> 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.


ACK, works as expected.

Pushed to master: 4272ba40ea909b1f783a6fada5b1eebb6efbdf93

-- 
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 534] replica promotion: let ipa-client-install validate enrollment options

[Jan Cholasta]

Hi,

the attached patch fixes .

Honza

--
Jan Cholasta
From 9981523cb21abba358f54ff6b801211c9e41b16c Mon Sep 17 00:00:00 2001
From: Jan Cholasta 
Date: Mon, 14 Dec 2015 15:15:44 +0100
Subject: [PATCH] replica promotion: let ipa-client-install validate enrollment
 options

ipa-client-install output is redirected to standard output, so let it print
its own error message for missing options.

https://fedorahosted.org/freeipa/ticket/5542
---
 ipaserver/install/server/replicainstall.py | 9 -
 1 file changed, 9 deletions(-)

diff --git a/ipaserver/install/server/replicainstall.py b/ipaserver/install/server/replicainstall.py
index c6927a5..4e6abde 100644
--- a/ipaserver/install/server/replicainstall.py
+++ b/ipaserver/install/server/replicainstall.py
@@ -812,15 +812,6 @@ def install(installer):
 def ensure_enrolled(installer):
 config = installer._config
 
-# Perform only if we have the necessary options
-if not any([installer.password,
-installer.admin_password,
-installer.keytab]):
-sys.exit("IPA client is not configured on this system.\n"
- "You must join the system by running 'ipa-client-install' "
- "first. Alternatively, you may specify enrollment related "
- "options directly, see man ipa-replica-install.")
-
 # Call client install script
 service.print_msg("Configuring client side components")
 try:
-- 
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 533] replica promotion: notify user about ignoring client enrollment options

[Jan Cholasta]

On 14.12.2015 13:41, Jan Cholasta wrote:

Hi,

the attached patch fixes .


Self-NACK, updated patch attached.

--
Jan Cholasta
From fdbe15a0fe488177184b740ab65a57f9166799f7 Mon Sep 17 00:00:00 2001
From: Jan Cholasta 
Date: Mon, 14 Dec 2015 13:30:33 +0100
Subject: [PATCH] replica promotion: notify user about ignoring client
 enrollment options

When IPA client is already installed, notify the user that the enrollment
options are ignored in ipa-replica-install.

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

diff --git a/ipaserver/install/server/replicainstall.py b/ipaserver/install/server/replicainstall.py
index 1d5b528..d62edd1 100644
--- a/ipaserver/install/server/replicainstall.py
+++ b/ipaserver/install/server/replicainstall.py
@@ -883,6 +883,12 @@ def promote_check(installer):
 client_fstore = sysrestore.FileStore(paths.IPA_CLIENT_SYSRESTORE)
 if not client_fstore.has_files():
 ensure_enrolled(installer)
+else:
+if (options.domain_name or options.server or options.realm_name or
+options.host_name or options.password or options.keytab):
+print("IPA client is already configured on this system, ignoring "
+  "the --domain, --server, --realm, --hostname, --password "
+  "and --keytab options.")
 
 sstore = sysrestore.StateFile(paths.SYSRESTORE)
 
-- 
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 533] replica promotion: notify user about ignoring client enrollment options

[Tomas Babej]

On 12/14/2015 02:19 PM, Tomas Babej wrote:
> 
> 
> On 12/14/2015 02:02 PM, Jan Cholasta wrote:
>> On 14.12.2015 13:41, Jan Cholasta wrote:
>>> Hi,
>>>
>>> the attached patch fixes .
>>
>> Self-NACK, updated patch attached.
>>
>>
>>
> 
> ACK, works fine.
> 

Pushed to master: d68613194b4a26239ebb689d7270cf6c2035afbd

-- 
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 0117] ipa-client-install: create a temporary directory for ccache files

[Martin Babinsky]

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

--
Martin^3 Babinsky
From 1e6dcfe235b1c9e563dd0fd3408ef93008010a89 Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Mon, 14 Dec 2015 14:28:41 +0100
Subject: [PATCH] ipa-client-install: create a temporary directory for ccache
 files

gssapi.Credentials instantiation in ipautil.kinit_keytab() raises 'Bad format
in credential cache' error when a name of an existing zero-length file is
passed as a ccache parameter. Use temporary directory instead and let GSSAPI
to create file-based ccache on demand.

https://fedorahosted.org/freeipa/ticket/5528
---
 ipa-client/ipa-install/ipa-client-install | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/ipa-client/ipa-install/ipa-client-install b/ipa-client/ipa-install/ipa-client-install
index 9556cdec0fbb5b07984ebf39ee8d4cdd8e53ed97..e9a7d45c3f82a58f6297db7354eb784f6416db4b 100755
--- a/ipa-client/ipa-install/ipa-client-install
+++ b/ipa-client/ipa-install/ipa-client-install
@@ -2578,8 +2578,8 @@ def install(options, env, fstore, statestore):
 root_logger.error("Test kerberos configuration failed")
 return CLIENT_INSTALL_ERROR
 env['KRB5_CONFIG'] = krb_name
-(ccache_fd, ccache_name) = tempfile.mkstemp()
-os.close(ccache_fd)
+ccache_dir = tempfile.mkdtemp(prefix='krbcc')
+ccache_name = os.path.join(ccache_dir, 'ccache')
 join_args = [paths.SBIN_IPA_JOIN,
  "-s", cli_server[0],
  "-b", str(realm_to_suffix(cli_realm)),
@@ -2727,7 +2727,7 @@ def install(options, env, fstore, statestore):
 except OSError:
 root_logger.error("Could not remove %s", krb_name)
 try:
-os.remove(ccache_name)
+os.rmdir(ccache_dir)
 except OSError:
 pass
 try:
-- 
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 0376] KRA: add RA cert during replica promotion

[David Kupka]

On 14/12/15 11:00, David Kupka wrote:

On 10/12/15 19:40, Martin Basti wrote:

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

patch attached.



Hi,
thanks for the patch. It works but only when WAIT_AFTER_ARCHIVE is raised.
Patch attached.
IOW, your patch works for me, ACK. To let tests pass (and eventually 
fall on other issue when it appears) please include my patch.


--
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 0070] Makefile: disable parallel build

[Tomas Babej]

On 12/11/2015 09:35 AM, Petr Spacek wrote:
> Hello,
> 
> Makefile: disable parallel build
> 
> IPA build system cannot cope with parallel build anyway, so this patch
> disables parallel build explicitly so it does not blow up when user
> has -j specified in default MAKEOPTS.
> 
> 
> 

ACK.

Pushed to:
master: e650e5eda161a912d22ee2e6ebaf94df3a07d7ff
ipa-4-2: 58453bc25092d93f79aca8fa2bd3cc3a10bc6810

-- 
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 0117] ipa-client-install: create a temporary directory for ccache files

[Tomas Babej]

On 12/14/2015 05:31 PM, Martin Babinsky wrote:
> fixes https://fedorahosted.org/freeipa/ticket/5528

Works as expected, code-wise looks good.

Thanks for looking into this, ACK!

Pushed to master: 5886f87f974fa508047a21350c2e6e75a3001da6

-- 
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/14/2015 07:21 AM, Jan Cholasta wrote:
> On 11.12.2015 19:01, Tomas Babej wrote:
>>
>>
>> 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.

I would not be too worried about miliseconds longer execution on a function
called during RPM upgrade.

>> 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
> 
> +1, also a single function is hardly "much code".

Ok then. If you all want to add yet another N-V-R parsing function in the
FreeIPA code, I can live with it (with raised eyebrows though).

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

[Petr Spacek]

On 14.12.2015 09:24, Martin Kosek wrote:
> On 12/14/2015 07:21 AM, Jan Cholasta wrote:
>> On 11.12.2015 19:01, Tomas Babej wrote:
>>>
>>>
>>> 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.
> 
> I would not be too worried about miliseconds longer execution on a function
> called during RPM upgrade.
> 
>>> 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
>>
>> +1, also a single function is hardly "much code".
> 
> Ok then. If you all want to add yet another N-V-R parsing function in the
> FreeIPA code, I can live with it (with raised eyebrows though).

+1

We already tried and failed in custom parsing (presumably caused by request to
not add any new dependency). I agree with Martin that copy code does
not look like a proper fix.

-- 
Petr^2 Spacek

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

Re: [Freeipa-devel] [PATCH 0114] harden domain level 1 topology connectivity checks

[Martin Babinsky]

On 12/08/2015 05:35 PM, Martin Babinsky wrote:

A sort of auxilliary patch which makes topology checks more resistant to
https://fedorahosted.org/freeipa/ticket/5526

If required I will open a separate ticket for it though.




Bump for review.

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