[Freeipa-devel] [patch 0037] spec: Add python-sssdconfig dependency for python-ipatests package

2016-04-22 Thread Milan Kubík 

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

Applies to ipa-4-3, master

--
Milan Kubik

From cb26da230ae358c1d1768d82c3be8e75bb2159d3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Milan=20Kub=C3=ADk?= 
Date: Fri, 22 Apr 2016 23:43:47 +0200
Subject: [PATCH] spec: Add python-sssdconfig dependency for python-ipatests
 package

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

diff --git a/freeipa.spec.in b/freeipa.spec.in
index aaa40cc9a2246ed1d244e160edf935da216c75c5..1bfe2cfedb9cfb3fd799205de032653b746a4e69 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -603,6 +603,7 @@ Requires: python2-polib
 Requires: python-pytest-multihost >= 0.5
 Requires: python-pytest-sourceorder
 Requires: ldns-utils
+Requires: python-sssdconfig
 
 Provides: %{alt_name}-tests = %{version}
 Conflicts: %{alt_name}-tests
@@ -634,6 +635,7 @@ Requires: python3-polib
 Requires: python3-pytest-multihost >= 0.5
 Requires: python3-pytest-sourceorder
 Requires: ldns-utils
+Requires: python-sssdconfig
 
 %description -n python3-ipatests
 IPA is an integrated solution to provide centrally managed Identity (users,
-- 
2.8.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] [python-pytest-multihost]-Ticket-6 run_command produces exit code 1 on windows

2016-04-22 Thread Niranjan 
Petr Viktorin wrote:
> On 04/21/2016 03:04 PM, Niranjan wrote:
> > Petr Viktorin wrote:
> >> On 04/21/2016 07:25 AM, Niranjan wrote:
> >>> Petr Viktorin wrote:
>  On 04/20/2016 06:11 AM, Niranjan wrote:
> > Petr Viktorin wrote:
> >> On 04/18/2016 12:39 PM, Niranjan wrote:
> >>> Niranjan wrote:
>  Niranjan wrote:
> > Petr Viktorin wrote:
> >> On 04/06/2016 10:55 AM, Niranjan wrote:
> >>> Greetings,
> >>>
> >>> For https://fedorahosted.org/python-pytest-multihost/ticket/6 , i 
> >>> have proposed
> >>> a patch, I think this patch is more of a workaround , than a 
> >>> solution. I would
> >>> like to get more inputs on how to use pytest-multihost to execute 
> >>> commands 
> >>> on Windows. The method i am using requires cygwin with 
> >>> openssh/bash to be
> >>> installed. 
> >>
> [...]
> >>
> >> If this works for you, I'll commit it and release.
> > 
> > With this latest patch it worked after removing the line "transport_class =
> > transport.SSHTransport" from QeBaseHost class. 
> > 
> > Please go ahead and commit it.
> 
> I've commited it and released version 1.1. Packages for Fedora Rawhide
> are being built; if you need this for older Fedoras let me know so I
> don't forget to build there too.
No for fedora it's fine, but mostly we are using this in RHEL7, so if you
can build it for epel7 it would be great, else it's fine, i will build
this for RHEL7, i intend to use this for sssd functional testing.

Also i tried to pull the latest from python-pytest-multihost.git from
git.fedoraproject.org, i could not see it merged. 
> 
> Thanks for your assistance!
> 
You're welcome. 
> 
> -- 
> Petr Viktorin


Regards
Niranjan


pgpzMWPh1aXZf.pgp
Description: PGP signature
-- 
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] 957 ipa-client-install: fix typo in nslcd service name

2016-04-22 Thread Lukas Slebodnik 
On (22/04/16 08:09), Martin Basti wrote:
>
>
>On 22.04.2016 05:02, Rob Crittenden wrote:
>> Lukas Slebodnik wrote:
>> > On (21/04/16 16:42), Rob Crittenden wrote:
>> > > Lukas Slebodnik wrote:
>> > > > On (21/04/16 19:25), Petr Vobornik wrote:
>> > > > > related but does not implement
>> > > > > https://fedorahosted.org/freeipa/ticket/5806
>> > > > > -- 
>> > > > > Petr Vobornik
>> > > > 
>> > > > >  From b9b8716ec9ba5a5cdbed1f6cdedf7cff8878f08f Mon Sep 17
>> > > > > 00:00:00 2001
>> > > > > From: Petr Vobornik 
>> > > > > Date: Thu, 21 Apr 2016 19:23:31 +0200
>> > > > > Subject: [PATCH] ipa-client-install: fix typo in nslcd service name
>> > > > > 
>> > > > > related but does not implement
>> > > > > https://fedorahosted.org/freeipa/ticket/5806
>> > > > > ---
>> > > > > client/ipa-client-install | 2 +-
>> > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
>> > > > > 
>> > > > > diff --git a/client/ipa-client-install b/client/ipa-client-install
>> > > > > index 
>> > > > > c38843f85639a9118cd1a471709992690643d79a..0e6e65c4ad4ce01fe1257eee4bb2633a70c3de4e
>> > > > > 100755
>> > > > > --- a/client/ipa-client-install
>> > > > > +++ b/client/ipa-client-install
>> > > > > @@ -2938,7 +2938,7 @@ def install(options, env, fstore, statestore):
>> > > > >   nscd.service_name)
>> > > > > 
>> > > > >   nslcd = services.knownservices.nslcd
>> > > > > -if nscd.is_installed():
>> > > > > +if nslcd.is_installed():
>> > > > >   save_state(nslcd)
>> > > > 
>> > > > I thought that milestone "Future Releases" has lower priority
>> > > > then "FreeIPA 4.4 Backlog"
>> > > > 
>> > > > Therefore I would prefer to close ticket #5806 and implement
>> > > > following one
>> > > > https://fedorahosted.org/freeipa/ticket/5557#comment:2
>> > > 
>> > > I don't understand what you are suggesting. Tickets aren't
>> > > swapped like this
>> > > and certainly non-related bugs aren't closed for another.
>> > > 
>> > > This patch just fixes an obvious one-liner as agreed upon in
>> > > triage. The rest
>> > > will be potentially addressed later.
>> > > 
>> > > ACK on the patch.
>+1
>> > > 
>> > I cannot see a reason to fix oneliner.
>> > This code is not tested and should be removed (#5557)
>> 
>> I fail to see the controversy here. These are two completely and
>> absolutely unrelated bugs.
>> 
>> > Or someone should write integration test.
>> > 
>> > I'm sorry but conditional-NACK (missing integration test)
>> 
>> I've been out of the project a while, so perhaps my knowledge is stale,
>> but AFAIU upstream QE writes the integration tests, not the developers.
>> This was at least true when I was a daily contributor.
>>
You were used to old style development model (waterfall).
And management mentioned that we should be more agile.
All agile methodologies assume that also developers write tests.

>> At least at one time QE did testing using --no-sssd. Whether that is
It should have been a long time ago.
Modern freeipa client (4.4) requires sssd and many features
will not work with nslcd. Trust, views ...

>> still the case I don't know but I do remember fixing bugs around it. And
>> this particular bug might not be immediately apparent unless a specific
>> configuration was present. I noticed the bug while reading code, not in
>> production.
>>
>> rob
>> 
>Patch do exactly the same as was agreed on devel meeting. I don't see reason
>why we shouldn't fix oneliner.
>
Because it's not tested and there might be much more things which
does not work. It should be either tested or removed.

Ticket #5557 was triaged before #5806. (4 monts ago vs 10 days ago).
The purpose of ticket "#5557" was to have a single line change in
freeipa spec file. But after a triage it was decided that this part of code
will be deprecated and later removed.

I know that this patch was a oneliner but I do not see a reason why
freeipa developers should waste time with fixing unested code.
It's obvious it could not work since 2013-11-07 and nonody has complained yet.
It's another reason to remove this code.

Another reason is that freeipa-client does not have a dependency
on nss-pam-ldapd.

BTW I moved ticket #5557 to reconsider it one more time.

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 0441] Configure httpd service from installer

