nodeipam: add match-node-labels annotation to filter nodes

Now that nodeipam consider all nodes as potential candidate in
the eTP=Cluster case, a way for user to filter nodes become way more
critical and thus this commit is implementing this.

Co-authored-by: Brendan Dalpe <bdalpe@gmail.com>
Signed-off-by: Arthur Outhenin-Chalandre <arthur@cri.epita.fr>

Documentation/network/node-ipam.rst

@@ -36,7 +36,12 @@ all the Pods selected by the Service (via their EndpointSlices) as candidates.
     <https://github.com/cilium/cilium/blob/495f228ad8791c89f0851e0abbad90f09b136f80/install/kubernetes/cilium/templates/cilium-ingress-service.yaml#L58>`_).
     Only the Cilium implementations are known to be broken in this specific configuration, expects
     most other implementations to work and if it doesn't check if the matching EndpointSlices
-    looks correct and/or try to setting ``.spec.externalTrafficPolicy`` to ``Cluster``.
+    looks correct and/or try setting ``.spec.externalTrafficPolicy`` to ``Cluster``.
+
+To restrict the Nodes that should listen for incoming traffic, add annotation
+``io.cilium.nodeipam/match-node-labels`` to the Service. The value of the
+annotation is a
+`Label Selector <https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#label-selectors>`__.
 
 Enable and use Node IPAM
 ------------------------

Documentation/network/servicemesh/ingress.rst

@@ -139,6 +139,8 @@ are supported.
 
 By default, annotations with values beginning with:
 
+* ``lbipam.cilium.io``
+* ``nodeipam.cilium.io``
 * ``service.beta.kubernetes.io``
 * ``service.kubernetes.io``
 * ``cloud.google.com``

install/kubernetes/cilium/values.yaml.tmpl

@@ -644,7 +644,7 @@ ingressController:
   # -- Enable proxy protocol for all Ingress listeners. Note that _only_ Proxy protocol traffic will be accepted once this is enabled.
   enableProxyProtocol: false
   # -- IngressLBAnnotations are the annotation and label prefixes, which are used to filter annotations and/or labels to propagate from Ingress to the Load Balancer service
-  ingressLBAnnotationPrefixes: ['lbipam.cilium.io', 'service.beta.kubernetes.io', 'service.kubernetes.io', 'cloud.google.com']
+  ingressLBAnnotationPrefixes: ['lbipam.cilium.io', 'nodeipam.cilium.io', 'service.beta.kubernetes.io', 'service.kubernetes.io', 'cloud.google.com']
   # -- Default secret namespace for ingresses without .spec.tls[].secretName set.
   defaultSecretNamespace:
   # -- Default secret name for ingresses without .spec.tls[].secretName set.

operator/pkg/nodeipam/nodesvclb.go

@@ -25,10 +25,14 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	controllerruntime "github.com/cilium/cilium/operator/pkg/controller-runtime"
+	"github.com/cilium/cilium/pkg/annotation"
 	"github.com/cilium/cilium/pkg/logging/logfields"
 )
 
-var nodeSvcLBClass = "io.cilium/node"
+var (
+	nodeSvcLBClass                 = annotation.Prefix + "/node"
+	nodeSvcLBMatchLabelsAnnotation = annotation.Prefix + ".nodeipam" + "/match-node-labels"
+)
 
 type nodeSvcLBReconciler struct {
 	client.Client
@@ -200,14 +204,31 @@ func (r *nodeSvcLBReconciler) getEndpointSliceNodeNames(ctx context.Context, svc
 
 // getRelevantNodes gets all the nodes candidates for seletion by nodeipam
 func (r *nodeSvcLBReconciler) getRelevantNodes(ctx context.Context, svc *corev1.Service) ([]corev1.Node, error) {
+	scopedLog := r.Logger.WithFields(logrus.Fields{
+		logfields.Controller: "node-service-lb",
+		logfields.Resource:   client.ObjectKeyFromObject(svc),
+	})
+
 	endpointSliceNames, err := r.getEndpointSliceNodeNames(ctx, svc)
 	if err != nil {
 		return []corev1.Node{}, err
 	}
+	nodeListOptions := &client.ListOptions{}
+	if val, ok := svc.Annotations[nodeSvcLBMatchLabelsAnnotation]; ok {
+		parsedLabels, err := labels.Parse(val)
+		if err != nil {
+			return []corev1.Node{}, err
+		}
+		nodeListOptions.LabelSelector = parsedLabels
+	}
+
 	var nodes corev1.NodeList
-	if err := r.List(ctx, &nodes); err != nil {
+	if err := r.List(ctx, &nodes, nodeListOptions); err != nil {
 		return []corev1.Node{}, err
 	}
+	if len(nodes.Items) == 0 {
+		scopedLog.WithFields(logrus.Fields{logfields.Labels: nodeListOptions.LabelSelector}).Warning("No Nodes found with configured label selector")
+	}
 
 	relevantNodes := []corev1.Node{}
 	for _, node := range nodes.Items {

operator/pkg/nodeipam/nodesvclb_test.go

@@ -4,6 +4,7 @@
 package nodeipam
 
 import (
+	"bytes"
 	"context"
 	"testing"
 	"time"
@@ -284,6 +285,53 @@ var (
 			}},
 		},
 	}
+
+	nodeSvcLabelFixtures = []client.Object{
+		&corev1.Node{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:   "node-1",
+				Labels: map[string]string{"ingress-ready": "true", "all": "true", "group": "first"},
+			},
+			Status: corev1.NodeStatus{
+				Addresses: []corev1.NodeAddress{
+					{Type: corev1.NodeInternalIP, Address: "10.0.0.1"},
+				},
+			},
+		},
+		&corev1.Node{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:   "node-2",
+				Labels: map[string]string{"all": "true", "group": "notfirst", "test/label": "is-good"},
+			},
+			Status: corev1.NodeStatus{
+				Addresses: []corev1.NodeAddress{
+					{Type: corev1.NodeInternalIP, Address: "10.0.0.2"},
+				},
+			},
+		},
+		&corev1.Node{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:   "node-3",
+				Labels: map[string]string{"all": "true", "group": "notfirst"},
+			},
+			Status: corev1.NodeStatus{
+				Addresses: []corev1.NodeAddress{
+					{Type: corev1.NodeInternalIP, Address: "10.0.0.3"},
+				},
+			},
+		},
+		&corev1.Service{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "svclabels",
+				Namespace: "default",
+			},
+			Spec: corev1.ServiceSpec{
+				Type:              corev1.ServiceTypeLoadBalancer,
+				IPFamilies:        []corev1.IPFamily{corev1.IPv4Protocol},
+				LoadBalancerClass: &nodeSvcLBClass,
+			},
+		},
+	}
 )
 
 func stringPtr(str string) *string {
@@ -396,3 +444,112 @@ func Test_httpRouteReconciler_Reconcile(t *testing.T) {
 		require.Equal(t, svc.Status.LoadBalancer.Ingress[1].IP, "42.0.0.3")
 	})
 }
