[JENKINS-73139] Converting tests to `com.sun.net.httpserver` #54

nevingeorgesunny · 2024-08-13T08:06:56Z

Overview

This PR refactors the OpenShiftBearerTokenCredentialTest.java test class by updating the approach to API mocking by using an in-memory server instead of jetty server.

Testing done

Running locally and in the pr builder against 2.472-rc35224.f615612a_1850 as 2.472 has not yet been released
This is expected to fail in PR builder until 2.472 is released

Key Changes

Switch from Jetty Server to In-Memory HTTP Server:

Previous Implementation: The test class was using a Jetty server to mock API responses.
New Implementation: Replaced Jetty server with com.sun.net.httpserver for in-memory HTTP server-based mocking. This change simplifies the test setup and improves performance by using an in-memory server that runs within the test process.
in AbstractOpenShiftBearerTokenCredentialFIPSTest a HttpsServer or a HttpServer is created based on the scheme

Closes #44
Closes #56

to be used in OpenShiftBearerTokenCredentialTest

and removed all the usaged of jetty server for doing moocks

…to using-in-memory-mock-httpserver

…ry mock server - setting <maven.compiler.release>17</maven.compiler.release> <jenkins-test-harness.version>2250.v03a_1295b_0a_30</jenkins-test-harness.version> <jenkins.version>2.472</jenkins.version>

pom.xml

    <bom.artifactId>bom-2.426.x</bom.artifactId>
    <bom.version>3208.vb_21177d4b_cd9</bom.version>


...a/org/jenkinsci/plugins/kubernetes/credentials/OpenShiftBearerTokenCredentialMockServer.java

MarkEWaite

This looks great to me. I'm running out of time today, so can't do more review, but wanted to share the review comments that I could.

pom.xml

    <bom.artifactId>bom-2.426.x</bom.artifactId>
    <bom.version>3208.vb_21177d4b_cd9</bom.version>


pom.xml

basil · 2024-08-14T14:24:22Z

...st/java/org/jenkinsci/plugins/kubernetes/credentials/OpenShiftBearerTokenCredentialTest.java

        server.start();
+        mockserverBaseUrl = "http:/"+server.getAddress()+"/";


I confirmed that it generates a valid URL. Question answered. The code is correct.

Yes, the code is correct, but it is illegible.

… package seem to use 4 space indentation. fixing this

raul-arabaolaza · 2024-08-14T08:57:19Z

...jenkinsci/plugins/kubernetes/credentials/AbstractOpenShiftBearerTokenCredentialFIPSTest.java

