[release 0.52] Fix numeric iface name in route (#993)

* Filter CNI veth interfaces by type and DeviceState (un/managed) (#809)

* Filter interfaces by type and DeviceState

Instead of filtering interfaces in NNS by name regex,
use type attribute and deviceState obtained from NetworkManager.

Drop veth interfaces that are not being managed by NetworkManager.

Signed-off-by: Radim Hrazdil <[email protected]>

* e2e: Add test verifying interface_filter isn't failing

Since NetworkManager client could fail to connect to dbus
(potentially), we should make sure that we catch such
events.

When the NetworkManager client fails to connect, the NNS
is not filtered and error message is logged by handler.

This commit adds test verifying that handler log doesn't
contain such message.

Signed-off-by: Radim Hrazdil <[email protected]>

* Remove unused INTERFACES_FILTER variable

Signed-off-by: Radim Hrazdil <[email protected]>

* Update section about vlan filtering from nns

It is now not possible to adjust vlan filtering, as
we stricly filter only unmanaged veth interfaces

Signed-off-by: Radim Hrazdil <[email protected]>

* Fix issue with Route next-hop-interface being interpreted as float64 (#987)

* unit test filtering of ifaces with numeric names

Signed-off-by: Radim Hrazdil <[email protected]>

* Fix next-hop-interface with numeric names being interpreted as float

Signed-off-by: Radim Hrazdil <[email protected]>

* Use centos 8 Stream as base image for handler

Signed-off-by: Radim Hrazdil <[email protected]>

Makefile

@@ -162,7 +162,7 @@ push-operator: operator
 push: push-handler push-operator
 
 test/unit: $(GO)
-	INTERFACES_FILTER="" NODE_NAME=node01 $(GINKGO) $(unit_test_args) $(WHAT)
+	NODE_NAME=node01 $(GINKGO) $(unit_test_args) $(WHAT)
 
 test-e2e-handler: $(GO)
 	KUBECONFIG=$(KUBECONFIG) OPERATOR_NAMESPACE=$(OPERATOR_NAMESPACE) $(GINKGO) $(e2e_test_args) ./test/e2e/handler ... -- $(E2E_TEST_SUITE_ARGS)

build/Dockerfile

@@ -1,4 +1,4 @@
-FROM centos:8
+FROM quay.io/centos/centos:stream8
 ARG NMSTATE_COPR_REPO
 ARG NM_COPR_REPO
 RUN dnf install -b -y dnf-plugins-core && \

controllers/node_controller.go

@@ -54,6 +54,7 @@ type NodeReconciler struct {
 	lastState      shared.State
 	nmstateUpdater NmstateUpdater
 	nmstatectlShow NmstatectlShow
+	deviceInfo     state.DeviceInfoer
 }
 
 // Reconcile reads that state of the cluster for a Node object and makes changes based on the state read
@@ -68,7 +69,7 @@ func (r *NodeReconciler) Reconcile(ctx context.Context, request ctrl.Request) (c
 		return ctrl.Result{}, err
 	}
 
-	currentState, err := state.FilterOut(shared.NewState(currentStateRaw))
+	currentState, err := state.FilterOut(shared.NewState(currentStateRaw), r.deviceInfo)
 	if err != nil {
 		return ctrl.Result{}, err
 	}
@@ -118,6 +119,7 @@ func (r *NodeReconciler) SetupWithManager(mgr ctrl.Manager) error {
 
 	r.nmstateUpdater = nmstate.CreateOrUpdateNodeNetworkState
 	r.nmstatectlShow = nmstatectl.Show
+	r.deviceInfo = state.DeviceInfo{}
 
 	// By default all this functors return true so controller watch all events,
 	// but we only want to watch create/delete for current node.

controllers/node_controller_test.go

@@ -19,13 +19,24 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
+	networkmanager "github.com/phoracek/networkmanager-go/src"
+
 	"github.com/nmstate/kubernetes-nmstate/api/shared"
 	nmstatev1beta1 "github.com/nmstate/kubernetes-nmstate/api/v1beta1"
 	"github.com/nmstate/kubernetes-nmstate/pkg/nmstatectl"
 	nmstatenode "github.com/nmstate/kubernetes-nmstate/pkg/node"
 	"github.com/nmstate/kubernetes-nmstate/pkg/state"
 )
 
+type fakeDeviceInfo struct{}
+
+func (fakeDeviceInfo) DeviceStates() (map[string]networkmanager.DeviceState, error) {
+	return map[string]networkmanager.DeviceState{
+		"eth1": networkmanager.DeviceStateActivated,
+		"eth2": networkmanager.DeviceStateActivated,
+	}, nil
+}
+
 var _ = Describe("Node controller reconcile", func() {
 	var (
 		cl                       client.Client
@@ -65,6 +76,7 @@ var _ = Describe("Node controller reconcile", func() {
 		reconciler.Scheme = s
 		reconciler.nmstateUpdater = nmstate.CreateOrUpdateNodeNetworkState
 		reconciler.nmstatectlShow = nmstatectl.Show
+		reconciler.deviceInfo = fakeDeviceInfo{}
 		reconciler.lastState = shared.NewState("lastState")
 		observedState = `
 ---
@@ -76,8 +88,9 @@ routes:
   running: []
   config: []
 `
+
 		var err error
-		filteredOutObservedState, err = state.FilterOut(shared.NewState(observedState))
+		filteredOutObservedState, err = state.FilterOut(shared.NewState(observedState), reconciler.deviceInfo)
 		Expect(err).ToNot(HaveOccurred())
 
 		reconciler.nmstatectlShow = func() (string, error) {
@@ -154,6 +167,7 @@ routes:
   config: []
 `
 			)
+
 			BeforeEach(func() {
 				By("Set last state")
 				reconciler.lastState = filteredOutObservedState
@@ -171,7 +185,7 @@ routes:
 				obtainedNNS := nmstatev1beta1.NodeNetworkState{}
 				err = cl.Get(context.TODO(), types.NamespacedName{Name: existingNodeName}, &obtainedNNS)
 				Expect(err).ToNot(HaveOccurred())
-				filteredOutExpectedState, err := state.FilterOut(shared.NewState(expectedStateRaw))
+				filteredOutExpectedState, err := state.FilterOut(shared.NewState(expectedStateRaw), reconciler.deviceInfo)
 				Expect(err).ToNot(HaveOccurred())
 				Expect(obtainedNNS.Status.CurrentState.String()).To(Equal(filteredOutExpectedState.String()))
 			})

deploy/handler/operator.yaml

@@ -249,8 +249,6 @@ spec:
               valueFrom:
                 fieldRef:
                   fieldPath: spec.nodeName
-            - name: INTERFACES_FILTER
-              value: "{veth*,cali*}"
             - name: ENABLE_PROFILER
               value: "False"
             - name: PROFILER_PORT

docs/user-guide/101-reporting.md

@@ -164,27 +164,10 @@ observed state is fresh enough.
 
 The reported state is updated every 5 seconds.
 
-## Filter reported interfaces
+## Node Network State interfaces filtering
 
-By default, all `veth*` interfaces are omitted from the report in order not to
-clutter the output with all Pod connections. This filtering can be adjusted via
-environment variable `INTERFACES_FILTER`at nmstate-handler daemonset.
-
-| Filter            | Effect                                                             |
-| ---               | ---                                                                |
-| `"veth*"`         | Omit all interfaces starting with `veth` from the report (default) |
-| `""`              | Disable the filter, show all interfaces found on the node          |
-| `"{veth*,vnet*}"` | Omit all interfaces starting with either `veth` or `vnet`          |
-
-
-To override the default configuration, use `set env` command. Additionally, in
-order for the config to take effect, all handlers will be restarted:
-
-```json
-kubectl set env -n nmstate daemonset/nmstate-handler INTERFACES_FILTER=""
-```
-
-The `NodeNetworkState` will now list all interfaces seen on the host.
+All unmanaged `veth` interfaces are omitted from the report in order to not
+clutter the output with all Pod connections.
 
 ## Continue reading
 

pkg/state/filter.go

@@ -1,47 +1,86 @@
 package state
 
 import (
-	"os"
+	networkmanager "github.com/phoracek/networkmanager-go/src"
 
-	"github.com/gobwas/glob"
 	"github.com/nmstate/kubernetes-nmstate/api/shared"
 	"github.com/nmstate/kubernetes-nmstate/pkg/environment"
 
 	goyaml "gopkg.in/yaml.v2"
+	logf "sigs.k8s.io/controller-runtime/pkg/log"
 	yaml "sigs.k8s.io/yaml"
 )
 
+const (
+	INTERFACE_FILTER = "interface_filter"
+)
+
 var (
-	interfacesFilterGlobFromEnv glob.Glob
+	filterLog = logf.Log.WithName(INTERFACE_FILTER)
 )
 
 func init() {
 	if !environment.IsHandler() {
 		return
 	}
-	interfacesFilter, isSet := os.LookupEnv("INTERFACES_FILTER")
-	if !isSet {
-		panic("INTERFACES_FILTER is mandatory")
+}
+
+type DeviceInfoer interface {
+	DeviceStates() (map[string]networkmanager.DeviceState, error)
+}
+
+type DeviceInfo struct{}
+
+func (d DeviceInfo) DeviceStates() (map[string]networkmanager.DeviceState, error) {
+	nmClient, err := networkmanager.NewClientPrivate()
+	if err != nil {
+		filterLog.Error(err, "failed to initialize NetworkManager client")
+		return nil, err
 	}
-	interfacesFilterGlobFromEnv = glob.MustCompile(interfacesFilter)
+	defer nmClient.Close()
+
+	devices, err := nmClient.GetDevices()
+	if err != nil {
+		filterLog.Error(err, "failed to list NetworkManager devices")
+		return nil, err
+	}
+
+	ifaceStates := map[string]networkmanager.DeviceState{}
+	for _, device := range devices {
+		ifaceStates[device.Interface] = device.State
+	}
+	return ifaceStates, nil
 }
 
-func FilterOut(currentState shared.State) (shared.State, error) {
-	return filterOut(currentState, interfacesFilterGlobFromEnv)
+func FilterOut(currentState shared.State, deviceInfo DeviceInfoer) (shared.State, error) {
+	devStates, err := deviceInfo.DeviceStates()
+	if err != nil {
+		filterLog.Error(err, "failed getting interface states, cannot filter managed interfaces")
+		return currentState, nil
+	}
+	return filterOut(currentState, devStates)
 }
 
-func filterOutRoutes(routes []interface{}, interfacesFilterGlob glob.Glob) []interface{} {
-	filteredRoutes := []interface{}{}
+func filterOutRoutes(routes []routeState, filteredInterfaces []interfaceState) []routeState {
+	filteredRoutes := []routeState{}
 	for _, route := range routes {
-		name := route.(map[string]interface{})["next-hop-interface"]
-		if !interfacesFilterGlob.Match(name.(string)) {
+		name := route.NextHopInterface
+		if isInInterfaces(name, filteredInterfaces) {
 			filteredRoutes = append(filteredRoutes, route)
 		}
 	}
-
 	return filteredRoutes
 }
 
+func isInInterfaces(interfaceName string, interfaces []interfaceState) bool {
+	for _, iface := range interfaces {
+		if iface.Name == interfaceName {
+			return true
+		}
+	}
+	return false
+}
+
 func filterOutDynamicAttributes(iface map[string]interface{}) {
 	// The gc-timer and hello-time are deep into linux-bridge like this
 	//    - bridge:
@@ -74,32 +113,36 @@ func filterOutDynamicAttributes(iface map[string]interface{}) {
 	delete(options, "hello-timer")
 }
 
-func filterOutInterfaces(ifacesState []interfaceState, interfacesFilterGlob glob.Glob) []interfaceState {
+func filterOutInterfaces(ifacesState []interfaceState, deviceStates map[string]networkmanager.DeviceState) []interfaceState {
+	if deviceStates == nil {
+		return ifacesState
+	}
+
 	filteredInterfaces := []interfaceState{}
 	for _, iface := range ifacesState {
-		if !interfacesFilterGlob.Match(iface.Name) {
+		if iface.Data["type"] != "veth" || deviceStates[iface.Name] != networkmanager.DeviceStateUnmanaged {
 			filterOutDynamicAttributes(iface.Data)
 			filteredInterfaces = append(filteredInterfaces, iface)
 		}
 	}
 	return filteredInterfaces
 }
 
-func filterOut(currentState shared.State, interfacesFilterGlob glob.Glob) (shared.State, error) {
+func filterOut(currentState shared.State, deviceStates map[string]networkmanager.DeviceState) (shared.State, error) {
 	var state rootState
 	if err := yaml.Unmarshal(currentState.Raw, &state); err != nil {
 		return currentState, err
 	}
-
 	if err := normalizeInterfacesNames(currentState.Raw, &state); err != nil {
 		return currentState, err
 	}
 
-	state.Interfaces = filterOutInterfaces(state.Interfaces, interfacesFilterGlob)
+	state.Interfaces = filterOutInterfaces(state.Interfaces, deviceStates)
 	if state.Routes != nil {
-		state.Routes.Running = filterOutRoutes(state.Routes.Running, interfacesFilterGlob)
-		state.Routes.Config = filterOutRoutes(state.Routes.Config, interfacesFilterGlob)
+		state.Routes.Running = filterOutRoutes(state.Routes.Running, state.Interfaces)
+		state.Routes.Config = filterOutRoutes(state.Routes.Config, state.Interfaces)
 	}
+
 	filteredState, err := yaml.Marshal(state)
 	if err != nil {
 		return currentState, err
@@ -115,8 +158,18 @@ func normalizeInterfacesNames(rawState []byte, state *rootState) error {
 	if err := goyaml.Unmarshal(rawState, &stateForNormalization); err != nil {
 		return err
 	}
+
 	for i, iface := range stateForNormalization.Interfaces {
 		state.Interfaces[i].Name = iface.Name
 	}
+
+	if stateForNormalization.Routes != nil {
+		for i, route := range stateForNormalization.Routes.Config {
+			state.Routes.Config[i].NextHopInterface = route.NextHopInterface
+		}
+		for i, route := range stateForNormalization.Routes.Running {
+			state.Routes.Running[i].NextHopInterface = route.NextHopInterface
+		}
+	}
 	return nil
 }