From 0693c997aa5d635dcfbbc6be1704b0feb07a2af5 Mon Sep 17 00:00:00 2001 From: Tanya Date: Tue, 13 Jun 2023 15:58:04 +0300 Subject: [PATCH] Equivalence query optimized implementation (#524) * Optimized implementation of EquivalenceQuery. Signed-off-by: Tanya * Align EquivalenceQuery difference explanation according to the original implementation. Signed-off-by: Tanya * Fixing lint errors. Signed-off-by: Tanya * 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 * 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 * yet another small fix of the fix. Signed-off-by: Tanya * 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 * Ignoring 'complex function' lint error. Returning 'passed' code for skipped queries. Signed-off-by: Tanya * Undo returning 'passed' code for skipped queries. Skipping non-implemented optimized cmdline queries in run_all_tests Signed-off-by: Tanya * Skipping non-implemented optimized cmdline queries in run_all_tests Signed-off-by: Tanya * Fixed the refined code of optimized equivalence calculation. Signed-off-by: Tanya * Update nca/NetworkConfig/NetworkConfigQuery.py Co-authored-by: Adi Sosnovich <82078442+adisos@users.noreply.github.com> * Reused definition of implemented_opt_queries Signed-off-by: Tanya * Fix of reusing definition of implemented_opt_queries Signed-off-by: Tanya * Removed redundant method. Signed-off-by: Tanya --------- Signed-off-by: Tanya Co-authored-by: Adi Sosnovich <82078442+adisos@users.noreply.github.com> --- nca/CoreDS/CanonicalHyperCubeSet.py | 3 + nca/CoreDS/ConnectivityCube.py | 2 +- nca/CoreDS/ConnectivityProperties.py | 11 ++- nca/NetworkConfig/NetworkConfig.py | 22 +++++ nca/NetworkConfig/NetworkConfigQuery.py | 99 ++++++++++++++----- nca/Parsers/GenericIngressLikeYamlParser.py | 4 +- nca/SchemeRunner.py | 11 ++- nca/nca_cli.py | 8 +- .../equivalence-with-sidecars-scheme.yaml | 4 +- tests/run_all_tests.py | 9 ++ 10 files changed, 137 insertions(+), 36 deletions(-) diff --git a/nca/CoreDS/CanonicalHyperCubeSet.py b/nca/CoreDS/CanonicalHyperCubeSet.py index e85bd5150..ad5568174 100644 --- a/nca/CoreDS/CanonicalHyperCubeSet.py +++ b/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 diff --git a/nca/CoreDS/ConnectivityCube.py b/nca/CoreDS/ConnectivityCube.py index a144893a5..44c9b54c7 100644 --- a/nca/CoreDS/ConnectivityCube.py +++ b/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) diff --git a/nca/CoreDS/ConnectivityProperties.py b/nca/CoreDS/ConnectivityProperties.py index c8d372ca5..8b67e953f 100644 --- a/nca/CoreDS/ConnectivityProperties.py +++ b/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: diff --git a/nca/NetworkConfig/NetworkConfig.py b/nca/NetworkConfig/NetworkConfig.py index c55117e7f..e2316cbf3 100644 --- a/nca/NetworkConfig/NetworkConfig.py +++ b/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 diff --git a/nca/NetworkConfig/NetworkConfigQuery.py b/nca/NetworkConfig/NetworkConfigQuery.py index f03ee1e4d..a44dcd26e 100644 --- a/nca/NetworkConfig/NetworkConfigQuery.py +++ b/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}' diff --git a/nca/Parsers/GenericIngressLikeYamlParser.py b/nca/Parsers/GenericIngressLikeYamlParser.py index 67dbecacd..bdb9b2374 100644 --- a/nca/Parsers/GenericIngressLikeYamlParser.py +++ b/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) diff --git a/nca/SchemeRunner.py b/nca/SchemeRunner.py index 2294a1e52..63f7f5d01 100644 --- a/nca/SchemeRunner.py +++ b/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) diff --git a/nca/nca_cli.py b/nca/nca_cli.py index 08e390a61..70a3ab429 100644 --- a/nca/nca_cli.py +++ b/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), diff --git a/tests/istio_testcases/example_policies/bookinfo-demo/sidecar_examples/equivalence-with-sidecars-scheme.yaml b/tests/istio_testcases/example_policies/bookinfo-demo/sidecar_examples/equivalence-with-sidecars-scheme.yaml index cbeb1a4db..50971743b 100644 --- a/tests/istio_testcases/example_policies/bookinfo-demo/sidecar_examples/equivalence-with-sidecars-scheme.yaml +++ b/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 diff --git a/tests/run_all_tests.py b/tests/run_all_tests.py index da4e642f2..42e6fc662 100644 --- a/tests/run_all_tests.py +++ b/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()