New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
KEP-127: Add UserNamespacesPodSecurityStandards e2e test #121606
KEP-127: Add UserNamespacesPodSecurityStandards e2e test #121606
Conversation
/test pull-kubernetes-node-kubelet-serial-containerd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the tests are not testing the functionality we are adding. Don't we want to test that you can run as root, with the PSS restricted and the feature gate enabled?
Am I missing something?
388883a
to
ffef6ec
Compare
/retest |
ac23fbe
to
59570a2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
/assign |
@@ -633,6 +634,39 @@ var _ = SIGDescribe("Security Context", func() { | |||
}) | |||
}) | |||
|
|||
var _ = SIGDescribe("User Namespaces for Pod Security Standards [Feature:UserNamespacesSupport] [Feature:UserNamespacesPodSecurityStandards] [LinuxOnly]", func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should you use the newly added valid feature variable here (UserNamespacesPodSecurityStandards
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only [Feature:UserNamespacesPodSecurityStandards]
? I was assuming that we use [Feature:UserNamespacesSupport]
as well because other tests are referring to it, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
LGTM label has been added. Git tree hash: 2d56b2d94bfd621ffd565d43d27765c0d3e0355d
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
I thought this #123183 is already merged. But no, we need somebody from sig testing to approve this PR. CC: @pohly @BenTheElder |
59570a2
to
ace3c38
Compare
Had to rebase on top of the latest |
/retest |
@saschagrunert: please fix these errors:
You can check locally with |
test/e2e/feature/feature.go
Outdated
@@ -328,6 +328,9 @@ var ( | |||
// TODO: document the feature (owning SIG, when to use this feature for a test) | |||
UserNamespacesSupport = framework.WithFeature(framework.ValidFeatures.Add("UserNamespacesSupport")) | |||
|
|||
// TODO: document the feature (owning SIG, when to use this feature for a test) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add documentation together the new feature.
@SergeyKanzhelev: would you have pointed out the same thing if I hadn't gotten involved?
I'm not sure how to ensure that developers do this without being reminded. Suggestions welcome 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SergeyKanzhelev: would you have pointed out the same thing if I hadn't gotten involved?
Yes, after the PR with TODO:
s was merged.
Adding a e2e test for the functionality added in kubernetes#118760. Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
ace3c38
to
e158a83
Compare
Thank you for the hint, fixed as suggested |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
LGTM label has been added. Git tree hash: bd7058dfccfdae42874f2be0cf2801851d327534
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rata, saschagrunert, SergeyKanzhelev 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:
Approvers can indicate their approval by writing |
@saschagrunert: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
/retest |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Adding a e2e test for the functionality added in #118760.
Which issue(s) this PR fixes:
None
Special notes for your reviewer:
cc @mrunalp @rata @giuseppe
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: