Skip to content

Commit eeae2a0

Browse files
mnasermnaser
committed
feat(plugin): support reading OvnPort from args.cni
Add support for reading OvnPort and MAC from args.cni in the CNI configuration, following the CNI conventions specification. Multus and other CNI meta-plugins pass runtime arguments via the args.cni field in the network configuration rather than the CNI_ARGS environment variable. This change enables ovs-cni to work properly when invoked through Multus with cni-args specified in the NetworkAttachmentDefinition. Per the CNI spec, args.cni takes precedence over the deprecated CNI_ARGS environment variable when both are present. This ensures consistent behavior with the specification while maintaining backward compatibility with existing CNI_ARGS users. Changes: - Add CNIArgs struct and Args field to NetConf in types.go - Update CmdAdd and CmdDel to check args.cni for OvnPort/MAC - Add integration tests for args.cni OvnPort handling - Add unit tests for JSON parsing of args.cni Signed-off-by: Mohammed Naser <mnaser@vexxhost.com>
1 parent 45bdf0b commit eeae2a0

File tree

4 files changed

+208
-0
lines changed

4 files changed

+208
-0
lines changed

‎pkg/plugin/plugin.go‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -277,6 +277,17 @@ func CmdAdd(args *skel.CmdArgs) error {
277277
return err
278278
}
279279

