Fix uniqueness bug with RoleAssignment owned by ARM ID

Fix bug where RoleAssignment owned by ARM ID doesn't account for the ARM ID in the seed of the random UUID generate. This bugfix is BREAKING if the owner is using ARM ID and in the following cases: * User migrates RoleAssignment from one cluster to another. * User sets reconcile-policy: skip, deletes the RoleAssignment and then recreates it. In the above two cases, the new correct algorithm will consider the ARM ID of the owner and generate a different UUID than before. Other cases such as standard updates will not be impacted as Kubernetes sends the WHOLE object to the mutating webhook and for updates the object contains the (old) generated UUID.
Azure · Jun 11, 2024 · 197c54d · 197c54d
1 parent cf2bb3b
commit 197c54d
Show file tree

Hide file tree

Showing 6 changed files with 173 additions and 30 deletions.
diff --git a/v2/api/authorization/v1api20200801preview/role_assignment_defaults.go b/v2/api/authorization/v1api20200801preview/role_assignment_defaults.go
@@ -43,13 +43,11 @@ func (assignment *RoleAssignment) defaultAzureName() {
 	}
 
 	if assignment.AzureName() == "" {
-		ownerGK := assignment.Owner().GroupKind()
 		gk := assignment.GroupVersionKind().GroupKind()
 		assignment.Spec.AzureName = randextensions.MakeUUIDName(
 			assignment.Name,
-			randextensions.MakeUniqueOwnerScopedString(
-				ownerGK,
-				assignment.Spec.Owner.Name,
+			randextensions.MakeUniqueOwnerScopedStringLegacy(
+				assignment.Owner(),
 				gk,
 				assignment.Namespace,
 				assignment.Name))

diff --git a/v2/api/authorization/v1api20220401/role_assignment_defaults.go b/v2/api/authorization/v1api20220401/role_assignment_defaults.go
@@ -43,13 +43,11 @@ func (assignment *RoleAssignment) defaultAzureName() {
 	}
 
 	if assignment.AzureName() == "" {
-		ownerGK := assignment.Owner().GroupKind()
 		gk := assignment.GroupVersionKind().GroupKind()
 		assignment.Spec.AzureName = randextensions.MakeUUIDName(
 			assignment.Name,
-			randextensions.MakeUniqueOwnerScopedString(
-				ownerGK,
-				assignment.Spec.Owner.Name,
+			randextensions.MakeUniqueOwnerScopedStringLegacy(
+				assignment.Owner(),
 				gk,
 				assignment.Namespace,
 				assignment.Name))

diff --git a/v2/api/documentdb/v1api20210515/sql_role_assignment_defaults.go b/v2/api/documentdb/v1api20210515/sql_role_assignment_defaults.go
@@ -36,13 +36,11 @@ func (assignment *SqlRoleAssignment) defaultAzureName() {
 	}
 
 	if assignment.AzureName() == "" {
-		ownerGK := assignment.Owner().GroupKind()
 		gk := assignment.GroupVersionKind().GroupKind()
 		assignment.Spec.AzureName = randextensions.MakeUUIDName(
 			assignment.Name,
-			randextensions.MakeUniqueOwnerScopedString(
-				ownerGK,
-				assignment.Spec.Owner.Name,
+			randextensions.MakeUniqueOwnerScopedStringLegacy(
+				assignment.Owner(),
 				gk,
 				assignment.Namespace,
 				assignment.Name))

diff --git a/v2/api/documentdb/v1api20231115/sql_role_assignment_defaults.go b/v2/api/documentdb/v1api20231115/sql_role_assignment_defaults.go
@@ -36,13 +36,11 @@ func (assignment *SqlRoleAssignment) defaultAzureName() {
 	}
 
 	if assignment.AzureName() == "" {
-		ownerGK := assignment.Owner().GroupKind()
 		gk := assignment.GroupVersionKind().GroupKind()
 		assignment.Spec.AzureName = randextensions.MakeUUIDName(
 			assignment.Name,
-			randextensions.MakeUniqueOwnerScopedString(
-				ownerGK,
-				assignment.Spec.Owner.Name,
+			randextensions.MakeUniqueOwnerScopedStringLegacy(
+				assignment.Owner(),
 				gk,
 				assignment.Namespace,
 				assignment.Name))

diff --git a/v2/internal/util/randextensions/uuid5.go b/v2/internal/util/randextensions/uuid5.go
@@ -5,36 +5,71 @@ package randextensions
 
 import (
 	"fmt"
+	"strings"
 
 	"github.com/google/uuid"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+
+	"github.com/Azure/azure-service-operator/v2/pkg/genruntime"
 )
 
 // Namespace is the ASOv2 UUIDv5 namespace UUID
 var Namespace = uuid.Must(uuid.Parse("9a329043-7ad7-4b1c-812f-9c7a93d6392a"))
 
-// MakeUniqueResourceScopedString generates a string that uniquely identifies a cluster resource. It includes the
-// following distinguishing parts:
-// * Group
-// * Kind
-// * Namespace
-// * Name
-func MakeUniqueResourceScopedString(gk schema.GroupKind, namespace string, name string) string {
-	return fmt.Sprintf("%s/%s:%s/%s", gk.Group, gk.Kind, namespace, name)
+// MakeUniqueOwnerScopedStringLegacy preserves the old buggy behavior with ARM ID owners, for now...
+// Deprecated: use MakeUniqueOwnerScopedString instead
+func MakeUniqueOwnerScopedStringLegacy(owner *genruntime.ResourceReference, gk schema.GroupKind, namespace string, name string) string {
+	var prefix string
+	if owner != nil {
+		prefix = fmt.Sprintf("%s/%s:%s/%s", owner.Group, owner.Kind, namespace, owner.Name)
+	}
+
+	var parts []string
+	if prefix != "" {
+		parts = append(parts, prefix)
+	}
+
+	parts = append(parts, fmt.Sprintf("%s/%s", gk.Kind, gk.Group))
+	parts = append(parts, fmt.Sprintf("%s/%s", namespace, name))
+
+	return strings.Join(parts, ":")
 }
 
-// TODO: This method has a bug where it is called with an empty owner gk when the owner is an ARM ID.
 // MakeUniqueOwnerScopedString generates a string that uniquely identifies a cluster resource.  It includes the
 // following distinguishing parts:
-// * Owner group
-// * Owner kind
-// * Owner name
+// * Owner (either group, kind, namespace, name or raw ARM ID)
 // * Group
 // * Kind
 // * Namespace
 // * Name
-func MakeUniqueOwnerScopedString(ownerGK schema.GroupKind, ownerName string, gk schema.GroupKind, namespace string, name string) string {
-	return fmt.Sprintf("%s/%s:%s/%s:%s/%s:%s/%s", ownerGK.Group, ownerGK.Kind, namespace, ownerName, gk.Kind, gk.Group, namespace, name)
+func MakeUniqueOwnerScopedString(owner *genruntime.ResourceReference, gk schema.GroupKind, namespace string, name string) string {
+	prefix := makeUniqueOwnerSubString(owner, namespace)
+
+	var parts []string
+	if prefix != "" {
+		parts = append(parts, prefix)
+	}
+
+	parts = append(parts, fmt.Sprintf("%s/%s", gk.Kind, gk.Group))
+	parts = append(parts, fmt.Sprintf("%s/%s", namespace, name))
+
+	return strings.Join(parts, ":")
+}
+
+func makeUniqueOwnerSubString(owner *genruntime.ResourceReference, namespace string) string {
+	if owner == nil {
+		return ""
+	}
+
+	if owner.IsDirectARMReference() {
+		return owner.ARMID
+	}
+
+	if owner.IsKubernetesReference() {
+		return fmt.Sprintf("%s/%s:%s/%s", owner.Group, owner.Kind, namespace, owner.Name)
+	}
+
+	return "" // This is not expected
 }
 
 // MakeUUIDName creates a stable UUID (v5) if the provided name is not already a UUID based on the specified

diff --git a/v2/internal/util/randextensions/uuid5_test.go b/v2/internal/util/randextensions/uuid5_test.go
@@ -0,0 +1,116 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT License.
+
+package randextensions_test
+
+import (
+	"testing"
+
+	. "github.com/onsi/gomega"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+
+	"github.com/Azure/azure-service-operator/v2/internal/util/randextensions"
+	"github.com/Azure/azure-service-operator/v2/pkg/genruntime"
+)
+
+func Test_MakeUniqueOwnerScopedString(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name         string
+		ref          *genruntime.ResourceReference
+		objGK        schema.GroupKind
+		objNamespace string
+		objName      string
+		expected     string
+	}{
+		{
+			name:         "nil owner returns empty parent string",
+			ref:          nil,
+			objGK:        schema.GroupKind{Group: "resources.azure.com", Kind: "ResourceGroup"},
+			objNamespace: "default",
+			objName:      "myrg",
+			// Note that group and kind are backwards for the object here because I typoed the ordering originally... This is OK as we just want a unique seed for a GUID.
+			expected: "ResourceGroup/resources.azure.com:default/myrg",
+		},
+		{
+			name:         "GVK-based owner, full owner string included",
+			ref:          &genruntime.ResourceReference{Group: "resources.azure.com", Kind: "ResourceGroup", Name: "myrg"},
+			objGK:        schema.GroupKind{Group: "authorization.azure.com", Kind: "RoleAssignment"},
+			objNamespace: "default",
+			objName:      "myroleassignment",
+			// Note that group and kind are backwards for the object here because I typoed the ordering originally... This is OK as we just want a unique seed for a GUID.
+			expected: "resources.azure.com/ResourceGroup:default/myrg:RoleAssignment/authorization.azure.com:default/myroleassignment",
+		},
+		{
+			name:         "ARM-ID-based owner, ARM-ID included",
+			ref:          &genruntime.ResourceReference{ARMID: "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/myrg"},
+			objGK:        schema.GroupKind{Group: "authorization.azure.com", Kind: "RoleAssignment"},
+			objNamespace: "default",
+			objName:      "myroleassignment",
+			// Note that group and kind are backwards for the object here because I typoed the ordering originally... This is OK as we just want a unique seed for a GUID.
+			expected: "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/myrg:RoleAssignment/authorization.azure.com:default/myroleassignment",
+		},
+	}
+
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			g := NewGomegaWithT(t)
+
+			result := randextensions.MakeUniqueOwnerScopedString(tt.ref, tt.objGK, tt.objNamespace, tt.objName)
+			g.Expect(result).To(Equal(tt.expected))
+		})
+	}
+}
+
+func Test_MakeUniqueOwnerScopedStringLegacy(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name         string
+		ref          *genruntime.ResourceReference
+		objGK        schema.GroupKind
+		objNamespace string
+		objName      string
+		expected     string
+	}{
+		{
+			name:         "nil owner returns empty parent string",
+			ref:          nil,
+			objGK:        schema.GroupKind{Group: "resources.azure.com", Kind: "ResourceGroup"},
+			objNamespace: "default",
+			objName:      "myrg",
+			expected:     "ResourceGroup/resources.azure.com:default/myrg",
+		},
+		{
+			name:         "GVK-based owner, full owner string included",
+			ref:          &genruntime.ResourceReference{Group: "resources.azure.com", Kind: "ResourceGroup", Name: "myrg"},
+			objGK:        schema.GroupKind{Group: "authorization.azure.com", Kind: "RoleAssignment"},
+			objNamespace: "default",
+			objName:      "myroleassignment",
+			expected:     "resources.azure.com/ResourceGroup:default/myrg:RoleAssignment/authorization.azure.com:default/myroleassignment",
+		},
+		{
+			name:         "ARM-ID-based owner, ARM-ID included",
+			ref:          &genruntime.ResourceReference{ARMID: "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/myrg"},
+			objGK:        schema.GroupKind{Group: "authorization.azure.com", Kind: "RoleAssignment"},
+			objNamespace: "default",
+			objName:      "myroleassignment",
+			// Note that group and kind are backwards for the object here because I typoed the ordering originally... This is OK as we just want a unique seed for a GUID.
+			expected: "/:default/:RoleAssignment/authorization.azure.com:default/myroleassignment",
+		},
+	}
+
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			g := NewGomegaWithT(t)
+
+			result := randextensions.MakeUniqueOwnerScopedStringLegacy(tt.ref, tt.objGK, tt.objNamespace, tt.objName)
+			g.Expect(result).To(Equal(tt.expected))
+		})
+	}
+}