ospf6d: rework filtering commands to be in line with ospfd

Issue #9535 describes how the export-list/import-list commands work
differently on ospfd and ospf6d.

In short:
* On ospfd, "area A.B.C.D export-list" filters which internal
  routes an ABR exports to other areas. On ospf6d, instead, that
  command filters which inter-area routes an ABR exports to the
  configured area (which is quite counter-intuitive). In other words,
  both commands do the same but in opposite directions.
* On ospfd, "area A.B.C.D import-list" filters which inter-area
  routes an ABR imports into the configured area. On ospf6d, that
  command filters which inter-area routes an interior router accepts.
* On both daemons, "area A.B.C.D filter-list prefix NAME <in|out>"
  works exactly the same as import/export lists, but using prefix-lists
  instead of ACLs.

The inconsistency on how those commands work is undesirable. This
PR proposes to adapt the ospf6d commands to behave like they do
in ospfd.

These changes are obviously backward incompatible and this PR doesn't
propose any mitigation strategy other than warning users about the
changes in the next release notes. Since these ospf6d commands are
undocumented and work in such a peculiar way, it's unlikely many
users will be affected (if any at all).

Signed-off-by: Renato Westphal <renato@opensourcerouting.org>

7 files changed, 224 insertions, 136 deletions

diff --git a/doc/user/ospf6d.rst b/doc/user/ospf6d.rst
index ede214410..f0b0638ee 100644
--- a/doc/user/ospf6d.rst
+++ b/doc/user/ospf6d.rst
@@ -198,6 +198,47 @@ OSPF6 area
    advertisement of summaries into the area. In that case, a single Type-3 LSA
    containing a default route is originated into the NSSA.
 
+.. clicmd:: area A.B.C.D export-list NAME
+
+.. clicmd:: area (0-4294967295) export-list NAME
+
+   Filter Type-3 summary-LSAs announced to other areas originated from intra-
+   area paths from specified area.
+
+   .. code-block:: frr
+
+      router ospf6
+       area 0.0.0.10 export-list foo
+      !
+      ipv6 access-list foo permit 2001:db8:1000::/64
+      ipv6 access-list foo deny any
+
+   With example above any intra-area paths from area 0.0.0.10 and from range
+   2001:db8::/32 (for example 2001:db8:1::/64 and 2001:db8:2::/64) are announced
+   into other areas as Type-3 summary-LSA's, but any others (for example
+   2001:200::/48) aren't.
+
+   This command is only relevant if the router is an ABR for the specified
+   area.
+
+.. clicmd:: area A.B.C.D import-list NAME
+
+.. clicmd:: area (0-4294967295) import-list NAME
+
+   Same as export-list, but it applies to paths announced into specified area
+   as Type-3 summary-LSAs.
+
+.. clicmd:: area A.B.C.D filter-list prefix NAME in
+
+.. clicmd:: area A.B.C.D filter-list prefix NAME out
+
+.. clicmd:: area (0-4294967295) filter-list prefix NAME in
+
+.. clicmd:: area (0-4294967295) filter-list prefix NAME out
+
+   Filtering Type-3 summary-LSAs to/from area using prefix lists. This command
+   makes sense in ABR only.
+
 .. _ospf6-interface:
 
 OSPF6 interface
diff --git a/ospf6d/ospf6_abr.c b/ospf6d/ospf6_abr.c
index fe1845907..57165201b 100644
--- a/ospf6d/ospf6_abr.c
+++ b/ospf6d/ospf6_abr.c
@@ -231,6 +231,69 @@ int ospf6_abr_originate_summary_to_area(struct ospf6_route *route,
 		return 0;
 	}
 
+	if (route->type == OSPF6_DEST_TYPE_NETWORK) {
+		bool filter = false;
+
+		route_area =
+			ospf6_area_lookup(route->path.area_id, area->ospf6);
+		assert(route_area);
+
+		/* Check export-list */
+		if (EXPORT_LIST(route_area)
+		    && access_list_apply(EXPORT_LIST(route_area),
+					 &route->prefix)
+			       == FILTER_DENY) {
+			if (IS_OSPF6_DEBUG_ABR)
+				zlog_debug(
+					"%s: prefix %pFX was denied by export-list",
+					__func__, &route->prefix);
+			filter = true;
+		}
+
+		/* Check output prefix-list */
+		if (PREFIX_LIST_OUT(route_area)
+		    && prefix_list_apply(PREFIX_LIST_OUT(route_area),
+					 &route->prefix)
+			       != PREFIX_PERMIT) {
+			if (IS_OSPF6_DEBUG_ABR)
+				zlog_debug(
+					"%s: prefix %pFX was denied by prefix-list out",
+					__func__, &route->prefix);
+			filter = true;
+		}
+
+		/* Check import-list */
+		if (IMPORT_LIST(area)
+		    && access_list_apply(IMPORT_LIST(area), &route->prefix)
+			       == FILTER_DENY) {
+			if (IS_OSPF6_DEBUG_ABR)
+				zlog_debug(
+					"%s: prefix %pFX was denied by import-list",
+					__func__, &route->prefix);
+			filter = true;
+		}
+
+		/* Check input prefix-list */
+		if (PREFIX_LIST_IN(area)
+		    && prefix_list_apply(PREFIX_LIST_IN(area), &route->prefix)
+			       != PREFIX_PERMIT) {
+			if (IS_OSPF6_DEBUG_ABR)
+				zlog_debug(
+					"%s: prefix %pFX was denied by prefix-list in",
+					__func__, &route->prefix);
+			filter = true;
+		}
+
+		if (filter) {
+			if (summary) {
+				ospf6_route_remove(summary, summary_table);
+				if (old)
+					ospf6_lsa_purge(old);
+			}
+			return 0;
+		}
+	}
+
 	/* do not generate if the nexthops belongs to the target area */
 	if (ospf6_abr_nexthops_belong_to_area(route, area)) {
 		if (IS_OSPF6_DEBUG_ABR)
@@ -430,39 +493,6 @@ int ospf6_abr_originate_summary_to_area(struct ospf6_route *route,
 		}
 	}
 
-	/* Check export list */
-	if (EXPORT_NAME(area)) {
-		if (EXPORT_LIST(area) == NULL)
-			EXPORT_LIST(area) =
-				access_list_lookup(AFI_IP6, EXPORT_NAME(area));
-
-		if (EXPORT_LIST(area))
-			if (access_list_apply(EXPORT_LIST(area), &route->prefix)
-			    == FILTER_DENY) {
-				if (is_debug)
-					zlog_debug(
-						"prefix %pFX was denied by export list",
-						&route->prefix);
-				ospf6_abr_delete_route(route, summary,
-						       summary_table, old);
-				return 0;
-			}
-	}
-
-	/* Check filter-list */
-	if (PREFIX_LIST_OUT(area))
-		if (prefix_list_apply(PREFIX_LIST_OUT(area), &route->prefix)
-		    != PREFIX_PERMIT) {
-			if (is_debug)
-				zlog_debug(
-					"prefix %pFX was denied by filter-list out",
-					&route->prefix);
-			ospf6_abr_delete_route(route, summary, summary_table,
-					       old);
-
-			return 0;
-		}
-
 	/* the route is going to be originated. store it in area's summary_table
 	 */
 	if (summary == NULL) {
@@ -1134,39 +1164,6 @@ void ospf6_abr_examin_summary(struct ospf6_lsa *lsa, struct ospf6_area *oa)
 		return;
 	}
 
-	/* Check import list */
-	if (IMPORT_NAME(oa)) {
-		if (IMPORT_LIST(oa) == NULL)
-			IMPORT_LIST(oa) =
-				access_list_lookup(AFI_IP6, IMPORT_NAME(oa));
-
-		if (IMPORT_LIST(oa))
-			if (access_list_apply(IMPORT_LIST(oa), &prefix)
-			    == FILTER_DENY) {
-				if (is_debug)
-					zlog_debug(
-						"Prefix %pFX was denied by import-list",
-						&prefix);
-				if (old)
-					ospf6_route_remove(old, table);
-				return;
-			}
-	}
-
-	/* Check input prefix-list */
-	if (PREFIX_LIST_IN(oa)) {
-		if (prefix_list_apply(PREFIX_LIST_IN(oa), &prefix)
-		    != PREFIX_PERMIT) {
-			if (is_debug)
-				zlog_debug(
-					"Prefix %pFX was denied by prefix-list in",
-					&prefix);
-			if (old)
-				ospf6_route_remove(old, table);
-			return;
-		}
-	}
-
 	/* (5),(6): the path preference is handled by the sorting
 	   in the routing table. Always install the path by substituting
 	   old route (if any). */
