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

Update FIPS140-3 exclusions #22

Merged
merged 1 commit into from
Aug 20, 2024
Merged

Conversation

JinhangZhang
Copy link
Contributor

@JinhangZhang JinhangZhang commented Jul 30, 2024

Exclude these tests to make it easier for the team to triage test cases and problems that occur due to changes going forward.

Ignore all tests affected by FIPS 140-3 testing. A quick review of all the failing tests was done by various people on the development team to get a quick sense to the nature of the failure. At this time we aren't documenting and opening issues for each individual failure as this will take a long time and is currently holding up regression testing of various releases.

Future work is planned to address necessary modifications to various tests on a package by package basis.

@JinhangZhang
Copy link
Contributor Author

@llxia @jasonkatonica As discussed, re-enable update where test ignores were removed in #18 & #19.

@JinhangZhang JinhangZhang force-pushed the m2updates branch 4 times, most recently from 4a30610 to 95971f9 Compare July 30, 2024 06:27
@JinhangZhang JinhangZhang marked this pull request as draft July 30, 2024 06:31
@JinhangZhang JinhangZhang force-pushed the m2updates branch 2 times, most recently from f2d4ad5 to 7f61947 Compare July 30, 2024 19:32
@JinhangZhang JinhangZhang marked this pull request as ready for review July 30, 2024 19:34
@JinhangZhang
Copy link
Contributor Author

@pshipton @JasonFengJ9 please help to review

@@ -428,11 +428,13 @@ sun/security/krb5/runNameEquals.sh https://github.com/ibmruntimes/openj9-openjdk
sun/security/krb5/auto/Cleaners.java https://github.com/ibmruntimes/openj9-openjdk-jdk23/issues/20 linux-x64,linux-ppc64le,linux-s390x
java/lang/ProcessBuilder/JspawnhelperWarnings.java https://github.com/ibmruntimes/openj9-openjdk-jdk23/issues/20 linux-x64,linux-ppc64le,linux-s390x,aix-all
java/lang/String/CompactString/NegativeSize.java https://github.com/ibmruntimes/openj9-openjdk-jdk23/issues/20 linux-ppc64le,linux-s390x,aix-all
java/security/Security/ConfigFileTest.java https://github.com/ibmruntimes/openj9-openjdk-jdk23/issues/20 linux-ppc64le,linux-s390x
Copy link
Member

Choose a reason for hiding this comment

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

This pull request seems misnamed. This appears to be the only test that is (re-enabled); every other change excludes extra tests or platforms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the name

Copy link
Member

Choose a reason for hiding this comment

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

The commit message could still be better. This doesn't just "add" exclusions it also re-enables ConfigFileTest. Perhaps it should say something like "Update FIPS140-3 exclusions" (at a minimum). I would like to see more information, like references to issues that relate to the problems that will need to be addressed (the blank issues (#20, ibmruntimes/openj9-openjdk-jdk22#68) are not sufficient).

