Merge branch 'feat-node-taint-when-not-ready' into 'main'

Ben Sykes · Ben Sykes · commit 19c78ff24de9 · 2024-02-22T18:54:17.000Z
feat(controller): added node taint for matching nodes without a PodCIDR

See merge request cloudnative/go/cidr-allocator!19
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
@@ -26,6 +26,6 @@ helm:
   trigger:
     include:
       - project: cloudnative/gitlab/gitlabci/helm-chart
-        ref: v3.x
+        ref: v4.x
         file: .helm.gitlab-ci.yml
     strategy: depend
diff --git a/.golangci.yml b/.golangci.yml
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5,6 +5,10 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## [v1.1.0] - 2024-02-22
+### Added
+- feat(taint): added custom node taint for matching Nodes that don't have a PodCIDR assigned
+
 ## [v1.0.1] - 2024-02-16
 ### Fixed
 - fix(controller): bad handling of Event Recorder
diff --git a/cmd/main.go b/cmd/main.go
@@ -63,8 +63,8 @@ var (
 	probeAddr string
 	// enableLeaderElection specifies whether or not leader election should be used for the controller manager
 	enableLeaderElection bool
-	// developmentLogging specifies whether to enable Development (Debug) logging for ZAP. Otherwise, Zap Production logging will be used
-	developmentLogging bool
+	// debugLogging specifies whether to enable Development (Debug) logging for ZAP. Otherwise, Zap Production logging will be used
+	debugLogging bool
 	// secureMetrics specifies whether the metrics endpoint is served over https
 	secureMetrics bool
 	// enableHTTP2 specifies that HTTP/2 will be enabled for the metrics and webhook servers (if exists)
@@ -98,8 +98,8 @@ func init() {
 		"Enable leader election for controller manager. Enabling this will ensure there is only one active controller manager.",
 	)
 	flag.BoolVar(
-		&developmentLogging,
-		"dev-logging",
+		&debugLogging,
+		"debug",
 		false,
 		"Enable development logging",
 	)
@@ -117,7 +117,7 @@ func init() {
 	)
 
 	opts := zap.Options{
-		Development: developmentLogging,
+		Development: debugLogging,
 	}
 	opts.BindFlags(flag.CommandLine)
 	flag.Parse()
diff --git a/config/default/manager_config_patch.yaml b/config/default/manager_config_patch.yaml
@@ -13,4 +13,5 @@ spec:
         args:
         - --leader-elect
         - --disable-webhooks
+        - --debug
         imagePullPolicy: Never
diff --git a/go.mod b/go.mod
@@ -6,6 +6,7 @@ require (
 	github.com/c-robinson/iplib v1.0.8
 	github.com/prometheus/client_golang v1.18.0
 	github.com/prometheus/client_model v0.5.0
+	golang.org/x/exp v0.0.0-20220722155223-a9213eeb770e
 	k8s.io/api v0.29.0
 	k8s.io/apimachinery v0.29.0
 	k8s.io/client-go v0.29.0
@@ -45,7 +46,6 @@ require (
 	github.com/spf13/pflag v1.0.5 // indirect
 	go.uber.org/multierr v1.11.0 // indirect
 	go.uber.org/zap v1.26.0 // indirect
-	golang.org/x/exp v0.0.0-20220722155223-a9213eeb770e // indirect
 	golang.org/x/net v0.20.0 // indirect
 	golang.org/x/oauth2 v0.12.0 // indirect
 	golang.org/x/sys v0.16.0 // indirect
diff --git a/install/kubernetes/cidr-allocator/Chart.yaml b/install/kubernetes/cidr-allocator/Chart.yaml
@@ -10,9 +10,9 @@ keywords:
   - Networking
   - Node
 
-version: 2.0.2
+version: 2.0.3
 kubeVersion: ">= 1.16.0-0"
-appVersion: "v1.0.1"
+appVersion: "v1.1.0"
 
 maintainers:
   - name: Ben Sykes
diff --git a/install/kubernetes/cidr-allocator/README.md b/install/kubernetes/cidr-allocator/README.md
@@ -2,7 +2,7 @@
 
 A Helm chart to deploy the STATCAN CIDR-Allocator Controller and CRDs to a Kubernetes Cluster
 
-![Version: 2.0.2](https://img.shields.io/badge/Version-2.0.2-informational?style=flat-square) ![Type: application](https://img.shields.io/badge/Type-application-informational?style=flat-square) ![AppVersion: v1.0.1](https://img.shields.io/badge/AppVersion-v1.0.1-informational?style=flat-square)
+![Version: 2.0.3](https://img.shields.io/badge/Version-2.0.3-informational?style=flat-square) ![Type: application](https://img.shields.io/badge/Type-application-informational?style=flat-square) ![AppVersion: v1.1.0](https://img.shields.io/badge/AppVersion-v1.1.0-informational?style=flat-square)
 
 A Helm chart to deploy the STATCAN CIDR-Allocator Controller and CRDs to a Kubernetes Cluster
 
diff --git a/internal/controller/nodecidrallocation_controller.go b/internal/controller/nodecidrallocation_controller.go
@@ -18,6 +18,7 @@ package controller
 
 import (
 	"context"
+	"errors"
 
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -28,6 +29,7 @@ import (
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
+	"sigs.k8s.io/controller-runtime/pkg/event"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/metrics"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
@@ -198,10 +200,9 @@ func (r *NodeCIDRAllocationReconciler) Reconcile(ctx context.Context, req ctrl.R
 		return ctrl.Result{}, err
 	}
 
-	rl.Info(
-		"reconciling matching Nodes with NodeCIDRAllocation ...",
-		"nodeCIDRAllocation", nodeCIDRAllocation.GetName(),
-	)
+	//
+	// Begin allocation process
+	//
 
 	freeSubnets := make(map[uint8][]string)
 	for _, node := range matchingNodes.Items {
@@ -323,6 +324,9 @@ func (r *NodeCIDRAllocationReconciler) finalizeReconcile(ctx context.Context, no
 	r.updateNodeCIDRAllocationStatus(ctx, nodeCIDRAllocation, nodes, err)
 	r.updatePrometheusMetrics(ctx)
 
+	// remove or add Node taints according to allocation status of each matching node
+	err = errors.Join(err, r.updateNodeTaints(ctx, nodes))
+
 	// passthrough for err (if non-nil) to the Reconcile Result
 	return err
 }
@@ -384,6 +388,24 @@ func (r *NodeCIDRAllocationReconciler) updateNodeCIDRAllocationStatus(ctx contex
 	}
 }
 
+func (r *NodeCIDRAllocationReconciler) updateNodeTaints(ctx context.Context, nodes *corev1.NodeList) error {
+	ntc := &NodeTaintClient{}
+	for _, n := range nodes.Items {
+		ntc.Handle(&n)
+		if err := r.Client.Update(ctx, &n); err != nil {
+			if apierrors.IsNotFound(err) {
+				// node was removed after reconcile request was created - skip the Node
+				continue
+			}
+
+			// error trying to add the Node taint to the Node object
+			return err
+		}
+	}
+
+	return nil
+}
+
 // triggerNodeCIDRAllocationReconcileFromNodeChange is a mapping function which takes a Node object
 // and returns a list of reconciliation requests for all NodeCIDRAllocation resources that have a matching NodeSelector
 func (r *NodeCIDRAllocationReconciler) triggerNodeCIDRAllocationReconcileFromNodeChange(ctx context.Context, o client.Object) []reconcile.Request {
@@ -429,9 +451,12 @@ func (r *NodeCIDRAllocationReconciler) SetupWithManager(mgr ctrl.Manager) error
 		Watches(
 			&corev1.Node{},
 			handler.EnqueueRequestsFromMapFunc(r.triggerNodeCIDRAllocationReconcileFromNodeChange),
-			builder.WithPredicates(predicate.Or(
-				predicate.LabelChangedPredicate{},
-			)),
+			builder.WithPredicates(predicate.Funcs{
+				CreateFunc:  func(_ event.CreateEvent) bool { return true },
+				UpdateFunc:  func(_ event.UpdateEvent) bool { return false },
+				DeleteFunc:  func(_ event.DeleteEvent) bool { return false },
+				GenericFunc: func(_ event.GenericEvent) bool { return false },
+			}),
 		).
 		Complete(r)
 }
diff --git a/internal/controller/taint.go b/internal/controller/taint.go
@@ -0,0 +1,69 @@
+/*
+MIT License
+
+Copyright (c) His Majesty the King in Right of Canada, as represented by the
+Minister responsible for Statistics Canada, 2024
+
+Permission is hereby granted, free of charge, to any person obtaining a copy
+of this software and associated documentation files (the "Software"), to deal
+in the Software without restriction, including without limitation the rights
+to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+copies of the Software, and to permit persons to whom the Software is
+furnished to do so, subject to the following conditions:
+
+The above copyright notice and this permission notice shall be included in all
+copies or substantial portions of the Software.
+
+THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+SOFTWARE.
+*/
+
+package controller
+
+import (
+	corev1 "k8s.io/api/core/v1"
+)
+
+const (
+	nodeTaintKey = "node.networking.statcan.gc.ca/network-unavailable"
+)
+
+type NodeTainter interface {
+	Handle(*corev1.Node)
+}
+
+type NodeTaintClient struct{}
+
+var _ NodeTainter = &NodeTaintClient{}
+
+func (n *NodeTaintClient) Handle(node *corev1.Node) {
+	if node.Spec.PodCIDR == "" {
+		n.addNodeTaint(node)
+	} else {
+		n.removeNodeTaintIfExists(node)
+	}
+}
+
+func (n *NodeTaintClient) addNodeTaint(node *corev1.Node) {
+	node.Spec.Taints = append(node.Spec.Taints, corev1.Taint{
+		Key:    nodeTaintKey,
+		Value:  "true",
+		Effect: corev1.TaintEffectNoSchedule,
+	})
+}
+
+func (n *NodeTaintClient) removeNodeTaintIfExists(node *corev1.Node) {
+	taints := []corev1.Taint{}
+	for _, taint := range node.Spec.Taints {
+		if taint.Key != nodeTaintKey {
+			taints = append(taints, taint)
+		}
+	}
+
+	node.Spec.Taints = taints
+}
diff --git a/internal/controller/taint_test.go b/internal/controller/taint_test.go
@@ -0,0 +1,69 @@
+/*
+MIT License
+
+Copyright (c) His Majesty the King in Right of Canada, as represented by the
+Minister responsible for Statistics Canada, 2024
+
+Permission is hereby granted, free of charge, to any person obtaining a copy
+of this software and associated documentation files (the "Software"), to deal
+in the Software without restriction, including without limitation the rights
+to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+copies of the Software, and to permit persons to whom the Software is
+furnished to do so, subject to the following conditions:
+
+The above copyright notice and this permission notice shall be included in all
+copies or substantial portions of the Software.
+
+THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+SOFTWARE.
+*/
+
+package controller_test
+
+import (
+	"testing"
+
+	corev1 "k8s.io/api/core/v1"
+	"statcan.gc.ca/cidr-allocator/internal/controller"
+)
+
+func TestNodeTaintClientHandleNoPodCIDR(t *testing.T) {
+	node := &corev1.Node{
+		Spec: corev1.NodeSpec{
+			PodCIDR: "",
+		},
+	}
+
+	ntc := &controller.NodeTaintClient{}
+	ntc.Handle(node)
+
+	want := 1
+	got := len(node.Spec.Taints)
+
+	if got != want {
+		t.Errorf("got %d, wanted %d", got, want)
+	}
+}
+
+func TestNodeTaintClientHandlePodCIDRAssigned(t *testing.T) {
+	node := &corev1.Node{
+		Spec: corev1.NodeSpec{
+			PodCIDR: "10.0.0.0/24",
+		},
+	}
+
+	ntc := &controller.NodeTaintClient{}
+	ntc.Handle(node)
+
+	want := 0
+	got := len(node.Spec.Taints)
+
+	if got != want {
+		t.Errorf("got %d, wanted %d", got, want)
+	}
+}
diff --git a/internal/helper/helper.go b/internal/helper/helper.go
@@ -56,13 +56,3 @@ func Keys[K, V comparable](m map[K]V) []K {
 
 	return keys
 }
-
-// RemoveByVal rebuilds the supplied list of items with the specified item removed
-func RemoveByVal[T comparable](l []T, item T) []T {
-	for i, other := range l {
-		if other == item {
-			return append(l[:i], l[i+1:]...)
-		}
-	}
-	return l
-}
diff --git a/internal/helper/helper_test.go b/internal/helper/helper_test.go
@@ -19,6 +19,7 @@ package helper_test
 import (
 	"testing"
 
+	"golang.org/x/exp/slices"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"statcan.gc.ca/cidr-allocator/internal/helper"
@@ -106,3 +107,31 @@ func TestStringInSlice(t *testing.T) {
 		t.Errorf("got %t, wanted %t", got, want)
 	}
 }
+
+func TestKeys(t *testing.T) {
+	m := make(map[uint8]struct{}, 3)
+	m[1] = struct{}{}
+	m[2] = struct{}{}
+	m[3] = struct{}{}
+
+	got := helper.Keys(m)
+	want := []uint8{1, 2, 3}
+
+	slices.Sort(got)
+
+	if len(got) != len(want) {
+		t.Errorf("got %d, wanted %d", got, want)
+	}
+
+	if got[0] != want[0] {
+		t.Errorf("got %d, wanted %d", got, want)
+	}
+
+	if got[1] != want[1] {
+		t.Errorf("got %d, wanted %d", got, want)
+	}
+
+	if got[2] != want[2] {
+		t.Errorf("got %d, wanted %d", got, want)
+	}
+}
diff --git a/internal/metrics/metrics.go b/internal/metrics/metrics.go
@@ -91,7 +91,7 @@ func Update(nodeCIDRAllocations *v1alpha1.NodeCIDRAllocationList, allNodes *core
 
 	for _, n := range allNodes.Items {
 		if n.Spec.PodCIDR == "" {
-			notAllocated += 1
+			notAllocated++
 			continue
 		}
 		nodeAllocationsCumulative[n.Spec.PodCIDR] = struct{}{}
@@ -146,11 +146,11 @@ func GetMetricValue(col prometheus.Collector) float64 {
 	}
 
 	if _, ok := col.(prometheus.Gauge); ok {
-		return *m.Gauge.Value
+		return m.GetGauge().GetValue()
 	}
 
 	if _, ok := col.(prometheus.Counter); ok {
-		return *m.Counter.Value
+		return m.GetCounter().GetValue()
 	}
 
 	return -1
diff --git a/internal/metrics/metrics_priv_test.go b/internal/metrics/metrics_priv_test.go
@@ -23,7 +23,7 @@ func TestAccumulatedHosts(t *testing.T) {
 
 	// Case 1: Valid CIDRs are provided and specify /26 and /27 as CIDR block size
 	// expected: 64 + 32 = 96
-	var got uint32 = accumulatedHosts(cidrs)
+	var got = accumulatedHosts(cidrs)
 	var want uint32 = 96
 
 	if got != want {
diff --git a/internal/metrics/metrics_test.go b/internal/metrics/metrics_test.go
diff --git a/internal/networking/networking.go b/internal/networking/networking.go
