e2e: support admissionapi.LevelRestricted in test/e2e/framework/pod #118134

pohly · 2023-05-19T15:50:14Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

CreatePod and MakePod only accepted an isPrivileged boolean, which made it impossible to write tests using those helpers which work in a default framework.Framework, because the default there is LevelRestricted.

The simple boolean gets replaced with admissionapi.Level. Passing LevelRestricted does the same as calling e2epod.MixinRestrictedPodSecurity.

Special notes for your reviewer:

Instead of explicitly passing a constant to these modified helpers, most tests get updated to pass f.NamespacePodSecurityEnforceLevel. This has the advantage that if that level gets lowered in the future, tests only need to be updated in one place.

In some cases, helpers taking client+namespace+timeouts parameters get replaced with passing the Framework instance to get access to f.NamespacePodSecurityEnforceLevel. These helpers don't need separate parameters because in practice all they ever used where the values from the Framework instance.

See also https://kubernetes.slack.com/archives/C019LFTGNQ3/p1684501567057229

Does this PR introduce a user-facing change?

NONE

k8s-ci-robot · 2023-05-19T15:50:22Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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/test-infra repository.

pohly · 2023-05-22T05:42:29Z

@chrishenzie, @humblec: tests should be passing now, please take a look.

/hold

For approval by some other test/e2e approver.

humblec · 2023-05-22T06:21:19Z

Ack.. will review this soon. thanks 👍

aojea · 2023-05-22T10:29:55Z

/cc @liggitt @s-urbaniak

since they authored the initial implementation #106454

humblec · 2023-05-22T10:57:57Z

/retitle e2e: support admissionapi.LevelRestricted in test/e2e/framework/pod

humblec · 2023-05-22T11:00:26Z

it looks good to me. In many e2e test cases, we have been taking seperate args/params for client+namespace+timeouts, I am not sure were there any specific reason to stick to this model instead of using it from framework..

liggitt · 2023-05-22T13:32:00Z

test/e2e/framework/deployment/fixtures.go