2016-04-22 Thread Martin Basti 



On 21.04.2016 22:55, Timo Aaltonen wrote:

21.04.2016, 20:50, Martin Basti kirjoitti:


On 21.04.2016 19:28, Stanislav Laznicka wrote:

On 04/21/2016 11:19 AM, Martin Basti wrote:


On 20.04.2016 17:27, Martin Basti wrote:


On 24.03.2016 14:27, Martin Basti wrote:


On 24.03.2016 13:55, Jan Cholasta wrote:

On 18.3.2016 23:27, Timo Aaltonen wrote:

On 17.03.2016 18:36, Martin Basti wrote:

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

would be nicer if ipa-httpd.conf was a template with the current
hardcoded values replaced with platform paths..

+1, I would also prefer if the file was renamed to
init/systemd/httpd.conf rather than install/share/ipa-httpd.conf.

ipa-httpd.conf.template should be in /user/share/ipa, directory
init/systemd copied only to rpm and then copied to
/etc/systemd/system AFAIK



not relevant to this patch, but there are others candidates for
templates like:

daemons/dnssec/ipa-dnskeysyncd.service
daemons/dnssec/ipa-ods-exporter.service
install/conf/ipa.conf

Updated patch attached, sorry for delay.



Updated patch attached (fixed unused import).



Seems to work as expected. However, wouldn't it be better to use
installutils.remove_file instead of remove_httpd_service_ipa_conf (or
at least log the possible error during os.unlink) to get the same
behavior as with the other config files?

It could be, but because I created platform specific method for adding
httpd service config, it seems natural to me to create inverse operation
platform specific too.
I have no strong opinion about this, Timo what might be better, you use
platform specific code more than we? :)

Well, with this patch in I'd just reuse the methods from
RedHatTaskNamespace() just like some others are being used right now.
Systemd is all I support anyway.




Updated patch attached, missing log added


From 263ff915870ab307b7191500b71db933e92fb505 Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Wed, 16 Mar 2016 09:04:42 +0100
Subject: [PATCH] Configure httpd service from installer instead of directly
 from RPM

File httpd.service was created by RPM, what causes that httpd service may
fail due IPA specific configuration even if IPA wasn't installed or was
uninstalled (without erasing RPMs).

With this patch httpd service is configured by httpd.d/ipa.conf during
IPA installation and this config is removed by uninstaller, so no
residual http configuration related to IPA should stay there.

https://fedorahosted.org/freeipa/ticket/5681
---
 freeipa.spec.in   |  3 +--
 init/systemd/httpd.service|  7 ---
 install/share/Makefile.am |  1 +
 install/share/ipa-httpd.conf.template |  7 +++
 ipaplatform/base/paths.py |  3 +++
 ipaplatform/base/tasks.py |  8 
 ipaplatform/redhat/tasks.py   | 29 +
 ipaserver/install/httpinstance.py |  6 ++
 ipaserver/install/server/upgrade.py   |  5 +
 9 files changed, 60 insertions(+), 9 deletions(-)
 delete mode 100644 init/systemd/httpd.service
 create mode 100644 install/share/ipa-httpd.conf.template

diff --git a/freeipa.spec.in b/freeipa.spec.in
index 1ded3048873fb9d4cb97b7aca52132345c209a96..aaa40cc9a2246ed1d244e160edf935da216c75c5 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -832,7 +832,6 @@ mkdir -p %{buildroot}%{_unitdir}
 mkdir -p %{buildroot}%{etc_systemd_dir}
 install -m 644 init/systemd/ipa.service %{buildroot}%{_unitdir}/ipa.service
 install -m 644 init/systemd/ipa_memcached.service %{buildroot}%{_unitdir}/ipa_memcached.service
-install -m 644 init/systemd/httpd.service %{buildroot}%{etc_systemd_dir}/httpd.service
 install -m 644 init/systemd/ipa-custodia.service %{buildroot}%{_unitdir}/ipa-custodia.service
 # END
 mkdir -p %{buildroot}/%{_localstatedir}/lib/ipa/backup
@@ -1143,7 +1142,7 @@ fi
 %{_tmpfilesdir}/%{name}.conf
 %attr(644,root,root) %{_unitdir}/ipa_memcached.service
 %attr(644,root,root) %{_unitdir}/ipa-custodia.service
-%attr(644,root,root) %{etc_systemd_dir}/httpd.service
+%ghost %attr(644,root,root) %{etc_systemd_dir}/httpd.d/ipa.conf
 # END
 %dir %{_usr}/share/ipa
 %{_usr}/share/ipa/wsgi.py*
diff --git a/init/systemd/httpd.service b/init/systemd/httpd.service
deleted file mode 100644
index 7ce8f04d8b9bb3663e59d4fdc610af0eb4478178..
--- a/init/systemd/httpd.service
+++ /dev/null
@@ -1,7 +0,0 @@
-.include /usr/lib/systemd/system/httpd.service
-
-[Service]
-Environment=KRB5CCNAME=/var/run/httpd/ipa/krbcache/krb5ccache
-Environment=KDCPROXY_CONFIG=/etc/ipa/kdcproxy/kdcproxy.conf
-ExecStartPre=/usr/libexec/ipa/ipa-httpd-kdcproxy
-ExecStopPost=-/usr/bin/kdestroy -A
diff --git a/install/share/Makefile.am b/install/share/Makefile.am
index b4cb8312471a68d8cd855f542478afe10d200c39..3a3bd2699efaf45ab79dd0257c2d26e7952891eb 100644
--- a/install/share/Makefile.am
+++ b/install/share/Makefile.am
@@ -88,6 +88,7 @@ app_DATA =\
 	kdcproxy.conf			\
 	kdcproxy-enable.uldif		\

Re: [Freeipa-devel] [PATCH] 0015, 16 webui: Add 'Skip overlap check' checkbox to the dns adder dialogs

2016-04-22 Thread Martin Basti 



On 21.04.2016 13:47, Martin Basti wrote:



On 20.04.2016 17:59, Pavel Vomacka wrote:



On 04/14/2016 02:03 PM, Martin Basti wrote:



On 14.04.2016 13:03, Pavel Vomacka wrote:

Hello,

The first patch (0015) adds the checkbox to the dns zone adder dialog.

The second patch (0016) adds the 'skip overlap check' checkbox to 
the dns forward zone adder dialog. This patch requires the previous 
one. The patch 0016 might not be used in case that the dns forward 
zone dialog shouldn't contain that checkbox.


--
Pavel^3 Vomacka


Can we add hint to webUI what 'skip overlap check' means? Maybe 
description from ipa dns[forward]zone-add --help.


Martin^2



Yes, it is included in these new pathes.

--
Pavel^3 Vomacka




Works for me



ACK, pushed to:

ipa-4-3:
* ea8f0297bb5910ecbafb8dffa767b6b3ae8f1a82 Add 'skip overlap check' 
checkbox into add zone dialog
* 5392042bbc0b64aeb6f963549dffd3ebaf1fd22b Add 'skip overlap check' 
checkbox to the add dns forward zone dialog

master:
* f4467923535b54795e79ca30e4a5de74ab66820d Add 'skip overlap check' 
checkbox into add zone dialog
* 822186b2715f8a3ce2f48e873d7e1568d03f9f97 Add 'skip overlap check' 
checkbox to the add dns forward zone dialog


-- 
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 0441] Configure httpd service from installer

2016-04-22 Thread Stanislav Laznicka 

On 04/22/2016 10:08 AM, Martin Basti wrote:



On 21.04.2016 22:55, Timo Aaltonen wrote:

21.04.2016, 20:50, Martin Basti kirjoitti:


On 21.04.2016 19:28, Stanislav Laznicka wrote:

On 04/21/2016 11:19 AM, Martin Basti wrote:


On 20.04.2016 17:27, Martin Basti wrote:


On 24.03.2016 14:27, Martin Basti wrote:


On 24.03.2016 13:55, Jan Cholasta wrote:

On 18.3.2016 23:27, Timo Aaltonen wrote:

On 17.03.2016 18:36, Martin Basti wrote:

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

would be nicer if ipa-httpd.conf was a template with the current
hardcoded values replaced with platform paths..

+1, I would also prefer if the file was renamed to
init/systemd/httpd.conf rather than install/share/ipa-httpd.conf.

ipa-httpd.conf.template should be in /user/share/ipa, directory
init/systemd copied only to rpm and then copied to
/etc/systemd/system AFAIK



not relevant to this patch, but there are others candidates for
templates like:

daemons/dnssec/ipa-dnskeysyncd.service
daemons/dnssec/ipa-ods-exporter.service
install/conf/ipa.conf

Updated patch attached, sorry for delay.



Updated patch attached (fixed unused import).



Seems to work as expected. However, wouldn't it be better to use
installutils.remove_file instead of remove_httpd_service_ipa_conf (or
at least log the possible error during os.unlink) to get the same
behavior as with the other config files?

It could be, but because I created platform specific method for adding
httpd service config, it seems natural to me to create inverse 
operation

platform specific too.
I have no strong opinion about this, Timo what might be better, you use
platform specific code more than we? :)

Well, with this patch in I'd just reuse the methods from
RedHatTaskNamespace() just like some others are being used right now.
Systemd is all I support anyway.




Updated patch attached, missing log added


Thanks, jolly good. 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] [PATCH] 957 ipa-client-install: fix typo in nslcd service name

2016-04-22 Thread Martin Basti 



On 22.04.2016 05:02, Rob Crittenden wrote:

Lukas Slebodnik wrote:

On (21/04/16 16:42), Rob Crittenden wrote:

Lukas Slebodnik wrote:

On (21/04/16 19:25), Petr Vobornik wrote:
related but does not implement 
https://fedorahosted.org/freeipa/ticket/5806

--
Petr Vobornik


 From b9b8716ec9ba5a5cdbed1f6cdedf7cff8878f08f Mon Sep 17 00:00:00 
2001

From: Petr Vobornik 
Date: Thu, 21 Apr 2016 19:23:31 +0200
Subject: [PATCH] ipa-client-install: fix typo in nslcd service name

related but does not implement 
https://fedorahosted.org/freeipa/ticket/5806

---
client/ipa-client-install | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/client/ipa-client-install b/client/ipa-client-install
index 
c38843f85639a9118cd1a471709992690643d79a..0e6e65c4ad4ce01fe1257eee4bb2633a70c3de4e 
100755

--- a/client/ipa-client-install
+++ b/client/ipa-client-install
@@ -2938,7 +2938,7 @@ def install(options, env, fstore, statestore):
  nscd.service_name)

  nslcd = services.knownservices.nslcd
-if nscd.is_installed():
+if nslcd.is_installed():
  save_state(nslcd)


I thought that milestone "Future Releases" has lower priority
then "FreeIPA 4.4 Backlog"

Therefore I would prefer to close ticket #5806 and implement 
following one

https://fedorahosted.org/freeipa/ticket/5557#comment:2


I don't understand what you are suggesting. Tickets aren't swapped 
like this

and certainly non-related bugs aren't closed for another.

This patch just fixes an obvious one-liner as agreed upon in triage. 
The rest

will be potentially addressed later.

ACK on the patch.

+1



I cannot see a reason to fix oneliner.
This code is not tested and should be removed (#5557)


I fail to see the controversy here. These are two completely and 
absolutely unrelated bugs.



Or someone should write integration test.

I'm sorry but conditional-NACK (missing integration test)


I've been out of the project a while, so perhaps my knowledge is 
stale, but AFAIU upstream QE writes the integration tests, not the 
developers. This was at least true when I was a daily contributor.


At least at one time QE did testing using --no-sssd. Whether that is 
still the case I don't know but I do remember fixing bugs around it. 
And this particular bug might not be immediately apparent unless a 
specific configuration was present. I noticed the bug while reading 
code, not in production.


rob

Patch do exactly the same as was agreed on devel meeting. I don't see 
reason why we shouldn't fix oneliner.


Pushed to master: a023dcbc5c2e60d29938588845905b62986c3a93

--
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 0464-0468] always set hostname during installation

2016-04-22 Thread Martin Basti 



On 20.04.2016 17:49, Martin Basti wrote:

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

It requires my patch 441.2
Patches attached.




Rebased patches attached, 441 has been pushed

Martin^2
From b3424c3453b62c2f02cb1b0fb4e9263853b6318b Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Tue, 19 Apr 2016 18:36:32 +0200
Subject: [PATCH 1/5] Always set hostname

This prevents cases when hostname on system is set inconsistently
(transient and static hostname differs) and may cause IPA errors.

This commit ensures that all hostnames are set properly.

https://fedorahosted.org/freeipa/ticket/5794
---
 client/ipa-client-install   |  4 ++--
 ipaplatform/base/paths.py   |  2 +-
 ipaplatform/base/tasks.py   | 12 --
 ipaplatform/redhat/tasks.py | 47 ++---
 ipapython/ipautil.py| 12 --
 ipaserver/install/server/install.py | 23 +-
 6 files changed, 36 insertions(+), 64 deletions(-)

diff --git a/client/ipa-client-install b/client/ipa-client-install
index 0e6e65c4ad4ce01fe1257eee4bb2633a70c3de4e..e56890463b2fd9d4e05a0d24bf5a796fa83ab3d7 100755
--- a/client/ipa-client-install
+++ b/client/ipa-client-install
@@ -713,12 +713,12 @@ def uninstall(options, env):
 root_logger.warning(
 "Failed to disable automatic startup of the SSSD daemon: %s", e)
 
+tasks.restore_hostname(fstore, statestore)
+
 if fstore.has_files():
 root_logger.info("Restoring client configuration files")
-tasks.restore_network_configuration(fstore, statestore)
 fstore.restore_all_files()
 
-ipautil.restore_hostname(statestore)
 unconfigure_nisdomain()
 
 nscd = services.knownservices.nscd
diff --git a/ipaplatform/base/paths.py b/ipaplatform/base/paths.py
index 585a5d26ed32a5f60cdb5d28de05b6468d03baa6..62d9e703d191c7b06fe37c917c7d99f2da76cc05 100644
--- a/ipaplatform/base/paths.py
+++ b/ipaplatform/base/paths.py
@@ -25,7 +25,7 @@ This base platform module exports default filesystem paths.
 class BasePathNamespace(object):
 BASH = "/bin/bash"
 BIN_FALSE = "/bin/false"
-BIN_HOSTNAME = "/bin/hostname"
+BIN_HOSTNAMECTL = "/bin/hostnamectl"
 LS = "/bin/ls"
 SH = "/bin/sh"
 SYSTEMCTL = "/bin/systemctl"
diff --git a/ipaplatform/base/tasks.py b/ipaplatform/base/tasks.py
index f5fb2b155020c213769830dd48ccc3b36bbd9e64..7c30088631e31be3b0722894dc49839b72805f32 100644
--- a/ipaplatform/base/tasks.py
+++ b/ipaplatform/base/tasks.py
@@ -48,7 +48,7 @@ class BaseTaskNamespace(object):
 def backup_and_replace_hostname(self, fstore, statestore, hostname):
 """
 Backs up the current hostname in the statestore (so that it can be
-restored by the restore_network_configuration platform task).
+restored by the restore_hostname platform task).
 
 Makes sure that new hostname (passed via hostname argument) is set
 as a new pemanent hostname for this host.
@@ -106,7 +106,7 @@ class BaseTaskNamespace(object):
 
 return
 
-def restore_network_configuration(self, fstore, statestore):
+def restore_hostname(self, fstore, statestore):
 """
 Restores the original hostname as backed up in the
 backup_and_replace_hostname platform task.
