Re: [Freeipa-devel] [PATCH] 0005 webui: topology graph: canvas resizes itself according to the window size

[Pavel Vomacka]

On 03/16/2016 06:34 PM, Petr Vobornik wrote:

On 03/11/2016 10:09 AM, Pavel Vomacka wrote:



On 03/10/2016 06:08 PM, Petr Vobornik wrote:

On 02/25/2016 03:50 PM, Pavel Vomacka wrote:



On 02/17/2016 06:29 PM, Petr Vobornik wrote:

On 02/15/2016 04:20 PM, Pavel Vomacka wrote:



On 02/12/2016 01:52 PM, Pavel Vomacka wrote:



On 02/11/2016 12:31 PM, Pavel Vomacka wrote:

Hello,

The canvas of the graph had static size. This patch fixes this 
issue

and from now the graph canvas is resized according to the window
size.

Pavel Vomacka


Because of changes in previous patch I'm sending also this one 
again.

Plus I fixed some jslint warnings.

And again a link to the ticket:
https://fedorahosted.org/freeipa/ticket/5647 .

--
Pavel^3 Vomacka


And another change in the code. This patch adds checking whether 
a svg

element even exists. And don't add 'col-sm-12' class to the svg
element
any more. This class just added useless paddings to the element.

--
Pavel^3 Vomacka



Hi,

thanks for the patch.

Hi,

thank you for reviewing.


1. I don't like the fact that the resize handler registered in
initialize method is active forever, even when viewing other facets.

I moved the handler to the topology graph facet. It is also removed
after hide event is emited.

2. The code will probably fail if there is other svg element present
on the page.

$('svg') searches for all svg elements in DOM, such search is usually
slow and undeterministic. It is better to use a stored reference(if
possible) or limit the search to some parent element, e.g. TopoGraph
can store and then use its container.

Would be funny if there were 2 graphs.
Yep, you are right. I avoid using this type of searching in this 
patch.




3. Why is there the toFixed(1) call? Or more specifically on that
position? It hides the fact that toFixed transforms Number to String
and then '-' operator with Number on the right casts it back to 
Number.
The toFixed(1) was used just because we don't need so accurate 
numbers,

but in this patch this function is not used any more.


4. width could be just: this._svg.parent().width()
The width is now solved by using this.content.width() in topology 
graph
facet. I think that the calculating of width and height should be 
at the

same place. That is why I didn't put calculating of width into the
TopoGraph.


5. Your approach for bottom padding works well but I don't like that
the component assumes that there is some col-sm-12 element on a page
whose right padding is actually equal to space on the left of the 
svg.

I agree, fixed.


#1 and #5 makes me think that the resize logic should be moved
topology facet. Something like:

* register resize handler on facet's 'show' event
* unregister resize handler on facet's 'hide' event (will solve #1)
* on window resize, compute the size in topology facet, call new
.resize(width, height) method of TopoGraph

Then, we wouldn't have to search whole DOM for 'svg' elements to 
check

if page is visible. The bottom padding can be obtained by:
parseInt(this.content.css('paddingLeft')) where 'this' is facet.


I followed these tips and here is a new patch.

--
Pavel^3 Vomacka



1.
-width: 960,
-height: 500,

Graph even without this patch allows to set initial size in a
constructor, e.g.:

E.g. so he could also use:
  this.graph = new topology_graph.TopoGraph({
 nodes: data.nodes,
 links: data.links,
 suffixes: data.suffixes
 height: height,
 width: width
 });

IMO we should leave some default size there, e.g. the old 960x500 so
that the graph is shown even without explicit configuration.


Ok, I put the default size back, but into graph specification as you
write here.


Ah, I badly expressed myself, sorry. I wanted to leave the original 
code on its place(TopoGraph). The above was just example what is 
possible with or without the change because it is not obvious from code.

Default size is returned back now.





2.
-update: function() {
+update: function(height, width) {

Update method should not required size params. E.g. if it should
trigger only data update. So it should contain at least a doc string
that the values are optional. Maybe it should be a single param.



These parameters are not required so I add doc string and also changed
them to single param.


Looks good.



--
Pavel^3 Vomacka





>From b222b002af92d5008f1f502f29ac02ac9b296aaa Mon Sep 17 00:00:00 2001
From: Pavel Vomacka 
Date: Thu, 25 Feb 2016 15:02:04 +0100
Subject: [PATCH] Resize topology graph canvas according to window size

The size of svg element is calculated when the topology graph facet is load
and then every time when the window is resized. The resize event listener
is removed after the topology graph facet emits hide event.

https://fedorahosted.org/freeipa/ticket/5647
---
 install/ui/src/freeipa/topology.js   | 48 +---
 install/ui/src/freeipa/topology_graph.js | 13 +
 2 files changed, 57 insertions(+), 4 deletions(-)

d

Re: [Freeipa-devel] [PATCH 0142] spec: require python-cryptography newer than 0.9

[Martin Basti]

On 18.03.2016 10:18, Martin Basti wrote:



On 17.03.2016 18:40, Martin Babinsky wrote:

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




ACK



Pushed to:
master: aa749957360b85fecaed2f9f8dc286f560b89e0b
ipa-4-3: 85d2cc054046aad9731f409bc92e1cbb5b09dde4

-- 
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 0143-0144] different errors/warnings for different LDAP limit type exceeded

[Petr Spacek]

On 21.3.2016 12:25, Jan Cholasta wrote:
> On 21.3.2016 10:17, Petr Spacek wrote:
>> On 18.3.2016 13:49, Rob Crittenden wrote:
>>> Martin Babinsky wrote:
 These patches implement behavior agreed upon during discussion of
 https://fedorahosted.org/freeipa/ticket/5677

 However I'm not sure if we want to push them into 4-3 branch (the ticket
 is triaged into 4.3.2 milestone) since they modify the framework
 behavior quite a bit.

 If there is no need to have it there (CC'ing Milan since he is the
 reporter), I would retriage it into 4.4 milestone.
>>>
>>>
>>> + desc="while getting entries (search base: '{}',"
>>> + "filter: {})".format(base_dn, filter))
>>>
>>> This is going to expose parts of the DIT in an error message to users. We 
>>> have
>>> tried in the past to hide the implementation. I'd propose logging the error
>>> and making the exception less verbose.
> 
> I agree with Rob here, we shouldn't expose internal stuff in error messages
> for users.
> 
> In this particular case, even if we included internal stuff in the error
> message, it should be the error message returned by the server rather than
> this ad-hoc message.
> 
>>
>> IMHO it actually helps to print the DN. At very least the user can see if the
>> error is happening always with the same DN or if it keeps changing.
>>
>> In other words, for user it is not that important to understand meaning of 
>> the
>> DN but it might be important to see if it is the same or not.
> 
> I can't imagine a situation where it would actually be useful for the user (as
> opposed to the admin, who has access to logs) to know the base DN of some
> arbitrary LDAP search operation. Could you give an example?

I meant anyone who is facing an issue. I always prefer messages 'operation XYZ
failed' over 'an operation failed'. Anyway, you are right that admin can look
into logs so I'm pulling my comment above.

-- 
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] [PATCHES 0442-0449] Pylint: sunday code cleanup

[Petr Spacek]

On 21.3.2016 13:50, Lukas Slebodnik wrote:
> On (21/03/16 12:30), Martin Basti wrote:
>> On 21.03.2016 10:33, Christian Heimes wrote:
>>> On 2016-03-21 10:29, Petr Spacek wrote:
 On 20.3.2016 21:56, Martin Basti wrote:
> Patches attached.
 I do not really like
 freeipa-mbasti-0442-pylint-remove-bare-except
 because it replaces most of

 try: ... except:

 with

 try: ... except Exception:


 which AFAIK does not add any value. It would be better to replace Exception
 with more specific exception so the code raises an error instead of 
 continuing
 when something really unexpected happens.