@@ -1355,35 +1352,6 @@ void ospf6_abr_examin_brouter(uint32_t router_id, struct ospf6_route *route,
 		ospf6_abr_examin_summary(lsa, oa);
 }
 
-void ospf6_abr_reimport(struct ospf6_area *oa)
-{
-	struct ospf6_lsa *lsa;
-	uint16_t type;
-
-	type = htons(OSPF6_LSTYPE_INTER_ROUTER);
-	for (ALL_LSDB_TYPED(oa->lsdb, type, lsa))
-		ospf6_abr_examin_summary(lsa, oa);
-
-	type = htons(OSPF6_LSTYPE_INTER_PREFIX);
-	for (ALL_LSDB_TYPED(oa->lsdb, type, lsa))
-		ospf6_abr_examin_summary(lsa, oa);
-}
-
-/* export filter removed so determine if we should reoriginate summary LSAs */
-void ospf6_abr_reexport(struct ospf6_area *oa)
-{
-	struct ospf6_route *route;
-
-	/* if not a ABR return success */
-	if (!ospf6_check_and_set_router_abr(oa->ospf6))
-		return;
-
-	/* Redo summaries if required */
-	for (route = ospf6_route_head(oa->ospf6->route_table); route;
-	     route = ospf6_route_next(route))
-		ospf6_abr_originate_summary_to_area(route, oa);
-}
-
 void ospf6_abr_prefix_resummarize(struct ospf6 *o)
 {
 	struct ospf6_route *route;
diff --git a/ospf6d/ospf6_abr.h b/ospf6d/ospf6_abr.h
index a5f0f124b..08521ecb0 100644
--- a/ospf6d/ospf6_abr.h
+++ b/ospf6d/ospf6_abr.h
@@ -73,8 +73,6 @@ extern void ospf6_abr_defaults_to_stub(struct ospf6 *ospf6);
 extern void ospf6_abr_examin_brouter(uint32_t router_id,
 				     struct ospf6_route *route,
 				     struct ospf6 *ospf6);
-extern void ospf6_abr_reimport(struct ospf6_area *oa);
-extern void ospf6_abr_reexport(struct ospf6_area *oa);
 extern void ospf6_abr_range_reset_cost(struct ospf6 *ospf6);
 extern void ospf6_abr_prefix_resummarize(struct ospf6 *ospf6);
 
@@ -88,7 +86,6 @@ extern void ospf6_abr_old_path_update(struct ospf6_route *old_route,
 				      struct ospf6_route *route,
 				      struct ospf6_route_table *table);
 extern void ospf6_abr_init(void);
-extern void ospf6_abr_reexport(struct ospf6_area *oa);
 extern void ospf6_abr_range_update(struct ospf6_route *range,
 				   struct ospf6 *ospf6);
 extern void ospf6_abr_remove_unapproved_summaries(struct ospf6 *ospf6);
diff --git a/ospf6d/ospf6_area.c b/ospf6d/ospf6_area.c
index 7423a1a9e..2f5328c66 100644
--- a/ospf6d/ospf6_area.c
+++ b/ospf6d/ospf6_area.c
@@ -696,17 +696,17 @@ DEFUN (area_filter_list,
 		XFREE(MTYPE_OSPF6_PLISTNAME, PREFIX_NAME_IN(area));
 		PREFIX_NAME_IN(area) =
 			XSTRDUP(MTYPE_OSPF6_PLISTNAME, plistname);
-		ospf6_abr_reimport(area);
 	} else {
 		PREFIX_LIST_OUT(area) = plist;
 		XFREE(MTYPE_OSPF6_PLISTNAME, PREFIX_NAME_OUT(area));
 		PREFIX_NAME_OUT(area) =
 			XSTRDUP(MTYPE_OSPF6_PLISTNAME, plistname);
-
-		/* Redo summaries if required */
-		ospf6_abr_reexport(area);
 	}
 
+	/* Redo summaries if required */
+	if (ospf6_check_and_set_router_abr(area->ospf6))
+		ospf6_schedule_abr_task(ospf6);
+
 	return CMD_SUCCESS;
 }
 
