Repository navigation
Conversation
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Excluded labels (none allowed) (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Amber reviewStatus: Complete |
8ef0440 to
db8727b
Compare
36e29f3 to
e30957f
Compare
e30957f to
e069cec
Compare
e069cec to
f9f317b
Compare
f9f317b to
6fa8ddf
Compare
6fa8ddf to
740e196
Compare
There was a problem hiding this comment.
Verdict
The placement-intent redesign remains coherent and well-tested, and all four prior Amber findings are still genuinely fixed at the current head 8b1c9669. I am holding at COMMENT only because one cross-PR coordination item needs a maintainer ownership/merge-order decision, not because of any defect in this PR.
Summary
Gateway provisioning moves from a client-supplied cluster_id to a placement intent that the API server resolves to an eligible, connected, registered cluster via crypto/rand. The REST contract is intentionally breaking (cluster_id -> required placement), the gRPC path keeps cluster_id for backward compatibility while accepting an optional Placement, managed-cluster registration tolerates legacy heartbeats without erasing placement metadata, and visibility is added as a nullable column with provider/visibility now required control-plane config. Since the last review the only change is a merge of main (it brings in the ADLC hsctl CLI additions); the PR's own code is unchanged, so I re-verified each prior concern and the cross-PR analysis against 8b1c9669.
Cross-PR coordination
One material coordination item requires a maintainer decision:
- PR #386 (
chore(deploy): remove orphaned base manifests and add orphan check) is a duplicate solution for part of this PR's deployment cleanup and introduces a gate this PR must satisfy. Both PRs delete the exact same filedeploy/base/control-plane/deployment.yaml. In addition, #386 wires a newcheck-deploy-orphanstarget intomake check(scripts/check_deploy_orphans.py) that requires everydeploy/basemanifest to be referenced by a kustomization; this PR's remainingdeploy/baseedits (theHYPERSHELL_PLATFORM_KINDenv ondeploy/base/applications/api-server.yamland the provider/visibility env ondeploy/base/platform-resources/controller.yaml) must still pass that gate once it lands. Maintainers should decide which PR owns the orphaned-manifest deletion and fix a merge order: if #386 lands first, this PR must drop the now-redundant deletion and confirm itsdeploy/baseedits passcheck-deploy-orphans; if this PR lands first, #386 must rebase its deletion set. This is an ownership/ordering decision, not a plain text merge conflict.
Previous concerns
The last Amber review ran at head 740e1962, which is no longer the branch head (it advanced only by a merge of main), so I re-checked each prior finding against the current head 8b1c9669. All four remain fixed:
- gofmt alignment in
PresentManagedCluster- addressed. The inline multi-linefunc() *string {...}()is gone; a localvisibilitypointer is computed before the struct literal so every literal value is single-line (components/api-server/plugins/managedClusters/presenter.go:34-45). - Dead required-var messages in control-plane config - addressed. The
requiredemptiness loop (includingHYPERSHELL_MANAGED_CLUSTER_PROVIDER/VISIBILITY) runs first (components/control-plane/internal/config/config.go:139-151); the placement-validity check follows it (:152-157), so the dedicated... is requiredmessages are reachable. - Swallowed cause / conflated failure modes in placement resolution - addressed.
Resolvemaps onlyerrNoEligiblePlacementto the "no eligible managed cluster" validation error and logs the underlying error viaglog.Errorf, returningGeneralErrorfor any other failure including thecrypto/randpath (components/api-server/plugins/gateways/plugin.go:215-223). - OpenAPI
requiredvs handler-toleratedprovider/visibility- addressed.ManagedClusterRegistrationRequestlists onlynameunderrequired(components/api-server/openapi/openapi.managedClusters.yaml:327-331), matching the handler's tolerance of an omitted provider/visibility pair.
The committer addressed the previous concerns.
Findings
No new blocking findings.
- The versioned SDK placement-availability path is in place: both the hand-generated clients and the generator templates agree with the registered route and the other gateway endpoints (
components/sdk-go/client/gateway_api.go:27). - The placement resolver fails closed for unsupported network/provider pairs, draws only from registered and connected clusters, and bounds-checks the random index (
placement.go,plugin.go). A nil cluster-reference lookup fails closed to a 500 rather than letting an unvalidated reference through (cluster_reference.go). - Legacy registration preserves existing placement metadata on an omitted heartbeat pair; the
visibilitycolumn is added as a nullable migration (managedClusters/migration.go), so pre-existing rows remain valid. - Test diff scrutiny: the removed REST
cluster_idrejection tests are a correct consequence of the breaking contract; the newTestGatewayPostRejectsDirectClusterID(integration_test.go:98) asserts a directcluster_idis now rejected, and the registered-cluster guarantee is retained viavalidateClusterReferenceon the resolved create path and the PATCH reassignment path. - [Minor]
NewGatewayGRPCHandlertakesplacement ...PlacementResolveras a variadic to keep the old constructor signature callable, but the single production caller now always passes exactly one resolver (plugin.go:313). A single required parameter would make the dependency explicit and remove the silent "resolver missing" path; the variadic is a non-blocking readability nit (components/api-server/plugins/gateways/grpc_handler.go:31).
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
| No secrets in logs or responses | Pass |
| Input validated (placement, provider, visibility, DNS label) | Pass |
| Backward-compatible registration / nullable column migration | Pass |
| OpenAPI contract matches server behavior | Pass |
| Generated SDK path matches registered route | Pass |
| Placement failure cause surfaced (not swallowed) | Pass |
| Config separate from code (env vars) | Pass |
| Test diff scrutiny (cluster_id guarantees relocated to resolver/PATCH) | Pass |
| No em dashes in text files | Pass |
Findings Summary (ordered by severity, highest first):
- [Minor]
NewGatewayGRPCHandleruses a variadicplacement ...PlacementResolverfor what is now a single required dependency - Readability (grpc_handler.go:31)

Summary
Breaking change? 🚨 Yes - Gateway create requests now use
placementinstead of a user-selectedcluster_id. Clients that create gateways directly must send a supported placement intent and use the updated generated clients or API contract. Existing gateway records continue to expose their resolvedcluster_id.New to this repo
GatewayPlacementIntentandGatewayPlacementAvailabilityprovide an intent-based placement contract and current availability state for the provisioning UI.Tracking
Issue #401
Specs and other PRs
Related specs and dependent PRs
specs/web-console/gateway-provision-placement.spec.md- Defines the placement intent, availability, managed-cluster metadata, local-kind behavior, validation, and accessibility requirements.Components changed
Changes with impact
cluster_idinput with aplacementintent, requiring direct API clients to update their request shape.providerandvisibility, which the backend uses to determine gateway placement eligibility.local-kindplacement when managed placement is unavailable.Verification
Screenshots / video
Questions for discussion
Details
Technical details (for humans and bots)
The form loads placement availability when it opens. Public placement defaults to IBM Cloud when available, while VPN selects AWS and disables IBM Cloud because IBM Cloud VPN placement isn't supported.
The client sends one of these placement intents:
{ network: "public", provider: "aws" }{ network: "public", provider: "ibm" }{ network: "vpn", provider: "aws" }{ mode: "local-kind" }in the Kind development environmentThe API server filters for registered clusters with a connected control plane and matching
providerandvisibility. When multiple clusters match, it selects one at random and persists that cluster's identifier on the Gateway record. Gateway reconciliation continues to use the persisted identifier.The managed-cluster registration contract accepts
providervaluesaws,ibm, orkind, andvisibilityvaluespublicorvpn. IBM Cloud with VPN visibility is rejected. Older control planes can continue sending heartbeats without placement metadata without erasing metadata already recorded for that cluster.Generated OpenAPI, protobuf, Go SDK, and TypeScript SDK artifacts are updated with the new placement and managed-cluster fields.