OPNET-783: BGP-based VIP management: FRRConfiguration rendering, DaemonSet placement and static pod RBAC#3047
OPNET-783: BGP-based VIP management: FRRConfiguration rendering, DaemonSet placement and static pod RBAC#3047mkowalski wants to merge 3 commits into
Conversation
|
@mkowalski: This pull request references OPNET-783 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. |
openshift-ci-robot
added
the
jira/valid-reference
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds BGP-based VIP management for BareMetal clusters, renders FRR configuration from a ConfigMap, updates FRR-K8s scheduling and rendering, adds static pod RBAC resources, and introduces coverage for single-stack, dual-stack, optional-field, and scheduling behavior. ChangesBGP VIP FRR integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Render
participant Infrastructure
participant ConfigMap
participant FRRBuilder
participant FRRConfiguration
Render->>Infrastructure: evaluate BGP VIP management
Render->>ConfigMap: read bgp-vip-config
ConfigMap-->>Render: return config.json
Render->>FRRBuilder: build FRR configuration objects
FRRBuilder-->>FRRConfiguration: return generated resources
Render-->>Render: apply BGP VIP scheduling state
🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mkowalski The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/network/render.go (1)
132-143: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGate BGP VIP FRRConfiguration rendering in
pkg/network/render.go:138-143.
renderBGPVIPFRRConfigurationstill runs whenever BGP VIP management is active, even ifRoutingCapabilitiesProviderFRRisn't enabled. That can emitFRRConfigurationobjects before thefrr-k8sCRD/manifests exist, so this should be gated on the FRR provider too.🤖 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/network/render.go` around lines 132 - 143, Gate the renderBGPVIPFRRConfiguration call in the rendering flow so it executes only when BGP VIP management is active and RoutingCapabilitiesProviderFRR is enabled. Preserve the existing error handling and object appending within that conditional block.
🧹 Nitpick comments (1)
pkg/network/render.go (1)
859-868: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDuplicate
isBGPVIPManagementcheck causes a redundant Infrastructure CR fetch per render.
isBGPVIPManagementis called here and again insiderenderBGPVIPFRRConfiguration(bgp_vip.go), each independently issuing a dynamic-clientGeton the Infrastructure CR everyRender()invocation. Compute it once inRender()and pass the boolean into both functions.🤖 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/network/render.go` around lines 859 - 868, Compute the BGP VIP management state once in Render() and pass the resulting boolean through the rendering call chain, including renderAdditionalRoutingCapabilities and renderBGPVIPFRRConfiguration. Remove the repeated isBGPVIPManagement call from renderAdditionalRoutingCapabilities and renderBGPVIPFRRConfiguration, updating their signatures and callers to use the shared value.
🤖 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 `@bindata/network/frr-k8s/003-static-pod-rbac.yaml`:
- Around line 10-53: Restrict the FRR static-pod RBAC in the ClusterRole named
frr-k8s-static-pod: render this ClusterRole and its associated
ClusterRoleBinding only when BGP VIP management/bootstrap is active, not merely
when AdditionalRoutingCapabilities is enabled, and reduce node-state permissions
to the minimum required by the static pod, removing unnecessary destructive
verbs such as delete. Update render tests, including the bootstrapResult == nil
case, to verify the resources are omitted when BGP VIP management is inactive.
In `@pkg/network/bgp_vip.go`:
- Line 77: Replace context.TODO() in Render(), renderBGPVIPFRRConfiguration, and
the other affected client calls with a real bounded context.Context propagated
through the call chain; derive it from the reconcile/request context and apply
an appropriate timeout or cancellation before each Kubernetes API call.
- Around line 307-311: Handle the error returned by uns.NestedString in the
session status check within the BGP status function, rather than discarding it.
If extracting status or encountering a type mismatch fails, return the error to
the caller; otherwise continue checking whether status equals "Established".
- Around line 44-53: Update buildFRRConfigurationObjects and
buildBGPVIPRawConfig to propagate bgpVIPConfigData.Communities and HostOverrides
into the rendered FRRConfiguration, applying host-specific peers from
HostOverrides instead of only DefaultPeers and preserving configured route
communities. Ensure buildBGPVIPRawConfig includes both fields in its generated
raw configuration so installer and administrator settings are not discarded.
---
Outside diff comments:
In `@pkg/network/render.go`:
- Around line 132-143: Gate the renderBGPVIPFRRConfiguration call in the
rendering flow so it executes only when BGP VIP management is active and
RoutingCapabilitiesProviderFRR is enabled. Preserve the existing error handling
and object appending within that conditional block.
---
Nitpick comments:
In `@pkg/network/render.go`:
- Around line 859-868: Compute the BGP VIP management state once in Render() and
pass the resulting boolean through the rendering call chain, including
renderAdditionalRoutingCapabilities and renderBGPVIPFRRConfiguration. Remove the
repeated isBGPVIPManagement call from renderAdditionalRoutingCapabilities and
renderBGPVIPFRRConfiguration, updating their signatures and callers to use the
shared value.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: af2a1a47-ab87-42a3-9874-a6d81393a5e8
📒 Files selected for processing (6)
bindata/network/frr-k8s/003-static-pod-rbac.yamlbindata/network/frr-k8s/frr-k8s.yamlpkg/network/bgp_vip.gopkg/network/bgp_vip_test.gopkg/network/render.gopkg/network/render_test.go
| apiVersion: rbac.authorization.k8s.io/v1 | ||
| kind: ClusterRole | ||
| metadata: | ||
| name: frr-k8s-static-pod | ||
| rules: | ||
| - apiGroups: | ||
| - frrk8s.metallb.io | ||
| resources: | ||
| - frrconfigurations | ||
| - frrk8sconfigurations | ||
| verbs: | ||
| - get | ||
| - list | ||
| - watch | ||
| - apiGroups: | ||
| - frrk8s.metallb.io | ||
| resources: | ||
| - frrnodestates | ||
| - bgpsessionstates | ||
| verbs: | ||
| - get | ||
| - list | ||
| - watch | ||
| - create | ||
| - update | ||
| - patch | ||
| - delete | ||
| - apiGroups: | ||
| - frrk8s.metallb.io | ||
| resources: | ||
| - frrnodestates/status | ||
| - bgpsessionstates/status | ||
| verbs: | ||
| - get | ||
| - update | ||
| - patch | ||
| - apiGroups: | ||
| - "" | ||
| resources: | ||
| - nodes | ||
| verbs: | ||
| - get | ||
| - list | ||
| - watch |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Broad, cluster-wide RBAC grant to a shared bootstrap ServiceAccount, applied unconditionally.
Two concerns:
- This ClusterRole/ClusterRoleBinding is rendered whenever the FRR
AdditionalRoutingCapabilitiesprovider is enabled — not conditioned on BGP VIP management being active (confirmed byrender_test.go, where the object count is identical withbootstrapResult == nil). Clusters using FRR for unrelated BGP use cases get this grant even though the static pod it's meant for never runs. node-bootstrapperis a single ServiceAccount used across every node's bootstrap process; granting it cluster-widecreate/update/patch/deleteonfrrnodestates/bgpsessionstates(not scoped to a single node's own objects, since plain RBAC can't express that) means a compromised node's bootstrap credential could tamper with every other node's BGP/FRR state.
As per path instructions, RBAC for Kubernetes manifests should follow "least privilege; no cluster-admin for workloads". Consider scoping the verbs down (e.g. dropping delete) and/or gating this manifest on BGP VIP management being active.
Also applies to: 55-66
🤖 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 `@bindata/network/frr-k8s/003-static-pod-rbac.yaml` around lines 10 - 53,
Restrict the FRR static-pod RBAC in the ClusterRole named frr-k8s-static-pod:
render this ClusterRole and its associated ClusterRoleBinding only when BGP VIP
management/bootstrap is active, not merely when AdditionalRoutingCapabilities is
enabled, and reduce node-state permissions to the minimum required by the static
pod, removing unnecessary destructive verbs such as delete. Update render tests,
including the bootstrapResult == nil case, to verify the resources are omitted
when BGP VIP management is inactive.
Source: Path instructions
| // bgpVIPConfigData is the parsed content of the bgp-vip-config ConfigMap's | ||
| // config.json key. The schema matches baremetal-runtimecfg's FRRPeerMapping. | ||
| type bgpVIPConfigData struct { | ||
| LocalASN int64 `json:"localASN"` | ||
| DefaultPeers []bgpVIPPeer `json:"defaultPeers"` | ||
| Communities []string `json:"communities,omitempty"` | ||
| APIVIPs []string `json:"apiVIPs"` | ||
| IngressVIPs []string `json:"ingressVIPs"` | ||
| HostOverrides map[string][]bgpVIPPeer `json:"hostOverrides,omitempty"` | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## File map\n'
ast-grep outline pkg/network/bgp_vip.go --view expanded || true
printf '\n## Search for field usages\n'
rg -n "Communities|HostOverrides|buildFRRConfigurationObjects|bgpVIPConfigData" pkg/network
printf '\n## Read relevant file sections\n'
wc -l pkg/network/bgp_vip.go pkg/network/bgp_vip_test.go
sed -n '1,240p' pkg/network/bgp_vip.go
printf '\n--- TESTS ---\n'
sed -n '1,260p' pkg/network/bgp_vip_test.goRepository: openshift/cluster-network-operator
Length of output: 22402
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## Read remaining bgp_vip.go section\n'
sed -n '240,340p' pkg/network/bgp_vip.go
printf '\n## Search for related terms in repo\n'
rg -n "communities|hostOverrides|defaultPeers|apiVIPs|ingressVIPs" pkg/networkRepository: openshift/cluster-network-operator
Length of output: 4736
BGP VIP config drops communities and hostOverrides
buildFRRConfigurationObjects and buildBGPVIPRawConfig only consume DefaultPeers, APIVIPs, and IngressVIPs; Communities and HostOverrides never affect the rendered FRRConfiguration. Any installer/admin config relying on per-host peer overrides or route communities is silently ignored.
🤖 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/network/bgp_vip.go` around lines 44 - 53, Update
buildFRRConfigurationObjects and buildBGPVIPRawConfig to propagate
bgpVIPConfigData.Communities and HostOverrides into the rendered
FRRConfiguration, applying host-specific peers from HostOverrides instead of
only DefaultPeers and preserving configured route communities. Ensure
buildBGPVIPRawConfig includes both fields in its generated raw configuration so
installer and administrator settings are not discarded.
| // vipManagement field. | ||
| infra, err := client.Default().Dynamic().Resource(schema.GroupVersionResource{ | ||
| Group: "config.openshift.io", Version: "v1", Resource: "infrastructures", | ||
| }).Get(context.TODO(), "cluster", metav1.GetOptions{}) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use a real context instead of context.TODO() for API calls.
All three client calls in this file use context.TODO(), giving no cancellation or timeout. These are external-call hot-paths executed during reconcile (Render() → renderBGPVIPFRRConfiguration); an unresponsive API server would block indefinitely with no bound.
As per path instructions, **/*.go files should use "context.Context for cancellation and timeouts".
🕐 Proposed fix (threading a bounded context)
- infra, err := client.Default().Dynamic().Resource(schema.GroupVersionResource{
- Group: "config.openshift.io", Version: "v1", Resource: "infrastructures",
- }).Get(context.TODO(), "cluster", metav1.GetOptions{})
+ ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+ defer cancel()
+ infra, err := client.Default().Dynamic().Resource(schema.GroupVersionResource{
+ Group: "config.openshift.io", Version: "v1", Resource: "infrastructures",
+ }).Get(ctx, "cluster", metav1.GetOptions{})Also applies to: 105-105, 303-303
🤖 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/network/bgp_vip.go` at line 77, Replace context.TODO() in Render(),
renderBGPVIPFRRConfiguration, and the other affected client calls with a real
bounded context.Context propagated through the call chain; derive it from the
reconcile/request context and apply an appropriate timeout or cancellation
before each Kubernetes API call.
Source: Path instructions
| for _, session := range sessionList.Items { | ||
| status, _, _ := uns.NestedString(session.Object, "status", "bgpStatus") | ||
| if status != "Established" { | ||
| return false, nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Don't discard the error from uns.NestedString.
status, _, _ := uns.NestedString(...) silently swallows a type-mismatch error, which could mask a malformed BGPSessionState object.
As per path instructions, Go files should "Never ignore error returns".
🛡️ Proposed fix
- status, _, _ := uns.NestedString(session.Object, "status", "bgpStatus")
+ status, _, err := uns.NestedString(session.Object, "status", "bgpStatus")
+ if err != nil {
+ return false, err
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for _, session := range sessionList.Items { | |
| status, _, _ := uns.NestedString(session.Object, "status", "bgpStatus") | |
| if status != "Established" { | |
| return false, nil | |
| } | |
| for _, session := range sessionList.Items { | |
| status, _, err := uns.NestedString(session.Object, "status", "bgpStatus") | |
| if err != nil { | |
| return false, err | |
| } | |
| if status != "Established" { | |
| return false, nil | |
| } |
🤖 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/network/bgp_vip.go` around lines 307 - 311, Handle the error returned by
uns.NestedString in the session status check within the BGP status function,
rather than discarding it. If extracting status or encountering a type mismatch
fails, return the error to the caller; otherwise continue checking whether
status equals "Established".
Source: Path instructions
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
@mkowalski: The following test 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. |
Render a sessions-only FRRConfiguration CR named bgp-vip (namespace openshift-frr-k8s) when BGP-based VIP management is active: BareMetal platform, the BGPBasedVIPManagement feature gate enabled, and the Infrastructure CR reporting vipManagement "BGP". Peer/VIP data is read from the installer's bgp-vip-config ConfigMap (schema = baremetal-runtimecfg's FRRPeerMapping). The CR carries only the BGP sessions (neighbors, BFD profiles). VIP advertisement deliberately does not use CRD prefixes/toAdvertise: frr-k8s renders router-level prefixes as unconditional `network` statements that would defeat kube-vip's health gating, and toAdvertise cannot express "advertise redistributed routes" - without it frr-k8s renders deny-any egress prefix-lists. Advertisement therefore happens exclusively via rawConfig: `ip import-table 198` plus health-gated table-direct redistribution of routing table 198 (populated by kube-vip only for VIPs with healthy backends), filtered through route-maps/prefix-lists permitting exactly the VIP prefixes, with high-sequence per-neighbor <peer>-out permits opening egress only for the VIP prefix-lists. The CR carries no node selector: workers' frr-k8s DaemonSet consumes the same sessions and gated redistribution so router-bearing workers advertise the ingress VIP. The feature gate is defined locally and guarded via KnownFeatures until openshift/api#2923 ships the gate, and the Infrastructure vipManagement field is read unstructured until it lands in the vendored openshift/api - the feature is inert until then. Validated end to end (github.com/mkowalski/bgp-vip-demo).
…anagement Under BGP VIP management the masters run the frr-k8s static pod (deployed by MCO), which owns the FRR sessions for the VIPs; the frr-k8s DaemonSet must therefore not schedule there. Label-based approaches fail: NodeRestriction prevents nodes from setting the exclusion label on themselves, and any external labeler races with the DaemonSet controller at node bring-up. A role-based required node-affinity (node-role.kubernetes.io/master DoesNotExist) has neither problem, so render it into the DaemonSet whenever BGP VIP management is active. renderAdditionalRoutingCapabilities gains the client, bootstrap result, and feature gates needed to make that call, reusing the same isBGPVIPManagement helper as the FRRConfiguration rendering.
The frr-k8s static pod on control plane nodes has no ServiceAccount; its controller and frr-status containers authenticate with the node kubeconfig (/etc/kubernetes/kubeconfig), whose identity is the MCO node-bootstrapper ServiceAccount (verified live via oc auth whoami). Grant that identity read access to FRRConfigurations and FRRK8sConfigurations, write access to the node state CRs (FRRNodeStates, BGPSessionStates and their statuses), and the namespace-scoped secrets/pods reads that 002-rbac.yaml gives the DaemonSet service account, so the static pod controller's informer caches can sync. Known limitation: these grants are not scoped per-node - any holder of the node-bootstrapper credential can write any node's state CRs. Admission-level enforcement is needed before GA.
mkowalski
force-pushed
the
bgp-vip-management
branch
from
f9ce13a to
70cd187
Compare
2 participants
What
CNO side of BGP-based VIP management (enhancement openshift/enhancements#1982, epic OPNET-595). Three commits:
bgp-vip-configConfigMap and renders abgp-vip-masterFRRConfiguration carrying the BGP sessions (neighbors, passwords, BFD). Advertisement deliberately stays out of the CRD surface (CRD prefixes render as unconditionalnetworkstatements, defeating kube-vip's health gating — validated failure mode) and is carried inrawConfig:ip import-table 198, per-family health-gatedredistribute table-direct 198behind route-map/prefix-list filters, plus high-sequence permits appended to frr-k8s's generated per-neighbor-outroute-maps (frr-k8s renders deny-any egress withouttoAdvertise; prefix-list deny = route-map no-match = fall-through, so egress opens exactly for the VIP prefixes).openshift-machine-config-operator/node-bootstrapperServiceAccount (verified live) — grants for FRRConfiguration/FRRK8sConfiguration reads, node state CR writes, and namespace-scoped secrets/pods reads. Per-node write scoping is a documented pre-GA follow-up (needs admission-level enforcement).Merge safety — no openshift/api dependency
openshift/api#2923 (OPNET-780) is not merged yet, so this PR deliberately avoids the new vendored types:
BGPBasedVIPManagementgate name is a localconfigv1.FeatureGateNameconstant; aKnownFeaturesguard makes every new path a no-op on any cluster whose FeatureGate status does not know the gate — i.e. this code is inert everywhere until OPNET-780: Add BGPBasedVIPManagement feature gate and BGP VIP management fields api#2923 and its cluster-config plumbing ship.Infrastructure.status.platformStatus.baremetal.vipManagementis read unstructured (the CRD field is equally gated api-side).A small follow-up will switch both to typed access once the api merges and re-vendors. No vendor/, go.mod or default-behavior changes;
featureSet: Default/TechPreview clusters are untouched.Testing
Unit tests per commit (schema round-trip incl. full installer-shaped payloads and dual-stack, rawConfig content, render-var/affinity branches, bindata object counts);
go test ./pkg/...fully green. Validated end to end in the enhancement reference implementation across 14 dev-scripts baremetal installs — install over a BGP-advertised API VIP, health-gated ECMP for API+Ingress VIPs, CRD handover: https://github.com/mkowalski/bgp-vip-demo.Related: #3046 (statusmanager fix this feature exposed), openshift/api#2923, openshift/baremetal-runtimecfg#395, openshift-metal3/dev-scripts#1929, kube-vip/kube-vip#1627.
CNO subtask: OPNET-783.