Add private (NetBird-only) services and access_groups to reverse proxy service - #162
Add private (NetBird-only) services and access_groups to reverse proxy service#162Caligone wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughAdds per-target reverse proxy backend options and private-service access groups, including validation, API/Terraform conversion, tests, documentation, examples, and dependency updates. ChangesReverse Proxy Service Enhancements
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Terraform
participant Provider
participant NetBirdAPI
Terraform->>Provider: Submit target options and privacy settings
Provider->>Provider: Validate and convert configuration
Provider->>NetBirdAPI: Create or update reverse proxy service
NetBirdAPI-->>Provider: Return service state
Provider-->>Terraform: Store converted options and privacy state
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
90d7f5e to
10a4837
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/provider/reverse_proxy_service_resource.go (1)
280-283: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider validating
request_timeoutformat.
request_timeoutaccepts an arbitrary string with no client-side format validation, unlikepath_rewritewhich usesstringvalidator.OneOf. An invalid Go duration string (e.g."abc") would only surface as an API error at apply time.terraform-plugin-framework-validatorsdoesn't ship a duration validator out of the box (per its own docs, format-specific validators are expected to be implemented separately), so this would require a small custom validator (e.g. wrappingtime.ParseDuration).♻️ Example custom validator sketch
type durationStringValidator struct{} func (v durationStringValidator) Description(_ context.Context) string { return "value must be a valid Go duration string (e.g. \"30s\", \"2m\")" } func (v durationStringValidator) MarkdownDescription(ctx context.Context) string { return v.Description(ctx) } func (v durationStringValidator) ValidateString(ctx context.Context, req validator.StringRequest, resp *validator.StringResponse) { if req.ConfigValue.IsNull() || req.ConfigValue.IsUnknown() { return } if _, err := time.ParseDuration(req.ConfigValue.ValueString()); err != nil { resp.Diagnostics.AddAttributeError(req.Path, "Invalid duration", err.Error()) } }🤖 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 `@internal/provider/reverse_proxy_service_resource.go` around lines 280 - 283, The request_timeout attribute in reverse_proxy_service_resource.go currently allows any string, so add client-side duration validation in the schema alongside the existing string attributes. Implement or wire in a small custom string validator for request_timeout using time.ParseDuration, then attach it to the request_timeout schema in the reverse proxy resource so invalid values are rejected before apply, similar to how path_rewrite uses stringvalidator.OneOf.
🤖 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.
Nitpick comments:
In `@internal/provider/reverse_proxy_service_resource.go`:
- Around line 280-283: The request_timeout attribute in
reverse_proxy_service_resource.go currently allows any string, so add
client-side duration validation in the schema alongside the existing string
attributes. Implement or wire in a small custom string validator for
request_timeout using time.ParseDuration, then attach it to the request_timeout
schema in the reverse proxy resource so invalid values are rejected before
apply, similar to how path_rewrite uses stringvalidator.OneOf.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5f2bbe5c-4297-4d9d-844a-bbf14e5d4b26
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
docs/data-sources/reverse_proxy_service.mddocs/resources/reverse_proxy_service.mdexamples/resources/netbird_reverse_proxy_service/resource.tfgo.modinternal/provider/reverse_proxy_acc_test.gointernal/provider/reverse_proxy_service_data_source.gointernal/provider/reverse_proxy_service_resource.gointernal/provider/reverse_proxy_service_test.go
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@go.mod`:
- Around line 57-61: Upgrade the indirect golang.org/x/crypto, golang.org/x/net,
and golang.org/x/sys dependencies in go.mod to versions containing the specified
fixes, updating parent or resolved modules as needed. Regenerate go.mod and
go.sum, while preserving the existing dependency resolution and annotations.
In `@internal/provider/reverse_proxy_service_resource.go`:
- Around line 647-652: Update the options-preservation logic around planByID and
the apiModels iteration so duplicate target_id values cannot reuse another
target’s sensitive options. Match each API target to its genuinely unique
planned identity, or retain all planned occurrences per target_id and consume
the corresponding entry deterministically before assigning planModel.Options.
- Around line 402-407: The reverse proxy service validation and read-state
handling must treat an explicitly configured empty access_groups set
consistently for public services. Either reject any non-null access_groups when
private is false, or update the read path to preserve an empty set instead of
converting it to null; apply the chosen behavior around the visible
access_groups validation and corresponding state-mapping logic.
- Around line 391-402: Update the access_groups validation in the
private-service checks so unknown access_groups values are not treated as empty.
Guard the required/forbidden validations with an access_groups known-state
check, while preserving the existing null and non-empty behavior for known
values and the Private handling.
🪄 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: 296c52b1-8636-44f5-87e2-687cc89d8f9b
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
docs/data-sources/reverse_proxy_service.mddocs/resources/reverse_proxy_service.mdexamples/resources/netbird_reverse_proxy_service/resource.tfgo.modinternal/provider/reverse_proxy_acc_test.gointernal/provider/reverse_proxy_service_data_source.gointernal/provider/reverse_proxy_service_resource.gointernal/provider/reverse_proxy_service_test.gointernal/provider/validators.gointernal/provider/validators_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/provider/reverse_proxy_service_data_source.go
- docs/resources/reverse_proxy_service.md
- internal/provider/reverse_proxy_acc_test.go
| golang.org/x/crypto v0.50.0 // indirect | ||
| golang.org/x/mod v0.34.0 // indirect | ||
| golang.org/x/net v0.53.0 // indirect | ||
| golang.org/x/sync v0.20.0 // indirect | ||
| golang.org/x/sys v0.43.0 // indirect |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
go run golang.org/x/vuln/cmd/govulncheck@latest ./...
go list -m all | rg 'golang.org/x/(crypto|net|sys)'Repository: netbirdio/terraform-provider-netbird
Length of output: 1309
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "go.mod excerpt:"
sed -n '1,120p' go.mod
echo
echo "Resolved module versions:"
go list -m all | rg 'golang.org/x/(crypto|net|sys)'
echo
echo "govulncheck (verbose summary):"
go run golang.org/x/vuln/cmd/govulncheck@latest -show verbose ./...Repository: netbirdio/terraform-provider-netbird
Length of output: 7917
🌐 Web query:
golang.org/x/crypto v0.50.0 fixed in v0.52.0 vulnerability SSH agent pkg.go.dev/vuln
💡 Result:
Version v0.52.0 of golang.org/x/crypto was released on May 22, 2026, to address multiple security vulnerabilities, including several issues in the golang.org/x/crypto/ssh/agent package [1]. Key vulnerabilities in the ssh/agent package fixed in v0.52.0 include: 1. Agent Constraints Dropped When Forwarding Keys (CVE-2026-39832): Constraint extensions, such as restrict-destination-v00@openssh.com, were not serialized when adding a key to a remote agent, causing them to be silently stripped [1][2]. The fix ensures the client serializes all constraint extensions and that NewKeyring rejects keys with unsupported constraint extensions [1][3]. 2. Failure to Enforce Key Constraints: The in-memory keyring previously accepted keys with the ConfirmBeforeUse constraint but did not actually enforce it, allowing keys to sign without a confirmation prompt [1][4]. NewKeyring now returns an error when unsupported constraints are requested [1][4]. 3. Pathological Inputs Leading to Client Panic: Certain crafted inputs could cause a panic by creating an ed25519.PrivateKey from malformed wire bytes [1][5]. The fix prevents this panic [1][6]. Users are advised to upgrade to v0.52.0 or higher to resolve these issues [1][7]. Official details and vulnerability reports can be found in the Go vulnerability database under identifiers such as GO-2026-5005, GO-2026-5006, and GO-2026-5033 [4][2][5][3].
Citations:
- 1: https://www.openwall.com/lists/oss-security/2026/05/22/6
- 2: https://pkg.go.dev/vuln/GO-2026-5006
- 3: https://osv.dev/vulnerability/GO-2026-5006
- 4: https://osv.dev/vulnerability/GO-2026-5005
- 5: https://osv.dev/vulnerability/GO-2026-5033
- 6: golang/crypto@v0.50.0...v0.52.0
- 7: https://security.snyk.io/vuln/SNYK-GOLANG-GOLANGORGXCRYPTOSSHAGENT-16796343
Upgrade the vulnerable indirect dependencies in go.mod. golang.org/x/net v0.53.0 is reachable and fixed in v0.55.0; golang.org/x/sys v0.43.0 is below the v0.44.0 Windows fix; and golang.org/x/crypto v0.50.0 is below v0.52.0. Bump the parent or resolved modules and regenerate go.mod/go.sum.
🧰 Tools
🪛 OSV Scanner (2.4.0)
[CRITICAL] 57-57: golang.org/x/crypto 0.50.0: Invoking key constraints not enforced in golang.org/x/crypto/ssh/agent
(GO-2026-5005)
[CRITICAL] 57-57: golang.org/x/crypto 0.50.0: Invoking agent constraints dropped when forwarding keys in golang.org/x/crypto/ssh/agent
(GO-2026-5006)
[CRITICAL] 57-57: golang.org/x/crypto 0.50.0: Invoking byte arithmetic causes underflow and panic in golang.org/x/crypto/ssh
(GO-2026-5013)
[CRITICAL] 57-57: golang.org/x/crypto 0.50.0: Invoking bypass of certificate restrictions in golang.org/x/crypto/ssh
(GO-2026-5014)
[CRITICAL] 57-57: golang.org/x/crypto 0.50.0: Invoking server panic during CheckHostKey/Authenticate in golang.org/x/crypto/ssh
(GO-2026-5015)
[CRITICAL] 57-57: golang.org/x/crypto 0.50.0: Invoking memory leak when rejecting channels can lead to DoS in golang.org/x/crypto/ssh
(GO-2026-5016)
[CRITICAL] 57-57: golang.org/x/crypto 0.50.0: Invoking client can cause server deadlock on unexpected responses in golang.org/x/crypto/ssh
(GO-2026-5017)
[CRITICAL] 57-57: golang.org/x/crypto 0.50.0: Invoking pathological RSA/DSA parameters may cause DoS in golang.org/x/crypto/ssh
(GO-2026-5018)
[CRITICAL] 57-57: golang.org/x/crypto 0.50.0: Invoking bypass of FIDO/U2F security keys physical interaction in golang.org/x/crypto/ssh
(GO-2026-5019)
[CRITICAL] 57-57: golang.org/x/crypto 0.50.0: Invoking infinite loop on large channel writes in golang.org/x/crypto/ssh
(GO-2026-5020)
[CRITICAL] 57-57: golang.org/x/crypto 0.50.0: Invoking auth bypass via unenforced @revoked status in golang.org/x/crypto/ssh/knownhosts
(GO-2026-5021)
[CRITICAL] 57-57: golang.org/x/crypto 0.50.0: Invoking VerifiedPublicKeyCallback permissions skip enforcement in golang.org/x/crypto/ssh
(GO-2026-5023)
[CRITICAL] 57-57: golang.org/x/crypto 0.50.0: Invoking pathological inputs can lead to client panic in golang.org/x/crypto/ssh/agent
(GO-2026-5033)
[CRITICAL] 57-57: golang.org/x/crypto 0.50.0: The golang.org/x/crypto/openpgp package is unmaintained, unsafe by design, and has known security issues
(GO-2026-5932)
[CRITICAL] 57-57: golang.org/x/crypto 0.50.0: golang.org/x/crypto vulnerable to invoking bypass of certificate restrictions
[CRITICAL] 57-57: golang.org/x/crypto 0.50.0: golang.org/x/crypto vulnerable to auth bypass via unenforced @revoked status
[CRITICAL] 57-57: golang.org/x/crypto 0.50.0: golang.org/x/crypto is vulnerable to invoking server panic during CheckHostKey/Authenticate flow
[CRITICAL] 57-57: golang.org/x/crypto 0.50.0: golang.org/x/crypto: FIDO/U2F security key physical presence check can be bypassed
[CRITICAL] 57-57: golang.org/x/crypto 0.50.0: golang.org/x/crypto: Invoking pathological inputs can lead to client panic
[CRITICAL] 57-57: golang.org/x/crypto 0.50.0: golang.org/x/crypto doesn't drop invoking agent constraints when forwarding keys
[CRITICAL] 57-57: golang.org/x/crypto 0.50.0: golang.org/x/crypto doesn't enforce invoking key constraints
[CRITICAL] 57-57: golang.org/x/crypto 0.50.0: golang.org/x/crypto: Invoking byte arithmetic causes underflow and panic
[CRITICAL] 57-57: golang.org/x/crypto 0.50.0: golang.org/x/crypto: Invoking memory leak when rejecting channels can lead to DoS
[CRITICAL] 57-57: golang.org/x/crypto 0.50.0: golang.org/x/crypto vulnerable to infinite loop on large channel writes
[CRITICAL] 57-57: golang.org/x/crypto 0.50.0: golang.org/x/crypto: Invoking client can cause server deadlock on unexpected responses
[CRITICAL] 57-57: golang.org/x/crypto 0.50.0: golang.org/x/crypto: Invoking pathological RSA/DSA parameters may cause DoS
[CRITICAL] 57-57: golang.org/x/crypto 0.50.0: golang.org/x/crypto: Invoking VerifiedPublicKeyCallback permissions skip enforcement
🤖 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 `@go.mod` around lines 57 - 61, Upgrade the indirect golang.org/x/crypto,
golang.org/x/net, and golang.org/x/sys dependencies in go.mod to versions
containing the specified fixes, updating parent or resolved modules as needed.
Regenerate go.mod and go.sum, while preserving the existing dependency
resolution and annotations.
Source: Linters/SAST tools
| // access_groups is only honored for private services and is required there | ||
| // (the API auto-generates the ACL policy from it). Enforce both directions | ||
| // unless the value is unknown at plan time. | ||
| private := data.Private.ValueBool() | ||
| if !data.Private.IsUnknown() && private && (data.AccessGroups.IsNull() || len(data.AccessGroups.Elements()) == 0) { | ||
| resp.Diagnostics.AddAttributeError( | ||
| path.Root("access_groups"), | ||
| "access_groups required for a private service", | ||
| "When private is true, access_groups must list at least one NetBird group ID allowed to reach the service over the tunnel.", | ||
| ) | ||
| } | ||
| if !data.Private.IsUnknown() && !private && !data.AccessGroups.IsNull() && len(data.AccessGroups.Elements()) > 0 { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and inspect the surrounding validation logic.
ast-grep outline internal/provider/reverse_proxy_service_resource.go --view expanded || true
echo '--- relevant lines ---'
sed -n '340,440p' internal/provider/reverse_proxy_service_resource.go
echo '--- search for access_groups handling ---'
rg -n "access_groups|AccessGroups|IsUnknown\(|IsNull\(|Elements\(" internal/provider/reverse_proxy_service_resource.go internal/provider -g '!**/*_test.go'Repository: netbirdio/terraform-provider-netbird
Length of output: 27756
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- schema around access_groups ---'
sed -n '220,260p' internal/provider/reverse_proxy_service_resource.go
echo '--- terraform-to-API conversion around access_groups ---'
sed -n '668,690p' internal/provider/reverse_proxy_service_resource.go
echo '--- any helper patterns for unknown set handling ---'
rg -n "IsUnknown\(\).*Elements\(|Elements\(\).*IsUnknown\(\)|ElementsAs\(" internal/provider -g '!**/*_test.go'Repository: netbirdio/terraform-provider-netbird
Length of output: 4888
🌐 Web query:
Terraform Plugin Framework types.Set IsUnknown Elements unknown set semantics ValidateConfig
💡 Result:
In the Terraform Plugin Framework, when handling types.Set within ValidateConfig or other lifecycle methods, you must explicitly account for unknown values because configuration data can be partially unknown during planning or validation [1][2]. Key semantic behaviors for types.Set and unknown elements include: 1. Set Unknown Status: The (types.Set).IsUnknown method returns true only if the entire set is unknown [3][4]. It returns false if the set itself is known, even if it contains individual elements that are unknown [3][4]. 2. Handling Elements: Because a known set can still contain unknown elements, checking the set's unknown status is insufficient for deep validation [5]. You must iterate over the set's elements using the Elements method and check the IsUnknown status of each individual element [3][4][5]. 3. Validation Strategy: In ValidateConfig, it is standard practice to return early if the set itself is unknown [5]. If the set is known, you should proceed to iterate through the elements, skipping any that are unknown to avoid premature validation errors [1][5]. Example pattern for ValidateConfig: func (r *myResource) ValidateConfig(ctx context.Context, req resource.ValidateConfigRequest, resp *resource.ValidateConfigResponse) { var data myResourceModel resp.Diagnostics.Append(req.Config.Get(ctx, &data)...) if resp.Diagnostics.HasError { return } // Return early if the entire set is unknown if data.MySet.IsUnknown { return } // Iterate through elements to handle potential "deep" unknowns for _, val:= range data.MySet.Elements { if val.IsUnknown { continue // Skip unknown elements to avoid false validation errors } // Perform validation on known elements... } } By following this pattern, you ensure that your provider remains resilient to partial configuration data while still performing validation when concrete values become available [1][5].
Citations:
- 1: https://docs.hashicorp.com/terraform/plugin/framework/validation
- 2: https://developer.hashicorp.com/terraform/plugin/framework/handling-data/accessing-values
- 3: https://developer.hashicorp.com/terraform/plugin/framework/handling-data/types/set
- 4: https://developer.hashicorp.com/terraform/plugin/framework/v1.15.x/handling-data/types/set
- 5: https://discuss.hashicorp.com/t/strategy-for-testing-and-erroring-on-deep-unknowns-in-validateconfig/57814
🌐 Web query:
Terraform Plugin Framework set Elements() unknown value length behavior
💡 Result:
In the Terraform Plugin Framework, the Elements method for collection types (such as types.List, types.Set, and types.Map) returns nil if the collection value is either null or unknown [1][2][3]. When a collection value is unknown, its length is effectively unknown, and the framework does not provide a list of elements [1][2][3]. To safely interact with such attributes, you must first check the status of the collection using the IsNull and IsUnknown methods [1][2][3]. The recommended pattern for handling these types is as follows: 1. Check if the value is null or unknown using data.Attribute.IsNull or data.Attribute.IsUnknown [1][2][3]. 2. Only if the value is known (i.e., IsNull is false and IsUnknown is false) should you proceed to access elements via Elements or ElementsAs [1][2][3]. The IsUnknown method itself returns true if the collection is unknown, and false if the number of elements is known (even if some of those specific elements are themselves unknown) [1][2][3]. Consequently, when IsUnknown returns false, it is safe to call Elements, which will return the known []attr.Value slice (or map) [1][2][3].
Citations:
- 1: https://developer.hashicorp.com/terraform/plugin/framework/handling-data/types/list
- 2: https://developer.hashicorp.com/terraform/plugin/framework/handling-data/types/set
- 3: https://developer.hashicorp.com/terraform/plugin/framework/handling-data/types/map
Skip validation when access_groups is unknown. len(data.AccessGroups.Elements()) treats an unknown set as empty, so this rejects valid plans where the group IDs are resolved during apply.
Proposed fix
- if !data.Private.IsUnknown() && private && (data.AccessGroups.IsNull() || len(data.AccessGroups.Elements()) == 0) {
+ if !data.Private.IsUnknown() && private && !data.AccessGroups.IsUnknown() &&
+ (data.AccessGroups.IsNull() || len(data.AccessGroups.Elements()) == 0) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // access_groups is only honored for private services and is required there | |
| // (the API auto-generates the ACL policy from it). Enforce both directions | |
| // unless the value is unknown at plan time. | |
| private := data.Private.ValueBool() | |
| if !data.Private.IsUnknown() && private && (data.AccessGroups.IsNull() || len(data.AccessGroups.Elements()) == 0) { | |
| resp.Diagnostics.AddAttributeError( | |
| path.Root("access_groups"), | |
| "access_groups required for a private service", | |
| "When private is true, access_groups must list at least one NetBird group ID allowed to reach the service over the tunnel.", | |
| ) | |
| } | |
| if !data.Private.IsUnknown() && !private && !data.AccessGroups.IsNull() && len(data.AccessGroups.Elements()) > 0 { | |
| // access_groups is only honored for private services and is required there | |
| // (the API auto-generates the ACL policy from it). Enforce both directions | |
| // unless the value is unknown at plan time. | |
| private := data.Private.ValueBool() | |
| if !data.Private.IsUnknown() && private && !data.AccessGroups.IsUnknown() && | |
| (data.AccessGroups.IsNull() || len(data.AccessGroups.Elements()) == 0) { | |
| resp.Diagnostics.AddAttributeError( | |
| path.Root("access_groups"), | |
| "access_groups required for a private service", | |
| "When private is true, access_groups must list at least one NetBird group ID allowed to reach the service over the tunnel.", | |
| ) | |
| } | |
| if !data.Private.IsUnknown() && !private && !data.AccessGroups.IsNull() && len(data.AccessGroups.Elements()) > 0 { |
🤖 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 `@internal/provider/reverse_proxy_service_resource.go` around lines 391 - 402,
Update the access_groups validation in the private-service checks so unknown
access_groups values are not treated as empty. Guard the required/forbidden
validations with an access_groups known-state check, while preserving the
existing null and non-empty behavior for known values and the Private handling.
| if !data.Private.IsUnknown() && !private && !data.AccessGroups.IsNull() && len(data.AccessGroups.Elements()) > 0 { | ||
| resp.Diagnostics.AddAttributeError( | ||
| path.Root("access_groups"), | ||
| "access_groups only applies to a private service", | ||
| "access_groups is ignored unless private is true. Set private = true or remove access_groups.", | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file and inspect the relevant areas with line numbers.
wc -l internal/provider/reverse_proxy_service_resource.go
sed -n '360,500p' internal/provider/reverse_proxy_service_resource.go
printf '\n---\n'
sed -n '650,720p' internal/provider/reverse_proxy_service_resource.go
# Search for access_groups handling across the provider.
printf '\n=== access_groups references ===\n'
rg -n "access_groups|AccessGroups" internal/provider -SRepository: netbirdio/terraform-provider-netbird
Length of output: 12527
🏁 Script executed:
#!/bin/bash
set -euo pipefail
wc -l internal/provider/reverse_proxy_service_resource.go
sed -n '360,500p' internal/provider/reverse_proxy_service_resource.go
printf '\n---\n'
sed -n '650,720p' internal/provider/reverse_proxy_service_resource.go
printf '\n=== access_groups references ===\n'
rg -n "access_groups|AccessGroups" internal/provider -SRepository: netbirdio/terraform-provider-netbird
Length of output: 12527
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the schema and surrounding comments for access_groups.
sed -n '210,255p' internal/provider/reverse_proxy_service_resource.go
printf '\n=== resource tests around access_groups ===\n'
sed -n '1240,1345p' internal/provider/reverse_proxy_service_test.go
printf '\n=== data source schema for comparison ===\n'
sed -n '70,95p' internal/provider/reverse_proxy_service_data_source.goRepository: netbirdio/terraform-provider-netbird
Length of output: 7456
Make empty public access_groups round-trip consistently. access_groups = [] is accepted for private = false, but the read path maps any empty API value back to null, so an intentionally configured empty set is lost after apply/refresh and can drift. Either reject non-null access_groups for public services or preserve the empty set in state.
🤖 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 `@internal/provider/reverse_proxy_service_resource.go` around lines 402 - 407,
The reverse proxy service validation and read-state handling must treat an
explicitly configured empty access_groups set consistently for public services.
Either reject any non-null access_groups when private is false, or update the
read path to preserve an empty set instead of converting it to null; apply the
chosen behavior around the visible access_groups validation and corresponding
state-mapping logic.
| // Always keep the planned options: custom_headers is sensitive (the API | ||
| // may redact or normalize it), so the applied value must match the plan | ||
| // exactly to avoid "inconsistent values for sensitive attribute". | ||
| if !planModel.Options.IsUnknown() { | ||
| apiModels[i].Options = planModel.Options | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not preserve options by target_id alone.
The schema permits multiple targets with the same target_id; planByID keeps only the last one. Line 651 can consequently copy another target’s sensitive headers and options into state. Match using a genuinely unique target identity or retain all occurrences per ID and consume them deterministically.
🤖 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 `@internal/provider/reverse_proxy_service_resource.go` around lines 647 - 652,
Update the options-preservation logic around planByID and the apiModels
iteration so duplicate target_id values cannot reuse another target’s sensitive
options. Match each API target to its genuinely unique planned identity, or
retain all planned occurrences per target_id and consume the corresponding entry
deterministically before assigning planModel.Options.
|
Thanks for flagging, and agreed on consolidating. You're right that the per-target For the privacy part, though, I don't think it overlaps with #133: that PR adds So I'd suggest re-scoping this PR down to just |
|
Sounds good, go ahead |
Adds `private` and `access_groups` to netbird_reverse_proxy_service (resource + data source), on top of the per-target options / L4 work in netbirdio#132. A private service is NetBird-only: peers authenticate by their WireGuard tunnel identity and an ACL policy is auto-generated from access_groups to the cluster's proxy-peer group. - access_groups is a Set (unordered): the management API may reorder the group IDs, which would drift a List forever. - ValidateConfig enforces the API invariants: access_groups required iff private=true (known values only), rejected on public services, and private is mutually exclusive with bearer (SSO) auth. - Bumps the netbird SDK v0.67.0 -> v0.74.3 (exposes Private/AccessGroups on ServiceRequest/Service). No further breaking changes beyond netbirdio#132's *[]Target / *Auth adaptation. - Docs and example regenerated.
b503505 to
fcdf5fe
Compare
Caligone
changed the title
2 participants
Summary
Adds
privateandaccess_groupstonetbird_reverse_proxy_service(resource + data source).A private service is NetBird-only: inbound peers authenticate via their WireGuard tunnel identity (no OIDC), and an ACL policy is auto-generated from
access_groupsto the cluster's proxy-peer group.private(bool) — enable NetBird-only mode.access_groups(set of group IDs) — peers allowed to reach the service over the tunnel. Modeled as a set since the management API may reorder the IDs (a list would drift forever).Scope
Rebased on top of #132 (per-target options + L4). This PR now contains only
private/access_groups— the per-targetoptionsblock is dropped and deferred to #132, as discussed.Validation
ValidateConfigenforces the API invariants:access_groupsrequired whenprivate = true(checked only for known values).access_groupsrejected on public services (the API ignores it; the read path maps it back to null, so an explicitly-set value would drift).privateis mutually exclusive with bearer (SSO) auth.SDK
Bumps
netbirdio/netbirdv0.67.0 → v0.74.3 (exposesPrivate/AccessGroupsonServiceRequest/Service). No breaking changes beyond #132's*[]ServiceTarget/*ServiceAuthConfigadaptation, which this builds on.Docs and example regenerated; unit tests cover the round-trip and the non-private (null access_groups) case.