[Freeipa-devel] announcing ipa-docker-test-runner

[Martin Babinsky]

Hi fellow FreeIPA developers,

Did you ever wanted to have a means to build FreeIPA rpms locally 
without pulling in all the build requires or firing up VMs just to do 
such a simple task?


Did you ever wish to fire up a quick script which will build RPMs and 
install FreeIPA server for you to play and test out some new feature you 
are working on?


Did you ever wish to run all out-of-tree tests from your local git repo 
with a single command?


I have put together a dumb frontend automating building FreeIPA and 
automating tests in a Docker container


The git repo is here along with a README explaining the usage:

https://github.com/martbab/ipa-docker-test-runner

The tool is far from perfect but it should be usable for the most common 
use cases. If you have any issue with it or would like to propose an 
enhancement, please open an issue or PR.


--
Martin^3 Babinsky

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

[Freeipa-devel] [freeipa PR#134][comment] DNS URI support

[mbasti-rh]

  URL: https://github.com/freeipa/freeipa/pull/134
Title: #134: DNS URI support

mbasti-rh commented:
"""
I'm so sorry, I didn't noticed earlier, but you forgot to bump API in VERSION

Otherwise LGTM and works for me
"""

See the full comment at 
https://github.com/freeipa/freeipa/pull/134#issuecomment-252273635
-- 
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] [freeipa PR#142][comment] CheckedIPAddress: Implement __(g|s)etstate__ and to ensure proper (un)pickling

[mbasti-rh]

  URL: https://github.com/freeipa/freeipa/pull/142
Title: #142: CheckedIPAddress: Implement __(g|s)etstate__ and to ensure proper 
(un)pickling

mbasti-rh commented:
"""
Works for me, I'm not pushing this yet if somebody wants to double check pickle 
implementation
"""

See the full comment at 
https://github.com/freeipa/freeipa/pull/142#issuecomment-252270287
-- 
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] Build system refactoring - design document

[Timo Aaltonen]

On 07.10.2016 12:56, Petr Spacek wrote:
> Dear FreeIPA developers and packagers,
> 
> you can find first version of the Build system refactoring design document on:
> http://www.freeipa.org/page/V4/Build_system_refactoring
> 
> If you do not care about implementation details, please be so kind and quickly
> scan through chapter
> http://www.freeipa.org/page/V4/Build_system_refactoring#Feature_Management
> 
> I'm not an FreeIPA packager so I might miss some important thing which needs
> to be configurable.
> 
> 
> Also, I would appreciate ideas how to handle build versioning:
> http://www.freeipa.org/page/V4/Build_system_refactoring#Versioning
> 
> My main questions are:
> * What is triggering IPA upgrade?
> * Would it be sufficient to bump release in RPM? (I mean - theoretically.
> Could the code be modified to detect this?)
> 
> Here I'm trying to avoid unnecessary rebuilds caused by changes to
> IPA_VENDOR_VERSION during each build.
> 
> 
> Timo, what can I do to help you with packaging for Ubuntu/Debian?

If you mean build system -wise, there isn't anything that I need, at
least if you migrate to autotools which sounds great.

This is the debian/rules of the current package, so if you'll have a
proper 'make clean' (as suggested already) and a one-pass build then
that's pretty much what I'd "need".

https://anonscm.debian.org/cgit/pkg-freeipa/freeipa.git/tree/debian/rules


-- 
t

-- 
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] Build system refactoring - design document

[Petr Spacek]

On 7.10.2016 12:59, Martin Basti wrote:
> 
> 
> On 07.10.2016 11:56, Petr Spacek wrote:
>> Dear FreeIPA developers and packagers,
>>
>> you can find first version of the Build system refactoring design document 
>> on:
>> http://www.freeipa.org/page/V4/Build_system_refactoring
>>
>> If you do not care about implementation details, please be so kind and 
>> quickly
>> scan through chapter
>> http://www.freeipa.org/page/V4/Build_system_refactoring#Feature_Management
>>
>> I'm not an FreeIPA packager so I might miss some important thing which needs
>> to be configurable.
>>
>>
>> Also, I would appreciate ideas how to handle build versioning:
>> http://www.freeipa.org/page/V4/Build_system_refactoring#Versioning
>>
>> My main questions are:
>> * What is triggering IPA upgrade?
>> * Would it be sufficient to bump release in RPM? (I mean - theoretically.
>> Could the code be modified to detect this?)
>>
>> Here I'm trying to avoid unnecessary rebuilds caused by changes to
>> IPA_VENDOR_VERSION during each build.
>>
>>
>> Timo, what can I do to help you with packaging for Ubuntu/Debian?
>>
>> Dream big, even wild ideas are welcome!
>>
> 
> Thank you for nice design,
> 
> 1)
> I'd like to have make clean as well (we have it there now, but I don't think
> that it works well)

I've added clean to the list. In general, we should get all targets listed in
https://www.gnu.org/software/automake/manual/html_node/Standard-Targets.html
"for free" (if we do it right).


> 2)
> Pylint:
> 
> currently we have (almost) python2/3 compatible code so what is the reason to
> have python2 and python3 separated checks? Pylint is doing that at once

I'm fine with just one pylint. Design updated:
http://www.freeipa.org/page/V4/Build_system_refactoring#Configuration

Thank you for clarification!

-- 
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] Build system refactoring - design document

[Martin Basti]

On 07.10.2016 11:56, Petr Spacek wrote:

Dear FreeIPA developers and packagers,

you can find first version of the Build system refactoring design document on:
http://www.freeipa.org/page/V4/Build_system_refactoring

If you do not care about implementation details, please be so kind and quickly
scan through chapter
http://www.freeipa.org/page/V4/Build_system_refactoring#Feature_Management

I'm not an FreeIPA packager so I might miss some important thing which needs
to be configurable.


Also, I would appreciate ideas how to handle build versioning:
http://www.freeipa.org/page/V4/Build_system_refactoring#Versioning

My main questions are:
* What is triggering IPA upgrade?
* Would it be sufficient to bump release in RPM? (I mean - theoretically.
Could the code be modified to detect this?)

Here I'm trying to avoid unnecessary rebuilds caused by changes to
IPA_VENDOR_VERSION during each build.


Timo, what can I do to help you with packaging for Ubuntu/Debian?

Dream big, even wild ideas are welcome!



Thank you for nice design,

1)
I'd like to have make clean as well (we have it there now, but I don't 
think that it works well)


2)
Pylint:

currently we have (almost) python2/3 compatible code so what is the 
reason to have python2 and python3 separated checks? Pylint is doing 
that at once


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

Re: [Freeipa-devel] [PATCH 0058] Make get_entries not ignore its size_limit argument

[Standa Laznicka]

On 10/07/2016 08:31 AM, Jan Cholasta wrote:

On 17.8.2016 13:47, Stanislav Laznicka wrote:

On 08/11/2016 02:59 PM, Stanislav Laznicka wrote:

On 08/11/2016 07:49 AM, Jan Cholasta wrote:

On 2.8.2016 13:47, Stanislav Laznicka wrote:

On 07/19/2016 09:20 AM, Jan Cholasta wrote:

Hi,

On 14.7.2016 14:36, Stanislav Laznicka wrote:

Hello,

This patch fixes https://fedorahosted.org/freeipa/ticket/5640.

With not so much experience with the framework, it raises question
in my
head whether ipaldap.get_entries is used properly throughout the
system
- does it always assume that it gets ALL the requested entries or
just a
few of those as configured by the 'ipaSearchRecordsLimit'
attribute of
ipaConfig.etc which it actually gets?


That depends. If you call get_entries() on the ldap2 plugin 
(which is

usually the case in the framework), then ipaSearchRecordsLimit is
used. If you call it on some arbitrary LDAPClient instance, the
hardcoded default (= unlimited) is used.



One spot that I know the get_entries method was definitely not used
properly before this patch is in the
baseldap.LDAPObject.get_memberindirect() method:

 692 result = self.backend.get_entries(
 693 self.api.env.basedn,
 694 filter=filter,
 695 attrs_list=['member'],
 696 size_limit=-1, # paged search will get
everything
anyway
 697 paged_search=True)

which to me seems kind of important if the environment size_limit
is not
set properly :) The patch does not fix the non-propagation of the
paged_search, though.


Why do you think size_limit is not used properly here?

AFAIU it is desired that the search is unlimited. However, due to the
fact that neither size_limit nor paged_search are passed from
ldap2.get_entries() to ldap2.find_entries() (methods inherited from
LDAPClient), only the number of records specified by
ipaSearchRecordsLimit is returned. That could eventually cause 
problems

should ipaSearchRecordsLimit be set to a low value as in the ticket.


I see. This is *not* intentional, the **kwargs of get_entries()
should be passed to find_entries(). This definitely needs to be fixed.



Anyway, this ticket is not really easily fixable without more 
profound

changes. Often, multiple LDAP searches are done during command
execution. What do you do with the size limit then? Do you pass the
same size limit to all the searches? Do you subtract the result size
from the size limit after each search? Do you do something else with
it? ... The answer is that it depends on the purpose of each
individual LDAP search (like in get_memberindirect() above, we 
have to

do unlimited search, otherwise the resulting entry would be
incomplete), and fixing this accross the whole framework is a
non-trivial task.


I do realize that the proposed fix for the permission plugin is not
perfect, it would probably be better to subtract the number of
currently
loaded records from the sizelimit, although in the end the number of
returned values will not be higher than the given size_limit. 
However,

