feat: role managed RGW placement - #814
Conversation
Document the architectural invariant that all on-disk config files (ceph.conf, keyrings, radosgw.conf, ganesha.conf, etc.) are written solely by microcephd, with dqlite as the source of truth. A config file is a projection of cluster state in the database, not an input to it: - Persist every setting that ends up in a config file in the database, then render the file from the database. - Config files must be reproducible from dqlite after restart/refresh/rejoin. - Never treat an on-disk config file as authoritative state to read back. This frames the CE142 RGW frontend work, which persists RGW frontend settings (port/SSL) in the rgw_frontends table and renders radosgw.conf from it. Assisted-by: pi:z-ai/glm-5.2 Signed-off-by: Peter Sabaini <peter.sabaini@canonical.com>
Replace the bare-bool rgw placement field with an object carrying the
frontend config so placement and RGW frontend config are applied atomically
(CE142 Option B). The charm never shipped a bool rgw flag, so the wire-type
break needs no legacy-bool shim; clients gate on the placement-rgw capability.
MemberPlacement.Rgw changes from *bool to *RgwPlacement{Enabled, Port,
SSLPort, SSLCertificate, SSLPrivateKey}. A bare-bool body fails to unmarshal
into the struct (rejected at the API layer as a client error).
Add RgwObservedFrontend{Port, SSLPort, SSL} for the GET /placement observed
state (ports + a TLS flag only, never cert/key bytes).
Advertise the placement-rgw capability marker in CapabilitiesSupported so a
charm can gate entry into role-managed RGW placement.
Assisted-by: pi:z-ai/glm-5.2
Signed-off-by: Peter Sabaini <peter.sabaini@canonical.com>
Add a dedicated rgw_frontends table modelling the observed RGW frontend per member (one row per member, FK to core_cluster_members with ON DELETE CASCADE), rather than columns on the shared services table. This avoids polluting the shared Service struct/mapper and mirrors the hand-written cluster_lifecycle/placement_policy pattern. schemaUpdate10 creates the table. Hand-written accessors in rgw_frontend.go: - UpsertRGWFrontend: INSERT OR REPLACE with a name->id subquery (matches the client_config pattern) so enable is idempotent and a re-render overwrites. - GetRGWFrontends: JOIN core_cluster_members, returns rows with member names. - DeleteRGWFrontendByMember: used by the member disable path. The table is the observed-state authority read in the same GET /placement transaction, so a down member does not block observed-frontend reporting. Assisted-by: pi:z-ai/glm-5.2 Signed-off-by: Peter Sabaini <peter.sabaini@canonical.com>
…ement GET Add the security and observability helpers for the RGW role-placement GET path (CE142 Option B): - PolicyForStorage: returns a copy of the policy with RGW SSL certificate and private key stripped, so key material is never persisted in dqlite. Called at PUT time by the API store path. - redactStoredPolicy: defense-in-depth that blanks cert/key on the declared policy returned via GET /placement, in case a future code path stores raw. - populateRGWFrontends: sets each observed member's RgwFrontend from the rgw_frontends table (ports + TLS flag only, never cert/key bytes), read in the same GET /placement transaction so a down member does not block it. - ErrRgwFrontendInvalid: client-side sentinel for malformed RGW frontend config (e.g. bad base64 TLS), mapped to HTTP 400 by the API layer. These are pure helpers over types + the rgw_frontends table; the engine and member-side enable path land in later commits. Assisted-by: pi:z-ai/glm-5.2 Signed-off-by: Peter Sabaini <peter.sabaini@canonical.com>
…end record Reconcile the member-side RGW enable/disable so it renders radosgw.conf from the database, compares in place, and restarts only on a real change: - applyRGWFrontend: renders radosgw.conf to a buffer and compares ONLY the 'rgw frontends' line (plus the SSL files) against the on-disk state, not the whole file. The 'mon host'/'run dir' lines are owned by UpdateConfig/ migrateStaleRunDir and are rewritten in place there; comparing the whole file would restart RGW on every reconcile in multi-mon/IPv6 clusters. Monitors are normalized (formatIPv6 + sort) so the rendered mon host is deterministic and byte-stable with the periodic writer. - effectiveRGWPorts: centralizes the default-port-80 rule and forces ssl_port=0 for a plaintext frontend so the observed frontend never reports a stray SSL port (e.g. the enable-rgw CLI's --ssl-port default). - RenderConfig: render-to-buffer helper on Config (configwriter) so applyRGWFrontend can compare without touching disk; WriteConfig keeps its atomic-write contract. - EnableRGW delegates to applyRGWFrontend; DisableRGW is idempotent (os.IsNotExist-tolerant removals); symlinkRGWKeyring is idempotent. - DbUpdate (services_placement_rgw): idempotent (ignores 409 conflict) and upserts the observed frontend into the rgw_frontends table. - removeServiceDatabase deletes the rgw_frontends row on service removal. - Injectable vars (startRGWFunc/restartRGWFunc/createRGWKeyringFunc) per AGENTS.md. Assisted-by: pi:z-ai/glm-5.2 Signed-off-by: Peter Sabaini <peter.sabaini@canonical.com>
Add the RGW reconcile pass to ApplyPlacement (CE142 Option B) and wire its production dispatch: - applyRgwPlacement: runs after the control pass, add-before-remove, sorted for determinism. No keep-one invariant (scale-to-zero allowed). Only removes RGW where it is observed. Short-circuits when no member has a non-nil rgw so control-only applies never touch RGW (and control-only tests skip the RGW observer stub). - Injectable getObservedRgwFunc/enableRgwServiceFunc/removeRgwServiceFunc with Prod* hooks; prodEnableRgwService/prodRemoveRgwService delegate to them. - prod_wiring.go: prodEnableRgwServiceFunc marshals the RgwServicePlacement payload (dropping Enabled) and dispatches via SendServicePlacementReq; prodRemoveRgwServiceFunc calls DeleteService. Both wired in wireProductionFuncs. Tests: a recorder asserts add-before-remove ordering, the dispatched payload, omitted-member handling, scale-to-zero, migration, unknown-member rejection, and control-before-rgw ordering. Assisted-by: pi:z-ai/glm-5.2 Signed-off-by: Peter Sabaini <peter.sabaini@canonical.com>
Wire the CE142 Option B security posture into the placement API handlers: - PUT /placement stores the policy via ceph.PolicyForStorage (strips SSL cert/key before persistence) on both the success path and the keep-one refusal path, so secrets are never persisted even on rejection. - GET /placement unmarshals the stored policy and runs redactStoredPolicy (defense-in-depth) and populates observed rgw_frontends from the rgw_frontends table in the same transaction. - isClientSidePlacementError maps ceph.ErrRgwFrontendInvalid (bad base64 TLS) to HTTP 400 instead of the SmartError 500 fallback. Handler tests assert strip-on-store, object decode, bare-bool 400, and malformed-TLS 400. Assisted-by: pi:z-ai/glm-5.2 Signed-off-by: Peter Sabaini <peter.sabaini@canonical.com>
Internal design note for the shipped CE142 RGW role-placement feature: the Option B object payload (enabled/port/ssl_port/ssl_certificate/ssl_private_key), the rgw_frontends observed-state table, the secret-strip/redact posture, the add-before-remove reconcile pass with no keep-one, and the implementation status superseding earlier out-of-scope TLS / bool-rgw notes. Assisted-by: pi:z-ai/glm-5.2 Signed-off-by: Peter Sabaini <peter.sabaini@canonical.com>
End-to-end Robot coverage for the Option B RGW role-placement feature plus
pure Python parsers for the integration suite:
- rgw-placement-tests: single outer VM + 3 loop OSDs driving the placement API:
placement-rgw capability advertised; PUT with the rgw object enables RGW
atomically (bare-bool rejected 400); GET /placement reports the observed
rgw_frontend {port,ssl} with no SSL material in the stored policy; PUT with
rgw:{enabled:false} scales RGW to zero (snap service inactive, observed
frontend dropped).
- placement_status parsers: member_rgw_frontend (observed frontend for a
member) and placement_leaks_rgw_secrets (detects any ssl_certificate /
ssl_private_key in the stored policy), with pytest coverage so the
integration suite asserts observed reporting and the no-leak posture.
Assisted-by: pi:z-ai/glm-5.2
Signed-off-by: Peter Sabaini <peter.sabaini@canonical.com>
also update / cleanup source and internal docs Signed-off-by: Peter Sabaini <peter.sabaini@canonical.com> Assisted-by: pi:anthropic/opus-4.8
| // Persist the policy only after successful application. | ||
| err = ceph.StorePlacementPolicyFunc(ctx, interfaces.CephState{State: s}, policy) | ||
| // Persist the policy only after successful application. Strip RGW SSL key | ||
| // material before storage (CE142 Option B secrets posture): the cert/key |
There was a problem hiding this comment.
Nit: I think we could just remove the mention of option B. Correct me if I'm wrong but I don't think we ever actually mentioned what option b is. It's possible to tell through deduction what that decision was, but it might be a little confusing if someone tries to figure out what this means.
This appears in various places across the PR
| continue // omitted: untouched | ||
| } | ||
| if mp.Rgw.Enabled { | ||
| desiredEnable = append(desiredEnable, memberName) |
There was a problem hiding this comment.
I believe we will get a failure here because we're not checking if it has already been enabled via the observedState. This isn't idempotent so we will get an error at
genericHospitalityCheck("rgw")
Which errors when it's already active.
I think the same problem occurs for changing the port. Since we go through this Hospitality check won't it always fail when you try to change the port?
I believe we need to allow it to not fail if it is already enabled but support modification of ports and other configuration that needs to be able to change, basically we need to allow it to get to the new code that you've written that will allow modification. It's my understanding that it might not be up to get there right now..
johnramsden
left a comment
johnramsden
left a comment
There was a problem hiding this comment.
Right now the robot tests don't actually run, we need to add the new suite to .github/workflows/tests.yml
johnramsden
left a comment
johnramsden
left a comment
There was a problem hiding this comment.
AI review
The shape of this is good: add-before-remove with sorted iteration, the frontend-line diff that stops mon-host churn from restarting RGW, strip-at-store plus redact-at-get for the key material, and observed state read from dqlite in the same transaction as the rest of GetPlacementStatus. go build, go vet and go test ./ceph/... ./api/... ./database/... all pass at be69165. The gaps are mostly at the edges the design didn't close: a wire-shape break with no upgrade path, several TLS/port misconfigurations that are accepted silently, and a reconcile pass that doesn't isolate per-member failure.
Findings are inline. Two more could not be anchored inline because they land in microceph/ceph/services_placement.go, which this PR does not touch:
RGW private key and certificate are logged in cleartext - microceph/ceph/services_placement.go:44
logger.Debugf("Enabling %s service, payload: %v", payload.Name, payload.Payload) prints the marshalled RgwServicePlacement, which includes SSLPrivateKey. The line is pre-existing, but this PR is what puts key material on an automated, repeating path through it - every enable, port change and cert rotation driven by the reconcile pass, rather than an occasional manual CLI call. It sits directly against the "never persisted, write-only" posture the PR documents. Redacting the payload for services that carry secrets seems in scope here.
Pre-existing, but now on the rotation path - writeSSLFiles renames cert and key as two independent operations, so a failure between them leaves a mismatched pair on disk that the next restart picks up. Cert rotation via placement exercises this far more often than the CLI did.
Generated by an AI assistant (Claude Opus 5) using a multi-agent review pass; every finding was re-checked by hand against the code at be69165 before posting. It deliberately excludes the three points already raised in review (the "Option B" naming, the suite not being wired into tests.yml, and the genericHospitalityCheck non-idempotency). Please verify before acting.
| sslChanged = true | ||
| } | ||
|
|
||
| if !frontendChanged && !sslChanged { |
There was a problem hiding this comment.
Blocking: a stopped-but-correctly-configured RGW can never be recovered through placement.
This is the mirror of the idempotency issue already raised, with the opposite trigger, so it needs its own fix. When the unit is not active - snapd gave up after repeated failures, an operator snap stop, or a prior enable that failed after writing the config - the sequence is:
genericHospitalityCheckpasses (service is not active).ServiceInitreachesapplyRGWFrontend; the on-disk files already match the desired render, so this early return fires and returns(false, nil)without ever callingstartRGWFunc/restartRGWFunc.genericPostPlacementCheckfails onsnapCheckActive, so the PUT returnsrgw service unable to sustain on host.
Re-applying the identical policy reproduces the same no-op forever. Restart decisions are derived purely from file contents; liveness needs to be an input too - treat "service not active" as changed regardless of the file diff.
| // port (e.g. the enable-rgw CLI's --ssl-port default of 443) for a plaintext | ||
| // gateway. Centralizing the rule keeps the on-disk render and the rgw_frontends | ||
| // DB record consistent, so observed state matches what is running. | ||
| func effectiveRGWPorts(port, sslPort int, sslCert, sslKey string) (int, int) { |
There was a problem hiding this comment.
effectiveRGWPorts never defaults ssl_port, so a TLS placement without an explicit port binds to an ephemeral port.
The default-port rule only runs in the plaintext branch; the TLS branch returns port, sslPort untouched. ssl_port is omitempty, so {"enabled":true,"ssl_certificate":"...","ssl_private_key":"..."} yields sslPort == 0. Rendering the actual template from configwriter.go with that data gives:
rgw frontends = beast ssl_port=0 ssl_certificate=/c ssl_private_key=/k
No plain port clause at all, and beast binds TLS to an OS-assigned port. It is then recorded in rgw_frontends as ssl_port: 0, so GET /placement cannot tell the operator where the gateway actually is.
The enable rgw CLI hides this behind a cobra default of 443; the placement path has no equivalent. Default to 443 when TLS is configured and ssl_port is 0, or reject it as ErrRgwFrontendInvalid.
| sslCertificatePath := "" | ||
| sslPrivateKeyPath := "" | ||
|
|
||
| sslConfigured := sslCert != "" && sslKey != "" |
There was a problem hiding this comment.
Half a TLS pair silently downgrades the gateway to plaintext.
sslConfigured := sslCert != "" && sslKey != "" treats "one set" identically to "neither set". A PUT carrying a certificate but no key - a certificates relation that is only half-ready is the obvious way to get there - returns 200, discards the certificate, and brings RGW up on plain port 80 via the default-port rule.
Related: because the fields are plain omitempty strings with no sentinel, "leave TLS as it is" and "go plaintext" are the same wire value. A client that reads GET, edits one field and PUTs it back therefore drops TLS with no signal. Rejecting a half-pair handles the first case and makes the second much harder to hit by accident.
| sslPrivateKeyPath = keyPath | ||
| } | ||
|
|
||
| port, sslPort = effectiveRGWPorts(port, sslPort, sslCert, sslKey) |
There was a problem hiding this comment.
No range or collision validation on the ports before they are rendered.
Nothing here or in the API layer checks that port != sslPort, or that either value is a valid TCP port. {"port":8080,"ssl_port":8080,...} renders
rgw frontends = beast port=8080 ssl_port=8080 ssl_certificate=... ssl_private_key=...
and beast fails its second bind(2); a negative or >65535 port is written to radosgw.conf and into rgw_frontends unchallenged. The ErrRgwFrontendInvalid -> 400 pattern is already established in this PR for bad base64, so this would fit naturally alongside it (validating in RgwServicePlacement.PopulateParams would catch it before any DB write).
| // cascade-deleted when the cluster member is removed, mirroring the services | ||
| // and host_tags tables. At most one row per member is enforced by UNIQUE on | ||
| // member_id. | ||
| func schemaUpdate10(ctx context.Context, tx *sql.Tx) error { |
There was a problem hiding this comment.
No backfill for RGW enabled before this schema update.
schemaUpdate10 creates rgw_frontends empty, and RgwServicePlacement.DbUpdate is the only path that ever writes a row. A member already running RGW from the CLI path therefore reports rgw: true with no rgw_frontend until it happens to be re-enabled, and populateRGWFrontends skips it silently - which on the wire is indistinguishable from "not TLS".
A backfill in this update (ports unknown, but the row could be seeded from the services table) or an explicit "unknown" signal in the observed output would avoid the ambiguity.
| f.SSL = sslInt != 0 | ||
| frontends = append(frontends, f) | ||
| } | ||
| if err := rows.Err(); err != nil { |
There was a problem hiding this comment.
Nit: one-line assign/test, which AGENTS.md forbids - err := rows.Err() then if err != nil. Same pattern at ceph/rgw.go:421 (if err := os.Symlink(...)).
| - Meaning | ||
| * - 400 | ||
| - The request cannot be satisfied: Ceph is not bootstrapped, the policy | ||
| names an unknown member, the RGW frontend configuration is malformed, or |
There was a problem hiding this comment.
The documented 400 for malformed TLS is a 500 in practice.
ErrRgwFrontendInvalid is raised inside applyRGWFrontend, which runs on the target member. It is flattened twice before the handler can test it:
ceph/services_placement.go:112wraps with%v, not%w.client.SendServicePlacementReqthen crosses an HTTP boundary, which reconstructs a generic error regardless.
So errors.Is in isClientSidePlacementError cannot match, and SmartError falls back to 500. The test covering this (api/placement_test.go:740) stubs ApplyPlacementFunc and returns the wrapped sentinel directly, so it passes either way.
Fixing the %v and mapping ErrRgwFrontendInvalid to 400 on the receiving member's handler would make the doc true; a test that goes through the real dispatch would keep it that way.
| # Observed frontend: port 8080, ssl false, no ssl_port (plaintext -> ssl_port=0 -> omitempty). | ||
| Should Contain ${placement} "rgw_frontend":{"port":8080,"ssl":false} msg=observed rgw_frontend not reported as expected: ${placement} | ||
| # Defense in depth: no SSL key material anywhere in the placement body. | ||
| Should Not Contain ${placement} ssl_certificate msg=ssl_certificate leaked into placement body |
There was a problem hiding this comment.
This assertion never exercises the code path it claims to test.
The enable PUT above sends {"enabled":true,"port":8080} - no TLS material at any point in the suite. So Should Not Contain ... ssl_certificate passes on a policy that never contained a secret, and would still pass with PolicyForStorage and redactStoredPolicy deleted outright.
Sending a real (throwaway) cert/key pair in the enable PUT and then asserting absence is what makes this a defence-in-depth check.
Also: placement_status.member_rgw_frontend and placement_status.placement_leaks_rgw_secrets are added in this PR with pytest coverage, but nothing outside those tests calls them - this suite uses raw-JSON substring matching instead, including "rgw_frontend":{"port":8080,"ssl":false}, which is sensitive to Go's struct field ordering. That is the opposite of the "fetch raw, decide in Python" rule the module's own docstring cites.
| # Bare-bool rgw must be rejected (HTTP 400) by the Option B parser. | ||
| ${bad}= MicroCeph API Put placement {"mode":"reconcile","members":{"${hn}":{"rgw":true}}} timeout=60 | ||
| ${bad_code}= Response Status Code ${bad} | ||
| Run Keyword And Continue On Failure Should Not Be Equal As Integers ${bad_code} 200 msg=bare-bool rgw must be rejected, got ${bad} |
There was a problem hiding this comment.
Nit: this asserts only "not 200", so it would pass on a 500 or a 409 just as happily as on the documented 400. Should Be Equal As Integers ${bad_code} 400 is what the docs promise.
Separately, on harness conventions in AGENTS.md: Placement Has No RGW Frontend hand-rolls a FOR/Sleep loop and RGW Snap Service Is Not Active hand-rolls an in-VM shell poll loop, where the rules call for _poll_until in Python for every poll loop.
|
Overall looks pretty good to me. I left a couple comments earlier and then executed an AI review. My only main concern is #814 (comment), along with the tests not currently executing #814 (review). Feel free to just resolve/skip anything the AI came up with |
2 participants
Description
Summary
Adds declarative, role-managed RGW placement to MicroCeph: a charm places RGW on a member by PUT /1.0/placement with an rgw object carrying enabled/port/ssl_port/ssl_certificate/ssl_private_key, and the daemon atomically enables the gateway with that frontend config. No separate enable rgw step, no out-of-band TLS handling. Observed frontend state (ports + a TLS flag) is reported back via GET /1.0/placement, sourced from a new dqlite table so it's available even when the member is down.
The placement rgw field moves from a bare boolean to an object. The charm was never shipped with the bool form, so the wire-type break needs no legacy shim; clients gate on the advertised placement-rgw capability marker.
What changes
Wire contract — MemberPlacement.Rgw changes from *bool to *RgwPlacement{Enabled, Port, SSLPort, SSLCertificate, SSLPrivateKey}. A bare-bool body is rejected as a client error. The placement-rgw capability marker is advertised in CapabilitiesSupported.
Placement engine — a new applyRgwPlacement reconcile pass runs after the control pass: add-before-remove (a migration keeps a gateway serving), sorted for determinism, no keep-one invariant (scale-to-zero is allowed), and only removes RGW where it is observed. Short-circuits when no member has a non-nil rgw so control-only applies never touch RGW. Production dispatch reuses the existing services/rgw PUT/DELETE transport (proxied to the target member).
Member side — applyRGWFrontend renders radosgw.conf from the database and compares only the rgw frontends line (+ SSL files) against the on-disk state, so restarts happen only on a real frontend/TLS change — not on mon host/run dir churn from the periodic in-place writers (the idempotency bug that would have restarted RGW on every reconcile in multi-mon/IPv6 clusters). effectiveRGWPorts centralizes the default-port-80 rule and forces ssl_port=0 for plaintext so observed state never reports a stray SSL port.
DisableRGW is idempotent.
Secrets posture — SSL key material travels over the authenticated API to the member that needs it, then is dropped before the policy is stored (PolicyForStorage strips at PUT time; redactStoredPolicy is defense-in-depth at GET time). Key material is never persisted in dqlite. Malformed base64 TLS maps to HTTP 400.
Observed state — a dedicated rgw_frontends table (one row per member, FK CASCADE to core_cluster_members) records the observed frontend; GET /placement reads it in the same transaction, so a down member doesn't block observed-frontend reporting.
Tests — unit tests for the types, DB accessors, member-side idempotency (incl. a multi-monitor reorder regression and plaintext ssl_port), engine ordering, and the secret-strip/redact helpers; a new rgw-placement-tests Robot suite drives the full feature end-to-end (capability → enable via object → observed frontend + no secret leak → scale-to-zero); pure Python parsers for the integration suite assert observed reporting and the no-leak posture.
Commits, reviewable by area
Breaking change
The placement rgw field type changes (*bool → *RgwPlacement). No shipped charm uses the bool form; gate on placement-rgw before sending the object payload.