Use hash for AzureClusterIdentity finalizer

Author: CecileRobertMichon

controllers/helpers.go

@@ -18,6 +18,8 @@ package controllers
 
 import (
 	"context"
+	"crypto/sha256"
+	"encoding/hex"
 	"encoding/json"
 	"fmt"
 
@@ -601,10 +603,21 @@ func GetClusterIdentityFromRef(ctx context.Context, c client.Client, azureCluste
 	return nil, nil
 }
 
-func clusterIdentityFinalizer(prefix, clusterNamespace, clusterName string) string {
+// deprecatedClusterIdentityFinalizer is was briefly used to compute a finalizer without a hash in releases v1.5.1 and v1.4.4.
+// It is kept here to ensure that we can remove it from existing clusters for backwards compatibility.
+// This function should be removed in a future release.
+func deprecatedClusterIdentityFinalizer(prefix, clusterNamespace, clusterName string) string {
 	return fmt.Sprintf("%s/%s-%s", prefix, clusterNamespace, clusterName)
 }
 
+// clusterIdentityFinalizer generates a finalizer key.
+// The finalizer key is a combination of the prefix and a hash of the cluster name and namespace.
+// We use a hash to ensure that the finalizer key name is not longer than 63 characters.
+func clusterIdentityFinalizer(prefix, clusterNamespace, clusterName string) string {
+	hash := sha256.Sum224([]byte(fmt.Sprintf("%s-%s", clusterNamespace, clusterName)))
+	return fmt.Sprintf("%s/%s", prefix, hex.EncodeToString(hash[:]))
+}
+
 // EnsureClusterIdentity ensures that the identity ref is allowed in the namespace and sets a finalizer.
 func EnsureClusterIdentity(ctx context.Context, c client.Client, object conditions.Setter, identityRef *corev1.ObjectReference, finalizerPrefix string) error {
 	name := object.GetName()
@@ -621,6 +634,8 @@ func EnsureClusterIdentity(ctx context.Context, c client.Client, object conditio
 	if err != nil {
 		return errors.Wrap(err, "failed to init patch helper")
 	}
+	// Remove deprecated finalizer if it exists.
+	controllerutil.RemoveFinalizer(identity, deprecatedClusterIdentityFinalizer(finalizerPrefix, namespace, name))
 	// If the AzureClusterIdentity doesn't have our finalizer, add it.
 	controllerutil.AddFinalizer(identity, clusterIdentityFinalizer(finalizerPrefix, namespace, name))
 	// Register the finalizer immediately to avoid orphaning Azure resources on delete.

controllers/helpers_test.go

@@ -20,6 +20,7 @@ import (
 	"context"
 	"fmt"
 	"os"
+	"strings"
 	"testing"
 
 	"github.com/Azure/go-autorest/autorest"
@@ -677,3 +678,86 @@ const (
     "cloudProviderBackoffJitter": 1.2000000000000002
 }`
 )
+
+func Test_clusterIdentityFinalizer(t *testing.T) {
+	type args struct {
+		prefix           string
+		clusterNamespace string
+		clusterName      string
+	}
+	tests := []struct {
+		name string
+		args args
+		want string
+	}{
+		{
+			name: "cluster identity finalizer should be deterministic",
+			args: args{
+				prefix:           infrav1.ClusterFinalizer,
+				clusterNamespace: "foo",
+				clusterName:      "bar",
+			},
+			want: "azurecluster.infrastructure.cluster.x-k8s.io/48998dbcd8fb929369c78981cbfb6f26145ea0412e6e05a1423941a6",
+		},
+		{
+			name: "long cluster name and namespace",
+			args: args{
+				prefix:           infrav1.ClusterFinalizer,
+				clusterNamespace: "this-is-a-very-very-very-very-very-very-very-very-very-long-namespace-name",
+				clusterName:      "this-is-a-very-very-very-very-very-very-very-very-very-long-cluster-name",
+			},
+			want: "azurecluster.infrastructure.cluster.x-k8s.io/557d064144d2b495db694dedc53c9a1e9bd8575bdf06b5b151972614",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := clusterIdentityFinalizer(tt.args.prefix, tt.args.clusterNamespace, tt.args.clusterName)
+			if got != tt.want {
+				t.Errorf("clusterIdentityFinalizer() = %v, want %v", got, tt.want)
+			}
+			key := strings.Split(got, "/")[1]
+			if len(key) > 63 {
+				t.Errorf("clusterIdentityFinalizer() name %v length = %v should be less than 63 characters", key, len(key))
+			}
+		})
+	}
+}
+
+func Test_deprecatedClusterIdentityFinalizer(t *testing.T) {
+	type args struct {
+		prefix           string
+		clusterNamespace string
+		clusterName      string
+	}
+	tests := []struct {
+		name string
+		args args
+		want string
+	}{
+		{
+			name: "cluster identity finalizer should be deterministic",
+			args: args{
+				prefix:           infrav1.ClusterFinalizer,
+				clusterNamespace: "foo",
+				clusterName:      "bar",
+			},
+			want: "azurecluster.infrastructure.cluster.x-k8s.io/foo-bar",
+		},
+		{
+			name: "long cluster name and namespace",
+			args: args{
+				prefix:           infrav1.ClusterFinalizer,
+				clusterNamespace: "this-is-a-very-very-very-very-very-very-very-very-very-long-namespace-name",
+				clusterName:      "this-is-a-very-very-very-very-very-very-very-very-very-long-cluster-name",
+			},
+			want: "azurecluster.infrastructure.cluster.x-k8s.io/this-is-a-very-very-very-very-very-very-very-very-very-long-namespace-name-this-is-a-very-very-very-very-very-very-very-very-very-long-cluster-name",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := deprecatedClusterIdentityFinalizer(tt.args.prefix, tt.args.clusterNamespace, tt.args.clusterName); got != tt.want {
+				t.Errorf("deprecatedClusterIdentityFinalizer() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}