From 11ff28247bda769e565222ada68b8e98751856c1 Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Fri, 22 Mar 2024 10:54:16 -0400 Subject: [PATCH] cni: fix regression in falling back to DNS owned by `dockerd` (#20189) In #20007 we fixed a bug where the DNS configuration set by CNI plugins was not threaded through to the task configuration. This resulted in a regression where a DNS override set by `dockerd` was not respected for `bridge` mode networking. Our existing handling of CNI DNS incorrectly assumed that the DNS field would be empty, when in fact it contains a single empty DNS struct. Handle this case correctly by checking whether the DNS struct we get back from CNI has any nameservers, and ignore it if it doesn't. Expand test coverage of this case. Fixes: https://github.com/hashicorp/nomad/issues/20174 --- .changelog/20189.txt | 3 + client/allocrunner/networking_cni.go | 13 +-- client/allocrunner/networking_cni_test.go | 18 ++-- .../taskrunner/task_runner_test.go | 86 ++++++++++--------- 4 files changed, 65 insertions(+), 55 deletions(-) create mode 100644 .changelog/20189.txt diff --git a/.changelog/20189.txt b/.changelog/20189.txt new file mode 100644 index 00000000000..eac272c0bca --- /dev/null +++ b/.changelog/20189.txt @@ -0,0 +1,3 @@ +```release-note:bug +cni: Fixed a regression where default DNS set by `dockerd` or other task drivers was not respected +``` diff --git a/client/allocrunner/networking_cni.go b/client/allocrunner/networking_cni.go index 654e7349dc0..3641aebcb89 100644 --- a/client/allocrunner/networking_cni.go +++ b/client/allocrunner/networking_cni.go @@ -184,12 +184,15 @@ func (c *cniNetworkConfigurator) cniToAllocNet(res *cni.Result) (*structs.AllocN } - // Use the first DNS results. + // Use the first DNS results, if non-empty if len(res.DNS) > 0 { - netStatus.DNS = &structs.DNSConfig{ - Servers: res.DNS[0].Nameservers, - Searches: res.DNS[0].Search, - Options: res.DNS[0].Options, + cniDNS := res.DNS[0] + if len(cniDNS.Nameservers) > 0 { + netStatus.DNS = &structs.DNSConfig{ + Servers: cniDNS.Nameservers, + Searches: cniDNS.Search, + Options: cniDNS.Options, + } } } diff --git a/client/allocrunner/networking_cni_test.go b/client/allocrunner/networking_cni_test.go index 5c25725b297..b773a9486f3 100644 --- a/client/allocrunner/networking_cni_test.go +++ b/client/allocrunner/networking_cni_test.go @@ -11,10 +11,11 @@ import ( "testing" "github.com/containerd/go-cni" + "github.com/containernetworking/cni/pkg/types" "github.com/hashicorp/nomad/ci" "github.com/hashicorp/nomad/helper/testlog" + "github.com/shoenig/test" "github.com/shoenig/test/must" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -148,9 +149,8 @@ func TestCNI_cniToAllocNet_NoInterfaces(t *testing.T) { func TestCNI_cniToAllocNet_Fallback(t *testing.T) { ci.Parallel(t) - // Calico's CNI plugin v3.12.3 has been observed to return the - // following: cniResult := &cni.Result{ + // Calico's CNI plugin v3.12.3 has been observed to return the following: Interfaces: map[string]*cni.Config{ "cali39179aa3-74": {}, "eth0": { @@ -161,6 +161,8 @@ func TestCNI_cniToAllocNet_Fallback(t *testing.T) { }, }, }, + // cni.Result will return a single empty struct, not an empty slice + DNS: []types.DNS{{}}, } // Only need a logger @@ -168,11 +170,11 @@ func TestCNI_cniToAllocNet_Fallback(t *testing.T) { logger: testlog.HCLogger(t), } allocNet, err := c.cniToAllocNet(cniResult) - require.NoError(t, err) - require.NotNil(t, allocNet) - assert.Equal(t, "192.168.135.232", allocNet.Address) - assert.Equal(t, "eth0", allocNet.InterfaceName) - assert.Nil(t, allocNet.DNS) + must.NoError(t, err) + must.NotNil(t, allocNet) + test.Eq(t, "192.168.135.232", allocNet.Address) + test.Eq(t, "eth0", allocNet.InterfaceName) + test.Nil(t, allocNet.DNS) } // TestCNI_cniToAllocNet_Invalid asserts an error is returned if a CNI plugin diff --git a/client/allocrunner/taskrunner/task_runner_test.go b/client/allocrunner/taskrunner/task_runner_test.go index e5fe60dc237..e4b6e678f3c 100644 --- a/client/allocrunner/taskrunner/task_runner_test.go +++ b/client/allocrunner/taskrunner/task_runner_test.go @@ -2935,41 +2935,39 @@ func TestTaskRunner_IdentityHook_Disabled(t *testing.T) { func TestTaskRunner_AllocNetworkStatus(t *testing.T) { ci.Parallel(t) - // Mock task with group network - alloc1 := mock.Alloc() - task1 := alloc1.Job.TaskGroups[0].Tasks[0] - alloc1.AllocatedResources.Shared.Networks = []*structs.NetworkResource{ - { - Device: "eth0", - IP: "192.168.0.100", - DNS: &structs.DNSConfig{ - Servers: []string{"1.1.1.1", "8.8.8.8"}, - Searches: []string{"test.local"}, - Options: []string{"ndots:1"}, - }, - ReservedPorts: []structs.Port{{Label: "admin", Value: 5000}}, - DynamicPorts: []structs.Port{{Label: "http", Value: 9876}}, - }} - task1.Driver = "mock_driver" - task1.Config = map[string]interface{}{"run_for": "2s"} - - // Mock task with task networking only - alloc2 := mock.Alloc() - task2 := alloc2.Job.TaskGroups[0].Tasks[0] - task2.Driver = "mock_driver" - task2.Config = map[string]interface{}{"run_for": "2s"} + alloc := mock.Alloc() + task := alloc.Job.TaskGroups[0].Tasks[0] + task.Driver = "mock_driver" + task.Config = map[string]interface{}{"run_for": "2s"} + + groupNetworks := []*structs.NetworkResource{{ + Device: "eth0", + IP: "192.168.0.100", + DNS: &structs.DNSConfig{ + Servers: []string{"1.1.1.1", "8.8.8.8"}, + Searches: []string{"test.local"}, + Options: []string{"ndots:1"}, + }, + ReservedPorts: []structs.Port{{Label: "admin", Value: 5000}}, + DynamicPorts: []structs.Port{{Label: "http", Value: 9876}}, + }} + + groupNetworksWithoutDNS := []*structs.NetworkResource{{ + Device: "eth0", + IP: "192.168.0.100", + ReservedPorts: []structs.Port{{Label: "admin", Value: 5000}}, + DynamicPorts: []structs.Port{{Label: "http", Value: 9876}}, + }} testCases := []struct { - name string - alloc *structs.Allocation - task *structs.Task - fromCNI *structs.DNSConfig - expect *drivers.DNSConfig + name string + networks []*structs.NetworkResource + fromCNI *structs.DNSConfig + expect *drivers.DNSConfig }{ { - name: "task with group networking overrides CNI", - alloc: alloc1, - task: task1, + name: "task with group networking overrides CNI", + networks: groupNetworks, fromCNI: &structs.DNSConfig{ Servers: []string{"10.37.105.17"}, Searches: []string{"node.consul"}, @@ -2982,9 +2980,7 @@ func TestTaskRunner_AllocNetworkStatus(t *testing.T) { }, }, { - name: "task with CNI alone", - alloc: alloc2, - task: task1, + name: "task with CNI alone", fromCNI: &structs.DNSConfig{ Servers: []string{"10.37.105.17"}, Searches: []string{"node.consul"}, @@ -2997,10 +2993,9 @@ func TestTaskRunner_AllocNetworkStatus(t *testing.T) { }, }, { - name: "task with group networking alone", - alloc: alloc1, - task: task1, - fromCNI: nil, + name: "task with group networking alone wth DNS", + networks: groupNetworks, + fromCNI: nil, expect: &drivers.DNSConfig{ Servers: []string{"1.1.1.1", "8.8.8.8"}, Searches: []string{"test.local"}, @@ -3008,9 +3003,13 @@ func TestTaskRunner_AllocNetworkStatus(t *testing.T) { }, }, { - name: "task without group networking", - alloc: alloc2, - task: task2, + name: "task with group networking and no CNI dns", + networks: groupNetworksWithoutDNS, + fromCNI: &structs.DNSConfig{}, + expect: &drivers.DNSConfig{}, + }, + { + name: "task without group networking or CNI", fromCNI: nil, expect: nil, }, @@ -3019,7 +3018,10 @@ func TestTaskRunner_AllocNetworkStatus(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - conf, cleanup := testTaskRunnerConfig(t, tc.alloc, tc.task.Name, nil) + testAlloc := alloc.Copy() + testAlloc.AllocatedResources.Shared.Networks = tc.networks + + conf, cleanup := testTaskRunnerConfig(t, testAlloc, task.Name, nil) t.Cleanup(cleanup) // note this will never actually be set if we don't have group/CNI