Conversation
Add WithUpdateOperatorVersion() to ReconcileResult, allowing controllers to optionally update the operator version in the ClusterOperator status when writing their conditions. Also switches controller_status tests from fake client to envtest for accurate SSA field ownership testing.
Convert Reason from string constants to an ordered iota type, enabling severity-based comparison for condition aggregation. Replaces ReasonSyncFailed with the standardised ReasonEphemeralError.
ClusterOperator version is now written by the revision controller instead of the clusteroperator controller.
Rewrite ClusterOperatorController to aggregate per-controller sub-conditions (Available/Progressing) into top-level ClusterOperator conditions.
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (41)
💤 Files with no reviewable changes (2)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (16)
WalkthroughThis PR introduces a new Changescapi-installer workload and status aggregation
🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Contributor
Author
|
/test e2e-aws-ovn-techpreview-upgrade |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/test/conditions.go (1)
327-329:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the
toMatchercomment to reflect current behavior.Line 328 says non-matcher values are wrapped with
gomega.Equal(), but the implementation now usesgomega.BeEquivalentTo().Suggested fix
-// Otherwise, it wraps the value in gomega.Equal(). +// Otherwise, it wraps the value in gomega.BeEquivalentTo().🤖 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/test/conditions.go` around lines 327 - 329, The comment above function toMatcher is out of date: update the docstring to state that non-matcher values are wrapped with gomega.BeEquivalentTo() instead of gomega.Equal(); locate the toMatcher function and replace or edit the sentence that currently mentions gomega.Equal() so it accurately references gomega.BeEquivalentTo(), keeping the rest of the comment intact.
🧹 Nitpick comments (3)
pkg/controllers/installerdeployment/controller.go (1)
164-166: ⚡ Quick winFilter watched Deployments by namespace as well as name.
The current predicate matches any namespace for
capi-installer. Adding a namespace check avoids unnecessary reconciles from unrelated objects.Suggested patch
- For(&appsv1.Deployment{}, builder.WithPredicates(predicate.NewPredicateFuncs(func(obj client.Object) bool { - return obj.GetName() == deploymentName + For(&appsv1.Deployment{}, builder.WithPredicates(predicate.NewPredicateFuncs(func(obj client.Object) bool { + return obj.GetName() == deploymentName && obj.GetNamespace() == r.Namespace }))).🤖 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/controllers/installerdeployment/controller.go` around lines 164 - 166, The predicate used in the controller watch for For(&appsv1.Deployment{}, builder.WithPredicates(predicate.NewPredicateFuncs(...))) only checks obj.GetName() == deploymentName and thus matches across all namespaces; update the predicate to also check obj.GetNamespace() == <desiredNamespaceVariable> (or a literal namespace like "capi-system") so the predicate returns true only when both name and namespace match, referencing the same deploymentName and the controller's target namespace variable when implementing the change.pkg/controllers/installerdeployment/deployment_test.go (2)
112-128: ⚡ Quick winCollapse repetitive
volumeNameForImageRefcases into aDescribeTable.These are table-style cases and are easier to maintain as
DescribeTable+Entry.Refactor pattern example
-var _ = Describe("volumeNameForImageRef", func() { - It("should generate DNS-label-safe volume names", func() { ... }) - It("should be deterministic for the same image ref", func() { ... }) - It("should generate different names for different image refs", func() { ... }) -}) +var _ = DescribeTable("volumeNameForImageRef", + func(assertion func()) { assertion() }, + Entry("should generate DNS-label-safe volume names", func() { + name := volumeNameForImageRef("registry/aws@sha256:abc123") + Expect(name).To(MatchRegexp(`^[a-z0-9]([-a-z0-9]*[a-z0-9])?$`)) + }), + Entry("should be deterministic for the same image ref", func() { + Expect(volumeNameForImageRef("registry/core@sha256:def456")). + To(Equal(volumeNameForImageRef("registry/core@sha256:def456"))) + }), + Entry("should generate different names for different image refs", func() { + Expect(volumeNameForImageRef("registry/aws@sha256:abc")). + NotTo(Equal(volumeNameForImageRef("registry/gcp@sha256:def"))) + }), +)As per coding guidelines, "Use Ginkgo
DescribeTablewithEntryfor table-driven tests instead of manual loops in test files."🤖 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/controllers/installerdeployment/deployment_test.go` around lines 112 - 128, Collapse the three independent It blocks testing volumeNameForImageRef into a single Ginkgo DescribeTable: create a DescribeTable named for volumeNameForImageRef that accepts input imageRef and expected assertions, add Entry rows for the DNS-label regex check, deterministic same-ref check, and different-refs check, and replace the existing It blocks with this table; reference the existing function volumeNameForImageRef and keep the same expectations (MatchRegexp, Equal, NotTo(Equal)) inside the table body so behavior remains unchanged.
33-38: ⚡ Quick winPrefer
HaveField-based assertions over manual nested field checks.These expectations are valid, but this repo’s Ginkgo style asks for
HaveField/chained matchers for struct assertions in tests.Refactor pattern example
-Expect(deployment.Name).To(Equal("capi-installer")) -Expect(deployment.Namespace).To(Equal(testNamespace)) -Expect(deployment.Spec.Template.Spec.Containers[0].Image).To(Equal(testImage)) +Expect(deployment).To(SatisfyAll( + HaveField("Name", Equal("capi-installer")), + HaveField("Namespace", Equal(testNamespace)), + HaveField("Spec.Template.Spec.Containers", HaveLen(1)), + HaveField("Spec.Template.Spec.Containers.0.Image", Equal(testImage)), +))As per coding guidelines, "Use Ginkgo
HaveField,HaveValue,HaveKeymatchers for struct/map assertions instead of manual field checks in test files."Also applies to: 103-109
🤖 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/controllers/installerdeployment/deployment_test.go` around lines 33 - 38, Replace manual nested Expect checks on the deployment test object with Ginkgo's HaveField/chain matchers: assert deployment has Name "capi-installer" and Namespace testNamespace using HaveField, then use chained HaveField calls to navigate Spec -> Template -> Spec and assert the container image equals testImage (e.g., index into Containers then check Image) and that ServiceAccountName equals "capi-installer"; apply the same refactor for the other occurrences around the 103-109 block to follow the repo's Ginkgo style and avoid direct nested field access in assertions.
🤖 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 `@cmd/capi-installer/main.go`:
- Line 75: The flag help text for the provider manifests directory claims dev
mode can "skip pod spec reading", but the main routine still unconditionally
requires POD_NAME/POD_NAMESPACE and fetches the Pod; either update the flag
description to remove the "skip pod spec reading" claim or change the runtime to
honor it. Specifically, either edit the provider manifests flag string (the flag
that describes the provider image manifests directory) to remove the dev-mode
skip wording, or wrap the pod-read logic (the block that reads
POD_NAME/POD_NAMESPACE and fetches the Pod) behind a conditional that checks the
dev-mode/providerManifestsDir setting so that when a local manifests dir is
supplied in dev mode the code does not require POD_NAME/POD_NAMESPACE or call
the Pod-fetching function.
In
`@manifests/0000_30_cluster-api-operator_02_capi-installer-metrics-service.yaml`:
- Around line 3-12: The Service manifest for capi-installer-metrics is missing
labels required by the ServiceMonitor selector; update the Service resource
named capi-installer-metrics by adding metadata.labels with k8s-app:
capi-installer so it matches the ServiceMonitor selector.matchLabels (k8s-app:
capi-installer) and will be discovered/scraped.
In `@manifests/0000_30_cluster-api-operator_06_deployment.yaml`:
- Around line 27-31: The Deployment's container "capi-operator" lacks hardened
securityContext and resource limits; add a securityContext at the pod or
container level for the container named capi-operator with runAsNonRoot: true,
readOnlyRootFilesystem: true, allowPrivilegeEscalation: false and
capabilities.drop: ["ALL"], and add resources with sensible requests and limits
(cpu and memory) to prevent unbounded usage; apply the same
securityContext/resource changes to the other container entries referenced (the
other container blocks at the same manifest sections) so every container
complies with the guidelines.
In `@pkg/controllers/installer/installer_controller.go`:
- Around line 57-58: ResultGenerator is declared inside a const block but
initialized by a function call
operatorstatus.ControllerResultGenerator(controllerName), which is not a
compile-time constant; move its declaration out of the const block and make it a
package-level var (e.g., var ResultGenerator =
operatorstatus.ControllerResultGenerator(controllerName)) or initialize it in an
init() function, ensuring the const block only contains true constants and
referencing ResultGenerator, operatorstatus.ControllerResultGenerator and
controllerName to locate the change.
In `@pkg/controllers/installerdeployment/assets/deployment.yaml`:
- Around line 21-23: Update the Pod spec under
spec.serviceAccountName/containers to harden the pod: add
automountServiceAccountToken: false at the pod spec level, and for each
container add a securityContext with runAsNonRoot: true, readOnlyRootFilesystem:
true, allowPrivilegeEscalation: false and capabilities.drop: ["ALL"], plus a
resources block that includes at least cpu and memory limits (and requests).
Apply these changes for the containers described in the manifest (look for spec
-> containers) and ensure the fields are present for all listed containers.
In `@pkg/controllers/installerdeployment/controller_test.go`:
- Around line 250-254: The test's Eventually block currently returns err != nil
from the cl.Get call which can hide transient errors; update the closure used in
the Eventually assertion (the one calling cl.Get with client.ObjectKey{Name:
deploymentName, Namespace: namespace} and using the deployment variable) to
explicitly check and return apierrors.IsNotFound(err) so the assertion only
succeeds once the deployment is actually deleted; import
k8s.io/apimachinery/pkg/api/errors as apierrors if needed and ensure the closure
returns false for nil err or non-NotFound errors so transient client errors
don't make the test pass.
In `@pkg/controllers/installerdeployment/deployment.go`:
- Around line 137-145: The generated volumeName (built with fmt.Sprintf("%s-%s",
imageName, shortHash)) can exceed Kubernetes DNS-1123 label length and become
invalid; update the code that builds volumeName to enforce DNS-label rules:
truncate the combined name to 63 characters, ensure it starts with an
alphanumeric (keep the existing check that prefixes "img-" if needed), and after
truncation ensure the last character is alphanumeric (trim trailing '-'
characters or replace with a hex char from shortHash if necessary) so the final
value always conforms to Kubernetes DNS-label constraints before returning
volumeName.
In `@pkg/controllers/revision/revision_controller.go`:
- Around line 55-56: ResultGenerator is being initialized with
operatorstatus.ControllerResultGenerator(controllerName), which is not a
compile-time constant; remove ResultGenerator from the const block and declare
it as a package-level variable instead (e.g., var ResultGenerator =
operatorstatus.ControllerResultGenerator(controllerName)), keeping the existing
comment and using the same identifiers (ResultGenerator, controllerName,
operatorstatus.ControllerResultGenerator) so the RevisionController code
references remain unchanged.
In `@pkg/operatorstatus/controller_status.go`:
- Around line 94-114: Update ReasonFromString to explicitly map legacy/removed
persisted reason strings to the correct current enums before falling back to
ReasonUnknown: add case branches in the switch for known legacy values (e.g.,
"SyncFailed" -> ReasonNonRetryableError, "Stalled" or "Syncing" ->
ReasonProgressing, "WaitingForResources" -> ReasonWaitingOnExternal, and any
other removed names your migration expects) so persisted statuses aren’t
down-ranked to ReasonUnknown; keep these new case entries in ReasonFromString
above the default return.
---
Outside diff comments:
In `@pkg/test/conditions.go`:
- Around line 327-329: The comment above function toMatcher is out of date:
update the docstring to state that non-matcher values are wrapped with
gomega.BeEquivalentTo() instead of gomega.Equal(); locate the toMatcher function
and replace or edit the sentence that currently mentions gomega.Equal() so it
accurately references gomega.BeEquivalentTo(), keeping the rest of the comment
intact.
---
Nitpick comments:
In `@pkg/controllers/installerdeployment/controller.go`:
- Around line 164-166: The predicate used in the controller watch for
For(&appsv1.Deployment{},
builder.WithPredicates(predicate.NewPredicateFuncs(...))) only checks
obj.GetName() == deploymentName and thus matches across all namespaces; update
the predicate to also check obj.GetNamespace() == <desiredNamespaceVariable> (or
a literal namespace like "capi-system") so the predicate returns true only when
both name and namespace match, referencing the same deploymentName and the
controller's target namespace variable when implementing the change.
In `@pkg/controllers/installerdeployment/deployment_test.go`:
- Around line 112-128: Collapse the three independent It blocks testing
volumeNameForImageRef into a single Ginkgo DescribeTable: create a DescribeTable
named for volumeNameForImageRef that accepts input imageRef and expected
assertions, add Entry rows for the DNS-label regex check, deterministic same-ref
check, and different-refs check, and replace the existing It blocks with this
table; reference the existing function volumeNameForImageRef and keep the same
expectations (MatchRegexp, Equal, NotTo(Equal)) inside the table body so
behavior remains unchanged.
- Around line 33-38: Replace manual nested Expect checks on the deployment test
object with Ginkgo's HaveField/chain matchers: assert deployment has Name
"capi-installer" and Namespace testNamespace using HaveField, then use chained
HaveField calls to navigate Spec -> Template -> Spec and assert the container
image equals testImage (e.g., index into Containers then check Image) and that
ServiceAccountName equals "capi-installer"; apply the same refactor for the
other occurrences around the 103-109 block to follow the repo's Ginkgo style and
avoid direct nested field access in assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 30df5c39-d01d-4875-9c1b-27066181d6ac
⛔ Files ignored due to path filters (2)
vendor/golang.org/x/tools/cmd/stringer/stringer.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (46)
Dockerfile.rhelMakefilecmd/capi-installer/main.gocmd/capi-operator/main.gogo.modmanifests/0000_30_cluster-api-installer_05_deployment.yamlmanifests/0000_30_cluster-api-operator_00_namespace.yamlmanifests/0000_30_cluster-api-operator_00_tombstones.yamlmanifests/0000_30_cluster-api-operator_01_metrics-service.yamlmanifests/0000_30_cluster-api-operator_01_serviceaccount.yamlmanifests/0000_30_cluster-api-operator_02_capi-installer-metrics-service.yamlmanifests/0000_30_cluster-api-operator_02_capi-installer-serviceaccount.yamlmanifests/0000_30_cluster-api-operator_02_capi-installer-servicemonitor.yamlmanifests/0000_30_cluster-api-operator_03_clusterrole.yamlmanifests/0000_30_cluster-api-operator_04_capi-installer-clusterrolebinding.yamlmanifests/0000_30_cluster-api-operator_04_clusterrolebinding.yamlmanifests/0000_30_cluster-api-operator_05_provider-images-configmap.yamlmanifests/0000_30_cluster-api-operator_06_deployment.yamlmanifests/0000_30_cluster-api-operator_07_clusterapi.yamlmanifests/0000_30_cluster-api_14_allow-ingress-to-metrics-operators.yamlmanifests/0000_30_cluster-api_16_allow-egress-operators.yamlpkg/controllers/clusteroperator/clusteroperator_controller.gopkg/controllers/clusteroperator/clusteroperator_controller_test.gopkg/controllers/clusteroperator/suite_test.gopkg/controllers/common_consts.gopkg/controllers/installer/installer_controller.gopkg/controllers/installerdeployment/assets/deployment.yamlpkg/controllers/installerdeployment/controller.gopkg/controllers/installerdeployment/controller_test.gopkg/controllers/installerdeployment/deployment.gopkg/controllers/installerdeployment/deployment_test.gopkg/controllers/installerdeployment/suite_test.gopkg/controllers/revision/revision_controller.gopkg/controllers/revision/revision_controller_test.gopkg/controllers/secretsync/secret_sync_controller.gopkg/operatorstatus/controller_status.gopkg/operatorstatus/controller_status_test.gopkg/operatorstatus/operator_status.gopkg/operatorstatus/reason_string.gopkg/operatorstatus/watch_predicates.gopkg/providerimages/configmap.gopkg/providerimages/configmap_test.gopkg/providerimages/providerimages_test.gopkg/providerimages/revision_images.gopkg/providerimages/revision_images_test.gopkg/test/conditions.go
💤 Files with no reviewable changes (2)
- manifests/0000_30_cluster-api-installer_05_deployment.yaml
- pkg/controllers/common_consts.go
| providerImageDir := extraflags.String( | ||
| "provider-image-dir", | ||
| defaultProviderImageDirPath, | ||
| "Directory containing provider image manifests. In dev mode, set to a local directory to skip pod spec reading.", |
There was a problem hiding this comment.
Flag description is inconsistent with runtime behavior.
Line 75 says dev mode can skip pod-spec reading, but Lines 139-149 always require POD_NAME/POD_NAMESPACE and fetch the Pod.
Proposed fix (description-only)
"provider-image-dir",
defaultProviderImageDirPath,
- "Directory containing provider image manifests. In dev mode, set to a local directory to skip pod spec reading.",
+ "Directory containing provider image manifests. The installer still reads image refs from the running pod via POD_NAME/POD_NAMESPACE.",
)Also applies to: 139-149
🤖 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 `@cmd/capi-installer/main.go` at line 75, The flag help text for the provider
manifests directory claims dev mode can "skip pod spec reading", but the main
routine still unconditionally requires POD_NAME/POD_NAMESPACE and fetches the
Pod; either update the flag description to remove the "skip pod spec reading"
claim or change the runtime to honor it. Specifically, either edit the provider
manifests flag string (the flag that describes the provider image manifests
directory) to remove the dev-mode skip wording, or wrap the pod-read logic (the
block that reads POD_NAME/POD_NAMESPACE and fetches the Pod) behind a
conditional that checks the dev-mode/providerManifestsDir setting so that when a
local manifests dir is supplied in dev mode the code does not require
POD_NAME/POD_NAMESPACE or call the Pod-fetching function.
Comment on lines
+3
to
+12
| metadata: | ||
| annotations: | ||
| exclude.release.openshift.io/internal-openshift-hosted: "true" | ||
| include.release.openshift.io/self-managed-high-availability: "true" | ||
| include.release.openshift.io/single-node-developer: "true" | ||
| release.openshift.io/feature-gate: "ClusterAPIMachineManagement" | ||
| service.beta.openshift.io/serving-cert-secret-name: capi-installer-metrics-tls | ||
| name: capi-installer-metrics | ||
| namespace: openshift-cluster-api-operator | ||
| spec: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify selector/label contract between ServiceMonitor and Service.
rg -n -C2 'kind: Service$|name: capi-installer-metrics|labels:|k8s-app: capi-installer|kind: ServiceMonitor|matchLabels' manifests/0000_30_cluster-api-operator_02_capi-installer-metrics-service.yaml manifests/0000_30_cluster-api-operator_02_capi-installer-servicemonitor.yamlRepository: openshift/cluster-capi-operator
Length of output: 2603
Add metadata.labels.k8s-app: capi-installer to the installer metrics Service
The ServiceMonitor selects Services using selector.matchLabels: k8s-app: capi-installer, but capi-installer-metrics has no metadata.labels, so it won’t be discovered/scraped.
Suggested fix
apiVersion: v1
kind: Service
metadata:
+ labels:
+ k8s-app: capi-installer
annotations:
exclude.release.openshift.io/internal-openshift-hosted: "true"
include.release.openshift.io/self-managed-high-availability: "true"
include.release.openshift.io/single-node-developer: "true"
release.openshift.io/feature-gate: "ClusterAPIMachineManagement"
service.beta.openshift.io/serving-cert-secret-name: capi-installer-metrics-tls
name: capi-installer-metrics
namespace: openshift-cluster-api-operator
spec:📝 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
| metadata: | |
| annotations: | |
| exclude.release.openshift.io/internal-openshift-hosted: "true" | |
| include.release.openshift.io/self-managed-high-availability: "true" | |
| include.release.openshift.io/single-node-developer: "true" | |
| release.openshift.io/feature-gate: "ClusterAPIMachineManagement" | |
| service.beta.openshift.io/serving-cert-secret-name: capi-installer-metrics-tls | |
| name: capi-installer-metrics | |
| namespace: openshift-cluster-api-operator | |
| spec: | |
| metadata: | |
| labels: | |
| k8s-app: capi-installer | |
| annotations: | |
| exclude.release.openshift.io/internal-openshift-hosted: "true" | |
| include.release.openshift.io/self-managed-high-availability: "true" | |
| include.release.openshift.io/single-node-developer: "true" | |
| release.openshift.io/feature-gate: "ClusterAPIMachineManagement" | |
| service.beta.openshift.io/serving-cert-secret-name: capi-installer-metrics-tls | |
| name: capi-installer-metrics | |
| namespace: openshift-cluster-api-operator | |
| spec: |
🤖 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/0000_30_cluster-api-operator_02_capi-installer-metrics-service.yaml`
around lines 3 - 12, The Service manifest for capi-installer-metrics is missing
labels required by the ServiceMonitor selector; update the Service resource
named capi-installer-metrics by adding metadata.labels with k8s-app:
capi-installer so it matches the ServiceMonitor selector.matchLabels (k8s-app:
capi-installer) and will be discovered/scraped.
Comment on lines
+27
to
+31
| spec: | ||
| serviceAccountName: capi-operator | ||
| containers: | ||
| - name: capi-operator | ||
| image: registry.ci.openshift.org/openshift:cluster-capi-operator |
There was a problem hiding this comment.
Harden the pod/container security posture and set container limits.
This Deployment omits required securityContext hardening and memory/cpu limits, leaving default root-capable behavior and unbounded resource usage.
Suggested fix
spec:
template:
spec:
+ securityContext:
+ runAsNonRoot: true
+ seccompProfile:
+ type: RuntimeDefault
serviceAccountName: capi-operator
containers:
- name: capi-operator
image: registry.ci.openshift.org/openshift:cluster-capi-operator
+ securityContext:
+ allowPrivilegeEscalation: false
+ readOnlyRootFilesystem: true
+ capabilities:
+ drop:
+ - ALL
command:
- /capi-operator
@@
resources:
requests:
cpu: 10m
memory: 50Mi
+ limits:
+ cpu: 10m
+ memory: 50MiAs per coding guidelines **/*.{yaml,yml}: require runAsNonRoot, readOnlyRootFilesystem, allowPrivilegeEscalation: false, dropped capabilities, and resource limits on every container.
Also applies to: 54-58, 63-74, 85-89
🧰 Tools
🪛 Trivy (0.69.3)
[error] 27-89: Default security context configured
deployment capi-operator in openshift-cluster-api-operator namespace is using the default security context, which allows root privileges
Rule: KSV-0118
(IaC/Kubernetes)
🤖 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/0000_30_cluster-api-operator_06_deployment.yaml` around lines 27 -
31, The Deployment's container "capi-operator" lacks hardened securityContext
and resource limits; add a securityContext at the pod or container level for the
container named capi-operator with runAsNonRoot: true, readOnlyRootFilesystem:
true, allowPrivilegeEscalation: false and capabilities.drop: ["ALL"], and add
resources with sensible requests and limits (cpu and memory) to prevent
unbounded usage; apply the same securityContext/resource changes to the other
container entries referenced (the other container blocks at the same manifest
sections) so every container complies with the guidelines.
Comment on lines
+57
to
+58
| // ResultGenerator is the controller result generator for the InstallerController. | ||
| ResultGenerator = operatorstatus.ControllerResultGenerator(controllerName) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 'const \\(|ResultGenerator\\s*=\\s*operatorstatus\\.ControllerResultGenerator\\(' pkg/controllers/installer/installer_controller.goRepository: openshift/cluster-capi-operator
Length of output: 284
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="pkg/controllers/installer/installer_controller.go"
echo "=== Context around ResultGenerator declaration ==="
nl -ba "$FILE" | sed -n '40,90p'
echo "=== Lines containing ResultGenerator ==="
rg -n "ResultGenerator" "$FILE"
echo "=== go.mod Go version ==="
cat go.mod | rg -n '^go ' || trueRepository: openshift/cluster-capi-operator
Length of output: 187
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="pkg/controllers/installer/installer_controller.go"
echo "=== Context around ResultGenerator declaration (lines 40-90) ==="
awk 'NR>=40 && NR<=90 {printf "%5d\t%s\n", NR, $0}' "$FILE" | sed -n '1,200p'
echo "=== Lines containing ResultGenerator ==="
rg -n "ResultGenerator" "$FILE"
echo "=== go.mod Go version ==="
rg -n '^go ' go.mod || trueRepository: openshift/cluster-capi-operator
Length of output: 3523
Fix invalid const initializer for ResultGenerator
pkg/controllers/installer/installer_controller.go declares ResultGenerator inside a const (...) block, but it’s initialized via a function call (operatorstatus.ControllerResultGenerator(controllerName)), which is not a constant expression and won’t compile.
Proposed fix
const (
controllerName = "InstallerController"
clusterAPIName = "cluster"
-
- // ResultGenerator is the controller result generator for the InstallerController.
- ResultGenerator = operatorstatus.ControllerResultGenerator(controllerName)
)
+
+var (
+ // ResultGenerator is the controller result generator for the InstallerController.
+ ResultGenerator = operatorstatus.ControllerResultGenerator(controllerName)
+)📝 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
| // ResultGenerator is the controller result generator for the InstallerController. | |
| ResultGenerator = operatorstatus.ControllerResultGenerator(controllerName) | |
| const ( | |
| controllerName = "InstallerController" | |
| clusterAPIName = "cluster" | |
| ) | |
| var ( | |
| // ResultGenerator is the controller result generator for the InstallerController. | |
| ResultGenerator = operatorstatus.ControllerResultGenerator(controllerName) | |
| ) |
🤖 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/controllers/installer/installer_controller.go` around lines 57 - 58,
ResultGenerator is declared inside a const block but initialized by a function
call operatorstatus.ControllerResultGenerator(controllerName), which is not a
compile-time constant; move its declaration out of the const block and make it a
package-level var (e.g., var ResultGenerator =
operatorstatus.ControllerResultGenerator(controllerName)) or initialize it in an
init() function, ensuring the const block only contains true constants and
referencing ResultGenerator, operatorstatus.ControllerResultGenerator and
controllerName to locate the change.
Comment on lines
+21
to
+23
| spec: | ||
| serviceAccountName: capi-installer | ||
| containers: |
There was a problem hiding this comment.
Harden pod/container security settings and add limits.
This manifest currently misses required hardening fields (securityContext, allowPrivilegeEscalation: false, readOnlyRootFilesystem, dropping capabilities), resource limits, and explicit automountServiceAccountToken: false.
Suggested patch
spec:
@@
template:
@@
spec:
+ automountServiceAccountToken: false
serviceAccountName: capi-installer
containers:
- name: capi-installer
+ securityContext:
+ runAsNonRoot: true
+ readOnlyRootFilesystem: true
+ allowPrivilegeEscalation: false
+ capabilities:
+ drop:
+ - ALL
@@
resources:
requests:
cpu: 10m
memory: 50Mi
+ limits:
+ cpu: 100m
+ memory: 200Mi
@@
+ securityContext:
+ runAsNonRoot: trueAs per coding guidelines, Kubernetes manifests must set runAsNonRoot, readOnlyRootFilesystem, allowPrivilegeEscalation: false, drop ALL capabilities, define container resource limits, and set automountServiceAccountToken: false unless needed.
Also applies to: 24-53, 45-48
🧰 Tools
🪛 Trivy (0.69.3)
[error] 21-80: Default security context configured
deployment capi-installer in default namespace is using the default security context, which allows root privileges
Rule: KSV-0118
(IaC/Kubernetes)
🤖 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/controllers/installerdeployment/assets/deployment.yaml` around lines 21 -
23, Update the Pod spec under spec.serviceAccountName/containers to harden the
pod: add automountServiceAccountToken: false at the pod spec level, and for each
container add a securityContext with runAsNonRoot: true, readOnlyRootFilesystem:
true, allowPrivilegeEscalation: false and capabilities.drop: ["ALL"], plus a
resources block that includes at least cpu and memory limits (and requests).
Apply these changes for the containers described in the manifest (look for spec
-> containers) and ensure the fields are present for all listed containers.
Comment on lines
+250
to
+254
| Eventually(func() bool { | ||
| err := cl.Get(ctx, client.ObjectKey{Name: deploymentName, Namespace: namespace}, deployment) | ||
|
|
||
| return err != nil | ||
| }).WithTimeout(testTimeout).WithPolling(testInterval).Should(BeTrue()) |
There was a problem hiding this comment.
Assert NotFound explicitly after deletion.
return err != nil can pass on transient client errors and mask regressions. Check apierrors.IsNotFound(err) instead.
Suggested patch
+import apierrors "k8s.io/apimachinery/pkg/api/errors"
@@
- Eventually(func() bool {
+ Eventually(func() bool {
err := cl.Get(ctx, client.ObjectKey{Name: deploymentName, Namespace: namespace}, deployment)
-
- return err != nil
+ return apierrors.IsNotFound(err)
}).WithTimeout(testTimeout).WithPolling(testInterval).Should(BeTrue())🤖 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/controllers/installerdeployment/controller_test.go` around lines 250 -
254, The test's Eventually block currently returns err != nil from the cl.Get
call which can hide transient errors; update the closure used in the Eventually
assertion (the one calling cl.Get with client.ObjectKey{Name: deploymentName,
Namespace: namespace} and using the deployment variable) to explicitly check and
return apierrors.IsNotFound(err) so the assertion only succeeds once the
deployment is actually deleted; import k8s.io/apimachinery/pkg/api/errors as
apierrors if needed and ensure the closure returns false for nil err or
non-NotFound errors so transient client errors don't make the test pass.
Comment on lines
+137
to
+145
| // Combine to create volume name: <imagename>-<shorthash> | ||
| volumeName := fmt.Sprintf("%s-%s", imageName, shortHash) | ||
|
|
||
| // Ensure it starts with an alphanumeric character | ||
| if len(volumeName) > 0 && (volumeName[0] < 'a' || volumeName[0] > 'z') && (volumeName[0] < '0' || volumeName[0] > '9') { | ||
| volumeName = "img-" + volumeName | ||
| } | ||
|
|
||
| return volumeName |
There was a problem hiding this comment.
Bound generated volume names to Kubernetes DNS-label length.
volumeName is not length-limited; long image refs can produce invalid volume names and fail Deployment apply.
Suggested patch
// Combine to create volume name: <imagename>-<shorthash>
volumeName := fmt.Sprintf("%s-%s", imageName, shortHash)
// Ensure it starts with an alphanumeric character
if len(volumeName) > 0 && (volumeName[0] < 'a' || volumeName[0] > 'z') && (volumeName[0] < '0' || volumeName[0] > '9') {
volumeName = "img-" + volumeName
}
+
+ // Kubernetes volume names must be valid DNS labels (max 63 chars).
+ if len(volumeName) > 63 {
+ // Preserve suffix uniqueness (`-<hash>`).
+ suffix := "-" + shortHash
+ maxPrefix := 63 - len(suffix)
+ if maxPrefix < 1 {
+ volumeName = "img" + suffix
+ } else {
+ volumeName = strings.TrimRight(volumeName[:maxPrefix], "-") + suffix
+ }
+ }
return volumeName
}📝 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
| // Combine to create volume name: <imagename>-<shorthash> | |
| volumeName := fmt.Sprintf("%s-%s", imageName, shortHash) | |
| // Ensure it starts with an alphanumeric character | |
| if len(volumeName) > 0 && (volumeName[0] < 'a' || volumeName[0] > 'z') && (volumeName[0] < '0' || volumeName[0] > '9') { | |
| volumeName = "img-" + volumeName | |
| } | |
| return volumeName | |
| // Combine to create volume name: <imagename>-<shorthash> | |
| volumeName := fmt.Sprintf("%s-%s", imageName, shortHash) | |
| // Ensure it starts with an alphanumeric character | |
| if len(volumeName) > 0 && (volumeName[0] < 'a' || volumeName[0] > 'z') && (volumeName[0] < '0' || volumeName[0] > '9') { | |
| volumeName = "img-" + volumeName | |
| } | |
| // Kubernetes volume names must be valid DNS labels (max 63 chars). | |
| if len(volumeName) > 63 { | |
| // Preserve suffix uniqueness (`-<hash>`). | |
| suffix := "-" + shortHash | |
| maxPrefix := 63 - len(suffix) | |
| if maxPrefix < 1 { | |
| volumeName = "img" + suffix | |
| } else { | |
| volumeName = strings.TrimRight(volumeName[:maxPrefix], "-") + suffix | |
| } | |
| } | |
| return volumeName |
🤖 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/controllers/installerdeployment/deployment.go` around lines 137 - 145,
The generated volumeName (built with fmt.Sprintf("%s-%s", imageName, shortHash))
can exceed Kubernetes DNS-1123 label length and become invalid; update the code
that builds volumeName to enforce DNS-label rules: truncate the combined name to
63 characters, ensure it starts with an alphanumeric (keep the existing check
that prefixes "img-" if needed), and after truncation ensure the last character
is alphanumeric (trim trailing '-' characters or replace with a hex char from
shortHash if necessary) so the final value always conforms to Kubernetes
DNS-label constraints before returning volumeName.
Comment on lines
+55
to
+56
| // ResultGenerator is the controller result generator for the RevisionController. | ||
| ResultGenerator = operatorstatus.ControllerResultGenerator(controllerName) |
There was a problem hiding this comment.
Move ResultGenerator out of the const block
ResultGenerator is initialized via operatorstatus.ControllerResultGenerator(controllerName), which is not a constant expression, so it cannot be declared inside a const block (compile blocker).
💡 Proposed fix
const (
controllerName = "RevisionController"
clusterAPIName = "cluster"
infrastructureName = "cluster"
maxRevisionsAllowed = 16
-
- // ResultGenerator is the controller result generator for the RevisionController.
- ResultGenerator = operatorstatus.ControllerResultGenerator(controllerName)
)
+
+// ResultGenerator is the controller result generator for the RevisionController.
+var ResultGenerator = operatorstatus.ControllerResultGenerator(controllerName)📝 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
| // ResultGenerator is the controller result generator for the RevisionController. | |
| ResultGenerator = operatorstatus.ControllerResultGenerator(controllerName) | |
| const ( | |
| controllerName = "RevisionController" | |
| clusterAPIName = "cluster" | |
| infrastructureName = "cluster" | |
| maxRevisionsAllowed = 16 | |
| ) | |
| // ResultGenerator is the controller result generator for the RevisionController. | |
| var ResultGenerator = operatorstatus.ControllerResultGenerator(controllerName) |
🤖 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/controllers/revision/revision_controller.go` around lines 55 - 56,
ResultGenerator is being initialized with
operatorstatus.ControllerResultGenerator(controllerName), which is not a
compile-time constant; remove ResultGenerator from the const block and declare
it as a package-level variable instead (e.g., var ResultGenerator =
operatorstatus.ControllerResultGenerator(controllerName)), keeping the existing
comment and using the same identifiers (ResultGenerator, controllerName,
operatorstatus.ControllerResultGenerator) so the RevisionController code
references remain unchanged.
Comment on lines
+94
to
+114
| // ReasonFromString returns a Reason enum value from a string. It returns | ||
| // ReasonUnknown if the string is not a valid Reason. | ||
| func ReasonFromString(reason string) Reason { | ||
| switch reason { | ||
| case ReasonUnknown.String(): | ||
| return ReasonUnknown | ||
| case ReasonAsExpected.String(): | ||
| return ReasonAsExpected | ||
| case ReasonUninitialized.String(): | ||
| return ReasonUninitialized | ||
| case ReasonProgressing.String(): | ||
| return ReasonProgressing | ||
| case ReasonWaitingOnExternal.String(): | ||
| return ReasonWaitingOnExternal | ||
| case ReasonEphemeralError.String(): | ||
| return ReasonEphemeralError | ||
| case ReasonNonRetryableError.String(): | ||
| return ReasonNonRetryableError | ||
| default: | ||
| return ReasonUnknown | ||
| } |
There was a problem hiding this comment.
Preserve legacy reason strings during upgrade.
Line 94 now maps any unrecognized persisted reason to ReasonUnknown, which is the least severe enum value. Existing clusters can still carry pre-upgrade subcondition reasons like SyncFailed; until those controllers rewrite status, the rollup will down-rank them and can briefly report a healthier top-level ClusterOperator status than the cluster actually has. Add explicit aliases for removed reason names before falling back to ReasonUnknown.
💡 Minimal compatibility fix
func ReasonFromString(reason string) Reason {
switch reason {
+ // Backward compatibility with statuses written before the enum rename.
+ case "SyncFailed":
+ return ReasonEphemeralError
case ReasonUnknown.String():
return ReasonUnknown
case ReasonAsExpected.String():
return ReasonAsExpected🤖 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/operatorstatus/controller_status.go` around lines 94 - 114, Update
ReasonFromString to explicitly map legacy/removed persisted reason strings to
the correct current enums before falling back to ReasonUnknown: add case
branches in the switch for known legacy values (e.g., "SyncFailed" ->
ReasonNonRetryableError, "Stalled" or "Syncing" -> ReasonProgressing,
"WaitingForResources" -> ReasonWaitingOnExternal, and any other removed names
your migration expects) so persisted statuses aren’t down-ranked to
ReasonUnknown; keep these new case entries in ReasonFromString above the default
return.
mdbooth
force-pushed
the
test-merge-OCPCLOUD-3443-2664
branch
from
June 5, 2026 09:22
823c511 to
c72a77f
Compare
Contributor
Author
|
/test e2e-aws-ovn-techpreview-upgrade e2e-azure-ovn-techpreview-upgrade |
Contributor
Author
|
They both look like flakes /test e2e-aws-ovn-techpreview-upgrade e2e-azure-ovn-techpreview-upgrade |
Contributor
Author
|
All the failures here are in monitoring tests and don't immediately look related. However, the Azure continuous reconciles are legit. I've opened the following to address:
I'll run them again anyway. /test e2e-aws-ovn-techpreview-upgrade e2e-azure-ovn-techpreview-upgrade |
Contributor
|
@mdbooth: 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. |
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
1 participant
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.
Summary by CodeRabbit
Release Notes
New Features
capi-installercomponent deployed alongside the operator to manage Cluster API installations.Infrastructure & Testing