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

Optionally disable TLS in RCS tests #94938

Merged
merged 7 commits into from
Apr 4, 2023

Conversation

n1v0lg
Copy link
Contributor

@n1v0lg n1v0lg commented Mar 31, 2023

More test coverage.

@n1v0lg n1v0lg added >test Issues or PRs that are addressing/adding tests :Security/TLS SSL/TLS, Certificates labels Mar 31, 2023
@n1v0lg n1v0lg self-assigned this Mar 31, 2023
* This is needed because `randomBoolean` is not available inside the outer `static {}` scope,
* where the initial cluster settings are set
*/
private static String atomicallyRandomizeSslEnabled(AtomicReference<Boolean> sslEnabledRef) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm passing the atomic ref explicitly since it reads a little clearer to me this way where the "atomically" comes from. No strong preference though, can also inline.

@@ -84,6 +85,17 @@ public class RemoteClusterSecurityRestIT extends AbstractRemoteClusterSecurityTe
.build();
}

/**
* This is needed because `randomBoolean` is not available inside the outer `static {}` scope,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A quirk here: using randomBoolean() in the static {} block results in a classMethod exception:

No context information for thread: Thread[id=32, name=SUITE-RemoteClusterSecurityRestIT-seed#[85BC9D463CA78D4], state=RUNNABLE, group=TGRP-RemoteClusterSecurityRestIT]. Is this thread running under a class com.carrotsearch.randomizedtesting.RandomizedRunner runner context? Add @RunWith(class com.carrotsearch.randomizedtesting.RandomizedRunner.class) to your test class. Make sure your code accesses random contexts within @BeforeClass and @AfterClass boundary (for example, static test class initializers are not permitted to access random contexts).
java.lang.IllegalStateException: No context information for thread: Thread[id=32, name=SUITE-RemoteClusterSecurityRestIT-seed#[85BC9D463CA78D4], state=RUNNABLE, group=TGRP-RemoteClusterSecurityRestIT]. Is this thread running under a class com.carrotsearch.randomizedtesting.RandomizedRunner runner context? Add @RunWith(class com.carrotsearch.randomizedtesting.RandomizedRunner.class) to your test class. Make sure your code accesses random contexts within @BeforeClass and @AfterClass boundary (for example, static test class initializers are not permitted to access random contexts).
	at com.carrotsearch.randomizedtesting.RandomizedContext.context(RandomizedContext.java:249)
	at com.carrotsearch.randomizedtesting.RandomizedContext.current(RandomizedContext.java:134)
	at org.apache.lucene.tests.util.LuceneTestCase.random(LuceneTestCase.java:792)
	at org.elasticsearch.test.ESTestCase.randomBoolean(ESTestCase.java:775)
	at org.elasticsearch.xpack.remotecluster.RemoteClusterSecurityRestIT.<clinit>(RemoteClusterSecurityRestIT.java:49)
	at java.base/java.lang.Class.forName0(Native Method)
	at java.base/java.lang.Class.forName(Class.java:496)
	at java.base/java.lang.Class.forName(Class.java:475)
	at com.carrotsearch.randomizedtesting.RandomizedRunner$2.run(RandomizedRunner.java:631)

"for example, static test class initializers are not permitted to access random contexts" explains this.

We could probably come up with a cleaner workaround but that requires some work. I think my hack is sufficient for now.

@n1v0lg n1v0lg marked this pull request as ready for review March 31, 2023 10:00
@n1v0lg n1v0lg requested a review from ywangd March 31, 2023 10:00
@elasticsearchmachine elasticsearchmachine added the Team:Security Meta label for security team label Mar 31, 2023
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-security (Team:Security)

Copy link
Member

@ywangd ywangd left a comment

Choose a reason for hiding this comment

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

LGTM

@n1v0lg
Copy link
Contributor Author

n1v0lg commented Apr 4, 2023

@elasticmachine update branch

@n1v0lg n1v0lg added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Apr 4, 2023
@n1v0lg
Copy link
Contributor Author

n1v0lg commented Apr 4, 2023

@elasticmachine update branch

@elasticsearchmachine elasticsearchmachine merged commit 295c6f0 into elastic:main Apr 4, 2023
@n1v0lg n1v0lg deleted the rcs-without-tls branch April 4, 2023 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Security/TLS SSL/TLS, Certificates Team:Security Meta label for security team >test Issues or PRs that are addressing/adding tests v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants