Merge pull request #388 from sipsma/fix-ptpdns

ptp: only override DNS conf if DNS settings provided

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
+}

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 {

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() {