[HYPERSHELL-299] feat(db): Add external database provisioning mode for gateways - #248
[HYPERSHELL-299] feat(db): Add external database provisioning mode for gateways#248rh-amarin wants to merge 15 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
REQUEST_CHANGES — The external database provisioning path is well-scoped, redacts credentials, and uses parameterized/quoted DDL, but two Major items need a decision before merge: the external placement code contradicts its own documented region-matching design (and ships an unused DAO method), and the admin connection is opened with an unbounded, context-less Ping that can stall the serialized reconcile loop. Several Minor cleanups (reinvented stdlib, unconditional Secret writes, stale CLAUDE.md guidance) are also called out inline.
Hi — Amber here. This adds external as a first-class DATABASE_PROVIDER across the API server (validation + placement), the control plane (in-process DDL provisioning, credential rotation, cleanup), and the Kind/E2E matrix. Overall the security posture is good: passwords are generated with crypto/rand, never logged, the DSN/connection errors are redacted, identifiers are quoted with pgQuoteIdent, and status is reported through a closed vocabulary. My concerns are correctness/design and doc drift, not secret handling.
Blocker
None.
Critical
None.
Major
-
External placement contradicts its documented design and ships dead code.
provider.gostates external placement selects "the external ManagedDatabase whose region matches the gateway's target cluster region," and this PR addsManagedDatabaseDao.FindByProviderAndRegion(plus a mock impl) to support that. But the wired-up path reusescnpgPlacement, which callsdbLookupAdapter.FindSole— it only succeeds when exactly oneexternalManagedDatabase exists and returns a validation error ("zero or multiple ManagedDatabases exist") otherwise.FindByProviderAndRegionis never called from production code. So multi-region external registration is silently unsupported, and the doc/intent and implementation disagree. Please decide: either wire region-based selection throughFindByProviderAndRegion, or drop the unused method and correct theprovider.gocomment (and the placement error string, which is misleading for the external case). (Confidence: High) -
Admin connection uses
db.Ping()with no context and no connect timeout.openAdminConn(external_db.go:131) callsdb.Ping()rather thandb.PingContext(ctx), and the DSN sets noconnect_timeout. An external host that black-holes TCP will block the per-resource serializedhandleOnegoroutine for the OS TCP timeout regardless of stream/context cancellation, stalling bothProbeExternalServerand gateway provisioning. ThreadctxintoopenAdminConnand usePingContext, and/or addconnect_timeout=<n>to the DSN. (mapConnErrorToStatusalready anticipatesi/o timeout, so honoring the deadline is consistent with intent.) (Confidence: High)
Minor
-
validateExternalConnectionSecretreinventsstringsand has a redundant condition. service.go hand-rollscontainsSlashandhasPrefixand the guardlen(*secret) > 0 && (*secret)[0] == '/' || len(*secret) > 0 && containsSlash(*secret)is subsumed by a singlestrings.Contains(*secret, "/"). Preferstrings.Contains/strings.HasPrefix. (Confidence: High) -
External tenant credentials Secret is updated unconditionally every reconcile. Unlike the deployment path (
copyDeploymentDatabaseCredentialsshort-circuits withreflect.DeepEqual),ReconcileExternalDatabaseResourcesalways issues anUpdatewhen the Secret exists, churningresourceVersionon every pass. Add an equality check before writing. (Confidence: High) -
CLAUDE.md guidance is stale relative to this PR's own refactor. The rewritten sections say
ManagedDatabaseReconciler.handleOne()branches oncnpg/deployment(omits the newexternal) and that adding a provider requires "a reconcile branch in ...ReconcileGatewayswitch" — but this PR replaces that switch with thenewDatabaseReconcilerfactory /DatabaseReconcilerinterface. Update the doc to match. (Confidence: High)
Cross-PR coordination
Another open pull request adopts the upstream OpenShell Helm chart for gateway deployment and rewrites ReconcileGateway (replacing manifest application with a deployGatewayViaHelm step) while extending the shared ReconcileOpts struct in internal/gateway/config.go with Helm fields. This PR independently rewrites the database-provisioning portion of the same ReconcileGateway function (switch → newDatabaseReconciler) and extends the same ReconcileOpts struct with ExternalDB. Maintainers need to agree on a merge order and on how external DB provisioning integrates with Helm-driven deployment — specifically whether the tenant credentials Secret is provisioned before the Helm release renders/consumes it. This is a design/ordering decision, not a mechanical merge.
Findings Summary (ordered by severity, highest first):
- [Major] External placement uses
FindSolebut documents/ships region-matching (FindByProviderAndRegionis dead code) — Spec Consistency / API design (provider.go L12, dao.go L121) - [Major]
openAdminConnuses context-lessdb.Ping()with no connect timeout — Control Plane / context propagation (external_db.go L131) - [Minor]
validateExternalConnectionSecretreinventsstringsand has a redundant guard — Code quality (service.go L115) - [Minor] External credentials Secret updated unconditionally each reconcile — Reconcile efficiency (external_db.go L367)
- [Minor] CLAUDE.md provider-onboarding guidance stale after the reconciler refactor — Docs (CLAUDE.md L106, L116)
Convention Checklist:
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
errors.IsNotFound handled for 404 scenarios |
Pass |
| No secrets in logs or error messages | Pass |
| Input validated (secret name prefix / no slash) | Pass |
| Reconcile pattern (update-or-create) | Pass |
| Proper context propagation | Fail |
| Conventional commit message | Pass |
| Test diff scrutiny (no silent contract flips) | Pass |
| // sole existing ManagedDatabase. | ||
| // sole existing ManagedDatabase. ProviderExternal selects external-server | ||
| // placement: the gateway is placed on the external ManagedDatabase whose | ||
| // region matches the gateway's target cluster region. |
There was a problem hiding this comment.
[Major] Doc/implementation mismatch — region matching is not actually wired.
This comment says external placement selects the ManagedDatabase "whose region matches the gateway's target cluster region," and the PR adds ManagedDatabaseDao.FindByProviderAndRegion to support it. But NewServiceLocator wires the external case to NewCNPGPlacement(&dbLookupAdapter{provider: ProviderExternal}), whose FindSole only succeeds when exactly one external ManagedDatabase exists and returns "zero or multiple ManagedDatabases exist" otherwise. Multi-region external registration is therefore silently unsupported.
Please either wire region-based selection through FindByProviderAndRegion, or drop the unused method and correct this comment (and the misleading placement error string).
There was a problem hiding this comment.
Fixed: removed the region-matching language from the ProviderExternal comment.
| return count > 0, nil | ||
| } | ||
|
|
||
| func (d *sqlManagedDatabaseDao) FindByProviderAndRegion(ctx context.Context, provider, region string) (ManagedDatabaseList, error) { |
There was a problem hiding this comment.
[Major, same issue as provider.go] FindByProviderAndRegion (and its mock impl) has no production caller — external placement resolves via FindSole. This is dead code that also implies a region-matching behavior the wired path does not provide. Either use it for external placement or remove it.
There was a problem hiding this comment.
Fixed: removed FindByProviderAndRegion from the ManagedDatabaseDao interface, the SQL implementation, and the mock. It was dead code with no production caller. External placement now goes through externalPlacement which uses FindSole (filtered to provider=external).
| return nil, fmt.Errorf("open admin connection: driver init failed") | ||
| } | ||
| db.SetMaxOpenConns(1) | ||
| if err := db.Ping(); err != nil { |
There was a problem hiding this comment.
[Major] Context-less Ping with no connect timeout can stall the reconcile loop.
openAdminConn uses db.Ping() (not db.PingContext(ctx)) and the DSN sets no connect_timeout. A host that black-holes TCP will block the serialized per-resource handleOne goroutine for the OS TCP timeout, ignoring gRPC-stream context cancellation — stalling both ProbeExternalServer and gateway provisioning. Thread ctx into openAdminConn and use PingContext, and/or add connect_timeout to the DSN. mapConnErrorToStatus already expects i/o timeout, so honoring the deadline is consistent.
There was a problem hiding this comment.
Fixed: openAdminConn now takes ctx context.Context as first argument and calls db.PingContext(ctx) instead of db.Ping(). All four call sites (ProbeExternalServer, ReconcileExternalDatabaseResources, DeleteExternalDatabaseResources, RotateExternalDatabaseCredentials) pass the reconcile context through.
| for k, v := range desiredLabels { | ||
| updated.Labels[k] = v | ||
| } | ||
| updated.Data = desiredData |
There was a problem hiding this comment.
[Minor] Unconditional Secret update every reconcile.
The deployment path (copyDeploymentDatabaseCredentials) short-circuits with reflect.DeepEqual before writing, but here the existing Secret is always Updated, churning resourceVersion on every pass. Add an equality check on labels/type/data before calling Update.
There was a problem hiding this comment.
Fixed: added a reflect.DeepEqual check on both .Data and .Labels before calling Update. The Secret is only written when it actually differs from the existing state, matching the pattern used by copyDeploymentDatabaseCredentials.
| if secret == nil || *secret == "" { | ||
| return errors.Validation("connection_secret is required for provider \"external\"") | ||
| } | ||
| if len(*secret) > 0 && (*secret)[0] == '/' || len(*secret) > 0 && containsSlash(*secret) { |
There was a problem hiding this comment.
[Minor] Reinvented stdlib + redundant guard.
containsSlash/hasPrefix duplicate strings.Contains/strings.HasPrefix, and len(*secret) > 0 && (*secret)[0] == '/' || len(*secret) > 0 && containsSlash(*secret) is fully covered by a single strings.Contains(*secret, "/"). Prefer the stdlib and drop the helper funcs.
There was a problem hiding this comment.
Fixed: replaced containsSlash/hasPrefix custom helpers with strings.Contains/strings.HasPrefix from stdlib, and simplified the redundant (*secret)[0] == '/' || guard to a single strings.Contains(*secret, "/") check.
| watcher.Watch[T] → event → reconciler.Handle() → per-resource serialized handleOne() | ||
| ``` | ||
|
|
||
| `ManagedDatabaseReconciler.handleOne()` branches on `db.Provider` (`cnpg` / `deployment`). `GatewayReconciler.ReconcileGateway()` branches on `opts.DatabaseProvider`. Adding a provider requires handling both paths. |
There was a problem hiding this comment.
[Minor] Stale after this PR's own refactor.
This says handleOne() branches on cnpg/deployment (missing the new external) and (L106) that adding a provider needs "a reconcile branch in ... ReconcileGateway switch" — but this PR removed that switch in favor of the newDatabaseReconciler factory / DatabaseReconciler interface. Please update both sentences to reflect the new interface and list external.
There was a problem hiding this comment.
Fixed in this iteration: updated the Control Plane Reconciler Pattern section to describe the DatabaseReconciler interface and newDatabaseReconciler() factory (which now handles cnpg/deployment/external), and updated the provider-addition guide to reference db_reconciler.go instead of the old switch in ReconcileGateway. Also restored the dropped conventions (Conventional commits, PostgreSQL for persistent storage, Verify contracts and references, Separate configuration from code, Domain probes for UI observability) that were accidentally removed in the initial rewrite.
fc365c5 to
3daa945
Compare
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
REQUEST_CHANGES. The external-database feature is well-structured and shows strong secret hygiene (admin-connection redaction, reserved-prefix validation, identifier quoting, per-provider DatabaseReconciler abstraction), but two Major issues need attention before merge: the documented region-based placement contract is scaffolded yet not wired (silent no-op with more than one external database), and the PR bundles a wholesale rewrite of CLAUDE.md that drops several unrelated cross-cutting conventions.
Amber Assessment
The external provider path is coherent end to end: API-server validation of connection_secret, control-plane in-process DDL, idempotent role/database provisioning, credential rotation, and best-effort cleanup on gateway deletion. Error handling generally wraps with %w, admin-connection errors are deliberately redacted, and the closed-vocabulary status strings are mapped without leaking raw driver errors. The refactor of the provider switch into a DatabaseReconciler interface is a good structural improvement. My concerns are the placement contract mismatch, the unrelated CLAUDE.md convention removals, and a few code-quality/observability nits detailed inline.
Blockers
None.
Major
-
Region-based placement is documented but not implemented.
provider.gostates external placement resolves "the external ManagedDatabase whose region matches the gateway's target cluster region," and aFindByProviderAndRegionDAO method plus aregionseed field were added, butdbLookupAdapter.FindSoleonly filters by provider and returns a match solely when exactly one external database exists. With more than one externalManagedDatabase,FindSolereturns""and placement silently fails to resolvedatabase_id. Either wire region matching (useFindByProviderAndRegion) or remove the dead method and correct the comment/spec to describe the actual "sole external database" behavior. -
Unrelated
CLAUDE.mdrewrite drops documented conventions. The wholesale rewrite removes several cross-cutting rules (for example "No em dashes", "Separate configuration from code", "Domain probes for UI observability", "Verify contracts and references", and the explicit "Conventional commits" / "PostgreSQL for persistent storage" bullets). These are governance guardrails unrelated to external database provisioning; removing them inside a feature PR is easy to miss in review. Split this into its own PR or restore the removed conventions.
Minor
-
Reinvented stdlib in
service.go.containsSlash/hasPrefixduplicatestrings.Contains/strings.HasPrefix, and the guard(*secret)[0] == '/' || containsSlash(*secret)is redundant. Use the standard library. -
DDL statements embed the plaintext password.
CREATE ROLE ... PASSWORD '...'/ALTER ROLE ... PASSWORD '...'errors are wrapped with%w.lib/pqdoes not echo statement text today, so risk is low, but prefer parameter-free error context (orpgcrypto-free redaction) so a future driver/error class cannot surface the literal. -
Discarded underlying errors reduce debuggability. Several existence checks (e.g. "check role existence ... query failed") drop the underlying
err; these are secret-freeSELECTstatements, so wrap with%wfor diagnosability.
Test Diff Scrutiny
The two changed assertions in managed_database_test.go / managed_database_lifecycle_test.go only change the constructor's namespace argument from "" to "hypershell" so the external path has a namespace to read from; the assertions themselves (nil-client error, hasCNPG false) are unchanged. No removed guarantee.
Cross-PR coordination
The Helm-chart gateway-deployment change (PR #194) and this PR both restructure ReconcileGateway and DeleteGatewayResources in components/control-plane/internal/gateway/reconciler.go in incompatible ways: this PR moves database provisioning out of ReconcileGateway behind a new DatabaseReconciler interface, while that PR changes those functions' signatures (adds a helmClient parameter) and replaces the manifest-based deploy path. Maintainers need to decide a merge order and how the DatabaseReconciler abstraction integrates into the Helm-based reconcile flow; whichever lands second must re-integrate rather than accept a line-level merge.
Findings Summary (ordered by severity, highest first)
- [Major] Region-based external placement documented but not wired; silent no-op with >1 external DB - Contract/Correctness (provider.go L12, plugin.go L46, dao.go L121)
- [Major]
CLAUDE.mdrewrite removes unrelated cross-cutting conventions - Governance/Scope (CLAUDE.md L166) - [Minor] Reinvented
strings.Contains/HasPrefixand redundant slash guard - Code Quality (service.go L115) - [Minor] Plaintext password embedded in DDL wrapped into errors - Security (defense-in-depth) (external_db.go L289)
- [Minor] Underlying errors discarded on existence-check failures - Observability (external_db.go L276)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass (minor: a few SELECT checks drop err) |
errors.IsNotFound handled for 404 scenarios |
Pass |
| No secrets in logs or error messages | Pass (minor defense-in-depth on DDL errors) |
| Input validated (secret name, prefix, connection_secret) | Pass |
| Reconcile pattern (update-or-create) | Pass |
| Proper context propagation | Pass |
| Conventional commit message | Pass |
| Documented convention integrity (CLAUDE.md) | Fail |
| Feature contract matches implementation (placement) | Fail |
| // sole existing ManagedDatabase. | ||
| // sole existing ManagedDatabase. ProviderExternal selects external-server | ||
| // placement: the gateway is placed on the external ManagedDatabase whose | ||
| // region matches the gateway's target cluster region. |
There was a problem hiding this comment.
This comment promises region-based placement ("the external ManagedDatabase whose region matches the gateway's target cluster region"), but the implementation in plugin.go (dbLookupAdapter.FindSole) filters only by provider and requires exactly one match. Please either wire region matching or correct this contract to describe the actual "sole external database" behavior.
There was a problem hiding this comment.
Fixed: the comment now accurately describes that placement resolves via FindSole against the sole existing external ManagedDatabase. The externalPlacement type returns a clear error when zero or more than one external ManagedDatabases exist.
| return all[0].ID, nil | ||
| var matches []*managedDatabases.ManagedDatabase | ||
| for _, db := range all { | ||
| if a.provider == "" || db.Provider == a.provider { |
There was a problem hiding this comment.
FindSole ignores region entirely and returns a match only when exactly one external database exists. With two or more external ManagedDatabase rows this returns "" and placement silently fails to resolve database_id. This contradicts the region-matching contract documented in provider.go and the region field seeded in seed.sh. Wire FindByProviderAndRegion here, or drop the region scaffolding and document the single-DB constraint.
There was a problem hiding this comment.
Fixed: plugin.go now wires NewExternalPlacement (not NewCNPGPlacement) for ProviderExternal. dbLookupAdapter.FindSole filters to provider=external, so it returns empty when zero or multiple exist — and externalPlacement.Resolve returns a validation error with a clear message in that case.
| return count > 0, nil | ||
| } | ||
|
|
||
| func (d *sqlManagedDatabaseDao) FindByProviderAndRegion(ctx context.Context, provider, region string) (ManagedDatabaseList, error) { |
There was a problem hiding this comment.
FindByProviderAndRegion is added to the interface, the SQL DAO, and the mock, but is never called anywhere. It appears to be the intended region-placement lookup that was left unwired (see plugin.go FindSole). Either use it for placement or remove the dead code across the interface/impl/mock.
There was a problem hiding this comment.
Fixed: removed FindByProviderAndRegion from the interface, SQL dao, and mock. It was dead code.
| if secret == nil || *secret == "" { | ||
| return errors.Validation("connection_secret is required for provider \"external\"") | ||
| } | ||
| if len(*secret) > 0 && (*secret)[0] == '/' || len(*secret) > 0 && containsSlash(*secret) { |
There was a problem hiding this comment.
This reimplements the standard library: containsSlash/hasPrefix duplicate strings.Contains/strings.HasPrefix, and (*secret)[0] == '/' || containsSlash(*secret) is redundant (the second term already covers the first). Prefer strings.Contains(*secret, "/") and strings.HasPrefix(*secret, externalSecretPrefix).
There was a problem hiding this comment.
Fixed: replaced with strings.Contains/strings.HasPrefix and removed the redundant guard.
| if _, err := db.ExecContext(ctx, | ||
| fmt.Sprintf("ALTER ROLE %s PASSWORD '%s'", pgQuoteIdent(pgName), pgQuoteLiteral(password)), | ||
| ); err != nil { | ||
| return fmt.Errorf("ALTER ROLE password for gateway %s: %w", gatewayID, err) |
There was a problem hiding this comment.
The DDL string embeds the plaintext password (... PASSWORD '%s') and this error wraps it with %w. lib/pq does not echo statement text today so the risk is low, but per the security spec (no secrets in errors) prefer building error context that cannot include the statement literal, so a future driver/error class can't surface the password. Same pattern at the CREATE ROLE (L283) and rotation ALTER ROLE (L475) sites.
There was a problem hiding this comment.
Fixed: CREATE ROLE and ALTER ROLE errors (both in provisioning and rotation) now return sentinel messages with '(credentials redacted)' instead of wrapping with %w. This ensures the plaintext password in the DDL statement cannot leak through error propagation even if a future pq driver version echoes statement text.
| if err := db.QueryRowContext(ctx, | ||
| "SELECT EXISTS(SELECT 1 FROM pg_roles WHERE rolname = $1)", pgName, | ||
| ).Scan(&roleExists); err != nil { | ||
| return fmt.Errorf("check role existence for gateway %s: query failed", gatewayID) |
There was a problem hiding this comment.
This drops the underlying error (query failed). The statement is a secret-free SELECT EXISTS, so wrap the real error with %w for diagnosability. Same for the database-existence check below.
There was a problem hiding this comment.
Fixed: the role-existence and database-existence SELECT EXISTS queries now wrap the real error with %w. These are safe to wrap (no secrets in SELECT EXISTS statements).
| ``` | ||
| Support skills: `/review-guidance`, `/amber-review`, `/ui-standards`, `/align`, `/maintain-ci`, `/update-openshell`, `/memory`, `/cloud-hub-ingress-bootstrap` | ||
|
|
||
| ## Critical Conventions |
There was a problem hiding this comment.
This rewrite removes several documented cross-cutting conventions unrelated to external database provisioning (e.g. "No em dashes", "Separate configuration from code", "Domain probes for UI observability", "Verify contracts and references", and the explicit "Conventional commits"/"PostgreSQL for persistent storage" bullets). Dropping governance guardrails inside a feature PR is easy to miss. Please split the CLAUDE.md restructuring into its own PR or restore the removed conventions.
There was a problem hiding this comment.
Fixed: restored all dropped conventions (PostgreSQL for persistent storage, Conventional commits, Verify contracts and references, Separate configuration from code, Domain probes for UI observability) and updated the Control Plane Reconciler Pattern section to reflect the DatabaseReconciler interface pattern introduced by this PR.
e04a2e0 to
55564a1
Compare
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
This PR adds a well-structured external database provider: a clean DatabaseReconciler interface, per-provider files, closed-vocabulary status mapping that matches the new spec, and careful credential redaction. The main substantive issue is that the external credential-rotation path omits the idempotency guard its CNPG sibling has, so it would re-rotate on every reconcile once the rotation trigger is ever wired; the remaining items are minor.
What is good
- The refactor from a monolithic
reconciler.gointo aDatabaseReconcilerinterface withcnpg_db.go/deployment_db.go/external_db.gois clean, and the extracted CNPG/deployment logic appears to be a faithful move (readiness wait, credential copy, and rotation all preserved). - Secret handling follows
security.spec.md: admin credentials are read from a K8s Secret reference, connection/DDL failures return generic "credentials redacted" errors, passwords are never logged, and the admin connection is short-lived (SetMaxOpenConns(1), deferredClose). - Input validation is solid: the
hypershell-managed-db-reserved-prefix + no-slash rule is enforced both at the API server (managedDatabases/service.go) and in the control plane (validateExternalSecretName), matchingnaming-multitenancy.spec.md. - The
mapConnErrorToStatusclosed vocabulary (Ready,Failed: secret_invalid|unreachable|auth_failed|insufficient_privilege|tls_failed) matches the status table inopenshell-gateway-database-external.spec.md, and the raw driver error is never surfaced to status. - DDL is parameterized where possible and identifiers are quoted via
pgQuoteIdent; provisioning/cleanup are existence-checked and idempotent.
Findings
[Major] External rotation lacks the CNPG idempotency guard (external_db.go RotateExternalDatabaseCredentials). rotateCNPGDatabaseCredentials reads the tenant Secret's hypershell.redhat.io/last-db-rotation annotation and returns early when it already equals the trigger value (cnpg_db.go:322-326). The external path never performs this comparison: whenever rotateAnnotation != "", it unconditionally generates a new password, issues ALTER ROLE, and rewrites the Secret. The spec says rotation happens "On a new trigger value" (openshell-gateway-database-external.spec.md:576), so once the trigger is populated this would rotate the password and roll the gateway on every reconcile pass. Read the existing Secret first and skip when last-db-rotation == triggerValue, mirroring the CNPG implementation. (Note: ReconcileOpts.RotateDBCredentials is not currently wired from any caller for either provider, so this is latent today - but it is new code that diverges from both the spec and the sibling provider.) Confidence: High that the guard is missing; Medium on live impact given the dormant wiring.
[Minor] Cleanup abandons the role when the database drop fails (external_db.go DeleteExternalDatabaseResources). If DROP DATABASE returns an error, the function logs and returns before attempting DROP ROLE, leaving the login role orphaned on the external server. Since cleanup is best-effort, consider continuing to the role drop (still logging the database-drop failure) so a transient database-drop error does not permanently strand the role. Confidence: Medium.
[Minor] Network-error classification uses a bare type assertion (external_db.go mapConnErrorToStatus). err.(net.Error) will not match a net.Error wrapped by the database/sql/lib/pq layers; use errors.As(err, &netErr). The substring checks below catch most cases, so impact is limited to occasional misclassification as unreachable. Confidence: Medium.
[Minor] Password is interpolated into DDL text (external_db.go, CREATE ROLE/ALTER ROLE ... PASSWORD '...'). lib/pq cannot parameterize DDL, so the plaintext password becomes part of the statement string and can land in the external server's logs if log_statement=all/log_min_error_statement captures it. The generated value is hex so there is no injection risk, and this is largely unavoidable with this driver; worth a code comment noting the external-server logging caveat. Confidence: Medium.
Test Diff Scrutiny
The two changed assertions in managed_database_test.go / managed_database_lifecycle_test.go only switch the constructor's controlPlaneNamespace argument from "" to "hypershell"; the assertions themselves (nil-client error, hasCNPG false) are unchanged, so no guarantee was removed. The added external branches ship with spec scenarios but no new Go unit tests for ReconcileExternalDatabaseResources / rotation / cleanup beyond the E2E leg - additional unit coverage for the rotation guard and cleanup ordering would be worthwhile.
Cross-PR coordination
Another open pull request re-platforms gateway deployment onto an upstream Helm chart and, in doing so, rewrites the same components/control-plane/internal/gateway/reconciler.go and config.go orchestration and ReconcileOpts that this PR restructures into the new DatabaseReconciler interface. That PR also relocates/removes rotateCNPGDatabaseCredentials (which this PR moves into cnpg_db.go) and wires the openshell-gateway-db-credentials Secret into Helm values (server.externalDbSecret) - the same Secret this PR now produces per-provider, including the new external path. These are incompatible restructurings of one code path plus a producer/consumer relationship on the credentials Secret, so maintainers should decide a merge order and how the per-provider DatabaseReconciler design (and the external provider) is integrated into the Helm-based deployment model. That coordination is between the owners of these two PRs.
Findings Summary (ordered by severity, highest first)
- [Major] External credential rotation omits the CNPG
last-db-rotationidempotency guard, so it would re-rotate every reconcile once wired - Reconciliation / Spec Consistency - [Minor]
DeleteExternalDatabaseResourcesreturns before dropping the role whenDROP DATABASEfails, orphaning the role - Reconciliation / Cleanup - [Minor]
mapConnErrorToStatususeserr.(net.Error)instead oferrors.Asfor wrapped driver errors - Error Handling - [Minor] Password interpolated into
CREATE/ALTER ROLEDDL may reach external-server logs - Security (informational)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
errors.IsNotFound handled for 404 scenarios |
Pass |
| No secrets in logs or error messages | Pass |
| Input validated (Secret name prefix / no slash) | Pass |
| Reconcile pattern (not create-or-skip) | Pass |
| Rotation idempotency parity across providers | Fail |
| Conventional commit message | Pass |
| } | ||
| }() | ||
|
|
||
| if _, err := db.ExecContext(ctx, |
There was a problem hiding this comment.
[Major] Missing idempotency guard on rotation. rotateCNPGDatabaseCredentials reads the tenant Secret's hypershell.redhat.io/last-db-rotation annotation and returns early when it equals the trigger value (cnpg_db.go:322-326). This external path never compares against the stored annotation, so every reconcile where rotateAnnotation != "" generates a new password, runs ALTER ROLE, and rewrites the Secret - re-rotating and rolling the gateway on each pass once the trigger is wired. Read the existing openshell-gateway-db-credentials Secret first and skip when last-db-rotation == triggerValue (mirroring the CNPG guard), then proceed to ALTER ROLE. The spec states rotation occurs "On a new trigger value" (openshell-gateway-database-external.spec.md).
There was a problem hiding this comment.
Fixed: added idempotency guard mirroring rotateCNPGDatabaseCredentials. RotateExternalDatabaseCredentials now reads the tenant Secret first, compares hypershell.redhat.io/last-db-rotation against triggerValue, and returns early if they match — so repeated reconciles with the same rotation annotation are no-ops.
| if err := db.QueryRowContext(ctx, "SELECT EXISTS(SELECT 1 FROM pg_database WHERE datname = $1)", pgName).Scan(&dbExists); err == nil && dbExists { | ||
| if _, err := db.ExecContext(ctx, fmt.Sprintf("DROP DATABASE %s", pgQuoteIdent(pgName))); err != nil { | ||
| log.Printf("WARN external DB cleanup for gateway %s: DROP DATABASE failed: %v", gatewayID, err) | ||
| return |
There was a problem hiding this comment.
[Minor] Role is orphaned when the database drop fails. Returning here after a DROP DATABASE error skips the DROP ROLE block below, leaving the login role behind on the external server. Since cleanup is documented as best-effort, consider logging the database-drop failure and continuing to attempt the role drop so a transient error does not permanently strand the role.
There was a problem hiding this comment.
Fixed: changed return after DROP DATABASE failure to log + continue. The role drop block now always runs, even when the database drop fails, so a transient database-drop error can't permanently strand the login role.
| lower := strings.ToLower(msg) | ||
|
|
||
| // Network-level failures | ||
| if _, ok := err.(net.Error); ok { |
There was a problem hiding this comment.
[Minor] err.(net.Error) will not match a net.Error wrapped by database/sql/lib/pq. Prefer var netErr net.Error; if errors.As(err, &netErr) { ... }. The substring checks below catch most cases, so impact is limited to occasional misclassification as unreachable.
There was a problem hiding this comment.
Fixed: replaced err.(net.Error) type assertion with errors.As(err, &netErr) so wrapped net.Error values (e.g. from database/sql or lib/pq) are correctly classified as unreachable instead of falling through to the generic catch-all.
|
|
||
| if !roleExists { | ||
| if _, err := db.ExecContext(ctx, | ||
| fmt.Sprintf("CREATE ROLE %s LOGIN PASSWORD '%s'", pgQuoteIdent(pgName), pgQuoteLiteral(password)), |
There was a problem hiding this comment.
[Minor / informational] The generated password is interpolated into the DDL string (lib/pq cannot parameterize CREATE ROLE/ALTER ROLE), so the plaintext value becomes part of the statement text and can be captured by the external server's log_statement=all. No injection risk (hex value, quoted), but a short comment noting the external-server logging caveat would help future readers.
There was a problem hiding this comment.
Added a comment above the CREATE ROLE / ALTER ROLE DDL calls noting that lib/pq cannot parameterize these statements so the password appears in statement text, and directing operators to restrict log_statement verbosity or use server-side log redaction.
55564a1 to
972e211
Compare
|
E2E Kind (external) fix: The timeout was caused by setting Fix (972e211): moved the
|
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
This is a well-structured, security-conscious feature: it adds external as a first-class DATABASE_PROVIDER, refactors the provider switch into a clean DatabaseReconciler interface, and consistently redacts credentials from logs and errors. The findings below are hardening and spec-consistency items rather than correctness blockers, so this is a COMMENT-level review.
I reviewed against CLAUDE.md, the security spec, and the control-plane conventions spec. No panic(), error wrapping is correct, errors.IsNotFound is handled, input is validated (connection_secret prefix/namespace rules on both the API server and control plane, identifier quoting via pgQuoteIdent), and admin credentials are kept out of logs, errors, and status vocabulary.
Findings
[Minor] sslrootcert is documented as a consumable admin-Secret key but is never read - Spec Consistency
readExternalAdminSecret/dsn() only assemble host/port/user/password/dbname/sslmode; sslrootcert is dropped. The spec's admin-Secret table lists sslrootcert as consumable and states "when the admin Secret carries sslrootcert, the reconciler SHALL propagate the CA ... and set verify-full". The spec does allow v1 to ship require-only with verify-full behind a follow-up, so this is not a blocker - but as written an operator who sets sslmode=verify-full + sslrootcert (e.g. an RDS CA bundle) will silently get no custom CA, and verify-full will fail against a private CA. Recommend either wiring sslrootcert into the admin connection now, or marking that key as reserved-for-follow-up in the spec so it is not advertised as working in v1. (Confidence: Medium)
[Minor] net.Error classification uses a bare type assertion - Best Practice
mapConnErrorToStatus does if _, ok := err.(net.Error); ok. The error returned by db.PingContext is typically wrapped by the pq/database/sql layers, so this assertion will usually miss and the classifier falls through to fragile substring matching. Prefer errors.As(err, &netErr) so wrapped network errors are still classified as Failed: unreachable. (Confidence: Medium)
[Minor] Deletion issues DROP DATABASE/DROP ROLE on a user-owned external server - Design confirmation
DeleteExternalDatabaseResources is irreversible data loss on infrastructure HyperShell does not own. This matches the documented per-gateway-DB contract and is best-effort, so it is intended - flagging only so the destructive-on-external-infra behavior is a conscious, spec-backed decision. (Confidence: High)
[Minor] Pre-existing reconciler tests changed the namespace arg from "" to "hypershell" - Test Diff Scrutiny
managed_database_test.go and managed_database_lifecycle_test.go flip the 4th NewManagedDatabaseReconciler argument from "" to "hypershell". The assertions themselves are unchanged (still expect the nil-client error / hasCNPG=false), so no guarantee was removed - but please confirm the literal is genuinely required by the new external code path rather than a cosmetic tweak, since the external handler now depends on controlPlaneNamespace. (Confidence: High)
Cross-PR coordination
An open pull request adopts the upstream OpenShell Helm chart for gateway deployments and rewrites the same ReconcileGateway database-provider block and the ReconcileOpts struct that this PR restructures into the new DatabaseReconciler interface and per-provider files. That PR also removes NetworkPolicies as a design decision, whereas this PR continues to thread SkipNetworkPolicies through ReconcileOpts. These are two competing refactors of the same gateway-reconcile core with conflicting assumptions about NetworkPolicies and the deployment mechanism. Maintainers should decide a merge order and how the DatabaseReconciler abstraction integrates with the Helm-based deploy path before both land, so the second PR is rebased onto the agreed structure rather than re-deriving it.
Findings Summary (ordered by severity, highest first):
- [Minor]
sslrootcertdocumented but never consumed;verify-fullhardening path is unreachable - Spec Consistency (external_db.go dsn/readExternalAdminSecret) - [Minor]
net.Errorclassified via bare type assertion instead oferrors.As- Best Practice (external_db.go mapConnErrorToStatus) - [Minor]
DROP DATABASE/DROP ROLEon external infra - confirm intended contract - Design (external_db.go DeleteExternalDatabaseResources) - [Minor] Pre-existing tests switched namespace arg
""->"hypershell"- confirm necessity - Test Diff Scrutiny (managed_database_test.go)
Convention Checklist:
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
errors.IsNotFound handled for 404 scenarios |
Pass |
| No secrets in logs, errors, or status | Pass |
| Input validated (secret prefix/namespace, SQL identifier quoting) | Pass |
| Reconcile pattern (update-or-create), not create-and-ignore | Pass |
| Conventional commit message | Pass |
| OpenAPI client not manually edited | Pass (n/a) |
| Spec/implementation consistency | Partial (sslrootcert) |
| if dbname == "" { | ||
| dbname = "postgres" | ||
| } | ||
| sslmode := get("sslmode") |
There was a problem hiding this comment.
sslrootcert is never read here (or in dsn()), so the spec-documented verify-full hardening path is unreachable: an operator who sets sslmode=verify-full plus an sslrootcert CA bundle in the admin Secret gets neither propagated. The spec permits v1 to ship require-only with verify-full as a follow-up, so this is non-blocking - but please either wire sslrootcert into the admin connection or mark that admin-Secret key as reserved-for-follow-up in the spec so it is not advertised as functional in v1.
| lower := strings.ToLower(msg) | ||
|
|
||
| // Network-level failures | ||
| if _, ok := err.(net.Error); ok { |
There was a problem hiding this comment.
err.(net.Error) is a bare type assertion, but the error from db.PingContext is normally wrapped by the pq/database/sql layers, so this branch will usually miss and fall through to substring matching. Use errors.As(err, &netErr) to reliably classify wrapped network errors as Failed: unreachable.
| // Drop database (guarded by existence check; cannot be in a transaction). | ||
| var dbExists bool | ||
| if err := db.QueryRowContext(ctx, "SELECT EXISTS(SELECT 1 FROM pg_database WHERE datname = $1)", pgName).Scan(&dbExists); err == nil && dbExists { | ||
| if _, err := db.ExecContext(ctx, fmt.Sprintf("DROP DATABASE %s", pgQuoteIdent(pgName))); err != nil { |
There was a problem hiding this comment.
DROP DATABASE / DROP ROLE here is irreversible data loss on an external server HyperShell does not own. This matches the documented per-gateway-DB contract and is best-effort, so flagging only to confirm the destructive-on-external-infra behavior is a conscious, spec-backed decision.
|
|
||
| func TestNewManagedDatabaseReconcilerWithoutKubernetesClient(t *testing.T) { | ||
| r := NewManagedDatabaseReconciler(nil, nil, nil, "") | ||
| r := NewManagedDatabaseReconciler(nil, nil, nil, "hypershell") |
There was a problem hiding this comment.
This pre-existing test flips the namespace argument from "" to "hypershell". The assertion is unchanged (still hasCNPG=false), so no guarantee is removed - please confirm the literal is genuinely required by the new external code path (which reads controlPlaneNamespace) rather than a cosmetic change.
152384e to
7bd65a2
Compare
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
This PR cleanly adds an external DATABASE_PROVIDER by refactoring the per-provider database logic behind a DatabaseReconciler interface and implementing in-process DDL against a user-managed PostgreSQL server, with strong secret-reference hardening (reserved hypershell-managed-db- prefix, namespace pinning, closed-vocabulary status, redacted connection errors). The refactor preserves the existing CNPG/deployment behavior and the security posture is good; findings below are non-blocking quality and coordination items.
What I verified
- Secret handling: admin credentials are read only from the control-plane namespace, the
connection_secretreference is validated (non-empty, no/, reserved prefix) in both the API server (Create/Replace, and PATCH viaReplace) and again in the control plane before any read. Connection/DDL errors are deliberately not wrapped with%won the paths that could carry the DSN, so credentials are not surfaced in errors. Good. - Error handling: no
panic(),errors.IsNotFoundhandled, best-effort delete logs and does not propagate (matches the documented interface contract), rotation is idempotent via thelast-db-rotationannotation. - The large deletion in
reconciler.gois a move of CNPG/deployment helpers intocnpg_db.go/deployment_db.go;DeploymentReadinessstill exists and the readiness test file was renamed, not dropped. - Test-assertion diffs: the only changed pre-existing assertions swap the
NewManagedDatabaseReconcilernamespace arg"" -> "hypershell"; both tests still assert the same guarantees (nil-client error,hasCNPGfalse). No removed guarantee. - No proto/OpenAPI generated files were hand-edited;
connection_secretalready existed on the model.
Cross-PR coordination
An open pull request proposes shifting gateway deployment from static SSA-managed manifests to installing the upstream OpenShell Helm chart at runtime, restructuring the same internal/gateway/reconciler.go and internal/gateway/config.go (ReconcileOpts) surface and introducing its own values-mapping from Gateway resources to chart values. This PR restructures that same surface by adding the DatabaseReconciler abstraction and the ExternalDB/ExternalDBConfig fields, and it delivers the gateway database credentials by writing the openshell-gateway-db-credentials Secret into the tenant namespace for the gateway Deployment to consume. Maintainers should decide the merge order and how the credentials Secret produced by the new (external and existing) DatabaseReconciler path is referenced by the Helm values mapping, so the two efforts do not land incompatible contracts for how a gateway obtains its database connection.
Findings
See inline comments. All are Minor.
Findings Summary (ordered by severity, highest first):
- [Minor] DDL interpolates the generated password into
CREATE/ALTER ROLEstatement text, which can appear in server logs underlog_statement=all- Security (external_db.go L283-289) - [Minor] Admin connection DSN sets no
connect_timeout; a hung TCP connect depends solely on a ctx deadline that may be absent - Robustness (external_db.go L120-135) - [Minor] ManagedDatabase external status is emitted as free-form magic strings with no shared/validated vocabulary - Spec Consistency (reconciler.go L322)
- [Minor] Kind external-postgres fixture omits a restricted SecurityContext (
runAsNonRoot: false, no dropped caps) - Security (scripts/kind/up.sh L321) - [Minor] CLAUDE.md is rewritten well beyond this feature and drops existing entries (packages/gateway-management-ui, apm.yml, several SDLC skills) - Docs / Scope (CLAUDE.md)
Convention Checklist:
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
errors.IsNotFound handled |
Pass |
| No secrets in logs or error messages | Pass |
| Input validated (secret reference rules) | Pass |
| SecurityContext on all pod specs | Fail (Kind test fixture only) |
| Reconcile pattern used | Pass |
| Image references consistent | Pass |
| OpenAPI/proto not hand-edited | Pass |
| Test diff scrutiny (no silent guarantee removal) | Pass |
| Conventional commit message | Pass |
| // the password will appear in the server log; operators should restrict | ||
| // log verbosity or use server-side log redaction accordingly. | ||
| if _, err := db.ExecContext(ctx, | ||
| fmt.Sprintf("CREATE ROLE %s LOGIN PASSWORD '%s'", pgQuoteIdent(pgName), pgQuoteLiteral(password)), |
There was a problem hiding this comment.
The generated role password is interpolated directly into the CREATE ROLE/ALTER ROLE statement text (PASSWORD '%s'). The inline comment correctly notes that under log_statement=all the password lands in the external server's logs. This is a real (if documented) credential-exposure surface for an operator-owned server. Since lib/pq cannot parameterize DDL, please at minimum surface this operational requirement in the external-DB spec/runbook (restrict log_statement / enable server-side redaction). Confidence: High.
| // openAdminConn opens a short-lived PostgreSQL admin connection. Callers must | ||
| // close it. Credentials must not appear in error messages. | ||
| func openAdminConn(ctx context.Context, params *externalAdminParams) (*sql.DB, error) { | ||
| db, err := sql.Open("postgres", params.dsn()) |
There was a problem hiding this comment.
openAdminConn opens the connection with a DSN that has no connect_timeout, and PingContext(ctx) will only bound the dial if the caller's context carries a deadline. If a reconcile ever calls in with a deadline-less context, a black-holed external host could block the reconcile goroutine indefinitely. Consider adding connect_timeout=<n> to the DSN as a defensive backstop. Confidence: Medium.
| event.ResourceID, db.Name, event.Type) | ||
|
|
||
| if db.GetConnectionSecret() == "" { | ||
| newStatus := "Failed: secret_invalid" |
There was a problem hiding this comment.
The external ManagedDatabase status is emitted as free-form strings ("Failed: secret_invalid", "Failed: unreachable", "Ready", etc.) constructed here and in external_db.go. These are not validated or shared, so they can drift from any consumer (metrics/console) that parses them. Consider a small typed vocabulary (constants + validator) mirroring how gateway phases are being standardized elsewhere, so the ManagedDatabase status set is a single source of truth. Confidence: Medium.
| app: postgres | ||
| spec: | ||
| securityContext: | ||
| runAsNonRoot: false |
There was a problem hiding this comment.
The stand-in external PostgreSQL Deployment sets runAsNonRoot: false and no container-level SecurityContext (no allowPrivilegeEscalation: false, no capabilities.drop: [ALL]). This is a Kind/CI-only fixture simulating a cloud-managed server, so it is not shipped to production, but it still diverges from the project's restricted-SecurityContext convention. A securityContext with dropped caps would keep CI fixtures aligned with the standard. Confidence: High (Minor - test infra only).
| @@ -1,35 +1,140 @@ | |||
| # HyperShell | |||
| # CLAUDE.md | |||
There was a problem hiding this comment.
This PR rewrites the top-level CLAUDE.md far beyond the external-DB feature. It adds genuinely useful material (module layout, plugin system, the 'Adding a new DATABASE_PROVIDER' checklist), but it also drops existing entries: the Structure section listing packages/gateway-management-ui/ and apm.yml, and the full SDLC skill list with links (e.g. /spec, /full-stack-pipeline, /dev-cluster, deploy skills). Recommend either splitting the unrelated doc rewrite into its own PR or preserving the dropped structure/skill references; a wholesale rewrite here also raises merge-conflict risk with other in-flight PRs touching this file. Confidence: Medium.
7bd65a2 to
0c5c4e8
Compare
|
Addressing all 5 Minor findings from the round 3 Amber review (commit 0c5c4e8): 1. [Minor] DDL password in server log — Added a spec-level caveat in 2. [Minor] No 3. [Minor] Free-form magic strings for status vocabulary — Defined package-level constants ( 4. [Minor] Kind fixture missing restricted SecurityContext — Added a container-level 5. [Minor] CLAUDE.md dropped entries — Restored the |
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
This PR adds external as a first-class DATABASE_PROVIDER and does so cleanly: a well-factored DatabaseReconciler interface extracts the previously-inline CNPG/deployment logic into per-provider files, credential handling redacts secrets from errors and logs, and the connection-Secret reference is guarded by a reserved-prefix security boundary. I found no Blocker or Critical issues; the notes below are Minor polish plus one cross-PR coordination item that needs a maintainer decision.
Strengths
- Secret hygiene is careful: raw connection/DDL errors are discarded and replaced with redacted messages, the admin DSN and generated passwords never reach logs, and the
log_statement=allcaveat forCREATE/ALTER ROLEis called out in-code. - The
hypershell-managed-db-reserved prefix plus the no-/rule (enforced in both the API serverservice.goand the control-planeexternal_db.go) is a real security boundary, documented innaming-multitenancy.spec.md, that prevents an API-level reference from naming an unrelated Secret such ashypershell-db-app. - Generated passwords are 32 bytes of
crypto/randrendered as hex, so the values interpolated into non-parameterizable DDL cannot carry an injection payload; identifiers derive from internal gateway IDs and are quoted viapgQuoteIdent. - The refactor moves
*kubernetes.Clientsettokubernetes.Interface, improving testability, and preserves the CNPG/deployment behavior verbatim. - Nil-client and empty-
connection_secretpaths are guarded before any external probe, and the provider switch rejects unknown providers rather than falling back.
Findings
[Minor] External Postgres test fixture runs as root - scripts/kind/up.sh:321 sets runAsNonRoot: false on the stand-in PostgreSQL Deployment. This is CI/dev-only infrastructure that simulates a cloud-managed server (the real external DB lives outside the cluster), so it is analogous to the CNPG DB exception, but it is worth a comment noting why the restricted context is intentionally relaxed here.
[Minor] Status string duplicated as a literal - components/control-plane/internal/reconciler/reconciler.go:322 hardcodes "Failed: secret_invalid", which must stay in lockstep with the unexported externalDBStatusSecretInvalid vocabulary in external_db.go. Consider exporting the status constants so the closed vocabulary has a single source of truth.
[Minor] CLAUDE.md rewrite is bundled into a DB feature PR - CLAUDE.md is rewritten (+133/-57) alongside the external-DB change. I verified the critical conventions (no panic, reconcile-not-create-or-skip, image-reference matching, restricted SecurityContext, no em dashes) all survive, but a top-level doc overhaul riding along with a feature makes both harder to review; splitting it would be cleaner.
[Minor] Orphaned external objects on ManagedDatabase deletion - handleExternalDatabase treats external ManagedDatabase deletion as register-only (per spec), so per-gateway roles/databases are only cleaned up through the gateway reconciler. If an external ManagedDatabase is deleted while gateways still reference it, roles/databases persist on the external server. This is a documented design choice; consider surfacing it in operator troubleshooting docs.
Test Diff Scrutiny
The two modified pre-existing tests (managed_database_test.go, managed_database_lifecycle_test.go) only change the constructor's controlPlaneNamespace argument from "" to "hypershell"; no assertion is flipped and no guarantee is removed. Benign scaffolding adaptation to the new external code path.
Cross-PR coordination
The control-plane Helm-adoption change for gateway deployments concurrently rewrites the same core functions this PR restructures: both edit ReconcileGateway/DeleteGatewayResources and the ReconcileOpts struct in components/control-plane/internal/gateway/config.go and reconciler.go, and both touch the database-provider control flow. This PR replaces the inline provider switch with a DatabaseReconciler interface and adds an ExternalDB field to ReconcileOpts, while the other PR rewrites those same functions around a Helm/values deployment model and its DatabaseProvider documentation still assumes the provider is "always deployment or CNPG." Maintainers should decide a merge order and reconcile the two designs so the external DatabaseReconciler (and the openshell-gateway-db-credentials Secret it writes) integrates with the Helm-based reconcile path rather than being dropped when the second PR rebases.
Findings Summary (ordered by severity, highest first):
- [Minor] External Postgres test fixture sets
runAsNonRoot: false- Container Security (up.sh L321) - [Minor] Status vocabulary duplicated as a literal instead of a shared constant - Maintainability (reconciler.go L322)
- [Minor] Large CLAUDE.md rewrite bundled with a feature change - Review Hygiene (CLAUDE.md)
- [Minor] Register-only external ManagedDatabase deletion can orphan per-gateway DB objects - Observability/Docs (reconciler.go handleExternalDatabase)
Convention Checklist:
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
errors.IsNotFound handled for 404 scenarios |
Pass |
| No secrets in logs or error messages | Pass |
| Input validated (connection_secret prefix/format) | Pass |
| SQL injection prevented (identifier quoting + hex passwords) | Pass |
| SecurityContext on pod specs | Pass (test fixture relaxes intentionally) |
| Reconcile pattern used (not create-or-skip) | Pass |
| Status updated on error paths | Pass |
| Proper context propagation | Pass |
| Conventional commit message | Pass |
| Test diff scrutiny (no silent contract flips) | Pass |
| // the password will appear in the server log; operators should restrict | ||
| // log verbosity or use server-side log redaction accordingly. | ||
| if _, err := db.ExecContext(ctx, | ||
| fmt.Sprintf("CREATE ROLE %s LOGIN PASSWORD '%s'", pgQuoteIdent(pgName), pgQuoteLiteral(password)), |
There was a problem hiding this comment.
The password is interpolated into non-parameterizable DDL here. This is handled responsibly - the password is crypto/rand hex (no injectable characters), the identifier is quoted via pgQuoteIdent, the raw error is discarded in favor of a redacted message, and the log_statement=all server-log caveat is documented. No change required; noting it so the security-sensitive path is visible in review. (Minor)
| event.ResourceID, db.Name, event.Type) | ||
|
|
||
| if db.GetConnectionSecret() == "" { | ||
| newStatus := "Failed: secret_invalid" |
There was a problem hiding this comment.
This literal "Failed: secret_invalid" must stay in lockstep with the unexported externalDBStatusSecretInvalid in external_db.go. Consider exporting the closed-vocabulary status constants so both packages share one source of truth and cannot drift. (Minor)
| app: postgres | ||
| spec: | ||
| securityContext: | ||
| runAsNonRoot: false |
There was a problem hiding this comment.
runAsNonRoot: false on this stand-in PostgreSQL relaxes the restricted SecurityContext convention. This is CI/dev-only infrastructure simulating an out-of-cluster cloud database, so the relaxation is defensible (analogous to the CNPG DB exception), but a one-line comment stating why root is required here would prevent it from being copied into a production manifest. (Minor)
0c5c4e8 to
e04610d
Compare
|
Addressing actionable findings from the round 4 Amber review (commit e04610d): [Minor] External Postgres fixture sets [Minor] Status vocabulary duplicated as a literal in reconciler.go - Exported the status constants ( [Minor] CLAUDE.md rewrite bundled with feature - Acknowledged. The CLAUDE.md changes are minimal corrections that are a direct consequence of this PR (the Control Plane Reconciler Pattern description was stale relative to the new [Minor] Register-only ManagedDatabase deletion can orphan per-gateway objects - This is a documented design choice in the spec (per-gateway cleanup runs through the gateway reconciler's delete path). No code change - the orphan risk is surfaced in the spec's "Operator troubleshooting" consideration by the DDL execution section. If a more explicit troubleshooting note is desired in a separate docs PR, that can be tracked independently. |
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
COMMENT - This is a carefully engineered feature that adds external as a first-class database provider, with good security instincts: connection-secret prefix validation, credential redaction in errors, closed-vocabulary status strings, parameterized existence checks, and identifier quoting. The main items to weigh are the best-effort cleanup semantics for objects that live on user-owned infrastructure and one cross-PR structural decision; none rise to a blocker.
Summary
The change extracts per-provider database reconcilers behind a clean DatabaseReconciler interface (cnpg, deployment, external, plus a legacy no-op), threads ExternalDBConfig through ReconcileOpts, and adds a register-only ManagedDatabase probe path for external servers. The refactor of gateway/reconciler.go into cnpg_db.go/deployment_db.go/external_db.go/db_reconciler.go reads as a faithful move with the external logic added on top; no panic(), no context.TODO(), and errors are wrapped with context throughout.
Findings
[Major] External database/role cleanup is best-effort and can leave orphaned objects plus valid credentials on user-owned infrastructure
Unlike deployment/cnpg (in-cluster, reclaimed with the namespace), the external provider's objects live on a server HyperShell does not own. DeleteExternalDatabaseResources (external_db.go:401) returns nothing; a failed DROP DATABASE/DROP ROLE, an unreadable admin Secret, or an unreachable server only produces a WARN with no status surfaced and no retry. The gateway role and its (still-valid) password can persist indefinitely on a billed external server. This is a documented best-effort design (db_reconciler.go:12-14), but for external infrastructure it is worth either surfacing a durable status/condition, a retry, or at minimum an operator runbook so orphans are discoverable. Confidence: High that the behavior is as described; Medium on desired remediation (design call).
[Minor] Misleading success log when DROP DATABASE fails during external cleanup
In DeleteExternalDatabaseResources, the INFO dropped external database ... line is emitted unconditionally after the DROP DATABASE exec, even when that exec logged a WARN ... DROP DATABASE failed. The log then claims success for an operation that failed. Move the INFO into the success branch. Confidence: High.
[Minor] Password interpolated into CREATE ROLE/ALTER ROLE statement text
The code correctly documents the lib/pq limitation (DDL cannot be parameterized) and redacts credentials from error messages, but the password still lands in the SQL text and thus in server logs when log_statement=all. The inline comment is the right mitigation; consider also noting this in the operator-facing external DB spec so operators restrict server log verbosity. Confidence: High that the limitation exists; the handling is reasonable.
[Minor] Scope: large CLAUDE.md rewrite bundled into a feature PR
The PR reworks CLAUDE.md (~133 additions / ~57 deletions) alongside the external-DB feature. This inflates the review surface and raises merge-conflict risk with other in-flight edits to the same file. Consider splitting the documentation restructuring into its own change. Confidence: High.
Cross-PR coordination
The Helm-chart gateway-deployment work rewrites the same gateway/reconciler.go and gateway/config.go this PR restructures, but in a different architectural direction (replacing manifest rendering with a rendered upstream chart) while still consuming the openshell-gateway-db-credentials tenant Secret that this PR's DatabaseReconciler produces. Maintainers should decide the merge order and confirm the ownership boundary for gateway database provisioning under the chart model (in-process control-plane DDL/credential reconcilers, including the new external path, vs. anything the chart renders). Whichever lands first, the other needs non-trivial rework, so this needs an explicit coordination decision rather than a mechanical merge.
Findings Summary (ordered by severity, highest first)
- [Major] External DB/role cleanup is best-effort; failures can orphan databases, roles, and valid credentials on user-owned infra with no surfaced status - Reconciliation / Resource Cleanup (external_db.go:401-455, db_reconciler.go:12-17)
- [Minor] Misleading
INFO dropped external databaselogged even whenDROP DATABASEfailed - Observability (external_db.go:441-444) - [Minor] Password interpolated into DDL statement text (documented lib/pq limit) - Security (external_db.go:298-300)
- [Minor] Large
CLAUDE.mdrewrite bundled into a feature PR - Change Scope (CLAUDE.md)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
errors.IsNotFound handled for 404 scenarios |
Pass |
| No secrets in logs or error messages | Pass |
| Input validated (connection_secret prefix / no namespace separator) | Pass |
| Reconcile pattern used (not create-or-skip) | Pass |
Proper context propagation (no context.TODO()) |
Pass |
| Never silently swallow partial failures | Partial (external Delete is best-effort by design) |
| Conventional commit message | Pass |
| Test assertion changes are non-weakening | Pass (constructor arg only) |
| // must be logged and must not propagate. | ||
| type DatabaseReconciler interface { | ||
| Reconcile(ctx context.Context, dynamicClient dynamic.Interface, clientset kubernetes.Interface, tenantNamespace, gatewayID, rotateAnnotation string) error | ||
| Delete(ctx context.Context, dynamicClient dynamic.Interface, clientset kubernetes.Interface, gatewayID string) |
There was a problem hiding this comment.
[Major] Best-effort Delete orphans objects on user-owned infrastructure for the external provider.
For cnpg/deployment the database lives in-cluster and is reclaimed with the tenant namespace, so a no-op/best-effort delete is fine. For external, the role and database live on a server HyperShell does not own, and DeleteExternalDatabaseResources returns nothing: a failed DROP DATABASE/DROP ROLE, an unreadable admin Secret, or an unreachable server only produces a WARN. The gateway role and its still-valid password can then persist indefinitely on a billed external server with no surfaced status and no retry.
Consider surfacing a durable status/condition (or a retry) for external cleanup failures, or documenting an operator runbook so orphaned roles/databases and live credentials are discoverable and revocable.
| if _, err := db.ExecContext(ctx, fmt.Sprintf("DROP DATABASE %s", pgQuoteIdent(pgName))); err != nil { | ||
| log.Printf("WARN external DB cleanup for gateway %s: DROP DATABASE failed (attempting role drop): %v", gatewayID, err) | ||
| } | ||
| log.Printf("INFO dropped external database %s for gateway %s", pgName, gatewayID) |
There was a problem hiding this comment.
[Minor] Misleading success log on DROP DATABASE failure.
This INFO dropped external database ... line runs unconditionally after the exec above, so it is emitted even when the exec logged WARN ... DROP DATABASE failed. That records success for an operation that failed. Move this INFO into the success branch of the if _, err := db.ExecContext(...) check.
| // the password will appear in the server log; operators should restrict | ||
| // log verbosity or use server-side log redaction accordingly. | ||
| if _, err := db.ExecContext(ctx, | ||
| fmt.Sprintf("CREATE ROLE %s LOGIN PASSWORD '%s'", pgQuoteIdent(pgName), pgQuoteLiteral(password)), |
There was a problem hiding this comment.
[Minor] Password lands in DDL statement text.
The comment correctly documents the lib/pq limitation and the code redacts credentials from error messages, so this is acceptable. Worth mirroring this caveat in the operator-facing external DB spec so operators know to restrict server log_statement verbosity (or enable server-side redaction) on the external PostgreSQL server.
e04610d to
8f676a6
Compare
|
Addressing findings from the round 5 Amber review (commit 8f676a6): [Minor] Misleading INFO log after DROP DATABASE failure - Fixed. The [Major] Best-effort delete can orphan objects on user-owned infrastructure - This is a real design gap: the [Minor] Password interpolated into DDL - This was already documented in-code and in the spec (added in a prior commit). The [Minor] CLAUDE.md rewrite scope - Acknowledged. The CLAUDE.md changes are a direct consequence of this PR's architectural changes to the Control Plane Reconciler Pattern section; the remaining additions restored entries that were accidentally dropped during a rebase. Splitting at this point would create more churn than value. |
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
COMMENT. This is a well-structured feature that adds an external DATABASE_PROVIDER and cleanly extracts a DatabaseReconciler interface (cnpg/deployment/external) out of a 400+ line inline switch. Credential handling is careful (redacted errors, no secret values in logs, idempotent rotation guard), and the change reuses the already-generated connection_secret field so no OpenAPI/proto/migration edits were needed. Findings below are Minor; the main action item is cross-PR merge-order coordination.
Amber Analysis
The database-provisioning refactor is high quality: newDatabaseReconciler() is a clean factory, the empty-provider noopDatabaseReconciler preserves legacy behavior, and the external path validates the admin Secret name (prefix + no namespace separator) before any read. Admin/connection errors are consistently mapped to a closed status vocabulary and never surfaced with credentials attached. Nothing here rises to Blocker/Critical/Major.
Strengths
- Secret redaction is disciplined.
openAdminConn,ReconcileExternalDatabaseResources,RotateExternalDatabaseCredentials, andDeleteExternalDatabaseResourcesall discard raw connection errors and return/log redacted messages;ProbeExternalServermaps errors to a status enum rather than echoing them. - Reconcile, not create-or-skip. The tenant credentials Secret is written with a diff-then-update path (
reflect.DeepEqualguard), and rotation is guarded by thehypershell.redhat.io/last-db-rotationannotation so re-reconciles do not roll the gateway pod. - Input validation for
connection_secretis enforced in both the api-server service (validateExternalConnectionSecret) and the control plane (validateExternalSecretName), sharing the samehypershell-managed-db-prefix constant. - Error handling wraps with
%w, handlesk8serrors.IsNotFound, and avoidspanic().
Test Diff Scrutiny
Two pre-existing tests changed NewManagedDatabaseReconciler(nil, nil, nil, "") to "hypershell". These do not flip any assertion - the assertions (hasCNPG == false, nil-client error) are unchanged; the value is only supplied because handleExternalDatabase now reads controlPlaneNamespace. This passes scrutiny (no removed guarantee, no optional->required contract flip on existing data).
Minor findings
- Kind external-Postgres stand-in omits a fully restricted SecurityContext. The dev/CI Deployment in
scripts/kind/up.shsetsrunAsNonRoot: false(documented) and onlyallowPrivilegeEscalation: false, withoutcapabilities.drop: [ALL]or a seccomp profile. This is dev-only tooling simulating an out-of-cluster RDS, so it is not a production spec, but adding the cap drop + seccomp would keep it consistent with the project container-security convention at near-zero cost. mapConnErrorToStatusclassifies by substring matching of driver error text ("tls","connection refused",28p01, ...). This is inherently fragile across driver/locale changes; consider preferring typed checks (net.Error,*pq.ErrorSQLSTATE codes) where available. Non-blocking - the fallback status is safe.pgQuoteLiteralescapes only single quotes. Safe today because every interpolated password is hex-encoded ([0-9a-f]), and the limitation of interpolating passwords into DDL is already documented in-code. Worth a one-line note that this quoting must never be reused for arbitrary user input.
Cross-PR coordination
The PR that adopts the upstream OpenShell Helm chart for gateway deployments (#194) and this PR both restructure the same ReconcileGateway flow and the same ReconcileOpts config struct in components/control-plane/internal/gateway/, and they take divergent approaches to the database step: this PR replaces the inline switch opts.DatabaseProvider with a DatabaseReconciler interface + newDatabaseReconciler() factory, while that PR keeps the inline switch and additionally makes gateway provisioning depend on the openshell-gateway-db-credentials Secret existing before the Helm install (the chart consumes it via server.externalDbSecret, and its documented ordering is "provision DB in step 2, Helm install in step 5"). That Secret is exactly what this PR's provider reconcilers now write. Maintainers need to decide a merge order and confirm that, after both land, the factory-based provisioning still runs before the Helm install and still emits openshell-gateway-db-credentials in the expected shape (host/port/dbname/user/password/uri). This is a design + ordering decision, not a plain text merge conflict.
Findings Summary (ordered by severity, highest first):
- [Minor] Kind external-Postgres stand-in Deployment lacks
capabilities.drop: [ALL]/ seccomp - Container Security (dev tooling) (scripts/kind/up.sh) - [Minor]
mapConnErrorToStatusrelies on error-string substring matching - Robustness (external_db.goL154) - [Minor]
pgQuoteLiteralescapes only single quotes; safe only because inputs are hex - Defense in depth (external_db.goL543)
Convention Checklist:
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
errors.IsNotFound handled for 404s |
Pass |
| No secrets in logs or error messages | Pass |
| Input validated (secret name prefix/format) | Pass |
| Reconcile pattern (not create-or-skip) | Pass |
| Restricted SecurityContext on pod specs | Minor (dev stand-in only) |
| Conventional commit message | Pass |
| OpenAPI/proto/generated files not hand-edited | Pass |
| Test Diff Scrutiny (no silent contract flips) | Pass |
…es for external DB - externalDatabaseReconciler.Delete pre-checks the admin secret before attempting cleanup: if it is missing or invalid the error is terminal (retrying without operator action cannot help), so it logs at ERROR and returns nil so in-cluster RBAC cleanup is not blocked. Connection/DDL errors remain transient and are propagated for reconcile retry. - DeleteGatewayResources now returns the transient error from dbReconciler.Delete so the delete-reconcile loop retries until cleanup succeeds. - Document that out-of-band server-side password drift is out of scope (use the rotate annotation to force re-sync). - Update DatabaseReconciler.Delete comment to reflect transient/terminal semantics. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
rh-amarin
left a comment
rh-amarin
left a comment
There was a problem hiding this comment.
Thanks for the round 15 review. All findings addressed in cc64d67:
[Major] Transient vs terminal delete distinction - externalDatabaseReconciler.Delete now pre-checks the admin secret before attempting cleanup. Terminal failures (secret missing/invalid) log ERROR and return nil (in-cluster cleanup proceeds). Transient failures (connect/DDL error) are returned for reconcile retry. DeleteGatewayResources propagates the transient error.
[Minor] Password in DDL - Accepted; already documented in code and spec. SCRAM pre-hashing is a future enhancement.
[Minor] Probe retry - The watcher reseeds all ManagedDatabases on gRPC reconnect, re-triggering probing. Periodic world-sync is a future enhancement, out of scope.
[Minor] Password drift - Added explicit comment: out-of-band drift is out of scope; operators should use the rotate annotation to force re-sync.
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
This PR adds a third external DATABASE_PROVIDER cleanly: it extracts per-provider database logic behind a new DatabaseReconciler interface, adds in-process PostgreSQL DDL provisioning/rotation/cleanup for user-managed servers, and backs it with a thorough spec, unit tests, and an added CI leg. The implementation is careful about secret redaction, input validation (reserved Secret prefix + namespace isolation), error wrapping, and transient-vs-terminal delete classification; findings are limited to minor hardening notes plus one cross-PR coordination item.
Strengths
- Clean refactor: the inline
switch opts.DatabaseProviderinReconcileGatewayis replaced bynewDatabaseReconciler()+ aDatabaseReconcilerinterface (cnpg_db.go,deployment_db.go,external_db.go), with CNPG/deployment behavior preserved on move. - Credentials are consistently redacted:
openAdminConnerrors are never wrapped up to callers verbatim; probe/reconcile/rotate/delete all return "(credentials redacted)" messages, and DDL identifiers/literals go throughpgQuoteIdent/pgQuoteLiteral. - The
hypershell-managed-db-reserved-prefix rule plus control-plane-namespace-only Secret reads form a real security boundary against referencing arbitrary Secrets, validated at both the API server (managedDatabases/service.go) and control plane. - Delete path distinguishes transient (server unreachable / DDL failure -> returned for retry) from terminal (admin Secret unreadable -> logged ERROR, returns nil so in-cluster RBAC cleanup proceeds), matching the control-plane conventions.
Findings
Minor
-
TLS default
requiredoes not authenticate the server (components/control-plane/internal/gateway/external_db.go:128-131). The admin connection (carryingCREATEDB/CREATEROLEcredentials) defaults tosslmode=require, which encrypts but does not verify the server certificate, and the WARN log only fires forsslmode=disable. This is a documented, intentional v1 trade-off (openshell-gateway-database-external.spec.mdtracksverify-fullas a follow-up), so no change is required to merge; consider extending the WARN to nudge operators towardverify-fullfor production external servers. -
Role password is interpolated into DDL text (
external_db.go:334-341).CREATE ROLE/ALTER ROLEembed the (hex, non-injectable) password in statement text, so it can appear in server logs whenlog_statement=all. This is already called out in the code comment and is inherent tolib/pq(no parameter binding for role DDL); flagging only so the operator-facing log-hardening guidance stays visible. No action required for this PR. -
CLAUDE.md restructuring bundled with the feature (
CLAUDE.md, +133/-57). The largely-unrelated top-to-bottom rewrite (heading rename, reordered sections, new architecture/commands content) inflates the diff and complicates review of the functional change. Consider splitting doc reorganization from feature PRs in future; the new "Adding a new DATABASE_PROVIDER" guidance is accurate and useful.
Cross-PR coordination
PR #194 (Helm chart adoption for gateway deployments) and this PR both restructure the same ReconcileGateway function and its database-provider handling in mutually incompatible ways. This PR removes the inline switch opts.DatabaseProvider block and the reconcileCNPGDatabaseResources/rotateCNPGDatabaseCredentials/deployment helpers from reconciler.go, relocating them behind the new DatabaseReconciler interface; PR #194 keeps that inline provider switch, changes the ReconcileGateway signature (adds a helmClient, drops the manifests argument), and relocates the same CNPG helpers differently. Maintainers should decide a merge order and the target structure (the DatabaseReconciler abstraction vs. the Helm-based inline flow), and the PR that merges second must be rebased onto the first's structure - this is a design/change-order decision, not a mechanical merge conflict.
Findings Summary (ordered by severity, highest first):
- [Minor] TLS admin default
requiredoes not verify the server certificate; WARN only coversdisable- Security (external_db.go:128-131) - [Minor] Role password interpolated into
CREATE ROLE/ALTER ROLEDDL text (server-log exposure withlog_statement=all) - Security (external_db.go:334-341) - [Minor] Unrelated CLAUDE.md restructuring bundled into the feature PR - Scope / Reviewability (CLAUDE.md)
Convention Checklist:
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
| No secrets in logs or error messages | Pass |
| Input validated (Secret name / prefix / namespace) | Pass |
| Reconcile pattern (update-or-create) used | Pass |
| Status updated on error paths (ManagedDatabase probe) | Pass |
| Proper context propagation (delete uses bounded ctx) | Pass |
| Test diff scrutiny (no flipped assertions) | Pass |
| Image references consistent across manifests | N/A |
| Conventional commit message | Pass |
| sslmode = "require" | ||
| } | ||
| if sslmode == "disable" { | ||
| log.Printf("WARN external DB secret %s: sslmode=disable is insecure; use require or verify-full for production", secretName) |
There was a problem hiding this comment.
[Minor] Security hardening. The admin connection defaults to sslmode=require, which encrypts but does not authenticate the server certificate, yet the WARN here only triggers for sslmode=disable. Since this connection carries CREATEDB/CREATEROLE admin credentials to an out-of-cluster server, consider also emitting a hint toward sslmode=verify-full (with sslrootcert) when a weaker mode is in use. This is a documented v1 trade-off (the external spec tracks verify-full as a follow-up), so it is not blocking.
There was a problem hiding this comment.
Fixed in 848cb02: the log now uses a switch on sslmode. disable keeps the original WARN. require, allow, and prefer get a new WARN noting they encrypt but do not verify the server certificate, and recommend verify-full with sslrootcert. verify-ca and verify-full are silent (correct posture).
| // the password will appear in the server log; operators should restrict | ||
| // log verbosity or use server-side log redaction accordingly. | ||
| if _, err := db.ExecContext(ctx, | ||
| fmt.Sprintf("CREATE ROLE %s LOGIN PASSWORD '%s'", pgQuoteIdent(pgName), pgQuoteLiteral(password)), |
There was a problem hiding this comment.
[Minor] Informational. The generated password is interpolated into CREATE ROLE ... PASSWORD '...' (and ALTER ROLE) statement text, so it can surface in the external server's logs when log_statement=all. This is already noted in the surrounding comment and is inherent to lib/pq (role DDL cannot be parameterized); keeping the operator-facing log-hardening guidance visible is sufficient. No change required for this PR.
There was a problem hiding this comment.
Acknowledged (accepted). The code comment and specs/platform/openshell-gateway-database-external.spec.md (Prerequisites section) document the risk and mitigation. No change needed.
…er modes The admin connection to an external PostgreSQL server carries CREATEDB/CREATEROLE credentials. sslmode=require (the default) encrypts but does not verify the server certificate, which is insufficient for a production external server. Extend the WARN log to cover require/allow/prefer so operators are nudged toward verify-full with sslrootcert in production deployments. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
rh-amarin
left a comment
rh-amarin
left a comment
There was a problem hiding this comment.
Thanks for the round 16 review. Findings addressed in 848cb02:
[Minor] TLS WARN - Extended to a switch on sslmode: disable keeps the existing WARN; require/allow/prefer now also log WARN noting they encrypt but do not authenticate the server certificate, recommending verify-full with sslrootcert. verify-ca/verify-full are silent.
[Minor] Password in DDL - Accepted as documented. No change.
[Minor] CLAUDE.md scope - Acknowledged. Future doc-only PRs will be separate.
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
The external-database provider is a careful, well-factored addition: credentials are redacted in every error/log path, DDL identifiers and literals are quoted, connection errors are mapped to a closed status vocabulary, and delete is split into transient (retry) vs terminal (log-and-continue) failures. I am not requesting changes for the code in this PR, but there is a material cross-PR design/ordering concern (see Cross-PR coordination) plus a few minor observations worth addressing before or shortly after merge.
What I checked
- Secret handling: admin/tenant passwords never appear in logs or returned errors;
openAdminConnfailures are discarded and replaced with generic "credentials redacted" messages;ProbeExternalServer/mapConnErrorToStatusmap raw driver errors to a fixed status vocabulary. Good. - Input validation:
validateExternalSecretName/validateExternalConnectionSecretenforce the reservedhypershell-managed-db-prefix and rejectnamespace/namereferences (a real security boundary, documented innaming-multitenancy.spec.md). - SQL safety:
pgQuoteIdent/pgQuoteLiteralquote identifiers and literals; interpolated passwords are hex-only. Thelog_statement=allcaveat is documented in-code. - Reconcile semantics:
DatabaseReconcilerinterface +newDatabaseReconcilerfactory cleanly replaces the old provider switch; CNPG/deployment behavior is preserved by the refactor (DeploymentReadinessmoved, not lost). Delete returns transient errors for retry and swallows terminal ones so gateway finalization is not stranded. - Error handling:
errors.IsNotFoundhandled; errors wrapped with context; nopanic()in production paths. - Migration/optional->required:
connection_secretbecomes required only for the brand-newexternalprovider, and provider cannot be changed on an existing record, so no backfill is needed. No pre-existing test assertion was flipped from accept->reject; the two changed test lines only swap a namespace argument from""to"hypershell"with assertions unchanged.
Findings
Minor
- Observability -
openAdminConn(external_db.go:175) drops the underlyingsql.Openerror with a barefmt.Errorf(...)(no%w).sql.Openerrors carry no credentials, so wrapping them would preserve debuggability without leaking secrets. Considerfmt.Errorf("open admin connection: %w", err). - Maintainability / security boundary - the reserved prefix
hypershell-managed-db-is duplicated asexternalSecretPrefixin two modules (external_db.go:68 and managedDatabases/service.go:22). Since this string is a security boundary, drift between the two would silently weaken enforcement on one side. Cross-referencenaming-multitenancy.spec.mdin both, or centralize. - Container security - the CI stand-in PostgreSQL in scripts/kind/up.sh (~L330) runs with
runAsNonRoot: falseand nocapabilities.drop: [ALL], deviating from the restricted-SecurityContext convention. The in-line rationale (test-only stand-in for a cloud-managed server,postgres:15needs root for data-dir init) is reasonable and it does setseccompProfile: RuntimeDefaultandallowPrivilegeEscalation: false; flagging only so the exception stays test-scoped and never reused for a HyperShell-managed pod.
Cross-PR coordination
Two items need maintainer decision or a defined merge order.
- #194 (adopt upstream OpenShell Helm chart for gateway deployments) competes with this PR's redesign of the same gateway reconcile / database-provisioning flow. This PR replaces the provider
switchinReconcileGatewaywith aDatabaseReconcilerinterface +newDatabaseReconcilerfactory and addsExternalDBtoReconcileOpts; #194 rewritesReconcileGateway/ReconcileOptsto install gateways via Helm and removes that same provider switch, wiring the DB credentials Secret (openshell-gateway-db-credentials) into the chart viaserver.externalDbSecret. The external provider provisions and writes exactly that Secret in-process before deployment. Maintainers should decide the merge order and how the new externalDatabaseReconciler(in-process DDL + tenant Secret) integrates with the Helm-based deployment path, so one design does not silently drop the other's DB wiring. - #150 (build LOCAL_IMAGES from working tree by default; add BUILD_SOURCE=baseline) is a prerequisite this PR assumes. This PR's CI external leg depends on the "baseline image predates external support, image swap carries the new DATABASE_PROVIDER env" behavior, edits the same
LOCAL_IMAGESblock inscripts/kind/up.sh(addingKIND_SKIP_BUILD), and documentsBUILD_SOURCE=baseline/KIND_SKIP_BUILDin CLAUDE.md and the Makefile help. Those build semantics do not exist onmainwithout #150. The owner/maintainers should confirm #150 merges first (or fold the sharedup.sh/Makefile edits together) so this PR's docs and external e2e leg are not describing behavior that is absent.
Findings Summary (ordered by severity, highest first)
- [Minor] Underlying
sql.Openerror dropped without%w- Observability (external_db.go:175) - [Minor] Security-boundary prefix constant duplicated across two modules - Maintainability (external_db.go:68, managedDatabases/service.go:22)
- [Minor] CI stand-in Postgres deviates from restricted SecurityContext - Container Security (scripts/kind/up.sh:330)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass (one bare error, see finding 1) |
errors.IsNotFound handled for 404 scenarios |
Pass |
| No secrets in logs or error messages | Pass |
| Input validated (K8s DNS labels / secret-name rules) | Pass |
| Reconcile pattern used (not create-or-skip) | Pass |
| Restricted SecurityContext on pod specs | Pass in production; test-only deviation (finding 3) |
| Optional->required change has migration/fallback | Pass (new provider only; provider immutable) |
| Test diff scrutiny (no silently flipped assertions) | Pass |
| Conventional commit messages | Pass |
| func openAdminConn(ctx context.Context, params *externalAdminParams) (*sql.DB, error) { | ||
| db, err := sql.Open("postgres", params.dsn()) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("open admin connection: driver init failed") |
There was a problem hiding this comment.
[Minor] Observability - this drops the underlying sql.Open error. sql.Open errors carry no credentials, so wrapping them keeps debuggability without leaking secrets:
return nil, fmt.Errorf("open admin connection: %w", err)(The redaction on PingContext/DDL paths further down is correct and should stay.)
There was a problem hiding this comment.
Fixed in 8f1c30b: changed to fmt.Errorf("open admin connection: %w", err). Driver registration errors carry no credentials (they happen before any connection is attempted), so wrapping is safe and preserves the underlying error for debugging.
| sslrootcert string // optional; enables verify-full when sslmode=verify-full | ||
| } | ||
|
|
||
| const externalSecretPrefix = "hypershell-managed-db-" |
There was a problem hiding this comment.
[Minor] Maintainability / security boundary - the reserved prefix hypershell-managed-db- is also declared as externalSecretPrefix in components/api-server/plugins/managedDatabases/service.go:22. This string is a security boundary (it is what stops an API-level reference from naming an unrelated Secret like hypershell-db-app), so silent drift between the two modules would weaken enforcement on one side. Cross-reference naming-multitenancy.spec.md in both, or centralize the constant.
There was a problem hiding this comment.
Added cross-reference comments to both enforcement points in 8f1c30b. Both externalSecretPrefix constants now have a comment noting the security boundary, the cross-module dual enforcement, and a pointer to naming-multitenancy.spec.md. Centralizing across two separate Go modules would require an interface-only shared package; the comment approach keeps the duplication visible without adding a new dependency.
| # seccompProfile: RuntimeDefault is applied to restrict syscalls within | ||
| # the permitted capability set. | ||
| securityContext: | ||
| runAsNonRoot: false |
There was a problem hiding this comment.
[Minor] Container security - this stand-in Postgres runs with runAsNonRoot: false and no capabilities.drop: [ALL], deviating from the restricted-SecurityContext convention. The rationale (test-only stand-in for a cloud-managed server; postgres:15 needs root for data-dir init) is reasonable and you do set seccompProfile: RuntimeDefault + allowPrivilegeEscalation: false. Flagging only to keep this exception test-scoped - it must never be reused for a HyperShell-managed pod spec.
There was a problem hiding this comment.
Acknowledged - the in-line rationale and security mitigations (seccompProfile: RuntimeDefault, allowPrivilegeEscalation: false) are already documented in up.sh. The exception is intentionally test-scoped and will not be reused for any HyperShell-managed pod spec.
…y prefix - openAdminConn: wrap the sql.Open error with %w so the driver-level failure is preserved for debugging; sql.Open errors carry no credentials so wrapping is safe - Add cross-reference comments to externalSecretPrefix in both the control-plane and API-server so the security boundary is visible in both enforcement points and drift is easier to detect (see naming-multitenancy.spec.md) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
rh-amarin
left a comment
rh-amarin
left a comment
There was a problem hiding this comment.
Thanks for the round 17 review. Findings addressed in 8f1c30b:
[Minor] sql.Open error - Changed to fmt.Errorf("open admin connection: %w", err). Driver registration errors carry no credentials, so wrapping is safe.
[Minor] Prefix constant duplication - Added cross-reference comments to both externalSecretPrefix constants (control-plane and API server), each pointing to the peer enforcement point and naming-multitenancy.spec.md. Centralizing across two separate Go modules would require a new shared package dependency, which is heavier than commenting.
[Minor] CI stand-in SecurityContext - Acknowledged. The rationale is already documented in up.sh and the exception is test-scoped only.
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
This PR adds a third external DATABASE_PROVIDER that provisions per-gateway roles/databases on a user-managed PostgreSQL server, refactors the provider switch into a clean DatabaseReconciler interface, and extends the Kind E2E matrix. The core feature is security-conscious (redacted credentials, reserved-prefix admin Secret boundary, idempotent DDL) and well documented; my concerns are an unrelated wholesale CLAUDE.md rewrite bundled into the change, a delete-path edge case, and cross-PR coordination on the gateway reconciler and E2E driver structure. Overall: sound feature, non-blocking findings.
Amber Analysis
The external-DB reconciler keeps credentials out of logs/errors, reads the admin Secret only from the control-plane namespace, enforces the hypershell-managed-db- prefix on both the API server and control plane, and mirrors the CNPG rotation idempotency guard. DDL identifier/literal quoting is confined to internally-derived names and hex passwords, and the log_statement server-log caveat is documented as an operator prerequisite. The findings below are improvements and coordination items, not correctness blockers.
Major
1. Unrelated wholesale CLAUDE.md rewrite bundled into a DB feature (CLAUDE.md, +133/-57).
The root CLAUDE.md is rewritten from the concise project overview into a long "guidance to Claude Code" document (new Commands, Go-module, plugin-system sections). This is unrelated to external database provisioning, enlarges the review surface, and risks clobbering deliberate structure and colliding with other in-flight edits to the same file. Recommend extracting the CLAUDE.md rewrite into its own PR so the DB change can be reviewed and reverted independently.
Minor
2. Delete can strand gateway finalization when the external server is permanently unreachable (external_db.go ~L480-L483, propagated in reconciler/reconciler.go DeleteGatewayResources).
A connect failure in DeleteExternalDatabaseResources is treated as transient and propagated, so the delete-reconcile loop retries indefinitely. For a decommissioned server the gateway can never be finalized. The documented escape hatch (remove the admin Secret to make cleanup terminal and return nil) works but is non-obvious; consider a bounded retry/emit-and-continue after N attempts, or surface the required operator action in the gateway status, so a dead external server does not block deletion silently.
3. Redacted connect errors drop the wrapped cause (external_db.go L322-L324, L560).
fmt.Errorf("connect to external server: connection failed (credentials redacted)") intentionally omits %w. sql.Open/Ping errors on the admin DSN do not carry the password, and mapConnErrorToStatus already classifies them safely; wrapping the classified status (not the raw DSN) would preserve debuggability without leaking secrets.
4. Provisioning DDL path has no unit coverage.
external_db_test.go covers DSN formatting and delete early-exit, but ReconcileExternalDatabaseResources/RotateExternalDatabaseCredentials DDL logic is only exercised by the E2E external leg. That is acceptable given the E2E job, but a sqlmock-backed unit test for the create-role/create-db/grant sequence and the password-reuse branch would guard against regressions without a live server.
Cross-PR coordination
The following require maintainer coordination before or at merge:
-
#194 (adopt upstream OpenShell Helm chart for gateway deployments): Both PRs restructure the same
components/control-plane/internal/gateway/reconciler.goandconfig.go. #194 replaces static-manifest gateway deployment with runtime Helm-chart installation (newhelm_deploy.go/values.go/chart.go, Helm binary in the Dockerfile, manifest deletions), while this PR introduces theDatabaseReconcilerinterface and in-process external-DB DDL plus theopenshell-gateway-db-credentialsSecret it writes. Maintainers must decide the merge order and how per-gateway external-DB provisioning and its credentials Secret integrate into a Helm-managed gateway deploy (Helm-owned vs. reconciler-owned), so the second PR to land re-homes its logic rather than silently reverting the other's architecture. -
#244 (unify OpenShift driver with Kind, dynamic namespace GC timing): #244 adds an
effective_database_provider()/cutover path in the cluster drivers that validatesDATABASE_PROVIDERagainst onlycnpg/deploymentand restructures the E2E driver scripts. This PR addsexternalas a third provider value and extends the Kind matrix andup.sh/e2e-openshell.sh/seed.shfor the external leg. Landing #244 first would cause its driver to rejectexternal, and this PR's inline external-leg seeding would need re-homing into the unified driver. Coordinate the provider vocabulary and merge order so theexternalleg lands in the new driver structure.
Findings Summary (ordered by severity, highest first):
- [Major] Unrelated wholesale
CLAUDE.mdrewrite bundled into a DB feature PR - Scope / Change Hygiene (CLAUDE.md) - [Minor] External-DB delete can strand gateway finalization for a permanently unreachable server - Reconciliation Robustness (external_db.go L480-L483)
- [Minor] Redacted connect errors omit the wrapped (secret-free) cause - Error Wrapping / Observability (external_db.go L322-L324, L560)
- [Minor] Provisioning/rotation DDL path lacks unit coverage - Testing (external_db_test.go)
Convention Checklist:
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass (redacted connect paths intentionally drop %w) |
errors.IsNotFound handled for 404 scenarios |
Pass |
| No secrets in logs or error messages | Pass |
| Input validated (reserved Secret prefix, no namespace slash) | Pass |
| Reconcile, not create-or-skip | Pass (documented password-reuse is deliberate) |
| Conventional commit messages | Pass |
OpenAPI/proto not manually edited (reuses existing connection_secret) |
Pass |
| Test diff scrutiny (no flipped assertions) | Pass |
| @@ -1,19 +1,16 @@ | |||
| # HyperShell | |||
| # CLAUDE.md | |||
There was a problem hiding this comment.
[Major] Unrelated scope: wholesale CLAUDE.md rewrite. This replaces the concise project overview with a long "guidance to Claude Code" document (new Commands / Go-module / plugin-system sections) that is unrelated to external database provisioning. It enlarges the review surface and risks clobbering deliberate structure and colliding with other in-flight edits. Please split the CLAUDE.md rewrite into its own PR so the DB feature can be reviewed and reverted independently.
There was a problem hiding this comment.
Fixed in 4247075: reverted CLAUDE.md to the main baseline and removed the now-stale whitelist entry for the em dash at the former line 201. The Commands / Architecture / Key Files / Domain Model / Conventions additions are real improvements but belong in a separate PR so this one stays focused and independently revertable.
| db, err := openAdminConn(ctx, params) | ||
| if err != nil { | ||
| return fmt.Errorf("external DB cleanup for gateway %s: cannot connect to server (credentials redacted)", gatewayID) | ||
| } |
There was a problem hiding this comment.
[Minor] Delete can strand gateway finalization for a permanently unreachable server. A connect failure here is treated as transient and returned, so DeleteGatewayResources propagates it and the delete-reconcile loop retries forever. For a decommissioned external server the gateway can never be finalized. The documented escape hatch (remove the admin Secret so cleanup becomes terminal and returns nil) works but is non-obvious - consider a bounded retry / emit-and-continue after N attempts, or surfacing the required operator action in gateway status, so a dead server does not block deletion silently.
There was a problem hiding this comment.
Addressed in 4247075: the cleanup connect-error message now includes the mapConnErrorToStatus() classification (e.g. 'Failed: unreachable') AND explicitly names the escape hatch: 'to unblock finalization on a decommissioned server, remove admin Secret %q'. This makes the operator action visible in logs without requiring architectural retry-counter changes. The pre-check in externalDatabaseReconciler.Delete (missing/invalid secret = terminal, return nil) remains the documented path for a permanently-gone server where the admin Secret itself needs removal.
| // Open admin connection and issue idempotent DDL. | ||
| db, err := openAdminConn(ctx, params) | ||
| if err != nil { | ||
| return fmt.Errorf("connect to external server: connection failed (credentials redacted)") |
There was a problem hiding this comment.
[Minor] Redacted connect error drops the wrapped cause. This intentionally omits %w, but sql.Open/Ping errors on the admin DSN do not carry the password and mapConnErrorToStatus already classifies them safely. Wrapping the classified status (not the raw DSN) would preserve debuggability without leaking secrets. Same pattern at the rotation connect path (~L560).
There was a problem hiding this comment.
Fixed in 4247075: all three redacted connect-error messages (reconcile, cleanup, rotation paths) now include mapConnErrorToStatus(err) - e.g. 'connect to external server (Failed: auth_failed): connection failed (credentials redacted)'. The DSN and password are still excluded from the error string, but the operator now sees the classified status in logs and error propagation.
…larity - Revert CLAUDE.md to main: the wholesale rewrite was unrelated to the external DB feature and should be a separate PR; remove now-stale whitelist entry for the em dash at the removed line 201 - Include mapConnErrorToStatus() classification in redacted connect error messages (reconcile, cleanup, rotation paths) so operators see "auth_failed" / "unreachable" / "tls_failed" rather than a generic redacted message - Add explicit escape-hatch guidance to the cleanup connect error: if the external server is permanently decommissioned, removing the admin Secret makes cleanup terminal and unblocks gateway finalization Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
rh-amarin
left a comment
rh-amarin
left a comment
There was a problem hiding this comment.
Thanks for round 18 review. Addressed in 4247075:
[Major] CLAUDE.md rewrite - Reverted to main baseline; also removed the now-stale whitelist entry for the em dash that referenced the former line 201. The Commands/Architecture/Key Files/Domain Model/Conventions additions are real improvements and will go in a separate, independently revertable PR.
[Minor] Delete stranding - Enhanced the cleanup connect-error message to include mapConnErrorToStatus() classification and explicitly name the escape hatch: 'to unblock finalization on a decommissioned server, remove admin Secret '. This makes the operator action visible in logs without needing architectural retry-counter changes.
[Minor] Redacted connect error - All three connect-error messages (reconcile, cleanup, rotation) now include the mapConnErrorToStatus() classification so operators see 'Failed: auth_failed' / 'Failed: unreachable' etc. rather than just '(credentials redacted)'.
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
This PR adds a well-designed external DATABASE_PROVIDER that registers a pre-existing PostgreSQL server and provisions a per-gateway database and login role via in-process DDL, with credential redaction, a reserved-Secret-name security boundary, and SQL-identifier/literal quoting all handled carefully. The database-provider switch is cleanly refactored into a DatabaseReconciler interface, the specs are thorough, and test coverage for the error-mapping and validation helpers is good; my only substantive concern is an unrelated convention removal bundled into the PR, plus one cross-PR coordination item.
Amber Assessment
Overall this is high-quality work. Secret handling is disciplined: raw connection errors are never logged or returned (only closed-vocabulary status strings via mapConnErrorToStatus), the admin Secret is read only from the control-plane namespace, the hypershell-managed-db- prefix is enforced on both the API-server and control-plane sides as a documented security boundary, and passwords are hex-encoded and quoted before DDL interpolation with an explicit comment restricting reuse of pgQuoteLiteral. Error wrapping, IsNotFound handling, idempotent DDL, and transient-vs-terminal delete semantics are all sound, and no panic() or new pod specs are introduced.
The findings below are non-blocking; I'm submitting as COMMENT.
Major
- Out-of-scope removal of a documented, still-enforced convention. This PR deletes the
- **No em dashes**: ...line fromCLAUDE.mdand drops the matching entry in.forbidden-terms-whitelist.json.CLAUDE.mdis the authoritative conventions source, yet the em-dash rule remains enforced byscripts/check_forbidden_terms.py(and documented inscripts/README.md). Silently removing the documentation while enforcement stays active will confuse contributors who hit the hook with no documented rule. This change is unrelated to external database provisioning. Please either restore the line (re-adding the whitelist entry) or split the convention change into its own PR with an explicit rationale so maintainers can decide it on its own merits. (Confidence: High that it's out of scope; Medium on whether the removal was intentional.)
Minor
mapConnErrorToStatusclassification order. The TLS substring checks ("tls","ssl","certificate","x509") run before the typedpq.ErrorSQLSTATE auth check, so an authentication error whose message happens to contain "SSL" could be reported astls_failedinstead ofauth_failed. Since these strings feed an observability-only status, impact is low, but moving the typed*pq.Errorcheck ahead of the TLS string matching would make classification deterministic. (Confidence: Medium.)
Cross-PR coordination
The external database provider restructures the shared gateway reconcile surface: it replaces the inline DatabaseProvider switch in ReconcileGateway/DeleteGatewayResources with a DatabaseReconciler interface, adds ExternalDB to ReconcileOpts and a provider constant to internal/config/config.go, and makes the per-gateway openshell-gateway-db-credentials Secret the integration point that the gateway workload consumes.
- #194 rebuilds the same gateway reconcile path by deploying gateways through the upstream OpenShell Helm chart (Helm values mapping,
deployGatewayViaHelm/Uninstall), and it changes ownership/cleanup of theopenshell-gateway-db-credentialsSecret while editing the sameReconcileOptsstruct andinternal/config/config.go. These two directions are not independently mergeable: maintainers must decide the integration order and how the external provider's per-gateway credential Secret is produced/owned relative to the Helm release (chart-owned values vs control-plane-provisioned Secret), and whether theDatabaseReconcilerabstraction lands before or after the Helm migration. This is a design/plan decision, not a mechanical merge conflict.
Findings Summary (ordered by severity, highest first):
- [Major] Out-of-scope deletion of the "No em dashes" convention from the authoritative
CLAUDE.mdwhile the hook still enforces it - Conventions / Scope (CLAUDE.md L125, .forbidden-terms-whitelist.json) - [Minor]
mapConnErrorToStatusmatches TLS substrings before the typedpqauth-code check, risking misclassification of auth errors - Correctness / Observability (external_db.go L210)
Convention Checklist:
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
errors.IsNotFound handled for 404 scenarios |
Pass |
| No secrets in logs or error messages | Pass |
| Input validated (Secret-name prefix / DDL identifier quoting) | Pass |
| Reconcile pattern used (not create-or-skip) | Pass |
| OpenAPI/proto client not manually edited | Pass |
| Conventional commit messages | Pass |
| Authoritative convention docs kept intact | Fail |
| - **Separate configuration from code**: Config changes must not require code changes | ||
| - **PatternFly 6 for web UI**: Reuse PatternFly and canonical shared components; do not create duplicate UI components | ||
| - **Narrow hexagonal UI boundary**: Put application workflows and external effects behind application-owned ports; keep React, TanStack Query, Fastify, generated SDKs, and infrastructure outside | ||
| - **Domain probes for UI observability**: Publish typed workflow and dependency facts through a fan-out port; no raw console or direct telemetry calls in production browser/BFF code |
There was a problem hiding this comment.
This hunk removes the - **No em dashes**: ... convention line (and its .forbidden-terms-whitelist.json entry). CLAUDE.md is the authoritative conventions source, but scripts/check_forbidden_terms.py still rejects em dashes, so the rule stays enforced while its documentation disappears - contributors will hit the hook with no documented rule to point to. This is unrelated to external DB provisioning. Please restore the line (re-adding the whitelist entry) or move the convention change to its own PR with a rationale.
There was a problem hiding this comment.
Fixed in 3d16409: added the 'No em dashes' convention line back to the Critical Conventions section of CLAUDE.md (line 125) with the whitelist entry re-added at that line. The hook continues to enforce it; now the documentation is also present.
| return ExternalDBStatusUnreachable | ||
| } | ||
| // TLS failures | ||
| if strings.Contains(lower, "tls") || |
There was a problem hiding this comment.
These TLS substring checks (tls/ssl/certificate/x509) run before the typed *pq.Error SQLSTATE auth check a few lines below. An authentication failure whose message contains "SSL" would be reported as tls_failed instead of auth_failed. Impact is limited (observability-only status), but moving the typed pq.Error auth check ahead of the TLS string matching makes classification deterministic.
There was a problem hiding this comment.
Fixed in 3d16409: moved the typed *pq.Error SQLSTATE check (28P01/28000) to run before the TLS substring matching block. A pq.Error with code 28P01 is now always classified auth_failed even when its message contains 'ssl'. Added a test case covering this (pq auth 28P01 with ssl message) to pin the behavior.
…ering - Restore the 'No em dashes' convention rule to CLAUDE.md (was dropped in the round 18 revert); add its whitelist entry at line 125 so the pre-commit hook continues to enforce the rule and its documentation stays in sync - Move typed *pq.Error SQLSTATE check before TLS substring matching in mapConnErrorToStatus: a 28P01/28000 pq.Error is now always classified auth_failed even when its message incidentally contains 'ssl' (e.g. SSL-wrapped authentication rejections on strict-TLS servers) - Add test case covering the pq.Error-with-ssl-message path Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
rh-amarin
left a comment
rh-amarin
left a comment
There was a problem hiding this comment.
Thanks for round 19 review (commit 3d16409):
[Minor] CLAUDE.md missing em dash convention - Restored the 'No em dashes' convention line at line 125, and re-added the whitelist entry pointing to that line. Documentation and enforcement are now in sync.
[Minor] TLS/auth classification ordering - Moved the typed *pq.Error SQLSTATE check (28P01/28000) before the TLS substring block. Added a test case covering the pq.Error-with-ssl-message path to pin the correct behavior.
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
This PR adds a well-structured external database provider: a clean DatabaseReconciler interface, credential/status vocabulary that is redacted end-to-end, defense-in-depth connection_secret prefix validation in both the API server and control plane, and solid unit coverage. I am approving in spirit with one substantive TLS finding to confirm and a couple of minor notes; there are no blockers.
Amber Analysis
The refactor that lifts CNPG/deployment provisioning out of reconciler.go into per-provider files behind a DatabaseReconciler interface is a genuine readability win, and the kubernetes.Interface switch makes the code testable. Secret handling is careful: connection errors are mapped to a closed status vocabulary and never surfaced raw, DDL password interpolation is confined to hex-only values with a documented rationale, and the tenant secret write follows a real reconcile (create-or-update) pattern.
Findings
[Major] Tenant verify-full / sslrootcert propagation likely breaks the gateway DB connection - Spec Consistency / Correctness
When the admin Secret uses sslmode=verify-full with sslrootcert, ReconcileExternalDatabaseResources mirrors that mode and the admin-side sslrootcert filesystem path into the tenant credentials Secret and the tenant uri (external_db.go:397-420, and the rotation path 581-582). That path is valid only inside the control-plane pod; the gateway workload runs in a different pod with no CA mounted there, so the gateway's DB connection would reference a nonexistent path and fail. This also disagrees with the tenant-secret contract in openshell-gateway-database-external.spec.md:508-509, which describes tenant sslrootcert as PEM content, not a path. The spec itself notes (516-518) that distributing the CA to the gateway workload is a follow-up and that v1 MAY ship with require as the enforced default. Suggested fix: until CA delivery to the gateway workload exists, keep the tenant connection at require (do not propagate verify-full/the path), or copy the CA PEM content and reference it at a path the gateway pod actually mounts. Confidence: Medium.
[Minor] Password interpolated into CREATE ROLE / ALTER ROLE DDL - Security
Because lib/pq cannot parameterize DDL, the generated password is interpolated into the statement text (external_db.go:348, 356, 574). This is confined to hex-only passwords and the code documents that servers with log_statement=all will log it. Consider recommending operators disable full statement logging during provisioning, or using a pre-computed SCRAM verifier so cleartext never crosses the wire. Confidence: High.
[Minor] openAdminConn returns the raw ping error unwrapped - Convention
external_db.go:184 returns the PingContext error without fmt.Errorf("...: %w", err) context. This appears intentional so callers can errors.As it for typed classification and then redact; a one-line comment stating that would prevent a future "fix" from wrapping/leaking it. Confidence: High.
Cross-PR coordination
Two open pull requests require a maintainer decision or a defined merge order.
- #244 reworks the same e2e driver/provider scripts this PR extends (
scripts/kind/up.sh,tests/e2e/e2e-openshell.sh,tests/e2e/lib.sh) and addsup.shvalidation that rejects anyDATABASE_PROVIDERother thancnpg/deployment, while this PR introducesexternalas a third valid provider and adds it to the CI matrix. These are competing definitions of the accepted provider vocabulary: whichever merges second must fold the third provider into the other's restructured validation and driver-selection logic, or #244's guard will reject this PR'sexternalCI leg. Maintainers should decide the canonical provider set and the merge order. - #194 restructures the same gateway reconciler entry points (
ReconcileGateway/DeleteGatewayResources) by moving gateway deployment onto the upstream Helm chart with a values mapping, whereas this PR restructures those same paths around a newDatabaseReconcilerabstraction and relies on the current deployment consuming theopenshell-gateway-db-credentialsSecret. A design decision is needed on how the external provider's credential-secret contract and the provider abstraction integrate with the Helm-based deployment, and a merge order so the second PR re-integrates onto the other's reconciler shape.
Findings Summary (ordered by severity, highest first)
- [Major] Tenant
verify-full/sslrootcertpath propagation likely breaks gateway DB connections and contradicts the spec - Spec Consistency / Correctness (external_db.go L397-420, L581-582) - [Minor] Password interpolated into role DDL can leak under
log_statement=all- Security (external_db.go L348, L356, L574) - [Minor]
openAdminConnreturns the ping error unwrapped - Convention (external_db.go L184)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
errors.IsNotFound handled for 404 scenarios |
Pass |
| No secrets in logs or error messages | Pass |
Input validated (secret name prefix, no /) |
Pass |
| Reconcile pattern (update-or-create) | Pass |
| Status updated on error paths | Pass |
Context propagation (no context.TODO()) |
Pass |
| Conventional commit messages | Pass |
| Test diff scrutiny (no weakened assertions) | Pass |
| TLS posture propagated correctly to tenant | Fail |
| "sslmode": {tenantSSLMode}, | ||
| } | ||
| if params.sslrootcert != "" { | ||
| tenantQ.Set("sslrootcert", params.sslrootcert) |
There was a problem hiding this comment.
[Major] Mirroring the admin sslmode=verify-full and the admin-side sslrootcert filesystem path into the tenant Secret/URI here (and at L419-420, and in the rotation path L581-582) is likely to break the gateway's DB connection. params.sslrootcert is a path that exists only in the control-plane pod; the gateway workload runs elsewhere with no CA mounted at that path, so a verify-full URI would point at a nonexistent file. This also conflicts with openshell-gateway-database-external.spec.md:508-509, which describes the tenant sslrootcert as PEM content, not a path, and with the spec's own follow-up note (L516-518) that CA delivery to the gateway is not yet implemented and v1 MAY enforce require.
Suggestion: until the CA is delivered to the gateway workload, keep the tenant connection at require (do not propagate verify-full/the path), or copy the CA PEM content and reference it at a path the gateway pod actually mounts. Confidence: Medium.
There was a problem hiding this comment.
Fixed in ca9d77b. Both the provision and rotation paths now: (1) omit sslrootcert from the tenant Secret and URI entirely - it is a control-plane filesystem path, not a value the gateway pod can use; (2) cap tenant sslmode at 'require' when the admin uses 'verify-full', keeping the connection encrypted without cert verification until CA PEM delivery is implemented in v2. The comment in the code references ReconcileExternalDatabaseResources for context in the rotation path.
| // the password will appear in the server log; operators should restrict | ||
| // log verbosity or use server-side log redaction accordingly. | ||
| if _, err := db.ExecContext(ctx, | ||
| fmt.Sprintf("CREATE ROLE %s LOGIN PASSWORD '%s'", pgQuoteIdent(pgName), pgQuoteLiteral(password)), |
There was a problem hiding this comment.
[Minor, Security] The generated password is interpolated into CREATE ROLE ... PASSWORD '...' (and ALTER ROLE at L356/L574) because lib/pq cannot parameterize DDL. The code already documents that servers with log_statement=all will log it, and the value is hex-only, so injection is not a concern. To close the residual cleartext-in-server-log risk, consider computing a SCRAM-SHA-256 verifier client-side and issuing PASSWORD 'SCRAM-SHA-256$...', so plaintext never reaches the server or its logs. Confidence: High.
There was a problem hiding this comment.
Acknowledged (accepted). Computing a SCRAM-SHA-256 verifier client-side is the correct long-term fix. Deferring to v2: it requires implementing the SCRAM-SHA-256 SaltedPassword derivation (RFC 5802) in Go and then formatting 'SCRAM-SHA-256$$:$'. For v1, the risk is bounded: the password is hex-only (no control characters), and operators should restrict log_statement on the external server.
| db.SetMaxOpenConns(1) | ||
| if err := db.PingContext(ctx); err != nil { | ||
| _ = db.Close() | ||
| return nil, err |
There was a problem hiding this comment.
[Minor, Convention] This returns the raw PingContext error without fmt.Errorf("...: %w", err) context. That looks intentional so callers can errors.As it for typed classification (pq.Error/net.Error) and then redact via mapConnErrorToStatus. A one-line comment saying so would prevent a well-meaning future change from wrapping it and reintroducing a redaction gap. Confidence: High.
There was a problem hiding this comment.
Fixed in ca9d77b: added a comment to openAdminConn explicitly documenting that PingContext errors are returned unwrapped so callers can errors.As them for typed classification (*pq.Error / net.Error) via mapConnErrorToStatus. This prevents a future wrapping change from breaking the classification chain.
…Secret The admin sslrootcert is a filesystem path in the control-plane pod; the gateway workload runs in a different pod with no CA bundle at that path. Mirroring it into the tenant credentials Secret/URI would cause 'verify-full' tenant connections to fail with a missing-file error. - Remove sslrootcert from tenant Secret and URI in both the provision and rotation paths - Cap tenant sslmode at 'require' when the admin uses 'verify-full', so the tenant connection is still encrypted even without cert verification; CA delivery as PEM content is planned for v2 - Add a comment to openAdminConn documenting that PingContext errors are returned unwrapped intentionally so callers can errors.As them for typed classification via mapConnErrorToStatus Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
rh-amarin
left a comment
rh-amarin
left a comment
There was a problem hiding this comment.
Thanks for round 20 review (commit ca9d77b):
[Major] sslrootcert mirrored into tenant Secret - Fixed in both provision and rotation paths: sslrootcert is now omitted from the tenant Secret and URI (it is a control-plane filesystem path, not usable by the gateway workload). When admin uses verify-full, tenant sslmode is capped at require to keep the connection encrypted without cert verification; CA PEM delivery is planned for v2. The comment in ReconcileExternalDatabaseResources and a backreference in RotateExternalDatabaseCredentials document the v1 limitation.
[Minor, Security] SCRAM-SHA-256 - Acknowledged and deferred to v2. The v1 risk is bounded: password is hex-only and operators should restrict log_statement on the external server.
[Minor, Convention] openAdminConn unwrapped return - Added comment documenting that PingContext errors are returned unwrapped intentionally for typed classification.
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
This PR adds a well-designed external DATABASE_PROVIDER mode with in-process DDL provisioning, credential rotation, and cleanup, and cleanly extracts per-provider database logic behind a new DatabaseReconciler interface. The implementation is careful about secret redaction, error wrapping, IsNotFound handling, idempotency, and input validation; I found no blockers or critical issues, only a few minor items plus one cross-PR coordination point.
What I verified
- Provider wiring is consistent across the API server (
resolveDatabaseProvider,managedDatabasesvalidation, placement) and the control plane (config.resolveDatabaseProvider,newDatabaseReconciler,handleExternalDatabase,resolveDatabaseConfig). Theexternalcase is added everywhere the existing two providers are handled, and the reservedhypershell-managed-db-Secret prefix is enforced on both sides and documented innaming-multitenancy.spec.md. - Secret handling: credentials are never logged; connection errors are mapped to a closed status vocabulary and returned redacted; the tenant credentials Secret is a K8s Secret reference (correct pattern). Rotation is idempotent via the
last-db-rotationannotation, mirroring the CNPG path. - Refactor is behavior-preserving: the CNPG and deployment logic removed from
gateway/reconciler.goreappears verbatim incnpg_db.go/deployment_db.go(with the clientset generalized from*kubernetes.Clientsettokubernetes.Interface), andDeploymentReadinessremains available.deployment_readiness.gowas folded intodeployment_db.go. - Test-diff scrutiny: the two modified assertions in
managed_database_test.go/managed_database_lifecycle_test.goonly change the pre-existingcontrolPlaneNamespaceargument from""to"hypershell"; the assertions themselves (nil-client error,hasCNPG=false) are unchanged. No guarantee was removed. - Conventions: no
panic(), errors wrapped with%w, no em dashes introduced, per-commit conventional messages (will squash cleanly).
Findings (Minor)
- [Minor] CNPG placement now filters by
provider=="cnpg", but the CNPG-mode spec text was not updated -plugin.godbLookupAdapter.FindSolenow requires exactly onecnpgManagedDatabase, whereasopenshell-gateway-database.spec.mdstill says CNPG mode "queries all ManagedDatabases. If exactly one ManagedDatabase exists". The external-mode text calls out the provider filter; the CNPG-mode text should be updated to match the new multi-provider reality. Spec Consistency - [Minor]
mapConnErrorToStatusrelies on brittle substring matching - classification falls back to matching"network","ssl","tls", etc. in the lowercased error string. The typed*pq.Errorandnet.Errorchecks are correct and take precedence; the substring fallbacks are only a status label (not a control-flow decision), so impact is low, but a future driver message change could misclassify. Robustness - [Minor] Password is interpolated into
CREATE ROLE/ALTER ROLEDDL text - safe against injection (hex-only value, quoted), and the comment already documents thelog_statement=allserver-log exposure with an operator prerequisite. Flagging only so maintainers confirm that prerequisite is surfaced to operators registering an external server. Security (informational) - [Minor] CI stand-in PostgreSQL relaxes the SecurityContext -
scripts/kind/up.shruns the simulated "external" server withrunAsNonRoot: falseand no dropped capabilities. The rationale (postgres:15 entrypoint needs root to init, and this simulates a non-HyperShell-managed server) is documented and this is Kind/CI-only, so it is acceptable; please just ensure this manifest never migrates into a production overlay. Security (informational)
Cross-PR coordination
Another open pull request performs a large competing refactor of components/control-plane/internal/gateway/reconciler.go and internal/gateway/config.go to move gateway deployment onto the upstream OpenShell Helm chart (feat(control-plane): adopt upstream OpenShell Helm chart for gateway deployments). That change rewrites the same ReconcileGateway body and ReconcileOpts struct that this PR restructures - this PR extracts the database-provisioning switch into a new DatabaseReconciler interface across new files, while the other keeps that logic inline and rebuilds the surrounding deployment path around it. The two approaches are structurally incompatible (both delete large overlapping regions and both edit ReconcileOpts), and that PR is already reported non-mergeable. Maintainers should decide the merge order and how this PR's DatabaseReconciler abstraction (including the external provider) composes with the Helm-based ReconcileGateway; a mechanical merge will not reconcile the two designs.
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
errors.IsNotFound handled for 404 scenarios |
Pass |
| No secrets in logs or error messages | Pass |
| Input validated (Secret name prefix / format) | Pass |
| Reconcile pattern (update-or-create, not create-or-skip) | Pass |
| Status updated on error paths | Pass |
| Proper context propagation | Pass |
| SecurityContext on pod specs | Pass (CI stand-in intentionally relaxed, documented) |
| Image references consistent across manifests | Pass |
| Conventional commit messages | Pass |
| Spec updated to match code | Partial (CNPG-mode placement text) |
| return all[0].ID, nil | ||
| var matches []*managedDatabases.ManagedDatabase | ||
| for _, db := range all { | ||
| if a.provider == "" || db.Provider == a.provider { |
There was a problem hiding this comment.
CNPG placement now filters FindSole to provider == "cnpg". That is arguably more correct now that multiple providers can coexist, but openshell-gateway-database.spec.md still describes CNPG mode as "queries all ManagedDatabases. If exactly one ManagedDatabase exists". Please update the CNPG-mode placement text (as you did for external mode) so spec and code agree.
There was a problem hiding this comment.
Fixed in commit 17e082d. Updated openshell-gateway-database.spec.md line 97: "queries all ManagedDatabases" -> "queries all ManagedDatabases with provider=cnpg" to match the dbLookupAdapter{provider: ProviderCNPG} filter the code already applies.
| if strings.Contains(lower, "connection refused") || | ||
| strings.Contains(lower, "no such host") || | ||
| strings.Contains(lower, "i/o timeout") || | ||
| strings.Contains(lower, "network") { |
There was a problem hiding this comment.
The typed *pq.Error and net.Error checks above are the reliable path; these substring fallbacks ("network", "ssl", "tls", ...) are brittle and could misclassify if a driver/runtime error message changes. Impact is low since the result is only a status label rather than a control-flow decision, but consider narrowing the substring set or documenting it as best-effort.
There was a problem hiding this comment.
Valid point. Added comments in 17e082d labeling the string-matching blocks as "best-effort for unwrapped driver errors" and noting that the result is a status label only with no control-flow impact - so a misclassification has low consequence. The typed checks (net.Error, *pq.Error) remain the authoritative path.
| // the password will appear in the server log; operators should restrict | ||
| // log verbosity or use server-side log redaction accordingly. | ||
| if _, err := db.ExecContext(ctx, | ||
| fmt.Sprintf("CREATE ROLE %s LOGIN PASSWORD '%s'", pgQuoteIdent(pgName), pgQuoteLiteral(password)), |
There was a problem hiding this comment.
The password is interpolated into the DDL text (safe here: hex-only value, quoted, no injection). The comment already notes it can appear in the server log under log_statement=all. Flagging only so maintainers confirm this operator prerequisite (restrict log verbosity / server-side redaction) is surfaced to anyone registering an external server.
There was a problem hiding this comment.
Confirmed and already documented. The comment at line 345-348 in external_db.go reads: "lib/pq cannot parameterize CREATE ROLE / ALTER ROLE, so the password is interpolated into the statement text. On servers with log_statement=all the password will appear in the server log; operators should restrict log verbosity or use server-side log redaction accordingly." The same note appears at the ALTER ROLE rotation path. The operator prerequisite is surfaced in both places.
| # seccompProfile: RuntimeDefault is applied to restrict syscalls within | ||
| # the permitted capability set. | ||
| securityContext: | ||
| runAsNonRoot: false |
There was a problem hiding this comment.
This CI/Kind stand-in server runs with runAsNonRoot: false and no dropped capabilities, which diverges from the SecurityContext convention. The documented rationale (simulating a non-HyperShell-managed external server; postgres:15 needs root to init) makes this acceptable for Kind/CI only. Please make sure this permissive spec never gets copied into a production overlay.
There was a problem hiding this comment.
Acknowledged. This permissive spec is intentional for the Kind CI stand-in only - the inline comment documents the exception and rationale ("postgres:15 requires root to init the data directory; this simulates a non-HyperShell-managed external server"). The deploy/kind/ and deploy/production/ overlays are completely separate trees and the external server Deployment does not exist in any production overlay. No action needed, but agreed the distinction should remain clearly documented.
…sification comments - Align CNPG placement spec text to match code: FindSole now filters to provider=cnpg; update openshell-gateway-database.spec.md accordingly - Document substring fallbacks in mapConnErrorToStatus as best-effort for unwrapped driver errors (typed checks remain authoritative); clarify low-impact label-only use of the classification result Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
This PR adds a well-scoped external DATABASE_PROVIDER mode: the control plane provisions a per-gateway role/database on a user-managed PostgreSQL server, writes the tenant credentials Secret, rotates on annotation, and cleans up on delete, while the API server validates the reserved-prefix admin Secret reference and resolves the sole external ManagedDatabase for placement. The implementation is careful about credential redaction, SQL identifier/literal quoting, typed error classification, and idempotent DDL; I found no blockers, only a few minor observations plus one cross-PR coordination item.
What looks good
- Credentials are consistently kept out of error messages and logs (CREATE/ALTER ROLE errors return "credentials redacted"; connect errors are classified, not echoed).
sql.Openis wrapped with%w(safe, no creds), whilePingContextis intentionally returned unwrapped for typederrors.Asclassification. - SQL injection surface is bounded: identifiers go through
pgQuoteIdent, and the only interpolated literal is a hex-only generated password with an explicit "do not reuse" warning onpgQuoteLiteral. - The admin Secret reserved prefix (
hypershell-managed-db-) is enforced at both the API server and the control plane, with the security boundary documented innaming-multitenancy.spec.mdand cross-referenced in code. - Idempotent, reconcile-style DDL (existence checks before CREATE, REVOKE/GRANT each pass) and a rotation idempotency guard mirroring the CNPG path.
- Provider dispatch is cleanly factored behind
DatabaseReconciler, and theexternalprovider correctly participates in provider-filtered placement (FindSolenow filters by provider).
Minor observations
- [Minor]
mapConnErrorToStatusfalls through toExternalDBStatusUnreachablefor genuinely unknown errors. Since the result is a status label only (no control-flow branch), impact is low, but a connect-time privilege/permission error would be mislabeled "unreachable". Consider a neutral default or a comment noting the deliberate collapse. - [Minor] Interpolating the password into
CREATE ROLE/ALTER ROLEtext is unavoidable with lib/pq (DDL cannot be parameterized) and is documented as an operator prerequisite (restrictlog_statement/redact). This is acceptable; flagging only so the residual server-side log exposure stays tracked. - [Minor] In
DeleteGatewayResources, a transient DB-cleanup error returns before the credential-namespace RBAC cleanup loop runs, so on repeated transient failures that in-cluster cleanup is deferred until DB cleanup succeeds. The delete-reconcile retry makes this eventually consistent; worth a one-line note or reordering. - [Minor] Two pre-existing tests flip the reconciler's
controlPlaneNamespacearg from""to"hypershell". The assertions themselves are unchanged (nil-client error,hasCNPGfalse), so no guarantee is removed - noting per test-diff scrutiny since the shared input changed in the same PR.
Convention checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
errors.IsNotFound handled for 404 scenarios |
Pass |
| No secrets in logs or error messages | Pass |
| Input validated (Secret ref prefix/no-slash, quoted identifiers) | Pass |
| Reconcile pattern (idempotent DDL, update-or-create) | Pass |
Proper context propagation (no context.TODO()) |
Pass |
| SecurityContext on pod specs | N/A (external mode creates no pods) |
| OpenAPI client not manually edited | N/A |
| Conventional commits | Pass |
Findings Summary (ordered by severity, highest first):
- [Minor]
mapConnErrorToStatusdefault collapses unknown errors to "unreachable" - Observability (external_db.go) - [Minor] Password interpolated into role DDL; residual server-side log exposure - Security (external_db.go)
- [Minor] DB-cleanup transient error defers credential-namespace RBAC cleanup - Reconciliation (reconciler.go)
- [Minor] Shared test precondition changed from
""to"hypershell"- Test Diff (managed_database_test.go)
Cross-PR coordination
The open Helm-adoption work that shifts gateway deployment from static manifests to the upstream OpenShell chart restructures the same components/control-plane/internal/gateway/reconciler.go provisioning flow and ReconcileOpts, and it keeps the database-provider dispatch inline in the reconciler while rewriting the reconcile entrypoint. This PR instead extracts that dispatch into the new DatabaseReconciler interface (cnpg_db.go/deployment_db.go/external_db.go) and adds a third provider. The two directions are compatible in intent - that PR's chart consumes the openshell-gateway-db-credentials Secret this PR provisions before install - but they cannot both land as-is: maintainers need to decide the target shape of the gateway provisioning flow and ReconcileOpts, and the merge order, so the external provider integrates with the Helm-based deployment path rather than being reverted by it. Please coordinate with that PR's owner on which restructure lands first and how the external provider plugs into it.
| return ExternalDBStatusAuthFailed | ||
| } | ||
| // Privilege (this is checked post-connect, not here) | ||
| return ExternalDBStatusUnreachable |
There was a problem hiding this comment.
[Minor] Observability. The fall-through default returns ExternalDBStatusUnreachable for any unclassified error. Because this value is used only as a status label (no branch depends on it) the blast radius is small, but a connect-time privilege or permission error that isn't a *pq.Error/net.Error would be reported as "unreachable" rather than something closer to the truth. Consider a neutral catch-all label or a short comment documenting the deliberate collapse.
There was a problem hiding this comment.
Fixed in 3282f12. Replaced the old comment ('// Privilege (this is checked post-connect, not here)') with an explicit note documenting the deliberate collapse: 'Unclassified errors: deliberately collapsed to unreachable. This is a status label only; no control-flow branch depends on the value, so a misclassification has low impact.'
| // the password will appear in the server log; operators should restrict | ||
| // log verbosity or use server-side log redaction accordingly. | ||
| if _, err := db.ExecContext(ctx, | ||
| fmt.Sprintf("CREATE ROLE %s LOGIN PASSWORD '%s'", pgQuoteIdent(pgName), pgQuoteLiteral(password)), |
There was a problem hiding this comment.
[Minor] Security (acknowledged). The generated password is interpolated into the CREATE ROLE/ALTER ROLE statement text because lib/pq cannot parameterize DDL. This is unavoidable and you already document the operator prerequisite (restrict log_statement / enable redaction) plus keep the value out of the returned error. Flagging only so the residual server-side log exposure stays tracked; no change required if the operator prerequisite is enforced.
There was a problem hiding this comment.
Acknowledged and tracked. The operator prerequisite is already documented in two places (CREATE ROLE path at line 345-348 and ALTER ROLE rotation path). No code change made; the residual exposure is unavoidable with lib/pq DDL and is gated on operator-side log_statement configuration.
| // so we log at ERROR and return nil to let in-cluster RBAC cleanup proceed. | ||
| // Connection/DDL failures are transient and are returned for reconcile retry. | ||
| if _, _, err := readExternalAdminSecret(ctx, clientset, r.cfg.Namespace, r.cfg.SecretName); err != nil { | ||
| log.Printf("ERROR gateway %s: database cleanup cannot proceed (admin secret unreadable; orphaned database/role may require manual cleanup): %v", gatewayID, err) |
There was a problem hiding this comment.
[Minor] Lifecycle. When the admin Secret is unreadable this path logs at ERROR and returns nil so in-cluster RBAC cleanup proceeds - a reasonable terminal choice, but it leaves the per-gateway role/database orphaned on the external server. Since the external server is out of HyperShell's GC scope, consider surfacing this as a metric or a durable status field (not just a log line) so operators can find and reclaim orphaned tenant databases.
There was a problem hiding this comment.
Added a TODO(v2) comment in 3282f12 at the log site: 'TODO(v2): surface orphaned tenant DB as a metric or durable status field so operators can find and reclaim them without scanning logs.' Agreed that a metric or status field is the right long-term answer; deferring to v2 since it requires API changes (new status fields) and observability wiring beyond this PR's scope.
|
|
||
| func TestNewManagedDatabaseReconcilerWithoutKubernetesClient(t *testing.T) { | ||
| r := NewManagedDatabaseReconciler(nil, nil, nil, "") | ||
| r := NewManagedDatabaseReconciler(nil, nil, nil, "hypershell") |
There was a problem hiding this comment.
[Minor] Test-diff note. This pre-existing test (and its sibling in managed_database_lifecycle_test.go) changed the controlPlaneNamespace argument from "" to "hypershell". The assertion is unchanged (nil clients still return an error), so no guarantee is removed - noting only because two tests changed the same shared input in one PR, which is the pattern worth calling out explicitly.
There was a problem hiding this comment.
Correct observation. The controlPlaneNamespace argument was changed from '' to 'hypershell' to match the real default used in reconciler initialization (empty string is not a valid namespace). The assertion remains identical (nil clients -> error) so no guarantee was removed; the change makes the test input more realistic.
…n TODO - Document the ExternalDBStatusUnreachable fall-through in mapConnErrorToStatus as a deliberate collapse (status label only, no control-flow impact) - Add TODO(v2) comment for surfacing orphaned tenant DB as a metric or durable status field when admin Secret is unreadable at delete time Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
COMMENT. This is a mature, carefully-iterated change (22 review rounds) that cleanly introduces external as a first-class DATABASE_PROVIDER behind a well-designed provider-strategy interface, with strong secret redaction, idempotent DDL, and solid unit tests. No blockers or critical issues were found; the notes below are minor, and the main action for maintainers is the cross-PR coordination called out at the end.
What this PR does well
- Provider-strategy refactor is clean. Extracting the inline
switch opts.DatabaseProviderinto aDatabaseReconcilerinterface (db_reconciler.go) withcnpg/deployment/external/noopimplementations, and moving the CNPG and deployment logic verbatim intocnpg_db.go/deployment_db.go, is a faithful, behavior-preserving extraction. The legacy empty-provider case is preserved vianoopDatabaseReconciler. - Secret hygiene. Passwords are generated with
crypto/rand, never logged (connection errors are redacted, DSNs useurl.UserPassword/url.QueryEscape), and the DDL password-interpolation limitation (log_statement=all) is documented and promoted to an operator prerequisite in the spec. - Security boundary on the admin Secret. The reserved
hypershell-managed-db-prefix is enforced in both the API server (managedDatabases/service.go) and the control plane (external_db.go), and documented innaming-multitenancy.spec.mdas a boundary (not a convention). Theno-slashrule prevents cross-namespace Secret naming. - Error classification.
mapConnErrorToStatusprefers typed checks (*pq.ErrorSQLSTATE,net.Error) before best-effort string matching, and the ordering rationale (auth SQLSTATE beatssslsubstring) is both commented and unit-tested. - Idempotency & rotation. Role/database existence checks,
REVOKE/GRANT CONNECT, the tenant Secret reconcile (drift check viareflect.DeepEqual), and the rotation trigger-value guard all follow the reconcile-not-create-or-skip pattern.
Findings
- [Minor - Design] External DB
Deletereturns a transient error when the admin Secret is readable but the server is unreachable/DDL fails, which propagates and strands gateway finalization until the server recovers or the operator removes the admin Secret. This is intentional and well-documented (the error text tells the operator how to unblock, and aTODO(v2)notes surfacing orphans as a metric). Please just confirm the delete-reconcile uses a bounded backoff so a decommissioned server does not create a hot retry loop. See inline onexternal_db.go. Reconciliation. Confidence: Medium. - [Minor - Observability]
ProbeExternalServermaps anyreadExternalAdminSecretfailure to the terminal-soundingFailed: secret_invalid, including a transient Kubernetes API error onSecrets().Get. Low impact (re-probed each event), but the status vocabulary would be more truthful if transient API errors mapped to a retryable status. See inline. Observability. Confidence: Medium. - [Minor - Docs] The strict-seed variable is inconsistent:
seed.shheader now documentsKIND_SEED_STRICT, butseed.sh:319printsSEED_STRICT=trueand theMakefilestill exports/documentsSEED_STRICT. Pick one canonical name and state the alias relationship. See inline. Spec Consistency. Confidence: High.
Test Diff Scrutiny
The two edits in managed_database_test.go / managed_database_lifecycle_test.go change a constructor argument from "" to "hypershell" (the control-plane namespace). This is not a flipped guarantee: the assertions are unchanged (hasCNPG == false; nil-client returns an error), the constructor signature is unchanged, and the namespace value does not affect these two tests' outcomes. It is a benign realism tweak, not a silently-tightened precondition. No fallback/backfill concern applies.
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
errors.IsNotFound handled for 404 scenarios |
Pass |
| No secrets in logs or error messages | Pass |
| Input validated (Secret name prefix / no-slash) | Pass |
| Reconcile pattern used (not create-or-skip) | Pass |
| SecurityContext on pod specs | Pass (only new pod is the CI stand-in external Postgres, with a documented, justified runAsNonRoot: false exception) |
| Status updated on error paths | Pass |
Context propagation (no context.TODO()) |
Pass |
| Conventional commit messages | Pass |
| OpenAPI/proto not manually edited | Pass (reuses pre-existing connection_secret field) |
Findings Summary (ordered by severity, highest first):
- [Minor] External DB
Deletestrands gateway finalization on transient errors (intentional; confirm bounded backoff) - Reconciliation (external_db.go L493) - [Minor] Transient Secret-read failures classified as terminal
Failed: secret_invalid- Observability (external_db.go L251) - [Minor] Strict-seed env var name inconsistent across doc/code/Makefile - Spec Consistency (seed.sh L17, L319)
Cross-PR coordination
The control-plane change that adopts the upstream OpenShell Helm chart for gateway deployments requires coordination with this PR. That change re-architects the same ReconcileGateway entrypoint and its spec explicitly assumes the database-provisioning step is "unchanged" and runs before the Helm install, wiring the provisioned openshell-gateway-db-credentials Secret into the chart via server.externalDbSecret. This PR instead restructures that provisioning step into a DatabaseReconciler strategy, adds a new external provider that issues DDL and cleanup out-of-process, and moves/renames the CNPG rotation functions that the other change edits in place. Maintainers need to decide the merge order and how the new provider strategy (especially the external provider and its finalization-blocking delete path) composes with the Helm install ordering and the server.externalDbSecret mapping, so that whichever lands second is rebased onto - rather than silently reverting - the other's design.
| // Transient: connection error will be retried by the reconcile loop. | ||
| // If the external server is permanently decommissioned, remove admin | ||
| // Secret %q to make cleanup terminal and unblock gateway finalization. | ||
| return fmt.Errorf("external DB cleanup for gateway %s (%s): cannot connect to server (credentials redacted) - to unblock finalization on a decommissioned server, remove admin Secret %q", gatewayID, mapConnErrorToStatus(err), cfg.SecretName) |
There was a problem hiding this comment.
[Minor] Design note - transient cleanup errors strand gateway finalization.
When the admin Secret is readable but the server is unreachable (or DDL fails), Delete returns an error, which DeleteGatewayResources propagates, blocking gateway finalization until the server recovers or an operator deletes the admin Secret. This is an intentional, well-documented trade-off (the error message even tells the operator how to unblock), so it is not a defect. Please confirm the delete-reconcile has a bounded backoff so a permanently decommissioned server does not produce an unbounded hot retry loop, and consider surfacing the stranded state as a metric/status rather than only in logs (the TODO(v2) above already acknowledges this). Confidence: Medium.
| params, _, err := readExternalAdminSecret(ctx, clientset, cfg.Namespace, cfg.SecretName) | ||
| if err != nil { | ||
| log.Printf("INFO external DB probe %s: %s", cfg.SecretName, ExternalDBStatusSecretInvalid) | ||
| return ExternalDBStatusSecretInvalid |
There was a problem hiding this comment.
[Minor] Transient Secret-read failures are classified as terminal Failed: secret_invalid.
readExternalAdminSecret returns the secret_invalid sentinel for any error, including a transient Kubernetes API error on Secrets().Get (not just NotFound / missing-key / bad-prefix). A blip talking to the API server therefore surfaces as Failed: secret_invalid, which reads as an operator misconfiguration rather than a retryable condition. Impact is low because the ManagedDatabase is re-probed on the next event, but consider distinguishing a NotFound/validation failure (terminal secret_invalid) from a transient API error (retryable/unreachable) so the status vocabulary stays truthful. Confidence: Medium.
| # SEED_STRICT when "true", a seeding failure exits non-zero instead of | ||
| # DATABASE_PROVIDER cnpg | deployment | external (default: deployment). Must | ||
| # match the provider kind-up provisioned infrastructure for. | ||
| # KIND_SEED_STRICT when "true", a seeding failure exits non-zero instead of |
There was a problem hiding this comment.
[Minor] Strict-seed env var name is inconsistent across doc, code, and Makefile.
This header now documents KIND_SEED_STRICT (and line 21 says "KIND_SEED_STRICT remains an alias"), but the failure message at seed.sh:319 still prints SEED_STRICT=true and the Makefile help/export still reference SEED_STRICT. Pick one canonical name and make the alias relationship explicit (which is primary, which is the alias) so operators are not left guessing which variable actually takes effect. Confidence: High.
2 participants
Summary
externalas a first-classDATABASE_PROVIDERmode alongsidedeploymentandcnpg[deployment, cnpg]to[deployment, cnpg, external]— all infra, seed, and assertion logic for the external leg was already implemented inup.sh,seed.sh, ande2e-openshell.shTest plan
make lintpasses (Go + TS, all components)make kind-up DATABASE_PROVIDER=external && make kind-seedprovisions a gateway against the in-cluster external postgres inexternal-cloud-dbE2E Kind (external)CI job green alongsidedeploymentandcnpglegs🤖 Generated with Claude Code