Add SCC support for blocking pods from using EmptyDir volumes.

[dgoodwin]

Builds off the example of AllowHostDirVolumePlugin. Change is being implemented to satisfy some online/multi-tenant use cases, and roughly matches a planned feature for pod security policy: https://github.com/pweil-/kubernetes/blob/security-policy/docs/proposals/security-context-constraints.md

There is however an outstanding problem I could use some guidance on, today use of empty dir volumes is implicitly allowed, but this SCC parameter is a boolean to allow it, which will zero to false effectively breaking every SCC out in the wild. I am unsure how best to roll this out.

Ideas:

(1) reconcile-sccs command

(2) Inverting it to DenyEmptyDirVolumePlugin. There's nothing else in the SCC that would match this 'deny' convention, so it feels inconsistent, but it might be more technically correct for what it actually needs to do.

(3) Alternatively could we make it a string so we can have a legitimate third state? Empty string = unset, "true" and "false"?

We don't really need a third state so in some ways I'd kind of lean towards (2) as being the best option, but please let me know what you think or if there are other ideas.

[liggitt]

current SCCs allow empty dir, so we have to preserve that behavior

[liggitt]

if you need to distinguish between unset, true, and false, you can make the v1 field a *bool, the internal field a bool, use defaulting in v1 to preserve prior behavior, and use conversion to take the external *bool to a bool internally

[ironcladlou]

I'm not sure we need to distinguish unset with a pointer type. I think we can use a defaulting function to set true.

[ironcladlou]

Whoops, nevermind. We need a pointer AND a defaulter.

[ironcladlou]

Existing SCC defaults: https://github.com/openshift/origin/blob/master/Godeps/_workspace/src/k8s.io/kubernetes/pkg/api/v1/defaults.go#L276

[dgoodwin]

Thanks for the tips @liggitt & @ironcladlou

Branch & commit message updated, now using a primitive bool on the API SCC but *bool on the versioned SCC types.

Registered a default for the type as well to ensure we make this true whenever possible. The bootstrappolicy still needs to set it explicitly however as we're just creating the objects directly.

The k8s "generated".go files are manually updated for the new struct field.

Removing WIP as I think it's ready for full review.

[ironcladlou]

I don't think this is the canonical format for a field error. Check out https://github.com/dgoodwin/origin/blob/block-emptydir-scc/Godeps/_workspace/src/k8s.io/kubernetes/pkg/securitycontextconstraints/provider.go#L264 for reference. I think you should address it as something like volumes[index] (and correctly prefixed).

[dgoodwin]

Original error:

Error creating: Pod "testrc-" is forbidden: unable to validate against any security context constraint: [provider restricted: .spec.containers[0].securityContext.VolumeMounts: invalid value 'container-data', Details: EmptyDir Volumes are not allowed to be used]

I changed it to this now:

Error creating: Pod "testrc-" is forbidden: unable to validate against any security context constraint: [provider restricted: .spec.containers[0].securityContext.volumes[0]: invalid value 'emptyDir', Details: EmptyDir volumes are not allowed to be used]

I also adjusted the HostPath error just above where I got the bad example originally.

[dgoodwin]

Updated for review, separate commit for now to help reviewers, will squash before merge.

[ironcladlou]

LGTM, but probably need a final review from @liggitt or @pweil-.

[liggitt]

undo whitespace change

[liggitt]

split into three commits:

1. all v1beta3 changes in a commit titled UPSTREAM: <carry>: v1beta3 scc
2. all other Godep changes in a commit titled UPSTREAM: <carry>: scc
3. all other changes in a regular commit

[liggitt]

the incongruence makes me very sad, but I don't see a better way to keep the current behavior

[pweil-]

This makes me wonder if we should change the upstream proposal to be a blacklist of plugins rather than a whitelist.

[dgoodwin]

Updated into three separate commits, rolled back whitespace change to script I didn't end up modifying.

@liggitt I saw in email you were thinking about removing the change to the proposal .md file, but looks like you removed it. If github ate that somehow just say the word and I'll pull it out as well.

