OPNET-785: Add FRR peer-file rendering for BGP-based VIP management - #395
OPNET-785: Add FRR peer-file rendering for BGP-based VIP management#395mkowalski wants to merge 3 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@mkowalski: This pull request references OPNET-785 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughThe render command adds optional FRR peer-file handling, new FRR configuration models and helpers, and template functions for IPv4 and IPv6 classification. ChangesFRR rendering
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RenderCommand
participant FRRPeerMapping
participant FRRRenderConfig
participant RenderFile
RenderCommand->>FRRPeerMapping: Load FRR peer mapping
RenderCommand->>FRRRenderConfig: Build FRR render configuration
RenderCommand->>RenderFile: Render template with selected configuration
RenderFile->>RenderFile: Apply IPv4 and IPv6 template helpers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mkowalski The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/render/render.go (1)
23-34: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueCorrect usage of
template.New(name).Funcs(...).ParseFiles(...).Naming the template by
filepath.Base(templatePath)beforeParseFilesensures the parsed definition matches the root template used byExecute. Logic looks correct.Minor:
funcMapis rebuilt on everyRenderFilecall (once per template inRender's loop). It's cheap, but could be hoisted to a package-levelvarsince it's stateless.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/render/render.go` around lines 23 - 34, Hoist the stateless template function map out of RenderFile so it is not rebuilt for every render. Define a package-level FuncMap containing the existing isIPv4 and isIPv6 functions, then have RenderFile use that shared map when calling template.New(name).Funcs(...).pkg/config/node.go (1)
1042-1052: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConfirm
mappingis never nil at call sites.
BuildFRRRenderConfigdereferencesmappingunconditionally; a nilmappingwill panic. Current caller (cmd/runtimecfg/render.go) only calls this after a successfulLoadFRRPeerMapping, so it's safe today, but there's no defensive nil-check here if future callers don't follow that contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/config/node.go` around lines 1042 - 1052, Verify every call to BuildFRRRenderConfig, especially cmd/runtimecfg/render.go, passes a non-nil mapping after successful LoadFRRPeerMapping; add or preserve validation at caller boundaries and document the non-nil contract, or add defensive handling in BuildFRRRenderConfig if future callers may provide nil.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/runtimecfg/render.go`:
- Around line 131-142: Handle the error returned by
cmd.Flags().GetString("peer-file") in the surrounding command logic, returning
it immediately if non-nil, consistent with the existing GetUint16 and
GetString("cluster-config") handling before using peerFilePath.
In `@pkg/config/node.go`:
- Around line 102-111: Prevent FRR peer passwords from being exposed by the
render pipeline: update render.RenderFile so rendered FRR configuration output
is not logged wholesale, or redact the Password value before each line is sent
to log.Info. Preserve the password in the template context and generated
configuration, while ensuring the FRRPeer-to-Peers path through
BuildFRRRenderConfig cannot emit plaintext secrets in logs.
---
Nitpick comments:
In `@pkg/config/node.go`:
- Around line 1042-1052: Verify every call to BuildFRRRenderConfig, especially
cmd/runtimecfg/render.go, passes a non-nil mapping after successful
LoadFRRPeerMapping; add or preserve validation at caller boundaries and document
the non-nil contract, or add defensive handling in BuildFRRRenderConfig if
future callers may provide nil.
In `@pkg/render/render.go`:
- Around line 23-34: Hoist the stateless template function map out of RenderFile
so it is not rebuilt for every render. Define a package-level FuncMap containing
the existing isIPv4 and isIPv6 functions, then have RenderFile use that shared
map when calling template.New(name).Funcs(...).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e93804a1-73e4-4b72-bea4-64b9b3a738e3
📒 Files selected for processing (3)
cmd/runtimecfg/render.gopkg/config/node.gopkg/render/render.go
| // Check if FRR peer file is provided for FRR config rendering | ||
| peerFilePath, _ := cmd.Flags().GetString("peer-file") | ||
| if peerFilePath != "" { | ||
| peerMapping, err := config.LoadFRRPeerMapping(peerFilePath) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| frrConfig := config.BuildFRRRenderConfig(cfg, peerMapping) | ||
| return render.Render(outDir, args[1:], frrConfig) | ||
| } | ||
|
|
||
| return render.Render(outDir, args[1:], cfg) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Ignored error from GetString("peer-file").
peerFilePath, _ := cmd.Flags().GetString("peer-file")discards the error return. Other flags in this function (e.g. GetUint16, GetString("cluster-config")) properly check and return errors.
As per path instructions, "Never ignore error returns."
🐛 Proposed fix
- peerFilePath, _ := cmd.Flags().GetString("peer-file")
+ peerFilePath, err := cmd.Flags().GetString("peer-file")
+ if err != nil {
+ return err
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/runtimecfg/render.go` around lines 131 - 142, Handle the error returned
by cmd.Flags().GetString("peer-file") in the surrounding command logic,
returning it immediately if non-nil, consistent with the existing GetUint16 and
GetString("cluster-config") handling before using peerFilePath.
Source: Path instructions
| // FRRPeer defines a BGP peer for FRR configuration rendering. | ||
| type FRRPeer struct { | ||
| PeerAddress string `json:"peerAddress"` | ||
| PeerASN int64 `json:"peerASN"` | ||
| Password string `json:"password,omitempty"` | ||
| BFDEnabled string `json:"bfdEnabled,omitempty"` | ||
| EBGPMultiHop string `json:"ebgpMultiHop,omitempty"` | ||
| HoldTime string `json:"holdTime,omitempty"` | ||
| KeepaliveTime string `json:"keepaliveTime,omitempty"` | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
BGP peer Password will be leaked into logs via the render pipeline.
FRRPeer.Password is plumbed through BuildFRRRenderConfig → Peers → the FRR template context, and pkg/render/render.go's RenderFile logs the entire rendered template output line-by-line via log.Info(line). If the frr.conf.tmpl template emits the neighbor password (which it must, to configure BGP session auth), the plaintext password will be written to application logs. Consider redacting secret fields before logging rendered output, or excluding FRR-config rendering from the full-buffer log dump.
As per coding guidelines, "Flag logging that may expose passwords, tokens, API keys, PII (email, SSN, credit card), session IDs, internal hostnames, or customer data."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/config/node.go` around lines 102 - 111, Prevent FRR peer passwords from
being exposed by the render pipeline: update render.RenderFile so rendered FRR
configuration output is not logged wholesale, or redact the Password value
before each line is sent to log.Info. Preserve the password in the template
context and generated configuration, while ensuring the FRRPeer-to-Peers path
through BuildFRRRenderConfig cannot emit plaintext secrets in logs.
Source: Coding guidelines
|
@mkowalski: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
What
Adds
runtimecfg render --peer-filesupport for rendering FRR configuration from a JSON peer mapping, consumed by the frr-k8s static pod introduced by BGP-based VIP management (openshift/enhancements#1982):FRRPeerMapping:localASN,defaultPeers,hostOverrideskeyed by short hostname,communities): per-node peer list resolution — hosts with an override use it, everything else falls back to the defaults. This is the two-phase rendering pattern keepalived uses today (identical file delivered to all nodes by MCO; node-specific config rendered locally at pod start).--peer-fileflag onrender: when set, templates receive FRR-specific data (.Hostname,.RouterID,.LocalASN,.Peers,.Communities) instead of the standard Node struct. Works without a live API server (bootstrap-safe): cluster name/domain parse locally and the router-id comes from the node's non-virtual IP.isIPv4/isIPv6template functions for address-family-aware FRR templates.The peer-file schema is shared verbatim with the installer-generated
bgp-vip-configConfigMap (extra keys are ignored), so one schema flows from install-config to the node.Validation
Consumed end to end by the reference implementation of enhancement 1982 across 14 dev-scripts baremetal installs (bootstrap + day-2 rendering on all control plane nodes): https://github.com/mkowalski/bgp-vip-demo. Unit tests included;
go test ./...green.Part of OPNET-595; this repo's subtask: OPNET-785.
Summary by CodeRabbit
New Features
Bug Fixes