Skip to content
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.

Sign up for GitHub

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

Jump to bottom

E2E: add tests for PSP in the policy API Group #60319

Merged
merged 2 commits into from
Feb 28, 2018
Merged

E2E: add tests for PSP in the policy API Group #60319

merged 2 commits into from
Feb 28, 2018

Conversation

php-coder
Copy link
Contributor

@php-coder php-coder commented Feb 23, 2018 •
edited
Loading

What this PR does / why we need it:
E2E tests were added for testing PSP from the "policy" API Group. They are similar to the tests for PSP from the "extensions" API Group.

Which issue(s) this PR fixes:
Addressed to: kubernetes/enhancements#5
Follow-up to: #54933 and #60145

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 23, 2018
@php-coder
Copy link
Contributor Author

/release-note-none

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Feb 23, 2018
@php-coder php-coder changed the title E2E: add tests for PSP in the policy API Group. E2E: add tests for PSP in the policy API Group Feb 23, 2018
@php-coder
Copy link
Contributor Author

@liggitt PTAL

CC @simo5

@php-coder php-coder force-pushed the psp_e2e_tests_and_policy_group branch from fc03088 to 90617e8 Compare February 23, 2018 19:37
php-coder
php-coder commented Feb 23, 2018
View reviewed changes
test/e2e/framework/psp_util.go Outdated
},
Labels: map[string]string{
"kubernetes.io/cluster-service": "true",
"addonmanager.kubernetes.io/mode": "Reconcile",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liggitt @tallclair BTW why we need these labels?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P.S. And why we have it only for restricted PSP?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pretty sure we don't need them

@php-coder
Copy link
Contributor Author

php-coder commented Feb 23, 2018 •
edited
Loading

@tallclair @liggitt E2E tests also have CreatePrivilegedPSPBinding() function that creates PSP in the "extensions" group. Let me know if we need to modify it to use "policy" group instead.

@php-coder
Copy link
Contributor Author

/test pull-kubernetes-bazel-test

@php-coder
Copy link
Contributor Author

@kubernetes/sig-auth-pr-reviews PTAL

@k8s-ci-robot k8s-ci-robot added the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label Feb 26, 2018
liggitt
liggitt reviewed Feb 27, 2018
View reviewed changes
test/e2e/framework/psp_util.go Outdated

// RestrictedPSPInPolicy creates a PodSecurityPolicy (in the "policy" API Group) that is most strict.
// TODO: merge these helpers when PSP will be completely moved out of the extensions
func RestrictedPSPInPolicy(name string) *policy.PodSecurityPolicy {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer keeping this private to the PSP test

liggitt
liggitt reviewed Feb 27, 2018
View reviewed changes
test/e2e/framework/psp_util.go Outdated
@@ -75,6 +78,159 @@ func PrivilegedPSP(name string) *extensionsv1beta1.PodSecurityPolicy {
}
}

// PrivilegedPSPInPolicy creates a PodSecurityPolicy (in the "policy" API Group) that allows everything.
// TODO: merge these helpers when PSP will be completely moved out of the extensions
func PrivilegedPSPInPolicy(name string) *policy.PodSecurityPolicy {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keep private to PSP test

@liggitt
Copy link
Member

liggitt commented Feb 27, 2018

a couple nits about visibility, LGTM otherwise

@liggitt
Copy link
Member

liggitt commented Feb 27, 2018

/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 27, 2018
@php-coder php-coder force-pushed the psp_e2e_tests_and_policy_group branch 4 times, most recently from a2a97de to 3ce2832 Compare February 27, 2018 15:27
@php-coder
Copy link
Contributor Author

I don't know why it fails :-| I run hack/update-bazel.sh and even make update locally a couple of times but they don't produce anything extra that I don't have already.

liggitt
liggitt reviewed Feb 27, 2018
View reviewed changes
test/e2e/framework/psp_util.go Outdated
@@ -75,6 +77,61 @@ func PrivilegedPSP(name string) *extensionsv1beta1.PodSecurityPolicy {
}
}

// RestrictedPSP creates a PodSecurityPolicy that is most strict.
func RestrictedPSP(name string) *extensionsv1beta1.PodSecurityPolicy {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, why not keep this private to the PSP test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To reduce a chances to introduce a duplicated function in other tests in future.

@liggitt
Copy link
Member

liggitt commented Feb 27, 2018

I don't know why it fails :-| I run hack/update-bazel.sh and even make update locally a couple of times but they don't produce anything extra that I don't have already.

paging @mikedanese...

@php-coder
Copy link
Contributor Author

Seems like I found why it was failing. It was my mistake.

@php-coder php-coder force-pushed the psp_e2e_tests_and_policy_group branch from 3ce2832 to 0aa1fa3 Compare February 27, 2018 16:06
@php-coder php-coder force-pushed the psp_e2e_tests_and_policy_group branch from 0aa1fa3 to 4e273a6 Compare February 27, 2018 16:15
@liggitt
Copy link
Member

liggitt commented Feb 27, 2018

lgtm, @tallclair has final approval
/assign @tallclair

@tallclair
Copy link
Member

/lgtm

There's a lot of duplication, but assuming we can remove it soon (1.11?) I'm ok with that.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 27, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, php-coder, tallclair

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jdumars jdumars added this to the v1.10 milestone Feb 28, 2018
@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 60475, 60514, 60506, 59903, 60319). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 24adcd5 into kubernetes:master Feb 28, 2018
@php-coder php-coder deleted the psp_e2e_tests_and_policy_group branch February 28, 2018 15:27
@wojtek-t wojtek-t mentioned this pull request Apr 5, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

@madhusudancs madhusudancs Awaiting requested review from madhusudancs

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

Assignees

@tallclair tallclair

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. sig/auth Categorizes an issue or PR as relevant to SIG Auth. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.10
Development

Successfully merging this pull request may close these issues.
6 participants
@php-coder @liggitt @tallclair @k8s-ci-robot @k8s-github-robot @jdumars