[dgoodwin]

UPDATE: github's stripping out the brackets thus why those commit messages didn't look right. I will add in carry as that looks like what most SCC commits are flagged, let me know if I've got that wrong.

[liggitt]

I'd still like @pweil-'s feedback, and if possible, to hold this until the rebase lands

[pweil-]

[test]

[dgoodwin]

Brought the volume scanning into one loop guarded on both conditions, and my apologies that's a legit test failure, I somehow slipped breakage in while refactoring during review. Should be healthy now.

[dgoodwin]

Updated with a master rebase and some things @dobbymoodge caught.

• renamed boolPtr helper to newBool to better match some other similar functions.
• added a missed assertion in the v1beta3 defaults_test.go.
• more closely match the spirit of some existing tests in both defaults_test.go files by explicitly setting an allowEmptyDirVolumePlugin value.

Still waiting on kube rebase for merge.

[dobbymoodge]

Shouldn't this be expectedAllowEmptyDir: false,, since default is true?

[dgoodwin]

This would then be identical to "preserve AllowEmptyDirVolumePlugin set to false" at the end, the only case missing was set to true and check true. Probably shouldn't be messing around with this test at all, it's named generically but I don't think it's actually meant for every property possible. Instead I should probably just add another test at the end for set true check true. Set nothing and expect true would be covered by all pre-existing tests.

[dgoodwin]

Rebased on top of new k8s, test added per comments above.

Should be GTG on my side @liggitt .

[dgoodwin]

re[test] please.

[liggitt]

...Child("volumes").Index(i)

[deads2k]

@deads2k, I assume we're not at the point of holding this out for the rebase yet?

No, the queue is still open. This could take a while.

[dgoodwin]

My local swagger diff problem was caused by a vim .swp file floating around in there.

However I am really not sure why it's still failing here.

$ sudo hack/verify-generated-swagger-spec.sh
===== Verifying API Swagger Spec =====
Generating a fresh spec...
Diffing current spec against freshly generated spec...
SUCCESS: Swagger spec up to date.

Travis is claiming a diff on a few fields unrelated to this PR... trying to sort it out.

[dgoodwin]

Alright that looks like a test infra flake. Re-[test] please.

[dgoodwin]

Re[test] please.

[dgoodwin]

[test]

[deads2k]

We're down to unit tests now. I'd like a hold on this until after the rebase (hopefully this week/early next week). Particularly since we're changing up how UPSTREAM patches are handled for rebases.

[dgoodwin]

Reapplied on top of the rebase but I don't think this is ready yet until master settles down, I'm seeing two issues that sound master related, one the default is coming out "false" suddenly, and two any attempt to edit an scc is met with a "apiVersion should not be changed" error. Will keep trying as master settles down.

[deads2k]

Reapplied on top of the rebase but I don't think this is ready yet until master settles down, I'm seeing two issues that sound master related, one the default is coming out "false" suddenly, and two any attempt to edit an scc is met with a "apiVersion should not be changed" error. Will keep trying as master settles down.

I'm not aware of either of those issues. Can you write them up with details so we can look at them?

[dgoodwin]

The only conflict was in the resource printer FWIW.

[dgoodwin]

@deads2k can do, one of them sounds similar to something in Clayton's email. I'll get issues up for both though one is kinda specific to my PR, not sure it can be reproduced otherwise. I'll start with the one that can though.

[dgoodwin]

The main one is reported in: #7085

I'll try to work a bit on the one that might be a problem with my PR and see if I can tell what it is or explain in a relevant way for an issue.

[openshift-bot]

Evaluated for origin test up to b6f497d

[openshift-bot]

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/968/)

[deads2k]

@pweil- I blocked this on the rebase last time. If its still good, he'd probably like a merge before the next one goes in :)

[pweil-]

I will give this one more review tomorrow and push it before I go any further on the next rebase.

[pweil-]

[merge]

[openshift-bot]

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/4923/) (Image: devenv-rhel7_3395)

[openshift-bot]

Evaluated for origin merge up to b6f497d