Support more security context properties behind a feature flag

[dprotaso]

Fixes #7924

Proposed Changes

• kubernetes.podspec-securitycontext feature flag now allows you to set securityContext options on the Pod & Container
• The Pod's context allows for runAsUser, runAsGroup, fsGroup runAsNonRoot, supplementalGroups
• The Container's context allows for runAsUser, runAsGroup, runAsNonRoot

Release Note

- `kubernetes.podspec-securitycontext` feature flag now allows you to set `securityContext` options on the Pod & Container
-  The Pod's context allows for `runAsUser`, `runAsGroup`, `fsGroup`  `runAsNonRoot`, `supplementalGroups`
-  The Container's context allows for `runAsUser`, `runAsGroup`, `runAsNonRoot`

/assign @julz

[knative-prow-robot]

@dprotaso: 2 warnings.

In response to this:

Fixes #7924

Proposed Changes

• kubernetes.podspec-securitycontext feature flag now allows you to set securityContext options on the Pod & Container
• The Pod's context allows for runAsUser, runAsGroup, fsGroup runAsNonRootUser
• The Container's context allows for runAsUser, runAsGroup, runAsNonRootUser

Release Note

- `kubernetes.podspec-securitycontext` feature flag now allows you to set `securityContext` options on the Pod & Container
-  The Pod's context allows for `runAsUser`, `runAsGroup`, `fsGroup`  `runAsNonRootUser`
-  The Container's context allows for `runAsUser`, `runAsGroup`, `runAsNonRootUser`

/assign @julz

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[dprotaso]

Marking it WIP since it's late and I want to go over it tomorrow - still worth putting up for review in the meantime

[julz]

so one thing I feel a little weird about here that feels worth discussing. This PR ends up not just exposing some new fields on the existing userContainer.SecurityContext (that bit is totally fine), it also exposes the pod security context to the user. Previously the user couldn't touch the pod's security spec, only the spec of their own containers.

I see why we need to do this -- the fsGroup is only present at the pod level, because volumes are shared -- but it seems like quite a big new bit of surface area. For example it means the Queue proxy must now run in whatever podSecurityContext the user supplies (modulo fields that it can explicitly override in the container securityContext).

Few potential upshots of that, off the top of my head:

• this will affect the ownership of the log volumes we mount in to queue proxy (it will arbitrarily chown them, I think..).
• any additional sidecars that are injected will also inherit these settings, unless overridden (e.g. Istio in the mesh case, I think).
• I've had a sneaky idea in the back of my head for a while that it'd be pretty nice to have a custom Queue Proxy that uses cgroups to pause/unpause the user container when there are no requests, for a lambda-like UX -- that likely requires giving QP elevated permissions. (etc)

Anyway, this may well be fine and reasonable, but I wanted to flag this up and make sure we explicitly think it through and decide it's ok.

[dprotaso]

(modulo fields that it can explicitly override in the container securityContext).

yeah for now that being the fsGroup property - (edit) and SupplementalGroups

this will affect the ownership of the log volumes we mount in to queue proxy (it will arbitrarily chown them, I think..).

This makes me think there is an existing issue with our /var/log logging. ie. user container writes logs to an emptyDir as root (unsure if uid mapping get applied). If the daemon set logging agent was running as non-root it could have permission problems reading the logs. Don't know the k8s internals deep enough.

Anyway, this may well be fine and reasonable, but I wanted to flag this up and make sure we explicitly think it through and decide it's ok.

I think this discussion is good and surfaces some potential upstream requirements in K8s (ie. does fsGroup property on the container security context make sense). For some existing pod-spec feature flags I don't think we intent to every change the default from Disabled - this flag may fall into the same class. Thus we should aim to at least let document to operators the issues you've surfaced.

This reminds me to update the _example key of the config map

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso

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:

• config/OWNERS [dprotaso]
• pkg/apis/OWNERS [dprotaso]

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

[knative-metrics-robot]

The following is the coverage report on the affected files.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/serving/fieldmask.go	97.9%	98.1%	0.1
pkg/apis/serving/k8s_validation.go	99.3%	99.3%	0.1

[dprotaso]

/assign @mattmoor

[mattmoor]

/lgtm
/hold

Holding in case @julz has anything further, but this LGTM esp. off by default.

[julz]

/lgtm too
/unhold

[knative-test-reporter-robot]

The following jobs failed:

Test name	Triggers	Retries
pull-knative-serving-upgrade-tests		0/3

Failed non-flaky tests preventing automatic retry of pull-knative-serving-upgrade-tests:

test/upgrade.TestCreateNewServicePostUpgrade
test/upgrade.TestProbe

[dprotaso]

/retest