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

Ensure proper test coverage for SSL/TLS implementation #1963

Closed
aahlenst opened this issue Sep 15, 2020 · 14 comments · Fixed by #2049
Closed

Ensure proper test coverage for SSL/TLS implementation #1963

aahlenst opened this issue Sep 15, 2020 · 14 comments · Fixed by #2049
Assignees
Milestone

Comments

@aahlenst
Copy link
Contributor

We almost shipped a JDK 15 with a broken SSL/TLS implementation on all platforms (bug report). This was caused by adoptium/temurin-build#2033. I cannot find failing tests that are related to that problem in TRSS. But that may be due to my inexperience with TRSS.

We missed similar problems in the past but on a much smaller scale: adoptium/adoptium-support#33. So it seems there's something amiss with out testing coverage.

Example of a broken nightly build: https://ci.adoptopenjdk.net/job/build-scripts/job/jobs/job/jdk11u/job/jdk11u-linux-x64-hotspot/716/

@smlambert smlambert added this to the September 2020 milestone Sep 15, 2020
@smlambert
Copy link
Contributor

extended.jdk target was running weekly but has not been enabled in nightlies (due to lack of triage resources) - it would have some of the tests that would catch this issue as it includes jdk_security1, jdk_security2, jdk_security3, jdk_security4, jdk_security_infra make targets.

see #1523 (less fun tasks for 2020 include triaging all extended.openjdk tests) and #1522 (proposed additions to AQA v2)

@smlambert
Copy link
Contributor

smlambert commented Sep 15, 2020

Marked this as top priority, essentially re-enable extended.openjdk target and include it in the release pipeline. If we do not have the resources to run it nightly, it can be run weekly but must be included in the release runs.

adoptium/temurin-build#2024 sets up the ability to define tests as nightly and weekly from the build pipelines, and notes that a release build should include both sets of tests.

@aahlenst
Copy link
Contributor Author

aahlenst commented Oct 3, 2020

Ongoing work: master...aahlenst:security-tests

@smlambert smlambert modified the milestones: October 2020, November 2020 Nov 4, 2020
@smlambert
Copy link
Contributor

@aahlenst - see master...smlambert:secTests which takes the testcases from your branch, adds build/playlist/testng files so that the testcases will be run as part of our suite and produce junit style output. New test target called SecurityTests is currently added into extended.functional.

Some sample Grinders:
jdk8 hotspot xlinux: Grinder/4599
jdk11 hotspot xlinux: Grinder/4600
jdk15 openj9 xlinux: Grinder/4601
jdk8 hotspot aarch64: Grinder/4602

@aahlenst
Copy link
Contributor Author

Awesome, thanks.

Is there a good way for "automatic" version selection, for example "Run only on 11 and only if it's 11.0.9 or newer"?

@smlambert
Copy link
Contributor

smlambert commented Nov 10, 2020

If its at a test target level (all test cases in a group that is defined in a playlist), then a mechanism is the <subset> tag:

<test>
		<testCaseName>SecurityTests</testCaseName>
		<command>$(JAVA_COMMAND) $(JVM_OPTIONS) -cp $(Q)$(RESOURCES_DIR)$(P)$(TESTNG)$(P)$(TEST_RESROOT)$(D)SecurityTests.jar$(Q) org.testng.TestNG $(Q)$(TEST_RESROOT)$(D)testng.xml$(Q) -d $(REPORTDIR) -testnames SecurityTests -groups $(TEST_GROUP) -excludegroups $(DEFAULT_EXCLUDE); \
		$(TEST_STATUS)</command>
		<levels>
			<level>extended</level>
		</levels>
		<groups>
			<group>functional</group>
		</groups>
		<subsets>
			<subset>11</subset>
		</subsets>
	</test>

The subset tag is currently only at the granularity of major version (and can support special branches like Valhalla/Panama). We do not currently have a mechanism to say 'only 11 and not less than 11.0.9', though may be useful to move that logic into a shared location for use by all tests. We had not yet encountered a need to match on update versions.

For checking inside of functional test code, we lay down the functional test material from openj9, so the VersionCheck class is available, but again, not serving update version info presently.

@aahlenst
Copy link
Contributor Author

<subsets>
    <subset>8</subset>
    <subset>11</subset>
</subset>

Is that legal syntax to express to run on 8 and 11 only? Is there some syntax to say 11 or newer?

... though may be useful to move that logic into a shared location for use by all tests. We had not yet encountered a need to match on update versions.

Do you think about a Java class or something to extend the XML declaration? We have plenty of use cases: JFR is only available on 8u262 or newer, TLS 1.3 only on 8u272 and newer, Shenandoah only on 11.0.9 and newer.

@smlambert
Copy link
Contributor

re: #1963 (comment) that is correct syntax if you only want to run against 8 and 11.

<subsets>
    <subset>11+</subset>
</subset>

for version 11 and up (so since we are currently running jdk15 tests, then in practical terms, would show up in jdk11 and jdk15 pipelines... though if someone tried to run a jdk11-jdk16 pipelines, tests would be run in that range).

We were discussing an update to how we represent subsets / versions in playlist xml files, tagging @renfeiw who would have good insight into evolving what we can do there (and also because he is currently extending the playlist xml files to handle more granular exclusion capabilities).

@renfeiw
Copy link
Contributor

renfeiw commented Nov 10, 2020

re #1963 (comment) We will update the tag "subset" to "version" later on to be more explicit. We are also thinking of using Interval notation such as [11, 11], [11.0.9, infinite) to represent java version more precisely.

@smlambert
Copy link
Contributor

thanks @renfeiw

@aahlenst - we can do this in stages, get your testing in place using PRE_223_PATTERN temporarily (with the plan to move the handling of more precise java version info to TKG in a second stage, need to create an issue so its not lost/forgotten)... or what is your preference?

@aahlenst
Copy link
Contributor Author

I'll start with what we have and continue from there. Or even easier: We can just merge your branch Shelley if you convert it into a PR. Beforehand, it might make sense to run a grinder with a nightly from Sep. 13 or 14 to check that the tests fail.

@smlambert
Copy link
Contributor

jdk11 hotspot Grinder (on Sept 13th nightly) indeed shows the tests fail / catch the issue

Screen Shot 2020-11-10 at 8 00 04 PM

@aahlenst
Copy link
Contributor Author

Hooray! So after merging the PR, the last step is to enable nightly runs of those tests.

@smlambert
Copy link
Contributor

smlambert commented Nov 11, 2020

Because they are added into extended.functional, they will automatically be run nightly, since I recently added that top-level target to the nightly test list.

aahlenst added a commit that referenced this issue Nov 12, 2020
Exercises the TLS/SSL stack by calling websites to ensure that TLS/SSL work and a functional trust store is present.

Fixes #1963.

Signed-off-by: Shelley Lambert <slambert@gmail.com>
Co-authored-by: Andreas Ahlenstorf <andreas@ahlenstorf.ch>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants