Merge pull request 'Add immutable placement-target selection to Bucket' (#7) from benvin/placement-targets into main

Reviewed-on: #7

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
+`<zonegroup>:<placementTarget>`; with `spec.zonegroup` empty (the default) that
+is `:<placementTarget>`, 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

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`

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

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

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

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 ("<zonegroup>:<placement>"), 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 ":<placement>", 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 != "" {

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())
+	}
+}

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

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)
+			}
+		})
+	}
+}