it seems reasonable that if get_entries is passed a size limit, it
should apply it over current ipaSearchRecordsLimit rather than 
ignoring
it. Then, any use of get_entries could be fixed accordingly if 
someone

sees fit.



Right. Anyway, this is a different issue than above, so please put
this into a separate commit.


Please see the attached patches, then.


Self-NACK, with Honza's help I found there was a mistake in the code. I
also found an off-by-one bug which I hope I could stick to the other two
patches (attaching only the modified and new patches).


Works for me, but this bit in patch 0064 looks suspicious to me:

+if max_entries > 0 and len(entries) == 
max_entries:


Shouldn't it rather be:

+if max_entries > 0 and len(entries) >= 
max_entries:


?

The length of entries list should not exceed max_entries if size_limit 
is properly implemented. Therefore the list you get from execute() 
should not have more then max_entries entries. You shouldn't also be 
able to append a legacy entry to entries list if this check is the first 
thing you do.


That being said, >= could be used but then some popping of entries from 
entries list would be necessary. But it's perhaps safer to do, although 
if there's a bug, it won't be that obvious :)


--
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] Build system refactoring - design document

[Petr Spacek]

On 7.10.2016 11:56, Petr Spacek wrote:
> Dear FreeIPA developers and packagers,
> 
> you can find first version of the Build system refactoring design document on:
> http://www.freeipa.org/page/V4/Build_system_refactoring
> 
> If you do not care about implementation details, please be so kind and quickly
> scan through chapter
> http://www.freeipa.org/page/V4/Build_system_refactoring#Feature_Management
> 
> I'm not an FreeIPA packager so I might miss some important thing which needs
> to be configurable.
> 
> 
> Also, I would appreciate ideas how to handle build versioning:
> http://www.freeipa.org/page/V4/Build_system_refactoring#Versioning
> 
> My main questions are:
> * What is triggering IPA upgrade?
> * Would it be sufficient to bump release in RPM? (I mean - theoretically.
> Could the code be modified to detect this?)
> 
> Here I'm trying to avoid unnecessary rebuilds caused by changes to
> IPA_VENDOR_VERSION during each build.
> 
> 
> Timo, what can I do to help you with packaging for Ubuntu/Debian?
> 
> Dream big, even wild ideas are welcome!

Also, if you are packager or tester, please describe your
requirements/use-cases/user stories/whatever so I can design the system to
suit your needs.

I've tried to capture developer's needs to
http://www.freeipa.org/page/V4/Build_system_refactoring#User_stories

Please let me know if you are okay with that or if it needs corrections.

Thank you for your time!

-- 
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] Build system refactoring - design document

[Petr Spacek]

Dear FreeIPA developers and packagers,

you can find first version of the Build system refactoring design document on:
http://www.freeipa.org/page/V4/Build_system_refactoring

If you do not care about implementation details, please be so kind and quickly
scan through chapter
http://www.freeipa.org/page/V4/Build_system_refactoring#Feature_Management

I'm not an FreeIPA packager so I might miss some important thing which needs
to be configurable.


Also, I would appreciate ideas how to handle build versioning:
http://www.freeipa.org/page/V4/Build_system_refactoring#Versioning

My main questions are:
* What is triggering IPA upgrade?
* Would it be sufficient to bump release in RPM? (I mean - theoretically.
Could the code be modified to detect this?)

Here I'm trying to avoid unnecessary rebuilds caused by changes to
IPA_VENDOR_VERSION during each build.


Timo, what can I do to help you with packaging for Ubuntu/Debian?

Dream big, even wild ideas are welcome!

-- 
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] [Test][Patch-0047] Added a test for Ticket N 5964

[Oleg Fayans]

Hi Ludwig,

Thanks for the clarification! But then why does CSRUV allows to be 
deleted on a working replica? Shouldn't we keep this behavior somehow 
consistent?


On 10/07/2016 09:29 AM, Ludwig Krispenz wrote:


On 09/13/2016 10:10 AM, Oleg Fayans wrote:

Hi Ludwig,

The ipa-replica-manage clean-ruv sometimes does not quite work.

For example: I have a master and 2 replicas. Initial output of
'ipa-replica-manage list-ruv' looks like this:


Replica Update Vectors:
f24replica2.pesen.net:389: 7
f24master.pesen.net:389: 4
f24replica1.pesen.net:389: 3
Certificate Server Replica Update Vectors:
f24master.pesen.net:389: 6
f24replica1.pesen.net:389: 5
f24replica2.pesen.net:389: 8


When I do 'ipa-replica-manage clean-ruv 5' and then list-ruv, it shows
the expected result:

Replica Update Vectors:
f24replica2.pesen.net:389: 7
f24master.pesen.net:389: 4
f24replica1.pesen.net:389: 3
Certificate Server Replica Update Vectors:
f24master.pesen.net:389: 6
f24replica2.pesen.net:389: 8

