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

Provider cross validation CI runs #20552

Closed
wants to merge 5 commits into from

Conversation

paulidale
Copy link
Contributor

  • documentation is added or updated
  • tests are added or updated

@paulidale paulidale added the branch: master Merge to master branch label Mar 21, 2023
@paulidale paulidale self-assigned this Mar 21, 2023
@paulidale paulidale force-pushed the provider-compat branch 28 times, most recently from e46f756 to 4d0181c Compare March 21, 2023 05:03
@paulidale
Copy link
Contributor Author

paulidale commented Mar 23, 2023

Cross validation tests are passing.

@paulidale paulidale changed the title Provider compat Provider cross validation CI runs Mar 23, 2023
@t8m
Copy link
Member

t8m commented Mar 23, 2023

I believe this is still testing too much and it would require too much maintenance that has no real effect.

IMO from the released versions we should only test the fips provider against the libcrypto & test suite from all the current branches. The full cross testing should be done only across the tips of the branches.

This is sufficient to test that the fips providers are forwards compatible - this is actually the main use-case we need to support. The test suites from the old branches CANNOT in principle be fully compatible with updated providers, otherwise we would not be ever able to change things in providers. So you cannot ever expect the test suite from the 3.0.0 release (or any other release) to fully pass with new fips providers.

If I count it properly we should have 3 released fips providers * 3 branches + 3*2 branches tests -> 15 tests and you can also drop the old FIPS provider compat test as that is just duplicate.

@paulidale
Copy link
Contributor Author

paulidale commented Mar 23, 2023

I'm basically doing what you suggest. No release is tested against any other release. Nothing is tested against itself.
I set up the jobs to cross test everything because I couldn't figure out a workable alternative. However, the unwanted cases are immediately short-circuited and early exit. E.g. look at this run. I suggest we leave any "additional" tests here, they can be removed once there is a problem.

The old FIPS provider compatibility check is per push not daily, so more timely. Happy to drop it.

@t8m
Copy link
Member

t8m commented Mar 27, 2023

I suggest we leave any "additional" tests here, they can be removed once there is a problem.

You already have some hacks there such as skipping the ssl tests from 3.0.0 release. I am afraid they will become unmaintainable at some point. I would simply drop the cases where you're running tests from released version tree.

@t8m t8m added triaged: feature The issue/pr requests/adds a feature tests: present The PR has suitable tests present approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member labels Mar 27, 2023
@paulidale
Copy link
Contributor Author

paulidale commented Mar 28, 2023

Isn't it useful to learn when the 3.1.x FIPS provider ceases working with the 3.1.0 release?
Dropping these cases loses this information.

The test for each version could dropped or limited to branch heads. It should be possible to not rebuild these each run but that's getting into some arcane depths of actions I'd prefer not to touch at the moment.

@t8m
Copy link
Member

t8m commented Mar 29, 2023

Isn't it useful to learn when the 3.1.x FIPS provider ceases working with the 3.1.0 release? Dropping these cases loses this information.

I am not sure it is that much useful. I would assume if there is really an incompatibility being introduced the test of 3.0.x branch with the 3.1.x FIPS provider would reveal it as well. And as inevitably this can and will happen you would have to limit the failing test (or patch it somehow) anyway.

@paulidale
Copy link
Contributor Author

https://github.com/openssl/openssl/actions/runs/4558459775 passing with the reduced testing.

@t8m t8m removed the approval: otc review pending This pull request needs review by an OTC member label Mar 30, 2023
@hlandau hlandau added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Apr 7, 2023
@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but this PR has failing CI tests. Once the tests pass it will get moved to 'approval: ready to merge' automatically, alternatively please review and set the label manually.

@paulidale
Copy link
Contributor Author

Trivial edit before merge to remove end of line white space from the new test.
Thanks for the reviews.

@paulidale paulidale closed this Apr 11, 2023
openssl-machine pushed a commit that referenced this pull request Apr 11, 2023
Tests all released FIPS approved (or in progress) versions against
all development branches and each other.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #20552)
openssl-machine pushed a commit that referenced this pull request Apr 11, 2023
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #20552)
openssl-machine pushed a commit that referenced this pull request Apr 11, 2023
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #20552)
@paulidale paulidale deleted the provider-compat branch June 28, 2023 06:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: done This pull request has the required number of approvals branch: master Merge to master branch tests: present The PR has suitable tests present triaged: feature The issue/pr requests/adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants