Repository navigation
Conversation
|
/ok to test 3f1fd80 |
Cluster Verification ReportVerified end-to-end on a real multi-tenant OpenShift (ROSA) cluster with two gateways sharing one base domain and wildcard certificate — the exact ambiguous scenario from #4840. Environment
Results
Operator-managed ingress was used per the PR's documented model: exact-host passthrough Routes created per emitted hashed hostname, each targeting its own gateway's Service. Also exercised during setup: Helm rendering of |
… clusters Closes NVIDIA#4840 Add an optional service_routing_gateway_name gateway setting (TOML, CLI, env, Helm) that disambiguates exposed service URLs when multiple gateways share a cluster, ingress, and wildcard certificate. When configured, exposed-service hostnames become a deterministic 16-character lowercase base32 SHA-256 label of (gateway name, workspace, sandbox, service) under the same base domain, keeping every hostname exactly one label deep for wildcard-certificate compatibility. Unset gateways preserve the existing readable hostname format byte-for-byte, and configured gateways still accept legacy hosts during migration. Hashed hosts resolve through an in-memory route index hydrated from persisted ServiceEndpoint records at startup, refreshed on miss for multi-replica deployments, maintained across expose, delete, and sandbox cleanup, and enforced for collisions before endpoint persistence. Signed-off-by: Jesse Jaggars <jjaggars@redhat.com>
3f1fd80 to
28551bd
Compare
|
Rebased onto the latest @TaylorMutch the branch was force-pushed for the rebase, so the earlier |
|
/ok to test 28551bd |
|
/ok to test a868504 |
|
/ok to test 17fd4cd |
Dev-cluster verificationVerified the dedicated-controller refactor live on an OpenShift dev cluster (two gateways, Architecture in action
Kind handling
End-to-end lifecycle (gateway-a,
RBAC separation confirmed on-cluster: the gateway ServiceAccount holds only the feed-publisher Role (pinned to the one ConfigMap); the controller ServiceAccount holds the ingress-object Role. No cluster-scoped permissions involved. Verification also surfaced one fix in this branch: the controller subcommand initially ran without a tracing subscriber (722016e). |
The shared-supervisor landing (9a85667) predates the last Branch Checks run on main, so its doc-comment style was never gated. Split the three overlong first doc paragraphs so workspace clippy -D warnings passes; behavior is unchanged. Signed-off-by: Jesse Jaggars <jjaggars@redhat.com>
722016e to
75fe66f
Compare
|
Per review, this PR is now scoped to just the hashed service-routing hostnames ( The follow-up plan is a discovery API on the gateway so ingress provisioning and reconciliation can be built as a controller alongside the gateway rather than inside it — the removed work is preserved on a side branch as a starting point for that design. |
TaylorMutch
left a comment
TaylorMutch
left a comment
There was a problem hiding this comment.
Thanks for separating the Kubernetes reconciliation work from this PR. The one-label hashed hostname fits the shared wildcard certificate and exact-host routing use case, and this functionality can land independently of RFC 0017.
I think we should resolve the routing identity model and two request-path issues before merging:
-
OpenShell already has
[openshell.gateway].name, an operator-assigned installation identity shared by its replicas. This introduces a second value described as a gateway name. If URL identity intentionally needs an independent lifecycle, could we name it something likeservice_routing_discriminatorand document that distinction? Otherwise, I would prefer an explicit hashed-hostname mode that derives its discriminator from the existing installation name.gateway_jwt.gateway_idshould remain separate because it has security and Kubernetes ownership semantics. -
A syntactically valid unknown hash currently causes a full all-workspaces service endpoint scan. Because this runs on the public request path before authentication, repeated random Host headers can amplify into repeated database scans.
-
Cross-origin rejections for hashed service hosts no longer emit the existing OCSF security event because the logging path still uses the readable-host parser.
The repository instructions also require updating skills/debug-openshell-cluster/SKILL.md when gateway TOML or Helm rendering changes.
| // A gateway can begin receiving traffic before an endpoint is exposed on | ||
| // this replica, so reload once from durable state before treating it as a | ||
| // normal, non-service request. | ||
| if let Err(error) = state.service_route_index.rebuild(&state.store).await { |
There was a problem hiding this comment.
Every well-formed unknown hash reaches rebuild(), which loads and decodes every service endpoint in every workspace. Unknown labels are not negatively cached or refresh-throttled, so an unauthenticated client can cause a complete database scan with each random Host header.
Could we make cross-replica refresh bounded—for example with single-flight refresh, a cooldown and negative cache—or use a store-backed lookup keyed by the route label? An unknown public hostname should not independently trigger an unbounded full scan.
| }; | ||
| let Some((_workspace, sandbox_name, service_name)) = | ||
| parse_host(host, &state.config.service_routing) | ||
| let Some((_, sandbox_name, service_name)) = parse_host(host, &state.config.service_routing) |
There was a problem hiding this comment.
The middleware can recognize a hashed hostname through ServiceRouteIndex, but this function reparses it with the legacy readable-host parser. For a hashed service request, parse_host() returns None, so the gateway returns the 403 without emitting the OCSF rejection event.
Could the already-resolved ServiceRouteTarget be passed into the rejection path, or could this path resolve the hash before building the event?
| /// | ||
| /// This value is unrelated to [`Config::name`] and | ||
| /// [`GatewayJwtConfig::gateway_id`]; it is never derived from them. | ||
| pub service_routing_gateway_name: Option<String>, |
There was a problem hiding this comment.
Could we clarify why this needs to be a second gateway name instead of using the existing Config::name?
Config::name is already documented as the operator-assigned installation identity and is shared across replicas. If this value needs an independent lifecycle because changing the installation/telemetry name must not change application URLs, I think it should be named as a routing discriminator rather than another gateway name. If it does not need that independence, an explicit hashed-hostname mode using Config::name would avoid two potentially divergent gateway identities.
Also, because this value is only hash input, does it need DNS-label validation? That restriction suggests it appears directly in DNS even though it does not.
Three changes from PR review: Identity model: replace service_routing_gateway_name with an explicit service_hostname_format mode (readable, the default, or hashed). In hashed mode the discriminator hashed into exposed-service hostnames is the existing gateway installation name ([openshell.gateway].name, operator- or platform-assigned, defaulting to the Helm release name), so no second per-gateway name is introduced. The name must be a DNS label and unique among gateways sharing the service base domain — an operational contract held by the deploying platform, documented in the configuration reference. gateway_jwt.gateway_id remains separate and untouched; renaming the installation now changes every minted URL. Request-path amplification: miss-triggered service route index rebuilds are debounced to one attempt per five seconds. Unknown hash labels reach the rebuild path before authentication, and without the debounce each request carrying a random well-formed label triggered a full all-workspaces endpoint scan. Startup hydration and explicit rebuilds bypass the debounce. OCSF regression: cross-origin rejections for hashed service hosts emitted no security event because the logging path used the readable-host parser only. The emitter now resolves hashed hosts through the in-memory route index (never the miss-triggered rebuild path) and still emits for unknown hash-shaped labels, attributed to the raw label. Also updates the debug-openshell-cluster skill for the new gateway TOML fields, per the repository instructions for gateway config changes. Signed-off-by: Jesse Jaggars <jjaggars@redhat.com>
|
Thanks for the thorough review — all three points are addressed in 2a442b5. Identity model: Took your preferred direction. Request-path amplification: Miss-triggered index rebuilds are now debounced to one attempt per five seconds (the timestamp stamps the attempt, so a failing store can't be retried more than once per window either). A stream of random well-formed labels now costs in-memory lookups plus at most one full-store scan per window. Startup hydration and explicit rebuilds bypass the debounce, so legitimate cold-start misses still recover immediately. OCSF regression: The cross-origin rejection emitter now resolves hashed hosts through the in-memory route index and emits the security event with the resolved sandbox/service identity; unknown hash-shaped labels still emit, attributed to the raw label. It deliberately does not call the miss-triggered rebuild path, so the emitter itself cannot be used for amplification. The On the follow-up: we'd like the discovery API to include cross-gateway hostname-claim validation — with the discriminator being platform-assigned this should be structurally prevented at creation time, but a domain-scoped check in the controller would catch platform bugs and cross-cluster conflicts in a shared zone. |
Dev-cluster verification of 2a442b5Deployed the review-feedback commit to both dev-cluster gateways (multi-arch image built from the branch), migrating values to the new surface: Discriminator derivation — verified exactly: the label minted for a fresh exposure equals the expected base32 SHA-256 of Manual ingress path (external automation owns ingress, per this PR's scope):
Rebuild debounce — fired a burst of concurrent requests carrying random well-formed hash labels: all rejected quickly, zero index-rebuild or error log lines in the gateway, health endpoint responsive throughout, and an in-flight live exposure kept serving 200s. Worst case is one store scan per five-second window as designed. OCSF emitter — the cross-origin rejection path serves the loopback-bound plaintext browser listener, which a Kubernetes gateway (bound non-loopback) correctly refuses plaintext on, so this path is not reachable in an ingress topology; it is covered by the new unit tests exercising the emitter with the OCSF capture harness, including resolved hashed hosts, unknown labels, and non-service hosts. |
Summary
Adds an optional
service_hostname_format = "hashed"gateway setting that disambiguates exposed-service URLs when multiple gateways share a Kubernetes cluster, ingress, and wildcard certificate. In hashed mode, service hostnames become a deterministic 16-character lowercase base32 SHA-256 label of(installation name, workspace, sandbox, service)under the same base domain — exactly one label deep, so a single shared*.domainwildcard certificate keeps working, and each gateway's URLs are globally unique. The discriminator is the existing[openshell.gateway].nameinstallation identity (operator- or platform-assigned, defaulting to the Helm release name), so no second per-gateway name is introduced; the platform deploying shared-domain gateways is responsible for keeping those names unique, andgateway_jwt.gateway_idstays separate with its security and Kubernetes-ownership semantics. The default (readable) preserves the existing hostname format byte-for-byte.Related Issue
Closes #4840
Changes
crates/openshell-core/src/config.rs:ServiceHostnameFormatenum (readabledefault /hashed),ServiceRoutingConfig.service_hostname_formatfield,with_service_hostname_formatbuilder,Config::service_routing_discriminator()(the installation name, only in hashed mode), DNS-1123-label validation of the installation name when hashed, and config tests.crates/openshell-server/src/config_file.rs:[openshell.gateway] service_hostname_formatTOML parsing/serialization.crates/openshell-server/src/cli.rs:--service-hostname-format/OPENSHELL_SERVICE_HOSTNAME_FORMATwith existing CLI > env > TOML precedence, startup validation of the installation name, and precedence tests.crates/openshell-server/src/service_routing.rs: SHA-256/NUL-delimited tuple hashing with a hand-rolled fixed-size lowercase base32 encoder (no new dependency), hashedendpoint_hostoutput,ServiceRouteIndex(register/reserve/remove/lookup/rebuild), async state-awareresolve_service_hostsupporting both hashed and legacy hosts, and routing tests with fixed vectors.crates/openshell-server/src/lib.rs: route index inServerState, hydrated from persistedServiceEndpointrecords at startup before listeners bind; ambiguous persisted hashes fail startup.crates/openshell-server/src/http.rs: HTTP middleware and loopback plaintext router use the async state-aware service-host classification.crates/openshell-server/src/grpc/service.rs: hash route reserved before endpoint persistence (AlreadyExistson collision, no write), rolled back on persistence failure, removed after successful delete; collision test.crates/openshell-server/src/compute/mod.rs: sandbox cleanup removes route index entries for deleted endpoints.deploy/helm/openshell/:server.serviceRoutingGatewayNamevalue rendered into gateway TOML via_toml.tpl(empty stays unset; explicitgatewayConfigtakes precedence), chart tests, regenerated README.docs/how-it-works/gateways/configuration.mdx(field reference),docs/how-it-works/gateways/overview.mdx(hostname grammar — also fixes the pre-existing omission of the workspace segment),docs/how-it-works/sandboxes/overview.mdx,docs/kubernetes/ingress.mdx(operator-managed exact-host ingress guidance for multi-gateway shared-cert deployments),docs/kubernetes/setup.mdx.Deviations from Plan
None functionally. The route reservation API internally tracks whether the current call inserted the route so persistence-failure rollback cannot remove a pre-existing idempotent reservation from a concurrent request (an implementation detail within the plan's intent).
Testing
mise run pre-commitpasses (format, license headers, Helm lint) — with one pre-existing, unrelated failure:rust:lockfiles:checkrejectsexamples/supervisor-middleware-content-guard/Cargo.lockbecauseclap 4.6.7was yanked from crates.io; that lockfile is untouched by this PR and fails on unmodifiedmainas well.e2e/were modified.Tests added:
openshell-coreconfig: format defaults, discriminator derivation and validation (DNS-label installation name required when hashed),with_server_sanspreserves the format.openshell-serverservice_routing: fixed hash vectors (enh47uqyj7bpyttz,eiud2z5apv3yeucg), hashed URL construction, index idempotency/removal/collision/rebuild-from-store, configured-hash host resolution for Host header and HTTP/2 authority, legacy-host acceptance when configured, unknown-hash fallthrough.openshell-servergrpc::service:expose_service_rejects_route_hash_collision_before_persistence.openshell-serverconfig_file/cli: TOML parse/serialize, file-populated value, CLI/env override precedence, effective-config reach.openshell-cli:service_url_for_gatewaypreserves the hashed label while normalizing the port.openshell-serverservice_routing tests.Verification runs (local):
cargo fmt --check,git diff --check— passcargo clippy --workspace --all-targets -- -D warnings— pass (includes a small fix for main-landedsandbox_runtime.rsdoc lints that predate the last Branch Checks run on main)cargo test -p openshell-core— 607 unit + 1 doctest passcargo test -p openshell-server— 2388 pass (21 Postgres-dependent ignored as expected), including new tests for the miss-triggered rebuild debounce and hashed-host cross-origin OCSF emissioncargo test -p openshell-cli service_url— passmise run helm:test— 361 chart tests passChecklist
Documentation updated:
docs/how-it-works/gateways/configuration.mdx: new TOML field reference (default, validation, precedence, hash URL example, distinction fromgateway_jwt.gateway_id)docs/how-it-works/gateways/overview.mdx: corrected hostname grammar (workspace segment) + legacy vs hashed formatsdocs/how-it-works/sandboxes/overview.mdx: consume printed URLs, not constructed hostnamesdocs/kubernetes/ingress.mdx: exact-host ingress guidance for multi-gateway shared-wildcard-cert deploymentsdocs/kubernetes/setup.mdx: chart-values table entrydeploy/helm/openshell/README.md: regenerated chart values docs