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

Add tests to the TLS certificate reload #38820

Merged
merged 1 commit into from
Feb 20, 2024

Conversation

cescoffier
Copy link
Member

This PR uses the new certificate generator utility.

Once in, we will be able to remove all the hand-crafted certificates.

@quarkus-bot quarkus-bot bot added area/dependencies Pull requests that update a dependency file area/documentation area/vertx labels Feb 16, 2024

This comment has been minimized.

Copy link

github-actions bot commented Feb 16, 2024

🙈 The PR is closed and the preview is expired.

This comment has been minimized.

@cescoffier
Copy link
Member Author

Ok... I broke everything :-). Will have a look Monday.

@sberyozkin
Copy link
Member

@cescoffier Sorry, also missed the review request, it looks promising in any case 👍

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

- for both the primary and management server
- also update the docs
Copy link

quarkus-bot bot commented Feb 19, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit fd46a61.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

⚠️ There are other check runs running, make sure you don't need to wait for their status before merging.

Copy link

quarkus-bot bot commented Feb 19, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit fd46a61.

Failing Jobs

Status Name Step Failures Logs Raw logs Build scan
Native Tests - Data5 Build Failures Logs Raw logs 🚧

Full information is available in the Build summary check run.

Failures

⚙️ Native Tests - Data5 #

- Failing: integration-tests/jpa-postgresql-withxml 

📦 integration-tests/jpa-postgresql-withxml

io.quarkus.it.jpa.postgresql.ImageMetricsITCase.verifyImageMetrics line 15 - History - More details - Source on GitHub

org.opentest4j.AssertionFailedError: Expected analysis_results.fields.reflection to be within range [163 +- 3%] but was 168 ==> expected: <true> but was: <false>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertTrue.failNotTrue(AssertTrue.java:63)
	at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:36)
	at org.junit.jupiter.api.Assertions.assertTrue(Assertions.java:214)
	at io.quarkus.test.junit.nativeimage.NativeBuildOutputExtension.assertValueWithinRange(NativeBuildOutputExtension.java:90)
	at io.quarkus.test.junit.nativeimage.NativeBuildOutputExtension.lambda$verifyImageMetrics$0(NativeBuildOutputExtension.java:66)

@cescoffier
Copy link
Member Author

@sberyozkin Fixed! Your review would be much appreciated.

The goal is to use this technique to avoid hand-crafted certificates. Here, it's slightly more complicated as I'm testing the hot reload, which means I need two sets of certificates: the initial certificate and the one triggering the reload.

In the other case, we should have only one.

Also, while adding that feature to the REST client, we discovered that the REST client did not support PEMs and P12 (getting fixed right now), and the trust store format needs to be completed. It is the case with gRPC, too. Because creating certificates was painful, I can understand why it was not adequately tested. So, a consequence would be the addition of more certificate formats in various extensions.

@cescoffier cescoffier requested a review from geoand February 19, 2024 14:54
Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

Nice!

@cescoffier cescoffier merged commit d972a37 into quarkusio:main Feb 20, 2024
52 of 53 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.9 - main milestone Feb 20, 2024
@cescoffier cescoffier deleted the cert-reloading-test branch February 20, 2024 09:58
Comment on lines +3596 to +3600
<dependency>
<groupId>me.escoffier.certs</groupId>
<artifactId>certificate-generator-junit5</artifactId>
<version>0.3.0</version>
</dependency>
Copy link
Member

Choose a reason for hiding this comment

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

@cescoffier I don't think this should be in the BOM. I would move it to the build-parent.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I will do that in my next PR about kafka TLS support (which as I needed to fix something in the junit5 extension)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dependencies Pull requests that update a dependency file area/documentation area/vertx
Projects
Development

Successfully merging this pull request may close these issues.

5 participants