+
+func Test_CiliumResources_Reconcile(t *testing.T) {
+	c := fake.NewClientBuilder().
+		WithObjects(nodeSvcLabelFixtures...).
+		WithStatusSubresource(&corev1.Service{}).
+		Build()
+	r := &nodeSvcLBReconciler{Client: c, Logger: logging.DefaultLogger}
+
+	key := types.NamespacedName{
+		Name:      "svclabels",
+		Namespace: "default",
+	}
+
+	t.Run("Managed Resource", func(t *testing.T) {
+		ctx := context.Background()
+		result, err := r.Reconcile(ctx, ctrl.Request{
+			NamespacedName: key,
+		})
+
+		require.NoError(t, err)
+		require.Equal(t, ctrl.Result{}, result, "Result should be empty")
+
+		svc := &corev1.Service{}
+		err = c.Get(ctx, key, svc)
+
+		require.NoError(t, err)
+		require.Len(t, svc.Status.LoadBalancer.Ingress, 3)
+		var ips []string
+		for _, v := range svc.Status.LoadBalancer.Ingress {
+			ips = append(ips, v.IP)
+		}
+		require.Equal(t, ips, []string{"10.0.0.1", "10.0.0.2", "10.0.0.3"})
+	})
+
+	t.Run("Node Label Filter", func(t *testing.T) {
+		ctx := context.Background()
+
+		for _, param := range []struct {
+			labelFilter string
+			results     []string
+		}{
+			{labelFilter: "all=true", results: []string{"10.0.0.1", "10.0.0.2", "10.0.0.3"}},
+			{labelFilter: "ingress-ready=true", results: []string{"10.0.0.1"}},
+			{labelFilter: "group=notfirst", results: []string{"10.0.0.2", "10.0.0.3"}},
+			{labelFilter: "group notin (first),test/label=is-good", results: []string{"10.0.0.2"}},
+		} {
+			svc := &corev1.Service{}
+			_ = c.Get(ctx, key, svc)
+			// Add the label to the service which should return on the first node
+			svc.Annotations = map[string]string{nodeSvcLBMatchLabelsAnnotation: param.labelFilter}
+			_ = c.Update(ctx, svc)
+			result, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: key})
+
+			require.NoError(t, err)
+			require.Equal(t, ctrl.Result{}, result, "Result should be empty")
+
+			svc = &corev1.Service{}
+			err = c.Get(ctx, key, svc)
+
+			require.NoError(t, err)
+			require.NotNil(t, svc.Annotations[nodeSvcLBMatchLabelsAnnotation])
+			require.Len(t, svc.Status.LoadBalancer.Ingress, len(param.results))
+
+			var ips []string
+			for _, v := range svc.Status.LoadBalancer.Ingress {
+				ips = append(ips, v.IP)
+			}
+			require.Equal(t, ips, param.results)
+		}
+	})
+
+	t.Run("Bad Node Label Filter", func(t *testing.T) {
+		ctx := context.Background()
+		svc := &corev1.Service{}
+		_ = c.Get(ctx, key, svc)
+		// Add the label to the service which should return on the first node
+		svc.Annotations = map[string]string{nodeSvcLBMatchLabelsAnnotation: "this is completely/bad=!;lf"}
+		_ = c.Update(ctx, svc)
+		result, err := r.Reconcile(ctx, ctrl.Request{
+			NamespacedName: key,
+		})
+
+		require.Error(t, err)
+		require.Equal(t, ctrl.Result{}, result, "Result should be empty")
+
+	})
+
+	t.Run("Ensure Warning raised if no Nodes found using configured label selector", func(t *testing.T) {
+		var buf bytes.Buffer
+		logger := logging.DefaultLogger
+		logger.SetOutput(&buf)
+
+		r.Logger = logger
+
+		ctx := context.Background()
+		svc := &corev1.Service{}
+		_ = c.Get(ctx, key, svc)
+		// Add the label to the service which should return on the first node
+		svc.Annotations = map[string]string{nodeSvcLBMatchLabelsAnnotation: "foo=bar"}
+		_ = c.Update(ctx, svc)
+		result, err := r.Reconcile(ctx, ctrl.Request{
+			NamespacedName: key,
+		})
+
+		require.NoError(t, err)
+		require.Equal(t, ctrl.Result{}, result, "Result should be empty")
+		require.Contains(t, buf.String(), "level=warning msg=\"No Nodes found with configured label selector\"")
+	})
+}