>>> It adds some value. A bare except also excepts signals like
>>> KeyboardInterrupt and SystemExit. except Exception doesn't block these
>>> exceptions.
>>>
>>> But yes, more specific exceptions are better.
>>>
>>> Christian
>>>
>>>
>>>
>>>
>> 'except Exception' is another pylint check :D
>>
>> I replaced bare except with a particular exception in cases where it was
>> clear. For other occurrences of bare except it covers too much Exception
>> types, so catch Exception is more sensible, or I need crystal ball to detect
>> what kind of exceptions can be raised there.
>>
> Agree.
> 
> It can be changed to more specific exceptions type of Exception in future.
> This change is less risky.
> 
> pylint passed on fedora {23, 24, rawhide}
> 
> ACK

Okay, it makes sense.

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

[Petr Spacek]

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.

-- 
Petr^2 Spacek
From 14fc37af42fedc230a6e2f8a942d9ca16abd36ff 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 0497a0f0549b392216c654ab67d98cedbf122a4b..cf558977cb9dbf3bb1a0ae8dbd6ad9be1955be26 100755
--- a/install/tools/ipa-replica-manage
+++ b/install/tools/ipa-replica-manage
@@ -37,7 +37,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
@@ -742,10 +742,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 95357fa54ecf7f836e07de4a4322a9040c57c87e..7c2e3bc189c22f803e8685e9cd29c5edcc1e8e40 100644
--- a/ipapython/ipautil.py
+++ b/ipapython/ipautil.py
@@ -1013,20 +1013,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.
-
-Returns True/False
-"""
-try:
-socket.getaddrinfo(host, 80)
-except socket.gaierror:
-return False
-else:
-return True
-
-
 def reverse_record_exists(ip_address):
 """
 Checks if IP address have some reverse record somewhere.
-- 
2.5.0

From 5e1a8528293f21cd2efe40fdf612295b10a6a508 Mon Sep 17 00:00:00 2001
From: Petr Spacek 
Date: Tue, 1 Mar 2016 11:13:18 +0100
Subject: [PATCH] Extend installers with --forward-policy option

This option specified forward policy for global forwarders.
The value is put inside /etc/named.conf.

https://fedorahosted.org/freeipa/ticket/5710
---
 install/share/bind.named.conf.template  | 2 +-
 install/tools/man/ipa-dns-install.1 | 3 +++
 install/tools/man/ipa-replica-install.1 | 3 +++
 install/tools/man/ipa-server-install.1  | 3 +++
 ipaserver/install/bindinstance.py   | 7 +--
 ipaserver/install/dns.py| 4 ++--
 ipaserver/install/server/common.py  | 5 +
 ipaserver/install/server/install.py | 5 +++--
 8 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/install/share/bind.named.conf.template b/install/share/bind.named.conf.template
index 3c19383c0dde4353b30b16240ec2b81d7ea65776..eb6d4ae278f8646a25e18ed95989b8b0c4d89d28 100644
--- a/install/share/bind.named.conf.template
+++ b/install/share/bind.named.conf.template
@@ -8,7 +8,7 @@ options {
 	statistics-file		"data/named_stats.txt";
 	memstatistics-file	"data/named_mem_stats.txt";
 
-	forward first;
+	forward $FORWARD_POLICY;
 	forwarders {$FORWARDERS};
 
 	// Any host is permitted to issue recursive queries
diff --git a/install/tools/man/ipa-dns-install.1 b/install/tools/man/ipa-dns-install.1
index a997eb84153fd0b28a8fcbf6a475a3a0361429fe..e3739e2bbeeccfb24721faa0a5da5600ee145f7c 100644
--- a/install/tools/man/ipa-dns-install.1
+++ b/install/tools/man/ipa-dns-install.1
@@ -41,6 +41,9 @@ Do not add any DNS forwarders, send non\-resolvable addresses to the DNS root se
 \fB\-\-auto\-forwarders\fR
 Add DNS forwarders c

[Freeipa-devel] Announcing FreeIPA 4.2.4

[Petr Vobornik]

The FreeIPA team would like to announce FreeIPA v4.2.4 bug fixing release!

It can be downloaded from http://www.freeipa.org/page/Downloads. The 
builds are available for Fedora 23.


https://bodhi.fedoraproject.org/updates/freeipa-4.2.4-1.fc23

This release notes are also available on 
http://www.freeipa.org/page/Releases/4.2.4


== Highlights in 4.2.4 ==

FreeIPA 4.2.4 is a bugfix release to improve upgrade experience from 
FreeIPA 4.1 for Fedora 23.


=== Bug fixes ===
* Fixed issue in installation of server with external CA where second 
step of installation "forgot" options from previous step which could 
lead, e.g., to DNS server not being installed. #5556
* Fixed issue in ipa-adtrust-install when a dash character was used in 
NetBIOS name
* Fixed issue with migration from old self-sign IPA(e.g. CentOS 6) and 
upgrading it to a server with CA #5611, #5598, #5602, #5595, #5636, 
#4492, #5506
* Fixed issue with bind not starting after update due to wrong file 
permissions. #5520
* Fixed issue in installation of server without CA when certmonger was 
not running. #5519

* Fixed issue in upgrade of NIS maps. #5507
* Fixed issue in handling of empty cookies. It prevented users from log 
in to Web UI using forms-based authentication. #5709

* Fixed issue with installation of KRA on a replica. #5346
* Fixed issue with DNSSEC key purging not being handled properly #5334
* Fixed issue in replica installation after update of master from 
previous version where certificate profiles and CA ACL were not properly 
added. #5269
* Fixed issue in installation of replica with external CA, when multiple 
certificates with the same nickname were provided. #5117
* Fixed issue after upgrade of sidgen and extdom plugins which prevented 
from generation of Security Identifiers(SIDs). As a result, all AD trust 
created after the upgrade did not work while advertising that the trust 
was established correctly. #5665
* Fixed issue with starting FreeIPA after upgrade which happened when 
FreeIPA server was turned off. #5655
* Fixed internal error during an upgrade from FreeIPA 4.0 to 4.2 which 
prevented the upgrade process from upgrading forward zones properly. #5472
* Fixed issue with missing "System: Read Replication Agreements" ACI on 
new replicas. #5631
* Fixed issue on Web UI password reset page where user was not notified 
when he entered invalid password #5567


=== Enhancements ===
* ipa-replica-prepare and ipa-replica-install no longer fails if PTR 
record is not resolvable #5686


== Upgrading ==
Upgrade instructions are available on upgrade 
page.


== Feedback ==
Please provide comments, bugs and other feedback via the freeipa-users 
mailing list (http://www.redhat.com/mailman/listinfo/freeipa-users) or 
#freeipa channel on Freenode.


== Detailed Changelog since 4.2.3 ==
=== Abhijeet Kasurde (2) ===
* Fixed small typo in stage-user documentation
* Fixed login error message box in LoginScreen page

=== Alexander Bokovoy (1) ===
* slapi-nis: update configuration to allow external members of IPA groups

=== Christian Heimes (1) ===
* Require Dogtag 10.2.6-13 to fix KRA uninstall

=== David Kupka (5) ===
* ipa-cacert-renew: Fix connection to ldap.
* ipa-otptoken-import: Fix connection to ldap.
* test: Temporarily increase timeout in vault test.
* installer: Propagate option values from components instead of copying 
them.

* installer: Fix logic of reading option values from cache.

=== Fraser Tweedale (5) ===
* TLS and Dogtag HTTPS request logging improvements
* Avoid race condition caused by profile delete and recreate
* Do not erroneously reinit NSS in Dogtag interface
* Add profiles and default CA ACL on migration
* Do not decode HTTP reason phrase from Dogtag

=== Gabe Alford (2) ===
* Incomplete ports for IPA AD Trust
* Check if IPA is configured before attempting a winsync migration

=== Jan Cholasta (9) ===
* install: fix command line option validation
* install: export KRA agent PEM file in ipa-kra-install
* cert renewal: make renewal of ipaCert atomic
* client install: do not corrupt OpenSSH config with Match sections
* ipalib: assume version 2.0 when skip_version_check is enabled
* cert renewal: import all external CA certs on IPA CA cert renewal
* CA install: explicitly set dogtag_version to 10
* replica install: validate DS and HTTP server certificates
* certdb: never use the -r option of certutil

=== Lenka Doudova (2) ===
* Adding descriptive IDs to stageuser tests
* Tests: Fix tests for (stage)user plugin

=== Martin Babinsky (13) ===
* fix error reporting when installer option is supplied with invalid choice
* suppress errors arising from adding existing LDAP entries during KRA 
install

* update idrange tests to reflect disabled modification of local ID ranges
* disconnect ldap2 backend after adding default CA ACL profiles
* do not disconnect when using existing connection to check default CA ACLs
* fix error message assertion in negative forced client reenrollm

Re: [Freeipa-devel] [PATCH 0143-0144] different errors/warnings for different LDAP limit type exceeded

[Petr Vobornik]

On 03/21/2016 12:25 PM, Jan Cholasta wrote:

On 21.3.2016 10:17, Petr Spacek wrote:

On 18.3.2016 13:49, Rob Crittenden wrote:

Martin Babinsky wrote:

These patches implement behavior agreed upon during discussion of
https://fedorahosted.org/freeipa/ticket/5677

However I'm not sure if we want to push them into 4-3 branch (the
ticket
is triaged into 4.3.2 milestone) since they modify the framework
behavior quite a bit.

If there is no need to have it there (CC'ing Milan since he is the
reporter), I would retriage it into 4.4 milestone.



+ desc="while getting entries (search base: '{}',"
+ "filter: {})".format(base_dn, filter))

