STOR-2963: Save SELinuxWarningController upgradeability to a ConfigMap#2671
STOR-2963: Save SELinuxWarningController upgradeability to a ConfigMap#2671jsafrane wants to merge 3 commits into
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@jsafrane: This pull request references STOR-2962 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. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
@jsafrane: 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 |
|
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:
WalkthroughThis PR adds a feature-gated SELinux conflict reporter that periodically reads cached conflicts, computes a present/absent condition, and writes ChangesSELinux Conflicts Reporter
Sequence Diagram(s)sequenceDiagram
participant Reporter as runOpenShiftSELinuxConflictsReporter
participant Cache as labelCache.ConflictCounter
participant Cond as getConflicts
participant Apply as applySELinuxConflictsConfigMap
participant PatchCM as patchSELinuxConflictsConfigMap
participant CreateCM as createSELinuxConflictsConfigMap
loop every 30s
Reporter->>Cache: GetConflictCount()
Cache-->>Reporter: count
Reporter->>Cond: getConflicts(count)
Cond-->>Reporter: condition status
Reporter->>Apply: applySELinuxConflictsConfigMap(status)
Apply->>PatchCM: JSONPatch /data
alt patch success
PatchCM-->>Apply: success
else patch NotFound
Apply->>CreateCM: create ConfigMap with conflictsPresent
CreateCM-->>Apply: success
end
Apply-->>Reporter: result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jsafrane 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 |
|
@jsafrane: This pull request references STOR-2962 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: 4
🤖 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.go`:
- Around line 23-25: The package-global variable previousUnogradeable should be
moved onto the Controller type to avoid shared state across controller instances
and tests: remove the package-level previousUnogradeable declaration and add a
field (e.g., previousUnogradeable metav1.ConditionStatus) to the Controller
struct, initialize it in the controller constructor/New function, and update all
references in openshift_upgrade_controller.go (including the usages around lines
43-55) to use c.previousUnogradeable instead of the global name; ensure any
concurrent access follows the Controller's existing synchronization model if
needed.
- Around line 57-59: In getUpgradeability, avoid the unchecked assertion
c.labelCache.(cache.ConflictCounter); instead use a safe type assertion (e.g.,
cc, ok := c.labelCache.(cache.ConflictCounter)) and handle the !ok case by
logging via the provided logger and returning a safe failure status (for example
metav1.ConditionFalse) so the controller won't panic; when ok is true continue
using cc.GetConflictCount() as before.
- Around line 68-81: The current JSON patch built with json.Marshal uses an op
"replace" on path "/data" which fails if the ConfigMap exists without a data
field; change the patch to perform a merge-patch or use a JSON Patch "add" op
for the specific key instead of replacing /data. Update the code that builds
patch (the json.Marshal call that creates patch) and the subsequent call to
c.kubeClient.CoreV1().ConfigMaps(configMapNamespace).Patch(...) (currently using
types.JSONPatchType) so it either constructs a strategic/merge JSON object that
upserts the "upgradeable" key under data, or uses a JSON Patch with op "add" and
path "/data/upgradeable"; ensure configMapName and configMapNamespace are left
intact and error handling remains the same.
In `@plugin/pkg/auth/authorizer/rbac/bootstrappolicy/controller_policy.go`:
- Line 567: The rule created by rbacv1helpers.NewRule("create",
"patch").Groups(legacyGroup).Resources("configmaps").RuleOrDie() is too broad;
restrict it to the specific assessment ConfigMap by adding
ResourceNames("selinux-warning-assessment") (i.e., change the chain to include
.ResourceNames("selinux-warning-assessment")) so the generated role matches the
golden least-privilege scope.
🪄 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: 8e9778de-4b6b-4bc0-b002-00c6cdb7e155
📒 Files selected for processing (10)
pkg/controller/volume/selinuxwarning/cache/openshift_patch.gopkg/controller/volume/selinuxwarning/cache/volumecache.gopkg/controller/volume/selinuxwarning/cache/volumecache_test.gopkg/controller/volume/selinuxwarning/metrics.gopkg/controller/volume/selinuxwarning/openshift_upgrade_controller.gopkg/controller/volume/selinuxwarning/selinux_warning_controller.gopkg/controller/volume/selinuxwarning/selinux_warning_controller_test.gopkg/controller/volume/selinuxwarning/translator/selinux_translator.goplugin/pkg/auth/authorizer/rbac/bootstrappolicy/controller_policy.goplugin/pkg/auth/authorizer/rbac/bootstrappolicy/testdata/controller-roles.yaml
| rbacv1helpers.NewRule("get", "list", "watch").Groups(legacyGroup).Resources("persistentvolumeclaims").RuleOrDie(), | ||
| rbacv1helpers.NewRule("get", "list", "watch").Groups(legacyGroup).Resources("pods").RuleOrDie(), | ||
| rbacv1helpers.NewRule("get", "list", "watch").Groups(storageGroup).Resources("csidrivers").RuleOrDie(), | ||
| rbacv1helpers.NewRule("create", "patch").Groups(legacyGroup).Resources("configmaps").RuleOrDie(), |
There was a problem hiding this comment.
Scope ConfigMap permissions to the assessment object (and align with golden).
This rule grants create/patch on all ConfigMaps, while the paired golden role scopes to selinux-warning-assessment. Please align policy generation with the intended least-privilege scope.
🤖 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 `@plugin/pkg/auth/authorizer/rbac/bootstrappolicy/controller_policy.go` at line
567, The rule created by rbacv1helpers.NewRule("create",
"patch").Groups(legacyGroup).Resources("configmaps").RuleOrDie() is too broad;
restrict it to the specific assessment ConfigMap by adding
ResourceNames("selinux-warning-assessment") (i.e., change the chain to include
.ResourceNames("selinux-warning-assessment")) so the generated role matches the
golden least-privilege scope.
|
@jsafrane: 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. |
jsafrane
force-pushed
the
kcm-selinux-configmap
branch
from
cde26e2 to
1432d8b
Compare
|
@jsafrane: 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 |
|
@jsafrane: This pull request references STOR-2962 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. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
plugin/pkg/auth/authorizer/rbac/bootstrappolicy/controller_policy.go (1)
567-567:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope ConfigMap permissions to the single assessment object.
This grants
create/patchon allconfigmaps. Restrict with.ResourceNames("selinux-warning-assessment")to enforce least privilege and align policy output.🤖 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 `@plugin/pkg/auth/authorizer/rbac/bootstrappolicy/controller_policy.go` at line 567, The rule currently grants create/patch on all configmaps via rbacv1helpers.NewRule("create", "patch").Groups(legacyGroup).Resources("configmaps").RuleOrDie(); restrict it to only the single assessment object by adding .ResourceNames("selinux-warning-assessment") to that rule so it becomes rbacv1helpers.NewRule(...).Groups(legacyGroup).Resources("configmaps").ResourceNames("selinux-warning-assessment").RuleOrDie(), thereby limiting permissions to the specific ConfigMap.pkg/controller/volume/selinuxwarning/openshift_upgrade_controller.go (3)
23-25:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid package-global status state for controller instance logic.
previousConflictsas package-global shares state across controller instances/tests. Keep it onControllerinstead.Also applies to: 43-55
🤖 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 23 - 25, The package-global variable previousConflicts (metav1.ConditionStatus) creates shared state; move it into the Controller struct as a field (e.g., Controller.previousConflicts) and remove the global declaration; update all references (previousConflicts -> c.previousConflicts) inside methods such as those changed around lines 43-55 and initialize it via the Controller constructor (NewController) or rely on the zero-value metav1.ConditionUnknown; also update any tests to construct Controller with the desired initial previousConflicts value instead of relying on package-global state.
82-95:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPatch strategy is brittle for ConfigMaps missing
.data.
replaceon/datacan fail when the field is absent. Use merge-patch (or JSON Patchaddon the key) for idempotent updates.🤖 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 82 - 95, The JSON Patch uses a "replace" on "/data" which fails if .data is missing; instead build a merge patch that sets data.conflictsPresent (e.g. marshal map[string]any{"data": map[string]string{"conflictsPresent": string(conflictsPresent)}}) and call Patch with types.MergePatchType, or switch to a JSON Patch "add" op targeting "/data/conflictsPresent" so the update is idempotent; update the code around the json.Marshal call and the Patch invocation that uses c.kubeClient.CoreV1().ConfigMaps(configMapNamespace).Patch to use the merge patch payload and types.MergePatchType (or use the "add" JSON Patch op) referencing configMapName, configMapNamespace and conflictsPresent.
57-59:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnchecked type assertion can panic.
Use a safe assertion before calling
GetConflictCount()to avoid runtime panic whenlabelCacheis swapped with a non-ConflictCounterimplementation.🤖 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 57 - 59, In getConflicts, replace the unchecked type assertion on c.labelCache to cache.ConflictCounter with a safe comma-ok assertion (e.g., cc, ok := c.labelCache.(cache.ConflictCounter)); if ok use cc.GetConflictCount(), otherwise log the mismatch via the provided logger and treat conflictsCount as 0 (returning metav1.ConditionFalse) to avoid a panic; reference getConflicts, c.labelCache, cache.ConflictCounter, and GetConflictCount when making the change.
🤖 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.go`:
- Around line 19-21: The ConfigMap name constant configMapName is set to
"selinux-conflicts" but the RBAC resourceNames scope targets
"selinux-warning-assessment", causing create/patch calls to be denied; update
the constant configMapName to "selinux-warning-assessment" (or alternatively
adjust the RBAC resourceNames to match the current constant) so the value used
in the controller (configMapName and any code that references it) matches the
RBAC-scoped object name.
---
Duplicate comments:
In `@pkg/controller/volume/selinuxwarning/openshift_upgrade_controller.go`:
- Around line 23-25: The package-global variable previousConflicts
(metav1.ConditionStatus) creates shared state; move it into the Controller
struct as a field (e.g., Controller.previousConflicts) and remove the global
declaration; update all references (previousConflicts -> c.previousConflicts)
inside methods such as those changed around lines 43-55 and initialize it via
the Controller constructor (NewController) or rely on the zero-value
metav1.ConditionUnknown; also update any tests to construct Controller with the
desired initial previousConflicts value instead of relying on package-global
state.
- Around line 82-95: The JSON Patch uses a "replace" on "/data" which fails if
.data is missing; instead build a merge patch that sets data.conflictsPresent
(e.g. marshal map[string]any{"data": map[string]string{"conflictsPresent":
string(conflictsPresent)}}) and call Patch with types.MergePatchType, or switch
to a JSON Patch "add" op targeting "/data/conflictsPresent" so the update is
idempotent; update the code around the json.Marshal call and the Patch
invocation that uses c.kubeClient.CoreV1().ConfigMaps(configMapNamespace).Patch
to use the merge patch payload and types.MergePatchType (or use the "add" JSON
Patch op) referencing configMapName, configMapNamespace and conflictsPresent.
- Around line 57-59: In getConflicts, replace the unchecked type assertion on
c.labelCache to cache.ConflictCounter with a safe comma-ok assertion (e.g., cc,
ok := c.labelCache.(cache.ConflictCounter)); if ok use cc.GetConflictCount(),
otherwise log the mismatch via the provided logger and treat conflictsCount as 0
(returning metav1.ConditionFalse) to avoid a panic; reference getConflicts,
c.labelCache, cache.ConflictCounter, and GetConflictCount when making the
change.
In `@plugin/pkg/auth/authorizer/rbac/bootstrappolicy/controller_policy.go`:
- Line 567: The rule currently grants create/patch on all configmaps via
rbacv1helpers.NewRule("create",
"patch").Groups(legacyGroup).Resources("configmaps").RuleOrDie(); restrict it to
only the single assessment object by adding
.ResourceNames("selinux-warning-assessment") to that rule so it becomes
rbacv1helpers.NewRule(...).Groups(legacyGroup).Resources("configmaps").ResourceNames("selinux-warning-assessment").RuleOrDie(),
thereby limiting permissions to the specific ConfigMap.
🪄 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: 3b898633-0b8e-48a5-a9cc-d00e6e0c9b42
📒 Files selected for processing (5)
pkg/controller/volume/selinuxwarning/cache/openshift_patch.gopkg/controller/volume/selinuxwarning/openshift_upgrade_controller.gopkg/controller/volume/selinuxwarning/selinux_warning_controller.goplugin/pkg/auth/authorizer/rbac/bootstrappolicy/controller_policy.goplugin/pkg/auth/authorizer/rbac/bootstrappolicy/testdata/controller-roles.yaml
| configMapNamespace = "openshift-config" | ||
| configMapName = "selinux-conflicts" | ||
| ) |
There was a problem hiding this comment.
ConfigMap name does not match the RBAC-scoped assessment object.
configMapName is selinux-conflicts, but this PR’s RBAC scope targets selinux-warning-assessment. With resourceNames scoping, patch/create will fail for the current name.
Suggested fix
const (
checkInterval = 30 * time.Second
configMapNamespace = "openshift-config"
- configMapName = "selinux-conflicts"
+ configMapName = "selinux-warning-assessment"
)📝 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.
| configMapNamespace = "openshift-config" | |
| configMapName = "selinux-conflicts" | |
| ) | |
| const ( | |
| checkInterval = 30 * time.Second | |
| configMapNamespace = "openshift-config" | |
| configMapName = "selinux-warning-assessment" | |
| ) |
🤖 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 19 - 21, The ConfigMap name constant configMapName is set to
"selinux-conflicts" but the RBAC resourceNames scope targets
"selinux-warning-assessment", causing create/patch calls to be denied; update
the constant configMapName to "selinux-warning-assessment" (or alternatively
adjust the RBAC resourceNames to match the current constant) so the value used
in the controller (configMapName and any code that references it) matches the
RBAC-scoped object name.
1432d8b to
8e71471
Compare
|
@jsafrane: the contents of this pull request could not be automatically validated. The following commits are valid:
The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
@jsafrane: This pull request references STOR-2962 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. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
pkg/controller/volume/selinuxwarning/openshift_upgrade_controller.go (4)
23-25:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMove conflict state off package-global storage.
previousConflictsbeing package-global makes state shared across controller instances and test cases. Keep this onControllerto avoid cross-instance leakage and concurrency hazards.🤖 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 23 - 25, The package-global variable previousConflicts should be removed and made a field on the Controller struct to avoid shared state across controller instances and tests; add a field like previousConflicts metav1.ConditionStatus to the Controller type, initialize it (default metav1.ConditionUnknown) in the controller constructor/new function, and update all references from previousConflicts to c.previousConflicts inside methods (e.g., reconcile loop and any helpers) so each Controller instance has its own state and you avoid cross-instance leakage and concurrency hazards.
20-20:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winConfigMap name does not match the RBAC-scoped assessment object.
configMapNameisselinux-conflicts, but this PR's RBAC scope targetsselinux-warning-assessment. WithresourceNamesscoping, patch/create will fail for the current name.🤖 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` at line 20, The configured constant configMapName is set to "selinux-conflicts" but the PR's RBAC resourceNames scope targets "selinux-warning-assessment", causing create/patch to be denied; update the configMapName constant to "selinux-warning-assessment" (and adjust any code that references configMapName if it assumes the old name) so the ConfigMap name matches the RBAC-scoped assessment object used by the controller.
81-96:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse merge-patch instead of
replace /data.
replaceon/datafails when the target ConfigMap exists withoutdata. This makes updates unnecessarily brittle. Use merge-patch for idempotent upsert behavior of the key.♻️ Proposed fix
func (c *Controller) patchSELinuxConflictsConfigMap(ctx context.Context, conflictsPresent metav1.ConditionStatus) error { - patch, err := json.Marshal([]map[string]any{ - { - "op": "replace", - "path": "/data", - "value": map[string]string{ - "conflictsPresent": string(conflictsPresent), - }, - }, + patch, err := json.Marshal(map[string]any{ + "data": map[string]string{ + "conflictsPresent": string(conflictsPresent), + }, }) if err != nil { return fmt.Errorf("error building config map %s patch: %w", configMapName, err) } - _, err = c.kubeClient.CoreV1().ConfigMaps(configMapNamespace).Patch(ctx, configMapName, types.JSONPatchType, patch, metav1.PatchOptions{}) + _, err = c.kubeClient.CoreV1().ConfigMaps(configMapNamespace).Patch(ctx, configMapName, types.MergePatchType, patch, metav1.PatchOptions{}) return err }🤖 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 81 - 96, The current patchSELinuxConflictsConfigMap builds a JSON Patch that uses an op "replace" on "/data", which fails when the ConfigMap exists without a data field; change the logic to send a JSON merge patch that upserts the key idempotently by marshaling {"data":{"conflictsPresent": "<value>"}} and call c.kubeClient.CoreV1().ConfigMaps(...).Patch with types.MergePatchType (keeping ctx, configMapName, configMapNamespace and metav1.PatchOptions as before) so the conflictsPresent key is merged/created rather than replaced.
57-59:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid unchecked type assertion on
labelCache.
c.labelCache.(cache.ConflictCounter)can panic iflabelCacheis swapped with an implementation that doesn't satisfyConflictCounter. Guard the assertion and fail safely.🛡️ Proposed fix
func (c *Controller) getConflicts(logger klog.Logger) metav1.ConditionStatus { - conflictsCount := c.labelCache.(cache.ConflictCounter).GetConflictCount() + conflictCounter, ok := c.labelCache.(cache.ConflictCounter) + if !ok { + logger.Error(nil, "labelCache does not implement ConflictCounter") + return metav1.ConditionUnknown + } + conflictsCount := conflictCounter.GetConflictCount() if conflictsCount > 0 {🤖 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 57 - 59, The getConflicts method currently does an unchecked type assertion on c.labelCache to cache.ConflictCounter which can panic; change it to a safe assertion (e.g., cc, ok := c.labelCache.(cache.ConflictCounter)) and handle the non-matching case by returning a safe default (such as metav1.ConditionFalse) and optionally logging the mismatch via the provided logger; update references to GetConflictCount to use cc.GetConflictCount() only when ok. Ensure you modify the Controller.getConflicts function to perform this guarded check and fallback rather than using a direct assertion.
🧹 Nitpick comments (1)
pkg/controller/volume/selinuxwarning/openshift_upgrade_controller_test.go (1)
316-316: 💤 Low valueRemove unused context assignment.
The assigned
ctxon line 316 is never used. Remove the assignment to keep the code clean.♻️ Proposed fix
t.Run(tt.name, func(t *testing.T) { - _, ctx := ktesting.NewTestContext(t) logger := ktesting.NewLogger(t, ktesting.NewConfig()) - _ = ctx🤖 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` at line 316, The line `_ = ctx` in the openshift_upgrade_controller_test.go file is an unused assignment that serves no purpose. Remove the entire line that contains the unused context assignment to keep the code clean and remove unnecessary code.
🤖 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`:
- Line 151: The tests mutate a package-global previousConflicts which breaks
isolation; move previousConflicts into the Controller struct and update callers
and tests to use the instance field. Add a field (e.g., previousConflicts
map[string]bool) to the Controller type in openshift_upgrade_controller.go,
initialize it in the controller constructor/new function, replace all references
to the package-level previousConflicts with c.previousConflicts (or the chosen
field name) throughout the controller code, remove the package-global variable,
and change the test in openshift_upgrade_controller_test.go to set the per-test
controller instance's previousConflicts (e.g., controller.previousConflicts =
tt.initialConflict) instead of mutating a global.
In `@pkg/controller/volume/selinuxwarning/openshift_upgrade_controller.go`:
- Around line 27-36: The loop in runOpenShiftSELinuxConflictsReporter uses
time.After which leaks timers; replace it by creating a
time.NewTicker(checkInterval) (ticker := time.NewTicker(checkInterval)) with
defer ticker.Stop(), then change the select to listen on ctx.Done() and
<-ticker.C and call c.reportSELinuxConflicts(ctx) when the ticker fires,
preserving existing behavior and ensuring the ticker is stopped on exit.
---
Duplicate comments:
In `@pkg/controller/volume/selinuxwarning/openshift_upgrade_controller.go`:
- Around line 23-25: The package-global variable previousConflicts should be
removed and made a field on the Controller struct to avoid shared state across
controller instances and tests; add a field like previousConflicts
metav1.ConditionStatus to the Controller type, initialize it (default
metav1.ConditionUnknown) in the controller constructor/new function, and update
all references from previousConflicts to c.previousConflicts inside methods
(e.g., reconcile loop and any helpers) so each Controller instance has its own
state and you avoid cross-instance leakage and concurrency hazards.
- Line 20: The configured constant configMapName is set to "selinux-conflicts"
but the PR's RBAC resourceNames scope targets "selinux-warning-assessment",
causing create/patch to be denied; update the configMapName constant to
"selinux-warning-assessment" (and adjust any code that references configMapName
if it assumes the old name) so the ConfigMap name matches the RBAC-scoped
assessment object used by the controller.
- Around line 81-96: The current patchSELinuxConflictsConfigMap builds a JSON
Patch that uses an op "replace" on "/data", which fails when the ConfigMap
exists without a data field; change the logic to send a JSON merge patch that
upserts the key idempotently by marshaling {"data":{"conflictsPresent":
"<value>"}} and call c.kubeClient.CoreV1().ConfigMaps(...).Patch with
types.MergePatchType (keeping ctx, configMapName, configMapNamespace and
metav1.PatchOptions as before) so the conflictsPresent key is merged/created
rather than replaced.
- Around line 57-59: The getConflicts method currently does an unchecked type
assertion on c.labelCache to cache.ConflictCounter which can panic; change it to
a safe assertion (e.g., cc, ok := c.labelCache.(cache.ConflictCounter)) and
handle the non-matching case by returning a safe default (such as
metav1.ConditionFalse) and optionally logging the mismatch via the provided
logger; update references to GetConflictCount to use cc.GetConflictCount() only
when ok. Ensure you modify the Controller.getConflicts function to perform this
guarded check and fallback rather than using a direct assertion.
---
Nitpick comments:
In `@pkg/controller/volume/selinuxwarning/openshift_upgrade_controller_test.go`:
- Line 316: The line `_ = ctx` in the openshift_upgrade_controller_test.go file
is an unused assignment that serves no purpose. Remove the entire line that
contains the unused context assignment to keep the code clean and remove
unnecessary code.
🪄 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: c9e22094-20bf-463c-8f29-0698e2fdf568
📒 Files selected for processing (13)
pkg/controller/volume/selinuxwarning/cache/openshift_patch.gopkg/controller/volume/selinuxwarning/cache/volumecache.gopkg/controller/volume/selinuxwarning/cache/volumecache_test.gopkg/controller/volume/selinuxwarning/internal/parse/selinux_label.gopkg/controller/volume/selinuxwarning/internal/parse/selinux_label_test.gopkg/controller/volume/selinuxwarning/metrics.gopkg/controller/volume/selinuxwarning/openshift_upgrade_controller.gopkg/controller/volume/selinuxwarning/openshift_upgrade_controller_test.gopkg/controller/volume/selinuxwarning/selinux_warning_controller.gopkg/controller/volume/selinuxwarning/selinux_warning_controller_test.gopkg/controller/volume/selinuxwarning/translator/selinux_translator.goplugin/pkg/auth/authorizer/rbac/bootstrappolicy/controller_policy.goplugin/pkg/auth/authorizer/rbac/bootstrappolicy/testdata/controller-roles.yaml
🚧 Files skipped from review as they are similar to previous changes (6)
- plugin/pkg/auth/authorizer/rbac/bootstrappolicy/controller_policy.go
- pkg/controller/volume/selinuxwarning/selinux_warning_controller.go
- plugin/pkg/auth/authorizer/rbac/bootstrappolicy/testdata/controller-roles.yaml
- pkg/controller/volume/selinuxwarning/translator/selinux_translator.go
- pkg/controller/volume/selinuxwarning/metrics.go
- pkg/controller/volume/selinuxwarning/cache/volumecache.go
jsafrane
force-pushed
the
kcm-selinux-configmap
branch
from
8e71471 to
c19aaca
Compare
|
@jsafrane: the contents of this pull request could not be automatically validated. The following commits are valid:
The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
@jsafrane: This pull request references STOR-2962 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. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
pkg/controller/volume/selinuxwarning/openshift_upgrade_controller.go (4)
27-35:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace
time.Afterloop with a ticker.Line 32 allocates a new timer every iteration; cancellation before fire can leave pending timers. Use
time.NewTickerwithdefer ticker.Stop().Suggested fix
func (c *Controller) runOpenShiftSELinuxConflictsReporter(ctx context.Context) { + ticker := time.NewTicker(checkInterval) + defer ticker.Stop() + for { select { case <-ctx.Done(): return - case <-time.After(checkInterval): + case <-ticker.C: c.reportSELinuxConflicts(ctx) } } }🤖 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 27 - 35, The loop in runOpenShiftSELinuxConflictsReporter uses time.After which allocates a timer each iteration; replace it with a time.NewTicker to avoid leaked timers: create ticker := time.NewTicker(checkInterval), defer ticker.Stop(), and change the select to wait on <-ctx.Done() and <-ticker.C then call c.reportSELinuxConflicts(ctx); keep using the existing checkInterval and reportSELinuxConflicts symbols.
23-25:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove conflict state off package-global storage.
Line 23 and Line 54 keep mutable state in
previousConflictsat package scope, which is shared across controller instances and tests. Store it onControllerinstead to avoid cross-instance leakage/races.Suggested fix
- var ( - previousConflicts metav1.ConditionStatus = metav1.ConditionUnknown - ) +// in Controller struct (pkg/controller/volume/selinuxwarning/selinux_warning_controller.go): +// previousConflicts metav1.ConditionStatus func (c *Controller) reportSELinuxConflicts(ctx context.Context) { @@ - if currentConflicts == previousConflicts { + if currentConflicts == c.previousConflicts { @@ - previousConflicts = currentConflicts + c.previousConflicts = currentConflicts }Also applies to: 43-55
🤖 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 23 - 25, The package-global mutable variable previousConflicts should be removed and made an instance field on the Controller struct to avoid cross-instance/shared-state races; add a field (e.g., previousConflicts metav1.ConditionStatus) to the Controller type, initialize it in the controller constructor (NewController or equivalent) to metav1.ConditionUnknown, and update all references that used the package-level previousConflicts variable to use c.previousConflicts (or the receiver name used on Controller) instead; also remove the package-scoped declaration and update any tests to construct a Controller and assert against its instance field rather than the package variable.
57-59:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard the
ConflictCountertype assertion.Line 58 can panic if
labelCacheis replaced with an implementation that does not satisfycache.ConflictCounter. Handle the assertion safely and return a fallback condition.Suggested fix
func (c *Controller) getConflicts(logger klog.Logger) metav1.ConditionStatus { - conflictsCount := c.labelCache.(cache.ConflictCounter).GetConflictCount() + conflictCounter, ok := c.labelCache.(cache.ConflictCounter) + if !ok { + logger.Error(nil, "labelCache does not implement ConflictCounter") + return metav1.ConditionUnknown + } + conflictsCount := conflictCounter.GetConflictCount() if conflictsCount > 0 {🤖 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 57 - 59, In getConflicts, avoid the unchecked type assertion on c.labelCache to cache.ConflictCounter: perform a safe assertion (conflictCounter, ok := c.labelCache.(cache.ConflictCounter)), and if ok is false log a warning via the provided logger and return a fallback metav1.ConditionStatus (e.g., metav1.ConditionFalse); otherwise call conflictCounter.GetConflictCount() and proceed as before. Ensure you reference getConflicts, c.labelCache, cache.ConflictCounter, and logger in the change.
82-95:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse merge patch (or add-op) instead of replacing
/data.Line 84 uses JSON Patch
replaceon/data; this fails when the ConfigMap exists without adatafield. Use merge patch to make updates idempotent.Suggested fix
- patch, err := json.Marshal([]map[string]any{ - { - "op": "replace", - "path": "/data", - "value": map[string]string{ - "conflictsPresent": string(conflictsPresent), - }, - }, - }) + patch, err := json.Marshal(map[string]any{ + "data": map[string]string{ + "conflictsPresent": string(conflictsPresent), + }, + }) @@ - _, err = c.kubeClient.CoreV1().ConfigMaps(configMapNamespace).Patch(ctx, configMapName, types.JSONPatchType, patch, metav1.PatchOptions{}) + _, err = c.kubeClient.CoreV1().ConfigMaps(configMapNamespace).Patch(ctx, configMapName, types.MergePatchType, patch, metav1.PatchOptions{}) return err }🤖 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 82 - 95, Current code builds a JSON Patch with op "replace" on "/data" and calls Patch with types.JSONPatchType which fails if the ConfigMap has no data field; instead build a merge-style patch (e.g. a JSON object like {"data":{"conflictsPresent": "<value>"}}), marshal that, and call Patch with types.MergePatchType (or types.StrategicMergePatchType if preferred) so the data key is added/updated idempotently; update the json.Marshal target and change types.JSONPatchType to types.MergePatchType in the c.kubeClient.CoreV1().ConfigMaps(...).Patch call, keeping configMapName, configMapNamespace and the conflictsPresent value logic the same.
🤖 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
`@plugin/pkg/auth/authorizer/rbac/bootstrappolicy/testdata/controller-roles.yaml`:
- Around line 1409-1417: The selinux-warning-controller RBAC rule is misaligned:
controller_policy.go grants system:controller:selinux-warning-controller
create,patch on configmaps without resourceNames, while controller-roles.yaml
restricts it to resourceNames: selinux-conflicts; update one to match the other.
Either remove the resourceNames: selinux-conflicts entry from the rule in
plugin/pkg/auth/authorizer/rbac/bootstrappolicy/testdata/controller-roles.yaml
so the testdata allows create,patch on all configmaps like controller_policy.go,
or conversely add the same resourceNames restriction in
plugin/pkg/auth/authorizer/rbac/bootstrappolicy/controller_policy.go for the
role system:controller:selinux-warning-controller (the rule affecting resources:
configmaps, verbs: create,patch) so both generator and golden testdata are
identical.
---
Duplicate comments:
In `@pkg/controller/volume/selinuxwarning/openshift_upgrade_controller.go`:
- Around line 27-35: The loop in runOpenShiftSELinuxConflictsReporter uses
time.After which allocates a timer each iteration; replace it with a
time.NewTicker to avoid leaked timers: create ticker :=
time.NewTicker(checkInterval), defer ticker.Stop(), and change the select to
wait on <-ctx.Done() and <-ticker.C then call c.reportSELinuxConflicts(ctx);
keep using the existing checkInterval and reportSELinuxConflicts symbols.
- Around line 23-25: The package-global mutable variable previousConflicts
should be removed and made an instance field on the Controller struct to avoid
cross-instance/shared-state races; add a field (e.g., previousConflicts
metav1.ConditionStatus) to the Controller type, initialize it in the controller
constructor (NewController or equivalent) to metav1.ConditionUnknown, and update
all references that used the package-level previousConflicts variable to use
c.previousConflicts (or the receiver name used on Controller) instead; also
remove the package-scoped declaration and update any tests to construct a
Controller and assert against its instance field rather than the package
variable.
- Around line 57-59: In getConflicts, avoid the unchecked type assertion on
c.labelCache to cache.ConflictCounter: perform a safe assertion
(conflictCounter, ok := c.labelCache.(cache.ConflictCounter)), and if ok is
false log a warning via the provided logger and return a fallback
metav1.ConditionStatus (e.g., metav1.ConditionFalse); otherwise call
conflictCounter.GetConflictCount() and proceed as before. Ensure you reference
getConflicts, c.labelCache, cache.ConflictCounter, and logger in the change.
- Around line 82-95: Current code builds a JSON Patch with op "replace" on
"/data" and calls Patch with types.JSONPatchType which fails if the ConfigMap
has no data field; instead build a merge-style patch (e.g. a JSON object like
{"data":{"conflictsPresent": "<value>"}}), marshal that, and call Patch with
types.MergePatchType (or types.StrategicMergePatchType if preferred) so the data
key is added/updated idempotently; update the json.Marshal target and change
types.JSONPatchType to types.MergePatchType in the
c.kubeClient.CoreV1().ConfigMaps(...).Patch call, keeping configMapName,
configMapNamespace and the conflictsPresent value logic the same.
🪄 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: 12d5ec43-5dcb-4e6e-ba06-06f1b6b34632
📒 Files selected for processing (6)
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.goplugin/pkg/auth/authorizer/rbac/bootstrappolicy/controller_policy.goplugin/pkg/auth/authorizer/rbac/bootstrappolicy/testdata/controller-roles.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/controller/volume/selinuxwarning/selinux_warning_controller.go
- plugin/pkg/auth/authorizer/rbac/bootstrappolicy/controller_policy.go
When calling ControllerSELinuxTranslator.Conflicts(), the SELinux label is repeatedly split into []string to detect conflicts. This causes a huge number of allocations when there are many comparisons. This is now made more efficient by pre-parsing the SELinux label and storing it in podInfo as [4]string for fast comparison when needed.
jsafrane
force-pushed
the
kcm-selinux-configmap
branch
from
c19aaca to
36fec1b
Compare
|
@jsafrane: the contents of this pull request could not be automatically validated. The following commits are valid:
The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
@jsafrane: This pull request references STOR-2962 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. |
jsafrane
force-pushed
the
kcm-selinux-configmap
branch
from
36fec1b to
2a71e03
Compare
|
@jsafrane: the contents of this pull request could not be automatically validated. The following commits are valid:
The following commits could not be validated and must be approved by a top-level approver:
Comment |
jsafrane
changed the title
|
@jsafrane: 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. |
jsafrane
force-pushed
the
kcm-selinux-configmap
branch
from
2a71e03 to
b343f24
Compare
|
@jsafrane: the contents of this pull request could not be automatically validated. The following commits are valid:
The following commits could not be validated and must be approved by a top-level approver:
Comment |
jsafrane
force-pushed
the
kcm-selinux-configmap
branch
from
b343f24 to
0f20dc8
Compare
|
@jsafrane: the contents of this pull request could not be automatically validated. The following commits are valid:
The following commits could not be validated and must be approved by a top-level approver:
Comment |
jsafrane
changed the title
…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.
jsafrane
force-pushed
the
kcm-selinux-configmap
branch
from
0f20dc8 to
4ef4ffb
Compare
|
@jsafrane: the contents of this pull request could not be automatically validated. The following commits are valid:
The following commits could not be validated and must be approved by a top-level approver:
Comment |
| func (c *SELinuxConflictsReporterController) patchSELinuxConflictsConfigMap(ctx context.Context, conflictsPresent metav1.ConditionStatus) error { | ||
| patch, err := json.Marshal(map[string]any{ | ||
| "data": map[string]string{ | ||
| "conflictsPresent": string(conflictsPresent), |
There was a problem hiding this comment.
The storage operator will block the upgrade (e.g. via a condition) when this is true, right? Is a single boolean enough information for the user to understand the scope of the issue?
| return | ||
| } | ||
| logger.V(2).Info("Starting OpenShift SELinux conflicts reporter") | ||
| timer := time.NewTimer(checkInterval) |
There was a problem hiding this comment.
If this were a permanent feature, I would suggest using a queue for better error handling/backoff and metrics. For a temporary feature, this should be fine.
| cm := &v1.ConfigMap{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: configMapName, | ||
| Namespace: configMapNamespace, |
There was a problem hiding this comment.
This ConfigMap is expected to be removed in the next (5.1?) release. Also, this controller is behind a feature gate.
Can we add some metadata here (annotation, ownerReference, ect.) to document the purpose and lifecycle of this config map?
| } | ||
| // Introduced in 5.0 | ||
| defaultVersionedKubernetesFeatureGates[SELinuxMountGAReadiness] = featuregate.VersionedSpecs{ | ||
| {Version: version.MustParse("1.35"), Default: false, PreRelease: featuregate.Alpha}, |
There was a problem hiding this comment.
openshift/api#288 adds this FG to TP. When is the feature going to be enabled? I thought, we were targeting this to be enabled by default in 5.0?
| "conflictsPresent": string(conflictsPresent), | ||
| }, | ||
| } | ||
| _, err := c.kubeClient.CoreV1().ConfigMaps(configMapNamespace).Create(ctx, cm, metav1.CreateOptions{}) |
There was a problem hiding this comment.
nit: we could use SSA instead, but the current implementation should suffice for a temporary feature as well
| if cm.Data["conflictsPresent"] != tt.existingConfigMap.Data["conflictsPresent"] { | ||
| t.Errorf("ConfigMap data changed unexpectedly: got %v, want %v", cm.Data, tt.existingConfigMap.Data) | ||
| } | ||
| } |
There was a problem hiding this comment.
else fail?
Shouldn't we ensure that there is an existingConfigMap field set if we expect no write?
4 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.
<carry>. Written in a way that minimizes a possibility of a conflict with future Kubernetes patches.Summary by CodeRabbit
New Features
SELinuxMountGAReadinessfeature gate added.Chores
Tests