From 19f47aa70557b4c41dc3f7ff5d5e4cf75a4f68cf Mon Sep 17 00:00:00 2001 From: haseeb Date: Thu, 17 Sep 2026 18:56:49 +0530 Subject: [PATCH] create subnet pools and their address scopes --- ansible/neutron-post-deploy.yaml | 1 - ansible/roles/custom_fields/tasks/main.yml | 81 --- .../application-openstack-sync-operator.yaml | 7 + ...tack.rackspace.net_neutronsubnetpools.yaml | 288 +++++++--- .../openstack-sync-operator/values.yaml | 3 + .../examples/subnet-pool-v1alpha1.yaml | 51 +- .../openstack_sync/hooks/subnet_pools.py | 66 ++- .../plugins/neutron/subnet_pools/__init__.py | 1 + .../plugins/neutron/subnet_pools/config.py | 5 + .../plugins/neutron/subnet_pools/markers.py | 23 + .../plugins/neutron/subnet_pools/nautobot.py | 356 +++++++++++++ .../plugins/neutron/subnet_pools/prune.py | 57 ++ .../plugins/neutron/subnet_pools/reconcile.py | 493 ++++++++++++++++++ python/openstack-sync/pyproject.toml | 1 + .../tests/test_subnet_pools_hook.py | 484 ++++++++++++++++- .../tests/test_subnet_pools_nautobot.py | 372 +++++++++++++ .../tests/test_subnet_pools_prune.py | 63 +++ .../tests/test_subnet_pools_reconcile.py | 408 +++++++++++++++ python/openstack-sync/uv.lock | 16 + 19 files changed, 2570 insertions(+), 206 deletions(-) create mode 100644 python/openstack-sync/openstack_sync/plugins/neutron/subnet_pools/__init__.py create mode 100644 python/openstack-sync/openstack_sync/plugins/neutron/subnet_pools/config.py create mode 100644 python/openstack-sync/openstack_sync/plugins/neutron/subnet_pools/markers.py create mode 100644 python/openstack-sync/openstack_sync/plugins/neutron/subnet_pools/nautobot.py create mode 100644 python/openstack-sync/openstack_sync/plugins/neutron/subnet_pools/prune.py create mode 100644 python/openstack-sync/openstack_sync/plugins/neutron/subnet_pools/reconcile.py create mode 100644 python/openstack-sync/tests/test_subnet_pools_nautobot.py create mode 100644 python/openstack-sync/tests/test_subnet_pools_prune.py create mode 100644 python/openstack-sync/tests/test_subnet_pools_reconcile.py diff --git a/ansible/neutron-post-deploy.yaml b/ansible/neutron-post-deploy.yaml index d6ebdeac0..19afbac43 100644 --- a/ansible/neutron-post-deploy.yaml +++ b/ansible/neutron-post-deploy.yaml @@ -25,5 +25,4 @@ roles: - role: neutron_segment_range - - role: openstack_subnet_pools - role: openstack_network diff --git a/ansible/roles/custom_fields/tasks/main.yml b/ansible/roles/custom_fields/tasks/main.yml index 476edcc61..b3afc7d38 100644 --- a/ansible/roles/custom_fields/tasks/main.yml +++ b/ansible/roles/custom_fields/tasks/main.yml @@ -85,87 +85,6 @@ - dcim.interface filter_logic: exact -- name: Create Custom Field Subnet Pool Name - networktocode.nautobot.custom_field: - state: present - url: "{{ nautobot_url }}" - token: "{{ nautobot_token }}" - description: Name of the OpenStack subnet pool to create from this prefix - label: Subnet Pool Name - key: subnet_pool_name - type: text - required: false - weight: 100 - content_types: - - ipam.prefix - filter_logic: exact - -- name: Create Custom Field Address Scope - networktocode.nautobot.custom_field: - state: present - url: "{{ nautobot_url }}" - token: "{{ nautobot_token }}" - description: OpenStack address scope to associate with this subnet pool prefix - label: Address Scope - key: address_scope - type: text - required: false - weight: 100 - content_types: - - ipam.prefix - filter_logic: exact - -- name: Create Custom Field Subnet Pool Default Prefixlen - networktocode.nautobot.custom_field: - state: present - url: "{{ nautobot_url }}" - token: "{{ nautobot_token }}" - description: Default prefix length for subnets created from this pool - label: Default Prefixlen - key: subnet_pool_default_prefixlen - type: integer - required: false - weight: 100 - content_types: - - ipam.prefix - validation_minimum: 1 - validation_maximum: 32 - filter_logic: exact - -- name: Create Custom Field Subnet Pool Min Prefixlen - networktocode.nautobot.custom_field: - state: present - url: "{{ nautobot_url }}" - token: "{{ nautobot_token }}" - description: Minimum prefix length for subnets created from this pool - label: Min Prefixlen - key: subnet_pool_min_prefixlen - type: integer - required: false - weight: 100 - content_types: - - ipam.prefix - validation_minimum: 1 - validation_maximum: 32 - filter_logic: exact - -- name: Create Custom Field Subnet Pool Max Prefixlen - networktocode.nautobot.custom_field: - state: present - url: "{{ nautobot_url }}" - token: "{{ nautobot_token }}" - description: Maximum prefix length for subnets created from this pool - label: Max Prefixlen - key: subnet_pool_max_prefixlen - type: integer - required: false - weight: 100 - content_types: - - ipam.prefix - validation_minimum: 1 - validation_maximum: 32 - filter_logic: exact - - name: Create Custom Field Subnet Pool VNI tag networktocode.nautobot.custom_field: state: present diff --git a/charts/argocd-understack/templates/application-openstack-sync-operator.yaml b/charts/argocd-understack/templates/application-openstack-sync-operator.yaml index e7413a354..5ce3073a3 100644 --- a/charts/argocd-understack/templates/application-openstack-sync-operator.yaml +++ b/charts/argocd-understack/templates/application-openstack-sync-operator.yaml @@ -29,6 +29,13 @@ spec: valueFiles: - $understack/components/openstack-sync-operator/values.yaml - $deploy/{{ include "understack.deploy_path" $ }}/openstack-sync-operator/values.yaml + {{- with index $.Values.appLabels "understack.rackspace.com/site" }} + # Site name (Nautobot leaf Site) the operator runs for. The subnet-pool + # hook uses it as the default require.location. + parameters: + - name: env.UNDERSTACK_SITE + value: {{ . | quote }} + {{- end }} - ref: deploy repoURL: {{ include "understack.deploy_url" $ }} targetRevision: {{ include "understack.deploy_ref" $ }} diff --git a/components/openstack-sync-operator/crds/neutron.understack.rackspace.net_neutronsubnetpools.yaml b/components/openstack-sync-operator/crds/neutron.understack.rackspace.net_neutronsubnetpools.yaml index ead447966..6bf6fd078 100644 --- a/components/openstack-sync-operator/crds/neutron.understack.rackspace.net_neutronsubnetpools.yaml +++ b/components/openstack-sync-operator/crds/neutron.understack.rackspace.net_neutronsubnetpools.yaml @@ -21,9 +21,6 @@ spec: - name: Pool type: string jsonPath: .spec.name - - name: IPVersion - type: integer - jsonPath: .spec.ip_version - name: AddressScope type: string jsonPath: .spec.address_scope.name @@ -33,16 +30,45 @@ spec: - name: SyncStatus type: string jsonPath: .status.syncStatus + # A converged pool stays SyncStatus=Synced; manual-action notes show here. + - name: Message + type: string + jsonPath: .status.message - name: Age type: date jsonPath: .metadata.creationTimestamp schema: openAPIV3Schema: description: >- - NeutronSubnetPool defines one Neutron subnet pool and links it to a - Neutron address scope. Neutron stores that link as address_scope_id on - the subnet pool; Understack L3/SVI validation relies on subnets - inheriting that address scope through their subnet pool. + NeutronSubnetPool declares one OpenStack Neutron subnet pool. The top + level of spec is the complete, self-describing OpenStack subnet-pool + contract, mirroring the Neutron subnet pool and address scope + resources: name, address_scope, minimum/default/maximum_prefix_length, + is_default, shared, description, project_id, and tags. These fields + are authored here and are the single source of truth for the pool's + policy; the operator does not read them from anywhere else. + + + spec.nautobot is the reference contract: it says where Nautobot is and + which prefixes supply this pool's CIDRs, plus optional guardrails each + prefix must satisfy. Nautobot owns only the CIDRs (IP allocation is + its job); the CR references them by prefix UUID rather than copying + them, so blocks can be added or renumbered in Nautobot without + editing the CR. IP version is inferred from the resolved CIDRs and + must be consistent across them. + + + The contract must be complete: a name, an address scope, all three + prefix-length bounds (minimum <= default <= maximum, within the IP + family), and at least one CIDR prefix resolved from Nautobot. The + operator requires the bounds explicitly rather than falling back to + Neutron's permissive defaults. A named address scope is created if + absent and adopted if present, then attached to the pool; a scope + referenced by id must already exist. The operator never deletes an + address scope, because a scope outlives any one pool and the L3/SVI + validation of every subnet under it depends on the scope surviving. + Prefixes can only be added to an existing pool, never removed, because + Neutron forbids shrinking a subnet pool. type: object required: - spec @@ -56,10 +82,13 @@ spec: spec: type: object required: + - cloudCredentialsRef - name - - prefixes - address_scope - - cloudCredentialsRef + - minimum_prefix_length + - default_prefix_length + - maximum_prefix_length + - nautobot properties: cloudCredentialsRef: description: >- @@ -80,31 +109,45 @@ spec: minLength: 1 maxLength: 253 cloudName: - description: >- - Name of the cloud entry within the clouds.yaml to - authenticate as. + description: Name of the cloud entry within clouds.yaml. type: string minLength: 1 maxLength: 256 name: - description: Neutron subnet pool name. - type: string - minLength: 1 - maxLength: 255 - pattern: ^[A-Za-z0-9._-]+$ - project_id: - description: >- - Optional OpenStack project ID to create the subnet pool in and - to use when resolving an address scope by name. + description: Neutron subnet pool name. Required. type: string minLength: 1 maxLength: 255 pattern: ^[A-Za-z0-9._:-]+$ + default_prefix_length: + description: >- + Default subnet prefix length allocated from this pool. + Required; must satisfy minimum <= default <= maximum. + type: integer + minimum: 0 + maximum: 128 + minimum_prefix_length: + description: >- + Smallest allowed subnet prefix length allocated from this + pool. Required. + type: integer + minimum: 0 + maximum: 128 + maximum_prefix_length: + description: >- + Largest allowed subnet prefix length allocated from this pool. + Required. + type: integer + minimum: 0 + maximum: 128 address_scope: description: >- - Address scope to attach to the Neutron subnet pool. Use id for - an exact lookup, or name with optional project_id when the - address scope name is unique for the selected cloud. + Neutron address scope for this subnet pool. Required. When + referenced by name, an existing scope is adopted and a missing + one is created (shared, IP version inferred from the resolved + prefixes), then linked to the pool; set create=false to require + a pre-existing scope. When referenced by id the scope must + already exist and is never created. type: object oneOf: - required: @@ -113,75 +156,172 @@ spec: - name properties: id: - description: Neutron address scope ID. + description: >- + Neutron address scope ID (UUID). A scope referenced by id + must already exist; it is never created. type: string - minLength: 1 + minLength: 36 maxLength: 36 - pattern: ^[A-Fa-f0-9-]+$ + pattern: >- + ^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$ name: - description: Neutron address scope name. + description: >- + Neutron address scope name. Created if absent unless + create is false. type: string minLength: 1 maxLength: 255 + create: + description: >- + Create the scope when referenced by name and absent. + Defaults true; set false to require a pre-existing scope. + Ignored when referenced by id. + type: boolean + default: true + shared: + description: >- + Whether a newly created scope is shared. Defaults true. + Applies only on create; an adopted or id-referenced scope + keeps its value. + type: boolean + default: true project_id: description: >- - Optional project ID used only when resolving this address - scope by name. Defaults to spec.project_id when omitted. + Optional project ID used when resolving this address scope + by name, and set as the owner when the operator creates + it. type: string minLength: 1 maxLength: 255 pattern: ^[A-Za-z0-9._:-]+$ - prefixes: - description: CIDR prefixes delegated by the Neutron subnet pool. - type: array - minItems: 1 - items: - type: string - minLength: 1 - maxLength: 128 - pattern: ^(([0-9]{1,3}\.){3}[0-9]{1,3}/[0-9]{1,2}|[0-9A-Fa-f:.]+/[0-9]{1,3})$ - ip_version: + project_id: description: >- - IP version for the subnet pool. Neutron derives this from - prefixes; the operator uses this optional value to validate - prefixes and address-scope family before creating the pool. - type: integer - enum: - - 4 - - 6 - default_prefixlen: - description: Default prefix length allocated from this pool. - type: integer - minimum: 0 - maximum: 128 - min_prefixlen: - description: Smallest prefix length that may be allocated. - type: integer - minimum: 0 - maximum: 128 - max_prefixlen: - description: Largest prefix length that may be allocated. - type: integer - minimum: 0 - maximum: 128 - default_quota: - description: Per-project subnet quota for this pool. - type: integer - minimum: 0 + OpenStack project ID that owns the subnet pool; also used for + lookups. + type: string + minLength: 1 + maxLength: 255 + pattern: ^[A-Za-z0-9._:-]+$ is_default: - description: Whether this is the default pool for its IP version. + description: >- + Whether this is the default subnet pool for its IP version. + Admin-only in Neutron, and Neutron permits at most one default + pool per IP family. When set, the operator checks for an + existing default of the same family before writing and fails + the CR with a message naming the conflicting pool rather than + surfacing Neutron's generic error. type: boolean default: false shared: - description: Whether the Neutron subnet pool is shared. + description: >- + Whether this subnet pool is shared across projects. Neutron + does not allow changing this on an existing pool, so it has no + default: set it explicitly, and the operator will refuse to + reconcile a pool whose current value differs rather than + silently diverge. Leave it unset only to adopt a pool without + asserting its sharing. type: boolean - default: false + nautobot: + description: >- + The Nautobot reference contract: where Nautobot is and which + prefixes supply this pool's CIDRs, plus optional guardrails + each prefix must satisfy. Nautobot owns only the CIDRs; the + pool's name, address scope, and prefix lengths are declared at + the top of spec, not read from Nautobot. + type: object + required: + - url + - tokenSecretRef + - prefix_refs + properties: + url: + description: Base Nautobot URL, for example https://nautobot.example.com. + type: string + minLength: 1 + maxLength: 2048 + api_version: + description: Nautobot REST API version sent in the Accept header. + type: string + minLength: 1 + maxLength: 32 + default: "2.0" + tokenSecretRef: + description: Secret key containing a Nautobot API token. + type: object + required: + - secretName + - key + properties: + secretName: + description: Name of a Secret in the same namespace as this resource. + type: string + minLength: 1 + maxLength: 253 + key: + description: Secret data key containing the token. + type: string + minLength: 1 + maxLength: 253 + prefix_refs: + description: >- + Nautobot prefix IDs that contribute CIDRs to this subnet + pool. Kubernetes stores references, not copied CIDRs. + type: array + minItems: 1 + items: + type: object + required: + - id + properties: + id: + description: Nautobot prefix UUID. + type: string + minLength: 36 + maxLength: 36 + pattern: >- + ^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$ + require: + description: >- + Optional guardrails each referenced Nautobot prefix must + satisfy before it is accepted for this subnet pool. + type: object + properties: + namespace: + description: Required Nautobot IPAM namespace, for example Rackspace. + type: string + minLength: 1 + maxLength: 255 + location: + description: >- + Nautobot location the prefix must be under. + type: string + minLength: 1 + maxLength: 255 + status: + description: Required Nautobot prefix status. + type: string + minLength: 1 + maxLength: 255 + type: + description: Required Nautobot prefix type. + type: string + minLength: 1 + maxLength: 255 + tags: + description: Required Nautobot tags. + type: array + items: + type: string + minLength: 1 + maxLength: 255 description: description: Description stored on the Neutron subnet pool. type: string maxLength: 1024 tags: - description: Neutron tags to apply to the subnet pool. + description: >- + Neutron tags to apply to this subnet pool, in addition to the + operator's ownership marker. type: array items: type: string @@ -199,11 +339,11 @@ spec: - Failed - Unknown lastSyncTime: - description: LastSyncTime is the last time the operator attempted to sync the subnet pool. + description: LastSyncTime is the last time the operator attempted this sync. type: string format: date-time observedGeneration: - description: ObservedGeneration is the metadata generation last processed by the operator. + description: ObservedGeneration is the metadata generation last processed. type: integer format: int64 message: @@ -239,9 +379,7 @@ spec: - "False" - Unknown reason: - description: >- - CamelCase programmatic identifier for the reason of the - last transition. + description: CamelCase programmatic identifier for the last transition. type: string minLength: 1 maxLength: 1024 diff --git a/components/openstack-sync-operator/values.yaml b/components/openstack-sync-operator/values.yaml index 7d5ee4ff0..ad0c7f61d 100644 --- a/components/openstack-sync-operator/values.yaml +++ b/components/openstack-sync-operator/values.yaml @@ -65,6 +65,9 @@ pluginData: envPrefix: NEUTRON_SUBNET_POOL env: SYNC_CRONTAB: "0 * * * *" + READY_RETRIES: 30 + READY_DELAY: 10 + PRUNE: false ironicRunbooks: hook: diff --git a/components/openstack-sync-plugins/neutron-subnet-pools/examples/subnet-pool-v1alpha1.yaml b/components/openstack-sync-plugins/neutron-subnet-pools/examples/subnet-pool-v1alpha1.yaml index ab72b9c5a..829bdb4a3 100644 --- a/components/openstack-sync-plugins/neutron-subnet-pools/examples/subnet-pool-v1alpha1.yaml +++ b/components/openstack-sync-plugins/neutron-subnet-pools/examples/subnet-pool-v1alpha1.yaml @@ -7,25 +7,46 @@ metadata: app.kubernetes.io/name: openstack-sync-plugins app.kubernetes.io/component: neutron-subnet-pools app.kubernetes.io/part-of: openstack-sync +# The top level of spec is the OpenStack subnet-pool contract and the single +# source of truth for this pool's policy: name, address_scope, the three +# prefix-length bounds, and sharing are declared here and are required. +# spec.nautobot is the reference contract: endpoint, token, the prefixes that +# supply the pool's CIDRs, and optional guardrails. Nautobot owns only the +# CIDRs; the CR references them by prefix UUID rather than copying them. spec: cloudCredentialsRef: secretName: infrasetup cloudName: understack - name: SAMPLE-PUBLIC-IP-POOL - description: SITE-Dev public network allocation + # OpenStack subnet-pool contract (required). + name: PUBLIC-IP-POOL address_scope: - name: sample-publicnet-ip4 - prefixes: - - 100.101.102.128/25 - - 10.4.88.0/24 - ip_version: 4 - default_prefixlen: 28 - min_prefixlen: 27 - max_prefixlen: 30 - default_quota: 0 - is_default: false + # Created if absent, adopted if present. create: false requires it to exist. + name: publicnet-ip4 + create: true + shared: true + minimum_prefix_length: 27 + default_prefix_length: 28 + maximum_prefix_length: 30 shared: true + is_default: false tags: - - openstack-subnet-pool - - public - - site-dev + - managed-by-openstack-sync + nautobot: + url: https://nautobot.example.com + api_version: "2.0" + tokenSecretRef: + secretName: nautobot + key: token + prefix_refs: + - id: 2bc3ecab-b6dc-46cd-9bd4-1c0ea8a07f87 + - id: 53c8ee3b-09b9-41ab-a413-b1a1f5ecec6a + require: + namespace: Rackspace + # Optional; defaults to the operator's site (UNDERSTACK_SITE). + location: site-dev + status: Active + type: pool + # Guards against a fat-fingered prefix_refs id: the referenced prefix + # must carry the IPAM marker that earmarks a block for OpenStack pools. + tags: + - openstack-subnet-pool diff --git a/python/openstack-sync/openstack_sync/hooks/subnet_pools.py b/python/openstack-sync/openstack_sync/hooks/subnet_pools.py index a92104f0b..3c5525b4e 100644 --- a/python/openstack-sync/openstack_sync/hooks/subnet_pools.py +++ b/python/openstack-sync/openstack_sync/hooks/subnet_pools.py @@ -1,41 +1,73 @@ #!/usr/bin/env python3 -"""Shell-operator hook skeleton for Neutron subnet pool CRs.""" +"""Shell-operator hook for Neutron subnet pool reconciliation.""" from __future__ import annotations -import logging import sys from typing import Any +from openstack_sync.hooks.framework import CleanupPolicy from openstack_sync.hooks.framework import HookConfig +from openstack_sync.hooks.framework import PruneRequest from openstack_sync.hooks.framework import SyncPlugin from openstack_sync.hooks.framework import build_crd_hook_config from openstack_sync.hooks.framework import hook_enabled from openstack_sync.hooks.framework import hook_inputs from openstack_sync.hooks.framework import run_hook from openstack_sync.hooks.framework import run_sync - -LOG = logging.getLogger(__name__) - -ENV_PREFIX = "NEUTRON_SUBNET_POOL" -BINDING_NAME = "neutron-subnet-pools" +from openstack_sync.plugins.common import wait_for_openstack_network +from openstack_sync.plugins.neutron.subnet_pools import prune as prune_module +from openstack_sync.plugins.neutron.subnet_pools import reconcile as reconcile_module +from openstack_sync.plugins.neutron.subnet_pools.config import BINDING_NAME +from openstack_sync.plugins.neutron.subnet_pools.config import ENV_PREFIX +from openstack_sync.utils import pod_namespace class SubnetPoolPlugin(SyncPlugin): - """Read NeutronSubnetPool CRs without reconciling OpenStack state yet.""" + """Sync NeutronSubnetPool CRs into Neutron subnet pools. + + The CR references Nautobot prefixes rather than raw CIDRs, so reconcile + resolves those references against Nautobot before converging Neutron. The + per-credential-group cache memoises the Nautobot client across every CR in + one group. + """ noun = "subnet pool" def wait_for_api(self, conn: Any) -> None: - # Contract-only skeleton: credentials are resolved by the framework and - # the CRs are readable before this runs. Neutron calls will be added - # when resource creation semantics land. - _ = conn - - def reconcile(self, conn: Any, spec: dict[str, Any], cache: Any) -> list[str]: - _ = (conn, cache) - LOG.info("Observed NeutronSubnetPool CR %s", spec.get("name", "")) - return [] + wait_for_openstack_network( + conn, + retries=self.config.ready_retries, + delay=self.config.ready_delay, + ) + + def reconcile( + self, conn: Any, spec: dict[str, Any], cache: dict[str, Any] + ) -> list[str]: + namespace = self.config.namespace or pod_namespace() + return reconcile_module.sync_subnet_pool(conn, spec, namespace, cache) + + def prune_resources(self, conn: Any, request: PruneRequest) -> None: + # The pool name is declared on the CR (spec.name is required), so the + # desired names are read straight from the surviving specs -- no Nautobot + # call. That keeps prune independent of Nautobot reachability, so an + # outage can never make a live CR's pool look undesired. + desired_names = reconcile_module.resolve_desired_names(request.desired_specs) + prune_module.prune_removed_subnet_pools( + conn, + desired_names, + authoritative_empty=request.authoritative_empty, + ) + + def cleanup_policy(self) -> CleanupPolicy: + # NONE, not BEST_EFFORT_PRUNE like router flavors: a subnet pool can + # have allocated subnets under it, so it must never be swept without the + # finalizer, and unlike an orphaned service profile there is no + # safe-every-cycle subset to reclaim. With prune off, deleting a CR + # leaves its Neutron pool in place for an operator to drain and remove. + if self.config.prune: + return CleanupPolicy.FINALIZED_PRUNE + return CleanupPolicy.NONE def main() -> int: diff --git a/python/openstack-sync/openstack_sync/plugins/neutron/subnet_pools/__init__.py b/python/openstack-sync/openstack_sync/plugins/neutron/subnet_pools/__init__.py new file mode 100644 index 000000000..4e4bd265c --- /dev/null +++ b/python/openstack-sync/openstack_sync/plugins/neutron/subnet_pools/__init__.py @@ -0,0 +1 @@ +"""Neutron subnet pool sync plugin.""" diff --git a/python/openstack-sync/openstack_sync/plugins/neutron/subnet_pools/config.py b/python/openstack-sync/openstack_sync/plugins/neutron/subnet_pools/config.py new file mode 100644 index 000000000..d2c609085 --- /dev/null +++ b/python/openstack-sync/openstack_sync/plugins/neutron/subnet_pools/config.py @@ -0,0 +1,5 @@ +"""Configuration constants for Neutron subnet pool sync.""" + +ENV_PREFIX = "NEUTRON_SUBNET_POOL" +BINDING_NAME = "neutron-subnet-pools" +OWNERSHIP_TAG = "understack-operator-neutron-subnet-pool" diff --git a/python/openstack-sync/openstack_sync/plugins/neutron/subnet_pools/markers.py b/python/openstack-sync/openstack_sync/plugins/neutron/subnet_pools/markers.py new file mode 100644 index 000000000..f4f359946 --- /dev/null +++ b/python/openstack-sync/openstack_sync/plugins/neutron/subnet_pools/markers.py @@ -0,0 +1,23 @@ +"""Ownership helpers for operator-managed Neutron subnet pools.""" + +from __future__ import annotations + +from typing import Any + +from openstack_sync.plugins.common import get_value +from openstack_sync.plugins.neutron.subnet_pools.config import OWNERSHIP_TAG + + +def subnet_pool_tags(pool: Any) -> list[str]: + return [str(tag) for tag in get_value(pool, "tags", default=[]) or []] + + +def is_managed_subnet_pool(pool: Any) -> bool: + return OWNERSHIP_TAG in subnet_pool_tags(pool) + + +def desired_tags(spec: dict[str, Any]) -> list[str]: + """Return the CR's ``spec.tags`` plus the ownership marker.""" + tags = {str(tag) for tag in spec.get("tags", [])} + tags.add(OWNERSHIP_TAG) + return sorted(tags) diff --git a/python/openstack-sync/openstack_sync/plugins/neutron/subnet_pools/nautobot.py b/python/openstack-sync/openstack_sync/plugins/neutron/subnet_pools/nautobot.py new file mode 100644 index 000000000..f1729b716 --- /dev/null +++ b/python/openstack-sync/openstack_sync/plugins/neutron/subnet_pools/nautobot.py @@ -0,0 +1,356 @@ +"""Resolve a NeutronSubnetPool CR's Nautobot prefix references into CIDRs. + +The CRD's top level is the OpenStack subnet-pool contract (name, address scope, +prefix lengths, ...) and is the single source of truth for the pool's policy. +``spec.nautobot`` is the reference contract: where Nautobot is, which prefixes +supply this pool's CIDRs, and optional guardrails each prefix must satisfy. + +Nautobot owns only the CIDRs -- IP allocation is its job -- so this module +loads each referenced prefix, checks the ``require`` guardrails, and hands the +reconcile step the resolved ``prefixes`` and ``ip_version``. It reads nothing +policy-shaped from Nautobot: the pool name, address scope, and prefix lengths +come from the CR spec, not from prefix ``custom_fields``. + +The reconcile step consumes the normalized spec this module returns, so it never +reads Nautobot itself. +""" + +from __future__ import annotations + +import logging +import os +from dataclasses import dataclass +from ipaddress import ip_network +from typing import Any + +import pynautobot + +from openstack_sync.plugins.common import ConfigError +from openstack_sync.utils import read_secret_key + +LOG = logging.getLogger(__name__) + +#: Cluster site (Nautobot leaf Site name) injected by the deployment. Used as +#: the default require.location when a CR does not set one. +SITE_ENV = "UNDERSTACK_SITE" + + +@dataclass(frozen=True) +class NautobotPrefix: + """Subset of Nautobot Prefix fields needed for subnet-pool planning.""" + + id: str | None + prefix: str + status: str | None + prefix_type: str | None + namespace: str | None + tags: tuple[str, ...] + + +def resolve_spec( + spec: dict[str, Any], cache: dict[str, Any], namespace: str +) -> dict[str, Any]: + """Return *spec* enriched with the CIDRs Nautobot is authoritative for. + + Loads every referenced prefix, checks the optional ``require`` guardrails, + then attaches the resolved ``prefixes`` and ``ip_version``. The pool's + policy (name, address scope, prefix lengths) is already on *spec* and is + left untouched. The returned dict is a shallow copy carrying Neutron-shaped + fields only, so the reconcile step never touches Nautobot. Raises + :exc:`ConfigError` when a referenced prefix is missing, fails the + guardrails, or the group mixes IP versions. + """ + _required_mapping(spec, "nautobot", "spec") + name = _required_string(spec, "name") + client = _nautobot_client(spec, cache, namespace) + prefix_refs = _nautobot_prefix_refs(spec) + requirements = _nautobot_requirements(spec) + prefixes = load_nautobot_prefixes( + client, + prefix_refs, + require_location=_require_location(requirements), + ) + _validate_prefixes(prefixes, requirements) + + resolved = dict(spec) + resolved["prefixes"] = [prefix.prefix for prefix in prefixes] + resolved["ip_version"] = _subnet_pool_ip_version(name, prefixes) + + LOG.info( + "Resolved subnet pool %s from %s Nautobot prefix(es): %s", + name, + len(prefixes), + ", ".join(prefix.prefix for prefix in prefixes), + ) + return resolved + + +# --------------------------------------------------------------------------- +# Nautobot client and prefix loading +# --------------------------------------------------------------------------- + + +def _nautobot_client( + spec: dict[str, Any], cache: dict[str, Any], namespace: str +) -> Any: + nautobot_spec = _required_mapping(spec, "nautobot", "spec") + token_ref = _required_mapping(nautobot_spec, "tokenSecretRef", "spec.nautobot") + url = _required_string(nautobot_spec, "url") + api_version = str(nautobot_spec.get("api_version") or "2.0") + secret_name = _required_string(token_ref, "secretName") + secret_key = _required_string(token_ref, "key") + + cache_key = ( + "nautobot_client", + url, + api_version, + secret_name, + secret_key, + namespace, + ) + if cache_key not in cache: + token = read_secret_key(secret_name, secret_key, namespace).strip() + cache[cache_key] = pynautobot.api( + url, + token=token, + api_version=api_version, + retries=3, + ) + return cache[cache_key] + + +def load_nautobot_prefixes( + client: Any, + prefix_refs: list[dict[str, str]], + *, + require_location: str | None = None, +) -> list[NautobotPrefix]: + """Load prefixes from Nautobot by id, filtered by *require_location*.""" + prefixes: list[NautobotPrefix] = [] + for ref in prefix_refs: + prefix_id = ref["id"] + query: dict[str, str] = {"id": prefix_id} + if require_location: + query["location"] = require_location + try: + record = client.ipam.prefixes.get(**query) + except pynautobot.RequestError as exc: + if require_location and _is_invalid_location_error(exc, require_location): + raise ConfigError( + f"spec.nautobot.require.location {require_location!r} is not a " + "valid Nautobot location; use the exact leaf Site name " + "(for example 'iad3-dev')" + ) from exc + raise ConfigError( + f"Nautobot prefix lookup failed for id {prefix_id}: {exc}" + ) from exc + + if record is None: + if require_location: + raise ConfigError( + f"Nautobot prefix {prefix_id} was not found under " + f"location {require_location!r}: it does not exist or is not " + "associated with that location (spec.nautobot.require.location)" + ) + raise ConfigError(f"Nautobot prefix {prefix_id} was not found") + prefixes.append(_prefix_from_record(record)) + return prefixes + + +def _is_invalid_location_error( + exc: pynautobot.RequestError, require_location: str +) -> bool: + """Return True when Nautobot rejected an unknown ``location`` (HTTP 400).""" + response = getattr(exc, "req", None) + if getattr(response, "status_code", None) != 400: + return False + error_text = str(getattr(exc, "error", "") or "") + return "location" in error_text or require_location in error_text + + +def _nautobot_prefix_refs(spec: dict[str, Any]) -> list[dict[str, str]]: + nautobot_spec = _required_mapping(spec, "nautobot", "spec") + raw_refs = nautobot_spec.get("prefix_refs") + if not isinstance(raw_refs, list) or not raw_refs: + raise ConfigError("spec.nautobot.prefix_refs must be a non-empty list") + + refs: list[dict[str, str]] = [] + for index, raw_ref in enumerate(raw_refs): + if not isinstance(raw_ref, dict): + raise ConfigError(f"spec.nautobot.prefix_refs[{index}] must be a mapping") + prefix_id = _field_text(raw_ref.get("id")) + if not prefix_id: + raise ConfigError(f"spec.nautobot.prefix_refs[{index}].id must be set") + refs.append({"id": prefix_id}) + return refs + + +def _nautobot_requirements(spec: dict[str, Any]) -> dict[str, Any]: + nautobot_spec = _required_mapping(spec, "nautobot", "spec") + requirements = nautobot_spec.get("require") or {} + if not isinstance(requirements, dict): + raise ConfigError("spec.nautobot.require must be a mapping") + return requirements + + +def _require_location(requirements: dict[str, Any]) -> str | None: + """Return require.location, defaulting to the operator's UNDERSTACK_SITE.""" + return _field_text(requirements.get("location")) or os.environ.get(SITE_ENV) or None + + +# --------------------------------------------------------------------------- +# Guardrail validation +# --------------------------------------------------------------------------- + + +def _validate_prefixes( + prefixes: list[NautobotPrefix], requirements: dict[str, Any] +) -> None: + for prefix in prefixes: + errors = _prefix_requirement_errors(prefix, requirements) + if errors: + prefix_id = prefix.id or prefix.prefix + raise ConfigError( + f"Nautobot prefix {prefix_id} does not satisfy " + f"spec.nautobot.require: {'; '.join(errors)}" + ) + + +def _prefix_requirement_errors( + prefix: NautobotPrefix, requirements: dict[str, Any] +) -> list[str]: + errors: list[str] = [] + expected_status = _field_text(requirements.get("status")) + if expected_status and prefix.status != expected_status: + errors.append(f"status is {prefix.status!r}, expected {expected_status!r}") + + expected_type = _field_text(requirements.get("type")) + if expected_type and prefix.prefix_type != expected_type: + errors.append(f"type is {prefix.prefix_type!r}, expected {expected_type!r}") + + expected_namespace = _field_text(requirements.get("namespace")) + if expected_namespace and prefix.namespace != expected_namespace: + errors.append( + f"namespace is {prefix.namespace!r}, expected {expected_namespace!r}" + ) + + # require.location is enforced via the ?location= query in + # load_nautobot_prefixes, not checked here. + + expected_tags = _requirement_tags(requirements) + missing_tags = [tag for tag in expected_tags if tag not in prefix.tags] + if missing_tags: + errors.append(f"missing tags {missing_tags!r}") + return errors + + +def _requirement_tags(requirements: dict[str, Any]) -> tuple[str, ...]: + raw_tags = requirements.get("tags") + if raw_tags is None: + return () + if not isinstance(raw_tags, list): + raise ConfigError("spec.nautobot.require.tags must be a list") + return tuple(tag for tag in (_field_text(tag) for tag in raw_tags) if tag) + + +# --------------------------------------------------------------------------- +# Record extraction +# --------------------------------------------------------------------------- + + +def _prefix_from_record(record: Any) -> NautobotPrefix: + prefix = _field_text(record.prefix) + if not prefix: + raise ConfigError(f"Nautobot prefix record has no prefix value: {record!r}") + + return NautobotPrefix( + id=_field_text(record.id) or None, + prefix=prefix, + status=_field_text(record.status) or None, + prefix_type=_field_text(record.type) or None, + namespace=_field_text(record.namespace) or None, + tags=_field_texts(record.tags), + ) + + +def _prefix_ip_version(prefix: NautobotPrefix) -> int: + try: + return ip_network(prefix.prefix, strict=False).version + except ValueError as exc: + raise ConfigError( + f"Nautobot prefix {prefix.prefix!r} is not valid CIDR notation" + ) from exc + + +def _subnet_pool_ip_version(name: str, prefixes: list[NautobotPrefix]) -> int: + if not prefixes: + raise ConfigError("spec.nautobot.prefix_refs must resolve at least one prefix") + versions = {_prefix_ip_version(prefix) for prefix in prefixes} + if len(versions) != 1: + prefix_values = ", ".join(prefix.prefix for prefix in prefixes) + raise ConfigError( + f"Nautobot subnet pool {name!r} contains mixed IP versions: {prefix_values}" + ) + return versions.pop() + + +# --------------------------------------------------------------------------- +# Small helpers +# --------------------------------------------------------------------------- + + +def _required_mapping( + source: dict[str, Any], key: str, parent_path: str +) -> dict[str, Any]: + value = source.get(key) + if not isinstance(value, dict): + raise ConfigError(f"{parent_path}.{key} must be set") + return value + + +def _required_string(source: dict[str, Any], key: str) -> str: + value = _field_text(source.get(key)) + if not value: + raise ConfigError(f"{key} must be set") + return value + + +def _field_texts(value: Any) -> tuple[str, ...]: + if value is None: + return () + if isinstance(value, list | tuple): + return tuple(_field_text(item) for item in value if _field_text(item)) + return (_field_text(value),) if _field_text(value) else () + + +def _field_text(value: Any) -> str: + """Extract a comparable string from a Nautobot field. + + Nautobot related fields come back in several shapes and pynautobot wraps + them as ``Record`` objects, so a single accessor is not enough: + + - ``status``/``namespace``: carry a ``name`` ("Active", "Rackspace"). + - ``type``: a choice field with ``value``/``label`` ({"value": "pool", + "label": "Pool"}); we compare against the machine value ("pool"). + - ``tags``: Records whose value is only reachable via ``str()`` (``display``). + + Order matters: ``name`` first, then the choice ``value``, then ``str()`` + (which pynautobot defines as display/name/label). ``type`` is handled before + the ``str()`` fallback because ``str()`` would yield the human label "Pool" + rather than the value "pool" the guardrail expects. + """ + if value is None: + return "" + if isinstance(value, str): + return value.strip() + if isinstance(value, dict): + text = value.get("name") or value.get("value") or value.get("display") or "" + return str(text).strip() + # pynautobot Record (or similar): prefer name, then choice value, then str(). + name = getattr(value, "name", None) + if name: + return str(name).strip() + choice_value = getattr(value, "value", None) + if choice_value: + return str(choice_value).strip() + return str(value).strip() diff --git a/python/openstack-sync/openstack_sync/plugins/neutron/subnet_pools/prune.py b/python/openstack-sync/openstack_sync/plugins/neutron/subnet_pools/prune.py new file mode 100644 index 000000000..39e5612e4 --- /dev/null +++ b/python/openstack-sync/openstack_sync/plugins/neutron/subnet_pools/prune.py @@ -0,0 +1,57 @@ +"""Delete Neutron subnet pools whose CR was removed.""" + +from __future__ import annotations + +import logging +from typing import Any + +from openstack import exceptions as openstack_exceptions + +from openstack_sync.plugins.common import get_value +from openstack_sync.plugins.common import resource_id +from openstack_sync.plugins.neutron.subnet_pools.markers import is_managed_subnet_pool + +LOG = logging.getLogger(__name__) + + +def prune_removed_subnet_pools( + conn: Any, + desired_names: list[str], + *, + authoritative_empty: bool = False, +) -> None: + """Delete operator-owned subnet pools absent from the desired names. + + *desired_names* are the Neutron pool names the surviving CRs declare (see + ``reconcile.resolve_desired_names``), read straight from ``spec.name`` with + no Nautobot call, so prune stays independent of Nautobot reachability. + """ + if not desired_names and not authoritative_empty: + LOG.warning( + "No desired subnet pools found; skipping prune to avoid deleting " + "all managed subnet pools" + ) + return + + wanted = {str(name) for name in desired_names if name} + incomplete = [] + LOG.info("Pruning removed subnet pools") + for pool in list(conn.network.subnet_pools()): + name = get_value(pool, "name") + if not name or name in wanted: + continue + if not is_managed_subnet_pool(pool): + continue + + pool_id = resource_id(pool) + LOG.info("Deleting removed subnet pool %s (%s)", name, pool_id) + try: + conn.network.delete_subnet_pool(pool, ignore_missing=True) + except openstack_exceptions.NotFoundException: + LOG.info("Subnet pool %s (%s) is already absent", name, pool_id) + except openstack_exceptions.ConflictException: + LOG.info("Subnet pool %s is still in use; skipping delete", name) + incomplete.append(str(name)) + + if incomplete: + raise RuntimeError("subnet pools still present: " + ", ".join(incomplete)) diff --git a/python/openstack-sync/openstack_sync/plugins/neutron/subnet_pools/reconcile.py b/python/openstack-sync/openstack_sync/plugins/neutron/subnet_pools/reconcile.py new file mode 100644 index 000000000..661ef6a2c --- /dev/null +++ b/python/openstack-sync/openstack_sync/plugins/neutron/subnet_pools/reconcile.py @@ -0,0 +1,493 @@ +"""Reconcile a NeutronSubnetPool CR onto Neutron. + +Resolve the CR's Nautobot prefix references into CIDRs and an IP version, +validate them, ensure the address scope (create if absent, adopt if present), +then converge the subnet pool and link the scope. Nautobot stays the source of +truth for CIDRs; ``nautobot.py`` hands this module a spec already carrying +``prefixes`` and ``ip_version``. +""" + +from __future__ import annotations + +import ipaddress +import json +import logging +from typing import Any + +from openstack import exceptions as openstack_exceptions + +from openstack_sync.plugins.common import ConfigError +from openstack_sync.plugins.common import get_value +from openstack_sync.plugins.common import resource_id +from openstack_sync.plugins.neutron.subnet_pools import nautobot as nautobot_module +from openstack_sync.plugins.neutron.subnet_pools.markers import desired_tags + +LOG = logging.getLogger(__name__) + + +def _prefix_networks( + prefixes: list[str], +) -> list[ipaddress.IPv4Network | ipaddress.IPv6Network]: + try: + return [ipaddress.ip_network(prefix) for prefix in prefixes] + except ValueError as exc: + raise ConfigError(f"Invalid subnet pool prefix: {exc}") from exc + + +#: All three prefix-length bounds are required. Neutron would otherwise fill in +#: permissive defaults for any that are missing, giving an over-broad pool. +REQUIRED_PREFIX_LENGTHS = ( + "minimum_prefix_length", + "default_prefix_length", + "maximum_prefix_length", +) + + +def validate_prefixes(spec: dict[str, Any]) -> int: + """Validate CIDR prefixes and prefix-length bounds; return the IP version. + + Enforces the parts of the Neutron subnet-pool contract the OpenAPI schema + cannot: the CIDR set (resolved from Nautobot) is non-empty and single-family + and matches spec.ip_version, and the three prefix-length bounds are within + the IP family and ordered minimum <= default <= maximum. + """ + missing = [name for name in REQUIRED_PREFIX_LENGTHS if spec.get(name) is None] + if missing: + raise ConfigError( + f"Subnet pool {spec.get('name')!r} is missing prefix length(s) " + f"{missing}: set minimum_prefix_length, default_prefix_length, and " + "maximum_prefix_length on the CR spec" + ) + + prefixes = spec["prefixes"] + networks = _prefix_networks(prefixes) + versions = {network.version for network in networks} + if len(versions) != 1: + raise ConfigError("All subnet pool prefixes must have the same IP version") + + (version,) = versions + requested_version = spec.get("ip_version") + if requested_version is not None and requested_version != version: + raise ConfigError( + f"Subnet pool {spec['name']!r} prefixes are IPv{version}, " + f"but spec.ip_version is {requested_version}" + ) + + family_max = 32 if version == 4 else 128 + length_names = ( + "default_prefix_length", + "minimum_prefix_length", + "maximum_prefix_length", + ) + for name in length_names: + length = spec.get(name) + if length is not None and int(length) > family_max: + raise ConfigError( + f"Subnet pool {spec['name']!r} {name}={length} exceeds " + f"IPv{version} maximum prefix length {family_max}" + ) + + min_len = spec.get("minimum_prefix_length") + default_len = spec.get("default_prefix_length") + max_len = spec.get("maximum_prefix_length") + if ( + min_len is not None + and default_len is not None + and int(min_len) > int(default_len) + ): + raise ConfigError( + "minimum_prefix_length must be less than or equal to default_prefix_length" + ) + if ( + default_len is not None + and max_len is not None + and int(default_len) > int(max_len) + ): + raise ConfigError( + "default_prefix_length must be less than or equal to maximum_prefix_length" + ) + if min_len is not None and max_len is not None and int(min_len) > int(max_len): + raise ConfigError( + "minimum_prefix_length must be less than or equal to maximum_prefix_length" + ) + + return version + + +def _get_address_scope(conn: Any, scope_id: str) -> Any: + try: + return conn.network.get_address_scope(scope_id) + except openstack_exceptions.NotFoundException as exc: + raise ConfigError(f"Address scope {scope_id!r} was not found") from exc + + +def _scope_project_query( + spec: dict[str, Any], scope_spec: dict[str, Any] +) -> str | None: + return scope_spec.get("project_id") or spec.get("project_id") + + +def _find_address_scope_by_name( + conn: Any, scope_name: str, ip_version: int, project_id: str | None +) -> Any | None: + """Return the single scope matching *scope_name* and *ip_version*, or None. + + More than one match in the same IP family is ambiguous and raises. + """ + query: dict[str, Any] = {"name": scope_name, "ip_version": ip_version} + if project_id: + query["project_id"] = project_id + scopes = [ + scope + for scope in conn.network.address_scopes(**query) + if get_value(scope, "name") == scope_name + ] + if len(scopes) > 1: + raise ConfigError( + f"Address scope name {scope_name!r} matched {len(scopes)} " + "scopes; set spec.address_scope.id or project_id" + ) + return scopes[0] if scopes else None + + +def _create_address_scope( + conn: Any, + scope_name: str, + ip_version: int, + scope_spec: dict[str, Any], + project_id: str | None, +) -> Any: + # ip_version follows the pool's prefixes; shared defaults true like the pools. + attrs: dict[str, Any] = { + "name": scope_name, + "ip_version": ip_version, + "is_shared": bool(scope_spec.get("shared", True)), + } + if project_id: + attrs["project_id"] = project_id + LOG.info( + "Creating address scope %s ip_version=%s shared=%s", + scope_name, + ip_version, + attrs["is_shared"], + ) + return conn.network.create_address_scope(**attrs) + + +def ensure_address_scope( + conn: Any, spec: dict[str, Any], ip_version: int +) -> Any | None: + """Find, adopt, or create the address scope a CR spec declares. + + A scope named by the spec is reused when present and created when absent + (unless create is false); a scope referenced by id must already exist. The + operator never deletes an address scope: it outlives any single pool and the + L3/SVI validation of every subnet under it depends on it surviving. + + Returns None only when the spec declares no address scope (the CRD requires + it, so this is a defensive fallback). + """ + scope_spec = spec.get("address_scope") + if not scope_spec: + return None + + if scope_id := scope_spec.get("id"): + scope = _get_address_scope(conn, str(scope_id)) + else: + scope_name = str(scope_spec["name"]) + project_id = _scope_project_query(spec, scope_spec) + scope = _find_address_scope_by_name(conn, scope_name, ip_version, project_id) + if scope is not None: + LOG.info( + "Reusing address scope %s (%s) for subnet pool %s", + scope_name, + resource_id(scope), + spec["name"], + ) + elif scope_spec.get("create", True): + scope = _create_address_scope( + conn, scope_name, ip_version, scope_spec, project_id + ) + else: + raise ConfigError( + f"Address scope {scope_name!r} was not found for IPv{ip_version} " + "and spec.address_scope.create is false" + ) + + scope_version = get_value(scope, "ip_version") + if scope_version is not None and int(scope_version) != ip_version: + raise ConfigError( + f"Address scope {resource_id(scope)!r} is IPv{scope_version}, " + f"but subnet pool {spec['name']!r} is IPv{ip_version}" + ) + return scope + + +def _normalized_prefixes(prefixes: list[str]) -> list[str]: + return sorted(str(network) for network in _prefix_networks(prefixes)) + + +def _network_is_covered( + network: ipaddress.IPv4Network | ipaddress.IPv6Network, + covering: list[ipaddress.IPv4Network | ipaddress.IPv6Network], +) -> bool: + """Return whether *network* falls entirely within one of *covering*.""" + return any( + network.version == other.version + and int(network.network_address) >= int(other.network_address) + and int(network.broadcast_address) <= int(other.broadcast_address) + for other in covering + ) + + +def _prefixes_removed(have: list[str], want: list[str]) -> list[str]: + """Return CIDRs in *have* that the *want* set no longer covers. + + Neutron forbids shrinking a subnet pool: on update the existing prefix set + must be a subset of the new one, or it raises IllegalSubnetPoolPrefixUpdate + (HTTP 409). Removing a Nautobot prefix reference from a CR therefore cannot + be reconciled by the operator; draining and deleting the pool is an operator + decision. Detecting it here lets the reconcile report the drift as a note + and stay Synced rather than failing every cycle on a 409 it cannot resolve. + """ + want_networks = _prefix_networks(want) + return sorted( + str(network) + for network in _prefix_networks(have) + if not _network_is_covered(network, want_networks) + ) + + +def _subnet_pool_values(pool: Any) -> dict[str, Any]: + return { + "address_scope_id": get_value(pool, "address_scope_id"), + "description": get_value(pool, "description", default=""), + "prefixes": get_value(pool, "prefixes", default=[]), + "default_prefix_length": get_value(pool, "default_prefix_length"), + "minimum_prefix_length": get_value(pool, "minimum_prefix_length"), + "maximum_prefix_length": get_value(pool, "maximum_prefix_length"), + "is_default": get_value(pool, "is_default", default=False), + } + + +def _desired_attrs( + spec: dict[str, Any], address_scope_id: str | None +) -> dict[str, Any]: + attrs: dict[str, Any] = { + "name": spec["name"], + "prefixes": spec["prefixes"], + "description": spec.get("description", ""), + "is_default": bool(spec.get("is_default", False)), + } + if address_scope_id is not None: + attrs["address_scope_id"] = address_scope_id + optional_fields = { + "project_id": "project_id", + "shared": "is_shared", + "default_prefix_length": "default_prefix_length", + "minimum_prefix_length": "minimum_prefix_length", + "maximum_prefix_length": "maximum_prefix_length", + } + for spec_name, attr_name in optional_fields.items(): + if spec_name in spec: + attrs[attr_name] = spec[spec_name] + return attrs + + +def _check_default_conflict( + conn: Any, spec: dict[str, Any], ip_version: int, pool: Any | None +) -> None: + """Fail early if another pool is already the default for this IP family. + + Neutron allows only one default subnet pool per IP family and returns a + generic ``InvalidInput`` (HTTP 400) if a second is created. is_default is + also admin-only. Detecting the conflict here turns that opaque error into a + message naming the pool that already holds the default, and avoids a CR that + would fail every reconcile cycle. *pool* is the existing pool this CR + manages, if any; a pool that is already the default does not conflict with + itself. + """ + if not bool(spec.get("is_default", False)): + return + pool_id = resource_id(pool) if pool else None + existing = [ + other + for other in conn.network.subnet_pools(is_default=True, ip_version=ip_version) + if bool(get_value(other, "is_default", default=False)) + and int(get_value(other, "ip_version") or ip_version) == ip_version + and resource_id(other) != pool_id + ] + if existing: + holder = existing[0] + raise ConfigError( + f"Subnet pool {spec['name']!r} requests is_default=true for IPv" + f"{ip_version}, but pool {get_value(holder, 'name')!r} " + f"({resource_id(holder)}) is already the default for that family. " + "Neutron allows only one default subnet pool per IP family; clear " + "is_default on the other pool or on this CR." + ) + + +def find_subnet_pool(conn: Any, spec: dict[str, Any]) -> Any | None: + """Return the subnet pool matching the spec name and optional project.""" + query: dict[str, Any] = {"name": spec["name"]} + if project_id := spec.get("project_id"): + query["project_id"] = project_id + matches = [ + pool + for pool in conn.network.subnet_pools(**query) + if get_value(pool, "name") == spec["name"] + ] + if len(matches) > 1: + raise ConfigError( + f"Subnet pool name {spec['name']!r} matched {len(matches)} pools; " + "set spec.project_id" + ) + return matches[0] if matches else None + + +def _update_attrs(pool: Any, desired: dict[str, Any]) -> dict[str, Any]: + current = _subnet_pool_values(pool) + updates: dict[str, Any] = {} + for name, want in desired.items(): + if name in {"name", "project_id", "is_shared"}: + continue + have = current.get(name) + if name == "prefixes": + if _normalized_prefixes(list(have or [])) != _normalized_prefixes( + list(want) + ): + updates[name] = want + continue + if have != want: + updates[name] = want + return updates + + +def _validate_existing(pool: Any, spec: dict[str, Any], ip_version: int) -> None: + pool_id = resource_id(pool) + if spec_project := spec.get("project_id"): + pool_project = get_value(pool, "project_id") + if pool_project and pool_project != spec_project: + raise ConfigError( + f"Subnet pool {spec['name']!r} ({pool_id}) belongs to " + f"project_id={pool_project!r}; expected {spec_project!r}" + ) + if "shared" in spec: + current_shared = bool(get_value(pool, "is_shared", default=False)) + if current_shared != bool(spec["shared"]): + raise ConfigError( + f"Subnet pool {spec['name']!r} ({pool_id}) has shared=" + f"{current_shared}; Neutron does not allow updating shared on an " + "existing subnet pool. Drain and delete the pool to let the " + "operator recreate it, or set spec.shared to match." + ) + pool_version = get_value(pool, "ip_version") + if pool_version is not None and int(pool_version) != ip_version: + raise ConfigError( + f"Subnet pool {spec['name']!r} ({pool_id}) is IPv{pool_version}; " + f"expected IPv{ip_version}" + ) + + +def _ensure_tags(conn: Any, pool: Any, spec: dict[str, Any]) -> None: + current_tags = sorted(str(tag) for tag in get_value(pool, "tags", default=[]) or []) + want_tags = desired_tags(spec) + if current_tags == want_tags: + return + LOG.info("Reconciling subnet pool %s tags to %s", resource_id(pool), want_tags) + conn.network.set_tags(pool, want_tags) + + +def render_subnet_pool(pool: Any) -> dict[str, Any]: + return { + "id": get_value(pool, "id"), + "name": get_value(pool, "name"), + "address_scope_id": get_value(pool, "address_scope_id"), + "project_id": get_value(pool, "project_id"), + "prefixes": get_value(pool, "prefixes", default=[]), + "ip_version": get_value(pool, "ip_version"), + "is_default": get_value(pool, "is_default"), + "is_shared": get_value(pool, "is_shared"), + "tags": get_value(pool, "tags", default=[]), + } + + +def resolve_desired_names(specs: list[dict[str, Any]]) -> list[str]: + """Return the Neutron pool name each CR spec manages. + + Prune matches Neutron pools by name. The name is declared directly on the + CR (``spec.name`` is required), so prune needs no Nautobot call to learn it: + it reads the literal name from each surviving spec. This keeps prune from + depending on Nautobot reachability, so a Nautobot outage can never make a + live CR's pool look undesired and turn it into a delete candidate. + + A spec missing a name is skipped with a warning; it also failed CRD + validation and reconcile, so the run already reports failure and prune is + skipped anyway. + """ + names: list[str] = [] + for spec in specs: + name = str(spec.get("name") or "").strip() + if not name: + LOG.warning("Skipping subnet pool spec with no name for prune") + continue + names.append(name) + return names + + +def sync_subnet_pool( + conn: Any, spec: dict[str, Any], namespace: str, cache: dict[str, Any] +) -> list[str]: + """Converge one NeutronSubnetPool spec. + + Resolves the CR's Nautobot prefix references first, so *spec* need only + carry the CRD fields; ``prefixes`` and ``ip_version`` are derived here. + """ + resolved = nautobot_module.resolve_spec(spec, cache, namespace) + name = resolved["name"] + ip_version = validate_prefixes(resolved) + address_scope = ensure_address_scope(conn, resolved, ip_version) + address_scope_id = resource_id(address_scope) if address_scope else None + desired = _desired_attrs(resolved, address_scope_id) + + notes: list[str] = [] + pool = find_subnet_pool(conn, resolved) + _check_default_conflict(conn, resolved, ip_version, pool) + if not pool: + LOG.info( + "Creating subnet pool %s address_scope_id=%s prefixes=%s", + name, + address_scope_id, + resolved["prefixes"], + ) + pool = conn.network.create_subnet_pool(**desired) + else: + LOG.info("Subnet pool %s already exists", name) + _validate_existing(pool, resolved, ip_version) + updates = _update_attrs(pool, desired) + removed = _prefixes_removed( + list(get_value(pool, "prefixes", default=[]) or []), + resolved["prefixes"], + ) + if removed: + # Neutron rejects a shrinking prefix update (409); pushing it would + # fail every cycle. Keep the existing prefixes, converge the rest, + # and surface the drift for an operator to resolve. + notes.append( + f"Neutron subnet pool {name!r} still holds prefixes no longer " + f"referenced in Nautobot ({', '.join(removed)}); Neutron does " + "not allow removing prefixes from a pool, so drain and delete " + "the pool to remove them" + ) + updates.pop("prefixes", None) + if updates: + LOG.info("Reconciling subnet pool %s fields %s", name, sorted(updates)) + pool = conn.network.update_subnet_pool(pool, **updates) + + _ensure_tags(conn, pool, resolved) + LOG.info( + "Reconciled subnet pool: %s", + json.dumps(render_subnet_pool(pool), sort_keys=True), + ) + return notes diff --git a/python/openstack-sync/pyproject.toml b/python/openstack-sync/pyproject.toml index c43f68b26..4b36f15d9 100644 --- a/python/openstack-sync/pyproject.toml +++ b/python/openstack-sync/pyproject.toml @@ -13,6 +13,7 @@ dynamic = ["version"] dependencies = [ "kubernetes>=32.0.0", "openstacksdk>=4.18.0", + "pynautobot>=3,<4", "python-ironicclient>=6.2.0", "python-openstackclient>=10.2.1", ] diff --git a/python/openstack-sync/tests/test_subnet_pools_hook.py b/python/openstack-sync/tests/test_subnet_pools_hook.py index b029a9ca0..5eec13160 100644 --- a/python/openstack-sync/tests/test_subnet_pools_hook.py +++ b/python/openstack-sync/tests/test_subnet_pools_hook.py @@ -1,23 +1,56 @@ -"""Tests for the subnet pool hook skeleton wiring.""" +"""Tests for the subnet pool hook: how the plugin wires into the framework. + +The generic driver is covered in ``test_framework.py``; these tests cover only +what is specific to this plugin, plus one end-to-end run through ``main()``. +Nautobot resolution is covered in ``test_subnet_pools_nautobot.py`` and Neutron +convergence in ``test_subnet_pools_reconcile.py``. +""" from __future__ import annotations +import importlib +import json +import types +from pathlib import Path from typing import Any from unittest import mock +import pytest + +import openstack_sync.utils as utils from openstack_sync.hooks import subnet_pools as hook +from openstack_sync.hooks.framework import CleanupPolicy from openstack_sync.hooks.framework import HookConfig +from openstack_sync.hooks.framework import PruneRequest +from openstack_sync.plugins.neutron.subnet_pools.config import BINDING_NAME +from openstack_sync.plugins.neutron.subnet_pools.config import ENV_PREFIX +from openstack_sync.plugins.neutron.subnet_pools.config import OWNERSHIP_TAG + +CRD_API_VERSION = "neutron.understack.rackspace.net/v1alpha1" +CRD_KIND = "NeutronSubnetPool" +CRD_RESOURCE = "neutronsubnetpools.neutron.understack.rackspace.net" -ENV_PREFIX = hook.ENV_PREFIX -BINDING_NAME = hook.BINDING_NAME +ENV_NAMES = ( + "BINDING_CONTEXT_PATH", + f"{ENV_PREFIX}_ENABLED", + f"{ENV_PREFIX}_SYNC_CRONTAB", + f"{ENV_PREFIX}_PRUNE", + f"{ENV_PREFIX}_STATUS_ENABLED", + f"{ENV_PREFIX}_READY_RETRIES", + f"{ENV_PREFIX}_READY_DELAY", + f"{ENV_PREFIX}_CRD_API_VERSION", + f"{ENV_PREFIX}_CRD_KIND", + f"{ENV_PREFIX}_CRD_RESOURCE", + "POD_NAMESPACE", +) def _config(**overrides: Any) -> HookConfig: defaults = { "prefix": ENV_PREFIX, - "crd_api_version": "neutron.understack.rackspace.net/v1alpha1", - "crd_kind": "NeutronSubnetPool", - "crd_resource": "neutronsubnetpools.neutron.understack.rackspace.net", + "crd_api_version": CRD_API_VERSION, + "crd_kind": CRD_KIND, + "crd_resource": CRD_RESOURCE, "binding_name": BINDING_NAME, "namespace": "openstack", "status_enabled": False, @@ -29,25 +62,442 @@ def _config(**overrides: Any) -> HookConfig: return HookConfig(**{**defaults, **overrides}) -def test_plugin_wait_for_api_is_noop_skeleton(): - plugin = hook.SubnetPoolPlugin(_config(ready_retries=5, ready_delay=0.25)) - conn = mock.MagicMock() +def clear_env(monkeypatch: pytest.MonkeyPatch) -> None: + for name in ENV_NAMES: + monkeypatch.delenv(name, raising=False) + + +def set_crd_identity(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv(f"{ENV_PREFIX}_CRD_API_VERSION", CRD_API_VERSION) + monkeypatch.setenv(f"{ENV_PREFIX}_CRD_KIND", CRD_KIND) + monkeypatch.setenv(f"{ENV_PREFIX}_CRD_RESOURCE", CRD_RESOURCE) + + +def _spec() -> dict[str, Any]: + """A CR carrying the full pool contract; Nautobot supplies only the CIDRs.""" + return { + "address_scope": {"name": "publicnet-ip4"}, + "minimum_prefix_length": 27, + "default_prefix_length": 28, + "maximum_prefix_length": 30, + "nautobot": { + "url": "https://nautobot.example.test", + "api_version": "2.0", + "tokenSecretRef": {"secretName": "nautobot-token", "key": "token"}, + "prefix_refs": [ + {"id": "2bc3ecab-b6dc-46cd-9bd4-1c0ea8a07f87"}, + {"id": "53c8ee3b-09b9-41ab-a413-b1a1f5ecec6a"}, + ], + "require": { + "location": "iad3-dev", + "tags": ["openstack-subnet-pool"], + "status": "Active", + "type": "pool", + "namespace": "Rackspace", + }, + }, + "is_default": False, + "shared": True, + "tags": ["managed-by-openstack-sync"], + } + + +def subnet_pool_object(name: str, spec: dict | None = None) -> dict: + pool_spec = _spec() + pool_spec["name"] = name + pool_spec["cloudCredentialsRef"] = { + "secretName": "infrasetup", + "cloudName": "understack", + } + pool_spec.update(spec or {}) + return { + "apiVersion": CRD_API_VERSION, + "kind": CRD_KIND, + "metadata": {"name": name, "namespace": "openstack", "generation": 3}, + "spec": pool_spec, + } + + +def write_binding_context(path: Path, contexts: list[dict]) -> str: + context_path = path / "binding-context.json" + context_path.write_text(json.dumps(contexts), encoding="utf-8") + return str(context_path) + + +# --------------------------------------------------------------------------- +# Import safety +# --------------------------------------------------------------------------- + + +def test_module_import_is_safe_with_bad_runtime_env(monkeypatch): + """Importing must not read runtime config. + + Shell-operator imports the hook to ask for its config before the full + environment is guaranteed, so a malformed value must not break import. + """ + monkeypatch.setenv(f"{ENV_PREFIX}_READY_RETRIES", "not-a-number") + monkeypatch.setenv(f"{ENV_PREFIX}_ENABLED", "not-a-bool") + + importlib.reload(hook) - plugin.wait_for_api(conn) +def test_enabled_config_flag_watches_this_crd(monkeypatch, capsys): + clear_env(monkeypatch) + set_crd_identity(monkeypatch) + monkeypatch.setenv(f"{ENV_PREFIX}_ENABLED", "true") + monkeypatch.setattr(hook.sys, "argv", ["subnet_pools.py", "--config"]) -def test_plugin_reconcile_reads_spec_without_openstack_calls(): - plugin = hook.SubnetPoolPlugin(_config()) + assert hook.main() == 0 + config = json.loads(capsys.readouterr().out) + (binding,) = config["kubernetes"] + assert binding["name"] == BINDING_NAME + assert binding["kind"] == CRD_KIND + + +# --------------------------------------------------------------------------- +# Plugin wiring +# --------------------------------------------------------------------------- + + +def test_plugin_reconcile_delegates_to_sync_subnet_pool(): + plugin = hook.SubnetPoolPlugin(_config(namespace="openstack")) conn = mock.MagicMock() + cache: dict[str, Any] = {} spec = {"name": "pool-a"} - notes = plugin.reconcile(conn, spec, {}) + with mock.patch.object( + hook.reconcile_module, "sync_subnet_pool", return_value=[] + ) as sync_subnet_pool: + notes = plugin.reconcile(conn, spec, cache) assert notes == [] - conn.assert_not_called() + sync_subnet_pool.assert_called_once_with(conn, spec, "openstack", cache) + + +def test_plugin_reconcile_falls_back_to_pod_namespace_when_unset(): + plugin = hook.SubnetPoolPlugin(_config(namespace=None)) + conn = mock.MagicMock() + + with ( + mock.patch.object( + hook.reconcile_module, "sync_subnet_pool", return_value=[] + ) as sync_subnet_pool, + mock.patch.object(hook, "pod_namespace", return_value="fallback-ns"), + ): + plugin.reconcile(conn, {"name": "pool-a"}, {}) + + assert sync_subnet_pool.call_args.args[2] == "fallback-ns" + + +def test_plugin_wait_for_api_uses_configured_retry_budget(): + plugin = hook.SubnetPoolPlugin(_config(ready_retries=5, ready_delay=0.25)) + conn = mock.MagicMock() + + with mock.patch.object(hook, "wait_for_openstack_network") as wait: + plugin.wait_for_api(conn) + + wait.assert_called_once_with(conn, retries=5, delay=0.25) + + +def test_plugin_prune_resolves_desired_names_before_deleting(): + plugin = hook.SubnetPoolPlugin(_config(prune=False, namespace="openstack")) + conn = mock.MagicMock() + specs = [{"nautobot": {"prefix_refs": [{"id": "x"}]}}] + + with ( + mock.patch.object( + hook.reconcile_module, "resolve_desired_names", return_value=["a"] + ) as resolve, + mock.patch.object(hook.prune_module, "prune_removed_subnet_pools") as prune, + ): + plugin.prune_resources( + conn, + PruneRequest( + credentials=("infrasetup", "understack"), + desired_specs=specs, + authoritative_empty=False, + ), + ) + + # The pool name is on the CR, so prune is fed names read from the specs. + resolve.assert_called_once_with(specs) + prune.assert_called_once_with(conn, ["a"], authoritative_empty=False) + + +def test_plugin_prune_forwards_authoritative_empty_when_enabled(): + plugin = hook.SubnetPoolPlugin(_config(prune=True, namespace="openstack")) + conn = mock.MagicMock() + specs = [{"nautobot": {"prefix_refs": [{"id": "x"}]}}] + + with ( + mock.patch.object( + hook.reconcile_module, "resolve_desired_names", return_value=["a"] + ), + mock.patch.object(hook.prune_module, "prune_removed_subnet_pools") as prune, + ): + plugin.prune_resources( + conn, + PruneRequest( + credentials=("infrasetup", "understack"), + desired_specs=specs, + authoritative_empty=True, + ), + ) + + prune.assert_called_once_with(conn, ["a"], authoritative_empty=True) + + +def test_plugin_cleanup_policy_has_no_prune_step_when_disabled(): + plugin = hook.SubnetPoolPlugin(_config(prune=False)) + + assert plugin.cleanup_policy() is CleanupPolicy.NONE + assert plugin.should_run_prune() is False + assert plugin.uses_finalizer() is False + + +def test_plugin_uses_finalized_prune_when_enabled(): + plugin = hook.SubnetPoolPlugin(_config(prune=True)) + + assert plugin.cleanup_policy() is CleanupPolicy.FINALIZED_PRUNE + assert plugin.should_run_prune() is True + assert plugin.uses_finalizer() is True + + +# --------------------------------------------------------------------------- +# End to end through main() +# --------------------------------------------------------------------------- + + +def _prefix(prefix: str, prefix_id: str) -> types.SimpleNamespace: + return types.SimpleNamespace( + id=prefix_id, + prefix=prefix, + status=types.SimpleNamespace(name="Active"), + type="pool", + namespace=types.SimpleNamespace(name="Rackspace"), + locations=[types.SimpleNamespace(name="iad3-dev")], + tags=[types.SimpleNamespace(name="openstack-subnet-pool")], + ) + + +def _neutron_conn() -> Any: + """A Neutron connection that already holds the desired subnet pool.""" + scope = types.SimpleNamespace(id="scope-id", name="publicnet-ip4", ip_version=4) + pool = types.SimpleNamespace( + id="pool-id", + name="PUBLIC-IP-POOL", + address_scope_id="scope-id", + project_id=None, + prefixes=["204.232.163.128/25", "10.4.88.0/24"], + ip_version=4, + default_prefix_length=28, + minimum_prefix_length=27, + maximum_prefix_length=30, + description="", + is_default=False, + is_shared=True, + tags=["managed-by-openstack-sync", OWNERSHIP_TAG], + ) + conn = mock.MagicMock() + conn.network.address_scopes.return_value = [scope] + conn.network.subnet_pools.return_value = [pool] + return conn + + +def _nautobot_client() -> Any: + # Keyed by id so reconcile can resolve each referenced prefix's CIDR. + prefixes = { + "2bc3ecab-b6dc-46cd-9bd4-1c0ea8a07f87": _prefix( + "204.232.163.128/25", "2bc3ecab-b6dc-46cd-9bd4-1c0ea8a07f87" + ), + "53c8ee3b-09b9-41ab-a413-b1a1f5ecec6a": _prefix( + "10.4.88.0/24", "53c8ee3b-09b9-41ab-a413-b1a1f5ecec6a" + ), + } + client = mock.MagicMock() + # reconcile passes id plus (when require.location is set) location=; accept + # and ignore the extra kwargs so the fake resolves purely by id. + client.ipam.prefixes.get.side_effect = lambda id, **_: prefixes.get(id) + return client + + +def _schedule_context(*names: str) -> list[dict]: + return [ + { + "binding": BINDING_NAME, + "type": "Schedule", + "snapshots": { + BINDING_NAME: [{"object": subnet_pool_object(n)} for n in names] + }, + } + ] + + +def _run_main(monkeypatch, tmp_path, contexts: list[dict], conn: Any): + monkeypatch.setenv( + "BINDING_CONTEXT_PATH", write_binding_context(tmp_path, contexts) + ) + with ( + mock.patch.object(hook.sys, "argv", ["subnet_pools.py"]), + mock.patch( + "openstack_sync.hooks.framework.get_openstack_connection", + return_value=conn, + ), + mock.patch( + "openstack_sync.hooks.framework.patch_resource_status" + ) as patch_status, + mock.patch( + "openstack_sync.hooks.framework.add_resource_finalizer", + return_value=True, + ), + mock.patch( + "openstack_sync.hooks.framework.remove_resource_finalizer", + return_value=True, + ), + mock.patch.object(hook, "wait_for_openstack_network"), + mock.patch( + "openstack_sync.plugins.neutron.subnet_pools.nautobot.read_secret_key", + return_value="nb-token", + ), + mock.patch( + "openstack_sync.plugins.neutron.subnet_pools.nautobot.pynautobot.api", + return_value=_nautobot_client(), + ), + ): + code = hook.main() + return code, patch_status + + +def test_main_returns_zero_when_hook_disabled(monkeypatch, tmp_path): + clear_env(monkeypatch) + set_crd_identity(monkeypatch) + conn = _neutron_conn() + + code, patch_status = _run_main( + monkeypatch, tmp_path, _schedule_context("PUBLIC-IP-POOL"), conn + ) + + assert code == 0 + patch_status.assert_not_called() + conn.network.subnet_pools.assert_not_called() + + +def test_main_reconciles_an_already_converged_pool(monkeypatch, tmp_path): + clear_env(monkeypatch) + set_crd_identity(monkeypatch) + monkeypatch.setenv(f"{ENV_PREFIX}_ENABLED", "true") + monkeypatch.setenv("POD_NAMESPACE", "openstack") + conn = _neutron_conn() + + code, patch_status = _run_main( + monkeypatch, tmp_path, _schedule_context("PUBLIC-IP-POOL"), conn + ) + + assert code == 0 + assert patch_status.call_args.kwargs["sync_status"] == "Synced" + # Already converged: no writes to Neutron. + conn.network.create_subnet_pool.assert_not_called() + conn.network.update_subnet_pool.assert_not_called() + conn.network.set_tags.assert_not_called() + + +def test_main_creates_a_missing_pool(monkeypatch, tmp_path): + clear_env(monkeypatch) + set_crd_identity(monkeypatch) + monkeypatch.setenv(f"{ENV_PREFIX}_ENABLED", "true") + conn = _neutron_conn() + conn.network.subnet_pools.return_value = [] + created = types.SimpleNamespace( + id="pool-id", + name="PUBLIC-IP-POOL", + prefixes=["204.232.163.128/25", "10.4.88.0/24"], + ip_version=4, + tags=[], + ) + conn.network.create_subnet_pool.return_value = created + + code, patch_status = _run_main( + monkeypatch, tmp_path, _schedule_context("PUBLIC-IP-POOL"), conn + ) + + assert code == 0 + assert patch_status.call_args.kwargs["sync_status"] == "Synced" + create_kwargs = conn.network.create_subnet_pool.call_args.kwargs + assert create_kwargs["name"] == "PUBLIC-IP-POOL" + assert create_kwargs["address_scope_id"] == "scope-id" + assert create_kwargs["prefixes"] == ["204.232.163.128/25", "10.4.88.0/24"] + conn.network.set_tags.assert_called_once_with( + created, ["managed-by-openstack-sync", OWNERSHIP_TAG] + ) + + +def test_main_reports_failure_and_skips_prune(monkeypatch, tmp_path): + clear_env(monkeypatch) + set_crd_identity(monkeypatch) + monkeypatch.setenv(f"{ENV_PREFIX}_ENABLED", "true") + monkeypatch.setenv(f"{ENV_PREFIX}_PRUNE", "true") + conn = _neutron_conn() + # An existing pool with a different IP version is a hard failure. + conn.network.subnet_pools.return_value[0].ip_version = 6 + + with mock.patch.object(hook.prune_module, "prune_removed_subnet_pools") as prune: + code, patch_status = _run_main( + monkeypatch, tmp_path, _schedule_context("PUBLIC-IP-POOL"), conn + ) + + assert code == 1 + assert patch_status.call_args.kwargs["sync_status"] == "Failed" + prune.assert_not_called() + + +def test_main_prunes_after_a_successful_reconcile(monkeypatch, tmp_path): + clear_env(monkeypatch) + set_crd_identity(monkeypatch) + monkeypatch.setenv(f"{ENV_PREFIX}_ENABLED", "true") + monkeypatch.setenv(f"{ENV_PREFIX}_PRUNE", "true") + conn = _neutron_conn() + + with mock.patch.object(hook.prune_module, "prune_removed_subnet_pools") as prune: + code, _ = _run_main( + monkeypatch, tmp_path, _schedule_context("PUBLIC-IP-POOL"), conn + ) + + assert code == 0 + prune.assert_called_once() + # Prune receives resolved Neutron pool names, not raw CR specs. + assert prune.call_args.args[1] == ["PUBLIC-IP-POOL"] + +def test_main_uses_the_credentials_named_by_each_cr(monkeypatch, tmp_path): + clear_env(monkeypatch) + set_crd_identity(monkeypatch) + monkeypatch.setenv(f"{ENV_PREFIX}_ENABLED", "true") + monkeypatch.setattr(utils, "_connection_cache", {}) + contexts = _schedule_context("PUBLIC-IP-POOL") + contexts[0]["snapshots"][BINDING_NAME][0]["object"]["spec"][ + "cloudCredentialsRef" + ] = {"secretName": "other-secret", "cloudName": "other-cloud"} + monkeypatch.setenv( + "BINDING_CONTEXT_PATH", write_binding_context(tmp_path, contexts) + ) -def test_plugin_cleanup_policy_has_no_prune_step(): - plugin = hook.SubnetPoolPlugin(_config()) + with ( + mock.patch.object(hook.sys, "argv", ["subnet_pools.py"]), + mock.patch( + "openstack_sync.hooks.framework.get_openstack_connection", + return_value=_neutron_conn(), + ) as connect, + mock.patch("openstack_sync.hooks.framework.patch_resource_status"), + mock.patch.object(hook, "wait_for_openstack_network"), + mock.patch( + "openstack_sync.plugins.neutron.subnet_pools.nautobot.read_secret_key", + return_value="nb-token", + ), + mock.patch( + "openstack_sync.plugins.neutron.subnet_pools.nautobot.pynautobot.api", + return_value=_nautobot_client(), + ), + ): + assert hook.main() == 0 - assert not plugin.should_run_prune() + connect.assert_called_once_with("other-secret", "other-cloud") diff --git a/python/openstack-sync/tests/test_subnet_pools_nautobot.py b/python/openstack-sync/tests/test_subnet_pools_nautobot.py new file mode 100644 index 000000000..15f17797b --- /dev/null +++ b/python/openstack-sync/tests/test_subnet_pools_nautobot.py @@ -0,0 +1,372 @@ +"""Tests for Nautobot resolution of NeutronSubnetPool CRs. + +The CR is the source of truth for pool policy (name, address scope, prefix +lengths); Nautobot owns only the CIDRs. These tests cover CIDR extraction, IP +version inference, and the ``require`` guardrails each referenced prefix must +satisfy. Policy derivation from custom_fields no longer exists. +""" + +from __future__ import annotations + +from types import SimpleNamespace +from typing import Any +from unittest import mock + +import pytest + +from openstack_sync.plugins.common import ConfigError +from openstack_sync.plugins.neutron.subnet_pools import nautobot + +PUBLIC_ID = "2bc3ecab-b6dc-46cd-9bd4-1c0ea8a07f87" +PUBLIC_ID_2 = "53c8ee3b-09b9-41ab-a413-b1a1f5ecec6a" + + +def _spec(**overrides: Any) -> dict[str, Any]: + """A CR carrying the full pool contract plus Nautobot prefix references.""" + spec: dict[str, Any] = { + "name": "PUBLIC-IP-POOL", + "address_scope": {"name": "publicnet-ip4"}, + "minimum_prefix_length": 27, + "default_prefix_length": 28, + "maximum_prefix_length": 30, + "shared": True, + "nautobot": { + "url": "https://nautobot.example.test", + "api_version": "2.0", + "tokenSecretRef": {"secretName": "nautobot-token", "key": "token"}, + "prefix_refs": [{"id": PUBLIC_ID}, {"id": PUBLIC_ID_2}], + "require": { + "location": "iad3-dev", + "tags": ["openstack-subnet-pool"], + "status": "Active", + "type": "pool", + "namespace": "Rackspace", + }, + }, + } + spec.update(overrides) + return spec + + +def _single_ref_spec(**overrides: Any) -> dict[str, Any]: + """A CR referencing exactly one prefix, for single-record cases.""" + spec = _spec(**overrides) + spec["nautobot"] = {**spec["nautobot"], "prefix_refs": [{"id": PUBLIC_ID}]} + return spec + + +def _prefix(prefix: str, **overrides: Any) -> SimpleNamespace: + # A Nautobot prefix serializes location as null; require.location is enforced + # by the ?location= query filter, so the record carries no location field. + defaults = { + "id": "prefix-id", + "prefix": prefix, + "status": SimpleNamespace(name="Active"), + "type": "pool", + "namespace": SimpleNamespace(name="Rackspace"), + "tags": [SimpleNamespace(name="openstack-subnet-pool")], + } + return SimpleNamespace(**{**defaults, **overrides}) + + +def _client(*prefixes: SimpleNamespace) -> Any: + client = mock.MagicMock() + client.ipam.prefixes.get.side_effect = list(prefixes) + return client + + +@pytest.fixture(autouse=True) +def _clear_site_env(monkeypatch: pytest.MonkeyPatch) -> None: + """Keep a real UNDERSTACK_SITE out of the tests; each sets it explicitly.""" + monkeypatch.delenv(nautobot.SITE_ENV, raising=False) + + +def _resolve(monkeypatch, spec, *prefixes, cache=None): + client = _client(*prefixes) + monkeypatch.setattr(nautobot, "read_secret_key", mock.Mock(return_value="nb-token")) + monkeypatch.setattr(nautobot.pynautobot, "api", mock.Mock(return_value=client)) + return nautobot.resolve_spec(spec, cache if cache is not None else {}, "openstack") + + +# --------------------------------------------------------------------------- +# CIDR extraction and IP version +# --------------------------------------------------------------------------- + + +def test_resolve_spec_attaches_cidrs_and_ip_version(monkeypatch): + resolved = _resolve( + monkeypatch, + _spec(), + _prefix("204.232.163.128/25", id=PUBLIC_ID), + _prefix("10.4.88.0/24", id=PUBLIC_ID_2), + ) + + assert resolved["prefixes"] == ["204.232.163.128/25", "10.4.88.0/24"] + assert resolved["ip_version"] == 4 + + +def test_resolve_spec_leaves_policy_fields_untouched(monkeypatch): + """Resolution must not read or alter the pool policy on the CR.""" + resolved = _resolve( + monkeypatch, + _spec(), + _prefix("204.232.163.128/25", id=PUBLIC_ID), + _prefix("10.4.88.0/24", id=PUBLIC_ID_2), + ) + + assert resolved["name"] == "PUBLIC-IP-POOL" + assert resolved["address_scope"] == {"name": "publicnet-ip4"} + assert resolved["minimum_prefix_length"] == 27 + assert resolved["default_prefix_length"] == 28 + assert resolved["maximum_prefix_length"] == 30 + + +def test_resolve_spec_requires_name(monkeypatch): + spec = _single_ref_spec() + del spec["name"] + with pytest.raises(ConfigError, match="name must be set"): + _resolve(monkeypatch, spec, _prefix("10.0.0.0/24")) + + +def test_resolve_spec_rejects_mixed_ip_versions(monkeypatch): + with pytest.raises(ConfigError, match="mixed IP versions"): + _resolve( + monkeypatch, + _spec(), + _prefix("10.0.0.0/24", id=PUBLIC_ID), + _prefix("2001:db8::/64", id=PUBLIC_ID_2), + ) + + +# --------------------------------------------------------------------------- +# Location is enforced server-side via the ?location= filter +# --------------------------------------------------------------------------- + + +def test_resolve_spec_filters_lookup_by_require_location(monkeypatch): + """require.location is passed to the prefix lookup as a query filter. + + A Nautobot prefix serializes location as null, so the association can only + be matched by querying ?location=; the resolver must send it, not read a + location field off the record. + """ + client = _client( + _prefix("204.232.163.128/25", id=PUBLIC_ID), + _prefix("10.4.88.0/24", id=PUBLIC_ID_2), + ) + monkeypatch.setattr(nautobot, "read_secret_key", mock.Mock(return_value="nb-token")) + monkeypatch.setattr(nautobot.pynautobot, "api", mock.Mock(return_value=client)) + + nautobot.resolve_spec(_spec(), {}, "openstack") + + for call in client.ipam.prefixes.get.call_args_list: + assert call.kwargs["location"] == "iad3-dev" + assert "id" in call.kwargs + + +def test_resolve_spec_rejects_prefix_not_under_required_location(monkeypatch): + """A prefix not returned under the required location fails the guardrail. + + Nautobot returns nothing for ?id=&location=, which the resolver + reports as the prefix not being under the required location. + """ + with pytest.raises(ConfigError, match="was not found under location 'iad3-dev'"): + _resolve(monkeypatch, _single_ref_spec(), None) + + +def _location_400_request_error() -> nautobot.pynautobot.RequestError: + """Build the RequestError Nautobot raises for an invalid location choice. + + Nautobot answers ``?location=`` with HTTP 400 and a body naming the + location field; pynautobot wraps the response in RequestError, exposing it + as ``.req`` with ``.error`` holding the body text. + """ + response = SimpleNamespace( + status_code=400, + reason="Bad Request", + url="https://nautobot.example.test/api/ipam/prefixes/", + text='{"location": ["Select a valid choice. iad is not one of the ' + 'available choices."]}', + request=SimpleNamespace(body=None), + ) + response.json = lambda: {"location": ["Select a valid choice."]} + return nautobot.pynautobot.RequestError(response) + + +def test_resolve_spec_reports_invalid_location_choice(monkeypatch): + """A wrong-level or typo'd location fails as a guardrail misconfig, not a 500. + + Location is a hierarchical choice filter, so Nautobot rejects a Region name + like 'iad' (instead of the Site 'iad3-dev') with HTTP 400. The resolver must + translate that into a message pointing at spec.nautobot.require.location. + """ + client = mock.MagicMock() + client.ipam.prefixes.get.side_effect = _location_400_request_error() + monkeypatch.setattr(nautobot, "read_secret_key", mock.Mock(return_value="nb-token")) + monkeypatch.setattr(nautobot.pynautobot, "api", mock.Mock(return_value=client)) + + spec = _single_ref_spec() + spec["nautobot"] = {**spec["nautobot"], "require": {"location": "iad"}} + with pytest.raises(ConfigError, match="is not a valid Nautobot location"): + nautobot.resolve_spec(spec, {}, "openstack") + + +def test_resolve_spec_omits_location_filter_when_not_required(monkeypatch): + """With no require.location, the lookup is by id only.""" + spec = _single_ref_spec() + spec["nautobot"] = {**spec["nautobot"], "require": {}} + client = _client(_prefix("10.0.0.0/24", id=PUBLIC_ID)) + monkeypatch.setattr(nautobot, "read_secret_key", mock.Mock(return_value="nb-token")) + monkeypatch.setattr(nautobot.pynautobot, "api", mock.Mock(return_value=client)) + + nautobot.resolve_spec(spec, {}, "openstack") + + (call,) = client.ipam.prefixes.get.call_args_list + assert "location" not in call.kwargs + + +def test_resolve_spec_defaults_location_from_site_env(monkeypatch): + """With no require.location, the lookup falls back to UNDERSTACK_SITE.""" + monkeypatch.setenv(nautobot.SITE_ENV, "iad3-dev") + spec = _single_ref_spec() + spec["nautobot"] = {**spec["nautobot"], "require": {}} + client = _client(_prefix("10.0.0.0/24", id=PUBLIC_ID)) + monkeypatch.setattr(nautobot, "read_secret_key", mock.Mock(return_value="nb-token")) + monkeypatch.setattr(nautobot.pynautobot, "api", mock.Mock(return_value=client)) + + nautobot.resolve_spec(spec, {}, "openstack") + + (call,) = client.ipam.prefixes.get.call_args_list + assert call.kwargs["location"] == "iad3-dev" + + +def test_resolve_spec_require_location_overrides_site_env(monkeypatch): + """An explicit require.location wins over UNDERSTACK_SITE.""" + monkeypatch.setenv(nautobot.SITE_ENV, "other-site") + client = _client(_prefix("10.0.0.0/24", id=PUBLIC_ID)) + monkeypatch.setattr(nautobot, "read_secret_key", mock.Mock(return_value="nb-token")) + monkeypatch.setattr(nautobot.pynautobot, "api", mock.Mock(return_value=client)) + + nautobot.resolve_spec(_single_ref_spec(), {}, "openstack") + + (call,) = client.ipam.prefixes.get.call_args_list + assert call.kwargs["location"] == "iad3-dev" + + +# --------------------------------------------------------------------------- +# Loading and guardrails +# --------------------------------------------------------------------------- + + +def test_resolve_spec_reuses_cached_client(monkeypatch): + client = _client( + _prefix("204.232.163.128/25", id=PUBLIC_ID), + _prefix("10.4.88.0/24", id=PUBLIC_ID_2), + _prefix("204.232.163.128/25", id=PUBLIC_ID), + _prefix("10.4.88.0/24", id=PUBLIC_ID_2), + ) + api = mock.Mock(return_value=client) + monkeypatch.setattr(nautobot, "read_secret_key", mock.Mock(return_value="nb-token")) + monkeypatch.setattr(nautobot.pynautobot, "api", api) + cache: dict[str, Any] = {} + + nautobot.resolve_spec(_spec(), cache, "openstack") + nautobot.resolve_spec(_spec(), cache, "openstack") + + api.assert_called_once() + + +def test_resolve_spec_rejects_missing_prefix(monkeypatch): + with pytest.raises(ConfigError, match="was not found"): + _resolve(monkeypatch, _spec(), None) + + +def test_resolve_spec_rejects_prefix_that_fails_requirements(monkeypatch): + with pytest.raises(ConfigError, match="status is 'Reserved'"): + _resolve( + monkeypatch, + _spec(), + _prefix("204.232.163.128/25", id=PUBLIC_ID), + _prefix( + "10.4.88.0/24", + id=PUBLIC_ID_2, + status=SimpleNamespace(name="Reserved"), + ), + ) + + +def test_resolve_spec_rejects_prefix_missing_required_tag(monkeypatch): + with pytest.raises(ConfigError, match="missing tags"): + _resolve( + monkeypatch, + _single_ref_spec(), + _prefix("10.0.0.0/24", tags=[]), + ) + + +# --------------------------------------------------------------------------- +# Field extraction across the real pynautobot Record shapes (see live data) +# --------------------------------------------------------------------------- + + +class _ChoiceRecord: + """Mimics a pynautobot choice field: {value, label}, no name.""" + + def __init__(self, value: str, label: str) -> None: + self.value = value + self.label = label + self.name = None + + def __str__(self) -> str: # pynautobot uses display/name/label + return self.label + + +class _DisplayRecord: + """Mimics a pynautobot tag/status Record whose text is only via str().""" + + def __init__(self, display: str) -> None: + self._display = display + self.name = None + self.value = None + + def __str__(self) -> str: + return self._display + + +def test_field_text_reads_choice_value_not_label(): + """Type comes back as {value: 'pool', label: 'Pool'}; match the value.""" + assert nautobot._field_text(_ChoiceRecord("pool", "Pool")) == "pool" + + +def test_field_text_falls_back_to_str_for_display_only_records(): + """Tags expose their value only through str(); _field_text must use it.""" + assert nautobot._field_text(_DisplayRecord("openstack-subnet-pool")) == ( + "openstack-subnet-pool" + ) + + +def test_resolve_spec_accepts_choice_type_and_display_tags(monkeypatch): + """End-to-end guardrails pass against the live Record shapes. + + Reproduces the shapes the live query returns: type as a value/label choice, + tags whose value is only reachable via str(). Regression for the guardrails + reporting spurious 'type is None' / 'missing tags' failures. + """ + prefix = _prefix( + "10.0.0.0/24", + id=PUBLIC_ID, + type=_ChoiceRecord("pool", "Pool"), + tags=[_DisplayRecord("openstack-subnet-pool")], + ) + resolved = _resolve(monkeypatch, _single_ref_spec(), prefix) + assert resolved["prefixes"] == ["10.0.0.0/24"] + + +def test_prefix_refs_require_id(): + with pytest.raises(ConfigError, match="prefix_refs\\[0\\].id"): + nautobot._nautobot_prefix_refs({"nautobot": {"prefix_refs": [{}]}}) + + +def test_prefix_refs_must_be_non_empty(): + with pytest.raises(ConfigError, match="prefix_refs must be a non-empty list"): + nautobot._nautobot_prefix_refs({"nautobot": {"prefix_refs": []}}) diff --git a/python/openstack-sync/tests/test_subnet_pools_prune.py b/python/openstack-sync/tests/test_subnet_pools_prune.py new file mode 100644 index 000000000..e4bd0f888 --- /dev/null +++ b/python/openstack-sync/tests/test_subnet_pools_prune.py @@ -0,0 +1,63 @@ +"""Tests for subnet pool prune behaviour.""" + +from __future__ import annotations + +import types +from typing import Any + +from openstack_sync.plugins.neutron.subnet_pools import prune +from openstack_sync.plugins.neutron.subnet_pools.config import OWNERSHIP_TAG + + +class FakeNetwork: + def __init__(self, pools: list[Any]): + self._pools = pools + self.deleted: list[str] = [] + + def subnet_pools(self) -> list[Any]: + return list(self._pools) + + def delete_subnet_pool(self, pool: Any, ignore_missing: bool = True) -> None: + self.deleted.append(pool.id) + self._pools = [item for item in self._pools if item.id != pool.id] + + +def _pool(name: str, *, managed: bool = True) -> Any: + tags = [OWNERSHIP_TAG] if managed else [] + return types.SimpleNamespace(id=f"{name}-id", name=name, tags=tags) + + +def _conn(pools: list[Any]) -> Any: + return types.SimpleNamespace(network=FakeNetwork(pools)) + + +def test_prune_deletes_removed_owned_subnet_pool(): + conn = _conn([_pool("removed-pool"), _pool("kept-pool")]) + + prune.prune_removed_subnet_pools(conn, ["kept-pool"]) + + assert conn.network.deleted == ["removed-pool-id"] + + +def test_prune_keeps_unowned_subnet_pool(): + conn = _conn([_pool("manual-pool", managed=False)]) + + prune.prune_removed_subnet_pools(conn, ["kept-pool"]) + + assert conn.network.deleted == [] + + +def test_prune_keeps_owned_pools_when_desired_list_is_empty(): + conn = _conn([_pool("managed-pool")]) + + prune.prune_removed_subnet_pools(conn, []) + + assert conn.network.deleted == [] + + +def test_prune_deletes_when_empty_desired_is_authoritative(): + conn = _conn([_pool("managed-pool")]) + + prune.prune_removed_subnet_pools(conn, [], authoritative_empty=True) + + assert conn.network.deleted == ["managed-pool-id"] diff --git a/python/openstack-sync/tests/test_subnet_pools_reconcile.py b/python/openstack-sync/tests/test_subnet_pools_reconcile.py new file mode 100644 index 000000000..44f254592 --- /dev/null +++ b/python/openstack-sync/tests/test_subnet_pools_reconcile.py @@ -0,0 +1,408 @@ +"""Tests for Neutron subnet pool reconciliation. + +Nautobot resolution is stubbed here so these tests exercise only the Neutron +convergence path; ``test_subnet_pools_nautobot.py`` covers resolution itself. +""" + +from __future__ import annotations + +import types +from typing import Any +from unittest import mock + +import pytest + +from openstack_sync.plugins.common import ConfigError +from openstack_sync.plugins.neutron.subnet_pools import reconcile +from openstack_sync.plugins.neutron.subnet_pools.config import OWNERSHIP_TAG + + +def _scope( + scope_id: str = "scope-id", + name: str = "scope-a", + ip_version: int = 4, +) -> Any: + return types.SimpleNamespace(id=scope_id, name=name, ip_version=ip_version) + + +def _pool( + pool_id: str = "pool-id", + name: str = "pool-a", + address_scope_id: str = "scope-id", + prefixes: list[str] | None = None, + tags: list[str] | None = None, +) -> Any: + return types.SimpleNamespace( + id=pool_id, + name=name, + address_scope_id=address_scope_id, + project_id="project-a", + prefixes=prefixes or ["10.0.0.0/8"], + ip_version=4, + default_prefix_length=24, + minimum_prefix_length=24, + maximum_prefix_length=28, + description="pool desc", + is_default=False, + is_shared=False, + tags=tags if tags is not None else [OWNERSHIP_TAG], + ) + + +def _spec(**overrides: Any) -> dict[str, Any]: + """A spec already carrying the Nautobot-resolved prefixes and IP version.""" + spec: dict[str, Any] = { + "name": "pool-a", + "project_id": "project-a", + "address_scope": {"name": "scope-a"}, + "prefixes": ["10.0.0.0/8"], + "ip_version": 4, + "default_prefix_length": 24, + "minimum_prefix_length": 24, + "maximum_prefix_length": 28, + "shared": False, + "description": "pool desc", + "tags": ["tenant"], + } + spec.update(overrides) + return spec + + +def _conn(network: Any) -> Any: + return types.SimpleNamespace(network=network) + + +@pytest.fixture(autouse=True) +def _stub_nautobot(monkeypatch: pytest.MonkeyPatch) -> None: + """Bypass Nautobot: return the given spec unchanged (it already resolves).""" + monkeypatch.setattr( + reconcile.nautobot_module, + "resolve_spec", + lambda spec, cache, namespace: spec, + ) + + +def test_sync_subnet_pool_creates_with_resolved_address_scope_and_tags(): + network = mock.MagicMock() + network.address_scopes.return_value = [_scope()] + network.subnet_pools.return_value = [] + created = _pool(tags=[]) + network.create_subnet_pool.return_value = created + conn = _conn(network) + + reconcile.sync_subnet_pool(conn, _spec(), "openstack", {}) + + create_kwargs = network.create_subnet_pool.call_args.kwargs + assert create_kwargs["address_scope_id"] == "scope-id" + assert create_kwargs["default_prefix_length"] == 24 + assert create_kwargs["minimum_prefix_length"] == 24 + assert create_kwargs["maximum_prefix_length"] == 28 + assert create_kwargs["is_shared"] is False + assert "ip_version" not in create_kwargs + network.set_tags.assert_called_once_with(created, ["tenant", OWNERSHIP_TAG]) + + +def test_sync_subnet_pool_creates_without_address_scope_when_absent(): + network = mock.MagicMock() + network.subnet_pools.return_value = [] + created = _pool(tags=[], address_scope_id=None) + network.create_subnet_pool.return_value = created + conn = _conn(network) + + reconcile.sync_subnet_pool(conn, _spec(address_scope=None), "openstack", {}) + + create_kwargs = network.create_subnet_pool.call_args.kwargs + assert "address_scope_id" not in create_kwargs + network.address_scopes.assert_not_called() + + +def test_sync_subnet_pool_updates_mutable_drift(): + existing = _pool( + address_scope_id="old-scope", + prefixes=["10.1.0.0/16"], + tags=[OWNERSHIP_TAG], + ) + updated = _pool(tags=[OWNERSHIP_TAG, "tenant"]) + network = mock.MagicMock() + network.address_scopes.return_value = [_scope()] + network.subnet_pools.return_value = [existing] + network.update_subnet_pool.return_value = updated + conn = _conn(network) + + reconcile.sync_subnet_pool(conn, _spec(), "openstack", {}) + + update_kwargs = network.update_subnet_pool.call_args.kwargs + assert update_kwargs["address_scope_id"] == "scope-id" + assert update_kwargs["prefixes"] == ["10.0.0.0/8"] + network.set_tags.assert_not_called() + + +def test_sync_adopts_ansible_created_pool_without_shrinking(): + """Adopt a pool the retired Ansible role created. + + Such a pool is untagged, has no address scope, and is shared. Adoption must + stamp the ownership tag and add the address scope without treating the + existing prefixes as a shrink. + """ + existing = _pool( + address_scope_id=None, + prefixes=["10.0.0.0/8"], + tags=[], + ) + existing.is_shared = True + updated = _pool(tags=[OWNERSHIP_TAG]) + network = mock.MagicMock() + network.address_scopes.return_value = [_scope()] + network.subnet_pools.return_value = [existing] + network.update_subnet_pool.return_value = updated + conn = _conn(network) + + notes = reconcile.sync_subnet_pool(conn, _spec(shared=True), "openstack", {}) + + assert notes == [] + update_kwargs = network.update_subnet_pool.call_args.kwargs + assert update_kwargs["address_scope_id"] == "scope-id" + assert "prefixes" not in update_kwargs # unchanged, so not pushed + network.set_tags.assert_called_once_with(updated, ["tenant", OWNERSHIP_TAG]) + + +def test_sync_reports_note_and_keeps_prefixes_when_pool_would_shrink(): + """Report a note instead of failing when a pool would shrink. + + Removing a prefix reference cannot be reconciled: Neutron forbids shrinking + a pool. The reconcile keeps the prefixes and reports a note. + """ + existing = _pool( + prefixes=["10.0.0.0/8", "192.0.2.0/24"], + tags=[OWNERSHIP_TAG], + ) + network = mock.MagicMock() + network.address_scopes.return_value = [_scope()] + network.subnet_pools.return_value = [existing] + network.update_subnet_pool.return_value = existing + conn = _conn(network) + + # Spec now only wants 10.0.0.0/8, dropping 192.0.2.0/24. + notes = reconcile.sync_subnet_pool(conn, _spec(prefixes=["10.0.0.0/8"]), "ns", {}) + + assert len(notes) == 1 + assert "192.0.2.0/24" in notes[0] + if network.update_subnet_pool.called: + assert "prefixes" not in network.update_subnet_pool.call_args.kwargs + + +def test_ensure_address_scope_by_id_uses_exact_lookup(): + network = mock.MagicMock() + network.get_address_scope.return_value = _scope(scope_id="scope-by-id") + conn = _conn(network) + + scope = reconcile.ensure_address_scope( + conn, + _spec(address_scope={"id": "scope-by-id"}), + 4, + ) + + assert scope.id == "scope-by-id" + network.get_address_scope.assert_called_once_with("scope-by-id") + network.address_scopes.assert_not_called() + network.create_address_scope.assert_not_called() + + +def test_ensure_address_scope_returns_none_when_absent(): + network = mock.MagicMock() + conn = _conn(network) + + assert reconcile.ensure_address_scope(conn, _spec(address_scope=None), 4) is None + network.get_address_scope.assert_not_called() + network.address_scopes.assert_not_called() + network.create_address_scope.assert_not_called() + + +def test_ensure_address_scope_rejects_ambiguous_name(): + network = mock.MagicMock() + network.address_scopes.return_value = [ + _scope(scope_id="scope-a"), + _scope(scope_id="scope-b"), + ] + conn = _conn(network) + + with pytest.raises(ConfigError, match="matched 2 scopes"): + reconcile.ensure_address_scope(conn, _spec(), 4) + + +def test_ensure_address_scope_adopts_existing_named_scope(): + """A scope that already exists is reused, never recreated.""" + network = mock.MagicMock() + network.address_scopes.return_value = [_scope(scope_id="existing")] + conn = _conn(network) + + scope = reconcile.ensure_address_scope(conn, _spec(), 4) + + assert scope.id == "existing" + network.create_address_scope.assert_not_called() + + +def test_ensure_address_scope_creates_named_scope_when_absent(): + """A greenfield pool creates its address scope, shared, in the pool family.""" + network = mock.MagicMock() + network.address_scopes.return_value = [] + network.create_address_scope.return_value = _scope(scope_id="created") + conn = _conn(network) + + scope = reconcile.ensure_address_scope( + conn, _spec(address_scope={"name": "scope-a", "shared": True}), 4 + ) + + assert scope.id == "created" + create_kwargs = network.create_address_scope.call_args.kwargs + assert create_kwargs["name"] == "scope-a" + assert create_kwargs["ip_version"] == 4 + assert create_kwargs["is_shared"] is True + + +def test_ensure_address_scope_creates_scope_in_pool_ip_family(): + """The created scope's ip_version follows the pool, not a fixed default.""" + network = mock.MagicMock() + network.address_scopes.return_value = [] + network.create_address_scope.return_value = _scope( + scope_id="created6", ip_version=6 + ) + conn = _conn(network) + + reconcile.ensure_address_scope(conn, _spec(), 6) + + assert network.create_address_scope.call_args.kwargs["ip_version"] == 6 + + +def test_ensure_address_scope_create_false_requires_existing_scope(): + """create: false resolves only, failing when the scope is absent.""" + network = mock.MagicMock() + network.address_scopes.return_value = [] + conn = _conn(network) + + with pytest.raises(ConfigError, match="create is false"): + reconcile.ensure_address_scope( + conn, _spec(address_scope={"name": "scope-a", "create": False}), 4 + ) + network.create_address_scope.assert_not_called() + + +def test_sync_subnet_pool_creates_address_scope_then_links_it(): + """End to end: a missing scope is created and its id lands on the pool.""" + network = mock.MagicMock() + network.address_scopes.return_value = [] + network.create_address_scope.return_value = _scope(scope_id="new-scope") + network.subnet_pools.return_value = [] + network.create_subnet_pool.return_value = _pool(tags=[]) + conn = _conn(network) + + reconcile.sync_subnet_pool(conn, _spec(), "openstack", {}) + + network.create_address_scope.assert_called_once() + assert ( + network.create_subnet_pool.call_args.kwargs["address_scope_id"] == "new-scope" + ) + + +def test_validate_prefixes_rejects_mixed_families(): + with pytest.raises(ConfigError, match="same IP version"): + reconcile.validate_prefixes(_spec(prefixes=["10.0.0.0/8", "fd00::/48"])) + + +def test_validate_prefixes_rejects_length_exceeding_family_max(): + with pytest.raises(ConfigError, match="maximum prefix length 32"): + reconcile.validate_prefixes(_spec(maximum_prefix_length=64)) + + +def test_validate_prefixes_requires_all_prefix_lengths(): + """Reject a pool with no prefix lengths. + + Rather than leave it to Neutron's permissive defaults, the operator + requires all three bounds. + """ + spec = _spec() + del spec["default_prefix_length"] + del spec["minimum_prefix_length"] + del spec["maximum_prefix_length"] + with pytest.raises(ConfigError, match="missing prefix length"): + reconcile.validate_prefixes(spec) + + +def test_validate_prefixes_rejects_min_greater_than_max(): + with pytest.raises(ConfigError, match="minimum_prefix_length must be less"): + reconcile.validate_prefixes( + _spec(minimum_prefix_length=28, maximum_prefix_length=24) + ) + + +# --------------------------------------------------------------------------- +# is_default single-per-family guard +# --------------------------------------------------------------------------- + + +def _routing_network(*, by_name: list[Any], default_holders: list[Any]) -> Any: + """A network mock that answers subnet_pools() by query kind. + + find_subnet_pool queries by name; the default-conflict check queries by + is_default + ip_version. Route each to its own list so one call does not + leak into the other. + """ + network = mock.MagicMock() + network.address_scopes.return_value = [_scope()] + + def subnet_pools(**kwargs: Any) -> list[Any]: + if kwargs.get("is_default"): + return list(default_holders) + return list(by_name) + + network.subnet_pools.side_effect = subnet_pools + return network + + +def test_sync_rejects_second_default_for_same_family(): + """A CR asking to be default when another pool already is fails clearly.""" + other_default = _pool(pool_id="other-id", name="other-default") + other_default.is_default = True + network = _routing_network(by_name=[], default_holders=[other_default]) + network.create_subnet_pool.return_value = _pool(tags=[]) + conn = _conn(network) + + with pytest.raises(ConfigError, match="already the default for that family"): + reconcile.sync_subnet_pool(conn, _spec(is_default=True), "openstack", {}) + + network.create_subnet_pool.assert_not_called() + + +def test_sync_allows_default_when_no_other_default_exists(): + network = _routing_network(by_name=[], default_holders=[]) + created = _pool(tags=[]) + network.create_subnet_pool.return_value = created + conn = _conn(network) + + reconcile.sync_subnet_pool(conn, _spec(is_default=True), "openstack", {}) + + assert network.create_subnet_pool.call_args.kwargs["is_default"] is True + + +def test_sync_default_does_not_conflict_with_itself(): + """The pool this CR already manages is exempt from the default check.""" + existing = _pool(prefixes=["10.0.0.0/8"], tags=[OWNERSHIP_TAG]) + existing.is_default = True + network = _routing_network(by_name=[existing], default_holders=[existing]) + network.update_subnet_pool.return_value = existing + conn = _conn(network) + + # Must not raise: the only default holder is the pool we manage. + reconcile.sync_subnet_pool(conn, _spec(is_default=True), "openstack", {}) + + +def test_sync_skips_default_check_when_not_default(): + """is_default=false must not query for existing defaults at all.""" + network = _routing_network(by_name=[], default_holders=[_pool()]) + network.create_subnet_pool.return_value = _pool(tags=[]) + conn = _conn(network) + + reconcile.sync_subnet_pool(conn, _spec(), "openstack", {}) + + for call in network.subnet_pools.call_args_list: + assert not call.kwargs.get("is_default") diff --git a/python/openstack-sync/uv.lock b/python/openstack-sync/uv.lock index 25f14b1bf..299abc63c 100644 --- a/python/openstack-sync/uv.lock +++ b/python/openstack-sync/uv.lock @@ -1005,6 +1005,7 @@ source = { editable = "." } dependencies = [ { name = "kubernetes" }, { name = "openstacksdk" }, + { name = "pynautobot" }, { name = "python-ironicclient" }, { name = "python-openstackclient" }, ] @@ -1020,6 +1021,7 @@ test = [ requires-dist = [ { name = "kubernetes", specifier = ">=32.0.0" }, { name = "openstacksdk", specifier = ">=4.18.0" }, + { name = "pynautobot", specifier = ">=3,<4" }, { name = "python-ironicclient", specifier = ">=6.2.0" }, { name = "python-openstackclient", specifier = ">=10.2.1" }, ] @@ -1382,6 +1384,20 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/f4/7e/a72dd26f3b0f4f2bf1dd8923c85f7ceb43172af56d63c7383eb62b332364/pygments-2.20.0-py3-none-any.whl", hash = "sha256:81a9e26dd42fd28a23a2d169d86d7ac03b46e2f8b59ed4698fb4785f946d0176", size = 1231151, upload-time = "2026-03-29T13:29:30.038Z" }, ] +[[package]] +name = "pynautobot" +version = "3.1.1" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "packaging" }, + { name = "requests" }, + { name = "urllib3" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/f4/48/7d9e65090a124a456ef6fae9e8515af92edeb6ddd52046f12063322d0918/pynautobot-3.1.1.tar.gz", hash = "sha256:8e3f167bd7a07659d0fb1a9cbfb3eb6e7a5375420872d3c08754e4dec5cb0bc1", size = 33440, upload-time = "2026-07-06T20:52:51.484Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/9f/6b/5561a21b6a922bca782d787015282d8be39402ad1e0cf51131e266b2cb6f/pynautobot-3.1.1-py3-none-any.whl", hash = "sha256:81bebe7aaaa2391886386821ec8dcba23add32a9d49e2719bb20e64c77b23a01", size = 41884, upload-time = "2026-07-06T20:52:50.127Z" }, +] + [[package]] name = "pyparsing" version = "3.3.2"