This is going to expose parts of the DIT in an error message to
users. We have
tried in the past to hide the implementation. I'd propose logging the
error
and making the exception less verbose.


I agree with Rob here, we shouldn't expose internal stuff in error
messages for users.

In this particular case, even if we included internal stuff in the error
message, it should be the error message returned by the server rather
than this ad-hoc message.



IMHO it actually helps to print the DN. At very least the user can see
if the
error is happening always with the same DN or if it keeps changing.

In other words, for user it is not that important to understand
meaning of the
DN but it might be important to see if it is the same or not.


I can't imagine a situation where it would actually be useful for the
user (as opposed to the admin, who has access to logs) to know the base
DN of some arbitrary LDAP search operation. Could you give an example?



+1 for the internal info to be only in logs.
--
Petr Vobornik

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

Re: [Freeipa-devel] [PATCH 016, 024, 025] First part of the replica promotion tests + testplan

[Oleg Fayans]

Hi Lukas, Martin,

Looks I've implemented the approach proposed by Martin. The issue seems
to have gone (see the external_ca_out for external_ca test output).
Would like you to take a look and tell me what'd you think.


On 03/17/2016 08:37 PM, Lukas Slebodnik wrote:
> On (17/03/16 16:00), Oleg Fayans wrote:
>> Hi Lukas,
>>
>> On 03/17/2016 11:28 AM, Lukas Slebodnik wrote:
>>> On (10/03/16 23:09), Oleg Fayans wrote:
 Hi Martin,



 On 03/08/2016 08:18 PM, Martin Basti wrote:
>
>
> On 08.03.2016 18:24, Martin Basti wrote:
>>
>>
>> On 08.03.2016 12:38, Oleg Fayans wrote:
>>> The patches were rebased against the current master
>>>
>>> On 03/04/2016 05:33 PM, Martin Basti wrote:
 * old messages have been removed *
 1)
 this method is unused please remove it

   def test_kra_install_master(self):
> Well, in fact it is used twice: in both domain levels, so I'd better
> keep it:
>
> -bash-4.3$ ipa-run-tests test_integration/test_replica_promotion.py
> --collect-only
> 
>
>
> test session starts
> =
>
>
> platform linux2 -- Python 2.7.10 -- py-1.4.30 -- pytest-2.7.3
> rootdir: /usr/lib/python2.7/site-packages/ipatests, inifile:
> pytest.ini
> plugins: sourceorder, multihost
> collected 8 items
> 
> 
>   
> 
> 
> 
> 
>   
> 
> 
>   
> 
> 
>   
> 
> 
> 
>   
> 
 aah my bad, I forgot that pytest executes it when it begins with test_*
 even in parent class
 2)
 Why are these there? I do not see any usage

 from env_config import get_global_config
 config = get_global_config()
> Removed
>
 3) nitpick
 +num_clients = 0
 this is set by default
> Removed
>
 otherwise LGTM

 Results of testing tomorrow.

 Martin^2

>>> I applied all patches including workarounds, but test failed.
>>>
>>> ipatests.test_integration.test_replica_promotion.TestReplicaPromotionLevel0
>>>
>>>
>>>
>>>
>>> [ipa.ipatests.test_integration.host.Host.replica1.cmd51] RUN
>>> ['ipa-replica-install', '-U', '-p', 'Secret123', '-w', 'Secret123',
>>> '--setup-ca', '--ip-address', '192.168.144.102',
>>> '/root/ipatests/replica-info.gpg']
>>> [ipa.ipatests.test_integration.host.Host.replica1.cmd51] The host
>>> replica1.ipa.test already exists on the master server.
>>> [ipa.ipatests.test_integration.host.Host.replica1.cmd51] You should
>>> remove it before proceeding:
>>> [ipa.ipatests.test_integration.host.Host.replica1.cmd51] % ipa
>>> host-del replica1.ipa.test
>>> [ipa.ipatests.test_integration.host.Host.replica1.cmd51]
>>> ipa.ipapython.install.cli.install_tool(Replica): ERRORThe
>>> ipa-replica-install command failed. See
>>> /var/log/ipareplica-install.log for more information
>>> [ipa.ipatests.test_integration.host.Host.replica1.cmd51] Exit
>>> code: 3
>>> FAILED
> this is exactly the error that happens when a workaround for 5627
> is not
> applied. I have re-run the tests with all the patches and everything
> passed. Could you please double-check, whether patch 0027 was applied
> correctly?
>
> bash-4.3$ ipa-run-tests test_integration/test_replica_promotion.py
> --pdb
> 
>
>
> test session starts
> =
>
>
> platform linux2 -- Python 2.7.10 -- py-1.4.30 -- pytest-2.7.3
> rootdir: /usr/lib/python2.7/site-packages/ipatests, inifile:
> pytest.ini
> plugins: sourceorder, multihost
> collected 8 items
>
> test_integration/test_replica_promotion.py 
>
> 
>
>
> 8 passed in 7561.93 seconds
> =
>
>
>
 I will
>

Re: [Freeipa-devel] user-* commands performance issues

[Petr Vobornik]

On 03/21/2016 01:02 PM, Martin Basti wrote:



On 21.03.2016 12:44, Jan Cholasta wrote:

On 21.3.2016 11:00, Petr Vobornik wrote:

On 03/17/2016 04:09 PM, Martin Basti wrote:

Hello all,

I would like to discuss the way how we should improve the speed of
user-find commands (and other commands too if possible):

0)
Do not do extra search for ipasshpubkey. This is clear, patch posted
for
review.
https://fedorahosted.org/freeipa/ticket/3376

commands: user, stageuser, host, idview

1)
make --no-members option visible in CLI
https://fedorahosted.org/freeipa/ticket/4995


There was a discussion around devconf that --no-members should be a
default behavior of xxx-find commands and I'm for it.


+1, although we should be backward compatible with old clients which
expect the attributes to be there.

Ok, I agree to have --no-members as default for *-find commands, but it
doesn't contradict to exposing --no-member option for all commands.


+1, xxx-show  can have --no-members







Reasoning: use case: 'find me all groups which satisfy this filter'.
Showing members clutters the output(one group with >500 member makes it
unusable) and makes things slow(both on server and CLI side).

For xxx-show commands it is a question where I don't have a strong
opinion.


I think it shouldn't hurt to keep them in -show commands, as there is
always only a single entry to process.

+1







I don't think we should implement also --no-indirect-members, I think
that this kind of granularity is not needed.
If --no-members is used, then indirect members will be ignored too.


+1


+1





commands: all which use members

2)
Limit the amount of searches for memberof[indirect] (group, netgroup,
role, hbacrule, sudorule) and search for each dn only once in find
commands.

We can have configurable option in default.conf (for example
memberof_search_limit=100 (0 unlimited)). Find commands will get
members
only for specified amount and if this limit is exceeded a warning
message is shown.
I do not like this idea much, I think it should be all or nothing, I
prefer to not do this.


+1


I'd also avoid anything special here. But there are sometimes cases when 
the behavior is not good. E.g. a command fails because something is not 
able to get members and you actually don't care about the members. Not 
sure if it is was "fixed"(sizelimit=0). But with new member handling it 
might not be a big issue.






However I like the idea of temporary caching inside find commands,
where
each memberof DN is resolved just once and results are cached in a map
and reused in current context of command. This should be improvement
mainly for indirect searches, but cache should be faster for direct
members than doing internal calls of framework objects. This part is
backward compatible, the first part is not.

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


What parts of the ticket can be solved with deref plugin? I guess we can
get the CNs, but not what is a direct member. Maybe it should be
discussed separately.


Indirect members are already resolved by a single LDAP search. What
kind of additional optimization would you like to do for them?


We can use deref plugin to get pkeys from one search in case that pkeys
are not part of DN. (I have to investigate if it is worth to do for
user-find, I'm not sure if any memberof attributes have pkey that is not
part of DN)


sudo rule, hbac rule



For indirect members, it is one search per entry, but for 1000 users, it
is 1000 searches and I would like to have just one for the particular
indirect member.


are we talking about user-find? If so then it is mostly solved with 
default --no-member style behavior.


But if a user or a group is  directly/indirectly a member of a lot of 
groups(1000) then it might become slow. But caching won't probably help 
much, not sure.











commands: user-find, stageuser-find, possibly all find commands

3)
Remove userPassword, krbPrincipalKey from search results
This change is not backward compatible, can we do this?

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

commands: user-find


I'm for it, would like to hear other opinions.

Note: it should be only in user-find commands. 'show' has to display it.


+1






--
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 0442-0449] Pylint: sunday code cleanup

[Lukas Slebodnik]

On (21/03/16 12:30), Martin Basti wrote:
>On 21.03.2016 10:33, Christian Heimes wrote:
>>On 2016-03-21 10:29, Petr Spacek wrote:
>>>On 20.3.2016 21:56, Martin Basti wrote:
Patches attached.
>>>I do not really like
>>>freeipa-mbasti-0442-pylint-remove-bare-except
>>>because it replaces most of
>>>
>>>try: ... except:
>>>
>>>with
>>>
>>>try: ... except Exception:
>>>
>>>
>>>which AFAIK does not add any value. It would be better to replace Exception
>>>with more specific exception so the code raises an error instead of 
>>>continuing
>>>when something really unexpected happens.
>>It adds some value. A bare except also excepts signals like
>>KeyboardInterrupt and SystemExit. except Exception doesn't block these
>>exceptions.
>>
>>But yes, more specific exceptions are better.
>>
>>Christian
>>
>>
>>
>>
>'except Exception' is another pylint check :D
>
>I replaced bare except with a particular exception in cases where it was
>clear. For other occurrences of bare except it covers too much Exception
>types, so catch Exception is more sensible, or I need crystal ball to detect
>what kind of exceptions can be raised there.
>
Agree.

It can be changed to more specific exceptions type of Exception in future.
This change is less risky.

pylint passed on fedora {23, 24, rawhide}

ACK

LS

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

[Freeipa-devel] [TEST][Patch-0030]Next part of replica promotion tests

[Oleg Fayans]

-- 
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
From a4f0bdbe3a8646070119ccb668ad0bfb8cfb269b Mon Sep 17 00:00:00 2001
From: Oleg Fayans 
Date: Mon, 21 Mar 2016 13:27:19 +0100
Subject: [PATCH] Added 5 more tests to Replica Promotion testsuite

https://fedorahosted.org/freeipa/ticket/5723
---
 .../test_integration/test_replica_promotion.py | 113 -
 1 file changed, 112 insertions(+), 1 deletion(-)

diff --git a/ipatests/test_integration/test_replica_promotion.py b/ipatests/test_integration/test_replica_promotion.py
index 9686c8f4931b1c8c40e60a7087c27380188d0d64..47e69c2ea8af349bd2955a32bc7f2a0275637628 100644
--- a/ipatests/test_integration/test_replica_promotion.py
+++ b/ipatests/test_integration/test_replica_promotion.py
@@ -170,6 +170,18 @@ class TestReplicaPromotionLevel1(ReplicaPromotionBase):
  " to generate replica file\n"
  "is supported only in 0-level IPA domain", 1)
 
+@replicas_cleanup
+def test_one_command_installation(self):
+"""
+TestCase:
+http://www.freeipa.org/page/V4/Replica_Promotion/Test_plan
+#Test_case:_Replica_can_be_installed_using_one_command
+"""
+self.replicas[0].run_command(['ipa-replica-install', '-p',
+ self.master.config.admin_password,
+ '-n', self.master.domain.name,
+ '-r', self.master.domain.realm])
+
 
 class TestReplicaManageCommands(IntegrationTest):
 topology = "star"
@@ -207,7 +219,7 @@ class TestReplicaManageCommands(IntegrationTest):
   ' deprecated with managed IPA replication'
   ' topology. Please use `ipa topologysegment-*`'
   ' commands to manage the topology', 1)
-tasks.create_segment(master, replica1, replica2)
+segment = tasks.create_segment(master, replica1, replica2)
 result4 = master.run_command(["ipa-replica-manage",
   "disconnect",
   replica1.hostname,
@@ -217,3 +229,102 @@ class TestReplicaManageCommands(IntegrationTest):
   ' deprecated with managed IPA replication'
   ' topology. Please use `ipa topologysegment-*`'
   ' commands to manage the topology', 1)
+
+# http://www.freeipa.org/page/V4/Replica_Promotion/Test_plan
+#Test_case:_ipa-csreplica-manage_connect_is_deprecated
+#_in_domain_level_1
+
+tasks.destroy_segment(master, segment[0]['name'])
+result5 = master.run_command(["ipa-csreplica-manage",
+  "connect",
+  replica1.hostname,
+  replica2.hostname,
+  '-p', master.config.dirman_password],
+ raiseonerr=False)
+assert_error(result5, "Creation of IPA CS replication agreement is"
+  " deprecated with managed IPA replication"
+  " topology", 1)
+tasks.create_segment(master, replica1, replica2)
+result6 = master.run_command(["ipa-csreplica-manage",
+  "disconnect",
+  replica1.hostname,
+  replica2.hostname,
+  '-p', master.config.dirman_password],
+ raiseonerr=False)
+assert_error(result6, "Removal of IPA CS replication agreement is"
+  " deprecated with managed IPA"
+  " replication topology", 1)
+
+
+class TestUnprivilegedUserPermissions(IntegrationTest):
+num_replicas = 1
+domain_level = DOMAIN_LEVEL_1
+
+@classmethod
+def install(cls, mh):
+tasks.install_master(cls.master, domain_level=cls.domain_level)
+
+def test_unprivileged(self):
+"""
+TestCase:
+http://www.freeipa.org/page/V4/Replica_Promotion/Test_plan
+#Test_case:_Unprivileged_users_are_not_allowed_to_enroll
+_and_promote_clients
+"""
+password = self.master.config.dirman_password
+new_password = '$ome0therPaaS'
+replica = self.replicas[0]
+adduser_stdin_text = "%s\n%s\n" % (self.master.config.admin_password,
+   self.master.config.admin_password)
+user_kinit_stdin_text = "%s\n%s\n%s\n" % (password, new_password,
+  new_password)
+tasks.kinit_admin(self.master)
+self.master.run_command(['ipa', 'user-add', 'testuser', '--password',
+ '--first', 'John', '--last', 'Donn'],
+ 

Re: [Freeipa-devel] user-* commands performance issues

[Martin Basti]

On 21.03.2016 12:44, Jan Cholasta wrote:

On 21.3.2016 11:00, Petr Vobornik wrote:

On 03/17/2016 04:09 PM, Martin Basti wrote:

Hello all,

I would like to discuss the way how we should improve the speed of
user-find commands (and other commands too if possible):

0)
Do not do extra search for ipasshpubkey. This is clear, patch posted 
for

review.
https://fedorahosted.org/freeipa/ticket/3376

commands: user, stageuser, host, idview

1)
make --no-members option visible in CLI
https://fedorahosted.org/freeipa/ticket/4995


