PR - Issue: 50112 - Port ACI test suit from TET to python3(keyaci) #3239

A lot of issues that you have here, already mentioned by @Firstyear in previous PRs.
I'll go once again through the stuff - just to be more explicit...

But. Let's first fix small issues regarding the code. It will increase the readability and it covers 80% of the issues we usually mention in the review.

Please, run this and fix the issues it points out:

pip3 install pytest-pylint
py.test --pylint -m pylint dirsrvtests/tests/suites/acl/keywords_part2_test.py dirsrvtests/tests/suites/acl/keywords_test.py

Also, reach to me or any of us if you have any doubts about the report.

389-ds-bot · 2020-09-13T04:56:43Z

Comment from aborah (@aborah-sudo) at 2019-02-20 17:28:39

rebased onto cda118046e82f0ef88e32c2c01781568bf77496f

389-ds-bot · 2020-09-13T04:56:44Z

Comment from firstyear (@Firstyear) at 2019-02-21 00:51:04

This may not work the way you want. Imagine I have my hosts file setup as:

172.24.10.1     systemhostname.localdomain

So socket.gethostname will give me my hostname, then it will give me the 172.24.x.x address BUT we are connected to the the test instance via localhost.

So I think this will have issues in containers and other host configurations.

Possibly the best thing to do here is actually just make ip '127.0.0.1', because in tests we generally connect to localhost only, and we should only be using that. Check topologies.py maybe.

389-ds-bot · 2020-09-13T04:56:45Z

Comment from firstyear (@Firstyear) at 2019-02-21 00:52:07

'(target = "ldap:///{}")(targetattr=*)(version 3.0; aci "IP aci"; allow(all) userdn = "ldap:///{}" and ip != "{}" ;)'

This could be turned into an ACI_TEMPLATE constant that you populate into, and use named formats, not positional, for clarity.

389-ds-bot · 2020-09-13T04:56:45Z

Comment from firstyear (@Firstyear) at 2019-02-21 00:54:04

You have this mod_seealso, but it only replaces one line? You also by doing the _mod_seealso are sneaky violating a rule I told you: ONLY test the exceptions
from a single function.

Your modsee also is initing the object with the conn and doing the read, when you actually, should have the object create outside of the "with pytest.raises".

Remember, all your code must follow this pattern. "correct, simple, fast". you have targetted "simple, correct, fast". As a result, you have broken a correctness rule by prematurely trying to over simplify.

389-ds-bot · 2020-09-13T04:56:46Z

Comment from firstyear (@Firstyear) at 2019-02-21 01:00:24

This is not a good test because you are relying on external state of the system to then check the TIME, and determine results.

Consider the CI server. It only runs at about 12:00 my time AEST, so that's probably about 02:00AM UTC at a guess. So then this test in CI will ONLY ever
attempt the MORNING path, and will never actually check the AFTERNOON path. Before you say "but there is a test data in the afternoon ..." time is continually flowing, and so between this test and the afternoon test what happens if it goes from morning to afternoon? Now what? There are cases left untested.

A better solution is to take the current time, +- 2 hours either side as your allow window, then check you are allowed. Then have a denied test where you have +12 (+- 2 hours) from now, and assert it's denied.

No timezone issues, now flow issues, no risks.

A better solution is LD_PRELOAD a time shim into the server but that's wayyyyy out of scope.

ALso, this is probably a really ineffective way to get a datetime. You actually want:

>>> from datetime import datetime
>>> datetime.now()
>>> datetime.now().hour

I don't really know why you chose to code this like this, but it seems like instead of taking the time to research the right way to do it, you chose to rush and bruteforce a solution.

389-ds-bot · 2020-09-13T04:56:47Z

Comment from firstyear (@Firstyear) at 2019-02-21 01:01:24

This is not a clear explanation, and your test code isn't clear what the ACI is testing due to your use of constants. I think you need a much better comment here about what this is attempting to do.

389-ds-bot · 2020-09-13T04:56:48Z

Comment from firstyear (@Firstyear) at 2019-02-21 01:02:21

Similar: this is not a good comment. WHY can they not access the data? How is the ACI constructed to not allow the access?

389-ds-bot · 2020-09-13T04:56:49Z

Comment from firstyear (@Firstyear) at 2019-02-21 01:02:41

Why?

389-ds-bot · 2020-09-13T04:56:50Z

Comment from firstyear (@Firstyear) at 2019-03-05 00:42:47

Email says @aborah-sudo has commented, but they are not appearing.

ip = socket.gethostbyname(socket.gethostname()) --->>> This is the real ip of my machine.
If i have to follow you point , i have to assign '127.0.0.1' to my machine or test will fail .

socket.gethostbyname(socket.gethostname()) is the universal way to detect any ip address . If
your machine have ip of "127.0.0.1", socket.gethostbyname(socket.gethostname()) will detect it
and will assign it to aci .