-        server.setConnectors(new Connector[]{serverConnector, sslConnector});
+    private void setupHttps(HttpsServer httpsServer) throws Exception {
+        SSLContext sslContext = SSLContext.getInstance("TLS");
+        KeyManagerFactory kmf = KeyManagerFactory.getInstance("SunX509");


This changes the original behaviour in some cases, see https://android.googlesource.com/platform/external/jetty/+/refs/heads/marshmallow-dev/src/java/org/eclipse/jetty/util/ssl/SslContextFactory.java#95 in some fips environments that property may be used.

And this can be applied to other parts, for example you are forcing a "tls" for SslContext but the original code may not be doing that or allow the algorithm to be overriden by a system property... This kind of things are important as we may be removing the ability for a fips env to override things.

Not a relevant comment I think. The purpose of the test is to ensure that TLS is in use and that TLS verification is not disabled. There are no changes to production code, so the only question is whether the effectiveness of the test is reduced. The test previously didn't require a FIPS compliant machine but verified FIPS compliance as described above, and the same conditions are true after this refactoring, so there is no issue here.

alecharp · 2024-08-14T09:07:47Z

...a/org/jenkinsci/plugins/kubernetes/credentials/OpenShiftBearerTokenCredentialMockServer.java

+/**
+ * Author: Nevin Sunny
+ * Date: 13/08/24
+ * Time: 11:24 am
+ */


Not really necessary.

alecharp · 2024-08-14T09:09:46Z

...a/org/jenkinsci/plugins/kubernetes/credentials/OpenShiftBearerTokenCredentialMockServer.java

+			String responseToClient = "{\n" + "  \"issuer\": \""
+			                          + scheme + "://" + hostname + ":" + port + "/\",\n" + "  \"authorization_endpoint\": \""
+			                          + scheme + "://" + hostname + ":" + port + matcher.group(1) + "/oauth/authorize\",\n"
+			                          + "  \"token_endpoint\": \""
+			                          + scheme + "://" + hostname + ":" + port + "/oauth/token\",\n" + "  \"scopes_supported\": [\n"
+			                          + "    \"user:check-access\",\n"
+			                          + "    \"user:full\",\n"
+			                          + "    \"user:info\",\n"
+			                          + "    \"user:list-projects\",\n"
+			                          + "    \"user:list-scoped-projects\"\n"
+			                          + "  ],\n"
+			                          + "  \"response_types_supported\": [\n"
+			                          + "    \"code\",\n"
+			                          + "    \"token\"\n"
+			                          + "  ],\n"
+			                          + "  \"grant_types_supported\": [\n"
+			                          + "    \"authorization_code\",\n"
+			                          + "    \"implicit\"\n"
+			                          + "  ],\n"
+			                          + "  \"code_challenge_methods_supported\": [\n"
+			                          + "    \"plain\",\n"
+			                          + "    \"S256\"\n"
+			                          + "  ]\n"
+			                          + "}";


Could be great to update the plugin to Java 17 so you could use String Blocks here.
That would require to update jenkins.version and build configuration but might worth the change imo. What do you think @MarkEWaite ?
Maybe for later ?

When I tried to mix general purpose Java 17 upgrades with Spring Security 6.x Upgrade work it did not end well for me. I tried to do that mixing with the git client plugin experiment with JGit 7.0.0 that requires Java 17. I was stacking two patches (one for JGit 7.0.0 and requiring Jenkins 2.463 minimum version and the other requiring the Jelly 12 EE 9 pre-release from the tip of the jakarta branch) and the addition of the Java 17 changes made me discard both of those changes and start with fresh branches.

I think that in general we should avoid making general Java 17 changes so that we don't delay these pull requests that are time critical.

In this case, the plugin does not need to require Java 17, so the Java 17 changes would actually break the plugin.

Totally fine by me. This can be a "cleanup" task later on when we know everything is ok with the plugin.

Co-authored-by: Mark Waite <mark.earl.waite@gmail.com>

The comment is unnecessary because it restates the code without providing any additional reasoning.

…k-httpserver

This test was previously listening on all interfaces before this change. Keep the same behavior after this change to avoid unrelated code change during a routine refactoring.

nevingeorgesunny and others added 5 commits August 13, 2024 11:44

create a mockserver with com.sun.net.httpserver

f6a24cd

to be used in OpenShiftBearerTokenCredentialTest

using the mockserver in OpenShiftBearerTokenCredentialTest

9d0e47a

and removed all the usaged of jetty server for doing moocks

Merge branch 'jenkinsci:master' into using-in-memory-mock-httpserver

d685500

removed JenkinsRule by mistake putting it back

9333af7

Merge remote-tracking branch 'dev/using-in-memory-mock-httpserver' in…

a016ef7

…to using-in-memory-mock-httpserver

nevingeorgesunny requested a review from a team as a code owner August 13, 2024 08:06

nevingeorgesunny marked this pull request as draft August 13, 2024 08:07

- AbstractOpenShiftBearerTokenCredentialFIPSTest is now using in-memo…

6daded2

…ry mock server - setting <maven.compiler.release>17</maven.compiler.release> <jenkins-test-harness.version>2250.v03a_1295b_0a_30</jenkins-test-harness.version> <jenkins.version>2.472</jenkins.version>

nevingeorgesunny changed the title ~~Using in-memory mock httpserver instead of Jetty server for OpenShiftBearerTokenCredentialTest~~ [JENKINS-73139] Adapt to Jetty 12 EE8 and using in-memory mock server instead of Jetty server Aug 13, 2024

MarkEWaite reviewed Aug 13, 2024

View reviewed changes

pom.xml Outdated

Comment on lines 46 to 47

<bom.artifactId>bom-2.426.x</bom.artifactId>

<bom.version>3208.vb_21177d4b_cd9</bom.version>

This comment was marked as outdated.

Sign in to view

This comment was marked as resolved.

Sign in to view

MarkEWaite reviewed Aug 13, 2024

View reviewed changes

...a/org/jenkinsci/plugins/kubernetes/credentials/OpenShiftBearerTokenCredentialMockServer.java Show resolved Hide resolved

MarkEWaite reviewed Aug 13, 2024

View reviewed changes

This file uses 8 space indentation when the rest of the files in this…

8e83a6a

… package seem to use 4 space indentation. fixing this

raul-arabaolaza reviewed Aug 14, 2024

View reviewed changes

alecharp reviewed Aug 14, 2024

View reviewed changes

nevingeorgesunny mentioned this pull request Aug 14, 2024

[JENKINS-73139] Adapt to Jetty 12 EE8 #56

Closed

basil and others added 9 commits August 14, 2024 07:05

Update pom.xml

87473a9

Co-authored-by: Mark Waite <mark.earl.waite@gmail.com>

Update pom.xml

18c9fbb

Co-authored-by: Mark Waite <mark.earl.waite@gmail.com>

sort imports

189fef9

Format code consistently with other repositories

1330786

Remove unnecessary throws and comment

f8df999

The comment is unnecessary because it restates the code without providing any additional reasoning.

Improve legibility

b92127d

Make tests consistent with each other

cc67e8b

Code cleanup

1fd801c

Merge remote-tracking branch 'origin/master' into using-in-memory-moc…

9567ac3

…k-httpserver

basil added the developer label Aug 14, 2024

basil changed the title ~~[JENKINS-73139] Adapt to Jetty 12 EE8 and using in-memory mock server instead of Jetty server~~ [JENKINS-73139] Converting tests to com.sun.net.httpserver Aug 14, 2024

basil marked this pull request as ready for review August 14, 2024 15:20

basil approved these changes Aug 14, 2024

View reviewed changes

De-duplicate

70851f4

basil added 3 commits August 14, 2024 08:25

Undo local testing changes

a4ee1f1

Reduce scope and size of code change

2c1f59b

No need to change test behavior

939395a

This test was previously listening on all interfaces before this change. Keep the same behavior after this change to avoid unrelated code change during a routine refactoring.

basil added the test label Aug 14, 2024

basil merged commit 90a488b into jenkinsci:master Aug 14, 2024
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-73139] Converting tests to `com.sun.net.httpserver` #54