There was a discussion around devconf that --no-members should be a
default behavior of xxx-find commands and I'm for it.


+1, although we should be backward compatible with old clients which 
expect the attributes to be there.
Ok, I agree to have --no-members as default for *-find commands, but it 
doesn't contradict to exposing --no-member option for all commands.






Reasoning: use case: 'find me all groups which satisfy this filter'.
Showing members clutters the output(one group with >500 member makes it
unusable) and makes things slow(both on server and CLI side).

For xxx-show commands it is a question where I don't have a strong 
opinion.


I think it shouldn't hurt to keep them in -show commands, as there is 
always only a single entry to process.

+1







I don't think we should implement also --no-indirect-members, I think
that this kind of granularity is not needed.
If --no-members is used, then indirect members will be ignored too.


+1


+1





commands: all which use members

2)
Limit the amount of searches for memberof[indirect] (group, netgroup,
role, hbacrule, sudorule) and search for each dn only once in find
commands.

We can have configurable option in default.conf (for example
memberof_search_limit=100 (0 unlimited)). Find commands will get 
members

only for specified amount and if this limit is exceeded a warning
message is shown.
I do not like this idea much, I think it should be all or nothing, I
prefer to not do this.


+1



However I like the idea of temporary caching inside find commands, 
where

each memberof DN is resolved just once and results are cached in a map
and reused in current context of command. This should be improvement
mainly for indirect searches, but cache should be faster for direct
members than doing internal calls of framework objects. This part is
backward compatible, the first part is not.

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


What parts of the ticket can be solved with deref plugin? I guess we can
get the CNs, but not what is a direct member. Maybe it should be
discussed separately.


Indirect members are already resolved by a single LDAP search. What 
kind of additional optimization would you like to do for them?


We can use deref plugin to get pkeys from one search in case that pkeys 
are not part of DN. (I have to investigate if it is worth to do for 
user-find, I'm not sure if any memberof attributes have pkey that is not 
part of DN)


For indirect members, it is one search per entry, but for 1000 users, it 
is 1000 searches and I would like to have just one for the particular 
indirect member.









commands: user-find, stageuser-find, possibly all find commands

3)
Remove userPassword, krbPrincipalKey from search results
This change is not backward compatible, can we do this?

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

commands: user-find


I'm for it, would like to hear other opinions.

Note: it should be only in user-find commands. 'show' has to display it.


+1



--
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] user-* commands performance issues

[Jan Cholasta]

On 21.3.2016 11:00, Petr Vobornik wrote:

On 03/17/2016 04:09 PM, Martin Basti wrote:

Hello all,

I would like to discuss the way how we should improve the speed of
user-find commands (and other commands too if possible):

0)
Do not do extra search for ipasshpubkey. This is clear, patch posted for
review.
https://fedorahosted.org/freeipa/ticket/3376

commands: user, stageuser, host, idview

1)
make --no-members option visible in CLI
https://fedorahosted.org/freeipa/ticket/4995


There was a discussion around devconf that --no-members should be a
default behavior of xxx-find commands and I'm for it.


+1, although we should be backward compatible with old clients which 
expect the attributes to be there.




Reasoning: use case: 'find me all groups which satisfy this filter'.
Showing members clutters the output(one group with >500 member makes it
unusable) and makes things slow(both on server and CLI side).

For xxx-show commands it is a question where I don't have a strong opinion.


I think it shouldn't hurt to keep them in -show commands, as there is 
always only a single entry to process.






I don't think we should implement also --no-indirect-members, I think
that this kind of granularity is not needed.
If --no-members is used, then indirect members will be ignored too.


+1


+1





commands: all which use members

2)
Limit the amount of searches for memberof[indirect] (group, netgroup,
role, hbacrule, sudorule) and search for each dn only once in find
commands.

We can have configurable option in default.conf (for example
memberof_search_limit=100 (0 unlimited)). Find commands will get members
only for specified amount and if this limit is exceeded a warning
message is shown.
I do not like this idea much, I think it should be all or nothing, I
prefer to not do this.


+1



However I like the idea of temporary caching inside find commands, where
each memberof DN is resolved just once and results are cached in a map
and reused in current context of command. This should be improvement
mainly for indirect searches, but cache should be faster for direct
members than doing internal calls of framework objects. This part is
backward compatible, the first part is not.

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


What parts of the ticket can be solved with deref plugin? I guess we can
get the CNs, but not what is a direct member. Maybe it should be
discussed separately.


Indirect members are already resolved by a single LDAP search. What kind 
of additional optimization would you like to do for them?






commands: user-find, stageuser-find, possibly all find commands

3)
Remove userPassword, krbPrincipalKey from search results
This change is not backward compatible, can we do this?

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

commands: user-find


I'm for it, would like to hear other opinions.

Note: it should be only in user-find commands. 'show' has to display it.


+1

--
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 0442-0449] Pylint: sunday code cleanup

[Martin Basti]

On 21.03.2016 10:33, Christian Heimes wrote:

On 2016-03-21 10:29, Petr Spacek wrote:

On 20.3.2016 21:56, Martin Basti wrote:

Patches attached.

I do not really like
freeipa-mbasti-0442-pylint-remove-bare-except
because it replaces most of

try: ... except:

with

try: ... except Exception:


which AFAIK does not add any value. It would be better to replace Exception
with more specific exception so the code raises an error instead of continuing
when something really unexpected happens.

It adds some value. A bare except also excepts signals like
KeyboardInterrupt and SystemExit. except Exception doesn't block these
exceptions.

But yes, more specific exceptions are better.

Christian





'except Exception' is another pylint check :D

I replaced bare except with a particular exception in cases where it was 
clear. For other occurrences of bare except it covers too much Exception 
types, so catch Exception is more sensible, or I need crystal ball to 
detect what kind of exceptions can be raised there.


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

Re: [Freeipa-devel] [PATCH 0143-0144] different errors/warnings for different LDAP limit type exceeded

[Jan Cholasta]

On 21.3.2016 10:17, Petr Spacek wrote:

On 18.3.2016 13:49, Rob Crittenden wrote:

Martin Babinsky wrote:

These patches implement behavior agreed upon during discussion of
https://fedorahosted.org/freeipa/ticket/5677

However I'm not sure if we want to push them into 4-3 branch (the ticket
is triaged into 4.3.2 milestone) since they modify the framework
behavior quite a bit.

If there is no need to have it there (CC'ing Milan since he is the
reporter), I would retriage it into 4.4 milestone.



+ desc="while getting entries (search base: '{}',"
+ "filter: {})".format(base_dn, filter))

This is going to expose parts of the DIT in an error message to users. We have
tried in the past to hide the implementation. I'd propose logging the error
and making the exception less verbose.


I agree with Rob here, we shouldn't expose internal stuff in error 
messages for users.


In this particular case, even if we included internal stuff in the error 
message, it should be the error message returned by the server rather 
than this ad-hoc message.




IMHO it actually helps to print the DN. At very least the user can see if the
error is happening always with the same DN or if it keeps changing.

In other words, for user it is not that important to understand meaning of the
DN but it might be important to see if it is the same or not.


I can't imagine a situation where it would actually be useful for the 
user (as opposed to the admin, who has access to logs) to know the base 
DN of some arbitrary LDAP search operation. Could you give an example?


--
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] Converting plugin output

[Jan Cholasta]

Hi,

On 18.3.2016 15:26, Christian Heimes wrote:

Hi,

I'd like to use FreeIPA's RPC interface from Ansible directly. But the
output of plugins is rather unfriendly and unpythonic:


print(api.Command.dnsconfig_show())

{u'result': {u'dn': u'cn=dns,dc=ipa,dc=example', u'idnsallowsyncptr':
(u'FALSE',)}, u'value': None, u'summary': None}

Please notice (u'FALSE',) instead of False.


This is how the framework does things - there is no internal consistency 
and no singular place where coding of values is handled, lot of the 
output is generated by ad-hoc code somewhere in post_callbacks. 
Unfortunately this is not easily fixable.





I have written a simple function that uses the parameter definitions to
convert most values automatically:

def converter(plugin, *args, **kwargs):
 response = plugin(*args, **kwargs)
 params = {p.name: p for p in plugin.obj.takes_params}
 if hasattr(plugin, 'output_params'):
 params.update({p.name: p for p in plugin.output_params()})
 results = response['result']
 if isinstance(results, dict):
 results = [results]
 for result in results:
 for key, value in result.iteritems():
 param = params.get(key)
 if param is None:
 continue
 if (value and not param.multivalue and
 isinstance(value, (list, tuple))):
 if len(value) > 1:
 raise ValueError(key, value)
 value = value[0]
 result[key] = param.convert(value)
 return response

It works like a charm for several plugins:


print(converter(api.Command.dnsconfig_show))

{u'result': {u'dn': u'cn=dns,dc=ipa,dc=example', u'idnsallowsyncptr':
False}, u'value': None, u'summary': None}


But it is failing for some plugins like user_find(). The plugin returns
u'memberof_group': (u'admins', u'trust admins'). However
global_output_params defines the value as an optional and single valued
string:

 Str('memberof_group?', label=_('Member of groups')).

I think the definition is wrong. memberof_group and some other fields
should be defined as optional and multivalued fields insteads. Even the
field's label uses a plural form.

What do you think?


Yes, the definition is wrong, but I don't think it's worth fixing, since 
you can't rely on a single-value param having a single value in the 
output for any other command and param anyway.


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] user-* commands performance issues

[Petr Vobornik]

On 03/17/2016 04:09 PM, Martin Basti wrote:

Hello all,

I would like to discuss the way how we should improve the speed of
user-find commands (and other commands too if possible):

0)
Do not do extra search for ipasshpubkey. This is clear, patch posted for
review.
https://fedorahosted.org/freeipa/ticket/3376

commands: user, stageuser, host, idview

1)
make --no-members option visible in CLI
https://fedorahosted.org/freeipa/ticket/4995


There was a discussion around devconf that --no-members should be a 
default behavior of xxx-find commands and I'm for it.


Reasoning: use case: 'find me all groups which satisfy this filter'. 
Showing members clutters the output(one group with >500 member makes it 
unusable) and makes things slow(both on server and CLI side).


For xxx-show commands it is a question where I don't have a strong opinion.



I don't think we should implement also --no-indirect-members, I think
that this kind of granularity is not needed.
If --no-members is used, then indirect members will be ignored too.


+1



commands: all which use members

2)
Limit the amount of searches for memberof[indirect] (group, netgroup,
role, hbacrule, sudorule) and search for each dn only once in find
commands.

We can have configurable option in default.conf (for example
memberof_search_limit=100 (0 unlimited)). Find commands will get members
only for specified amount and if this limit is exceeded a warning
message is shown.
I do not like this idea much, I think it should be all or nothing, I
prefer to not do this.

However I like the idea of temporary caching inside find commands, where
each memberof DN is resolved just once and results are cached in a map
and reused in current context of command. This should be improvement
mainly for indirect searches, but cache should be faster for direct
members than doing internal calls of framework objects. This part is
backward compatible, the first part is not.

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


What parts of the ticket can be solved with deref plugin? I guess we can 
get the CNs, but not what is a direct member. Maybe it should be 
discussed separately.




commands: user-find, stageuser-find, possibly all find commands

3)
Remove userPassword, krbPrincipalKey from search results
This change is not backward compatible, can we do this?

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

commands: user-find


I'm for it, would like to hear other opinions.

Note: it should be only in user-find commands. 'show' has to display it.



Martin^2


--
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 0442-0449] Pylint: sunday code cleanup

[Christian Heimes]

On 2016-03-21 10:29, Petr Spacek wrote:
> On 20.3.2016 21:56, Martin Basti wrote:
>> Patches attached.
> 
> I do not really like
> freeipa-mbasti-0442-pylint-remove-bare-except
> because it replaces most of
> 
> try: ... except:
> 
> with
> 
> try: ... except Exception:
> 
> 
> which AFAIK does not add any value. It would be better to replace Exception
> with more specific exception so the code raises an error instead of continuing
> when something really unexpected happens.

It adds some value. A bare except also excepts signals like
KeyboardInterrupt and SystemExit. except Exception doesn't block these
exceptions.

But yes, more specific exceptions are better.

Christian




signature.asc
Description: OpenPGP digital 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] [PATCHES 0442-0449] Pylint: sunday code cleanup

[Petr Spacek]

On 20.3.2016 21:56, Martin Basti wrote:
> Patches attached.

I do not really like
freeipa-mbasti-0442-pylint-remove-bare-except
because it replaces most of

try: ... except:

with

try: ... except Exception:


which AFAIK does not add any value. It would be better to replace Exception
with more specific exception so the code raises an error instead of continuing
when something really unexpected happens.


Other patches look sensible to me.

-- 
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] [DESIGN] Server Roles

[Martin Babinsky]

On 03/21/2016 09:02 AM, Martin Kosek wrote:

On 03/18/2016 03:43 PM, Martin Babinsky wrote:

On 03/18/2016 02:44 PM, Petr Vobornik wrote:

On 03/18/2016 10:59 AM, Martin Kosek wrote:

On 03/18/2016 10:47 AM, Martin Babinsky wrote:

On 03/18/2016 10:21 AM, Martin Kosek wrote:

On 03/17/2016 06:16 PM, Martin Babinsky wrote:

Hi list,

here is a link (http://www.freeipa.org/page/V4/Server_Roles) to WIP
design
document concerning the concept of Server Roles as a user-friendly
abstraction
of the services running on IPA masters.

The main aim of this feature is to provide a higher level interface
to query
and manipulate service-related information stored in dirsrv backend.

I have not touched the design much from the post-Devconf session,
mainly
because there are some points to clarify and agree upon.


Initial thoughts:

* Use Cases: these are rather vague points what you want to
implement. In Use
Case section, I would like to see what specific *user* use cases you
are
addressing, i.e. what user problems you are solving. Ideally in a
form of a
user story. Like here:

http://www.freeipa.org/page/V4/User_Life-Cycle_Management#Use_Cases
or here:
http://www.freeipa.org/page/V4/Authentication_Indicators#Use_Cases
or here:
http://www.freeipa.org/page/V4/External_trust_to_AD#Use_Cases


Ok I will thing of some clearer points.


I have the following points to discuss:

1.) the design assumes that there is a distinction between roles
such as DNS
server, CA, etc. and the more specific sub-roles such as DNSSec key
master, CRL
master, etc. Now in the hindsight I think this distinction is quite
artificial
and just clutters the interface unnecessarily. We might implement
this kind of
hierarchy in the code itself but that is something the user needs
not be
aware of.


Well, there are dependencies. A server cannot be a CRL master
without also
being a CA role. I assume same applies to DNSSEC master.

I think we need to think more about distinguishing what is role,
what is just
an attribute of a role, etc. AD for example distinguishes roles,
role service
and features:

https://technet.microsoft.com/en-us/library/cc754923.aspx


We will have to implement the role/subrole/unicorn hierarchy anyhow.
What I
would like to discuss is whether it is necessary to expose this
hierarchy to
the users. Consider a case when user wants to find which server is a
CA renewal
master:

ipa server-role-find "CA renewal master"

vs.

ipa server-role-find --subrole "Renewal master"

Behind the scenes, the code has to do the same thing (e.g. issue a
search using
(&(cn=CA)(ipaConfigString=enabledService)(ipaConfigString=caRenewalMaster))),

but the UX is a bit different.


Well, even the LDAP structure is different in this case. CA role is an
object
in cn=masters, caRenewalMaster is it's property. So they will likely be
different user objects too.