@@ -72,7 +73,8 @@ func NewDeployment(deploymentName string, replicas int32, podLabels map[string]s

 // CreateDeployment creates a deployment.
 func CreateDeployment(ctx context.Context, client clientset.Interface, replicas int32, podLabels map[string]string, nodeSelector map[string]string, namespace string, pvclaims []*v1.PersistentVolumeClaim, command string) (*appsv1.Deployment, error) {
-	deploymentSpec := testDeployment(replicas, podLabels, nodeSelector, namespace, pvclaims, false, command)
+	// TODO: let the caller decide about the security level.


mark the todo with a name or an issue

I started writing an issue and then noticed that the function is only called in three places. Addressing the TODO by passing admissionapi.LevelRestricted is now in this PR.

liggitt · 2023-05-22T13:34:38Z

test/e2e/framework/volume/fixtures.go

@@ -398,8 +399,9 @@ func runVolumeTesterPod(ctx context.Context, client clientset.Interface, timeout
 	When SELinux is enabled on the host, client-pod can not read the content, with permission denied.
 	Invoking client-pod as privileged, so that it can access the volume content, even when SELinux is enabled on the host.
 	*/
-	if config.Prefix == "hostpathsymlink" || config.Prefix == "hostpath" {
-		privileged = true
+	securityLevel := admissionapi.LevelBaseline // TODO: also support LevelRestricted


associate the todo with a github user or an issue

liggitt · 2023-05-22T13:35:35Z

test/e2e/storage/nfs_persistent_volume-disruptive.go

@@ -178,7 +178,7 @@ var _ = utils.SIGDescribe("NFSPersistentVolumes[Disruptive][Flaky]", func() {
 			framework.ExpectNoError(e2epv.WaitOnPVandPVC(ctx, c, f.Timeouts, ns, pv2, pvc2))

 			ginkgo.By("Attaching both PVC's to a single pod")
-			clientPod, err = e2epod.CreatePod(ctx, c, ns, nil, []*v1.PersistentVolumeClaim{pvc1, pvc2}, true, "")
+			clientPod, err = e2epod.CreatePod(ctx, c, ns, nil, []*v1.PersistentVolumeClaim{pvc1, pvc2}, f.NamespacePodSecurityEnforceLevel, "")


f.NamespacePodSecurityEnforceLevel is not obviously equivalent to privileged... is it supposed to be? comment applied everywhere this replacement was made

The reasoning is this: if the function had been used to create a privileged pod in a namespace that only had LevelBaseline, then creating that pod would have failed. Therefore those namespaces must have been configured with LevelPrivileged and thus this change is valid.

pohly · 2023-05-22T17:48:50Z

it looks good to me. In many e2e test cases, we have been taking seperate args/params for client+namespace+timeouts, I am not sure were there any specific reason to stick to this model instead of using it from framework

These helper functions are unnecessarily complex: in theory, they could be used with different combination of parameters, but in practice, that flexibility is never used. But the complexity comes at a cost when reading the code ("where do these values come from?"), when calling them (need to pass several parameters in the right order), or when extending them (adding another value in this PR). I think the helpers should be as simple as possible.

BenTheElder · 2023-05-23T18:52:36Z

/retest
[debugging https://github.com//issues/118201, have not had a chance to look at this PR]

CreatePod and MakePod only accepted an `isPrivileged` boolean, which made it impossible to write tests using those helpers which work in a default framework.Framework, because the default there is LevelRestricted. The simple boolean gets replaced with admissionapi.Level. Passing LevelRestricted does the same as calling e2epod.MixinRestrictedPodSecurity. Instead of explicitly passing a constant to these modified helpers, most tests get updated to pass f.NamespacePodSecurityLevel. This has the advantage that if that level gets lowered in the future, tests only need to be updated in one place. In some cases, helpers taking client+namespace+timeouts parameters get replaced with passing the Framework instance to get access to f.NamespacePodSecurityEnforceLevel. These helpers don't need separate parameters because in practice all they ever used where the values from the Framework instance.

liggitt · 2023-07-05T13:29:32Z

/lgtm

k8s-ci-robot · 2023-07-05T13:29:40Z

LGTM label has been added.

Git tree hash: 342dbda0f62df098072f1fb5086670bb9c991d45

liggitt · 2023-07-05T13:29:42Z

/approve
/hold cancel

k8s-ci-robot · 2023-07-05T13:30:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, pohly

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~test/OWNERS~~ [liggitt,pohly]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot requested review from chrishenzie and humblec May 19, 2023 15:51

pohly force-pushed the e2e-pod-security-levels branch from 0e728f7 to 842388b Compare May 19, 2023 18:32

bart0sh added this to WIP in SIG Node PR Triage May 19, 2023

pohly force-pushed the e2e-pod-security-levels branch 2 times, most recently from 865b289 to c1ecff5 Compare May 22, 2023 05:41

pohly changed the title ~~WIP: e2e: support admissionapi.LevelRestricted in test/e2e/framwork/pod~~ e2e: support admissionapi.LevelRestricted in test/e2e/framwork/pod May 22, 2023

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 22, 2023

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 22, 2023

k8s-ci-robot requested review from liggitt and s-urbaniak May 22, 2023 10:29

k8s-ci-robot changed the title ~~e2e: support admissionapi.LevelRestricted in test/e2e/framwork/pod~~ e2e: support admissionapi.LevelRestricted in test/e2e/framework/pod May 22, 2023

liggitt reviewed May 22, 2023

View reviewed changes

pohly force-pushed the e2e-pod-security-levels branch from c1ecff5 to 0ebe1ca Compare May 22, 2023 18:13

SergeyKanzhelev added this to Triage in SIG Node CI/Test Board May 24, 2023

SergeyKanzhelev moved this from Triage to Archive-it in SIG Node CI/Test Board May 24, 2023

bart0sh moved this from WIP to Needs Reviewer in SIG Node PR Triage Jun 1, 2023

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 29, 2023

pohly force-pushed the e2e-pod-security-levels branch from 0ebe1ca to 5635e31 Compare July 3, 2023 14:13

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 3, 2023

pohly force-pushed the e2e-pod-security-levels branch from 5635e31 to c903c29 Compare July 3, 2023 14:27

k8s-ci-robot assigned liggitt Jul 5, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 5, 2023

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 5, 2023

k8s-ci-robot merged commit ce7fd46 into kubernetes:master Jul 5, 2023
19 checks passed

SIG Node CI/Test Board automation moved this from Archive-it to Done Jul 5, 2023

SIG Node PR Triage automation moved this from Needs Reviewer to Done Jul 5, 2023

k8s-ci-robot added this to the v1.28 milestone Jul 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

e2e: support admissionapi.LevelRestricted in test/e2e/framework/pod #118134

e2e: support admissionapi.LevelRestricted in test/e2e/framework/pod #118134

pohly commented May 19, 2023 •

edited

k8s-ci-robot commented May 19, 2023

pohly commented May 22, 2023

humblec commented May 22, 2023

aojea commented May 22, 2023

humblec commented May 22, 2023

humblec commented May 22, 2023 •

edited

liggitt May 22, 2023

pohly May 22, 2023

liggitt May 22, 2023

pohly May 22, 2023

liggitt May 22, 2023

pohly May 22, 2023

pohly commented May 22, 2023 •

edited

BenTheElder commented May 23, 2023 •

edited

liggitt commented Jul 5, 2023

k8s-ci-robot commented Jul 5, 2023

liggitt commented Jul 5, 2023

k8s-ci-robot commented Jul 5, 2023

e2e: support admissionapi.LevelRestricted in test/e2e/framework/pod #118134

e2e: support admissionapi.LevelRestricted in test/e2e/framework/pod #118134

Conversation

pohly commented May 19, 2023 • edited

What type of PR is this?

What this PR does / why we need it:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

k8s-ci-robot commented May 19, 2023

pohly commented May 22, 2023

humblec commented May 22, 2023

aojea commented May 22, 2023

humblec commented May 22, 2023

humblec commented May 22, 2023 • edited

liggitt May 22, 2023

Choose a reason for hiding this comment

pohly May 22, 2023

Choose a reason for hiding this comment

liggitt May 22, 2023

Choose a reason for hiding this comment

pohly May 22, 2023

Choose a reason for hiding this comment

liggitt May 22, 2023

Choose a reason for hiding this comment

pohly May 22, 2023

Choose a reason for hiding this comment

pohly commented May 22, 2023 • edited

BenTheElder commented May 23, 2023 • edited

liggitt commented Jul 5, 2023

k8s-ci-robot commented Jul 5, 2023

liggitt commented Jul 5, 2023

k8s-ci-robot commented Jul 5, 2023

pohly commented May 19, 2023 •

edited

humblec commented May 22, 2023 •

edited

pohly commented May 22, 2023 •

edited

BenTheElder commented May 23, 2023 •

edited