Merge branch 'fix-node-taint-uniqueness' into 'main'

fix(taint): apply Node taint if not exists already

See merge request cloudnative/go/cidr-allocator!20

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.1] - 2024-02-26
+### Fixed
+- fix(taints): fixed issue where Node taint was being applied irrespective of whether it was already applied
+
 ## [v1.1.0] - 2024-02-22
 ### Added
 - feat(taint): added custom node taint for matching Nodes that don't have a PodCIDR assigned

api/v1alpha1/nodecidrallocation_types.go

@@ -77,6 +77,7 @@ type NodeCIDRAllocationStatus struct {
 // This is a CRD that defines a NodeCIDRAllocation resource which allows for the allocation of PodCIDRs / Pod Subnets to Kubernetes Nodes
 // to be assigned to nodes in a cluster. This is implemented using a list of network CIDRs as blocks of available address space that can be allocated
 // to nodes using a node selector to filter the nodes upon which to apply the Pod CIDRs.
+//
 // +kubebuilder:printcolumn:name="Created",type="date",JSONPath=".metadata.creationTimestamp",description="NodeCIDRAllocation creation timestamp"
 // +kubebuilder:printcolumn:name="Pools",type="string",JSONPath=".spec.addressPools",description="NodeCIDRAllocation Address Pools"
 // +kubebuilder:printcolumn:name="NodeSelector",type="string",JSONPath=".spec.nodeSelector",description="NodeCIDRAllocation NodeSelector value"

install/kubernetes/cidr-allocator/Chart.yaml

@@ -10,9 +10,9 @@ keywords:
   - Networking
   - Node
 
-version: 2.0.3
+version: 2.0.4
 kubeVersion: ">= 1.16.0-0"
-appVersion: "v1.1.0"
+appVersion: "v1.1.1"
 
 maintainers:
   - name: Ben Sykes

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.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)
+![Version: 2.0.4](https://img.shields.io/badge/Version-2.0.4-informational?style=flat-square) ![Type: application](https://img.shields.io/badge/Type-application-informational?style=flat-square) ![AppVersion: v1.1.1](https://img.shields.io/badge/AppVersion-v1.1.1-informational?style=flat-square)
 
 A Helm chart to deploy the STATCAN CIDR-Allocator Controller and CRDs to a Kubernetes Cluster
 

install/kubernetes/cidr-allocator/values.yaml

@@ -11,7 +11,7 @@ image:
   # -- can be one of "Always", "IfNotPresent", "Never"
   pullPolicy: IfNotPresent
   # -- Overrides the image tag whose default is the chart appVersion.
-  # tag: "v1.0.1"
+  # tag: "v1.1.1"
 
 # -- specifies credentials for a private registry to pull source image
 imagePullSecrets: []

internal/controller/nodecidrallocation_controller.go

@@ -42,12 +42,21 @@ import (
 	"statcan.gc.ca/cidr-allocator/internal/helper"
 	statcan_metrics "statcan.gc.ca/cidr-allocator/internal/metrics"
 	statcan_net "statcan.gc.ca/cidr-allocator/internal/networking"
+	"statcan.gc.ca/cidr-allocator/internal/taint"
 )
 
 const (
 	finalizerName = "nodecidrallocation.networking.statcan.gc.ca/finalizer"
 )
 
+var (
+	nodeTaint = corev1.Taint{
+		Key:    "node.networking.statcan.gc.ca/network-unavailable",
+		Value:  "true",
+		Effect: corev1.TaintEffectNoSchedule,
+	}
+)
+
 // NodeCIDRAllocationReconciler reconciles a NodeCIDRAllocation object
 type NodeCIDRAllocationReconciler struct {
 	client.Client
@@ -184,7 +193,9 @@ func (r *NodeCIDRAllocationReconciler) Reconcile(ctx context.Context, req ctrl.R
 
 	if len(matchingNodes.Items) == 0 {
 		rl.V(1).Info("no matching nodes exist. skipping")
-		return ctrl.Result{}, nil
+
+		// nodeCIDRAllocation does not have any matching nodes - return and do not requeue
+		return ctrl.Result{}, r.finalizeReconcile(ctx, &nodeCIDRAllocation, &matchingNodes, nil)
 	}
 
 	// retrieve a list of all Nodes in the cluster.
@@ -197,7 +208,7 @@ func (r *NodeCIDRAllocationReconciler) Reconcile(ctx context.Context, req ctrl.R
 		)
 
 		// could not list Nodes in the cluster - return and requeue
-		return ctrl.Result{}, err
+		return ctrl.Result{}, r.finalizeReconcile(ctx, &nodeCIDRAllocation, &matchingNodes, err)
 	}
 
 	//
@@ -265,7 +276,7 @@ func (r *NodeCIDRAllocationReconciler) Reconcile(ctx context.Context, req ctrl.R
 				&nodeCIDRAllocation,
 				corev1.EventTypeWarning,
 				EventReasonNoAddressSpace,
-				"There are no available subnets for the requested size (/%s). Could not assign PodCIDR to Node (%s)", requiredCIDRMask, node.GetName(),
+				"There are no available subnets for the requested size (/%d). Could not assign PodCIDR to Node (%s)", requiredCIDRMask, node.GetName(),
 			)
 
 			// no available subnet to assign to Node - return and do not requeue
@@ -389,10 +400,19 @@ 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 {
+	ntc := taint.New()
+	for _, node := range nodes.Items {
+		// does not have an assigned PodCIDR - taint the Node
+		if node.Spec.PodCIDR == "" && !ntc.HasTaint(&node, nodeTaint.Key) {
+			ntc.AddNodeTaint(&node, nodeTaint)
+		}
+
+		// has a PodCIDR assigned - remove the taint from the Node
+		if node.Spec.PodCIDR != "" && ntc.HasTaint(&node, nodeTaint.Key) {
+			ntc.RemoveNodeTaint(&node, nodeTaint.Key)
+		}
+
+		if err := r.Client.Update(ctx, &node); err != nil {
 			if apierrors.IsNotFound(err) {
 				// node was removed after reconcile request was created - skip the Node
 				continue
@@ -413,7 +433,7 @@ func (r *NodeCIDRAllocationReconciler) triggerNodeCIDRAllocationReconcileFromNod
 	usedByNodeCIDRAllocation := map[*v1alpha1.NodeCIDRAllocation]struct{}{} // implements a set-like structure to ensure that we only process a single reconcile for each unique match
 
 	// get all the available NodeCIDRAllocations on the cluster
-	if err := r.Client.List(context.TODO(), allNodeCIDRAllocations, &client.ListOptions{
+	if err := r.Client.List(ctx, allNodeCIDRAllocations, &client.ListOptions{
 		Namespace: corev1.NamespaceAll,
 	}); err != nil {
 		return []reconcile.Request{}
@@ -454,7 +474,7 @@ func (r *NodeCIDRAllocationReconciler) SetupWithManager(mgr ctrl.Manager) error
 			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 },
+				DeleteFunc:  func(_ event.DeleteEvent) bool { return true },
 				GenericFunc: func(_ event.GenericEvent) bool { return false },
 			}),
 		).

internal/controller/taint.go → internal/taint/taint.go

@@ -23,44 +23,51 @@ OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
 SOFTWARE.
 */
 
-package controller
+package taint
 
 import (
 	corev1 "k8s.io/api/core/v1"
 )
 
-const (
-	nodeTaintKey = "node.networking.statcan.gc.ca/network-unavailable"
-)
-
+// NodeTainter is an interface that describes how to handle Node taints
 type NodeTainter interface {
-	Handle(*corev1.Node)
+	AddNodeTaint(*corev1.Node, corev1.Taint)
+	RemoveNodeTaint(*corev1.Node, string)
+	HasTaint(*corev1.Node, string) bool
 }
 
+// NodeTaintClient provides a set of related functionality for managing Node taints for the CIDR-Allocator.
+//
+// This implementation is opinionated and designed for use with the CIDR-Allocator
 type NodeTaintClient struct{}
 
+// A blank assignment to ensure conformity to the NodeTainer interface
 var _ NodeTainter = &NodeTaintClient{}
 
-func (n *NodeTaintClient) Handle(node *corev1.Node) {
-	if node.Spec.PodCIDR == "" {
-		n.addNodeTaint(node)
-	} else {
-		n.removeNodeTaintIfExists(node)
+// New creates a new NodeTaintClient and provides a reference to it
+func New() *NodeTaintClient {
+	return &NodeTaintClient{}
+}
+
+// taintExists checks the existing taints on the provided Node for one that has the key provided. If one exists, it returns true
+func (n *NodeTaintClient) HasTaint(node *corev1.Node, key string) bool {
+	for _, t := range node.Spec.Taints {
+		if t.Key == key {
+			return true
+		}
 	}
+
+	return false
 }
 
-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) AddNodeTaint(node *corev1.Node, taint corev1.Taint) {
+	node.Spec.Taints = append(node.Spec.Taints, taint)
 }
 
-func (n *NodeTaintClient) removeNodeTaintIfExists(node *corev1.Node) {
+func (n *NodeTaintClient) RemoveNodeTaint(node *corev1.Node, key string) {
 	taints := []corev1.Taint{}
 	for _, taint := range node.Spec.Taints {
-		if taint.Key != nodeTaintKey {
+		if taint.Key != key {
 			taints = append(taints, taint)
 		}
 	}

internal/controller/taint_test.go → internal/taint/taint_test.go

@@ -23,47 +23,75 @@ OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
 SOFTWARE.
 */
 
-package controller_test
+package taint_test
 
 import (
 	"testing"
 
 	corev1 "k8s.io/api/core/v1"
-	"statcan.gc.ca/cidr-allocator/internal/controller"
+	"statcan.gc.ca/cidr-allocator/internal/taint"
 )
 
-func TestNodeTaintClientHandleNoPodCIDR(t *testing.T) {
+const nodeTaintKey = "abc"
+
+func TestNodeTaintClientHasTaint(t *testing.T) {
 	node := &corev1.Node{
 		Spec: corev1.NodeSpec{
-			PodCIDR: "",
+			Taints: []corev1.Taint{
+				{
+					Key: nodeTaintKey,
+				},
+			},
 		},
 	}
 
-	ntc := &controller.NodeTaintClient{}
-	ntc.Handle(node)
+	ntc := taint.New()
+
+	want := true
+	got := ntc.HasTaint(node, nodeTaintKey)
+
+	if got != want {
+		t.Errorf("got %t, wanted %t", got, want)
+	}
+
+	want = false
+	got = ntc.HasTaint(node, "xyz")
+
+	if got != want {
+		t.Errorf("got %t, wanted %t", got, want)
+	}
+}
+
+func TestNodeTaintClientAddNodeTaint(t *testing.T) {
+	node := &corev1.Node{
+		Spec: corev1.NodeSpec{},
+	}
+
+	ntc := taint.New()
+	ntc.AddNodeTaint(node, corev1.Taint{Key: nodeTaintKey, Value: "true"})
 
-	want := 1
-	got := len(node.Spec.Taints)
+	want := true
+	got := len(node.Spec.Taints) == 1 && node.Spec.Taints[0].Key == nodeTaintKey
 
 	if got != want {
-		t.Errorf("got %d, wanted %d", got, want)
+		t.Errorf("got %v, wanted %v", node.Spec.Taints, []corev1.Taint{{Key: nodeTaintKey, Value: "true"}})
 	}
 }
 
-func TestNodeTaintClientHandlePodCIDRAssigned(t *testing.T) {
+func TestNodeTaintClientRemoveNodeTaint(t *testing.T) {
 	node := &corev1.Node{
 		Spec: corev1.NodeSpec{
-			PodCIDR: "10.0.0.0/24",
+			Taints: []corev1.Taint{{Key: nodeTaintKey}},
 		},
 	}
 
-	ntc := &controller.NodeTaintClient{}
-	ntc.Handle(node)
+	ntc := taint.New()
+	ntc.RemoveNodeTaint(node, nodeTaintKey)
 
-	want := 0
-	got := len(node.Spec.Taints)
+	want := true
+	got := len(node.Spec.Taints) == 0
 
 	if got != want {
-		t.Errorf("got %d, wanted %d", got, want)
+		t.Errorf("got %v, wanted %v", node.Spec.Taints, []corev1.Taint{})
 	}
 }