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

Fix x509 test fails on old openssl systems #682

Merged
merged 3 commits into from
Oct 2, 2024

Conversation

m-czernek
Copy link
Contributor

@m-czernek m-czernek commented Sep 24, 2024

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.

Copy link
Contributor

@vzhestkov vzhestkov left a comment

Choose a reason for hiding this comment

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

Explicit check for OpenSSL version looks bit redundant here, with the latest alignments for pyopenssl module I've fixed the case when there was a segfault erros on calling certain calls and as a result the test ends unexpectedly with ERROR or FAIL, but with such checks for the version before calling the test we can hide such segfaults.

@m-czernek
Copy link
Contributor Author

@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
Copy link
Contributor

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
Copy link
Contributor Author

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

Copy link
Contributor

@vzhestkov vzhestkov left a comment

Choose a reason for hiding this comment

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

Thanks for adjusting and taking care of it 👍

@m-czernek m-czernek merged commit 7daf461 into openSUSE:openSUSE/release/3006.0 Oct 2, 2024
8 checks passed
@m-czernek m-czernek deleted the x509-fails branch October 2, 2024 07:09
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.

2 participants