@@ -739,7 +739,6 @@ DEFUN (no_area_filter_list,
 
 		PREFIX_LIST_IN(area) = NULL;
 		XFREE(MTYPE_OSPF6_PLISTNAME, PREFIX_NAME_IN(area));
-		ospf6_abr_reimport(area);
 	} else {
 		if (PREFIX_NAME_OUT(area))
 			if (!strmatch(PREFIX_NAME_OUT(area), plistname))
@@ -747,9 +746,12 @@ DEFUN (no_area_filter_list,
 
 		XFREE(MTYPE_OSPF6_PLISTNAME, PREFIX_NAME_OUT(area));
 		PREFIX_LIST_OUT(area) = NULL;
-		ospf6_abr_reexport(area);
 	}
 
+	/* Redo summaries if required */
+	if (ospf6_check_and_set_router_abr(area->ospf6))
+		ospf6_schedule_abr_task(ospf6);
+
 	return CMD_SUCCESS;
 }
 
@@ -760,19 +762,30 @@ void ospf6_filter_update(struct access_list *access)
 	struct ospf6 *ospf6;
 
 	for (ALL_LIST_ELEMENTS(om6->ospf6, node, nnode, ospf6)) {
+		bool update = false;
+
 		for (ALL_LIST_ELEMENTS_RO(ospf6->area_list, n, oa)) {
 			if (IMPORT_NAME(oa)
-			    && strcmp(IMPORT_NAME(oa), access->name) == 0)
-				ospf6_abr_reimport(oa);
+			    && strcmp(IMPORT_NAME(oa), access->name) == 0) {
+				IMPORT_LIST(oa) = access_list_lookup(
+					AFI_IP6, IMPORT_NAME(oa));
+				update = true;
+			}
 
 			if (EXPORT_NAME(oa)
-			    && strcmp(EXPORT_NAME(oa), access->name) == 0)
-				ospf6_abr_reexport(oa);
+			    && strcmp(EXPORT_NAME(oa), access->name) == 0) {
+				EXPORT_LIST(oa) = access_list_lookup(
+					AFI_IP6, EXPORT_NAME(oa));
+				update = true;
+			}
 		}
+
+		if (update && ospf6_check_and_set_router_abr(ospf6))
+			ospf6_schedule_abr_task(ospf6);
 	}
 }
 
-void ospf6_area_plist_update(struct prefix_list *plist, int add)
+void ospf6_plist_update(struct prefix_list *plist)
 {
 	struct listnode *node, *nnode;
 	struct ospf6_area *oa;
@@ -780,19 +793,29 @@ void ospf6_area_plist_update(struct prefix_list *plist, int add)
 	const char *name = prefix_list_name(plist);
 	struct ospf6 *ospf6 = NULL;
 
+	if (prefix_list_afi(plist) != AFI_IP6)
+		return;
+
 	for (ALL_LIST_ELEMENTS(om6->ospf6, node, nnode, ospf6)) {
+		bool update = false;
+
 		for (ALL_LIST_ELEMENTS_RO(ospf6->area_list, n, oa)) {
 			if (PREFIX_NAME_IN(oa)
 			    && !strcmp(PREFIX_NAME_IN(oa), name)) {
-				PREFIX_LIST_IN(oa) = add ? plist : NULL;
-				ospf6_abr_reexport(oa);
+				PREFIX_LIST_IN(oa) = prefix_list_lookup(
+					AFI_IP6, PREFIX_NAME_IN(oa));
+				update = true;
 			}
 			if (PREFIX_NAME_OUT(oa)
 			    && !strcmp(PREFIX_NAME_OUT(oa), name)) {
-				PREFIX_LIST_OUT(oa) = add ? plist : NULL;
-				ospf6_abr_reexport(oa);
+				PREFIX_LIST_OUT(oa) = prefix_list_lookup(
+					AFI_IP6, PREFIX_NAME_OUT(oa));
+				update = true;
 			}
 		}
+
+		if (update && ospf6_check_and_set_router_abr(ospf6))
+			ospf6_schedule_abr_task(ospf6);
 	}
 }
 
