OPNET-781: Add BGP VIP management support - #10718
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@mkowalski: This pull request references OPNET-781 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. |
|
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:
📝 WalkthroughWalkthroughBare-metal BGP VIP configuration now has typed install-config fields, feature gating, validation, runtime ConfigMap generation, FRR network integration, and inclusion in the generated operator manifests. ChangesBGP VIP management
Estimated code review effort: 3 (Moderate) | ~25 minutes 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)
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)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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.github.com/in/347564?s=60&v=4){kind=small}
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 `@pkg/asset/manifests/bgpvipconfig.go`:
- Around line 134-137: Implement BGPVIPConfigMap.Load using the provided
asset.FileFetcher to read the already-rendered manifest, populate the asset’s
ConfigMap and File fields, and return the appropriate loaded status and errors
instead of always returning (false, nil). Add test coverage for restoring the
writable asset from disk.
- Around line 76-110: Update the BGP VIP manifest generation around
bgpVIPConfigJSON and hostOverrides so BGPPeerConfig.Password is never serialized
into config.json or stored in the ConfigMap. Use the existing secret-backed
mechanism for any required BGP peer credentials, and remove or avoid MD5-based
authentication material; flag any remaining MD5 usage according to project
guidelines.
In `@pkg/asset/manifests/network.go`:
- Around line 149-157: Update platform validation to reject bare-metal
configurations that set BGPVIPConfig when the selected network type is not
OVNKubernetes. In the baremetal branch of the manifest generation logic around
ovnNetworkOperatorConfig, retain an explicit OVNKubernetes guard before creating
the OVN configuration, and add a regression test covering the rejected
combination.
In `@pkg/types/baremetal/validation/platform.go`:
- Around line 595-617: Update validateBGPPeer to validate peer.Port when it is
nonzero, rejecting values outside the inclusive range 1..65535 while allowing
zero to remain omitted and default to port 179. Add the validation error using
the existing field.ErrorList pattern and the peer port field path.
- Around line 530-535: Update the host iteration in the platform validation flow
to check whether each host is nil before accessing host.BGPPeers. Skip nil hosts
while preserving existing validation for non-nil hosts and their BGP peers,
consistent with the ConfigMap generator’s behavior.
🪄 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: 447449cb-06c3-41a5-a789-05d6e9f36f68
⛔ Files ignored due to path filters (1)
pkg/types/baremetal/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (9)
pkg/asset/manifests/bgpvipconfig.gopkg/asset/manifests/bgpvipconfig_test.gopkg/asset/manifests/infrastructure.gopkg/asset/manifests/network.gopkg/asset/manifests/network_test.gopkg/asset/manifests/operators.gopkg/types/baremetal/platform.gopkg/types/baremetal/validation/featuregates.gopkg/types/baremetal/validation/platform.go
| configData := bgpVIPConfigJSON{ | ||
| LocalASN: bgpConfig.LocalASN, | ||
| DefaultPeers: bgpConfig.Peers, | ||
| Communities: bgpConfig.Communities, | ||
| APIVIPs: bm.APIVIPs, | ||
| IngressVIPs: bm.IngressVIPs, | ||
| } | ||
|
|
||
| // Collect per-host BGP peer overrides. | ||
| hostOverrides := make(map[string][]baremetal.BGPPeerConfig) | ||
| for _, host := range bm.Hosts { | ||
| if host != nil && len(host.BGPPeers) > 0 { | ||
| hostOverrides[host.Name] = host.BGPPeers | ||
| } | ||
| } | ||
| if len(hostOverrides) > 0 { | ||
| configData.HostOverrides = hostOverrides | ||
| } | ||
|
|
||
| jsonBytes, err := json.Marshal(configData) | ||
| if err != nil { | ||
| return errors.Wrap(err, "failed to marshal BGP VIP config JSON") | ||
| } | ||
|
|
||
| cm := &corev1.ConfigMap{ | ||
| TypeMeta: metav1.TypeMeta{ | ||
| APIVersion: corev1.SchemeGroupVersion.String(), | ||
| Kind: "ConfigMap", | ||
| }, | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Namespace: bgpVIPConfigMapNamespace, | ||
| Name: bgpVIPConfigMapName, | ||
| }, | ||
| Data: map[string]string{ | ||
| bgpVIPConfigMapDataKey: string(jsonBytes), |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Do not render BGP credentials into a ConfigMap.
BGPPeerConfig.Password is marshaled into config.json, then published in a ConfigMap. This exposes BGP session credentials in rendered manifests and implements the newly added MD5-based authentication model. Use a secret-backed mechanism and avoid MD5 authentication material.
As per coding guidelines, flag MD5 usage.
🤖 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/asset/manifests/bgpvipconfig.go` around lines 76 - 110, Update the BGP
VIP manifest generation around bgpVIPConfigJSON and hostOverrides so
BGPPeerConfig.Password is never serialized into config.json or stored in the
ConfigMap. Use the existing secret-backed mechanism for any required BGP peer
credentials, and remove or avoid MD5-based authentication material; flag any
remaining MD5 usage according to project guidelines.
Source: Coding guidelines
There was a problem hiding this comment.
Known DevPreview limitation, deliberate for now: the password is BGP TCP-MD5 session authentication (RFC 2385) — the hash is mandated by the BGP protocol option itself, not a choice of ours — and the whole config flows installer → ConfigMap → MCO/CNO as one schema. Moving it to a Secret requires the full chain (installer generates the Secret, MCO bootstrap render and CNO FRRConfiguration consume passwordSecret references) and is planned alongside the TechPreview structured API; the CNO side documents the same limitation (cluster-network-operator#3047, docs/bgp_vip_management.md). Splitting just the installer side now would leave mixed sources without removing the exposure.
This reply was written with AI assistance. Please verify before acting on it.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '=== Relevant installer symbols ==='
ast-grep outline pkg/types/baremetal/platform.go --items all --match 'BGPPeerConfig|BGPVIPConfig|Host|Platform' --view expanded
ast-grep outline pkg/asset/manifests/bgpvipconfig.go --items all --match 'bgpVIPConfigJSON|BGPVIPConfigMap' --view expanded
printf '%s\n' '=== BGP fields and serialization ==='
rg -n -A18 -B4 'type BGPPeerConfig|type BGPVIPConfig|BGPPeers|BGPVIPConfig|json\.Marshal|hostOverrides' \
pkg/types/baremetal/platform.go pkg/asset/manifests/bgpvipconfig.go pkg/asset/manifests/bgpvipconfig_test.go
printf '%s\n' '=== In-repository BGP VIP documentation/references ==='
rg -n -i -C3 'BGPBasedVIPManagement|BGP VIP|bgpVIPConfig|passwordSecret|TCP-MD5|RFC 2385|bgp_vip_management' \
README.md docs pkg data cmd 2>/dev/null || true
printf '%s\n' '=== Referenced CNO item metadata ==='
gh api repos/openshift/cluster-network-operator/issues/3047 \
--jq '{number, title, state, body, html_url, labels: [.labels[].name]}' || true
printf '%s\n' '=== Referenced CNO item comments containing credential/schema terms ==='
gh api --paginate repos/openshift/cluster-network-operator/issues/3047/comments \
--jq '.[] | select((.body | ascii_downcase) | test("password|secret|md5|bgp-vip|frrconfiguration")) | {user: .user.login, created_at, body, html_url}' || trueLength of output: 104813
@mkowalski, verified. BGPPeerConfig.Password is currently serialized end-to-end through bgp-vip-config, and the referenced CNO change consumes that same schema for BGP session configuration. There is no compatible secret-reference field in this branch, so removing the field only in installer would break the feature without eliminating the broader exposure.
I also correct the MD5 wording: installer does not implement MD5; this is the BGP TCP-MD5 shared secret required by the protocol.
Given the DevPreview scope and coupled consumer changes, I won’t require a partial installer-only change. The plaintext ConfigMap exposure remains a known pre-GA security limitation and should stay explicitly documented and tracked for the Secret-backed structured API work. Would you like a dedicated follow-up issue created for that end-to-end migration?
✏️ Learnings added
Learnt from: mkowalski
Repo: openshift/installer PR: 10718
File: pkg/asset/manifests/bgpvipconfig.go:76-110
Timestamp: 2026-07-28T07:57:44.312Z
Learning: For BareMetal BGP VIP management, `pkg/asset/manifests/bgpvipconfig.go` intentionally serializes `baremetal.BGPPeerConfig.Password` into the `bgp-vip-config` ConfigMap as a DevPreview limitation. The current installer → CNO → baremetal-runtimecfg schema consumes inline BGP TCP-MD5 session secrets, and an installer-only Secret migration would break that coupled schema. A Secret-backed design requires coordinated installer, MCO bootstrap/static-pod, and CNO `FRRConfiguration` support and is planned with the TechPreview structured API.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| case baremetal.Name: | ||
| // BGP-based VIP management needs frr-k8s CRDs/namespace deployed by | ||
| // CNO; enable the FRR routing capability provider. | ||
| if ic.Config.Platform.BareMetal.BGPVIPConfig != nil { | ||
| cnoCfg = ovnNetworkOperatorConfig(cns, sn) | ||
| cnoCfg.Spec.AdditionalRoutingCapabilities = &operatorv1.AdditionalRoutingCapabilities{ | ||
| Providers: []operatorv1.RoutingCapabilitiesProvider{operatorv1.RoutingCapabilitiesProviderFRR}, | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject BGP VIP configuration on non-OVN networks.
Line 153 always creates an OVN configuration, whose default network type is OVNKubernetes. A BGP VIP-configured bare-metal install using another network type would silently render an incompatible network manifest. Reject that install-config combination in platform validation and retain an OVN-type guard here; add a regression test.
🤖 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/asset/manifests/network.go` around lines 149 - 157, Update platform
validation to reject bare-metal configurations that set BGPVIPConfig when the
selected network type is not OVNKubernetes. In the baremetal branch of the
manifest generation logic around ovnNetworkOperatorConfig, retain an explicit
OVNKubernetes guard before creating the OVN configuration, and add a regression
test covering the rejected combination.
There was a problem hiding this comment.
Fixed: install-config validation now rejects bgpVIPConfig with any network type other than OVNKubernetes, and the manifest generator additionally guards the OVN config creation on the network type. Regression test added (TestValidateBGPVIP: 'bgp requires OVNKubernetes').
This reply was written with AI assistance. Please verify before acting on it.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== Candidate files =='
fd -t f . pkg/types/baremetal pkg/asset/manifests | rg '(platform|network).*(_test)?\.go$|featuregates\.go$'
printf '%s\n' '== Validation structure =='
ast-grep outline pkg/types/baremetal/validation --items all --type function --match 'ValidatePlatform|validateBGPVIPConfig|TestValidateBGPVIP' 2>/dev/null || true
printf '%s\n' '== Relevant validation and regression-test references =='
rg -n -C 8 'BGPVIPConfig|OVNKubernetes|bgp requires OVNKubernetes|TestValidateBGPVIP' \
pkg/types/baremetal/validation pkg/asset/manifests/network.go pkg/asset/manifests/network_test.go
printf '%s\n' '== Manifest guard =='
sed -n '140,170p' pkg/asset/manifests/network.goLength of output: 31127
@mkowalski, verified on the current PR branch.
- Validation rejects
bgpVIPConfigunlessNetworkTypeisOVNKubernetes. - The manifest generator retains the explicit OVN guard before rendering the FRR-enabled CNO configuration.
TestValidateBGPVIPincludes thebgp requires OVNKubernetesregression case and checks the expected error.
This resolves the incompatible manifest path. Static verification passed; tests were not executed in this environment.
There was a problem hiding this comment.
Pull request overview
Adds installer-side support for BGP-based VIP management on BareMetal by introducing new install-config types/validation and rendering the required handover artifacts/manifests (Infrastructure status, CNO routing capability, and a CNO-consumed ConfigMap) when platform.baremetal.bgpVIPConfig is set.
Changes:
- Added
BGPVIPConfig/BGPPeerConfigtypes (plus per-hostbgpPeersoverrides) and updated deepcopy generation. - Added validation + feature-gate wiring for
bgpVIPConfig, and enabled BGP VIP management in rendered manifests (Infrastructure status + CNO FRR routing capability). - Added generation + tests for a
bgp-vip-configConfigMap containingconfig.jsonin the runtimecfg-aligned schema.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/types/baremetal/zz_generated.deepcopy.go | Adds deepcopy support for new BareMetal BGP VIP config types/fields. |
| pkg/types/baremetal/platform.go | Introduces BGPVIPConfig, BGPPeerConfig, and per-host bgpPeers override field. |
| pkg/types/baremetal/validation/platform.go | Validates bgpVIPConfig and per-host BGP peer settings. |
| pkg/types/baremetal/validation/featuregates.go | Gates bgpVIPConfig behind BGPBasedVIPManagement. |
| pkg/asset/manifests/operators.go | Wires the new BGP VIP ConfigMap asset into manifest generation. |
| pkg/asset/manifests/bgpvipconfig.go | Generates the bgp-vip-config ConfigMap manifest with runtimecfg-aligned JSON. |
| pkg/asset/manifests/bgpvipconfig_test.go | Tests that generated config.json matches the expected schema. |
| pkg/asset/manifests/infrastructure.go | Sets BareMetal Infrastructure status VIP management to BGP when configured. |
| pkg/asset/manifests/network.go | Enables CNO FRR routing capability for baremetal when BGP VIP config is set. |
| pkg/asset/manifests/network_test.go | Adds tests for the CNO FRR capability enablement / negative case. |
Files not reviewed (1)
- pkg/types/baremetal/zz_generated.deepcopy.go: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| config.Spec.PlatformSpec.BareMetal.MachineNetworks = types.MachineNetworksToCIDRs(installConfig.Config.MachineNetwork) | ||
| config.Status.PlatformStatus.BareMetal.MachineNetworks = types.MachineNetworksToCIDRs(installConfig.Config.MachineNetwork) | ||
| if installConfig.Config.Platform.BareMetal.BGPVIPConfig != nil { | ||
| config.Status.PlatformStatus.BareMetal.VIPManagement = "BGP" |
There was a problem hiding this comment.
Fixed: uses configv1.VIPManagementTypeBGP — the typed constant is available now that the merged API is vendored on main.
This reply was written with AI assistance. Please verify before acting on it.
| Networking: &types.Networking{NetworkType: "OVNKubernetes"}, | ||
| Platform: types.Platform{ | ||
| BareMetal: &baremetal.Platform{ | ||
| BGPVIPConfig: &baremetal.BGPVIPConfig{LocalASN: 64512}, |
There was a problem hiding this comment.
Fixed: the fixture now carries a valid peer (validation requires at least one).
This reply was written with AI assistance. Please verify before acting on it.
| type bgpVIPConfigJSON struct { | ||
| LocalASN int64 `json:"localASN"` | ||
| DefaultPeers []baremetal.BGPPeerConfig `json:"defaultPeers"` | ||
| Communities []string `json:"communities,omitempty"` | ||
| APIVIPs []string `json:"apiVIPs"` | ||
| IngressVIPs []string `json:"ingressVIPs"` | ||
| HostOverrides map[string][]baremetal.BGPPeerConfig `json:"hostOverrides,omitempty"` |
There was a problem hiding this comment.
Known DevPreview limitation, deliberate for now: the password is BGP TCP-MD5 session authentication (RFC 2385) — the hash is mandated by the BGP protocol option itself, not a choice of ours — and the whole config flows installer → ConfigMap → MCO/CNO as one schema. Moving it to a Secret requires the full chain (installer generates the Secret, MCO bootstrap render and CNO FRRConfiguration consume passwordSecret references) and is planned alongside the TechPreview structured API; the CNO side documents the same limitation (cluster-network-operator#3047, docs/bgp_vip_management.md). Splitting just the installer side now would leave mixed sources without removing the exposure.
This reply was written with AI assistance. Please verify before acting on it.
| for i, host := range p.Hosts { | ||
| for j, peer := range host.BGPPeers { | ||
| hostPath := fldPath.Child("hosts").Index(i).Child("bgpPeers").Index(j) | ||
| allErrs = append(allErrs, validateBGPPeer(peer, hostPath)...) | ||
| } | ||
| } |
There was a problem hiding this comment.
Both fixed: nil host entries are skipped in the peer-override loop (see the sibling thread for the pre-existing upstream caveat), and host-level bgpPeers are now rejected with field.Forbidden when platform.baremetal.bgpVIPConfig is not set — no more silent ignoring. Tests added for both.
This reply was written with AI assistance. Please verify before acting on it.
cb75feb to
dcd7dbf
Compare
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 `@pkg/types/baremetal/platform.go`:
- Around line 89-92: Apply the 16-peer maximum to the host override field
BGPPeers in pkg/types/baremetal/platform.go, using the existing validation
mechanism so lists exceeding 16 peers are rejected. Add a regression test in
pkg/types/baremetal/validation/platform_test.go covering a 17-peer host override
and verify the boundary cases remain covered.
- Around line 58-75: Extend BGP validation to cover BGPPeerConfig HoldTime and
KeepaliveTime duration syntax and BGPVIPConfig Communities ASN:value format
before generating runtime configuration, using allow-list validation consistent
with existing peer address/ASN/port/flag checks. Update the validation tests in
pkg/types/baremetal/validation/platform_test.go for invalid timer and community
values, while preserving valid configurations.
- Around line 45-46: Remove the BGP password from BGPPeerConfig and the
generated bgp-vip-config ConfigMap serialization; update the BGP authentication
flow to consume the credential from a Kubernetes Secret-backed path instead,
ensuring no plaintext password remains in ConfigMap data.
🪄 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: 2f9fe70f-4d2c-4e34-b202-09c3a4dd9429
⛔ Files ignored due to path filters (1)
pkg/types/baremetal/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (10)
pkg/asset/manifests/bgpvipconfig.gopkg/asset/manifests/bgpvipconfig_test.gopkg/asset/manifests/infrastructure.gopkg/asset/manifests/network.gopkg/asset/manifests/network_test.gopkg/asset/manifests/operators.gopkg/types/baremetal/platform.gopkg/types/baremetal/validation/featuregates.gopkg/types/baremetal/validation/platform.gopkg/types/baremetal/validation/platform_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- pkg/types/baremetal/validation/featuregates.go
- pkg/asset/manifests/infrastructure.go
- pkg/asset/manifests/operators.go
- pkg/asset/manifests/network_test.go
- pkg/types/baremetal/validation/platform.go
- pkg/asset/manifests/bgpvipconfig_test.go
- pkg/asset/manifests/bgpvipconfig.go
dcd7dbf to
839b271
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- pkg/types/baremetal/zz_generated.deepcopy.go: Generated file
Comments suppressed due to low confidence (2)
pkg/types/baremetal/validation/platform.go:638
- BGPPeerConfig includes holdTime/keepaliveTime duration strings, but validateBGPPeer currently does not validate them. This means invalid values (e.g. "90" or "abc") will pass install-config validation and likely fail later during static-pod/FRR rendering. Consider validating non-empty values with time.ParseDuration (and optionally rejecting keepaliveTime > holdTime if both are set).
if peer.Port != 0 && (peer.Port < 1 || peer.Port > 65535) {
allErrs = append(allErrs, field.Invalid(fldPath.Child("port"), peer.Port,
"must be between 1 and 65535, or omitted for the default 179"))
}
pkg/types/baremetal/platform.go:72
- validateBGPVIPConfig enforces peers length (min 1, max 16), but the BGPVIPConfig.Peers field lacks corresponding kubebuilder validation markers. As a result, the generated install-config schema (data/data/install.openshift.io_installconfigs.yaml) does not reflect these constraints, and invalid configs may only fail at installer validation time. Add kubebuilder tags like +kubebuilder:validation:MinItems=1 and +kubebuilder:validation:MaxItems=16 on the Peers field (and regenerate the installconfig CRD) to keep schema and validation aligned.
// LocalASN is the Autonomous System Number for this cluster's BGP speaker.
LocalASN int64 `json:"localASN"`
// Peers is the list of BGP peers to advertise VIPs to.
Peers []BGPPeerConfig `json:"peers"`
When BGPVIPConfig is present in install-config.yaml, generate a ConfigMap named bgp-vip-config in the openshift-network-operator namespace. This ConfigMap contains the full BGP peer configuration as JSON so that CNO can read it and create FRRConfiguration CRs during the bootstrap-to-CRD handover. The config.json data includes: - localASN from bgpVIPConfig.localASN - peers from bgpVIPConfig.peers (default peer list) - communities from bgpVIPConfig.communities - apiVIPs from platform.baremetal.apiVIPs - ingressVIPs from platform.baremetal.ingressVIPs - hostOverrides from hosts[].bgpPeers (keyed by hostname)
839b271 to
64e7189
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/types/baremetal/validation/platform_test.go (1)
1221-1228: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover invalid
keepaliveTimetoo.Add a table case with malformed
KeepaliveTime; it shares the timer-validation path but currently has no regression coverage.As per path instructions, “Verify edge cases are covered, especially for validation and defaulting logic.”
🤖 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/types/baremetal/validation/platform_test.go` around lines 1221 - 1228, Add a table-driven validation case alongside “invalid holdTime” that sets BGPVIPConfig.Peers[0].KeepaliveTime to a malformed duration, uses the existing ovnNetwork fixture, and expects the same “must be a valid duration” validation error.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.
Nitpick comments:
In `@pkg/types/baremetal/validation/platform_test.go`:
- Around line 1221-1228: Add a table-driven validation case alongside “invalid
holdTime” that sets BGPVIPConfig.Peers[0].KeepaliveTime to a malformed duration,
uses the existing ovnNetwork fixture, and expects the same “must be a valid
duration” validation error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: f7b3c0fe-37b9-4bc8-a861-80d083a08d41
⛔ Files ignored due to path filters (2)
data/data/install.openshift.io_installconfigs.yamlis excluded by!data/data/install.openshift.io_installconfigs.yamlpkg/types/baremetal/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (10)
pkg/asset/manifests/bgpvipconfig.gopkg/asset/manifests/bgpvipconfig_test.gopkg/asset/manifests/infrastructure.gopkg/asset/manifests/network.gopkg/asset/manifests/network_test.gopkg/asset/manifests/operators.gopkg/types/baremetal/platform.gopkg/types/baremetal/validation/featuregates.gopkg/types/baremetal/validation/platform.gopkg/types/baremetal/validation/platform_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- pkg/asset/manifests/infrastructure.go
- pkg/asset/manifests/network.go
- pkg/types/baremetal/validation/featuregates.go
- pkg/asset/manifests/bgpvipconfig_test.go
- pkg/asset/manifests/bgpvipconfig.go
- pkg/types/baremetal/validation/platform.go
- pkg/asset/manifests/operators.go
|
@mkowalski: all tests passed! 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. |
|
/testwith openshift/installer/main/e2e-metal-ipi-bgp-vip openshift/machine-config-operator#6326 openshift/cluster-network-operator#3047 openshift/baremetal-runtimecfg#395 |
|
/testwith openshift/installer/main/e2e-metal-ipi-bgp-vip openshift/machine-config-operator#6326 openshift/cluster-network-operator#3047 openshift/baremetal-runtimecfg#395 |
cybertron
left a comment
cybertron
left a comment
There was a problem hiding this comment.
A couple of comments inline.
| "must be between 1 and 65535, or omitted for the default 179")) | ||
| } | ||
|
|
||
| for name, value := range map[string]string{"holdTime": peer.HoldTime, "keepaliveTime": peer.KeepaliveTime} { |
There was a problem hiding this comment.
Claude notes that validating these fields with ParseDuration is incompatible with the actual FRR config. ParseDuration requires a unit suffix, while FRR requires a bare number that represents the number of seconds. This appears to be a valid concern.
There was a problem hiding this comment.
Fixed in 8632e2e. Both findings were valid: the install-config keeps human-friendly duration strings (1m30s), but the manifest generation now converts them to whole-second decimal strings (90) in the peer data, which is what FRR's timers <keepalive> <hold> expects. Validation now also requires whole-second values in 0-65535s and requires holdTime/keepaliveTime to be set together, since FRR takes them as a pair. The consuming sides were updated to match the bare-seconds contract: runtimecfg templates render them directly (openshift/baremetal-runtimecfg#395, 38a7b106), MCO templates likewise (openshift/machine-config-operator#6326, 4b0d26808), and CNO converts 90 back to a 90s metav1.Duration for the FRRConfiguration CR (openshift/cluster-network-operator#3047, e6c26833a).
This comment was generated using AI. Please verify before acting on it.
| `must be "true", "false", or empty`)) | ||
| } | ||
|
|
||
| if peer.Port != 0 && (peer.Port < 1 || peer.Port > 65535) { |
There was a problem hiding this comment.
Claude says this Port field is dead code because it's not wired all the way into the config. The peer struct in runtimecfg is missing a port field, and it's not referenced in the MCO templates.
There was a problem hiding this comment.
Wired up now. The installer already serialized port into the peer data; the missing consumers are added: runtimecfg's FRRPeer gained a Port field (openshift/baremetal-runtimecfg#395, 38a7b106), the FRR templates in both runtimecfg and MCO render neighbor <addr> port <port> (openshift/machine-config-operator#6326, 4b0d26808), and CNO maps it to the FRRConfiguration neighbor port field with range validation (openshift/cluster-network-operator#3047, e6c26833a).
This comment was generated using AI. Please verify before acting on it.
Review findings (review 4919561631): FRR's 'timers <keepalive> <hold>'
takes bare seconds, while the install-config's holdTime/keepaliveTime
are human-friendly duration strings that the rendering templates were
passing through verbatim - producing config FRR rejects. The manifest
generation now converts the durations to whole-second decimal strings
in the peer data (the rendering contract), validation requires
whole-second values between 0 and 65535s (BGP timers are second-granular
16-bit) and requires holdTime/keepaliveTime to be set together (FRR
takes the pair).
The peer port field was the second finding (dead code end to end); the
consuming side is wired in baremetal-runtimecfg (FRRPeer.Port) and the
MCO templates ('neighbor <addr> port <port>') - the installer already
serializes it into the peer data.
Assisted-By: Claude Fable 5
Signed-off-by: Mat Kowalski <mko@redhat.com>
Review findings on openshift/installer#10718: the install-config port field was dead code (never present in this struct, so never rendered), and the timer fields were documented as free-form durations while FRR's 'timers <keepalive> <hold>' takes bare seconds. Add Port (0 = default 179) and document HoldTime/KeepaliveTime as whole-second decimal strings that must be set together; the producers (installer, MCO) convert install-config durations to seconds when generating the peer data. Assisted-By: Claude Fable 5 Signed-off-by: Mat Kowalski <mko@redhat.com>
Review findings on openshift/installer#10718 (review 4919561631): - FRR's 'timers <keepalive> <hold>' takes bare seconds, but the templates passed the peer data's duration strings ('90s') through verbatim, producing config FRR rejects. The peer-data contract is now whole-second decimal strings (converted from the install-config durations by the producers); the templates render them directly. The timers line also only renders when BOTH values are present (FRR requires the pair; previously a lone holdTime rendered 'timers <empty> <hold>'). - The install-config peer port was dead code: serialized into the peer data but never rendered. Both templates now emit 'neighbor <addr> port <port>' when set. Bootstrap asset and day-2 MachineConfig template kept in sync; rendering test covers both constructs, the both-or-neither guard, and the sync. Assisted-By: Claude Fable 5 Signed-off-by: Mat Kowalski <mko@redhat.com>
Review findings on openshift/installer#10718 (review 4919561631): the peer-data contract now carries whole-second decimal strings for the BGP timers (FRR's 'timers <keepalive> <hold>' takes bare seconds; the installer converts the install-config durations) and the previously dead peer port field is wired through. CNO validates seconds (0-65535, pair required together) and port range, renders the FRRConfiguration neighbor timers as metav1.Duration ('90' -> '90s') and the port field. Assisted-By: Claude Fable 5 Signed-off-by: Mat Kowalski <mko@redhat.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/asset/manifests/bgpvipconfig_test.go (1)
96-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover all source timer fields.
The test checks only the default peer
HoldTime.Generatealso converts defaultKeepaliveTimeand both host peer timer fields. Assert all four original values to detect a mutation in either peer slice.Proposed test update
assert.Equal(t, "1m30s", ic.Config.Platform.BareMetal.BGPVIPConfig.Peers[0].HoldTime) + assert.Equal(t, "30s", ic.Config.Platform.BareMetal.BGPVIPConfig.Peers[0].KeepaliveTime) + assert.Equal(t, "90s", ic.Config.Platform.BareMetal.Hosts[0].BGPPeers[0].HoldTime) + assert.Equal(t, "30s", ic.Config.Platform.BareMetal.Hosts[0].BGPPeers[0].KeepaliveTime)As per path instructions,
Test files. Verify edge cases are covered, especially for validation and defaulting logic.🤖 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/asset/manifests/bgpvipconfig_test.go` around lines 96 - 98, Expand the immutability assertions in the test around Generate to verify all source timer fields remain unchanged: the default peer’s HoldTime and KeepaliveTime, plus both timer fields on the host peer. Use the existing original values from the install-config object and cover both peer slices.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.
Nitpick comments:
In `@pkg/asset/manifests/bgpvipconfig_test.go`:
- Around line 96-98: Expand the immutability assertions in the test around
Generate to verify all source timer fields remain unchanged: the default peer’s
HoldTime and KeepaliveTime, plus both timer fields on the host peer. Use the
existing original values from the install-config object and cover both peer
slices.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: b01c3af8-2f4b-4cb1-90ed-8f385d975874
📒 Files selected for processing (4)
pkg/asset/manifests/bgpvipconfig.gopkg/asset/manifests/bgpvipconfig_test.gopkg/types/baremetal/validation/platform.gopkg/types/baremetal/validation/platform_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/types/baremetal/validation/platform.go
- pkg/types/baremetal/validation/platform_test.go
|
@cybertron I got your comments fixed, valid ones indeed. They weren't firing during e2e tests I have because both fields are optionals and never used in the config. Also FRR fails soft, so when you feed it with invalid times, it only logs an error but keeps running nevertheless. Sadly, there is no common schema for all that somewhere in the openshift/API to validate against. So we are stuck with this kind of reviews and potentially finding out stuff. Not sure if that's even fixable. I guess I need to modify e2e to consume every optional value. |
|
/lgtm Both Claude and I are satisfied. :-) |
|
Scheduling tests matching the |
What
Installer side of BGP-based VIP management for baremetal
(enhancement #1982, OPNET-773; API merged in
openshift/api#2923, already vendored on main via #10713). Adds
platform.baremetal.bgpVIPConfigto the install-config: when set, the cluster's API/ingress VIPs are advertised via BGP (kube-vip
routing-table mode + frr-k8s static pods, rendered by MCO) instead of managed by keepalived/VRRP.
Seven commits:
bgpVIPConfigis only accepted with theBGPBasedVIPManagementgate (DevPreviewNoUpgrade); invalid ASNs/peer addresses rejected atinstall-config validation
eBGP multihop, timers), communities, per-host overrides
status.platformStatus.baremetal.vipManagement: BGP(immutableonce set, per the API's CEL) so MCO/CNO select the BGP rendering path
bootstrap render and CNO's FRRConfiguration rendering
FRRPeerMapping exactly (one schema end to end; frr-peers.json == config.json)
additionalRoutingCapabilities.providers: [FRR]on theoperator Network CR when bgpVIPConfig is set (ships the FRRConfiguration CRD; without it CNO
degrades)
baremetal install)
Inert without the feature gate and the new install-config section: no vendoring changes (the API
is already on main), no behavior change for existing installs.
Companions
Testing
Unit tests included (validation, manifest generation, negative case). Validated end-to-end on
dev-scripts baremetal IPI clusters (3 masters + 2 workers, dual VIP): API VIP ECMP from all
masters, ingress VIP ECMP from router-bearing workers, health-gated withdrawal, bootstrap-to-CRD
handover on every install. Note:
pkg/asset/installconfig/vsphereunit tests currently fail onuntouched main (post-#10713 fallout), unrelated to this change.
This PR was prepared with AI assistance. Please verify before acting on it.
Summary by CodeRabbit