From a70e87c3aaa104e5ecfd1b5a77061852892c76e9 Mon Sep 17 00:00:00 2001
From: Kern Walster <walster@amazon.com>
Date: Thu, 21 Apr 2022 05:05:06 +0000
Subject: [PATCH] bridge: support IPAM DNS settings

Previously, the bridge plugin ignored DNS settings returned
from an IPAM plugin (e.g. the host-local plugin parsing
resolv.conf to configure DNS). With this change, the bridge plugin
uses IPAM DNS settings.

Similarly to #388, this change will use incoming DNS settings if set,
otherwise IPAM plugin returned DNS settings

Signed-off-by: Kern Walster <walster@amazon.com>
---
 plugins/main/bridge/bridge.go      | 16 +++++++++--
 plugins/main/bridge/bridge_test.go | 46 ++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+), 2 deletions(-)

diff --git a/plugins/main/bridge/bridge.go b/plugins/main/bridge/bridge.go
index 23c1256ff..ddb3f81db 100644
--- a/plugins/main/bridge/bridge.go
+++ b/plugins/main/bridge/bridge.go
@@ -489,6 +489,7 @@ func cmdAdd(args *skel.CmdArgs) error {
 
 		result.IPs = ipamResult.IPs
 		result.Routes = ipamResult.Routes
+		result.DNS = ipamResult.DNS
 
 		if len(result.IPs) == 0 {
 			return errors.New("IPAM plugin returned missing IP config")
@@ -611,18 +612,29 @@ func cmdAdd(args *skel.CmdArgs) error {
 	}
 	brInterface.Mac = br.Attrs().HardwareAddr.String()
 
-	result.DNS = n.DNS
-
 	// Return an error requested by testcases, if any
 	if debugPostIPAMError != nil {
 		return debugPostIPAMError
 	}
 
+	// Use incoming DNS settings if provided, otherwise use the
+	// settings that were already configued by the IPAM plugin
+	if dnsConfSet(n.DNS) {
+		result.DNS = n.DNS
+	}
+
 	success = true
 
 	return types.PrintResult(result, 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 {
 	n, _, err := loadNetConf(args.StdinData, args.Args)
 	if err != nil {
diff --git a/plugins/main/bridge/bridge_test.go b/plugins/main/bridge/bridge_test.go
index 118074ed6..2d93adca3 100644
--- a/plugins/main/bridge/bridge_test.go
+++ b/plugins/main/bridge/bridge_test.go
@@ -45,6 +45,7 @@ const (
 	BRNAME     = "bridge0"
 	BRNAMEVLAN = "bridge0.100"
 	IFNAME     = "eth0"
+	NAMESERVER = "192.0.2.0"
 )
 
 type Net struct {
@@ -70,6 +71,7 @@ type testCase struct {
 	subnet      string      // Single subnet config: Subnet CIDR
 	gateway     string      // Single subnet config: Gateway
 	ranges      []rangeInfo // Ranges list (multiple subnets config)
+	resolvConf  string      // host-local resolvConf file path
 	isGW        bool
 	isLayer2    bool
 	expGWCIDRs  []string // Expected gateway addresses in CIDR form
@@ -139,6 +141,9 @@ const (
 	ipamDataDirStr = `,
         "dataDir": "%s"`
 
+	ipamResolvConfStr = `,
+		"resolvConf": "%s"`
+
 	ipMasqConfStr = `,
 	"ipMasq": %t`
 
@@ -215,6 +220,9 @@ func (tc testCase) netConfJSON(dataDir string) string {
 			if tc.ranges != nil {
 				conf += tc.rangesConfig()
 			}
+			if tc.resolvConf != "" {
+				conf += tc.resolvConfConfig()
+			}
 			conf += ipamEndStr
 		}
 	} else {
@@ -252,6 +260,26 @@ func (tc testCase) rangesConfig() string {
 	return conf + rangesEndStr
 }
 
+func (tc testCase) resolvConfConfig() string {
+	conf := fmt.Sprintf(ipamResolvConfStr, tc.resolvConf)
+	return conf
+}
+
+func newResolvConf() (string, error) {
+	f, err := ioutil.TempFile("", "host_local_resolv")
+	if err != nil {
+		return "", err
+	}
+	defer f.Close()
+	name := f.Name()
+	_, err = f.WriteString(fmt.Sprintf("nameserver %s", NAMESERVER))
+	return name, err
+}
+
+func deleteResolvConf(path string) error {
+	return os.Remove(path)
+}
+
 var counter uint
 
 // createCmdArgs generates network configuration and creates command
@@ -565,6 +593,11 @@ func (tester *testerV10x) cmdAddTest(tc testCase, dataDir string) (types.Result,
 			Expect(link.Attrs().HardwareAddr.String()).NotTo(Equal(bridgeMAC))
 		}
 
+		// Check that resolvConf was used properly
+		if !tc.isLayer2 && tc.resolvConf != "" {
+			Expect(result.DNS.Nameservers).To(Equal([]string{NAMESERVER}))
+		}
+
 		return nil
 	})
 	Expect(err).NotTo(HaveOccurred())
@@ -1587,6 +1620,9 @@ var _ = Describe("bridge Operations", func() {
 	var originalNS, targetNS ns.NetNS
 	var dataDir string
 
+	resolvConf, err := newResolvConf()
+	Expect(err).NotTo(HaveOccurred())
+
 	BeforeEach(func() {
 		// Create a new NetNS so we don't modify the host
 		var err error
@@ -1610,6 +1646,10 @@ var _ = Describe("bridge Operations", func() {
 		Expect(testutils.UnmountNS(targetNS)).To(Succeed())
 	})
 
+	AfterSuite(func() {
+		deleteResolvConf(resolvConf)
+	})
+
 	for _, ver := range testutils.AllSpecVersions {
 		// Redefine ver inside for scope so real value is picked up by each dynamically defined It()
 		// See Gingkgo's "Patterns for dynamically generating tests" documentation.
@@ -1706,6 +1746,12 @@ var _ = Describe("bridge Operations", func() {
 				AddErr010: "CNI version 0.1.0 does not support more than 1 address per family",
 				DelErr010: "CNI version 0.1.0 does not support more than 1 address per family",
 			},
+			{
+				// with resolvConf DNS settings
+				subnet:     "10.1.2.0/24",
+				expGWCIDRs: []string{"10.1.2.1/24"},
+				resolvConf: resolvConf,
+			},
 		} {
 			tc := tc
 			i := i