[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: Pro Plus 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.
…end namespace quota enforcement Add a new GatewayProfile resource that defines Kubernetes namespace-level ResourceQuota totals and container-level LimitRange defaults. Every gateway must reference a profile (client-supplied or inherited from the cluster default) so the control plane can enforce resource constraints on the namespace it provisions. API server: - New gatewayProfiles plugin: model, DAO, service, gRPC handler/presenter, REST handler/presenter, plugin registration, and DB migration - Field validation at the API boundary via resource.ParseQuantity and non-negative count checks — invalid values return HTTP 400 rather than propagating to control-plane reconciliation - Gateway create now requires profile_id: falls back to the cluster's default_profile_id when none is supplied; returns HTTP 400 if neither source yields a value - Cluster and profile existence checks on gateway create and patch - Deletion protection: a profile referenced by a cluster default or any gateway returns HTTP 409 - Added profile_id to Gateway and default_profile_id / default_database_id to ManagedCluster; new DB migrations for both - Protobuf definitions and generated Go stubs for GatewayProfile gRPC service - OpenAPI spec extended with GatewayProfile CRUD endpoints, Gateway.profile_id, and ManagedCluster default fields; generated OpenAPI Go client regenerated Control plane: - New QuotaConfig struct (translation of GatewayProfile fields) - ReconcileQuota: update-or-create ResourceQuota and LimitRange in each gateway namespace using semantic equality checks; delete managed objects when all fields are unset (update-or-create, not create-and-skip) - resolveGatewayProfile: unary gRPC fetch before provisioning; a fetch failure marks the gateway Failed and blocks provisioning so no profiled gateway runs unconstrained; empty profile_id (legacy gateway) skips quota - Controller ClusterRole extended with resourcequotas/limitranges RBAC verbs in both the base overlay and the IBM overlay SDKs: - Go SDK: GatewayProfile type, GatewayProfileAPI client, Gateway.ProfileId, ManagedCluster.DefaultProfileId / DefaultDatabaseId fields - TypeScript SDK: GatewayProfile type, GatewayProfileApi client, same field additions; index.ts exports updated CLI: - gatewayProfile CRUD commands: create, get, list, delete - gateway create --profile-id flag; gateway list shows profile_id column Web console: - GatewayProfile routes: /gateway-profiles, /gateway-profiles/new, /gateway-profiles/:id - ApplicationShell wired with GatewayProfileUiProvider, breadcrumbs, and nav - gateway-profile-operations adapter, observability, and list-state feature slice - locales/en.json re-extracted with GatewayProfile strings gateway-management-ui package: - GatewayProfile create form, delete dialog, row actions, load-state, and pages (list and detail) - GatewayProfileUiProvider and message catalogue - GatewayProfileSelect component wired into gateway-create form so users pick a profile when creating a gateway - gateway-profile-operations, gateway-profile-probes, gateway-profile-types application layer Dev cluster / E2E: - kind/up.sh seeds three profiles (small/medium/big) and sets the local-kind cluster default to small so gateway creates always have a real profile - E2E suite extended with GatewayProfile seed and validation coverage Spec: - New specs/platform/openshell-gateway-quota.spec.md describing the full design - data-model.spec.md updated with GatewayProfile entity and cross-references
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.
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