@@ -237,6 +237,14 @@ class BaseTaskNamespace(object):
 """
 return parse_version(version)
 
+def set_hostname(self, hostname):
+"""
+Set hostname for the system
+
+No return value expected, raise CalledProcessError when error occurred
+"""
+return
+
 def configure_httpd_service_ipa_conf(self):
 """Configure httpd service to work with IPA"""
 raise NotImplementedError()
diff --git a/ipaplatform/redhat/tasks.py b/ipaplatform/redhat/tasks.py
index 4be9a146e8fa1e78a454d92cba05484e7817f56d..2a110c9947942609dc38373d116a2e812f1ed539 100644
--- a/ipaplatform/redhat/tasks.py
+++ b/ipaplatform/redhat/tasks.py
@@ -26,10 +26,10 @@ system tasks.
 from __future__ import print_function
 
 import os
-import stat
 import socket
-import sys
 import base64
+import traceback
+
 from cffi import FFI
 from ctypes.util import find_library
 from functools import total_ordering
@@ -330,38 +330,31 @@ class RedHatTaskNamespace(BaseTaskNamespace):
 def backup_and_replace_hostname(self, fstore, statestore, hostname):
 old_hostname = socket.gethostname()
 try:
-ipautil.run([paths.BIN_HOSTNAME, hostname])
+self.set_hostname(hostname)
 except ipautil.CalledProcessError as e:
 print(("Failed to set this machine hostname to "
  "%s (%s)." % (hostname, str(e))), file=sys.stderr)
 
 filepath = paths.ETC_HOSTNAME
 if os.path.exists(filepath):
-# read old hostname
-with open(filepath, 'r') as f:
-for line in f.readlines():
-

[Freeipa-devel] [PATCH 0097-0098] Makefile: replace perl with sed

2016-04-22 Thread Petr Spacek 
Hello,

Makefile: add sed to BuildRequires

It was requried since forever but we did not explicitly mention it.

Makefile: replace perl with sed

Perl was missing in BuildRequires anyway and it is used only on one place,
all other places are using sed.

-- 
Petr^2 Spacek
From 3c7a3c87d62358407d856119e384c70040acec1e Mon Sep 17 00:00:00 2001
From: Petr Spacek 
Date: Fri, 22 Apr 2016 10:40:11 +0200
Subject: [PATCH] Makefile: replace perl with sed

Perl was missing in BuildRequires anyway and it is used only on one place,
all other places are using sed.
---
 Makefile | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/Makefile b/Makefile
index 8c1a5dea7cbd04fbd31bfe0de205608a3c347872..82e6936a7162ff6e0359521d5c1299ff8ee55220 100644
--- a/Makefile
+++ b/Makefile
@@ -164,15 +164,15 @@ version-update: release-update
 		> ipaclient/setup.py
 	sed -e s/__NUM_VERSION__/$(IPA_NUM_VERSION)/ install/ui/src/libs/loader.js.in \
 		> install/ui/src/libs/loader.js
-	perl -pi -e "s:__API_VERSION__:$(IPA_API_VERSION_MAJOR).$(IPA_API_VERSION_MINOR):" install/ui/src/libs/loader.js
-	perl -pi -e "s:__NUM_VERSION__:$(IPA_NUM_VERSION):" ipapython/version.py
-	perl -pi -e "s:__VENDOR_VERSION__:$(IPA_VENDOR_VERSION):" ipapython/version.py
-	perl -pi -e "s:__API_VERSION__:$(IPA_API_VERSION_MAJOR).$(IPA_API_VERSION_MINOR):" ipapython/version.py
+	sed -i -e "s:__API_VERSION__:$(IPA_API_VERSION_MAJOR).$(IPA_API_VERSION_MINOR):" install/ui/src/libs/loader.js
+	sed -i -e "s:__NUM_VERSION__:$(IPA_NUM_VERSION):" ipapython/version.py
+	sed -i -e "s:__VENDOR_VERSION__:$(IPA_VENDOR_VERSION):" ipapython/version.py
+	sed -i -e "s:__API_VERSION__:$(IPA_API_VERSION_MAJOR).$(IPA_API_VERSION_MINOR):" ipapython/version.py
 	touch -r ipapython/version.py.in ipapython/version.py
 	sed -e s/__VERSION__/$(IPA_VERSION)/ daemons/ipa-version.h.in \
 		> daemons/ipa-version.h
-	perl -pi -e "s:__NUM_VERSION__:$(IPA_NUM_VERSION):" daemons/ipa-version.h
-	perl -pi -e "s:__DATA_VERSION__:$(IPA_DATA_VERSION):" daemons/ipa-version.h
+	sed -i -e "s:__NUM_VERSION__:$(IPA_NUM_VERSION):" daemons/ipa-version.h
+	sed -i -e "s:__DATA_VERSION__:$(IPA_DATA_VERSION):" daemons/ipa-version.h
 
 	sed -e s/__VERSION__/$(IPA_VERSION)/ client/version.m4.in \
 		> client/version.m4
-- 
2.5.5

From 2deaef91ab71c0e78b2142c2102cfe65f0e4ed96 Mon Sep 17 00:00:00 2001
From: Petr Spacek 
Date: Fri, 22 Apr 2016 10:40:37 +0200
Subject: [PATCH] Makefile: add sed to BuildRequires

It was requried since forever but we did not explicitly mention it.
---
 freeipa.spec.in | 1 +
 1 file changed, 1 insertion(+)

diff --git a/freeipa.spec.in b/freeipa.spec.in
index aaa40cc9a2246ed1d244e160edf935da216c75c5..c60b3960d9613e58f81bcd1d732b34bd4eecd945 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -104,6 +104,7 @@ BuildRequires:  custodia
 BuildRequires:  libini_config-devel >= 1.2.0
 BuildRequires:  dbus-python
 BuildRequires:  python-netifaces >= 0.10.4
+BuildRequires:  sed
 
 # Build dependencies for unit tests
 BuildRequires:  libcmocka-devel
-- 
2.5.5

-- 
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 0463] Performance: do not download password attributes in host/find-user command

2016-04-22 Thread David Kupka 

On 22/04/16 10:58, Martin Basti wrote:



On 21.04.2016 09:17, Martin Basti wrote:



On 20.04.2016 16:57, Martin Basti wrote:

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

Patch attached.



selfNACK



Updated patch attached.




Works for me, ACK.

--
David Kupka

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

Re: [Freeipa-devel] [PATCH] BUILD: Remove detection of libcheck

2016-04-22 Thread Martin Basti 



On 22.04.2016 13:18, Petr Spacek wrote:

On 10.3.2016 10:40, Lukas Slebodnik wrote:

ehlo,

simple patch is attached.

>From 2b34cdbb3b36dcf95746fdf3d843f66989b0f1c0 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik 
Date: Thu, 10 Mar 2016 10:26:52 +0100
Subject: [PATCH] BUILD: Remove detection of libcheck

The unit test framework check has not been used in freeipa for long time
(if ever) but there was still conditional check for this framework.
It just produced confusing warning:
Without the 'CHECK' library, you will be unable
to run all tests in the 'make check' suite
---

ACK


Pushed to master: dbc3a7511029dd954fff4cdb722f51e1f4e4b054

--
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] BUILD: Remove detection of libcheck

2016-04-22 Thread Petr Spacek 
On 10.3.2016 10:40, Lukas Slebodnik wrote:
> ehlo,
> 
> simple patch is attached.
> 
>>From 2b34cdbb3b36dcf95746fdf3d843f66989b0f1c0 Mon Sep 17 00:00:00 2001
> From: Lukas Slebodnik 
> Date: Thu, 10 Mar 2016 10:26:52 +0100
> Subject: [PATCH] BUILD: Remove detection of libcheck
> 
> The unit test framework check has not been used in freeipa for long time
> (if ever) but there was still conditional check for this framework.
> It just produced confusing warning:
> Without the 'CHECK' library, you will be unable
> to run all tests in the 'make check' suite
> ---

ACK

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

[Freeipa-devel] [PATCH 0096] Batch command: avoid accessing potentially undefined context.principa

2016-04-22 Thread Petr Spacek 
Hello,

Batch command: avoid accessing potentially undefined context.principal

This might happen when the command is called directly in Python,
e.g. in installers and so on.

Pylint pylint-1.5.5-1.fc24.noarch caught this.

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

-- 
Petr^2 Spacek
From f85a79c15752e0328c9b4c7d6c28fcdb7dbc0282 Mon Sep 17 00:00:00 2001
From: Petr Spacek 
Date: Fri, 22 Apr 2016 10:54:44 +0200
Subject: [PATCH] Batch command: avoid accessing potentially undefined
 context.principal

This might happen when the command is called directly in Python,
e.g. in installers and so on.

Pylint pylint-1.5.5-1.fc24.noarch caught this.

https://fedorahosted.org/freeipa/ticket/5838
---
 ipalib/plugins/batch.py | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/ipalib/plugins/batch.py b/ipalib/plugins/batch.py
index 2da7b7ca811fc67b22c43655352ace539488ce0d..51899f61a33cf77f90055a4908c3bc0168e988da 100644
--- a/ipalib/plugins/batch.py
+++ b/ipalib/plugins/batch.py
@@ -107,7 +107,10 @@ class batch(Command):
 
 result = api.Command[name](*a, **newkw)
 self.info(
-'%s: batch: %s(%s): SUCCESS', context.principal, name, ', '.join(api.Command[name]._repr_iter(**params))
+'%s: batch: %s(%s): SUCCESS',
+getattr(context, 'principal', ''),
+name,
+', '.join(api.Command[name]._repr_iter(**params))
 )
 result['error']=None
 except Exception as e:
-- 
2.5.5

-- 
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 0463] Performance: do not download password attributes in host/find-user command

2016-04-22 Thread Martin Basti 



On 21.04.2016 09:17, Martin Basti wrote:



On 20.04.2016 16:57, Martin Basti wrote:

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

Patch attached.



selfNACK



Updated patch attached.
From bc4c2a0b206b8b85c6c66088575efedc772e5a21 Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Fri, 8 Apr 2016 16:18:08 +0200
Subject: [PATCH] Performace: don't download password attributes in
 host/user-find

For each entry in user/host-find was executed an extra search for password
attributes what has significant impact on performance (for 2000 users
there were 2000 additional searches)

http://www.freeipa.org/page/V4/Performance_Improvements

https://fedorahosted.org/freeipa/ticket/5281
---
 ipalib/plugins/baseuser.py  | 1 -
 ipalib/plugins/host.py  | 5 -
 ipatests/test_xmlrpc/test_host_plugin.py| 2 +-
 ipatests/test_xmlrpc/tracker/host_plugin.py | 6 --
 ipatests/test_xmlrpc/tracker/user_plugin.py | 8 ++--
 5 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/ipalib/plugins/baseuser.py b/ipalib/plugins/baseuser.py
index 252d40ae3828417d9692510d5036aaadaeb9edce..cb6bf263160d33d396d740d6e25be811cf816c71 100644
--- a/ipalib/plugins/baseuser.py
+++ b/ipalib/plugins/baseuser.py
@@ -632,7 +632,6 @@ class baseuser_find(LDAPSearch):
 
 def post_common_callback(self, ldap, entries, lockout=False, **options):
 for attrs in entries:
-self.obj.get_password_attributes(ldap, attrs.dn, attrs)
 self.obj.convert_usercertificate_post(attrs, **options)
 if (lockout):
 attrs['nsaccountlock'] = True
diff --git a/ipalib/plugins/host.py b/ipalib/plugins/host.py
index 19327d8323d945062e06ccdb33ea2106cd1c6a00..611f9a9c7c51d5f81ba9ca91c6a900028aa249f4 100644
--- a/ipalib/plugins/host.py
+++ b/ipalib/plugins/host.py
@@ -1032,12 +1032,7 @@ class host_find(LDAPSearch):
 set_certificate_attrs(entry_attrs)
 set_kerberos_attrs(entry_attrs, options)
 rename_ipaallowedtoperform_from_ldap(entry_attrs, options)
-self.obj.get_password_attributes(ldap, entry_attrs.dn, entry_attrs)
 self.obj.suppress_netgroup_memberof(ldap, entry_attrs)
-if entry_attrs['has_password']:
-# If an OTP is set there is no keytab, at least not one
-# fetched anywhere.
-entry_attrs['has_keytab'] = False
 
 if options.get('all', False):
 entry_attrs['managing'] = self.obj.get_managed_hosts(entry_attrs.dn)
diff --git a/ipatests/test_xmlrpc/test_host_plugin.py b/ipatests/test_xmlrpc/test_host_plugin.py
index 47f05a403ddb519f201b11251c2acb71faa9133b..ea8f49656b47b91950130f78375f1c4447157ecb 100644
--- a/ipatests/test_xmlrpc/test_host_plugin.py
+++ b/ipatests/test_xmlrpc/test_host_plugin.py
@@ -416,7 +416,7 @@ class TestManagedHosts(XMLRPC_test):
 count=1,
 truncated=False,
 summary=u'1 host matched',
-result=[host.filter_attrs(host.retrieve_keys)],
+result=[host.filter_attrs(host.find_keys)],
 ), result)
 
 def search_man_hosts(self, host1, host2):
diff --git a/ipatests/test_xmlrpc/tracker/host_plugin.py b/ipatests/test_xmlrpc/tracker/host_plugin.py
index 0a69d39c0b77f3f6a7f09ef0785a78143b586c8f..67faa1acf9eeb6174e8d09ca012ae20636fb0f51 100644
--- a/ipatests/test_xmlrpc/tracker/host_plugin.py
+++ b/ipatests/test_xmlrpc/tracker/host_plugin.py
@@ -42,6 +42,8 @@ class HostTracker(Tracker):
 update_keys = retrieve_keys - {'dn'}
 managedby_keys = retrieve_keys - {'has_keytab', 'has_password'}
 allowedto_keys = retrieve_keys - {'has_keytab', 'has_password'}
+find_keys = retrieve_keys - {'has_keytab', 'has_password'}
+find_all_keys = retrieve_all_keys - {'has_keytab', 'has_password'}
 
 def __init__(self, name, fqdn=None, default_version=None):
 super(HostTracker, self).__init__(default_version=default_version)
@@ -136,9 +138,9 @@ class HostTracker(Tracker):
 def check_find(self, result, all=False, raw=False):
 """Check `host_find` command result"""
 if all:
-expected = self.filter_attrs(self.retrieve_all_keys)
+expected = self.filter_attrs(self.find_all_keys)
 else:
-expected = self.filter_attrs(self.retrieve_keys)
+expected = self.filter_attrs(self.find_keys)
 assert_deepequal(dict(
 count=1,
 truncated=False,
diff --git a/ipatests/test_xmlrpc/tracker/user_plugin.py b/ipatests/test_xmlrpc/tracker/user_plugin.py
index 216112db50ff909846e7bb900e76e961177cd10b..5acfc63cd62e28c7dd3ce1060bbf4fd262920f22 100644
--- a/ipatests/test_xmlrpc/tracker/user_plugin.py
+++ b/ipatests/test_xmlrpc/tracker/user_plugin.py
@@ -51,8 +51,12 @@ class UserTracker(Tracker):
 update_keys = retrieve_keys - {u'dn'}
 activate_keys = retrieve_keys
 
-find_keys = retrieve_keys - {u'mepmanagedentry',

Re: [Freeipa-devel] [PATCH 0024] ipa-replica-manage: added --suffix option for certain commands

2016-04-22 Thread Martin Basti 



On 15.04.2016 14:30, Stanislav Laznicka wrote:

On 04/13/2016 01:40 PM, Martin Basti wrote:



On 06.04.2016 14:04, Stanislav Laznicka wrote:

On 03/30/2016 04:52 PM, Martin Basti wrote:

On 24.03.2016 19:10, Stanislav Laznicka wrote:

On 03/23/2016 08:13 PM, Martin Basti wrote:

[...]
Can you please update design
http://www.freeipa.org/page/V4/Manage_replication_topology_4_4 
(mainly

the --suffix option)? Also there are missing clean-ruv and list-ruv
commands in design, and fix usage at the bottom.

1)
I don't understand this expression
+if dirman_passwd is None or (
+   not dirman_passwd and args[0] in 
cs_enabled_commands):


You already tested if subcommand belongs to cs_enabled_commands few
lines above, IMO the 'dirman_password is None' expression is enough.
If I understand it well, when empty password is entered, the 
program continues and uses Kerberos credentials for some 
operations. E.g. for list-ruv, if empty password is entered, the 
command would only display RUVs for domain tree but not for the CA 
tree no matter if CA is set up or not (in the current state of the 
patch, after get_ruv refactoring). This here is one possible way 
around this, although the check for non-empty password might 
probably just as well be in get_ruv_both_suffixes.


ok

2)
+# tuple of commands that work with ca tree and need Directory 
Manager

password
+cs_enabled_commands = ("list-ruv", "clean-ruv", "abort-clean-ruv")

this variable is used only toi detect if dirman passwd is needed, I
suggest to rename it to commands_req_dirman_passwd, or something 
better.


3)
Q: Do we need is_cs_set() function?
A: Yes!

I wanted to give you ultimate NACK, but then I checked how 
get_ruv code

works and I changed my mind.

Please write a comment where is_cs_set function is used, why we need
extra function instead of catching an exception, possibly you can 
open a

refactoring ticket.
After the refactoring changes, is_cs_set should not be needed 
anymore, removed it.


Shame:
1)
+if not test_connection(realm, host, options.nolookup) or\
Please use parentheses instead of backslash

2)
+   args[0] in cs_enabled_commands:

+   not dirman_passwd and args[0] in 
cs_enabled_commands):


Indentation is not multiplication of 4

Shame on me indeed, fixed it.


Nitpicks (I don't insist on fixing these):
1)
+if servers.get('ca', None):

None is default

2)
+for (netloc, rid) in servers['ca']:
parentheses are not needed

3)
+ print("\t%s: %s" % (netloc, rid))
Would be nice to use .format() instead of %

Martin^2



I changed my mind, ultimate NACK.
Please fix get_ruv function, is_cs_set will not help. In case 
there are no RUVs but CA is installed, sys.exit there prevents us 
from removing RUVs (or any else operation) on domain suffix, and 
vice versa.
I propose to move ticket to 4.4 milestone because I would like to 
avoid breaking something in 4.3, as this will hit many places in 
ipa-replica-manage.


Please provide the refactoring of get_ruv as separate patch a put 
these patches on top of it.


Martin2
Did the refactoring. There also seemed to be duplicit code in 
abort_clean_ruv for some reason, removed it (I hope it does not 
break anything, but it seemed rather useless). Also had to change 
the numbers of the patches so that they would apply.


NACK

* ipa-replica-manage refactoring *

1)
Instead of:
-print("Failed to connect to server %s: %s" % (host, e))
-sys.exit(1)
+root_logger.debug("Failed to connect to server {host}: {err}"
+  .format(host=host, err=e))
+raise RuntimeError(e)

I expected

-print("Failed to connect to server %s: %s" % (host, e))
-sys.exit(1)
+root_logger.debug(traceback.format_exc())
+raise RuntimeError("Failed to connect to server {host}: 
{err}"

+  .format(host=host, err=e)))

Should be fixed now.


2)
-print("No RUV records found.")
-sys.exit(0)

Here is exit state 0, so we should not raise error.

I think that we should create new nonfatal exception.
Made a new nonfatal error exception, then. This step was a bit 
controversial when it comes to get_ruv_both_suffixes because it 
needs to catch both this new exception and a RuntimeError exception 
for both trees. As the main idea here was not to stop if either tree 
is missing (resulting in RuntimeError in get_ruv) or contains no 
records (NonFatalError), it is only printed that something bad may 
have happened (or it's debug-logged in case of nonfatal errors). In 
the end, only NonFatalError exception is raised if no records were 
found for whatever reason resulting in the script always returning 0.


3)
-print("unable to decode: %s" % ruv)
+root_logger.debug("unable to decode: %s" % ruv)
This may break tests, because the logger logs to stderr, leave it 
as print for now


4)
-servers = get_ruv(realm, host,

Re: [Freeipa-devel] [PATCH 0441] Configure httpd service from installer

2016-04-22 Thread Martin Basti 



On 22.04.2016 10:17, Stanislav Laznicka wrote:

On 04/22/2016 10:08 AM, Martin Basti wrote:



On 21.04.2016 22:55, Timo Aaltonen wrote:

21.04.2016, 20:50, Martin Basti kirjoitti:


On 21.04.2016 19:28, Stanislav Laznicka wrote:

On 04/21/2016 11:19 AM, Martin Basti wrote:


On 20.04.2016 17:27, Martin Basti wrote:


On 24.03.2016 14:27, Martin Basti wrote:


On 24.03.2016 13:55, Jan Cholasta wrote:

On 18.3.2016 23:27, Timo Aaltonen wrote:

On 17.03.2016 18:36, Martin Basti wrote:

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

would be nicer if ipa-httpd.conf was a template with the current
hardcoded values replaced with platform paths..

+1, I would also prefer if the file was renamed to
init/systemd/httpd.conf rather than install/share/ipa-httpd.conf.

ipa-httpd.conf.template should be in /user/share/ipa, directory
init/systemd copied only to rpm and then copied to
/etc/systemd/system AFAIK



not relevant to this patch, but there are others candidates for
templates like:

daemons/dnssec/ipa-dnskeysyncd.service
daemons/dnssec/ipa-ods-exporter.service
install/conf/ipa.conf

Updated patch attached, sorry for delay.



Updated patch attached (fixed unused import).



Seems to work as expected. However, wouldn't it be better to use
installutils.remove_file instead of remove_httpd_service_ipa_conf (or
at least log the possible error during os.unlink) to get the same
behavior as with the other config files?

It could be, but because I created platform specific method for adding
httpd service config, it seems natural to me to create inverse 
operation

platform specific too.
I have no strong opinion about this, Timo what might be better, you 
use

platform specific code more than we? :)

Well, with this patch in I'd just reuse the methods from
RedHatTaskNamespace() just like some others are being used right now.
Systemd is all I support anyway.




Updated patch attached, missing log added


Thanks, jolly good. ACK.


Pushed to master:
* 586fee293f42388510fa5436af19460bbe1fdec5 Configure httpd service from 
installer instead of directly from RPM


--
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 0088-0095] Add --forward-policy option into installers

2016-04-22 Thread Petr Spacek 
On 12.4.2016 17:26, Martin Basti wrote:
> 
> 
> On 04.04.2016 17:37, Petr Spacek wrote:
>> On 31.3.2016 13:45, Martin Basti wrote:
>>>
>>> On 21.03.2016 16:51, Petr Spacek wrote:
 On 10.3.2016 22:17, Lukas Slebodnik wrote:
> On (10/03/16 22:14), Petr Spacek wrote:
>> Hello,
>>
>> I forgot to send a patches before I leave, so here it is:
>>
>> Auto-detect default value for --forward-policy option in installers
>>
>> See
>> https://fedorahosted.org/freeipa/ticket/5710
>> commit messages, and design page
>> https://fedorahosted.org/bind-dyndb-ldap/wiki/BIND9/Design/AutomaticEmptyZones
>>
>>
>>
>>
>> I did not have time to test it thoroughly but it LGTM :-D
>>
>> Please note that this is first part, it does not solve upgrade (yet) and
>> warnings in forwardzone-* interface.
>>
>> This can be solved in another patch set, this can be pushed if it passes
>> review.
>>
> ENOPATH
 LOL, here it is.



>>>   * Remove function ipapython.ipautil.host_exists() *
>>> ACK
>>>
>>>
>>> * Extend installers with --forward-policy option *
>>> 1)
>>> There is no --forward-policy option in ipa-dns-install
>>>
>>>
>>> * Move automatic empty zone list into ipapython.dnsutil and make it 
>>> reusable *
>>> ACK
>>>
>>>
>>> * Add assert_absolute_dnsname() helper to ipapython.dnsutil *
>>> ACK
>>>
>>>
>>> * Move function is_auto_empty_zone() into ipapython.dnsutil *
>>> ACK
>>>
>>>
>>> * Use shared sanity check and tests ipapython.dnsutil.is_auto_empty_zone() *
>>> ACK
>>>
>>> * Add function ipapython.dnsutil.inside_auto_empty_zone() *
>>> ACK
>>>
>>> * Auto-detect default value for --forward-policy option in installers *
>>> LGTM, but ipa-dns-install is missing option --forward-policy
>>>
>>> # ipa-dns-install
>>> ...
>>> Unexpected error - see /var/log/ipaserver-install.log for details:
>>> AttributeError: Values instance has no attribute 'forward_policy'
>>>
>>>
>>> Summary: 6 ACKs, 1 LGTM, 1 NACK => NACK
>> Thank you very much for review.
>>
>> Here is my second attempt :-)
>>
> Hello,
> code works as expected, but it is quite inconsistent with current behavior
> 
> ipa-server-install --forward-policy should raise error without --setup-dns 
> option
> 
> Like here:
> [root@vm-058-134 ~]# ipa-server-install --forwarder=10.2.3.4
> Usage: ipa-server-install [options]
> 
> ipa-server-install: error: You cannot specify a --forwarder option without the
> --setup-dns option
> ipa.ipapython.install.cli.install_tool(Server): ERRORThe
> ipa-server-install command failed. See /var/log/ipaserver-install.log for more
> information
> 
> Martin

Fixed patches are attached. Thank you for your time!

-- 
Petr^2 Spacek
From 186c73a9e92dd2c95c9f947a494aef6d66a0889b Mon Sep 17 00:00:00 2001
From: Petr Spacek 
Date: Mon, 7 Mar 2016 10:52:35 +0100
Subject: [PATCH] Remove function ipapython.ipautil.host_exists()

The function duplicated ipalib.util.verify_host_resolvable() in slightly
incompatible way because it used NSS while rest of IPA is using only DNS.
---
 install/tools/ipa-replica-manage | 12 
 ipapython/ipautil.py | 14 --
 2 files changed, 8 insertions(+), 18 deletions(-)

diff --git a/install/tools/ipa-replica-manage b/install/tools/ipa-replica-manage
index 6d1fc93c4c8499dcb4cd0924ff837a0216395821..3d2a467afd1c8552a64dd0f1c72efb92d9789bf1 100755
--- a/install/tools/ipa-replica-manage
+++ b/install/tools/ipa-replica-manage
@@ -38,7 +38,7 @@ from ipaserver.install import opendnssecinstance, dnskeysyncinstance
 from ipapython import version, ipaldap
 from ipalib import api, errors
 from ipalib.constants import CACERT
-from ipalib.util import has_managed_topology
+from ipalib.util import has_managed_topology, verify_host_resolvable
 from ipapython.ipa_log_manager import root_logger, standard_logging_setup
 from ipapython.dn import DN
 from ipapython.config import IPAOptionParser
@@ -743,10 +743,14 @@ def check_last_link(delrepl, realm, dirman_passwd, force):
 
 
 def enforce_host_existence(host, message=None):
-if host is not None and not ipautil.host_exists(host):
+if host is None:
+return
+
+try:
+verify_host_resolvable(host, root_logger)
+except errors.DNSNotARecordError as ex:
 if message is None:
-message = "Unknown host %s" % host
-
+message = "Unknown host %s: %s" % (host, ex)
 sys.exit(message)
 
 def ensure_last_services(conn, hostname, masters, options):
diff --git a/ipapython/ipautil.py b/ipapython/ipautil.py
index e595d80ca3b971ad2f0031972e9e58b5adff8e54..e6ce0541f56236c815eca7b92817b607b1fb4bf5 100644
--- a/ipapython/ipautil.py
+++ b/ipapython/ipautil.py
@@ -1015,20 +1015,6 @@ def bind_port_responder(port, socket_type=socket.SOCK_STREAM, socket_timeout=Non
 raise last_socket_error # pylint: disable=E0702
 
 
-def host_exists(host):
-"""
-Resolve the host to see if it exists.

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

