From 13942babcf20d76a9137db58d40b1f4c4318c602 Mon Sep 17 00:00:00 2001 From: Matthew Christopher Date: Mon, 10 Jun 2024 14:58:51 -0700 Subject: [PATCH] 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. --- .../role_assignment_defaults.go | 6 +- .../v1api20220401/role_assignment_defaults.go | 6 +- .../sql_role_assignment_defaults.go | 6 +- .../sql_role_assignment_defaults.go | 6 +- v2/internal/util/randextensions/uuid5.go | 63 +++++++--- v2/internal/util/randextensions/uuid5_test.go | 116 ++++++++++++++++++ 6 files changed, 173 insertions(+), 30 deletions(-) create mode 100644 v2/internal/util/randextensions/uuid5_test.go diff --git a/v2/api/authorization/v1api20200801preview/role_assignment_defaults.go b/v2/api/authorization/v1api20200801preview/role_assignment_defaults.go index 9a62c690d35..45f65a39138 100644 --- 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 index 479655dc15c..5a0a9f13fc0 100644 --- 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 index b2d7f673fcf..b0c883b7450 100644 --- 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 index 11ec0df8df2..b825060e721 100644 --- 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 index 66c8b2b28da..73d19ece934 100644 --- 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 new file mode 100644 index 00000000000..719aeb93210 --- /dev/null +++ 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)) + }) + } +}