Skip to content

feat(gateway-management-ui): provision gateways by placement intent - #412

Open
kdoberst wants to merge 13 commits into
openshift-online:mainfrom
kdoberst:HYPERSHELL-361-simplified-gateway-provisioning
Open

kdoberst wants to merge 13 commits into
openshift-online:mainfrom
kdoberst:HYPERSHELL-361-simplified-gateway-provisioning

Conversation

@kdoberst

@kdoberst kdoberst commented Oct 1, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • Problem: Gateway provisioning currently asks users to select a specific cluster. That exposes an implementation detail and makes the form responsible for placement decisions that belong in the backend.
  • Fix: Replace the cluster selector with network visibility and cloud provider choices. The UI submits placement intent, and the backend selects an eligible connected cluster based on that intent. Managed clusters now include provider and visibility metadata so the backend can resolve placement.
  • Alternatives considered: Keep the cluster selector and add placement metadata to it. This would preserve the current user-facing coupling between gateway provisioning and individual clusters.

Breaking change? 🚨 Yes - Gateway create requests now use placement instead of a user-selected cluster_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 resolved cluster_id.

New to this repo

  • GatewayPlacementIntent and GatewayPlacementAvailability provide an intent-based placement contract and current availability state for the provisioning UI.
  • Backend placement resolution randomly selects from eligible connected clusters instead of using capacity data or a client-selected cluster.

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

  • API server - gateway placement resolution, availability, managed-cluster metadata, and API contracts
  • Control plane - registration of provider and visibility metadata
  • Gateway management UI - provisioning form and placement availability state
  • Web console - gateway provisioning adapter and story
  • Go and TypeScript SDKs - regenerated placement and managed-cluster types
  • Deployment configuration - placement-related environment configuration
  • Specs and mockups - placement workflow documentation and updated mockup story

Changes with impact

  • 🚨 HIGH: Gateway create requests replace the concrete cluster_id input with a placement intent, requiring direct API clients to update their request shape.
  • 🚨 HIGH: Managed-cluster registration adds provider and visibility, which the backend uses to determine gateway placement eligibility.
  • ⚠️ MEDIUM: The gateway provisioning UI no longer lets users select a cluster. Users choose Public or VPN visibility and then select an eligible provider.
  • ⚠️ MEDIUM: AWS supports Public and VPN placement. IBM Cloud supports Public placement only. VPN placement requires AWS.
  • ⚠️ MEDIUM: Placement availability is calculated from registered connected clusters and is rechecked during gateway creation. Stale availability returns a recoverable provisioning error.
  • ✅ LOW: Kind development environments can use the registered local-kind placement when managed placement is unavailable.

Verification

  • Unit/integration tests created or updated
  • Error paths considered and addressed
  • Code changes match spec or acceptance criteria
  • Code changes match Jira

Screenshots / video

Screenshot 2026-10-01 at 3 43 56 PM

Questions for discussion

  • None

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 environment

The API server filters for registered clusters with a connected control plane and matching provider and visibility. 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 provider values aws, ibm, or kind, and visibility values public or vpn. 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.

@kdoberst kdoberst added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 1, 2026
@coderabbitai

coderabbitai Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are limited based on label configuration.

🚫 Excluded labels (none allowed) (2)
  • do-not-merge/work-in-progress
  • do-not-merge/hold

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration
  • Configuration used: Repository YAML (base), Central YAML (inherited)
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 4d4b2449-1dcf-42a4-ad7e-33d629973b2b

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Comment @coderabbitai help to get the list of available commands.

@hypershell-delivery

hypershell-delivery Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

Amber review: comment

Amber review

Status: Complete

View the submitted review.

hypershell-delivery[bot]

This comment was marked as outdated.

hypershell-delivery[bot]

This comment was marked as outdated.

hypershell-delivery[bot]

This comment was marked as outdated.

@kdoberst
kdoberst force-pushed the HYPERSHELL-361-simplified-gateway-provisioning branch from 36e29f3 to e30957f Compare October 5, 2026 15:23
hypershell-delivery[bot]

This comment was marked as outdated.

@hypershell-delivery hypershell-delivery Bot added the amber/approved The Amber review agent has approved this PR. label Oct 5, 2026
@kdoberst
kdoberst force-pushed the HYPERSHELL-361-simplified-gateway-provisioning branch from e30957f to e069cec Compare October 5, 2026 16:14
hypershell-delivery[bot]

This comment was marked as outdated.

@hypershell-delivery hypershell-delivery Bot removed the amber/approved The Amber review agent has approved this PR. label Oct 5, 2026
@kdoberst
kdoberst force-pushed the HYPERSHELL-361-simplified-gateway-provisioning branch from e069cec to f9f317b Compare October 5, 2026 17:54
hypershell-delivery[bot]

This comment was marked as outdated.

@kdoberst
kdoberst force-pushed the HYPERSHELL-361-simplified-gateway-provisioning branch from f9f317b to 6fa8ddf Compare October 5, 2026 18:16
hypershell-delivery[bot]

This comment was marked as outdated.

@kdoberst
kdoberst force-pushed the HYPERSHELL-361-simplified-gateway-provisioning branch from 6fa8ddf to 740e196 Compare October 7, 2026 18:03
hypershell-delivery[bot]

This comment was marked as outdated.

@hypershell-delivery hypershell-delivery Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 file deploy/base/control-plane/deployment.yaml. In addition, #386 wires a new check-deploy-orphans target into make check (scripts/check_deploy_orphans.py) that requires every deploy/base manifest to be referenced by a kustomization; this PR's remaining deploy/base edits (the HYPERSHELL_PLATFORM_KIND env on deploy/base/applications/api-server.yaml and the provider/visibility env on deploy/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 its deploy/base edits pass check-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-line func() *string {...}() is gone; a local visibility pointer 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 required emptiness loop (including HYPERSHELL_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 required messages are reachable.
  • Swallowed cause / conflated failure modes in placement resolution - addressed. Resolve maps only errNoEligiblePlacement to the "no eligible managed cluster" validation error and logs the underlying error via glog.Errorf, returning GeneralError for any other failure including the crypto/rand path (components/api-server/plugins/gateways/plugin.go:215-223).
  • OpenAPI required vs handler-tolerated provider/visibility - addressed. ManagedClusterRegistrationRequest lists only name under required (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 visibility column is added as a nullable migration (managedClusters/migration.go), so pre-existing rows remain valid.
  • Test diff scrutiny: the removed REST cluster_id rejection tests are a correct consequence of the breaking contract; the new TestGatewayPostRejectsDirectClusterID (integration_test.go:98) asserts a direct cluster_id is now rejected, and the registered-cluster guarantee is retained via validateClusterReference on the resolved create path and the PATCH reassignment path.
  • [Minor] NewGatewayGRPCHandler takes placement ...PlacementResolver as 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):

  1. [Minor] NewGatewayGRPCHandler uses a variadic placement ...PlacementResolver for what is now a single required dependency - Readability (grpc_handler.go:31)

@kdoberst kdoberst removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 7, 2026
@kdoberst kdoberst changed the title feat(gateway-management-ui): provision gateways by placement intent (HYPERSHELL-361) feat(gateway-management-ui): provision gateways by placement intent Oct 7, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant