From cd3252c255e17c6d2da441ef5f60f837f8114baf Mon Sep 17 00:00:00 2001 From: Georgi Sabev Date: Wed, 20 Nov 2024 06:51:12 +0000 Subject: [PATCH] Use valid UUIDs for Korifi resources guids * Do not prefix uuids for orgs, spaces and routes * Use uuidv5 for CFProcess guids for optimistic locking. The app GUID is used as a UUID namespace, the process type is used as name * Document the decision in an ADR fixes #2330, #2177 Co-authored-by: Danail Branekov --- api/repositories/app_repository.go | 34 +++++++++++-------- api/repositories/app_repository_test.go | 22 ++++++------ api/repositories/build_repository.go | 3 +- api/repositories/build_repository_test.go | 2 +- api/repositories/domain_repository_test.go | 2 +- api/repositories/org_repository.go | 3 +- api/repositories/org_repository_test.go | 2 +- api/repositories/package_repository.go | 3 +- api/repositories/package_repository_test.go | 4 +-- api/repositories/process_repository.go | 2 +- api/repositories/process_repository_test.go | 17 ++++++++-- api/repositories/repositories_suite_test.go | 2 +- api/repositories/route_repository.go | 7 +--- api/repositories/route_repository_test.go | 4 +-- .../service_binding_repository.go | 3 +- .../service_binding_repository_test.go | 4 +-- .../service_broker_repository_test.go | 2 +- .../service_instance_repository.go | 5 ++- .../service_instance_repository_test.go | 4 +-- api/repositories/space_repository.go | 3 +- api/repositories/space_repository_test.go | 2 +- api/repositories/task_repository.go | 4 +-- api/repositories/task_repository_test.go | 6 ++-- controllers/api/v1alpha1/cfprocess_types.go | 13 +------ .../controllers/workloads/apps/controller.go | 5 +-- .../workloads/apps/controller_test.go | 14 ++++++++ .../controllers/workloads/env/builder_test.go | 5 +-- .../0012-resource-name-prefixes.md | 2 +- .../0018-uuid-resource-names.md | 26 ++++++++++++++ tests/e2e/apps_test.go | 1 - tests/e2e/orgs_test.go | 1 - tests/e2e/processes_test.go | 3 +- tests/e2e/routes_test.go | 2 +- tests/e2e/spaces_test.go | 1 - tests/matchers/uuid.go | 33 ++++++++++++++++++ tools/uuid.go | 6 ++-- 36 files changed, 157 insertions(+), 95 deletions(-) create mode 100644 docs/architecture-decisions/0018-uuid-resource-names.md create mode 100644 tests/matchers/uuid.go diff --git a/api/repositories/app_repository.go b/api/repositories/app_repository.go index 7c2c75268..cdd6a20b3 100644 --- a/api/repositories/app_repository.go +++ b/api/repositories/app_repository.go @@ -36,8 +36,6 @@ const ( StartedState DesiredState = "STARTED" StoppedState DesiredState = "STOPPED" - Kind string = "CFApp" - APIVersion string = "korifi.cloudfoundry.org/v1alpha1" CFAppGUIDLabel string = "korifi.cloudfoundry.org/app-guid" AppResourceType string = "App" AppEnvResourceType string = "App Env" @@ -292,7 +290,7 @@ func (f *AppRepo) CreateApp(ctx context.Context, authInfo authorization.Info, ap envSecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: GenerateEnvSecretName(cfApp.Name), + Name: cfApp.Spec.EnvSecretName, Namespace: cfApp.Namespace, Labels: map[string]string{ CFAppGUIDLabel: cfApp.Name, @@ -388,16 +386,27 @@ func (f *AppRepo) ListApps(ctx context.Context, authInfo authorization.Info, mes } func (f *AppRepo) PatchAppEnvVars(ctx context.Context, authInfo authorization.Info, message PatchAppEnvVarsMessage) (AppEnvVarsRecord, error) { - secretObj := corev1.Secret{ + userClient, err := f.userClientFactory.BuildClient(authInfo) + if err != nil { + return AppEnvVarsRecord{}, fmt.Errorf("failed to build user client: %w", err) + } + + cfApp := &korifiv1alpha1.CFApp{ ObjectMeta: metav1.ObjectMeta{ - Name: GenerateEnvSecretName(message.AppGUID), Namespace: message.SpaceGUID, + Name: message.AppGUID, }, } - - userClient, err := f.userClientFactory.BuildClient(authInfo) + err = userClient.Get(ctx, client.ObjectKeyFromObject(cfApp), cfApp) if err != nil { - return AppEnvVarsRecord{}, fmt.Errorf("failed to build user client: %w", err) + return AppEnvVarsRecord{}, apierrors.FromK8sError(err, AppEnvResourceType) + } + + secretObj := corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: cfApp.Spec.EnvSecretName, + Namespace: message.SpaceGUID, + }, } err = PatchResource(ctx, userClient, &secretObj, func() { @@ -617,15 +626,10 @@ func getAppEnv(ctx context.Context, userClient client.Client, app AppRecord) (ma return appEnvMap, nil } -func GenerateEnvSecretName(appGUID string) string { - return appGUID + "-env" -} - func (m *CreateAppMessage) toCFApp() korifiv1alpha1.CFApp { - guid := uuid.NewString() return korifiv1alpha1.CFApp{ ObjectMeta: metav1.ObjectMeta{ - Name: guid, + Name: uuid.NewString(), Namespace: m.SpaceGUID, Labels: m.Labels, Annotations: m.Annotations, @@ -633,7 +637,7 @@ func (m *CreateAppMessage) toCFApp() korifiv1alpha1.CFApp { Spec: korifiv1alpha1.CFAppSpec{ DisplayName: m.Name, DesiredState: korifiv1alpha1.AppState(m.State), - EnvSecretName: GenerateEnvSecretName(guid), + EnvSecretName: uuid.NewString(), Lifecycle: korifiv1alpha1.Lifecycle{ Type: korifiv1alpha1.LifecycleType(m.Lifecycle.Type), Data: korifiv1alpha1.LifecycleData{ diff --git a/api/repositories/app_repository_test.go b/api/repositories/app_repository_test.go index 5671db677..f3bbc05b7 100644 --- a/api/repositories/app_repository_test.go +++ b/api/repositories/app_repository_test.go @@ -742,21 +742,18 @@ var _ = Describe("AppRepository", func() { ) var ( - envSecretName string - secretRecord repositories.AppEnvVarsRecord - patchErr error + secretRecord repositories.AppEnvVarsRecord + patchErr error ) BeforeEach(func() { - envSecretName = cfApp.Name + "-env" - envVars := map[string]string{ key0: "VAL0", key1: "original-value", } secret := corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: repositories.GenerateEnvSecretName(cfApp.Name), + Name: cfApp.Spec.EnvSecretName, Namespace: cfSpace.Name, }, StringData: envVars, @@ -796,13 +793,16 @@ var _ = Describe("AppRepository", func() { }) It("patches the underlying secret", func() { - cfAppSecretLookupKey := types.NamespacedName{Name: envSecretName, Namespace: cfSpace.Name} - - var updatedSecret corev1.Secret - err := k8sClient.Get(ctx, cfAppSecretLookupKey, &updatedSecret) + envSecret := corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: cfApp.Spec.EnvSecretName, + Namespace: cfSpace.Name, + }, + } + err := k8sClient.Get(ctx, client.ObjectKeyFromObject(&envSecret), &envSecret) Expect(err).NotTo(HaveOccurred()) - Expect(asMapOfStrings(updatedSecret.Data)).To(SatisfyAll( + Expect(asMapOfStrings(envSecret.Data)).To(SatisfyAll( HaveLen(2), HaveKeyWithValue(key0, "VAL0"), HaveKeyWithValue(key2, "VAL2"), diff --git a/api/repositories/build_repository.go b/api/repositories/build_repository.go index 2b772fc04..94f7b5544 100644 --- a/api/repositories/build_repository.go +++ b/api/repositories/build_repository.go @@ -197,10 +197,9 @@ type CreateBuildMessage struct { } func (m CreateBuildMessage) toCFBuild() korifiv1alpha1.CFBuild { - guid := uuid.NewString() return korifiv1alpha1.CFBuild{ ObjectMeta: metav1.ObjectMeta{ - Name: guid, + Name: uuid.NewString(), Namespace: m.SpaceGUID, Labels: m.Labels, Annotations: m.Annotations, diff --git a/api/repositories/build_repository_test.go b/api/repositories/build_repository_test.go index 9417f6001..4e04e7b46 100644 --- a/api/repositories/build_repository_test.go +++ b/api/repositories/build_repository_test.go @@ -479,7 +479,7 @@ var _ = Describe("BuildRepository", func() { It("returns correct build record", func() { Expect(buildCreateErr).NotTo(HaveOccurred()) - Expect(buildCreateRecord.GUID).NotTo(BeEmpty()) + Expect(buildCreateRecord.GUID).To(matchers.BeValidUUID()) Expect(buildCreateRecord.Lifecycle.Type).To(Equal("docker")) Expect(buildCreateRecord.Lifecycle.Data).To(Equal(repositories.LifecycleData{})) }) diff --git a/api/repositories/domain_repository_test.go b/api/repositories/domain_repository_test.go index 32a561234..9cb7d69ad 100644 --- a/api/repositories/domain_repository_test.go +++ b/api/repositories/domain_repository_test.go @@ -122,7 +122,7 @@ var _ = Describe("DomainRepository", func() { Expect(createErr).NotTo(HaveOccurred()) createdDomainGUID := createdDomain.GUID - Expect(createdDomainGUID).NotTo(BeEmpty()) + Expect(createdDomainGUID).To(matchers.BeValidUUID()) Expect(createdDomain.Name).To(Equal("my.domain")) Expect(createdDomain.Labels).To(HaveKeyWithValue("foo", "bar")) Expect(createdDomain.Annotations).To(HaveKeyWithValue("bar", "baz")) diff --git a/api/repositories/org_repository.go b/api/repositories/org_repository.go index d8a79c314..3bc9aba8f 100644 --- a/api/repositories/org_repository.go +++ b/api/repositories/org_repository.go @@ -21,7 +21,6 @@ import ( ) const ( - OrgPrefix = "cf-org-" OrgResourceType = "Org" ) @@ -95,7 +94,7 @@ func (r *OrgRepo) CreateOrg(ctx context.Context, info authorization.Info, messag cfOrg := &korifiv1alpha1.CFOrg{ ObjectMeta: metav1.ObjectMeta{ - Name: OrgPrefix + uuid.NewString(), + Name: uuid.NewString(), Namespace: r.rootNamespace, Labels: message.Labels, Annotations: message.Annotations, diff --git a/api/repositories/org_repository_test.go b/api/repositories/org_repository_test.go index 8622889a1..80e9ffe8a 100644 --- a/api/repositories/org_repository_test.go +++ b/api/repositories/org_repository_test.go @@ -109,7 +109,7 @@ var _ = Describe("OrgRepository", func() { Expect(createErr).NotTo(HaveOccurred()) Expect(orgRecord.Name).To(Equal(orgGUID)) - Expect(orgRecord.GUID).To(HavePrefix("cf-org-")) + Expect(orgRecord.GUID).To(matchers.BeValidUUID()) Expect(orgRecord.CreatedAt).To(BeTemporally("~", time.Now(), timeCheckThreshold)) Expect(orgRecord.UpdatedAt).To(PointTo(BeTemporally("~", time.Now(), timeCheckThreshold))) Expect(orgRecord.DeletedAt).To(BeNil()) diff --git a/api/repositories/package_repository.go b/api/repositories/package_repository.go index 68b429bdd..89100a75f 100644 --- a/api/repositories/package_repository.go +++ b/api/repositories/package_repository.go @@ -172,14 +172,13 @@ type PackageData struct { } func (message CreatePackageMessage) toCFPackage() *korifiv1alpha1.CFPackage { - packageGUID := uuid.NewString() pkg := &korifiv1alpha1.CFPackage{ TypeMeta: metav1.TypeMeta{ Kind: kind, APIVersion: korifiv1alpha1.SchemeGroupVersion.Identifier(), }, ObjectMeta: metav1.ObjectMeta{ - Name: packageGUID, + Name: uuid.NewString(), Namespace: message.SpaceGUID, Labels: message.Metadata.Labels, Annotations: message.Metadata.Annotations, diff --git a/api/repositories/package_repository_test.go b/api/repositories/package_repository_test.go index 463d8fb2b..86a53bfea 100644 --- a/api/repositories/package_repository_test.go +++ b/api/repositories/package_repository_test.go @@ -152,7 +152,7 @@ var _ = Describe("PackageRepository", func() { Expect(createErr).NotTo(HaveOccurred()) packageGUID := createdPackage.GUID - Expect(packageGUID).NotTo(BeEmpty()) + Expect(packageGUID).To(matchers.BeValidUUID()) Expect(createdPackage.Type).To(Equal("bits")) Expect(createdPackage.AppGUID).To(Equal(appGUID)) Expect(createdPackage.State).To(Equal("AWAITING_UPLOAD")) @@ -244,7 +244,7 @@ var _ = Describe("PackageRepository", func() { Expect(createErr).NotTo(HaveOccurred()) packageGUID := createdPackage.GUID - Expect(packageGUID).NotTo(BeEmpty()) + Expect(packageGUID).To(matchers.BeValidUUID()) Expect(createdPackage.Type).To(Equal("docker")) Expect(createdPackage.AppGUID).To(Equal(appGUID)) Expect(createdPackage.Labels).To(HaveKeyWithValue("bob", "foo")) diff --git a/api/repositories/process_repository.go b/api/repositories/process_repository.go index d079399bb..42e600269 100644 --- a/api/repositories/process_repository.go +++ b/api/repositories/process_repository.go @@ -217,6 +217,7 @@ func (r *ProcessRepo) CreateProcess(ctx context.Context, authInfo authorization. process := &korifiv1alpha1.CFProcess{ ObjectMeta: metav1.ObjectMeta{ Namespace: message.SpaceGUID, + Name: tools.NamespacedUUID(message.AppGUID, message.Type), }, Spec: korifiv1alpha1.CFProcessSpec{ AppRef: corev1.LocalObjectReference{Name: message.AppGUID}, @@ -231,7 +232,6 @@ func (r *ProcessRepo) CreateProcess(ctx context.Context, authInfo authorization. DiskQuotaMB: message.DiskQuotaMB, }, } - process.SetStableName(message.AppGUID) err = userClient.Create(ctx, process) return apierrors.FromK8sError(err, ProcessResourceType) } diff --git a/api/repositories/process_repository_test.go b/api/repositories/process_repository_test.go index bd2dc6aa5..7d22eee3f 100644 --- a/api/repositories/process_repository_test.go +++ b/api/repositories/process_repository_test.go @@ -363,8 +363,7 @@ var _ = Describe("ProcessRepo", func() { Expect(list.Items).To(HaveLen(1)) process := list.Items[0] - Expect(process.Name).To(HavePrefix("cf-proc-")) - Expect(process.Name).To(HaveSuffix("-web")) + Expect(process.Name).NotTo(BeEmpty()) Expect(process.Spec).To(Equal(korifiv1alpha1.CFProcessSpec{ AppRef: corev1.LocalObjectReference{Name: app1GUID}, ProcessType: "web", @@ -382,6 +381,20 @@ var _ = Describe("ProcessRepo", func() { DiskQuotaMB: 123, })) }) + + When("a process with that process type already exists", func() { + BeforeEach(func() { + Expect(processRepo.CreateProcess(ctx, authInfo, repositories.CreateProcessMessage{ + AppGUID: app1GUID, + SpaceGUID: space.Name, + Type: "web", + })).To(Succeed()) + }) + + It("returns an already exists error", func() { + Expect(createErr).To(MatchError(ContainSubstring("already exists"))) + }) + }) }) When("the user is not authorized in the space", func() { diff --git a/api/repositories/repositories_suite_test.go b/api/repositories/repositories_suite_test.go index a57f842c6..42ffc308e 100644 --- a/api/repositories/repositories_suite_test.go +++ b/api/repositories/repositories_suite_test.go @@ -422,7 +422,7 @@ func createAppWithGUID(space, guid string) *korifiv1alpha1.CFApp { CurrentDropletRef: corev1.LocalObjectReference{ Name: uuid.NewString(), }, - EnvSecretName: repositories.GenerateEnvSecretName(guid), + EnvSecretName: uuid.NewString(), }, } Expect(k8sClient.Create(context.Background(), cfApp)).To(Succeed()) diff --git a/api/repositories/route_repository.go b/api/repositories/route_repository.go index 8a86355ee..82fac8236 100644 --- a/api/repositories/route_repository.go +++ b/api/repositories/route_repository.go @@ -23,7 +23,6 @@ import ( const ( RouteResourceType = "Route" - RoutePrefix = "cf-route-" ) type RouteRepo struct { @@ -149,12 +148,8 @@ type DeleteRouteMessage struct { func (m CreateRouteMessage) toCFRoute() korifiv1alpha1.CFRoute { return korifiv1alpha1.CFRoute{ - TypeMeta: metav1.TypeMeta{ - Kind: Kind, - APIVersion: APIVersion, - }, ObjectMeta: metav1.ObjectMeta{ - Name: RoutePrefix + uuid.NewString(), + Name: uuid.NewString(), Namespace: m.SpaceGUID, Labels: m.Labels, Annotations: m.Annotations, diff --git a/api/repositories/route_repository_test.go b/api/repositories/route_repository_test.go index 07d68cce6..3a713ed8d 100644 --- a/api/repositories/route_repository_test.go +++ b/api/repositories/route_repository_test.go @@ -576,7 +576,7 @@ var _ = Describe("RouteRepository", func() { }) It("returns a RouteRecord with matching fields", func() { - Expect(createdRouteRecord.GUID).To(HavePrefix("cf-route-")) + Expect(createdRouteRecord.GUID).To(matchers.BeValidUUID()) Expect(createdRouteRecord.Host).To(Equal(routeHost), "Route Host in record did not match input") Expect(createdRouteRecord.Path).To(Equal(routePath), "Route Path in record did not match input") Expect(createdRouteRecord.SpaceGUID).To(Equal(space.Name), "Route Space GUID in record did not match input") @@ -711,7 +711,7 @@ var _ = Describe("RouteRepository", func() { It("returns an RouteRecord with matching fields", func() { Expect(routeErr).NotTo(HaveOccurred()) - Expect(routeRecord.GUID).To(HavePrefix("cf-route-")) + Expect(routeRecord.GUID).To(matchers.BeValidUUID()) Expect(routeRecord.Host).To(Equal(routeHost), "Route Host in record did not match input") Expect(routeRecord.Path).To(Equal(routePath), "Route Path in record did not match input") Expect(routeRecord.SpaceGUID).To(Equal(space.Name), "Route Space GUID in record did not match input") diff --git a/api/repositories/service_binding_repository.go b/api/repositories/service_binding_repository.go index ed0d29644..bfb125bd8 100644 --- a/api/repositories/service_binding_repository.go +++ b/api/repositories/service_binding_repository.go @@ -111,10 +111,9 @@ func (m *ListServiceBindingsMessage) matches(serviceBinding korifiv1alpha1.CFSer } func (m CreateServiceBindingMessage) toCFServiceBinding() *korifiv1alpha1.CFServiceBinding { - guid := uuid.NewString() return &korifiv1alpha1.CFServiceBinding{ ObjectMeta: metav1.ObjectMeta{ - Name: guid, + Name: uuid.NewString(), Namespace: m.SpaceGUID, Labels: map[string]string{LabelServiceBindingProvisionedService: "true"}, }, diff --git a/api/repositories/service_binding_repository_test.go b/api/repositories/service_binding_repository_test.go index bae8e3f26..c221d7782 100644 --- a/api/repositories/service_binding_repository_test.go +++ b/api/repositories/service_binding_repository_test.go @@ -273,7 +273,7 @@ var _ = Describe("ServiceBindingRepo", func() { It("creates a new CFServiceBinding resource and returns a record", func() { Expect(createErr).NotTo(HaveOccurred()) - Expect(serviceBindingRecord.GUID).NotTo(BeEmpty()) + Expect(serviceBindingRecord.GUID).To(matchers.BeValidUUID()) Expect(serviceBindingRecord.Type).To(Equal("app")) Expect(serviceBindingRecord.Name).To(BeNil()) Expect(serviceBindingRecord.AppGUID).To(Equal(appGUID)) @@ -523,7 +523,7 @@ var _ = Describe("ServiceBindingRepo", func() { It("creates a new CFServiceBinding resource and returns a record", func() { Expect(createErr).NotTo(HaveOccurred()) - Expect(serviceBindingRecord.GUID).NotTo(BeEmpty()) + Expect(serviceBindingRecord.GUID).To(matchers.BeValidUUID()) Expect(serviceBindingRecord.Type).To(Equal("app")) Expect(serviceBindingRecord.Name).To(BeNil()) Expect(serviceBindingRecord.AppGUID).To(Equal(appGUID)) diff --git a/api/repositories/service_broker_repository_test.go b/api/repositories/service_broker_repository_test.go index 53ab6461c..33fb999e8 100644 --- a/api/repositories/service_broker_repository_test.go +++ b/api/repositories/service_broker_repository_test.go @@ -93,7 +93,7 @@ var _ = Describe("ServiceBrokerRepo", func() { "annotation": "annotation-value", }, })) - Expect(brokerRecord.CFResource.GUID).NotTo(BeEmpty()) + Expect(brokerRecord.GUID).To(BeValidUUID()) Expect(brokerRecord.CFResource.CreatedAt).NotTo(BeZero()) }) diff --git a/api/repositories/service_instance_repository.go b/api/repositories/service_instance_repository.go index 583bc5919..f7b5c132f 100644 --- a/api/repositories/service_instance_repository.go +++ b/api/repositories/service_instance_repository.go @@ -201,17 +201,16 @@ func (r *ServiceInstanceRepo) CreateUserProvidedServiceInstance(ctx context.Cont return ServiceInstanceRecord{}, fmt.Errorf("failed to build user client: %w", err) } - guid := uuid.NewString() cfServiceInstance := &korifiv1alpha1.CFServiceInstance{ ObjectMeta: metav1.ObjectMeta{ - Name: guid, + Name: uuid.NewString(), Namespace: message.SpaceGUID, Labels: message.Labels, Annotations: message.Annotations, }, Spec: korifiv1alpha1.CFServiceInstanceSpec{ DisplayName: message.Name, - SecretName: guid, + SecretName: uuid.NewString(), Type: korifiv1alpha1.UserProvidedType, Tags: message.Tags, }, diff --git a/api/repositories/service_instance_repository_test.go b/api/repositories/service_instance_repository_test.go index 1097fb5f6..e120927a1 100644 --- a/api/repositories/service_instance_repository_test.go +++ b/api/repositories/service_instance_repository_test.go @@ -99,7 +99,7 @@ var _ = Describe("ServiceInstanceRepository", func() { It("returns a service instance record", func() { Expect(createErr).NotTo(HaveOccurred()) - Expect(record.GUID).NotTo(BeEmpty()) + Expect(record.GUID).To(matchers.BeValidUUID()) Expect(record.SpaceGUID).To(Equal(space.Name)) Expect(record.Name).To(Equal(serviceInstanceName)) Expect(record.Type).To(Equal("user-provided")) @@ -203,7 +203,7 @@ var _ = Describe("ServiceInstanceRepository", func() { It("returns a service instance record", func() { Expect(createErr).NotTo(HaveOccurred()) - Expect(record.GUID).NotTo(BeEmpty()) + Expect(record.GUID).To(matchers.BeValidUUID()) Expect(record.SpaceGUID).To(Equal(space.Name)) Expect(record.Name).To(Equal(serviceInstanceName)) Expect(record.Type).To(Equal("managed")) diff --git a/api/repositories/space_repository.go b/api/repositories/space_repository.go index 46b703181..e7ad2a6b0 100644 --- a/api/repositories/space_repository.go +++ b/api/repositories/space_repository.go @@ -22,7 +22,6 @@ import ( ) const ( - SpacePrefix = "cf-space-" SpaceResourceType = "Space" ) @@ -112,7 +111,7 @@ func (r *SpaceRepo) CreateSpace(ctx context.Context, info authorization.Info, me cfSpace := &korifiv1alpha1.CFSpace{ ObjectMeta: metav1.ObjectMeta{ - Name: SpacePrefix + uuid.NewString(), + Name: uuid.NewString(), Namespace: message.OrganizationGUID, }, Spec: korifiv1alpha1.CFSpaceSpec{ diff --git a/api/repositories/space_repository_test.go b/api/repositories/space_repository_test.go index 5b8b3d9ae..651520e4b 100644 --- a/api/repositories/space_repository_test.go +++ b/api/repositories/space_repository_test.go @@ -136,7 +136,7 @@ var _ = Describe("SpaceRepository", func() { Expect(k8sClient.Get(ctx, client.ObjectKey{Namespace: orgGUID, Name: spaceRecord.GUID}, spaceCR)).To(Succeed()) Expect(spaceRecord.Name).To(Equal(spaceName)) - Expect(spaceRecord.GUID).To(HavePrefix("cf-space-")) + Expect(spaceRecord.GUID).To(matchers.BeValidUUID()) Expect(spaceRecord.CreatedAt).To(BeTemporally("~", time.Now(), timeCheckThreshold)) Expect(spaceRecord.UpdatedAt).To(PointTo(BeTemporally("~", time.Now(), timeCheckThreshold))) Expect(spaceRecord.DeletedAt).To(BeNil()) diff --git a/api/repositories/task_repository.go b/api/repositories/task_repository.go index 21c5e49a3..317af6c3e 100644 --- a/api/repositories/task_repository.go +++ b/api/repositories/task_repository.go @@ -81,11 +81,9 @@ type PatchTaskMetadataMessage struct { } func (m *CreateTaskMessage) toCFTask() *korifiv1alpha1.CFTask { - guid := uuid.NewString() - return &korifiv1alpha1.CFTask{ ObjectMeta: metav1.ObjectMeta{ - Name: guid, + Name: uuid.NewString(), Namespace: m.SpaceGUID, Labels: m.Labels, Annotations: m.Annotations, diff --git a/api/repositories/task_repository_test.go b/api/repositories/task_repository_test.go index 585459bf5..41a401d20 100644 --- a/api/repositories/task_repository_test.go +++ b/api/repositories/task_repository_test.go @@ -121,7 +121,7 @@ var _ = Describe("TaskRepository", func() { Expect(createErr).NotTo(HaveOccurred()) Expect(taskRecord.Name).NotTo(BeEmpty()) - Expect(taskRecord.GUID).NotTo(BeEmpty()) + Expect(taskRecord.GUID).To(matchers.BeValidUUID()) Expect(taskRecord.Command).To(Equal("echo 'hello world'")) Expect(taskRecord.AppGUID).To(Equal(cfApp.Name)) Expect(taskRecord.SequenceID).To(Equal(int64(4))) @@ -224,7 +224,7 @@ var _ = Describe("TaskRepository", func() { It("returns the task", func() { Expect(getErr).NotTo(HaveOccurred()) Expect(taskRecord.Name).To(Equal(taskGUID)) - Expect(taskRecord.GUID).NotTo(BeEmpty()) + Expect(taskRecord.GUID).To(matchers.BeValidUUID()) Expect(taskRecord.Command).To(Equal("echo hello")) Expect(taskRecord.AppGUID).To(Equal(cfApp.Name)) Expect(taskRecord.SequenceID).To(BeEquivalentTo(6)) @@ -583,7 +583,7 @@ var _ = Describe("TaskRepository", func() { It("returns a cancelled task record", func() { Expect(cancelErr).NotTo(HaveOccurred()) Expect(taskRecord.Name).To(Equal(taskGUID)) - Expect(taskRecord.GUID).NotTo(BeEmpty()) + Expect(taskRecord.GUID).To(matchers.BeValidUUID()) Expect(taskRecord.Command).To(Equal("echo hello")) Expect(taskRecord.AppGUID).To(Equal(cfApp.Name)) Expect(taskRecord.SequenceID).To(BeEquivalentTo(6)) diff --git a/controllers/api/v1alpha1/cfprocess_types.go b/controllers/api/v1alpha1/cfprocess_types.go index 55ad4267a..b0ab97a1b 100644 --- a/controllers/api/v1alpha1/cfprocess_types.go +++ b/controllers/api/v1alpha1/cfprocess_types.go @@ -17,15 +17,12 @@ limitations under the License. package v1alpha1 import ( - "strings" - v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) const ( - ProcessTypeWeb = "web" - processNamePrefix = "cf-proc" + ProcessTypeWeb = "web" ) // CFProcessSpec defines the desired state of CFProcess @@ -118,14 +115,6 @@ type CFProcessList struct { Items []CFProcess `json:"items"` } -func (p *CFProcess) SetStableName(appGUID string) { - p.Name = strings.Join([]string{processNamePrefix, appGUID, p.Spec.ProcessType}, "-") - if p.Labels == nil { - p.Labels = map[string]string{} - } - p.Labels[CFProcessGUIDLabelKey] = p.Name -} - func (p *CFProcess) StatusConditions() *[]metav1.Condition { return &p.Status.Conditions } diff --git a/controllers/controllers/workloads/apps/controller.go b/controllers/controllers/workloads/apps/controller.go index d6bb688e3..18e0aeced 100644 --- a/controllers/controllers/workloads/apps/controller.go +++ b/controllers/controllers/workloads/apps/controller.go @@ -8,6 +8,7 @@ import ( korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1" "code.cloudfoundry.org/korifi/controllers/controllers/shared" + "code.cloudfoundry.org/korifi/tools" "code.cloudfoundry.org/korifi/tools/k8s" "github.com/go-logr/logr" @@ -45,7 +46,7 @@ func NewReconciler(k8sClient client.Client, scheme *runtime.Scheme, log logr.Log vcapServicesEnvBuilder: vcapServicesBuilder, vcapApplicationEnvBuilder: vcapApplicationBuilder, } - return k8s.NewPatchingReconciler[korifiv1alpha1.CFApp, *korifiv1alpha1.CFApp](log, k8sClient, &appReconciler) + return k8s.NewPatchingReconciler(log, k8sClient, &appReconciler) } func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) *builder.Builder { @@ -269,6 +270,7 @@ func (r *Reconciler) createCFProcess(ctx context.Context, process korifiv1alpha1 desiredCFProcess := &korifiv1alpha1.CFProcess{ ObjectMeta: metav1.ObjectMeta{ Namespace: cfApp.Namespace, + Name: tools.NamespacedUUID(cfApp.Name, process.Type), Labels: map[string]string{ korifiv1alpha1.CFAppGUIDLabelKey: cfApp.Name, korifiv1alpha1.CFProcessTypeLabelKey: process.Type, @@ -280,7 +282,6 @@ func (r *Reconciler) createCFProcess(ctx context.Context, process korifiv1alpha1 DetectedCommand: process.Command, }, } - desiredCFProcess.SetStableName(cfApp.Name) if err := controllerutil.SetControllerReference(cfApp, desiredCFProcess, r.scheme); err != nil { err = fmt.Errorf("failed to set OwnerRef on CFProcess: %w", err) diff --git a/controllers/controllers/workloads/apps/controller_test.go b/controllers/controllers/workloads/apps/controller_test.go index e3563c526..8cba1b4e2 100644 --- a/controllers/controllers/workloads/apps/controller_test.go +++ b/controllers/controllers/workloads/apps/controller_test.go @@ -243,6 +243,13 @@ var _ = Describe("CFAppReconciler Integration Tests", func() { ).To(Succeed()) g.Expect(cfProcessList.Items).To(ConsistOf( MatchFields(IgnoreExtras, Fields{ + "ObjectMeta": MatchFields(IgnoreExtras, Fields{ + "Name": Equal(tools.NamespacedUUID(cfApp.Name, "web")), + "Labels": SatisfyAll( + HaveKeyWithValue(korifiv1alpha1.CFAppGUIDLabelKey, cfApp.Name), + HaveKeyWithValue(korifiv1alpha1.CFProcessTypeLabelKey, "web"), + ), + }), "Spec": MatchFields(IgnoreExtras, Fields{ "ProcessType": Equal("web"), "DetectedCommand": Equal("web-process command"), @@ -250,6 +257,13 @@ var _ = Describe("CFAppReconciler Integration Tests", func() { }), }), MatchFields(IgnoreExtras, Fields{ + "ObjectMeta": MatchFields(IgnoreExtras, Fields{ + "Name": Equal(tools.NamespacedUUID(cfApp.Name, "worker")), + "Labels": SatisfyAll( + HaveKeyWithValue(korifiv1alpha1.CFAppGUIDLabelKey, cfApp.Name), + HaveKeyWithValue(korifiv1alpha1.CFProcessTypeLabelKey, "worker"), + ), + }), "Spec": MatchFields(IgnoreExtras, Fields{ "ProcessType": Equal("worker"), "DetectedCommand": Equal("process-worker command"), diff --git a/controllers/controllers/workloads/env/builder_test.go b/controllers/controllers/workloads/env/builder_test.go index f66cf2f5d..e506ab630 100644 --- a/controllers/controllers/workloads/env/builder_test.go +++ b/controllers/controllers/workloads/env/builder_test.go @@ -8,6 +8,7 @@ import ( "code.cloudfoundry.org/korifi/controllers/controllers/workloads/env" "code.cloudfoundry.org/korifi/tests/helpers" "code.cloudfoundry.org/korifi/tools" + "github.com/google/uuid" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" . "github.com/onsi/gomega/gstruct" @@ -315,7 +316,7 @@ var _ = Describe("EnvBuilder", func() { BeforeEach(func() { destinations := []korifiv1alpha1.Destination{{ - GUID: "dest-guid", + GUID: uuid.NewString(), Port: tools.PtrTo[int32](1234), AppRef: corev1.LocalObjectReference{ Name: cfApp.Name, @@ -326,7 +327,7 @@ var _ = Describe("EnvBuilder", func() { cfRoute = &korifiv1alpha1.CFRoute{ ObjectMeta: metav1.ObjectMeta{ Namespace: cfSpace.Status.GUID, - Name: "cf-route-guid", + Name: uuid.NewString(), }, Spec: korifiv1alpha1.CFRouteSpec{ Destinations: destinations, diff --git a/docs/architecture-decisions/0012-resource-name-prefixes.md b/docs/architecture-decisions/0012-resource-name-prefixes.md index 829cfdce4..ca7542bb2 100644 --- a/docs/architecture-decisions/0012-resource-name-prefixes.md +++ b/docs/architecture-decisions/0012-resource-name-prefixes.md @@ -4,7 +4,7 @@ Date: 2022-01-14 ## Status -Accepted +Invalidated by [0018-uuid-resource-names.md](./0018-uuid-resource-names.md) ## Context diff --git a/docs/architecture-decisions/0018-uuid-resource-names.md b/docs/architecture-decisions/0018-uuid-resource-names.md new file mode 100644 index 000000000..fd3cabc95 --- /dev/null +++ b/docs/architecture-decisions/0018-uuid-resource-names.md @@ -0,0 +1,26 @@ +# Valid UUIDs for CF Resource GUIDs + +Date: 2024-11-21 + +## Status + +Accepted + +## Context + +We have received feedback from CF API users (such as the [MultiApps Controller](https://github.com/cloudfoundry/multiapps-controller)) that [using prefixes in resource GUIDS](./0012-resource-name-prefixes.md) violates the [CF API specification](https://v3-apidocs.cloudfoundry.org/version/3.181.0/index.html#api-resource) and such GUIDs cannot be stored in database columns that are configured with the `UUID` type. We have introduced those prefixes in the past as a request to improve operators' ergonomics. However, ergonomics cannot justify violating the CF API specification. + +## Decision +We are going to use valid UUIDs for Korifi k8s resource names when new resources are being created. Existing resources should not be affected by this change. + +* [V4 UUIDs](https://www.rfc-editor.org/rfc/rfc9562.html#name-uuid-version-4) are going to be used by default +* Whereever we want to leverage optimistic name locking (such as ensure one process resource per app per process type) we use [v5 UUIDs](https://www.rfc-editor.org/rfc/rfc9562.html#name-uuid-version-5), where the "parent resource" guid is used as namespace. + +## Consequences + +### Pros +* CF API specification compliance + +### Cons +* Reduced operators' ergonomics. One could implement a custom tool to map CFSpace to k8s namespaces if such a mapping is required. + diff --git a/tests/e2e/apps_test.go b/tests/e2e/apps_test.go index a7a0ded11..c7fa27cc5 100644 --- a/tests/e2e/apps_test.go +++ b/tests/e2e/apps_test.go @@ -420,7 +420,6 @@ var _ = Describe("Apps", func() { It("succeeds", func() { Expect(resp).To(HaveRestyStatusCode(http.StatusOK)) Expect(resultList.Resources[0].GUID).To(Equal(buildGUID)) - }) }) diff --git a/tests/e2e/orgs_test.go b/tests/e2e/orgs_test.go index 827b4adef..9b98cfd9f 100644 --- a/tests/e2e/orgs_test.go +++ b/tests/e2e/orgs_test.go @@ -49,7 +49,6 @@ var _ = Describe("Orgs", func() { Expect(resp).To(HaveRestyStatusCode(http.StatusCreated)) Expect(result.Name).To(Equal(orgName)) Expect(result.GUID).NotTo(BeEmpty()) - Expect(result.GUID).To(HavePrefix("cf-org-")) }) }) diff --git a/tests/e2e/processes_test.go b/tests/e2e/processes_test.go index f13672d85..3adcdbc1a 100644 --- a/tests/e2e/processes_test.go +++ b/tests/e2e/processes_test.go @@ -41,8 +41,7 @@ var _ = Describe("Processes", func() { It("returns the processes for the app", func() { Expect(resp).To(HaveRestyStatusCode(http.StatusOK)) - Expect(webProcessGUID).To(HavePrefix("cf-proc-")) - Expect(webProcessGUID).To(HaveSuffix("-web")) + Expect(webProcessGUID).NotTo(BeEmpty()) // If DEFAULT_APP_BITS_PATH is set, then there may also be non-web processes. // To avoid failures in this case, we only test that the web process is included in the response. Expect(result.Resources).To(ContainElement( diff --git a/tests/e2e/routes_test.go b/tests/e2e/routes_test.go index 464e86304..507006d18 100644 --- a/tests/e2e/routes_test.go +++ b/tests/e2e/routes_test.go @@ -137,7 +137,7 @@ var _ = Describe("Routes", func() { HavePrefix(host), HaveSuffix(path), )) - Expect(route.GUID).To(HavePrefix("cf-route-")) + Expect(route.GUID).NotTo(BeEmpty()) }) }) diff --git a/tests/e2e/spaces_test.go b/tests/e2e/spaces_test.go index ab1658373..5677524d2 100644 --- a/tests/e2e/spaces_test.go +++ b/tests/e2e/spaces_test.go @@ -55,7 +55,6 @@ var _ = Describe("Spaces", func() { It("creates a space", func() { Expect(resp).To(HaveRestyStatusCode(http.StatusCreated)) Expect(result.Name).To(Equal(spaceName)) - Expect(result.GUID).To(HavePrefix("cf-space-")) Expect(result.GUID).NotTo(BeEmpty()) }) }) diff --git a/tests/matchers/uuid.go b/tests/matchers/uuid.go new file mode 100644 index 000000000..f517000cd --- /dev/null +++ b/tests/matchers/uuid.go @@ -0,0 +1,33 @@ +package matchers + +import ( + "fmt" + + "github.com/google/uuid" + "github.com/onsi/gomega/format" + "github.com/onsi/gomega/types" +) + +type validUUIDMatcher struct{} + +func BeValidUUID() types.GomegaMatcher { + return &validUUIDMatcher{} +} + +func (m *validUUIDMatcher) Match(actual interface{}) (bool, error) { + actualString, isString := actual.(string) + if !isString { + return false, fmt.Errorf("%#v is not a string", actual) + } + + _, err := uuid.Parse(actualString) + return err == nil, nil +} + +func (m *validUUIDMatcher) FailureMessage(actual interface{}) (message string) { + return format.Message(actual, "to be a valud UUID") +} + +func (m *validUUIDMatcher) NegatedFailureMessage(actual interface{}) (message string) { + return format.Message(actual, "not to be a valid UUID") +} diff --git a/tools/uuid.go b/tools/uuid.go index 0abf07673..e0aea654e 100644 --- a/tools/uuid.go +++ b/tools/uuid.go @@ -6,8 +6,6 @@ import ( uuid "github.com/satori/go.uuid" ) -var korifiNs = uuid.NewV5(uuid.NamespaceDNS, "korifi.cloudfoundry.org") - -func NamespacedUUID(segments ...string) string { - return uuid.NewV5(korifiNs, strings.TrimSpace(strings.Join(segments, "::"))).String() +func NamespacedUUID(ns string, segments ...string) string { + return uuid.NewV5(uuid.NewV5(uuid.NamespaceDNS, ns), strings.TrimSpace(strings.Join(segments, ":"))).String() }