From: Nathan Bahr Date: Fri, 21 Feb 2025 17:59:04 +0000 (+0000) Subject: pim: Fix autorp group joins X-Git-Tag: docker/10.3.0~18^2 X-Git-Url: https://git.puffer.fish/?a=commitdiff_plain;h=673410bd3089476d35d1562b38f7c83df3a495a4;p=matthieu%2Ffrr.git pim: Fix autorp group joins Group joining got broken when moving the autorp socket to open/close as needed. This fixes it so autorp group joining is properly handled as part of opening the socket. Signed-off-by: Nathan Bahr (cherry picked from commit d840560b74e3a6117aa1e4b1203dcdd8fb254ef6) --- diff --git a/pimd/pim_autorp.c b/pimd/pim_autorp.c index dc077dbbd6..09bbf43885 100644 --- a/pimd/pim_autorp.c +++ b/pimd/pim_autorp.c @@ -967,6 +967,7 @@ err: static bool pim_autorp_socket_enable(struct pim_autorp *autorp) { int fd; + struct interface *ifp; /* Return early if socket is already enabled */ if (autorp->sock != -1) @@ -990,6 +991,11 @@ static bool pim_autorp_socket_enable(struct pim_autorp *autorp) autorp->sock = fd; + /* Join autorp groups on all pim enabled interfaces in the VRF */ + FOR_ALL_INTERFACES (autorp->pim->vrf, ifp) { + pim_autorp_add_ifp(ifp); + } + if (PIM_DEBUG_AUTORP) zlog_debug("%s: AutoRP socket enabled (fd=%u)", __func__, fd); @@ -1002,6 +1008,7 @@ static bool pim_autorp_socket_disable(struct pim_autorp *autorp) if (autorp->sock == -1) return true; + /* No need to leave the autorp groups explicitly, they are left when the socket is closed */ if (close(autorp->sock)) { zlog_warn("Failure closing autorp socket: fd=%d errno=%d: %s", autorp->sock, errno, safe_strerror(errno)); @@ -1428,10 +1435,10 @@ void pim_autorp_add_ifp(struct interface *ifp) { /* Add a new interface for autorp * When autorp is enabled, we must join the autorp groups on all - * pim/multicast interfaces. When autorp first starts, if finds all - * current multicast interfaces and joins on them. If a new interface - * comes up or is configured for multicast after autorp is running, then - * this method will add it for autorp-> + * pim/multicast interfaces. When autorp becomes enabled, it finds all + * current pim enabled interfaces and joins the autorp groups on them. + * Any new interfaces added after autorp is enabled will use this function + * to join the autorp groups * This is called even when adding a new pim interface that is not yet * active, so make sure the check, it'll call in again once the interface is up. */ @@ -1441,7 +1448,8 @@ void pim_autorp_add_ifp(struct interface *ifp) pim_ifp = ifp->info; if (CHECK_FLAG(ifp->status, ZEBRA_INTERFACE_ACTIVE) && pim_ifp && pim_ifp->pim_enable) { pim = pim_ifp->pim; - if (pim && pim->autorp && pim->autorp->do_discovery) { + if (pim && pim->autorp && + (pim->autorp->do_discovery || pim->autorp->send_rp_discovery)) { if (PIM_DEBUG_AUTORP) zlog_debug("%s: Adding interface %s to AutoRP, joining AutoRP groups", __func__, ifp->name); @@ -1477,44 +1485,37 @@ void pim_autorp_rm_ifp(struct interface *ifp) void pim_autorp_start_discovery(struct pim_instance *pim) { - struct interface *ifp; struct pim_autorp *autorp = pim->autorp; + if (autorp->do_discovery) + return; + + autorp->do_discovery = true; + /* Make sure the socket is open and ready */ if (!pim_autorp_socket_enable(autorp)) { zlog_err("%s: AutoRP failed to open socket", __func__); return; } - if (!autorp->do_discovery) { - autorp->do_discovery = true; - autorp_read_on(autorp); - - FOR_ALL_INTERFACES (autorp->pim->vrf, ifp) { - pim_autorp_add_ifp(ifp); - } + autorp_read_on(autorp); - if (PIM_DEBUG_AUTORP) - zlog_debug("%s: AutoRP Discovery started", __func__); - } + if (PIM_DEBUG_AUTORP) + zlog_debug("%s: AutoRP Discovery started", __func__); } void pim_autorp_stop_discovery(struct pim_instance *pim) { - struct interface *ifp; struct pim_autorp *autorp = pim->autorp; - if (autorp->do_discovery) { - FOR_ALL_INTERFACES (autorp->pim->vrf, ifp) { - pim_autorp_rm_ifp(ifp); - } + if (!autorp->do_discovery) + return; - autorp->do_discovery = false; - autorp_read_off(autorp); + autorp->do_discovery = false; + autorp_read_off(autorp); - if (PIM_DEBUG_AUTORP) - zlog_debug("%s: AutoRP Discovery stopped", __func__); - } + if (PIM_DEBUG_AUTORP) + zlog_debug("%s: AutoRP Discovery stopped", __func__); /* Close the socket if we need to */ if (pim_autorp_should_close(autorp) && !pim_autorp_socket_disable(autorp)) diff --git a/pimd/pim_iface.c b/pimd/pim_iface.c index 9316cebc0a..6d0f2798f9 100644 --- a/pimd/pim_iface.c +++ b/pimd/pim_iface.c @@ -1898,9 +1898,7 @@ static int pim_ifp_up(struct interface *ifp) } #if PIM_IPV == 4 - if (pim->autorp && pim->autorp->do_discovery && pim_ifp && - pim_ifp->pim_enable) - pim_autorp_add_ifp(ifp); + pim_autorp_add_ifp(ifp); #endif pim_cand_addrs_changed(); @@ -2017,8 +2015,7 @@ void pim_pim_interface_delete(struct interface *ifp) return; #if PIM_IPV == 4 - if (pim_ifp->pim_enable) - pim_autorp_rm_ifp(ifp); + pim_autorp_rm_ifp(ifp); #endif pim_ifp->pim_enable = false; diff --git a/tests/topotests/pim_autorp/test_pim_autorp.py b/tests/topotests/pim_autorp/test_pim_autorp.py index 61cf8ebbc5..5edf1db6af 100644 --- a/tests/topotests/pim_autorp/test_pim_autorp.py +++ b/tests/topotests/pim_autorp/test_pim_autorp.py @@ -158,6 +158,103 @@ def test_pim_autorp_init(request): ) +def test_pim_autorp_disable_enable(request): + "Test PIM AutoRP disable and re-enable works properly" + tgen = get_topogen() + tc_name = request.node.name + write_test_header(tc_name) + + if tgen.routers_have_failure(): + pytest.skip(tgen.errors) + + step("Ensure AutoRP groups are joined on all routers") + for rtr in ["r1", "r2", "r3", "r4"]: + expected = { + f"{rtr}-eth0": { + "name": f"{rtr}-eth0", + "224.0.1.39": "*", + "224.0.1.40": "*", + }, + f"{rtr}-eth1": { + "name": f"{rtr}-eth1", + "224.0.1.39": "*", + "224.0.1.40": "*", + }, + } + + test_func = partial( + topotest.router_json_cmp, + tgen.gears[rtr], + "show ip igmp sources json", + expected, + ) + _, result = topotest.run_and_expect(test_func, None) + assert result is None, "{} does not have correct autorp groups joined".format( + rtr + ) + + step("Disable AutoRP on all routers") + for rtr in ["r1", "r2", "r3", "r4"]: + tgen.routers()[rtr].vtysh_cmd( + """ + conf + router pim + no autorp discovery + """ + ) + + step("Ensure AutoRP groups are no longer joined on all routers") + for rtr in ["r1", "r2", "r3", "r4"]: + expected = {f"{rtr}-eth0": None, f"{rtr}-eth1": None} + + test_func = partial( + topotest.router_json_cmp, + tgen.gears[rtr], + "show ip igmp sources json", + expected, + ) + _, result = topotest.run_and_expect(test_func, None) + assert result is None, "{} does not have correct autorp groups joined".format( + rtr + ) + + step("Re-enable AutoRP on all routers") + for rtr in ["r1", "r2", "r3", "r4"]: + tgen.routers()[rtr].vtysh_cmd( + """ + conf + router pim + autorp discovery + """ + ) + + step("Ensure AutoRP groups are re-joined on all routers") + for rtr in ["r1", "r2", "r3", "r4"]: + expected = { + f"{rtr}-eth0": { + "name": f"{rtr}-eth0", + "224.0.1.39": "*", + "224.0.1.40": "*", + }, + f"{rtr}-eth1": { + "name": f"{rtr}-eth1", + "224.0.1.39": "*", + "224.0.1.40": "*", + }, + } + + test_func = partial( + topotest.router_json_cmp, + tgen.gears[rtr], + "show ip igmp sources json", + expected, + ) + _, result = topotest.run_and_expect(test_func, None) + assert result is None, "{} does not have correct autorp groups joined".format( + rtr + ) + + def test_pim_autorp_no_mapping_agent_rp(request): "Test PIM AutoRP candidate with no mapping agent" tgen = get_topogen()