Equivalence query optimized implementation (#524)

* Optimized implementation of EquivalenceQuery.

Signed-off-by: Tanya <[email protected]>

* Align EquivalenceQuery difference explanation according to the original implementation.

Signed-off-by: Tanya <[email protected]>

* Fixing lint errors.

Signed-off-by: Tanya <[email protected]>

* Fix: when src_peers/dst_peers is not active domain, returning the current full domain (according To DimensionManager) rather than None.

Signed-off-by: Tanya <[email protected]>

* Fix the fix: when src_peers/dst_peers is not active domain, returning the current full domain (according To DimensionManager) rather than None.
No more expect src_peers/dst_peers be None. Check for is_active_dim instead.

Signed-off-by: Tanya <[email protected]>

* yet another small fix of the fix.

Signed-off-by: Tanya <[email protected]>

* When computing projection on one dimension, return the full dimension value for inactive dimensions.
Adding check in command-line flow for non-implemented optimized queries.
Optimized equivalence query code refactoring for better readability.

Signed-off-by: Tanya <[email protected]>

* Ignoring 'complex function' lint error.
Returning 'passed' code for skipped queries.

Signed-off-by: Tanya <[email protected]>

* Undo returning 'passed' code for skipped queries.
Skipping non-implemented optimized cmdline queries in run_all_tests

Signed-off-by: Tanya <[email protected]>

* Skipping non-implemented optimized cmdline queries in run_all_tests

Signed-off-by: Tanya <[email protected]>

* Fixed the refined code of optimized equivalence calculation.

Signed-off-by: Tanya <[email protected]>

* Update nca/NetworkConfig/NetworkConfigQuery.py

Co-authored-by: Adi Sosnovich <[email protected]>

* Reused definition of implemented_opt_queries

Signed-off-by: Tanya <[email protected]>

* Fix of reusing definition of implemented_opt_queries

Signed-off-by: Tanya <[email protected]>

* Removed redundant method.

Signed-off-by: Tanya <[email protected]>

---------

Signed-off-by: Tanya <[email protected]>
Co-authored-by: Adi Sosnovich <[email protected]>

Author: tanyaveksler

nca/CoreDS/CanonicalHyperCubeSet.py

@@ -850,3 +850,6 @@ def _apply_layer_elements_union(self):
                 layer_0_new_elem |= elem
             new_layers[layer_0_new_elem] = layer_1_elem
         self.layers = new_layers
+
+    def is_active_dimension(self, dim_name):
+        return dim_name in self.active_dimensions

nca/CoreDS/ConnectivityCube.py

@@ -141,7 +141,7 @@ def __getitem__(self, dim_name):
                 # translate CanonicalIntervalSet back to PeerSet
                 return BasePeerSet().get_peer_set_by_indices(dim_value)
             else:
-                return None
+                return BasePeerSet().get_peer_set_by_indices(DimensionsManager().get_dimension_domain_by_name(dim_name))
         elif dim_name in ["src_ports", "dst_ports"]:
             res = PortSet()
             res.add_ports(dim_value)

nca/CoreDS/ConnectivityProperties.py

@@ -345,7 +345,10 @@ def project_on_one_dimension(self, dim_name):
         """
         assert dim_name not in ["icmp_type", "icmp_code"]  # not supporting icmp dimensions
         if dim_name not in self.active_dimensions:
-            return None
+            if dim_name == "src_peers" or dim_name == "dst_peers":
+                return BasePeerSet().get_peer_set_by_indices(DimensionsManager().get_dimension_domain_by_name(dim_name))
+            else:
+                return DimensionsManager().get_dimension_domain_by_name(dim_name)
         if dim_name == "src_peers" or dim_name == "dst_peers":
             res = PeerSet()
         elif dim_name == "src_ports" or dim_name == "dst_ports":
@@ -397,9 +400,9 @@ def make_conn_props(conn_cube):
 
         src_ports = conn_cube["src_ports"]
         dst_ports = conn_cube["dst_ports"]
-        dst_peers = conn_cube["dst_peers"]
         assert not src_ports.named_ports and not src_ports.excluded_named_ports
-        if (not dst_ports.named_ports and not dst_ports.excluded_named_ports) or not dst_peers:
+        if (not dst_ports.named_ports and not dst_ports.excluded_named_ports) or \
+                not conn_cube.is_active_dim("dst_peers"):
             # Should not resolve named ports
             return ConnectivityProperties._make_conn_props_no_named_ports_resolution(conn_cube)
 
@@ -414,7 +417,7 @@ def make_conn_props(conn_cube):
 
         # Resolving dst named ports
         protocols = conn_cube["protocols"]
-        assert dst_peers
+        dst_peers = conn_cube["dst_peers"]
         for peer in dst_peers:
             real_ports = ConnectivityProperties._resolve_named_ports(dst_ports.named_ports, peer, protocols)
             if real_ports:

nca/NetworkConfig/NetworkConfig.py

@@ -316,3 +316,25 @@ def append_policy_to_config(self, policy):
         :return: None
         """
         self.policies_container.append_policy(policy)
+
+    def filter_conns_by_peer_types(self, conns, all_peers):
+        """
+        Filter the given connections by removing several connection kinds that are never allowed
+        (such as IpBlock to IpBlock connections, connections from DNSEntries, and more).
+        :param ConnectivityProperties conns: the given connections.
+        :param PeerSet all_peers: all peers in the system.
+        :return The resulting connections.
+        :rtype ConnectivityProperties
+        """
+        res = conns
+        # avoid IpBlock -> {IpBlock, DNSEntry} connections
+        all_ips = Peer.IpBlock.get_all_ips_block_peer_set()
+        all_dns_entries = self.peer_container.get_all_dns_entries()
+        ip_to_ip_or_dns_conns = ConnectivityProperties.make_conn_props_from_dict({"src_peers": all_ips,
+                                                                                  "dst_peers": all_ips | all_dns_entries})
+        res -= ip_to_ip_or_dns_conns
+        # avoid DNSEntry->anything connections
+        dns_to_any_conns = ConnectivityProperties.make_conn_props_from_dict({"src_peers": all_dns_entries,
+                                                                             "dst_peers": all_peers})
+        res -= dns_to_any_conns
+        return res

nca/NetworkConfig/NetworkConfigQuery.py

@@ -184,28 +184,6 @@ def execute(self, cmd_line_flag):
     def exec(self):
         raise NotImplementedError
 
-    def filter_conns_by_peer_types(self, conns, all_peers):
-        """
-        Filter the given connections by removing several connection kinds that are never allowed
-        (such as IpBlock to IpBlock connections, connections from DNSEntries, and more).
-        :param ConnectivityProperties conns: the given connections.
-        :param PeerSet all_peers: all peers in the system.
-        :return The resulting connections.
-        :rtype ConnectivityProperties
-        """
-        res = conns
-        # avoid IpBlock -> {IpBlock, DNSEntry} connections
-        all_ips = IpBlock.get_all_ips_block_peer_set()
-        all_dns_entries = self.config.peer_container.get_all_dns_entries()
-        ip_to_ip_or_dns_conns = ConnectivityProperties.make_conn_props_from_dict({"src_peers": all_ips,
-                                                                                  "dst_peers": all_ips | all_dns_entries})
-        res -= ip_to_ip_or_dns_conns
-        # avoid DNSEntry->anything connections
-        dns_to_any_conns = ConnectivityProperties.make_conn_props_from_dict({"src_peers": all_dns_entries,
-                                                                             "dst_peers": all_peers})
-        res -= dns_to_any_conns
-        return res
-
 
 class DisjointnessQuery(NetworkConfigQuery):
     """
@@ -832,7 +810,7 @@ def compute_connectivity_output_optimized(self):
             subset_conns = ConnectivityProperties.make_conn_props_from_dict({"src_peers": subset_peers}) | \
                            ConnectivityProperties.make_conn_props_from_dict({"dst_peers": subset_peers})
             all_conns_opt &= subset_conns
-        all_conns_opt = self.filter_conns_by_peer_types(all_conns_opt, opt_peers_to_compare)
+        all_conns_opt = self.config.filter_conns_by_peer_types(all_conns_opt, opt_peers_to_compare)
         if self.config.policies_container.layers.does_contain_layer(NetworkLayerName.Istio):
             output_res, opt_fw_rules_tcp, opt_fw_rules_non_tcp = \
                 self.get_props_output_split_by_tcp(all_conns_opt, opt_peers_to_compare)
@@ -1185,9 +1163,36 @@ def disjoint_referenced_ip_blocks(self):
         :rtype: PeerSet
         """
         exclude_ipv6 = self.output_config.excludeIPv6Range
+        # TODO - consider including also non referenced IPBlocks, as in ConnectivityMapQuery
+        #  (see issue https://github.com/IBM/network-config-analyzer/issues/522)
         return IpBlock.disjoint_ip_blocks(self.config1.get_referenced_ip_blocks(exclude_ipv6),
                                           self.config2.get_referenced_ip_blocks(exclude_ipv6), exclude_ipv6)
 
+    def filter_conns_by_input_or_internal_constraints(self, conns1, conns2):
+        """
+        Given two allowed connections (in config1 and in config2 respectively), filter those connections
+        according to required IP blocks (external constrain - excludeIPv6Range option) and
+        peer types (internal constraints).
+        :param conns1: the first config allowed connections
+        :param conns2: the second config allowed connections
+        :rtype: [ConnectivityProperties, ConnectivityProperties]
+        :return: two resulting allowed connections
+        """
+        peers_to_compare = conns1.project_on_one_dimension('src_peers') | conns1.project_on_one_dimension('dst_peers') | \
+            conns2.project_on_one_dimension('src_peers') | conns2.project_on_one_dimension('dst_peers')
+        exclude_ipv6 = self.output_config.excludeIPv6Range
+        ref_ip_blocks = self.config1.get_referenced_ip_blocks(exclude_ipv6) | \
+            self.config2.get_referenced_ip_blocks(exclude_ipv6)
+        ip_blocks_mask = IpBlock() if ref_ip_blocks else IpBlock.get_all_ips_block(exclude_ipv6)
+        for ip_block in ref_ip_blocks:
+            ip_blocks_mask |= ip_block
+        peers_to_compare.filter_ipv6_blocks(ip_blocks_mask)
+        conns_filter = ConnectivityProperties.make_conn_props_from_dict({"src_peers": peers_to_compare,
+                                                                         "dst_peers": peers_to_compare})
+        res_conns1 = self.config1.filter_conns_by_peer_types(conns1, peers_to_compare) & conns_filter
+        res_conns2 = self.config2.filter_conns_by_peer_types(conns2, peers_to_compare) & conns_filter
+        return res_conns1, res_conns2
+
     @staticmethod
     def clone_without_ingress(config):
         """
@@ -1227,7 +1232,12 @@ def exec(self, cmd_line_flag=False, layer_name=None):
         if query_answer.output_result:
             query_answer.numerical_result = not query_answer.bool_result
             return query_answer
+        if self.config1.optimized_run == 'false':
+            return self.check_equivalence_original(layer_name)
+        else:
+            return self.check_equivalence_optimized(layer_name)
 
+    def check_equivalence_original(self, layer_name=None):
         peers_to_compare = \
             self.config1.peer_container.get_all_peers_group(include_dns_entries=True)
         peers_to_compare |= self.disjoint_referenced_ip_blocks()
@@ -1252,6 +1262,49 @@ def exec(self, cmd_line_flag=False, layer_name=None):
         return QueryAnswer(True, self.name1 + ' and ' + self.name2 + ' are semantically equivalent.',
                            numerical_result=0)
 
+    def _append_different_conns_to_list(self, conn_props, different_conns_list, props_based_on_config1):
+        """
+        Adds difference between config1 and config2 connectivities into the list of differences
+        :param ConnectivityProperties conn_props: connectivity properties representing a difference between config1 and config2
+        :param list different_conns_list: the list to add differences to
+        :param bool props_based_on_config1: whether conn_props represent connections present in config1 but not in config2
+        (the value True) or connections present in config2 but not in config1 (the value False)
+        """
+        no_conns = ConnectionSet()
+        for cube in conn_props:
+            conn_cube = conn_props.get_connectivity_cube(cube)
+            conns, src_peers, dst_peers = \
+                ConnectionSet.get_connection_set_and_peers_from_cube(conn_cube, self.config1.peer_container)
+            conns1 = conns if props_based_on_config1 else no_conns
+            conns2 = no_conns if props_based_on_config1 else conns
+            if self.output_config.fullExplanation:
+                if self.config1.optimized_run == 'true':
+                    different_conns_list.append(PeersAndConnections(str(src_peers), str(dst_peers), conns1, conns2))
+                else:  # 'debug': produce the same output format as in the original implementation (per peer pairs)
+                    for src_peer in src_peers:
+                        for dst_peer in dst_peers:
+                            if src_peer != dst_peer:
+                                different_conns_list.append(PeersAndConnections(str(src_peer), str(dst_peer),
+                                                                                conns1, conns2))
+            else:
+                different_conns_list.append(PeersAndConnections(src_peers.rep(), dst_peers.rep(), conns1, conns2))
+
+    def check_equivalence_optimized(self, layer_name=None):
+        conn_props1 = self.config1.allowed_connections_optimized(layer_name)
+        conn_props2 = self.config2.allowed_connections_optimized(layer_name)
+        all_conns1, all_conns2 = self.filter_conns_by_input_or_internal_constraints(conn_props1.all_allowed_conns,
+                                                                                    conn_props2.all_allowed_conns)
+        if all_conns1 == all_conns2:
+            return QueryAnswer(True, self.name1 + ' and ' + self.name2 + ' are semantically equivalent.',
+                               numerical_result=0)
+
+        conns1_not_in_conns2 = all_conns1 - all_conns2
+        conns2_not_in_conns1 = all_conns2 - all_conns1
+        different_conns_list = []
+        self._append_different_conns_to_list(conns1_not_in_conns2, different_conns_list, True)
+        self._append_different_conns_to_list(conns2_not_in_conns1, different_conns_list, False)
+        return self._query_answer_with_relevant_explanation(sorted(different_conns_list))
+
     def _query_answer_with_relevant_explanation(self, explanation_list):
         output_result = self.name1 + ' and ' + self.name2 + ' are not semantically equivalent.'
         explanation_description = f'Connections allowed in {self.name1} which are different in {self.name2}'

nca/Parsers/GenericIngressLikeYamlParser.py

@@ -76,13 +76,11 @@ def _make_rules_from_conns(conn_props):
         peers_to_conns = {}
         res = []
         # extract peers dimension from cubes
+        assert not conn_props.is_active_dimension("src_peers")
         for cube in conn_props:
             conn_cube = conn_props.get_connectivity_cube(cube)
-            src_peer_set = conn_cube["src_peers"]
-            conn_cube.unset_dim("src_peers")
             dst_peer_set = conn_cube["dst_peers"]
             conn_cube.unset_dim("dst_peers")
-            assert not src_peer_set
             new_props = ConnectivityProperties.make_conn_props(conn_cube)
             new_conns = ConnectionSet()
             new_conns.add_connections('TCP', new_props)

nca/SchemeRunner.py

@@ -17,6 +17,8 @@ class SchemeRunner(GenericYamlParser):
     This class takes a scheme file, build all its network configurations and runs all its queries
     """
 
+    implemented_opt_queries = set(['connectivityMap', 'equivalence'])
+
     def __init__(self, scheme_file_name, output_format=None, output_path=None, optimized_run='false'):
         GenericYamlParser.__init__(self, scheme_file_name)
         self.network_configs = {}
@@ -35,6 +37,10 @@ def __init__(self, scheme_file_name, output_format=None, output_path=None, optim
                 if not isinstance(self.scheme, dict):
                     self.syntax_error("The scheme's top-level object must be a map")
 
+    @staticmethod
+    def has_implemented_opt_queries(queries):
+        return SchemeRunner.implemented_opt_queries.intersection(queries)
+
     def _get_input_file(self, given_path, out_flag=False):
         """
         Attempts to locate a file specified in the scheme file (possibly relatively to the scheme file)
@@ -190,10 +196,9 @@ def run_queries(self, query_array):
             not_executed = 0
             self.check_fields_validity(query, 'query', allowed_elements)
             query_name = query['name']
-            if self.optimized_run == 'debug':
+            if self.optimized_run == 'debug' or self.optimized_run == 'true':
                 # TODO - update/remove the optimization below when all queries are supported in optimized implementation
-                # optimization - currently only connectivityMap query has optimized implementation and can be compared
-                if not query.get('connectivityMap'):
+                if not self.has_implemented_opt_queries(set(query.keys())):
                     print(f'Skipping query {query_name} since it does not have optimized implementation yet')
                     continue
             print('Running query', query_name)

nca/nca_cli.py

@@ -133,7 +133,7 @@ def _make_recursive(path_list):
     return path_list
 
 
-def run_args(args):
+def run_args(args):  # noqa: C901
     """
     Given the parsed cmdline, decide what to run
     :param Namespace args: argparse-style parsed cmdline
@@ -216,6 +216,12 @@ def run_args(args):
         pair_query_flag = True
         expected_output = args.expected_output or None
 
+    if args.optimized_run == 'debug' or args.optimized_run == 'true':
+        # TODO - update/remove the optimization below when all queries are supported in optimized implementation
+        if not SchemeRunner.has_implemented_opt_queries({query_name}):
+            print(f'Not running query {query_name} since it does not have optimized implementation yet')
+            return _compute_return_value(0, 0, 1)
+
     resources_handler = ResourcesHandler()
     network_config = resources_handler.get_network_config(_make_recursive(np_list), _make_recursive(ns_list),
                                                           _make_recursive(pod_list), _make_recursive(resource_list),

tests/istio_testcases/example_policies/bookinfo-demo/sidecar_examples/equivalence-with-sidecars-scheme.yaml

@@ -48,7 +48,9 @@ queries:
       - sidecar-with-selector-registery-only
     outputConfiguration:
       fullExplanation: True
-    expectedOutput: ../../../expected_output/equiv_configs_w_sidecars_different_outbound_mode.txt
+    # expectedOutput in the optimized solution is more refined than in the original one.
+    # uncomment the line below and updated the expectedOutput result after moving to optimized solution.
+    #expectedOutput: ../../../expected_output/equiv_configs_w_sidecars_different_outbound_mode.txt
     expected: 1  # not equal , the second restricts conns to ip-blocks for app: ratings
 
   - name: strong-equiv-allow-any-different-outbound-modes

tests/run_all_tests.py

@@ -104,6 +104,15 @@ def initialize_test(self):
 
     def run_all_test_flow(self, all_results):
         # should be overriden by inheriting classes
+        tmp_opt = [i for i in self.test_queries_obj.args_obj.args if '-opt=' in i]
+        opt = tmp_opt[0].split('=')[1] if tmp_opt else 'false'
+        if isinstance(self.test_queries_obj, CliQuery) and (opt == 'debug' or opt == 'true'):
+            implemented_opt_queries = ['--connectivity']
+            # TODO - update/remove the optimization below when all queries are supported in optimized implementation
+            if not set(implemented_opt_queries).intersection(set(self.test_queries_obj.args_obj.args)):
+                print(f'Skipping {self.test_queries_obj.test_name} since it does not have optimized implementation yet')
+                return 0, 0
+
         self.initialize_test()
         self.run_test()
         self.evaluate_test_results()