Merge pull request #23 from cooperwalbrun/better-logging

Indicate the VPCs to Which CIDR Omissions Correspond in break_down_to_desired_prefix

CHANGELOG.md

@@ -9,6 +9,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 Nothing currently!
 
+## v0.4.1 - 2023-10-18
+
+### Changed
+
+* Messages relating to omitted prefixes during the handling of the `--prefix` argument will now
+  indicate the VPC to which the omission corresponds (by
+  [@cooperwalbrun](https://github.com/cooperwalbrun))
+
 ## v0.4.0 - 2023-10-18
 
 ### Added

src/aws_cidr_finder/__main__.py

@@ -83,7 +83,7 @@ def main() -> None:
         # yapf: enable
         if arguments.get("prefix") is not None:
             converted_cidrs, m = core.break_down_to_desired_prefix(
-                subnet_cidr_gaps[vpc], arguments["prefix"]
+                vpc.readable_name, subnet_cidr_gaps[vpc], arguments["prefix"]
             )
             subnet_cidr_gaps[vpc] = converted_cidrs
             messages += m

src/aws_cidr_finder/core.py

@@ -84,7 +84,9 @@ def _is_cidr_inside(parent_cidr: str, child_cidr: str) -> bool:
 
 def sort_cidrs(cidrs: list[str]) -> list[str]:
     ret = cidrs.copy()
-    ret.sort(key=cmp_to_key(lambda a, b: ip_network(a).compare_networks(ip_network(b))))  # type: ignore
+    ret.sort(
+        key=cmp_to_key(lambda a, b: ip_network(a).compare_networks(ip_network(b)))  # type: ignore
+    )
     return ret
 
 
@@ -141,22 +143,23 @@ def find_subnet_holes(vpc_cidr: str, subnet_cidrs: list[str]) -> list[str]:
     return sort_cidrs(ret)
 
 
-def break_down_to_desired_prefix(cidrs: list[str], prefix: int) -> tuple[list[str], list[str]]:
+def break_down_to_desired_prefix(readable_vpc_name: str, cidrs: list[str],
+                                 prefix: int) -> tuple[list[str], list[str]]:
     ret = []
     messages = []
     for cidr in cidrs:
         old_prefix = get_prefix(cidr)
         if old_prefix > prefix:
             messages.append((
-                f"Note: skipping the CIDR '{cidr}' because its prefix ({old_prefix}) is "
-                f"numerically greater than the requested prefix ({prefix})"
+                f"Note: skipping the CIDR '{cidr}' in the VPC '{readable_vpc_name}' because its "
+                f"prefix ({old_prefix}) is numerically greater than the requested prefix ({prefix})"
             ))
             continue
         elif prefix - old_prefix > 8:
             messages.append((
-                f"Warning: skipping the CIDR '{cidr}' because its prefix is only {old_prefix} "
-                f"and converting it to a list of CIDRs with a prefix of {prefix} will result "
-                f"in a list containing {2**(prefix-old_prefix)} CIDRs!"
+                f"Warning: skipping the CIDR '{cidr}' in the VPC '{readable_vpc_name}' because its "
+                f"prefix is only {old_prefix} and converting it to a list of CIDRs whose prefixes "
+                f"are {prefix} will result in a list containing {2**(prefix-old_prefix)} CIDRs!"
             ))
             continue
 

tests/test___main__.py

@@ -13,10 +13,10 @@ def test_main_no_arguments(mocker: MockerFixture) -> None:
     mocker.patch("aws_cidr_finder.__main__._get_arguments", return_value=[])
     print_mock: MagicMock = mocker.patch("builtins.print")
 
-    with pytest.raises(SystemExit) as wrapped_exit_code:
+    with pytest.raises(SystemExit) as wrapped_system_exit:
         __main__.main()
 
-    assert wrapped_exit_code.value.code == 1
+    assert wrapped_system_exit.value.code == 1
     print_mock.assert_has_calls([
         call((
             "You must specify either a profile or an access keypair via the environment variables "
@@ -29,18 +29,17 @@ def test_main_no_arguments(mocker: MockerFixture) -> None:
 def test_main_only_profile_argument(mocker: MockerFixture) -> None:
     boto_wrapper_mock = MagicMock()
     boto_wrapper_mock.get_vpc_data = lambda ipv6: [
-        VPC(id="test", name="test-vpc", cidrs=["172.31.0.0/19"], subnets=["172.31.0.0/20"])
+        VPC(id="test1", name="test-vpc1", cidrs=["172.31.0.0/19"], subnets=["172.31.0.0/20"])
     ]
     mocker.patch("aws_cidr_finder.__main__.BotoWrapper", return_value=boto_wrapper_mock)
-    mocker.patch("aws_cidr_finder.__main__.BotoWrapper", return_value=boto_wrapper_mock)
     mocker.patch("aws_cidr_finder.__main__._get_arguments", return_value=["--profile", "test"])
     print_mock: MagicMock = mocker.patch("builtins.print")
 
     __main__.main()
 
     print_mock.assert_has_calls([
         call((
-            "Here are the available CIDR blocks in the 'test-vpc' VPC (VPC CIDR block "
+            "Here are the available CIDR blocks in the 'test-vpc1' VPC (VPC CIDR block "
             "'172.31.0.0/19'):"
         )),
         call(tabulate([["172.31.16.0/20", 4096], ["Total", 4096]], headers=["CIDR", "IP Count"]))
@@ -50,10 +49,10 @@ def test_main_only_profile_argument(mocker: MockerFixture) -> None:
 def test_main_json_output(mocker: MockerFixture) -> None:
     boto_wrapper_mock = MagicMock()
     boto_wrapper_mock.get_vpc_data = lambda ipv6: [
-        VPC(id="test", name="test-vpc", cidrs=["172.31.0.0/19"], subnets=["172.31.0.0/20"])
+        VPC(id="test1", name="test-vpc1", cidrs=["172.31.0.0/19"], subnets=["172.31.0.0/20"]),
+        VPC(id="test2", name="test-vpc2", cidrs=["172.31.32.0/20"], subnets=["172.31.32.0/21"])
     ]
     mocker.patch("aws_cidr_finder.__main__.BotoWrapper", return_value=boto_wrapper_mock)
-    mocker.patch("aws_cidr_finder.__main__.BotoWrapper", return_value=boto_wrapper_mock)
     mocker.patch(
         "aws_cidr_finder.__main__._get_arguments",
         return_value=["--profile", "test", "--json", "--prefix", "20"]
@@ -65,10 +64,16 @@ def test_main_json_output(mocker: MockerFixture) -> None:
     print_mock.assert_has_calls([
         call(
             json.dumps({
-                "aws-cidr-finder-messages": [],
+                "aws-cidr-finder-messages": [(
+                    "Note: skipping the CIDR '172.31.40.0/21' in the VPC 'test-vpc2' because its "
+                    "prefix (21) is numerically greater than the requested prefix (20)"
+                )],
                 "vpcs": {
-                    "test-vpc": {
+                    "test-vpc1": {
                         "172.31.0.0/19": ["172.31.16.0/20"]
+                    },
+                    "test-vpc2": {
+                        "172.31.32.0/20": []
                     }
                 }
             })

tests/test_core_ipv4.py

@@ -3,9 +3,9 @@
 from aws_cidr_finder import core
 
 
-def _assert_lists_equal(actual: list[Any], expected: list[Any]) -> None:
-    assert len(actual) == len(expected)
-    assert set(actual) == set(expected)
+def _assert_lists_equal(expected: list[Any], actual: list[Any]) -> None:
+    assert len(expected) == len(actual)
+    assert set(expected) == set(actual)
 
 
 def test_get_first_ip_in_next_cidr() -> None:
@@ -88,30 +88,35 @@ def test_break_down_to_desired_prefix() -> None:
     test_cases = [
         # Test 1
         ([], 7,
-         []),
+         [], []),
         # Test 2
         (["0.0.0.0/0"], 2,
-         ["0.0.0.0/2", "64.0.0.0/2", "128.0.0.0/2", "192.0.0.0/2"]),
-        # Test 3
+         ["0.0.0.0/2", "64.0.0.0/2", "128.0.0.0/2", "192.0.0.0/2"], []),
+        # Test 3 - requesting a prefix whose numeric value is greater than an input CIDR's prefix
         (["172.31.96.0/19", "172.31.128.0/17"], 17,
-         ["172.31.128.0/17"]),
-        # Test 4 - tests requesting a prefix that would result in too many CIDRs
+         ["172.31.128.0/17"], [
+            "Note: skipping the CIDR '172.31.96.0/19' in the VPC 'test' because its prefix (19) is numerically greater than the requested prefix (17)"
+         ]),
+        # Test 4 - requesting a prefix that would result in too many CIDRs
         (["172.31.96.0/16"], 32,
-         []),
+         [], [
+            "Warning: skipping the CIDR '172.31.96.0/16' in the VPC 'test' because its prefix is only 16 and converting it to a list of CIDRs whose prefixes are 32 will result in a list containing 65536 CIDRs!"
+         ]),
         # Test 5
         (["172.31.96.0/19", "172.31.128.0/17"], 12,
-         [])
+         [], [
+            "Note: skipping the CIDR '172.31.96.0/19' in the VPC 'test' because its prefix (19) is numerically greater than the requested prefix (12)",
+            "Note: skipping the CIDR '172.31.128.0/17' in the VPC 'test' because its prefix (17) is numerically greater than the requested prefix (12)"
+         ])
     ]
     # yapf: enable
 
-    for cidrs, prefix, expected in test_cases:
-        actual = core.break_down_to_desired_prefix(cidrs, prefix)
-        _assert_lists_equal(actual[0], expected)
-
-        reversed = cidrs.copy()
-        reversed.reverse()  # To assert that order is irrelevant
-        actual_reverse = core.break_down_to_desired_prefix(reversed, prefix)
-        _assert_lists_equal(actual_reverse[0], expected)
+    for input_cidrs, prefix, expected_cidrs, expected_messages in test_cases:
+        actual_cidrs, actual_messages = core.break_down_to_desired_prefix(
+            "test", input_cidrs, prefix
+        )
+        _assert_lists_equal(expected_cidrs, actual_cidrs)
+        _assert_lists_equal(expected_messages, actual_messages)
 
 
 def test_find_subnet_holes() -> None:
@@ -135,11 +140,6 @@ def test_find_subnet_holes() -> None:
     ]
     # yapf: enable
 
-    for vpc_cidr, cidrs, expected in test_cases:
-        actual = core.find_subnet_holes(vpc_cidr, cidrs)
-        _assert_lists_equal(actual, expected)
-
-        reversed = cidrs.copy()
-        reversed.reverse()  # To assert that order is irrelevant
-        actual_reverse = core.find_subnet_holes(vpc_cidr, reversed)
-        _assert_lists_equal(actual_reverse, expected)
+    for vpc_cidr, input_cidrs, expected_cidrs in test_cases:
+        actual_cidrs = core.find_subnet_holes(vpc_cidr, input_cidrs)
+        _assert_lists_equal(expected_cidrs, actual_cidrs)

tests/test_core_ipv6.py

@@ -3,9 +3,9 @@
 from aws_cidr_finder import core
 
 
-def _assert_lists_equal(actual: list[Any], expected: list[Any]) -> None:
-    assert len(actual) == len(expected)
-    assert set(actual) == set(expected)
+def _assert_lists_equal(expected: list[Any], actual: list[Any]) -> None:
+    assert len(expected) == len(actual)
+    assert set(expected) == set(actual)
 
 
 def test_get_first_ip_in_next_cidr() -> None:
@@ -88,30 +88,35 @@ def test_break_down_to_desired_prefix() -> None:
     test_cases = [
         # Test 1
         ([], 7,
-         []),
+         [], []),
         # Test 2
         (["::/0"], 2,
-         ["::/2", "4000::/2", "8000::/2", "c000::/2"]),
+         ["::/2", "4000::/2", "8000::/2", "c000::/2"], []),
         # Test 3
-        (["::/96", "0:0:0:1::/64"], 64,
-         ["0:0:0:1::/64"]),
-        # Test 4 - tests requesting a prefix that would result in too many CIDRs
+        (["::/96", "0:0:0:1::/90"], 91,
+         ["0:0:0:1::/91", "::1:0:20:0:0/91"], [
+             "Note: skipping the CIDR '::/96' in the VPC 'test' because its prefix (96) is numerically greater than the requested prefix (91)"
+         ]),
+        # Test 4 - requesting a prefix that would result in too many CIDRs
         (["::/32"], 64,
-         []),
-        # Test 5
+         [], [
+             "Warning: skipping the CIDR '::/32' in the VPC 'test' because its prefix is only 32 and converting it to a list of CIDRs whose prefixes are 64 will result in a list containing 4294967296 CIDRs!"
+         ]),
+        # # Test 5
         (["::/64", "0:0:0:1::/65"], 47,
-         [])
+         [], [
+             "Note: skipping the CIDR '::/64' in the VPC 'test' because its prefix (64) is numerically greater than the requested prefix (47)",
+             "Note: skipping the CIDR '0:0:0:1::/65' in the VPC 'test' because its prefix (65) is numerically greater than the requested prefix (47)"
+         ])
     ]
     # yapf: enable
 
-    for cidrs, prefix, expected in test_cases:
-        actual = core.break_down_to_desired_prefix(cidrs, prefix)
-        _assert_lists_equal(actual[0], expected)
-
-        reversed = cidrs.copy()
-        reversed.reverse()  # To assert that order is irrelevant
-        actual_reverse = core.break_down_to_desired_prefix(reversed, prefix)
-        _assert_lists_equal(actual_reverse[0], expected)
+    for input_cidrs, prefix, expected_cidrs, expected_messages in test_cases:
+        actual_cidrs, actual_messages = core.break_down_to_desired_prefix(
+            "test", input_cidrs, prefix
+        )
+        _assert_lists_equal(expected_cidrs, actual_cidrs)
+        _assert_lists_equal(expected_messages, actual_messages)
 
 
 def test_find_subnet_holes() -> None:
@@ -134,11 +139,6 @@ def test_find_subnet_holes() -> None:
     ]
     # yapf: enable
 
-    for vpc_cidr, cidrs, expected in test_cases:
-        actual = core.find_subnet_holes(vpc_cidr, cidrs)
-        _assert_lists_equal(actual, expected)
-
-        reversed = cidrs.copy()
-        reversed.reverse()  # To assert that order is irrelevant
-        actual_reverse = core.find_subnet_holes(vpc_cidr, reversed)
-        _assert_lists_equal(actual_reverse, expected)
+    for vpc_cidr, input_cidrs, expected_cidrs in test_cases:
+        actual_cidrs = core.find_subnet_holes(vpc_cidr, input_cidrs)
+        _assert_lists_equal(expected_cidrs, actual_cidrs)