STOR-2963: Save SELinuxWarningController upgradeability to a ConfigMap - #2720
Conversation
…ConfigMap To upgrade OCP to Kubernetes 1.37 with SELinuxMount enabled, we need to ensure there are no user workloads that could get broken by the feature gate. SELinuxWarningController in KCP has the information and emits it as a metric. To mark the cluster un-upgradeable easily using API objects, store the information as a ConfigMap too. Reading metrics in an operator is too complicated.
openshift-ci-robot
added
the
backports/unvalidated-commits
|
@rvagner78: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
WalkthroughAdds a feature-gated controller that periodically reports SELinux conflict presence in an OpenShift ConfigMap, integrates it into the existing warning controller, exposes conflict counting, grants required RBAC permissions, and adds unit tests. ChangesSELinux conflict reporting
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant SELinuxWarningController
participant SELinuxConflictsReporterController
participant volumeCache
participant KubernetesAPI
SELinuxWarningController->>SELinuxConflictsReporterController: start Run(ctx)
SELinuxConflictsReporterController->>volumeCache: GetConflictCount()
volumeCache-->>SELinuxConflictsReporterController: conflict count
SELinuxConflictsReporterController->>KubernetesAPI: apply selinux-conflicts ConfigMap
KubernetesAPI-->>SELinuxConflictsReporterController: apply result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
rvagner78
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@rvagner78: This pull request references STOR-2963 which is a valid jira issue. 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/controller/volume/selinuxwarning/openshift_upgrade_controller.go (1)
41-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
wait.UntilWithContextfor periodic execution.Using
wait.UntilWithContextinstead of a manual timer loop is idiomatic in Kubernetes controllers. It ensures that the first execution happens immediately on startup (rather than being delayed by the 30-secondcheckInterval) and automatically provides panic recovery viautilruntime.HandleCrash()inside its backoff loop.Please consider applying the following refactor. Note that this requires adding the
waitpackage to your imports.♻️ Proposed refactor
Add the required import at the top of the file:
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/wait" utilfeature "k8s.io/apiserver/pkg/util/feature"Simplify the
Runloop:func (c *SELinuxConflictsReporterController) Run(ctx context.Context) { logger := klog.FromContext(ctx) if !utilfeature.DefaultFeatureGate.Enabled(features.SELinuxMountGAReadiness) { logger.V(2).Info("SELinuxMountGAReadiness feature gate is disabled, not starting OpenShift SELinux conflicts reporter") return } logger.V(2).Info("Starting OpenShift SELinux conflicts reporter") - timer := time.NewTimer(checkInterval) - defer timer.Stop() - for { - select { - case <-ctx.Done(): - return - case <-timer.C: - c.reportSELinuxConflicts(ctx) - timer.Reset(checkInterval) - } - } + wait.UntilWithContext(ctx, c.reportSELinuxConflicts, checkInterval) }🤖 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/volume/selinuxwarning/openshift_upgrade_controller.go` around lines 41 - 59, Refactor SELinuxConflictsReporterController.Run to use wait.UntilWithContext for periodic execution instead of the manual timer/select loop. Preserve the feature-gate check and startup logging, invoke reportSELinuxConflicts immediately and then at checkInterval while respecting context cancellation, and add the required wait import.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/controller/volume/selinuxwarning/openshift_upgrade_controller_test.go`:
- Around line 152-180: Update the test around reportSELinuxConflicts to assert
fake client write actions, rather than relying only on the final ConfigMap
state, so identical-data patches fail expectNoWrite cases. For transition cases,
invoke c.reportSELinuxConflicts twice and verify exactly one write action
occurred, confirming the first successful write updates previousConflicts and
suppresses the second write.
---
Nitpick comments:
In `@pkg/controller/volume/selinuxwarning/openshift_upgrade_controller.go`:
- Around line 41-59: Refactor SELinuxConflictsReporterController.Run to use
wait.UntilWithContext for periodic execution instead of the manual timer/select
loop. Preserve the feature-gate check and startup logging, invoke
reportSELinuxConflicts immediately and then at checkInterval while respecting
context cancellation, and add the required wait import.
🪄 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: af1c68c4-995c-4a6d-8de7-8e9483fb6967
📒 Files selected for processing (7)
pkg/controller/volume/selinuxwarning/cache/openshift_patch.gopkg/controller/volume/selinuxwarning/openshift_upgrade_controller.gopkg/controller/volume/selinuxwarning/openshift_upgrade_controller_test.gopkg/controller/volume/selinuxwarning/selinux_warning_controller.gopkg/features/openshift_features.goplugin/pkg/auth/authorizer/rbac/bootstrappolicy/controller_policy.goplugin/pkg/auth/authorizer/rbac/bootstrappolicy/testdata/controller-roles.yaml
| c.reportSELinuxConflicts(ctx) | ||
|
|
||
| if tt.expectNoWrite { | ||
| cm, err := fakeClient.CoreV1().ConfigMaps(configMapNamespace).Get(ctx, configMapName, metav1.GetOptions{}) | ||
| if tt.existingConfigMap != nil { | ||
| // The ConfigMap should still exist unchanged. | ||
| if err != nil { | ||
| t.Fatalf("expected ConfigMap to exist, got error: %v", err) | ||
| } | ||
| if cm.Data["conflictsPresent"] != tt.existingConfigMap.Data["conflictsPresent"] { | ||
| t.Errorf("ConfigMap data changed unexpectedly: got %v, want %v", cm.Data, tt.existingConfigMap.Data) | ||
| } | ||
| } else { | ||
| if err == nil || !apierrors.IsNotFound(err) { | ||
| t.Fatalf("expected ConfigMap to not exist, got error: %v", err) | ||
| } | ||
| } | ||
| return | ||
| } | ||
|
|
||
| cm, err := fakeClient.CoreV1().ConfigMaps(configMapNamespace).Get(ctx, configMapName, metav1.GetOptions{}) | ||
| if err != nil { | ||
| t.Fatalf("failed to get ConfigMap: %v", err) | ||
| } | ||
| for key, expectedValue := range tt.expectConfigMapData { | ||
| if cm.Data[key] != expectedValue { | ||
| t.Errorf("ConfigMap data[%q] = %q, want %q", key, cm.Data[key], expectedValue) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert client write actions rather than only the final ConfigMap state.
An unnecessary patch with identical data passes expectNoWrite. Also invoke reporting twice after a transition to verify that the successful first write updates previousConflicts and suppresses the second write.
Proposed assertion pattern
c.reportSELinuxConflicts(ctx)
+ c.reportSELinuxConflicts(ctx)
+
+ writeCount := 0
+ for _, action := range fakeClient.Actions() {
+ switch action.GetVerb() {
+ case "create", "patch", "update":
+ writeCount++
+ }
+ }
if tt.expectNoWrite {
+ if writeCount != 0 {
+ t.Fatalf("expected no ConfigMap writes, got %d", writeCount)
+ }For transition cases, expect exactly one write across both calls.
🤖 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/volume/selinuxwarning/openshift_upgrade_controller_test.go`
around lines 152 - 180, Update the test around reportSELinuxConflicts to assert
fake client write actions, rather than relying only on the final ConfigMap
state, so identical-data patches fail expectNoWrite cases. For transition cases,
invoke c.reportSELinuxConflicts twice and verify exactly one write action
occurred, confirming the first successful write updates previousConflicts and
suppresses the second write.
|
@rvagner78: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
@rvagner78: This pull request references STOR-2963 which is a valid jira issue. 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. |
|
/retest |
|
/test e2e-aws-ovn-techpreview |
|
/test e2e-aws-ovn-techpreview-serial-1of2 |
|
/test e2e-aws-ovn-techpreview-serial-2of2 |
|
/test e2e-aws-ovn-fips |
|
/test e2e-aws-ovn-cgroupsv2 |
|
@rvagner78: This PR was included in a payload test run from openshift/cluster-storage-operator#715
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/078c4380-847c-11f1-9203-9b5af984e1ff-0 |
|
/test e2e-aws-ovn-fips |
|
@rvagner78: This PR was included in a payload test run from openshift/cluster-storage-operator#715
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/bf495ad0-84c7-11f1-8571-fadc318cb7ed-0 |
|
/test e2e-aws-ovn-fips |
|
@rvagner78: This PR was included in a payload test run from openshift/cluster-storage-operator#715
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/3e37dcc0-84ed-11f1-8261-f3069dc9f396-0 |
|
/test e2e-aws-ovn-fips |
|
@rvagner78: This PR was included in a payload test run from openshift/cluster-storage-operator#715
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/73b80740-850f-11f1-9cb4-af5855bf2d14-0 |
|
/test e2e-aws-ovn-fips |
|
@rvagner78: This PR was included in a payload test run from openshift/cluster-storage-operator#715
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/1f27d410-8539-11f1-96f0-2533b3cae18e-0 |
|
/test e2e-aws-ovn-fips |
|
/test e2e-aws-ovn-fips |
1 similar comment
|
/test e2e-aws-ovn-fips |
|
/test hypershift-e2e-aks |
|
/retest |
|
/test e2e-aws-ovn-fips |
|
/lgtm /assign @jubittajohn |
|
This PR pulls in commit from #2671. |
|
/verified by @jsafrane #2671 (comment) |
openshift-ci-robot
added
the
verified
|
@jubittajohn: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/remove-label backports/unvalidated-commits |
openshift-ci
Bot
removed
the
backports/unvalidated-commits
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dobsonj, jubittajohn, rvagner78 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/retest-required |
|
@rvagner78: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
5 participants
KCM's SELinuxWarningController knows how many Pods could get broken by upgrade to Kubernetes 1.37 / a version where
SELinuxMountfeature gate is enabled.Add a carry patch to KCM to store the information into a ConfigMap
openshift-config/selinux-conflicts.cluster-storage-operator can read it from there and mark itself
Upgradeable: false.See openshift/enhancements#2010 for details.
This PR reinstates the carry patch from #2671. The remaining commits from that original PR are already upstreamed and present in Kubernetes v1.36.2.
Summary by CodeRabbit
New Features
selinux-conflictsConfigMap, including updates as conflict presence changes.SELinuxMountGAReadinessfeature gate (Alpha, disabled by default) to control when reporting runs.selinux-conflictsConfigMap when needed.Tests