Another issue (that I've pointed out previously) is that things should be organized (e.g. sorted) to help people find what they're looking for. To that end, tests should be grouped and sorted by name within each group; platform names should be sorted by name.

Copy link
Contributor Author

@JinhangZhang JinhangZhang Aug 19, 2024

Choose a reason for hiding this comment

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

  • Update the name.
  • It takes time to investigate and category those failures, but we do have a plan to check and address the failures one-by-one. Like TLS failures under javax/net/ssl folder. @jasonkatonica FYI.
  • Sorted tests and platform names by name.

Copy link
Member

Choose a reason for hiding this comment

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

The point of making reference to an issue in the problem list is to provide some background as to why the test was excluded. Please provide at least some of that background.

Copy link
Contributor

Choose a reason for hiding this comment

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

At this point in time we are only attempting to ignore all tests effected by FIPS 140-3 testing. A quick review of all the failing tests was done by various people on the development team to get a quick sense to the nature of the failure. At this time we don't plan on documenting and opening issues for each individual failure as this will take a long time and is currently holding up regression testing of various releases. This has been a similar strategy on other releases and platforms for some time until we are able to address necessary modifications to various tests on a package by package basis.

Copy link
Member

Choose a reason for hiding this comment

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

I wasn't asking for separate issues for each failure, but neither of the issues referenced above have any details at all. Surely we can do better than that. I'd be fine with this if those issues described even broad categories of reasons for excluding tests (e.g. algorithm not allowed in FIPS-140-3). I don't know what the exclusion reasons are and nobody else could figure it out based on the links provided.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi Keith we don't plan on scheduling this work at this time. Based upon previous experiences categorizing FIPS 140-2 tests a few years ago took this team a few months to do a good job with. At this point in time we only plan on ignoring these failures without a deep dive into root cause, categorizations, and proposing long term fixes to either the tests or the FIPS support itself. The main goal at this point is to :

  1. Make it easier for the team to triage test cases and problems that occur due to changes going forward. We can start with this while additional work is being done to fix tests and find root causes of each test failure.
  2. We need to make additional updates on top of these updates for windows such that we can run the FIPS tests for 140-3 on that platform which is currently not being done.

I agree that we certainly can do better, and we plan to do so iteratively over time with various components. Work is underway for such a deep dive already on the TLS tests.

Copy link
Member

Choose a reason for hiding this comment

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

I added a summary to the PR description.

Copy link
Member

Choose a reason for hiding this comment

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

Added the same to #20

@JinhangZhang JinhangZhang changed the title Re-enable FIPS140-3 failures Append more FIPS140-3 failures Aug 14, 2024
@JinhangZhang JinhangZhang changed the title Append more FIPS140-3 failures Add more FIPS140-3 failures Aug 14, 2024
@JasonFengJ9
Copy link
Member

@jasonkatonica please review.

@jasonkatonica
Copy link
Contributor

This looks good to me. Ignoring further tests on Java 23 is expected.

@pshipton
Copy link
Member

@keithc-ca are you good with this now?

@@ -428,11 +428,13 @@ sun/security/krb5/runNameEquals.sh https://github.com/ibmruntimes/openj9-openjdk
sun/security/krb5/auto/Cleaners.java https://github.com/ibmruntimes/openj9-openjdk-jdk23/issues/20 linux-x64,linux-ppc64le,linux-s390x
java/lang/ProcessBuilder/JspawnhelperWarnings.java https://github.com/ibmruntimes/openj9-openjdk-jdk23/issues/20 linux-x64,linux-ppc64le,linux-s390x,aix-all
java/lang/String/CompactString/NegativeSize.java https://github.com/ibmruntimes/openj9-openjdk-jdk23/issues/20 linux-ppc64le,linux-s390x,aix-all
java/security/Security/ConfigFileTest.java https://github.com/ibmruntimes/openj9-openjdk-jdk23/issues/20 linux-ppc64le,linux-s390x
Copy link
Member

Choose a reason for hiding this comment

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

The commit message could still be better. This doesn't just "add" exclusions it also re-enables ConfigFileTest. Perhaps it should say something like "Update FIPS140-3 exclusions" (at a minimum). I would like to see more information, like references to issues that relate to the problems that will need to be addressed (the blank issues (#20, ibmruntimes/openj9-openjdk-jdk22#68) are not sufficient).

Another issue (that I've pointed out previously) is that things should be organized (e.g. sorted) to help people find what they're looking for. To that end, tests should be grouped and sorted by name within each group; platform names should be sorted by name.

@JinhangZhang JinhangZhang changed the title Add more FIPS140-3 failures Update FIPS140-3 exclusions Aug 19, 2024
@JinhangZhang
Copy link
Contributor Author

JinhangZhang commented Aug 19, 2024

  • Update the name.
  • It takes time to investigate and category those failures, but we do have a plan to check and address the failures one-by-one. Like TLS failures under javax/net/ssl folder. @jasonkatonica FYI.
  • Sorted tests and platform names by name.

@llxia
Copy link

llxia commented Aug 20, 2024

We're pending the JDK23 M1 FIPS testing on this. Could we create an issue to track the enhancements and address them in separate PRs later?

@keithc-ca
Copy link
Member

We already have (at least two) issues. The problem is that they don't provide any information about why these tests are excluded. Creating another issue to see that the existing issues are filled in doesn't seem helpful.

In the interest of moving things along, I'm prepared to merge this (after the requested changes are made) with the understanding that future pull requests will have documented motivations. Please signal your agreement: @JinhangZhang and @jasonkatonica.

@pshipton
Copy link
Member

I added a summary of the motivation to this PR description and #20

Signed-off-by: Jinhang Zhang <Jinhang.Zhang@ibm.com>
@keithc-ca
Copy link
Member

keithc-ca commented Aug 20, 2024

I added a summary of the motivation to this PR description and #20

Thank you, but that is something that should be expected of the contributor.

@keithc-ca keithc-ca merged commit f97359b into ibmruntimes:openj9 Aug 20, 2024
2 checks passed
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.

6 participants