But when I then do 'ipa-replica-manage clean-ruv 3', the command
executes successfully, but list-ruv still shows 5 RUVs instead of four.

After all nodes are restarted still 5 RUV's are displaayed, but if I
clean the RUV N 3 manually again, it works and leaves (expected) 4 RUVs.

Do you have an idea, what it might be and how to debug this?

did you remove the replica before cleaning the ruv, you cannot just run
cleanruv for an active replica, it always will come back.




On 08/05/2016 06:36 PM, Martin Basti wrote:



On 03.08.2016 14:45, Oleg Fayans wrote:

Hi Martin,

Thanks for the review! Both patches were updated.

On 07/28/2016 04:11 PM, Martin Basti wrote:



On 08.07.2016 15:41, Oleg Fayans wrote:

Hi Martin,

Thanks for the review!

On 07/08/2016 02:18 PM, Martin Basti wrote:



On 27.06.2016 13:53, Oleg Fayans wrote:

Hi guys,

Is there a chance the patches NN 0047.1 and 0048.1 get reviewed
before
4.4 release? They cover a good part of the Managed Topology 4.4
feature.

On 06/17/2016 11:18 AM, Oleg Fayans wrote:

One more test was added to the patch-0048

On 06/17/2016 09:43 AM, Oleg Fayans wrote:

Fixed a bug in the previous patch, automated 2 more testcases
from
http://www.freeipa.org/page/V4/Manage_replication_topology_4_4/Test_Plan





On 06/16/2016 04:46 PM, Oleg Fayans wrote:










IIUC, this will turn off the machine completely, how is cleanup done
then.  AFAIK our tests cannot turn on machine again and run
cleanup, so
you will not be able to run more tests on the same topology without
manual cleanup and manual start.

+replica = self.replicas[0]
+replica.run_command(['poweroff'])

IMO would be better to just call 'ipactl stop' instead of 'poweroff'


Agreed! Fixed.



Martin^2






*Automated ipa-replica-manage del tests*

1)
+replica.run_command(['ipactl', 'stop'])
+time.sleep(3)

Why do you need sleep here?


Removed, it was left from the old "poweroff" approach




2)
+ruvid_re = re.compile(".*%s:389: (\d+).*" % replica.hostname)
+replica_ruvs = ruvid_re.findall(result.stdout_text)
+master.run_command(['ipa-replica-manage', 'clean-ruv', 'f',
+'-p', master.config.dirman_password,
+replica_ruvs[0]])

Because you are using re.findall(), without any match you will receive
IndexError here replica_ruvs[0]. IMO it deserves assert before


Implemented the assert which checks that the output contains enough
replica RUVs



3)
assert(replica.hostname in result1.stdout_text)

I think that this is error prone. What if there is just error
'could not
connect to replica ', or something similar.
instead of
listing/cleaning/whatever operation was executed. I think that it
should
be more specific regexp than just finding a replica name substring
(Yes
In IPA we dont always print error so stderr)

I'm not sure, but probably there might be cases when non critical
error
happen and exist status is still 0


Agree. Implemented a regex-based search



4)

+replica.run_command(['poweroff'])
+time.sleep(3)

There should not be poweroff, probably sleep could be removed too.


Gone




  *   Automated clean-ruv subcommand test*

1) PEP8, 2 new lines expected
./ipatests/test_integration/test_topology.py:163:1: E302 expected 2
blank lines, found 0
./ipatests/test_integration/test_topology.py:182:80: E501 line too
long
(85 > 79 characters)


Fixed




2)
I dont like doing assert just with count of occurences of substring in
STDOUT, would be possible to improve this somehow?


Maybe, but frankly, I don't see how. In this case we are making sure
that both simple and CA-specific RUVs of a replica are displayed. The
format of the output is strict:
Replica Update Vectors:
replica1_hostname:389: RUV_id
replica2_hostname:389: RUV_id
Certificate Server Replica Update Vectors:
replica1_hostname:389: RUV_id
replica2_hostname:389: RUV_id
If we do not see 2 occurrences of the 

[Freeipa-devel] [freeipa PR#133][synchronized] Tests: print what was expected from exceptions and callables in xmlrpc_tests

[pspacek]

   URL: https://github.com/freeipa/freeipa/pull/133
Author: pspacek
 Title: #133: Tests: print what was expected from exceptions and callables in 
xmlrpc_tests
Action: synchronized

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/133/head:pr133
git checkout pr133
From 5fff40983557934fa69fb699a5b9212849515cca Mon Sep 17 00:00:00 2001
From: Petr Spacek 
Date: Fri, 7 Oct 2016 09:34:54 +0200
Subject: [PATCH] Tests: print what was expected from callables in xmlrpc_tests

---
 ipatests/test_xmlrpc/xmlrpc_test.py | 36 ++--
 1 file changed, 22 insertions(+), 14 deletions(-)

diff --git a/ipatests/test_xmlrpc/xmlrpc_test.py b/ipatests/test_xmlrpc/xmlrpc_test.py
index 78d9638..0ce1245 100644
--- a/ipatests/test_xmlrpc/xmlrpc_test.py
+++ b/ipatests/test_xmlrpc/xmlrpc_test.py
@@ -23,6 +23,7 @@
 from __future__ import print_function
 
 import datetime
+import inspect
 
 import nose
 import contextlib
@@ -237,7 +238,8 @@ def failsafe_del(cls, obj, pk):
 UNEXPECTED = """Expected %r to raise %s, but caught different.
   args = %r
   options = %r
-  %s: %s"""
+  expected = %s: %s
+  got = %s: %s"""
 
 
 KWARGS = """Command %r raised %s with wrong kwargs.
@@ -329,19 +331,20 @@ def check(self, nice, desc, command, expected, extra_check=None):
 
 def check_exception(self, nice, cmd, args, options, expected):
 klass = expected.__class__
-name = klass.__name__
+expected_name = klass.__name__
 try:
 output = api.Command[cmd](*args, **options)
 except Exception as e:
-exception = e
+got = e
 else:
 raise AssertionError(
-EXPECTED % (cmd, name, args, options, output)
+EXPECTED % (cmd, expected_name, args, options, output)
 )
-if not isinstance(exception, klass):
+if not isinstance(got, klass):
 raise AssertionError(
-UNEXPECTED % (cmd, name, args, options,
-  exception.__class__.__name__, exception)
+UNEXPECTED % (cmd, expected_name, args, options,
+  expected_name, expected,
+  got.__class__.__name__, got)
 )
 # FIXME: the XML-RPC transport doesn't allow us to return structured
 # information through the exception, so we can't test the kw on the
@@ -349,21 +352,26 @@ def check_exception(self, nice, cmd, args, options, expected):
 # transport, the exception is a free-form data structure (dict).
 # For now just compare the strings
 # pylint: disable=no-member
-assert_deepequal(expected.strerror, exception.strerror)
+assert_deepequal(expected.strerror, got.strerror)
 # pylint: enable=no-member
 
 def check_callable(self, nice, cmd, args, options, expected):
-name = expected.__class__.__name__
+expected_name = expected.__class__.__name__
+try:
+expected_text = inspect.getsource(expected).strip()
+except TypeError:
+expected_text = str(expected)
 output = dict()
-exception = None
+got = None
 try:
 output = api.Command[cmd](*args, **options)
 except Exception as e:
-exception = e
-if not expected(exception, output):
+got = e
+if not expected(got, output):
 raise AssertionError(
-UNEXPECTED % (cmd, name, args, options,
-  type(exception).__name__, exception)
+UNEXPECTED % (cmd, expected_name, args, options,
+  expected_name, expected_text,
+  got.__class__.__name__, got)
 )
 
 def check_output(self, nice, cmd, args, options, expected, extra_check):
-- 
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-0047] Added a test for Ticket N 5964

[Ludwig Krispenz]

On 09/13/2016 10:10 AM, Oleg Fayans wrote:

Hi Ludwig,

The ipa-replica-manage clean-ruv sometimes does not quite work.

For example: I have a master and 2 replicas. Initial output of 
'ipa-replica-manage list-ruv' looks like this:



Replica Update Vectors:
f24replica2.pesen.net:389: 7
f24master.pesen.net:389: 4
f24replica1.pesen.net:389: 3
Certificate Server Replica Update Vectors:
f24master.pesen.net:389: 6
f24replica1.pesen.net:389: 5
f24replica2.pesen.net:389: 8


When I do 'ipa-replica-manage clean-ruv 5' and then list-ruv, it shows 
the expected result:


Replica Update Vectors:
f24replica2.pesen.net:389: 7
f24master.pesen.net:389: 4
f24replica1.pesen.net:389: 3
Certificate Server Replica Update Vectors:
f24master.pesen.net:389: 6
f24replica2.pesen.net:389: 8

But when I then do 'ipa-replica-manage clean-ruv 3', the command 
executes successfully, but list-ruv still shows 5 RUVs instead of four.


After all nodes are restarted still 5 RUV's are displaayed, but if I 
clean the RUV N 3 manually again, it works and leaves (expected) 4 RUVs.


Do you have an idea, what it might be and how to debug this?
did you remove the replica before cleaning the ruv, you cannot just run 
cleanruv for an active replica, it always will come back.





On 08/05/2016 06:36 PM, Martin Basti wrote:



On 03.08.2016 14:45, Oleg Fayans wrote:

Hi Martin,

Thanks for the review! Both patches were updated.

On 07/28/2016 04:11 PM, Martin Basti wrote:



On 08.07.2016 15:41, Oleg Fayans wrote:

Hi Martin,

Thanks for the review!

On 07/08/2016 02:18 PM, Martin Basti wrote:



On 27.06.2016 13:53, Oleg Fayans wrote:

Hi guys,

Is there a chance the patches NN 0047.1 and 0048.1 get reviewed
before
4.4 release? They cover a good part of the Managed Topology 4.4
feature.

On 06/17/2016 11:18 AM, Oleg Fayans wrote:

One more test was added to the patch-0048

On 06/17/2016 09:43 AM, Oleg Fayans wrote:
Fixed a bug in the previous patch, automated 2 more testcases 
from
http://www.freeipa.org/page/V4/Manage_replication_topology_4_4/Test_Plan 






On 06/16/2016 04:46 PM, Oleg Fayans wrote:










IIUC, this will turn off the machine completely, how is cleanup done
then.  AFAIK our tests cannot turn on machine again and run
cleanup, so
you will not be able to run more tests on the same topology without
manual cleanup and manual start.

+replica = self.replicas[0]
+replica.run_command(['poweroff'])

IMO would be better to just call 'ipactl stop' instead of 'poweroff'


Agreed! Fixed.



Martin^2






*Automated ipa-replica-manage del tests*

1)
+replica.run_command(['ipactl', 'stop'])
+time.sleep(3)

Why do you need sleep here?


Removed, it was left from the old "poweroff" approach




2)
+ruvid_re = re.compile(".*%s:389: (\d+).*" % replica.hostname)
+replica_ruvs = ruvid_re.findall(result.stdout_text)
+master.run_command(['ipa-replica-manage', 'clean-ruv', 'f',
+'-p', master.config.dirman_password,
+replica_ruvs[0]])

Because you are using re.findall(), without any match you will receive
IndexError here replica_ruvs[0]. IMO it deserves assert before


Implemented the assert which checks that the output contains enough
replica RUVs



3)
assert(replica.hostname in result1.stdout_text)

I think that this is error prone. What if there is just error 
'could not
connect to replica ', or something similar. 
instead of
listing/cleaning/whatever operation was executed. I think that it 
should
be more specific regexp than just finding a replica name substring  
(Yes

In IPA we dont always print error so stderr)

I'm not sure, but probably there might be cases when non critical 
error

happen and exist status is still 0


Agree. Implemented a regex-based search



4)

+replica.run_command(['poweroff'])
+time.sleep(3)

There should not be poweroff, probably sleep could be removed too.


Gone




  *   Automated clean-ruv subcommand test*

1) PEP8, 2 new lines expected
./ipatests/test_integration/test_topology.py:163:1: E302 expected 2
blank lines, found 0
./ipatests/test_integration/test_topology.py:182:80: E501 line too 
long

(85 > 79 characters)


Fixed




2)
I dont like doing assert just with count of occurences of substring in
STDOUT, would be possible to improve this somehow?


Maybe, but frankly, I don't see how. In this case we are making sure
that both simple and CA-specific RUVs of a replica are displayed. The
format of the output is strict:
Replica Update Vectors:
replica1_hostname:389: RUV_id
replica2_hostname:389: RUV_id
Certificate Server Replica Update Vectors:
replica1_hostname:389: RUV_id
replica2_hostname:389: RUV_id
If we do not see 2 occurrences of the replica hostname than definitely
something went wrong



3)
I'm not sure if clean-ruv is instant operations or there is some magic
happening in background (we have abort-clean-ruv). Maybe some 

Re: [Freeipa-devel] [Test][Patch-0047] Added a test for Ticket N 5964

[Oleg Fayans]

Ping for review

On 10/05/2016 12:02 PM, Oleg Fayans wrote:

Hi Ludwig,

Could you please take a look at it when you have time?

On 09/13/2016 10:10 AM, Oleg Fayans wrote:

Hi Ludwig,

The ipa-replica-manage clean-ruv sometimes does not quite work.

For example: I have a master and 2 replicas. Initial output of
'ipa-replica-manage list-ruv' looks like this:


Replica Update Vectors:
f24replica2.pesen.net:389: 7
f24master.pesen.net:389: 4
f24replica1.pesen.net:389: 3
Certificate Server Replica Update Vectors:
f24master.pesen.net:389: 6
f24replica1.pesen.net:389: 5
f24replica2.pesen.net:389: 8


When I do 'ipa-replica-manage clean-ruv 5' and then list-ruv, it shows
the expected result:

Replica Update Vectors:
f24replica2.pesen.net:389: 7
f24master.pesen.net:389: 4
f24replica1.pesen.net:389: 3
Certificate Server Replica Update Vectors:
f24master.pesen.net:389: 6
f24replica2.pesen.net:389: 8

