From cd9e3f0bf6c82721a65f131b0b9914f15ec1b401 Mon Sep 17 00:00:00 2001 From: Martina Santangelo Date: Thu, 27 Jun 2024 16:49:28 -0400 Subject: [PATCH 1/6] cni: allow users to set CNI args in job spec --- api/resources.go | 6 +- client/allocrunner/networking_cni.go | 19 +++++- client/allocrunner/networking_cni_test.go | 81 +++++++++++++++++++++++ client/taskenv/network.go | 7 ++ client/taskenv/network_test.go | 36 ++++++++++ command/agent/job_endpoint.go | 5 ++ nomad/structs/cni_config.go | 27 ++++++++ nomad/structs/cni_config_test.go | 25 +++++++ nomad/structs/structs.go | 14 ++++ nomad/structs/structs_test.go | 32 +++++++++ 10 files changed, 250 insertions(+), 2 deletions(-) create mode 100644 nomad/structs/cni_config.go create mode 100644 nomad/structs/cni_config_test.go diff --git a/api/resources.go b/api/resources.go index a6fe60d20eb..1682fb1be61 100644 --- a/api/resources.go +++ b/api/resources.go @@ -143,6 +143,9 @@ type DNSConfig struct { Searches []string `mapstructure:"searches" hcl:"searches,optional"` Options []string `mapstructure:"options" hcl:"options,optional"` } +type CNIArgs struct { + Args map[string]string `hcl:"args,optional"` +} // NetworkResource is used to describe required network // resources of a given task. @@ -160,7 +163,8 @@ type NetworkResource struct { // XXX Deprecated. Please do not use. The field will be removed in Nomad // 0.13 and is only being kept to allow any references to be removed before // then. - MBits *int `hcl:"mbits,optional"` + MBits *int `hcl:"mbits,optional"` + CNI *CNIArgs `hcl:"cni,block"` } // COMPAT(0.13) diff --git a/client/allocrunner/networking_cni.go b/client/allocrunner/networking_cni.go index ba967e92549..bbc119f26cc 100644 --- a/client/allocrunner/networking_cni.go +++ b/client/allocrunner/networking_cni.go @@ -12,6 +12,7 @@ import ( "context" "encoding/json" "fmt" + "github.com/hashicorp/go-set/v2" "math/rand" "os" "path/filepath" @@ -27,7 +28,6 @@ import ( "github.com/coreos/go-iptables/iptables" consulIPTables "github.com/hashicorp/consul/sdk/iptables" log "github.com/hashicorp/go-hclog" - "github.com/hashicorp/go-set/v2" "github.com/hashicorp/nomad/helper" "github.com/hashicorp/nomad/helper/envoy" "github.com/hashicorp/nomad/nomad/structs" @@ -102,6 +102,19 @@ const ( ConsulIPTablesConfigEnvVar = "CONSUL_IPTABLES_CONFIG" ) +// Adds user inputted custom CNI args to cniArgs map +func addCustomCNIArgs(tg *structs.TaskGroup, cniArgs map[string]string) { + for _, net := range tg.Networks { + if net.CNI == nil { + continue + } + for k, v := range net.CNI.Args { + cniArgs[k] = v + } + } + +} + // Setup calls the CNI plugins with the add action func (c *cniNetworkConfigurator) Setup(ctx context.Context, alloc *structs.Allocation, spec *drivers.NetworkIsolationSpec) (*structs.AllocNetworkStatus, error) { if err := c.ensureCNIInitialized(); err != nil { @@ -114,6 +127,10 @@ func (c *cniNetworkConfigurator) Setup(ctx context.Context, alloc *structs.Alloc "IgnoreUnknown": "true", } + tg := alloc.Job.LookupTaskGroup(alloc.TaskGroup) + + addCustomCNIArgs(tg, cniArgs) + portMaps := getPortMapping(alloc, c.ignorePortMappingHostIP) tproxyArgs, err := c.setupTransparentProxyArgs(alloc, spec, portMaps) diff --git a/client/allocrunner/networking_cni_test.go b/client/allocrunner/networking_cni_test.go index d58787d693a..be21d1f4c96 100644 --- a/client/allocrunner/networking_cni_test.go +++ b/client/allocrunner/networking_cni_test.go @@ -205,6 +205,87 @@ func TestCNI_cniToAllocNet_Invalid(t *testing.T) { require.Nil(t, allocNet) } +// TestCNI_addCustomCNIArgs fails if given a task group with a +// network resource including CNI Args calling addCustomCNIArgs does not +// result in the expected map of CNI Args created. +func TestCNI_addCustomCNIArgs(t *testing.T) { + ci.Parallel(t) + cniArgs := map[string]string{ + // CNI plugins are called one after the other with the same set of + // arguments. Passing IgnoreUnknown=true signals to plugins that they + // should ignore any arguments they don't understand + "IgnoreUnknown": "true", + } + alloc := mock.ConnectAlloc() + + alloc.AllocatedResources.Shared.Networks = []*structs.NetworkResource{ + { + Mode: "bridge", + CNI: &structs.CNIArgs{ + Args: map[string]string{ + "first_arg": "example", + "new_arg": "example_2", + }, + }, + }, + } + tg := alloc.Job.LookupTaskGroup(alloc.TaskGroup) + tg.Networks = []*structs.NetworkResource{{ + Mode: "bridge", + CNI: &structs.CNIArgs{ + Args: map[string]string{ + "first_arg": "example", + "new_arg": "example_2", + }, + }, + }} + addCustomCNIArgs(tg, cniArgs) + var actualMap = map[string]string{ + "IgnoreUnknown": "true", + "first_arg": "example", + "new_arg": "example_2", + } + test.Eq(t, actualMap, cniArgs) + +} + +// TestCNI_noCNIArgs fails if given a task group with a +// network resource with no CNI Args specified, calling addCustomCNIArgs does not +// result in a map with no args (except for "IgnoreUnknown" :"true"). +func TestCNI_noCNIArgs(t *testing.T) { + ci.Parallel(t) + + cniArgs := map[string]string{ + // CNI plugins are called one after the other with the same set of + // arguments. Passing IgnoreUnknown=true signals to plugins that they + // should ignore any arguments they don't understand + "IgnoreUnknown": "true", + } + alloc := mock.ConnectAlloc() + + alloc.AllocatedResources.Shared.Networks = []*structs.NetworkResource{ + { + Mode: "bridge", + CNI: &structs.CNIArgs{ + Args: map[string]string{ + "first_arg": "example", + "new_arg": "example_2", + }, + }, + }, + } + tg := alloc.Job.LookupTaskGroup(alloc.TaskGroup) + tg.Networks = []*structs.NetworkResource{{ + Mode: "bridge", + }} + addCustomCNIArgs(tg, cniArgs) + var actualMap = map[string]string{ + "IgnoreUnknown": "true", + } + test.Eq(t, actualMap, cniArgs) + +} + func TestCNI_setupTproxyArgs(t *testing.T) { ci.Parallel(t) diff --git a/client/taskenv/network.go b/client/taskenv/network.go index a59f96d3d91..51653855103 100644 --- a/client/taskenv/network.go +++ b/client/taskenv/network.go @@ -13,6 +13,7 @@ import ( // Current interoperable fields: // - Hostname // - DNS +// - CNI func InterpolateNetworks(taskEnv *TaskEnv, networks structs.Networks) structs.Networks { // Guard against not having a valid taskEnv. This can be the case if the @@ -32,6 +33,12 @@ func InterpolateNetworks(taskEnv *TaskEnv, networks structs.Networks) structs.Ne interpolated[i].DNS.Searches = taskEnv.ParseAndReplace(interpolated[i].DNS.Searches) interpolated[i].DNS.Options = taskEnv.ParseAndReplace(interpolated[i].DNS.Options) } + if interpolated[i].CNI != nil { + for k, v := range interpolated[i].CNI.Args { + interpolated[i].CNI.Args[k] = taskEnv.ReplaceEnv(v) + + } + } } return interpolated diff --git a/client/taskenv/network_test.go b/client/taskenv/network_test.go index a63fd5fef8f..6d1373fbeab 100644 --- a/client/taskenv/network_test.go +++ b/client/taskenv/network_test.go @@ -84,6 +84,42 @@ func Test_InterpolateNetworks(t *testing.T) { }, name: "interpolated dns servers", }, + { + inputTaskEnv: testEnv, + inputNetworks: structs.Networks{ + { + CNI: &structs.CNIArgs{ + Args: map[string]string{"static": "example"}, + }, + }, + }, + expectedOutputNetworks: structs.Networks{ + { + CNI: &structs.CNIArgs{ + Args: map[string]string{"static": "example"}, + }, + }, + }, + name: "non-interpolated cni args", + }, + { + inputTaskEnv: testEnv, + inputNetworks: structs.Networks{ + { + CNI: &structs.CNIArgs{ + Args: map[string]string{"static": "${foo}-opt"}, + }, + }, + }, + expectedOutputNetworks: structs.Networks{ + { + CNI: &structs.CNIArgs{ + Args: map[string]string{"static": "bar-opt"}, + }, + }, + }, + name: "interpolated cni args", + }, } for _, tc := range testCases { diff --git a/command/agent/job_endpoint.go b/command/agent/job_endpoint.go index b12427d9795..d6cb29e4181 100644 --- a/command/agent/job_endpoint.go +++ b/command/agent/job_endpoint.go @@ -1553,6 +1553,11 @@ func ApiNetworkResourceToStructs(in []*api.NetworkResource) []*structs.NetworkRe Options: nw.DNS.Options, } } + if nw.CNI != nil { + out[i].CNI = &structs.CNIArgs{ + Args: nw.CNI.Args, + } + } if l := len(nw.DynamicPorts); l != 0 { out[i].DynamicPorts = make([]structs.Port, l) diff --git a/nomad/structs/cni_config.go b/nomad/structs/cni_config.go new file mode 100644 index 00000000000..c6411437efb --- /dev/null +++ b/nomad/structs/cni_config.go @@ -0,0 +1,27 @@ +package structs + +import "reflect" + +type CNIArgs struct { + Args map[string]string +} + +func (d *CNIArgs) Copy() *CNIArgs { + if d == nil { + return nil + } + newMap := make(map[string]string) + for k, v := range d.Args { + newMap[k] = v + } + return &CNIArgs{ + Args: newMap, + } +} + +func (d *CNIArgs) Equal(o *CNIArgs) bool { + if d == nil || o == nil { + return d == o + } + return reflect.DeepEqual(d.Args, o.Args) +} diff --git a/nomad/structs/cni_config_test.go b/nomad/structs/cni_config_test.go new file mode 100644 index 00000000000..5e272448231 --- /dev/null +++ b/nomad/structs/cni_config_test.go @@ -0,0 +1,25 @@ +package structs + +import ( + "github.com/hashicorp/nomad/ci" + "github.com/shoenig/test/must" + "testing" +) + +func TestCNIConfig_Equal(t *testing.T) { + ci.Parallel(t) + + must.Equal[*CNIArgs](t, nil, nil) + must.NotEqual[*CNIArgs](t, nil, new(CNIArgs)) + must.NotEqual[*CNIArgs](t, nil, &CNIArgs{Args: map[string]string{"first": "second"}}) + + must.StructEqual(t, &CNIArgs{ + Args: map[string]string{ + "arg": "example_1", + "new_arg": "example_2", + }, + }, []must.Tweak[*CNIArgs]{{ + Field: "Args", + Apply: func(c *CNIArgs) { c.Args = map[string]string{"different": "arg"} }, + }}) +} diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index e0315654d1b..3f770a58cda 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -2872,6 +2872,7 @@ type NetworkResource struct { DNS *DNSConfig // DNS Configuration ReservedPorts []Port // Host Reserved ports DynamicPorts []Port // Host Dynamically assigned ports + CNI *CNIArgs //CNIArgs Configuration } func (n *NetworkResource) Hash() uint32 { @@ -7147,6 +7148,7 @@ func (tg *TaskGroup) validateNetworks() error { portLabels := make(map[string]string) // host_network -> static port tracking staticPortsIndex := make(map[string]map[int]string) + cniArgKeys := set.New[string](len(tg.Networks)) for _, net := range tg.Networks { for _, port := range append(net.ReservedPorts, net.DynamicPorts...) { @@ -7186,6 +7188,18 @@ func (tg *TaskGroup) validateNetworks() error { mErr.Errors = append(mErr.Errors, err) } } + // Validate the cniArgs in each network resource. Make sure there are no duplicate Args in + // different network resources + if net.CNI != nil { + for k, _ := range net.CNI.Args { + if cniArgKeys.Contains(k) { + err := fmt.Errorf("duplicate CNI arg %v", k) + mErr.Errors = append(mErr.Errors, err) + } else { + cniArgKeys.Insert(k) + } + } + } // Validate the hostname field to be a valid DNS name. If the parameter // looks like it includes an interpolation value, we skip this. It diff --git a/nomad/structs/structs_test.go b/nomad/structs/structs_test.go index 0c81555e78b..82b2ebcc74c 100644 --- a/nomad/structs/structs_test.go +++ b/nomad/structs/structs_test.go @@ -2015,6 +2015,36 @@ func TestTaskGroupNetwork_Validate(t *testing.T) { }, ErrContains: "Hostname is not a valid DNS name", }, + { + TG: &TaskGroup{ + Name: "testing-duplicate-cni-arg-keys", + Networks: []*NetworkResource{ + { + CNI: &CNIArgs{Args: map[string]string{"static": "first_value"}}, + }, + { + CNI: &CNIArgs{Args: map[string]string{"static": "new_value"}}, + }, + }, + }, + ErrContains: "duplicate CNI arg", + }, + { + TG: &TaskGroup{ + Name: "testing-valid-cni-arg-keys", + Networks: []*NetworkResource{ + { + CNI: &CNIArgs{Args: map[string]string{"static": "first_value"}}, + }, + { + CNI: &CNIArgs{Args: map[string]string{"new_key": "new_value"}}, + }, + { + CNI: &CNIArgs{Args: map[string]string{"newest_key": "new_value", "second_key": "second_value"}}, + }, + }, + }, + }, } for i := range cases { @@ -2186,6 +2216,7 @@ func TestTask_Validate_Resources(t *testing.T) { HostNetwork: "loopback", }, }, + CNI: &CNIArgs{map[string]string{"static": "new_val"}}, }, }, }, @@ -2318,6 +2349,7 @@ func TestNetworkResource_Copy(t *testing.T) { HostNetwork: "public", }, }, + CNI: &CNIArgs{Args: map[string]string{"foo": "bar", "hello": "world"}}, }, name: "fully populated input check", }, From ada45781445563039eb820587d6a375a513d8fe1 Mon Sep 17 00:00:00 2001 From: Martina Santangelo Date: Wed, 10 Jul 2024 12:47:27 -0400 Subject: [PATCH 2/6] cni: allow users to see diff when cni block is edited --- nomad/structs/diff.go | 19 +++++++++ nomad/structs/diff_test.go | 80 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+) diff --git a/nomad/structs/diff.go b/nomad/structs/diff.go index 5b75d8cb9bb..91953da84d7 100644 --- a/nomad/structs/diff.go +++ b/nomad/structs/diff.go @@ -2663,6 +2663,10 @@ func (n *NetworkResource) Diff(other *NetworkResource, contextual bool) *ObjectD diff.Objects = append(diff.Objects, dnsDiff) } + if cniDiff := n.CNI.Diff(other.CNI, contextual); cniDiff != nil { + diff.Objects = append(diff.Objects, cniDiff) + } + return diff } @@ -2706,6 +2710,21 @@ func (d *DNSConfig) Diff(other *DNSConfig, contextual bool) *ObjectDiff { return diff } +// Diff returns a diff of two CNIArgs structs +func (d *CNIArgs) Diff(other *CNIArgs, contextual bool) *ObjectDiff { + if d.Equal(other) { + return nil + } + if d == nil { + d = &CNIArgs{} + } + if other == nil { + other = &CNIArgs{} + } + + return primitiveObjectDiff(d.Args, other.Args, nil, "CNIArgs", contextual) +} + func disconectStrategyDiffs(old, new *DisconnectStrategy, contextual bool) *ObjectDiff { diff := &ObjectDiff{Type: DiffTypeNone, Name: "Disconnect"} var oldDisconnectFlat, newDisconnectFlat map[string]string diff --git a/nomad/structs/diff_test.go b/nomad/structs/diff_test.go index 15b095889d1..227f4e6ea7b 100644 --- a/nomad/structs/diff_test.go +++ b/nomad/structs/diff_test.go @@ -5221,6 +5221,86 @@ func TestTaskGroupDiff(t *testing.T) { }, }, }, + {TestCase: "TaskGroup CNI edited", + Contextual: false, + Old: &TaskGroup{ + Networks: Networks{ + { + DNS: &DNSConfig{ + Servers: []string{"1.1.1.1"}, + }, + }, + }, + }, + New: &TaskGroup{ + Networks: Networks{ + { + DNS: &DNSConfig{ + Servers: []string{"1.1.1.1"}, + }, + CNI: &CNIArgs{ + Args: map[string]string{ + "example": "example1", + }, + }, + }, + }, + }, + Expected: &TaskGroupDiff{ + Type: DiffTypeEdited, + Objects: []*ObjectDiff{ + { + Type: DiffTypeAdded, + Name: "Network", + Objects: []*ObjectDiff{ + + { + Type: DiffTypeAdded, + Name: "DNS", + Fields: []*FieldDiff{ + { + Type: DiffTypeAdded, + Name: "Servers", + Old: "", + New: "1.1.1.1", + }, + }, + }, + { + Type: DiffTypeAdded, + Name: "CNIArgs", + Fields: []*FieldDiff{ + { + Type: DiffTypeAdded, + Name: "example", + Old: "", + New: "example1", + }, + }, + }, + }, + }, + { + Type: DiffTypeDeleted, + Name: "Network", + Objects: []*ObjectDiff{ + { + Type: DiffTypeDeleted, + Name: "DNS", + Fields: []*FieldDiff{ + { + Type: DiffTypeDeleted, + Name: "Servers", + Old: "1.1.1.1", + New: "", + }, + }, + }, + }, + }, + }, + }, + }, } for i, c := range cases { From fabb11d63fc1b64efb9938516d35941ebbf77ccf Mon Sep 17 00:00:00 2001 From: Martina Santangelo Date: Wed, 10 Jul 2024 17:03:53 -0400 Subject: [PATCH 3/6] add extra stuff to cni --- client/allocrunner/networking_cni.go | 9 ++- client/allocrunner/networking_cni_test.go | 67 +++++------------------ nomad/structs/cni_config.go | 3 + nomad/structs/cni_config_test.go | 3 + nomad/structs/structs.go | 2 +- 5 files changed, 25 insertions(+), 59 deletions(-) diff --git a/client/allocrunner/networking_cni.go b/client/allocrunner/networking_cni.go index bbc119f26cc..a15a336204f 100644 --- a/client/allocrunner/networking_cni.go +++ b/client/allocrunner/networking_cni.go @@ -12,7 +12,6 @@ import ( "context" "encoding/json" "fmt" - "github.com/hashicorp/go-set/v2" "math/rand" "os" "path/filepath" @@ -28,6 +27,7 @@ import ( "github.com/coreos/go-iptables/iptables" consulIPTables "github.com/hashicorp/consul/sdk/iptables" log "github.com/hashicorp/go-hclog" + "github.com/hashicorp/go-set/v2" "github.com/hashicorp/nomad/helper" "github.com/hashicorp/nomad/helper/envoy" "github.com/hashicorp/nomad/nomad/structs" @@ -103,8 +103,8 @@ const ( ) // Adds user inputted custom CNI args to cniArgs map -func addCustomCNIArgs(tg *structs.TaskGroup, cniArgs map[string]string) { - for _, net := range tg.Networks { +func addCustomCNIArgs(networks []*structs.NetworkResource, cniArgs map[string]string) { + for _, net := range networks { if net.CNI == nil { continue } @@ -112,7 +112,6 @@ func addCustomCNIArgs(tg *structs.TaskGroup, cniArgs map[string]string) { cniArgs[k] = v } } - } // Setup calls the CNI plugins with the add action @@ -129,7 +128,7 @@ func (c *cniNetworkConfigurator) Setup(ctx context.Context, alloc *structs.Alloc tg := alloc.Job.LookupTaskGroup(alloc.TaskGroup) - addCustomCNIArgs(tg, cniArgs) + addCustomCNIArgs(tg.Networks, cniArgs) portMaps := getPortMapping(alloc, c.ignorePortMappingHostIP) diff --git a/client/allocrunner/networking_cni_test.go b/client/allocrunner/networking_cni_test.go index be21d1f4c96..84194f95a49 100644 --- a/client/allocrunner/networking_cni_test.go +++ b/client/allocrunner/networking_cni_test.go @@ -205,33 +205,13 @@ func TestCNI_cniToAllocNet_Invalid(t *testing.T) { require.Nil(t, allocNet) } -// TestCNI_addCustomCNIArgs fails if given a task group with a -// network resource including CNI Args calling addCustomCNIArgs does not -// result in the expected map of CNI Args created. func TestCNI_addCustomCNIArgs(t *testing.T) { ci.Parallel(t) cniArgs := map[string]string{ - // CNI plugins are called one after the other with the same set of - // arguments. Passing IgnoreUnknown=true signals to plugins that they - // should ignore any arguments they don't understand - "IgnoreUnknown": "true", + "default": "yup", } - alloc := mock.ConnectAlloc() - alloc.AllocatedResources.Shared.Networks = []*structs.NetworkResource{ - { - Mode: "bridge", - CNI: &structs.CNIArgs{ - Args: map[string]string{ - "first_arg": "example", - "new_arg": "example_2", - }, - }, - }, - } - tg := alloc.Job.LookupTaskGroup(alloc.TaskGroup) - tg.Networks = []*structs.NetworkResource{{ - Mode: "bridge", + network := []*structs.NetworkResource{{ CNI: &structs.CNIArgs{ Args: map[string]string{ "first_arg": "example", @@ -239,50 +219,31 @@ func TestCNI_addCustomCNIArgs(t *testing.T) { }, }, }} - addCustomCNIArgs(tg, cniArgs) - var actualMap = map[string]string{ - "IgnoreUnknown": "true", - "first_arg": "example", - "new_arg": "example_2", + addCustomCNIArgs(network, cniArgs) + var expectMap = map[string]string{ + "default": "yup", + "first_arg": "example", + "new_arg": "example_2", } - test.Eq(t, actualMap, cniArgs) + test.Eq(t, expectMap, cniArgs) } -// TestCNI_noCNIArgs fails if given a task group with a -// network resource with no CNI Args specified, calling addCustomCNIArgs does not -// result in a map with no args (except for "IgnoreUnknown" :"true"). func TestCNI_noCNIArgs(t *testing.T) { ci.Parallel(t) cniArgs := map[string]string{ - // CNI plugins are called one after the other with the same set of - // arguments. Passing IgnoreUnknown=true signals to plugins that they - // should ignore any arguments they don't understand - "IgnoreUnknown": "true", + "default": "yup", } - alloc := mock.ConnectAlloc() - alloc.AllocatedResources.Shared.Networks = []*structs.NetworkResource{ - { - Mode: "bridge", - CNI: &structs.CNIArgs{ - Args: map[string]string{ - "first_arg": "example", - "new_arg": "example_2", - }, - }, - }, - } - tg := alloc.Job.LookupTaskGroup(alloc.TaskGroup) - tg.Networks = []*structs.NetworkResource{{ + network := []*structs.NetworkResource{{ Mode: "bridge", }} - addCustomCNIArgs(tg, cniArgs) - var actualMap = map[string]string{ - "IgnoreUnknown": "true", + addCustomCNIArgs(network, cniArgs) + var expectMap = map[string]string{ + "default": "yup", } - test.Eq(t, actualMap, cniArgs) + test.Eq(t, expectMap, cniArgs) } diff --git a/nomad/structs/cni_config.go b/nomad/structs/cni_config.go index c6411437efb..0e5f4abd2a8 100644 --- a/nomad/structs/cni_config.go +++ b/nomad/structs/cni_config.go @@ -1,3 +1,6 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: BUSL-1.1 + package structs import "reflect" diff --git a/nomad/structs/cni_config_test.go b/nomad/structs/cni_config_test.go index 5e272448231..3b33d35d1a4 100644 --- a/nomad/structs/cni_config_test.go +++ b/nomad/structs/cni_config_test.go @@ -1,3 +1,6 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: BUSL-1.1 + package structs import ( diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 3f770a58cda..f9b3b2ffd6a 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -7191,7 +7191,7 @@ func (tg *TaskGroup) validateNetworks() error { // Validate the cniArgs in each network resource. Make sure there are no duplicate Args in // different network resources if net.CNI != nil { - for k, _ := range net.CNI.Args { + for k := range net.CNI.Args { if cniArgKeys.Contains(k) { err := fmt.Errorf("duplicate CNI arg %v", k) mErr.Errors = append(mErr.Errors, err) From 988a56f821ca4e5e7cae9106ea2861e422737424 Mon Sep 17 00:00:00 2001 From: Martina Santangelo Date: Thu, 11 Jul 2024 11:48:23 -0400 Subject: [PATCH 4/6] add docs for cni args --- .../docs/job-specification/network.mdx | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/website/content/docs/job-specification/network.mdx b/website/content/docs/job-specification/network.mdx index 8ce2bce7c46..3136567d8db 100644 --- a/website/content/docs/job-specification/network.mdx +++ b/website/content/docs/job-specification/network.mdx @@ -89,6 +89,8 @@ All other operating systems use the `host` networking mode. 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. +- `cni` ([CNIConfig](#cni-parameters): nil) - Sets the custom CNI + arguments for a network configuration per allocation, for use with `mode="cni/*`. ### `port` Parameters @@ -128,6 +130,14 @@ The label of the port is just text - it has no special meaning to Nomad. These parameters support [interpolation](/nomad/docs/runtime/interpolation). +## `cni` Parameters + +- `args` `(map: nil)` - Sets CNI arguments for network configuration. + These get turned into `CNI_ARGS` per the + [CNI spec](https://www.cni.dev/docs/spec/#parameters). + +These parameters support [interpolation](/nomad/docs/runtime/interpolation). + ## `network` Examples The following examples only show the `network` blocks. Remember that the @@ -294,6 +304,24 @@ network { The Nomad client will build the correct [capabilities arguments](https://github.com/containernetworking/cni/blob/v0.8.0/CONVENTIONS.md#well-known-capabilities) for the portmap plugin based on the defined port blocks. +### CNI Args + +The following example specifies CNI args for the custom CNI plugin specified above. + +```hcl +network { + mode = "cni/mynet" + port "http" { + to = 8080 + } + cni { + args = { + "nomad.region" : "${node.region}" + } + } +} +``` + ### Host Networks In some cases a port should only be allocated to a specific interface or address on the host. From 4c36858724d7c628e27b0e18eefb5b239a268a23 Mon Sep 17 00:00:00 2001 From: Martina Santangelo Date: Thu, 11 Jul 2024 15:27:57 -0400 Subject: [PATCH 5/6] fix cni field and tests --- api/resources.go | 6 +- client/allocrunner/networking_cni_test.go | 53 ++++--- client/taskenv/network_test.go | 34 ++-- command/agent/job_endpoint.go | 2 +- nomad/structs/cni_config.go | 8 +- nomad/structs/cni_config_test.go | 12 +- nomad/structs/diff.go | 10 +- nomad/structs/diff_test.go | 183 ++++++++++++++++++++-- nomad/structs/structs.go | 4 +- nomad/structs/structs_test.go | 14 +- 10 files changed, 242 insertions(+), 84 deletions(-) diff --git a/api/resources.go b/api/resources.go index 1682fb1be61..5e1adce500e 100644 --- a/api/resources.go +++ b/api/resources.go @@ -143,7 +143,7 @@ type DNSConfig struct { Searches []string `mapstructure:"searches" hcl:"searches,optional"` Options []string `mapstructure:"options" hcl:"options,optional"` } -type CNIArgs struct { +type CNIConfig struct { Args map[string]string `hcl:"args,optional"` } @@ -163,8 +163,8 @@ type NetworkResource struct { // XXX Deprecated. Please do not use. The field will be removed in Nomad // 0.13 and is only being kept to allow any references to be removed before // then. - MBits *int `hcl:"mbits,optional"` - CNI *CNIArgs `hcl:"cni,block"` + MBits *int `hcl:"mbits,optional"` + CNI *CNIConfig `hcl:"cni,block"` } // COMPAT(0.13) diff --git a/client/allocrunner/networking_cni_test.go b/client/allocrunner/networking_cni_test.go index 84194f95a49..31b4452aea3 100644 --- a/client/allocrunner/networking_cni_test.go +++ b/client/allocrunner/networking_cni_test.go @@ -211,40 +211,45 @@ func TestCNI_addCustomCNIArgs(t *testing.T) { "default": "yup", } - network := []*structs.NetworkResource{{ - CNI: &structs.CNIArgs{ + networkWithArgs := []*structs.NetworkResource{{ + CNI: &structs.CNIConfig{ Args: map[string]string{ "first_arg": "example", "new_arg": "example_2", }, }, }} - addCustomCNIArgs(network, cniArgs) - var expectMap = map[string]string{ - "default": "yup", - "first_arg": "example", - "new_arg": "example_2", - } - test.Eq(t, expectMap, cniArgs) - -} - -func TestCNI_noCNIArgs(t *testing.T) { - ci.Parallel(t) - - cniArgs := map[string]string{ - "default": "yup", - } - - network := []*structs.NetworkResource{{ + networkWithoutArgs := []*structs.NetworkResource{{ Mode: "bridge", }} - addCustomCNIArgs(network, cniArgs) - var expectMap = map[string]string{ - "default": "yup", + testCases := []struct { + name string + network []*structs.NetworkResource + expectMap map[string]string + }{ + { + name: "cni args not specified", + network: networkWithoutArgs, + expectMap: map[string]string{ + "default": "yup", + }, + }, { + name: "cni args specified", + network: networkWithArgs, + expectMap: map[string]string{ + "default": "yup", + "first_arg": "example", + "new_arg": "example_2", + }, + }, } - test.Eq(t, expectMap, cniArgs) + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + addCustomCNIArgs(tc.network, cniArgs) + test.Eq(t, tc.expectMap, cniArgs) + }) + } } func TestCNI_setupTproxyArgs(t *testing.T) { diff --git a/client/taskenv/network_test.go b/client/taskenv/network_test.go index 6d1373fbeab..5ff3016c1a1 100644 --- a/client/taskenv/network_test.go +++ b/client/taskenv/network_test.go @@ -88,37 +88,25 @@ func Test_InterpolateNetworks(t *testing.T) { inputTaskEnv: testEnv, inputNetworks: structs.Networks{ { - CNI: &structs.CNIArgs{ - Args: map[string]string{"static": "example"}, + CNI: &structs.CNIConfig{ + Args: map[string]string{ + "static": "example", + "second": "${foo}-opt", + }, }, }, }, expectedOutputNetworks: structs.Networks{ { - CNI: &structs.CNIArgs{ - Args: map[string]string{"static": "example"}, + CNI: &structs.CNIConfig{ + Args: map[string]string{ + "static": "example", + "second": "bar-opt", + }, }, }, }, - name: "non-interpolated cni args", - }, - { - inputTaskEnv: testEnv, - inputNetworks: structs.Networks{ - { - CNI: &structs.CNIArgs{ - Args: map[string]string{"static": "${foo}-opt"}, - }, - }, - }, - expectedOutputNetworks: structs.Networks{ - { - CNI: &structs.CNIArgs{ - Args: map[string]string{"static": "bar-opt"}, - }, - }, - }, - name: "interpolated cni args", + name: "interpolated and non-interpolated cni args", }, } diff --git a/command/agent/job_endpoint.go b/command/agent/job_endpoint.go index d6cb29e4181..a2fd087699b 100644 --- a/command/agent/job_endpoint.go +++ b/command/agent/job_endpoint.go @@ -1554,7 +1554,7 @@ func ApiNetworkResourceToStructs(in []*api.NetworkResource) []*structs.NetworkRe } } if nw.CNI != nil { - out[i].CNI = &structs.CNIArgs{ + out[i].CNI = &structs.CNIConfig{ Args: nw.CNI.Args, } } diff --git a/nomad/structs/cni_config.go b/nomad/structs/cni_config.go index 0e5f4abd2a8..72a8c0a1577 100644 --- a/nomad/structs/cni_config.go +++ b/nomad/structs/cni_config.go @@ -5,11 +5,11 @@ package structs import "reflect" -type CNIArgs struct { +type CNIConfig struct { Args map[string]string } -func (d *CNIArgs) Copy() *CNIArgs { +func (d *CNIConfig) Copy() *CNIConfig { if d == nil { return nil } @@ -17,12 +17,12 @@ func (d *CNIArgs) Copy() *CNIArgs { for k, v := range d.Args { newMap[k] = v } - return &CNIArgs{ + return &CNIConfig{ Args: newMap, } } -func (d *CNIArgs) Equal(o *CNIArgs) bool { +func (d *CNIConfig) Equal(o *CNIConfig) bool { if d == nil || o == nil { return d == o } diff --git a/nomad/structs/cni_config_test.go b/nomad/structs/cni_config_test.go index 3b33d35d1a4..e7cf78b8819 100644 --- a/nomad/structs/cni_config_test.go +++ b/nomad/structs/cni_config_test.go @@ -12,17 +12,17 @@ import ( func TestCNIConfig_Equal(t *testing.T) { ci.Parallel(t) - must.Equal[*CNIArgs](t, nil, nil) - must.NotEqual[*CNIArgs](t, nil, new(CNIArgs)) - must.NotEqual[*CNIArgs](t, nil, &CNIArgs{Args: map[string]string{"first": "second"}}) + must.Equal[*CNIConfig](t, nil, nil) + must.NotEqual[*CNIConfig](t, nil, new(CNIConfig)) + must.NotEqual[*CNIConfig](t, nil, &CNIConfig{Args: map[string]string{"first": "second"}}) - must.StructEqual(t, &CNIArgs{ + must.StructEqual(t, &CNIConfig{ Args: map[string]string{ "arg": "example_1", "new_arg": "example_2", }, - }, []must.Tweak[*CNIArgs]{{ + }, []must.Tweak[*CNIConfig]{{ Field: "Args", - Apply: func(c *CNIArgs) { c.Args = map[string]string{"different": "arg"} }, + Apply: func(c *CNIConfig) { c.Args = map[string]string{"different": "arg"} }, }}) } diff --git a/nomad/structs/diff.go b/nomad/structs/diff.go index 91953da84d7..de8a8d45126 100644 --- a/nomad/structs/diff.go +++ b/nomad/structs/diff.go @@ -2710,19 +2710,19 @@ func (d *DNSConfig) Diff(other *DNSConfig, contextual bool) *ObjectDiff { return diff } -// Diff returns a diff of two CNIArgs structs -func (d *CNIArgs) Diff(other *CNIArgs, contextual bool) *ObjectDiff { +// Diff returns a diff of two CNIConfig structs +func (d *CNIConfig) Diff(other *CNIConfig, contextual bool) *ObjectDiff { if d.Equal(other) { return nil } if d == nil { - d = &CNIArgs{} + d = &CNIConfig{} } if other == nil { - other = &CNIArgs{} + other = &CNIConfig{} } - return primitiveObjectDiff(d.Args, other.Args, nil, "CNIArgs", contextual) + return primitiveObjectDiff(d.Args, other.Args, nil, "CNIConfig", contextual) } func disconectStrategyDiffs(old, new *DisconnectStrategy, contextual bool) *ObjectDiff { diff --git a/nomad/structs/diff_test.go b/nomad/structs/diff_test.go index 227f4e6ea7b..71b91061f05 100644 --- a/nomad/structs/diff_test.go +++ b/nomad/structs/diff_test.go @@ -5221,7 +5221,157 @@ func TestTaskGroupDiff(t *testing.T) { }, }, }, + {TestCase: "TaskGroup CNI added", + Contextual: false, + Old: &TaskGroup{ + Networks: Networks{}, + }, + New: &TaskGroup{ + Networks: Networks{ + { + CNI: &CNIConfig{ + Args: map[string]string{ + "example": "example1", + }, + }, + }, + }, + }, + Expected: &TaskGroupDiff{ + Type: DiffTypeEdited, + Objects: []*ObjectDiff{ + { + Type: DiffTypeAdded, + Name: "Network", + Objects: []*ObjectDiff{ + + { + Type: DiffTypeAdded, + Name: "CNIConfig", + Fields: []*FieldDiff{ + { + Type: DiffTypeAdded, + Name: "example", + Old: "", + New: "example1", + }, + }, + }, + }, + }, + }, + }, + }, + {TestCase: "TaskGroup CNI deleted", + Contextual: false, + Old: &TaskGroup{ + Networks: Networks{ + { + CNI: &CNIConfig{ + Args: map[string]string{ + "example": "example1", + }, + }, + }, + }, + }, + New: &TaskGroup{ + Networks: Networks{}, + }, + Expected: &TaskGroupDiff{ + Type: DiffTypeEdited, + Objects: []*ObjectDiff{ + { + Type: DiffTypeDeleted, + Name: "Network", + Objects: []*ObjectDiff{ + + { + Type: DiffTypeDeleted, + Name: "CNIConfig", + Fields: []*FieldDiff{ + { + Type: DiffTypeDeleted, + Name: "example", + Old: "example1", + New: "", + }, + }, + }, + }, + }, + }, + }, + }, {TestCase: "TaskGroup CNI edited", + Contextual: false, + Old: &TaskGroup{ + Networks: Networks{ + { + CNI: &CNIConfig{ + Args: map[string]string{ + "example": "example1", + }, + }, + }, + }, + }, + New: &TaskGroup{ + Networks: Networks{ + { + CNI: &CNIConfig{ + Args: map[string]string{ + "example": "example2", + }, + }, + }, + }, + }, + Expected: &TaskGroupDiff{ + Type: DiffTypeEdited, + Objects: []*ObjectDiff{ + { + Type: DiffTypeAdded, + Name: "Network", + Objects: []*ObjectDiff{ + + { + Type: DiffTypeAdded, + Name: "CNIConfig", + Fields: []*FieldDiff{ + { + Type: DiffTypeAdded, + Name: "example", + Old: "", + New: "example2", + }, + }, + }, + }, + }, + { + Type: DiffTypeDeleted, + Name: "Network", + Objects: []*ObjectDiff{ + + { + Type: DiffTypeDeleted, + Name: "CNIConfig", + Fields: []*FieldDiff{ + { + Type: DiffTypeDeleted, + Name: "example", + Old: "example1", + New: "", + }, + }, + }, + }, + }, + }, + }, + }, + {TestCase: "TaskGroup Networks edited behavior", Contextual: false, Old: &TaskGroup{ Networks: Networks{ @@ -5238,9 +5388,11 @@ func TestTaskGroupDiff(t *testing.T) { DNS: &DNSConfig{ Servers: []string{"1.1.1.1"}, }, - CNI: &CNIArgs{ - Args: map[string]string{ - "example": "example1", + DynamicPorts: []Port{ + { + Label: "bar", + To: 8081, + HostNetwork: "public", }, }, }, @@ -5256,25 +5408,37 @@ func TestTaskGroupDiff(t *testing.T) { { Type: DiffTypeAdded, - Name: "DNS", + Name: "Dynamic Port", Fields: []*FieldDiff{ { Type: DiffTypeAdded, - Name: "Servers", + Name: "HostNetwork", Old: "", - New: "1.1.1.1", + New: "public", + }, + { + Type: DiffTypeAdded, + Name: "Label", + Old: "", + New: "bar", + }, + { + Type: DiffTypeAdded, + Name: "To", + Old: "", + New: "8081", }, }, }, { Type: DiffTypeAdded, - Name: "CNIArgs", + Name: "DNS", Fields: []*FieldDiff{ { Type: DiffTypeAdded, - Name: "example", + Name: "Servers", Old: "", - New: "example1", + New: "1.1.1.1", }, }, }, @@ -5284,6 +5448,7 @@ func TestTaskGroupDiff(t *testing.T) { Type: DiffTypeDeleted, Name: "Network", Objects: []*ObjectDiff{ + { Type: DiffTypeDeleted, Name: "DNS", diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index f9b3b2ffd6a..08ed59a745d 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -2872,7 +2872,7 @@ type NetworkResource struct { DNS *DNSConfig // DNS Configuration ReservedPorts []Port // Host Reserved ports DynamicPorts []Port // Host Dynamically assigned ports - CNI *CNIArgs //CNIArgs Configuration + CNI *CNIConfig // CNIConfig Configuration } func (n *NetworkResource) Hash() uint32 { @@ -7193,7 +7193,7 @@ func (tg *TaskGroup) validateNetworks() error { if net.CNI != nil { for k := range net.CNI.Args { if cniArgKeys.Contains(k) { - err := fmt.Errorf("duplicate CNI arg %v", k) + err := fmt.Errorf("duplicate CNI arg %q", k) mErr.Errors = append(mErr.Errors, err) } else { cniArgKeys.Insert(k) diff --git a/nomad/structs/structs_test.go b/nomad/structs/structs_test.go index 82b2ebcc74c..d63538fc6b6 100644 --- a/nomad/structs/structs_test.go +++ b/nomad/structs/structs_test.go @@ -2020,10 +2020,10 @@ func TestTaskGroupNetwork_Validate(t *testing.T) { Name: "testing-duplicate-cni-arg-keys", Networks: []*NetworkResource{ { - CNI: &CNIArgs{Args: map[string]string{"static": "first_value"}}, + CNI: &CNIConfig{Args: map[string]string{"static": "first_value"}}, }, { - CNI: &CNIArgs{Args: map[string]string{"static": "new_value"}}, + CNI: &CNIConfig{Args: map[string]string{"static": "new_value"}}, }, }, }, @@ -2034,13 +2034,13 @@ func TestTaskGroupNetwork_Validate(t *testing.T) { Name: "testing-valid-cni-arg-keys", Networks: []*NetworkResource{ { - CNI: &CNIArgs{Args: map[string]string{"static": "first_value"}}, + CNI: &CNIConfig{Args: map[string]string{"static": "first_value"}}, }, { - CNI: &CNIArgs{Args: map[string]string{"new_key": "new_value"}}, + CNI: &CNIConfig{Args: map[string]string{"new_key": "new_value"}}, }, { - CNI: &CNIArgs{Args: map[string]string{"newest_key": "new_value", "second_key": "second_value"}}, + CNI: &CNIConfig{Args: map[string]string{"newest_key": "new_value", "second_key": "second_value"}}, }, }, }, @@ -2216,7 +2216,7 @@ func TestTask_Validate_Resources(t *testing.T) { HostNetwork: "loopback", }, }, - CNI: &CNIArgs{map[string]string{"static": "new_val"}}, + CNI: &CNIConfig{map[string]string{"static": "new_val"}}, }, }, }, @@ -2349,7 +2349,7 @@ func TestNetworkResource_Copy(t *testing.T) { HostNetwork: "public", }, }, - CNI: &CNIArgs{Args: map[string]string{"foo": "bar", "hello": "world"}}, + CNI: &CNIConfig{Args: map[string]string{"foo": "bar", "hello": "world"}}, }, name: "fully populated input check", }, From 6689a15e0abc394e6c23d01fd1a28c70cea7fd8e Mon Sep 17 00:00:00 2001 From: Martina Santangelo Date: Fri, 12 Jul 2024 11:17:29 -0400 Subject: [PATCH 6/6] PR comments, changelog --- .changelog/23538.txt | 3 +++ nomad/structs/cni_config.go | 6 ++++-- nomad/structs/diff.go | 6 +++--- nomad/structs/diff_test.go | 2 +- nomad/structs/structs.go | 12 ++++++++++-- nomad/structs/structs_test.go | 22 ++++++++++++++++++++++ 6 files changed, 43 insertions(+), 8 deletions(-) create mode 100644 .changelog/23538.txt diff --git a/.changelog/23538.txt b/.changelog/23538.txt new file mode 100644 index 00000000000..0db0b224238 --- /dev/null +++ b/.changelog/23538.txt @@ -0,0 +1,3 @@ +```release-note:improvement +cni: allow users to input CNI args in job specification +``` diff --git a/nomad/structs/cni_config.go b/nomad/structs/cni_config.go index 72a8c0a1577..e821ebef13b 100644 --- a/nomad/structs/cni_config.go +++ b/nomad/structs/cni_config.go @@ -3,7 +3,9 @@ package structs -import "reflect" +import ( + "maps" +) type CNIConfig struct { Args map[string]string @@ -26,5 +28,5 @@ func (d *CNIConfig) Equal(o *CNIConfig) bool { if d == nil || o == nil { return d == o } - return reflect.DeepEqual(d.Args, o.Args) + return maps.Equal(d.Args, o.Args) } diff --git a/nomad/structs/diff.go b/nomad/structs/diff.go index de8a8d45126..c00f150983d 100644 --- a/nomad/structs/diff.go +++ b/nomad/structs/diff.go @@ -2712,15 +2712,15 @@ func (d *DNSConfig) Diff(other *DNSConfig, contextual bool) *ObjectDiff { // Diff returns a diff of two CNIConfig structs func (d *CNIConfig) Diff(other *CNIConfig, contextual bool) *ObjectDiff { - if d.Equal(other) { - return nil - } if d == nil { d = &CNIConfig{} } if other == nil { other = &CNIConfig{} } + if d.Equal(other) { + return nil + } return primitiveObjectDiff(d.Args, other.Args, nil, "CNIConfig", contextual) } diff --git a/nomad/structs/diff_test.go b/nomad/structs/diff_test.go index 71b91061f05..d24b4b6af9e 100644 --- a/nomad/structs/diff_test.go +++ b/nomad/structs/diff_test.go @@ -5371,7 +5371,7 @@ func TestTaskGroupDiff(t *testing.T) { }, }, }, - {TestCase: "TaskGroup Networks edited behavior", + {TestCase: "Editing Networks deletes and re-adds", Contextual: false, Old: &TaskGroup{ Networks: Networks{ diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 08ed59a745d..ebd81514fb6 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -7189,15 +7189,23 @@ func (tg *TaskGroup) validateNetworks() error { } } // Validate the cniArgs in each network resource. Make sure there are no duplicate Args in - // different network resources + // different network resources or illegal characters (;) in key or value ;) if net.CNI != nil { - for k := range net.CNI.Args { + for k, v := range net.CNI.Args { if cniArgKeys.Contains(k) { err := fmt.Errorf("duplicate CNI arg %q", k) mErr.Errors = append(mErr.Errors, err) } else { cniArgKeys.Insert(k) } + if strings.Contains(k, ";") { + err := fmt.Errorf("invalid ';' character in CNI arg key %q", k) + mErr.Errors = append(mErr.Errors, err) + } + if strings.Contains(v, ";") { + err := fmt.Errorf("invalid ';' character in CNI arg value %q", v) + mErr.Errors = append(mErr.Errors, err) + } } } diff --git a/nomad/structs/structs_test.go b/nomad/structs/structs_test.go index d63538fc6b6..acc17ec1a18 100644 --- a/nomad/structs/structs_test.go +++ b/nomad/structs/structs_test.go @@ -2045,6 +2045,28 @@ func TestTaskGroupNetwork_Validate(t *testing.T) { }, }, }, + { + TG: &TaskGroup{ + Name: "testing-invalid-character-cni-arg-key", + Networks: []*NetworkResource{ + { + CNI: &CNIConfig{Args: map[string]string{"static;": "first_value"}}, + }, + }, + }, + ErrContains: "invalid ';' character in CNI arg key \"static;", + }, + { + TG: &TaskGroup{ + Name: "testing-invalid-character-cni-arg-value", + Networks: []*NetworkResource{ + { + CNI: &CNIConfig{Args: map[string]string{"static": "first_value;"}}, + }, + }, + }, + ErrContains: "invalid ';' character in CNI arg value \"first_value;", + }, } for i := range cases {