@@ -822,7 +845,8 @@ DEFUN (area_import_list,
 		free(IMPORT_NAME(area));
 
 	IMPORT_NAME(area) = strdup(argv[idx_name]->arg);
-	ospf6_abr_reimport(area);
+	if (ospf6_check_and_set_router_abr(area->ospf6))
+		ospf6_schedule_abr_task(ospf6);
 
 	return CMD_SUCCESS;
 }
@@ -844,13 +868,14 @@ DEFUN (no_area_import_list,
 
 	OSPF6_CMD_AREA_GET(argv[idx_ipv4]->arg, area, ospf6);
 
-	IMPORT_LIST(area) = 0;
+	IMPORT_LIST(area) = NULL;
 
 	if (IMPORT_NAME(area))
 		free(IMPORT_NAME(area));
 
 	IMPORT_NAME(area) = NULL;
-	ospf6_abr_reimport(area);
+	if (ospf6_check_and_set_router_abr(area->ospf6))
+		ospf6_schedule_abr_task(ospf6);
 
 	return CMD_SUCCESS;
 }
@@ -883,7 +908,8 @@ DEFUN (area_export_list,
 	EXPORT_NAME(area) = strdup(argv[idx_name]->arg);
 
 	/* Redo summaries if required */
-	ospf6_abr_reexport(area);
+	if (ospf6_check_and_set_router_abr(area->ospf6))
+		ospf6_schedule_abr_task(ospf6);
 
 	return CMD_SUCCESS;
 }
@@ -905,13 +931,14 @@ DEFUN (no_area_export_list,
 
 	OSPF6_CMD_AREA_GET(argv[idx_ipv4]->arg, area, ospf6);
 
-	EXPORT_LIST(area) = 0;
+	EXPORT_LIST(area) = NULL;
 
 	if (EXPORT_NAME(area))
 		free(EXPORT_NAME(area));
 
 	EXPORT_NAME(area) = NULL;
-	ospf6_abr_reexport(area);
+	if (ospf6_check_and_set_router_abr(area->ospf6))
+		ospf6_schedule_abr_task(ospf6);
 
 	return CMD_SUCCESS;
 }
diff --git a/ospf6d/ospf6_area.h b/ospf6d/ospf6_area.h
index aed9589bf..1aefcb9eb 100644
--- a/ospf6d/ospf6_area.h
+++ b/ospf6d/ospf6_area.h
@@ -163,7 +163,7 @@ extern void ospf6_area_disable(struct ospf6_area *oa);
 extern void ospf6_area_show(struct vty *vty, struct ospf6_area *oa,
 			    json_object *json_areas, bool use_json);
 
-extern void ospf6_area_plist_update(struct prefix_list *plist, int add);
+extern void ospf6_plist_update(struct prefix_list *plist);
 extern void ospf6_filter_update(struct access_list *access);
 extern void ospf6_area_config_write(struct vty *vty, struct ospf6 *ospf6);
 extern void ospf6_area_init(void);
diff --git a/ospf6d/ospf6d.c b/ospf6d/ospf6d.c
index 2c8c9b9d4..5e6dcde99 100644
--- a/ospf6d/ospf6d.c
+++ b/ospf6d/ospf6d.c
@@ -1355,20 +1355,6 @@ DEFUN(show_ipv6_ospf6_linkstate_detail, show_ipv6_ospf6_linkstate_detail_cmd,
 	return CMD_SUCCESS;
 }
 
