Add support for NodeSelector in TargetGroupBindings (kubernetes-sigs#1785)

* Add support for NodeSelector in TargetGroupBindings

Signed-off-by: Alex Williams <[email protected]>

* NodeSelector adds to default selector not overrides

* Update docs on merging NodeSelector behaviour

Author: Smirl

apis/elbv2/v1beta1/targetgroupbinding_types.go

@@ -127,6 +127,10 @@ type TargetGroupBindingSpec struct {
 	// networking defines the networking rules to allow ELBV2 LoadBalancer to access targets in TargetGroup.
 	// +optional
 	Networking *TargetGroupBindingNetworking `json:"networking,omitempty"`
+
+	// node selector for instance type target groups to only register certain nodes
+	// +optional
+	NodeSelector *metav1.LabelSelector `json:"nodeSelector,omitempty"`
 }
 
 // TargetGroupBindingStatus defines the observed state of TargetGroupBinding

apis/elbv2/v1beta1/zz_generated.deepcopy.go

@@ -2,6 +2,7 @@ package eventhandlers
 
 import (
 	"context"
+
 	"github.com/go-logr/logr"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/labels"
@@ -79,7 +80,11 @@ func (h *enqueueRequestsForNodeEvent) enqueueImpactedTargetGroupBindings(queue w
 			continue
 		}
 
-		nodeSelector := backend.GetTrafficProxyNodeSelector(&tgb)
+		nodeSelector, err := backend.GetTrafficProxyNodeSelector(&tgb)
+		if err != nil {
+			h.logger.Error(err, "failed to get nodeSelector", "TargetGroupBinding", tgb)
+			return
+		}
 
 		nodeOldIsTrafficProxy := false
 		nodeNewIsTrafficProxy := false

config/crd/bases/elbv2.k8s.aws_targetgroupbindings.yaml

@@ -18,7 +18,7 @@ TargetGroupBinding CR supports TargetGroups of either `instance` or `ip` TargetT
 
 
 ## Sample YAML
-```
+```yaml
 apiVersion: elbv2.k8s.aws/v1beta1
 kind: TargetGroupBinding
 metadata:
@@ -30,5 +30,48 @@ spec:
   targetGroupARN: <arn-to-targetGroup>
 ```
 
+
+## NodeSelector
+
+### Default Node Selector
+
+For `TargetType: instance`, all nodes of a cluster that match the following
+selector are added to the target group by default:
+
+```yaml
+matchExpressions:
+  - key: node-role.kubernetes.io/master
+    operator: DoesNotExist
+  - key: node.kubernetes.io/exclude-from-external-load-balancers
+    operator: DoesNotExist
+  - key: alpha.service-controller.kubernetes.io/exclude-balancer
+    operator: DoesNotExist
+  - key: eks.amazonaws.com/compute-type
+    operator: NotIn
+    values: ["fargate"]
+```
+
+### Custom Node Selector
+
+TargetGroupBinding CR supports `NodeSelector` which is a
+[LabelSelector][LabelSelector]. This will select nodes to attach to the
+`instance` TargetType target group and **is merged with the default node
+selector above**.
+
+```yaml
+apiVersion: elbv2.k8s.aws/v1beta1
+kind: TargetGroupBinding
+metadata:
+  name: my-tgb
+spec:
+  nodeSelector:
+    matchLabels:
+      foo: bar
+  ...
+```
+
+
 ## Reference
-See the [reference](./spec.md) for TargetGroupBinding CR
+See the [reference](./spec.md) for TargetGroupBinding CR
+
+[LabelSelector]: https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.19/#labelselector-v1-meta

controllers/elbv2/eventhandlers/node.go

@@ -14,6 +14,7 @@ const (
 )
 
 var (
+	// Remember to update docs/guide/targetgroupbinding/targetgroupbinding.md if changing
 	defaultTrafficProxyNodeLabelSelector = metav1.LabelSelector{
 		MatchExpressions: []metav1.LabelSelectorRequirement{
 			{
@@ -38,8 +39,18 @@ var (
 )
 
 // GetTrafficProxyNodeSelector returns the trafficProxy node label selector for specific targetGroupBinding.
-func GetTrafficProxyNodeSelector(_ *elbv2api.TargetGroupBinding) labels.Selector {
-	// TODO: consider expose nodeSelector on targetGroupBindings, so users can optionally specify different nodes for different tgbs.
-	selector, _ := metav1.LabelSelectorAsSelector(&defaultTrafficProxyNodeLabelSelector)
-	return selector
+func GetTrafficProxyNodeSelector(tgb *elbv2api.TargetGroupBinding) (labels.Selector, error) {
+	selector, err := metav1.LabelSelectorAsSelector(&defaultTrafficProxyNodeLabelSelector)
+	if err != nil {
+		return nil, err
+	}
+	if tgb.Spec.NodeSelector != nil {
+		customSelector, err := metav1.LabelSelectorAsSelector(tgb.Spec.NodeSelector)
+		if err != nil {
+			return nil, err
+		}
+		req, _ := customSelector.Requirements()
+		selector = selector.Add(req...)
+	}
+	return selector, nil
 }

docs/guide/targetgroupbinding/targetgroupbinding.md

@@ -0,0 +1,78 @@
+package backend
+
+import (
+	"errors"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/labels"
+	"k8s.io/apimachinery/pkg/selection"
+	elbv2api "sigs.k8s.io/aws-load-balancer-controller/apis/elbv2/v1beta1"
+)
+
+func TestDefaultTrafficProxyNodeLabelSelector(t *testing.T) {
+	// Test that the default is able to be converted into a label selector
+	_, err := metav1.LabelSelectorAsSelector(&defaultTrafficProxyNodeLabelSelector)
+	assert.NoError(t, err)
+}
+
+func TestGetTrafficProxyNodeSelector(t *testing.T) {
+	// Set up the labels.Selector expected objects
+	defaultSelector, _ := metav1.LabelSelectorAsSelector(&defaultTrafficProxyNodeLabelSelector)
+	reqs, _ := defaultSelector.Requirements()
+
+	customSelector := labels.NewSelector()
+	req, _ := labels.NewRequirement("key", selection.Equals, []string{"value"})
+	customSelector = customSelector.Add(*req)
+	customSelector = customSelector.Add(reqs...) // add default selector rules as well
+
+	tests := []struct {
+		name               string
+		targetGroupBinding *elbv2api.TargetGroupBinding
+		want               labels.Selector
+		wantErr            error
+	}{
+		{
+			name:               "default node selector when not specified",
+			targetGroupBinding: &elbv2api.TargetGroupBinding{},
+			want:               defaultSelector,
+		},
+		{
+			name: "selector from TargetGroupBinding",
+			targetGroupBinding: &elbv2api.TargetGroupBinding{
+				Spec: elbv2api.TargetGroupBindingSpec{
+					NodeSelector: &metav1.LabelSelector{
+						MatchLabels: map[string]string{
+							"key": "value",
+						},
+					},
+				},
+			},
+			want: customSelector,
+		},
+		{
+			name: "error with bad selector",
+			targetGroupBinding: &elbv2api.TargetGroupBinding{
+				Spec: elbv2api.TargetGroupBindingSpec{
+					NodeSelector: &metav1.LabelSelector{
+						MatchExpressions: []metav1.LabelSelectorRequirement{
+							{Key: "key", Operator: "BadOperatorValue", Values: []string{"value"}},
+						},
+					},
+				},
+			},
+			wantErr: errors.New("\"BadOperatorValue\" is not a valid pod selector operator"),
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := GetTrafficProxyNodeSelector(tt.targetGroupBinding)
+			if tt.wantErr != nil {
+				assert.EqualError(t, err, tt.wantErr.Error())
+			} else {
+				assert.Equal(t, tt.want, got)
+			}
+		})
+	}
+}

pkg/backend/endpoint_utils.go

@@ -4,6 +4,8 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"time"
+
 	awssdk "github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/aws/awserr"
 	elbv2sdk "github.com/aws/aws-sdk-go/service/elbv2"
@@ -22,7 +24,6 @@ import (
 	"sigs.k8s.io/aws-load-balancer-controller/pkg/networking"
 	"sigs.k8s.io/aws-load-balancer-controller/pkg/runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
-	"time"
 )
 
 const defaultTargetHealthRequeueDuration = 15 * time.Second
@@ -137,7 +138,11 @@ func (m *defaultResourceManager) reconcileWithIPTargetType(ctx context.Context,
 
 func (m *defaultResourceManager) reconcileWithInstanceTargetType(ctx context.Context, tgb *elbv2api.TargetGroupBinding) error {
 	svcKey := buildServiceReferenceKey(tgb, tgb.Spec.ServiceRef)
-	nodeSelector := backend.GetTrafficProxyNodeSelector(tgb)
+	nodeSelector, err := backend.GetTrafficProxyNodeSelector(tgb)
+	if err != nil {
+		return err
+	}
+
 	resolveOpts := []backend.EndpointResolveOption{backend.WithNodeSelector(nodeSelector)}
 	endpoints, err := m.endpointResolver.ResolveNodePortEndpoints(ctx, svcKey, tgb.Spec.ServiceRef.Port, resolveOpts...)
 	if err != nil {

pkg/backend/endpoint_utils_test.go

@@ -2,14 +2,15 @@ package elbv2
 
 import (
 	"context"
+	"strings"
+
 	"github.com/go-logr/logr"
 	"github.com/pkg/errors"
 	"k8s.io/apimachinery/pkg/runtime"
 	elbv2api "sigs.k8s.io/aws-load-balancer-controller/apis/elbv2/v1beta1"
 	"sigs.k8s.io/aws-load-balancer-controller/pkg/webhook"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
-	"strings"
 )
 
 const apiPathValidateELBv2TargetGroupBinding = "/validate-elbv2-k8s-aws-v1beta1-targetgroupbinding"
@@ -36,6 +37,9 @@ func (v *targetGroupBindingValidator) ValidateCreate(ctx context.Context, obj ru
 	if err := v.checkRequiredFields(tgb); err != nil {
 		return err
 	}
+	if err := v.checkNodeSelector(tgb); err != nil {
+		return err
+	}
 	return nil
 }
 
@@ -48,7 +52,9 @@ func (v *targetGroupBindingValidator) ValidateUpdate(ctx context.Context, obj ru
 	if err := v.checkImmutableFields(tgb, oldTgb); err != nil {
 		return err
 	}
-
+	if err := v.checkNodeSelector(tgb); err != nil {
+		return err
+	}
 	return nil
 }
 
@@ -87,6 +93,14 @@ func (v *targetGroupBindingValidator) checkImmutableFields(tgb *elbv2api.TargetG
 	return nil
 }
 
+//checkNodeSelector ensures that NodeSelector is only set when TargetType is ip
+func (v *targetGroupBindingValidator) checkNodeSelector(tgb *elbv2api.TargetGroupBinding) error {
+	if (*tgb.Spec.TargetType == elbv2api.TargetTypeIP) && (tgb.Spec.NodeSelector != nil) {
+		return errors.Errorf("TargetGroupBinding cannot set NodeSelector when TargetType is ip")
+	}
+	return nil
+}
+
 // +kubebuilder:webhook:path=/validate-elbv2-k8s-aws-v1beta1-targetgroupbinding,mutating=false,failurePolicy=fail,groups=elbv2.k8s.aws,resources=targetgroupbindings,verbs=create;update,versions=v1beta1,name=vtargetgroupbinding.elbv2.k8s.aws,sideEffects=None,webhookVersions=v1beta1
 
 func (v *targetGroupBindingValidator) SetupWithManager(mgr ctrl.Manager) {