Skip to content

[management] Accept domain target type for L4 reverse proxy services#5612

Merged
lixmal merged 2 commits intomainfrom
fix/l4-domain-target-type
Mar 17, 2026
Merged

[management] Accept domain target type for L4 reverse proxy services#5612
lixmal merged 2 commits intomainfrom
fix/l4-domain-target-type

Conversation

@lixmal
Copy link
Copy Markdown
Collaborator

@lixmal lixmal commented Mar 17, 2026

Describe your changes

L4 services (TCP/TLS/UDP) rejected target_type: "domain" even though HTTP services accepted it. This caused domain-backed resources to fail when used as targets for L4 services. Additionally, there was no validation that target_type matched the actual network resource type, so sending target_type: "host" for a domain resource would silently produce an invalid backend address.

This PR adds TargetTypeDomain to L4 validation and cross-validates that the declared target_type matches the actual resource type during validateTargetReferences.

Issue ticket number and link

Stack

Checklist

  • Is it a bug fix
  • Is a typo/documentation fix
  • Is a feature enhancement
  • It is a refactor
  • Created tests that fail without the change (if possible)

By submitting this pull request, you confirm that you have read and agree to the terms of the Contributor License Agreement.

Documentation

Select exactly one:

  • I added/updated documentation for this change
  • Documentation is not needed for this change (explain why)

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

    • Domain targets are now supported in Layer 4 services (TCP, TLS, UDP).
  • Improvements

    • Target validation now centralizes checks and enforces that configured target types match the actual resource types, producing clearer errors for mismatches.
  • Tests

    • Added tests covering target type validation and Layer 4 domain target acceptance.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 17, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 185f1f9c-b93e-42f6-9c71-8afb98f684e0

📥 Commits

Reviewing files that changed from the base of the PR and between 63c2611 and 8d15835.

📒 Files selected for processing (2)
  • management/internals/modules/reverseproxy/service/manager/manager.go
  • management/internals/modules/reverseproxy/service/service_test.go

📝 Walkthrough

Walkthrough

This PR centralizes target validation in the reverse-proxy manager by adding helpers that validate peers and network resources (with explicit resource-type checks) and expands L4 service validation to allow Domain-type targets.

Changes

Cohort / File(s) Summary
Manager: target validation
management/internals/modules/reverseproxy/service/manager/manager.go
Introduced helper functions validatePeerTarget, validateResourceTarget, and validateResourceTargetType; replaced direct lookups with delegated validation and added an import alias for network resource types.
Manager tests
management/internals/modules/reverseproxy/service/manager/manager_test.go
Added tests for resource-type mismatch and valid peer targets; mocks for GetNetworkResourceByID and GetPeerByID to assert error/no-error paths.
L4 service validation
management/internals/modules/reverseproxy/service/service.go
Relaxed L4 target validation to allow TargetTypeDomain in addition to TargetTypePeer and TargetTypeHost.
Service tests
management/internals/modules/reverseproxy/service/service_test.go
Added TestValidate_L4DomainTargetValid to verify L4 modes (tcp/tls/udp) accept Domain targets.
Module file
go.mod
Minor module change (single-line adjustment).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • mlsmaycon
  • bcmmbaga

Poem

🐰 I hop through code with joyful cheer,
I check each target far and near,
Peers and resources now inspected right,
Domains can L4 dance through the night. ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: enabling domain target type support for L4 reverse proxy services.
Description check ✅ Passed The description covers the problem statement, the solution, includes the completed checklist with bug fix and test creation marked, and properly addresses the documentation requirement.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/l4-domain-target-type
📝 Coding Plan
  • Generate coding plan for human review comments

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.

❤️ Share

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

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@management/internals/modules/reverseproxy/service/service_test.go`:
- Around line 866-868: The UDP test case in service_test.go sets the
Target.Protocol incorrectly to "tcp"; update the Target literal in the Targets
slice (the entry with TargetId "resource-1" and TargetType TargetTypeDomain) to
use Protocol: "udp" for the UDP mode so it matches other UDP tests (e.g.,
TestValidate_UDPProxyProtocolRejected) and correctly exercises UDP behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ebc8783c-4082-4fa6-9160-e61024b20d86

📥 Commits

Reviewing files that changed from the base of the PR and between a590c38 and 63c2611.

📒 Files selected for processing (4)
  • management/internals/modules/reverseproxy/service/manager/manager.go
  • management/internals/modules/reverseproxy/service/manager/manager_test.go
  • management/internals/modules/reverseproxy/service/service.go
  • management/internals/modules/reverseproxy/service/service_test.go

Comment thread management/internals/modules/reverseproxy/service/service_test.go
@sonarqubecloud
Copy link
Copy Markdown

@lixmal lixmal merged commit f0eed50 into main Mar 17, 2026
45 checks passed
@lixmal lixmal deleted the fix/l4-domain-target-type branch March 17, 2026 15:29
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.

2 participants