[HYPERSHELL-84] feat(gateway-profile): Define and enforce per-gateway resource quotas - #227
[HYPERSHELL-84] feat(gateway-profile): Define and enforce per-gateway resource quotas#227rh-amarin wants to merge 1 commit 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 VerdictREQUEST_CHANGES. The GatewayProfile feature itself is well-structured — clean plugin layering, an idempotent update-or-create quota reconciler, deletion protection, boundary validation, and a legacy (no-profile) fallback path — but this PR also silently reverts an unrelated one-time-credential safety guarantee in the Hi, Amber here. Two-sentence summary above. Details and findings below. Critical1. Service-account credential is now fetched before the output file is reserved — a one-time secret can be generated and then lost. On Major2. Several CLI Go files are not The Minor3. PR body describes cluster columns as 4. What I verified and liked
Cross-PR coordinationTwo open pull requests make design choices that intersect with this one and need a maintainer decision or a defined merge order:
Findings Summary (ordered by severity, highest first):
Convention Checklist:
|
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
REQUEST_CHANGES. The GatewayProfile feature itself is well-structured — clean plugin layering, an idempotent update-or-create quota reconciler, deletion protection, boundary validation, and a legacy (no-profile) fallback path — but this PR also silently reverts an unrelated one-time-credential safety guarantee in the hsctl service-account flow (and deletes the tests that proved it), and ships several CLI Go files that are not gofmt-clean. The credential regression is a correctness/security issue and must be pulled out or fixed before merge.
Hi, Amber here. Two-sentence summary above. Details and findings below.
Critical
1. Service-account credential is now fetched before the output file is reserved — a one-time secret can be generated and then lost. components/cli/pkg/serviceaccount/serviceaccount.go and .../create/serviceAccount/cmd.go
On main, the command reserved the output target (os.OpenFile(..., O_EXCL, 0600)) before the POST /service_accounts request, precisely because "the server returns the client secret only once, so the write destination must exist before the secret is generated." This PR removes ReserveOutput/ReleaseOutput/WriteReserved, and WriteStructured now opens the file with O_EXCL after the credential has already been requested and rendered. If --output-file already exists (or is unwritable), the server has already minted the one-time client secret, the O_EXCL open fails, and the secret is burned — unrecoverable by the user. This also deletes the two tests that guarded the behavior (TestReserveOutputRejectsExistingTargetWithoutRequest, TestReleaseOutputRemovesEmptyReservation), so the removed guarantee (zero HTTP requests when the target exists) is gone with no replacement. This change is unrelated to GatewayProfile and looks like an accidental revert bundled into the feature branch. Restore the reserve-before-request flow (or move this out of the PR). Confidence: High.
Major
2. Several CLI Go files are not gofmt-clean and will fail make check/lint. components/cli/pkg/urls/urls.go, components/cli/cmd/hypershell/create/fleet/cmd.go, components/cli/cmd/hypershell/create/serviceAccount/cmd.go
The const block in urls.go and the args structs in the CLI commands had their column alignment stripped (e.g. FleetsPath = APIPrefix + "/fleets" is no longer tab-aligned with its siblings), and urls.go gained a trailing blank line. gofmt aligns consecutive single-line declarations and trims trailing blank lines, so these files are not formatted. CLAUDE.md requires gofmt -w . before commit. Run gofmt -w ./components/... and re-commit. Confidence: High.
Minor
3. PR body describes cluster columns as default_profile_id / default_database_id, but the actual model/DB columns are profile_id / database_id. The code and SDK are internally consistent (ManagedCluster.ProfileId/profile_id), so this is only a description/spec-wording mismatch, but it makes the "cluster default" semantics ambiguous when read against the schema. Align the wording (or the column names) so reviewers and operators aren't misled. Confidence: High.
4. profileResolverAdapter.ClusterDefaultProfileID loads every cluster via All(ctx) and scans for a match, while the sibling ClusterExists uses Get(ctx, id). components/api-server/plugins/gateways/plugin.go:75. On create this is an O(clusters) scan for a single-row lookup; prefer clusters.Get(ctx, clusterID) for consistency and to avoid growth cost. Confidence: Medium.
What I verified and liked
ReconcileNamespaceQuotais genuinely idempotent (create-when-absent, update-on-divergence, no-op-on-match, delete-managed-object-when-empty) and only deletes objects carrying the managed label — no create-or-skip anti-pattern.resolveGatewayProfilecorrectly blocks provisioning and marks the gatewayFailedon a profile fetch failure, and treats an emptyprofile_idas legacy (nil quota → reconcile toward absence), so pre-existing gateways with noprofile_idare not broken. This is the required fallback for the optional→required transition at the create boundary.- Boundary validation via
resource.ParseQuantity(rejecting negatives) returns 400 rather than persisting values that would later fail control-plane reconciliation; deletion protection returns 409 for referenced profiles. - Quota RBAC is added to
deploy/base/controller-rbac.yaml; the IBM overlay inherits it transitively throughdeploy/openshift, so no separate overlay edit is needed. - Errors are wrapped with
%wand context throughout; SecurityContext is untouched; no secrets are logged.
Cross-PR coordination
Two open pull requests make design choices that intersect with this one and need a maintainer decision or a defined merge order:
-
#223 (remove Fleet entity and
fleet_idacross the stack): Both PRs re-edit the same generated OpenAPI artifacts and source (openapi.yaml,model_gateway.go,model_gateway_create_request.go,model_gateway_patch_request.go,model_managed_cluster.go,api_default.go) and the canonicalspecs/platform/data-model.spec.md— #223 removesfleet_id, this PR addsprofile_id/quota schema. Beyond the mechanical regeneration (whoever merges second must rebase and re-runmake generate), there is a real design question: #223 replaces fleet-based tenancy with an RBAC model, while this PR introduces GatewayProfile as a global, unscoped mutable resource whose deletion/creation affects quota enforcement for all gateways. Maintainers should decide GatewayProfile's tenancy/ownership under the post-fleet model and set the merge order accordingly. -
#194 (adopt upstream OpenShell Helm chart for gateway deployments): This PR inserts
ReconcileNamespaceQuotaintoReconcileGatewayand adds aQuotafield toReconcileOptsininternal/gateway/config.go/reconciler.go— the same reconcile path and files #194 restructures to install gateways via a Helm release. A decision is needed on whether theResourceQuota/LimitRangebecome chart-managed values or remain control-plane-reconciled objects outside the Helm release (which also affects whether the LimitRange defaults will cover Helm-templated pod specs so quota admission does not reject them). Coordinate the ownership boundary and the insertion point before both land.
Findings Summary (ordered by severity, highest first):
- [Critical] One-time service-account credential fetched before the output file is reserved; secret can be generated then lost, and the guarding tests were deleted - Security / Correctness / Test Diff Scrutiny (serviceaccount.go L57-L79, serviceAccount/cmd.go L86-L90)
- [Major] CLI Go files not
gofmt-clean; will failmake check- Go Conventions (urls.go L5, fleet/cmd.go L20) - [Minor] PR/spec wording (
default_profile_id/default_database_id) does not match actual columns (profile_id/database_id) - Spec Consistency - [Minor]
ClusterDefaultProfileIDusesAll()+scan instead ofGet()- API Design (plugin.go L75)
Convention Checklist:
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
errors.IsNotFound / 404 handled |
Pass |
| No secrets in logs or responses | Pass |
| Input validated (quantities, references) | Pass |
| Reconcile pattern (update-or-create, not create-or-skip) | Pass |
| SecurityContext on pod specs | N/A (no pod specs changed) |
| Image references consistent across manifests | Pass |
| OpenAPI client generated, not hand-edited | Pass |
gofmt applied |
Fail |
| Test Diff Scrutiny (removed guarantees) | Fail |
| Optional→required has fallback/backfill | Pass (control-plane legacy nil-quota path) |
4373850 to
bd5fae2
Compare
Amber reviewStatus: Complete VerdictThis is a large but cohesive, well-layered feature: the new What works well
FindingsMinor items are inline. Highlights:
Cross-PR coordinationThe following require maintainer coordination or a merge-order decision; each is a design/assumption interaction, not a plain merge conflict.
Convention Checklist
Findings Summary (ordered by severity, highest first):
|
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
This is a large but cohesive, well-layered feature: the new GatewayProfile resource is plumbed end-to-end and the control-plane quota reconciler follows the update-or-create-then-delete-when-empty contract correctly. I found no blocking convention or security violations; my comments are minor hardening/consistency items plus cross-PR coordination that maintainers should sequence deliberately.
What works well
- Legacy-safe rollout of a newly-required field.
gateway.profile_idbecomes required at create time only, with a cluster-default fallback, while the control plane treats an emptyprofile_idasnilQuotaConfigand reconciles toward absence of both managed objects (resolveGatewayProfileFromClient,ReconcileNamespaceQuota). Pre-existing gateways with a blankprofile_idare not markedFailed, so this does not silently invalidate records already in a running environment. - Reconcile, not create-or-skip.
reconcileResourceQuota/reconcileLimitRangeget-then-create/update/delete, gate deletes behind the managed label, and useapiequality.Semantic.DeepEqualto avoid spurious writes. - Validation at the boundary.
validateProfileFieldsrejects invalid/negative quantities with HTTP 400 before persistence; deletion protection returns 409 when a profile is referenced by a cluster default or any gateway. - Error wrapping and
IsNotFoundhandling are consistent throughoutquota.goand the reconciler; nopanic(); no secret values logged (only object names). - RBAC for
resourcequotas/limitrangeswas added; the test-count guard inopenapi_embed_test.go(42 -> 47) is a legitimate additive change, not a weakened assertion.
Findings
Minor items are inline. Highlights:
- [Minor] The gRPC
UpdateGatewaypath assignsprofile_idwithout the existence check the RESTPatchpath enforces, so REST and gRPC accept different inputs. Interface consistency - [Minor]
validateProfileFieldsvalidates each quantity independently but never checks logical consistency (e.g. request <= limit, container default <= max). Inconsistent profiles are accepted and only surface later as namespace admission failures. Validation completeness - [Minor] Because
Handleskips gateways in phaseRunning/Provisioning/Degraded, editing aGatewayProfilereferenced by a Running gateway does not re-apply the updatedResourceQuota/LimitRangeuntil the gateway next leaves those phases. Worth documenting the propagation boundary. Reconciliation drift
Cross-PR coordination
The following require maintainer coordination or a merge-order decision; each is a design/assumption interaction, not a plain merge conflict.
-
#223 (remove Fleet entity and
fleet_id). This PR adds a new resource and new gateway/managed-cluster fields on the assumption that the fleet data model (and the security spec's Fleet Isolation query-scoping rule) still holds, while #223 deletesfleet_idend-to-end and removes that rule. The newGatewayProfileDAO/service is deliberately not fleet-scoped, which is consistent with #223's direction but conflicts with the fleet-scoping requirement that is still in effect until #223 lands. Maintainers should decide the merge order and confirm whetherGatewayProfileis intended to be a global (non-fleet-scoped) resource; both PRs also add migrations and regenerate the same OpenAPI/gRPC gateway + managed-cluster artifacts, so the second to merge must rebase against the other's data-model shape. -
#151 (gate gateway re-provisioning on desired-state convergence). Both PRs change
GatewayReconciler.Handleand add a new gateways migration. #151 re-keys the provisioning gate away from phase (Running/Provisioning/Degraded) onto ageneration/observed_generationconvergence signal — the exact gate that currently prevents this PR's quota changes from reaching Running gateways (finding #3). The "mark Failed on profile-fetch error" behavior added here and #151's convergence gating need to be reconciled so a quota/profile change re-triggers reconciliation under the new gate. A design decision on how the profile-quota path participates in convergence is needed before both land. -
#194 (adopt upstream OpenShell Helm chart for gateway deployments). #194 replaces the direct-manifest deployment path and rewrites
internal/gateway/reconciler.goandinternal/gateway/config.go, while this PR injectsReconcileNamespaceQuotaintoReconcileGatewayand adds aQuotafield toReconcileOptsin those same files. #194 enumerates exactly what the control plane still manages inside the gateway namespace (SCC binding, trusted-CA ConfigMap) and does not include ResourceQuota/LimitRange. Maintainers must decide whether the quota objects become Helm-chart values/release-owned or remain control-plane-managed SSA objects, and sequence the two reconciler rewrites accordingly.
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 responses | Pass |
| Input validated (K8s quantities) | Pass |
| Reconcile pattern (update-or-create, not create-or-skip) | Pass |
| SecurityContext / RBAC updated for new managed objects | Pass |
| OpenAPI client generated, not hand-edited | Pass |
| Optional->required field has fallback/legacy path | Pass |
| Conventional commit message | Pass |
Findings Summary (ordered by severity, highest first):
- [Minor] gRPC
UpdateGatewayskips theprofile_idexistence check REST enforces - Interface Consistency (grpc_handler.go L159) - [Minor] No cross-field consistency validation on profile quantities - Validation Completeness (validate.go L41)
- [Minor] Quota updates to Running gateways are not re-applied due to the phase gate - Reconciliation Drift (reconciler.go L1427)
| } | ||
| // database_id is server-owned placement state. Ignore values supplied by | ||
| // callers; gateway creation business logic is the only assignment path. | ||
| if req.ProfileId != nil && *req.ProfileId != "" { |
There was a problem hiding this comment.
The gRPC UpdateGateway path assigns profile_id with no existence check, while the REST Patch handler validates via ProfileExists and rejects unknown/empty ids. This lets a gRPC caller point a gateway at a non-existent profile, which the control plane then cannot fetch (blocking provisioning and marking the gateway Failed). Consider mirroring the REST existence validation here for parity.
| // validateProfileFields validates every quantity and count field on a | ||
| // GatewayProfile at the API boundary so invalid values are rejected with HTTP | ||
| // 400 rather than persisted and later failing control-plane reconciliation. | ||
| func validateProfileFields(p *GatewayProfile) *errors.ServiceError { |
There was a problem hiding this comment.
validateProfileFields validates each quantity/count independently but never checks logical relationships (e.g. cpu_request_total <= cpu_limit_total, container_cpu_request_default <= container_cpu_limit_max). An internally inconsistent profile is accepted and only fails later as a namespace admission / ResourceQuota error, which is harder to trace back to the profile. Consider adding cross-field consistency checks so bad profiles are rejected at the API boundary.
bd5fae2 to
93f4c4d
Compare
Amber reviewStatus: Complete VerdictAssessment: COMMENT. This is a large, well-structured, mostly-generated feature that plumbs a new What looks good
Findings (see inline comments for locations)
|
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
Assessment: COMMENT. This is a large, well-structured, mostly-generated feature that plumbs a new GatewayProfile resource end to end; the hand-written core (control-plane quota reconcile, API validation, gateway create/patch wiring, deletion protection, migrations) follows project conventions cleanly. Two design-level concerns — the namespace-total/container-default coupling that can brick a namespace, and the authorization model for a global enforcement resource — plus cross-PR coordination should be resolved before merge, but nothing here is a hard blocker or secret leak.
What looks good
- Errors are wrapped with
fmt.Errorf("context: %w", err); nopanic()in production paths;errors.IsNotFound/Is404handled correctly. - Control-plane quota reconcile is genuine update-or-create with delete-when-empty, and only ever deletes objects carrying the managed label — good idempotency and blast-radius control.
- Namespace is created before
ReconcileNamespaceQuota, and the quota is applied before workloads so the gateway's own pods are admitted under it. - Deletion protection (HTTP 409 when a profile is referenced by a cluster default or a gateway) prevents dangling references; legacy gateways with empty
profile_idreconcile toward absence of quota, so the new field is effectively optional for pre-existing data (no missing backfill). - Migrations are reversible; the OpenAPI operation-count assertion bump (42→47) is a legitimate additive change, not a weakened guarantee.
Findings (see inline comments for locations)
- [Major] No cross-field validation between namespace request/limit totals and container defaults — a profile can be authored that makes every pod fail admission.
- [Major]
GatewayProfileis a global, enforcement-governing resource with only generic API authz; confirm profile management is platform-admin-only. - [Minor] Cluster default
profile_idaccepted on PATCH without an existence check. - [Minor] Profile-fetch failure marks the gateway
Failedwithout distinguishing transient from terminal errors.
Cross-PR coordination
- #223 (remove Fleet entity /
fleet_idacross the stack): This PR introducesGatewayProfileas a global, non-fleet-scoped resource guarded only by generic API authz, while its gateway PATCH path still writesfleet_id. #223 removesfleet_idand the Fleet-isolation security requirement entirely, moving tenancy to RBAC. Maintainers need to decide the tenancy/authorization model forGatewayProfile(platform-admin-owned vs tenant-scoped) so it lands consistent with the fleet-removal direction, and agree a merge order, since both PRs rewrite the gateway and managed-cluster models, protobufs, OpenAPI, SDK, and migrations end to end. - #151 (gate re-provisioning on desired-state convergence): This PR relies on
ReconcileGatewayre-running each pass to fetch the profile and re-apply the namespaceResourceQuota/LimitRange. #151 re-keys the provisioning gate on the gateway's own generation converging, which would skip re-apply for aRunninggateway when only its referenced profile changed (the gateway spec's generation never advances). Maintainers must decide how aGatewayProfileedit triggers re-reconciliation of already-provisioned gateways under that gate. - #194 (adopt upstream OpenShell Helm chart for gateway deployments): This PR makes the control plane the owner of two new in-namespace objects (
ResourceQuota+LimitRange) applied insideReconcileGateway. #194's ownership-boundary analysis enumerates only the SCC binding and the trusted-CA ConfigMap as control-plane-managed namespace objects. Maintainers must decide whether quota objects stay control-plane-managed or move into Helm chart values, and coordinate the reconcile insertion point.
Findings Summary (ordered by severity, highest first):
- [Major] Missing coupling validation: a namespace request/limit total without a matching container default (or explicit pod requests) causes pod admission failures - Design / Input validation (validate.go, quota.go)
- [Major] Global
GatewayProfileresource governs enforcement but has no tenancy/admin scoping beyond generic API authz - Security / Authorization (plugin.go L60-61) - [Minor] Cluster default
profile_idaccepted on PATCH without existence check - Spec Consistency (managedClusters/handler.go L86-88) - [Minor] Profile-fetch failure marks gateway
Failedon transient errors too - Reconciliation (reconciler.go L1434)
Convention Checklist:
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
errors.IsNotFound/Is404 handled |
Pass |
| No secrets in logs or responses | Pass |
| Input validated (quantities, DNS labels) | Pass (see Major coupling gap) |
| Reconcile pattern (update-or-create, delete-when-empty) | Pass |
| Status updated on error paths | Pass |
Context propagation (no context.TODO()) |
Pass |
| OpenAPI/gRPC client generated, not hand-edited | Pass |
| Migrations reversible | Pass |
| Conventional commit message | Pass |
| Test diff scrutiny (no silently flipped assertions) | Pass |
| if svcErr := validateCount("pvc_count", p.PvcCount); svcErr != nil { | ||
| return svcErr | ||
| } | ||
| return nil |
There was a problem hiding this comment.
[Major] No cross-field validation between namespace totals and container defaults. validateProfileFields treats every quantity as independently optional, and the spec (openshell-gateway-quota.spec.md) does the same. But a Kubernetes ResourceQuota that sets requests.cpu/requests.memory/limits.* requires every pod in the namespace to declare the corresponding request/limit. If a profile sets e.g. cpu_request_total but leaves container_cpu_request_default empty (and any managed pod — gateway, supervisor, or a user-created sandbox — omits explicit requests), the LimitRange provides no default and admission rejects the pod with failed quota / must specify requests.cpu, so the gateway namespace can never converge. Consider validating the coupling (when a namespace request/limit total is set, require the matching container defaultRequest/max) or at minimum documenting the requirement so an operator cannot author a self-bricking profile. Confidence: Medium.
| gatewayProfilesRouter.HandleFunc("/{id}", gatewayProfileHandler.Patch).Methods(http.MethodPatch) | ||
| gatewayProfilesRouter.HandleFunc("/{id}", gatewayProfileHandler.Delete).Methods(http.MethodDelete) | ||
| gatewayProfilesRouter.Use(authMiddleware.AuthenticateAccountJWT) | ||
| gatewayProfilesRouter.Use(authzMiddleware.AuthorizeApi) |
There was a problem hiding this comment.
[Major] Confirm the authorization model for a global, enforcement-governing resource. GatewayProfile is not tenant-scoped (no fleet_id, no per-object RoleBinding visibility filter like gateways has) and is guarded only by the generic authzMiddleware.AuthorizeApi. Because a profile defines the resource ceiling the control plane enforces, any caller who can reach POST /gateway_profiles can mint a profile with arbitrarily large quotas and assign a gateway to it, defeating the enforcement this PR adds. Please confirm AuthorizeApi restricts create/patch/delete of profiles to a platform-admin role, and state that intent in the spec. Confidence: Medium.
| found.ApiServerUrl = patch.ApiServerUrl | ||
| } | ||
| if patch.ProfileId != nil { | ||
| found.ProfileId = patch.ProfileId |
There was a problem hiding this comment.
[Minor] Cluster default profile_id is accepted on PATCH without an existence check. Unlike the gateway PATCH path (which calls ProfileExists), setting a cluster's default profile_id here is unvalidated. A cluster pointed at a non-existent profile surfaces later as a confusing gateway profile <id> does not exist failure at gateway-create time (via the cluster-default fallback), far from where the bad value was set. Consider validating the referenced profile exists here. Confidence: High.
| if profileErr != nil { | ||
| // Mark the gateway Failed so its declared quota is never silently | ||
| // unenforced, and record the failure in the trace span via reconcileErr. | ||
| r.updateGatewayPhase(ctx, event.ResourceID, "Failed") |
There was a problem hiding this comment.
[Minor] Profile-fetch failure marks the gateway Failed without distinguishing transient from terminal errors. Any gRPC error (including a momentary API-server unavailability) flips the phase to Failed. Returning reconcileErr still triggers a retry, but the phase can flap Failed→Provisioning on transient blips. Blocking provisioning is the right call for a truly missing profile; consider only marking Failed on a terminal (NotFound/empty-payload) result and leaving transient errors to plain retry. Confidence: Medium.
93f4c4d to
cecad07
Compare
Amber reviewStatus: Complete VerdictThe GatewayProfile feature is well-structured, idiomatic, and reconcile-correct: the control-plane quota reconciler is genuinely update-or-create/delete-when-empty, fetch failure blocks provisioning so no gateway runs unconstrained, and legacy (empty profile_id) gateways get a clean nil-quota fallback. Two things need a maintainer decision before merge — a normative spec that contradicts this PR's own code/OpenAPI on how SummaryThis PR adds an end-to-end Findings[Major] [Major] Web console silently drops hub-default gateway provisioning and deletes its resilience tests — Removed Guarantee / Test Diff Scrutiny [Minor] [Minor] Create/delete race can persist a dangling Cross-PR coordinationTwo open pull requests need maintainer coordination with this one:
Findings Summary (ordered by severity, highest first)
Convention Checklist
|
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
The GatewayProfile feature is well-structured, idiomatic, and reconcile-correct: the control-plane quota reconciler is genuinely update-or-create/delete-when-empty, fetch failure blocks provisioning so no gateway runs unconstrained, and legacy (empty profile_id) gateways get a clean nil-quota fallback. Two things need a maintainer decision before merge — a normative spec that contradicts this PR's own code/OpenAPI on how profile_id is assigned at create, and a web-console behavior change (hub-default provisioning removed, two resilience tests deleted) that isn't called out in the description.
Summary
This PR adds an end-to-end GatewayProfile resource (API-server plugin, gRPC, OpenAPI, control-plane quota reconciler, SDKs, CLI, web console) that projects a Kubernetes ResourceQuota + LimitRange onto each gateway namespace. The backend, reconciler, and RBAC changes are coherent and cover error paths; my findings are contract-clarity and validation-gap issues, not correctness blockers.
Findings
[Major] data-model.spec.md contradicts this PR's own code and OpenAPI on create-time profile_id — Spec Consistency
specs/platform/data-model.spec.md:308 states profile_id is "Server-assigned from ManagedCluster.profile_id at creation (client-supplied values on create are ignored)." But plugins/gateways/service.go:152 ("A client-supplied profile_id wins"), ConvertGateway, the gRPC CreateGateway handler, openapi.gateways.yaml ("required at creation (client-supplied or inherited from the cluster default)"), the web console (which sends profileId on create), and the PR body all treat a client-supplied profile_id as authoritative with the cluster default as fallback. specs/ is the authoritative desired-state; this line should be corrected to match the implemented "client value wins, else cluster default, else 400" contract (contrast with database_id, which really is ignored). Confidence: High.
[Major] Web console silently drops hub-default gateway provisioning and deletes its resilience tests — Removed Guarantee / Test Diff Scrutiny
gateway-create.tsx changes the cluster field default from "" to null and the validation from if (value === null) to if (!value) (:107, :126), making an explicit cluster selection mandatory. The tests provisions on the hub by default without exposing a namespace and keeps hub provisioning available when managed clusters fail to load were removed rather than replaced. This deletes a previously guaranteed behavior (hub provisioning + graceful fallback when the cluster list API fails) and diverges the UI from the API, which still accepts an empty cluster_id. The change is reasonable if intentional, but it isn't mentioned in the PR description and isn't scoped to quota enforcement. Please confirm intent and, if kept, call it out and either restore a hub path or document its removal. Confidence: High.
[Minor] ManagedCluster PATCH does not validate profile_id / database_id references — Input Validation
plugins/managedClusters/handler.go:86 assigns patch.ProfileId/patch.DatabaseId with no existence check, whereas the gateway PATCH/create paths validate ProfileExists. A cluster default pointing at a non-existent profile is only discovered later, at gateway-create time, as an HTTP 400 that names the gateway request rather than the bad default. Consider validating the referenced profile (and database) on cluster PATCH for a clearer failure surface. Confidence: Medium.
[Minor] Create/delete race can persist a dangling profile_id — Concurrency
Deletion protection (gatewayProfiles/service.go:121) checks referrers, and gateway create checks ProfileExists, but the two are not mutually serialized: a profile delete that runs between a gateway's ProfileExists check and its row insert can leave the new gateway referencing a deleted profile. The control plane then marks that gateway Failed (no data loss, but stuck). Acceptable for now; worth a note or a FK/transactional guard if this path is expected under load. Confidence: Medium.
Cross-PR coordination
Two open pull requests need maintainer coordination with this one:
-
#223 (remove Fleet entity and
fleet_idacross the stack). Both PRs rewritespecs/platform/data-model.spec.md(ERD + entity definitions) and regenerate the shared OpenAPI/gRPC/SDK models forGatewayandManagedCluster, and both reshape the platform tenancy model. This PR keeps the Fleet-centric data model and introducesGatewayProfileas a global resource with nofleet_idand no per-tenant scoping, while #223 removes Fleet entirely and moves tenancy to RBAC. Maintainers must decide (a) the ownership/tenancy scoping ofGatewayProfileunder the post-Fleet RBAC model, and (b) a merge order — whichever lands second must regenerate the SDKs and reconcile the ERD/entity text, and re-confirm thatprofile_id/database_idrows carry the intended (or no) fleet scoping. -
#194 (adopt upstream OpenShell Helm chart for gateway deployments). This PR makes the control plane directly manage two new namespace objects (
ResourceQuotahypershell-gateway-quota,LimitRangehypershell-gateway-limits) via the K8s client. #194's gap analysis enumerates only SCC binding and the trusted-CA ConfigMap as control-plane-managed namespace objects. If #194's Helm adoption proceeds, maintainers must decide whether these quota objects remain control-plane-managed (as implemented here) or move into the chart-managed set, and update #194's ownership-boundary gap analysis accordingly to avoid dual ownership.
Findings Summary (ordered by severity, highest first)
- [Major]
data-model.spec.mdsays create-timeprofile_idis ignored, but code/OpenAPI/UI/PR treat client value as authoritative - Spec Consistency (data-model.spec.md L308, service.go L152) - [Major] Web console removes hub-default provisioning and deletes two resilience tests, undocumented - Removed Guarantee (gateway-create.tsx L107, L126)
- [Minor]
ManagedClusterPATCH does not validateprofile_id/database_idreferences - Input Validation (managedClusters/handler.go L86) - [Minor] Profile create/delete race can persist a dangling
profile_id- Concurrency (gatewayProfiles/service.go L121)
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 responses | Pass |
| Input validated (quantities, counts, IDs) | Pass |
| SecurityContext / managed-label deletion guard | Pass |
| Reconcile pattern (update-or-create, delete-when-empty) | Pass |
| Status updated on error paths (gateway marked Failed) | Pass |
| Image references consistent across manifests | Pass |
| RBAC extended for new resources (base + IBM overlays) | Pass |
| OpenAPI client generated, not hand-edited | Pass |
| Spec matches implemented contract | Fail |
| Test changes additive, no silent removed guarantees | Fail |
|
|
||
| | Field | Type | Description | | ||
| |---|---|---| | ||
| | `profile_id` | string | GatewayProfile ID enforced on this gateway's namespace. Server-assigned from `ManagedCluster.profile_id` at creation (client-supplied values on create are ignored). Reassignable via PATCH; a reassigned value is validated to reference an existing GatewayProfile. | |
There was a problem hiding this comment.
This normative spec line says a client-supplied profile_id on create is ignored and the value is server-assigned from the cluster default. That contradicts this PR's own implementation and OpenAPI: service.go Create says "A client-supplied profile_id wins", ConvertGateway/the gRPC handler copy the client value, and openapi.gateways.yaml documents profile_id as "required at creation (client-supplied or inherited from the cluster default)". Please update this row to match the implemented contract (client value wins, else cluster default, else HTTP 400) so it doesn't read like database_id, which really is ignored. [Major]
| } | ||
| } | ||
|
|
||
| // Resolve the gateway quota profile. A client-supplied profile_id wins; if |
There was a problem hiding this comment.
This "client-supplied profile_id wins, else cluster default, else 400" logic is the intended behavior per the PR body and OpenAPI, but specs/platform/data-model.spec.md:308 still states client values are ignored on create. Reconcile the spec with this code path. [Major]
| if patch.ApiServerUrl != nil { | ||
| found.ApiServerUrl = patch.ApiServerUrl | ||
| } | ||
| if patch.ProfileId != nil { |
There was a problem hiding this comment.
PATCH assigns profile_id (and database_id) with no existence check, unlike the gateway create/PATCH paths which call ProfileExists. A cluster default pointing at a missing profile is only surfaced later as an HTTP 400 on a gateway create request, which is confusing. Consider validating the referenced profile here for a clearer failure surface. [Minor]
| .nullable() | ||
| .transform((value, context) => { | ||
| if (value === null) { | ||
| if (!value) { |
There was a problem hiding this comment.
Changing the cluster default to null and the guard from if (value === null) to if (!value) makes an explicit cluster selection mandatory, removing hub-default provisioning. The two tests provisions on the hub by default... and keeps hub provisioning available when managed clusters fail to load were deleted rather than replaced, so a resilience guarantee (provisioning still works when the cluster list API fails) is gone, and the UI now diverges from the API, which still accepts an empty cluster_id. If intentional, please call this out in the PR description and either restore a hub path or document its removal. [Major]
rh-amarin
changed the title
cecad07 to
ebbc5be
Compare
Amber reviewStatus: Complete VerdictThis is a large, well-structured, and well-tested feature: it introduces the GatewayProfile resource end-to-end (API server, gRPC, OpenAPI, SDKs, CLI, web console, control plane) and enforces per-namespace Findings[Major] Create validates [Major] [Minor] Existence checks silently pass when no resolver is wired — Cross-PR coordinationTwo open pull requests have material conflicts with this one that require a maintainer decision on scope, interface ownership, and merge order:
Findings Summary (ordered by severity, highest first):
Convention Checklist:
|
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
This is a large, well-structured, and well-tested feature: it introduces the GatewayProfile resource end-to-end (API server, gRPC, OpenAPI, SDKs, CLI, web console, control plane) and enforces per-namespace ResourceQuota/LimitRange. The core reconciliation logic is idempotent and correctly treats legacy (profile-less) gateways as "reconcile toward absence," so the optional→required transition is safe. I have two Major items worth addressing before merge (a create-time ordering issue that can orphan a ManagedDatabase, and control-plane RBAC coverage on non-IBM environments) plus a minor observation.
Findings
[Major] Create validates cluster_id/profile_id after placement has already provisioned a database — components/api-server/plugins/gateways/service.go
s.placement.Resolve(...) runs first, and the deployment placement strategy's CreateForGateway creates a real per-gateway ManagedDatabase (which the control plane then provisions). The new cluster_id existence check and the profile_id required/existence checks run after that, and each can reject the request with HTTP 400. On any of those rejections the just-created ManagedDatabase is orphaned (no gateway references it). Move the cluster_id and profile_id validation before s.placement.Resolve(...) so a bad request is rejected before any server-owned resource is created. Confidence: Medium-High.
[Major] resourcequotas/limitranges RBAC only added to the IBM overlay — components/api-server/deploy/ibm/controller-clusterrbac.yaml
The reconciler now hard-fails gateway provisioning when it cannot create/update the ResourceQuota/LimitRange (ReconcileNamespaceQuota returns an error → ReconcileGateway returns an error → gateway goes Failed). Only the IBM overlay ClusterRole was granted these verbs. Please confirm the credentials the control plane uses on kind/OpenShift managed clusters also grant get/list/watch/create/update/patch/delete on resourcequotas and limitranges; otherwise this feature blocks all gateway provisioning on those environments, not just quota. The PR body says "base and IBM overlays," but I only see the IBM overlay changed. Confidence: Medium.
[Minor] Existence checks silently pass when no resolver is wired — components/api-server/plugins/gateways/service.go
ProfileExists/ClusterExists return true when s.profiles == nil. This is documented and convenient for tests, but it means validation is silently disabled in any wiring path that omits the resolver. Consider asserting the resolver is present in production wiring so a wiring regression can't quietly drop the guarantee. Confidence: Medium.
Cross-PR coordination
Two open pull requests have material conflicts with this one that require a maintainer decision on scope, interface ownership, and merge order:
- #210 adds
GatewayVersionto theGatewayprotobuf message at field number 23 (components/api-server/proto/hypershell/v1/gateways.proto), while this PR addsprofile_idat the same field number 23 in the same message. This is a competing protobuf interface change: the two field numbers collide, and whichever merges second must be renumbered and the generated.pb.go/OpenAPI regenerated. #210 and this PR also both add a newgatewaysmigration, a newGatewaymodel field, and both extend the control-planeReconcileOpts/gateway reconciler — a coordinated merge order and a single regeneration pass are needed. - #223 removes the Fleet entity and
fleet_idacross the stack, rewriting the sameGateway/ManagedClustermodels, migrations, protobufs, and generated OpenAPI that this PR extends withprofile_id/database_id; it also changes the embedded-spec operation-count assertion inopenapi_embed_test.goin the opposite direction from this PR's42 → 47. This PR still reads/writesFleetId(patch handler, CLI--fleet-id), which #223 deletes. Maintainers should decide the merge order and which PR reconciles the shared models, migrations, generated clients, and the operation-count test.
Findings Summary (ordered by severity, highest first):
- [Major] Create-time
cluster_id/profile_idvalidation runs after placement provisions aManagedDatabase, orphaning it on a rejected create - Reconciliation / Data Integrity (service.go L130-L176) - [Major]
resourcequotas/limitrangesRBAC added only to the IBM overlay; quota failure now blocks all provisioning - Security / Deployment (controller-clusterrbac.yaml L27) - [Minor] Existence checks return
truewhen no resolver is wired, silently disabling validation - API Design (service.go L272-L286)
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 responses | Pass |
| Input validated (K8s resource quantities) | Pass |
| Reconcile pattern (update-or-create, delete-when-empty) | Pass |
| Status updated on error paths | Pass |
Context propagation (no context.TODO()) |
Pass |
| Optional→required change has legacy fallback path | Pass |
| Image references consistent | N/A |
| OpenAPI client not hand-edited | Pass (generated) |
| Control-plane RBAC complete across overlays | Needs verification |
| return nil, errors.GeneralError("gateway placement did not assign database_id") | ||
| } | ||
|
|
||
| // Validate that cluster_id references a real ManagedCluster. |
There was a problem hiding this comment.
[Major] Ordering issue: s.placement.Resolve(...) runs here first, and the deployment placement strategy's CreateForGateway creates a real per-gateway ManagedDatabase. The new cluster_id existence check (L141) and the required/existence profile_id checks (L152-L176) run afterward and can each reject with HTTP 400. On rejection the freshly created ManagedDatabase is orphaned. Validate cluster_id and profile_id before calling s.placement.Resolve(...) so bad requests never provision a server-owned database.
| return nil | ||
| } | ||
|
|
||
| func (s *sqlGatewayService) ProfileExists(ctx context.Context, profileID string) (bool, *errors.ServiceError) { |
There was a problem hiding this comment.
[Minor] ProfileExists/ClusterExists return true when s.profiles == nil, silently disabling validation. This is fine for tests, but a production wiring regression that dropped the resolver would quietly remove the guarantee. Consider requiring the resolver in production wiring (or logging a warning) so the fail-open path can't be reached unnoticed.
| # Per-namespace gateway quota enforcement (ResourceQuota + LimitRange) derived | ||
| # from the gateway's GatewayProfile. See openshell-gateway-quota.spec.md. | ||
| - apiGroups: [""] | ||
| resources: ["resourcequotas", "limitranges"] |
There was a problem hiding this comment.
[Major] Only the IBM overlay ClusterRole gains resourcequotas/limitranges. ReconcileNamespaceQuota now returns an error on any create/update failure, which fails the whole gateway reconcile. Please confirm the control plane's effective credentials on kind and OpenShift managed clusters also grant these verbs; otherwise quota enforcement blocks all gateway provisioning on those environments. The PR description mentions a base overlay, but only the IBM overlay appears to be changed.
ebbc5be to
cd13ffe
Compare
Amber reviewStatus: Complete VerdictCOMMENT — This is a well-engineered, cohesive feature: the GatewayProfile resource, quota reconciler, and end-to-end plumbing are consistent, and the tricky parts (optional→required What I liked
Findings[Minor] Asymmetric [Minor] Quota update/adopt path doesn't check the managed label (Robustness). [Minor] Cross-PR coordinationThree items require maintainer decision or ordered coordination:
Findings Summary (ordered by severity, highest first)
Convention Checklist
|
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
COMMENT — This is a well-engineered, cohesive feature: the GatewayProfile resource, quota reconciler, and end-to-end plumbing are consistent, and the tricky parts (optional→required profile_id, terminal-vs-transient profile fetch, legacy gateways with no profile) are handled deliberately. No blockers or criticals; a few minor consistency/robustness items below, plus cross-PR coordination that maintainers should decide before merge.
What I liked
- Optional→required handled correctly. New gateways require
profile_id(with a cluster-default fallback), while the control plane treats an emptyprofile_idas a legacy gateway and reconciles toward absence of quota (resolveGatewayProfileFromClientreturns(nil, nil)), so pre-existing gateways keep provisioning. This is the fallback path the review standards require for a tightened precondition. - Terminal vs. transient fetch failures.
errTerminalProfilecleanly separates "profile genuinely missing" (markFailed) from "API server momentarily unreachable" (retry without flapping the phase). A profiled gateway is never allowed to run unconstrained. - Reconcile, don't create-or-skip.
ReconcileNamespaceQuotais properly update-or-create with delete-when-empty, and only deletes objects carrying the managed label. - RBAC. Mutations on
GatewayProfileare restricted to platform admins in both the REST (isAuthorized) and gRPC (isGRPCAuthorized) paths; reads require a binding. The newresourcequotas/limitrangesverbs were added to both the base (deploy/base/controller-rbac.yaml) and IBM overlay ClusterRoles. - Error wrapping (
fmt.Errorf("…: %w", err)),IsNotFound/codes.NotFoundhandling, nopanic(), nocontext.TODO(), and no secrets in logs. Test changes are additive; the only edited assertion (operationCount42→47) is a legitimate count bump for the five new operations.
Findings
[Minor] Asymmetric profile_id validation on ManagedCluster (Spec Consistency).
ManagedCluster PATCH validates that profile_id references an existing profile, but the REST and gRPC create paths set ProfileId/DatabaseId without an existence check. A cluster can be created with a dangling default profile_id; the failure only surfaces later when a gateway that relies on the cluster default is created (profile_id is required / does not exist). Consider validating on create for parity and a clearer error.
[Minor] Quota update/adopt path doesn't check the managed label (Robustness).
In reconcileResourceQuota/reconcileLimitRange, the delete-when-empty branch guards with isManagedObject(existing.Labels), but the update branch does not: a pre-existing object named hypershell-gateway-quota/hypershell-gateway-limits that HyperShell did not create would be overwritten and stamped with managed labels. Low risk given the HyperShell-specific names, but consider guarding the update/adopt path symmetrically.
[Minor] name required on gRPC but not REST create (Spec Consistency).
CreateGatewayProfile (gRPC) requires name, but the REST Create only validates that id is empty; validateProfileFields never checks name presence, so a profile can be created via REST with an empty name. Align the two entry points.
Cross-PR coordination
Three items require maintainer decision or ordered coordination:
- #210 (reconcile gateway version): Both PRs add a new field to the
Gatewayprotobuf message at field number 23 — this PR uses it forprofile_id, #210 uses it forgateway_version. Protobuf field numbers are a wire contract, so this is a hard collision, not a text merge conflict: whichever lands second must renumber to 24 and regenerate the.pb.go, OpenAPI client, and SDK artifacts. Both PRs also add migrations and new fields to thegatewaysplugin model, so maintainers should decide a merge order and assign the renumber/regeneration to the second PR. - #223 (remove Fleet entity and
fleet_id): That PR removesfleet_idfrom the gateway/managed-cluster data model, deletes the "Fleet Isolation" requirement from the security spec, and reworks the RBAC/authz layer. This PR adds new migrations and code to the samegateways/managedClustersplugins (and still setsfound.FleetIdin the gateway PATCH handler) on the assumption that the fleet-scoped model still exists, and independently edits the same gRPC authz function. These are incompatible data-model/authz directions; maintainers need to choose a merge order and reconcile the migration sequence and the gateway/managed-cluster models so the second PR rebases cleanly. - #229 (GCP managed cluster): That PR introduces a new GCP controller
ClusterRoleoverlay that enumerates the controller's RBAC but does not include theresourcequotas/limitrangesverbs this PR adds. If both merge, the quota reconciler will fail on GCP clusters with a permissions error. Maintainers must ensure the GCP overlay's ClusterRole gains the same verbs (or that both overlays converge on the base grant) so quota enforcement is not silently broken on GCP.
Findings Summary (ordered by severity, highest first)
- [Minor] ManagedCluster create doesn't validate
profile_idexistence (only PATCH does) - Spec Consistency (managedClusters/handler.go L96) - [Minor] Quota update/adopt path overwrites same-named objects without a managed-label check - Robustness (quota.go L100-111)
- [Minor] GatewayProfile
namerequired on gRPC create but not REST create - Spec Consistency (gatewayProfiles/handler.go L34)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
errors.IsNotFound / codes.NotFound handled |
Pass |
| No secrets in logs or responses | Pass |
| Input validated (K8s quantities, IDs) | Pass |
| Reconcile pattern (update-or-create, not create-or-skip) | Pass |
| Status updated on error paths | Pass |
Proper context propagation (no context.TODO()) |
Pass |
| SecurityContext / RBAC for new cluster access | Pass |
| Image references consistent across manifests | N/A |
| OpenAPI client not manually edited | Pass |
| Test Diff Scrutiny (optional→required has fallback) | Pass |
| Conventional commit message | Pass |
| if patch.ApiServerUrl != nil { | ||
| found.ApiServerUrl = patch.ApiServerUrl | ||
| } | ||
| if patch.ProfileId != nil { |
There was a problem hiding this comment.
The PATCH path validates that profile_id references an existing GatewayProfile, but the REST Create handler (and the gRPC CreateManagedCluster) set ProfileId/DatabaseId with no existence check. A cluster can therefore be created with a dangling default profile_id; the error only surfaces later at gateway-create time (gateway profile ... does not exist). Consider mirroring this existence check on create for parity and a clearer, earlier 400.
| cfg := &handlers.HandlerConfig{ | ||
| Body: &gatewayProfile, | ||
| Validators: []handlers.Validate{ | ||
| handlers.ValidateEmpty(&gatewayProfile, "Id", "id"), |
There was a problem hiding this comment.
The gRPC CreateGatewayProfile requires name (ValidateStringField("name", ..., true)), but this REST create only validates that id is empty and validateProfileFields never checks name presence, so a profile can be created via REST with an empty name. Align the two entry points (e.g., add a ValidateEmpty/required-name check here).
cd13ffe to
2cde52e
Compare
Amber reviewStatus: Complete VerdictCOMMENT — This is a well-structured, end-to-end feature: the new SummaryThe control-plane path is careful: The one thing I want a maintainer to look at is that the gateway-create form quietly dropped hub/default (empty-cluster) provisioning and deleted the tests that proved it, inside a PR whose stated scope is quotas. |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
COMMENT — This is a well-structured, end-to-end feature: the new GatewayProfile resource, the API-boundary quantity validation, deletion protection, the update-or-create ResourceQuota/LimitRange reconciler, and the terminal-vs-transient profile-fetch handling are all thoughtfully done and match the spec. My findings are one bundled behavioral change in the gateway-create UI that removed pre-existing tests/guarantees, plus a few minor hardening/consistency items; none are blockers.
Summary
The control-plane path is careful: resolveGatewayProfileFromClient correctly treats NotFound/empty-payload as terminal (marks the gateway Failed) and everything else as transient (retries without flapping the phase), and ReconcileNamespaceQuota is genuinely idempotent (create-when-absent, update-on-drift, delete-only-managed-objects). Error wrapping, IsNotFound handling, RBAC parity between REST and gRPC, and the legacy-empty-profile fallback (reconciles toward no quota) are all present, so existing gateways keep working after the migration.
The one thing I want a maintainer to look at is that the gateway-create form quietly dropped hub/default (empty-cluster) provisioning and deleted the tests that proved it, inside a PR whose stated scope is quotas.
Findings
Major
1. Gateway-create UI silently removes hub/default provisioning and deletes the tests that guaranteed it — Test Diff Scrutiny / Scope
packages/gateway-management-ui/src/gateways/gateway-create.tsx changes the clusterId default from "" to null and tightens the resolver from if (value === null) to if (!value), making cluster selection mandatory. gateway-create.test.tsx correspondingly deletes two pre-existing tests — "provisions on the hub by default without exposing a namespace" and "keeps hub provisioning available when managed clusters fail to load" — and flips createdGateway.clusterId from "" to "cluster-east".
That removes a real capability (create a gateway with no explicit cluster) and a resilience path (degrade gracefully when the placements API is down), which is orthogonal to introducing quotas. Per Test Diff Scrutiny, deleting tests that proved the old behavior — rather than adding new coverage alongside — hides a contract change. Please either (a) call this removal out explicitly in the PR description and confirm it is intended, or (b) keep hub/default provisioning working (a user can still pick a profile explicitly without a cluster) and restore the deleted tests. Confidence: Medium.
Minor
2. GatewayProfile.Name is logged unsanitized (log-injection surface) — Security
plugins/gatewayProfiles/service.go:52 logs name=%s with a value that is only validated as non-empty (ValidateNotEmpty), never sanitized or constrained to a DNS label. Per security.spec.md (Sanitize for Log Injection), strip \n/\r before logging user-controlled strings. Confidence: High.
3. ManagedCluster.database_id is now client-settable via PATCH — confirm the ownership model — API Design / Consistency
plugins/managedClusters/handler.go:118 copies patch.DatabaseId straight onto the record with no existence check, while the sibling gateway.database_id is deliberately treated as server-owned and ignored from public input. If managed_cluster.database_id is meant to be admin-configurable that is fine, but the asymmetry is worth an explicit decision (and a referential existence check like the one applied to profile_id). Confidence: Medium.
Cross-PR coordination
The Fleet-removal effort ("remove Fleet entity and fleet_id across the stack") and this PR make incompatible platform-model assumptions and edit the same authorization and spec surfaces, so maintainers must decide a merge order and reconcile the second one. That PR deletes fleet_id from the gateways and managedClusters plugins, removes the fleet role-binding scope, and rewrites the RBAC authorization logic and the data-model.spec.md / rbac-enforcement.spec.md / security.spec.md fleet sections. This PR instead adds a new resource and new RBAC rules into the still-fleet-scoped model: it keeps Gateway.FleetId, edits the same grpc_interceptor.go/authorization.go authorization functions and their tests, and amends the same rbac-enforcement.spec.md permission matrix (still listing "Fleets" in the platform-admin denial list). Whichever lands second needs a design pass to re-home GatewayProfile RBAC and the new spec text onto the chosen data model; this is a plan/ordering decision, not a mechanical merge.
Findings Summary
- [Major] Gateway-create UI drops hub/default provisioning and deletes the tests that proved it, bundled into a quota PR - Test Diff Scrutiny / Scope (gateway-create.tsx L107, L126; gateway-create.test.tsx L47)
- [Minor]
GatewayProfile.Namelogged unsanitized (log injection) - Security (service.go L52) - [Minor]
ManagedCluster.database_idclient-settable via PATCH with no existence check, asymmetric with server-ownedgateway.database_id- API Design (handler.go L118)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
errors.IsNotFound handled for 404 scenarios |
Pass |
| Reconcile (update-or-create), not create-or-skip | Pass |
| Status updated on error paths (terminal profile -> Failed) | Pass |
| Input validated (resource quantities) | Pass |
| No secrets in logs or responses | Pass |
| Log injection prevented | Fail |
| RBAC parity REST/gRPC | Pass |
| OpenAPI client generated, not hand-edited | Pass |
| Conventional commit message | Pass |
| >({ | ||
| defaultValues: { | ||
| clusterId: "", | ||
| clusterId: null, |
There was a problem hiding this comment.
This form now defaults clusterId to null and rejects empty via if (!value), making cluster selection mandatory and removing the previous hub/default (empty-cluster) provisioning path. That capability is orthogonal to quotas — please confirm the removal is intended and call it out in the PR description, or preserve hub provisioning (a user can still pick a profile explicitly without a cluster).
|
|
||
| const createdGateway = { | ||
| clusterId: "", | ||
| clusterId: "cluster-east", |
There was a problem hiding this comment.
These changes delete two pre-existing tests ("provisions on the hub by default without exposing a namespace" and "keeps hub provisioning available when managed clusters fail to load") and flip createdGateway.clusterId from "" to "cluster-east". Per Test Diff Scrutiny, removing tests that guaranteed the old behavior hides a contract change. Restore coverage for hub/default provisioning (or add new tests documenting its intentional removal) rather than deleting it.
| return err | ||
| } | ||
|
|
||
| logger.Infof("GatewayProfile upserted: %s (name=%s)", gatewayProfile.ID, gatewayProfile.Name) |
There was a problem hiding this comment.
Log-injection surface: gatewayProfile.Name is only validated as non-empty and is logged here unsanitized. Per security.spec.md (Sanitize for Log Injection), strip \n/\r from user-controlled strings before logging.
| found.ProfileId = patch.ProfileId | ||
| } | ||
| if patch.DatabaseId != nil { | ||
| found.DatabaseId = patch.DatabaseId |
There was a problem hiding this comment.
Unlike gateway.database_id (server-owned, ignored from public input), managed_cluster.database_id is copied straight from the PATCH body with no existence check. Confirm this asymmetry is intended and consider a referential existence check like the one applied to profile_id.
2cde52e to
f8a6a6f
Compare
Amber reviewStatus: Complete VerdictCOMMENT — This is a well-structured, end-to-end feature: the SummaryThe PR introduces Findings[Major] UI silently removes hub-default gateway provisioning and its load-failure fallback — Test Diff Scrutiny / undocumented behavior change [Minor] Cross-PR coordinationAnother open pull request removes the Fleet entity and Findings Summary (highest first)
Convention Checklist
|
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
COMMENT — This is a well-structured, end-to-end feature: the GatewayProfile resource, quota validation at the API boundary, and the control-plane ReconcileNamespaceQuota (idempotent create/update/delete with a managed-label guard) are cleanly designed, and the terminal-vs-transient profile-fetch handling correctly blocks provisioning without flapping the gateway phase. My one substantive concern is an undocumented change to the gateway-create UX that silently drops two pre-existing behaviors; the rest are minor.
Summary
The PR introduces GatewayProfile (ResourceQuota totals + LimitRange container defaults), threads profile_id from create/patch through gRPC into the control plane, and reconciles a ResourceQuota/LimitRange per gateway namespace. Error handling, wrapping, IsNotFound/NotFound handling, the legacy (nil-profile) fallback path for gateways that predate the migration, and RBAC (platform-admin for profile mutation, binding-gated reads) all look correct.
Findings
[Major] UI silently removes hub-default gateway provisioning and its load-failure fallback — Test Diff Scrutiny / undocumented behavior change
gateway-create.tsx changes clusterId from a nullable field defaulting to "" ("Hub cluster (default)") into a strictly required field (if (!value) rejects both null and "", default is now null). The companion diff to gateway-create.test.tsx deletes two pre-existing tests that guaranteed prior behavior: "provisions on the hub by default without exposing a namespace" and "keeps hub provisioning available when managed clusters fail to load". The latter was a resilience guarantee (the form stayed usable when the managed-cluster API was down). The backend still accepts an empty cluster_id as long as an explicit profile_id is supplied, so this is a UI-only capability removal. Neither the removal nor the resilience regression is mentioned in the PR description. Please either restore the hub-default path (with a profile selector) and its fallback test, or call out the removal explicitly as an intended breaking change. Confidence: Medium.
[Minor] GatewayProfile PATCH can blank a required name — Input validation
gatewayProfiles/handler.go applies if patch.Name != nil { found.Name = *patch.Name } with no non-empty check, so a client sending "name": "" clears the name. Create enforces ValidateNotEmpty on name, but Replace/validateProfileFields does not re-check it, so the update path can persist an empty name. Reject empty name on patch to match create. Confidence: High.
Cross-PR coordination
Another open pull request removes the Fleet entity and fleet_id across the stack (spec, OpenAPI, backend, gRPC, RBAC, web console), rewriting tenancy to be purely RBAC-based: PR #223. It and this PR both rewrite the shared design/spec artifacts specs/platform/data-model.spec.md (the ERD) and specs/security/rbac-enforcement.spec.md, and both edit plugins/gateways/model.go, plugins/gateways/handler.go, the gateways/managedClusters OpenAPI, and the RBAC layer. The conflict is logical, not just textual: this PR adds GatewayProfile as a new top-level entity plus profile_id relationships and a new RBAC rule, while #223 deletes the Fleet grouping and fleet_id that this PR's gateway create/patch code still reads (found.FleetId = *patch.FleetId). Maintainers need to (a) decide a merge order, (b) reconcile the ERD and RBAC spec so they aren't independently rewritten, and (c) confirm where GatewayProfile sits under the fleet-less tenancy model. This needs a decision by the two PR owners together.
Findings Summary (highest first)
- [Major] UI drops hub-default provisioning + managed-cluster load-failure fallback; two pre-existing tests deleted, behavior not documented - Test Diff Scrutiny (gateway-create.tsx, gateway-create.test.tsx)
- [Minor]
GatewayProfilePATCH allows clearing the requirednameto""- Input validation (gatewayProfiles/handler.go L66)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
errors.IsNotFound / gRPC NotFound handled |
Pass |
| No secrets in logs or responses | Pass |
| Input validated (quantities, counts, references) | Pass |
| SecurityContext / quota on managed objects | Pass |
| Reconcile pattern (update-or-create, delete-when-empty) | Pass |
| Optional→required field has legacy/backfill path | Pass |
| Test Diff Scrutiny (removed guarantees justified) | Fail |
| OpenAPI client not hand-edited | Pass |
| Conventional commit message | Pass |
| .nullable() | ||
| .transform((value, context) => { | ||
| if (value === null) { | ||
| if (!value) { |
There was a problem hiding this comment.
[Major] Undocumented behavior change: hub-default provisioning removed.
clusterId is now strictly required (if (!value) rejects both null and "", and the default became null). Previously an empty clusterId meant "Hub cluster (default)". The companion test file deletes two pre-existing tests that guaranteed this: "provisions on the hub by default..." and "keeps hub provisioning available when managed clusters fail to load" — the latter a resilience guarantee for when the managed-cluster API is unavailable.
The backend still accepts an empty cluster_id when an explicit profile_id is supplied, so this is a UI-only capability removal that isn't mentioned in the PR description. Please restore the hub-default path (with a profile selector) and its fallback test, or explicitly document this as an intended breaking change.
There was a problem hiding this comment.
this is fine until we enhance the PlacementResolver and have the logic for where to place a gateway
| return nil, err | ||
| } | ||
|
|
||
| if patch.Name != nil { |
There was a problem hiding this comment.
[Minor] PATCH can blank the required name.
if patch.Name != nil { found.Name = *patch.Name } has no non-empty check, so "name": "" clears the name. Create enforces ValidateNotEmpty("name"), but Replace/validateProfileFields never re-validate it, so the update path can persist an empty name. Reject an empty name here to match create.
f8a6a6f to
0a8be53
Compare
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, end-to-end GatewayProfile feature with strong error wrapping, terminal-vs-transient fetch semantics, deletion protection, and genuinely additive tests. One design gap around LimitRange default limits (below) can silently break admission of namespace workloads under a limits-enforcing profile and deserves discussion before merge; the rest are minor consistency nits.
Summary
The PR introduces a GatewayProfile resource and enforces its ResourceQuota/LimitRange on each gateway namespace, plumbed across API server, control plane, SDKs, CLI, and console. The control-plane reconcile path is careful (fetch-before-provision, fail-closed on a terminal profile error, retry on transient), migrations backfill legacy rows to an empty/nil profile with a compatible legacy code path, and validation happens at the API boundary — but the enforced LimitRange never sets default limits, which interacts badly with the limits.cpu/limits.memory totals the seed profiles set.
Findings
Major
1. LimitRange sets DefaultRequest and Max but never Default (default limits) — components/control-plane/internal/gateway/quota.go.
desiredLimitRangeItem populates DefaultRequest (default requests) and Max (max limits) only. When a profile sets cpu_limit_total/memory_limit_total — all three seeded profiles do (small = cpu_limit_total:"4", memory_limit_total:"4Gi") — the ResourceQuota enforces limits.cpu/limits.memory, which makes an explicit per-container CPU/memory limit mandatory for admission. The Kubernetes LimitRanger only backfills a container limit from LimitRangeItem.Default, never from Max; with Default unset, any container in the namespace that omits limits is rejected.
ReconcileNamespaceQuota runs before reconcileCNPGDatabaseResources (same gateway namespace). CNPG-managed PostgreSQL pods do not set container limits by default, so a limits-enforcing profile can block per-gateway database provisioning, plus any future/operator-injected sidecar. The manual cpu limit added to the console container (console.go) is a symptom of this gap — it had to be added by hand because the LimitRange does not backfill limits.
Fix: also set LimitRangeItem.Default (e.g., derive default limits from ContainerCPULimitMax/ContainerMemoryLimitMax, or add explicit default-limit profile fields) so containers that omit limits inherit a quota-compliant limit. Confidence: Medium (Kubernetes LimitRanger mechanics are certain; the CNPG-pod impact depends on CNPG defaults and whether the in-namespace DB path is exercised).
Minor
2. Inconsistent empty-profile_id handling across write paths. REST PATCH rejects profile_id:"" with a clear validation error (gateways/handler.go), but gRPC UpdateGateway silently ignores an empty profile_id (gateways/grpc_handler.go:147, guarded by *req.ProfileId != ""). Align the two so the contract is consistent. Confidence: High.
3. managed_cluster.database_id is newly client-writable with no validation or documented purpose (managedClusters/handler.go:114, grpc_handler.go, model.go), in contrast to gateway.database_id, which is deliberately server-owned and cleared. Confirm the asymmetry is intended and consider validating/documenting the field. Confidence: Medium.
Cross-PR coordination
A separate in-flight effort moves gateway workload deployment from the control-plane's static manifests to the upstream OpenShell Helm chart (PR #194). It reworks the same reconcile surface this PR extends — internal/gateway/config.go (ReconcileOpts) and internal/gateway/reconciler.go (ReconcileGateway) — and its gap analysis of "what the control plane manages in the gateway namespace" lists only the OpenShift SCC binding and the trusted-CA ConfigMap, explicitly not a ResourceQuota/LimitRange. This PR adds exactly that quota/LimitRange enforcement and assumes control-plane-owned pod specs it can adjust so they satisfy the quota (e.g., the manual console CPU-limit change). Those two views of gateway-namespace ownership are incompatible: under Helm-owned pod specs, the control plane can no longer patch limits into workloads to fit an enforced quota, and combined with Finding 1 (no default limits) Helm-deployed pods that omit limits would be rejected. Maintainers should decide the ownership boundary and the merge order — whether quota/LimitRange enforcement is folded into the Helm values mapping, and whether the Helm chart guarantees compliant per-container limits — before both land.
Findings Summary (ordered by severity, highest first)
- [Major] LimitRange never sets default limits, so a limits-enforcing profile can reject namespace workloads (incl. CNPG DB pods) that omit limits — Control Plane / Reconciliation (quota.go:
desiredLimitRangeItem) - [Minor] Empty
profile_idaccepted-silently on gRPC update but rejected on REST patch — API Consistency (grpc_handler.go:147, handler.go:116) - [Minor]
managed_cluster.database_idnewly client-writable, unvalidated and undocumented — API Design (managedClusters/handler.go:114)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
errors.IsNotFound / gRPC NotFound handled |
Pass |
| No secrets in logs or responses | Pass |
| Input validated (quantities, names) | Pass |
| Reconcile pattern (update-or-create, delete-when-empty) | Pass |
| Status updated on error paths (terminal → Failed, transient → retry) | Pass |
Proper context propagation (no context.TODO()) |
Pass |
| Optional→required field has legacy fallback/backfill | Pass |
| Test diff scrutiny (no flipped pre-existing assertions) | Pass |
| OpenAPI client generated, not hand-edited | Pass |
| Conventional commit messages | Pass |
| item.Max[m.name] = parsed | ||
| } | ||
|
|
||
| nonEmpty := len(item.DefaultRequest) > 0 || len(item.Max) > 0 |
There was a problem hiding this comment.
[Major] This LimitRangeItem sets DefaultRequest (default requests) and Max (max limits) but never Default (default limits). The Kubernetes LimitRanger backfills a missing container limit only from Default, never from Max. When a profile sets cpu_limit_total/memory_limit_total (all three seeded profiles do), the generated ResourceQuota enforces limits.cpu/limits.memory, which makes an explicit per-container limit mandatory for admission. Any container in the namespace that omits limits — notably CNPG-managed PostgreSQL pods provisioned into this same namespace after ReconcileNamespaceQuota runs — is then rejected, which can block per-gateway database provisioning. Consider also populating item.Default (e.g. from ContainerCPULimitMax/ContainerMemoryLimitMax) so omitted limits inherit a quota-compliant default.
| } | ||
| // database_id is server-owned placement state. Ignore values supplied by | ||
| // callers; gateway creation business logic is the only assignment path. | ||
| if req.ProfileId != nil && *req.ProfileId != "" { |
There was a problem hiding this comment.
[Minor] gRPC UpdateGateway silently ignores an empty profile_id (*req.ProfileId != ""), while REST PATCH rejects profile_id:"" with an explicit profile_id cannot be removed... validation error. Align the two write paths so the API contract is consistent.
| found.ProfileId = patch.ProfileId | ||
| } | ||
| if patch.DatabaseId != nil { | ||
| found.DatabaseId = patch.DatabaseId |
There was a problem hiding this comment.
[Minor] database_id is now client-writable via PATCH (and gRPC) with no validation and no documented purpose, unlike gateway.database_id, which is deliberately server-owned and cleared. Please confirm this asymmetry is intentional and consider validating/documenting the field.
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
APPROVE-leaning COMMENT. This is a large, well-structured feature that lands GatewayProfile end-to-end (API server plugin, gRPC, control-plane quota reconciler, SDKs, CLI, web console) with genuinely careful error handling, an idempotent update-or-create-or-delete quota reconciler, a legacy-safe profile_id="" path for pre-existing gateways, and admin-gated mutation RBAC. I have no blockers; the notes below are one correctness footgun worth a decision, plus two minor items and one cross-PR coordination point.
What I verified
- Error handling / reconcile discipline —
ReconcileNamespaceQuotais a clean update-or-create, no-op-on-equal, delete-when-empty loop that only deletes objects carrying the managed label;IsNotFoundis handled on every get/delete.resolveGatewayProfileFromClientcorrectly distinguishes terminal (NotFound/empty payload → wrapserrTerminalProfile, marks the gatewayFailed) from transient failures (left unwrapped → retry without flapping). Nopanic(); errors are wrapped with%wand context. - Legacy safety (test-diff scrutiny) — the new
gateway.profile_idmigration backfills existing rows to"", and both the API create fallback and the control-plane resolver treat""as "no profile → reconcile toward absence of quota". Existing gateways therefore keep reconciling. Deletion protection (HTTP 409) prevents a referenced profile from becoming a dangling reference. Good. - Input validation — quantities validated with
resource.ParseQuantity(non-negative, request ≤ limit) at the API boundary and again in the control plane. RBAC mutations gated on platform-admin (REST + gRPC), reads open to any binding holder. - Manifests —
resourcequotas/limitrangesverbs added to the base/openshift ClusterRole, inherited by the IBM overlay via../openshift, mirrored in the kind overlay. Consistent.
Findings
1. [Major] A profile can set namespace quota totals without the matching LimitRange container defaults, which can make pods unschedulable.
desiredResourceQuotaSpec and desiredLimitRangeItem are built independently, and validateProfileFields cross-checks only request≤limit — it does not require that when a *_request_total is set, a corresponding container_*_request_default exists. In Kubernetes, once a ResourceQuota constrains requests.cpu/requests.memory, every pod in the namespace must declare that request or inherit it from a LimitRange default, or admission rejects it. A profile with only namespace totals and no container defaults can therefore block the gateway's own workload (and any tenant pod that omits requests) from being admitted — surfacing as a confusing scheduling failure well after the profile was accepted. Please either enforce the coupling in validateProfileFields (a request-total requires a container default) or document the constraint on the profile API. Confidence: Medium.
2. [Minor] A deleted UI test removed a graceful-degradation guarantee rather than adapting it.
gateway-create.test.tsx drops keeps hub provisioning available when managed clusters fail to load and the hub-default cases, and flips the required-field count from 1 to 2. Requiring cluster + profile is an intended, documented change, but the deleted test proved the create form still worked when the cluster list failed to load; with cluster now mandatory there is no replacement test covering that failure path. Consider adding a test for the new "cluster load fails" UX so the behavior is pinned rather than silently dropped. Confidence: Medium.
3. [Minor] OnUpsert logs an unsanitized, user-controlled resource name (log-injection vector).
GatewayProfile upserted: ... (name=%s) and Gateway upserted: ... (name=%s ...) log names that are not validated as DNS labels and can contain newlines/control characters (security.spec.md "Sanitize for Log Injection"). Low severity and consistent with existing patterns, but worth stripping \r/\n before logging. Confidence: Low.
Cross-PR coordination
The Helm-chart gateway-deployment PR and this PR both edit the same ReconcileOpts struct region in internal/gateway/config.go and both restructure the body of ReconcileGateway, but the substantive issue is an ownership/design decision, not the textual overlap: this PR reconciles the per-namespace ResourceQuota/LimitRange as standalone control-plane-managed objects applied before workload deploy, whereas that PR makes the gateway namespace's resources a Helm release whose uninstall "removes all chart-managed resources in the namespace." Maintainers should decide who owns namespace-scoped quota once gateways deploy via Helm — express the profile-derived quota as chart values owned by the release, or keep it as a separate reconcile step ordered before the Helm install — and sequence the two merges accordingly so the quota is not left unmanaged or double-owned.
Findings Summary (ordered by severity, highest first)
- [Major] Profile quota totals decoupled from LimitRange container defaults can block pod admission - Design / Correctness (quota.go L173, validate.go L82)
- [Minor] Deleted UI test removed the cluster-load-failure graceful-degradation guarantee - Test Diff Scrutiny (gateway-create.test.tsx L338)
- [Minor] Unsanitized user-controlled name in
OnUpsertlogs - Security / Log Injection (gatewayProfiles/service.go L52, gateways/service.go L99)
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 responses | Pass |
| Input validated (K8s quantities, existence checks) | Pass |
| Reconcile pattern (update-or-create, not create-or-skip) | Pass |
| Proper context propagation | Pass |
| RBAC mutations admin-gated (REST + gRPC) | Pass |
| Image/manifest references consistent across overlays | Pass |
| OpenAPI/gRPC clients regenerated, not hand-edited | Pass |
| Conventional commit messages | Pass |
| Log injection prevented | Fail |
| return svcErr | ||
| } | ||
|
|
||
| crossChecks := []struct { |
There was a problem hiding this comment.
[Major] The cross-checks validate only request ≤ limit. They do not require that when a *_request_total is set, the matching container_*_request_default is also set. Once a ResourceQuota constrains requests.cpu/requests.memory, every pod must declare that request or inherit it from a LimitRange default, or admission rejects it — so a totals-only profile can make the gateway's own pods unschedulable. Consider enforcing the coupling here (a request-total requires a container default) or documenting the constraint on the profile API.
| // desiredResourceQuotaSpec builds the ResourceQuota hard map from quota, | ||
| // omitting empty/zero fields. It reports whether any field is set; an empty | ||
| // map means no ResourceQuota should exist. | ||
| func desiredResourceQuotaSpec(quota *QuotaConfig) (corev1.ResourceList, bool, error) { |
There was a problem hiding this comment.
desiredResourceQuotaSpec (namespace totals) and desiredLimitRangeItem (container defaults) are derived independently. When a profile sets request totals but no container defaults, pods that omit requests will be rejected by the quota at admission. Pairs with the validation note in validate.go — either couple the two or document that a request-total profile must also supply container defaults.
| @@ -397,11 +336,67 @@ describe("GatewayCreatePage", () => { | |||
| await user.click(screen.getByRole("button", { name: "Provision gateway" })); | |||
|
|
|||
| expect(await screen.findAllByText("This field is required.")).toHaveLength( | |||
There was a problem hiding this comment.
[Minor] This PR deletes keeps hub provisioning available when managed clusters fail to load and the hub-default cases, and flips this required-field count from 1 to 2. Requiring cluster + profile is intended, but the deleted test proved the form still worked when the cluster list failed to load, and there is no replacement for that failure path now that a cluster is mandatory. Consider adding a test for the new "cluster load fails" UX rather than dropping the guarantee.
| return err | ||
| } | ||
|
|
||
| logger.Infof("GatewayProfile upserted: %s (name=%s)", gatewayProfile.ID, gatewayProfile.Name) |
There was a problem hiding this comment.
[Minor] name is user-controlled and not validated as a DNS label, so a newline/control char can forge log lines (security.spec.md “Sanitize for Log Injection”). Strip \r/\n before logging. Same pattern in gateways/service.go OnUpsert.
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 large, carefully engineered feature that plumbs a new GatewayProfile resource end-to-end and correctly handles legacy (profile-less) gateways with a nil-quota fallback, proper error wrapping, IsNotFound/gRPC-code handling, and parameterized SQL. I found no blockers or critical issues; the notes below are minor code-quality/completeness items, plus cross-PR coordination that maintainers must resolve before merge.
Strengths
resolveGatewayProfiledistinguishes terminal (NotFound, empty payload) from transient failures viaerrTerminalProfile+errors.Is, blocking provisioning without flapping the phase — a genuinely subtle case handled well.ReconcileNamespaceQuotais true update-or-create with semanticDeepEqual, deletes only label-managed objects, and reconciles toward absence for legacy gateways (emptyprofile_id). This satisfies the optional→required Test-Diff-Scrutiny concern: pre-existing gateways with a blankprofile_idkeep working instead of hard-failing.- Field validation via
resource.ParseQuantityat the API boundary (HTTP 400) with request≤limit cross-checks; deletion protection returns HTTP 409 for referenced profiles; RBAC restricts profile mutation to platform admins while keeping reads open.
Findings
[Minor] ManagedCluster.database_id is stored end-to-end but never consumed. profile_id has a real create-time fallback (ClusterDefaultProfileID), but the parallel cluster-level database_id (model, migration, gRPC, OpenAPI, patch handler) has no reader — gateway placement still assigns the database itself and never consults the cluster default. As shipped this is dead config whose OpenAPI description ("Default ManagedDatabase identifier for gateways placed on this cluster") over-promises behavior that does not exist. Either wire a placement fallback or drop the field until it is used.
[Minor] REST and gRPC disagree on clearing profile_id. The REST PATCH path rejects an empty profile_id with HTTP 400 ("cannot be removed…"), but gRPC UpdateGateway silently ignores an empty value (if req.ProfileId != nil && *req.ProfileId != ""). The invariant "a gateway always has a profile" is enforced on one interface and not stated on the other. Consider mirroring the rejection (or an explicit comment) so the two write paths encode the same contract.
[Minor] Removed UI resilience guarantee. gateway-create.test.tsx deletes the pre-existing tests "provisions on the hub by default…" and "keeps hub provisioning available when managed clusters fail to load", and flips the required-field count from 1 to 2. This is an intentional UX tightening (cluster + profile now required), but it removes the degraded-mode path where a user could still provision when the cluster/profile lists fail to load. Please confirm that blocking gateway creation entirely on a profile-list load failure is the intended behavior, and consider a replacement test for the new degraded state rather than a bare deletion.
Cross-PR coordination
Two open pull requests require maintainer coordination with this one:
-
#210 adds a new field to the same
Gatewayprotobuf message at the same field number — this PR assignsoptional string profile_id = 23;while #210 assignsoptional string gateway_version = 23;. This is a wire-level collision, not a text merge conflict: whichever merges second must renumber to field 24 and regenerate the.pb.go, OpenAPI, and Go/TypeScript SDK artifacts accordingly. Both PRs also independently change theNewGatewayService/GatewayServiceconstructor and add a gateways migration, so a merge order and the field-number allocation need to be decided explicitly. -
#207 establishes a cross-cutting invariant that every resource embeds
TraceMeta(traceparent/tracestate) and propagates trace context through the gRPCObjectReference, applied uniformly across all existing plugins. This PR introduces an entirely newgatewayProfilesplugin and gRPC service that would fall outside that invariant. Maintainers should decide merge order and who extends the trace-correlation contract to the new resource so the newGatewayProfilecreate/update path is not silently excluded from reconcile-to-request correlation.
Findings Summary (ordered by severity, highest first)
- [Minor]
ManagedCluster.database_idadded end-to-end but never consumed; OpenAPI over-promises a fallback that does not exist — Spec Completeness / Dead Config - [Minor] REST rejects empty
profile_idon PATCH but gRPCUpdateGatewaysilently ignores it — inconsistent write-path contract — API Design - [Minor] Removed UI resilience tests (hub/degraded provisioning) without replacement coverage — Test Diff Scrutiny
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf("...: %w", err) |
Pass |
errors.IsNotFound / gRPC codes handled for 404 |
Pass |
| No secrets in logs or responses | Pass |
| Input validated (quantities, counts, request≤limit) | Pass |
| Reconcile pattern (update-or-create, not create-or-skip) | Pass |
| Optional→required change has legacy fallback/backfill | Pass |
| SecurityContext on pod specs | N/A (quota/limit objects only) |
| OpenAPI client generated, not hand-edited | Pass |
| Conventional commit messages | Pass |
| found.ProfileId = patch.ProfileId | ||
| } | ||
| if patch.DatabaseId != nil { | ||
| found.DatabaseId = patch.DatabaseId |
There was a problem hiding this comment.
database_id is persisted, presented, and patchable on ManagedCluster, but nothing consumes it: gateway placement assigns the database itself (gateways/placement.go, service.go clears then re-sets DatabaseId) and never reads the cluster's default. The OpenAPI text calls it the "Default ManagedDatabase identifier for gateways placed on this cluster," which describes behavior this PR does not implement. Contrast with profile_id, which has a real fallback via ClusterDefaultProfileID. Either wire a placement fallback or drop this field until a consumer exists.
| } | ||
| // database_id is server-owned placement state. Ignore values supplied by | ||
| // callers; gateway creation business logic is the only assignment path. | ||
| if req.ProfileId != nil && *req.ProfileId != "" { |
There was a problem hiding this comment.
The REST PATCH path rejects an empty profile_id with HTTP 400 ("profile_id cannot be removed from a gateway…"), but here an empty value is silently ignored (*req.ProfileId != ""). The "a gateway always has a profile" invariant is enforced on one write path and unstated on the other. Consider mirroring the rejection or adding an explicit comment so both interfaces encode the same contract.
| @@ -397,11 +336,67 @@ describe("GatewayCreatePage", () => { | |||
| await user.click(screen.getByRole("button", { name: "Provision gateway" })); | |||
|
|
|||
| expect(await screen.findAllByText("This field is required.")).toHaveLength( | |||
There was a problem hiding this comment.
This change (required-field count 1→2) plus the deletion of "provisions on the hub by default…" and "keeps hub provisioning available when managed clusters fail to load" removes the previously-guaranteed degraded-mode path where a user could still provision when placement/profile data failed to load. Per Test Diff Scrutiny, a removed guarantee should be replaced, not just deleted: please confirm blocking creation entirely on a profile-list load failure is intended, and add a test for the new degraded behavior.
dfa1f58 to
c91b72f
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 GatewayProfile resource and end-to-end quota enforcement, with sound reconcile-toward-desired-state semantics, correct terminal-vs-transient gRPC error handling, and a proper legacy fallback (empty profile_id reconciles toward no quota, so pre-existing gateways are not broken by the new required field). The main concern is that the new quota/profile resolution sits after the control plane's phase gate, so profile reassignment and quota edits silently do not re-apply to already-running gateways.
What works well
- Optional -> required handled correctly. Making
profile_idrequired at gateway create is paired with a real fallback path: the control plane treats an emptyprofile_idas a legacy gateway and reconciles toward absence of quota, so existing DB rows do not fail reconciliation. No flipped/removed test guarantees were found (the modified assertions inopenapi_embed_test.go,authorization_test.go, andgrpc_interceptor_test.goare additive count/role bumps, not contract reversals). - Idempotent reconcile.
ReconcileNamespaceQuotais update-or-create with delete-when-empty, only ever deletes objects carrying the managed label, and usesapiequality.Semantic.DeepEqual(which comparesresource.Quantityby value) to avoid spurious updates from server-side canonicalization. - Error handling. Terminal (
NotFound/empty payload) vs transient fetch failures are distinguished viaerrTerminalProfile+errors.Is; terminal marks the gatewayFailed(which is not in the skip set, so it retries), transient leaves the phase untouched to avoid flapping. Errors are wrapped with context throughout; nopanic()in production paths. - RBAC. Profile mutations require platform admin on both REST (
authorization.go) and gRPC (grpc_interceptor.go); reads stay open to any caller holding a binding. Deletion protection returns 409 when a profile is referenced by a cluster default or any gateway. - Security. Validation via
resource.ParseQuantityat the API boundary (HTTP 400), non-negative and request<=limit cross-checks, and no secret values in the new fields or logs. Parameterized DAO queries.
Findings (see inline comments)
- [Major] Quota/profile resolution runs after the
Running/Provisioning/Degradedphase gate, so profile reassignment and profile edits do not re-apply to live gateways - Reconciliation (control-plane reconciler.go:1400). - [Major] Gateway PATCH comment claims reassignment "re-triggers reconcile," which the control-plane gate contradicts - Consistency (gateways/handler.go:118).
- [Minor] Nil-
ProfileResolverwarning says validation is skipped, but the required-profile_idcheck still rejects the create - Consistency (gateways/service.go:160).
Cross-PR coordination
A separate open pull request (#151) changes the exact reconcile gate this PR depends on: it proposes gating gateway re-provisioning on desired-state convergence so that spec changes to Running/Provisioning/Degraded gateways actually re-apply instead of returning early. This PR's quota re-application and profile_id reassignment are only effective once such drift handling exists, and this PR's PATCH path already promises that behavior. The maintainers must decide whether profile_id (and the derived quota) is part of the convergence/drift set that PR #151 acts on, and coordinate merge order so the two designs agree on when a live gateway is re-provisioned.
A separate open pull request (#194) reworks how gateways are deployed - moving from static manifests to the upstream OpenShell Helm chart via the Helm Go SDK, and restructuring internal/gateway/reconciler.go, internal/gateway/config.go (GatewayDeployOptions), and internal/reconciler/reconciler.go. This PR inserts ReconcileNamespaceQuota into that same deploy flow and adds a Quota field to GatewayDeployOptions, relying on the ordering "apply quota before deploying the workload." A maintainer decision is needed on where quota/LimitRange enforcement lives once gateways deploy via Helm (and whether the chart's own pod resource specs must satisfy the enforced ResourceQuota), plus coordinated merge ordering so the quota step is preserved in the Helm path rather than lost in the restructure.
Findings Summary (ordered by severity, highest first)
- [Major] Quota/profile resolution sits after the phase gate; live gateways never re-apply quota or honor profile reassignment - Reconciliation (reconciler.go:1400)
- [Major] PATCH comment claims reassignment re-triggers reconcile; control-plane gate blocks it - Consistency (handler.go:118)
- [Minor] Nil-resolver warning vs still-enforced required
profile_id- Consistency (service.go:160)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
errors.IsNotFound / NotFound handled |
Pass |
| No secrets in logs or responses | Pass |
| Input validated (K8s quantities, non-negative, req<=limit) | Pass |
| Reconcile pattern (update-or-create, delete-when-empty) | Pass |
| Status updated on error paths | Pass |
| Optional->required has legacy fallback | Pass |
| OpenAPI client generated, not hand-edited | Pass |
| Test diff scrutiny (no flipped assertions) | Pass |
| Behavior matches documented intent | Fail |
| // Resolve the gateway's quota profile before provisioning. A fetch failure | ||
| // blocks provisioning (a profiled gateway must not run unconstrained); an | ||
| // empty profile_id yields nil, reconciling toward no quota (legacy). | ||
| quotaConfig, profileErr := r.resolveGatewayProfile(ctx, gw) |
There was a problem hiding this comment.
[Major] Quota resolution runs after the phase gate. Handle returns early for any gateway whose phase is Running, Provisioning, or Degraded (reconciler.go:1278-1282), and this resolveGatewayProfile / ReconcileNamespaceQuota path only executes below that gate. Consequences:
- Editing a
GatewayProfile's quantities does not re-apply the namespaceResourceQuota/LimitRangefor any already-Runninggateway. - Re-assigning a gateway's
profile_id(allowed by the PATCH handler) does not take effect until the gateway leaves those phases.
Initial provisioning works (new gateways have an empty phase), so the feature is not broken end-to-end, but the update path silently no-ops. Please either make profile/quota part of the drift set that bypasses the gate, or document this limitation. Confidence: High.
| // through the public API; only gateway creation business logic assigns it. | ||
| if patch.ProfileId != nil { | ||
| // A gateway must always have a profile: reject clearing it. Reassignment | ||
| // to a different existing profile is allowed and re-triggers reconcile. |
There was a problem hiding this comment.
[Major] This comment states reassignment "re-triggers reconcile," but on the control-plane side the reconcile gate (reconciler.go:1278-1282) returns early for Running/Provisioning/Degraded gateways before the new quota-resolution code runs. A profile reassignment on a live gateway is persisted and emits an update event, yet the quota is never re-applied. Align the comment with actual behavior, or ensure a profile change forces re-provisioning. Confidence: High.
| gateway.ProfileId = clusterDefault | ||
| } | ||
| if gateway.ProfileId == "" { | ||
| return nil, errors.Validation("profile_id is required: none supplied and cluster %s has no default profile", gateway.ClusterId) |
There was a problem hiding this comment.
[Minor] When s.profiles == nil the warning above (service.go:131) says cluster/profile validation is skipped, but this required-profile_id check still fires, so a nil-resolver create with no profile_id is rejected as "required" rather than being fully skipped. Minor inconsistency between the logged intent and the enforced behavior; worth reconciling so the no-resolver path (tests/wiring regression) behaves as documented. Confidence: Medium.
c91b72f to
7083a8b
Compare
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, well-tested feature: it introduces the GatewayProfile resource end-to-end, enforces per-namespace ResourceQuota/LimitRange through an idempotent reconcile-toward-desired-state, and correctly treats legacy gateways (empty profile_id) as "no quota" so pre-existing records keep provisioning. Findings are minor; the main action item is cross-PR coordination on the gateway provisioning path.
Amber Analysis
The change is disciplined: input is validated at the API boundary with resource.ParseQuantity, the control-plane reconciler is idempotent (create/update/delete-when-empty, only touching managed-labelled objects), errors are wrapped with context, no panic() in production paths, and terminal vs. transient profile-fetch failures are distinguished so a blip does not flap the gateway phase to Failed. Migrations add gateway.profile_id (non-null, defaults to "" → legacy/no-quota) and nullable managed_cluster.profile_id/database_id, giving existing rows a clean fallback path, and RBAC restricts profile mutation to platform admins while leaving reads open to binding holders. New behavior is covered by unit, integration, and E2E tests, and the create-time contract (profile required, with cluster-default fallback) is enforced consistently across REST, gRPC, and CLI.
Strengths
resolveGatewayProfileFromClientcleanly separates terminal (NotFound, nil payload →errTerminalProfile) from transient failures, and blocks provisioning rather than letting a profiled gateway run unconstrained — this is the correct safety posture and is well tested.ReconcileNamespaceQuotareconciles toward absence and only deletes objects carrying the managed label, avoiding clobbering operator-created quotas.- Deletion protection (409 when a profile is referenced by a cluster default or any gateway) prevents dangling references that would block provisioning.
- The optional→required transition for
profile_idis handled safely: existing gateways withprofile_id == ""reconcile as legacy (no quota), so there is a real fallback path rather than a bare validation error.
Minor findings
- [Minor]
validateRequestNotExceedsLimitdiscards parse errors (components/api-server/plugins/gatewayProfiles/validate.go:44). The function relies onvalidateProfileFieldshaving already parsed each quantity, soreq, _ := resource.ParseQuantity(...)is safe today — but only by ordering. If the call order ever changes, a malformed value would silently compare as zero. Consider parsing once and reusing, or asserting the error. - [Minor] Cross-table raw SQL couples the profiles DAO to other plugins' schemas (
components/api-server/plugins/gatewayProfiles/dao.go:94,103).ExistsByClusterProfileID/ExistsByGatewayProfileIDhard-codemanaged_clusters/gatewaystable names. Queries are parameterized (no injection), but a future rename of those tables would silently break deletion protection. Consider driving these checks through the owning services or documenting the coupling. - [Minor]
Createstill requiresprofile_idwhen no resolver is wired (components/api-server/plugins/gateways/service.go:130). The comment says a nilProfileResolver"disables cluster/profile validation," but the laterprofile_id is requiredcheck (line 160) still fires, so the nil-resolver path is not fully validation-free. Harmless (production always injects a resolver), but the comment slightly overstates the behavior.
Cross-PR coordination
- #194 (adopt upstream OpenShell Helm chart for gateway deployments): This PR injects
ReconcileNamespaceQuotadirectly into the gateway provisioning function incomponents/control-plane/internal/gateway/reconciler.goand adds aQuotafield toReconcileOptsinconfig.go. #194 rewrites that same provisioning path to deploy (and, on delete,Uninstall) all chart-managed resources via Helm and modifiesReconcileOptsin the same struct region. Maintainers need to decide whether the per-namespaceResourceQuota/LimitRangecontinues to be applied directly by the control plane (this PR) or becomes part of the Helm chart/values (#194), and to agree a merge order — otherwise the Helm-based delete path could leave the separately-applied quota objects orphaned, and the twoReconcileOptsextensions will need manual reconciliation.
Findings Summary (ordered by severity, highest first):
- [Minor]
validateRequestNotExceedsLimitignoresParseQuantityerrors, relying on call ordering - Robustness (validate.go L44) - [Minor] Cross-table raw SQL hard-codes other plugins' table names in the profiles DAO - Maintainability (dao.go L94, L103)
- [Minor] Nil-
ProfileResolverpath still enforcesprofile_id, so comment overstates "validation disabled" - Clarity (service.go L130)
Convention Checklist (omit conventions not applicable to the diff):
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
errors.IsNotFound / gRPC NotFound handled |
Pass |
| No secrets in logs or responses | Pass |
| Input validated (K8s quantities, counts) | Pass |
| Reconcile pattern (update-or-create, not create-or-skip) | Pass |
| Status updated on error paths (terminal → Failed) | Pass |
Context propagation (no context.TODO()) |
Pass |
| Image references consistent | N/A |
| Migration provides fallback for pre-existing rows | Pass |
| OpenAPI client generated (not hand-edited) | Pass |
| RBAC restricts privileged mutation | Pass |
| Conventional commit message | Pass |
| if requestValue == nil || *requestValue == "" || limitValue == nil || *limitValue == "" { | ||
| return nil | ||
| } | ||
| req, _ := resource.ParseQuantity(*requestValue) |
There was a problem hiding this comment.
[Minor] req, _ := resource.ParseQuantity(*requestValue) discards the parse error. This is safe only because validateProfileFields parses (and rejects) every quantity earlier in the same pass. If that ordering ever changes, a malformed value would parse to the zero value and silently pass the request-vs-limit check. Consider parsing once (reusing the already-validated quantities) or handling the error explicitly.
| func (d *sqlGatewayProfileDao) ExistsByClusterProfileID(ctx context.Context, profileID string) (bool, error) { | ||
| g2 := (*d.sessionFactory).New(ctx) | ||
| var count int64 | ||
| if err := g2.Raw("SELECT COUNT(*) FROM managed_clusters WHERE profile_id = ? AND deleted_at IS NULL", profileID).Scan(&count).Error; err != nil { |
There was a problem hiding this comment.
[Minor] Deletion protection reaches into other plugins' tables via raw SQL (managed_clusters here, gateways on L103). Parameterized, so no injection risk, but the table names are hard-coded, so a future rename in those plugins would silently break this guard. Consider routing these existence checks through the owning services, or at least noting the cross-schema coupling here.
| // A nil resolver disables cluster/profile validation and quota enforcement. | ||
| // Production wiring always injects one (see NewServiceLocator), so this path | ||
| // is reachable only from tests or a wiring regression; make the latter loud. | ||
| if s.profiles == nil { |
There was a problem hiding this comment.
[Minor] The comment says a nil ProfileResolver "disables cluster/profile validation," but the profile_id is required check below (L160) still runs on this path, so a nil resolver is not fully validation-free. Harmless in production (a resolver is always injected), but the comment slightly overstates the behavior — worth a one-line clarification.
|
Closing this PR until we understand better how users use the platform |
2 participants
Summary
ResourceQuotatotals andLimitRangecontainer defaults; every gateway must reference a profile so the control plane enforces resource constraints on the namespace it provisionsWhat changed
API server
gatewayProfilesplugin: model, DAO, service, gRPC handler/presenter, REST handler/presenter, plugin registration, DB migrationresource.ParseQuantityat the API boundary — invalid values return HTTP 400profile_id; falls back to the cluster'sprofile_id; returns HTTP 400 if neither source yields a valuegateway.profile_idandmanaged_cluster.profile_id / database_idControl plane
ReconcileNamespaceQuota: update-or-createResourceQuotaandLimitRange; deletes managed objects when all fields are unsetresolveGatewayProfile: unary gRPC fetch before provisioning; fetch failure marks the gateway Failed so no gateway runs unconstrainedClusterRoleextended withresourcequotas/limitrangesRBAC verbs (base and IBM overlays)SDKs / CLI / Web console
GatewayProfiletype and API client;Gateway.ProfileId,ManagedCluster.ProfileId/DatabaseIdfieldscreate/get/list/delete gatewayProfilecommands;gateway create --profile-idflag/gateway-profilesroutes,GatewayProfileUiProvider,GatewayProfileSelectwired into gateway-create formSpec / dev-cluster / E2E
specs/platform/openshell-gateway-quota.spec.mddescribing the full designkind/up.shseeds three profiles (small/medium/big) and sets the cluster default to smallTest plan
cd components/api-server && make testpassescd components/api-server && make test-integrationpasses (gatewayProfiles integration tests)cd components/control-plane && go vet ./...passesmake kind-up→ create profile → create gateway with that profile → verifyResourceQuotaandLimitRangeappear in the gateway namespacetests/e2e/e2e-openshell.shGatewayProfile sections pass🤖 Generated with Claude Code