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.

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

PodSecurityPolicies for addons #55509

Merged
merged 4 commits into from
Nov 14, 2017
Merged

Conversation

tallclair
Copy link
Member

What this PR does / why we need it:

  1. Colocate addon PodSecurityPolicy config with the addons (in a podsecuritypolicies subdirectory).
  2. Add policies for addons that are currently missing policies (not in the default GCE suite)
  3. Remove HostPath SSL certs from several heapster deployments, so that heapster doesn't require a special PSP

Which issue(s) this PR fixes:
#43538

Release note:

- Add PodSecurityPolicies for cluster addons
- Remove SSL cert HostPath volumes from heapster addons

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 10, 2017
@tallclair tallclair changed the title Psp addons PodSecurityPolicies for addons Nov 10, 2017
@tallclair
Copy link
Member Author

/cc @liggitt
/cc @crassirostris (for heapster changes)

@@ -58,26 +58,12 @@ spec:
- /heapster
- --source=kubernetes.summary_api:''
- --sink=gcm
volumeMounts:
- name: ssl-certs
Copy link
Member

Choose a reason for hiding this comment

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

Does heapster not need to talk to googleapis.com?

Copy link
Member Author

Choose a reason for hiding this comment

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

With this change heapster uses the SSL certs that are built into the container, rather than those on the host. My understanding is that the certificates in the container are sufficient that.

Copy link
Member Author

Choose a reason for hiding this comment

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

(To clarify - usually this practice of mounting the host certificates is only needed for FROM scratch containers. However, heapster copies in the host certs when it's built, so this isn't necessary).

Copy link
Member

Choose a reason for hiding this comment

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

how long are those valid for?

Copy link
Member Author

Choose a reason for hiding this comment

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

$ openssl x509 -in heapster/92474ba266e5a063e88a4cc5f136d5bac305cb75538b4086a73c1ac27d834ce6/etc/ssl/certs/ca-certificates.crt  -text
Certificate:
    Data:
        Version: 3 (0x2)
        Serial Number: 6828503384748696800 (0x5ec3b7a6437fa4e0)
    Signature Algorithm: sha1WithRSAEncryption
        Issuer: CN=ACCVRAIZ1, OU=PKIACCV, O=ACCV, C=ES
        Validity
            Not Before: May  5 09:37:37 2011 GMT
            Not After : Dec 31 09:37:37 2030 GMT
        Subject: CN=ACCVRAIZ1, OU=PKIACCV, O=ACCV, C=ES
...

Looks like that shouldn't be a problem, as long as they aren't revoked.

That said, this is a a common requirement for containers. Maybe we need to build a solution that doesn't require a HostPath volume...

Copy link
Member

Choose a reason for hiding this comment

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

Ok, ya. I agree.

@mikedanese
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 13, 2017
@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 13, 2017
@tallclair tallclair added this to the v1.8 milestone Nov 13, 2017
@crassirostris
Copy link

For the heapster changes
/lgtm

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: crassirostris, mikedanese, tallclair

Associated issue: 43538

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
Copy link

Automatic merge from submit-queue (batch tested with PRs 54602, 54877, 55243, 55509, 55128). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit b2125f5 into kubernetes:master Nov 14, 2017
@jpbetz jpbetz added cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. and removed cherrypick-candidate labels Nov 16, 2017
k8s-github-robot pushed a commit that referenced this pull request Nov 17, 2017
…509-upstream-release-1.8

Automatic merge from submit-queue.

Automated cherry pick of #55509 upstream release 1.8

Cherry pick of #55509 on release-1.8.

#55509: PodSecurityPolicies for addons

Justification: configuration-only changes to add PodSecurityPolicies for cluster addons, which is required for enabling the controller.

```release-note
- Add PodSecurityPolicies for cluster addons
- Remove SSL cert HostPath volumes from heapster addons
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants