Allow Proxying Domain without Wildcard#5468
Allow Proxying Domain without Wildcard#5468kritgrover wants to merge 10 commits intonetbirdio:mainfrom
Conversation
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds domain normalization and validation, refactors custom-domain matching to prefer exact non-wildcard matches then longest-suffix wildcard/subdomain matches, updates DNS validation for wildcard inputs, adds unit tests for matching logic, and documents custom-domain behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Manager as Manager
participant ClusterStore as ClusterStore
participant Validator as Validator
participant DNS as DNS
Client->>Manager: Request route for host
Manager->>Manager: Normalize host (trim, lowercase)
Manager->>ClusterStore: Query custom domain rules
alt Exact non-wildcard match
ClusterStore-->>Manager: Exact cluster
else No exact match
ClusterStore-->>Manager: Wildcard/subdomain candidates
Manager->>Validator: Normalize wildcard candidates (strip "*.")
Validator->>DNS: Resolve validation.<base> for candidates
DNS-->>Validator: CNAME/response
Validator-->>Manager: Validation results
Manager->>ClusterStore: Select candidate with longest matching suffix
ClusterStore-->>Manager: Selected cluster
end
Manager-->>Client: Return cluster or not found
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can disable poems in the walkthrough.Disable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
management/internals/modules/reverseproxy/domain/manager/manager_test.go (1)
108-113: Consider adding an empty host test case.The tests cover empty
customDomainsbut not an emptyhost. While unlikely in practice, adding this edge case would ensure the function handles it gracefully.📝 Suggested test case
"empty host returns false": { host: "", customDomains: []*domain.Domain{ {Domain: "example.com", TargetCluster: "cluster-a"}, }, wantCluster: "", wantOK: false, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/internals/modules/reverseproxy/domain/manager/manager_test.go` around lines 108 - 113, Add a new table-driven test case in manager_test.go alongside the existing cases (e.g., next to "empty custom domains returns false") named "empty host returns false" that calls the same function under test (the manager lookup function used in the existing test) with host set to "" and customDomains set to a slice containing one domain.Domain{Domain: "example.com", TargetCluster: "cluster-a"}, and assert wantCluster == "" and wantOK == false so the function correctly handles an empty host input.proxy/README.md (1)
49-51: Add language specification to the fenced code block.Per static analysis, the code block should have a language specified for proper syntax highlighting. Since this is DNS configuration,
dnsortextwould be appropriate.📝 Suggested fix
-``` +```text validation.example.com. CNAME <proxy-cluster-address>.</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@proxy/README.mdaround lines 49 - 51, Update the fenced code block that
currently contains the DNS line "validation.example.com. CNAME
." to include a language specifier (e.g.,text ordns) so the block becomestext (ordns) ... ``` for proper syntax
highlighting in the README; locate the block in proxy/README.md that wraps that
exact line and add the language token immediately after the opening backticks.</details> </blockquote></details> <details> <summary>management/internals/modules/reverseproxy/domain/manager/manager.go (1)</summary><blockquote> `290-307`: **Minor: redundant normalization in the second loop.** `normalizedCD` at line 291 re-normalizes domains that were already normalized in the first loop (line 280). While functionally correct, you could pre-normalize custom domains once and reuse the results. This is a minor optimization and acceptable to defer. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@management/internals/modules/reverseproxy/domain/manager/manager.go` around lines 290 - 307, customDomains are being normalized repeatedly inside the loop via the normalizedCD variable; precompute and reuse normalized domain values to avoid redundant work. Create a preprocessed collection (e.g., map or slice) that stores each domain's normalized form (strings.ToLower(strings.TrimSuffix(cd.Domain, "."))) alongside its TargetCluster before the matching loop, then update the matching logic that uses normalizedCD and TargetCluster (and compares to normalizedHost) to read from the precomputed normalized value instead of recomputing it per iteration. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In@management/internals/modules/reverseproxy/domain/manager/manager_test.go:
- Around line 108-113: Add a new table-driven test case in manager_test.go
alongside the existing cases (e.g., next to "empty custom domains returns
false") named "empty host returns false" that calls the same function under test
(the manager lookup function used in the existing test) with host set to "" and
customDomains set to a slice containing one domain.Domain{Domain: "example.com",
TargetCluster: "cluster-a"}, and assert wantCluster == "" and wantOK == false so
the function correctly handles an empty host input.In
@management/internals/modules/reverseproxy/domain/manager/manager.go:
- Around line 290-307: customDomains are being normalized repeatedly inside the
loop via the normalizedCD variable; precompute and reuse normalized domain
values to avoid redundant work. Create a preprocessed collection (e.g., map or
slice) that stores each domain's normalized form
(strings.ToLower(strings.TrimSuffix(cd.Domain, "."))) alongside its
TargetCluster before the matching loop, then update the matching logic that uses
normalizedCD and TargetCluster (and compares to normalizedHost) to read from the
precomputed normalized value instead of recomputing it per iteration.In
@proxy/README.md:
- Around line 49-51: Update the fenced code block that currently contains the
DNS line "validation.example.com. CNAME ." to include a
language specifier (e.g.,text ordns) so the block becomestext (ordns) ... ``` for proper syntax highlighting in the README; locate the block
in proxy/README.md that wraps that exact line and add the language token
immediately after the opening backticks.</details> --- <details> <summary>ℹ️ Review info</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 333e0450993354323c5d181ee45730b9e7e361f1 and 8c92e70ac3371cfad7d86812c2e2f334ea57dffe. </details> <details> <summary>📒 Files selected for processing (4)</summary> * `management/internals/modules/reverseproxy/domain/manager/manager.go` * `management/internals/modules/reverseproxy/domain/manager/manager_test.go` * `management/internals/modules/reverseproxy/domain/validator.go` * `proxy/README.md` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
|
@kritgrover thanks for your contribution. We are waiting for a refactor on the proxy code from #5472, which caused some merge conflicts. After that, we can review this one. |
|
@mlsmaycon sounds good! |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
management/internals/modules/reverseproxy/domain/manager/manager.go (1)
279-317: Implementation is correct and deterministic.The logic correctly handles all matching scenarios:
- Exact non-wildcard matches are prioritized and return immediately
- The fallback uses the longest-suffix matching strategy for wildcards and subdomains
- Normalization is consistent across host and domain processing
The comparison operator
>(strictly greater-than) ensures deterministic behavior: when multiple custom domains match with identical suffix lengths, the first-processed domain is selected. This is consistent with the documented behavior ("the longest matching suffix wins").One optional enhancement: add a test case explicitly covering mixed wildcard and non-wildcard domains with equal suffix lengths (e.g., host "sub.example.com" matching both "*.example.com" and "example.com") to clarify this edge-case behavior for future maintainers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/internals/modules/reverseproxy/domain/manager/manager.go` around lines 279 - 317, Add a unit test for extractClusterFromCustomDomains that covers the mixed wildcard/non-wildcard edge case: create customDomains containing both a non-wildcard "example.com" and a wildcard "*.example.com" (with different TargetCluster values), call extractClusterFromCustomDomains with host "sub.example.com" (and variants with trailing dot and mixed case) and assert the function returns the deterministic winner (the first-processed domain when suffix lengths are equal) and true; include assertions for normalization (case and trailing dot) to prevent regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@management/internals/modules/reverseproxy/domain/manager/manager.go`:
- Around line 279-317: Add a unit test for extractClusterFromCustomDomains that
covers the mixed wildcard/non-wildcard edge case: create customDomains
containing both a non-wildcard "example.com" and a wildcard "*.example.com"
(with different TargetCluster values), call extractClusterFromCustomDomains with
host "sub.example.com" (and variants with trailing dot and mixed case) and
assert the function returns the deterministic winner (the first-processed domain
when suffix lengths are equal) and true; include assertions for normalization
(case and trailing dot) to prevent regressions.
@mlsmaycon any update on this? |
|
Hey! I see that this PR is still out while #5472 was closed. If there's any help needed on this PR, please let me know, I can dedicate some good time to this (personal interest lol). |
|
|
Any news on this pr? |
|
+1 |
|
For now i was able to proxy www and 301 root domain to it in order to expose my site, but will be better to have the option. Btw, i'm using netbird cloud not selfhost |
|
Would be great to have this merged 🙏 |
|
Yep, we are looking for this feature too. |
8 participants
Describe your changes
Issue ticket number and link
Closes #5417
Link: #5417
Stack
This PR is standalone and not part of a stacked series.
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
New Features
Tests
Documentation