Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Activate running LDAP tests for GH actions #25

Closed
wants to merge 38 commits into from
Closed

Activate running LDAP tests for GH actions #25

wants to merge 38 commits into from

Conversation

kevinpapst
Copy link

@kevinpapst kevinpapst commented Feb 10, 2022

Enables research on further issues. See #24

This is me, poking around in an existing CI system with existing tests which I hardly understand.

I cannot get the iptables rules to import, maybe some docker issue or something simple that I just don't see.
I don't have experience with iptables / iptables.restore, so I give up and wait for someone who has an idea.

At least the test results show that there is an issue in 8.1 that is covered by the tests, as the failures change:

  • 7.3: Tests: 522, Assertions: 1330, Failures: 25, Skipped: 5.
  • 7.4: Tests: 522, Assertions: 1330, Failures: 25, Skipped: 5.
  • 8.0: Tests: 522, Assertions: 1327, Errors: 1, Failures: 24, Skipped: 5.
  • 8.1: Tests: 522, Assertions: 795, Errors: 177, Failures: 16, Skipped: 1.

@samsonasik samsonasik marked this pull request as ready for review February 10, 2022 11:55
@samsonasik samsonasik marked this pull request as draft February 10, 2022 11:55
@froschdesign froschdesign linked an issue Feb 10, 2022 that may be closed by this pull request
@Ocramius
Copy link
Member

uh-oh, all the phpvfscomposer:// failures seem to be related to composer/composer:2.2.0 and similar releases. @asgrim do you have some pointers? You've seen that before...

@asgrim
Copy link
Member

asgrim commented Feb 11, 2022

uh-oh, all the phpvfscomposer:// failures seem to be related to composer/composer:2.2.0 and similar releases. @asgrim do you have some pointers? You've seen that before...

Hmm, I don't see anything specifically that could be caused by the phpvfscomposer:// thing; you will of course see it in the stack trace, but that is expected; the failures I saw were very edge case, these just looks like regular test failures?

22) LaminasTest\Ldap\SearchTest::testReverseSortingWithSearchEntriesShortcut
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'e'
+'mylocation2'

Error: /github/workspace/test/SearchTest.php:377
phpvfscomposer:///github/workspace/vendor/phpunit/phpunit/phpunit:91

@@ -4,4 +4,10 @@ WORKING_DIRECTORY=$2
JOB=$3
PHP_VERSION=$(echo "${JOB}" | jq -r '.php')

apt install -y php8.1-ldap || exit 1
apt-get install -y iptables conntrack || exit 1
iptables-restore --verbose --test ./.ci/iptables.rules
Copy link
Author

@kevinpapst kevinpapst Feb 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't work, the rules are not accepted, no matter what I tried:

iptables-restore v1.8.4 (legacy): iptables-restore: unable to initialize table 'filter'
# Generated by iptables-save v1.4.21 on Fri Sep 29 15:37:54 2017

Error occurred at line: 2

I tested the rule on another system and it was successful, so my guess is that this is some kind of docker issue that I don't understand. I extracted the rules to a different file, just for the sake of testing, because nothing else worked....

All ReconnectTest issues are caused by this problem.

@@ -5,7 +5,7 @@ LDAP_DB=/tmp/ldap_db

echo "Creating database directory"

rm -rf ${LDAP_DB} && mkdir ${LDAP_DB} && cp /usr/share/doc/slapd/examples/DB_CONFIG ${LDAP_DB}
rm -rf ${LDAP_DB} && mkdir ${LDAP_DB}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This DB_CONFIG file doesn't exist at multiple locations and I couldn't find it on my Ubuntu systems as well. New Ubuntu system have the config extracted in the filesystem, maybe the file is obsolete nowadays?
Besides, the manpage says that the file only applies to bdb (Berkley) config backend. But we have hdb configured as backend. So I guess it can be safely removed.

@kevinpapst
Copy link
Author

kevinpapst commented Feb 11, 2022

If you check the test failures, there are only 2 failing categories:

  • ReconnectTest: this one is caused by the iptables problem (explained above: rules cannot be imported)
  • SearchTest: seems to be a logical issue, maybe caused by a different OpenLDAP version/default configuration

@kevinpapst
Copy link
Author

@heiglandreas @Maks3w both of you worked on the Laminas LDAP tests in the past. Could you probably spare some minutes and help me fixing the Github action integration of the LDAP rules for PHP 8.1?

@heiglandreas
Copy link
Contributor

Sure. How, where and when?

@kevinpapst
Copy link
Author

kevinpapst commented Mar 7, 2022

Well, background story: we are are stuck with a broken/non-working LDAP package on 8.1. As the LDAP functions changed their signatures in 8.1 we have to apply some critical changes, which obviously should be covered by tests.
These tests currently do not work after the migration from Travis to Github actions.

I was able to re-activate most tests. But there is one specific set of tests (which I documented above) in the integration testsuite that performs real connection tests. These connection tests rely on the import of iptable rules, which I cannot get to work. The import fails and after some hours of try&error I had to give up. Now I am hoping that someone can help me to fix the test problem , so I can afterwards start fixing the real issue with 8.1

EDIT: and thanks for your quick response 👍

@heiglandreas heiglandreas mentioned this pull request Mar 13, 2022
@heiglandreas
Copy link
Contributor

Looks like the reconnect-test issues are caused by the hack that we used previously to drop the flows for the LDAP-Port from the TCP-State table of the kernel does not work any more as it requires root access which we do not seem to have. And docker is much more restricted in what we can do to the actual kernel...

I'll see how we can find a way to get around that...

@heiglandreas
Copy link
Contributor

It looks like the easiest would be to use a separate container for running the LDAP-Server as well as the webserver to initiate dropping the TCP-Connection to that server. That way the container can be ran with -cap-add SYS_ADMIN --cap-add NET_ADMIN (which might also solve the issues with setting the IP-Table rules) which should allow to actually drop the connection.

Drawback seems to be that

  • it needs to be ran manually (see https://github.community/t/how-to-run-privileged-docker-container/16431) which is kinda dirty.
  • It also means that we need to build and host a separate image somewhere and
  • It means that we run one container for the whole of the matrix so we need to possibly handle the dropping more carefully to not drop the wrong connection. Or we need a way to start the container from the pre-install script. Any hints on that, anyone (@Ocramius, @MWOP, @asgrim)?

@Ocramius
Copy link
Member

Q: why do we need privileged network access at all? Isn't an LDAP server just a normal TCP/UDP server listening on some port?

@heiglandreas
Copy link
Contributor

If we want to continue testing that the lib works when network connections to the LDAP server are dropped (and other network quirkeries) we need to be able to acrually drop the connection on the server side. Which requires access to the networking stack which in turn requires privileged access.

So either we find a way to get privileged access or we drop the tests around failing networks.

@Ocramius
Copy link
Member

Makes sense, thanks!

As for starting docker containers manually, I kinda do it all the time for work, but it isn't really laminas-ci friendly 😬

Laminas-ci allows to run some pre-start hooks, but those scripts are run from within the CI container 🤔

@kevinpapst
Copy link
Author

What about being pragmatic here, grouping the test into two testsuites and concentrating on the ones we can adjust/fix now? Then the way would be open to address the PHP 8.1 issues.

If we keep on trying to fix these networks tests first, then Laminas LDAP might be incompatible with PHP 8.1 for weeks ahead...while falsly announcing that it is.

@kevinpapst
Copy link
Author

As my suggestion with the two test suites was added by @heiglandreas we can close this PR now and work on #27

@kevinpapst kevinpapst closed this Mar 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PHP 8.1 no longer returns a resource
4 participants