From ef0aaba94f85ca90f3cab9aad30908d01cec25fb Mon Sep 17 00:00:00 2001 From: BG Date: Fri, 28 Nov 2025 12:01:16 +0100 Subject: [PATCH 1/3] Fix issues with renaming orgs --- api/payloads/org.go | 5 +++-- api/repositories/org_repository.go | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/api/payloads/org.go b/api/payloads/org.go index 2872cd616..10c57a487 100644 --- a/api/payloads/org.go +++ b/api/payloads/org.go @@ -31,8 +31,9 @@ func (p OrgCreate) ToMessage() repositories.CreateOrgMessage { } type OrgPatch struct { - Name *string `json:"name"` - Metadata MetadataPatch `json:"metadata"` + Name *string `json:"name"` + Suspended bool `json:"suspended"` + Metadata MetadataPatch `json:"metadata"` } func (p OrgPatch) Validate() error { diff --git a/api/repositories/org_repository.go b/api/repositories/org_repository.go index c2e6acccf..6d167ef73 100644 --- a/api/repositories/org_repository.go +++ b/api/repositories/org_repository.go @@ -60,8 +60,9 @@ type DeleteOrgMessage struct { type PatchOrgMessage struct { MetadataPatch - GUID string - Name *string + Suspended bool + GUID string + Name *string } func (p *PatchOrgMessage) Apply(org *korifiv1alpha1.CFOrg) { From cc29fd2669276fe95a28bd292d84fcb22d109ab0 Mon Sep 17 00:00:00 2001 From: Danail Branekov Date: Thu, 12 Feb 2026 13:25:54 +0000 Subject: [PATCH 2/3] POC: Fix org raname --- api/payloads/metadata.go | 34 ++------------------ api/payloads/org.go | 2 +- api/repositories/metadata.go | 51 ++++++++++++++++++++++++++++-- api/repositories/org_repository.go | 7 ++-- api/tools/metadata/keys.go | 25 +++++++++++++++ 5 files changed, 81 insertions(+), 38 deletions(-) create mode 100644 api/tools/metadata/keys.go diff --git a/api/payloads/metadata.go b/api/payloads/metadata.go index d0dc35469..eea41104a 100644 --- a/api/payloads/metadata.go +++ b/api/payloads/metadata.go @@ -1,11 +1,7 @@ package payloads import ( - "errors" - "fmt" - "net/url" - "strings" - + "code.cloudfoundry.org/korifi/api/tools/metadata" "github.com/jellydator/validation" ) @@ -28,8 +24,8 @@ type Metadata struct { func (m Metadata) Validate() error { return validation.ValidateStruct(&m, - validation.Field(&m.Annotations, validation.Map().Keys(validation.By(cloudfoundryKeyCheck)).AllowExtraKeys()), - validation.Field(&m.Labels, validation.Map().Keys(validation.By(cloudfoundryKeyCheck)).AllowExtraKeys()), + validation.Field(&m.Annotations, validation.Map().Keys(validation.By(metadata.CloudfoundryKeyCheck)).AllowExtraKeys()), + validation.Field(&m.Labels, validation.Map().Keys(validation.By(metadata.CloudfoundryKeyCheck)).AllowExtraKeys()), ) } @@ -37,27 +33,3 @@ type MetadataPatch struct { Annotations map[string]*string `json:"annotations,omitempty"` Labels map[string]*string `json:"labels,omitempty"` } - -func (p MetadataPatch) Validate() error { - return validation.ValidateStruct(&p, - validation.Field(&p.Annotations, validation.Map().Keys(validation.By(cloudfoundryKeyCheck)).AllowExtraKeys()), - validation.Field(&p.Labels, validation.Map().Keys(validation.By(cloudfoundryKeyCheck)).AllowExtraKeys()), - ) -} - -func cloudfoundryKeyCheck(key any) error { - keyStr, ok := key.(string) - if !ok { - return fmt.Errorf("expected string key, got %T", key) - } - - u, err := url.ParseRequestURI("https://" + keyStr) // without the scheme, the hostname will be parsed as a path - if err != nil { - return nil - } - - if strings.HasSuffix(u.Hostname(), "cloudfoundry.org") { - return errors.New("label/annotation key cannot use the cloudfoundry.org domain") - } - return nil -} diff --git a/api/payloads/org.go b/api/payloads/org.go index 10c57a487..808278730 100644 --- a/api/payloads/org.go +++ b/api/payloads/org.go @@ -32,7 +32,7 @@ func (p OrgCreate) ToMessage() repositories.CreateOrgMessage { type OrgPatch struct { Name *string `json:"name"` - Suspended bool `json:"suspended"` + Suspended *bool `json:"suspended"` Metadata MetadataPatch `json:"metadata"` } diff --git a/api/repositories/metadata.go b/api/repositories/metadata.go index fb86516ac..da1ba08c2 100644 --- a/api/repositories/metadata.go +++ b/api/repositories/metadata.go @@ -1,6 +1,13 @@ package repositories -import "sigs.k8s.io/controller-runtime/pkg/client" +import ( + "fmt" + + apierrors "code.cloudfoundry.org/korifi/api/errors" + "code.cloudfoundry.org/korifi/api/tools/metadata" + "code.cloudfoundry.org/korifi/tools" + "sigs.k8s.io/controller-runtime/pkg/client" +) type Metadata struct { Annotations map[string]string @@ -12,7 +19,11 @@ type MetadataPatch struct { Labels map[string]*string } -func (p *MetadataPatch) Apply(obj client.Object) { +func (p *MetadataPatch) Apply(obj client.Object) error { + if err := p.validate(obj); err != nil { + return err + } + if obj.GetAnnotations() == nil { obj.SetAnnotations(map[string]string{}) } @@ -23,6 +34,33 @@ func (p *MetadataPatch) Apply(obj client.Object) { patchMap(obj.GetAnnotations(), p.Annotations) patchMap(obj.GetLabels(), p.Labels) + + return nil +} + +func (p *MetadataPatch) validate(obj client.Object) error { + for patchKey, patchValue := range p.Annotations { + if !isUpdate(obj.GetAnnotations(), patchKey, patchValue) { + continue + } + + if err := metadata.CloudfoundryKeyCheck(patchKey); err != nil { + // TODO: the error message probably may not contain that much details, logging them should be sufficient + return apierrors.NewUnprocessableEntityError(err, fmt.Sprintf("invalid annotations patch: %q: %q -> %q", patchKey, obj.GetAnnotations()[patchKey], tools.ZeroIfNil(patchValue))) + } + } + + for patchKey, patchValue := range p.Labels { + if !isUpdate(obj.GetLabels(), patchKey, patchValue) { + continue + } + + if err := metadata.CloudfoundryKeyCheck(patchKey); err != nil { + return apierrors.NewUnprocessableEntityError(err, fmt.Sprintf("invalid labels patch: %q: %q -> %q", patchKey, obj.GetLabels()[patchKey], tools.ZeroIfNil(patchValue))) + } + } + + return nil } func patchMap(original map[string]string, patch map[string]*string) { @@ -34,3 +72,12 @@ func patchMap(original map[string]string, patch map[string]*string) { } } } + +func isUpdate(valuesMap map[string]string, newKey string, newValuePtr *string) bool { + if newValuePtr == nil { + // TODO: I think we do not keep nil metadata values, double check that + return true + } + + return valuesMap[newKey] != *newValuePtr +} diff --git a/api/repositories/org_repository.go b/api/repositories/org_repository.go index 6d167ef73..d506125a1 100644 --- a/api/repositories/org_repository.go +++ b/api/repositories/org_repository.go @@ -65,11 +65,11 @@ type PatchOrgMessage struct { Name *string } -func (p *PatchOrgMessage) Apply(org *korifiv1alpha1.CFOrg) { +func (p *PatchOrgMessage) Apply(org *korifiv1alpha1.CFOrg) error { if p.Name != nil { org.Spec.DisplayName = *p.Name } - p.MetadataPatch.Apply(org) + return p.MetadataPatch.Apply(org) } type OrgRecord struct { @@ -189,8 +189,7 @@ func (r *OrgRepo) PatchOrg(ctx context.Context, authInfo authorization.Info, mes } err = r.klient.Patch(ctx, cfOrg, func() error { - message.Apply(cfOrg) - return nil + return message.Apply(cfOrg) }) if err != nil { return OrgRecord{}, apierrors.FromK8sError(err, OrgResourceType) diff --git a/api/tools/metadata/keys.go b/api/tools/metadata/keys.go new file mode 100644 index 000000000..1004b845a --- /dev/null +++ b/api/tools/metadata/keys.go @@ -0,0 +1,25 @@ +package metadata + +import ( + "errors" + "fmt" + "net/url" + "strings" +) + +func CloudfoundryKeyCheck(key any) error { + keyStr, ok := key.(string) + if !ok { + return fmt.Errorf("expected string key, got %T", key) + } + + u, err := url.ParseRequestURI("https://" + keyStr) // without the scheme, the hostname will be parsed as a path + if err != nil { + return nil + } + + if strings.HasSuffix(u.Hostname(), "cloudfoundry.org") { + return errors.New("label/annotation key cannot use the cloudfoundry.org domain") + } + return nil +} From 296b7ac2f7c2c7551ffd4fe0116bc07700cef879 Mon Sep 17 00:00:00 2001 From: Yevgen Bykov Date: Tue, 14 Apr 2026 18:04:37 +0200 Subject: [PATCH 3/3] Implement validating webhook for internal Korifi labels --- api/payloads/app_test.go | 14 --- api/payloads/domain_test.go | 14 --- api/payloads/droplet_test.go | 28 ------ api/payloads/metadata_test.go | 20 ---- api/payloads/org_test.go | 12 --- api/payloads/package_test.go | 14 --- api/payloads/route_test.go | 13 --- api/payloads/service_binding_test.go | 13 --- api/payloads/service_instance_test.go | 10 -- api/payloads/space_test.go | 12 --- api/payloads/task_test.go | 14 --- api/repositories/app_repository.go | 7 +- api/repositories/app_repository_test.go | 15 +++ api/repositories/domain_repository.go | 3 +- api/repositories/domain_repository_test.go | 16 ++++ api/repositories/droplet_repository.go | 4 +- api/repositories/droplet_repository_test.go | 16 ++++ api/repositories/metadata_test.go | 2 +- api/repositories/org_repository_test.go | 11 ++- api/repositories/package_repository.go | 4 +- api/repositories/package_repository_test.go | 25 ++++- api/repositories/process_repository.go | 4 +- api/repositories/process_repository_test.go | 17 ++++ api/repositories/repositories_suite_test.go | 12 ++- api/repositories/route_repository.go | 4 +- .../service_binding_repository.go | 3 +- .../service_binding_repository_test.go | 15 +++ api/repositories/service_broker_repository.go | 7 +- .../service_broker_repository_test.go | 17 ++++ .../service_instance_repository.go | 7 +- .../service_instance_repository_test.go | 36 +++++-- .../service_offering_repository.go | 7 +- .../service_offering_repository_test.go | 15 +++ api/repositories/space_repository.go | 7 +- api/repositories/space_repository_test.go | 4 + api/repositories/task_repository.go | 3 +- api/repositories/task_repository_test.go | 9 +- controllers/api/v1alpha1/shared_types.go | 2 + controllers/main.go | 14 ++- .../webhooks/label_indexer/signer/signer.go | 37 ++++++++ .../webhooks/label_indexer/suite_test.go | 2 +- controllers/webhooks/label_indexer/webhook.go | 10 +- .../webhooks/label_validator/suite_test.go | 94 +++++++++++++++++++ .../webhooks/label_validator/webhook.go | 79 ++++++++++++++++ .../webhooks/label_validator/webhook_test.go | 94 +++++++++++++++++++ helm/korifi/controllers/deployment.yaml | 7 ++ .../controllers/label-signing-secret.yaml | 13 +++ helm/korifi/controllers/manifests.yaml | 35 +++++++ .../migration/post-upgrade-migration.yaml | 11 +++ helm/korifi/values.yaml | 1 + migration/main.go | 12 ++- migration/migration/executor.go | 61 ++++++++++-- migration/migration/executor_test.go | 2 +- tests/e2e/orgs_test.go | 93 ++++++++++++++++++ tests/smoke/orgs_test.go | 15 +++ 55 files changed, 789 insertions(+), 227 deletions(-) create mode 100644 controllers/webhooks/label_indexer/signer/signer.go create mode 100644 controllers/webhooks/label_validator/suite_test.go create mode 100644 controllers/webhooks/label_validator/webhook.go create mode 100644 controllers/webhooks/label_validator/webhook_test.go create mode 100644 helm/korifi/controllers/label-signing-secret.yaml diff --git a/api/payloads/app_test.go b/api/payloads/app_test.go index 7deec800e..90ce4b254 100644 --- a/api/payloads/app_test.go +++ b/api/payloads/app_test.go @@ -313,20 +313,6 @@ var _ = Describe("App payload validation", func() { Expect(decodedPayload).To(gstruct.PointTo(Equal(payload))) }) - When("metadata is invalid", func() { - BeforeEach(func() { - payload.Metadata = payloads.MetadataPatch{ - Labels: map[string]*string{ - "foo.cloudfoundry.org/bar": tools.PtrTo("jim"), - }, - } - }) - - It("returns an appropriate error", func() { - expectUnprocessableEntityError(validatorErr, "label/annotation key cannot use the cloudfoundry.org domain") - }) - }) - When("name is invalid", func() { BeforeEach(func() { payload.Name = "!@#" diff --git a/api/payloads/domain_test.go b/api/payloads/domain_test.go index 7750ab32f..9f2b114ec 100644 --- a/api/payloads/domain_test.go +++ b/api/payloads/domain_test.go @@ -165,20 +165,6 @@ var _ = Describe("DomainUpdate", func() { Expect(validatorErr).NotTo(HaveOccurred()) Expect(decodedUpdatePayload).To(gstruct.PointTo(Equal(updatePayload))) }) - - When("metadata is invalid", func() { - BeforeEach(func() { - updatePayload.Metadata = payloads.MetadataPatch{ - Labels: map[string]*string{ - "foo.cloudfoundry.org/bar": tools.PtrTo("jim"), - }, - } - }) - - It("returns an appropriate error", func() { - expectUnprocessableEntityError(validatorErr, "cannot use the cloudfoundry.org domain") - }) - }) }) var _ = Describe("DomainList", func() { diff --git a/api/payloads/droplet_test.go b/api/payloads/droplet_test.go index 5bfd5e7f5..4ca244493 100644 --- a/api/payloads/droplet_test.go +++ b/api/payloads/droplet_test.go @@ -94,33 +94,5 @@ var _ = Describe("DropletUpdate", func() { Expect(validatorErr).NotTo(HaveOccurred()) Expect(decodedDropletPayload).To(gstruct.PointTo(Equal(updatePayload))) }) - - When("metadata.labels contains an invalid key", func() { - BeforeEach(func() { - updatePayload.Metadata = payloads.MetadataPatch{ - Labels: map[string]*string{ - "foo.cloudfoundry.org/bar": tools.PtrTo("jim"), - }, - } - }) - - It("returns an appropriate error", func() { - expectUnprocessableEntityError(validatorErr, "cannot use the cloudfoundry.org domain") - }) - }) - - When("metadata.annotations contains an invalid key", func() { - BeforeEach(func() { - updatePayload.Metadata = payloads.MetadataPatch{ - Annotations: map[string]*string{ - "foo.cloudfoundry.org/bar": tools.PtrTo("jim"), - }, - } - }) - - It("returns an appropriate error", func() { - expectUnprocessableEntityError(validatorErr, "cannot use the cloudfoundry.org domain") - }) - }) }) }) diff --git a/api/payloads/metadata_test.go b/api/payloads/metadata_test.go index c6fbd43bf..1b51f5c00 100644 --- a/api/payloads/metadata_test.go +++ b/api/payloads/metadata_test.go @@ -84,24 +84,4 @@ var _ = Describe("MetadataPatch", func() { Expect(validatorErr).NotTo(HaveOccurred()) Expect(decodedMetadataPatchPayload).To(gstruct.PointTo(Equal(metadataPatchPayload))) }) - - When("metadata.labels contains an invalid key", func() { - BeforeEach(func() { - metadataPatchPayload.Labels["foo.cloudfoundry.org/bar"] = tools.PtrTo("jim") - }) - - It("returns an appropriate error", func() { - expectUnprocessableEntityError(validatorErr, "cannot use the cloudfoundry.org domain") - }) - }) - - When("metadata.annotations contains an invalid key", func() { - BeforeEach(func() { - metadataPatchPayload.Annotations["foo.cloudfoundry.org/bar"] = tools.PtrTo("jim") - }) - - It("returns an appropriate error", func() { - expectUnprocessableEntityError(validatorErr, "cannot use the cloudfoundry.org domain") - }) - }) }) diff --git a/api/payloads/org_test.go b/api/payloads/org_test.go index 727be8865..7f183d08a 100644 --- a/api/payloads/org_test.go +++ b/api/payloads/org_test.go @@ -86,18 +86,6 @@ var _ = Describe("Org", func() { Expect(decodedPayload).To(gstruct.PointTo(Equal(payload))) }) - When("the metadata is invalid", func() { - BeforeEach(func() { - payload.Metadata.Labels["cloudfoundry.org/test"] = tools.PtrTo("production") - }) - - It("returns an unprocessable entity error", func() { - expectUnprocessableEntityError( - validatorErr, - "label/annotation key cannot use the cloudfoundry.org domain", - ) - }) - }) When("the name is invalid", func() { BeforeEach(func() { payload.Name = tools.PtrTo("") diff --git a/api/payloads/package_test.go b/api/payloads/package_test.go index 7e345e4d6..3b09d6021 100644 --- a/api/payloads/package_test.go +++ b/api/payloads/package_test.go @@ -282,20 +282,6 @@ var _ = Describe("PackageUpdate", func() { Expect(validatorErr).NotTo(HaveOccurred()) Expect(decodedPayload).To(gstruct.PointTo(Equal(payload))) }) - - When("metadata is invalid", func() { - BeforeEach(func() { - payload.Metadata = payloads.MetadataPatch{ - Labels: map[string]*string{ - "foo.cloudfoundry.org/bar": tools.PtrTo("jim"), - }, - } - }) - - It("returns an appropriate error", func() { - expectUnprocessableEntityError(validatorErr, "label/annotation key cannot use the cloudfoundry.org domain") - }) - }) }) Describe("ToMessage", func() { diff --git a/api/payloads/route_test.go b/api/payloads/route_test.go index 08b082682..a6ddd3e63 100644 --- a/api/payloads/route_test.go +++ b/api/payloads/route_test.go @@ -188,7 +188,6 @@ var _ = Describe("RoutePatch", func() { patchPayload payloads.RoutePatch routePatch *payloads.RoutePatch validatorErr error - apiError errors.ApiError ) BeforeEach(func() { @@ -203,24 +202,12 @@ var _ = Describe("RoutePatch", func() { JustBeforeEach(func() { validatorErr = validator.DecodeAndValidateJSONPayload(createJSONRequest(patchPayload), routePatch) - apiError, _ = validatorErr.(errors.ApiError) }) It("succeeds", func() { Expect(validatorErr).NotTo(HaveOccurred()) Expect(routePatch).To(gstruct.PointTo(Equal(patchPayload))) }) - - When("metadata uses the cloudfoundry domain", func() { - BeforeEach(func() { - patchPayload.Metadata.Labels["foo.cloudfoundry.org/bar"] = tools.PtrTo("baz") - }) - - It("fails", func() { - Expect(apiError).To(HaveOccurred()) - Expect(apiError.Detail()).To(ContainSubstring("cannot use the cloudfoundry.org domain")) - }) - }) }) var _ = Describe("Add destination", func() { diff --git a/api/payloads/service_binding_test.go b/api/payloads/service_binding_test.go index e2f8cd5e6..ced928fce 100644 --- a/api/payloads/service_binding_test.go +++ b/api/payloads/service_binding_test.go @@ -257,7 +257,6 @@ var _ = Describe("ServiceBindingUpdate", func() { patchPayload payloads.ServiceBindingUpdate serviceBindingPatch *payloads.ServiceBindingUpdate validatorErr error - apiError errors.ApiError ) BeforeEach(func() { @@ -272,22 +271,10 @@ var _ = Describe("ServiceBindingUpdate", func() { JustBeforeEach(func() { validatorErr = validator.DecodeAndValidateJSONPayload(createJSONRequest(patchPayload), serviceBindingPatch) - apiError, _ = validatorErr.(errors.ApiError) }) It("succeeds", func() { Expect(validatorErr).NotTo(HaveOccurred()) Expect(serviceBindingPatch).To(PointTo(Equal(patchPayload))) }) - - When("metadata uses the cloudfoundry domain", func() { - BeforeEach(func() { - patchPayload.Metadata.Labels["foo.cloudfoundry.org/bar"] = tools.PtrTo("baz") - }) - - It("fails", func() { - Expect(apiError).To(HaveOccurred()) - Expect(apiError.Detail()).To(ContainSubstring("cannot use the cloudfoundry.org domain")) - }) - }) }) diff --git a/api/payloads/service_instance_test.go b/api/payloads/service_instance_test.go index 589274008..4a0693dd8 100644 --- a/api/payloads/service_instance_test.go +++ b/api/payloads/service_instance_test.go @@ -521,16 +521,6 @@ var _ = Describe("ServiceInstancePatch", func() { }) }) - When("metadata is invalid", func() { - BeforeEach(func() { - patchPayload.Metadata.Labels["foo.cloudfoundry.org/bar"] = tools.PtrTo("baz") - }) - - It("returns an appropriate error", func() { - expectUnprocessableEntityError(validatorErr, "label/annotation key cannot use the cloudfoundry.org domain") - }) - }) - Context("ToServiceInstancePatchMessage", func() { It("converts to repo message correctly", func() { msg := serviceInstancePatch.ToServiceInstancePatchMessage("space-guid", "app-guid") diff --git a/api/payloads/space_test.go b/api/payloads/space_test.go index 7bac5027e..af6ece3fe 100644 --- a/api/payloads/space_test.go +++ b/api/payloads/space_test.go @@ -126,18 +126,6 @@ var _ = Describe("Space", func() { Expect(decodedPayload).To(gstruct.PointTo(Equal(payload))) }) - When("the metadata is invalid", func() { - BeforeEach(func() { - payload.Metadata.Labels["cloudfoundry.org/test"] = tools.PtrTo("production") - }) - - It("returns an unprocessable entity error", func() { - expectUnprocessableEntityError( - validatorErr, - "label/annotation key cannot use the cloudfoundry.org domain", - ) - }) - }) When("the name is invalid", func() { BeforeEach(func() { payload.Name = tools.PtrTo("") diff --git a/api/payloads/task_test.go b/api/payloads/task_test.go index a4dd9cfd5..12fa5d5d7 100644 --- a/api/payloads/task_test.go +++ b/api/payloads/task_test.go @@ -216,20 +216,6 @@ var _ = Describe("TaskUpdate", func() { Expect(validatorErr).NotTo(HaveOccurred()) Expect(decodedPayload).To(gstruct.PointTo(Equal(payload))) }) - - When("metadata is invalid", func() { - BeforeEach(func() { - payload.Metadata = payloads.MetadataPatch{ - Labels: map[string]*string{ - "foo.cloudfoundry.org/bar": tools.PtrTo("jim"), - }, - } - }) - - It("returns an appropriate error", func() { - expectUnprocessableEntityError(validatorErr, "label/annotation key cannot use the cloudfoundry.org domain") - }) - }) }) Describe("ToMessage()", func() { diff --git a/api/repositories/app_repository.go b/api/repositories/app_repository.go index 1ec4126b4..9b4805ca8 100644 --- a/api/repositories/app_repository.go +++ b/api/repositories/app_repository.go @@ -249,8 +249,7 @@ func (f *AppRepo) PatchApp(ctx context.Context, authInfo authorization.Info, app } err := GetAndPatch(ctx, f.klient, cfApp, func() error { - appPatchMessage.Apply(cfApp) - return nil + return appPatchMessage.Apply(cfApp) }) if err != nil { return AppRecord{}, apierrors.FromK8sError(err, AppResourceType) @@ -554,7 +553,7 @@ func (m *CreateAppMessage) toCFApp() korifiv1alpha1.CFApp { } } -func (m *PatchAppMessage) Apply(app *korifiv1alpha1.CFApp) { +func (m *PatchAppMessage) Apply(app *korifiv1alpha1.CFApp) error { if m.Name != "" { app.Spec.DisplayName = m.Name } @@ -573,7 +572,7 @@ func (m *PatchAppMessage) Apply(app *korifiv1alpha1.CFApp) { } } - m.MetadataPatch.Apply(app) + return m.MetadataPatch.Apply(app) } func cfAppToAppRecord(cfApp korifiv1alpha1.CFApp) (AppRecord, error) { diff --git a/api/repositories/app_repository_test.go b/api/repositories/app_repository_test.go index 4d2b5721a..013cca8d5 100644 --- a/api/repositories/app_repository_test.go +++ b/api/repositories/app_repository_test.go @@ -456,6 +456,21 @@ var _ = Describe("AppRepository", func() { createRoleBinding(ctx, userName, spaceDeveloperRole.Name, cfSpace.Name) }) + When("a label is invalid", func() { + BeforeEach(func() { + appPatchMessage.MetadataPatch.Labels["foo.cloudfoundry.org/bar"] = tools.PtrTo("baz") + }) + + It("returns an UnprocessableEntityError", func() { + var unprocessableEntityError apierrors.UnprocessableEntityError + Expect(errors.As(patchErr, &unprocessableEntityError)).To(BeTrue()) + Expect(unprocessableEntityError.Detail()).To(SatisfyAll( + ContainSubstring("invalid labels patch"), + ContainSubstring(`"foo.cloudfoundry.org/bar"`), + )) + }) + }) + It("updates the app", func() { Expect(patchErr).NotTo(HaveOccurred()) diff --git a/api/repositories/domain_repository.go b/api/repositories/domain_repository.go index 358699231..de80c9ed4 100644 --- a/api/repositories/domain_repository.go +++ b/api/repositories/domain_repository.go @@ -124,8 +124,7 @@ func (r *DomainRepo) UpdateDomain(ctx context.Context, authInfo authorization.In } err = r.klient.Patch(ctx, domain, func() error { - message.MetadataPatch.Apply(domain) - return nil + return message.MetadataPatch.Apply(domain) }) if err != nil { return DomainRecord{}, fmt.Errorf("failed to patch domain metadata: %w", apierrors.FromK8sError(err, DomainResourceType)) diff --git a/api/repositories/domain_repository_test.go b/api/repositories/domain_repository_test.go index 38d138ed9..288e55794 100644 --- a/api/repositories/domain_repository_test.go +++ b/api/repositories/domain_repository_test.go @@ -2,6 +2,7 @@ package repositories_test import ( "context" + "errors" "time" apierrors "code.cloudfoundry.org/korifi/api/errors" @@ -187,6 +188,21 @@ var _ = Describe("DomainRepository", func() { createRoleBinding(ctx, userName, adminRole.Name, rootNamespace) }) + When("a label is invalid", func() { + BeforeEach(func() { + updatePayload.MetadataPatch.Labels["foo.cloudfoundry.org/bar"] = tools.PtrTo("baz") + }) + + It("returns an UnprocessableEntityError", func() { + var unprocessableEntityError apierrors.UnprocessableEntityError + Expect(errors.As(updateErr, &unprocessableEntityError)).To(BeTrue()) + Expect(unprocessableEntityError.Detail()).To(SatisfyAll( + ContainSubstring("invalid labels patch"), + ContainSubstring(`"foo.cloudfoundry.org/bar"`), + )) + }) + }) + It("updates the domain metadata", func() { Expect(updateErr).NotTo(HaveOccurred()) diff --git a/api/repositories/droplet_repository.go b/api/repositories/droplet_repository.go index 1d3373c2b..2e113cfa8 100644 --- a/api/repositories/droplet_repository.go +++ b/api/repositories/droplet_repository.go @@ -168,9 +168,7 @@ func (r *DropletRepo) UpdateDroplet(ctx context.Context, authInfo authorization. } err = r.klient.Patch(ctx, build, func() error { - message.MetadataPatch.Apply(build) - - return nil + return message.MetadataPatch.Apply(build) }) if err != nil { return DropletRecord{}, fmt.Errorf("failed to patch droplet metadata: %w", apierrors.FromK8sError(err, DropletResourceType)) diff --git a/api/repositories/droplet_repository_test.go b/api/repositories/droplet_repository_test.go index 628bace27..f4c98687d 100644 --- a/api/repositories/droplet_repository_test.go +++ b/api/repositories/droplet_repository_test.go @@ -1,6 +1,7 @@ package repositories_test import ( + "errors" "time" apierrors "code.cloudfoundry.org/korifi/api/errors" @@ -320,6 +321,21 @@ var _ = Describe("DropletRepository", func() { })).To(Succeed()) }) + When("a label is invalid", func() { + BeforeEach(func() { + dropletUpdateMsg.MetadataPatch.Labels["foo.cloudfoundry.org/bar"] = tools.PtrTo("baz") + }) + + It("returns an UnprocessableEntityError", func() { + var unprocessableEntityError apierrors.UnprocessableEntityError + Expect(errors.As(updateError, &unprocessableEntityError)).To(BeTrue()) + Expect(unprocessableEntityError.Detail()).To(SatisfyAll( + ContainSubstring("invalid labels patch"), + ContainSubstring(`"foo.cloudfoundry.org/bar"`), + )) + }) + }) + It("updates the build metadata in kubernetes", func() { Expect(updateError).NotTo(HaveOccurred()) diff --git a/api/repositories/metadata_test.go b/api/repositories/metadata_test.go index 5ded5dd63..27b5386aa 100644 --- a/api/repositories/metadata_test.go +++ b/api/repositories/metadata_test.go @@ -50,7 +50,7 @@ var _ = Describe("MetadataPatch", func() { }) JustBeforeEach(func() { - metadataPatch.Apply(pod) + Expect(metadataPatch.Apply(pod)).To(Succeed()) }) It("updates labels and annotations correctly", func() { diff --git a/api/repositories/org_repository_test.go b/api/repositories/org_repository_test.go index f30a472f8..8a8512737 100644 --- a/api/repositories/org_repository_test.go +++ b/api/repositories/org_repository_test.go @@ -114,7 +114,7 @@ var _ = Describe("OrgRepository", func() { Expect(orgRecord.UpdatedAt).To(PointTo(BeTemporally("~", time.Now(), timeCheckThreshold))) Expect(orgRecord.DeletedAt).To(BeNil()) Expect(orgRecord.Labels).To(HaveKeyWithValue("test-label-key", "test-label-val")) - Expect(orgRecord.Annotations).To(Equal(map[string]string{"test-annotation-key": "test-annotation-val"})) + Expect(orgRecord.Annotations).To(HaveKeyWithValue("test-annotation-key", "test-annotation-val")) }) It("creates a CFOrg resource in the root namespace", func() { @@ -125,7 +125,10 @@ var _ = Describe("OrgRepository", func() { Expect(cfOrg.Spec.DisplayName).To(Equal(orgGUID)) Expect(cfOrg.Labels).To(HaveKeyWithValue("test-label-key", "test-label-val")) - Expect(cfOrg.Annotations).To(Equal(map[string]string{"test-annotation-key": "test-annotation-val"})) + Expect(cfOrg.Annotations).To(Equal(map[string]string{ + "test-annotation-key": "test-annotation-val", + korifiv1alpha1.LabelSignatureAnnotationKey: testLabelSig(cfOrg.Labels), + })) }) It("awaits the ready condition", func() { @@ -506,6 +509,7 @@ var _ = Describe("OrgRepository", func() { map[string]string{ "key-one": "value-one", "key-two": "value-two", + korifiv1alpha1.LabelSignatureAnnotationKey: testLabelSig(orgRecord.Labels), }, )) Expect(orgRecord.Name).To(Equal(*orgNewName)) @@ -523,6 +527,7 @@ var _ = Describe("OrgRepository", func() { map[string]string{ "key-one": "value-one", "key-two": "value-two", + korifiv1alpha1.LabelSignatureAnnotationKey: testLabelSig(updatedCFOrg.Labels), }, )) Expect(updatedCFOrg.Spec.DisplayName).To(Equal(*orgNewName)) @@ -568,6 +573,7 @@ var _ = Describe("OrgRepository", func() { "before-key-one": "value-one", "key-one": "value-one-updated", "key-two": "value-two", + korifiv1alpha1.LabelSignatureAnnotationKey: testLabelSig(orgRecord.Labels), }, )) Expect(orgRecord.Name).To(Equal(*orgNewName)) @@ -588,6 +594,7 @@ var _ = Describe("OrgRepository", func() { "before-key-one": "value-one", "key-one": "value-one-updated", "key-two": "value-two", + korifiv1alpha1.LabelSignatureAnnotationKey: testLabelSig(updatedCFOrg.Labels), }, )) Expect(orgRecord.Name).To(Equal(*orgNewName)) diff --git a/api/repositories/package_repository.go b/api/repositories/package_repository.go index 1ab993559..7a1caf97a 100644 --- a/api/repositories/package_repository.go +++ b/api/repositories/package_repository.go @@ -259,9 +259,7 @@ func (r *PackageRepo) UpdatePackage(ctx context.Context, authInfo authorization. } err = r.klient.Patch(ctx, cfPackage, func() error { - updateMessage.MetadataPatch.Apply(cfPackage) - - return nil + return updateMessage.MetadataPatch.Apply(cfPackage) }) if err != nil { return PackageRecord{}, fmt.Errorf("failed to patch package metadata: %w", apierrors.FromK8sError(err, PackageResourceType)) diff --git a/api/repositories/package_repository_test.go b/api/repositories/package_repository_test.go index 030da7209..a0655a554 100644 --- a/api/repositories/package_repository_test.go +++ b/api/repositories/package_repository_test.go @@ -684,16 +684,37 @@ var _ = Describe("PackageRepository", func() { createRoleBinding(ctx, userName, spaceDeveloperRole.Name, space.Name) }) + When("a label is invalid", func() { + BeforeEach(func() { + updateMessage.MetadataPatch.Labels["foo.cloudfoundry.org/bar"] = tools.PtrTo("baz") + }) + + It("returns an UnprocessableEntityError", func() { + var unprocessableEntityError apierrors.UnprocessableEntityError + Expect(errors.As(updateErr, &unprocessableEntityError)).To(BeTrue()) + Expect(unprocessableEntityError.Detail()).To(SatisfyAll( + ContainSubstring("invalid labels patch"), + ContainSubstring(`"foo.cloudfoundry.org/bar"`), + )) + }) + }) + It("succeeds", func() { Expect(updateErr).NotTo(HaveOccurred()) Expect(packageRecord.GUID).To(Equal(packageGUID)) Expect(packageRecord.Labels).To(HaveKeyWithValue("foo", "bar")) - Expect(packageRecord.Annotations).To(Equal(map[string]string{"bar": "baz"})) + Expect(packageRecord.Annotations).To(Equal(map[string]string{ + "bar": "baz", + korifiv1alpha1.LabelSignatureAnnotationKey: testLabelSig(packageRecord.Labels), + })) Eventually(func(g Gomega) { g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(cfPackage), cfPackage)).To(Succeed()) g.Expect(cfPackage.Labels).To(HaveKeyWithValue("foo", "bar")) - g.Expect(cfPackage.Annotations).To(Equal(map[string]string{"bar": "baz"})) + g.Expect(cfPackage.Annotations).To(Equal(map[string]string{ + "bar": "baz", + korifiv1alpha1.LabelSignatureAnnotationKey: testLabelSig(cfPackage.Labels), + })) }).Should(Succeed()) }) diff --git a/api/repositories/process_repository.go b/api/repositories/process_repository.go index 4d117d17d..cfccffde5 100644 --- a/api/repositories/process_repository.go +++ b/api/repositories/process_repository.go @@ -256,7 +256,9 @@ func (r *ProcessRepo) PatchProcess(ctx context.Context, authInfo authorization.I updatedProcess.Spec.HealthCheck.Data.TimeoutSeconds = *message.HealthCheckTimeoutSeconds } if message.MetadataPatch != nil { - message.MetadataPatch.Apply(updatedProcess) + if err := message.MetadataPatch.Apply(updatedProcess); err != nil { + return err + } } return nil diff --git a/api/repositories/process_repository_test.go b/api/repositories/process_repository_test.go index b77c3a4e7..cac4260b6 100644 --- a/api/repositories/process_repository_test.go +++ b/api/repositories/process_repository_test.go @@ -2,6 +2,7 @@ package repositories_test import ( "context" + "errors" "time" apierrors "code.cloudfoundry.org/korifi/api/errors" @@ -515,6 +516,22 @@ var _ = Describe("ProcessRepo", func() { createRoleBinding(ctx, userName, spaceDeveloperRole.Name, space.Name) }) + When("a label is invalid", func() { + BeforeEach(func() { + message.MetadataPatch.Labels["foo.cloudfoundry.org/bar"] = tools.PtrTo("baz") + }) + + It("returns an UnprocessableEntityError", func() { + _, err := processRepo.PatchProcess(ctx, authInfo, message) + var unprocessableEntityError apierrors.UnprocessableEntityError + Expect(errors.As(err, &unprocessableEntityError)).To(BeTrue()) + Expect(unprocessableEntityError.Detail()).To(SatisfyAll( + ContainSubstring("invalid labels patch"), + ContainSubstring(`"foo.cloudfoundry.org/bar"`), + )) + }) + }) + It("updates the process", func() { updatedProcessRecord, err := processRepo.PatchProcess(ctx, authInfo, message) Expect(err).NotTo(HaveOccurred()) diff --git a/api/repositories/repositories_suite_test.go b/api/repositories/repositories_suite_test.go index 610e4d78d..dc48752f1 100644 --- a/api/repositories/repositories_suite_test.go +++ b/api/repositories/repositories_suite_test.go @@ -16,6 +16,7 @@ import ( korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1" "code.cloudfoundry.org/korifi/controllers/webhooks/common_labels" "code.cloudfoundry.org/korifi/controllers/webhooks/label_indexer" + "code.cloudfoundry.org/korifi/controllers/webhooks/label_indexer/signer" "code.cloudfoundry.org/korifi/tests/helpers" "code.cloudfoundry.org/korifi/tools/k8s" authv1 "k8s.io/api/authorization/v1" @@ -116,7 +117,7 @@ var _ = BeforeSuite(func() { k8sManager := helpers.NewK8sManager(testEnv, filepath.Join("helm", "korifi", "controllers", "role.yaml")) common_labels.NewWebhook().SetupWebhookWithManager(k8sManager) - label_indexer.NewWebhook().SetupWebhookWithManager(k8sManager) + label_indexer.NewWebhook([]byte("test-secret")).SetupWebhookWithManager(k8sManager) routesdestwebhook.NewRouteAppDestinationsWebhook().SetupWebhookWithManager(k8sManager) stopManager = helpers.StartK8sManager(k8sManager) @@ -217,6 +218,15 @@ var _ = AfterEach(func() { Expect(k8sClient.Delete(context.Background(), &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: rootNamespace}})).To(Succeed()) }) +const testSigningSecret = "test-secret" + +// testLabelSig computes the expected label-signature annotation value for a +// set of labels using the same secret that is configured in the test suite's +// label indexer webhook. +func testLabelSig(labels map[string]string) string { + return signer.Sign([]byte(testSigningSecret), labels) +} + func createOrgWithCleanup(ctx context.Context, displayName string) *korifiv1alpha1.CFOrg { guid := uuid.NewString() cfOrg := &korifiv1alpha1.CFOrg{ diff --git a/api/repositories/route_repository.go b/api/repositories/route_repository.go index b8ff56945..9efaca4b5 100644 --- a/api/repositories/route_repository.go +++ b/api/repositories/route_repository.go @@ -448,9 +448,7 @@ func (r *RouteRepo) PatchRouteMetadata(ctx context.Context, authInfo authorizati } err := GetAndPatch(ctx, r.klient, route, func() error { - message.Apply(route) - - return nil + return message.MetadataPatch.Apply(route) }) if err != nil { return RouteRecord{}, apierrors.FromK8sError(err, RouteResourceType) diff --git a/api/repositories/service_binding_repository.go b/api/repositories/service_binding_repository.go index 4f7e534e7..6140316ea 100644 --- a/api/repositories/service_binding_repository.go +++ b/api/repositories/service_binding_repository.go @@ -470,8 +470,7 @@ func (r *ServiceBindingRepo) UpdateServiceBinding(ctx context.Context, authInfo } err = r.klient.Patch(ctx, serviceBinding, func() error { - updateMsg.MetadataPatch.Apply(serviceBinding) - return nil + return updateMsg.MetadataPatch.Apply(serviceBinding) }) if err != nil { return ServiceBindingRecord{}, fmt.Errorf("failed to patch service binding metadata: %w", apierrors.FromK8sError(err, ServiceBindingResourceType)) diff --git a/api/repositories/service_binding_repository_test.go b/api/repositories/service_binding_repository_test.go index 3490d54de..7f98a278b 100644 --- a/api/repositories/service_binding_repository_test.go +++ b/api/repositories/service_binding_repository_test.go @@ -1353,6 +1353,21 @@ var _ = Describe("ServiceBindingRepo", func() { createRoleBinding(ctx, userName, adminRole.Name, space.Name) }) + When("a label is invalid", func() { + BeforeEach(func() { + updateMessage.MetadataPatch.Labels["foo.cloudfoundry.org/bar"] = tools.PtrTo("baz") + }) + + It("returns an UnprocessableEntityError", func() { + var unprocessableEntityError apierrors.UnprocessableEntityError + Expect(errors.As(updateErr, &unprocessableEntityError)).To(BeTrue()) + Expect(unprocessableEntityError.Detail()).To(SatisfyAll( + ContainSubstring("invalid labels patch"), + ContainSubstring(`"foo.cloudfoundry.org/bar"`), + )) + }) + }) + It("updates the service binding metadata", func() { Expect(updateErr).NotTo(HaveOccurred()) diff --git a/api/repositories/service_broker_repository.go b/api/repositories/service_broker_repository.go index 7ed7c195b..5512ebb79 100644 --- a/api/repositories/service_broker_repository.go +++ b/api/repositories/service_broker_repository.go @@ -59,7 +59,7 @@ type UpdateServiceBrokerMessage struct { MetadataPatch MetadataPatch } -func (m UpdateServiceBrokerMessage) apply(broker *korifiv1alpha1.CFServiceBroker) { +func (m UpdateServiceBrokerMessage) apply(broker *korifiv1alpha1.CFServiceBroker) error { if m.Name != nil { broker.Spec.Name = *m.Name } @@ -68,7 +68,7 @@ func (m UpdateServiceBrokerMessage) apply(broker *korifiv1alpha1.CFServiceBroker broker.Spec.URL = *m.URL } - m.MetadataPatch.Apply(broker) + return m.MetadataPatch.Apply(broker) } type ServiceBrokerRepo struct { @@ -231,8 +231,7 @@ func (r *ServiceBrokerRepo) UpdateServiceBroker(ctx context.Context, authInfo au } if err := GetAndPatch(ctx, r.klient, cfServiceBroker, func() error { - message.apply(cfServiceBroker) - return nil + return message.apply(cfServiceBroker) }); err != nil { return ServiceBrokerRecord{}, apierrors.FromK8sError(err, ServiceBrokerResourceType) } diff --git a/api/repositories/service_broker_repository_test.go b/api/repositories/service_broker_repository_test.go index 15876ba55..132d0ecec 100644 --- a/api/repositories/service_broker_repository_test.go +++ b/api/repositories/service_broker_repository_test.go @@ -1,6 +1,7 @@ package repositories_test import ( + "errors" "time" apierrors "code.cloudfoundry.org/korifi/api/errors" @@ -87,6 +88,7 @@ var _ = Describe("ServiceBrokerRepo", func() { "Labels": HaveKeyWithValue("label", "label-value"), "Annotations": Equal(map[string]string{ "annotation": "annotation-value", + korifiv1alpha1.LabelSignatureAnnotationKey: testLabelSig(brokerRecord.Metadata.Labels), }), }), })) @@ -428,6 +430,21 @@ var _ = Describe("ServiceBrokerRepo", func() { createRoleBinding(ctx, userName, adminRole.Name, rootNamespace) }) + When("a label is invalid", func() { + BeforeEach(func() { + updateMessage.MetadataPatch.Labels["foo.cloudfoundry.org/bar"] = tools.PtrTo("baz") + }) + + It("returns an UnprocessableEntityError", func() { + var unprocessableEntityError apierrors.UnprocessableEntityError + Expect(errors.As(updateErr, &unprocessableEntityError)).To(BeTrue()) + Expect(unprocessableEntityError.Detail()).To(SatisfyAll( + ContainSubstring("invalid labels patch"), + ContainSubstring(`"foo.cloudfoundry.org/bar"`), + )) + }) + }) + It("returns an updated ServiceBrokerRecord", func() { Expect(updateErr).NotTo(HaveOccurred()) Expect(brokerRecord).To(MatchFields(IgnoreExtras, Fields{ diff --git a/api/repositories/service_instance_repository.go b/api/repositories/service_instance_repository.go index 3d1f62eee..d10f70816 100644 --- a/api/repositories/service_instance_repository.go +++ b/api/repositories/service_instance_repository.go @@ -77,14 +77,14 @@ type PatchServiceInstanceMessage struct { MetadataPatch } -func (p PatchServiceInstanceMessage) Apply(cfServiceInstance *korifiv1alpha1.CFServiceInstance) { +func (p PatchServiceInstanceMessage) Apply(cfServiceInstance *korifiv1alpha1.CFServiceInstance) error { if p.Name != nil { cfServiceInstance.Spec.DisplayName = *p.Name } if p.Tags != nil { cfServiceInstance.Spec.Tags = *p.Tags } - p.MetadataPatch.Apply(cfServiceInstance) + return p.MetadataPatch.Apply(cfServiceInstance) } type ListServiceInstanceMessage struct { @@ -286,8 +286,7 @@ func (r *ServiceInstanceRepo) PatchServiceInstance(ctx context.Context, authInfo } err := r.klient.Patch(ctx, cfServiceInstance, func() error { - message.Apply(cfServiceInstance) - return nil + return message.Apply(cfServiceInstance) }) if err != nil { return ServiceInstanceRecord{}, apierrors.FromK8sError(err, ServiceInstanceResourceType) diff --git a/api/repositories/service_instance_repository_test.go b/api/repositories/service_instance_repository_test.go index 35401f69f..108665eae 100644 --- a/api/repositories/service_instance_repository_test.go +++ b/api/repositories/service_instance_repository_test.go @@ -551,15 +551,32 @@ var _ = Describe("ServiceInstanceRepository", func() { createRoleBinding(ctx, userName, spaceDeveloperRole.Name, space.Name) }) + When("a label is invalid", func() { + BeforeEach(func() { + patchMessage.MetadataPatch.Labels["foo.cloudfoundry.org/bar"] = tools.PtrTo("baz") + }) + + It("returns an UnprocessableEntityError", func() { + var unprocessableEntityError apierrors.UnprocessableEntityError + Expect(errors.As(err, &unprocessableEntityError)).To(BeTrue()) + Expect(unprocessableEntityError.Detail()).To(SatisfyAll( + ContainSubstring("invalid labels patch"), + ContainSubstring(`"foo.cloudfoundry.org/bar"`), + )) + }) + }) + It("returns the updated record", func() { Expect(err).NotTo(HaveOccurred()) Expect(serviceInstanceRecord.Name).To(Equal("new-name")) Expect(serviceInstanceRecord.Tags).To(ConsistOf("new")) Expect(serviceInstanceRecord.Labels).To(HaveKeyWithValue("a-label", "a-label-value")) Expect(serviceInstanceRecord.Labels).To(HaveKeyWithValue("new-label", "new-label-value")) - Expect(serviceInstanceRecord.Annotations).To(HaveLen(2)) - Expect(serviceInstanceRecord.Annotations).To(HaveKeyWithValue("an-annotation", "an-annotation-value")) - Expect(serviceInstanceRecord.Annotations).To(HaveKeyWithValue("new-annotation", "new-annotation-value")) + Expect(serviceInstanceRecord.Annotations).To(Equal(map[string]string{ + "an-annotation": "an-annotation-value", + "new-annotation": "new-annotation-value", + korifiv1alpha1.LabelSignatureAnnotationKey: testLabelSig(serviceInstanceRecord.Labels), + })) }) It("updates the service instance", func() { @@ -572,9 +589,11 @@ var _ = Describe("ServiceInstanceRepository", func() { g.Expect(serviceInstance.Spec.Tags).To(ConsistOf("new")) g.Expect(serviceInstance.Labels).To(HaveKeyWithValue("a-label", "a-label-value")) g.Expect(serviceInstance.Labels).To(HaveKeyWithValue("new-label", "new-label-value")) - g.Expect(serviceInstance.Annotations).To(HaveLen(2)) - g.Expect(serviceInstance.Annotations).To(HaveKeyWithValue("an-annotation", "an-annotation-value")) - g.Expect(serviceInstance.Annotations).To(HaveKeyWithValue("new-annotation", "new-annotation-value")) + g.Expect(serviceInstance.Annotations).To(Equal(map[string]string{ + "an-annotation": "an-annotation-value", + "new-annotation": "new-annotation-value", + korifiv1alpha1.LabelSignatureAnnotationKey: testLabelSig(serviceInstance.Labels), + })) }).Should(Succeed()) }) @@ -845,7 +864,10 @@ var _ = Describe("ServiceInstanceRepository", func() { Expect(record.Tags).To(Equal(serviceInstance.Spec.Tags)) Expect(record.Type).To(Equal(string(serviceInstance.Spec.Type))) Expect(record.Labels).To(HaveKeyWithValue("a-label", "a-label-value")) - Expect(record.Annotations).To(Equal(map[string]string{"an-annotation": "an-annotation-value"})) + Expect(record.Annotations).To(Equal(map[string]string{ + "an-annotation": "an-annotation-value", + korifiv1alpha1.LabelSignatureAnnotationKey: testLabelSig(record.Labels), + })) Expect(record.Relationships()).To(Equal(map[string]string{ "space": serviceInstance.Namespace, })) diff --git a/api/repositories/service_offering_repository.go b/api/repositories/service_offering_repository.go index cbb04553e..92758f415 100644 --- a/api/repositories/service_offering_repository.go +++ b/api/repositories/service_offering_repository.go @@ -77,8 +77,8 @@ type DeleteServiceOfferingMessage struct { Purge bool } -func (m UpdateServiceOfferingMessage) apply(offering *korifiv1alpha1.CFServiceOffering) { - m.MetadataPatch.Apply(offering) +func (m UpdateServiceOfferingMessage) apply(offering *korifiv1alpha1.CFServiceOffering) error { + return m.MetadataPatch.Apply(offering) } func (m *ListServiceOfferingMessage) toListOptions() []ListOption { @@ -289,8 +289,7 @@ func (r *ServiceOfferingRepo) UpdateServiceOffering(ctx context.Context, authInf }, } if err := GetAndPatch(ctx, r.rootNSKlient, offering, func() error { - message.apply(offering) - return nil + return message.apply(offering) }); err != nil { return ServiceOfferingRecord{}, fmt.Errorf("failed to patch service offering metadata: %w", apierrors.FromK8sError(err, ServiceOfferingResourceType)) } diff --git a/api/repositories/service_offering_repository_test.go b/api/repositories/service_offering_repository_test.go index f0602a0b2..68813ace4 100644 --- a/api/repositories/service_offering_repository_test.go +++ b/api/repositories/service_offering_repository_test.go @@ -510,6 +510,21 @@ var _ = Describe("ServiceOfferingRepo", func() { createRoleBinding(ctx, userName, adminRole.Name, rootNamespace) }) + When("a label is invalid", func() { + BeforeEach(func() { + updateMessage.MetadataPatch.Labels["foo.cloudfoundry.org/bar"] = tools.PtrTo("baz") + }) + + It("returns an UnprocessableEntityError", func() { + var unprocessableEntityError apierrors.UnprocessableEntityError + Expect(errors.As(updateErr, &unprocessableEntityError)).To(BeTrue()) + Expect(unprocessableEntityError.Detail()).To(SatisfyAll( + ContainSubstring("invalid labels patch"), + ContainSubstring(`"foo.cloudfoundry.org/bar"`), + )) + }) + }) + It("updates the service offering metadata", func() { Expect(updateErr).NotTo(HaveOccurred()) Expect(updatedServiceOffering.Metadata.Labels).To(SatisfyAll( diff --git a/api/repositories/space_repository.go b/api/repositories/space_repository.go index 8e2a45b85..ec289b6c4 100644 --- a/api/repositories/space_repository.go +++ b/api/repositories/space_repository.go @@ -63,11 +63,11 @@ type PatchSpaceMessage struct { Name *string } -func (p *PatchSpaceMessage) Apply(space *korifiv1alpha1.CFSpace) { +func (p *PatchSpaceMessage) Apply(space *korifiv1alpha1.CFSpace) error { if p.Name != nil { space.Spec.DisplayName = *p.Name } - p.MetadataPatch.Apply(space) + return p.MetadataPatch.Apply(space) } type SpaceRecord struct { @@ -233,8 +233,7 @@ func (r *SpaceRepo) PatchSpace(ctx context.Context, authInfo authorization.Info, } err = r.klient.Patch(ctx, cfSpace, func() error { - message.Apply(cfSpace) - return nil + return message.Apply(cfSpace) }) if err != nil { return SpaceRecord{}, apierrors.FromK8sError(err, SpaceResourceType) diff --git a/api/repositories/space_repository_test.go b/api/repositories/space_repository_test.go index 99fde8e63..b8094058f 100644 --- a/api/repositories/space_repository_test.go +++ b/api/repositories/space_repository_test.go @@ -518,6 +518,7 @@ var _ = Describe("SpaceRepository", func() { map[string]string{ "key-one": "value-one", "key-two": "value-two", + korifiv1alpha1.LabelSignatureAnnotationKey: testLabelSig(spaceRecord.Labels), }, )) Expect(spaceRecord.Name).To(Equal(*spaceNewName)) @@ -535,6 +536,7 @@ var _ = Describe("SpaceRepository", func() { map[string]string{ "key-one": "value-one", "key-two": "value-two", + korifiv1alpha1.LabelSignatureAnnotationKey: testLabelSig(updatedCFSpace.Labels), }, )) Expect(updatedCFSpace.Spec.DisplayName).To(Equal(*spaceNewName)) @@ -582,6 +584,7 @@ var _ = Describe("SpaceRepository", func() { "before-key-one": "value-one", "key-one": "value-one-updated", "key-two": "value-two", + korifiv1alpha1.LabelSignatureAnnotationKey: testLabelSig(spaceRecord.Labels), }, )) Expect(spaceRecord.Name).To(Equal(*spaceNewName)) @@ -601,6 +604,7 @@ var _ = Describe("SpaceRepository", func() { "before-key-one": "value-one", "key-one": "value-one-updated", "key-two": "value-two", + korifiv1alpha1.LabelSignatureAnnotationKey: testLabelSig(updatedCFSpace.Labels), }, )) Expect(spaceRecord.Name).To(Equal(*spaceNewName)) diff --git a/api/repositories/task_repository.go b/api/repositories/task_repository.go index 072bfd9b2..c9faa2ccd 100644 --- a/api/repositories/task_repository.go +++ b/api/repositories/task_repository.go @@ -203,8 +203,7 @@ func (r *TaskRepo) PatchTaskMetadata(ctx context.Context, authInfo authorization } err := GetAndPatch(ctx, r.klient, task, func() error { - message.Apply(task) - return nil + return message.MetadataPatch.Apply(task) }) if err != nil { return TaskRecord{}, apierrors.FromK8sError(err, TaskResourceType) diff --git a/api/repositories/task_repository_test.go b/api/repositories/task_repository_test.go index 1e7dcf549..6ffc1c371 100644 --- a/api/repositories/task_repository_test.go +++ b/api/repositories/task_repository_test.go @@ -137,7 +137,10 @@ var _ = Describe("TaskRepository", func() { Expect(taskRecord.DropletGUID).To(Equal(cfApp.Spec.CurrentDropletRef.Name)) Expect(taskRecord.State).To(Equal(repositories.TaskStatePending)) Expect(taskRecord.Labels).To(HaveKeyWithValue("color", "blue")) - Expect(taskRecord.Annotations).To(Equal(map[string]string{"extra-bugs": "true"})) + Expect(taskRecord.Annotations).To(Equal(map[string]string{ + "extra-bugs": "true", + korifiv1alpha1.LabelSignatureAnnotationKey: testLabelSig(taskRecord.Labels), + })) }) When("the task never becomes initialized", func() { @@ -629,6 +632,7 @@ var _ = Describe("TaskRepository", func() { map[string]string{ "key-one": "value-one", "key-two": "value-two", + korifiv1alpha1.LabelSignatureAnnotationKey: testLabelSig(taskRecord.Labels), }, )) }) @@ -645,6 +649,7 @@ var _ = Describe("TaskRepository", func() { map[string]string{ "key-one": "value-one", "key-two": "value-two", + korifiv1alpha1.LabelSignatureAnnotationKey: testLabelSig(updatedCFTask.Labels), }, )) }) @@ -690,6 +695,7 @@ var _ = Describe("TaskRepository", func() { "before-key-one": "value-one", "key-one": "value-one-updated", "key-two": "value-two", + korifiv1alpha1.LabelSignatureAnnotationKey: testLabelSig(taskRecord.Labels), }, )) }) @@ -708,6 +714,7 @@ var _ = Describe("TaskRepository", func() { "before-key-one": "value-one", "key-one": "value-one-updated", "key-two": "value-two", + korifiv1alpha1.LabelSignatureAnnotationKey: testLabelSig(updatedCFTask.Labels), }, )) }) diff --git a/controllers/api/v1alpha1/shared_types.go b/controllers/api/v1alpha1/shared_types.go index ed7f49b26..168b9dfaa 100644 --- a/controllers/api/v1alpha1/shared_types.go +++ b/controllers/api/v1alpha1/shared_types.go @@ -50,6 +50,8 @@ const ( PropagateDeletionAnnotation = "cloudfoundry.org/propagate-deletion" PropagatedFromLabel = "cloudfoundry.org/propagated-from" + LabelSignatureAnnotationKey = "korifi.cloudfoundry.org/label-signature" + RelationshipsLabelPrefix = "korifi.cloudfoundry.org/rel-" RelServiceBrokerGUIDLabel = RelationshipsLabelPrefix + "service-broker-guid" RelServiceBrokerNameLabel = RelationshipsLabelPrefix + "service-broker-name" diff --git a/controllers/main.go b/controllers/main.go index b3f0cad53..6bc23af51 100644 --- a/controllers/main.go +++ b/controllers/main.go @@ -22,6 +22,7 @@ import ( "fmt" "log" "os" + "path/filepath" korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1" "code.cloudfoundry.org/korifi/controllers/cleanup" @@ -51,6 +52,7 @@ import ( "code.cloudfoundry.org/korifi/controllers/webhooks/common_labels" controllers_finalizer "code.cloudfoundry.org/korifi/controllers/webhooks/finalizer" "code.cloudfoundry.org/korifi/controllers/webhooks/label_indexer" + label_validator "code.cloudfoundry.org/korifi/controllers/webhooks/label_validator" domainswebhook "code.cloudfoundry.org/korifi/controllers/webhooks/networking/domains" routeswebhook "code.cloudfoundry.org/korifi/controllers/webhooks/networking/routes" routesdestwebhook "code.cloudfoundry.org/korifi/controllers/webhooks/networking/routes/app_destinations" @@ -150,6 +152,15 @@ func main() { ctrl.Log.Info("starting Korifi controllers", "version", version.Version) + labelSigningSecretPath := os.Getenv("LABEL_SIGNING_SECRET_PATH") + if labelSigningSecretPath == "" { + labelSigningSecretPath = "/etc/korifi-label-signing-secret/key" + } + labelSigningSecret, err := os.ReadFile(filepath.Clean(labelSigningSecretPath)) + if err != nil { + panic(fmt.Sprintf("could not read label signing secret: %v", err)) + } + conf := ctrl.GetConfigOrDie() k8sClient, err := k8sclient.NewForConfig(conf) if err != nil { @@ -536,7 +547,8 @@ func main() { versionwebhook.NewVersionWebhook(version.Version).SetupWebhookWithManager(mgr) controllers_finalizer.NewControllersFinalizerWebhook().SetupWebhookWithManager(mgr) common_labels.NewWebhook().SetupWebhookWithManager(mgr) - label_indexer.NewWebhook().SetupWebhookWithManager(mgr) + label_indexer.NewWebhook(labelSigningSecret).SetupWebhookWithManager(mgr) + label_validator.NewWebhook(labelSigningSecret).SetupWebhookWithManager(mgr) routesdestwebhook.NewRouteAppDestinationsWebhook().SetupWebhookWithManager(mgr) if err = packageswebhook.NewValidator().SetupWebhookWithManager(mgr); err != nil { diff --git a/controllers/webhooks/label_indexer/signer/signer.go b/controllers/webhooks/label_indexer/signer/signer.go new file mode 100644 index 000000000..28cee5a39 --- /dev/null +++ b/controllers/webhooks/label_indexer/signer/signer.go @@ -0,0 +1,37 @@ +package signer + +import ( + "crypto/hmac" + "crypto/sha256" + "encoding/hex" + "sort" + "strings" +) + +const ReservedLabelDomain = "korifi.cloudfoundry.org/" + +func Sign(secret []byte, labels map[string]string) string { + mac := hmac.New(sha256.New, secret) + for _, k := range sortedReservedKeys(labels) { + mac.Write([]byte(k)) + mac.Write([]byte("=")) + mac.Write([]byte(labels[k])) + mac.Write([]byte("\n")) + } + return hex.EncodeToString(mac.Sum(nil)) +} + +func Verify(secret []byte, labels map[string]string, sig string) bool { + return hmac.Equal([]byte(Sign(secret, labels)), []byte(sig)) +} + +func sortedReservedKeys(labels map[string]string) []string { + keys := make([]string, 0, len(labels)) + for k := range labels { + if strings.HasPrefix(k, ReservedLabelDomain) { + keys = append(keys, k) + } + } + sort.Strings(keys) + return keys +} diff --git a/controllers/webhooks/label_indexer/suite_test.go b/controllers/webhooks/label_indexer/suite_test.go index 4f4c5d848..264566304 100644 --- a/controllers/webhooks/label_indexer/suite_test.go +++ b/controllers/webhooks/label_indexer/suite_test.go @@ -68,7 +68,7 @@ var _ = BeforeSuite(func() { adminClient, stopClientCache = helpers.NewCachedClient(testEnv.Config) - label_indexer.NewWebhook().SetupWebhookWithManager(k8sManager) + label_indexer.NewWebhook([]byte("test-secret")).SetupWebhookWithManager(k8sManager) stopManager = helpers.StartK8sManager(k8sManager) diff --git a/controllers/webhooks/label_indexer/webhook.go b/controllers/webhooks/label_indexer/webhook.go index f197da0e3..bad4ac216 100644 --- a/controllers/webhooks/label_indexer/webhook.go +++ b/controllers/webhooks/label_indexer/webhook.go @@ -8,7 +8,8 @@ import ( "net/http" korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1" - . "code.cloudfoundry.org/korifi/controllers/webhooks/label_indexer/rules" //lint:ignore ST1001 for readability + . "code.cloudfoundry.org/korifi/controllers/webhooks/label_indexer/rules" //lint:ignore ST1001 for readability + "code.cloudfoundry.org/korifi/controllers/webhooks/label_indexer/signer" . "code.cloudfoundry.org/korifi/controllers/webhooks/label_indexer/values" //lint:ignore ST1001 for readability "code.cloudfoundry.org/korifi/tools" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -23,10 +24,12 @@ type IndexingRule interface { type LabelIndexerWebhook struct { decoder admission.Decoder indexingRules map[string][]IndexingRule + signingSecret []byte } -func NewWebhook() *LabelIndexerWebhook { +func NewWebhook(signingSecret []byte) *LabelIndexerWebhook { return &LabelIndexerWebhook{ + signingSecret: signingSecret, indexingRules: map[string][]IndexingRule{ "CFRoute": { LabelRule{Label: korifiv1alpha1.CFDomainGUIDLabelKey, IndexingFunc: Unquote(JSONValue("$.spec.domainRef.name"))}, @@ -157,6 +160,9 @@ func (r *LabelIndexerWebhook) Handle(ctx context.Context, req admission.Request) } } + sig := signer.Sign(r.signingSecret, obj.GetLabels()) + obj.SetAnnotations(tools.SetMapValue(obj.GetAnnotations(), korifiv1alpha1.LabelSignatureAnnotationKey, sig)) + marshalled, err := json.Marshal(obj) if err != nil { return admission.Errored(http.StatusInternalServerError, err) diff --git a/controllers/webhooks/label_validator/suite_test.go b/controllers/webhooks/label_validator/suite_test.go new file mode 100644 index 000000000..9aab364ab --- /dev/null +++ b/controllers/webhooks/label_validator/suite_test.go @@ -0,0 +1,94 @@ +package label_validator_test + +import ( + "context" + "os" + "path/filepath" + "testing" + "time" + + korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1" + "code.cloudfoundry.org/korifi/controllers/webhooks/label_indexer" + "code.cloudfoundry.org/korifi/controllers/webhooks/label_validator" + "code.cloudfoundry.org/korifi/tests/helpers" + + "github.com/google/uuid" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/envtest" + logf "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/log/zap" +) + +const testSigningSecret = "test-secret" + +var ( + ctx context.Context + stopManager context.CancelFunc + stopClientCache context.CancelFunc + testEnv *envtest.Environment + adminClient client.Client + namespace string +) + +func TestLabelValidatorWebhook(t *testing.T) { + SetDefaultEventuallyTimeout(10 * time.Second) + SetDefaultEventuallyPollingInterval(250 * time.Millisecond) + + RegisterFailHandler(Fail) + RunSpecs(t, "Label Validator Webhook Integration Test Suite") +} + +var _ = BeforeSuite(func() { + logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true))) + + webhookManifestsPath := helpers.GenerateWebhookManifest( + "code.cloudfoundry.org/korifi/controllers/webhooks/label_indexer", + "code.cloudfoundry.org/korifi/controllers/webhooks/label_validator", + ) + DeferCleanup(func() { + Expect(os.RemoveAll(filepath.Dir(webhookManifestsPath))).To(Succeed()) + }) + + testEnv = &envtest.Environment{ + CRDDirectoryPaths: []string{ + filepath.Join("..", "..", "..", "helm", "korifi", "controllers", "crds"), + }, + ErrorIfCRDPathMissing: true, + WebhookInstallOptions: envtest.WebhookInstallOptions{ + Paths: []string{webhookManifestsPath}, + }, + } + + _, err := testEnv.Start() + Expect(err).NotTo(HaveOccurred()) + + Expect(korifiv1alpha1.AddToScheme(scheme.Scheme)).To(Succeed()) + + k8sManager := helpers.NewK8sManager(testEnv, filepath.Join("helm", "korifi", "controllers", "role.yaml")) + + adminClient, stopClientCache = helpers.NewCachedClient(testEnv.Config) + + label_indexer.NewWebhook([]byte(testSigningSecret)).SetupWebhookWithManager(k8sManager) + label_validator.NewWebhook([]byte(testSigningSecret)).SetupWebhookWithManager(k8sManager) + + stopManager = helpers.StartK8sManager(k8sManager) + + ctx = context.Background() + namespace = uuid.NewString() + Expect(adminClient.Create(ctx, &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: namespace, + }, + })).To(Succeed()) +}) + +var _ = AfterSuite(func() { + stopManager() + stopClientCache() + Eventually(testEnv.Stop, "1m").Should(Succeed()) +}) diff --git a/controllers/webhooks/label_validator/webhook.go b/controllers/webhooks/label_validator/webhook.go new file mode 100644 index 000000000..2094c1c2e --- /dev/null +++ b/controllers/webhooks/label_validator/webhook.go @@ -0,0 +1,79 @@ +package label_validator + +//+kubebuilder:webhook:path=/validate-korifi-cloudfoundry-org-v1alpha1-controllers-label-validator,mutating=false,failurePolicy=fail,sideEffects=None,groups=korifi.cloudfoundry.org,resources=cfroutes;cfapps;cfbuilds;cfdomains;cfpackages;cfprocesses;cfservicebindings;cfserviceinstances;cftasks;cforgs;cfspaces;cfserviceofferings;cfserviceplans;cfservicebrokers,verbs=create;update,versions=v1alpha1,name=vcflabelvalidator.korifi.cloudfoundry.org,admissionReviewVersions={v1,v1beta1} + +import ( + "context" + "fmt" + "net/http" + "net/url" + "strings" + + korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1" + "code.cloudfoundry.org/korifi/controllers/webhooks/label_indexer/signer" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" +) + +type LabelValidatorWebhook struct { + decoder admission.Decoder + signingSecret []byte +} + +func NewWebhook(signingSecret []byte) *LabelValidatorWebhook { + return &LabelValidatorWebhook{ + signingSecret: signingSecret, + } +} + +func (r *LabelValidatorWebhook) SetupWebhookWithManager(mgr ctrl.Manager) { + mgr.GetWebhookServer().Register("/validate-korifi-cloudfoundry-org-v1alpha1-controllers-label-validator", &admission.Webhook{ + Handler: r, + }) + r.decoder = admission.NewDecoder(mgr.GetScheme()) +} + +func (r *LabelValidatorWebhook) Handle(_ context.Context, req admission.Request) admission.Response { + var obj metav1.PartialObjectMetadata + if err := r.decoder.Decode(req, &obj); err != nil { + return admission.Errored(http.StatusBadRequest, err) + } + + // Reject any label key in the broader cloudfoundry.org domain that is NOT + // a korifi-managed key (those are handled by the signature check below). + for k := range obj.GetLabels() { + if !strings.HasPrefix(k, signer.ReservedLabelDomain) && isCloudfoundryDomain(k) { + return admission.Denied(fmt.Sprintf( + "label key %q cannot use the cloudfoundry.org domain", k, + )) + } + } + + storedSig := obj.GetAnnotations()[korifiv1alpha1.LabelSignatureAnnotationKey] + + // On first create (no existing signature yet) the label indexer has already + // run and written a fresh signature, so storedSig will be present. + // If it is still absent (e.g. pre-existing objects during rollout) we allow + // the request through to avoid blocking legitimate operations. + if storedSig == "" { + return admission.Allowed("no existing label signature") + } + + if !signer.Verify(r.signingSecret, obj.GetLabels(), storedSig) { + return admission.Denied(fmt.Sprintf( + "reserved %s labels may not be modified by API clients", + signer.ReservedLabelDomain, + )) + } + + return admission.Allowed("label signature verified") +} + +func isCloudfoundryDomain(key string) bool { + u, err := url.ParseRequestURI("https://" + key) + if err != nil { + return false + } + return strings.HasSuffix(u.Hostname(), "cloudfoundry.org") +} diff --git a/controllers/webhooks/label_validator/webhook_test.go b/controllers/webhooks/label_validator/webhook_test.go new file mode 100644 index 000000000..3fd6c91ce --- /dev/null +++ b/controllers/webhooks/label_validator/webhook_test.go @@ -0,0 +1,94 @@ +package label_validator_test + +import ( + korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1" + "code.cloudfoundry.org/korifi/tools/k8s" + + "github.com/google/uuid" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +var _ = Describe("LabelValidatorWebhook", func() { + var org *korifiv1alpha1.CFOrg + + BeforeEach(func() { + org = &korifiv1alpha1.CFOrg{ + ObjectMeta: metav1.ObjectMeta{ + Name: uuid.NewString(), + Namespace: namespace, + }, + Spec: korifiv1alpha1.CFOrgSpec{ + DisplayName: uuid.NewString(), + }, + } + }) + + Describe("on create", func() { + It("allows a resource with no user labels", func() { + Expect(adminClient.Create(ctx, org)).To(Succeed()) + Eventually(func(g Gomega) { + g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(org), org)).To(Succeed()) + g.Expect(org.Annotations).To(HaveKey(korifiv1alpha1.LabelSignatureAnnotationKey)) + }).Should(Succeed()) + }) + + It("allows a resource with arbitrary user labels", func() { + org.Labels = map[string]string{"user-label": "value"} + Expect(adminClient.Create(ctx, org)).To(Succeed()) + }) + + It("rejects a non-korifi cloudfoundry.org label key", func() { + org.Labels = map[string]string{"foo.cloudfoundry.org/bar": "baz"} + Expect(adminClient.Create(ctx, org)).To(MatchError(ContainSubstring("cannot use the cloudfoundry.org domain"))) + }) + }) + + Describe("on update", func() { + JustBeforeEach(func() { + Expect(adminClient.Create(ctx, org)).To(Succeed()) + Eventually(func(g Gomega) { + g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(org), org)).To(Succeed()) + g.Expect(org.Annotations).To(HaveKey(korifiv1alpha1.LabelSignatureAnnotationKey)) + }).Should(Succeed()) + }) + + It("allows user labels that don't touch korifi labels", func() { + Expect(k8s.Patch(ctx, adminClient, org, func() { + if org.Labels == nil { + org.Labels = map[string]string{} + } + org.Labels["my-custom-label"] = "my-value" + })).To(Succeed()) + }) + + It("rejects tampering with a korifi.cloudfoundry.org label", func() { + originalReservedValue := org.Labels[korifiv1alpha1.CFOrgDisplayNameKey] + + err := k8s.Patch(ctx, adminClient, org, func() { + if org.Labels == nil { + org.Labels = map[string]string{} + } + org.Labels[korifiv1alpha1.CFOrgDisplayNameKey] = "tampered-value" + }) + Expect(err).NotTo(HaveOccurred()) + + Eventually(func(g Gomega) { + g.Expect(adminClient.Get(ctx, client.ObjectKeyFromObject(org), org)).To(Succeed()) + g.Expect(org.Labels).To(HaveKeyWithValue(korifiv1alpha1.CFOrgDisplayNameKey, originalReservedValue)) + }).Should(Succeed()) + }) + + It("rejects a non-korifi cloudfoundry.org label key", func() { + err := k8s.Patch(ctx, adminClient, org, func() { + if org.Labels == nil { + org.Labels = map[string]string{} + } + org.Labels["foo.cloudfoundry.org/bar"] = "baz" + }) + Expect(err).To(MatchError(ContainSubstring("cannot use the cloudfoundry.org domain"))) + }) + }) +}) diff --git a/helm/korifi/controllers/deployment.yaml b/helm/korifi/controllers/deployment.yaml index d83c5c32b..7ee002c0e 100644 --- a/helm/korifi/controllers/deployment.yaml +++ b/helm/korifi/controllers/deployment.yaml @@ -58,6 +58,9 @@ spec: - mountPath: /etc/korifi-controllers-config name: korifi-controllers-config readOnly: true + - mountPath: /etc/korifi-label-signing-secret + name: label-signing-secret + readOnly: true {{- include "korifi.podSecurityContext" . | indent 6 }} serviceAccountName: korifi-controllers-controller-manager {{- if .Values.controllers.nodeSelector }} @@ -77,3 +80,7 @@ spec: - configMap: name: korifi-controllers-config name: korifi-controllers-config + - name: label-signing-secret + secret: + defaultMode: 420 + secretName: {{ .Values.controllers.labelSigningSecret }} diff --git a/helm/korifi/controllers/label-signing-secret.yaml b/helm/korifi/controllers/label-signing-secret.yaml new file mode 100644 index 000000000..2695a3215 --- /dev/null +++ b/helm/korifi/controllers/label-signing-secret.yaml @@ -0,0 +1,13 @@ +{{- $existing := lookup "v1" "Secret" .Release.Namespace .Values.controllers.labelSigningSecret }} +apiVersion: v1 +kind: Secret +metadata: + name: {{ .Values.controllers.labelSigningSecret }} + namespace: {{ .Release.Namespace }} +type: Opaque +data: + {{- if $existing }} + key: {{ index $existing.data "key" }} + {{- else }} + key: {{ randBytes 32 | b64enc }} + {{- end }} diff --git a/helm/korifi/controllers/manifests.yaml b/helm/korifi/controllers/manifests.yaml index 0295dab19..15e7138cb 100644 --- a/helm/korifi/controllers/manifests.yaml +++ b/helm/korifi/controllers/manifests.yaml @@ -302,6 +302,41 @@ webhooks: resources: - cfdomains sideEffects: None + - admissionReviewVersions: + - v1 + - v1beta1 + clientConfig: + service: + name: korifi-controllers-webhook-service + namespace: '{{ .Release.Namespace }}' + path: /validate-korifi-cloudfoundry-org-v1alpha1-controllers-label-validator + caBundle: '{{ include "korifi.webhookCaBundle" . }}' + failurePolicy: Fail + name: vcflabelvalidator.korifi.cloudfoundry.org + rules: + - apiGroups: + - korifi.cloudfoundry.org + apiVersions: + - v1alpha1 + operations: + - CREATE + - UPDATE + resources: + - cfroutes + - cfapps + - cfbuilds + - cfdomains + - cfpackages + - cfprocesses + - cfservicebindings + - cfserviceinstances + - cftasks + - cforgs + - cfspaces + - cfserviceofferings + - cfserviceplans + - cfservicebrokers + sideEffects: None - admissionReviewVersions: - v1 - v1beta1 diff --git a/helm/korifi/migration/post-upgrade-migration.yaml b/helm/korifi/migration/post-upgrade-migration.yaml index 9a5116344..050b1289f 100644 --- a/helm/korifi/migration/post-upgrade-migration.yaml +++ b/helm/korifi/migration/post-upgrade-migration.yaml @@ -31,7 +31,13 @@ spec: env: - name: KORIFI_VERSION value: "{{ .Chart.Version }}" + - name: LABEL_SIGNING_SECRET_PATH + value: /etc/korifi-label-signing-secret/key image: {{ .Values.migration.image }} + volumeMounts: + - mountPath: /etc/korifi-label-signing-secret + name: label-signing-secret + readOnly: true securityContext: allowPrivilegeEscalation: false runAsNonRoot: true @@ -41,3 +47,8 @@ spec: - ALL seccompProfile: type: RuntimeDefault + volumes: + - name: label-signing-secret + secret: + defaultMode: 420 + secretName: {{ .Values.controllers.labelSigningSecret }} diff --git a/helm/korifi/values.yaml b/helm/korifi/values.yaml index f4ccca936..85561ea99 100644 --- a/helm/korifi/values.yaml +++ b/helm/korifi/values.yaml @@ -76,6 +76,7 @@ api: controllers: image: cloudfoundry/korifi-controllers:latest webhookCertSecret: "korifi-controllers-webhook-cert" + labelSigningSecret: "korifi-controllers-label-signing-secret" nodeSelector: {} tolerations: [] diff --git a/migration/main.go b/migration/main.go index 23d95ac51..8efe744fe 100644 --- a/migration/main.go +++ b/migration/main.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "os" + "path/filepath" "runtime" korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1" @@ -31,9 +32,18 @@ func main() { panic("KORIFI_VERSION must be set") } + labelSigningSecretPath := os.Getenv("LABEL_SIGNING_SECRET_PATH") + if labelSigningSecretPath == "" { + labelSigningSecretPath = "/etc/korifi-label-signing-secret/key" + } + labelSigningSecret, err := os.ReadFile(filepath.Clean(labelSigningSecretPath)) + if err != nil { + panic(fmt.Sprintf("could not read label signing secret: %v", err)) + } + workersCount := tools.Max(1, runtime.NumCPU()/2) - migrator := migration.New(k8sClient, korifiVersion, workersCount) + migrator := migration.New(k8sClient, korifiVersion, labelSigningSecret, workersCount) err = migrator.Run(context.Background()) if err != nil { panic(err) diff --git a/migration/migration/executor.go b/migration/migration/executor.go index d2d2d3976..ee242e964 100644 --- a/migration/migration/executor.go +++ b/migration/migration/executor.go @@ -8,6 +8,8 @@ import ( "sync" "time" + korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1" + "code.cloudfoundry.org/korifi/controllers/webhooks/label_indexer/signer" "code.cloudfoundry.org/korifi/tools" "code.cloudfoundry.org/korifi/tools/k8s" "sigs.k8s.io/controller-runtime/pkg/client" @@ -18,22 +20,45 @@ const MigratedByLabelKey = "korifi.cloudfoundry.org/migrated-by" var korifiObjectLists = []client.ObjectList{} type Migrator struct { - k8sClient client.Client - korifiVersion string - workersCount int + k8sClient client.Client + korifiVersion string + labelSigningSecret []byte + workersCount int } -func New(k8sClient client.Client, korifiVersion string, workersCount int) *Migrator { +func New(k8sClient client.Client, korifiVersion string, labelSigningSecret []byte, workersCount int) *Migrator { return &Migrator{ - k8sClient: k8sClient, - korifiVersion: korifiVersion, - workersCount: workersCount, + k8sClient: k8sClient, + korifiVersion: korifiVersion, + labelSigningSecret: labelSigningSecret, + workersCount: workersCount, } } +var backfillObjectLists = []client.ObjectList{ + &korifiv1alpha1.CFOrgList{}, + &korifiv1alpha1.CFSpaceList{}, + &korifiv1alpha1.CFAppList{}, + &korifiv1alpha1.CFBuildList{}, + &korifiv1alpha1.CFPackageList{}, + &korifiv1alpha1.CFProcessList{}, + &korifiv1alpha1.CFRouteList{}, + &korifiv1alpha1.CFDomainList{}, + &korifiv1alpha1.CFServiceInstanceList{}, + &korifiv1alpha1.CFServiceBindingList{}, + &korifiv1alpha1.CFTaskList{}, + &korifiv1alpha1.CFServiceBrokerList{}, + &korifiv1alpha1.CFServiceOfferingList{}, + &korifiv1alpha1.CFServicePlanList{}, +} + func (m *Migrator) Run(ctx context.Context) error { startTime := time.Now() + if err := m.backfillLabelSignatures(ctx); err != nil { + return fmt.Errorf("failed to backfill label signatures: %v", err) + } + objectsToUpdate, err := m.collectObjects(ctx, korifiObjectLists) if err != nil { return fmt.Errorf("failed to collect objects to migrate: %v", err) @@ -82,6 +107,28 @@ func (m *Migrator) setMigratedByLabel(ctx context.Context, obj client.Object) er }) } +func (m *Migrator) backfillLabelSignatures(ctx context.Context) error { + objects, err := m.collectObjects(ctx, backfillObjectLists) + if err != nil { + return fmt.Errorf("failed to collect objects for label signature backfill: %v", err) + } + + fmt.Fprintf(os.Stdout, "Backfilling label signatures on %d objects\n", len(objects)) + for _, obj := range objects { + if obj.GetAnnotations()[korifiv1alpha1.LabelSignatureAnnotationKey] != "" { + continue + } + sig := signer.Sign(m.labelSigningSecret, obj.GetLabels()) + if err := k8s.PatchResource(ctx, m.k8sClient, obj, func() { + obj.SetAnnotations(tools.SetMapValue(obj.GetAnnotations(), korifiv1alpha1.LabelSignatureAnnotationKey, sig)) + }); err != nil { + fmt.Fprintf(os.Stderr, "failed to backfill label signature on %s %s/%s: %v\n", + obj.GetObjectKind().GroupVersionKind().Kind, obj.GetNamespace(), obj.GetName(), err) + } + } + return nil +} + func (m *Migrator) collectObjects(ctx context.Context, objectLists []client.ObjectList) ([]client.Object, error) { var objects []client.Object for _, list := range objectLists { diff --git a/migration/migration/executor_test.go b/migration/migration/executor_test.go index aef44f567..bb5cdbc9c 100644 --- a/migration/migration/executor_test.go +++ b/migration/migration/executor_test.go @@ -13,7 +13,7 @@ var _ = Describe("Executor", func() { ) BeforeEach(func() { - migrator = migration.New(k8sClient, "test-version", 1) + migrator = migration.New(k8sClient, "test-version", []byte{}, 1) }) JustBeforeEach(func() { diff --git a/tests/e2e/orgs_test.go b/tests/e2e/orgs_test.go index 66bc59dd8..eaebf447e 100644 --- a/tests/e2e/orgs_test.go +++ b/tests/e2e/orgs_test.go @@ -250,6 +250,99 @@ var _ = Describe("Orgs", func() { }) }) + Describe("rename", func() { + var ( + orgGUID string + result resource + errResp cfErrs + ) + + BeforeEach(func() { + orgGUID = createOrg(generateGUID("org")) + result = resource{} + errResp = cfErrs{} + }) + + AfterEach(func() { + deleteOrg(orgGUID) + }) + + JustBeforeEach(func() { + var err error + resp, err = adminClient.R(). + SetBody(resource{Name: generateGUID("renamed-org")}). + SetResult(&result). + SetError(&errResp). + Patch("/v3/organizations/" + orgGUID) + Expect(err).NotTo(HaveOccurred()) + }) + + It("succeeds", func() { + Expect(resp).To(HaveRestyStatusCode(http.StatusOK)) + Expect(result.Name).To(HavePrefix("renamed-org")) + }) + + It("can be renamed again", func() { + Expect(resp).To(HaveRestyStatusCode(http.StatusOK)) + + var secondResult resource + secondResp, err := adminClient.R(). + SetBody(resource{Name: generateGUID("renamed-again-org")}). + SetResult(&secondResult). + Patch("/v3/organizations/" + orgGUID) + Expect(err).NotTo(HaveOccurred()) + Expect(secondResp).To(HaveRestyStatusCode(http.StatusOK)) + Expect(secondResult.Name).To(HavePrefix("renamed-again-org")) + }) + }) + + Describe("update metadata", func() { + var ( + orgGUID string + result resource + errResp cfErrs + ) + + BeforeEach(func() { + orgGUID = createOrg(generateGUID("org")) + result = resource{} + errResp = cfErrs{} + }) + + AfterEach(func() { + deleteOrg(orgGUID) + }) + + It("allows user-defined labels", func() { + var err error + resp, err = adminClient.R(). + SetBody(metadataResource{Metadata: &metadataPatch{ + Labels: &map[string]string{"user-label": "value"}, + }}). + SetResult(&result). + SetError(&errResp). + Patch("/v3/organizations/" + orgGUID) + Expect(err).NotTo(HaveOccurred()) + Expect(resp).To(HaveRestyStatusCode(http.StatusOK)) + Expect(result.Metadata.Labels).To(HaveKeyWithValue("user-label", "value")) + }) + + It("rejects cloudfoundry.org domain labels", func() { + var err error + resp, err = adminClient.R(). + SetBody(metadataResource{Metadata: &metadataPatch{ + Labels: &map[string]string{"foo.cloudfoundry.org/bar": "baz"}, + }}). + SetError(&errResp). + Patch("/v3/organizations/" + orgGUID) + Expect(err).NotTo(HaveOccurred()) + Expect(resp).To(HaveRestyStatusCode(http.StatusUnprocessableEntity)) + Expect(errResp.Errors).To(ContainElement( + MatchFields(IgnoreExtras, Fields{"Detail": ContainSubstring("invalid labels patch")}), + )) + }) + }) + Describe("get", func() { var result resource diff --git a/tests/smoke/orgs_test.go b/tests/smoke/orgs_test.go index 62e77d787..2dacc7a57 100644 --- a/tests/smoke/orgs_test.go +++ b/tests/smoke/orgs_test.go @@ -21,6 +21,21 @@ var _ = Describe("Orgs", func() { )).To(Exit(0)) }) + Describe("cf rename-org", func() { + It("renames the org", func() { + session := helpers.Cf("rename-org", orgName, "renamed-org") + Expect(session).To(Exit(0)) + + session = helpers.Cf("orgs") + Expect(session).To(Exit(0)) + + lines := it.MustCollect(it.LinesString(session.Out)) + Expect(lines).To(ContainElement( + matchSubstrings("renamed-org"), + )) + }) + }) + Describe("cf org", func() { It("returns successful", func() { session := helpers.Cf("org", orgName)