Fix applying vlan filtering via nmcli (#903)

Only apply vlan filtering via nmcli to bridges and ports that are listed in desiredState and also exist in current state. Signed-off-by: Radim Hrazdil <[email protected]>
nmstate · Nov 26, 2021 · c2b607d · c2b607d
1 parent 6881ba2
commit c2b607d
Show file tree

Hide file tree

Showing 3 changed files with 218 additions and 5 deletions.
diff --git a/pkg/helper/bridges.go b/pkg/helper/bridges.go
@@ -11,7 +11,6 @@ import (
 
 	yaml "sigs.k8s.io/yaml"
 
-	"github.com/nmstate/kubernetes-nmstate/api/shared"
 	nmstate "github.com/nmstate/kubernetes-nmstate/api/shared"
 	"github.com/nmstate/kubernetes-nmstate/pkg/nmstatectl"
 )
@@ -57,16 +56,21 @@ func ApplyDefaultVlanFiltering(desiredState nmstate.State) (nmstate.State, error
 	return nmstate.State{Raw: resultYaml}, nil
 }
 
-func EnableVlanFiltering() (string, error) {
+func EnableVlanFiltering(desiredState nmstate.State) (string, error) {
 	currentState, err := nmstatectl.Show()
 	if err != nil {
 		return "failed to get currentState", err
 	}
-	upBridgesWithPorts, err := GetUpLinuxBridgesWithPorts(shared.NewState(currentState))
+	upBridgesWithPortsAtCurrentState, err := GetUpLinuxBridgesWithPorts(nmstate.NewState(currentState))
 	if err != nil {
 		return "failed to list bridges with ports", err
 	}
-	out, err := enableVlanFiltering(upBridgesWithPorts)
+	filteredExistingUpBridgesWithPortsAtDesiredState, err := filterExistingLinuxBridgesWithPorts(upBridgesWithPortsAtCurrentState, desiredState)
+	if err != nil {
+		return "failed to filter existing bridges with ports from desiredState", err
+	}
+
+	out, err := enableVlanFiltering(filteredExistingUpBridgesWithPortsAtDesiredState)
 	if err != nil {
 		return fmt.Sprintf("failed to enable vlan-filtering via nmcli: %s", out), err
 	}
@@ -97,6 +101,22 @@ func GetUpLinuxBridgesWithPorts(desiredState nmstate.State) (map[string][]string
 	return bridgesWithPorts, nil
 }
 
+func filterExistingLinuxBridgesWithPorts(bridgesAtCurrentState map[string][]string, desiredState nmstate.State) (map[string][]string, error) {
+	filteredBridgesWithPorts := map[string][]string{}
+	bridgesAtDesiredState, err := GetUpLinuxBridgesWithPorts(desiredState)
+	if err != nil {
+		return nil, err
+	}
+
+	for bridge, ports := range bridgesAtDesiredState {
+		if currentBridgePorts, ok := bridgesAtCurrentState[bridge]; ok && len(currentBridgePorts) > 0 {
+			filteredBridgesWithPorts[bridge] = intersectSlices(ports, currentBridgePorts)
+		}
+	}
+
+	return filteredBridgesWithPorts, nil
+}
+
 func enableVlanFiltering(upBridgesWithPorts map[string][]string) (string, error) {
 	for bridge, ports := range upBridgesWithPorts {
 		out, err := enableBridgeVlanFiltering(bridge)
@@ -136,6 +156,23 @@ func runCommand(command string, args []string) (string, error) {
 	return stdout.String(), nil
 }
 
+func intersectSlices(s1, s2 []string) []string {
+	cache := map[string]bool{}
+	result := []string{}
+
+	for _, e := range s1 {
+		cache[e] = false
+	}
+
+	for _, e := range s2 {
+		if cached, ok := cache[e]; ok && !cached {
+			result = append(result, e)
+			cache[e] = true
+		}
+	}
+	return result
+}
+
 func isLinuxBridgeUp(iface gjson.Result) bool {
 	return iface.Get("type").String() == "linux-bridge" && iface.Get("state").String() == "up"
 }

diff --git a/pkg/helper/bridges_test.go b/pkg/helper/bridges_test.go
@@ -2,6 +2,7 @@ package helper
 
 import (
 	. "github.com/onsi/ginkgo"
+	"github.com/onsi/ginkgo/extensions/table"
 	. "github.com/onsi/gomega"
 
 	nmstate "github.com/nmstate/kubernetes-nmstate/api/shared"
@@ -342,3 +343,178 @@ var _ = Describe("test listing linux bridges with ports", func() {
 			))
 	})
 })
+
+var _ = Describe("test listing linux bridges with ports", func() {
+	currentState := nmstate.NewState(`interfaces:
+  - name: br22
+    type: linux-bridge
+    state: up
+    bridge:
+      port:
+      - name: test-veth1
+        vlan:
+          enable-native: true
+  - name: br3
+    type: linux-bridge
+    state: up
+    bridge:
+      port:
+      - name: eth2
+      - name: eth3
+      - name: eth4
+      - name: eth5
+  - name: br4
+    type: linux-bridge
+    state: up
+    bridge:
+      port:
+      - name: eth4
+  - name: br5
+    type: linux-bridge
+    state: up
+    bridge:
+      port:
+      - name: eth5
+  - name: br7
+    type: linux-bridge
+    state: up
+    bridge:
+      port:
+      - name: eth777
+  - name: br8
+    type: linux-bridge
+    state: up
+    bridge:
+      port:
+      - name: eth888
+`)
+	desiredState := nmstate.NewState(`interfaces:
+- name: br3
+  type: linux-bridge
+  state: up
+  bridge:
+    port:
+    - name: eth2
+    - name: eth3
+- name: br4
+  type: linux-bridge
+  state: up
+  bridge:
+    port:
+    - name: eth4
+- name: br5
+  type: linux-bridge
+  state: up
+  bridge:
+    port: []
+- name: br6
+  type: linux-bridge
+  state: down
+  bridge:
+    port:
+    - name: eth666
+- name: br7
+  type: linux-bridge
+  state: up
+  bridge:
+    port:
+    - name: eth777
+    - name: eth778
+    - name: eth779
+- name: br8
+  type: linux-bridge
+  state: absent
+  bridge:
+    port:
+    - name: eth888
+`)
+	expectedFilteredExistingBridgesWithPorts := map[string][]string{
+		"br3": {"eth2", "eth3"},
+		"br4": {"eth4"},
+		"br7": {"eth777"},
+	}
+	It("should filter correct bridges and ports", func() {
+		upBridgesWithPortsAtCurrentState, err := GetUpLinuxBridgesWithPorts(currentState)
+		Expect(err).ShouldNot(HaveOccurred())
+
+		filteredExistingUpBridgesWithPorts, err := filterExistingLinuxBridgesWithPorts(upBridgesWithPortsAtCurrentState, desiredState)
+		Expect(err).ShouldNot(HaveOccurred())
+		Expect(filteredExistingUpBridgesWithPorts).To(Equal(expectedFilteredExistingBridgesWithPorts))
+	})
+})
+
+var _ = Describe("testing slice intersection", func() {
+	type intersectionCase struct {
+		s1                   []string
+		s2                   []string
+		expectedIntersection []string
+	}
+
+	table.DescribeTable("Slice intersection cases",
+		func(c intersectionCase) {
+			result := intersectSlices(c.s1, c.s2)
+			Expect(result).To(Equal(c.expectedIntersection))
+		},
+		table.Entry(
+			"Both slices empty",
+			intersectionCase{
+				s1:                   []string{},
+				s2:                   []string{},
+				expectedIntersection: []string{},
+			}),
+		table.Entry("Empty first slice",
+			intersectionCase{
+				s1:                   []string{},
+				s2:                   []string{"foo"},
+				expectedIntersection: []string{},
+			}),
+		table.Entry("Empty second slice",
+			intersectionCase{
+				s1:                   []string{"foo"},
+				s2:                   []string{},
+				expectedIntersection: []string{},
+			}),
+		table.Entry("No common elements",
+			intersectionCase{
+				s1:                   []string{"foo"},
+				s2:                   []string{"bar"},
+				expectedIntersection: []string{},
+			}),
+		table.Entry("One common element with extra in first slice",
+			intersectionCase{
+				s1:                   []string{"foo", "bar"},
+				s2:                   []string{"bar"},
+				expectedIntersection: []string{"bar"},
+			}),
+		table.Entry("One common element with extra in first slice",
+			intersectionCase{
+				s1:                   []string{"bar"},
+				s2:                   []string{"bar", "foo"},
+				expectedIntersection: []string{"bar"},
+			}),
+		table.Entry("One common element with extra in first slice",
+			intersectionCase{
+				s1:                   []string{"bar"},
+				s2:                   []string{"bar", "foo"},
+				expectedIntersection: []string{"bar"},
+			}),
+		table.Entry("Both identical with two elements",
+			intersectionCase{
+				s1:                   []string{"foo", "bar"},
+				s2:                   []string{"bar", "foo"},
+				expectedIntersection: []string{"bar", "foo"},
+			}),
+		table.Entry("Duplicates in first slice",
+			intersectionCase{
+				s1:                   []string{"foo", "bar", "one", "two", "three", "one", "two", "three"},
+				s2:                   []string{"bar", "foo", "three", "one"},
+				expectedIntersection: []string{"bar", "foo", "three", "one"},
+			}),
+		table.Entry("Duplicates in second slice",
+			intersectionCase{
+				s1:                   []string{"bar", "foo", "three", "one"},
+				s2:                   []string{"foo", "bar", "one", "two", "three", "one", "two", "three"},
+				expectedIntersection: []string{"foo", "bar", "one", "three"},
+			}),
+	)
+})
diff --git a/pkg/helper/client.go b/pkg/helper/client.go
@@ -103,7 +103,7 @@ func ApplyDesiredState(client client.Client, desiredState shared.State) (string,
 		return "Ignoring empty desired state", nil
 	}
 
-	out, err := EnableVlanFiltering()
+	out, err := EnableVlanFiltering(desiredState)
 	if err != nil {
 		return out, fmt.Errorf("failed to enable vlan filtering via nmcli: %s", err.Error())
 	}