[JENKINS-73139] Converting tests to `com.sun.net.httpserver` #54

nevingeorgesunny commented Aug 13, 2024 •

edited by basil

Loading

This comment was marked as outdated.

This comment was marked as resolved.

MarkEWaite left a comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

basil Aug 14, 2024

raul-arabaolaza Aug 14, 2024 •

edited

Loading

basil Aug 14, 2024 •

edited

Loading

alecharp Aug 14, 2024

alecharp Aug 14, 2024

MarkEWaite Aug 14, 2024

alecharp Aug 14, 2024

		<bom.artifactId>bom-2.426.x</bom.artifactId>
		<bom.version>3208.vb_21177d4b_cd9</bom.version>

		server.start();
		mockserverBaseUrl = "http:/"+server.getAddress()+"/";

[JENKINS-73139] Converting tests to com.sun.net.httpserver #54

[JENKINS-73139] Converting tests to com.sun.net.httpserver #54

Conversation

nevingeorgesunny commented Aug 13, 2024 • edited by basil Loading

Overview

Testing done

Key Changes

Switch from Jetty Server to In-Memory HTTP Server:

This comment was marked as outdated.

This comment was marked as resolved.

MarkEWaite left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

basil Aug 14, 2024

Choose a reason for hiding this comment

raul-arabaolaza Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

basil Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

alecharp Aug 14, 2024

Choose a reason for hiding this comment

alecharp Aug 14, 2024

Choose a reason for hiding this comment

MarkEWaite Aug 14, 2024

Choose a reason for hiding this comment

alecharp Aug 14, 2024

Choose a reason for hiding this comment

[JENKINS-73139] Converting tests to `com.sun.net.httpserver` #54

[JENKINS-73139] Converting tests to `com.sun.net.httpserver` #54

nevingeorgesunny commented Aug 13, 2024 •

edited by basil

Loading

raul-arabaolaza Aug 14, 2024 •

edited

Loading

basil Aug 14, 2024 •

edited

Loading