Fix x509 test fails on old openssl systems

openSUSE / salt

openSUSE and SUSE patches and backports for SaltStack

Apache License 2.0

22 stars 52 forks source link

Fix x509 test fails on old openssl systems #682

Closed m-czernek closed 2 weeks ago

m-czernek commented 3 weeks ago

What does this PR do?

This PR:

fixes our failing x509 tests on SLES12SP5 and CentOS7
also fixes the test_performance test, which cannot run in GH CI (it was previously skipped in CI due to the missing docker binary, which we now provide)

Note: We're already diverging in the x509 tests from upstream, because upstream does not test Salt on old systems (like Centos7 or SLES12). Not sure if we need to upstream these changes (or if they are relevant to upstream), but I'm open to at least attempting it.

What issues does this PR fix or reference?

Relates to: https://github.com/SUSE/spacewalk/issues/23286

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

m-czernek commented 2 weeks ago

@vzhestkov we are only skipping execution on unsupported algorithms, which are: ec, ed25519, and ed448. If a test uses the @pytest.mark.skipif annotation, it's because it hardcodes an unsupported algorithm.

In other words, tests that are currently passing on the older openssl version are not skipped.

Previously, we had a hacky "run the test, and skip it based on a specific exception" solution. However, this is a problem because not all tests provide nice exception. For example, I think some tests that used the ed25519 and ed448 algorithms simply failed to read a PEM key (which failed to be created due to an unsupported algorithm).

With that in mind, do you still object to the solution? Would you prefer to do the "run the code, skip based on the exception" approach? Or is there another solution that you can think of?

vzhestkov commented 2 weeks ago

Yes, I understand, but I juse mentioned there were some tests before which was causing segfaults while calling some certain functions, so in case if we just disable the test by the condition before trying to execute the test we could potentially hide some of such issues, sure these algorithms are not supported but better ensure that in this case the call returning the exception with the proper type or description, but not just disable them.

m-czernek commented 2 weeks ago

Ack. This should be more in line with your views @vzhestkov :)