Uh oh!
There was an error while loading. Please reload this page.
OPNET-780: Add BGPBasedVIPManagement feature gate and BGP VIP management fields - #2923
Conversation
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
@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. |
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 |
@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. |
661c092 to
00a1277CompareRender 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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
00a1277 to
9742e1fCompareThere 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
mkowalski
commented
Jul 13, 2026
@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
Uh oh!
There was an error while loading. Please reload this page.
9742e1f to
eb996a3CompareThere 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
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.
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.
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.
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
left a comment
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)
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=65536 | ||
| // +optional | ||
| BGPVIPPeersJSON string `json:"bgpVIPPeersJSON,omitempty"` |
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.
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.
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.
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.
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.
JoelSpeed
commented
Jul 23, 2026
/lgtm |
@JoelSpeed: Overrode contexts on behalf of JoelSpeed: ci/prow/verify-hypershift-integration These overrides will persist across retests on the current HEAD SHA. Pushing a new commit will clear them. Use 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 kubernetes-sigs/prow repository. |
Scheduling tests matching the |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JoelSpeed, yuqi-zhang The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
mkowalski
commented
Jul 23, 2026
/verified by myself later Next step will be to vendor this into MCO and Installer. The code here "builds" as per ci/prow/unit |
openshift-ci-robot
commented
Jul 23, 2026
@mkowalski: This PR has been marked as verified by 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
commented
Jul 23, 2026
/retest-required |
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.
mkowalski
commented
Jul 23, 2026
/override ci/prow/e2e-aws-serial-techpreview-2of2 /override ci/prow/e2e-aws-serial-techpreview-1of2 /override https://prow.ci.openshift.org/job-history/gs/test-platform-results/pr-logs/directory/pull-ci-openshift-api-master-e2e-vsphere-ovn-techpreview |
@mkowalski: /override requires failed status contexts, check run or a prowjob name to operate on.
Only the following failed contexts/checkruns were expected:
If you are trying to override a checkrun that has a space in it, you must put a double quote on the context. 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 kubernetes-sigs/prow repository. |
mkowalski
commented
Jul 23, 2026
/override ci/prow/e2e-aws-serial-techpreview-2of2 /override ci/prow/e2e-aws-serial-techpreview-1of2 /override ci/prow/e2e-vsphere-ovn-techpreview |
@mkowalski: Overrode contexts on behalf of mkowalski: ci/prow/e2e-aws-serial-techpreview-1of2, ci/prow/e2e-aws-serial-techpreview-2of2, ci/prow/e2e-vsphere-ovn-techpreview 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 kubernetes-sigs/prow repository. |
mkowalski
commented
Jul 23, 2026
/override-sticky ci/prow/e2e-aws-serial-techpreview-1of2 |
@mkowalski: Overrode contexts on behalf of mkowalski: ci/prow/e2e-aws-serial-techpreview-1of2, ci/prow/e2e-aws-serial-techpreview-2of2, ci/prow/e2e-vsphere-ovn-techpreview These overrides will persist across retests on the current HEAD SHA. Pushing a new commit will clear them. Use 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 kubernetes-sigs/prow repository. |
mkowalski
commented
Jul 24, 2026
/override ci/prow/verify-hypershift-integration |
@mkowalski: Overrode contexts on behalf of mkowalski: ci/prow/verify-hypershift-integration 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 kubernetes-sigs/prow repository. |
@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. |
Uh oh!
There was an error while loading. Please reload this page.
Vendors the merged openshift/api#2923: the BGPBasedVIPManagement feature gate, BareMetalPlatformStatus.VIPManagement (immutable once set), and ControllerConfigSpec.BGPVIPPeersJSON. Assisted-By: Claude Fable 5 Signed-off-by: Mat Kowalski <mko@redhat.com>
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.