From a96c469e62e341040a69483afd89610556101cd5 Mon Sep 17 00:00:00 2001 From: Erik Sipsma Date: Tue, 17 Sep 2019 21:23:21 +0000 Subject: [PATCH] ptp: only override DNS conf if DNS settings provided Previously, if an IPAM plugin provided DNS settings in the result to the PTP plugin, those settings were always lost because the PTP plugin would always provide its own DNS settings in the result even if the PTP plugin was not configured with any DNS settings. This was especially problematic when trying to use, for example, the host-local IPAM plugin's support for retrieving DNS settings from a resolv.conf file on the host. Before this change, those DNS settings were always lost when using the PTP plugin and couldn't be specified as part of PTP instead because PTP does not support parsing a resolv.conf file. This change checks to see if any fields were actually set in the PTP plugin's DNS settings and only overrides any previous DNS results from an IPAM plugin in the case that settings actually were provided to PTP. In the case where no DNS settings are provided to PTP, the DNS results of the IPAM plugin (if any) are used instead. Signed-off-by: Erik Sipsma --- pkg/testutils/dns.go | 60 ++++++++++++++++ plugins/main/ptp/ptp.go | 15 +++- plugins/main/ptp/ptp_test.go | 131 +++++++++++++++++++++++++++++++++-- 3 files changed, 199 insertions(+), 7 deletions(-) create mode 100644 pkg/testutils/dns.go diff --git a/pkg/testutils/dns.go b/pkg/testutils/dns.go new file mode 100644 index 00000000..bd1b69d1 --- /dev/null +++ b/pkg/testutils/dns.go @@ -0,0 +1,60 @@ +// Copyright 2019 CNI authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package testutils + +import ( + "fmt" + "io/ioutil" + "os" + "strings" + + "github.com/containernetworking/cni/pkg/types" +) + +// TmpResolvConf will create a temporary file and write the provided DNS settings to +// it in the resolv.conf format. It returns the path of the created temporary file or +// an error if any occurs while creating/writing the file. It is the caller's +// responsibility to remove the file. +func TmpResolvConf(dnsConf types.DNS) (string, error) { + f, err := ioutil.TempFile("", "cni_test_resolv.conf") + if err != nil { + return "", fmt.Errorf("failed to get temp file for CNI test resolv.conf: %v", err) + } + defer f.Close() + + path := f.Name() + defer func() { + if err != nil { + os.RemoveAll(path) + } + }() + + // see "man 5 resolv.conf" for the format of resolv.conf + var resolvConfLines []string + for _, nameserver := range dnsConf.Nameservers { + resolvConfLines = append(resolvConfLines, fmt.Sprintf("nameserver %s", nameserver)) + } + resolvConfLines = append(resolvConfLines, fmt.Sprintf("domain %s", dnsConf.Domain)) + resolvConfLines = append(resolvConfLines, fmt.Sprintf("search %s", strings.Join(dnsConf.Search, " "))) + resolvConfLines = append(resolvConfLines, fmt.Sprintf("options %s", strings.Join(dnsConf.Options, " "))) + + resolvConf := strings.Join(resolvConfLines, "\n") + _, err = f.Write([]byte(resolvConf)) + if err != nil { + return "", fmt.Errorf("failed to write temp resolv.conf for CNI test: %v", err) + } + + return path, err +} diff --git a/plugins/main/ptp/ptp.go b/plugins/main/ptp/ptp.go index a748d7f6..a9464351 100644 --- a/plugins/main/ptp/ptp.go +++ b/plugins/main/ptp/ptp.go @@ -247,12 +247,25 @@ func cmdAdd(args *skel.CmdArgs) error { } } - result.DNS = conf.DNS + // Only override the DNS settings in the previous result if any DNS fields + // were provided to the ptp plugin. This allows, for example, IPAM plugins + // to specify the DNS settings instead of the ptp plugin. + if dnsConfSet(conf.DNS) { + result.DNS = conf.DNS + } + result.Interfaces = []*current.Interface{hostInterface, containerInterface} return types.PrintResult(result, conf.CNIVersion) } +func dnsConfSet(dnsConf types.DNS) bool { + return dnsConf.Nameservers != nil || + dnsConf.Search != nil || + dnsConf.Options != nil || + dnsConf.Domain != "" +} + func cmdDel(args *skel.CmdArgs) error { conf := NetConf{} if err := json.Unmarshal(args.StdinData, &conf); err != nil { diff --git a/plugins/main/ptp/ptp_test.go b/plugins/main/ptp/ptp_test.go index bedc2dda..7cc2cce0 100644 --- a/plugins/main/ptp/ptp_test.go +++ b/plugins/main/ptp/ptp_test.go @@ -17,6 +17,7 @@ package main import ( "encoding/json" "fmt" + "os" "github.com/containernetworking/cni/pkg/skel" "github.com/containernetworking/cni/pkg/types" @@ -101,7 +102,7 @@ var _ = Describe("ptp Operations", func() { Expect(testutils.UnmountNS(originalNS)).To(Succeed()) }) - doTest := func(conf string, numIPs int) { + doTest := func(conf string, numIPs int, expectedDNSConf types.DNS) { const IFNAME = "ptp0" targetNs, err := testutils.NewNS() @@ -176,6 +177,9 @@ var _ = Describe("ptp Operations", func() { Expect(res.Interfaces[1].Mac).To(Equal(wantMac)) Expect(res.Interfaces[1].Sandbox).To(Equal(targetNs.Path())) + // make sure DNS is correct + Expect(res.DNS).To(Equal(expectedDNSConf)) + // Call the plugins with the DEL command, deleting the veth endpoints err = originalNS.Do(func(ns.NetNS) error { defer GinkgoRecover() @@ -328,7 +332,16 @@ var _ = Describe("ptp Operations", func() { } It("configures and deconfigures a ptp link with ADD/DEL", func() { - conf := `{ + dnsConf := types.DNS{ + Nameservers: []string{"10.1.2.123"}, + Domain: "some.domain.test", + Search: []string{"search.test"}, + Options: []string{"option1:foo"}, + } + dnsConfBytes, err := json.Marshal(dnsConf) + Expect(err).NotTo(HaveOccurred()) + + conf := fmt.Sprintf(`{ "cniVersion": "0.3.1", "name": "mynet", "type": "ptp", @@ -337,10 +350,11 @@ var _ = Describe("ptp Operations", func() { "ipam": { "type": "host-local", "subnet": "10.1.2.0/24" - } -}` + }, + "dns": %s +}`, string(dnsConfBytes)) - doTest(conf, 1) + doTest(conf, 1, dnsConf) }) It("configures and deconfigures a dual-stack ptp link with ADD/DEL", func() { @@ -359,7 +373,112 @@ var _ = Describe("ptp Operations", func() { } }` - doTest(conf, 2) + doTest(conf, 2, types.DNS{}) + }) + + It("does not override IPAM DNS settings if no DNS settings provided", func() { + ipamDNSConf := types.DNS{ + Nameservers: []string{"10.1.2.123"}, + Domain: "some.domain.test", + Search: []string{"search.test"}, + Options: []string{"option1:foo"}, + } + resolvConfPath, err := testutils.TmpResolvConf(ipamDNSConf) + Expect(err).NotTo(HaveOccurred()) + defer os.RemoveAll(resolvConfPath) + + conf := fmt.Sprintf(`{ + "cniVersion": "0.3.1", + "name": "mynet", + "type": "ptp", + "ipMasq": true, + "mtu": 5000, + "ipam": { + "type": "host-local", + "subnet": "10.1.2.0/24", + "resolvConf": "%s" + } +}`, resolvConfPath) + + doTest(conf, 1, ipamDNSConf) + }) + + It("overrides IPAM DNS settings if any DNS settings provided", func() { + ipamDNSConf := types.DNS{ + Nameservers: []string{"10.1.2.123"}, + Domain: "some.domain.test", + Search: []string{"search.test"}, + Options: []string{"option1:foo"}, + } + resolvConfPath, err := testutils.TmpResolvConf(ipamDNSConf) + Expect(err).NotTo(HaveOccurred()) + defer os.RemoveAll(resolvConfPath) + + for _, ptpDNSConf := range []types.DNS{ + { + Nameservers: []string{"10.1.2.234"}, + }, + { + Domain: "someother.domain.test", + }, + { + Search: []string{"search.elsewhere.test"}, + }, + { + Options: []string{"option2:bar"}, + }, + } { + dnsConfBytes, err := json.Marshal(ptpDNSConf) + Expect(err).NotTo(HaveOccurred()) + + conf := fmt.Sprintf(`{ + "cniVersion": "0.3.1", + "name": "mynet", + "type": "ptp", + "ipMasq": true, + "mtu": 5000, + "ipam": { + "type": "host-local", + "subnet": "10.1.2.0/24", + "resolvConf": "%s" + }, + "dns": %s +}`, resolvConfPath, string(dnsConfBytes)) + + doTest(conf, 1, ptpDNSConf) + } + }) + + It("overrides IPAM DNS settings if any empty list DNS settings provided", func() { + ipamDNSConf := types.DNS{ + Nameservers: []string{"10.1.2.123"}, + Domain: "some.domain.test", + Search: []string{"search.test"}, + Options: []string{"option1:foo"}, + } + resolvConfPath, err := testutils.TmpResolvConf(ipamDNSConf) + Expect(err).NotTo(HaveOccurred()) + defer os.RemoveAll(resolvConfPath) + + conf := fmt.Sprintf(`{ + "cniVersion": "0.3.1", + "name": "mynet", + "type": "ptp", + "ipMasq": true, + "mtu": 5000, + "ipam": { + "type": "host-local", + "subnet": "10.1.2.0/24", + "resolvConf": "%s" + }, + "dns": { + "nameservers": [], + "search": [], + "options": [] + } +}`, resolvConfPath) + + doTest(conf, 1, types.DNS{}) }) It("deconfigures an unconfigured ptp link with DEL", func() {