-static void ospf6_plist_add(struct prefix_list *plist)
-{
-	if (prefix_list_afi(plist) != AFI_IP6)
-		return;
-	ospf6_area_plist_update(plist, 1);
-}
-
-static void ospf6_plist_del(struct prefix_list *plist)
-{
-	if (prefix_list_afi(plist) != AFI_IP6)
-		return;
-	ospf6_area_plist_update(plist, 0);
-}
-
 /* Install ospf related commands. */
 void ospf6_init(struct thread_master *master)
 {
@@ -1387,8 +1373,8 @@ void ospf6_init(struct thread_master *master)
 	ospf6_gr_helper_config_init();
 
 	/* initialize hooks for modifying filter rules */
-	prefix_list_add_hook(ospf6_plist_add);
-	prefix_list_delete_hook(ospf6_plist_del);
+	prefix_list_add_hook(ospf6_plist_update);
+	prefix_list_delete_hook(ospf6_plist_update);
 	access_list_add_hook(ospf6_filter_update);
 	access_list_delete_hook(ospf6_filter_update);
 
diff --git a/tests/topotests/ospf6_topo2/test_ospf6_topo2.py b/tests/topotests/ospf6_topo2/test_ospf6_topo2.py
index bea3aeaaf..acaa3b8a9 100644
--- a/tests/topotests/ospf6_topo2/test_ospf6_topo2.py
+++ b/tests/topotests/ospf6_topo2/test_ospf6_topo2.py
@@ -431,6 +431,75 @@ def test_nssa_no_summary():
     assert result is None, assertmsg
 
 
+def test_area_filters():
+    """
+    Test ABR import/export filters.
+    """
+    tgen = get_topogen()
+    if tgen.routers_have_failure():
+        pytest.skip(tgen.errors)
+
+    #
+    # Configure import/export filters on r2 (ABR for area 1).
+    #
+    config = """
+    configure terminal
+    ipv6 access-list ACL_IMPORT seq 5 permit 2001:db8:2::/64
+    ipv6 access-list ACL_IMPORT seq 10 deny any
+    ipv6 access-list ACL_EXPORT seq 10 deny any
+    router ospf6
+    area 1 import-list ACL_IMPORT
+    area 1 export-list ACL_EXPORT
+    """
+    tgen.gears["r2"].vtysh_cmd(config)
+
+    logger.info("Expecting inter-area routes to be removed on r1")
+    for route in ["::/0", "2001:db8:3::/64"]:
+        test_func = partial(dont_expect_route, "r1", route, type="inter-area")
+        _, result = topotest.run_and_expect(test_func, None, count=130, wait=1)
+        assertmsg = "{}'s {} inter-area route still exists".format("r1", route)
+        assert result is None, assertmsg
+
+    logger.info("Expecting inter-area routes to be removed on r3")
+    for route in ["2001:db8:1::/64"]:
+        test_func = partial(dont_expect_route, "r3", route, type="inter-area")
+        _, result = topotest.run_and_expect(test_func, None, count=130, wait=1)
+        assertmsg = "{}'s {} inter-area route still exists".format("r3", route)
+        assert result is None, assertmsg
+
+    #
+    # Update the ACLs used by the import/export filters.
+    #
+    config = """
+    configure terminal
+    ipv6 access-list ACL_IMPORT seq 6 permit 2001:db8:3::/64
+    ipv6 access-list ACL_EXPORT seq 5 permit 2001:db8:1::/64
+    """
+    tgen.gears["r2"].vtysh_cmd(config)
+
+    logger.info("Expecting 2001:db8:3::/64 to be re-added on r1")
+    routes = {"2001:db8:3::/64": {}}
+    expect_ospfv3_routes("r1", routes, wait=30, type="inter-area")
+    logger.info("Expecting 2001:db8:1::/64 to be re-added on r3")
+    routes = {"2001:db8:1::/64": {}}
+    expect_ospfv3_routes("r3", routes, wait=30, type="inter-area")
+
+    #
+    # Unconfigure r2's ABR import/export filters.
+    #
+    config = """
+    configure terminal
+    router ospf6
+    no area 1 import-list ACL_IMPORT
+    no area 1 export-list ACL_EXPORT
+    """
+    tgen.gears["r2"].vtysh_cmd(config)
+
+    logger.info("Expecting ::/0 to be re-added on r1")
+    routes = {"::/0": {}}
+    expect_ospfv3_routes("r1", routes, wait=30, type="inter-area")
+
+
 def teardown_module(_mod):
     "Teardown the pytest environment"
     tgen = get_topogen()