OPNET-780: Add BGPBasedVIPManagement feature gate and BGP VIP management fields#2923
Open
mkowalski wants to merge 2 commits into
Open
OPNET-780: Add BGPBasedVIPManagement feature gate and BGP VIP management fields#2923mkowalski wants to merge 2 commits into
mkowalski wants to merge 2 commits into
Conversation
Contributor
|
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-595 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 epic 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. |
Contributor
|
Hello @mkowalski! Some important instructions when contributing to openshift/api: |
|
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:
📝 WalkthroughWalkthroughThis PR adds BGP VIP management API fields, feature-gate registration, DevPreviewNoUpgrade enablement for Hypershift and SelfManagedHA, generated CRD schema updates, release feature-gate manifests, and CRD validation tests for supported and invalid values. Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
size/XXL
Contributor
|
[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 |
mkowalski
changed the title
|
@mkowalski: This pull request references OPNET-780 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. |
mkowalski
force-pushed
the
opnet-595-bgp-vip-api
branch
from
July 9, 2026 18:10
661c092 to
00a1277
Compare
mkowalski
added a commit
to mkowalski/cluster-network-operator
that referenced
this pull request
Jul 10, 2026
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).
There was a problem hiding this comment.
Pull request overview
This PR introduces the API surface (behind a new BGPBasedVIPManagement feature gate) needed to support BGP-based management of API/Ingress VIPs on on-premise clusters, along with the corresponding feature-set manifests, CRD/OpenAPI regeneration, and validation tests.
Changes:
- Adds the
BGPBasedVIPManagementfeature gate (enabled only inDevPreviewNoUpgrade) and wires it into payload featuregate manifests. - Adds feature-gated API fields:
config/v1:BareMetalPlatformStatus.VIPManagementto report the active VIP management mechanism (Keepalived/BGP).machineconfiguration/v1:ControllerConfigSpec.BGPVIPPeersJSONto carry installer-provided BGP peer config to MCO internals.
- Updates generated CRDs/OpenAPI artifacts and adds feature-gated API validation tests.
Reviewed changes
Copilot reviewed 43 out of 45 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-TechPreviewNoUpgrade.yaml | Adds BGPBasedVIPManagement to the disabled set for this feature-set/profile. |
| payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yaml | Adds BGPBasedVIPManagement to the disabled set for OKD. |
| payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-DevPreviewNoUpgrade.yaml | Ensures BGPBasedVIPManagement is enabled in DevPreviewNoUpgrade. |
| payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yaml | Adds BGPBasedVIPManagement to the disabled set for Default. |
| payload-manifests/featuregates/featureGate-4-10-Hypershift-TechPreviewNoUpgrade.yaml | Adds BGPBasedVIPManagement to the disabled set for this feature-set/profile. |
| payload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yaml | Adds BGPBasedVIPManagement to the disabled set for OKD. |
| payload-manifests/featuregates/featureGate-4-10-Hypershift-DevPreviewNoUpgrade.yaml | Ensures BGPBasedVIPManagement is enabled in DevPreviewNoUpgrade. |
| payload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yaml | Adds BGPBasedVIPManagement to the disabled set for Default. |
| payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml | Generated CRD update reflecting regrouping/annotations. |
| payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-DevPreviewNoUpgrade.crd.yaml | Generated CRD includes feature-gated bgpVIPPeersJSON and related schema updates. |
| payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-CustomNoUpgrade.crd.yaml | Generated CRD includes feature-gated bgpVIPPeersJSON and related schema updates. |
| payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Hypershift-DevPreviewNoUpgrade.crd.yaml | Generated CRD includes feature-gated bgpVIPPeersJSON and related schema updates. |
| payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Hypershift-CustomNoUpgrade.crd.yaml | Generated CRD includes feature-gated bgpVIPPeersJSON and related schema updates. |
| payload-manifests/crds/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yaml | Generated CRD update reflecting regrouping/annotations. |
| payload-manifests/crds/0000_10_config-operator_01_infrastructures-SelfManagedHA-DevPreviewNoUpgrade.crd.yaml | Generated CRD includes feature-gated vipManagement schema. |
| payload-manifests/crds/0000_10_config-operator_01_infrastructures-SelfManagedHA-CustomNoUpgrade.crd.yaml | Generated CRD includes feature-gated vipManagement schema. |
| payload-manifests/crds/0000_10_config-operator_01_infrastructures-Hypershift-TechPreviewNoUpgrade.crd.yaml | Removes/regroups generated CRD due to manifest-merge grouping changes. |
| payload-manifests/crds/0000_10_config-operator_01_infrastructures-Hypershift-DevPreviewNoUpgrade.crd.yaml | Generated CRD includes feature-gated vipManagement schema. |
| payload-manifests/crds/0000_10_config-operator_01_infrastructures-Hypershift-CustomNoUpgrade.crd.yaml | Generated CRD includes feature-gated vipManagement schema. |
| openapi/openapi.json | Updates generated OpenAPI definitions to include vipManagement field. |
| openapi/generated_openapi/zz_generated.openapi.go | Updates generated Go OpenAPI schema for the new field(s). |
| machineconfiguration/v1/zz_generated.swagger_doc_generated.go | Updates generated swagger docs for bgpVIPPeersJSON. |
| machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/BGPBasedVIPManagement.yaml | Generated feature-gated CRD manifest updated for the new field. |
| machineconfiguration/v1/zz_generated.featuregated-crd-manifests.yaml | Registers the new feature gate for feature-gated CRD generation. |
| machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml | Generated CRD update reflecting regrouping/annotations. |
| machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-DevPreviewNoUpgrade.crd.yaml | Generated CRD includes feature-gated bgpVIPPeersJSON. |
| machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-CustomNoUpgrade.crd.yaml | Generated CRD includes feature-gated bgpVIPPeersJSON. |
| machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Hypershift-DevPreviewNoUpgrade.crd.yaml | Generated CRD includes feature-gated bgpVIPPeersJSON. |
| machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Hypershift-CustomNoUpgrade.crd.yaml | Generated CRD includes feature-gated bgpVIPPeersJSON. |
| machineconfiguration/v1/types.go | Adds ControllerConfigSpec.BGPVIPPeersJSON behind BGPBasedVIPManagement. |
| machineconfiguration/v1/tests/controllerconfigs.machineconfiguration.openshift.io/BGPBasedVIPManagement.yaml | Adds feature-gated CRD validation tests for bgpVIPPeersJSON. |
| features/features.go | Registers the new BGPBasedVIPManagement feature gate (DevPreviewNoUpgrade). |
| features.md | Updates the feature gate matrix to include BGPBasedVIPManagement. |
| config/v1/zz_generated.swagger_doc_generated.go | Updates generated swagger docs for vipManagement. |
| config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/BGPBasedVIPManagement.yaml | Generated feature-gated CRD manifest updated for the new field. |
| config/v1/zz_generated.featuregated-crd-manifests.yaml | Registers the new feature gate for feature-gated CRD generation. |
| config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yaml | Generated CRD update reflecting regrouping/annotations. |
| config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-SelfManagedHA-DevPreviewNoUpgrade.crd.yaml | Generated CRD includes feature-gated vipManagement. |
| config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-SelfManagedHA-CustomNoUpgrade.crd.yaml | Generated CRD includes feature-gated vipManagement. |
| config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Hypershift-DevPreviewNoUpgrade.crd.yaml | Generated CRD includes feature-gated vipManagement. |
| config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Hypershift-CustomNoUpgrade.crd.yaml | Generated CRD includes feature-gated vipManagement. |
| config/v1/types_infrastructure.go | Adds BareMetalPlatformStatus.VIPManagement behind BGPBasedVIPManagement. |
| config/v1/types_infrastructure_test.go | Updates the TechPreview CRD filename reference to match regrouped manifests. |
| config/v1/tests/infrastructures.config.openshift.io/BGPBasedVIPManagement.yaml | Adds feature-gated CRD validation tests for vipManagement. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mkowalski
force-pushed
the
opnet-595-bgp-vip-api
branch
from
July 13, 2026 13:01
00a1277 to
9742e1f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
machineconfiguration/v1/tests/controllerconfigs.machineconfiguration.openshift.io/BGPBasedVIPManagement.yaml (1)
8-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the upper length bound.
The suite verifies acceptance and
MinLength=1, but the PR contract describesbgpVIPPeersJSONas bounded. Add a payload exceeding the declaredMaxLengthand assert rejection so the generated schema cannot silently lose its upper bound.Also applies to: 355-405
🤖 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 `@machineconfiguration/v1/tests/controllerconfigs.machineconfiguration.openshift.io/BGPBasedVIPManagement.yaml` around lines 8 - 14, Add a test case in the BGPBasedVIPManagement validation suite for bgpVIPPeersJSON whose payload exceeds the declared MaxLength, and assert that the ControllerConfig is rejected. Reuse the existing acceptance and minimum-length test structure so the generated schema’s upper bound is explicitly covered.
🤖 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
`@machineconfiguration/v1/tests/controllerconfigs.machineconfiguration.openshift.io/BGPBasedVIPManagement.yaml`:
- Around line 8-14: Add a test case in the BGPBasedVIPManagement validation
suite for bgpVIPPeersJSON whose payload exceeds the declared MaxLength, and
assert that the ControllerConfig is rejected. Reuse the existing acceptance and
minimum-length test structure so the generated schema’s upper bound is
explicitly covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 180f7e11-7943-4787-9044-da86fbd9b33f
⛔ Files ignored due to path filters (19)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Hypershift-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-SelfManagedHA-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/zz_generated*config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/BGPBasedVIPManagement.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Hypershift-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/zz_generated*machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/BGPBasedVIPManagement.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*openapi/openapi.jsonis excluded by!openapi/**
📒 Files selected for processing (26)
config/v1/tests/infrastructures.config.openshift.io/BGPBasedVIPManagement.yamlconfig/v1/types_infrastructure.goconfig/v1/types_infrastructure_test.gofeatures.mdfeatures/features.gomachineconfiguration/v1/tests/controllerconfigs.machineconfiguration.openshift.io/BGPBasedVIPManagement.yamlmachineconfiguration/v1/types.gopayload-manifests/crds/0000_10_config-operator_01_infrastructures-Hypershift-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-Hypershift-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-Hypershift-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-SelfManagedHA-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Hypershift-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Hypershift-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-TechPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-TechPreviewNoUpgrade.yaml
🚧 Files skipped from review as they are similar to previous changes (17)
- payload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yaml
- features.md
- payload-manifests/crds/0000_10_config-operator_01_infrastructures-Hypershift-CustomNoUpgrade.crd.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-DevPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-DevPreviewNoUpgrade.yaml
- machineconfiguration/v1/types.go
- payload-manifests/crds/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-CustomNoUpgrade.crd.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yaml
- payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Hypershift-CustomNoUpgrade.crd.yaml
- features/features.go
- config/v1/types_infrastructure_test.go
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yaml
- payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Hypershift-DevPreviewNoUpgrade.crd.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-TechPreviewNoUpgrade.yaml
- payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-DevPreviewNoUpgrade.crd.yaml
Contributor
Author
|
@coderabbitai full-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Hypershift-CustomNoUpgrade.crd.yaml`:
- Around line 65-76: Add API-path or validating-webhook validation for
bgpVIPPeersJSON so every non-empty value is parsed as JSON before persistence,
rejecting malformed payloads while preserving omission behavior. Do not rely on
the CRD schema’s length constraints or CEL; anchor the change to the
MachineConfig/ControllerConfig validation flow that persists this field.
🪄 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: ccb298ce-1390-4611-9ae7-131580e4c236
⛔ Files ignored due to path filters (19)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Hypershift-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-SelfManagedHA-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/zz_generated*config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/BGPBasedVIPManagement.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Hypershift-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/zz_generated*machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/BGPBasedVIPManagement.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*openapi/openapi.jsonis excluded by!openapi/**
📒 Files selected for processing (26)
config/v1/tests/infrastructures.config.openshift.io/BGPBasedVIPManagement.yamlconfig/v1/types_infrastructure.goconfig/v1/types_infrastructure_test.gofeatures.mdfeatures/features.gomachineconfiguration/v1/tests/controllerconfigs.machineconfiguration.openshift.io/BGPBasedVIPManagement.yamlmachineconfiguration/v1/types.gopayload-manifests/crds/0000_10_config-operator_01_infrastructures-Hypershift-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-Hypershift-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-Hypershift-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-SelfManagedHA-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Hypershift-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Hypershift-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-TechPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-TechPreviewNoUpgrade.yaml
mkowalski
force-pushed
the
opnet-595-bgp-vip-api
branch
from
July 14, 2026 11:28
9742e1f to
eb996a3
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
machineconfiguration/v1/tests/controllerconfigs.machineconfiguration.openshift.io/BGPBasedVIPManagement.yaml (1)
355-405: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the upper bound too.
BGPVIPPeersJSONis capped at 65536 chars, but this suite only checks the minimum. Add 65536 acceptance and 65537 rejection to guard the generated CRD’s max-length bound.🤖 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 `@machineconfiguration/v1/tests/controllerconfigs.machineconfiguration.openshift.io/BGPBasedVIPManagement.yaml` around lines 355 - 405, The BGPVIPPeersJSON validation tests only cover the minimum length. Extend the ControllerConfig test cases to accept a value with exactly 65536 characters and reject one with 65537 characters, using the existing validation fixture structure and asserting the generated max-length error for the oversized 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.
Nitpick comments:
In
`@machineconfiguration/v1/tests/controllerconfigs.machineconfiguration.openshift.io/BGPBasedVIPManagement.yaml`:
- Around line 355-405: The BGPVIPPeersJSON validation tests only cover the
minimum length. Extend the ControllerConfig test cases to accept a value with
exactly 65536 characters and reject one with 65537 characters, using the
existing validation fixture structure and asserting the generated max-length
error for the oversized value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 0ada3a9f-1610-41a6-88a5-1a98cb24a510
⛔ Files ignored due to path filters (19)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Hypershift-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-SelfManagedHA-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/zz_generated*config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/BGPBasedVIPManagement.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Hypershift-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/zz_generated*machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/BGPBasedVIPManagement.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*openapi/openapi.jsonis excluded by!openapi/**
📒 Files selected for processing (26)
config/v1/tests/infrastructures.config.openshift.io/BGPBasedVIPManagement.yamlconfig/v1/types_infrastructure.goconfig/v1/types_infrastructure_test.gofeatures.mdfeatures/features.gomachineconfiguration/v1/tests/controllerconfigs.machineconfiguration.openshift.io/BGPBasedVIPManagement.yamlmachineconfiguration/v1/types.gopayload-manifests/crds/0000_10_config-operator_01_infrastructures-Hypershift-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-Hypershift-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-Hypershift-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-SelfManagedHA-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Hypershift-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Hypershift-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-TechPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-TechPreviewNoUpgrade.yaml
🚧 Files skipped from review as they are similar to previous changes (21)
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-DevPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-TechPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-TechPreviewNoUpgrade.yaml
- config/v1/tests/infrastructures.config.openshift.io/BGPBasedVIPManagement.yaml
- payload-manifests/crds/0000_10_config-operator_01_infrastructures-Hypershift-CustomNoUpgrade.crd.yaml
- payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Hypershift-CustomNoUpgrade.crd.yaml
- payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-CustomNoUpgrade.crd.yaml
- payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Hypershift-DevPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_10_config-operator_01_infrastructures-SelfManagedHA-DevPreviewNoUpgrade.crd.yaml
- features/features.go
- payload-manifests/featuregates/featureGate-4-10-Hypershift-DevPreviewNoUpgrade.yaml
- features.md
- payload-manifests/crds/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_10_config-operator_01_infrastructures-SelfManagedHA-CustomNoUpgrade.crd.yaml
- machineconfiguration/v1/types.go
- config/v1/types_infrastructure.go
- payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-DevPreviewNoUpgrade.crd.yaml
mkowalski
added a commit
to mkowalski/cluster-network-operator
that referenced
this pull request
Jul 15, 2026
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). The raw config carries no ip import-table: redistribute table-direct reads the kernel table directly.
mkowalski
added a commit
to mkowalski/cluster-network-operator
that referenced
this pull request
Jul 15, 2026
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). The raw config carries no ip import-table: redistribute table-direct reads the kernel table directly.
mkowalski
added a commit
to mkowalski/cluster-network-operator
that referenced
this pull request
Jul 15, 2026
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). The raw config carries no ip import-table: redistribute table-direct reads the kernel table directly.
mkowalski
added a commit
to mkowalski/cluster-network-operator
that referenced
this pull request
Jul 15, 2026
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). The raw config carries no ip import-table: redistribute table-direct reads the kernel table directly.
yuqi-zhang
reviewed
Jul 16, 2026
yuqi-zhang
left a comment
yuqi-zhang
left a comment
Contributor
There was a problem hiding this comment.
Some comments inline based on my read of openshift/enhancements#1982
(since you are starting in dev preview, we have the additional benefit of merging this before openshift/enhancements#1982 needs to merge)
| // When omitted, the default keepalived-based VIP management is used. | ||
| // +openshift:enable:FeatureGate=BGPBasedVIPManagement | ||
| // +optional | ||
| VIPManagement VIPManagementType `json:"vipManagement,omitempty"` |
Contributor
There was a problem hiding this comment.
I think based on the enhancement this should be immutable post-install? Maybe we can validate that // +kubebuilder:validation:XValidation:rule="oldSelf == '' || self == oldSelf",message="vipManagement is immutable once set" explicitly here
Contributor
Author
There was a problem hiding this comment.
In the first iteration it's immutable, but sometime in 5.x we will allow modifying it. Do you want the CEL now and then drop it?
Contributor
There was a problem hiding this comment.
I think that would be good to have, if we want to use it as a tracker almost. Relaxing validation is easier than adding it, so if necessary we can always drop it later
Contributor
Author
There was a problem hiding this comment.
Good, adding validation via CEL
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=65536 | ||
| // +optional | ||
| BGPVIPPeersJSON string `json:"bgpVIPPeersJSON,omitempty"` |
Contributor
There was a problem hiding this comment.
What's the benefit of this approach, vs having the controller that does the rendering directly read off the configmap?
This field seems somewhat redundant and also is a full json object serialized into a string, which doesn't seem like an API field on the controllerconfig spec.
Contributor
Author
There was a problem hiding this comment.
vs having the controller that does the rendering directly read off the configmap
We can't do it with configmap because this needs to run already at bootstrap when there is no API server. It would work for day-2 but not for install-time.
This field seems somewhat redundant
I don't see redundancy yet. Is there any other place which MCO can use to read things when generating stuff needed at boostrap? Installer feeds MCO with data, MCO generates static pods with configs they need to run.
also is a full json object serialized into a string
Right. Now it's like this because of how the data flows from installer to MCO. BGPPeerConfig is owned by the installer. Baremetal-runtimecfg is responsible for rendering a config for a specific node out of the complete configuration. In the middle we have MCO.
So from installer I get the manifests as a file (technically, it will be serialized BGPPeerConfig). MCO bootstrap reads those and feeds a field in ControllerConfigSpec (currently BGPVIPPeersJSON under discussion). MCO templates then render field from ControllerConfigSpec and the output is frr-peers.json installed via MachineConfig. Then baremetal-runtimecfg reads frr-peers.json and does the real job.
Because communication between installer and MCO happens via files, I just use MCO as a postman carrying serialized payload to the next recipient (baremetal-runtimecfg).
Let me know if you see some simpler/better way of doing it.
Contributor
There was a problem hiding this comment.
I don't see redundancy yet. Is there any other place which MCO can use to read things when generating stuff needed at boostrap? Installer feeds MCO with data, MCO generates static pods with configs they need to run.
Most things do go through the controllerconfig process, but there are a few examples (pull secrets, templates, image references) that goes through install-config -> master ignition without the controllerconfig intermediary. The pull secret example is that the operator writes it to disk from parsing installconfig (https://github.com/openshift/machine-config-operator/blob/main/pkg/operator/bootstrap.go#L53) , the controller explicitly reads it https://github.com/openshift/machine-config-operator/blob/main/pkg/controller/bootstrap/bootstrap.go#L76 and then does the MachineConfig rendering. But now I think about it a bit more, this probably is better left as an exception rather than the norm.
I think my main hesitation here is that it's an unstructured string (although we have precedents here for legacy fields, I would lean towards adhering to better API practices where possible). Would you consider lifting BGPVIPConfig/BGPPeerConfig from the installer as a proper go struct here instead? We could also circle back on this after dev preview if you prefer.
Contributor
Author
There was a problem hiding this comment.
there are a few examples [..] that goes through install-config -> master ignition without the controllerconfig intermediary
As you said, it's rather exception. I think from those two options, it's better to have a proper API rather than going directly bypassing ControllerConfig.
Let's do it before TechPreview. This will allow us to use DevPreview to understand fully what is the best form of the structured API.
mkowalski
added a commit
to mkowalski/cluster-network-operator
that referenced
this pull request
Jul 16, 2026
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). The raw config carries no ip import-table: redistribute table-direct reads the kernel table directly.
mkowalski
added a commit
to mkowalski/cluster-network-operator
that referenced
this pull request
Jul 16, 2026
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). The raw config carries no ip import-table: redistribute table-direct reads the kernel table directly.
mkowalski
added a commit
to mkowalski/cluster-network-operator
that referenced
this pull request
Jul 16, 2026
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). The raw config carries no ip import-table: redistribute table-direct reads the kernel table directly.
mkowalski
force-pushed
the
opnet-595-bgp-vip-api
branch
from
July 20, 2026 16:22
eb996a3 to
cf6e3d5
Compare
mkowalski
added a commit
to mkowalski/openshift-enhancements
that referenced
this pull request
Jul 20, 2026
Dev Preview carries the peer payload as serialized JSON on MCO's internal ControllerConfig (agreed in openshift/api#2923 review) so the schema can bake. Tech Preview replaces the ConfigMap and the JSON field with a cluster-scoped BGPVIPConfig CRD: one admission-validated schema for installer, CNO and MCO, passwordSecretRef instead of inline passwords, and a real day-2 flow (watches, status conditions, a NodeDisruptionPolicy so peer changes do not reboot nodes). GA hardens the same CRD and removes the ConfigMap path.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
machineconfiguration/v1/tests/controllerconfigs.machineconfiguration.openshift.io/BGPBasedVIPManagement.yaml (1)
355-405: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing upper-bound (maxLength) test for
bgpVIPPeersJSON.Only the lower bound (empty string,
minLength) is exercised. The PR description calls this field "bounded," implying a maximum length constraint as well; add a case with a value exceeding the max length to confirm it's rejected with the expected error, mirroring the empty-string test here.♻️ Suggested additional test case
- name: Should reject a too-long bgpVIPPeersJSON initial: | apiVersion: machineconfiguration.openshift.io/v1 kind: ControllerConfig spec: additionalTrustBundle: Y2VydGlmaWNhdGUK bgpVIPPeersJSON: '<value exceeding the configured maxLength>' # ... remaining fields identical to the other test cases expectedError: "spec.bgpVIPPeersJSON in body should be at most <N> chars long"🤖 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 `@machineconfiguration/v1/tests/controllerconfigs.machineconfiguration.openshift.io/BGPBasedVIPManagement.yaml` around lines 355 - 405, Add a ControllerConfig validation case alongside “Should reject an empty bgpVIPPeersJSON” using a bgpVIPPeersJSON value longer than the configured maximum, while keeping the remaining spec identical. Assert the validation error reports the field must be at most the configured maxLength, using the actual limit and matching existing test formatting.
🤖 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
`@machineconfiguration/v1/tests/controllerconfigs.machineconfiguration.openshift.io/BGPBasedVIPManagement.yaml`:
- Around line 355-405: Add a ControllerConfig validation case alongside “Should
reject an empty bgpVIPPeersJSON” using a bgpVIPPeersJSON value longer than the
configured maximum, while keeping the remaining spec identical. Assert the
validation error reports the field must be at most the configured maxLength,
using the actual limit and matching existing test formatting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: cf4f7e1f-cfa5-4a88-a48c-2a2079b7edc0
⛔ Files ignored due to path filters (19)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Hypershift-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-SelfManagedHA-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/zz_generated*config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/BGPBasedVIPManagement.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Hypershift-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/zz_generated*machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/BGPBasedVIPManagement.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*openapi/openapi.jsonis excluded by!openapi/**
📒 Files selected for processing (26)
config/v1/tests/infrastructures.config.openshift.io/BGPBasedVIPManagement.yamlconfig/v1/types_infrastructure.goconfig/v1/types_infrastructure_test.gofeatures.mdfeatures/features.gomachineconfiguration/v1/tests/controllerconfigs.machineconfiguration.openshift.io/BGPBasedVIPManagement.yamlmachineconfiguration/v1/types.gopayload-manifests/crds/0000_10_config-operator_01_infrastructures-Hypershift-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-Hypershift-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-Hypershift-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-SelfManagedHA-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Hypershift-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Hypershift-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-TechPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-TechPreviewNoUpgrade.yaml
🚧 Files skipped from review as they are similar to previous changes (23)
- payload-manifests/featuregates/featureGate-4-10-Hypershift-TechPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-DevPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-TechPreviewNoUpgrade.yaml
- features.md
- payload-manifests/featuregates/featureGate-4-10-Hypershift-DevPreviewNoUpgrade.yaml
- machineconfiguration/v1/types.go
- payload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yaml
- payload-manifests/crds/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_10_config-operator_01_infrastructures-Hypershift-DevPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_10_config-operator_01_infrastructures-Hypershift-CustomNoUpgrade.crd.yaml
- payload-manifests/crds/0000_10_config-operator_01_infrastructures-SelfManagedHA-CustomNoUpgrade.crd.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yaml
- features/features.go
- config/v1/types_infrastructure_test.go
- payload-manifests/crds/0000_10_config-operator_01_infrastructures-SelfManagedHA-DevPreviewNoUpgrade.crd.yaml
- config/v1/tests/infrastructures.config.openshift.io/BGPBasedVIPManagement.yaml
- payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-DevPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-CustomNoUpgrade.crd.yaml
- payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Hypershift-CustomNoUpgrade.crd.yaml
- payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Hypershift-DevPreviewNoUpgrade.crd.yaml
- config/v1/types_infrastructure.go
mkowalski
added a commit
to mkowalski/openshift-enhancements
that referenced
this pull request
Jul 20, 2026
vipManagement (openshift/api#2923) is the API-level signal; session health is per-node in frr-k8s BGPSessionState/FRRNodeState. Echoing localASN/peers into Infrastructure status would restate configuration and require CNO as a second status writer.
…ield BGP-based VIP management for on-premise clusters (enhancement 1982, OPNET-595): the gate ships in DevPreviewNoUpgrade; BareMetalPlatformStatus.VIPManagement reports which mechanism (Keepalived/BGP) manages the API and Ingress VIPs. Set at install time by the installer; consumed by MCO and CNO. Safe with no backing implementation: feature-gated (field pruned outside DevPreviewNoUpgrade CRD variants), optional, and nothing in-payload sets or reads it until the implementation lands.
Internal MCO API carrying the installer-generated BGP peer configuration from the bgp-vip-config ConfigMap to the template controller, which renders the frr-k8s static pod peer file on control plane nodes (enhancement 1982, OPNET-595). Feature-gated, optional and bounded; a no-op until the MCO implementation consumes it. The controllerconfigs CRD manifests also pick up the vipManagement enum refresh through the embedded Infrastructure schema.
mkowalski
force-pushed
the
opnet-595-bgp-vip-api
branch
from
July 20, 2026 20:58
cf6e3d5 to
3b96df0
Compare
mkowalski
added a commit
to mkowalski/machine-config-operator
that referenced
this pull request
Jul 20, 2026
Temporary: vendors the openshift/api#2923 branch (BGPBasedVIPManagement feature gate, Infrastructure vipManagement, ControllerConfigSpec BGPVIPPeersJSON) via a module replace. Swapped for the merged module as soon as openshift/api#2923 lands; every subsequent commit builds against the identical API surface either way.
Contributor
|
@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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
API surface for BGP-based VIP management on on-premise clusters — enhancement openshift/enhancements#1982 (OPNET-595). Two commits:
BGPBasedVIPManagementfeature gate (DevPreviewNoUpgrade) andBareMetalPlatformStatus.VIPManagement— reports which mechanism (Keepalived/BGP) manages the API and Ingress VIPs. Set at install time by the installer; consumed by MCO and CNO.ControllerConfigSpec.BGPVIPPeersJSON— internal MCO API carrying the installer-generated BGP peer configuration (thebgp-vip-configConfigMap payload, validated and compacted by the MCO operator) to the template controller, which renders the frr-k8s static pod peer file on control plane nodes. Bounded (1–65536), optional.Safety without backing implementation
Deliberately safe to merge stand-alone:
+openshift:enable:FeatureGate=BGPBasedVIPManagement; the gate is enabled only in DevPreviewNoUpgrade (verified across all 8 payload featuregate manifests). The fields are pruned from Default/TechPreview/OKD CRD variants (verified: zero occurrences outside CustomNoUpgrade/DevPreviewNoUpgrade schemas, including the ControllerConfig CRDs that embed Infrastructure).+optional/omitemptywith no defaults; nothing in-payload sets or reads them until the implementation PRs land.make lintclean,make updateidempotent on the branch.Reviewer note on the large generated diff in commit 1
Enabling a profile-agnostic gate in DevPreviewNoUpgrade changes which feature-set CRD schemas are byte-identical, so the manifest-merge generator regroups files (SelfManagedHA {CustomNoUpgrade,DevPreviewNoUpgrade} pair up; Hypershift TechPreview variants merge across profiles). Verified with a control experiment:
make updatetwice on pristine master produces zero changes — the regrouping is entirely attributable to the new gate, pertools/codegen/pkg/manifestmerge/generator.gogrouping logic.Validation
The consuming implementation exists and was validated end to end on a dev-scripts baremetal cluster (installation over a BGP-advertised API VIP, health-gated ECMP for both VIPs, CRD handover); reference implementation and evidence: https://github.com/mkowalski/bgp-vip-demo. Related PRs in flight: kube-vip/kube-vip#1627, openshift-metal3/dev-scripts#1929.