Jeff feedback

docs/user/lib_use_cases_acl.md

@@ -2,7 +2,7 @@
 
 The ACL classes are intended to help guide the ACL conversation. It is not intended to solve every ACL challenge you may have. In essence, it provides sane defaults and welcomes you to extend the logic via supported extension mechanisms. Three patterns that heavily make up the capabilities are:
 
-- Expanding data to the ["Cartesian product"](#cartesian-product) (or combination) of each rule, so that each product can be easily evaluated.
+- Expanding data to the ["Cartesian product"](#cartesian-product) (or permutations) of each rule, so that each product can be easily evaluated.
 - Providing a `f"{type}_*` method pattern, to dynamically find any `validate_*` or `enforce_*` method you provide.
 - Providing a `f"{type}_{attr}` method pattern, to dynamically find any `process_{attr}` or `match_{attr}` method you provide for the given attrs.
 
@@ -12,17 +12,19 @@ Here you can see how the Python classes work together. There is a lot going on,
 
 ![ACL Classes](../images/acl-workflow.png)
 
-> It may be helpful to open the diagram in a new tab to view the full size, as an example, in Chrome you can right-click on the image and select "Open Image on New Tab".
+!!! info
+    It may be helpful to open the diagram in a new tab to view the full size, as an example, in Chrome you can right-click on the image and select "Open Image on New Tab".
 
 The intention of this page is not to cover every attribute and it's behavior, but a more human (although highly technical) understanding of what is going on. For more detailed information, please see the [test](https://github.com/networktocode/netutils/blob/develop/tests/unit/test_acl.py) and [code docs](../../dev/code_reference/acl/).
 
-> In the future the intention is to add features such as better de-duplication, partial match, and path analysis.
+!!! info
+    In the future the intention is to add features such as better de-duplication, partial match, and path analysis.
 
 ## Core Concepts
 
 ### Cartesian Product
 
-This ["Cartesian product"](https://en.wikipedia.org/wiki/Cartesian_product) concept is used throughout the page, so I thought it would be good to review. In this example, we have a single `rule`, and like many rules, it has multiple sources, destinations, and protocols. The `_cartesian_product` function creates the combinations, each of which is technically called a product.
+This ["Cartesian product"](https://en.wikipedia.org/wiki/Cartesian_product) concept is used throughout the page, so I thought it would be good to review. In this example, we have a single `rule`, and like many rules, it has multiple sources, destinations, and protocols. The `_cartesian_product` function creates the permutations, each of which is technically called a product.
 
 
 ```python
@@ -50,7 +52,7 @@ This ["Cartesian product"](https://en.wikipedia.org/wiki/Cartesian_product) conc
 
 Now that you have the Cartesian products, you can evaluate each one individually. In this example perhaps '192.168.0.0/24' -> '172.16.0.0/16' is allowed, but '192.168.0.0/24' -> '192.168.250.10-192.168.250.20' is denied. Yet another example is the access could be allowed on both sources and destinations IPs, but not for udp/53 (DNS).
 
-Having the ability to look at each product individually allows you to only have to worry about the check you wish to create, versus custom logic that attempts to understand the combinations. Building a `is_pci_to_non_pci` becomes trivial when looking at each product. This idea applies to validating, enforcing, matching, etc.. 
+Having the ability to look at each product individually allows you to only have to worry about the check you wish to create, versus custom logic that attempts to understand the permutations. Building a `is_pci_to_non_pci` becomes trivial when looking at each product. This idea applies to validating, enforcing, matching, etc.
 
 
 ### Dynamic Method - Attrs 
@@ -71,7 +73,7 @@ The `ACLRule` class at a high level:
 - Is built with extensibility in mind to allow you to customize how your business operates.
 - It contains a list of attributes such as "name," "src_ip," "src_zone," "dst_ip," "dst_port," "dst_zone," and "action" that are commonly used to work with ACLs.
 - Provides the ability for you to expand data such as converting an address-group name into detailed addresses with your custom code.
-- Provides the Cartesian product (or combinations) of the rules to make evaluation simple.
+- Provides the Cartesian product (or permutations) of the rules to make evaluation simple.
 - It provides options for verifying input data and result data (data expanded) with corresponding JSON schemas.
 - Provides the ability to validate data, generally for tech feasibility testing such as "are IPs on our network and in IPAM" or "is NAT IP provided vs actual IP".
 - Provides the ability to enforce data, generally for security testing such as "is PCI attempting to talk with a non-PCI environment" or "is IP range not narrowly scoped enough".
@@ -80,9 +82,9 @@ The `ACLRule` class at a high level:
 
 ### Initialization & Loading Data
 
-The initialization process calls on the `load_data` method. This on a high level verifies schema of initial data, allows you to process data (e.g. convert tcp/https -> 80/443), expand data, determine Cartesian product (or combinations) of the firewall rule (traditionally 5-tuple), and verifies schema of result data.
+The initialization process calls on the `load_data` method. This on a high level verifies schema of initial data, allows you to process data (e.g. convert tcp/https -> 80/443), expand data, determine Cartesian product (or permutations) of the firewall rule (traditionally 5-tuple), and verifies schema of result data.
 
-The Cartesian product (or combination) is key to the functionality of other steps, this allows you to evaluate each rule based on the smallest view of the data, so pay close attention to those steps, as it is important to other methods as well.
+The Cartesian product (or permutations) is key to the functionality of other steps, this allows you to evaluate each rule based on the smallest view of the data, so pay close attention to those steps, as it is important to other methods as well.
 
 To provide some ideas on what you may validate:
 
@@ -110,10 +112,10 @@ Here you will find a written understanding of what is happening in the code:
         - This is controlled by the `result_data_verify` attribute which is disabled by default.
     - The `validate` method validating the rule using a series of custom methods starting with `validate_` prefixes.
         - The ordering can be controlled based on the `order_validate` attribute or defaults to all matches.
-    - The rules are expanded into `self.expanded_rules` by creating each combination of the tuple, using a Cartesian product function.
-        - An example combination would be converting source: 10.1.1.1, 10.1.1.2, destination: 10.100.100.100, port: 80 -> 10.1.1.1, destination: 10.100.100.100, port: 80 and 10.1.1.2, destination: 10.100.100.100, port: 80.
-        - This will be key, so that each combination can be compared individually later.
-    - Filter out the combinations that have the same source and destination IP, if `self.filter_same_ip` which is on by default.
+    - The rules are expanded into `self.expanded_rules` by creating each permutations of the tuple, using a Cartesian product function.
+        - An example permutations would be converting source: 10.1.1.1, 10.1.1.2, destination: 10.100.100.100, port: 80 -> 10.1.1.1, destination: 10.100.100.100, port: 80 and 10.1.1.2, destination: 10.100.100.100, port: 80.
+        - This will be key, so that each permutations can be compared individually later.
+    - Filter out the permutations that have the same source and destination IP, if `self.filter_same_ip` which is on by default.
 
 ### Enforce
 
@@ -133,7 +135,7 @@ Here you will find a written understanding of what is happening in the code:
     - The ordering can be controlled based on the `order_enforce` attribute or defaults to all matches.
     - The `enforce_matrix` method enforces ACL rules based on a predefined matrix feature.
         - This is controlled by the `self.matrix_enforced` attribute and is off by default.
-        - The `enforce_matrix` method runs the enforcement checks for each of the `self.expanded_rules` (or combinations of tuples)
+        - The `enforce_matrix` method runs the enforcement checks for each of the `self.expanded_rules` (or permutations of tuples)
         - This matrix definition is very simple and not likely ready to be be used in a production environment, instead used for simple demonstrations and communicating potential ideas.
     - Each method should return a dictionary or list of dictionaries as both of these are handled
         - In the example there is the `obj` and `action` key.
@@ -148,7 +150,7 @@ The `match_details` method provides a verbose way of verifying match details bet
 
 Here you will find a written understanding of what is happening in the code:
 
-- The `self.expanded_rules` is looped over for every combination.
+- The `self.expanded_rules` is looped over for every permutations.
     - For each `self.attrs`, a method name matching `f"match_{attr}"`, (e.g. `match_src_ip()`) is called.
         - This allows you to inherit from and provide your own custom equality check or verify with your business logic.
         - You do not need to have a `f"match_{attr}"` method for every attr, description as example would not be a good candidate to match on.

netutils/acl.py

@@ -94,7 +94,7 @@
 
 
 def _cartesian_product(data: t.Dict[str, str]) -> t.List[t.Dict[str, t.Any]]:
-    """Function to create the Cartesian product/combinations from a data dictionary.
+    """Function to create the Cartesian product/permutations from a data dictionary.
 
     Args:
         data: A dictionary with string keys and either string or list of string values
@@ -245,7 +245,15 @@ def validate(self) -> t.Any:
     def process_dst_port(
         self, dst_port: t.Any
     ) -> t.Union[t.List[str], None]:  # pylint: disable=inconsistent-return-statements
-        """Convert port and protocol information."""
+        """Convert port and protocol information.
+
+        Method supports a single format of `{protocol}/{port}`, and will translate the
+        protocol for all IANA defined protocols. The port will be translated for TCP and
+        UDP ports only. For all other protocols should use port of 0, e.g. `ICMP/0` for ICMP
+        or `50/0` for ESP. Similarly, IANA defines the port mappings, while these are mostly
+        staying unchanged, but sourced from
+        https://www.iana.org/assignments/service-names-port-numbers/service-names-port-numbers.csv.
+        """
         output = []
         if not self.dst_port_process:
             return None
@@ -496,7 +504,7 @@ def load_data(self) -> None:
             self.rules.append(self.class_obj(item))
 
     def match(self, rule: ACLRule) -> str:
-        """Check the rules loaded in `load_data` against a new `rule`.
+        """Check the rules loaded in `load_data` match against a new `rule`.
 
         Args:
             rule: The `ACLRule` rule to test against the list of `ACLRule`s loaded in initiation.
@@ -510,7 +518,7 @@ def match(self, rule: ACLRule) -> str:
         return str(item.deny)  # pylint: disable=undefined-loop-variable
 
     def match_details(self, rule: ACLRule) -> t.Any:
-        """Verbosely check the rules loaded in `load_data` against a new `rule`.
+        """Verbosely check the rules loaded in `load_data` match against a new `rule`.
 
         Args:
             rule: The `ACLRule` rule to test against the list of `ACLRule`s loaded in initiation.

netutils/ip.py

@@ -265,7 +265,7 @@ def is_ip_range(ip_range: str) -> bool:
         return False
     start_ip_obj = ipaddress.ip_address(start_ip)
     end_ip_obj = ipaddress.ip_address(end_ip)
-    if not type(start_ip_obj) == type(end_ip_obj):  # pylint: disable=unidiomatic-typecheck
+    if not isinstance(start_ip_obj, type(end_ip_obj)):
         return False
     # IP version being the same is enforced above, mypy disagrees, can safely ignore
     if not start_ip_obj < end_ip_obj:  # type: ignore
@@ -327,7 +327,10 @@ def _convert_ip(ip: str) -> str:
             item_obj = ipaddress.ip_network(_convert_ip(item))
             item_obj_start = item_obj[0]
             item_obj_end = item_obj[-1]
-        assert type(item_obj_start) == type(item_obj_end)  # nosec # pylint: disable=unidiomatic-typecheck
+        if not isinstance(item_obj_start, type(item_obj_end)):
+            raise ValueError(
+                f"IP range start `{item_obj_start}` and end `{item_obj_end}` IPs must both be same IPVersion."
+            )
         # Use this validation method, since it is consitent with ranges
         # vs the `.subnet_of` method which is not.
         if item_obj_start <= ip_obj_start <= ip_obj_end <= item_obj_end:  # type: ignore