Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cni: add DNS set by CNI plugins to task configuration #20007

Merged
merged 3 commits into from
Feb 20, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .changelog/20007.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
cni: Fixed a bug where DNS set by CNI plugins was not provided to task drivers
```
1 change: 1 addition & 0 deletions client/allocrunner/alloc_runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -909,6 +909,7 @@ func (ar *allocRunner) SetNetworkStatus(s *structs.AllocNetworkStatus) {
ar.stateLock.Lock()
defer ar.stateLock.Unlock()
ar.state.NetworkStatus = s.Copy()
ar.hookResources.SetAllocNetworkStatus(s) // will copy in getter
tgross marked this conversation as resolved.
Show resolved Hide resolved
}

func (ar *allocRunner) NetworkStatus() *structs.AllocNetworkStatus {
Expand Down
16 changes: 16 additions & 0 deletions client/allocrunner/taskrunner/task_runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -1135,6 +1135,22 @@ func (tr *TaskRunner) buildTaskConfig() *drivers.TaskConfig {
Options: interpolatedNetworks[0].DNS.Options,
}
}

// merge in the DNS set by any CNI plugins
netStatus := tr.allocHookResources.GetAllocNetworkStatus()
if netStatus != nil && netStatus.DNS != nil {
tgross marked this conversation as resolved.
Show resolved Hide resolved
if dns == nil {
dns = &drivers.DNSConfig{
tgross marked this conversation as resolved.
Show resolved Hide resolved
Servers: netStatus.DNS.Servers,
Searches: netStatus.DNS.Searches,
Options: netStatus.DNS.Options,
}
} else {
dns.Servers = append(netStatus.DNS.Servers, dns.Servers...)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I understand the goal here, I am not sure if merging DNS is the proper way to go here. For instance if options has rotate set then there would be no guarantee that the first server is actually used. https://developer.hashicorp.com/nomad/docs/job-specification/network#dns also says that the defaults from the client host are used, so we don't necessarily know what is set etc… I fear that this might end up being a bit fragile.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you've noted, there are three potential sources for DNS configuration here: the output of CNI plugins, the user-provided DNS config, and whatever is on the client. This code path only involves the first two. The task driver is then responsible for what happens after that... the built-in task drivers use GenerateDNSMount to create the /etc/resolv.conf. (So for example, you could have a third-party task driver that requires a particular CNI plugin be used, in which case it'd use that value alone and not get anything from the client host.)

All that being said, any DNS configuration from the CNI plugins is frankly likely to conflict with any jobspec network.dns configuration. Our default network configuration for bridge networking doesn't generate any DNS values. So if you're intentionally using a CNI plugin that does, you should know not to set conflicting values on the network.dns block. Otherwise yes, it's fragile... we want to give users knobs here but its up to them to use them responsibly.

Could definitely do with some warnings around network.dns usage in the docs though.

Copy link
Member Author

@tgross tgross Feb 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having second thoughts on the order though here... lemme get some input from the team on that. (And happy to hear your thoughts as well!)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So since DNS is usually at fault when there are issues I'd prefer a "stable" configuration like the following:

  • By default use the resolv.conf like now (ie I have a custom DNS set in docker which nicely ends up in the resolv.conf). Now here being as in current master before this patch.
  • If DNS is set by CNI, override the default.
  • if DNS is set in the task, override also whatever was set before. I would consider setting DNS on the task level a rather specific and advanced configuration, people doing that can as well set a DNS server that knows how to handle .consul

In a perfect world I'd love to be able to customize the template for the default bridge CNI configuration (with and without tproxy) so I could globally override that for all bridge users without having to go into every task and change mode to cni/my_cni.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aye, that's pretty much what the rest of the Nomad team suggested we do here, so I was definitely off the rails a bit 😀

In a perfect world I'd love to be able to customize the template for the default bridge CNI configuration (with and without tproxy) so I could globally override that for all bridge users without having to go into every task and change mode to cni/my_cni.

Definitely an interesting idea. A little challenging from the perspective of fingerprinting, because if there's behavior the application needs from that specific bridge configuration there's no reasonable way for us to tell the server which clients have that configuration set.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why this would require fingerprinting. In it's easiest implementation one could just specify an alternate template in the client-configuration for

const nomadCNIConfigTemplate = `{
(we probably need templating to play nicely with subnets etc…).

We could even add cni_consul to that list by default (assuming nomad) ships it or so and have it activate/deactivate based on cni args. This way the same template could be used independent of whether tproxy is used or not.

dns.Searches = append(netStatus.DNS.Searches, dns.Searches...)
dns.Options = append(netStatus.DNS.Options, dns.Options...)
}
}
}

memoryLimit := taskResources.Memory.MemoryMB
Expand Down
89 changes: 89 additions & 0 deletions client/allocrunner/taskrunner/task_runner_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2931,3 +2931,92 @@ 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{},
},
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
expect *drivers.DNSConfig
}{
{
name: "task with group networking",
alloc: alloc1,
task: task1,
expect: &drivers.DNSConfig{
Servers: []string{"10.37.105.17", "1.1.1.1", "8.8.8.8"},
Searches: []string{"node.consul", "test.local"},
Options: []string{"ndots:2", "edns0"},
},
},
{
name: "task without group networking",
alloc: alloc2,
task: task2,
expect: nil,
tgross marked this conversation as resolved.
Show resolved Hide resolved
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {

conf, cleanup := testTaskRunnerConfig(t, tc.alloc, tc.task.Name, nil)
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: &structs.DNSConfig{
Servers: []string{"10.37.105.17"},
Searches: []string{"node.consul"},
Options: []string{"ndots:2", "edns0"},
},
})

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)
})
}
}
24 changes: 22 additions & 2 deletions client/structs/allochook.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,17 @@ import (
consulapi "github.com/hashicorp/consul/api"
"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
consulTokens map[string]map[string]*consulapi.ACLToken // Consul cluster -> service identity -> token
csiMounts map[string]*csimanager.MountInfo
consulTokens map[string]map[string]*consulapi.ACLToken // Consul cluster -> service identity -> token
networkStatus *structs.AllocNetworkStatus

mu sync.RWMutex
}
Expand Down Expand Up @@ -67,3 +69,21 @@ func (a *AllocHookResources) SetConsulTokens(m map[string]map[string]*consulapi.
a.consulTokens[k] = v
}
}

// 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
}
Loading