280+
// Check for OvnPort and MAC in args.cni (CNI convention)
281+
// args.cni takes precedence over CNI_ARGS per CNI spec
282+
if netconf.Args != nil && netconf.Args.CNI != nil {
283+
if netconf.Args.CNI.OvnPort != "" {
284+
ovnPort = netconf.Args.CNI.OvnPort
285+
}
286+
if netconf.Args.CNI.MAC != "" {
287+
mac = netconf.Args.CNI.MAC
288+
}
289+
}
290+
280291
var vlanTagNum uint = 0
281292
trunks := make([]uint, 0)
282293
portType := "access"
@@ -558,6 +569,12 @@ func CmdDel(args *skel.CmdArgs) error {
558569
if envArgs != nil {
559570
ovnPort = string(envArgs.OvnPort)
560571
}
572+
// Check for OvnPort in args.cni (CNI convention) - takes precedence
573+
if cache.Netconf.Args != nil && cache.Netconf.Args.CNI != nil {
574+
if cache.Netconf.Args.CNI.OvnPort != "" {
575+
ovnPort = cache.Netconf.Args.CNI.OvnPort
576+
}
577+
}
561578
ovsDriver, err := ovsdb.NewOvsDriver(cache.Netconf.SocketFile)
562579
if err != nil {
563580
return err
@@ -679,6 +696,12 @@ func CmdCheck(args *skel.CmdArgs) error {
679696
if envArgs != nil {
680697
ovnPort = string(envArgs.OvnPort)
681698
}
699+
// Check for OvnPort in args.cni (CNI convention) - takes precedence
700+
if netconf.Args != nil && netconf.Args.CNI != nil {
701+
if netconf.Args.CNI.OvnPort != "" {
702+
ovnPort = netconf.Args.CNI.OvnPort
703+
}
704+
}
682705
ovsDriver, err := ovsdb.NewOvsDriver(netconf.SocketFile)
683706
if err != nil {
684707
return err

‎pkg/plugin/plugin_test.go‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -735,6 +735,70 @@ var testFunc = func(version string) {
735735
Expect(string(output[:len(output)-1])).To(Equal(ovsOutput))
736736
})
737737
})
738+
Context("specified OvnPort via args.cni", func() {
739+
It("should successfully complete ADD, CHECK and DEL commands with args.cni OvnPort", func() {
740+
const ovsOutput = "external_ids : {iface-id=test-port-from-args}"
741+
742+
// OvnPort specified in args.cni (CNI convention) instead of CNI_ARGS
743+
conf := fmt.Sprintf(`{
744+
"cniVersion": "%s",
745+
"name": "mynet",
746+
"type": "ovs",
747+
"bridge": "%s",
748+
"args": {
749+
"cni": {
750+
"OvnPort": "test-port-from-args"
751+
}
752+
}}`, version, bridgeName)
753+
754+
targetNs := newNS()
755+
defer func() {
756+
closeNS(targetNs)
757+
}()
758+
759+
By("Calling ADD command with args.cni OvnPort")
760+
hostIfName, result := testAdd(conf, false, false, "", targetNs)
761+
hostIface := result.(*current.Result).Interfaces[0]
762+
763+
By("Verifying iface-id was set from args.cni")
764+
output, err := exec.Command("ovs-vsctl", "--column=external_ids", "find", "Interface", fmt.Sprintf("name=%s", hostIface.Name)).CombinedOutput()
765+
Expect(err).NotTo(HaveOccurred())
766+
Expect(string(output[:len(output)-1])).To(Equal(ovsOutput))
767+
768+
By("Calling CHECK command")
769+
testCheck(conf, result, targetNs)
770+
771+
By("Calling DEL command")
772+
testDel(conf, hostIfName, targetNs, true)
773+
})
774+
It("should prefer args.cni OvnPort over CNI_ARGS", func() {
775+
// args.cni should take precedence per CNI conventions
776+
const ovsOutput = "external_ids : {iface-id=args-cni-port}"
777+
778+
conf := fmt.Sprintf(`{
779+
"cniVersion": "%s",
780+
"name": "mynet",
781+
"type": "ovs",
782+
"bridge": "%s",
783+
"args": {
784+
"cni": {
785+
"OvnPort": "args-cni-port"
786+
}
787+
}}`, version, bridgeName)
788+
789+
targetNs := newNS()
790+
defer func() {
791+
closeNS(targetNs)
792+
}()
793+
794+
// Pass a different ovnPort via CNI_ARGS - args.cni should win
795+
result := attach(targetNs, conf, IFNAME, "", "cni-args-port")
796+
hostIface := result.Interfaces[0]
797+
output, err := exec.Command("ovs-vsctl", "--column=external_ids", "find", "Interface", fmt.Sprintf("name=%s", hostIface.Name)).CombinedOutput()
798+
Expect(err).NotTo(HaveOccurred())
799+
Expect(string(output[:len(output)-1])).To(Equal(ovsOutput))
800+
})
801+
})
738802
Context("specified OfportRequest", func() {
739803
It("should configure an ovs interface with a specific ofport", func() {
740804
// Pick a random ofport 5000-6000

‎pkg/types/types.go‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,13 @@ type NetConfs interface {
2626
NetConf | MirrorNetConf
2727
}
2828

29+
// CNIArgs contains arguments passed via args.cni in the CNI config
30+
// This follows CNI conventions: https://www.cni.dev/docs/conventions/
31+
type CNIArgs struct {
32+
OvnPort string `json:"OvnPort,omitempty"`
33+
MAC string `json:"MAC,omitempty"`
34+
}
35+
2936
// NetConf extends types.NetConf for ovs-cni
3037
type NetConf struct {
3138
types.NetConf
@@ -40,6 +47,10 @@ type NetConf struct {
4047
SocketFile string `json:"socket_file"`
4148
LinkStateCheckRetries int `json:"link_state_check_retries"`
4249
LinkStateCheckInterval int `json:"link_state_check_interval"`
50+
// Args contains CNI args passed via args.cni in the config
51+
Args *struct {
52+
CNI *CNIArgs `json:"cni,omitempty"`
53+
} `json:"args,omitempty"`
4354
}
4455

4556
// MirrorNetConf extends types.NetConf for ovs-mirrors

‎pkg/types/types_test.go‎

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,110 @@
1+
// Copyright (c) 2024 Red Hat, Inc.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package types
16+
17+
import (
18+
"encoding/json"
19+
"testing"
20+
)
21+
22+
func TestNetConfArgsCNIParsing(t *testing.T) {
23+
tests := []struct {
24+
name string
25+
config string
26+
expectedOvnPort string
27+
expectedMAC string
28+
}{
29+
{
30+
name: "args.cni with OvnPort",
31+
config: `{
32+
"cniVersion": "1.0.0",
33+
"name": "test-net",
34+
"type": "ovs",
35+
"bridge": "br-int",
36+
"args": {
37+
"cni": {
38+
"OvnPort": "test-port-id"
39+
}
40+
}
41+
}`,
42+
expectedOvnPort: "test-port-id",
43+
expectedMAC: "",
44+
},
45+
{
46+
name: "args.cni with OvnPort and MAC",
47+
config: `{
48+
"cniVersion": "1.0.0",
49+
"name": "test-net",
50+
"type": "ovs",
51+
"bridge": "br-int",
52+
"args": {
53+
"cni": {
54+
"OvnPort": "my-ovn-port",
55+
"MAC": "fa:16:3e:aa:bb:cc"
56+
}
57+
}
58+
}`,
59+
expectedOvnPort: "my-ovn-port",
60+
expectedMAC: "fa:16:3e:aa:bb:cc",
61+
},
62+
{
63+
name: "no args section",
64+
config: `{
65+
"cniVersion": "1.0.0",
66+
"name": "test-net",
67+
"type": "ovs",
68+
"bridge": "br-int"
69+
}`,
70+
expectedOvnPort: "",
71+
expectedMAC: "",
72+
},
73+
{
74+
name: "empty args.cni",
75+
config: `{
76+
"cniVersion": "1.0.0",
77+
"name": "test-net",
78+
"type": "ovs",
79+
"bridge": "br-int",
80+
"args": {
81+
"cni": {}
82+
}
83+
}`,
84+
expectedOvnPort: "",
85+
expectedMAC: "",
86+
},
87+
}
88+
89+
for _, tt := range tests {
90+
t.Run(tt.name, func(t *testing.T) {
91+
var netconf NetConf
92+
if err := json.Unmarshal([]byte(tt.config), &netconf); err != nil {
93+
t.Fatalf("Failed to unmarshal config: %v", err)
94+
}
95+
96+
var gotOvnPort, gotMAC string
97+
if netconf.Args != nil && netconf.Args.CNI != nil {
98+
gotOvnPort = netconf.Args.CNI.OvnPort
99+
gotMAC = netconf.Args.CNI.MAC
100+
}
101+
102+
if gotOvnPort != tt.expectedOvnPort {
103+
t.Errorf("OvnPort = %q, want %q", gotOvnPort, tt.expectedOvnPort)
104+
}
105+
if gotMAC != tt.expectedMAC {
106+
t.Errorf("MAC = %q, want %q", gotMAC, tt.expectedMAC)
107+
}
108+
})
109+
}
110+
}

0 commit comments

Comments
 (0)