-
Notifications
You must be signed in to change notification settings - Fork 0
feat: add nodeselector annotation to limit LB pool members #33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| # Deploys the docker.io/nginxdemos/hello:plain-text container and creates a | ||
| # loadbalancer service with a node-selector annotation for it: | ||
| # | ||
| # export KUBECONFIG=path/to/kubeconfig | ||
| # kubectl apply -f nginx-hello.yml | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: |
||
| # | ||
| # Wait for `kubectl describe service hello` to show "Loadbalancer Ensured", | ||
| # then use the IP address found under "LoadBalancer Ingress" to connect to the | ||
| # service. | ||
| # | ||
| # You can also use the following shortcut: | ||
| # | ||
| # curl http://$(kubectl get service hello -o jsonpath='{.status.loadBalancer.ingress[0].ip}') | ||
| # | ||
| # If you follow the nginx log, you will see that nginx sees a cluster internal | ||
| # IP address as source of requests: | ||
| # | ||
| # kubectl logs -l "app=hello" | ||
| # | ||
| --- | ||
| apiVersion: apps/v1 | ||
| kind: Deployment | ||
| metadata: | ||
| name: hello | ||
| spec: | ||
| replicas: 2 | ||
| selector: | ||
| matchLabels: | ||
| app: hello | ||
| template: | ||
| metadata: | ||
| labels: | ||
| app: hello | ||
| spec: | ||
| containers: | ||
| - name: hello | ||
| image: docker.io/nginxdemos/hello:plain-text | ||
| nodeSelector: | ||
| kubernetes.io/hostname: k8test-worker-2 | ||
| --- | ||
| apiVersion: v1 | ||
| kind: Service | ||
| metadata: | ||
| labels: | ||
| app: hello | ||
| annotations: | ||
| k8s.cloudscale.ch/loadbalancer-node-selector: "kubernetes.io/hostname=k8test-worker-2" | ||
| name: hello | ||
| spec: | ||
| ports: | ||
| - port: 80 | ||
| protocol: TCP | ||
| targetPort: 80 | ||
| name: http | ||
| selector: | ||
| app: hello | ||
| type: LoadBalancer | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: I am wondering if this example would be more interesting with |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,6 @@ package cloudscale_ccm | |
|
|
||
| import ( | ||
| "context" | ||
| "errors" | ||
| "fmt" | ||
| "slices" | ||
| "strings" | ||
|
|
@@ -11,7 +10,9 @@ import ( | |
| "github.com/cloudscale-ch/cloudscale-go-sdk/v6" | ||
| v1 "k8s.io/api/core/v1" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/apimachinery/pkg/labels" | ||
| "k8s.io/client-go/kubernetes" | ||
| "k8s.io/client-go/tools/record" | ||
| "k8s.io/klog/v2" | ||
| "k8s.io/utils/ptr" | ||
| ) | ||
|
|
@@ -208,7 +209,7 @@ const ( | |
| // connections timing out while the monitor is updated. | ||
| LoadBalancerHealthMonitorTimeoutS = "k8s.cloudscale.ch/loadbalancer-health-monitor-timeout-s" | ||
|
|
||
| // LoadBalancerHealthMonitorDownThreshold is the number of the checks that | ||
| // LoadBalancerHealthMonitorUpThreshold is the number of the checks that | ||
| // need to succeed before a pool member is considered up. Defaults to 2. | ||
| LoadBalancerHealthMonitorUpThreshold = "k8s.cloudscale.ch/loadbalancer-health-monitor-up-threshold" | ||
|
|
||
|
|
@@ -278,7 +279,7 @@ const ( | |
| // Changing this annotation on an established service is considered safe. | ||
| LoadBalancerListenerTimeoutMemberDataMS = "k8s.cloudscale.ch/loadbalancer-timeout-member-data-ms" | ||
|
|
||
| // LoadBalancerSubnetLimit is a JSON list of subnet UUIDs that the | ||
| // LoadBalancerListenerAllowedSubnets is a JSON list of subnet UUIDs that the | ||
| // loadbalancer should use. By default, all subnets of a node are used: | ||
| // | ||
| // * `[]` means that anyone is allowed to connect (default). | ||
|
|
@@ -291,12 +292,17 @@ const ( | |
| // This is an advanced feature, useful if you have nodes that are in | ||
| // multiple private subnets. | ||
| LoadBalancerListenerAllowedSubnets = "k8s.cloudscale.ch/loadbalancer-listener-allowed-subnets" | ||
|
|
||
| // LoadBalancerNodeSelector can be set to restrict which nodes are added to the LB pool. | ||
| // It accepts a standard Kubernetes label selector string. | ||
| LoadBalancerNodeSelector = "k8s.cloudscale.ch/loadbalancer-node-selector" | ||
| ) | ||
|
|
||
| type loadbalancer struct { | ||
| lbs lbMapper | ||
| srv serverMapper | ||
| k8s kubernetes.Interface | ||
| lbs lbMapper | ||
| srv serverMapper | ||
| k8s kubernetes.Interface | ||
| recorder record.EventRecorder | ||
| } | ||
|
|
||
| // GetLoadBalancer returns whether the specified load balancer exists, and | ||
|
|
@@ -387,16 +393,23 @@ func (l *loadbalancer) EnsureLoadBalancer( | |
| return nil, err | ||
| } | ||
|
|
||
| // Refuse to do anything if there are no nodes | ||
| nodes, err := filterNodesBySelector(serviceInfo, nodes) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if len(nodes) == 0 { | ||
| return nil, errors.New( | ||
| "no valid nodes for service found, please verify there is " + | ||
| "at least one that allows load balancers", | ||
| l.recorder.Event( | ||
| service, | ||
| v1.EventTypeWarning, | ||
| "NoValidNodes", | ||
| "No valid nodes for service found, "+ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: can we include the configured value from |
||
| "double-check node-selector annotation", | ||
| ) | ||
| } | ||
|
|
||
| // Reconcile | ||
| err := reconcileLbState(ctx, l.lbs.client, func() (*lbState, error) { | ||
| err = reconcileLbState(ctx, l.lbs.client, func() (*lbState, error) { | ||
| // Get the desired state from Kubernetes | ||
| servers, err := l.srv.mapNodes(ctx, nodes).All() | ||
| if err != nil { | ||
|
|
@@ -442,6 +455,28 @@ func (l *loadbalancer) EnsureLoadBalancer( | |
| return result, nil | ||
| } | ||
|
|
||
| func filterNodesBySelector( | ||
| serviceInfo *serviceInfo, | ||
| nodes []*v1.Node, | ||
| ) ([]*v1.Node, error) { | ||
| selector := labels.Everything() | ||
| if v := serviceInfo.annotation(LoadBalancerNodeSelector); v != "" { | ||
| var err error | ||
| selector, err = labels.Parse(v) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("unable to parse selector: %w", err) | ||
| } | ||
| } | ||
| selectedNodes := make([]*v1.Node, 0, len(nodes)) | ||
| for _, node := range nodes { | ||
| if selector.Matches(labels.Set(node.Labels)) { | ||
| selectedNodes = append(selectedNodes, node) | ||
| } | ||
| } | ||
|
|
||
| return selectedNodes, nil | ||
| } | ||
|
|
||
| // UpdateLoadBalancer updates hosts under the specified load balancer. | ||
| // Implementations must treat the *v1.Service and *v1.Node | ||
| // parameters as read-only and not modify them. | ||
|
|
@@ -461,6 +496,21 @@ func (l *loadbalancer) UpdateLoadBalancer( | |
| return err | ||
| } | ||
|
|
||
| nodes, err := filterNodesBySelector(serviceInfo, nodes) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: introduce new variable such as |
||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if len(nodes) == 0 { | ||
| l.recorder.Event( | ||
| service, | ||
| v1.EventTypeWarning, | ||
| "NoValidNodes", | ||
| "No valid nodes for service found, "+ | ||
| "double-check node-selector annotation", | ||
| ) | ||
| } | ||
|
|
||
| // Reconcile | ||
| return reconcileLbState(ctx, l.lbs.client, func() (*lbState, error) { | ||
| // Get the desired state from Kubernetes | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1156,10 +1156,10 @@ func TestLimitSubnets(t *testing.T) { | |
| assert.Equal(t, "10.0.1.1", state.members[state.pools[0]][0].Address) | ||
| assert.Equal(t, "10.0.1.2", state.members[state.pools[0]][1].Address) | ||
|
|
||
| // If we have no valid addresses, we get an error | ||
| // If we have no valid addresses, we get no error | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since a typo in a label selector now causes immediate downtime (by wiping the pool) instead of a "fail-safe" error, could we ensure this behavioral change is clearly highlighted in the documentation and release notes? |
||
| s.Annotations[LoadBalancerListenerAllowedSubnets] = ` | ||
| ["00000000-0000-0000-0000-000000000003"]` | ||
|
|
||
| _, err = desiredLbState(i, nodes, servers) | ||
| assert.Error(t, err) | ||
| assert.NoError(t, err) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMHO unit tests for the new annotations should be added to this file. |
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this might not be the correct category. Firstly, the your implementation does not recreate the pools. Secondly, if executed correctly, this change should not result in any downtime (the approach would be to schedule pods on the new node, then add the new node and remove the old one).