From 03aeedcca9a1c9d6faa3dacd78b798b7953359fa Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 4 Mar 2025 12:17:50 +0100 Subject: [PATCH 1/3] libnetwork/resolvconf: update tests to use more correct values The tests were depending on the implementation ignoring invalid nameservers, as these should always be an IP-address, not a hostname. Update the tests to use correct values, instead of codifying the bad behavior. Signed-off-by: Sebastiaan van Stijn --- libnetwork/resolvconf/resolvconf_unix_test.go | 38 ++++++++++++++++--- 1 file changed, 32 insertions(+), 6 deletions(-) diff --git a/libnetwork/resolvconf/resolvconf_unix_test.go b/libnetwork/resolvconf/resolvconf_unix_test.go index 5cf6ea8397..31cd256550 100644 --- a/libnetwork/resolvconf/resolvconf_unix_test.go +++ b/libnetwork/resolvconf/resolvconf_unix_test.go @@ -289,6 +289,16 @@ func strSlicesEqual(a, b []string) bool { return true } +const ( + // Example IP-addresses as defined in [RFC 5737], [RFC 3849, section 2]. + // + // [RFC 5737]: https://datatracker.ietf.org/doc/html/rfc5737 + // [RFC 3849, section 2]: https://datatracker.ietf.org/doc/html/rfc3849#section-2 + testNS1 = "192.0.2.1" + testNS2 = "2001:db8::1" + testNS3 = "203.0.113.3" +) + func TestBuild(t *testing.T) { tmpDir := t.TempDir() file, err := os.CreateTemp(tmpDir, "") @@ -296,12 +306,18 @@ func TestBuild(t *testing.T) { t.Fatal(err) } - f, err := Build(file.Name(), []string{"ns1", "ns2", "ns3"}, []string{"search1"}, []string{"opt1"}) + f, err := Build(file.Name(), []string{testNS1, testNS2, testNS3}, []string{"search1"}, []string{"opt1"}) if err != nil { t.Fatal(err) } - const expected = "search search1\nnameserver ns1\nnameserver ns2\nnameserver ns3\noptions opt1\n" + const expected = `search search1 +nameserver 192.0.2.1 +nameserver 2001:db8::1 +nameserver 203.0.113.3 +options opt1 +` + if !bytes.Equal(f.Content, []byte(expected)) { t.Errorf("Expected to find '%s' got '%s'", expected, f.Content) } @@ -321,12 +337,17 @@ func TestBuildWithZeroLengthDomainSearch(t *testing.T) { t.Fatal(err) } - f, err := Build(file.Name(), []string{"ns1", "ns2", "ns3"}, []string{"."}, []string{"opt1"}) + f, err := Build(file.Name(), []string{testNS1, testNS2, testNS3}, []string{"."}, []string{"opt1"}) if err != nil { t.Fatal(err) } - const expected = "nameserver ns1\nnameserver ns2\nnameserver ns3\noptions opt1\n" + const expected = `nameserver 192.0.2.1 +nameserver 2001:db8::1 +nameserver 203.0.113.3 +options opt1 +` + if !bytes.Equal(f.Content, []byte(expected)) { t.Errorf("Expected to find '%s' got '%s'", expected, f.Content) } @@ -346,12 +367,17 @@ func TestBuildWithNoOptions(t *testing.T) { t.Fatal(err) } - f, err := Build(file.Name(), []string{"ns1", "ns2", "ns3"}, []string{"search1"}, []string{}) + f, err := Build(file.Name(), []string{testNS1, testNS2, testNS3}, []string{"search1"}, []string{}) if err != nil { t.Fatal(err) } - const expected = "search search1\nnameserver ns1\nnameserver ns2\nnameserver ns3\n" + const expected = `search search1 +nameserver 192.0.2.1 +nameserver 2001:db8::1 +nameserver 203.0.113.3 +` + if !bytes.Equal(f.Content, []byte(expected)) { t.Errorf("Expected to find '%s' got '%s'", expected, f.Content) } From 2f19577877bdb25a9220fb51ce8dabb5197b3095 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 4 Mar 2025 12:26:30 +0100 Subject: [PATCH 2/3] libnetwork/resolvconf: Build: align order with new implementation The new resolvconf implementation in libnetwork/internal/resolvconf uses a different order for "search" and puts it after the resolvers. While this changes the checksum of the file, and thus would be potentially detected as "user modified", we no longer use this function ourselves, and BuildKit (which uses this function) only consumes the Content field, and discards the Hash; https://github.com/moby/buildkit/blob/v0.20.0/executor/oci/resolvconf.go#L99-L103 Signed-off-by: Sebastiaan van Stijn --- libnetwork/resolvconf/resolvconf.go | 10 +++++----- libnetwork/resolvconf/resolvconf_unix_test.go | 8 ++++---- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/libnetwork/resolvconf/resolvconf.go b/libnetwork/resolvconf/resolvconf.go index 4805b0eadf..a18526ce74 100644 --- a/libnetwork/resolvconf/resolvconf.go +++ b/libnetwork/resolvconf/resolvconf.go @@ -128,6 +128,11 @@ func GetOptions(resolvConf []byte) []string { // Note that the resolv.conf file is written, but the hash file is not. func Build(path string, nameservers, dnsSearch, dnsOptions []string) (*File, error) { content := bytes.NewBuffer(nil) + for _, dns := range nameservers { + if _, err := content.WriteString("nameserver " + dns + "\n"); err != nil { + return nil, err + } + } if len(dnsSearch) > 0 { if searchString := strings.Join(dnsSearch, " "); strings.Trim(searchString, " ") != "." { if _, err := content.WriteString("search " + searchString + "\n"); err != nil { @@ -135,11 +140,6 @@ func Build(path string, nameservers, dnsSearch, dnsOptions []string) (*File, err } } } - for _, dns := range nameservers { - if _, err := content.WriteString("nameserver " + dns + "\n"); err != nil { - return nil, err - } - } if len(dnsOptions) > 0 { if optsString := strings.Join(dnsOptions, " "); strings.Trim(optsString, " ") != "" { if _, err := content.WriteString("options " + optsString + "\n"); err != nil { diff --git a/libnetwork/resolvconf/resolvconf_unix_test.go b/libnetwork/resolvconf/resolvconf_unix_test.go index 31cd256550..3f98a7e39d 100644 --- a/libnetwork/resolvconf/resolvconf_unix_test.go +++ b/libnetwork/resolvconf/resolvconf_unix_test.go @@ -311,10 +311,10 @@ func TestBuild(t *testing.T) { t.Fatal(err) } - const expected = `search search1 -nameserver 192.0.2.1 + const expected = `nameserver 192.0.2.1 nameserver 2001:db8::1 nameserver 203.0.113.3 +search search1 options opt1 ` @@ -372,10 +372,10 @@ func TestBuildWithNoOptions(t *testing.T) { t.Fatal(err) } - const expected = `search search1 -nameserver 192.0.2.1 + const expected = `nameserver 192.0.2.1 nameserver 2001:db8::1 nameserver 203.0.113.3 +search search1 ` if !bytes.Equal(f.Content, []byte(expected)) { From 00bd916203d01831bea2173ead6cd6736b53a877 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 4 Mar 2025 12:34:09 +0100 Subject: [PATCH 3/3] libnetwork/resolvconf: Build: re-implement using new implementation Re-implement the Build function using the new implementation from libnetwork/internal/resolvconf. BuildKit is the only consumer of the Build function currently, and we should consider either moving the internal package out of internal, or to provide a more customized variant of Build that fits BuildKit's needs (i.e., only patch content, but not write to a file). Signed-off-by: Sebastiaan van Stijn --- libnetwork/resolvconf/resolvconf.go | 43 +++++++++++++++-------------- 1 file changed, 22 insertions(+), 21 deletions(-) diff --git a/libnetwork/resolvconf/resolvconf.go b/libnetwork/resolvconf/resolvconf.go index a18526ce74..3be2bff6ca 100644 --- a/libnetwork/resolvconf/resolvconf.go +++ b/libnetwork/resolvconf/resolvconf.go @@ -5,7 +5,6 @@ import ( "bytes" "net/netip" "os" - "strings" "github.com/docker/docker/libnetwork/internal/resolvconf" "github.com/opencontainers/go-digest" @@ -127,31 +126,33 @@ func GetOptions(resolvConf []byte) []string { // // Note that the resolv.conf file is written, but the hash file is not. func Build(path string, nameservers, dnsSearch, dnsOptions []string) (*File, error) { - content := bytes.NewBuffer(nil) - for _, dns := range nameservers { - if _, err := content.WriteString("nameserver " + dns + "\n"); err != nil { + var ns []netip.Addr + for _, addr := range nameservers { + ipAddr, err := netip.ParseAddr(addr) + if err != nil { return nil, err } + ns = append(ns, ipAddr) } - if len(dnsSearch) > 0 { - if searchString := strings.Join(dnsSearch, " "); strings.Trim(searchString, " ") != "." { - if _, err := content.WriteString("search " + searchString + "\n"); err != nil { - return nil, err - } - } - } - if len(dnsOptions) > 0 { - if optsString := strings.Join(dnsOptions, " "); strings.Trim(optsString, " ") != "" { - if _, err := content.WriteString("options " + optsString + "\n"); err != nil { - return nil, err - } - } - } + rc := resolvconf.ResolvConf{} + rc.OverrideNameServers(ns) + rc.OverrideSearch(dnsSearch) + rc.OverrideOptions(dnsOptions) - if err := os.WriteFile(path, content.Bytes(), 0o644); err != nil { + content, err := rc.Generate(false) + if err != nil { return nil, err } - hash := digest.FromBytes(content.Bytes()) - return &File{Content: content.Bytes(), Hash: []byte(hash)}, nil + // Write the resolv.conf file - it's bind-mounted into the container, so can't + // move a temp file into place, just have to truncate and write it. + // + // TODO(thaJeztah): the Build function is currently only used by BuildKit, which only uses "File.Content", and doesn't require the file to be written. + if err := os.WriteFile(path, content, 0o644); err != nil { + return nil, err + } + + // TODO(thaJeztah): the Build function is currently only used by BuildKit, which does not use the Hash + hash := digest.FromBytes(content) + return &File{Content: content, Hash: []byte(hash)}, nil }