I'm sorry, but this is incorrect in this context. gethostname get's you the primary interface of the machine, and only if your hosts file is filled correctly. Imagine my machine hostname was "candice" but /etc/hosts didn't have that entry? Then socket.gethostbyname will fail because there is no host candice.

When you are writing tests like this, the only ip you can guarantee will exist is 127.0.0.1. That's it. As well, you are always connecting by localhost (because that's what DirSrv does in tests!). Which means if you want to test an ACI by ip, you have to test by the ip we are connecting on which is always localhost. There is no way for you to avoid that.

Perhaps to make this more clear, here the output of ip from my docker env:

1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN group default qlen 1
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
    inet 127.0.0.1/8 scope host lo
       valid_lft forever preferred_lft forever
12: eth0@if13: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP group default
    link/ether 02:42:ac:11:00:04 brd ff:ff:ff:ff:ff:ff link-netnsid 0
    inet 172.17.0.4/16 brd 172.17.255.255 scope global eth0
       valid_lft forever preferred_lft forever

So here, socket gethostname will give me 'ldapkdc.example.com' for the container, that then resolves to '172.17.0.4', but dirsrv will connect via localhost, so will bypass the aci anyway.

For a different example, my laptop:

lo0: flags=8049<UP,LOOPBACK,RUNNING,MULTICAST> mtu 16384
	options=1203<RXCSUM,TXCSUM,TXSTATUS,SW_TIMESTAMP>
	inet 127.0.0.1 netmask 0xff000000
	inet6 ::1 prefixlen 128
	inet6 fe80::1%lo0 prefixlen 64 scopeid 0x1
	nd6 options=201<PERFORMNUD,DAD>
...
en0: flags=8863<UP,BROADCAST,SMART,RUNNING,SIMPLEX,MULTICAST> mtu 1500
	ether 38:f9:d3:51:eb:07
	inet6 fe80::1c5e:d5d5:c994:6652%en0 prefixlen 64 secured scopeid 0xa
	inet 192.168.1.119 netmask 0xffffff00 broadcast 192.168.1.255
	nd6 options=201<PERFORMNUD,DAD>
	media: autoselect
	status: active

>>> import socket
>>> socket.gethostname()
'amethyst'
>>> socket.gethostbyname('amethyst')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
socket.gaierror: [Errno 8] nodename nor servname provided, or not known

So the test would FAIL on my machine here, because my hosts is:

127.0.0.1	localhost
255.255.255.255	broadcasthost
::1             localhost
fe80::1%lo0	localhost

Please, just use localhost. I'm offering a simpler, more robust way for you to conduct the test case here (just use localhost), but instead you seem to be arguing for a more complex, less portable, more fragile solution? I'm won't accept it I'm sorry.

389-ds-bot · 2020-09-13T04:56:51Z

Comment from aborah (@aborah-sudo) at 2019-03-27 05:03:06

rebased onto 550bb86943571086cd30aaabab507f2717f0c6d3

389-ds-bot · 2020-09-13T04:56:52Z

Comment from aborah (@aborah-sudo) at 2019-03-27 05:05:50

Changes are done , Please check

389-ds-bot · 2020-09-13T04:56:53Z

Comment from aborah (@aborah-sudo) at 2019-03-27 05:55:53

rebased onto 31500960a5a566f39c7e4b32bd43ade5ed2ea1d3

389-ds-bot · 2020-09-13T04:56:53Z

Comment from aborah (@aborah-sudo) at 2019-04-01 06:48:30

@droideck , @Firstyear , please review. @mreynolds389 already has already ack for this one

389-ds-bot · 2020-09-13T04:56:54Z

Comment from spichugi (@droideck) at 2019-04-01 12:56:33

The comments like this look weird. Also I don't see any sense in them if we already have the test function name and docstrings. They just repeat.

389-ds-bot · 2020-09-13T04:56:55Z

Comment from spichugi (@droideck) at 2019-04-01 12:57:41

The docstring doesn't make much sense...

389-ds-bot · 2020-09-13T04:56:56Z

Comment from spichugi (@droideck) at 2019-04-01 13:05:05

dirsrvtests/tests/suites/acl/keywords_part2_test.py::test_access_from_certain_network_only_ip 
dirsrvtests/tests/suites/acl/keywords_part2_test.py::test_timeofday_keyword

These tests have failed for me on OpenStack machine (1minutetip).
Exceptions - DID NOT RAISE

389-ds-bot · 2020-09-13T04:56:56Z

Comment from firstyear (@Firstyear) at 2019-04-02 01:32:06

The comments like this look weird. Also I don't see any sense in them if we already have the test function name and docstrings. They just repeat.

I've asked him to write comments explaining what the tests do, but I think these comments need work as you point out ... They should explain more about the intent, and why we test it the way we do, and why things are structured as they are for example.

389-ds-bot · 2020-09-13T04:56:57Z

Comment from vashirov (@vashirov) at 2019-04-02 12:38:54

These tests have failed for me on OpenStack machine (1minutetip).
Exceptions - DID NOT RAISE

This worked for me, what do you have in the logs?

389-ds-bot · 2020-09-13T04:56:58Z

Comment from aborah (@aborah-sudo) at 2019-04-03 08:24:40

rebased onto c0acc23cebea1f719f5cce63239627aeb59733b1

389-ds-bot · 2020-09-13T04:56:59Z

Comment from aborah (@aborah-sudo) at 2019-04-03 08:27:50

@droideck , @Firstyear , @vashirov All changes are done , please check

389-ds-bot · 2020-09-13T04:57:31Z

Comment from aborah (@aborah-sudo) at 2019-04-30 18:22:52

@droideck all changes are done as per your suggestion

389-ds-bot · 2020-09-13T04:57:32Z

Comment from aborah (@aborah-sudo) at 2019-04-30 18:22:53

rebased onto 5b244257999c76e28823c9cdabf2566bfcf8a0a3

389-ds-bot · 2020-09-13T04:57:32Z

Comment from aborah (@aborah-sudo) at 2019-05-01 05:29:14

rebased onto 53f94845fb6f8086a050ba0fefe555e4cb516ec6

389-ds-bot · 2020-09-13T04:57:33Z

Comment from aborah (@aborah-sudo) at 2019-05-01 05:31:51

rebased onto 61d1e12b73ea87b19de2aae6f99f094c8ec4be4e

389-ds-bot · 2020-09-13T04:57:34Z

Comment from spichugi (@droideck) at 2019-05-01 11:11:16

Please, sort the list.
The returned order matters.
Can you please point it out again , i missed it

(Pdb) ous.sort
<built-in method="" sort="" of="" list="" object="" at="" 0x7f046bb18b48="">
(Pdb) ous.sort()
*** TypeError: '<' not supported between instances of 'OrganizationalUnit' and 'OrganizationalUnit'
(Pdb) ous.reverse()
it can be reversed , but cant be sorted

The return order of the ldapsearch (and search_s) is not defined.
To be sure, we should sort the objects.

OrganizationalUnit doesn't have sort but you still can sort.
For that, you can use sorted() function - set key to the OrganizationalUnit.dn length using a lambda function. And set reverse to True.
Then you can safely run through the list and delete the objects.

389-ds-bot · 2020-09-13T04:57:35Z

Comment from aborah (@aborah-sudo) at 2019-05-01 12:04:17

rebased onto 8aeeae3b54f47afd867ef09cdb75c3419fd02151

389-ds-bot · 2020-09-13T04:57:36Z

Comment from aborah (@aborah-sudo) at 2019-05-01 12:05:30

@droideck all changes are done as per your suggestion

389-ds-bot · 2020-09-13T04:57:36Z

Comment from aborah (@aborah-sudo) at 2019-05-02 08:13:05

rebased onto 5bae063a0b771d12a977c2a868640d7a4a3a5ca5

389-ds-bot · 2020-09-13T04:57:37Z

Comment from spichugi (@droideck) at 2019-05-02 10:52:27

You can sort directly the ous_next.list(). For that, you should set the key to the lambda function for len(i.dn).

389-ds-bot · 2020-09-13T04:57:38Z

Comment from aborah (@aborah-sudo) at 2019-05-02 11:07:45

rebased onto 9b04ab2e041b5a8d78bed8929115cc959935e143

389-ds-bot · 2020-09-13T04:57:39Z

Comment from aborah (@aborah-sudo) at 2019-05-02 11:13:42

@droideck all changes are done as per your suggestion

389-ds-bot · 2020-09-13T04:57:40Z

Comment from spichugi (@droideck) at 2019-05-02 19:55:49

Looks good. Small nitpick though - could you please change the i to something like ou? It is better to use meaningful names always.
Otherwise - very good job! Great

389-ds-bot · 2020-09-13T04:57:41Z

Comment from spichugi (@droideck) at 2019-05-02 19:59:20

The test case still fails on 1minutetip machine.
As @viktor pointed out, it happens because of the race condition. You can see it on fast machines while no special logging is enabled.
Could you please check it (reproduce) and then fix it with a flush of the logs or some other sane way?

389-ds-bot · 2020-09-13T04:57:41Z

Comment from aborah (@aborah-sudo) at 2019-05-03 03:25:39

@droideck , till i figure out why that specific test case is failing (on fast machines), can you please start reviewing this PR : #3251

389-ds-bot · 2020-09-13T04:57:42Z

Comment from aborah (@aborah-sudo) at 2019-05-06 12:02:24

rebased onto 046f9e2b724145a56a328f0763efcc26f1784fdc

389-ds-bot · 2020-09-13T04:57:43Z

Comment from aborah (@aborah-sudo) at 2019-05-06 12:04:00

@droideck all changes are done as per your suggestion , i have done some changes in failed test cases , hopefully these test cases will not fail now

389-ds-bot · 2020-09-13T04:57:44Z

Comment from spichugi (@droideck) at 2019-05-06 15:55:17

Just realised, dn_dn looks pretty weird. What is it?
I suggested to use ou here because it represents OrganisationalUnit object. It is not a DN, it is a DSLdapObject.

Could you please fix it here and in other places? dn_dn is really confusing.

389-ds-bot · 2020-09-13T04:57:45Z

Comment from spichugi (@droideck) at 2019-05-06 15:56:31

Please, check the documentation about conftest.py. You don't need to import it. It is used by pytest by default.

389-ds-bot · 2020-09-13T04:57:45Z

Comment from spichugi (@droideck) at 2019-05-06 15:58:48

dirsrvtests/tests/suites/acl/keywords_part2_test.py::test_timeofday_keyword still fails for me.

Also dirsrvtests/tests/suites/acl/keywords_part2_test.py::test_connectin_from_an_unauthorized_network has strated to fail.

>       ip_ip = topo.standalone.ds_access_log.match('.* connection from ')[0].split()[-1]
E       IndexError: list index out of range

389-ds-bot · 2020-09-13T04:57:47Z

Comment from aborah (@aborah-sudo) at 2019-05-06 16:16:48

rebased onto 91361fa3db77c4930222b524b5cbae9528053457

389-ds-bot · 2020-09-13T04:57:48Z

Comment from aborah (@aborah-sudo) at 2019-05-06 16:20:30

rebased onto 77aa76b178e7a0e33c915d75f2f82e2f3b21512b

389-ds-bot · 2020-09-13T04:57:49Z

Comment from aborah (@aborah-sudo) at 2019-05-06 16:21:36

@droideck all changes are done as per your suggestion, Can you please rerun the tests

389-ds-bot · 2020-09-13T04:57:49Z

Comment from aborah (@aborah-sudo) at 2019-05-06 16:23:16

rebased onto cfe033a6713316a40c970f4cde3f884fc4053990

389-ds-bot · 2020-09-13T04:57:50Z

Comment from spichugi (@droideck) at 2019-05-06 17:06:54

dirsrvtests/tests/suites/acl/keywords_part2_test.py::test_connectin_from_an_unauthorized_network is still failing but with another error.

        # now user can access data
>       org.replace("seeAlso", "cn=1")
E         ldap.INSUFFICIENT_ACCESS: {'desc': 'Insufficient access', 'info': "Insufficient 'write' privilege to the 'seeAlso' attribute of entry 'ou=ip,ou=keywords,dc=example,dc=com'.\n"}

And about dirsrvtests/tests/suites/acl/keywords_part2_test.py::test_timeofday_keyword...
Why did you remove the failing part of the test case instead of fixing it?
I've mentioned a possible cause of the failure here - #3239#comment-85197

389-ds-bot · 2020-09-13T04:57:51Z

Comment from aborah (@aborah-sudo) at 2019-05-06 17:36:15

rebased onto 468b8a8

389-ds-bot · 2020-09-13T04:57:52Z

Comment from aborah (@aborah-sudo) at 2019-05-06 17:37:11

@droideck all changes are done as per your suggestion, Can you please rerun the tests

389-ds-bot · 2020-09-13T04:57:53Z

Comment from spichugi (@droideck) at 2019-05-06 18:32:24

LGTM! Thanks!
Ack

389-ds-bot · 2020-09-13T04:57:54Z

Comment from vashirov (@vashirov) at 2019-05-07 12:06:07

Pull-Request has been merged by vashirov

389-ds-bot · 2020-09-13T04:57:56Z

Patch
50180.patch

389-ds-bot added merged Migration flag - PR pr Migration flag - PR labels Sep 13, 2020

389-ds-bot closed this as completed Sep 13, 2020

389-ds-bot mentioned this issue Sep 13, 2020

PR - Issue: 50112 - Port ACI test suit from TET to python3(Global Group) #3227

Closed

PR - Issue: 50112 - Port ACI test suit from TET to python3(keyaci) #3239

PR - Issue: 50112 - Port ACI test suit from TET to python3(keyaci) #3239

Comments

389-ds-bot commented Sep 13, 2020 • edited

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020 • edited

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020 • edited

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020

389-ds-bot commented Sep 13, 2020 •

edited

389-ds-bot commented Sep 13, 2020 •

edited

389-ds-bot commented Sep 13, 2020 •

edited