fix(controller): normalize vhost values on the write path#2184
Draft
mehara-rothila wants to merge 1 commit into
Draft
fix(controller): normalize vhost values on the write path#2184mehara-rothila wants to merge 1 commit into
mehara-rothila wants to merge 1 commit into
Conversation
Contributor
|
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 pull request adds vhost canonicalization helpers (trim + lowercase, sentinel detection, and template-preserving canonicalization), applies them in resolveVhostSentinels for RestAPI/WebSubAPI/WebBrokerApi (using normalized router defaults and using normalized sentinel checks), and normalizes router vhost defaults during config validation. Tests were added to verify template-expression preservation, explicit normalization, and sentinel normalization. Sequence Diagram(s)sequenceDiagram
participant Client as API submission
participant Controller as resolveVhostSentinels
participant RouterCfg as RouterConfig (normalized defaults)
participant Store as API store
Client->>Controller: submit API (may include vhosts.main / vhosts.sandbox)
Controller->>RouterCfg: read defaults (already normalized)
Controller->>Controller: isGatewayDefaultVhost / canonicalizeVhost / normalizeVhost
Controller->>Store: persist resolved/canonicalized vhosts
🚥 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 |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
gateway/gateway-controller/pkg/utils/api_deployment_test.go (1)
999-1060: ⚡ Quick winAdd a regression case for mixed-case router default values.
Current new tests validate input/sentinel normalization well, but they don’t cover router defaults containing mixed/upper case. Add a case where
routerCfg.VHosts.Main.Default/Sandbox.Defaultare mixed-case and verify resolved behavior remains consistent.🤖 Prompt for 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. In `@gateway/gateway-controller/pkg/utils/api_deployment_test.go` around lines 999 - 1060, Add a regression test to ensure router default vhost values are normalized when they contain mixed/upper-case: update or add a TestResolveVhostSentinels_* case that builds a routerCfg whose VHosts.Main.Default and VHosts.Sandbox.Default include mixed case and surrounding whitespace (e.g. "*.WSO2.COM " and " *-SANDBOX.WSO2.COM"), call resolveVhostSentinels(&cfg, routerCfg) and assert the resolved values (from cfg.(api.RestAPI).Spec.Vhosts) equal the expected lower-cased, trimmed defaults ("*.wso2.com" and "*-sandbox.wso2.com"); reference resolveVhostSentinels and the VHosts.Default fields when locating where to add the assertions.
🤖 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/utils/api_deployment.go`:
- Around line 682-699: The code compares c.Spec.Vhosts.Main against
routerCfg.VHosts.Main.Default but normalizeVhost now lowercases
stored/default-resolved vhosts, so make the equality check use the same
canonical form: call normalizeVhost(...) on routerCfg.VHosts.Main.Default (and
routerCfg.VHosts.Sandbox.Default where used) before passing to
isGatewayDefaultVhost or assigning/ comparing; e.g., replace direct uses of
routerCfg.VHosts.Main.Default with normalizeVhost(routerCfg.VHosts.Main.Default)
and ensure isGatewayDefaultVhost is invoked with the normalized value so
default-domain expansion triggers correctly for mixed-case configured defaults
(update both the isGatewayDefaultVhost(c.Spec.Vhosts.Main) check and the
assignment branches involving c.Spec.Vhosts.Main).
---
Nitpick comments:
In `@gateway/gateway-controller/pkg/utils/api_deployment_test.go`:
- Around line 999-1060: Add a regression test to ensure router default vhost
values are normalized when they contain mixed/upper-case: update or add a
TestResolveVhostSentinels_* case that builds a routerCfg whose
VHosts.Main.Default and VHosts.Sandbox.Default include mixed case and
surrounding whitespace (e.g. "*.WSO2.COM " and " *-SANDBOX.WSO2.COM"), call
resolveVhostSentinels(&cfg, routerCfg) and assert the resolved values (from
cfg.(api.RestAPI).Spec.Vhosts) equal the expected lower-cased, trimmed defaults
("*.wso2.com" and "*-sandbox.wso2.com"); reference resolveVhostSentinels and the
VHosts.Default fields when locating where to add the assertions.
🪄 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: 59af2b40-2aad-407a-b481-5afd610b809b
📒 Files selected for processing (2)
gateway/gateway-controller/pkg/utils/api_deployment.gogateway/gateway-controller/pkg/utils/api_deployment_test.go
mehara-rothila
force-pushed
the
fix/vhost-normalization-collision
branch
2 times, most recently
from
7615025 to
2a7f13e
Compare
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Vhost values were stored verbatim and emitted as Envoy virtual-host
domains. Envoy compares domains case-insensitively and ignores surrounding
whitespace, so two APIs whose vhosts differ only in case or whitespace
produced duplicate domains. Envoy then rejected the whole route
configuration and stopped applying further route updates gateway-wide,
while the deploy still returned success.
Normalize vhost values (lower-case and trim) when resolving the
gateway-default sentinel and when populating omitted defaults, so the
stored value is canonical, and match the sentinel case- and
whitespace-insensitively. Canonicalize the configured router defaults at
config validation too. The xDS translator default-domain expansion now
compares the stored vhost against the configured default case- and
whitespace-insensitively, so APIs stored before this change still match on
upgrade without a redeploy. Template expressions in a vhost (for example
{{ env "API_HOST" }}) are left untouched, since they are rendered later and
their case is significant for the lookup key.
- Add unit tests covering explicit case/whitespace normalization, sentinel
case/whitespace variants, configured-default canonicalization, template
preservation, and case-insensitive default-domain matching
Fixes wso2#2183
mehara-rothila
force-pushed
the
fix/vhost-normalization-collision
branch
from
2a7f13e to
200751e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
The gateway controller stored API vhost values verbatim and emitted them as Envoy virtual-host domains. Envoy compares domains case-insensitively, ignores surrounding whitespace, and requires every domain in a route configuration to be unique. Two APIs whose vhosts differed only in case or whitespace therefore produced duplicate Envoy domains. The control plane returned success (
201), but Envoy rejected the entireshared_route_configand stopped applying any further route updates gateway-wide until the offending API was removed.Resolves #2183.
Goals
Store vhost values in a canonical form (lower-cased and trimmed) so that values differing only in case or surrounding whitespace are no longer emitted as separate, colliding Envoy domains, while keeping legitimate vhost sharing (multiple APIs on one host, routed by path) working.
Approach
Normalize vhost values inside the shared
resolveVhostSentinelsresolver, which runs on the deploy path before vhosts are persisted and rendered. HelpersnormalizeVhostandisGatewayDefaultVhostare added, and the resolver now:Supporting changes that keep the canonical form consistent everywhere it is compared:
router.vhosts.main.defaultandrouter.vhosts.sandbox.default) are canonicalized once at config validation (validateVHostsConfig), so a mixed-case configured default cannot drift from the stored values.getVHostDomains) compares the stored vhost against the configured default case- and whitespace-insensitively. This keeps domain expansion working for APIs that were stored before this change with a non-canonical default (for example a previously mixed-case configured default), so the change is safe on upgrade without a redeploy.Template expressions in a vhost (for example
{{ env "API_HOST" }}) are left untouched by normalization, because they are rendered later byRenderSpecand their case is significant for the lookup key. A helper,canonicalizeVhost, normalizes a value only when it is not a template (does not contain{{), matching the template checkRenderSpecitself uses. Sentinel and configured-default substitutions are never templates, so they continue to be normalized directly.Because normalization collapses case and whitespace variants to the same string, they group into a single Envoy virtual host instead of duplicate domains.
A cross-API vhost uniqueness rejection is intentionally not added: multiple APIs may legitimately share a vhost and route by path (this is how the default host is used), so rejecting a shared vhost would break normal routing. The fix is canonical storage, not rejection.
User stories
As an API developer, when I deploy an API whose vhost differs only in letter case or surrounding whitespace from another API's vhost, the gateway should keep programming routes normally instead of silently freezing all route updates.
Documentation
N/A. This is a bug fix. The stored and echoed vhost value is now lower-cased and trimmed; a vhost is a hostname and is case-insensitive, so this does not change routing behavior. Templated vhosts are unaffected, their template text is preserved for rendering. Existing deployments are unaffected on upgrade, the translator's default-domain comparison is case-insensitive, so already-stored non-canonical vhosts still match the configured default.
Automation tests
Security checks
Samples
Two APIs deployed on the same host with different letter case:
Before: API B was stored as
A.EXAMPLE.COM, Envoy saw twoa.example.comdomains, rejected the whole route configuration, and route programming froze gateway-wide. After: API B is stored asa.example.com, both APIs share one virtual host and route by path, and the route configuration is applied normally.Related PRs
N/A.
Test environment
gateway/gateway-controller/go.mod)go test ./pkg/utils/... ./pkg/config/... ./pkg/xds/...).