cnf network: add templates for frr config#1270
Open
ajaggapa wants to merge 1 commit intorh-ecosystem-edge:mainfrom
Open
cnf network: add templates for frr config#1270ajaggapa wants to merge 1 commit intorh-ecosystem-edge:mainfrom
ajaggapa wants to merge 1 commit intorh-ecosystem-edge:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a new frrconfig package that templates FRR Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.3)Command failed Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/cnf/core/network/internal/frrconfig/frrconfig.go`:
- Around line 158-180: The StaticRoute model currently only has Destination and
Nexthop so BuildStaticRoutes loses address-family info; update the StaticRoute
struct to include an AddressFamily (e.g., "ipv4" / "ipv6" or an enum) and set it
inside BuildStaticRoutes when you detect IP.To4() != nil (set "ipv4") or else
"ipv6", including for appended host-prefixes (/32 vs /128); ensure any
code/templates that consume StaticRoute (the template rendering the
static-route) are updated to branch on AddressFamily to render the correct
family-specific FRR stanza.
- Around line 168-172: The BuildStaticRoutes helper assumes destinations and
nexthops are equal length but doesn't check, causing index panics or silent
truncation; add a length check at the top of BuildStaticRoutes that compares
len(destinations) and len(nexthops) and fail fast with a clear error (e.g.,
panic or returning an error) when they differ, including both lengths in the
message so callers can see the mismatch; keep the rest of the loop over
destinations/nexthops intact (use the existing variables destinations, nexthops,
BuildStaticRoutes and routes) so the function no longer panics silently on
out-of-range indexes.
In `@tests/cnf/core/network/internal/frrconfig/templates.go`:
- Around line 132-145: The template always emits a trailing "neighbor unnumbered
bfd" regardless of .Unnumbered.EnableBFD, so change the block that renders
unnumbered neighbor peers to only output "neighbor unnumbered bfd" when
.Unnumbered.EnableBFD is true (remove the unconditional emission); in other
words, ensure the existing conditional that checks .Unnumbered.EnableBFD is the
sole producer of the "neighbor unnumbered bfd" line and delete the
duplicate/unconditional line so the no-BFD path (when .Unnumbered.EnableBFD is
false) actually renders without BFD.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1723c2b5-e0fa-44d5-856d-a1308da66735
📒 Files selected for processing (7)
tests/cnf/core/network/internal/frrconfig/frrconfig.gotests/cnf/core/network/internal/frrconfig/templates.gotests/cnf/core/network/metallb/tests/bgp-remote-as-dynamic.gotests/cnf/core/network/metallb/tests/bgp-tests.gotests/cnf/core/network/metallb/tests/bgp-unnumbered.gotests/cnf/core/network/metallb/tests/common.gotests/cnf/core/network/metallb/tests/frrk8-tests.go
ajaggapa
force-pushed
the
frrconfigtemplate
branch
2 times, most recently
from
998b75f to
900650d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/cnf/core/network/metallb/tests/frrk8-tests.go`:
- Around line 439-449: Add a brief inline comment above the
hub0BRstaticIPAnnotation and hub1BRstaticIPAnnotation assignments (where
CreateStaticIPAnnotations is called) explaining the intentional index swap of
hubIPv4ExternalAddresses (hub0 uses [1], hub1 uses [0]) — state the reason
(e.g., to validate cross-pod/cross-hub connectivity or match a specific test
topology) and note that this pattern is intentional and mirrored in related
tests like bgp-remote-as-dynamic.go to avoid confusion for future readers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 28ae219d-0528-49c5-b3c9-1612bbeabcc7
📒 Files selected for processing (7)
tests/cnf/core/network/internal/frrconfig/frrconfig.gotests/cnf/core/network/internal/frrconfig/templates.gotests/cnf/core/network/metallb/tests/bgp-remote-as-dynamic.gotests/cnf/core/network/metallb/tests/bgp-tests.gotests/cnf/core/network/metallb/tests/bgp-unnumbered.gotests/cnf/core/network/metallb/tests/common.gotests/cnf/core/network/metallb/tests/frrk8-tests.go
✅ Files skipped from review due to trivial changes (3)
- tests/cnf/core/network/metallb/tests/bgp-tests.go
- tests/cnf/core/network/metallb/tests/common.go
- tests/cnf/core/network/internal/frrconfig/templates.go
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/cnf/core/network/metallb/tests/bgp-remote-as-dynamic.go
- tests/cnf/core/network/internal/frrconfig/frrconfig.go
ajaggapa
force-pushed
the
frrconfigtemplate
branch
from
900650d to
aa4e39b
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/cnf/core/network/internal/frrconfig/frrconfig.go (1)
187-205:⚠️ Potential issue | 🟠 MajorFail fast on mismatched route slices.
The docstring says this helper panics on length mismatch, but
nexthops[idx]is still indexed without any guard. A shorternexthopsslice will panic here, and a longer one is silently ignored.🛠️ Proposed fix
func BuildStaticRoutes(destinations, nexthops []string) []StaticRoute { - routes := []StaticRoute{} + if len(destinations) != len(nexthops) { + panic(fmt.Sprintf( + "destinations and nexthops length mismatch: %d != %d", + len(destinations), len(nexthops))) + } + + routes := make([]StaticRoute, 0, len(destinations)) for idx := 0; idx < len(destinations); idx++ {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cnf/core/network/internal/frrconfig/frrconfig.go` around lines 187 - 205, BuildStaticRoutes currently indexes nexthops[idx] without validating slice lengths; add an explicit fast-fail check at the start of BuildStaticRoutes that compares len(destinations) and len(nexthops) and panics (per the docstring) with a clear message if they differ, so shorter nexthops won't cause an out-of-range panic and longer nexthops aren't silently ignored; keep the rest of the function (destination normalization and StaticRoute construction using addressFamilyForDestination) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/cnf/core/network/internal/frrconfig/frrconfig.go`:
- Around line 333-338: WithStaticRoute currently appends the destination
verbatim causing mismatch with BuildStaticRoutes; update WithStaticRoute to run
the incoming destination through the same normalization used by
BuildStaticRoutes (the helper that upgrades bare IPv4/IPv6 addresses to /32 or
/128) before appending to b.params.StaticRoutes so both one-at-a-time and bulk
routes produce identical AddressFamily and CIDR-normalized Destination values
(refer to WithStaticRoute, BuildStaticRoutes, and addressFamilyForDestination to
locate the change).
---
Duplicate comments:
In `@tests/cnf/core/network/internal/frrconfig/frrconfig.go`:
- Around line 187-205: BuildStaticRoutes currently indexes nexthops[idx] without
validating slice lengths; add an explicit fast-fail check at the start of
BuildStaticRoutes that compares len(destinations) and len(nexthops) and panics
(per the docstring) with a clear message if they differ, so shorter nexthops
won't cause an out-of-range panic and longer nexthops aren't silently ignored;
keep the rest of the function (destination normalization and StaticRoute
construction using addressFamilyForDestination) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e6abb53e-85dc-47e4-a3c8-21462ae17760
📒 Files selected for processing (7)
tests/cnf/core/network/internal/frrconfig/frrconfig.gotests/cnf/core/network/internal/frrconfig/templates.gotests/cnf/core/network/metallb/tests/bgp-remote-as-dynamic.gotests/cnf/core/network/metallb/tests/bgp-tests.gotests/cnf/core/network/metallb/tests/bgp-unnumbered.gotests/cnf/core/network/metallb/tests/common.gotests/cnf/core/network/metallb/tests/frrk8-tests.go
✅ Files skipped from review due to trivial changes (1)
- tests/cnf/core/network/internal/frrconfig/templates.go
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/cnf/core/network/metallb/tests/bgp-remote-as-dynamic.go
- tests/cnf/core/network/metallb/tests/bgp-tests.go
- tests/cnf/core/network/metallb/tests/common.go
- tests/cnf/core/network/metallb/tests/frrk8-tests.go
Signed-off-by: Anvesh J <ajaggapa@redhat.com>
ajaggapa
force-pushed
the
frrconfigtemplate
branch
from
aa4e39b to
a5f7719
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/cnf/core/network/internal/frrconfig/frrconfig.go (1)
201-217:⚠️ Potential issue | 🟡 MinorAdd explicit length validation to match docstring claim.
The docstring states "It panics if the lengths of the slices are not equal," but the implementation relies on an implicit index-out-of-range panic when
nexthopsis shorter. Whennexthopsis longer, extra entries are silently ignored. An explicit check provides a clearer error message and consistent behavior.🛠️ Proposed fix
func BuildStaticRoutes(destinations, nexthops []string) []StaticRoute { + if len(destinations) != len(nexthops) { + panic(fmt.Sprintf("destinations and nexthops length mismatch: %d != %d", + len(destinations), len(nexthops))) + } + routes := []StaticRoute{} for idx := 0; idx < len(destinations); idx++ {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cnf/core/network/internal/frrconfig/frrconfig.go` around lines 201 - 217, The BuildStaticRoutes function currently assumes destinations and nexthops have equal lengths; add an explicit length check at the start of BuildStaticRoutes to verify len(destinations) == len(nexthops) and panic with a clear message (e.g., including both lengths) if they differ, so the behavior matches the docstring; keep the rest of the function (calls to normalizeStaticRouteDestination, creation of StaticRoute with Destination, Nexthop, AddressFamily via addressFamilyForDestination) unchanged.
🧹 Nitpick comments (1)
tests/cnf/core/network/internal/frrconfig/frrconfig.go (1)
168-183: Consider logging or returning an error for unparseable destinations.When
net.ParseIP(dest)returnsnil(e.g., for malformed input like"not-an-ip"), the function silently defaults to"ipv4". This could mask configuration errors in tests. Consider either logging a warning or returning an error for invalid input.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cnf/core/network/internal/frrconfig/frrconfig.go` around lines 168 - 183, The function addressFamilyForDestination currently swallows parse failures and returns "ipv4"; change it to signal errors by returning (string, error) instead of only string: keep the prefix-trimming logic, call net.ParseIP(dest) and if it returns nil return "", fmt.Errorf("invalid destination: %q", dest); otherwise return "ipv4"/"ipv6" and nil. Update all callers of addressFamilyForDestination to handle the error (propagate or log/fail the test) so malformed destinations like "not-an-ip" are detected rather than silently treated as IPv4.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/cnf/core/network/internal/frrconfig/frrconfig.go`:
- Around line 428-442: BGPParamsByAddressFamily calls
net.ParseIP(ipAddress).To4() which can panic if ParseIP returns nil for a
malformed/empty ipAddress; update BGPParamsByAddressFamily to first store the
result of net.ParseIP(ipAddress) in a variable, check it for nil, and handle the
invalid case (e.g., skip the address or return an error) before calling To4();
ensure the logic still calls WithIPv6BGPNeighbors or WithIPv4BGPNeighbors only
when parsedIP is non-nil and use parsedIP.String() or the original ipAddress as
appropriate.
---
Duplicate comments:
In `@tests/cnf/core/network/internal/frrconfig/frrconfig.go`:
- Around line 201-217: The BuildStaticRoutes function currently assumes
destinations and nexthops have equal lengths; add an explicit length check at
the start of BuildStaticRoutes to verify len(destinations) == len(nexthops) and
panic with a clear message (e.g., including both lengths) if they differ, so the
behavior matches the docstring; keep the rest of the function (calls to
normalizeStaticRouteDestination, creation of StaticRoute with Destination,
Nexthop, AddressFamily via addressFamilyForDestination) unchanged.
---
Nitpick comments:
In `@tests/cnf/core/network/internal/frrconfig/frrconfig.go`:
- Around line 168-183: The function addressFamilyForDestination currently
swallows parse failures and returns "ipv4"; change it to signal errors by
returning (string, error) instead of only string: keep the prefix-trimming
logic, call net.ParseIP(dest) and if it returns nil return "",
fmt.Errorf("invalid destination: %q", dest); otherwise return "ipv4"/"ipv6" and
nil. Update all callers of addressFamilyForDestination to handle the error
(propagate or log/fail the test) so malformed destinations like "not-an-ip" are
detected rather than silently treated as IPv4.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 76ed3594-ba2b-42c3-9beb-479e23dc1764
📒 Files selected for processing (7)
tests/cnf/core/network/internal/frrconfig/frrconfig.gotests/cnf/core/network/internal/frrconfig/templates.gotests/cnf/core/network/metallb/tests/bgp-remote-as-dynamic.gotests/cnf/core/network/metallb/tests/bgp-tests.gotests/cnf/core/network/metallb/tests/bgp-unnumbered.gotests/cnf/core/network/metallb/tests/common.gotests/cnf/core/network/metallb/tests/frrk8-tests.go
✅ Files skipped from review due to trivial changes (1)
- tests/cnf/core/network/internal/frrconfig/templates.go
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/cnf/core/network/metallb/tests/bgp-remote-as-dynamic.go
- tests/cnf/core/network/metallb/tests/common.go
- tests/cnf/core/network/metallb/tests/bgp-unnumbered.go
- tests/cnf/core/network/metallb/tests/frrk8-tests.go
Comment on lines
+428
to
+442
| // BGPParamsByAddressFamily returns params for BGP with each neighbor in the AF matching its IP (v4→IPv4, v6→IPv6). | ||
| func BGPParamsByAddressFamily(localAS int, remoteAddresses []string, remoteAS int, password string, | ||
| enableBFD, enableMultiHop bool) FRRConfigParams { | ||
| params := NewFRRConfigParams().WithLocalAS(localAS) | ||
|
|
||
| for _, ipAddress := range remoteAddresses { | ||
| if net.ParseIP(ipAddress).To4() == nil { | ||
| params.WithIPv6BGPNeighbors([]string{ipAddress}, remoteAS, password, enableBFD, enableMultiHop) | ||
| } else { | ||
| params.WithIPv4BGPNeighbors([]string{ipAddress}, remoteAS, password, enableBFD, enableMultiHop) | ||
| } | ||
| } | ||
|
|
||
| return params.Build() | ||
| } |
Contributor
There was a problem hiding this comment.
Potential nil pointer dereference if ipAddress is invalid.
Line 434 calls net.ParseIP(ipAddress).To4() without checking if ParseIP returns nil. If ipAddress is malformed or empty, this will panic.
🛠️ Proposed fix
func BGPParamsByAddressFamily(localAS int, remoteAddresses []string, remoteAS int, password string,
enableBFD, enableMultiHop bool) FRRConfigParams {
params := NewFRRConfigParams().WithLocalAS(localAS)
for _, ipAddress := range remoteAddresses {
- if net.ParseIP(ipAddress).To4() == nil {
+ ip := net.ParseIP(ipAddress)
+ if ip == nil {
+ // Skip invalid addresses or panic/log as appropriate
+ continue
+ }
+
+ if ip.To4() == nil {
params.WithIPv6BGPNeighbors([]string{ipAddress}, remoteAS, password, enableBFD, enableMultiHop)
} else {
params.WithIPv4BGPNeighbors([]string{ipAddress}, remoteAS, password, enableBFD, enableMultiHop)
}
}
return params.Build()
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/cnf/core/network/internal/frrconfig/frrconfig.go` around lines 428 -
442, BGPParamsByAddressFamily calls net.ParseIP(ipAddress).To4() which can panic
if ParseIP returns nil for a malformed/empty ipAddress; update
BGPParamsByAddressFamily to first store the result of net.ParseIP(ipAddress) in
a variable, check it for nil, and handle the invalid case (e.g., skip the
address or return an error) before calling To4(); ensure the logic still calls
WithIPv6BGPNeighbors or WithIPv4BGPNeighbors only when parsedIP is non-nil and
use parsedIP.String() or the original ipAddress as appropriate.
gkopels
reviewed
Mar 23, 2026
|
|
||
| // DaemonsParams holds the customizable parameters for the daemons file. | ||
| // Only Bgpd, Bfdd, and BGPPort are exposed; all other options are fixed. | ||
| type DaemonsParams struct { |
Contributor
There was a problem hiding this comment.
Hi Anvesh do we want to add this to eco-goinfra?
| } | ||
|
|
||
| // BuildStaticRoutes builds a slice of StaticRoute from parallel slices of destinations and nexthops. | ||
| // It panics if the lengths of the slices are not equal. |
Contributor
There was a problem hiding this comment.
Should you explicitly check the lenght before the loop?
| return "ipv6" | ||
| } | ||
|
|
||
| return "ipv4" |
Contributor
There was a problem hiding this comment.
Can this potentially send something other then a valid ipv4 address?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adding templates for defining
frrconfigSummary by CodeRabbit
New Features
Bug Fixes
Tests