2016-04-22 Thread Aleš Mareček 
Design doc reviewed. Some minor specifications discussed with Petr and Martin, 
added to the doc. UQE_ACK.
Thanks,
 - alich -

- Original Message -
> From: "Martin Basti" 
> To: "Simo Sorce" , "Petr Spacek" 
> Cc: freeipa-devel@redhat.com
> Sent: Thursday, April 21, 2016 7:39:02 PM
> Subject: Re: [Freeipa-devel] Locations design v2: LDAP schema & user  
> interface
> 
> 
> 
> On 21.04.2016 18:58, Simo Sorce wrote:
> > On Thu, 2016-04-21 at 17:39 +0200, Petr Spacek wrote:
> >> On 19.4.2016 19:17, Simo Sorce wrote:
> >>> On Tue, 2016-04-19 at 11:11 +0200, Petr Spacek wrote:
>  On 18.4.2016 21:33, Simo Sorce wrote:
> > On Mon, 2016-04-18 at 17:44 +0200, Petr Spacek wrote:
> >> * Find, filter and copy hand-made records from main tree into the
> >> _locations sub-trees. This means that every hand-made record
> >> needs to be copied and synchronized N-times where N = number of IPA
> >> locations.
> > This ^^ seem the one that provides the best semantics for admins and
> > the
> > least unexpected results.
> >
> >> My favorite option for the first version is 'document that enabling
> >> DNS location will hide hand-made records in IPA domain.'
> > I do not think this is acceptable, sorry.
> >
> >> The feature is disabled by default and needs additional configuration
> >> anyway so simply upgrading should not break anything.
> > It is also useless this way.
> >
> >> I'm eager to hear opinions and answers to questions above.
> > HTH,
>  Well it does not help because you did not answer the questions listed in
>  the
>  design page.
> 
>  Anyway, here is third version of the design. It avoids copying user-made
>  records (basically 2 DNAMEs were replaced with bunch of CNAMEs):
> 
>  http://www.freeipa.org/page/V4/DNS_Location_Mechanism#Design_.28Version_3:_CNAME_per_service_name.29
> 
>  It seems like a good middle ground:
>  http://www.freeipa.org/page/V4/DNS_Location_Mechanism#Comparison_of_proposals
> >>> It does seem like a decent middle ground.
> >>> And I guess an admin would be able to add custom templates if he wants
> >>> to have specific services forwarded to the location specific subtree ?
> >> Yes, the bind-dyndb-ldap's RecordGenerator and PerServerConfigInLDAP are
> >> generic enough. At the moment we do not plan to expose these mechanisms in
> >> user interface, we might do that later on.
> >>
> >>
>  This required changes in RecordGenerator design, too:
>  https://fedorahosted.org/bind-dyndb-ldap/wiki/Design/RecordGenerator
> >>> I do not see where you specify the specific record names you forward to
> >>> the location trees here?
> >> I do not understand the question. Let's have a look at the example:
> >>
> >> # DN specifies DNS node name which will hold the generated record:
> >> dn: idnsName=_udp,idnsname=example.com.,cn=dns,dc=example,dc=com
> >> # this is equivalent to _udp.example.com.
> >>
> >> objectClass: idnsTemplateObject
> >> objectClass: top
> >> objectClass: idnsRecord
> >> idnsName: _udp
> >>
> >> # sub-type determines type of the generated record = DNAME
> >> idnsTemplateAttribute;dnamerecord:
> >> _udp.\{substitutionvariable_ipalocation\}._locations
> >> # generated value will be _udp.your-location._locations
> >> # it is a relative name so zone name (example.com) will be automatically
> >> appended
> >>
> >> The template is just string, so you can specify an absolute name if you
> >> want:
> >> idnsTemplateAttribute;dnamerecord:
> >> _udp.\{substitutionvariable_ipalocation\}._locations.another.zone.example.
> >>
> >> Of course 'ipalocation' is just a variable name so user can define his own
> >> in
> >> PerServerConfigInLDAP.
> >>
> >> Is it clearer now?
> > Sorry I thought you said in option 3 that you would only create records
> > for specific services using CNAMEs
> > I was looking for how you configure which services you are going to pick
> > in that case and couldn't see it.
> > This example is a DNAME one and looks to me it is about option 2 ?
> >
> I put there image for version 3 there, and put/fix some implementation
> details there. I will add more implementation details tomorrow.
> 
> Basically, IPA knows what services are on which server (except NTP, will
> be fixed), so based on this we are able to generate proper SRV records
> in all locations, and mark the original one by attribute
> 'idnsTemplateAttribute;cnamerecord'  Please see example here, I will
> refer on it later
> http://www.freeipa.org/page/V4/DNS_Location_Mechanism#CNAME_data_generation
> 
> 
> In case that server is not configured to provide Location specific data,
> or the server is old, the original SRV record (marked with
> 'idnsTemplateAttribute') will be used. In case that server is configured
> to provide location specific data, bind-dyndb-ldap will replace the
> original SRV record by CNAME according to

Re: [Freeipa-devel] [python-pytest-multihost]-Ticket-6 run_command produces exit code 1 on windows

2016-04-22 Thread Petr Viktorin 
On 04/21/2016 03:04 PM, Niranjan wrote:
> Petr Viktorin wrote:
>> On 04/21/2016 07:25 AM, Niranjan wrote:
>>> Petr Viktorin wrote:
 On 04/20/2016 06:11 AM, Niranjan wrote:
> Petr Viktorin wrote:
>> On 04/18/2016 12:39 PM, Niranjan wrote:
>>> Niranjan wrote:
 Niranjan wrote:
> Petr Viktorin wrote:
>> On 04/06/2016 10:55 AM, Niranjan wrote:
>>> Greetings,
>>>
>>> For https://fedorahosted.org/python-pytest-multihost/ticket/6 , i 
>>> have proposed
>>> a patch, I think this patch is more of a workaround , than a 
>>> solution. I would
>>> like to get more inputs on how to use pytest-multihost to execute 
>>> commands 
>>> on Windows. The method i am using requires cygwin with openssh/bash 
>>> to be
>>> installed. 
>>
[...]
>>
>> If this works for you, I'll commit it and release.
> 
> With this latest patch it worked after removing the line "transport_class =
> transport.SSHTransport" from QeBaseHost class. 
> 
> Please go ahead and commit it.

I've commited it and released version 1.1. Packages for Fedora Rawhide
are being built; if you need this for older Fedoras let me know so I
don't forget to build there too.

Thanks for your assistance!


-- 
Petr Viktorin

-- 
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 0024] ipa-replica-manage: added --suffix option for certain commands

2016-04-22 Thread Stanislav Laznicka 

On 04/22/2016 01:13 PM, Martin Basti wrote:



On 15.04.2016 14:30, Stanislav Laznicka wrote:

On 04/13/2016 01:40 PM, Martin Basti wrote:



On 06.04.2016 14:04, Stanislav Laznicka wrote:

On 03/30/2016 04:52 PM, Martin Basti wrote:

On 24.03.2016 19:10, Stanislav Laznicka wrote:

On 03/23/2016 08:13 PM, Martin Basti wrote:

[...]
Can you please update design
http://www.freeipa.org/page/V4/Manage_replication_topology_4_4 
(mainly

the --suffix option)? Also there are missing clean-ruv and list-ruv
commands in design, and fix usage at the bottom.

1)
I don't understand this expression
+if dirman_passwd is None or (
+   not dirman_passwd and args[0] in 
cs_enabled_commands):


You already tested if subcommand belongs to cs_enabled_commands few
lines above, IMO the 'dirman_password is None' expression is 
enough.
If I understand it well, when empty password is entered, the 
program continues and uses Kerberos credentials for some 
operations. E.g. for list-ruv, if empty password is entered, the 
command would only display RUVs for domain tree but not for the 
CA tree no matter if CA is set up or not (in the current state of 
the patch, after get_ruv refactoring). This here is one possible 
way around this, although the check for non-empty password might 
probably just as well be in get_ruv_both_suffixes.


