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

fix(dns) allow resolution of service names with "." #1448

Merged
merged 1 commit into from
Jan 20, 2021

Conversation

lahabana
Copy link
Contributor

When specifying a kuma.io/service with a "." DNS resolution would not work
because it would only look at the first part of the DNS name.
This would also cause foo.bar.mesh and foo.mesh to resolve to the same thing
which may give a false sense of isolation of meshes in the DNS
We now only strips the last part of the DNS name if it matches the .mesh domain suffix.

@lahabana lahabana requested a review from a team as a code owner January 20, 2021 09:20
@lahabana
Copy link
Contributor Author

This patch makes DNS resolution more restrictive so it's potentially not backward compatible depending on your treatment of undefined behavior as breaking changes.
Let me know if you'd prefer something backward compatible (I'm thinking of a flag in DNSServiceConfig).

Copy link
Contributor

@nickolaev nickolaev left a comment

Choose a reason for hiding this comment

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

Please fix the tests as suggested. Looks great otherwise.

// given
dnsResolver.SetVIPs(vips.List{
"my.service": "240.0.0.1",
})
Copy link
Contributor

Choose a reason for hiding this comment

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

If you do this in the It let's remove it from BeforeEach and put it in each It.

When specifying a `kuma.io/service` with a "." DNS resolution would not work
because it would only look at the first part of the DNS name.
This would also cause `foo.bar.mesh` and `foo.mesh` to resolve to the same thing
which may give a false sense of isolation of meshes in the DNS
We now only strips the last part of the DNS name if it matches the `.mesh` domain suffix.

Signed-off-by: Charly Molter <charly@koyeb.com>
@lahabana
Copy link
Contributor Author

@nickolaev updated thanks!

@lahabana
Copy link
Contributor Author

@nickolaev seems like the build failure is unrelated can you retrigger the build?

@nickolaev nickolaev merged commit de170f1 into kumahq:master Jan 20, 2021
mergify bot pushed a commit that referenced this pull request Jan 20, 2021
When specifying a `kuma.io/service` with a "." DNS resolution would not work
because it would only look at the first part of the DNS name.
This would also cause `foo.bar.mesh` and `foo.mesh` to resolve to the same thing
which may give a false sense of isolation of meshes in the DNS
We now only strips the last part of the DNS name if it matches the `.mesh` domain suffix.

Signed-off-by: Charly Molter <charly@koyeb.com>
(cherry picked from commit de170f1)
nickolaev pushed a commit that referenced this pull request Jan 20, 2021
When specifying a `kuma.io/service` with a "." DNS resolution would not work
because it would only look at the first part of the DNS name.
This would also cause `foo.bar.mesh` and `foo.mesh` to resolve to the same thing
which may give a false sense of isolation of meshes in the DNS
We now only strips the last part of the DNS name if it matches the `.mesh` domain suffix.

Signed-off-by: Charly Molter <charly@koyeb.com>
(cherry picked from commit de170f1)

Co-authored-by: Charly Molter <charly@koyeb.com>
@lahabana lahabana deleted the fix/dns-with-dot branch March 29, 2024 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants