From 8a9c601dfa5b1a926a4cb3c5e9af92702f7cfc1a Mon Sep 17 00:00:00 2001 From: Juunjus Date: Mon, 13 Jul 2026 20:35:39 +0200 Subject: [PATCH 1/2] feat(controller): added nameOverride to buckets (#111) --- api/v1alpha1/bucket_types.go | 16 +- .../crds/garage.getclustered.net_buckets.yaml | 21 ++- .../garage.getclustered.net_buckets.yaml | 21 ++- internal/controller/bucket_controller.go | 47 ++++- internal/controller/bucket_controller_test.go | 171 +++++++++++++++++- 5 files changed, 265 insertions(+), 11 deletions(-) diff --git a/api/v1alpha1/bucket_types.go b/api/v1alpha1/bucket_types.go index 8b7832a..19b23d8 100644 --- a/api/v1alpha1/bucket_types.go +++ b/api/v1alpha1/bucket_types.go @@ -41,7 +41,7 @@ type ExistingBucketSpec struct { // BucketSpec defines the desired state of Bucket // -// +kubebuilder:validation:XValidation:rule="(has(self.name) && size(self.name) > 0) != has(self.existingBucket)",message="exactly one of spec.name or spec.existingBucket must be set" +// +kubebuilder:validation:ExactlyOneOf=name;nameOverride;existingBucket // +kubebuilder:validation:XValidation:rule="!has(self.existingBucket) || !has(oldSelf.existingBucket) || self.existingBucket.name == oldSelf.existingBucket.name",message="spec.existingBucket.name is immutable" type BucketSpec struct { // Name is the desired bucket name. @@ -54,8 +54,20 @@ type BucketSpec struct { // +optional Name string `json:"name,omitempty"` + // NameOverride is the exact global alias assigned to a newly created Garage bucket. + // Mutually exclusive with Name and ExistingBucket. + // + // The controller does not adopt an existing bucket with this alias. Once the bucket ID + // is recorded in status, the controller will not recreate a missing bucket. + // + // +kubebuilder:validation:MinLength=3 + // +kubebuilder:validation:MaxLength=63 + // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="spec.nameOverride is immutable" + // +optional + NameOverride string `json:"nameOverride,omitempty"` + // ExistingBucket identifies a pre-existing Garage bucket to import or reclaim. - // Mutually exclusive with Name. + // Mutually exclusive with Name and NameOverride. // +optional ExistingBucket *ExistingBucketSpec `json:"existingBucket,omitempty"` diff --git a/chart/crds/garage.getclustered.net_buckets.yaml b/chart/crds/garage.getclustered.net_buckets.yaml index 0457e37..2415260 100644 --- a/chart/crds/garage.getclustered.net_buckets.yaml +++ b/chart/crds/garage.getclustered.net_buckets.yaml @@ -54,7 +54,7 @@ spec: existingBucket: description: |- ExistingBucket identifies a pre-existing Garage bucket to import or reclaim. - Mutually exclusive with Name. + Mutually exclusive with Name and NameOverride. properties: name: description: Name is the exact bucket name as it exists in Garage. @@ -99,13 +99,28 @@ spec: x-kubernetes-validations: - message: spec.name is immutable rule: self == oldSelf + nameOverride: + description: |- + NameOverride is the exact global alias assigned to a newly created Garage bucket. + Mutually exclusive with Name and ExistingBucket. + + The controller does not adopt an existing bucket with this alias. Once the bucket ID + is recorded in status, the controller will not recreate a missing bucket. + maxLength: 63 + minLength: 3 + type: string + x-kubernetes-validations: + - message: spec.nameOverride is immutable + rule: self == oldSelf type: object x-kubernetes-validations: - - message: exactly one of spec.name or spec.existingBucket must be set - rule: (has(self.name) && size(self.name) > 0) != has(self.existingBucket) - message: spec.existingBucket.name is immutable rule: '!has(self.existingBucket) || !has(oldSelf.existingBucket) || self.existingBucket.name == oldSelf.existingBucket.name' + - message: exactly one of the fields in [name nameOverride existingBucket] + must be set + rule: '[has(self.name),has(self.nameOverride),has(self.existingBucket)].filter(x,x==true).size() + == 1' status: description: status defines the observed state of Bucket properties: diff --git a/config/crd/bases/garage.getclustered.net_buckets.yaml b/config/crd/bases/garage.getclustered.net_buckets.yaml index 0457e37..2415260 100644 --- a/config/crd/bases/garage.getclustered.net_buckets.yaml +++ b/config/crd/bases/garage.getclustered.net_buckets.yaml @@ -54,7 +54,7 @@ spec: existingBucket: description: |- ExistingBucket identifies a pre-existing Garage bucket to import or reclaim. - Mutually exclusive with Name. + Mutually exclusive with Name and NameOverride. properties: name: description: Name is the exact bucket name as it exists in Garage. @@ -99,13 +99,28 @@ spec: x-kubernetes-validations: - message: spec.name is immutable rule: self == oldSelf + nameOverride: + description: |- + NameOverride is the exact global alias assigned to a newly created Garage bucket. + Mutually exclusive with Name and ExistingBucket. + + The controller does not adopt an existing bucket with this alias. Once the bucket ID + is recorded in status, the controller will not recreate a missing bucket. + maxLength: 63 + minLength: 3 + type: string + x-kubernetes-validations: + - message: spec.nameOverride is immutable + rule: self == oldSelf type: object x-kubernetes-validations: - - message: exactly one of spec.name or spec.existingBucket must be set - rule: (has(self.name) && size(self.name) > 0) != has(self.existingBucket) - message: spec.existingBucket.name is immutable rule: '!has(self.existingBucket) || !has(oldSelf.existingBucket) || self.existingBucket.name == oldSelf.existingBucket.name' + - message: exactly one of the fields in [name nameOverride existingBucket] + must be set + rule: '[has(self.name),has(self.nameOverride),has(self.existingBucket)].filter(x,x==true).size() + == 1' status: description: status defines the observed state of Bucket properties: diff --git a/internal/controller/bucket_controller.go b/internal/controller/bucket_controller.go index bde0a4e..70e93b2 100644 --- a/internal/controller/bucket_controller.go +++ b/internal/controller/bucket_controller.go @@ -152,7 +152,11 @@ func (r *BucketReconciler) reconcileBucket(ctx context.Context, bucket *garagev1 }, nil } } else { - s3Bucket, alias, err = r.resolveNewBucket(ctx, bucket) + if bucket.Spec.NameOverride != "" { + s3Bucket, alias, err = r.resolveNewNameOverrideBucket(ctx, bucket) + } else { + s3Bucket, alias, err = r.resolveNewBucket(ctx, bucket) + } if err != nil { return ctrl.Result{}, err } @@ -315,6 +319,47 @@ func (r *BucketReconciler) resolveNewBucket(ctx context.Context, bucket *garagev return s3Bucket, alias, nil } +func (r *BucketReconciler) resolveNewNameOverrideBucket(ctx context.Context, bucket *garagev1alpha1.Bucket) (s3.Bucket, string, error) { + alias := bucket.Spec.NameOverride + s3Bucket, err := r.bucket.Get(ctx, alias) + if err != nil { + if !errors.Is(err, s3.ErrResourceNotFound) { + markBucketNotReady(bucket, "UnknownState", "S3 API error: %v", err) + return s3.Bucket{}, "", fmt.Errorf("retrieving existing bucket: %w", err) + } + + if bucket.Status.BucketID != "" { + markBucketNotReady(bucket, "BucketMissing", "Bucket %q is missing: it has been deleted and will not be recreated", alias) + r.recorder.Eventf(bucket, corev1.EventTypeWarning, ReasonBucketMissing, "Bucket %q is missing: it has been deleted and will not be recreated", alias) + return s3.Bucket{}, "", fmt.Errorf("bucket %q is missing: it has been deleted and will not be recreated", alias) + } + + s3Bucket, err = r.bucket.Create(ctx, alias) + if err != nil { + markBucketNotReady(bucket, "CreateFailed", "failed to create bucket '%s': %v", alias, err) + r.recorder.Eventf(bucket, corev1.EventTypeWarning, ReasonBucketCreateFailed, "failed to create Garage bucket %q: %v", alias, err) + return s3.Bucket{}, "", fmt.Errorf("create new bucket: %w", err) + } + + r.recorder.Eventf(bucket, corev1.EventTypeNormal, ReasonBucketCreated, "Created Garage bucket %q", alias) + return s3Bucket, alias, nil + } + + if bucket.Status.BucketID == "" { + markBucketNotReady(bucket, "CreateFailed", "Failed to create bucket %q: bucket already exists. Use spec.existingBucket to adopt it", alias) + r.recorder.Eventf(bucket, corev1.EventTypeWarning, ReasonBucketCreateFailed, "Failed to create bucket %q: bucket already exists. Use spec.existingBucket to adopt it", alias) + return s3.Bucket{}, "", fmt.Errorf("failed to create bucket %q: bucket already exists. Use spec.existingBucket to adopt it", alias) + } + + if bucket.Status.BucketID != s3Bucket.ID { + markBucketNotReady(bucket, "BucketMissing", "Bucket %q is missing: its bucket ID changed and it will not be adopted", alias) + r.recorder.Eventf(bucket, corev1.EventTypeWarning, ReasonBucketMissing, "Bucket %q is missing: its bucket ID changed and it will not be adopted", alias) + return s3.Bucket{}, "", fmt.Errorf("bucket %q is missing: its bucket ID changed and it will not be adopted", alias) + } + + return s3Bucket, alias, nil +} + func (r *BucketReconciler) resolveExistingBucket(ctx context.Context, bucket *garagev1alpha1.Bucket) (s3.Bucket, string, error) { spec := bucket.Spec.ExistingBucket diff --git a/internal/controller/bucket_controller_test.go b/internal/controller/bucket_controller_test.go index 4f19fbe..46ff5b6 100644 --- a/internal/controller/bucket_controller_test.go +++ b/internal/controller/bucket_controller_test.go @@ -102,6 +102,49 @@ var _ = Describe("Bucket Controller", func() { }).Should(Succeed()) }) + It("creates an external S3 bucket matching spec.nameOverride", func() { + By("creating a Bucket custom resource") + bucket := newBucket(namespace) + bucket.Spec.Name = "" + bucket.Spec.NameOverride = fixture.RandAlpha(8) + Expect(k8sClient.Create(ctx, &bucket)).To(Succeed()) + + By("reconciling") + s3Fake := newS3APIFake() + s3Endpoint := "https://foo.bar:3456/baz" + controllerReconciler := NewBucketReconciler(k8sClient, k8sClient.Scheme(), s3Fake, s3Endpoint, nil, record.NewFakeRecorder(10)) + + _, err := controllerReconciler.Reconcile(ctx, + reconcile.Request{NamespacedName: namespacedName(bucket.ObjectMeta)}) + Expect(err).ShouldNot(HaveOccurred()) + + By("waiting for external bucket to be provisioned") + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(ctx, namespacedName(bucket.ObjectMeta), &bucket)).To(Succeed()) + g.Expect(checkCondition(bucket.Status.Conditions, Ready, metav1.ConditionTrue)).To(Succeed()) + bucketCond := meta.FindStatusCondition(bucket.Status.Conditions, Ready) + g.Expect(bucketCond.ObservedGeneration).To(Equal(bucket.Generation)) + }).Should(Succeed()) + + By("retrieving bucket with name override") + expectedName := bucket.Spec.NameOverride + created, err := s3Fake.Get(ctx, expectedName) + Expect(err).ToNot(HaveOccurred(), "bucket should exist: %s", expectedName) + Expect(created.GlobalAliases).To(ContainElement(expectedName)) + + By("creating a ConfigMap with bucket details") + expectedCMName := bucket.Name + Eventually(func(g Gomega) { + var configmap corev1.ConfigMap + g.Expect(k8sClient.Get(ctx, + types.NamespacedName{Namespace: namespace, Name: expectedCMName}, + &configmap)).To(Succeed()) + + g.Expect(configmap.Data[configMapKeyBucketName]).To(Equal(bucket.Spec.NameOverride)) + g.Expect(configmap.Data[configMapKeyEndpoint]).To(Equal(s3Endpoint)) + }).Should(Succeed()) + }) + It("should create ConfigMap with connection details", func() { By("creating the bucket") resource := newBucket(namespace) @@ -259,6 +302,52 @@ var _ = Describe("Bucket Controller", func() { Consistently(rec.Events).ShouldNot(Receive()) }) + It("allows a bound nameOverride bucket", func() { + By("creating a Bucket resource") + bucket := newBucket(namespace) + bucket.Spec.Name = "" + bucket.Spec.NameOverride = fixture.RandAlpha(63) + Expect(k8sClient.Create(ctx, &bucket)).To(Succeed()) + bucket.Status.BucketID = "existing-id" + Expect(k8sClient.Status().Update(ctx, &bucket)).To(Succeed()) + + By("pre-seeding existing bucket in the Garage instance") + rec := record.NewFakeRecorder(10) + s3Fake := newS3APIFake() + alias := bucket.Spec.NameOverride + s3Fake.buckets["existing-id"] = s3.Bucket{ID: "existing-id", GlobalAliases: []string{alias}} + sut := NewBucketReconciler(k8sClient, k8sClient.Scheme(), s3Fake, "https://s3.test:9001", nil, rec) + + By("reconciling") + _, err := sut.Reconcile(ctx, reconcile.Request{NamespacedName: namespacedName(bucket.ObjectMeta)}) + Expect(err).ToNot(HaveOccurred()) + + By("asserting no events were emitted") + Consistently(rec.Events).ShouldNot(Receive()) + }) + + It("rejects an unbound nameOverride bucket that already exists", func() { + By("creating a Bucket resource") + bucket := newBucket(namespace) + bucket.Spec.Name = "" + bucket.Spec.NameOverride = fixture.RandAlpha(8) + Expect(k8sClient.Create(ctx, &bucket)).To(Succeed()) + + By("pre-seeding existing bucket in the Garage instance") + rec := record.NewFakeRecorder(10) + s3Fake := newS3APIFake() + alias := bucket.Spec.NameOverride + s3Fake.buckets["existing-id"] = s3.Bucket{ID: "existing-id", GlobalAliases: []string{alias}} + sut := NewBucketReconciler(k8sClient, k8sClient.Scheme(), s3Fake, "https://s3.test:9001", nil, rec) + + By("reconciling") + _, err := sut.Reconcile(ctx, reconcile.Request{NamespacedName: namespacedName(bucket.ObjectMeta)}) + By("asserting failure err was passed and event was emitted") + Expect(err).To(HaveOccurred()) + Expect(err).Should(MatchError(ContainSubstring("failed"))) + Eventually(rec.Events).Should(Receive(ContainSubstring("Warning BucketCreateFailed"))) + }) + It("should emit BucketCreated exactly once across several reconciles", func() { By("creating a Bucket") bucket := newBucket(namespace) @@ -301,6 +390,52 @@ var _ = Describe("Bucket Controller", func() { Eventually(rec.Events).Should(Receive(ContainSubstring("Warning BucketCreateFailed"))) }) + It("reports a nameOverride bucket with a changed ID as missing", func() { + By("creating a Bucket resource") + bucket := newBucket(namespace) + bucket.Spec.Name = "" + bucket.Spec.NameOverride = fixture.RandAlpha(63) + Expect(k8sClient.Create(ctx, &bucket)).To(Succeed()) + bucket.Status.BucketID = "old-id" + Expect(k8sClient.Status().Update(ctx, &bucket)).To(Succeed()) + + By("pre-seeding existing bucket in the Garage instance") + rec := record.NewFakeRecorder(10) + s3Fake := newS3APIFake() + alias := bucket.Spec.NameOverride + s3Fake.buckets["existing-id"] = s3.Bucket{ID: "existing-id", GlobalAliases: []string{alias}} + sut := NewBucketReconciler(k8sClient, k8sClient.Scheme(), s3Fake, "https://s3.test:9001", nil, rec) + + By("reconciling") + _, err := sut.Reconcile(ctx, reconcile.Request{NamespacedName: namespacedName(bucket.ObjectMeta)}) + By("asserting failure err was passed and event was emitted") + Expect(err).To(HaveOccurred()) + Expect(err).Should(MatchError(ContainSubstring("missing"))) + Eventually(rec.Events).Should(Receive(ContainSubstring("missing"))) + }) + + It("reports a missing bound nameOverride bucket", func() { + By("creating a Bucket resource") + bucket := newBucket(namespace) + bucket.Spec.Name = "" + bucket.Spec.NameOverride = fixture.RandAlpha(63) + Expect(k8sClient.Create(ctx, &bucket)).To(Succeed()) + bucket.Status.BucketID = "old-id" + Expect(k8sClient.Status().Update(ctx, &bucket)).To(Succeed()) + + By("creating controller") + rec := record.NewFakeRecorder(10) + s3Fake := newS3APIFake() + sut := NewBucketReconciler(k8sClient, k8sClient.Scheme(), s3Fake, "https://s3.test:9001", nil, rec) + + By("reconciling") + _, err := sut.Reconcile(ctx, reconcile.Request{NamespacedName: namespacedName(bucket.ObjectMeta)}) + By("asserting failure err was passed and event was emitted") + Expect(err).To(HaveOccurred()) + Expect(err).Should(MatchError(ContainSubstring("missing"))) + Eventually(rec.Events).Should(Receive(ContainSubstring("missing"))) + }) + It("recovers from conflict once configMapName is set", func() { conflictingName := "foobar" originalData := map[string]string{"foo": "bar"} @@ -500,7 +635,7 @@ var _ = Describe("Bucket Controller", func() { Entry("starts with hyphen", "-invalid", false), ) - DescribeTable("validates existingBucket field combinations", + DescribeTable("validates bucket source field combinations", func(spec garagev1alpha1.BucketSpec, isValid bool) { resource := garagev1alpha1.Bucket{ ObjectMeta: metav1.ObjectMeta{ @@ -531,7 +666,11 @@ var _ = Describe("Bucket Controller", func() { }}, true, ), - Entry("neither name nor existingBucket", + Entry("nameOverride only", + garagev1alpha1.BucketSpec{NameOverride: fixture.RandAlpha(8)}, + true, + ), + Entry("no bucket source", garagev1alpha1.BucketSpec{}, false, ), @@ -544,6 +683,34 @@ var _ = Describe("Bucket Controller", func() { }, }, false), + Entry("both name and nameOverride", + garagev1alpha1.BucketSpec{ + Name: fixture.RandAlpha(8), + NameOverride: fixture.RandAlpha(8), + }, + false, + ), + Entry("both nameOverride and existingBucket", + garagev1alpha1.BucketSpec{ + NameOverride: fixture.RandAlpha(8), + ExistingBucket: &garagev1alpha1.ExistingBucketSpec{ + Name: fixture.RandAlpha(8), + OwnerKeySecret: fixture.RandAlpha(6), + }, + }, + false, + ), + Entry("all bucket sources", + garagev1alpha1.BucketSpec{ + Name: fixture.RandAlpha(8), + NameOverride: fixture.RandAlpha(8), + ExistingBucket: &garagev1alpha1.ExistingBucketSpec{ + Name: fixture.RandAlpha(8), + OwnerKeySecret: fixture.RandAlpha(6), + }, + }, + false, + ), Entry("existingBucket without ownerKeySecret", garagev1alpha1.BucketSpec{ ExistingBucket: &garagev1alpha1.ExistingBucketSpec{ From efa9432c1f606db008fcba25c1a43ad38fb309c0 Mon Sep 17 00:00:00 2001 From: Juunjus Date: Tue, 14 Jul 2026 08:08:27 +0200 Subject: [PATCH 2/2] revert(chart): remove generated CRD changes --- .../crds/garage.getclustered.net_buckets.yaml | 21 +++---------------- 1 file changed, 3 insertions(+), 18 deletions(-) diff --git a/chart/crds/garage.getclustered.net_buckets.yaml b/chart/crds/garage.getclustered.net_buckets.yaml index 2415260..0457e37 100644 --- a/chart/crds/garage.getclustered.net_buckets.yaml +++ b/chart/crds/garage.getclustered.net_buckets.yaml @@ -54,7 +54,7 @@ spec: existingBucket: description: |- ExistingBucket identifies a pre-existing Garage bucket to import or reclaim. - Mutually exclusive with Name and NameOverride. + Mutually exclusive with Name. properties: name: description: Name is the exact bucket name as it exists in Garage. @@ -99,28 +99,13 @@ spec: x-kubernetes-validations: - message: spec.name is immutable rule: self == oldSelf - nameOverride: - description: |- - NameOverride is the exact global alias assigned to a newly created Garage bucket. - Mutually exclusive with Name and ExistingBucket. - - The controller does not adopt an existing bucket with this alias. Once the bucket ID - is recorded in status, the controller will not recreate a missing bucket. - maxLength: 63 - minLength: 3 - type: string - x-kubernetes-validations: - - message: spec.nameOverride is immutable - rule: self == oldSelf type: object x-kubernetes-validations: + - message: exactly one of spec.name or spec.existingBucket must be set + rule: (has(self.name) && size(self.name) > 0) != has(self.existingBucket) - message: spec.existingBucket.name is immutable rule: '!has(self.existingBucket) || !has(oldSelf.existingBucket) || self.existingBucket.name == oldSelf.existingBucket.name' - - message: exactly one of the fields in [name nameOverride existingBucket] - must be set - rule: '[has(self.name),has(self.nameOverride),has(self.existingBucket)].filter(x,x==true).size() - == 1' status: description: status defines the observed state of Bucket properties: