feat: add reverse-proxy services and custom domain management - #62
Conversation
Add full support for the NetBird Services API (/api/reverse-proxies/*) covering services, custom domains, and proxy clusters. New module: - netbird_service_domain: create/delete custom domains with target_cluster association and DNS validation trigger Extended modules: - netbird_service: add 'cluster' to target_type choices - netbird_info: add service_domains and proxy_clusters resource types Infrastructure: - netbird_api.py: 6 new API methods (domains + clusters CRUD) - netbird_resolve.py: _resolve_service for access_groups name→ID - meta/runtime.yml: register netbird_service_domain in action_groups - tasks/services.yml: task file for services, domains, cluster cleanup - tasks/main.yml: include services after networks, before routes - defaults/main.yml: netbird_services, netbird_service_domains, netbird_proxy_clusters_absent variables with examples - config_skeleton/services.yml: config-as-code skeleton Roles: - configure: load services config, pre-flight validation, apply phase, strict-mode cleanup, blast-radius guard - export: fetch and export services/domains (clean + raw) - export template: services.yml.j2 with group ID resolution Testing: - Unit tests for netbird_service_domain (6 test cases) - Integration test playbook with comprehensive coverage: smoke test, domain lifecycle, private/public services, auth variants (password, PIN, bearer), path routing, header options, idempotency, delete/recreate Documentation: - README: module examples, updated info resources, config structure, dependency ordering, API reference link Tested against live preprod environment. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds Ansible support for NetBird reverse-proxy services, custom service domains, and proxy-cluster cleanup. It adds API operations, configuration, lifecycle automation, diff reporting, export handling, documentation, and tests. ChangesService management
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ConfigureRole
participant ServiceModules
participant NetBirdAPI
participant NetBird
ConfigureRole->>ServiceModules: apply service and domain configuration
ServiceModules->>NetBirdAPI: list, create, validate, or delete resources
NetBirdAPI->>NetBird: send service and domain requests
NetBird-->>NetBirdAPI: return resource state
NetBirdAPI-->>ConfigureRole: return operation results
Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
roles/configure/tasks/main.yml (1)
12-19: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate outdated "services" omissions in operator-facing text.
Two blocks of operator-facing text were not updated when services were added to strict mode:
- Lines 12-19: the file's top "Execution order" comment lists 7 phases and omits Services, even though the inline comment at line 636 marks it "--- 8. Services (depend on groups for access_groups) ---" and the rescue block at line 839 was updated to include "services" in the apply order.
- Lines 299-306: the strict-mode blast-radius
fail_msgstates an empty-config strict apply would remove "ALL groups, policies, networks, DNS nameserver groups, DNS zones, setup keys and posture checks" but does not mention services, even thoughnetbird_serviceswas added to the guard condition at lines 295-296 and services are now deleted in strict mode (lines 796-804).An operator reading either message would underestimate what strict mode removes.
📝 Proposed fix
# Execution order (respects dependencies): # 1. Account settings (no dependencies) # 2. Posture checks (no dependencies, needed by policies) # 3. Groups (no dependencies, needed by everything else) # 4. Setup keys (depend on groups for auto_groups) # 5. DNS (depend on groups) # 6. Networks (depend on groups) -# 7. Policies (depend on groups + posture checks) +# 7. Services (depend on groups for access_groups) +# 8. Policies (depend on groups + posture checks)Strict mode (strict=true) is enabled but the configuration loaded from {{ _config_dir_abs }}/ contains zero managed resources. A strict apply deletes everything on the control plane that is not in the config, so from an empty config it would remove ALL groups, policies, networks, - DNS nameserver groups, DNS zones, setup keys and posture checks. + DNS nameserver groups, DNS zones, setup keys, posture checks and services.Also applies to: 299-306
🤖 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 `@roles/configure/tasks/main.yml` around lines 12 - 19, Update the operator-facing comments in the execution-order block and strict-mode blast-radius fail_msg to include services. Add Services as phase 8 with its dependency on groups in the order description, and mention services among the resources removed by an empty-config strict apply, keeping the existing wording and ordering otherwise unchanged.plugins/modules/netbird_service.py (1)
108-118: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate
target_iddocumentation for the newclustertarget_type.The
target_iddescription says it is the "ID of the NetBird network resource (subnet/host) the target rides." This description does not cover the newclusterchoice. Fortarget_type: cluster,target_idholds a proxy-cluster address (for examplesubdomain.netbird.io, as shown indefaults/main.yml), not a network-resource ID. Update the description to clarify this distinction so users do not expect a resource ID when usingcluster.📝 Proposed documentation fix
target_id: description: - - ID of the NetBird network resource (subnet/host) the target rides. + - ID of the NetBird network resource (subnet/host) the target rides, + or the proxy-cluster address when C(target_type=cluster). type: str required: trueAlso applies to: 468-468
🤖 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 `@plugins/modules/netbird_service.py` around lines 108 - 118, Update the target_id description in the module argument specification to cover all target_type values: retain the network resource ID meaning for subnet/host and clarify that cluster uses a proxy-cluster address such as a subdomain. Keep the existing target_type choices and behavior unchanged.
🧹 Nitpick comments (2)
tests/unit/plugins/modules/test_netbird_service_domain.py (2)
143-152: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the call order for the re-create path.
The recreate path must delete the old domain before it creates the replacement. The current assertions confirm that both calls happened, but not their order. Assert the exact sequence so a future reordering fails the test.
♻️ Proposed test tightening
assert module.exit_kwargs['changed'] is True - assert ('delete', 'dom-1') in recorded['calls'] - assert any( - isinstance(c, tuple) and c[0] == 'create' - for c in recorded['calls']) + assert recorded['calls'] == [ + 'list', + ('delete', 'dom-1'), + ('create', { + 'domain': 'app.example.com', + 'target_cluster': 'us.proxy.netbird.io', + }), + ]🤖 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 `@tests/unit/plugins/modules/test_netbird_service_domain.py` around lines 143 - 152, Update test_different_cluster_triggers_recreate to assert the recorded delete call occurs before the replacement create call, using the ordered recorded['calls'] sequence and preserving the existing changed assertion.
107-173: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd check_mode coverage.
netbird_service_domaindeclarescheck_mode: fullsupport.DummyModulealready accepts acheck_modeflag, but no test sets it, andrun_moduledoes not forward it. The check-mode branches in the module (create skip, delete skip, and the recreate branch that returns the existing domain) stay untested.💚 Proposed helper change plus a test
-def run_module(monkeypatch, params, existing_domains=None): +def run_module(monkeypatch, params, existing_domains=None, check_mode=False): """Drive netbird_service_domain.main() with patched deps. Returns (module, recorded_calls) where recorded_calls tracks which API methods were called and with what arguments. """ if existing_domains is None: existing_domains = []- module = DummyModule(full) + module = DummyModule(full, check_mode=check_mode)Then add a test:
def test_check_mode_reports_change_without_api_write(self, monkeypatch): module, recorded = run_module(monkeypatch, { 'domain': 'app.example.com', 'target_cluster': 'eu.proxy.netbird.io', }, check_mode=True) assert module.exit_kwargs['changed'] is True assert recorded['calls'] == ['list']🤖 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 `@tests/unit/plugins/modules/test_netbird_service_domain.py` around lines 107 - 173, Extend the test helper run_module to accept and forward a check_mode argument to DummyModule, then add coverage for check-mode create, delete, and differing-cluster recreate paths. Verify these paths report the correct changed status while recording only the list API call, including the provided create scenario.
🤖 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 `@plugins/modules/netbird_service_domain.py`:
- Around line 163-168: Validate the response returned by
api.create_service_domain before accessing created['id'] in the service-domain
creation flow, including the corresponding path around the second create call.
Ensure it is a mapping containing a valid id; otherwise fail with the module’s
clear task error mechanism instead of allowing KeyError or TypeError, while
preserving validation for valid responses.
- Around line 159-172: Update the target_cluster replacement flow in the
existing-domain branch to preserve the original domain details before
api.delete_service_domain, then restore that domain if api.create_service_domain
fails. Re-raise the original creation error after rollback, while keeping
successful creation and validation behavior unchanged.
In `@README.md`:
- Line 662: Update the Available resources list in README.md to include
dns_zones alongside the other supported resources, matching the resource choices
accepted by netbird_info.
In `@roles/configure/tasks/main.yml`:
- Around line 547-552: Extend the preview diff flow in preview_diff_report.yml
to compare the fetched api_services data, produce diff_services, and include
service/domain changes in the displayed diff and summary totals. Ensure the
services diff is computed and reported for both preview and strict modes,
reusing the existing diff/report patterns for groups, policies, and other
resources.
In `@roles/export/tasks/main.yml`:
- Line 380: Update the raw file listing immediately following the services
summary entry in the export summary task to include services_raw.yml and
service_domains_raw.yml alongside the existing resource files, preserving the
current listing format.
In `@roles/export/templates/export/services.yml.j2`:
- Around line 86-88: Update the selectattr expression in the
service_domains_data guard to apply the same default-empty-string handling used
by dom.type at line 80, so records without a type are safely excluded while
custom-typed records continue to match.
- Around line 13-73: Update the service loop in services.yml.j2 to preserve
svc.auth metadata during export, matching the fields consumed by
tasks/services.yml and config-role service management. Emit non-secret
authentication enablement and group fields, while clearly marking masked secret
fields or adding a warning for services whose authentication secrets cannot be
exported. Ensure imported configurations do not interpret omitted auth as a
request to disable existing authentication.
In `@tests/integration/test_services.yml`:
- Around line 526-543: Update the Phase 4e and Phase 4f service tests to
generate unique password and PIN values at runtime instead of using literal
credentials, then add unconditional teardown at the end of Phase 4 that deletes
both registered services regardless of CLEANUP. Preserve the existing optional
cleanup behavior for other resources.
- Around line 169-173: Ensure the resources created in Phase 1, particularly
_group_svc and _group_access, are available whenever later phases require them:
move their creation outside the smoke_test-conditional block, or apply the same
smoke_test condition to Phase 4 and related unconditional cleanup. Preserve the
documented SMOKE_TEST=false behavior without undefined-variable failures.
- Around line 31-36: Update the test_prefix, smoke_test, and cleanup variables
to make default() replace empty environment values by passing true as its second
argument, and cast smoke_test and cleanup to booleans so string values such as
"false" are handled correctly. In the Phase 1 task guard, simplify the condition
to `when: smoke_test | bool` while preserving the existing prefix and documented
defaults.
---
Outside diff comments:
In `@plugins/modules/netbird_service.py`:
- Around line 108-118: Update the target_id description in the module argument
specification to cover all target_type values: retain the network resource ID
meaning for subnet/host and clarify that cluster uses a proxy-cluster address
such as a subdomain. Keep the existing target_type choices and behavior
unchanged.
In `@roles/configure/tasks/main.yml`:
- Around line 12-19: Update the operator-facing comments in the execution-order
block and strict-mode blast-radius fail_msg to include services. Add Services as
phase 8 with its dependency on groups in the order description, and mention
services among the resources removed by an empty-config strict apply, keeping
the existing wording and ordering otherwise unchanged.
---
Nitpick comments:
In `@tests/unit/plugins/modules/test_netbird_service_domain.py`:
- Around line 143-152: Update test_different_cluster_triggers_recreate to assert
the recorded delete call occurs before the replacement create call, using the
ordered recorded['calls'] sequence and preserving the existing changed
assertion.
- Around line 107-173: Extend the test helper run_module to accept and forward a
check_mode argument to DummyModule, then add coverage for check-mode create,
delete, and differing-cluster recreate paths. Verify these paths report the
correct changed status while recording only the list API call, including the
provided create scenario.
🪄 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 Plus
Run ID: be08307b-8580-484b-8416-bf55d0981c55
📒 Files selected for processing (16)
README.mdconfig_skeleton/services.ymldefaults/main.ymlmeta/runtime.ymlplugins/filter/netbird_resolve.pyplugins/module_utils/netbird_api.pyplugins/modules/netbird_info.pyplugins/modules/netbird_service.pyplugins/modules/netbird_service_domain.pyroles/configure/tasks/main.ymlroles/export/tasks/main.ymlroles/export/templates/export/services.yml.j2tasks/main.ymltasks/services.ymltests/integration/test_services.ymltests/unit/plugins/modules/test_netbird_service_domain.py
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
I'll work on all the CodeRabbit suggestions in the coming week |
|
Thanks @RollLikeRollo — the custom-domain module and services coverage in the roles fill real gaps. Rebase: v1.3.0 shipped this week. Your Blockers:
Also:
Will re-review once rebased. |
# Conflicts: # plugins/modules/netbird_service.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
README.md (2)
22-32: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the duplicate
Servicesfeature entry.The Features list documents Services on Lines 22 and 30. Keep one entry and merge the reverse-proxy details into it.
🤖 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 `@README.md` around lines 22 - 32, Remove the duplicate Services entry in the README Features list and merge the reverse-proxy details from the later entry into the retained Services description, leaving a single comprehensive Services feature.
691-697: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDocument the destructive
target_clusterchange.When
target_clusterchanges,plugins/modules/netbird_service_domain.pydeletes and recreates the domain. This resets validation and changes the domain ID. Add a warning to the custom-domain section.Suggested documentation
+> **Warning:** Changing `target_cluster` deletes and recreates the custom domain. This resets validation and changes the domain ID.🤖 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 `@README.md` around lines 691 - 697, Add a warning to the README custom-domain documentation explaining that changing target_cluster deletes and recreates the domain, resetting validation and changing its domain ID. Place it near the custom-domain configuration or deletion example, without changing the module behavior.
🤖 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.
Outside diff comments:
In `@README.md`:
- Around line 22-32: Remove the duplicate Services entry in the README Features
list and merge the reverse-proxy details from the later entry into the retained
Services description, leaving a single comprehensive Services feature.
- Around line 691-697: Add a warning to the README custom-domain documentation
explaining that changing target_cluster deletes and recreates the domain,
resetting validation and changing its domain ID. Place it near the custom-domain
configuration or deletion example, without changing the module behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3c992ff9-3857-4f91-96cb-8fe0adc5ae60
📒 Files selected for processing (4)
README.mddefaults/main.ymlplugins/module_utils/netbird_api.pyroles/configure/tasks/main.yml
🚧 Files skipped from review as they are similar to previous changes (3)
- defaults/main.yml
- roles/configure/tasks/main.yml
- plugins/module_utils/netbird_api.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@plugins/filter/netbird_diff.py`:
- Around line 261-262: Update the desired-service construction in the `des`
comprehension to exclude the lifecycle-only `state` key alongside the existing
`_SERVICE_SKIP` keys. Ensure `_deep_diff` compares only API-represented service
fields so `state: present` does not produce a spurious change.
In `@plugins/modules/netbird_service_domain.py`:
- Around line 172-193: Update the domain handling flow around the rollback
create in the module’s existing-domain logic to retain its response and run
validation when validate is enabled, including marking the operation changed and
starting asynchronous validation. When the existing domain already targets the
requested cluster, inspect its validation state and start validation only if
unvalidated, reporting changed in check mode when validation would run. Add
tests covering successful rollback validation and validation of an
already-existing unvalidated domain.
🪄 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 Plus
Run ID: ed630ca0-a980-4043-8f21-f1f745a35eb4
📒 Files selected for processing (6)
plugins/filter/netbird_diff.pyplugins/modules/netbird_service_domain.pyroles/configure/tasks/preview_diff_report.ymlroles/export/tasks/main.ymlroles/export/templates/export/services.yml.j2tests/integration/test_services.yml
🚧 Files skipped from review as they are similar to previous changes (3)
- roles/export/tasks/main.yml
- tests/integration/test_services.yml
- roles/export/templates/export/services.yml.j2
… PUT exists): new ID, validation reset, and the server has no in-use check, so services bound to the domain break. Needs a doc note and a module.warn().
|
Thanks for all the suggestions @SunsetDrifter. i am in the process of remedying what is needed.
I think that Services are not missing, but it is true that Service domains are missing. I'll work with the narrative that Service domains are to be added and that was the intention of your comment. |
…ds path and protocol: http, which the API rejects for TCP services.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
roles/configure/tasks/main.yml (2)
872-872: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the partial-apply order.
Line 872 omits service domains. The role applies service domains before services. Add
service domainsbeforeservicesso the recovery guidance matches the task order.🤖 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 `@roles/configure/tasks/main.yml` at line 872, Update the recovery-order message at the affected configuration string to include “service domains” immediately before “services,” preserving the existing ordering and wording of all other resource types.
286-314: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInclude service domains in the strict-mode non-empty check.
Line 298 includes
netbird_servicesbut excludesnetbird_service_domains. A strict apply that manages only custom domains fails before it applies the declared domains. Addnetbird_service_domainsto this expression. Also include services and service domains in the failure message.Proposed fix
(netbird_posture_checks | default([])) + - (netbird_services | default([])) + (netbird_services | default([])) + + (netbird_service_domains | default([])) ) | length > 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 `@roles/configure/tasks/main.yml` around lines 286 - 314, Update the strict-mode non-empty assertion in the pre-flight validation to include netbird_service_domains alongside the existing managed-resource variables. Extend the fail_msg resource list to mention both services and service domains, preserving the current validation behavior and messaging structure.tests/integration/test_services.yml (1)
271-275: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle an empty
service_domainsresult.Line 274 indexes the list before
default('netbird.app')can apply. If the API returns an empty list, this task fails. The Phase 2 assertion permits an empty list. Select the first domain safely before applying the fallback.Proposed fix
- {{ _info_domains.data[0].domain - | default('netbird.app') }} + {{ _info_domains.data + | map(attribute='domain') + | first + | default('netbird.app', true) }}🤖 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 `@tests/integration/test_services.yml` around lines 271 - 275, Update the “Phase 3: Derive base domain” task’s _base_domain expression to safely select the first element of _info_domains.data when the list is empty, then apply the existing netbird.app fallback. Preserve the current domain value for non-empty results and remain consistent with the Phase 2 empty-list behavior.
🤖 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 `@roles/configure/tasks/main.yml`:
- Around line 555-559: Update the task registering api_service_domains to run
only when declared service domains exist in the configuration, while preserving
the existing commit and strict gating. Ensure preview or strict runs with no
services or netbird_service_domains skip the service_domains API request instead
of contacting the endpoint.
---
Outside diff comments:
In `@roles/configure/tasks/main.yml`:
- Line 872: Update the recovery-order message at the affected configuration
string to include “service domains” immediately before “services,” preserving
the existing ordering and wording of all other resource types.
- Around line 286-314: Update the strict-mode non-empty assertion in the
pre-flight validation to include netbird_service_domains alongside the existing
managed-resource variables. Extend the fail_msg resource list to mention both
services and service domains, preserving the current validation behavior and
messaging structure.
In `@tests/integration/test_services.yml`:
- Around line 271-275: Update the “Phase 3: Derive base domain” task’s
_base_domain expression to safely select the first element of _info_domains.data
when the list is empty, then apply the existing netbird.app fallback. Preserve
the current domain value for non-empty results and remain consistent with the
Phase 2 empty-list behavior.
🪄 Autofix
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 Plus
Run ID: 5928e104-380b-48f9-832a-5bf83e76fb9f
📒 Files selected for processing (14)
README.mdchangelogs/fragments/service-domains-and-proxy-clusters.ymldefaults/main.ymlmeta/runtime.ymlplugins/filter/netbird_diff.pyplugins/modules/netbird_proxy_cluster.pyplugins/modules/netbird_service_domain.pyroles/configure/tasks/main.ymlroles/configure/tasks/preview_diff_report.ymlroles/export/tasks/main.ymlroles/export/templates/export/services.yml.j2tasks/services.ymltests/integration/test_services.ymltests/unit/plugins/modules/test_netbird_service_domain.py
🚧 Files skipped from review as they are similar to previous changes (9)
- defaults/main.yml
- meta/runtime.yml
- README.md
- roles/configure/tasks/preview_diff_report.yml
- tasks/services.yml
- plugins/filter/netbird_diff.py
- roles/export/templates/export/services.yml.j2
- plugins/modules/netbird_service_domain.py
- roles/export/tasks/main.yml
| - name: Fetch current service domains | ||
| community.ansible_netbird.netbird_info: | ||
| resource: service_domains | ||
| register: api_service_domains | ||
| when: not commit | bool or strict | bool |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline plugins/modules/netbird_info.py --view expanded
rg -n -C 8 \
'service_domains|proxy_clusters|reverse-proxies|NetBirdAPIError|status_code|404' \
plugins/modules/netbird_info.py \
plugins/module_utils/netbird_api.py \
roles/configure/tasks/main.yml \
roles/configure/tasks/preview_diff_report.ymlRepository: netbirdio/ansible-netbird
Length of output: 42601
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
module_path = Path('plugins/modules/netbird_info.py')
util_path = Path('plugins/module_utils/netbird_api.py')
module_src = module_path.read_text()
util_src = util_path.read_text()
checks = {
'list_service_domains endpoint': "'/api/reverse-proxies/domains'" in util_src,
'netbird_info catches API errors for all resources': 'except NetBirdAPIError as e' in module_src,
'netbird_info fails JSON on API errors with status_code and response': 'module.fail_json(msg=str(e), status_code=e.status_code, response=e.response)' in module_src,
'service_domains task not skipped by empty YAML services/domains': '/api/reverse-proxies/domains' not in util_src,
'API request returns 4xx directly': 'status_code = e.code' in util_src and 'raise NetBirdAPIError' in util_src,
}
for name, value in checks.items():
print(f'{name}: {value}')
# Minimal parser-probe of relevant sections.
if re.search(r'if status_code.*[4-5]\d\d.*raise NetBirdAPIError', util_src, re.S):
print('contains_4xx_raises_api_error: True')
else:
print('contains_4xx_raises_api_error: False')
print('--- netbird_info module snippet ---')
for line_no in range(190, 232):
print(f'{line_no}: {module_src.splitlines()[line_no-1]}')
print('--- utility HTTPError handling ---')
matches = list(re.finditer(r'except HTTPError as e:(.*?)raise NetBirdAPIError[(][^)]*?[:\n]', util_src, re.S))
for match in matches:
print(match.group(0))
PYRepository: netbirdio/ansible-netbird
Length of output: 3050
🌐 Web query:
ansible.netbird module_netbird_http_utils NetBirdAPIError HTTPError handling
💡 Result:
In the community.ansible_netbird Ansible collection, NetBirdAPIError is the base exception class used for handling API-related failures [1][2]. This exception and its subclasses are managed within the collection's internal module utilities (specifically plugins/module_utils/netbird_api.py) to encapsulate HTTP communication errors with the NetBird Management API [1][3]. Key aspects of error handling in the collection include: 1. Exception Hierarchy: NetBirdAPIError serves as the root exception [1]. Recent updates have subclassed this to include more specific error types such as NetBirdConnectionError and NetBirdSSLError, allowing for more granular debugging and handling (e.g., distinguishing between SSL/TLS failures and general connection timeouts) [1]. 2. Transport Handling: The collection is designed to be resilient by avoiding the auto-following of HTTP redirects to prevent security risks like leaking API tokens [3]. If a request is redirected (e.g., an HTTP URL bouncing to HTTPS), it is surfaced as a configuration error rather than a silent failure [4][3]. 3. Debugging and Tracebacks: When errors occur, the collection is designed to chain underlying exceptions using 'raise... from e', which ensures that tracebacks are preserved and visible when running Ansible with increased verbosity (e.g., ansible -vvvv) [1]. 4. Common Troubleshooting: - Timeouts: Requests default to a 30-second timeout [4][5]. If operations are failing due to latency, you can adjust this globally via module_defaults or per-module using the timeout parameter [4]. - SSL/TLS: If verification fails, the recommended practice is to trust the CA on the control node rather than disabling certificate validation [3][5]. - Configuration: If you encounter 401 or 403 errors, ensure your API token and API URL are correctly configured; the API URL should be the base URL without the /api suffix [6][4]. Note that this collection's internal error handling logic is distinct from the Python netbird SDK, which also uses a NetBirdAPIError class but operates as a separate library for client-side integrations [7].
Citations:
- 1: fcac60c
- 2: 51f3141
- 3: d716765
- 4: https://docs.netbird.io/selfhosted/iac/ansible
- 5: https://github.com/netbirdio/docs/blob/main/src/pages/selfhosted/iac/ansible.mdx
- 6: https://github.com/netbirdio/ansible-netbird
- 7: https://pypi.org/project/netbird/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
util_src = Path('plugins/module_utils/netbird_api.py').read_text()
http_util_candidates = list(Path('plugins/module_utils').glob('*.py'))
netbird_http_utils_src = None
for candidate in http_util_candidates:
if 'netbird_http' in candidate.name or 'http_utils' in candidate.name:
try:
netbird_http_utils_src = candidate.read_text()
print(f'FOUND: {candidate}')
break
except UnicodeDecodeError:
continue
if netbird_http_utils_src is None:
print("No existing module_utils NetBird HTTP helper file; only netbird_api.py matters for HTTPError handling.")
else:
print("'NetBirdAPIError' in module_utils helper:", 'NetBirdAPIError' in netbird_http_utils_src)
# Read-only behavioral model: if _request raises NetBirdAPIError for HTTPError, netbird_info exits via fail_json.
# Show the relevant raise expression and absence of special 404 empty-data handling for service_domains.
for expr in ['raise NetBirdAPIError(', 'status_code = None', 'response_data = []']:
print(f"'{expr}' in netbird_api.py: {expr in util_src}")
task_src = Path('roles/configure/tasks/main.yml').read_text()
preview_src = Path('roles/configure/tasks/preview_diff_report.yml').read_text()
print('service_domains referenced before management task:', task_src.index('list_service_domains') if 'list_service_domains' in task_src else 'N/A')
print('service_domains task condition includes any YAML-domain check:', bool(re.search(r'when:.*netbird_service_domains|when:.*services', task_src[task_src.find("Fetch current service domains"):task_src.find("Fetch current service domains")+500])))
PYRepository: netbirdio/ansible-netbird
Length of output: 531
Gate service-domain metadata fetches behind declared service domains.
netbird_info forwards API HTTP errors, including unavailable reverse-proxy endpoints, through NetBirdAPIError to fail_json; it does not return empty data for missing service_domains. In preview or strict runs with no services/netbird_service_domains, this fetch still calls /api/reverse-proxies/domains and can fail unrelated to the configured resources. Move the service_domains fetch behind a YAML-domain check or add explicit empty-data handling only if the NetBird API path is introduced.
🤖 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 `@roles/configure/tasks/main.yml` around lines 555 - 559, Update the task
registering api_service_domains to run only when declared service domains exist
in the configuration, while preserving the existing commit and strict gating.
Ensure preview or strict runs with no services or netbird_service_domains skip
the service_domains API request instead of contacting the endpoint.
There was a problem hiding this comment.
I looked into this but skipping the fetch breaks strict mode — if the config has no service domains but the server does, those orphans become invisible and strict mode silently leaves them in place. None of the other resource fetches (api_services, api_policies, etc.) are gated this way for the same reason.
The 404 concern is real for the export role (handled there with ignore_errors), but the configure role only gets here after loading services.yml, which user has toc onfigure willingly, so we can presume reverse-proxy API is available when the user wants to configure services.
There was a problem hiding this comment.
@RollLikeRollo, agreed: strict mode must fetch service domains even when netbird_service_domains is empty. Otherwise, strict mode cannot detect unmanaged custom domains. A condition based only on declared domains would be incorrect.
One detail remains: services.yml is optional. The configure role reaches this task when no services.yml file exists because Load services is conditional, but the fetch is not. Therefore, a preview run for an installation without reverse-proxy configuration can still call the unavailable endpoint.
A safe condition would preserve the fetch in strict mode and skip it only for non-strict preview runs with no declared services or service domains. I will narrow the finding accordingly rather than request the original YAML-domain-only guard.
🐇
✏️ Learnings added
Learnt from: RollLikeRollo
Repo: netbirdio/ansible-netbird PR: 62
File: roles/configure/tasks/main.yml:555-559
Timestamp: 2026-08-05T11:48:03.728Z
Learning: In `roles/configure/tasks/main.yml`, strict mode must fetch the service-domain inventory even when `netbird_service_domains` is empty. The role uses that inventory to detect and remove unmanaged custom domains during the strict-mode cleanup phase. `services.yml` is optional, so the configure role can reach service API fetch tasks without loading that file.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
|
Ran the export→apply round trip with services present and hit one real problem, plus a smaller preview issue. Group names leak into
|
Export resolves group IDs to names in auth.bearer_auth.distribution_groups, but _resolve_service only resolved access_groups back to IDs. This caused an export→apply round trip to silently replace stored group IDs with group names in the API, breaking bearer auth with no self-healing path. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… config Three shape mismatches between the exported YAML and the API response made _compare_service flag every service as CHANGED on an unedited export: 1. access_groups/distribution_groups: names vs IDs — pass group_ids into _compare_service and resolve exported names before comparing. 2. targets: flat keys (direct_upstream, skip_tls_verify) vs nested options dict — flatten both sides and filter API targets to only keys the desired config declares. 3. auth: top-level-only declared-keys filter reported password_auth and pin_auth as removed when only bearer_auth was exported — apply the filter recursively into auth sub-dicts. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 participants
Add full support for the NetBird Services API (/api/reverse-proxies/*) covering services, custom domains, and proxy clusters.
New module:
Extended modules:
Infrastructure:
Roles:
Testing:
Documentation:
Tested against live preprod environment.
Co-authored with Claude Opus 4.6
Summary by CodeRabbit
New Features
Documentation
Tests