From 27595eee61ddf18e5b547109956ded1ecba2a573 Mon Sep 17 00:00:00 2001 From: Jack Carter <128555021+SunsetDrifter@users.noreply.github.com> Date: Fri, 17 Apr 2026 16:59:52 +0200 Subject: [PATCH] fix: raise on unresolved group/peer/posture-check references instead of silent drop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The configure role's name resolver previously used dict.get(name, name) fallbacks, so any unresolvable reference in YAML config was silently passed through to the API as if it were a valid ID. On the server side, the reference was discarded — producing half-applied policies whose rules ended up with sources: null (or destinations/auto_groups wiped). Common failure mode: a typo in a group name ships a broken firewall rule that reports changed=true but has no source constraint. This change makes the resolver strict: - Unknown name that is ALSO not an existing ID -> AnsibleFilterError with a message naming the resource, field, and unresolved value. - Known name -> resolves to ID (unchanged). - Value that matches an existing ID -> passes through (preserves backward compatibility for YAML configs that use raw IDs). Applies to groups in policy sources/destinations, setup_key auto_groups, policy source_posture_checks, peer refs in policy source_resource/ destination_resource, and network router.peer / resource.groups. Also: the /api/networks/{id}/routers endpoint returns router.peer as the peer's HOSTNAME rather than its canonical name. The peer_ids map (configure role) and peer_id_map (export role) now include hostname aliases so round-trips stay idempotent. Files: - plugins/filter/netbird_resolve.py: strict _resolve_names, _resolve_resource_ref, new _resolve_peer_id helper - roles/configure/tasks/main.yml: peer_ids merges hostname->id aliases with name->id - roles/export/tasks/main.yml: peer_id_map merges hostname->name aliases with id->name Co-Authored-By: Claude Opus 4.7 (1M context) --- plugins/filter/netbird_resolve.py | 121 +++++++++++++++++++++++++----- roles/configure/tasks/main.yml | 10 ++- roles/export/tasks/main.yml | 9 ++- 3 files changed, 118 insertions(+), 22 deletions(-) diff --git a/plugins/filter/netbird_resolve.py b/plugins/filter/netbird_resolve.py index 53ecb16..7ea81ca 100644 --- a/plugins/filter/netbird_resolve.py +++ b/plugins/filter/netbird_resolve.py @@ -7,63 +7,128 @@ from __future__ import absolute_import, division, print_function __metaclass__ = type +from ansible.errors import AnsibleFilterError -def _resolve_names(names, id_map): - """Resolve a list of names using an ID map, falling back to original.""" + +def _resolve_names(names, id_map, kind='group', context=''): + """Resolve a list of names using an ID map. + + A value passes through unchanged if it is already a known API ID (i.e. + appears as a value in id_map), which keeps YAML configs that reference + groups/checks by raw ID working. An unknown value (neither a known name + nor a known ID) raises AnsibleFilterError so that silent typos don't + produce half-applied resources. + """ if not names: return [] - return [id_map.get(name, name) for name in names] + known_ids = set(id_map.values()) + resolved = [] + for name in names: + if name in id_map: + resolved.append(id_map[name]) + elif name in known_ids: + resolved.append(name) + else: + raise AnsibleFilterError( + "Unknown %s '%s'%s. It is neither a known %s name nor an " + "existing %s ID. Check for typos or a missing %s definition." + % (kind, name, (" in " + context) if context else '', + kind, kind, kind) + ) + return resolved def _resolve_setup_key(sk, group_ids): """Resolve a single setup key's auto_groups.""" result = dict(sk) if 'auto_groups' in sk: - result['auto_groups'] = _resolve_names(sk['auto_groups'], group_ids) + result['auto_groups'] = _resolve_names( + sk['auto_groups'], group_ids, + kind='group', + context="setup key '%s' auto_groups" % sk.get('name', ''), + ) return result -def _resolve_resource_ref(resource, peer_ids): +def _resolve_resource_ref(resource, peer_ids, context=''): """Resolve {name, type: peer} to {id, type: peer} using peer_ids map. Non-peer resources (host/domain/subnet) pass through unchanged since their IDs in exported YAML are already concrete API IDs. + + Raises AnsibleFilterError if a peer name cannot be resolved to an ID. """ if not isinstance(resource, dict): return resource if resource.get('type') == 'peer' and 'name' in resource and 'id' not in resource: name = resource['name'] - return {'id': peer_ids.get(name, name), 'type': 'peer'} + known_ids = set(peer_ids.values()) + if name in peer_ids: + return {'id': peer_ids[name], 'type': 'peer'} + if name in known_ids: + return {'id': name, 'type': 'peer'} + raise AnsibleFilterError( + "Unknown peer '%s'%s. It is neither a known peer name nor an " + "existing peer ID." % (name, (" in " + context) if context else '') + ) return resource +def _resolve_peer_id(peer_ref, peer_ids, context=''): + """Resolve a single peer name or ID to an ID. Raises on unknown.""" + if peer_ref is None: + return peer_ref + known_ids = set(peer_ids.values()) + if peer_ref in peer_ids: + return peer_ids[peer_ref] + if peer_ref in known_ids: + return peer_ref + raise AnsibleFilterError( + "Unknown peer '%s'%s. It is neither a known peer name nor an " + "existing peer ID." % (peer_ref, (" in " + context) if context else '') + ) + + def _resolve_policy(policy, group_ids, posture_check_ids, peer_ids=None): """Resolve a single policy's group, posture check, and peer references.""" peer_ids = peer_ids or {} result = dict(policy) + policy_name = policy.get('name', '') if 'source_posture_checks' in policy: result['source_posture_checks'] = _resolve_names( - policy.get('source_posture_checks', []), posture_check_ids + policy.get('source_posture_checks', []), + posture_check_ids, + kind='posture_check', + context="policy '%s' source_posture_checks" % policy_name, ) if 'rules' in policy: resolved_rules = [] for rule in policy.get('rules', []): + rule_name = rule.get('name', '') resolved_rule = dict(rule) resolved_rule['sources'] = _resolve_names( - rule.get('sources', []), group_ids + rule.get('sources', []), + group_ids, + kind='group', + context="policy '%s' rule '%s' sources" % (policy_name, rule_name), ) resolved_rule['destinations'] = _resolve_names( - rule.get('destinations', []), group_ids + rule.get('destinations', []), + group_ids, + kind='group', + context="policy '%s' rule '%s' destinations" % (policy_name, rule_name), ) if rule.get('source_resource') is not None: resolved_rule['source_resource'] = _resolve_resource_ref( - rule['source_resource'], peer_ids + rule['source_resource'], peer_ids, + context="policy '%s' rule '%s' source_resource" % (policy_name, rule_name), ) if rule.get('destination_resource') is not None: resolved_rule['destination_resource'] = _resolve_resource_ref( - rule['destination_resource'], peer_ids + rule['destination_resource'], peer_ids, + context="policy '%s' rule '%s' destination_resource" % (policy_name, rule_name), ) resolved_rules.append(resolved_rule) result['rules'] = resolved_rules @@ -74,14 +139,19 @@ def _resolve_policy(policy, group_ids, posture_check_ids, peer_ids=None): def _resolve_network(network, group_ids, peer_ids): """Resolve a single network's group, peer, and peer_group references.""" result = dict(network) + network_name = network.get('name', '') if 'resources' in network: resolved_resources = [] for resource in network.get('resources', []): + resource_name = resource.get('name', resource.get('address', '')) resolved = dict(resource) if 'groups' in resource: resolved['groups'] = _resolve_names( - resource.get('groups', []), group_ids + resource.get('groups', []), + group_ids, + kind='group', + context="network '%s' resource '%s' groups" % (network_name, resource_name), ) resolved_resources.append(resolved) result['resources'] = resolved_resources @@ -90,11 +160,17 @@ def _resolve_network(network, group_ids, peer_ids): resolved_routers = [] for router in network.get('routers', []): resolved = dict(router) - if 'peer' in router: - resolved['peer'] = peer_ids.get(router['peer'], router['peer']) + if 'peer' in router and router['peer'] is not None: + resolved['peer'] = _resolve_peer_id( + router['peer'], peer_ids, + context="network '%s' router peer" % network_name, + ) if 'peer_groups' in router: resolved['peer_groups'] = _resolve_names( - router.get('peer_groups', []), group_ids + router.get('peer_groups', []), + group_ids, + kind='group', + context="network '%s' router peer_groups" % network_name, ) resolved_routers.append(resolved) result['routers'] = resolved_routers @@ -111,7 +187,12 @@ def netbird_resolve_ids(resource_list, resource_type, **kwargs): **kwargs: group_ids, peer_ids, posture_check_ids Returns: - list of resource dicts with names replaced by IDs + list of resource dicts with names replaced by IDs. + + Raises: + AnsibleFilterError: if any referenced group/posture-check/peer name + cannot be resolved to an ID. This prevents silent half-applies + where typos produce policies with dropped references. """ if not isinstance(resource_list, list): return [] @@ -138,17 +219,19 @@ def netbird_resolve_ids(resource_list, resource_type, **kwargs): return result -def netbird_resolve_names(name_list, id_map): +def netbird_resolve_names(name_list, id_map, kind='group', context=''): """Resolve a simple list of names to IDs. Used for inline resolution (e.g., DNS zone distribution_groups). - Falls back to original name if not found in map. + + Raises AnsibleFilterError if a name is neither a known key nor a known + value (ID) in the map. """ if not isinstance(name_list, list): return [] if not isinstance(id_map, dict): return name_list - return _resolve_names(name_list, id_map) + return _resolve_names(name_list, id_map, kind=kind, context=context) class FilterModule(object): diff --git a/roles/configure/tasks/main.yml b/roles/configure/tasks/main.yml index 66659d9..bfaff72 100644 --- a/roles/configure/tasks/main.yml +++ b/roles/configure/tasks/main.yml @@ -209,9 +209,15 @@ resource: peers register: api_peers - - name: Build peer name→ID map + - name: Build peer name→ID map (hostname aliases included) + # The /api/networks/{id}/routers endpoint returns router.peer as the + # peer's HOSTNAME (not name). To let exported network configs resolve + # cleanly, include hostname→id pairs as aliases for name→id. + # The name map is combined last so peer.name takes precedence on collision. ansible.builtin.set_fact: - peer_ids: "{{ dict(api_peers.data | map(attribute='name') | zip(api_peers.data | map(attribute='id'))) }}" + peer_ids: >- + {{ dict(api_peers.data | map(attribute='hostname') | zip(api_peers.data | map(attribute='id'))) + | combine(dict(api_peers.data | map(attribute='name') | zip(api_peers.data | map(attribute='id')))) }} - name: Display ID maps ansible.builtin.debug: diff --git a/roles/export/tasks/main.yml b/roles/export/tasks/main.yml index 3914dbc..ad4e63a 100644 --- a/roles/export/tasks/main.yml +++ b/roles/export/tasks/main.yml @@ -175,7 +175,14 @@ ansible.builtin.set_fact: group_id_map: "{{ dict(groups_data.data | map(attribute='id') | zip(groups_data.data | map(attribute='name'))) }}" posture_check_id_map: "{{ dict(posture_checks_data.data | map(attribute='id') | zip(posture_checks_data.data | map(attribute='name'))) }}" - peer_id_map: "{{ dict(peers_data.data | map(attribute='id') | zip(peers_data.data | map(attribute='name'))) }}" + # peer_id_map: lookup both peer.id AND peer.hostname -> peer.name. + # The networks router API returns router.peer as hostname in some + # cases, so we need to resolve either form to the canonical peer name + # when exporting. peer.id map is combined last so an ID always wins + # on the rare chance a hostname collides with an ID. + peer_id_map: >- + {{ dict(peers_data.data | map(attribute='hostname') | zip(peers_data.data | map(attribute='name'))) + | combine(dict(peers_data.data | map(attribute='id') | zip(peers_data.data | map(attribute='name')))) }} # ===================================================================== # Write clean config files (ready to use with configure role)