OPNET-782: Render BGP VIP static pods instead of keepalived#6326
Open
mkowalski wants to merge 4 commits into
Open
OPNET-782: Render BGP VIP static pods instead of keepalived#6326mkowalski wants to merge 4 commits into
mkowalski wants to merge 4 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
Contributor
|
@mkowalski: This pull request references OPNET-782 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. |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mkowalski The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
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:
WalkthroughAdds BareMetal BGP VIP management with peer synchronization, image wiring, FRR and kube-vip static pods, conditional keepalived replacement, bootstrap validation, RBAC access, and related tests. It also updates Go modules and container runtime policy fixtures. ChangesBGP VIP management
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Bootstrap
participant ConfigMap
participant Operator
participant FRR
participant KubeVIP
Bootstrap->>ConfigMap: read bgp-vip-config
ConfigMap-->>Bootstrap: return config.json peer data
Bootstrap->>Operator: provide images and peer configuration
Operator->>FRR: render peer data and BGP configuration
Operator->>KubeVIP: render API and ingress pods
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)go.modTraceback (most recent call last): install/0000_80_machine-config_00_rbac.yamlTraceback (most recent call last): install/0000_80_machine-config_02_images.configmap.yamlTraceback (most recent call last):
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/operator/render.go (1)
280-313: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGate kube-vip-ingress on ingress VIPs
isBGPVIPManagementonly checksBareMetal.APIServerInternalIPs, sotemplates/common/on-prem/files/0020-kube-vip-ingress.yaml:26can still render{{ onPremPlatformIngressIP . }}as<no value>whenIngressIPsis empty. Add an ingress-IP check before enabling this manifest, or switch to the plural helper with a length guard.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/operator/render.go` around lines 280 - 313, Update the kube-vip-ingress enablement logic associated with isBGPVIPManagement so the manifest renders only when the selected on-prem platform has at least one IngressIP. Add the ingress-IP presence check before enabling 0020-kube-vip-ingress.yaml, or use the plural ingress-IP helper with an explicit length guard, preventing onPremPlatformIngressIP from producing a missing value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@go.mod`:
- Around line 476-477: Restrict the github.com/openshift/api replacement to the
specific required upstream version by adding that version to the replace
directive, rather than applying it to all selected versions. Preserve the fork
module and pinned fork version, and use the approved upstream/internal module if
the fork is not intended to remain globally substituted.
In `@manifests/on-prem/0000-frr-k8s.yaml`:
- Around line 42-51: Update the volume mounts for resource-dir,
frr-conf/frr-startup, kubeconfig, and manifests in the pod specifications to set
readOnly: true, including the additionally referenced mounts. Preserve
mountPropagation: HostToContainer on manifests and do not alter mounts that
require write access.
- Around line 73-105: Add liveness and readiness probes to the FRR container
definition in manifests/on-prem/0000-frr-k8s.yaml:73-105, using an appropriate
FRR health check. Add corresponding liveness and readiness probes to the
kube-vip container in manifests/on-prem/0010-kube-vip-api.yaml:15-52, ensuring
both critical static pods expose checks that detect unhealthy or hung processes.
- Around line 60-95: Harden both static pod manifests: in
manifests/on-prem/0000-frr-k8s.yaml lines 60-95, update every init and main
container securityContext to run as non-root with privilege escalation disabled,
drop ALL capabilities, and remove SYS_ADMIN unless explicitly justified; in
manifests/on-prem/0010-kube-vip-api.yaml lines 47-51, add the required non-root,
no-escalation, read-only-root-filesystem settings and drop ALL capabilities
while preserving only justified additions.
- Around line 54-59: Update the shell command in the init container’s command
block to fail immediately when either FRR configuration copy fails. Add `set
-eu` before the `cp` commands or chain both copies with `&&`, preserving the
existing copy paths and order.
- Around line 68-72: Update the pod volume definitions and mounts associated
with frr-startup and frr-conf to use the prepared hostPath directories instead
of emptyDir, mounting the host configuration and socket paths under
/run/frr-k8s/{conf,sockets}. Ensure the paths are writable by UID 100 and shared
with the metrics companion while preserving the existing container mount
targets.
In `@manifests/on-prem/frr.conf.tmpl`:
- Around line 44-50: Apply the VIP-COMMUNITY route-map to outbound peer updates
by adding the neighbor route-map command inside the peer loop in
manifests/on-prem/frr.conf.tmpl#L10-L24 and
templates/master/00-master/on-prem/files/frr-k8s-conf.yaml#L14-L28, using each
peer’s PeerAddress. Keep the existing VIP-COMMUNITY definition and community
entries unchanged.
In `@pkg/operator/bootstrap_dependencies.go`:
- Around line 285-305: Update fillBGPVIPConfig to reject an empty config.json
payload consistently with syncBGPVIPPeersJSON. After reading the ConfigMap,
ensure compactBGPVIPPeersJSON or the surrounding logic returns the same error
for empty input instead of assigning a blank BGPVIPPeersJSON; preserve the
existing optional-missing-ConfigMap behavior.
In `@templates/common/on-prem/files/0020-kube-vip-ingress.yaml`:
- Around line 51-59: Update the container specification in the Kubernetes
manifest to add CPU and memory resource limits alongside the existing requests,
drop all capabilities before adding only the required NET_ADMIN and NET_RAW
capabilities, and set allowPrivilegeEscalation to false in the container
securityContext.
In `@templates/master/00-master/on-prem/files/0000-frr-k8s.yaml`:
- Around line 14-173: Harden every container and initContainer in
templates/master/00-master/on-prem/files/0000-frr-k8s.yaml (lines 14-173) and
templates/master/00-master/on-prem/files/0010-kube-vip-api.yaml (lines 15-55)
with allowPrivilegeEscalation: false, readOnlyRootFilesystem: true, drop ALL
capabilities, only explicitly required capabilities, and CPU/memory limits;
retain hostNetwork only where required. For the frr container, remove SYS_ADMIN
unless a documented exception justifies it, and document that exception if
retained.
---
Outside diff comments:
In `@pkg/operator/render.go`:
- Around line 280-313: Update the kube-vip-ingress enablement logic associated
with isBGPVIPManagement so the manifest renders only when the selected on-prem
platform has at least one IngressIP. Add the ingress-IP presence check before
enabling 0020-kube-vip-ingress.yaml, or use the plural ingress-IP helper with an
explicit length guard, preventing onPremPlatformIngressIP from producing a
missing value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9ee0fd22-7ea3-478e-a550-d1d305d76a75
⛔ Files ignored due to path filters (76)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/.golangci.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_infrastructure.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_ingress.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Hypershift-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-SelfManagedHA-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/features.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Hypershift-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/types_authentication.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_csi_cluster_driver.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_ingresscontroller.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_console_01_consoles.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/types_clusterapi.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/osin/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/osin/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/google.golang.org/protobuf/encoding/protodelim/protodelim.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/encoding/protojson/decode.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/encoding/protojson/well_known_types.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/encoding/prototext/decode.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/descfmt/stringer.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/encoding/tag/tag.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/encoding/text/decode.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/filedesc/desc.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/filedesc/desc_lazy.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/genid/descriptor_gen.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/impl/codec_map.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/impl/decode.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/impl/validate.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/version/version.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/proto/decode.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/reflect/protodesc/desc.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/reflect/protodesc/desc_init.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/reflect/protodesc/editions.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/types/descriptorpb/descriptor.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/types/known/timestamppb/timestamp.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (34)
cmd/machine-config-operator/bootstrap.gogo.modinstall/0000_80_machine-config_00_rbac.yamlinstall/0000_80_machine-config_02_images.configmap.yamlinstall/image-referencesmanifests/on-prem/0000-frr-k8s.yamlmanifests/on-prem/0010-kube-vip-api.yamlmanifests/on-prem/frr-peers.json.tmplmanifests/on-prem/frr-startup-daemonsmanifests/on-prem/frr-startup-vtysh.confmanifests/on-prem/frr.conf.tmplpkg/controller/common/helpers.gopkg/controller/common/images.gopkg/controller/container-runtime-config/actual.jsonpkg/controller/container-runtime-config/expected.jsonpkg/controller/template/constants.gopkg/controller/template/render.gopkg/controller/template/render_test.gopkg/operator/bootstrap.gopkg/operator/bootstrap_dependencies.gopkg/operator/bootstrap_test.gopkg/operator/render.gopkg/operator/render_test.gopkg/operator/sync.gopkg/operator/sync_test.gotemplates/common/on-prem/files/0020-kube-vip-ingress.yamltemplates/common/on-prem/files/keepalived.yamltemplates/master/00-master/on-prem/files/0000-frr-k8s.yamltemplates/master/00-master/on-prem/files/0010-kube-vip-api.yamltemplates/master/00-master/on-prem/files/frr-k8s-conf.yamltemplates/master/00-master/on-prem/files/frr-k8s-peers.yamltemplates/master/00-master/on-prem/files/frr-k8s-startup-daemons.yamltemplates/master/00-master/on-prem/files/frr-k8s-startup-vtysh.yamltemplates/master/00-master/on-prem/units/frr-k8s-hostpath.service.yaml
mkowalski
force-pushed
the
OPNET-595-mco-pr
branch
from
July 22, 2026 11:13
31750b2 to
a3aa0f9
Compare
Contributor
Author
|
Re the outside-diff comment on pkg/operator/render.go (kube-vip-ingress rendering with empty IngressIPs): fixed at the shared gate — Summary of the review round: all inline comments addressed or answered in-thread; the securityContext changes were validated live on a running BGP-VIP cluster (which also caught that This comment was written with AI assistance. Please verify before acting on it. |
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 `@templates/common/on-prem/files/0020-kube-vip-ingress.yaml`:
- Around line 55-63: Set runAsNonRoot: true and an explicit non-zero runAsUser
in the kube-vip pod security contexts at
templates/common/on-prem/files/0020-kube-vip-ingress.yaml lines 55-63 and
templates/master/00-master/on-prem/files/0010-kube-vip-api.yaml lines 51-59. In
templates/master/00-master/on-prem/files/0000-frr-k8s.yaml lines 41-46, apply
the same non-root settings only to render-config-frr; leave cp-frr-files, frr,
reloader, and frr-status unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9cd86428-3547-4439-8d98-23f21081658d
📒 Files selected for processing (31)
cmd/machine-config-operator/bootstrap.goinstall/0000_80_machine-config_00_rbac.yamlinstall/0000_80_machine-config_02_images.configmap.yamlinstall/image-referencesmanifests/on-prem/0000-frr-k8s.yamlmanifests/on-prem/0010-kube-vip-api.yamlmanifests/on-prem/frr-peers.json.tmplmanifests/on-prem/frr-startup-daemonsmanifests/on-prem/frr-startup-vtysh.confmanifests/on-prem/frr.conf.tmplpkg/controller/common/helpers.gopkg/controller/common/images.gopkg/controller/template/constants.gopkg/controller/template/render.gopkg/controller/template/render_test.gopkg/operator/bootstrap.gopkg/operator/bootstrap_dependencies.gopkg/operator/bootstrap_test.gopkg/operator/render.gopkg/operator/render_test.gopkg/operator/sync.gopkg/operator/sync_test.gotemplates/common/on-prem/files/0020-kube-vip-ingress.yamltemplates/common/on-prem/files/keepalived.yamltemplates/master/00-master/on-prem/files/0000-frr-k8s.yamltemplates/master/00-master/on-prem/files/0010-kube-vip-api.yamltemplates/master/00-master/on-prem/files/frr-k8s-conf.yamltemplates/master/00-master/on-prem/files/frr-k8s-peers.yamltemplates/master/00-master/on-prem/files/frr-k8s-startup-daemons.yamltemplates/master/00-master/on-prem/files/frr-k8s-startup-vtysh.yamltemplates/master/00-master/on-prem/units/frr-k8s-hostpath.service.yaml
🚧 Files skipped from review as they are similar to previous changes (24)
- manifests/on-prem/frr.conf.tmpl
- install/0000_80_machine-config_02_images.configmap.yaml
- templates/master/00-master/on-prem/files/frr-k8s-peers.yaml
- templates/master/00-master/on-prem/files/frr-k8s-startup-vtysh.yaml
- templates/master/00-master/on-prem/files/frr-k8s-startup-daemons.yaml
- install/image-references
- pkg/operator/render_test.go
- manifests/on-prem/frr-peers.json.tmpl
- manifests/on-prem/0010-kube-vip-api.yaml
- pkg/controller/common/helpers.go
- pkg/controller/common/images.go
- manifests/on-prem/frr-startup-daemons
- pkg/controller/template/constants.go
- templates/master/00-master/on-prem/files/frr-k8s-conf.yaml
- pkg/controller/template/render_test.go
- manifests/on-prem/0000-frr-k8s.yaml
- pkg/operator/sync.go
- pkg/operator/render.go
- pkg/operator/bootstrap_dependencies.go
- install/0000_80_machine-config_00_rbac.yaml
- pkg/operator/bootstrap_test.go
- cmd/machine-config-operator/bootstrap.go
- pkg/controller/template/render.go
- pkg/operator/bootstrap.go
mkowalski
force-pushed
the
OPNET-595-mco-pr
branch
from
July 22, 2026 11:28
a3aa0f9 to
9d3ad42
Compare
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 `@templates/master/00-master/on-prem/files/0000-frr-k8s.yaml`:
- Around line 13-19: Update the pod specification’s
terminationGracePeriodSeconds in the FRR Kubernetes manifest from zero to a
positive grace period sufficient for the frr, reloader, and controller
containers to gracefully close BGP sessions and withdraw VIP routes during
termination.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4d2dc89b-eaa1-4776-9104-161bf8a49944
📒 Files selected for processing (31)
cmd/machine-config-operator/bootstrap.goinstall/0000_80_machine-config_00_rbac.yamlinstall/0000_80_machine-config_02_images.configmap.yamlinstall/image-referencesmanifests/on-prem/0000-frr-k8s.yamlmanifests/on-prem/0010-kube-vip-api.yamlmanifests/on-prem/frr-peers.json.tmplmanifests/on-prem/frr-startup-daemonsmanifests/on-prem/frr-startup-vtysh.confmanifests/on-prem/frr.conf.tmplpkg/controller/common/helpers.gopkg/controller/common/images.gopkg/controller/template/constants.gopkg/controller/template/render.gopkg/controller/template/render_test.gopkg/operator/bootstrap.gopkg/operator/bootstrap_dependencies.gopkg/operator/bootstrap_test.gopkg/operator/render.gopkg/operator/render_test.gopkg/operator/sync.gopkg/operator/sync_test.gotemplates/common/on-prem/files/0020-kube-vip-ingress.yamltemplates/common/on-prem/files/keepalived.yamltemplates/master/00-master/on-prem/files/0000-frr-k8s.yamltemplates/master/00-master/on-prem/files/0010-kube-vip-api.yamltemplates/master/00-master/on-prem/files/frr-k8s-conf.yamltemplates/master/00-master/on-prem/files/frr-k8s-peers.yamltemplates/master/00-master/on-prem/files/frr-k8s-startup-daemons.yamltemplates/master/00-master/on-prem/files/frr-k8s-startup-vtysh.yamltemplates/master/00-master/on-prem/units/frr-k8s-hostpath.service.yaml
🚧 Files skipped from review as they are similar to previous changes (26)
- manifests/on-prem/frr-startup-vtysh.conf
- install/0000_80_machine-config_02_images.configmap.yaml
- manifests/on-prem/frr-peers.json.tmpl
- templates/master/00-master/on-prem/files/frr-k8s-startup-vtysh.yaml
- manifests/on-prem/0010-kube-vip-api.yaml
- install/image-references
- install/0000_80_machine-config_00_rbac.yaml
- manifests/on-prem/frr-startup-daemons
- templates/master/00-master/on-prem/files/frr-k8s-peers.yaml
- manifests/on-prem/frr.conf.tmpl
- templates/master/00-master/on-prem/files/frr-k8s-startup-daemons.yaml
- pkg/operator/render.go
- pkg/controller/common/helpers.go
- pkg/operator/render_test.go
- templates/master/00-master/on-prem/files/frr-k8s-conf.yaml
- pkg/controller/common/images.go
- pkg/controller/template/constants.go
- pkg/operator/bootstrap_test.go
- pkg/controller/template/render_test.go
- manifests/on-prem/0000-frr-k8s.yaml
- pkg/operator/bootstrap_dependencies.go
- cmd/machine-config-operator/bootstrap.go
- pkg/operator/bootstrap.go
- pkg/controller/template/render.go
- pkg/operator/sync.go
- pkg/operator/sync_test.go
mkowalski
force-pushed
the
OPNET-595-mco-pr
branch
from
July 22, 2026 13:09
9d3ad42 to
9b1afc6
Compare
Contributor
Author
|
@CodeRabbit full review |
✅ Action performedFull review finished. |
Contributor
Author
|
@coderabbitai The hostNetwork and SYS_ADMIN usage flagged by the Container-Privileges pre-merge check is required and now justified explicitly in the PR description ("Privileged settings justification" section) and as comments in the manifests: hostNetwork is functionally required (BGP sessions originate from the node IP; VIP routes live in the host routing table — keepalived parity), and SYS_ADMIN on the frr container matches the CNO-shipped frr-k8s DaemonSet and is required by FRR's docker-start/watchfrr supervision. Please re-evaluate the check. This comment was written with AI assistance. Please verify before acting on it. |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
templates/master/00-master/on-prem/files/frr-k8s-conf.yaml (1)
1-63: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse decimal
420for the file mode
Ignition file permissions are decimal integers, so0644is parser-dependent here;420makes the intended-rw-r--r--explicit.🤖 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 `@templates/master/00-master/on-prem/files/frr-k8s-conf.yaml` around lines 1 - 63, Update the file mode declaration for the FRR configuration template to use decimal 420 instead of the octal-style 0644, preserving the resulting -rw-r--r-- permissions.
🧹 Nitpick comments (1)
pkg/operator/bootstrap.go (1)
249-294: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse the typed constant instead of a bare string literal for the BGP comparison.
vipManagement == "BGP"compares against a literal string, while the assignment site (line 175) correctly usesconfigv1.VIPManagementTypeBGP. A silent typo or future constant-value change here would make BGP clusters silently render keepalived manifests instead of failing loudly.♻️ Suggested fix
- if vipManagement == "BGP" { + if vipManagement == configv1.VIPManagementTypeBGP {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/operator/bootstrap.go` around lines 249 - 294, Update the BGP comparison in getPlatformManifests to use configv1.VIPManagementTypeBGP instead of the bare "BGP" string literal, preserving the existing manifest-selection behavior for BGP and non-BGP VIP management types.
🤖 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 `@go.mod`:
- Around line 476-477: Document the temporary fork replacement directly
alongside the github.com/openshift/api replace directive, including a reference
to the relevant upstream issue or PR and the condition or planned point for
removing it. Keep the replacement unchanged unless upstream support is already
available, in which case remove it.
In `@manifests/on-prem/0010-kube-vip-api.yaml`:
- Around line 20-46: Add the kube-vip control-plane health-check address setting
alongside the existing health-check environment variables in the on-prem API
manifest, using the local apiserver endpoint required to withdraw the VIP when
unhealthy. Apply the same configuration to the matching day-2 manifest,
preserving the existing backend_health_check_interval setting.
In `@pkg/operator/sync.go`:
- Around line 393-407: Update syncBGPVIPPeersJSON to avoid an unbounded live API
call: first reuse an existing informer/lister for the
openshift-network-operator/bgp-vip-config ConfigMap if one is already available,
otherwise create a context.Context with an appropriate timeout and pass it to
the ConfigMaps Get call, ensuring cancellation and cleanup while preserving the
existing not-found and other error handling.
In `@templates/master/00-master/on-prem/files/0010-kube-vip-api.yaml`:
- Around line 20-46: Add the kube-vip control-plane health gate to the container
environment in the manifest, matching the ingress VIP’s
control_plane_health_check_address and timeout settings while preserving the
existing routing-table configuration.
In `@templates/master/00-master/on-prem/files/frr-k8s-peers.yaml`:
- Around line 1-5: Update the mode for the frr-peers.json resource in the
surrounding template to restrict access to owner/group-only rather than
world-readable, using the repository’s established mode encoding convention
consistently with frr-k8s-conf.yaml.
---
Outside diff comments:
In `@templates/master/00-master/on-prem/files/frr-k8s-conf.yaml`:
- Around line 1-63: Update the file mode declaration for the FRR configuration
template to use decimal 420 instead of the octal-style 0644, preserving the
resulting -rw-r--r-- permissions.
---
Nitpick comments:
In `@pkg/operator/bootstrap.go`:
- Around line 249-294: Update the BGP comparison in getPlatformManifests to use
configv1.VIPManagementTypeBGP instead of the bare "BGP" string literal,
preserving the existing manifest-selection behavior for BGP and non-BGP VIP
management types.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 39f9155a-27dd-40e6-923b-7306a3c00826
⛔ Files ignored due to path filters (76)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/.golangci.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_infrastructure.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_ingress.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Hypershift-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-SelfManagedHA-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/features.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Hypershift-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/types_authentication.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_csi_cluster_driver.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_ingresscontroller.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_console_01_consoles.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/types_clusterapi.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/osin/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/osin/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/google.golang.org/protobuf/encoding/protodelim/protodelim.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/encoding/protojson/decode.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/encoding/protojson/well_known_types.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/encoding/prototext/decode.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/descfmt/stringer.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/encoding/tag/tag.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/encoding/text/decode.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/filedesc/desc.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/filedesc/desc_lazy.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/genid/descriptor_gen.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/impl/codec_map.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/impl/decode.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/impl/validate.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/version/version.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/proto/decode.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/reflect/protodesc/desc.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/reflect/protodesc/desc_init.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/reflect/protodesc/editions.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/types/descriptorpb/descriptor.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/types/known/timestamppb/timestamp.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (34)
cmd/machine-config-operator/bootstrap.gogo.modinstall/0000_80_machine-config_00_rbac.yamlinstall/0000_80_machine-config_02_images.configmap.yamlinstall/image-referencesmanifests/on-prem/0000-frr-k8s.yamlmanifests/on-prem/0010-kube-vip-api.yamlmanifests/on-prem/frr-peers.json.tmplmanifests/on-prem/frr-startup-daemonsmanifests/on-prem/frr-startup-vtysh.confmanifests/on-prem/frr.conf.tmplpkg/controller/common/helpers.gopkg/controller/common/images.gopkg/controller/container-runtime-config/actual.jsonpkg/controller/container-runtime-config/expected.jsonpkg/controller/template/constants.gopkg/controller/template/render.gopkg/controller/template/render_test.gopkg/operator/bootstrap.gopkg/operator/bootstrap_dependencies.gopkg/operator/bootstrap_test.gopkg/operator/render.gopkg/operator/render_test.gopkg/operator/sync.gopkg/operator/sync_test.gotemplates/common/on-prem/files/0020-kube-vip-ingress.yamltemplates/common/on-prem/files/keepalived.yamltemplates/master/00-master/on-prem/files/0000-frr-k8s.yamltemplates/master/00-master/on-prem/files/0010-kube-vip-api.yamltemplates/master/00-master/on-prem/files/frr-k8s-conf.yamltemplates/master/00-master/on-prem/files/frr-k8s-peers.yamltemplates/master/00-master/on-prem/files/frr-k8s-startup-daemons.yamltemplates/master/00-master/on-prem/files/frr-k8s-startup-vtysh.yamltemplates/master/00-master/on-prem/units/frr-k8s-hostpath.service.yaml
Comment on lines
+20
to
+46
| - name: kube-vip | ||
| image: {{.Images.kubeVipImage}} | ||
| args: | ||
| - manager | ||
| env: | ||
| - name: address | ||
| value: "{{ onPremPlatformAPIServerInternalIP . }}" | ||
| - name: k8s_config_file | ||
| value: "/etc/kubernetes/kubeconfig" | ||
| - name: kubernetes_addr | ||
| value: "https://localhost:6443" | ||
| - name: port | ||
| value: "6443" | ||
| - name: cp_enable | ||
| value: "true" | ||
| - name: vip_leaderelection | ||
| value: "true" | ||
| - name: vip_routingtable | ||
| value: "true" | ||
| - name: vip_cleanroutingtable | ||
| value: "true" | ||
| - name: vip_routingtableid | ||
| value: "198" | ||
| - name: vip_routingtableprotocol | ||
| value: "248" | ||
| - name: backend_health_check_interval | ||
| value: "5" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
printf '\n## File listing\n'
git ls-files 'templates/master/00-master/on-prem/files/0010-kube-vip-api.yaml' \
'manifests/on-prem/0010-kube-vip-api.yaml' \
'templates/common/on-prem/files/0020-kube-vip-ingress.yaml' \
'manifests/on-prem/0020-kube-vip-ingress.yaml'
printf '\n## Search health-check env vars\n'
rg -n 'control_plane_health_check|backend_health_check_interval|vip_leaderelection|kube-vip' templates manifests -g '*.yaml' -g '*.yml'
printf '\n## Read target file excerpt\n'
sed -n '1,120p' templates/master/00-master/on-prem/files/0010-kube-vip-api.yaml
printf '\n## Read sibling ingress excerpt\n'
sed -n '1,160p' templates/common/on-prem/files/0020-kube-vip-ingress.yaml
printf '\n## Read bootstrap api excerpt\n'
sed -n '1,120p' manifests/on-prem/0010-kube-vip-api.yamlRepository: openshift/machine-config-operator
Length of output: 1952
🌐 Web query:
kube-vip control_plane_health_check_address control_plane_health_check_timeout_seconds env vars manager mode api vip local apiserver health check
💡 Result:
Kube-vip provides a mechanism to monitor the local Kubernetes API server and automatically withdraw BGP routes if the server becomes unresponsive. This feature is designed primarily for BGP mode to maintain high availability [1][2]. Control Plane Health Check Configuration The health check is disabled by default [1]. To enable it, you must configure the following environment variables (or their corresponding CLI flags) [1][3]: - control_plane_health_check_address: The URL to poll for health status (e.g., https://localhost:6443/livez) [1][3]. Setting this enables the health check [2]. - control_plane_health_check_period_seconds: The interval between health check requests (default: 5) [1][3]. - control_plane_health_check_timeout_seconds: The timeout for each individual HTTP request (default: 3) [1][3]. - control_plane_health_check_failure_threshold: The number of consecutive failed probes before the BGP route is withdrawn (default: 3) [1][3]. - control_plane_health_check_ca_path: The path to a CA certificate for HTTPS verification. This is required if the API server uses a private CA [1][3]. How the Health Check Works When enabled, kube-vip continuously polls the specified address. If the endpoint fails to respond successfully within the configured timeout, it records a failure. The BGP route is withdrawn only after the number of consecutive failures reaches the configured threshold [1][2]. This approach prevents route flapping caused by transient network issues or temporary API server restarts [2]. Important Considerations - Behavioral Change: When this feature is enabled, BGP routes are not announced until the first successful health check probe is completed [2]. - Performance: Reducing the period and timeout values allows for faster detection of failures but increases the polling load on the API server [1]. - API server dependency: While commonly used for the local API server (e.g., localhost:6443), the address field is generic and can technically be used to monitor any service endpoint [2]. For additional context on kube-vip configuration, see the official documentation for environment variables [3] and BGP-specific health check details [1].
Citations:
- 1: https://kube-vip.io/docs/usage/bgp-health-check/
- 2: Add configurable control-plane health check for BGP mode kube-vip/kube-vip#1498
- 3: https://kube-vip.io/docs/installation/flags/
🌐 Web query:
kube-vip vip_routingtable control_plane_health_check_address routing table mode health check official docs
💡 Result:
In kube-vip, Routing Table mode allows the tool to manage virtual IP (VIP) addresses by adding or deleting them directly in the node's routing tables (typically using table ID 198 and protocol ID 248) rather than using dynamic routing protocols like BGP [1][2]. The control_plane_health_check_address functionality, initially designed for BGP mode [3][4], has been extended to support Routing Table mode [5]. This health check mechanism polls a specified URL (e.g., https://localhost:6443/livez) to determine the health of the control plane [3][6]. Key configuration details include: * Health Check Address (control_plane_health_check_address): Set this URL to enable the health check [3][6]. When enabled, kube-vip will not announce or configure the VIP route until the first successful probe [3][4]. If the probe fails consecutively beyond the failure threshold, the route is withdrawn [3][4]. * Thresholds and Timing: * control_plane_health_check_period_seconds: Defaults to 5 seconds [3][6]. * control_plane_health_check_timeout_seconds: Defaults to 3 seconds [3][6]. * control_plane_health_check_failure_threshold: Defaults to 3 consecutive failures before the route is withdrawn [3][6]. * HTTPS Support: Use control_plane_health_check_ca_path to specify the path to a CA certificate if using HTTPS with a private CA [3][6]. * Routing Table Mode Specifics: In Routing Table mode, enabling this health check ensures that the VIP is only managed (present in the routing table) when the polled endpoint is healthy [5]. For Routing Table mode, ensure vip_routingtable is set to true in your configuration [7]. You can also optionally use vip_cleanroutingtable to clear unwanted routes at startup [7]. While BGP mode relies on a routing daemon (like BIRD) to read and advertise these routes to neighbors [7], Routing Table mode simply performs the netlink operations to update the local node's routing table [1].
Citations:
- 1: https://kube-vip.io/docs/modes/routingtables/
- 2: https://kube-vip.io/docs/troubleshooting/routingtables/
- 3: https://kube-vip.io/docs/usage/bgp-health-check/
- 4: Add configurable control-plane health check for BGP mode kube-vip/kube-vip#1498
- 5: kube-vip/kube-vip@fcd3eec
- 6: https://kube-vip.io/docs/installation/flags/
- 7: https://kube-vip.io/docs/usage/routingtables/
Add the control-plane health gate to the API VIP manifest
templates/master/00-master/on-prem/files/0010-kube-vip-api.yaml should set control_plane_health_check_address (and timeout) like the ingress VIP does; routing-table mode supports this gate, and without it kube-vip can keep advertising the API VIP even when the local apiserver is unhealthy.
🤖 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 `@templates/master/00-master/on-prem/files/0010-kube-vip-api.yaml` around lines
20 - 46, Add the kube-vip control-plane health gate to the container environment
in the manifest, matching the ingress VIP’s control_plane_health_check_address
and timeout settings while preserving the existing routing-table configuration.
Contributor
Author
There was a problem hiding this comment.
Same as on the bootstrap manifest: the API VIP is already health-gated by kube-vip's backend check (ServerVersion against the local apiserver every backend_health_check_interval); control_plane_health_check_address is the override used by the ingress pod because routers have no apiserver. See the fuller explanation on the bootstrap-manifest thread.
This reply was written with AI assistance. Please verify before acting on it.
There was a problem hiding this comment.
@mkowalski I couldn't re-verify this change because repository access failed, so I'm leaving the finding unresolved. Please retry the recheck after sandbox access is restored.
✏️ Learnings added
Learnt from: mkowalski
Repo: openshift/machine-config-operator PR: 6326
File: templates/master/00-master/on-prem/files/0010-kube-vip-api.yaml:20-46
Timestamp: 2026-07-22T15:32:55.850Z
Learning: In openshift/machine-config-operator PR `#6326` BGP VIP static pods, `templates/master/00-master/on-prem/files/0010-kube-vip-api.yaml` already health-gates the API VIP using kube-vip’s `backend_health_check_interval`: with its local API kubeconfig/endpoint configuration, kube-vip probes the local apiserver through `ServerVersion()`. Do not require `control_plane_health_check_address` there. That explicit HTTP health-check override is used by the ingress kube-vip pod because ingress routers do not have a local apiserver for the backend check.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: mkowalski
Repo: openshift/machine-config-operator PR: 6326
File: manifests/on-prem/0000-frr-k8s.yaml:91-131
Timestamp: 2026-07-22T11:15:44.757Z
Learning: For BGP VIP static pods, do not require liveness/readiness probes on the FRR daemon container in `0000-frr-k8s.yaml` (reason: FRR is supervised by `watchfrr`; an unreliable `vtysh`-based probe can restart the container and tear down BGP sessions). In the day-2 FRR static pod template `templates/master/00-master/on-prem/files/0000-frr-k8s.yaml`, ensure probes are provided by the separate controller container via `/healthz` and `/readyz`, not by probing the FRR daemon directly; rely on the CNO-managed metrics companion DaemonSet for externally visible metrics/live status. For the kube-vip container in `0010-kube-vip-api.yaml`, do not add probes; in routing-table mode kube-vip fails safe by withdrawing the VIP route on death, and there is no suitable independent health endpoint to probe.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== PR description ==='
gh pr view 6326 --repo openshift/machine-config-operator --json body --jq .body
echo '=== Candidate manifest files ==='
fd -t f . manifests templates | rg '(frr-k8s|kube-vip|keepalived)'
echo '=== hostNetwork, SYS_ADMIN, and justification comments ==='
rg -n -i -C 8 '(hostNetwork:|SYS_ADMIN|privileged settings|BGP sessions|routing table|watchfrr|docker-start)' \
manifests templatesLength of output: 46704
I’m withdrawing the Container-Privileges finding. ✏️ Learnings added
|
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
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.
When the BareMetal Infrastructure CR requests vipManagement "BGP" (BGPBasedVIPManagement gate), render the frr-k8s static pod on control plane nodes and the kube-vip static pods (API VIP on masters, ingress VIP on every node, keepalived parity) instead of the keepalived manifests. kube-vip runs in routing table mode: it installs a VIP route into kernel table 198 only while the local backend health check passes, and FRR advertises that table, giving per-node health-gated ECMP. The frr-k8s sockets and config live on a hostPath prepared by a boot unit (0777 emptyDir-parity for the mixed-uid FRR daemons, container_file_t for the MCS-confined metrics companion that CNO deploys against them). BGP mode additionally requires VIPs to exist: with a user-managed load balancer there is nothing to advertise and the templates would render empty values.
Bootstrap: RenderBootstrap reads the installer-generated bgp-vip-config ConfigMap manifest (--bgp-vip-config-file) and exposes its config.json as ControllerConfigSpec.BGPVIPPeersJSON, from which the templates render the node peers file that baremetal-runtimecfg resolves on each node. The bootstrap node itself runs an FRR-only static pod so the API VIP is advertised while the control plane forms. When BGP VIP management is enabled, missing frr-k8s/kube-vip images fail the bootstrap render instead of dying later with empty image fields. Day-2: the operator re-reads the same ConfigMap on every sync (new Role/RoleBinding for openshift-network-operator) so ControllerConfig follows peer changes; absence degrades the operator rather than blanking the peers file fleet-wide. The frr-k8s and kube-vip images resolve from the release payload; the image-references addition ships separately (payload assembly needs the kube-vip tag onboarded first).
Split from the previous commit on purpose: nightly payload assembly fails on an image-references entry whose tag does not exist, so this lands only after the kube-vip image is onboarded (ART-21663, openshift/release#81957).
mkowalski
force-pushed
the
OPNET-595-mco-pr
branch
from
July 22, 2026 15:30
9b1afc6 to
dfb4e14
Compare
Contributor
Author
|
Also addressing the outside-diff comment on frr-k8s-conf.yaml (mode 0644 vs decimal 420): keeping the octal form — the MCO template loader parses YAML 1.1 octal literals and every existing template file in this repo uses that convention (0644/0755/0600 across templates/). frr-peers.json was however tightened to 0600 since it can carry BGP MD5 passwords. @coderabbitai full review This comment was written with AI assistance. Please verify before acting on it. |
|
✏️ Learnings added
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
manifests/on-prem/frr-peers.json.tmpl (1)
1-2: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winBootstrap render still writes
frr-peers.jsonas0644
pkg/operator/bootstrap.gowrites this payload withos.WriteFile(..., 0o644), so it does not get the same0600hardening as the day-2 file. Make the bootstrap write0600too.🤖 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 `@manifests/on-prem/frr-peers.json.tmpl` around lines 1 - 2, Update the bootstrap write for the BGP VIP peers payload in pkg/operator/bootstrap.go to use file mode 0600 instead of 0644, ensuring the frr-peers.json bootstrap output matches the day-2 file hardening. Locate the os.WriteFile call handling BGPVIPPeersJSON and change only its permission mode.
🤖 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 `@templates/master/00-master/on-prem/units/frr-k8s-hostpath.service.yaml`:
- Line 18: Update the ExecStart setup in the frr-k8s-hostpath service to remove
world-writable 0777 permissions from the shared sockets and conf directories.
Configure a shared group with 0770 permissions, or equivalent ACLs limited to
the FRR and metrics companion identities, while preserving directory creation
and SELinux labeling.
---
Outside diff comments:
In `@manifests/on-prem/frr-peers.json.tmpl`:
- Around line 1-2: Update the bootstrap write for the BGP VIP peers payload in
pkg/operator/bootstrap.go to use file mode 0600 instead of 0644, ensuring the
frr-peers.json bootstrap output matches the day-2 file hardening. Locate the
os.WriteFile call handling BGPVIPPeersJSON and change only its permission mode.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 71eaad5c-e346-4329-91f9-d91616684fb9
⛔ Files ignored due to path filters (76)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/.golangci.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_infrastructure.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_ingress.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Hypershift-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-SelfManagedHA-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/features.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Hypershift-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/types_authentication.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_csi_cluster_driver.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_ingresscontroller.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_console_01_consoles.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/types_clusterapi.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/osin/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/osin/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/google.golang.org/protobuf/encoding/protodelim/protodelim.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/encoding/protojson/decode.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/encoding/protojson/well_known_types.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/encoding/prototext/decode.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/descfmt/stringer.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/encoding/tag/tag.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/encoding/text/decode.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/filedesc/desc.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/filedesc/desc_lazy.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/genid/descriptor_gen.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/impl/codec_map.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/impl/decode.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/impl/validate.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/version/version.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/proto/decode.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/reflect/protodesc/desc.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/reflect/protodesc/desc_init.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/reflect/protodesc/editions.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/types/descriptorpb/descriptor.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/types/known/timestamppb/timestamp.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (34)
cmd/machine-config-operator/bootstrap.gogo.modinstall/0000_80_machine-config_00_rbac.yamlinstall/0000_80_machine-config_02_images.configmap.yamlinstall/image-referencesmanifests/on-prem/0000-frr-k8s.yamlmanifests/on-prem/0010-kube-vip-api.yamlmanifests/on-prem/frr-peers.json.tmplmanifests/on-prem/frr-startup-daemonsmanifests/on-prem/frr-startup-vtysh.confmanifests/on-prem/frr.conf.tmplpkg/controller/common/helpers.gopkg/controller/common/images.gopkg/controller/container-runtime-config/actual.jsonpkg/controller/container-runtime-config/expected.jsonpkg/controller/template/constants.gopkg/controller/template/render.gopkg/controller/template/render_test.gopkg/operator/bootstrap.gopkg/operator/bootstrap_dependencies.gopkg/operator/bootstrap_test.gopkg/operator/render.gopkg/operator/render_test.gopkg/operator/sync.gopkg/operator/sync_test.gotemplates/common/on-prem/files/0020-kube-vip-ingress.yamltemplates/common/on-prem/files/keepalived.yamltemplates/master/00-master/on-prem/files/0000-frr-k8s.yamltemplates/master/00-master/on-prem/files/0010-kube-vip-api.yamltemplates/master/00-master/on-prem/files/frr-k8s-conf.yamltemplates/master/00-master/on-prem/files/frr-k8s-peers.yamltemplates/master/00-master/on-prem/files/frr-k8s-startup-daemons.yamltemplates/master/00-master/on-prem/files/frr-k8s-startup-vtysh.yamltemplates/master/00-master/on-prem/units/frr-k8s-hostpath.service.yaml
🚧 Files skipped from review as they are similar to previous changes (25)
- manifests/on-prem/frr-startup-vtysh.conf
- templates/master/00-master/on-prem/files/frr-k8s-peers.yaml
- pkg/controller/container-runtime-config/actual.json
- install/image-references
- pkg/controller/container-runtime-config/expected.json
- manifests/on-prem/frr-startup-daemons
- pkg/controller/common/helpers.go
- install/0000_80_machine-config_02_images.configmap.yaml
- pkg/controller/template/constants.go
- templates/master/00-master/on-prem/files/frr-k8s-startup-daemons.yaml
- manifests/on-prem/0010-kube-vip-api.yaml
- pkg/controller/common/images.go
- install/0000_80_machine-config_00_rbac.yaml
- pkg/operator/render_test.go
- cmd/machine-config-operator/bootstrap.go
- templates/master/00-master/on-prem/files/frr-k8s-conf.yaml
- manifests/on-prem/0000-frr-k8s.yaml
- pkg/operator/bootstrap_test.go
- go.mod
- pkg/operator/render.go
- pkg/operator/sync.go
- manifests/on-prem/frr.conf.tmpl
- pkg/controller/template/render_test.go
- pkg/operator/bootstrap.go
- pkg/operator/bootstrap_dependencies.go
Contributor
|
@mkowalski: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
templates/master/00-master/on-prem/units/frr-k8s-hostpath.service.yaml (1)
18-18: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftRestrict the shared FRR directories.
Line 18 makes both runtime directories world-writable. This permits tampering with FRR sockets/configuration, including configuration that can contain BGP peer passwords. Preserve companion access with explicit shared ownership/ACLs and restrictive modes instead of
0777.#!/bin/bash set -euo pipefail rg -n -C 4 '/run/frr-k8s/(sockets|conf)|runAsUser|runAsGroup|fsGroup|frr.conf.tmpl|frr-peers.json' \ templates manifests -g '*.yaml' -g '*.yml'🤖 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 `@templates/master/00-master/on-prem/units/frr-k8s-hostpath.service.yaml` at line 18, Restrict the /run/frr-k8s/sockets and /run/frr-k8s/conf directories in the ExecStart command instead of applying 0777. Preserve access for the FRR companion processes by using the established shared group/ACL configuration and restrictive directory modes, ensuring configuration and socket contents are not writable by everyone.
🧹 Nitpick comments (2)
pkg/operator/bootstrap.go (1)
249-294: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider comparing against the typed constant instead of a literal string.
vipManagement == "BGP"(line 273) duplicates the value already expressed byconfigv1.VIPManagementTypeBGP(used to setvipManagementa few lines earlier at line 175). Using the constant on both sides avoids silent drift if the underlying value ever changes.♻️ Proposed refactor
- if lbType == configv1.LoadBalancerTypeOpenShiftManagedDefault || lbType == "" { - if vipManagement == "BGP" { + if lbType == configv1.LoadBalancerTypeOpenShiftManagedDefault || lbType == "" { + if vipManagement == configv1.VIPManagementTypeBGP {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/operator/bootstrap.go` around lines 249 - 294, Update the VIP management comparison in getPlatformManifests to use configv1.VIPManagementTypeBGP instead of the literal "BGP", preserving the existing BGP and non-BGP manifest-selection behavior.pkg/controller/template/render_test.go (1)
599-743: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood coverage for
isBGPVIPManagement; consider also covering the nil-IP-list fix directly.The table-driven test thoroughly covers nil infra, nil/empty platform status, BGP with/without VIPs, Keepalived, and non-BareMetal platforms. However, none of the cases exercise
onPremPlatformIngressIP/onPremPlatformAPIServerInternalIPdirectly for the new empty-slice guard added inrender.go(Lines 501-513, 526-528, 573-585, 598-600) — that behavior change (returningnilinstead of panicking onIngressIPs[0]) is only indirectly implied here. Consider adding a small test rendering{{ onPremPlatformIngressIP . }}against aBareMetalstatus with emptyIngressIPsto lock in the fix.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/controller/template/render_test.go` around lines 599 - 743, Add a focused test for onPremPlatformIngressIP that renders the template with a BareMetal platform status whose IngressIPs slice is empty, and assert it returns the expected nil-safe result without an error or panic. Anchor the new case to the existing renderTemplate test setup and, if appropriate, add the analogous coverage for onPremPlatformAPIServerInternalIP.
🤖 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 `@go.mod`:
- Line 3: Update the go directive in go.mod from Go 1.26.0 to at least Go
1.26.5, ensuring the repository uses a patched Go 1.26 toolchain; alternatively,
add an equivalent newer toolchain pin in the project’s CI configuration.
In `@pkg/operator/bootstrap.go`:
- Around line 115-123: Extend the BGP validation guard in buildSpec, alongside
the existing imgs.FRRK8s and imgs.KubeVip checks, to reject an empty
dependencies.BGPVIPPeersJSON when
ctrlcommon.IsBGPVIPManagement(dependencies.Infrastructure) is true. Return a
descriptive error identifying the missing BGP VIP peers payload, while
preserving the current validation behavior for non-BGP configurations.
---
Duplicate comments:
In `@templates/master/00-master/on-prem/units/frr-k8s-hostpath.service.yaml`:
- Line 18: Restrict the /run/frr-k8s/sockets and /run/frr-k8s/conf directories
in the ExecStart command instead of applying 0777. Preserve access for the FRR
companion processes by using the established shared group/ACL configuration and
restrictive directory modes, ensuring configuration and socket contents are not
writable by everyone.
---
Nitpick comments:
In `@pkg/controller/template/render_test.go`:
- Around line 599-743: Add a focused test for onPremPlatformIngressIP that
renders the template with a BareMetal platform status whose IngressIPs slice is
empty, and assert it returns the expected nil-safe result without an error or
panic. Anchor the new case to the existing renderTemplate test setup and, if
appropriate, add the analogous coverage for onPremPlatformAPIServerInternalIP.
In `@pkg/operator/bootstrap.go`:
- Around line 249-294: Update the VIP management comparison in
getPlatformManifests to use configv1.VIPManagementTypeBGP instead of the literal
"BGP", preserving the existing BGP and non-BGP manifest-selection behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c0c3a62b-891b-4fd8-9cc1-5d6061a65a03
⛔ Files ignored due to path filters (76)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/.golangci.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_infrastructure.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_ingress.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Hypershift-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-SelfManagedHA-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/features.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Hypershift-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/types_authentication.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_csi_cluster_driver.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_ingresscontroller.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_console_01_consoles.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/types_clusterapi.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/osin/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/osin/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/google.golang.org/protobuf/encoding/protodelim/protodelim.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/encoding/protojson/decode.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/encoding/protojson/well_known_types.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/encoding/prototext/decode.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/descfmt/stringer.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/encoding/tag/tag.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/encoding/text/decode.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/filedesc/desc.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/filedesc/desc_lazy.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/genid/descriptor_gen.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/impl/codec_map.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/impl/decode.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/impl/validate.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/version/version.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/proto/decode.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/reflect/protodesc/desc.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/reflect/protodesc/desc_init.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/reflect/protodesc/editions.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/types/descriptorpb/descriptor.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/types/known/timestamppb/timestamp.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (34)
cmd/machine-config-operator/bootstrap.gogo.modinstall/0000_80_machine-config_00_rbac.yamlinstall/0000_80_machine-config_02_images.configmap.yamlinstall/image-referencesmanifests/on-prem/0000-frr-k8s.yamlmanifests/on-prem/0010-kube-vip-api.yamlmanifests/on-prem/frr-peers.json.tmplmanifests/on-prem/frr-startup-daemonsmanifests/on-prem/frr-startup-vtysh.confmanifests/on-prem/frr.conf.tmplpkg/controller/common/helpers.gopkg/controller/common/images.gopkg/controller/container-runtime-config/actual.jsonpkg/controller/container-runtime-config/expected.jsonpkg/controller/template/constants.gopkg/controller/template/render.gopkg/controller/template/render_test.gopkg/operator/bootstrap.gopkg/operator/bootstrap_dependencies.gopkg/operator/bootstrap_test.gopkg/operator/render.gopkg/operator/render_test.gopkg/operator/sync.gopkg/operator/sync_test.gotemplates/common/on-prem/files/0020-kube-vip-ingress.yamltemplates/common/on-prem/files/keepalived.yamltemplates/master/00-master/on-prem/files/0000-frr-k8s.yamltemplates/master/00-master/on-prem/files/0010-kube-vip-api.yamltemplates/master/00-master/on-prem/files/frr-k8s-conf.yamltemplates/master/00-master/on-prem/files/frr-k8s-peers.yamltemplates/master/00-master/on-prem/files/frr-k8s-startup-daemons.yamltemplates/master/00-master/on-prem/files/frr-k8s-startup-vtysh.yamltemplates/master/00-master/on-prem/units/frr-k8s-hostpath.service.yaml
| module github.com/openshift/machine-config-operator | ||
|
|
||
| go 1.25.3 | ||
| go 1.26.0 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --hidden \
-g 'go.mod' -g 'go.work' -g 'Dockerfile*' -g '*.yml' -g '*.yaml' -g 'Makefile' \
'go1\.26|GO_VERSION|golang:' .Repository: openshift/machine-config-operator
Length of output: 2936
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== go.mod ==\n'
cat -n go.mod
printf '\n== go toolchain / version directives in repo ==\n'
rg -n --hidden --glob '!vendor/**' '^[[:space:]]*(toolchain|go)[[:space:]]' go.mod go.work .github . || true
printf '\n== CI Go version pins ==\n'
rg -n --hidden --glob '!vendor/**' 'go-version|GO_VERSION|golang:' .github . || trueRepository: openshift/machine-config-operator
Length of output: 38773
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== workflow files ==\n'
fd -t f '.*\.(yml|yaml)$' .github 2>/dev/null || true
printf '\n== setup-go / toolchain references ==\n'
rg -n --hidden --glob '!vendor/**' --glob '.github/**/*.yml' --glob '.github/**/*.yaml' \
'setup-go|go-version|toolchain|GOTOOLCHAIN|golang:' .github . || trueRepository: openshift/machine-config-operator
Length of output: 232
🌐 Web query:
Go 1.26.5 release notes security fixes July 7 2026
💡 Result:
Go 1.26.5 was released on July 7, 2026 [1][2]. This minor point release includes two critical security fixes along with various bug fixes for the compiler, runtime, go command, and several standard library packages [1][3]. The two security vulnerabilities addressed in this release are: 1. os: Root escape via symlink plus trailing slash (CVE-2026-39822) On Unix systems, the os.Root functionality improperly followed symbolic links to locations outside the intended root directory when the final path component was a symbolic link and the path ended with a trailing slash [2][4]. This failure to account for openat behavior with trailing slashes allowed paths to escape the root. The issue has been resolved by properly sanitizing the path parameter [4][5]. 2. crypto/tls: Encrypted Client Hello privacy leak (CVE-2026-42505) The Encrypted Client Hello (ECH) implementation leaked pre-shared key (PSK) identities during the handshake [2][6]. This vulnerability could allow a passive network observer to de-anonymize the server's hostname even when ECH was in use [4][5]. Additional updates in Go 1.26.5 include bug fixes for the compiler, runtime, the go command, and the net, os, and syscall packages [1][3]. Detailed technical changes can be found in the official Go release history [1][7].
Citations:
- 1: https://go.dev/doc/devel/release
- 2: https://groups.google.com/g/golang-announce/c/OrmQE_Yp5Sc/m/G9tnwhteAAAJ
- 3: https://tip.golang.org/doc/devel/release
- 4: https://www.openwall.com/lists/oss-security/2026/07/08/10
- 5: https://seclists.org/oss-sec/2026/q3/102
- 6: https://pkg.go.dev/vuln/GO-2026-5856
- 7: golang/go@go1.26.4...go1.26.5
Pin a patched Go toolchain. go 1.26.0 leaves this repo on the initial 1.26 release, and there’s no toolchain or CI version pin here. Bump to at least go 1.26.5 or enforce a newer toolchain in CI.
🤖 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 `@go.mod` at line 3, Update the go directive in go.mod from Go 1.26.0 to at
least Go 1.26.5, ensuring the repository uses a patched Go 1.26 toolchain;
alternatively, add an equivalent newer toolchain pin in the project’s CI
configuration.
Comment on lines
+115
to
+123
| // BGP-based VIP management renders frr-k8s and kube-vip static pods at | ||
| // bootstrap; without their images the manifests would carry empty image | ||
| // fields and the install dies much later with no useful signal. | ||
| if ctrlcommon.IsBGPVIPManagement(dependencies.Infrastructure) { | ||
| if imgs.FRRK8s == "" || imgs.KubeVip == "" { | ||
| return nil, fmt.Errorf("BGP-based VIP management is enabled but the frr-k8s (%q) or kube-vip (%q) image is missing from the release payload", imgs.FRRK8s, imgs.KubeVip) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
BGP bootstrap doesn't hard-fail on an empty peers payload, unlike the adjacent image check.
buildSpec requires imgs.FRRK8s/imgs.KubeVip to be non-empty when BGP VIP management is active, but doesn't apply the same guard to dependencies.BGPVIPPeersJSON. fillBGPVIPConfig in pkg/operator/bootstrap_dependencies.go tolerates a missing bgp-vip-config file unconditionally — it never checks whether BGP is actually enabled on the infrastructure before deciding absence is fine. If the file is missing while BGP is enabled (wrong --bgp-vip-config-file path, installer ordering issue, etc.), spec.BGPVIPPeersJSON stays empty, and manifests/on-prem/frr-peers.json.tmpl ({{- .ControllerConfig.BGPVIPPeersJSON -}}) silently renders an empty peers file — the FRR static pod comes up with no configured peers instead of the bootstrap failing loudly, the same failure mode the image check right above was added to prevent. TestBuildSpecBGPImageValidation doesn't cover this case either.
🐛 Proposed fix: extend the existing BGP guard to cover the peers payload
if ctrlcommon.IsBGPVIPManagement(dependencies.Infrastructure) {
if imgs.FRRK8s == "" || imgs.KubeVip == "" {
return nil, fmt.Errorf("BGP-based VIP management is enabled but the frr-k8s (%q) or kube-vip (%q) image is missing from the release payload", imgs.FRRK8s, imgs.KubeVip)
}
+ if dependencies.BGPVIPPeersJSON == "" {
+ return nil, fmt.Errorf("BGP-based VIP management is enabled but no BGP VIP peers configuration (bgp-vip-config ConfigMap) was found")
+ }
}Also applies to: 142-142
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/operator/bootstrap.go` around lines 115 - 123, Extend the BGP validation
guard in buildSpec, alongside the existing imgs.FRRK8s and imgs.KubeVip checks,
to reject an empty dependencies.BGPVIPPeersJSON when
ctrlcommon.IsBGPVIPManagement(dependencies.Infrastructure) is true. Return a
descriptive error identifying the missing BGP VIP peers payload, while
preserving the current validation behavior for non-BGP configurations.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
pkg/operator/bootstrap.go (1)
118-122: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winBGP bootstrap still doesn't hard-fail on an empty peers payload.
The guard validates
imgs.FRRK8s/imgs.KubeVipbut notdependencies.BGPVIPPeersJSON. If BGP is genuinely enabled (VIPs present) but the peers ConfigMap/file is missing,spec.BGPVIPPeersJSONstays empty andfrr-peers.json.tmplsilently renders no peers instead of failing bootstrap loudly — the same failure mode the image check was added to prevent.🐛 Proposed fix
if ctrlcommon.IsBGPVIPManagement(dependencies.Infrastructure) { if imgs.FRRK8s == "" || imgs.KubeVip == "" { return nil, fmt.Errorf("BGP-based VIP management is enabled but the frr-k8s (%q) or kube-vip (%q) image is missing from the release payload", imgs.FRRK8s, imgs.KubeVip) } + if dependencies.BGPVIPPeersJSON == "" { + return nil, fmt.Errorf("BGP-based VIP management is enabled but no BGP VIP peers configuration (bgp-vip-config ConfigMap) was found") + } }Also applies to: 142-142
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/operator/bootstrap.go` around lines 118 - 122, The BGP validation in the bootstrap flow must also reject a missing peers payload. Within the ctrlcommon.IsBGPVIPManagement guard, validate dependencies.BGPVIPPeersJSON alongside imgs.FRRK8s and imgs.KubeVip, returning the existing bootstrap error path when it is empty so frr-peers.json.tmpl cannot render without peers.
🧹 Nitpick comments (1)
pkg/operator/bootstrap.go (1)
249-293: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer the typed constant over a raw string literal.
if vipManagement == "BGP"works today but drifts fromconfigv1.VIPManagementTypeBGPused inhelpers.goandrender.go; using the constant avoids future typos if moreVIPManagementTypevalues are added.- if vipManagement == "BGP" { + if vipManagement == configv1.VIPManagementTypeBGP {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/operator/bootstrap.go` around lines 249 - 293, Update the vipManagement comparison in getPlatformManifests to use the typed configv1.VIPManagementTypeBGP constant instead of the raw "BGP" string, preserving the existing BGP manifest-selection behavior.
🤖 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 `@go.mod`:
- Line 425: Update the google.golang.org/protobuf dependency in go.mod from the
unreleased pseudo-version to an available tagged release; only retain the
pseudo-version if this specific commit is required, and document that temporary
exception.
In `@pkg/controller/template/render_test.go`:
- Around line 663-675: The render test suite is missing coverage for ingress
VIPs being present while the API VIP is absent. Add a case alongside the
existing baremetal VIPManagement BGP cases in the relevant test table,
configuring ingress VIPs without APIServerInternalIPs and asserting the result
remains "false", thereby preserving the APIServerInternalIPs guard.
In `@templates/master/00-master/on-prem/files/frr-k8s-conf.yaml`:
- Around line 1-17: Change the mode for the rendered FRR configuration in the
manifest entry containing frr.conf.tmpl from 0644 to 0600, keeping the existing
path and contents unchanged so BGP peer passwords remain readable only by the
owning user.
---
Duplicate comments:
In `@pkg/operator/bootstrap.go`:
- Around line 118-122: The BGP validation in the bootstrap flow must also reject
a missing peers payload. Within the ctrlcommon.IsBGPVIPManagement guard,
validate dependencies.BGPVIPPeersJSON alongside imgs.FRRK8s and imgs.KubeVip,
returning the existing bootstrap error path when it is empty so
frr-peers.json.tmpl cannot render without peers.
---
Nitpick comments:
In `@pkg/operator/bootstrap.go`:
- Around line 249-293: Update the vipManagement comparison in
getPlatformManifests to use the typed configv1.VIPManagementTypeBGP constant
instead of the raw "BGP" string, preserving the existing BGP manifest-selection
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c0c3a62b-891b-4fd8-9cc1-5d6061a65a03
⛔ Files ignored due to path filters (76)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/.golangci.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_infrastructure.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_ingress.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Hypershift-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-SelfManagedHA-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/features.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Hypershift-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/types_authentication.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_csi_cluster_driver.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_ingresscontroller.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_console_01_consoles.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.model_name.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/types_clusterapi.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/osin/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/osin/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/google.golang.org/protobuf/encoding/protodelim/protodelim.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/encoding/protojson/decode.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/encoding/protojson/well_known_types.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/encoding/prototext/decode.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/descfmt/stringer.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/encoding/tag/tag.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/encoding/text/decode.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/filedesc/desc.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/filedesc/desc_lazy.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/genid/descriptor_gen.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/impl/codec_map.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/impl/decode.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/impl/validate.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/internal/version/version.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/proto/decode.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/reflect/protodesc/desc.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/reflect/protodesc/desc_init.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/reflect/protodesc/editions.gois excluded by!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/types/descriptorpb/descriptor.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/google.golang.org/protobuf/types/known/timestamppb/timestamp.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (34)
cmd/machine-config-operator/bootstrap.gogo.modinstall/0000_80_machine-config_00_rbac.yamlinstall/0000_80_machine-config_02_images.configmap.yamlinstall/image-referencesmanifests/on-prem/0000-frr-k8s.yamlmanifests/on-prem/0010-kube-vip-api.yamlmanifests/on-prem/frr-peers.json.tmplmanifests/on-prem/frr-startup-daemonsmanifests/on-prem/frr-startup-vtysh.confmanifests/on-prem/frr.conf.tmplpkg/controller/common/helpers.gopkg/controller/common/images.gopkg/controller/container-runtime-config/actual.jsonpkg/controller/container-runtime-config/expected.jsonpkg/controller/template/constants.gopkg/controller/template/render.gopkg/controller/template/render_test.gopkg/operator/bootstrap.gopkg/operator/bootstrap_dependencies.gopkg/operator/bootstrap_test.gopkg/operator/render.gopkg/operator/render_test.gopkg/operator/sync.gopkg/operator/sync_test.gotemplates/common/on-prem/files/0020-kube-vip-ingress.yamltemplates/common/on-prem/files/keepalived.yamltemplates/master/00-master/on-prem/files/0000-frr-k8s.yamltemplates/master/00-master/on-prem/files/0010-kube-vip-api.yamltemplates/master/00-master/on-prem/files/frr-k8s-conf.yamltemplates/master/00-master/on-prem/files/frr-k8s-peers.yamltemplates/master/00-master/on-prem/files/frr-k8s-startup-daemons.yamltemplates/master/00-master/on-prem/files/frr-k8s-startup-vtysh.yamltemplates/master/00-master/on-prem/units/frr-k8s-hostpath.service.yaml
| golang.org/x/text v0.40.0 // indirect | ||
| golang.org/x/tools v0.47.0 // indirect | ||
| google.golang.org/protobuf v1.36.10 // indirect | ||
| google.golang.org/protobuf v1.36.12-0.20260120151049-f2248ac996af // indirect |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
go list -m -json google.golang.org/protobuf
go mod graph | rg 'google\.golang\.org/protobuf'
git ls-remote --tags https://github.com/protocolbuffers/protobuf-go.git 'v1.36.*'Repository: openshift/machine-config-operator
Length of output: 8699
Avoid the protobuf pseudo-version google.golang.org/protobuf is pinned to unreleased v1.36.12-0.20260120151049-f2248ac996af with no matching tagged v1.36.12 release. Switch to a tagged version, or document a temporary exception if this commit is required.
🤖 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 `@go.mod` at line 425, Update the google.golang.org/protobuf dependency in
go.mod from the unreleased pseudo-version to an available tagged release; only
retain the pseudo-version if this specific commit is required, and document that
temporary exception.
Source: Path instructions
Comment on lines
+663
to
+675
| name: "baremetal with VIPManagement BGP but no ingress VIPs", | ||
| infra: &configv1.Infrastructure{ | ||
| Status: configv1.InfrastructureStatus{ | ||
| PlatformStatus: &configv1.PlatformStatus{ | ||
| Type: configv1.BareMetalPlatformType, | ||
| BareMetal: &configv1.BareMetalPlatformStatus{ | ||
| VIPManagement: "BGP", | ||
| APIServerInternalIPs: []string{"192.168.111.5"}, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| result: "false", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover the missing API-VIP branch. The suite tests absent ingress VIPs but not ingress-present/API-absent. Add that case so a regression removing the APIServerInternalIPs guard cannot enable BGP rendering without an API VIP.
Proposed test case
+ {
+ name: "baremetal with VIPManagement BGP but no API VIPs",
+ infra: &configv1.Infrastructure{
+ Status: configv1.InfrastructureStatus{
+ PlatformStatus: &configv1.PlatformStatus{
+ Type: configv1.BareMetalPlatformType,
+ BareMetal: &configv1.BareMetalPlatformStatus{
+ VIPManagement: "BGP",
+ IngressIPs: []string{"192.168.111.4"},
+ },
+ },
+ },
+ },
+ result: "false",
+ },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| name: "baremetal with VIPManagement BGP but no ingress VIPs", | |
| infra: &configv1.Infrastructure{ | |
| Status: configv1.InfrastructureStatus{ | |
| PlatformStatus: &configv1.PlatformStatus{ | |
| Type: configv1.BareMetalPlatformType, | |
| BareMetal: &configv1.BareMetalPlatformStatus{ | |
| VIPManagement: "BGP", | |
| APIServerInternalIPs: []string{"192.168.111.5"}, | |
| }, | |
| }, | |
| }, | |
| }, | |
| result: "false", | |
| name: "baremetal with VIPManagement BGP but no ingress VIPs", | |
| infra: &configv1.Infrastructure{ | |
| Status: configv1.InfrastructureStatus{ | |
| PlatformStatus: &configv1.PlatformStatus{ | |
| Type: configv1.BareMetalPlatformType, | |
| BareMetal: &configv1.BareMetalPlatformStatus{ | |
| VIPManagement: "BGP", | |
| APIServerInternalIPs: []string{"192.168.111.5"}, | |
| }, | |
| }, | |
| }, | |
| }, | |
| result: "false", | |
| }, | |
| { | |
| name: "baremetal with VIPManagement BGP but no API VIPs", | |
| infra: &configv1.Infrastructure{ | |
| Status: configv1.InfrastructureStatus{ | |
| PlatformStatus: &configv1.PlatformStatus{ | |
| Type: configv1.BareMetalPlatformType, | |
| BareMetal: &configv1.BareMetalPlatformStatus{ | |
| VIPManagement: "BGP", | |
| IngressIPs: []string{"192.168.111.4"}, | |
| }, | |
| }, | |
| }, | |
| }, | |
| result: "false", | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/controller/template/render_test.go` around lines 663 - 675, The render
test suite is missing coverage for ingress VIPs being present while the API VIP
is absent. Add a case alongside the existing baremetal VIPManagement BGP cases
in the relevant test table, configuring ingress VIPs without
APIServerInternalIPs and asserting the result remains "false", thereby
preserving the APIServerInternalIPs guard.
Comment on lines
+1
to
+17
| mode: 0644 | ||
| path: "/etc/kubernetes/static-pod-resources/frr-k8s/frr.conf.tmpl" | ||
| contents: | ||
| inline: | | ||
| frr version 9.1 | ||
| frr defaults traditional | ||
| hostname {{`{{.Hostname}}`}} | ||
| ! | ||
| router bgp {{`{{.LocalASN}}`}} | ||
| bgp router-id {{`{{.RouterID}}`}} | ||
| no bgp ebgp-requires-policy | ||
| no bgp default ipv4-unicast | ||
| ! | ||
| {{`{{- range .Peers}}`}} | ||
| neighbor {{`{{.PeerAddress}}`}} remote-as {{`{{.PeerASN}}`}} | ||
| {{`{{- if .Password}}`}} | ||
| neighbor {{`{{.PeerAddress}}`}} password {{`{{.Password}}`}} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Restrict the rendered FRR configuration.
This file renders peer passwords on Line 17 but is written as 0644; use 0600 so local non-root users cannot read BGP credentials.
Proposed fix
-mode: 0644
+mode: 0600Based on learnings, sensitive rendered content should use restrictive mode 0600.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| mode: 0644 | |
| path: "/etc/kubernetes/static-pod-resources/frr-k8s/frr.conf.tmpl" | |
| contents: | |
| inline: | | |
| frr version 9.1 | |
| frr defaults traditional | |
| hostname {{`{{.Hostname}}`}} | |
| ! | |
| router bgp {{`{{.LocalASN}}`}} | |
| bgp router-id {{`{{.RouterID}}`}} | |
| no bgp ebgp-requires-policy | |
| no bgp default ipv4-unicast | |
| ! | |
| {{`{{- range .Peers}}`}} | |
| neighbor {{`{{.PeerAddress}}`}} remote-as {{`{{.PeerASN}}`}} | |
| {{`{{- if .Password}}`}} | |
| neighbor {{`{{.PeerAddress}}`}} password {{`{{.Password}}`}} | |
| mode: 0600 | |
| path: "/etc/kubernetes/static-pod-resources/frr-k8s/frr.conf.tmpl" | |
| contents: | |
| inline: | | |
| frr version 9.1 | |
| frr defaults traditional | |
| hostname {{`{{.Hostname}}`}} | |
| ! | |
| router bgp {{`{{.LocalASN}}`}} | |
| bgp router-id {{`{{.RouterID}}`}} | |
| no bgp ebgp-requires-policy | |
| no bgp default ipv4-unicast | |
| ! | |
| {{`{{- range .Peers}}`}} | |
| neighbor {{`{{.PeerAddress}}`}} remote-as {{`{{.PeerASN}}`}} | |
| {{`{{- if .Password}}`}} | |
| neighbor {{`{{.PeerAddress}}`}} password {{`{{.Password}}`}} |
🤖 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 `@templates/master/00-master/on-prem/files/frr-k8s-conf.yaml` around lines 1 -
17, Change the mode for the rendered FRR configuration in the manifest entry
containing frr.conf.tmpl from 0644 to 0600, keeping the existing path and
contents unchanged so BGP peer passwords remain readable only by the owning
user.
Source: Learnings
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
2 participants
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
MCO side of BGP-based VIP management for on-premise platforms
(enhancement #1982, OPNET-773). When the BareMetal
Infrastructure CR requests
vipManagement: BGP(behind theBGPBasedVIPManagementfeature gate,DevPreviewNoUpgrade), render frr-k8s + kube-vip static pods on control plane nodes instead of the
keepalived manifests. kube-vip runs in routing-table mode: it installs the VIP route into kernel
table 198 only while the local health check passes, and FRR advertises that table — per-node
health-gated ECMP from the ToR.
Four commits, each builds on its own:
(OPNET-780: Add BGPBasedVIPManagement feature gate and BGP VIP management fields api#2923); becomes a one-commit swap to the merged api on rebase.
kube-vip API/ingress static pods (keepalived parity), hostPath sockets prepared by a boot unit
(0777 for the mixed-uid FRR daemons,
container_file_tfor the MCS-confined metrics companionthat CNO deploys against them); requires non-empty VIPs (user-managed LB is rejected in BGP mode).
bgp-vip-configConfigMap (--bgp-vip-config-file) intoControllerConfigSpec.BGPVIPPeersJSON;bootstrap runs an FRR-only static pod so the API VIP is advertised while the control plane forms;
day-2 the operator re-reads the ConfigMap each sync (absence degrades instead of blanking peers
fleet-wide); missing frr-k8s/kube-vip images fail bootstrap render early.
on an image-references entry whose tag does not exist, so this must land only after the kube-vip
payload tag is onboarded (ART-21663, OPNET-779: kube-vip: build the image and promote to ocp/5.0 release#81957).
Dependencies / ordering
anti-affinity, static pod RBAC, metrics companion)
Privileged settings justification
The new static pod manifests intentionally use the following privileged
settings (also documented as comments in the manifests themselves):
hostNetwork: true(frr-k8s and kube-vip pods): required by function.The BGP session must originate from the node IP so the ToR peers with the
node itself, and the health-gated VIP routes are installed into the host
routing table (kernel table 198) that FRR redistributes. This is exact
parity with both the keepalived/haproxy static pods being replaced and the
frr-k8s DaemonSet shipped by CNO.
SYS_ADMINon thefrrcontainer only: required by FRR'sdocker-start/watchfrrprocess supervision. Same capability set as thefrrcontainer of the CNO-shipped frr-k8s DaemonSet (upstreammetallb/frr-k8s parity). Note this container deliberately does NOT use
drop: ALL: it was tested live and FRR's privilege-separated startup(watchfrr as root spawning daemons as the frr user) fails without
root-implicit SETUID/SETGID/DAC_OVERRIDE/CHOWN.
re-adds (kube-vip: NET_ADMIN/NET_RAW for netlink route management;
reloader/frr-status: DAC_OVERRIDE for root vtysh against frr-user-owned
vty sockets), sets
allowPrivilegeEscalation: falseandreadOnlyRootFilesystem: true, and has CPU/memory limits.Testing
Validated end-to-end on dev-scripts baremetal IPI clusters (3 masters + 2 workers, dual VIP), most
recently on a 5.0.0-0.nightly-2026-07-20-081439 base: API VIP ECMP from all masters, ingress VIP
ECMP from router-bearing workers, health-gated withdrawal on node failure, console served over the
BGP-routed path, full frr metrics coverage in Prometheus (static pods via the CNO metrics companion).
Bootstrap-to-CRD handover exercised on every install.
This PR description was prepared with AI assistance. Please verify before acting on it.
Summary by CodeRabbit