Skip to content

libnetwork: make resolvconf more self-contained#42755

Merged
thaJeztah merged 5 commits intomoby:masterfrom
thaJeztah:move_resolvconf_consts
Aug 20, 2021
Merged

libnetwork: make resolvconf more self-contained#42755
thaJeztah merged 5 commits intomoby:masterfrom
thaJeztah:move_resolvconf_consts

Conversation

@thaJeztah
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah commented Aug 18, 2021

libnetwork: move resolvconf consts into the resolvconf package

This allows using the package without having to import the "types" package, and without having to consume github.com/ishidawataru/sctp.

libnetwork: remove resolvconf/dns package

The IsLocalhost utility was not used, and go's "net" package provides a IsLoopBack() check, which can be used in stead of IsIPv4Localhost.

move pkg/ioutils.HashData() to libnetwork/resolvconf

It's the only location it's used, so we might as well move it there. This also removes the "crypto/sha256" and "encoding/hex" dependencies from pkg/ioutils..

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@thaJeztah thaJeztah added status/2-code-review area/networking Networking kind/refactor PR's that refactor, or clean-up code labels Aug 18, 2021
@thaJeztah thaJeztah changed the title libnetwork: move resolvconf consts into the resolvconf package libnetwork: make resolvconf more self-contained Aug 18, 2021
thaJeztah added a commit to thaJeztah/buildkit that referenced this pull request Aug 18, 2021
Includes the changes from moby/moby#42755

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah marked this pull request as ready for review August 18, 2021 11:36
@thaJeztah
Copy link
Copy Markdown
Member Author

relates to moby/buildkit#2317

@thaJeztah thaJeztah force-pushed the move_resolvconf_consts branch from ddd02f3 to ebc554b Compare August 18, 2021 12:04
thaJeztah added a commit to thaJeztah/buildkit that referenced this pull request Aug 18, 2021
Includes the changes from moby/moby#42755

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
Copy link
Copy Markdown
Member Author

Something broke, but I'm confused what. I suspect it's related to the pkg/ioutils.HashData() utility, but that was just moved 🤔 ;

=== Failed
=== FAIL: reference TestLoad (0.00s)
    store_test.go:53: failed to parse reference: unsupported digest algorithm

=== FAIL: reference TestSave (0.00s)
    store_test.go:82: failed to parse reference: unsupported digest algorithm

=== FAIL: reference TestAddDeleteGet (0.00s)
    store_test.go:174: could not parse reference: unsupported digest algorithm

=== FAIL: reference TestInvalidTags (0.00s)
    store_test.go:355: assertion failed: error is not nil: unsupported digest algorithm

@thaJeztah
Copy link
Copy Markdown
Member Author

Opened opencontainers/go-digest#64 to fix go-digest

@thaJeztah thaJeztah force-pushed the move_resolvconf_consts branch from 5102ae7 to 049c8ab Compare August 18, 2021 14:42
Comment thread pkg/ioutils/readers.go Outdated
Comment on lines 7 to 10
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Included a temporary fix here (until opencontainers/go-digest#64 is merged)

Comment thread libnetwork/sandbox_dns_unix.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Isn't this different since it could be an ipv6 loopback?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ah, yes. Brainfart; I was thinking this was to skip loopback IP-addresses (as they won't work as DNS for container), but this is the bit where we forward requests to them. Yup, that shouldn't use IPv6 likely. Or maybe it should, but we can do that separately.

This allows using the package without having to import the "types" package,
and without having to consume github.com/ishidawataru/sctp.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
The IsLocalhost utility was not used, which only leaves the IsIPv4Localhost
utility.

Go's "net" package provides a `IsLoopBack()` check, but it checks for both
IPv4 and IPv6 loopback interfaces. We likely should also do IPv6 here, but
that's better left for a separate change, so instead, I replicated the IPv4
bits from Go's net.IP.IsLoopback().

Signed-off-by: Sebastiaan van Stijn <[email protected]>
It's the only location it's used, so we might as well move it there.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
Previously, ioutils imported the crypty/sha256 package, because it was
used by the HashData() utility. As a side-effect of that import, the
sha256 algorithm was registered through its `init()` function.

Now that the HashData() utility is removed, the import is no longer needed
in this package, but some parts of our code depended on the side-effect, and
without this, it fail to recognise the algorithms, unless something else
happens to import crypto/sha256 / crypto/sha512, which made our
tests fail:

```
=== Failed
=== FAIL: reference TestLoad (0.00s)
    store_test.go:53: failed to parse reference: unsupported digest algorithm

=== FAIL: reference TestSave (0.00s)
    store_test.go:82: failed to parse reference: unsupported digest algorithm

=== FAIL: reference TestAddDeleteGet (0.00s)
    store_test.go:174: could not parse reference: unsupported digest algorithm

=== FAIL: reference TestInvalidTags (0.00s)
    store_test.go:355: assertion failed: error is not nil: unsupported digest algorithm
```

While it would be better to do the import in the actual locations where it's
expected, there may be code-paths we overlook, so instead adding the import
here temporarily. Until the PR in go-digest has been merged and released.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
This information was already documented in the package's GoDoc, so
no need to repeat it.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah force-pushed the move_resolvconf_consts branch from 049c8ab to d215d34 Compare August 19, 2021 22:35
@thaJeztah
Copy link
Copy Markdown
Member Author

@cpuguy83 updated; PTAL

Copy link
Copy Markdown
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

😝 LGTM

// isIPv4Loopback checks if the given IP address is an IPv4 loopback address.
// It's based on the logic in Go's net.IP.IsLoopback(), but only the IPv4 part:
// https://github.com/golang/go/blob/go1.16.6/src/net/ip.go#L120-L126
func isIPv4Loopback(ipAddress string) bool {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a lot less eyebrow raising than the regular expressions above for parsing IP addresses in a language with a pretty decent standard library for parsing IP addresses. 😅 👍

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ha! True. Yes, I didn't compare performance between this and the original approach, but I assume the stdlib code is usually optimised.

@thaJeztah
Copy link
Copy Markdown
Member Author

Failure is unrelated, and a known flaky test ; #36907

=== RUN   TestCreateServiceConfigFileMode
    create_test.go:355: assertion failed: 2 (int) != 1 (int)
--- FAIL: TestCreateServiceConfigFileMode (8.57s)

@thaJeztah thaJeztah merged commit 5ea3e12 into moby:master Aug 20, 2021
@thaJeztah thaJeztah deleted the move_resolvconf_consts branch August 20, 2021 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/networking Networking kind/refactor PR's that refactor, or clean-up code status/2-code-review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants