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

Resolve trusted proxy host names to all available A/AAAA records #43188

Merged
merged 2 commits into from
Feb 18, 2025

Conversation

ahus1
Copy link
Contributor

@ahus1 ahus1 commented Sep 10, 2024

Closes #42782

During my tests, I found that CNAMEs are resolved implicitly already, so no manual extra where necessary to handle those.

I might need some help for additional tests, as I don't know how to best mock a DNS.

Copy link
Member

@cescoffier cescoffier left a comment

Choose a reason for hiding this comment

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

LGTM, but yes, a bit more testing would be great.

Unfortunately, testing DNS is hard. @vietj how do you do that in Vert.x?

@ahus1 Look at https://github.com/eclipse-vertx/vert.x/blob/master/vertx-core/src/test/java/io/vertx/test/fakedns/FakeDNSServer.java.

@vietj
Copy link

vietj commented Sep 13, 2024

You can look at the FakeDNSServer in vertx core tests (or reuse it) to do proper testing.

@michalvavrik
Copy link
Member

Changes LGTM. +1 for test, I remember I didn't know how to make one.

@geoand
Copy link
Contributor

geoand commented Sep 24, 2024

@ahus1 do you plan to add a test like @cescoffier mentions?

@ahus1
Copy link
Contributor Author

ahus1 commented Sep 24, 2024

I had a look at FakeDNSServer. In commit 61f8329 I copied it over as I didn't find it available in a dependency, and used it in a first test which is green. A future commit might clear the bits that are not needed. As this is more like a PoC for a DNS based test, I kept it for now.

It still tests only part of the code, as it is not testing IPv6 as the caller is using IPv4 all the time. Also event.remoteAddress()).ipAddress() is always available, so no resolving of the remote address is necessary. It seems I need some help here to also test those code paths.

@cescoffier
Copy link
Member

I think it looks OK. Just a bit of polish, but it can be included.

@@ -12,6 +12,10 @@
<artifactId>quarkus-vertx-http-deployment</artifactId>
<name>Quarkus - Vert.x - HTTP - Deployment</name>

<properties>
<apacheds-protocol-dns.version>2.0.0-M23</apacheds-protocol-dns.version>
Copy link
Member

Choose a reason for hiding this comment

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

Should be declared in the BOM or in the parent.

@cescoffier
Copy link
Member

I think it is good to go.

@geoand
Copy link
Contributor

geoand commented Feb 18, 2025

Can we have this rebased and taken out of draft?

@ahus1 ahus1 force-pushed the is-42782-fix-dns-lookup-trusted-proxies branch from 61f8329 to 5e2a78e Compare February 18, 2025 10:33
@ahus1 ahus1 marked this pull request as ready for review February 18, 2025 10:33
@geoand geoand requested a review from cescoffier February 18, 2025 10:36

This comment has been minimized.

This comment has been minimized.

Copy link

github-actions bot commented Feb 18, 2025

🎊 PR Preview f3e1c28 has been successfully built and deployed to https://quarkus-pr-main-43188-preview.surge.sh/version/main/guides/

  • Images of blog posts older than 3 months are not available.
  • Newsletters older than 3 months are not available.

@ahus1 ahus1 force-pushed the is-42782-fix-dns-lookup-trusted-proxies branch from 5e2a78e to 36af0c1 Compare February 18, 2025 13:39
@ahus1
Copy link
Contributor Author

ahus1 commented Feb 18, 2025

Commit updated to make the import sort check pass

Copy link

quarkus-bot bot commented Feb 18, 2025

Status for workflow Quarkus Documentation CI

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

✅ 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.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

@geoand geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Feb 18, 2025
Copy link

quarkus-bot bot commented Feb 18, 2025

Status for workflow Quarkus CI

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

✅ 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.

You can consult the Develocity build scans.


Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 extensions/smallrye-reactive-messaging/deployment

io.quarkus.smallrye.reactivemessaging.hotreload.ConnectorChangeTest.testUpdatingConnector - History

  • Expecting actual: ["-4","-5","-6","-7","-8","-9","-10","-11"] to start with: ["-3", "-4", "-5", "-6"] - java.lang.AssertionError
java.lang.AssertionError: 

Expecting actual:
  ["-4","-5","-6","-7","-8","-9","-10","-11"]
to start with:
  ["-3", "-4", "-5", "-6"]

	at io.quarkus.smallrye.reactivemessaging.hotreload.ConnectorChangeTest.testUpdatingConnector(ConnectorChangeTest.java:36)

@geoand geoand merged commit 0af5881 into quarkusio:main Feb 18, 2025
57 checks passed
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Feb 18, 2025
@quarkus-bot quarkus-bot bot added this to the 3.21 - main milestone Feb 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

Lookup of trusted proxies by hostname broken due to DNS issues in Vert.x
5 participants