But when I then do 'ipa-replica-manage clean-ruv 3', the command
executes successfully, but list-ruv still shows 5 RUVs instead of four.

After all nodes are restarted still 5 RUV's are displaayed, but if I
clean the RUV N 3 manually again, it works and leaves (expected) 4 RUVs.

Do you have an idea, what it might be and how to debug this?


On 08/05/2016 06:36 PM, Martin Basti wrote:



On 03.08.2016 14:45, Oleg Fayans wrote:

Hi Martin,

Thanks for the review! Both patches were updated.

On 07/28/2016 04:11 PM, Martin Basti wrote:



On 08.07.2016 15:41, Oleg Fayans wrote:

Hi Martin,

Thanks for the review!

On 07/08/2016 02:18 PM, Martin Basti wrote:



On 27.06.2016 13:53, Oleg Fayans wrote:

Hi guys,

Is there a chance the patches NN 0047.1 and 0048.1 get reviewed
before
4.4 release? They cover a good part of the Managed Topology 4.4
feature.

On 06/17/2016 11:18 AM, Oleg Fayans wrote:

One more test was added to the patch-0048

On 06/17/2016 09:43 AM, Oleg Fayans wrote:

Fixed a bug in the previous patch, automated 2 more testcases
from
http://www.freeipa.org/page/V4/Manage_replication_topology_4_4/Test_Plan






On 06/16/2016 04:46 PM, Oleg Fayans wrote:










IIUC, this will turn off the machine completely, how is cleanup done
then.  AFAIK our tests cannot turn on machine again and run
cleanup, so
you will not be able to run more tests on the same topology without
manual cleanup and manual start.

+replica = self.replicas[0]
+replica.run_command(['poweroff'])

IMO would be better to just call 'ipactl stop' instead of 'poweroff'


Agreed! Fixed.



Martin^2






*Automated ipa-replica-manage del tests*

1)
+replica.run_command(['ipactl', 'stop'])
+time.sleep(3)

Why do you need sleep here?


Removed, it was left from the old "poweroff" approach




2)
+ruvid_re = re.compile(".*%s:389: (\d+).*" % replica.hostname)
+replica_ruvs = ruvid_re.findall(result.stdout_text)
+master.run_command(['ipa-replica-manage', 'clean-ruv', 'f',
+'-p', master.config.dirman_password,
+replica_ruvs[0]])

Because you are using re.findall(), without any match you will receive
IndexError here replica_ruvs[0]. IMO it deserves assert before


Implemented the assert which checks that the output contains enough
replica RUVs



3)
assert(replica.hostname in result1.stdout_text)

I think that this is error prone. What if there is just error 'could
not
connect to replica ', or something similar.
instead of
listing/cleaning/whatever operation was executed. I think that it
should
be more specific regexp than just finding a replica name substring
(Yes
In IPA we dont always print error so stderr)

I'm not sure, but probably there might be cases when non critical
error
happen and exist status is still 0


Agree. Implemented a regex-based search



4)

+replica.run_command(['poweroff'])
+time.sleep(3)

There should not be poweroff, probably sleep could be removed too.


Gone




  *   Automated clean-ruv subcommand test*

1) PEP8, 2 new lines expected
./ipatests/test_integration/test_topology.py:163:1: E302 expected 2
blank lines, found 0
./ipatests/test_integration/test_topology.py:182:80: E501 line too
long
(85 > 79 characters)


Fixed




2)
I dont like doing assert just with count of occurences of substring in
STDOUT, would be possible to improve this somehow?


Maybe, but frankly, I don't see how. In this case we are making sure
that both simple and CA-specific RUVs of a replica are displayed. The
format of the output is strict:
Replica Update Vectors:
replica1_hostname:389: RUV_id
replica2_hostname:389: RUV_id
Certificate Server Replica Update Vectors:
replica1_hostname:389: RUV_id
replica2_hostname:389: RUV_id
If we do not see 2 occurrences of the replica hostname than definitely
something went wrong



3)
I'm not sure if clean-ruv is instant operations or there is some magic
happening in background (we have abort-clean-ruv). Maybe some sleep
should be there, but 

Re: [Freeipa-devel] [RFC] Matching and Mapping Certificates

[Alexander Bokovoy]

On pe, 07 loka 2016, Fraser Tweedale wrote:

On Thu, Oct 06, 2016 at 12:49:30PM +0200, Sumit Bose wrote:


Question, do we need search-and-replace at all (or at this
stage)? Most of the interesting values from the SAN should be
directly map-able to LDAP attributes. And processing the string
representation of  might be tricky as discussed below.
Nevertheless the following might be possible:

* /regexp/replacement/
* /regexp/replacement/

where "/regexp/replacement/" stands for optional sed-like
substitution rules. E.g. a rule like

   /^CN=\([^,]*\).*$/\1/

would take the subject string
'CN=Certuser,CN=Users,DC=example,DC=com' from the certificate and
generate a LDAP search filter component
'(samAccountName=Certuser)' which can be included in a LDAP search
filter which includes additional components like e.g. an
objectClass.


A counter-proposal w.r.t. DN mapping:

   

Where OID is either an actual OID or the corresponding string i.e.
"CN", "O", etc.  This would extract the "most specific" (leftmost in
the LDAP sense, rightmost in the X.500 sense) attribute value of the
specified type from the Subject DN.

IMO this would cover most DN mapping use cases whilst avoiding regex
or confusion about RDN order.  Therefore your original example of:

   /^CN=\([^,]*\).*$/\1/

can be accomplished with:

   

In the spirit of "make the simple things simple, and the hard things
possible" it is probably necessary to retain the regex variant to
handle more complex DN mapping use cases, e.g. where there are
multiple occurrences of a single attribute type, a particular fixed
RDN must be matched, etc.

w.r.t. SAN mapping, I concur that search/replace is probably not
needed.

How all these syntax extensions are going to handle multi-valued RDN?

--
/ Alexander Bokovoy

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

Re: [Freeipa-devel] [PATCH 0058] Make get_entries not ignore its size_limit argument

[Jan Cholasta]

On 17.8.2016 13:47, Stanislav Laznicka wrote:

On 08/11/2016 02:59 PM, Stanislav Laznicka wrote:

On 08/11/2016 07:49 AM, Jan Cholasta wrote:

On 2.8.2016 13:47, Stanislav Laznicka wrote:

On 07/19/2016 09:20 AM, Jan Cholasta wrote:

Hi,

On 14.7.2016 14:36, Stanislav Laznicka wrote:

Hello,

This patch fixes https://fedorahosted.org/freeipa/ticket/5640.

With not so much experience with the framework, it raises question
in my
head whether ipaldap.get_entries is used properly throughout the
system
- does it always assume that it gets ALL the requested entries or
just a
few of those as configured by the 'ipaSearchRecordsLimit'
attribute of
ipaConfig.etc which it actually gets?


That depends. If you call get_entries() on the ldap2 plugin (which is
usually the case in the framework), then ipaSearchRecordsLimit is
used. If you call it on some arbitrary LDAPClient instance, the
hardcoded default (= unlimited) is used.



One spot that I know the get_entries method was definitely not used
properly before this patch is in the
baseldap.LDAPObject.get_memberindirect() method:

 692 result = self.backend.get_entries(
 693 self.api.env.basedn,
 694 filter=filter,
 695 attrs_list=['member'],
 696 size_limit=-1, # paged search will get
everything
anyway
 697 paged_search=True)

which to me seems kind of important if the environment size_limit
is not
set properly :) The patch does not fix the non-propagation of the
paged_search, though.


Why do you think size_limit is not used properly here?

AFAIU it is desired that the search is unlimited. However, due to the
fact that neither size_limit nor paged_search are passed from
ldap2.get_entries() to ldap2.find_entries() (methods inherited from
LDAPClient), only the number of records specified by
ipaSearchRecordsLimit is returned. That could eventually cause problems
should ipaSearchRecordsLimit be set to a low value as in the ticket.


I see. This is *not* intentional, the **kwargs of get_entries()
should be passed to find_entries(). This definitely needs to be fixed.



Anyway, this ticket is not really easily fixable without more profound
changes. Often, multiple LDAP searches are done during command
execution. What do you do with the size limit then? Do you pass the
same size limit to all the searches? Do you subtract the result size
from the size limit after each search? Do you do something else with
it? ... The answer is that it depends on the purpose of each
individual LDAP search (like in get_memberindirect() above, we have to
do unlimited search, otherwise the resulting entry would be
incomplete), and fixing this accross the whole framework is a
non-trivial task.


I do realize that the proposed fix for the permission plugin is not
perfect, it would probably be better to subtract the number of
currently
loaded records from the sizelimit, although in the end the number of
returned values will not be higher than the given size_limit. However,
it seems reasonable that if get_entries is passed a size limit, it
should apply it over current ipaSearchRecordsLimit rather than ignoring
it. Then, any use of get_entries could be fixed accordingly if someone
sees fit.



Right. Anyway, this is a different issue than above, so please put
this into a separate commit.


Please see the attached patches, then.


Self-NACK, with Honza's help I found there was a mistake in the code. I
also found an off-by-one bug which I hope I could stick to the other two
patches (attaching only the modified and new patches).


Works for me, but this bit in patch 0064 looks suspicious to me:

+if max_entries > 0 and len(entries) == max_entries:

Shouldn't it rather be:

+if max_entries > 0 and len(entries) >= max_entries:

?

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