OPNET-783: network: Add BGP VIP management support#3047
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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary by CodeRabbit
WalkthroughAdds feature-gated BGP VIP management for BareMetal clusters, rendering validated FRR configuration from a ConfigMap and conditionally changing FRR-K8s placement, monitoring, and static-pod RBAC. ChangesBGP VIP management
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Render
participant Infrastructure
participant ConfigMap
participant FRRK8sManifests
participant FRRConfiguration
Render->>Infrastructure: check BareMetal BGP VIP mode
Render->>FRRK8sManifests: render enabled FRR-K8s resources
Render->>ConfigMap: read bgp-vip-config
ConfigMap-->>Render: return config.json
Render->>FRRConfiguration: validate and generate bgp-vip
Suggested labels: Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 4 warnings, 1 inconclusive)
✅ Passed checks (18 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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.12.2)level=error msg="Running error: context loading failed: failed to load packages: failed to load packages: failed to load with go/packages: err: exit status 1: stderr: go: inconsistent vendoring in :\n\tgithub.com/Masterminds/semver@v1.5.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/Masterminds/sprig/v3@v3.2.3: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/containernetworking/cni@v1.3.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/ghodss/yaml@v1.0.1-0.20190212211648-25d852aebe32: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/go-bindata/go-bindata@v3.1.2+incompatible: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/onsi/gomega@v1.39.1: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/ope ... [truncated 17377 characters] ... red in go.mod, but not marked as explicit in vendor/modules.txt\n\tk8s.io/gengo/v2@v2.0.0-20251215205346-5ee0d033ba5b: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tk8s.io/kms@v0.35.2: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tk8s.io/kube-aggregator@v0.35.1: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tsigs.k8s.io/randfill@v1.0.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tsigs.k8s.io/structured-merge-diff/v6@v6.3.2: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\n\tTo ignore the vendor directory, use -mod=readonly or -mod=mod.\n\tTo sync the vendor directory, run:\n\t\tgo mod vendor\n" 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
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
70cd187 to
445a366
Compare
|
@CodeRabbit help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
Status, support, documentation and community
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
mkowalski
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
bindata/network/frr-k8s/003-static-pod-rbac.yaml (1)
24-45:⚠️ Potential issue | 🟠 MajorKeep
node-bootstrapperfrom receiving cluster-wide state and Secret access.This binding gives the shared MCO ServiceAccount cluster-wide mutation of all FRR/BGP node-state objects, including deletion and status writes. It also permits
list/watchon every Secret inopenshift-frr-k8s, although the stated need is only BGP session passwords. A compromised node bootstrap credential could modify other nodes’ state or read unrelated Secret data. Use node-scoped ownership or identities, narrow the verbs and Secret access to the static pod’s actual needs, and verify the renderer emits this RBAC only when BGP VIP bootstrap is active. This remains the previous review finding; the current manifest still contains it.As per path instructions, RBAC must follow least privilege.
Also applies to: 55-66, 77-84, 94-106
🤖 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 24 - 45, Restrict the RBAC emitted by the static-pod renderer so node-bootstrapper cannot access cluster-wide FRR/BGP state or broadly read Secrets. Update the bindings covering the referenced FRR state and Secret rules to use node-scoped ownership or identities, grant only the verbs and BGP session-password Secret access required by the static pod, and ensure the renderer emits these rules only when BGP VIP bootstrap is enabled.Source: Path instructions
🧹 Nitpick comments (1)
pkg/network/render.go (1)
859-868: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
isBGPVIPManagementruns even when FRR isn't a configured provider.The check (and its Infrastructure CR GET) executes unconditionally for any non-nil
AdditionalRoutingCapabilities, but its result is only used inside theRoutingCapabilitiesProviderFRRcase (Line 878). Move the call inside that case to avoid an unnecessary API call on every render pass when FRR isn't configured.♻️ Proposed refactor
- // Under BGP VIP management the static FRR pods own the control plane - // nodes, so the frr-k8s DaemonSet must avoid masters by role. - bgpVIP, err := isBGPVIPManagement(client, bootstrapResult, featureGates) - if err != nil { - return nil, fmt.Errorf("failed to check VIPManagement mode: %v", err) - } var out []*uns.Unstructured for _, provider := range conf.AdditionalRoutingCapabilities.Providers { switch provider { case operv1.RoutingCapabilitiesProviderFRR: + // Under BGP VIP management the static FRR pods own the control + // plane nodes, so the frr-k8s DaemonSet must avoid masters by role. + bgpVIP, err := isBGPVIPManagement(client, bootstrapResult, featureGates) + if err != nil { + return nil, fmt.Errorf("failed to check VIPManagement mode: %v", err) + } data := render.MakeRenderData()🤖 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, Move the isBGPVIPManagement call and its error handling from the start of renderAdditionalRoutingCapabilities into the RoutingCapabilitiesProviderFRR case, immediately before its result is needed. Keep the existing behavior and error message for FRR, while avoiding the API call for other providers.
🤖 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/frr-k8s.yaml`:
- Around line 297-305: Update the BGP VIP management scheduling logic around
isBGPVIPManagement and the shown frr-k8s affinity so single-node and compact
BareMetal topologies retain a schedulable node. If the feature is HA-only,
disable it through isBGPVIPManagement for those topologies; otherwise replace
the unconditional master-excluding affinity with topology-aware placement while
preserving the existing HA behavior.
In `@pkg/network/bgp_vip.go`:
- Around line 225-291: The buildBGPVIPRawConfig function must validate all
values before interpolating them into raw FRR configuration. Parse each VIP as a
valid IP or CIDR and each DefaultPeers PeerAddress as a valid IP, and ensure
LocalASN is validated as an acceptable ASN before rendering; reject or safely
handle invalid inputs rather than emitting configuration. Only splice the
validated canonical values into the generated config.
- Around line 34-42: Update the BGP VIP peer configuration flow centered on
bgpVIPPeer to accept the installer-provided kubernetes.io/basic-auth Secret and
reference it through FRRConfiguration neighbor.passwordSecret instead of reading
bgp-vip-config from a ConfigMap or populating neighbor.password. Remove
plaintext password propagation while preserving the existing peer authentication
behavior.
---
Duplicate comments:
In `@bindata/network/frr-k8s/003-static-pod-rbac.yaml`:
- Around line 24-45: Restrict the RBAC emitted by the static-pod renderer so
node-bootstrapper cannot access cluster-wide FRR/BGP state or broadly read
Secrets. Update the bindings covering the referenced FRR state and Secret rules
to use node-scoped ownership or identities, grant only the verbs and BGP
session-password Secret access required by the static pod, and ensure the
renderer emits these rules only when BGP VIP bootstrap is enabled.
---
Nitpick comments:
In `@pkg/network/render.go`:
- Around line 859-868: Move the isBGPVIPManagement call and its error handling
from the start of renderAdditionalRoutingCapabilities into the
RoutingCapabilitiesProviderFRR case, immediately before its result is needed.
Keep the existing behavior and error message for FRR, while avoiding the API
call for other providers.
🪄 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: 257523d9-b259-40c4-b682-29a43a397d67
📒 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
266cf7d to
37ff2cc
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
37ff2cc to
6917c46
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
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)
138-142: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winGate BGP VIP
FRRConfigurationon FRR enablement.
BGPBasedVIPManagementis independent ofAdditionalRoutingCapabilities.Providers, butFRRConfigurationonly exists in thefrr-k8sbundle. This path can render a CR before its CRD is installed; guard it on the FRR provider or install the CRD unconditionally.🤖 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 138 - 142, Gate the renderBGPVIPFRRConfiguration call on whether the FRR provider is enabled, using the existing FRR enablement/provider configuration symbol. Keep BGPBasedVIPManagement independent, and avoid appending or rendering the FRRConfiguration unless its CRD is available; preserve the existing error handling for enabled FRR rendering.
🤖 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 70-77: Restrict the Secret RBAC rule for node-bootstrapper to the
mounted frr-k8s-certs-secret by adding resourceNames, and reduce the verbs to
get only if the pod does not require collection operations. Preserve access to
that specific certificate Secret while removing namespace-wide Secret
permissions.
---
Outside diff comments:
In `@pkg/network/render.go`:
- Around line 138-142: Gate the renderBGPVIPFRRConfiguration call on whether the
FRR provider is enabled, using the existing FRR enablement/provider
configuration symbol. Keep BGPBasedVIPManagement independent, and avoid
appending or rendering the FRRConfiguration unless its CRD is available;
preserve the existing error handling for enabled FRR rendering.
🪄 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: 45e0fdea-5e7c-499e-89c5-0f1c9280a6e1
📒 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
2d3fd17 to
f0979ba
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
bindata/network/frr-k8s/003-static-pod-rbac.yaml (1)
20-31: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftDo not grant cluster-wide write access to the shared bootstrap identity.
This binding allows
node-bootstrapperto create, update, and patch everyFRRNodeStateandBGPSessionStatein the cluster. Any holder of that identity could alter another node’s BGP state. Use a per-node identity or another authorization mechanism that scopes writes to the local node, and verify this trust model before merging.As per path instructions, RBAC must follow least privilege.
Also applies to: 50-61
🤖 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 20 - 31, Restrict the RBAC rule for node-bootstrapper in the ClusterRole to read-only verbs, removing create, update, and patch access for frrnodestates and bgpsessionstates. If writes are required, move them to a per-node identity or authorization mechanism scoped to the local node rather than granting cluster-wide write access.Source: Path instructions
🤖 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 `@docs/bgp_vip_management.md`:
- Around line 3-6: Update the prerequisites in the BGP VIP management
documentation to state that
Infrastructure.status.platformStatus.baremetal.additionalRoutingCapabilities.providers
must include the FRR provider. Clarify that renderBGPVIPFRRConfiguration
requires this capability in addition to the existing feature gate, BareMetal
platform, and BGP vipManagement conditions.
In `@pkg/network/bgp_vip.go`:
- Around line 101-104: Update the vipManagement lookup in the BGP mode logic to
handle the error from uns.NestedString separately: propagate or return the error
when it is non-nil, and only treat found == false as an absent field selecting
the existing non-BGP path. Do not discard malformed-value errors.
- Around line 162-204: Update buildFRRConfigurationObjects so each neighbor
includes the configured holdTime and keepaliveTime values from cfg.DefaultPeers,
preserving empty optional fields as omitted or explicitly rejecting unsupported
non-empty values. Extend the all-optional-fields test to verify these settings
are propagated.
In `@pkg/network/render.go`:
- Around line 132-138: In Render, evaluate the BGP VIP mode from the
Infrastructure CR once and store the result for the render operation. Pass this
shared value into both renderAdditionalRoutingCapabilities and
renderBGPVIPFRRConfiguration, updating their signatures and call sites so both
helpers use the same snapshot instead of independently reading the CR.
---
Duplicate comments:
In `@bindata/network/frr-k8s/003-static-pod-rbac.yaml`:
- Around line 20-31: Restrict the RBAC rule for node-bootstrapper in the
ClusterRole to read-only verbs, removing create, update, and patch access for
frrnodestates and bgpsessionstates. If writes are required, move them to a
per-node identity or authorization mechanism scoped to the local node rather
than granting cluster-wide write access.
🪄 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: 58526bcb-b4c8-41dd-8c10-d1aa6e838439
📒 Files selected for processing (7)
bindata/network/frr-k8s/003-static-pod-rbac.yamlbindata/network/frr-k8s/frr-k8s.yamldocs/bgp_vip_management.mdpkg/network/bgp_vip.gopkg/network/bgp_vip_test.gopkg/network/render.gopkg/network/render_test.go
f0979ba to
4c30cbe
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@docs/bgp_vip_management.md`:
- Around line 7-10: Update the documented Network operator resource path in the
BGP VIP management prerequisite from networks.operator/cluster to
network.operator.openshift.io/cluster, including the associated
additionalRoutingCapabilities.providers reference.
In `@pkg/network/bgp_vip.go`:
- Around line 70-81: The peer validation loop in the BGP configuration
validation function must allow-list the BFD and eBGP multihop fields to only "",
"true", or "false". Reject any other casing or malformed value before the
feature-handling logic that checks exact "true" values, using the existing peer
fields and returning a validation error that identifies the peer and invalid
value.
- Around line 311-324: Update the BGPSessionState list call in the surrounding
BGP VIP readiness check to restrict results to the openshift-frr-k8s namespace
and the selector identifying the bgp-vip FRRConfiguration or its owner labels.
Keep the existing status validation and empty-result behavior unchanged.
🪄 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: a35c8e24-aa3c-4e94-ab2f-214afd65ddf1
📒 Files selected for processing (7)
bindata/network/frr-k8s/003-static-pod-rbac.yamlbindata/network/frr-k8s/frr-k8s.yamldocs/bgp_vip_management.mdpkg/network/bgp_vip.gopkg/network/bgp_vip_test.gopkg/network/render.gopkg/network/render_test.go
4c30cbe to
e0b9f15
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
e0b9f15 to
ef0900a
Compare
|
@coderabbitai full review |
|
@coderabbitai summary |
✅ Action performedSummary regeneration triggered. |
ℹ️ No Pre-Merge checks found.No Per-Merge checks were run on this pull request. No override is required. |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/master-monitor.yaml`:
- Around line 34-68: Update the cp-metrics initContainer and frr-metrics
container resource definitions to include CPU and memory limits alongside their
existing requests. Add readiness and liveness probes to frr-metrics targeting
its HTTPS metrics endpoint on port 9141, so failed readiness removes it from
Service endpoints and failed liveness restarts it.
- Around line 26-68: Update the pod spec around the frr-metrics workload: remove
hostNetwork, disable automatic service-account token mounting, and add hardened
security contexts to both initContainer cp-metrics and container frr-metrics.
Configure each container to run as non-root with a read-only root filesystem,
disallow privilege escalation, and drop all Linux capabilities while preserving
the existing FRR volume mounts and commands.
In `@docs/bgp_vip_management.md`:
- Around line 40-45: Update the RBAC description near 003-static-pod-rbac.yaml
to state that it grants the static pod the read permissions required by the
frr-k8s controller informers, while preserving the surrounding write-access and
scoping statements.
- Around line 3-10: Clarify the activation prerequisites in the introductory
text: the feature gate, BareMetal platform, and Infrastructure CR status
activate BGP VIP management, while the FRR additional routing capability
provider is a separate hard prerequisite for successful rendering. Remove or
revise the “all three” inertness wording so it does not imply missing FRR leaves
the feature inert, and preserve the explicit rendering-failure behavior when the
provider is absent.
In `@pkg/network/bgp_vip.go`:
- Around line 58-89: Extend validateBGPVIPConfig to validate each peer’s
LocalASN and PeerASN before returning success. Reject missing or out-of-range
ASN values, including the zero LocalASN case, using the valid ASN bounds
expected by FRR; return descriptive validation errors so malformed values never
reach FRRConfiguration rendering.
🪄 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: 2694c27c-88dc-4879-9459-39c28ec80917
📒 Files selected for processing (8)
bindata/network/frr-k8s/003-static-pod-rbac.yamlbindata/network/frr-k8s/frr-k8s.yamlbindata/network/frr-k8s/master-monitor.yamldocs/bgp_vip_management.mdpkg/network/bgp_vip.gopkg/network/bgp_vip_test.gopkg/network/render.gopkg/network/render_test.go
| spec: | ||
| serviceAccountName: frr-k8s-daemon | ||
| hostNetwork: true | ||
| nodeSelector: | ||
| kubernetes.io/os: linux | ||
| node-role.kubernetes.io/master: "" | ||
| tolerations: | ||
| - operator: "Exists" | ||
| initContainers: | ||
| - name: cp-metrics | ||
| image: {{.FRRK8sImage}} | ||
| command: ["/bin/sh", "-c", "cp -f /frr-metrics /etc/frr_metrics/"] | ||
| terminationMessagePolicy: FallbackToLogsOnError | ||
| volumeMounts: | ||
| - name: metrics | ||
| mountPath: /etc/frr_metrics | ||
| containers: | ||
| - name: frr-metrics | ||
| image: {{.FRRK8sImage}} | ||
| command: ["/etc/frr_metrics/frr-metrics"] | ||
| args: | ||
| - --metrics-port=9141 | ||
| - --metrics-bind-address=0.0.0.0 | ||
| - --tls-cert-file=/etc/metrics/tls.crt | ||
| - --tls-private-key-file=/etc/metrics/tls.key | ||
| ports: | ||
| - containerPort: 9141 | ||
| name: frrmetricshttps | ||
| resources: | ||
| requests: | ||
| cpu: 10m | ||
| memory: 20Mi | ||
| terminationMessagePolicy: FallbackToLogsOnError | ||
| volumeMounts: | ||
| - name: frr-sockets | ||
| mountPath: /var/run/frr | ||
| - name: frr-conf | ||
| mountPath: /etc/frr | ||
| - name: metrics | ||
| mountPath: /etc/frr_metrics | ||
| - name: metrics-certs | ||
| mountPath: /etc/metrics | ||
| readOnly: true |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Remove avoidable host-level access and apply container hardening.
This exporter reaches FRR through mounted host paths; hostNetwork: true unnecessarily shares the control-plane node network namespace. The pod also lacks hardened security contexts and mounts its service-account token by default. Remove host networking unless required, disable token mounting if no API calls are made, and set non-root, read-only filesystem, no privilege escalation, and dropped capabilities on both containers.
🤖 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/master-monitor.yaml` around lines 26 - 68, Update the
pod spec around the frr-metrics workload: remove hostNetwork, disable automatic
service-account token mounting, and add hardened security contexts to both
initContainer cp-metrics and container frr-metrics. Configure each container to
run as non-root with a read-only root filesystem, disallow privilege escalation,
and drop all Linux capabilities while preserving the existing FRR volume mounts
and commands.
Sources: Coding guidelines, Path instructions
| initContainers: | ||
| - name: cp-metrics | ||
| image: {{.FRRK8sImage}} | ||
| command: ["/bin/sh", "-c", "cp -f /frr-metrics /etc/frr_metrics/"] | ||
| terminationMessagePolicy: FallbackToLogsOnError | ||
| volumeMounts: | ||
| - name: metrics | ||
| mountPath: /etc/frr_metrics | ||
| containers: | ||
| - name: frr-metrics | ||
| image: {{.FRRK8sImage}} | ||
| command: ["/etc/frr_metrics/frr-metrics"] | ||
| args: | ||
| - --metrics-port=9141 | ||
| - --metrics-bind-address=0.0.0.0 | ||
| - --tls-cert-file=/etc/metrics/tls.crt | ||
| - --tls-private-key-file=/etc/metrics/tls.key | ||
| ports: | ||
| - containerPort: 9141 | ||
| name: frrmetricshttps | ||
| resources: | ||
| requests: | ||
| cpu: 10m | ||
| memory: 20Mi | ||
| terminationMessagePolicy: FallbackToLogsOnError | ||
| volumeMounts: | ||
| - name: frr-sockets | ||
| mountPath: /var/run/frr | ||
| - name: frr-conf | ||
| mountPath: /etc/frr | ||
| - name: metrics | ||
| mountPath: /etc/frr_metrics | ||
| - name: metrics-certs | ||
| mountPath: /etc/metrics | ||
| readOnly: true |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add limits and health probes for the control-plane exporter.
cp-metrics and frr-metrics have requests only, and frr-metrics has neither readiness nor liveness probes. Define CPU/memory limits for both containers and health checks for the serving container so an unresponsive exporter is removed from Service endpoints and restarted.
🤖 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/master-monitor.yaml` around lines 34 - 68, Update the
cp-metrics initContainer and frr-metrics container resource definitions to
include CPU and memory limits alongside their existing requests. Add readiness
and liveness probes to frr-metrics targeting its HTTPS metrics endpoint on port
9141, so failed readiness removes it from Service endpoints and failed liveness
restarts it.
Source: Path instructions
| The static pod authenticates with the node kubeconfig (the MCO | ||
| node-bootstrapper ServiceAccount). `003-static-pod-rbac.yaml` grants it the | ||
| reads the frr-k8s controller's informers need and write access to | ||
| `FRRNodeState`/`BGPSessionState`. Rendered only under BGP VIP management. | ||
| Per-node write scoping is not expressible in RBAC; a ValidatingAdmissionPolicy | ||
| is planned follow-up. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the RBAC sentence.
“Grants it the reads” is grammatically incorrect and obscures the permission being granted. Use “grants the static pod the read permissions required by the frr-k8s controller informers.”
Suggested wording
-`003-static-pod-rbac.yaml` grants it the
-reads the frr-k8s controller's informers need and write access to
+`003-static-pod-rbac.yaml` grants the static pod the read permissions
+required by the frr-k8s controller's informers and write access to📝 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.
| The static pod authenticates with the node kubeconfig (the MCO | |
| node-bootstrapper ServiceAccount). `003-static-pod-rbac.yaml` grants it the | |
| reads the frr-k8s controller's informers need and write access to | |
| `FRRNodeState`/`BGPSessionState`. Rendered only under BGP VIP management. | |
| Per-node write scoping is not expressible in RBAC; a ValidatingAdmissionPolicy | |
| is planned follow-up. | |
| The static pod authenticates with the node kubeconfig (the MCO | |
| node-bootstrapper ServiceAccount). `003-static-pod-rbac.yaml` grants the static pod the read permissions | |
| required by the frr-k8s controller's informers and write access to | |
| `FRRNodeState`/`BGPSessionState`. Rendered only under BGP VIP management. | |
| Per-node write scoping is not expressible in RBAC; a ValidatingAdmissionPolicy | |
| is planned follow-up. |
🤖 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 `@docs/bgp_vip_management.md` around lines 40 - 45, Update the RBAC description
near 003-static-pod-rbac.yaml to state that it grants the static pod the read
permissions required by the frr-k8s controller informers, while preserving the
surrounding write-access and scoping statements.
When the BGPBasedVIPManagement gate is enabled and the BareMetal Infrastructure CR requests vipManagement "BGP", render a cluster-wide sessions-only FRRConfiguration from the installer's bgp-vip-config ConfigMap. VIP advertisement lives in rawConfig as health-gated table-direct redistribution of kernel table 198 (populated by kube-vip only while the VIP's backend is healthy), filtered to exactly the VIP prefixes. The CRD surface cannot express this: declared prefixes render as unconditional network statements, defeating the health gating (native support proposed in metallb/frr-k8s#469).
…anagement Masters run the MCO-rendered frr-k8s static pod, so the DaemonSet must not schedule there. Role-based required node-affinity avoids the NodeRestriction and node bring-up races that label-based exclusion hits. On compact/SNO the DaemonSet correctly matches zero nodes.
The static pod authenticates with the node kubeconfig, i.e. the MCO node-bootstrapper ServiceAccount. Grant it the reads the stock frr-k8s controller's informers require plus write access to the node state CRs, only when BGP VIP management is active. Per-node write scoping is not expressible in RBAC; admission-level enforcement is planned before GA.
Static pods cannot mount Secrets, so the FRR metrics exporter cannot run inside them. Run it in a masters-only companion DaemonSet instead: it reads the static pod's FRR through the hostPath sockets MCO exposes under /run/frr-k8s and serves TLS with a serving certificate, scraped via its own Service and ServiceMonitor. Validated on a clean install: metric coverage from every node, masters via the companion and workers via the regular DaemonSet.
45184c0 to
07be631
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
bindata/network/frr-k8s/master-monitor.yaml (1)
34-84: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winResource limits still missing on both containers.
Both
cp-metricsandfrr-metricsdefine CPU/memory requests but no limits. Path instructions require resource limits on every container. This was flagged in a prior review; probes were added but limits remain absent.🛡️ Proposed fix
resources: requests: cpu: 10m memory: 20Mi + limits: + cpu: 100m + memory: 50Mi🤖 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/master-monitor.yaml` around lines 34 - 84, Add CPU and memory limits to both the cp-metrics and frr-metrics container resource specifications, alongside their existing requests. Ensure every container in the manifest, including the containers identified by those names, defines the required limits while preserving the current request values and other settings.Source: Path instructions
docs/bgp_vip_management.md (1)
42-44: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winGrammar fix still needed in RBAC description.
"grants it the reads the frr-k8s controller's informers need" is grammatically incorrect. This was flagged in a prior review but not yet addressed.
📝 Proposed fix
-`003-static-pod-rbac.yaml` grants it the -reads the frr-k8s controller's informers need and write access to +`003-static-pod-rbac.yaml` grants the static pod the read permissions +required by the frr-k8s controller's informers and write access to🤖 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 `@docs/bgp_vip_management.md` around lines 42 - 44, Correct the RBAC description text around “003-static-pod-rbac.yaml” by rewriting the phrase about informer permissions so it is grammatically complete, while preserving the stated read access and write access to FRRNodeState/BGPSessionState.
🤖 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.
Duplicate comments:
In `@bindata/network/frr-k8s/master-monitor.yaml`:
- Around line 34-84: Add CPU and memory limits to both the cp-metrics and
frr-metrics container resource specifications, alongside their existing
requests. Ensure every container in the manifest, including the containers
identified by those names, defines the required limits while preserving the
current request values and other settings.
In `@docs/bgp_vip_management.md`:
- Around line 42-44: Correct the RBAC description text around
“003-static-pod-rbac.yaml” by rewriting the phrase about informer permissions so
it is grammatically complete, while preserving the stated read access and write
access to FRRNodeState/BGPSessionState.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: af1414d5-d25d-4a63-958b-49ec6d273f4c
📒 Files selected for processing (8)
bindata/network/frr-k8s/003-static-pod-rbac.yamlbindata/network/frr-k8s/frr-k8s.yamlbindata/network/frr-k8s/master-monitor.yamldocs/bgp_vip_management.mdpkg/network/bgp_vip.gopkg/network/bgp_vip_test.gopkg/network/render.gopkg/network/render_test.go
|
@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
CNO side of BGP-based VIP management (enhancement openshift/enhancements#1982, epic OPNET-595). Four commits:
bgp-vip-configConfigMap and renders a cluster-widebgp-vipFRRConfiguration (no node selector: the masters' static-pod controller and the workers' DaemonSet consume the same object) 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: 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). Noip import-tableis needed:table-directreads the kernel table directly (lab-verified; a first upstreaming step to express this natively in the frr-k8s CRD is design: redistribute support metallb/frr-k8s#470).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)./run/frr-k8s, serving TLS with a serving certificate via its own Service/ServiceMonitor. Workers keep the regular in-DaemonSet exporter — together this gives full per-node BGP metric coverage.Why
On-prem VIP failover today relies on keepalived (VRRP), which requires L2 adjacency between nodes. BGP-based VIP management (enhancement 1982, feature gate
BGPBasedVIPManagement) advertises the API and Ingress VIPs as /32 (/128) BGP routes instead: health-gated per node, ECMP across healthy nodes, no L2 requirement — enabling routed (L3/spine-leaf) datacenter topologies. CNO's part is rendering the frr-k8s session/advertisement config and keeping the frr-k8s DaemonSet off nodes that run the MCO-rendered static pods.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 (run in
ci/prow/unit): per-commit coverage — schema round-trip incl. full installer-shaped payloads and dual-stack, rawConfig content, input validation (IP/injection), render-var/affinity/RBAC-gating branches, bindata object counts;go test ./pkg/...fully green.CI lanes on this PR: the standard CNO presubmits (
e2e-aws-ovn,e2e-metal-ipi-ovn-ipv6,e2e-gcp-ovn, etc.) exercise the inert path — the gate is DevPreviewNoUpgrade and no CI platform setsvipManagement: BGP, so these lanes prove zero behavior change for existing FRR/MetalLB users (the render tests additionally pin the inactive path explicitly).Feature e2e: this repository has no in-repo e2e suite (there is no
test/e2e/directory — CNO is covered by the openshift/origin e2e framework via the prow lanes above). Feature-specific e2e is additionally not yet automatable in openshift/release — it requires a BGP ToR peer next to the cluster. A containerized ToR pattern for CI exists (validated in the reference implementation, dev-scripts PR openshift-metal3/dev-scripts#1929) and dedicated[OCPFeatureGate:BGPBasedVIPManagement]e2e lanes are tracked as OPNET-621/622/623 per the enhancement's graduation criteria.Live validation: end to end in the enhancement reference implementation across 21 dev-scripts baremetal installs (3-master compact and 3+2 with routers on workers) — install over a BGP-advertised API VIP, health-gated ECMP for API+Ingress VIPs, router-kill failover (withdraw ~50s, restore ≤20s), CRD handover, and Prometheus scraping
frrk8s_*metrics from all five nodes (masters via the companion DaemonSet, workers via the regular one) with zero dead targets: 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.