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

Automated cherry pick of #52849 #54029

Merged
merged 12 commits into from
Oct 31, 2017
Merged

Automated cherry pick of #52849 #54029

merged 12 commits into from
Oct 31, 2017

Conversation

liggitt
Copy link
Member

@liggitt liggitt commented Oct 17, 2017 •
edited
Loading

Cherry pick of #52849 on release-1.8.

#52849: PodSecurityPolicy: Do not mutate nil privileged field

This is a larger change than usual for a cherry-pick, but changes are isolated to the PSP component, the cherry-pick was clean, and this fixes severe usability issues with systems with more than one PSP. Would like to get feedback from community users making use of PSP and let the master PR soak as we consider this.

PodSecurityPolicy: when multiple policies allow a submitted pod, priority is given to ones which do not require any fields in the pod spec to be defaulted. If the pod must be defaulted, the first policy (ordered by name) that allows the pod is used.

liggitt added 12 commits October 16, 2017 21:03
@liggitt
PodSecurityPolicy: Do not mutate nil privileged field to false
cdc66b5
@liggitt
PodSecurityPolicy: only set runAsNonRoot when runAsUser is nil
3c00486
@liggitt
PodSecurityPolicy: avoid unnecessary mutation of container capabilities
43d930f
@liggitt
PodSecurityPolicy: avoid unnecessary mutation of supplemental groups
6842783
@liggitt
PodSecurityPolicy: pass effective capabilities to validation interface
454c000
@liggitt
PodSecurityPolicy: limit validation to provided groups
2187095
@liggitt
PodSecurityPolicy: pass effective selinux options to validate
d260ffe
@liggitt
PodSecurityPolicy: pass effective runAsNonRoot and runAsUser to user …
6e78901 
…validation interface
@liggitt
GC: Add check for nil interface
44355fb
@liggitt
SecurityContext: Add accessors/mutators for effective container secur…
8dcab1f 
…ity context
@liggitt
PodSecurityPolicy: avoid unnecessary securitycontext mutation
329f5b5
@liggitt
PodSecurityPolicy: Order by name, prefer non-mutating policies, requi…
2f45324 
…re *api.Pod, allow GC updates
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 17, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: liggitt
We suggest the following additional approver: brendandburns

Assign the PR to them by writing /assign @brendandburns in a comment when ready.

Associated issue: 52849

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the do-not-merge/cherry-pick-not-approved Indicates that a PR is not yet approved to merge into a release branch. label Oct 17, 2017
@liggitt liggitt assigned pweil- and tallclair and unassigned brendandburns and dchen1107 Oct 17, 2017
@liggitt
Copy link
Member Author

liggitt commented Oct 17, 2017

cc @jhorwit2

@liggitt liggitt added this to the v1.8 milestone Oct 17, 2017
@liggitt liggitt added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. sig/auth Categorizes an issue or PR as relevant to SIG Auth. cherrypick-candidate labels Oct 17, 2017
@k8s-ci-robot k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Oct 17, 2017
@liggitt liggitt mentioned this pull request Oct 19, 2017
Merged
@ericchiang
Copy link
Contributor

From a user perspective, I'd like to see this considered for a 1.8 patch release.

CoreOS wants to start experimenting with Pod Security Policy with customers and providing feedback, but there were serious enough concerns about the behavior of PSP in 1.8 (#52367 (comment)) that we're not sure we can adequately evaluate the feature.

cc @joshrosso

@liggitt
Copy link
Member Author

liggitt commented Oct 21, 2017

/retest

@jpbetz
Copy link
Contributor

jpbetz commented Oct 23, 2017

Let's hold this off for 1.8.2 given the size and the current hold on the CP. If there is strong buy-in from sig-auth we can consider for 1.8.3.

@jhorwit2
Copy link
Contributor

I validated this PR against my existing test cluster and ran into no issues while upgrading or testing the functionality.

Scenarios tested:

  • v1.8.1 -> v1.8.2 upgrade with existing restricted & privileged PSP's.
  • Validated that an admin user who has access to the restricted (non-mutating) and privileged (non-mutating) PSP's had privileged chosen since it was first when sorted by name.
  • Validated that an admin user who has access to the restricted(mutating) and privileged (non-mutating) PSP's had privileged chosen since it was non-mutating.
  • Renamed privileged to z to ensure that the prior test didn't pick privileged due to sorting. z was chosen because it was non-mutating.
  • Validated normal users with access only to restricted were not able to deploy privileged PSP pods regardless if it required mutating or not.

@jpbetz jpbetz added approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm "Looks good to me", indicates that a PR is ready to be merged. cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. cherrypick-candidate labels Oct 30, 2017
@jpbetz
Copy link
Contributor

jpbetz commented Oct 30, 2017

Merging this in for 1.8.3.

@k8s-github-robot k8s-github-robot removed the do-not-merge/cherry-pick-not-approved Indicates that a PR is not yet approved to merge into a release branch. label Oct 30, 2017
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue.

@k8s-github-robot k8s-github-robot merged commit 4e74128 into kubernetes:release-1.8 Oct 31, 2017
@ericchiang ericchiang mentioned this pull request Nov 2, 2017
Closed
@liggitt liggitt deleted the automated-cherry-pick-of-#52849-upstream-release-1.8 branch April 22, 2019 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pweil- pweil- pweil- approved these changes

Assignees

@pweil- pweil-

@tallclair tallclair

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. 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 Denotes a PR that will be considered when it comes time to generate release notes. sig/auth Categorizes an issue or PR as relevant to SIG Auth. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v1.8
Development

Successfully merging this pull request may close these issues.