Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a design proposal to support route redistribution from kernel routing tables in the FRRConfiguration CRD. The review feedback identifies several critical areas for improvement in the design: clarifying IPv6 and dual-stack behavior, preventing naming collisions for route-maps and prefix-lists across multiple VRFs, and resolving a contradiction in how egress filtering is applied to neighbors with explicit prefix limits versus those with unrestricted advertisement modes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a design proposal for adding route redistribution support from kernel routing tables to the FRRConfiguration CRD. The reviewer provided valuable feedback on the proposal: first, the validation logic must be updated to include redistributed prefixes in the allowed outgoing prefixes to avoid false-positive validation failures; second, the allowed table ID range should be expanded up to 4294967295 to align with modern Linux and FRR capabilities; and third, duplicate table IDs across different VRFs should be rejected because routing table IDs are global in Linux, and allowing duplicates could lead to unexpected route leaking.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Signed-off-by: Mat Kowalski <mko@redhat.com>
Signed-off-by: Mat Kowalski <mko@redhat.com>
Signed-off-by: Mat Kowalski <mko@redhat.com>
3155856 to
555cf1b
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request proposes a design for route redistribution support in the FRRConfiguration CRD, allowing users to advertise routes from a kernel routing table using table-direct. The feedback suggests clarifying the reason for deferring VRF support by referencing table-direct instead of import-table, and adding a validation rule to explicitly reject invalid CIDR blocks in the allowedPrefixes field.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Signed-off-by: Mat Kowalski <mko@redhat.com>
Signed-off-by: Mat Kowalski <mko@redhat.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a design proposal for adding route redistribution support to the FRRConfiguration CRD, allowing users to advertise routes from a kernel routing table. The feedback highlights a critical issue where appending redistributed prefixes as separate route-map rules would bypass neighbor-specific modifiers like nextHop, suggesting instead to dynamically append them to the neighbor's prefix lists. Additionally, the reviewer recommends validating and restricting the use of reserved Linux kernel routing table IDs (253, 254, 255) and ensuring that the protocol for a given table is identical when merging multiple configurations.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
|
||
| ### Validation | ||
|
|
||
| - Reject `table` outside 1-65535. This mirrors FRR's `redistribute table-direct (1-65535)`. |
There was a problem hiding this comment.
Consider rejecting or warning against the use of reserved Linux kernel routing table IDs, specifically 253 (default), 254 (main), and 255 (local).
Redistributing from these tables via table-direct is generally discouraged and dangerous, as they contain local node routes, pod/service network routes, and other system-critical paths. While allowedPrefixes acts as a safeguard, explicitly blocking or warning against these table IDs prevents accidental misconfigurations that could lead to routing loops or IP leaks.
Signed-off-by: Mat Kowalski <mko@redhat.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a design proposal for adding route redistribution support to the FRRConfiguration CRD, allowing users to advertise routes from a kernel routing table with explicit prefix filtering. The review feedback suggests several improvements to the design, including clarifying IPv6 route-map matching syntax, adding validation to prevent duplicate advertisements between allowedPrefixes and router.prefixes, scoping duplicate table checks appropriately for future protocols, and ensuring configuration merging is scoped per BGP router instance.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| ip prefix-list redistribute-default-198-allowed-ipv4 seq 2 permit 192.168.111.5/32 | ||
| ``` | ||
|
|
||
| IPv6 prefixes render the same under `address-family ipv6 unicast`, with `ipv6 prefix-list` and `-ipv6` names. |
There was a problem hiding this comment.
To avoid ambiguity during implementation, it would be helpful to explicitly state that the generated route-map for IPv6 will use match ipv6 address prefix-list instead of match ip address prefix-list.
There was a problem hiding this comment.
We assume during implementation you are smart enough to figure this out
| ### Validation | ||
|
|
||
| - Reject `table` outside 1-65535. This mirrors FRR's `redistribute table-direct (1-65535)`. | ||
| - Reject empty `allowedPrefixes` or any invalid CIDR block within it. |
There was a problem hiding this comment.
Consider adding a validation rule to reject or warn when a prefix in allowedPrefixes is also defined in router.prefixes. Since router.prefixes generates unconditional network statements, having the same prefix in both would cause it to be advertised unconditionally, defeating the purpose of the dynamic kernel-table gating for that prefix.
There was a problem hiding this comment.
We assume during configuration you are smart enough to figure this out
Signed-off-by: Mat Kowalski <mko@redhat.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request proposes a design for adding route redistribution support to the FRRConfiguration CRD, enabling route advertisements from a kernel routing table with explicit prefix filtering. The review feedback highlights two key areas for improvement: first, addressing prefix matching behavior (exact matching vs. ranges) to prevent unexpected filtering of kernel routes; and second, ensuring generated route-map and prefix-list names are properly formatted without double hyphens when using the default VRF.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Signed-off-by: Mat Kowalski <mko@redhat.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a design proposal for adding route redistribution support to the FRRConfiguration CRD, allowing route advertisement from a kernel routing table with explicit prefix filtering. The reviewer feedback suggests defining the table field as a pointer type in Go to ensure API extensibility for future protocols, and points out that the explicit deny clause in the generated FRR route-map is redundant and can be removed.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| Fields: | ||
|
|
||
| - `protocol`: only `table-direct` initially. Enum, extensible. | ||
| - `table`: kernel table id. Required for `table-direct`. |
There was a problem hiding this comment.
To ensure the API is extensible for future protocols that may not require a routing table ID (such as connected or ospf), the table field in the Go struct should be defined as a pointer type (e.g., *uint32 or *int32) rather than a plain integer. If defined as a plain uint32, the field will default to 0 when omitted in Go, which would conflict with the Minimum=1 validation or make it impossible to distinguish between an explicitly set table ID and an omitted one. Using a pointer allows the field to be truly optional (nil) for other protocols, while the webhook can enforce that it is non-nil and within 1-65535 specifically when the protocol is table-direct.
There was a problem hiding this comment.
This is implementation detail, not design phase
| redistribute table-direct 198 route-map redistribute-default-198-ipv4 | ||
| route-map redistribute-default-198-ipv4 permit 1 | ||
| match ip address prefix-list redistribute-default-198-allowed-ipv4 | ||
| route-map redistribute-default-198-ipv4 deny 2 |
There was a problem hiding this comment.
In FRR, route-maps have an implicit deny at the end of the evaluation chain. Therefore, the explicit route-map redistribute-default-198-ipv4 deny 2 clause (without any match statement) is redundant and can be omitted to keep the generated FRR configuration cleaner and more concise.
There was a problem hiding this comment.
It's deliberate and may look redundant, but for any config that is merged later, this will avoid leaking/widening the filter.
|
Hey @fedepaol, as per #469 (comment) I believe this doc is now ready to discuss |
Add metallb/frr-k8s#470 (redistribute design, open) and openshift/release#82912 (coexistence lanes, open); kube-vip#1636 already shown merged. Today-line moved to Aug 5; final phase band widened to 'CI + coexistence lanes'. 23 PRs across 12 repositories. Assisted-By: Claude Fable 5 Signed-off-by: Mat Kowalski <mko@redhat.com>
|
This PR has been automatically marked as stale because it has been open 30 days
|
|
Active, still needed |
|
This PR has been automatically marked as stale because it has been open 30 days
|
1 participant
Is this a BUG FIX or a FEATURE ?:
/kind design
What this PR does / why we need it:
We want ability for FRR to advertise rules dynamically coming from the kernel table. This enables a scenario of health checker installing routes into the kernel table and FRR announcing them when they exist and not announcing them when they do not.
The logic of health checker is completely up to the user's agent. FRR only consumes content of the kernel table.
Special notes for your reviewer:
Supersedes #469
Release note: