OCPBUGS-10431: allow worker pools to set staticPodPath by relaxing validation to only block control plane pools#5724
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@harche: This pull request references Jira Issue OCPBUGS-10431, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
/jira refresh |
|
@harche: This pull request references Jira Issue OCPBUGS-10431, which is invalid:
Comment 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. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@harche: This pull request references Jira Issue OCPBUGS-10431, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (schoudha@redhat.com), skipping review request. 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-required |
|
/retest |
|
/cc @haircommander |
|
I think a better alternative would be to relax the validation to allow the worker pool to set its static pod path to "" but not control plane. Is this something you want to try @harche ? |
harche
force-pushed
the
remove-worker-staticpodpath
branch
from
de2e5d1 to
489498b
Compare
|
@harche: An error was encountered searching for bug OCPBUGS-10431 on the Jira server at https://issues.redhat.com. No known errors were detected, please see the full error message for details. Full error message.
No response returned: Get "https://issues.redhat.com/rest/api/2/issue/OCPBUGS-10431": GET https://issues.redhat.com/rest/api/2/issue/OCPBUGS-10431 giving up after 5 attempt(s)
Please contact an administrator to resolve this issue, then request a bug refresh with 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughAdds pool-aware validation that rejects non-empty Kubelet StaticPodPath per-pool during bootstrap and sync; introduces a helper to enforce this and updates tests accordingly. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can customize the high-level summary generated by CodeRabbit.Configure the |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: harche 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 |
harche
changed the title
|
@harche: An error was encountered searching for bug OCPBUGS-10431 on the Jira server at https://issues.redhat.com. No known errors were detected, please see the full error message for details. Full error message.
No response returned: Get "https://issues.redhat.com/rest/api/2/issue/OCPBUGS-10431": GET https://issues.redhat.com/rest/api/2/issue/OCPBUGS-10431 giving up after 5 attempt(s)
Please contact an administrator to resolve this issue, then request a bug refresh with 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/controller/kubelet-config/helpers.go`:
- Around line 393-406: The validation currently only rejects non-empty
kcDecoded.StaticPodPath for control-plane pools; update
validateStaticPodPathForPool so any non-empty StaticPodPath is rejected
(workers/custom pools may only set it to the empty string to disable static
pods). Specifically, in validateStaticPodPathForPool, after DecodeKubeletConfig
and checking kcDecoded.StaticPodPath, return an error whenever
kcDecoded.StaticPodPath != "" (include poolName and kcDecoded.StaticPodPath in
the message) instead of restricting the check to
ctrlcommon.MachineConfigPoolMaster/Arbiter.
In `@pkg/controller/kubelet-config/kubelet_config_controller.go`:
- Around line 600-605: The current for-loop validates each mcp pool inside the
same loop that later mutates MachineConfigs, which can cause partial updates if
a later pool fails; change the logic to perform a two-phase approach: first
iterate over mcpPools and call validateStaticPodPathForPool(cfg, role) for every
pool and short-circuit by returning ctrl.syncStatusOnly(cfg, err) on the first
validation error, and only if all validations pass proceed to the existing
apply/mutation loop that updates MachineConfigs; ensure you reference the same
cfg, mcpPools, validateStaticPodPathForPool, and ctrl.syncStatusOnly symbols so
that validation is fully separated from mutation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5d85d52c-1731-4652-b443-c22182afe04d
📒 Files selected for processing (4)
pkg/controller/kubelet-config/helpers.gopkg/controller/kubelet-config/kubelet_config_bootstrap.gopkg/controller/kubelet-config/kubelet_config_controller.gopkg/controller/kubelet-config/kubelet_config_controller_test.go
…lidation to only block control plane pools Signed-off-by: Harshal Patil <12152047+harche@users.noreply.github.com>
harche
force-pushed
the
remove-worker-staticpodpath
branch
from
489498b to
cb8c576
Compare
|
@harche: An error was encountered searching for bug OCPBUGS-10431 on the Jira server at https://issues.redhat.com. No known errors were detected, please see the full error message for details. Full error message.
No response returned: Get "https://issues.redhat.com/rest/api/2/issue/OCPBUGS-10431": GET https://issues.redhat.com/rest/api/2/issue/OCPBUGS-10431 giving up after 5 attempt(s)
Please contact an administrator to resolve this issue, then request a bug refresh with 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-required |
| if poolName == ctrlcommon.MachineConfigPoolMaster || poolName == ctrlcommon.MachineConfigPoolArbiter { | ||
| return fmt.Errorf("KubeletConfiguration: staticPodPath is not allowed to be set for pool %q, but contains: %s", poolName, kcDecoded.StaticPodPath) | ||
| } | ||
| return fmt.Errorf("KubeletConfiguration: staticPodPath must be empty for non-control-plane pool %q, but contains: %s", poolName, kcDecoded.StaticPodPath) |
There was a problem hiding this comment.
couldn't this error be covered by the above error case? why do we need different errors for different pool names (especially when we embed pool name in error)
| return ctrl.syncStatusOnly(cfg, err, "could not get the TLSSecurityProfile from %v: %v", ctrlcommon.APIServerInstanceName, err) | ||
| } | ||
|
|
||
| // Validate staticPodPath for all pools before applying any changes |
There was a problem hiding this comment.
why not closer to where we get the mcpPools (587 or so)?
|
@harche: 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. |
3 participants
- What I did
Relaxed the
staticPodPathvalidation to be pool-aware instead of a blanket ban. Worker and custom pools are now allowed to setstaticPodPath(e.g. to"") via a KubeletConfig CR, while control plane pools (master, arbiter) remain blocked.Fixes: https://issues.redhat.com/browse/OCPBUGS-10431
- How to verify it
staticPodPath: ""— it should be acceptedstaticPodPath: ""— the controller should reject it with an error condition- Description for the changelog
Allow worker pools to set
staticPodPathvia KubeletConfig CR to resolve DISA STIG V-242397 compliance finding.