Skip to content

Convert built-in hosts to unambiguous domain names #1104

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

Merged
merged 2 commits into from
Oct 17, 2022

Conversation

Nino-K
Copy link
Contributor

@Nino-K Nino-K commented Oct 14, 2022

This change ensures that user-defined host names are converted correctly to unambiguous domain names. Regardless if the user declares them as fully qualified or not we always convert them to fully qualified.

Addresses: rancher-sandbox/rancher-desktop#3163

Signed-off-by: Nino Kodabande nkodabande@suse.com

Copy link
Member

@jandubois jandubois left a comment

Choose a reason for hiding this comment

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

Needs some changes we discussed in person

@Nino-K Nino-K force-pushed the fix-unambiguous-host-to-ip branch 3 times, most recently from 94bb788 to c47eb28 Compare October 14, 2022 21:46
Signed-off-by: Nino Kodabande <nkodabande@suse.com>
@Nino-K Nino-K force-pushed the fix-unambiguous-host-to-ip branch from c47eb28 to 7c79a59 Compare October 14, 2022 22:04
Copy link
Member

@jandubois jandubois left a comment

Choose a reason for hiding this comment

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

Use dns.CanonicalName instead of re-implementing it.

@@ -132,10 +131,11 @@ func NewHandler(opts HandlerOptions) (dns.Handler, error) {
hostToIP: make(map[string]net.IP),
}
for host, address := range opts.StaticHosts {
fqdn := cname(host)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
fqdn := cname(host)
cname := dns.CanonicalName(host)

if ip := net.ParseIP(address); ip != nil {
h.hostToIP[host] = ip
h.hostToIP[fqdn] = ip
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
h.hostToIP[fqdn] = ip
h.hostToIP[cname] = ip

} else {
h.cnameToHost[host] = limayaml.Cname(address)
h.cnameToHost[fqdn] = cname(address)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
h.cnameToHost[fqdn] = cname(address)
h.cnameToHost[cname] = dns.CanonicalName(address)

Comment on lines 403 to 410

func cname(host string) string {
host = strings.ToLower(host)
if !strings.HasSuffix(host, ".") {
host += "."
}
return host
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func cname(host string) string {
host = strings.ToLower(host)
if !strings.HasSuffix(host, ".") {
host += "."
}
return host
}

@Nino-K Nino-K force-pushed the fix-unambiguous-host-to-ip branch from 7c79a59 to d34b86e Compare October 14, 2022 22:31
@Nino-K Nino-K requested a review from jandubois October 14, 2022 22:31
Copy link
Member

@jandubois jandubois left a comment

Choose a reason for hiding this comment

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

Please leave the tests in pkg/limayaml/defaults_tests.go. The names are no longer normalized to canonical names, but we still want to test the merging behaviour. So just adjust the expected values to match the original values without any modifications.

- Adds test cases for A records and Cnames
- Move cname conversion out of yaml into dns package

Signed-off-by: Nino Kodabande <nkodabande@suse.com>
@Nino-K Nino-K force-pushed the fix-unambiguous-host-to-ip branch from d34b86e to e78d91d Compare October 17, 2022 19:50
@Nino-K Nino-K requested a review from jandubois October 17, 2022 19:51
Copy link
Member

@jandubois jandubois left a comment

Choose a reason for hiding this comment

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

Thank you!

@jandubois jandubois merged commit aeb39e7 into lima-vm:master Oct 17, 2022
@AkihiroSuda AkihiroSuda added this to the v0.12.1 milestone Oct 17, 2022
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.

3 participants