diff --git a/docs/source_aws.md b/docs/source_aws.md index 91b9071a..f4b4cf06 100644 --- a/docs/source_aws.md +++ b/docs/source_aws.md @@ -104,6 +104,16 @@ For example, the following supplemental policy enables all three: } ``` +The policy includes `s3:GetBucketTagging` for optional S3 tag synchronization. +If bucket tagging is desired, grant this permission to the inventory role used +by your AWS profile (or every selected member-account role in organization mode) +and configure `tag_keys` to select the tags to copy to NetBox. + +**Current S3 discovery always requests bucket tags**, even when `tag_keys` is +empty. Omitting `s3:GetBucketTagging` can therefore cause an authorization denial +that skips the affected bucket and postpones orphaning/pruning for this source, +as described below; it does not simply disable tag synchronization. + Where your IAM policy supports resource-level permissions, restrict the resource-specific actions to the intended buckets, functions, and EKS resources. Listing actions still need their documented wildcard scope. No S3 object reads, diff --git a/module/netbox/object_classes.py b/module/netbox/object_classes.py index c382d326..073acea6 100644 --- a/module/netbox/object_classes.py +++ b/module/netbox/object_classes.py @@ -862,7 +862,6 @@ def get_display_name(self, data=None, including_second_key=False): [enforce_secondary_key, including_second_key, include_secondary_key_if_present]: secondary_key_value = this_data_set.get(secondary_key) - org_secondary_key_value = str(secondary_key_value) read_from_netbox = False if isinstance(secondary_key_value, NetBoxObject): @@ -873,6 +872,11 @@ def get_display_name(self, data=None, including_second_key=False): read_from_netbox = True if secondary_key_value.get("id", 0) != 0 else False secondary_key_value = self.get_display_name(data=secondary_key_value) + org_secondary_key_value = str(secondary_key_value) + + if secondary_key_value is None and include_secondary_key_if_present: + return my_name + if secondary_key_value is None and read_from_netbox is False and include_secondary_key_if_present is False: log.warning(f"Unable to determine second key '{secondary_key}' for {self.name} '{my_name}', " f"got: {org_secondary_key_value}") @@ -1935,6 +1939,7 @@ class NBCluster(NetBoxObject): object_type = "virtualization.cluster" primary_key = "name" secondary_key = "scope_id" + include_secondary_key_if_present = True prune = False def __init__(self, *args, **kwargs): diff --git a/module/sources/aws/connection.py b/module/sources/aws/connection.py index 8d139652..7146cc28 100644 --- a/module/sources/aws/connection.py +++ b/module/sources/aws/connection.py @@ -324,8 +324,26 @@ def resource_name(self, name, identity, limit): return f"{name[:limit - 13]}-{suffix}" def service_object(self, object_type, identity, data): - matches = [item for item in self.inventory.get_all_items(object_type) + items = self.inventory.get_all_items(object_type) + matches = [item for item in items if (item.data.get("custom_fields") or {}).get("aws_resource_id") == identity] + parent = "virtual_machine" if object_type is NBVirtualDisk else "cluster" if object_type is NBVM else "group" + if not matches: + name_matches = [item for item in items + if item.data.get("name") == data["name"] and + item.data.get(parent) is data.get(parent)] + if len(name_matches) == 1: + candidate = name_matches[0] + fields = candidate.data.get("custom_fields") or {} + candidate_identity = fields.get("aws_resource_id") + candidate_service = fields.get("aws_service") + identity_missing = candidate_identity is None or ( + isinstance(candidate_identity, str) and not candidate_identity.strip()) + service = (data.get("custom_fields") or {}).get("aws_service") + service_matches = candidate_service in (None, "", service) + source_managed = candidate.source is self or self.source_tag in candidate.get_tags() + if identity_missing and service_matches and source_managed: + matches = [candidate] if len(matches) > 1: raise ValueError(f"Multiple NetBox {object_type.name} objects match AWS resource '{identity}'") if matches: @@ -334,9 +352,8 @@ def service_object(self, object_type, identity, data): raise ValueError(f"AWS resource '{identity}' belongs to another source") item.update(data=data, source=self) else: - parent = "virtual_machine" if object_type is NBVirtualDisk else "cluster" if object_type is NBVM else "group" if any(item.data.get("name") == data["name"] and item.data.get(parent) is data.get(parent) - for item in self.inventory.get_all_items(object_type)): + for item in items): raise ValueError(f"NetBox {object_type.name} name collision for AWS resource '{identity}'") item = self.inventory.add_object(object_type, data=data, source=self) self.reconcile_tags(item, data["tags"]) diff --git a/tests/test_aws_services.py b/tests/test_aws_services.py index 2c7160d3..0fb693af 100644 --- a/tests/test_aws_services.py +++ b/tests/test_aws_services.py @@ -265,10 +265,13 @@ def test_s3_conflict_skips_dependencies_but_continues(make_source, inventory, ca "name": "inventory-bucket", "virtual_machine": holder, "size": 0, }) source.apply() - assert "name collision" in caplog.text + assert ("name collision" in caplog.text) == (stage == "bucket") buckets = [disk.data["name"] for disk in inventory.get_all_items(NBVirtualDisk) if disk.data.get("custom_fields", {}).get("aws_service") == "s3"] - assert buckets == (["second"] if stage == "bucket" else []) + assert buckets == (["second"] if stage == "bucket" else ["inventory-bucket", "second"]) + assert cluster.data["custom_fields"]["aws_resource_id"] == identity + if stage == "holder": + assert holder.data["custom_fields"]["aws_resource_id"] == identity assert any(vm.data.get("custom_fields", {}).get("aws_instance_id") == INSTANCE for vm in inventory.get_all_items(NBVM)) @@ -295,6 +298,56 @@ def test_eks_conflict_skips_workers_and_continues_next_region(make_source, inven assert "i-next-region" in instances +@pytest.mark.parametrize("fields", [ + {}, + {"aws_resource_id": "", "aws_service": ""}, +]) +def test_eks_cluster_with_missing_identity_fields_is_reconciled(make_source, inventory, fields, caplog): + source = make_source("sync_eks = True\nsync_private_ips = False", data=service_data()) + group = inventory.add_object(NBClusterGroup, data={"name": f"AWS {ACCOUNT}"}) + cluster = inventory.add_object(NBCluster, data={ + "name": f"EKS {ACCOUNT}/{REGION}/inventory-cluster", + "group": group, + "custom_fields": fields, + }) + cluster.add_tags(source.source_tag) + + source.apply() + + assert not any("Skipping EKS cluster 'inventory-cluster'" in message + for message in caplog.messages) + assert len([item for item in inventory.get_all_items(NBCluster) + if item.data.get("name") == cluster.data["name"]]) == 1 + assert cluster.data["custom_fields"]["aws_resource_id"] == CLUSTER_ARN + assert cluster.data["custom_fields"]["aws_service"] == "eks" + worker = next(vm for vm in inventory.get_all_items(NBVM) + if vm.data.get("custom_fields", {}).get("aws_instance_id") == INSTANCE) + assert worker.data["cluster"] is cluster + + +@pytest.mark.parametrize("fields", [ + {"aws_resource_id": f"arn:aws:eks:{REGION}:{ACCOUNT}:cluster/another-cluster", "aws_service": "eks"}, + {"aws_resource_id": "", "aws_service": "lambda"}, +]) +def test_eks_cluster_does_not_adopt_conflicting_identity_fields(make_source, inventory, fields, caplog): + source = make_source("sync_eks = True\nsync_private_ips = False", data=service_data()) + source.custom_fields() + group = inventory.add_object(NBClusterGroup, data={"name": f"AWS {ACCOUNT}"}) + cluster = inventory.add_object(NBCluster, data={ + "name": f"EKS {ACCOUNT}/{REGION}/inventory-cluster", + "group": group, + "custom_fields": fields, + }) + cluster.add_tags(source.source_tag) + + source.apply() + + assert "Skipping EKS cluster 'inventory-cluster'" in caplog.text + assert cluster.data["custom_fields"] == fields + assert not any(vm.data.get("custom_fields", {}).get("aws_instance_id") == INSTANCE + for vm in inventory.get_all_items(NBVM)) + + def test_lambda_function_conflict_continues_next_function(make_source, inventory, caplog): data = service_data() source = make_source("sync_lambda = True\nsync_private_ips = False", data=data) diff --git a/tests/test_cluster_scope.py b/tests/test_cluster_scope.py index a43eb4a6..b609242f 100644 --- a/tests/test_cluster_scope.py +++ b/tests/test_cluster_scope.py @@ -13,10 +13,11 @@ that object again, otherwise every run reports a change. """ import logging +from datetime import datetime import pytest -from module.netbox.object_classes import NBCluster, NBClusterType, NBSite, NBSiteGroup +from module.netbox.object_classes import NBCluster, NBClusterType, NBVM, NBSite, NBSiteGroup def test_cluster_scope_is_resolved_from_an_id(inventory): @@ -46,6 +47,23 @@ def test_a_site_object_survives_resolving(inventory): assert cluster.data.get("scope_type") == "dcim.site", "scope type was dropped" +def test_unscoped_cluster_has_no_missing_scope_warning(inventory, caplog): + cluster = inventory.add_object(NBCluster, data={"name": "global"}) + + assert cluster.get_display_name(including_second_key=True) == "global" + assert "Unable to determine second key 'scope_id'" not in caplog.text + + +def test_vm_lookup_does_not_serialize_cluster_metadata(inventory): + cluster = inventory.add_object(NBCluster, data={ + "name": "eks-cluster", + "custom_fields": {"aws_metadata": {"createdAt": datetime(2026, 10, 23)}}, + }) + vm = inventory.add_object(NBVM, data={"name": "worker", "cluster": cluster}) + + assert inventory.get_by_data(NBVM, data={"name": "worker", "cluster": cluster}) is vm + + def test_source_scopes_a_cluster_with_the_site_object(vcsim, inventory, load_config, vmware_settings): """A dict here is sent to NetBox as is and rejected with 'scope_id: A valid integer is required.'""" from module.sources import instantiate_sources