KEP-3619: Run SupplementalGroupsPolicy e2e test in CI Jobs #130956
Conversation
|
Skipping CI for Draft Pull Request. |
|
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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 kubernetes-sigs/prow repository. |
|
/test all |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: everpeace The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
/assign @BenTheElder @pohly
would you mind reviewing this small PR? 🙇 🙇
| @@ -714,7 +714,7 @@ var _ = SIGDescribe("Security Context", func() { | |||
| }) | |||
| }) | |||
|
|
|||
| f.Context("SupplementalGroupsPolicy [LinuxOnly]", feature.SupplementalGroupsPolicy, framework.WithFeatureGate(features.SupplementalGroupsPolicy), func() { | |||
| f.Context("SupplementalGroupsPolicy [LinuxOnly]", framework.WithFeatureGate(features.SupplementalGroupsPolicy), func() { | |||
There was a problem hiding this comment.
The "gracefully skip" part of your description raises an alarm bell for me.
See https://github.com/kubernetes/kubernetes/pull/130210/files#r2007744517.
IMHO we should only select tests in jobs which are expected to run, and tests should fail if they cannot run.
There was a problem hiding this comment.
To do that, keep the feature.SupplementalGroupsPolicy, identify the jobs which support it, change them to --label-filter=Feature: isSubsetOf { SupplementalGroupsPolicy, ... }, and remove the runtime skip from the test.
There was a problem hiding this comment.
Thanks for the comment.
IMHO we should only select tests in jobs which are expected to run, and tests should fail if they cannot run.
Yeah, I agreed, ideally.
identify the jobs which support it, change them to --label-filter=Feature: isSubsetOf { SupplementalGroupsPolicy, ... }, and remove the runtime skip from the test.
Currently, it seems we currently don't have e2e jobs with old containerd < v2 (which does not support this feature). So, in this case, do you mean to create a new dedicated job for this?
There was a problem hiding this comment.
So the feature needs containerd >= v2 and wouldn't run with some older containerd? Then you don't need to add a job with containerd < 2 because the feature doesn't need to be tested with it.
There was a problem hiding this comment.
So the feature needs containerd >= v2 and wouldn't run with some older containerd?
Esseentially, yes.
But, there is a corner case, kubelet should now rejects pod creations for pods with pod.spec.securityContext.supplementalGroupsPolicy: Strict on such nodes(older containerd, i.e.node.status.features.supplementalGroupsPoliycy: false) and issues events for the rejections. So, I wrote a testcase for it.
Please also refer to https://github.com/kubernetes/website/blob/dev-1.33/content/en/docs/tasks/configure-pod-container/security-context.md#implementations-implementations-supplementalgroupspolicy (v1.33's document for this feature).
I'm not sure how to handle this corner case in CI Jobs with handy way. I actually felt that introducing a new CI Job was too much only for this case.
I agree your warning. So, now I'm leaning to remove test cases for old CRI runtimes that does not support this feature from E2E test.
WDYT??
Let me ping to KEP-3619 reviewers to have reviewers' feedbacks:
cc/ @haircommander @mrunalp @SergeyKanzhelev @thockin (KEP reviewers)
There was a problem hiding this comment.
If this was aimed at [Conformance] and not [NodeConformance] (NodeConformance is not an official output from sig arch / CNCF etc) then this sort of thing would be a problem, I'm less clear for [NodeConformance].
Self-skips are definitely surprising, but sprinkling ~all of our jobs with this transitional "feature" isn't great either, and also isn't feasible for jobs still using regex (And I don't think we're quite ready to convert everything to label filter yet)
|
/assign |
|
/cc @ajaysundark |
|
@SergeyKanzhelev: GitHub didn't allow me to request PR reviews from the following users: ajaysundark. Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs. In 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 kubernetes-sigs/prow repository. |
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR enables CI jobs to pick up
SupplementalGroupsPolicye2e tests.SupplementalGroupsPolicye2e test can gracefully skip in either case when the CRI runtime does or does not support this feature (the test cases implement both cases in graceful manner).ref: #130210 (comment)
Which issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?