ok

2)
+# tuple of commands that work with ca tree and need Directory 
Manager

password
+cs_enabled_commands = ("list-ruv", "clean-ruv", "abort-clean-ruv")

this variable is used only toi detect if dirman passwd is needed, I
suggest to rename it to commands_req_dirman_passwd, or something 
better.


3)
Q: Do we need is_cs_set() function?
A: Yes!

I wanted to give you ultimate NACK, but then I checked how 
get_ruv code

works and I changed my mind.

Please write a comment where is_cs_set function is used, why we 
need
extra function instead of catching an exception, possibly you 
can open a

refactoring ticket.
After the refactoring changes, is_cs_set should not be needed 
anymore, removed it.


Shame:
1)
+if not test_connection(realm, host, options.nolookup) or\
Please use parentheses instead of backslash

2)
+   args[0] in cs_enabled_commands:

+   not dirman_passwd and args[0] in 
cs_enabled_commands):


Indentation is not multiplication of 4

Shame on me indeed, fixed it.


Nitpicks (I don't insist on fixing these):
1)
+if servers.get('ca', None):

None is default

2)
+for (netloc, rid) in servers['ca']:
parentheses are not needed

3)
+ print("\t%s: %s" % (netloc, rid))
Would be nice to use .format() instead of %

Martin^2



I changed my mind, ultimate NACK.
Please fix get_ruv function, is_cs_set will not help. In case 
there are no RUVs but CA is installed, sys.exit there prevents 
us from removing RUVs (or any else operation) on domain suffix, 
and vice versa.
I propose to move ticket to 4.4 milestone because I would like 
to avoid breaking something in 4.3, as this will hit many places 
in ipa-replica-manage.


Please provide the refactoring of get_ruv as separate patch a 
put these patches on top of it.


Martin2
Did the refactoring. There also seemed to be duplicit code in 
abort_clean_ruv for some reason, removed it (I hope it does not 
break anything, but it seemed rather useless). Also had to change 
the numbers of the patches so that they would apply.


NACK

* ipa-replica-manage refactoring *

1)
Instead of:
-print("Failed to connect to server %s: %s" % (host, e))
-sys.exit(1)
+root_logger.debug("Failed to connect to server {host}: 
{err}"

+  .format(host=host, err=e))
+raise RuntimeError(e)

I expected

-print("Failed to connect to server %s: %s" % (host, e))
-sys.exit(1)
+root_logger.debug(traceback.format_exc())
+raise RuntimeError("Failed to connect to server {host}: 
{err}"

+  .format(host=host, err=e)))

Should be fixed now.


2)
-print("No RUV records found.")
-sys.exit(0)

Here is exit state 0, so we should not raise error.

I think that we should create new nonfatal exception.
Made a new nonfatal error exception, then. This step was a bit 
controversial when it comes to get_ruv_both_suffixes because it 
needs to catch both this new exception and a RuntimeError exception 
for both trees. As the main idea here was not to stop if either 
tree is missing (resulting in RuntimeError in get_ruv) or contains 
no records (NonFatalError), it is only printed that something bad 
may have happened (or it's debug-logged in case of nonfatal 
errors). In the end, only NonFatalError exception is raised if no 
records were found for whatever reason resulting in the script 
always returning 0.


3)
-print("unable to decode: %s" % ruv)
+root_logger.debug("unable to decode: %s" % ruv)
This may break tests, because the logger logs to stderr, leave it 
as print for now