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

Pod Security Policies #3498

Merged
merged 2 commits into from
Apr 10, 2018
Merged

Pod Security Policies #3498

merged 2 commits into from
Apr 10, 2018

Conversation

gambol99
Copy link
Contributor

@gambol99 gambol99 commented Sep 30, 2017 •
edited
Loading

The current implementation doesn't work with PodSecurityPolicies admission controller enabled due to no psp policies. This PR adds a default psp policy for the kubelet users and and the kube-system namespace.

  • added a default pod security policy covering the kube-system namespace
  • this also covers the kubelet user as without this static pod are not longer visible, which effects the ability of the dns-controller to add the internal api dns records and thus no nodes can come up.

I still need to test this with 1.8.0; thus far only 1.7.2 / 1.7.7 has been tested

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 30, 2017
@gambol99 gambol99 force-pushed the psp branch 2 times, most recently from 2d1d894 to 8a2e562 Compare October 2, 2017 12:51
@gambol99
Copy link
Contributor Author

gambol99 commented Oct 2, 2017 •
edited
Loading

/assign @gambol99
/assign @justinsb
/assign @chrislovecnm

@gambol99 gambol99 changed the title Pod Security Policies Pod Security Policies - WIP Oct 2, 2017
@gambol99
Copy link
Contributor Author

gambol99 commented Oct 2, 2017 •
edited
Loading

i'm gonna leave this one as WIP for now and perhaps offline chat on it. The experience of PSP hasn't been great and at present there a number of outstanding PR's targeted for a 1.8.x release to make this admission controller more user friendly.

  • one issue if a you can see more than one policy, the admission controller will randomly choose one; this effects the deployments in kube-system as they deployed via kops with an user in the system:master group which is tied to cluster-admin clusterrole. This a known issue and two PRs, one adding a weighting factory and ordering have been raised.

@chrislovecnm chrislovecnm added WIP: Work In Progress do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. and removed WIP: Work In Progress labels Oct 3, 2017
@justinsb justinsb added this to the 1.9 milestone Nov 22, 2017
@kubernetes kubernetes deleted a comment from k8s-github-robot Dec 5, 2017
@chrislovecnm
Copy link
Contributor

@gambol99 is this something that you are working on? Just grooming old PRs

@gambol99
Copy link
Contributor Author

apologizes for the delay @chrislovecnm ... currently away for xmas in italy so trying to stay away from a laptop :-) .. The PR was functional, the issue was due to problem with the admission controller itself. Effectively if you had a user or service account which via rbac can see all policies (i.e. pretty much most things deployed in kube-system as it was deployed via an cluster wide admin) the controller will randomly pick a policy. So there's a percentage of pods restarted which will end up with a policy which might conflict. In our case a few restarts of kube-dns would fail due to the RunAsNonRoot requirement. Their we two PR's which fix this behavior, one which orders the policy via name and the other which add's a priority field to the policies. Both are scheduled for a 1.9 release ..

@k8s-github-robot
Copy link

@gambol99 PR needs rebase

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 18, 2017
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 15, 2018
@gambol99 gambol99 force-pushed the psp branch 2 times, most recently from aed819f to 07d18b1 Compare April 4, 2018 15:37
@gambol99
Pod Security Policies
e8e3ac0 
The current implementation doesn't work with PodSecurityPolicies enabled due to no psp policies. This PR adds a default psp policy for the kubelet users and and the kube-system namespace
@gambol99 gambol99 changed the title Pod Security Policies - WIP Pod Security Policies Apr 4, 2018
@k8s-ci-robot 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 Apr 4, 2018
@gambol99
Copy link
Contributor Author

gambol99 commented Apr 4, 2018 •
edited
Loading

@justinsb @chrislovecnm ... since >= 1.9.0 this one is good to go as the issue effecting pods who's rbac permits seeing multiple policies has been fixed

gambol99
gambol99 commented Apr 4, 2018
View reviewed changes
upup/pkg/fi/cloudup/bootstrapchannelbuilder.go Outdated
Version: fi.String(version),
Selector: map[string]string{"k8s-addon": key},
Manifest: fi.String(location),
KubernetesVersion: ">=1.6.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

can i just change this to >=1.9.0 @justinsb

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah probably best to lock down to k8s 1.9+, also update the template name to match (and the references to it on L119 - L120)

@KashifSaadat
Copy link
Contributor

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gambol99, KashifSaadat

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:
  • OWNERS [KashifSaadat,gambol99]

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

@KashifSaadat
Copy link
Contributor

/test pull-kops-bazel-build

@k8s-ci-robot k8s-ci-robot merged commit 22a3458 into kubernetes:master Apr 10, 2018
@gambol99 gambol99 deleted the psp branch February 21, 2019 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KashifSaadat KashifSaadat KashifSaadat left review comments

@justinsb justinsb justinsb left review comments

Assignees

@justinsb justinsb

@gambol99 gambol99

@KashifSaadat KashifSaadat

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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
1.9.0
Development

Successfully merging this pull request may close these issues.
6 participants
@gambol99 @chrislovecnm @k8s-github-robot @KashifSaadat @k8s-ci-robot @justinsb