For your example, I can image a search like that:

$ ipa server-role-find "CA" --subrole "renewal-master"

(for the case when you have "DNS" role also with "renewal-master"
sub-role).

Martin



I don't have a strong option about this matter.

Number of roles will be limited. I don't see any point in developing
hierarchies in CLI/API/Web UI. Simply describing the roles and their
dependencies in server-role help should be enough.

Hierarchy and dependency should be checked internally.

Question is how it should behave in practice. There is no example in the
design page. Imagine these use cases:

$ server-role-find
"CA"
"CA renewal master"
"DNS server"
"DNSSec Key Master"
...

maybe is should print also description, but help might be enough.


$ server-role-find
===
Certificate Authority
Manages certificate requests and revocation...
(optionally list masters)
Enabled on: master1.ipa.test, replica3.ipa.test

===
DNS Server
manages forward and reverse name resolution
Enabled on: master1.ipa.test

===
CA renewal master
Manages automatic renewal of certificates nearing expiration
Enabled on: replica3.ipa.test
...


Even though I disliked having renewal master as separate role and rather as a
property of existing CA role, this looks reasonable.

What plans do you have around the data model, that currently "CA renewal
master" and "Certificate Authority" roles are implemented completely
differently in cn=masters? (I mean LDAP entry vs. LDAP attribute) It would be
transformed during upgrade?

IMHO e may not need to touch LDAP structure at all if we use subtree 
search of LDAP entries. Consider the following search for CA renewal 
master: http://fpaste.org/343249/85492491/


I plan to use a set (meta)classes which will dynamically provide 
attributes and values to match against for each role.


The obvious disadvantage here is the additional DN manipulation to get 
the master entry itself.


But maybe I am missing something.

Also see Jan's reply to OP, he proposes that these singular "(sub) 
roles" be transformed into virtual attributes coupled to each role:


ipa server-find --dnssec-key-master/--ca-renewal-master=Tru

Re: [Freeipa-devel] [PATCH 0143-0144] different errors/warnings for different LDAP limit type exceeded

[Petr Spacek]

On 18.3.2016 13:49, Rob Crittenden wrote:
> Martin Babinsky wrote:
>> These patches implement behavior agreed upon during discussion of
>> https://fedorahosted.org/freeipa/ticket/5677
>>
>> However I'm not sure if we want to push them into 4-3 branch (the ticket
>> is triaged into 4.3.2 milestone) since they modify the framework
>> behavior quite a bit.
>>
>> If there is no need to have it there (CC'ing Milan since he is the
>> reporter), I would retriage it into 4.4 milestone.
> 
> 
> + desc="while getting entries (search base: '{}',"
> + "filter: {})".format(base_dn, filter))
> 
> This is going to expose parts of the DIT in an error message to users. We have
> tried in the past to hide the implementation. I'd propose logging the error
> and making the exception less verbose.

IMHO it actually helps to print the DN. At very least the user can see if the
error is happening always with the same DN or if it keeps changing.

In other words, for user it is not that important to understand meaning of the
DN but it might be important to see if it is the same or not.

-- 
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] [DESIGN] Server Roles

[Jan Cholasta]

On 17.3.2016 18:16, Martin Babinsky wrote:

Hi list,

here is a link (http://www.freeipa.org/page/V4/Server_Roles) to WIP
design document concerning the concept of Server Roles as a
user-friendly abstraction of the services running on IPA masters.

The main aim of this feature is to provide a higher level interface to
query and manipulate service-related information stored in dirsrv backend.

I have not touched the design much from the post-Devconf session, mainly
because there are some points to clarify and agree upon.

I have the following points to discuss:

1.) the design assumes that there is a distinction between roles such as
DNS server, CA, etc. and the more specific sub-roles such as DNSSec key
master, CRL master, etc. Now in the hindsight I think this distinction
is quite artificial and just clutters the interface unnecessarily. We
might implement this kind of hierarchy in the code itself but that is
something the user needs not be aware of.


These shouldn't be (sub-)roles at all - they are inherently a 
one-to-many relationship between the logical services and servers, 
whereas roles are many-to-many relationship between the logical services 
and servers. I would rather see them exposed in the global service 
config, such as:


$ ipa dnsconfig-mod --sec-master=ipa12.example.com
  DNSSEC master: ipa12.example.com



2.) I guess the role names should be case insensitive so that users are
not hindered by trying to get the case right.


+1



3.) Do we need an internal API call which will add all services
belonging to a role to the corresponding master entry? (basically a
'server_add_role' type of command). Currently, each service instance
adds its own service entry during service installation so we probably do
not need to duplicate this functionality.


+1, we don't need more duplicate code.



That is all I can think of right now. I had many more questions popping
up during this night's bout of insomnia, but they got lost during the day.


How are we going to expose the different states of server roles? They 
can be:


a) available/unavailable (the package providing the role was/was not 
installed on the server)
b) configured/unconfigured (the installer for the role was/was not 
successfully run on the server, LDAP service entries exist)

c) enabled/disabled

My preference would be to make server-role commands work on top of 
available services, like this:


# ipa server-role-show $HOSTNAME DNS
ipa: ERROR: DNS: server role not found

# dnf install freeipa-server-dns
...

# ipa server-role-show $HOSTNAME DNS
  Name: DNS
  Configured: False
  Enabled: False

# ipa-dns-install
...

# ipa server-role-show $HOSTNAME DNS
  Name: DNS
  Configured: True
  Enabled: True



Do not be afraid to bring up other questions/remarks/comments. This is
my first design documents so I expect them to be plenty.


The CLI commands are a little bit self-inconsistent, see any other 
plugin for how the general layout of arguments should look like.


--
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] [TEST][Patch-0027] Fixed test failure during in-tree session, ticket N 5736

[Oleg Fayans]

Hi Martin,

On 03/16/2016 03:35 PM, Martin Basti wrote:
> 
> 
> On 16.03.2016 15:13, Martin Basti wrote:
>>
>>
>> On 16.03.2016 14:59, Oleg Fayans wrote:
>>> Hi Martin
>>>
>>> On 03/16/2016 02:39 PM, Martin Basti wrote:

 On 16.03.2016 10:59, Oleg Fayans wrote:
> With this patch applied integration tests pass and in-tree tests are
> gracefully skipped.
>
> @mkubik, It is not possible to put the decorator to util.py as per our
> discussion, because it uses tasks, so tasks must be imported. But
> tasks
> already import util, which leads to circular imports. So I've put
> it to
> tasks.py
>
>
>
 NACK

 1)
 Use right ticket in commit message (#5723)
>>> But (#5736) is exactly the issue that is being addressed. Probably note
>>> both tickets in the commit message?
>> But as I wrote in ticket #5736, this ticket should be closed, because
>> issue is caused by ticket which is not finished yet, so we should
>> continue just with original ticket.

Done

>>
>>>
 2)
 Link to ticket should be last in the commit message

Done


 3)
 dereplicafy

 3a)
 wrong doc string, it removes *only* replicas not clients
>>> No, in fact it removes both:
>>> uninstall_replica(args[0].master, host)
>>> uninstall_client(host)
>>>
>>> Both tasks have raiseonerr set to False, which means that even if
>>> replica was not installed but the client was - it will also be removed
>> I see just
>> for host in args[0].replicas
>>
>> I don't see any
>> for host in args[0].clients
>> there
>>
>> Also uninstall_client should not be there. ipa-server-install
>> --uninstall removes client too. The extra call of uninstall client is
>> IMO there just because an ancient bug that is already fixed.

That's done because some tests install client separately and then
deliberately install replica the wrong way to test that the installer
fails in a predicted way. That's why this separate uninstall_client
call. The doc string was corrected.


>>
>>>
 3b)
 can we rename it to something different? (replicas_cleanup,
 replicas_uninstall, replicas_teardown)
>>> replicas_cleanup, or even topo_cleanup sounds OK to me.

replicas_cleanup it is

>>>
 4)
 Please fix commit message
 - Wile trated correctly
 - followiong
 - rewrote -> rewrite
>>> Will do

