fix(gateway): reject identical main and sandbox vhosts#2146
fix(gateway): reject identical main and sandbox vhosts#2146mehara-rothila wants to merge 4 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR ensures main and sandbox vhosts do not resolve to the same effective hostname. It resolves gateway-default vhost sentinels to concrete router defaults, validates REST API spec.vhosts (case- and whitespace-insensitive) when a sandbox upstream exists, adds the same guard to the XDS translator, and emits a startup warning if router defaults collide. Unit tests cover sentinel resolution, validator behavior, and translator rejection cases. Sequence DiagramsequenceDiagram
participant Client
participant Service as REST API Service
participant Resolve as ResolveVhostSentinels
participant Validator as APIValidator
participant Translator as XDS Translator
participant Controller as Gateway Controller
Client->>Service: Submit/Update RestAPI
Service->>Resolve: ResolveVhostSentinels(cfg, routerCfg)
Resolve-->>Service: Return config with concrete vhosts
Service->>Validator: Validate resolved config (spec.vhosts)
alt vhosts equal (case/whitespace-insensitive) and sandbox present
Validator-->>Client: 400 Bad Request (spec.vhosts validation error)
else validation passed
Service->>Translator: translateAPIConfig(resolvedConfig)
alt translator detects same effective vhost
Translator-->>Service: error ("same vhost")
Service-->>Client: 400 Bad Request (translation error)
else translation succeeds
Translator->>Controller: Apply XDS config
Controller-->>Client: 201 Created
end
end
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
206b216 to
9fa9d4b
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gateway/gateway-controller/pkg/xds/translator.go`:
- Around line 834-838: The comparison between effectiveMainVHost and
effectiveSandboxVHost should mirror validator semantics by normalizing case and
surrounding whitespace before comparing; update the equality check in the
translator logic that currently compares effectiveMainVHost ==
effectiveSandboxVHost to instead compare trimmed, case-insensitive versions
(e.g., use strings.TrimSpace and strings.EqualFold) so that values differing
only by case or extra whitespace are treated as equal and the same
route-conflict error is triggered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8c1ac97a-09a4-4e6d-aee2-6b26e053448e
📒 Files selected for processing (8)
gateway/gateway-controller/cmd/controller/main.gogateway/gateway-controller/pkg/config/api_validator.gogateway/gateway-controller/pkg/config/api_validator_test.gogateway/gateway-controller/pkg/service/restapi/service.gogateway/gateway-controller/pkg/utils/api_deployment.gogateway/gateway-controller/pkg/utils/api_deployment_test.gogateway/gateway-controller/pkg/xds/translator.gogateway/gateway-controller/pkg/xds/translator_test.go
Identical main and sandbox vhosts collide on the same route key, but only one xDS path rejected them while the other accepted them, so the API deployed (201) then returned 500. Both paths now reject the collision and the validator returns a 400 at deploy time; equal vhost defaults log a startup warning instead of failing to boot.
9fa9d4b to
1a1e03e
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
Review feedback: the latent collision must not be storable. The xDS builders are unchanged since no sandbox upstream means no sandbox routes.
8cf393e to
ef47b59
Compare
A templated vhost that renders blank slipped past validation and the collision only surfaced later in xDS. Re-resolving after RenderSpec fills the blank with the router default before the same-vhost check, giving a clean 400.
Also clarifies the equal-default startup warning, which overstated that all REST deploys are rejected (explicit distinct vhosts still pass). From code review.
Purpose
A RestApi with a sandbox upstream whose vhost resolves to the same value as the main vhost was rejected by the RuntimeDeployConfig transform path but accepted by the Envoy xDS translator. Because per-API translation errors are only logged and swallowed, the deployment returned
201 Createdwhile the policy xDS received no configuration for the API. With the policy engine running fail-closed, every request to the API then returned500at runtime, with no signal to the API author beyond a swallowed log line.Resolves #2145
Goals
Reject the same-vhost collision consistently and at deploy time, returning a clear
400to the author instead of accepting a misconfiguration that silently fails at runtime. Ensure the two xDS builders can never disagree about whether a configuration is valid, since that disagreement was the root cause.Approach
The collision is now caught at every layer it can arise:
400at deploy time. The comparison is case-insensitive and whitespace-trimmed, since Envoy matches domains case-insensitively.User stories
As an API developer, when I deploy an API whose main and sandbox vhosts collide, I want it rejected immediately with a clear error, rather than accepted and then failing every request at runtime.
Documentation
N/A. This changes internal validation and xDS-translation behavior only; there is no user-facing API or product-documentation impact.
Automation tests
Security checks
Samples
Before the fix, this configuration deployed with
201and then returned500on every request:After the fix, the same deployment is rejected at admission:
Related PRs
None.
Test environment
gateway/gateway-controller/go.mod)