ripd: poison redistributed routes on route-map change

[Evelyn0828]

When a RIP redistribution route-map is changed, routes advertised under the previous policy need to be withdrawn if the new policy denies them. Mark existing redistributed routes on route-map changes and let the triggered update send metric 16 for denied entries instead of skipping them.

CLOSES #22360

[greptile-apps]

Greptile Summary

This PR fixes issue #22360 by introducing a "policy withdraw" mechanism for RIP redistributed routes: when a redistribution route-map is changed, previously advertised routes that are now denied by the new map are sent with metric=16 (poison reverse) instead of being silently dropped from future updates.

• Adds RIP_RTF_POLICY_WITHDRAW (bit 4) to rip_info.flags and a new rip_redistribute_policy_update() function that marks all routes of the changed redistribution type with this flag and RIP_RTF_CHANGED, then fires a triggered update.
• In rip_output_process, the existing continue for route-map denials is replaced with a conditional: if RIP_RTF_POLICY_WITHDRAW is set, set metric_out = RIP_METRIC_INFINITY and fall through to actually transmit the entry; otherwise continue as before. Both flags are cleared in rip_clear_changed_flag after the triggered update completes.

Confidence Score: 3/5

The withdrawal logic works correctly in the common case, but marks all routes of the given type for poisoning — including ones that were always filtered and never advertised — causing unnecessary metric=16 messages to peers. Additionally, if a periodic update races ahead of the triggered update, the POLICY_WITHDRAW flag is never cleared and metric=16 keeps appearing in every subsequent periodic update for the affected routes.

The core mechanism is sound and the normal path works correctly. However, the unconditional marking of all type-X routes without pre-filtering against the previous route-map means peers receive spurious withdrawal messages for routes they never knew about, and the flag-persistence edge case can cause metric=16 to appear in every periodic update until the entry is replaced or times out.

ripd/ripd.c — specifically rip_redistribute_policy_update (marking logic) and the interaction between rip_clear_changed_flag and the triggered-update suppression path in rip_update.

Important Files Changed

Filename	Overview
ripd/ripd.c	Adds rip_redistribute_policy_update() to mark all routes of a given type for poisoning; modifies rip_output_process() to send metric=16 instead of skipping when RIP_RTF_POLICY_WITHDRAW is set; also clears the new flag in rip_clear_changed_flag(). Routes previously denied by the old map (never advertised) are also marked and will trigger spurious metric=16 messages.
ripd/rip_nb_config.c	Calls rip_redistribute_policy_update() in the route-map modify callback; the destroy callback correctly omits this call since removing a route-map only makes routes more permissive, and the existing apply_finish mechanism handles re-advertisement.
ripd/ripd.h	Adds RIP_RTF_POLICY_WITHDRAW flag (bit 4, correctly following the bitmask sequence) and declares the new rip_redistribute_policy_update() function prototype.

Sequence Diagram

sequenceDiagram
    participant CLI as CLI/NETCONF
    participant NB as Northbound
    participant RIPD as ripd
    participant EVQ as Event Queue
    participant Zebra as Zebra

    CLI->>NB: set redistribute route-map new-map
    NB->>RIPD: ripd_instance_redistribute_route_map_modify
    RIPD->>RIPD: "update rip->redist[type].route_map to new map"
    RIPD->>RIPD: "rip_redistribute_policy_update() mark ALL type-X routes RIP_RTF_CHANGED | RIP_RTF_POLICY_WITHDRAW"
    RIPD->>EVQ: event_add_event(rip_triggered_update)
    NB->>RIPD: ripd_instance_redistribute_apply_finish
    RIPD->>Zebra: ZEBRA_REDISTRIBUTE_ADD async

    EVQ-->>RIPD: rip_triggered_update fires
    RIPD->>RIPD: rip_update_process(rip_changed_route)
    Note over RIPD: route-map DENIES + POLICY_WITHDRAW: metric_out=16 send RTE
    Note over RIPD: route-map PERMITS: advertised normally
    RIPD->>RIPD: rip_clear_changed_flag() clears both flags

    Zebra-->>RIPD: re-sends type-X routes async
    RIPD->>RIPD: rip_redistribute_add() rip_ecmp_replace() set RIP_RTF_CHANGED
    RIPD->>EVQ: event_add_event(rip_triggered_update)
    EVQ-->>RIPD: 2nd triggered update fires
    Note over RIPD: route-map DENIES POLICY_WITHDRAW not set continue route omitted

Loading

Comments Outside Diff (1)

