From c1b3ba1c342e06a04dae9ce35872b640a11e0190 Mon Sep 17 00:00:00 2001 From: Ben Vincent Date: Wed, 29 Jul 2026 00:16:31 +1000 Subject: [PATCH] Add immutable placement-target selection to Bucket Buckets could not choose which RGW placement target (and thus durability profile) backs them, so all data landed on the cluster default. The estate's radosgw exposes two targets - default-placement (3x replicated) and ec (4+1 erasure-coded) - and archival workloads want ec. - Validate spec.placementTarget: a DNS-ish pattern, 63-char cap, and a CEL self==oldSelf immutability rule (RGW fixes placement at bucket creation and cannot move a bucket between targets); make spec.zonegroup immutable too. - Thread the target into the S3 CreateBucket LocationConstraint via the existing helper; an empty zonegroup yields ":", selecting the local zonegroup so callers need not name the zonegroup api-name. - Read the live placement_rule and zonegroup back from the Admin Ops bucket stats and surface them: status.placementTarget plus a Placement print column. - Guard the controller: if a live bucket's placement differs from spec, set an Error phase with a PlacementImmutable reason instead of deleting/recreating. - Cover locationConstraint construction, placement readback (httptest), and the placementConflict guard with tests; document targets and immutability in the README and add config/samples/06-bucket-ec.yaml. Claude-Session: https://claude.ai/code/session_015ur3i7D2azsMAWTSVABApv --- README.md | 37 ++++++++++ api/v1alpha1/bucket_types.go | 28 +++++++- config/crd/bases/ceph.unkin.net_buckets.yaml | 40 +++++++++-- config/crd/install.yaml | 40 +++++++++-- config/samples/06-bucket-ec.yaml | 18 +++++ internal/ceph/buckets.go | 23 ++++++- internal/ceph/buckets_test.go | 68 +++++++++++++++++++ internal/controller/bucket_controller.go | 24 +++++++ internal/controller/bucket_controller_test.go | 27 ++++++++ 9 files changed, 292 insertions(+), 13 deletions(-) create mode 100644 config/samples/06-bucket-ec.yaml create mode 100644 internal/ceph/buckets_test.go create mode 100644 internal/controller/bucket_controller_test.go diff --git a/README.md b/README.md index e3a2073..04cd2c3 100644 --- a/README.md +++ b/README.md @@ -60,6 +60,43 @@ refine it (see `config/samples/04-access-fine-grained.yaml`): RGW honours S3 bucket policy on **Reef 18.2+ / Squid**; condition-key support is a subset of AWS, so validate exotic conditions against your cluster. +### Placement targets + +`Bucket.spec.placementTarget` selects the RGW **placement target** that backs the +bucket — i.e. which pools, and therefore which durability profile, store its +data. The valid values are cluster configuration, not a fixed set baked into the +operator. On this estate radosgw exposes two: + +- `default-placement` — 3× replicated (the cluster default). +- `ec` — 4+1 erasure-coded (cheaper capacity, for bulk/archival data). + +Leaving `placementTarget` empty keeps the current behaviour: the owning user's +`default_placement` (falling back to the zonegroup default). When set, the +operator threads it into the S3 `CreateBucket` `LocationConstraint` as +`:`; with `spec.zonegroup` empty (the default) that +is `:`, which selects the local/master zonegroup with the given +placement — so you do not need to know the zonegroup's api-name to pick a target. + +```yaml +apiVersion: ceph.unkin.net/v1alpha1 +kind: Bucket +metadata: + name: raw-archive +spec: + ownerRef: logarchiver + placementTarget: ec # 4+1 erasure-coded pool +``` + +Placement is **immutable**: RGW fixes it at bucket creation and cannot move an +existing bucket between targets. The CRD rejects changing `placementTarget` (and +`zonegroup`) on an existing `Bucket`, and if a bucket already lives on a +different target than the spec requests (e.g. an adopted bucket, or a value +sneaked in around the CRD guard) the controller sets an `Error` phase with a +`PlacementImmutable` reason rather than ever deleting and recreating it. The +placement RGW actually stores the bucket on is reported in +`status.placementTarget` (and the `Placement` print column), so drift is visible. +See `config/samples/06-bucket-ec.yaml`. + ### Adopting existing buckets and users The operator can take over buckets/users that already exist in radosgw and hand diff --git a/api/v1alpha1/bucket_types.go b/api/v1alpha1/bucket_types.go index dffcece..f945737 100644 --- a/api/v1alpha1/bucket_types.go +++ b/api/v1alpha1/bucket_types.go @@ -40,11 +40,29 @@ type BucketSpec struct { // with BucketAccess objects. OwnerRef string `json:"ownerRef"` - // Zonegroup optionally pins the bucket to a specific RGW zonegroup. + // Zonegroup optionally pins the bucket to a specific RGW zonegroup by its + // api-name. Empty (the default) uses the cluster's local/master zonegroup, so + // PlacementTarget selection works without naming the zonegroup. Immutable: + // RGW resolves the zonegroup at bucket creation and cannot move it afterwards. + // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="zonegroup is immutable; RGW fixes it at bucket creation" // +optional Zonegroup string `json:"zonegroup,omitempty"` - // PlacementTarget optionally selects a non-default placement target/pool. + // PlacementTarget optionally selects the RGW placement target that backs the + // bucket, choosing which pools (and thus replication/erasure profile) store + // its data. Empty (the default) uses the owning user's default_placement, or + // the zonegroup default. The valid values are cluster configuration, not a + // fixed set; on this estate the two configured targets are + // "default-placement" (3x replicated) and "ec" (4+1 erasure-coded). + // + // Immutable: RGW chooses the placement at bucket creation (from the S3 + // LocationConstraint) and cannot move an existing bucket between placement + // targets. Set it on a fresh Bucket; changing it later is rejected, and if a + // pre-existing bucket is on a different placement the operator reports an + // error instead of recreating it. + // +kubebuilder:validation:MaxLength=63 + // +kubebuilder:validation:Pattern=`^[a-zA-Z0-9]([a-zA-Z0-9._-]*[a-zA-Z0-9])?$` + // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="placementTarget is immutable; RGW cannot move a bucket between placement targets" // +optional PlacementTarget string `json:"placementTarget,omitempty"` @@ -100,6 +118,11 @@ type BucketStatus struct { // Owner is the RGW uid that owns the bucket. // +optional Owner string `json:"owner,omitempty"` + // PlacementTarget is the placement target RGW actually stores the bucket on, + // read back from the live bucket. It makes placement drift (a bucket landing + // on a different target than spec requested) visible. + // +optional + PlacementTarget string `json:"placementTarget,omitempty"` // PolicyPrincipals is the number of extra principals granted via // BucketAccess and reflected in the bucket policy. // +optional @@ -121,6 +144,7 @@ type BucketStatus struct { // +kubebuilder:resource:shortName=bkt // +kubebuilder:printcolumn:name="Bucket",type=string,JSONPath=`.status.bucketName` // +kubebuilder:printcolumn:name="Owner",type=string,JSONPath=`.status.owner` +// +kubebuilder:printcolumn:name="Placement",type=string,JSONPath=`.status.placementTarget` // +kubebuilder:printcolumn:name="Grants",type=integer,JSONPath=`.status.policyPrincipals` // +kubebuilder:printcolumn:name="Adopted",type=boolean,JSONPath=`.status.adopted` // +kubebuilder:printcolumn:name="Phase",type=string,JSONPath=`.status.phase` diff --git a/config/crd/bases/ceph.unkin.net_buckets.yaml b/config/crd/bases/ceph.unkin.net_buckets.yaml index 676c22c..36fbe54 100644 --- a/config/crd/bases/ceph.unkin.net_buckets.yaml +++ b/config/crd/bases/ceph.unkin.net_buckets.yaml @@ -23,6 +23,9 @@ spec: - jsonPath: .status.owner name: Owner type: string + - jsonPath: .status.placementTarget + name: Placement + type: string - jsonPath: .status.policyPrincipals name: Grants type: integer @@ -105,9 +108,26 @@ spec: with BucketAccess objects. type: string placementTarget: - description: PlacementTarget optionally selects a non-default placement - target/pool. + description: |- + PlacementTarget optionally selects the RGW placement target that backs the + bucket, choosing which pools (and thus replication/erasure profile) store + its data. Empty (the default) uses the owning user's default_placement, or + the zonegroup default. The valid values are cluster configuration, not a + fixed set; on this estate the two configured targets are + "default-placement" (3x replicated) and "ec" (4+1 erasure-coded). + + Immutable: RGW chooses the placement at bucket creation (from the S3 + LocationConstraint) and cannot move an existing bucket between placement + targets. Set it on a fresh Bucket; changing it later is rejected, and if a + pre-existing bucket is on a different placement the operator reports an + error instead of recreating it. + maxLength: 63 + pattern: ^[a-zA-Z0-9]([a-zA-Z0-9._-]*[a-zA-Z0-9])?$ type: string + x-kubernetes-validations: + - message: placementTarget is immutable; RGW cannot move a bucket + between placement targets + rule: self == oldSelf purgeOnDelete: description: |- PurgeOnDelete deletes the bucket together with all objects it contains @@ -148,9 +168,15 @@ spec: description: Versioning enables S3 object versioning on the bucket. type: boolean zonegroup: - description: Zonegroup optionally pins the bucket to a specific RGW - zonegroup. + description: |- + Zonegroup optionally pins the bucket to a specific RGW zonegroup by its + api-name. Empty (the default) uses the cluster's local/master zonegroup, so + PlacementTarget selection works without naming the zonegroup. Immutable: + RGW resolves the zonegroup at bucket creation and cannot move it afterwards. type: string + x-kubernetes-validations: + - message: zonegroup is immutable; RGW fixes it at bucket creation + rule: self == oldSelf required: - ownerRef type: object @@ -236,6 +262,12 @@ spec: phase: description: Phase is a coarse lifecycle summary (Pending/Ready/Error). type: string + placementTarget: + description: |- + PlacementTarget is the placement target RGW actually stores the bucket on, + read back from the live bucket. It makes placement drift (a bucket landing + on a different target than spec requested) visible. + type: string policyPrincipals: description: |- PolicyPrincipals is the number of extra principals granted via diff --git a/config/crd/install.yaml b/config/crd/install.yaml index 1d1bef6..7604266 100644 --- a/config/crd/install.yaml +++ b/config/crd/install.yaml @@ -293,6 +293,9 @@ spec: - jsonPath: .status.owner name: Owner type: string + - jsonPath: .status.placementTarget + name: Placement + type: string - jsonPath: .status.policyPrincipals name: Grants type: integer @@ -375,9 +378,26 @@ spec: with BucketAccess objects. type: string placementTarget: - description: PlacementTarget optionally selects a non-default placement - target/pool. + description: |- + PlacementTarget optionally selects the RGW placement target that backs the + bucket, choosing which pools (and thus replication/erasure profile) store + its data. Empty (the default) uses the owning user's default_placement, or + the zonegroup default. The valid values are cluster configuration, not a + fixed set; on this estate the two configured targets are + "default-placement" (3x replicated) and "ec" (4+1 erasure-coded). + + Immutable: RGW chooses the placement at bucket creation (from the S3 + LocationConstraint) and cannot move an existing bucket between placement + targets. Set it on a fresh Bucket; changing it later is rejected, and if a + pre-existing bucket is on a different placement the operator reports an + error instead of recreating it. + maxLength: 63 + pattern: ^[a-zA-Z0-9]([a-zA-Z0-9._-]*[a-zA-Z0-9])?$ type: string + x-kubernetes-validations: + - message: placementTarget is immutable; RGW cannot move a bucket + between placement targets + rule: self == oldSelf purgeOnDelete: description: |- PurgeOnDelete deletes the bucket together with all objects it contains @@ -418,9 +438,15 @@ spec: description: Versioning enables S3 object versioning on the bucket. type: boolean zonegroup: - description: Zonegroup optionally pins the bucket to a specific RGW - zonegroup. + description: |- + Zonegroup optionally pins the bucket to a specific RGW zonegroup by its + api-name. Empty (the default) uses the cluster's local/master zonegroup, so + PlacementTarget selection works without naming the zonegroup. Immutable: + RGW resolves the zonegroup at bucket creation and cannot move it afterwards. type: string + x-kubernetes-validations: + - message: zonegroup is immutable; RGW fixes it at bucket creation + rule: self == oldSelf required: - ownerRef type: object @@ -506,6 +532,12 @@ spec: phase: description: Phase is a coarse lifecycle summary (Pending/Ready/Error). type: string + placementTarget: + description: |- + PlacementTarget is the placement target RGW actually stores the bucket on, + read back from the live bucket. It makes placement drift (a bucket landing + on a different target than spec requested) visible. + type: string policyPrincipals: description: |- PolicyPrincipals is the number of extra principals granted via diff --git a/config/samples/06-bucket-ec.yaml b/config/samples/06-bucket-ec.yaml new file mode 100644 index 0000000..443ba94 --- /dev/null +++ b/config/samples/06-bucket-ec.yaml @@ -0,0 +1,18 @@ +# A bucket placed on the erasure-coded (4+1) placement target instead of the +# default 3x-replicated pool. Good for bulk/archival data where capacity matters +# more than the extra replica. +# +# placementTarget is immutable: RGW chooses the placement at bucket creation and +# cannot move an existing bucket between targets, so it can only be set on a +# fresh Bucket. The operator reports the live placement in status.placementTarget. +apiVersion: ceph.unkin.net/v1alpha1 +kind: Bucket +metadata: + name: raw-archive + namespace: default +spec: + bucketName: raw-archive + ownerRef: app-owner + # Cluster-configured placement target. On this estate: "default-placement" + # (3x replicated) or "ec" (4+1 erasure-coded). + placementTarget: ec diff --git a/internal/ceph/buckets.go b/internal/ceph/buckets.go index b23a141..2eb5a6d 100644 --- a/internal/ceph/buckets.go +++ b/internal/ceph/buckets.go @@ -18,6 +18,11 @@ type BucketInfo struct { Bid string ID string Owner string + // PlacementRule is the placement target RGW stores the bucket on (e.g. + // "default-placement" or "ec"), read from the Admin Ops bucket stats. + PlacementRule string + // Zonegroup is the RGW zonegroup id the bucket belongs to. + Zonegroup string } // Name returns the bucket name regardless of the field radosgw used. @@ -50,7 +55,13 @@ func (c *Client) GetBucket(ctx context.Context, name string) (*BucketInfo, error if err != nil { return nil, err } - return &BucketInfo{Bucket: b.Bucket, ID: b.ID, Owner: b.Owner}, nil + return &BucketInfo{ + Bucket: b.Bucket, + ID: b.ID, + Owner: b.Owner, + PlacementRule: b.PlacementRule, + Zonegroup: b.Zonegroup, + }, nil } // CreateBucket provisions a bucket owned by spec.OwnerUID. The Admin Ops API @@ -211,8 +222,14 @@ func (c *Client) setObjectLockDefault(ctx context.Context, owner func(*s3.Option return err } -// locationConstraint renders the RGW LocationConstraint from a zonegroup and -// placement target (":"), or "" for default placement. +// locationConstraint renders the RGW S3 CreateBucket LocationConstraint from a +// zonegroup api-name and a placement target. RGW's S3 create-bucket handler +// splits the value on the first ":" — the part before is the zonegroup api-name, +// the part after is the placement target id. An empty zonegroup (the common +// case) yields ":", which selects the local/master zonegroup with the +// given placement, so callers need not know the zonegroup's api-name to pick a +// placement target. Both empty yields "" (no constraint: user/zonegroup +// default). Placement empty with a zonegroup set yields just the zonegroup. func locationConstraint(zonegroup, placement string) string { loc := zonegroup if placement != "" { diff --git a/internal/ceph/buckets_test.go b/internal/ceph/buckets_test.go new file mode 100644 index 0000000..dbff4ff --- /dev/null +++ b/internal/ceph/buckets_test.go @@ -0,0 +1,68 @@ +package ceph + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" +) + +func TestLocationConstraint(t *testing.T) { + cases := []struct { + name string + zonegroup string + placement string + want string + }{ + {"both empty -> no constraint", "", "", ""}, + {"placement only -> local zonegroup", "", "ec", ":ec"}, + {"placement only default target", "", "default-placement", ":default-placement"}, + {"zonegroup and placement", "default", "ec", "default:ec"}, + {"zonegroup only", "default", "", "default"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := locationConstraint(tc.zonegroup, tc.placement); got != tc.want { + t.Errorf("locationConstraint(%q,%q)=%q want %q", tc.zonegroup, tc.placement, got, tc.want) + } + }) + } +} + +// TestGetBucketPlacement verifies GetBucket surfaces the placement target and +// zonegroup from the Admin Ops bucket-stats response, so the controller can +// detect placement drift. +func TestGetBucketPlacement(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{ + "bucket": "raw-archive", + "id": "eae688bc-ee35-445d-9188-111b73c8b4a0.12345.1", + "owner": "logarchiver", + "zonegroup": "eae688bc-ee35-445d-9188-111b73c8b4a0", + "placement_rule": "ec" + }`)) + })) + defer srv.Close() + + c, err := NewClient(Config{Endpoint: srv.URL, AccessKey: "a", SecretKey: "s"}) + if err != nil { + t.Fatalf("NewClient: %v", err) + } + info, err := c.GetBucket(context.Background(), "raw-archive") + if err != nil { + t.Fatalf("GetBucket: %v", err) + } + if info.PlacementRule != "ec" { + t.Errorf("PlacementRule=%q want %q", info.PlacementRule, "ec") + } + if info.Zonegroup != "eae688bc-ee35-445d-9188-111b73c8b4a0" { + t.Errorf("Zonegroup=%q unexpected", info.Zonegroup) + } + if info.Owner != "logarchiver" { + t.Errorf("Owner=%q want logarchiver", info.Owner) + } + if info.Name() != "raw-archive" { + t.Errorf("Name()=%q want raw-archive", info.Name()) + } +} diff --git a/internal/controller/bucket_controller.go b/internal/controller/bucket_controller.go index 59faff2..012e03c 100644 --- a/internal/controller/bucket_controller.go +++ b/internal/controller/bucket_controller.go @@ -112,6 +112,18 @@ func (r *BucketReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr } bucketID := info.InstanceID() + // Placement is fixed at creation: RGW cannot move an existing bucket between + // placement targets. If the live bucket sits on a different target than the + // spec asks for (a changed spec, or an adopted bucket that predates the + // request), surface a clear error instead of ever deleting/recreating it. An + // empty PlacementTarget imposes no constraint. + if pc := placementConflict(b.Spec.PlacementTarget, info.PlacementRule); pc { + b.Status.PlacementTarget = info.PlacementRule + return r.fail(ctx, &b, "PlacementImmutable", fmt.Errorf( + "bucket %q is on placement target %q but spec requests %q; RGW cannot move a bucket between placement targets", + bucketName, info.PlacementRule, b.Spec.PlacementTarget)) + } + // Versioning (forced on when object lock is enabled). if b.Spec.Versioning || (b.Spec.ObjectLock != nil && b.Spec.ObjectLock.Enabled) { if err := r.Ceph.SetBucketVersioning(ctx, bucketName, bucketID, ownerUID, true); err != nil { @@ -167,6 +179,7 @@ func (r *BucketReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr b.Status.BucketName = bucketName b.Status.BucketID = bucketID b.Status.Owner = ownerUID + b.Status.PlacementTarget = info.PlacementRule b.Status.PolicyPrincipals = int32(principals) b.Status.ObservedGeneration = b.Generation setReady(&b.Status.Conditions, b.Generation, true, "Provisioned", "bucket provisioned") @@ -250,6 +263,17 @@ func managePolicy(b *v1alpha1.Bucket) bool { return b.Spec.ManagePolicy == nil || *b.Spec.ManagePolicy } +// placementConflict reports whether a bucket's live placement target violates +// the spec. An empty spec placement imposes no constraint (the bucket may sit on +// whatever default it was created with). Otherwise the live placement must match +// exactly, since RGW cannot move a bucket between placement targets. +func placementConflict(specPlacement, livePlacement string) bool { + if specPlacement == "" { + return false + } + return specPlacement != livePlacement +} + func (r *BucketReconciler) pending(ctx context.Context, b *v1alpha1.Bucket, reason, msg string) (ctrl.Result, error) { b.Status.Phase = "Pending" b.Status.ObservedGeneration = b.Generation diff --git a/internal/controller/bucket_controller_test.go b/internal/controller/bucket_controller_test.go new file mode 100644 index 0000000..522ec44 --- /dev/null +++ b/internal/controller/bucket_controller_test.go @@ -0,0 +1,27 @@ +package controller + +import "testing" + +func TestPlacementConflict(t *testing.T) { + cases := []struct { + name string + specPlacement string + livePlacement string + want bool + }{ + {"unset spec never conflicts", "", "default-placement", false}, + {"unset spec unset live", "", "", false}, + {"matching ec", "ec", "ec", false}, + {"matching default", "default-placement", "default-placement", false}, + {"ec requested but default live", "ec", "default-placement", true}, + {"default requested but ec live", "default-placement", "ec", true}, + {"spec set live empty", "ec", "", true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := placementConflict(tc.specPlacement, tc.livePlacement); got != tc.want { + t.Errorf("placementConflict(%q,%q)=%v want %v", tc.specPlacement, tc.livePlacement, got, tc.want) + } + }) + } +}