Done

>>>
 5)
 decorator
 +def wrapped(*args):
 +func(*args)
 +for host in args[0].replicas:

 Shouldn't be there try-finally around func() call, or something?
>>> No, the wrapped function is a test_* method: if it fails we need to see
>>> the original failure
>> but if something raise an exception in func(), cleanup will not be
>> executed.
>>
>> You can do
>> In [4]: try:
>>...: raise ValueError('Hello')
>>...: finally:
>>...: try:
>>...: raise ValueError('Cleanup')
>>...: except Exception:
>>...: pass
>>...:
>> ---
>>
>> ValueErrorTraceback (most recent call
>> last)
>>  in ()
>>   1 try:
>> > 2 raise ValueError('Hello')
>>   3 finally:
>>   4 try:
>>   5 raise ValueError('Cleanup')
>>
>> ValueError: Hello
> On the other hand, I do not want cleanup with --pdb option, so maybe it
> should just fail
> 
>>
>>>
 Are you sure that there is no need to return result of func()?
>>> The same applies here: we never return results from test_* methods
>> ok
>>>
 *) Please create additional patch that will add licence there


>>> Will do :)
>>>
>>>
>>
> 

The license-related patch is attached too

-- 
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
From 02ffe2514252e51744a6691bfb6692cadcc69a8d Mon Sep 17 00:00:00 2001
From: Oleg Fayans 
Date: Mon, 21 Mar 2016 08:46:11 +0100
Subject: [PATCH] rewrite a misprocessed teardown_method method as a custom
 decorator

teardown_method is a standard pytest method used to put any code to be executed
after each test method is executed. While treated correctly by our integration
tests, this method is misinterpreted by in-tree tests in the following way:
in-tree tests try to execute it even if all the test methods are skipped due to
test resources being not configured. This causes the tests, that otherwise would
have been skipped, to fail

https://fedorahosted.org/freeipa/ticket/5723
---
 ipatests/test_integration/tasks.py | 22 ++
 .../test_integration/test_replica_promotion.py | 17 -
 2 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/ipatests/test_integration/tasks.py b/ipatests/test_integration/tasks.py
index c2129708b5e75d9b492fb8b33784478b50122115..2aea4f14c01538bf77dbe45a947c5d415c9ea744 100644
--- a/ipatests/test_integration/tasks.py
+++ b/ipatests/test_integration/tasks.py
@@ -1153,3 +1153,25 @@ def uninstall_replica(master, repli

Re: [Freeipa-devel] [DESIGN] Server Roles

[Martin Kosek]

On 03/18/2016 03:43 PM, Martin Babinsky wrote:
> On 03/18/2016 02:44 PM, Petr Vobornik wrote:
>> On 03/18/2016 10:59 AM, Martin Kosek wrote:
>>> On 03/18/2016 10:47 AM, Martin Babinsky wrote:
 On 03/18/2016 10:21 AM, Martin Kosek wrote:
> On 03/17/2016 06:16 PM, Martin Babinsky wrote:
>> Hi list,
>>
>> here is a link (http://www.freeipa.org/page/V4/Server_Roles) to WIP
>> design
>> document concerning the concept of Server Roles as a user-friendly
>> abstraction
>> of the services running on IPA masters.
>>
>> The main aim of this feature is to provide a higher level interface
>> to query
>> and manipulate service-related information stored in dirsrv backend.
>>
>> I have not touched the design much from the post-Devconf session,
>> mainly
>> because there are some points to clarify and agree upon.
>
> Initial thoughts:
>
> * Use Cases: these are rather vague points what you want to
> implement. In Use
> Case section, I would like to see what specific *user* use cases you
> are
> addressing, i.e. what user problems you are solving. Ideally in a
> form of a
> user story. Like here:
>
> http://www.freeipa.org/page/V4/User_Life-Cycle_Management#Use_Cases
> or here:
> http://www.freeipa.org/page/V4/Authentication_Indicators#Use_Cases
> or here:
> http://www.freeipa.org/page/V4/External_trust_to_AD#Use_Cases
>
 Ok I will thing of some clearer points.

>> I have the following points to discuss:
>>
>> 1.) the design assumes that there is a distinction between roles
>> such as DNS
>> server, CA, etc. and the more specific sub-roles such as DNSSec key
>> master, CRL
>> master, etc. Now in the hindsight I think this distinction is quite
>> artificial
>> and just clutters the interface unnecessarily. We might implement
>> this kind of
>> hierarchy in the code itself but that is something the user needs
>> not be
>> aware of.
>
> Well, there are dependencies. A server cannot be a CRL master
> without also
> being a CA role. I assume same applies to DNSSEC master.
>
> I think we need to think more about distinguishing what is role,
> what is just
> an attribute of a role, etc. AD for example distinguishes roles,
> role service
> and features:
>
> https://technet.microsoft.com/en-us/library/cc754923.aspx
>
 We will have to implement the role/subrole/unicorn hierarchy anyhow.
 What I
 would like to discuss is whether it is necessary to expose this
 hierarchy to
 the users. Consider a case when user wants to find which server is a
 CA renewal
 master:

 ipa server-role-find "CA renewal master"

 vs.

 ipa server-role-find --subrole "Renewal master"

 Behind the scenes, the code has to do the same thing (e.g. issue a
 search using
 (&(cn=CA)(ipaConfigString=enabledService)(ipaConfigString=caRenewalMaster))),

 but the UX is a bit different.
>>>
>>> Well, even the LDAP structure is different in this case. CA role is an
>>> object
>>> in cn=masters, caRenewalMaster is it's property. So they will likely be
>>> different user objects too.
>>>
>>> For your example, I can image a search like that:
>>>
>>> $ ipa server-role-find "CA" --subrole "renewal-master"
>>>
>>> (for the case when you have "DNS" role also with "renewal-master"
>>> sub-role).
>>>
>>> Martin
>>>
>>
>> I don't have a strong option about this matter.
>>
>> Number of roles will be limited. I don't see any point in developing
>> hierarchies in CLI/API/Web UI. Simply describing the roles and their
>> dependencies in server-role help should be enough.
>>
>> Hierarchy and dependency should be checked internally.
>>
>> Question is how it should behave in practice. There is no example in the
>> design page. Imagine these use cases:
>>
>> $ server-role-find
>> "CA"
>> "CA renewal master"
>> "DNS server"
>> "DNSSec Key Master"
>> ...
>>
>> maybe is should print also description, but help might be enough.
>>
> $ server-role-find
> ===
> Certificate Authority
> Manages certificate requests and revocation...
> (optionally list masters)
> Enabled on: master1.ipa.test, replica3.ipa.test
> 
> ===
> DNS Server
> manages forward and reverse name resolution
> Enabled on: master1.ipa.test
> 
> ===
> CA renewal master
> Manages automatic renewal of certificates nearing expiration
> Enabled on: replica3.ipa.test
> ...

Even though I disliked having renewal master as separate role and rather as a
property of existing CA role, this looks reasonable.

What plans do you have around the data model, that currently "CA renewal
master" and "Certificate Authority" roles are implemented completely
differently in cn=masters? (I mean LDAP entry vs. LDAP attribute) It would be
transformed during upgrade?

Also, how do you plan hiding the "services" we were not interested in se

Re: [Freeipa-devel] [DESIGN] Server Roles

[Martin Kosek]

On 03/18/2016 03:58 PM, Simo Sorce wrote:
> On Fri, 2016-03-18 at 15:28 +0100, Petr Vobornik wrote:
>> On 03/18/2016 02:59 PM, Simo Sorce wrote:
...
>> Use cases I see:
>> 1. Administrator wants to know which servers are configured with 
>> CA|KRA|DNS.
>> 2. Administrator wants to know which server is CRL master.
>> 3. We want this info to be able to display it in topology graph (but 
>> this is for 4.5).
> 
> Ack, ack and ack.

+1. *This* is what I was looking for in the Design page Use Case section, that
I mentioned in my first reply. The rest of the design page should be written
with these use cases in mind.

>> Should there be an NTP server role?
> 
> Probably.
> 

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