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

Switch to miekg/dns #3096

Merged
merged 2 commits into from
Mar 29, 2023
Merged

Conversation

christoph-zededa
Copy link
Contributor

@christoph-zededa christoph-zededa commented Mar 13, 2023

  1. prepare by splitting controllerDNSCache into pieces to make it more readable

  2. switch from go-dns-resolver to miekg/dns

    this allows us to set the source interface when doing
    DNS requests

    go-dns-resolver itself is using miekg/dns under the hood,
    so now nim is using it directly instead

@christoph-zededa christoph-zededa changed the title Switch to miekg/dns [WIP] Switch to miekg/dns Mar 13, 2023
@christoph-zededa christoph-zededa force-pushed the use_miekg_dns_newgo branch 5 times, most recently from 98e9f39 to 66d86d2 Compare March 17, 2023 14:38
@christoph-zededa christoph-zededa marked this pull request as ready for review March 17, 2023 14:38
@christoph-zededa christoph-zededa changed the title [WIP] Switch to miekg/dns Switch to miekg/dns Mar 17, 2023
Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

It makes reviewing the PRs easier if you have

  1. A useful PR description
  2. Commits with a useful description (which you have)

One way to do this is to create 1 using the concatenation (+/- edits) of 2.

Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

Run eden again

pkg/pillar/cmd/nim/controllerdns.go Outdated Show resolved Hide resolved
pkg/pillar/cmd/nim/controllerdns_test.go Outdated Show resolved Hide resolved
pkg/pillar/cmd/nim/controllerdns.go Outdated Show resolved Hide resolved
pkg/pillar/cmd/nim/controllerdns.go Outdated Show resolved Hide resolved
pkg/pillar/cmd/nim/controllerdns.go Outdated Show resolved Hide resolved
pkg/pillar/cmd/nim/controllerdns_test.go Outdated Show resolved Hide resolved
pkg/pillar/dpcmanager/dpcmanager.go Outdated Show resolved Hide resolved
@christoph-zededa christoph-zededa force-pushed the use_miekg_dns_newgo branch 2 times, most recently from bae9362 to 38bfe8e Compare March 23, 2023 19:26
pkg/pillar/devicenetwork/dns.go Outdated Show resolved Hide resolved
}

// ResolveWithPortsLambda resolves a domain by using source IPs and dns servers from DeviceNetworkStatus as a resolver func ResolveWithSrcIP can be used
func ResolveWithPortsLambda(domain string, dns types.DeviceNetworkStatus, resolve func(string, net.IP, net.IP) ([]DNSResponse, error)) ([]DNSResponse, []error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We do not really have coding style documented anywhere (mostly relying on go fmt), but personally I try to avoid long lines for better readability (my limit is 90 characters at most, but anything below 100 is fine IMHO).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done; perhaps we should integrate https://github.com/segmentio/golines into our build

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks interesting.
Or maybe just having a Yetus complain for a very long line, not sure if there is an option for that.

pkg/pillar/cmd/nim/controllerdns.go Outdated Show resolved Hide resolved
to make it more readable

Signed-off-by: Christoph Ostarek <christoph@zededa.com>
this allows us to set the source interface when doing
DNS requests

go-dns-resolver itself is using miekg/dns under the hood,
so now nim is using it directly instead

Signed-off-by: Christoph Ostarek <christoph@zededa.com>
Copy link
Contributor

@milan-zededa milan-zededa left a comment

Choose a reason for hiding this comment

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

Run cancelled eden again

@eriknordmark eriknordmark merged commit 8573372 into lf-edge:master Mar 29, 2023
Comment on lines +113 to +114
ttlSec = int(dnsResponses[0].TTL)
lookupIPaddr = dnsResponses[0].IP.String()
Copy link
Contributor

Choose a reason for hiding this comment

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

Finally see the complete panic stacktrace from the ztests run and it says
2023-03-30T17:12:55Z,pillar;panic: runtime error: index out of range [0] with length 0
2023-03-30T17:12:55Z,pillar;
2023-03-30T17:12:55Z,pillar;goroutine 512 [running]:
2023-03-30T17:12:55Z,pillar;github.com/lf-edge/eve/pkg/pillar/cmd/nim.(*nim).controllerDNSCache(0xc00035f400, {0xc000c1b200, 0xa0, 0x200}, {0xc000fbf200, 0x19, 0x200}, {0xc002376d60?, 0xc1018bbeb05b9c8e?})
2023-03-30T17:12:55Z,pillar; /pillar/cmd/nim/controllerdns.go:113 +0x387
2023-03-30T17:12:55Z,pillar;github.com/lf-edge/eve/pkg/pillar/cmd/nim.(*nim).queryControllerDNS(0xc00035f400)
2023-03-30T17:12:55Z,pillar; /pillar/cmd/nim/controllerdns.go:65 +0x3d6
2023-03-30T17:12:55Z,pillar;created by github.com/lf-edge/eve/pkg/pillar/cmd/nim.(*nim).run.func1
2023-03-30T17:12:55Z,pillar; /pillar/cmd/nim/nim.go:316 +0x196

Instead of the various checks (and these missing ones) for len(dnsResponses) == 0, it would be easier to read if there was a single if (len(dnsResponses) == 0 block here.

Also, if we get multiple responses shouldn't we use all of them? (I didn't check that the old dns code did when multiple responses for a name).

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