1. ripd/ripd.c, line 2559-2577 (link)

   RIP_RTF_POLICY_WITHDRAW can persist indefinitely if the triggered update is suppressed

   rip_clear_changed_flag is called exclusively from rip_triggered_update. When rip_event(RIP_TRIGGERED_UPDATE) is called while t_triggered_interval is already running, it sets rip->trigger = 1 instead of scheduling t_triggered_update. If rip_update (the periodic timer) fires before t_triggered_interval expires, it cancels t_triggered_interval and clears trigger, leaving t_triggered_update unscheduled and rip_clear_changed_flag never called. RIP_RTF_POLICY_WITHDRAW then remains set, causing every subsequent periodic update to emit metric=16 for the affected routes rather than simply omitting them — a behaviour that persists until the route entry is replaced (e.g., by the zebra re-redistribution triggered from apply_finish) or times out.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: ripd/ripd.c
   Line: 2559-2577
   
   Comment:
   **`RIP_RTF_POLICY_WITHDRAW` can persist indefinitely if the triggered update is suppressed**
   
   `rip_clear_changed_flag` is called exclusively from `rip_triggered_update`. When `rip_event(RIP_TRIGGERED_UPDATE)` is called while `t_triggered_interval` is already running, it sets `rip->trigger = 1` instead of scheduling `t_triggered_update`. If `rip_update` (the periodic timer) fires before `t_triggered_interval` expires, it cancels `t_triggered_interval` and clears `trigger`, leaving `t_triggered_update` unscheduled and `rip_clear_changed_flag` never called. `RIP_RTF_POLICY_WITHDRAW` then remains set, causing every subsequent periodic update to emit metric=16 for the affected routes rather than simply omitting them — a behaviour that persists until the route entry is replaced (e.g., by the zebra re-redistribution triggered from `apply_finish`) or times out.
   
   How can I resolve this? If you propose a fix, please make it concise.

Prompt To Fix All With AI

Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
ripd/ripd.c:2666-2691
**Poison sent for routes that were never advertised**

`rip_redistribute_policy_update` marks every type-`type` route in the table with `RIP_RTF_POLICY_WITHDRAW | RIP_RTF_CHANGED` unconditionally, without first checking whether the route was actually being advertised under the old policy. Routes that were already denied by the previous route-map sit in `rip->table` (added via `rip_redistribute_add`) but have never appeared in any RIP update. When the triggered update fires, `rip_output_process` will enter the `RIP_RTF_POLICY_WITHDRAW` branch for these routes and send metric=16 to all neighbors — a withdrawal for a route the neighbor never received.

While RFC 2453 compliant implementations typically ignore metric=16 for unknown routes, sending spurious unreachability messages for prefixes that were always filtered is semantically incorrect and can confuse peers with non-standard implementations. The fix would be to apply the *previous* route-map first (before overwriting `rip->redist[type].route_map`), or at least skip the flag on routes that are currently denied by the existing map, so only routes that transition from "permitted → denied" are flagged.

### Issue 2 of 2
ripd/ripd.c:2559-2577
**`RIP_RTF_POLICY_WITHDRAW` can persist indefinitely if the triggered update is suppressed**

`rip_clear_changed_flag` is called exclusively from `rip_triggered_update`. When `rip_event(RIP_TRIGGERED_UPDATE)` is called while `t_triggered_interval` is already running, it sets `rip->trigger = 1` instead of scheduling `t_triggered_update`. If `rip_update` (the periodic timer) fires before `t_triggered_interval` expires, it cancels `t_triggered_interval` and clears `trigger`, leaving `t_triggered_update` unscheduled and `rip_clear_changed_flag` never called. `RIP_RTF_POLICY_WITHDRAW` then remains set, causing every subsequent periodic update to emit metric=16 for the affected routes rather than simply omitting them — a behaviour that persists until the route entry is replaced (e.g., by the zebra re-redistribution triggered from `apply_finish`) or times out.

_{Reviews (1): Last reviewed commit: "ripd: poison redistributed routes on rou..." | Re-trigger Greptile}

[donaldsharp]

why not just set a flag on the rinfo when the route map is applied once every 30 seconds on what choice was made. Then you just compare the old value to the new route map application and do the right thing. Then we don't need this loop at all.

[riw777]

Signed off by needs to be fixed. Waiting on @donaldsharp 's comments as well.

[Evelyn0828]

Sign-off email fixed (jxshuibei@gmail.com) and force-pushed.

[Evelyn0828]

Thanks @donaldsharp, reworked along the lines you suggested.

Dropped the separate rip_redistribute_policy_update() table walk and the RIP_RTF_POLICY_WITHDRAW flag entirely. Instead each route now records the decision made the last time its redistribute route-map was applied (RIP_RTF_POLICY_ADVERTISED, set in rip_output_process() when the route passes the map). On a later update, if the route is now denied and was previously advertised, it is poisoned once with metric 16 and the flag is cleared; routes that were never advertised are skipped as before.

Since the comparison happens on the regular update where the route-map is already applied, there's no extra loop and no config-time trigger. This also resolves the two greptile notes: routes denied from the start are never flagged (no spurious metric 16), and there is no separate withdraw flag that could persist across a suppressed triggered update.

[mergify]

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request

[riw777]

Can you fix the linter errors? :-)

[Evelyn0828]

Fixed the style — applied the frrbot/clang-format suggestion and force-pushed. Thanks @riw777.

[riw777]

looks good, will wait a bit to see if @donaldsharp has the time to check his comments and look at this again