Backport of cni: add DNS set by CNI plugins to task configuration int…

…o release/1.6.x (#20013) CNI plugins may set DNS configuration, but this isn't threaded through to the task configuration so that we can write it to the `/etc/resolv.conf` file as needed. Add the `AllocNetworkStatus` to the alloc hook resources so they're accessible from the taskrunner. Any DNS entries provided by the user will override these values. Fixes: #11102 Co-authored-by: Tim Gross <[email protected]>
hashicorp · Feb 20, 2024 · cdeafaf · cdeafaf
1 parent 132b674
commit cdeafaf
Show file tree

Hide file tree

Showing 7 changed files with 166 additions and 5 deletions.
diff --git a/.changelog/20007.txt b/.changelog/20007.txt
@@ -0,0 +1,3 @@
+```release-note:bug
+cni: Fixed a bug where DNS set by CNI plugins was not provided to task drivers
+```
diff --git a/client/allocrunner/alloc_runner.go b/client/allocrunner/alloc_runner.go
@@ -871,7 +871,9 @@ func (ar *allocRunner) SetClientStatus(clientStatus string) {
 func (ar *allocRunner) SetNetworkStatus(s *structs.AllocNetworkStatus) {
 	ar.stateLock.Lock()
 	defer ar.stateLock.Unlock()
-	ar.state.NetworkStatus = s.Copy()
+	ans := s.Copy()
+	ar.state.NetworkStatus = ans
+	ar.hookResources.SetAllocNetworkStatus(ans)
 }
 
 func (ar *allocRunner) NetworkStatus() *structs.AllocNetworkStatus {

diff --git a/client/allocrunner/taskrunner/task_runner.go b/client/allocrunner/taskrunner/task_runner.go
@@ -1119,6 +1119,18 @@ func (tr *TaskRunner) buildTaskConfig() *drivers.TaskConfig {
 	defer tr.networkIsolationLock.Unlock()
 
 	var dns *drivers.DNSConfig
+
+	// set DNS from any CNI plugins
+	netStatus := tr.allocHookResources.GetAllocNetworkStatus()
+	if netStatus != nil && netStatus.DNS != nil {
+		dns = &drivers.DNSConfig{
+			Servers:  netStatus.DNS.Servers,
+			Searches: netStatus.DNS.Searches,
+			Options:  netStatus.DNS.Options,
+		}
+	}
+
+	// override DNS if set by job submitter
 	if alloc.AllocatedResources != nil && len(alloc.AllocatedResources.Shared.Networks) > 0 {
 		allocDNS := alloc.AllocatedResources.Shared.Networks[0].DNS
 		if allocDNS != nil {

diff --git a/client/allocrunner/taskrunner/task_runner_test.go b/client/allocrunner/taskrunner/task_runner_test.go
@@ -34,6 +34,7 @@ import (
 	regMock "github.com/hashicorp/nomad/client/serviceregistration/mock"
 	"github.com/hashicorp/nomad/client/serviceregistration/wrapper"
 	cstate "github.com/hashicorp/nomad/client/state"
+	cstructs "github.com/hashicorp/nomad/client/structs"
 	ctestutil "github.com/hashicorp/nomad/client/testutil"
 	"github.com/hashicorp/nomad/client/vaultclient"
 	agentconsul "github.com/hashicorp/nomad/command/agent/consul"
@@ -152,6 +153,7 @@ func testTaskRunnerConfig(t *testing.T, alloc *structs.Allocation, taskName stri
 		ShutdownDelayCancelFn: shutdownDelayCancelFn,
 		ServiceRegWrapper:     wrapperMock,
 		Getter:                getter.TestSandbox(t),
+		AllocHookResources:    cstructs.NewAllocHookResources(),
 	}
 
 	// Set the cgroup path getter if we are in v2 mode
@@ -2709,3 +2711,121 @@ func TestTaskRunner_IdentityHook_Disabled(t *testing.T) {
 	taskEnv := tr.envBuilder.Build()
 	must.MapNotContainsKey(t, taskEnv.EnvMap, "NOMAD_TOKEN")
 }
+
+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"}
+
+	testCases := []struct {
+		name    string
+		alloc   *structs.Allocation
+		task    *structs.Task
+		fromCNI *structs.DNSConfig
+		expect  *drivers.DNSConfig
+	}{
+		{
+			name:  "task with group networking overrides CNI",
+			alloc: alloc1,
+			task:  task1,
+			fromCNI: &structs.DNSConfig{
+				Servers:  []string{"10.37.105.17"},
+				Searches: []string{"node.consul"},
+				Options:  []string{"ndots:2", "edns0"},
+			},
+			expect: &drivers.DNSConfig{
+				Servers:  []string{"1.1.1.1", "8.8.8.8"},
+				Searches: []string{"test.local"},
+				Options:  []string{"ndots:1"},
+			},
+		},
+		{
+			name:  "task with CNI alone",
+			alloc: alloc2,
+			task:  task1,
+			fromCNI: &structs.DNSConfig{
+				Servers:  []string{"10.37.105.17"},
+				Searches: []string{"node.consul"},
+				Options:  []string{"ndots:2", "edns0"},
+			},
+			expect: &drivers.DNSConfig{
+				Servers:  []string{"10.37.105.17"},
+				Searches: []string{"node.consul"},
+				Options:  []string{"ndots:2", "edns0"},
+			},
+		},
+		{
+			name:    "task with group networking alone",
+			alloc:   alloc1,
+			task:    task1,
+			fromCNI: nil,
+			expect: &drivers.DNSConfig{
+				Servers:  []string{"1.1.1.1", "8.8.8.8"},
+				Searches: []string{"test.local"},
+				Options:  []string{"ndots:1"},
+			},
+		},
+		{
+			name:    "task without group networking",
+			alloc:   alloc2,
+			task:    task2,
+			fromCNI: nil,
+			expect:  nil,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+
+			conf, cleanup := testTaskRunnerConfig(t, tc.alloc, tc.task.Name)
+			t.Cleanup(cleanup)
+
+			// note this will never actually be set if we don't have group/CNI
+			// networking, but it's a good validation no-group/CNI code path
+			conf.AllocHookResources.SetAllocNetworkStatus(&structs.AllocNetworkStatus{
+				InterfaceName: "",
+				Address:       "",
+				DNS:           tc.fromCNI,
+			})
+
+			tr, err := NewTaskRunner(conf)
+			must.NoError(t, err)
+
+			// Run the task runner.
+			go tr.Run()
+			t.Cleanup(func() {
+				tr.Kill(context.Background(), structs.NewTaskEvent("cleanup"))
+			})
+
+			// Wait for task to complete.
+			testWaitForTaskToStart(t, tr)
+
+			tr.stateLock.RLock()
+			t.Cleanup(tr.stateLock.RUnlock)
+
+			must.Eq(t, tc.expect, tr.localState.TaskHandle.Config.DNS)
+		})
+	}
+}
diff --git a/client/structs/allochook.go b/client/structs/allochook.go
@@ -8,14 +8,16 @@ import (
 
 	"github.com/hashicorp/nomad/client/pluginmanager/csimanager"
 	"github.com/hashicorp/nomad/helper"
+	"github.com/hashicorp/nomad/nomad/structs"
 )
 
 // AllocHookResources contains data that is provided by AllocRunner Hooks for
 // consumption by TaskRunners. This should be instantiated once in the
 // AllocRunner and then only accessed via getters and setters that hold the
 // lock.
 type AllocHookResources struct {
-	csiMounts map[string]*csimanager.MountInfo
+	csiMounts     map[string]*csimanager.MountInfo
+	networkStatus *structs.AllocNetworkStatus
 
 	mu sync.RWMutex
 }
@@ -43,3 +45,21 @@ func (a *AllocHookResources) SetCSIMounts(m map[string]*csimanager.MountInfo) {
 
 	a.csiMounts = m
 }
+
+// GetAllocNetworkStatus returns a copy of the AllocNetworkStatus previously
+// written the group's network_hook
+func (a *AllocHookResources) GetAllocNetworkStatus() *structs.AllocNetworkStatus {
+	a.mu.RLock()
+	defer a.mu.RUnlock()
+
+	return a.networkStatus.Copy()
+}
+
+// SetAllocNetworkStatus stores the AllocNetworkStatus for later use by the
+// taskrunner's buildTaskConfig() method
+func (a *AllocHookResources) SetAllocNetworkStatus(ans *structs.AllocNetworkStatus) {
+	a.mu.Lock()
+	defer a.mu.Unlock()
+
+	a.networkStatus = ans
+}
diff --git a/command/agent/consul/int_test.go b/command/agent/consul/int_test.go
@@ -21,6 +21,7 @@ import (
 	regMock "github.com/hashicorp/nomad/client/serviceregistration/mock"
 	"github.com/hashicorp/nomad/client/serviceregistration/wrapper"
 	"github.com/hashicorp/nomad/client/state"
+	cstructs "github.com/hashicorp/nomad/client/structs"
 	"github.com/hashicorp/nomad/client/vaultclient"
 	"github.com/hashicorp/nomad/command/agent/consul"
 	"github.com/hashicorp/nomad/helper/testlog"
@@ -169,6 +170,7 @@ func TestConsul_Integration(t *testing.T) {
 		DriverManager:       drivermanager.TestDriverManager(t),
 		StartConditionMetCh: closedCh,
 		ServiceRegWrapper:   wrapper.NewHandlerWrapper(logger, serviceClient, regMock.NewServiceRegistrationHandler(logger)),
+		AllocHookResources:  cstructs.NewAllocHookResources(),
 	}
 
 	tr, err := taskrunner.NewTaskRunner(config)

diff --git a/website/content/docs/job-specification/network.mdx b/website/content/docs/job-specification/network.mdx
@@ -84,9 +84,11 @@ All other operating systems use the `host` networking mode.
   [mode](#mode) is set to [`bridge`](#bridge). This parameter supports
   [interpolation](/nomad/docs/runtime/interpolation).
 
-- `dns` <code>([DNSConfig](#dns-parameters): nil)</code> - Sets the DNS configuration
-  for the allocations. By default all DNS configuration is inherited from the client host.
-  DNS configuration is only supported on Linux clients at this time.
+- `dns` <code>([DNSConfig](#dns-parameters): nil)</code> - Sets the DNS
+  configuration for the allocations. By default all task drivers will inherit
+  DNS configuration from the client host. DNS configuration is only supported on
+  Linux clients at this time. Note that if you are using a `mode="cni/*`, these
+  values will override any DNS configuration the CNI plugins return.
 
 ### `port` Parameters