API Changes for RunAsGroup by krmayankk · Pull Request #52077 · kubernetes/kubernetes

krmayankk · 2017-09-07T08:31:12Z

First set of api changes for feature kubernetes/community#756

Add ability to control primary GID of containers through Pod Spec and PodSecurityPolicy

k8s-github-robot · 2017-09-07T08:31:39Z

Adding do-not-merge/release-note-label-needed because the release note process has not been followed.
One of the following labels is required "release-note", "release-note-action-required", or "release-note-none".
Please see: https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md#write-release-notes-if-needed.

dashpole · 2017-09-07T16:47:39Z

/unassign
/assign @pmorie

cmluciano

Can you please separate this into controller changes, api changes, generated code? I'm having a hard time finding all the parts that changed while reviewing.

The test failures that you noted are due to the roundtrip fuzzer receiving a nil value when it injects something in.

This can happen for a number of reasons:

conversion code is not accurate (if manually crafted)
fuzzer is filling the value with an improperly formatted type (requires changes to the fuzzer)
new required fields are added without defaults (doesn't seem to be the case here)
not all supported APIs (v1beta1,v1,etc) have the necessary changes in their types

object.Spec.Template.Spec.Containers[0].SecurityContext.RunAsGroup:
	  a: (*int64)(0xc4207c10d8)
	  b: <nil>

liggitt · 2017-09-24T01:40:01Z

pkg/apis/extensions/types.go

+const (
+	// container must run as a particular gid.
+	RunAsGroupStrategyMustRunAs RunAsGroupStrategy = "MustRunAs"
+	// container must run as a non-root uid


liggitt · 2017-09-24T01:40:09Z

pkg/apis/extensions/types.go

+	RunAsGroupStrategyMustRunAs RunAsGroupStrategy = "MustRunAs"
+	// container must run as a non-root uid
+	RunAsGroupStrategyMustRunAsNonRoot RunAsGroupStrategy = "MustRunAsNonRoot"
+	// container may make requests for any uid.


liggitt · 2017-09-24T01:41:01Z

pkg/apis/extensions/v1beta1/conversion.go

 	return autoConvert_extensions_PodSecurityPolicySpec_To_v1beta1_PodSecurityPolicySpec(in, out, s)
 }
+
+func Convert_v1beta1_PodSecurityPolicySpec_To_extensions_PodSecurityPolicySpec(in *extensionsv1beta1.PodSecurityPolicySpec, out *extensions.PodSecurityPolicySpec, s conversion.Scope) error {


why is this added as a manual conversion?

frankly i dont know how to auto generate it , i ran all hack/updatexxx and didn't see it generated. if you have suggestions let me know

@krmayankk what is the output from the generator when you tried to generate the conversions?

if you remove this, what fails? if all the manual conversion is doing is calling the autoConvert_... function, it shouldn't be needed

It still troubles me that this is a manual conversion. That seems not-right to me.

liggitt · 2017-09-24T01:41:49Z

pkg/apis/extensions/validation/validation.go

+		allErrs = append(allErrs, field.NotSupported(fldPath.Child("rule"), runAsGroup.Rule, supportedRunAsGroupRules.List()))
+	}
+
+	// validate range settings


is it valid to specify ranges with RunAsGroupStrategyMustRunAsNonRoot or RunAsGroupStrategyRunAsAny?

hmm , its not valid i think. The check is missing for RunAsUser as well. I will add a check if Ranges is not nil, the strategy must be MustRunAs . Adding @pmorie and @pweil- in case i am missing something

non-root and any do not support ranges for UID at this point cc @pmorie

@krmayankk sounds like we need to validate that you don't get a range with a policy that doesn't support it.

added the check

liggitt · 2017-09-24T01:43:05Z

pkg/kubelet/apis/cri/v1alpha1/runtime/api.pb.go

+	RunAsGroup *Int64Value `protobuf:"bytes,4,opt,name=run_as_group,json=runAsGroup" json:"run_as_group,omitempty"`
 	// If set, the root filesystem of the sandbox is read-only.
-	ReadonlyRootfs bool `protobuf:"varint,4,opt,name=readonly_rootfs,json=readonlyRootfs,proto3" json:"readonly_rootfs,omitempty"`
+	ReadonlyRootfs bool `protobuf:"varint,5,opt,name=readonly_rootfs,json=readonlyRootfs,proto3" json:"readonly_rootfs,omitempty"`


doesn't changing proto tags here break compatibility?

liggitt · 2017-09-24T01:45:06Z

staging/src/k8s.io/api/core/v1/types.go

+	// PodSecurityContext, the value specified in SecurityContext takes precedence
+	// for that container.
+	// +optional
+	RunAsGroup *int64 `json:"runAsGroup,omitempty" protobuf:"varint,3,opt,name=runAsGroup"`


have to take an available proto tag at the end of the range... reassigning existing numbers breaks the ability to read in previously persisted data

liggitt · 2017-09-24T01:47:30Z

staging/src/k8s.io/api/core/v1/types.go

+	// May also be set in PodSecurityContext.  If set in both SecurityContext and
+	// PodSecurityContext, the value specified in SecurityContext takes precedence.
+	// +optional
+	RunAsGroup *int64 `json:"runAsGroup,omitempty" protobuf:"varint,5,opt,name=runAsGroup"`


same here, use a free proto tag from the end, don't reassign existing tags

liggitt · 2017-09-24T01:53:00Z

staging/src/k8s.io/api/extensions/v1beta1/types.go

 	// runAsUser is the strategy that will dictate the allowable RunAsUser values that may be set.
 	RunAsUser RunAsUserStrategyOptions `json:"runAsUser" protobuf:"bytes,11,opt,name=runAsUser"`
+	// runAsGroup is the strategy that will dictate the allowable RunAsGroup values that may be set.
+	RunAsGroup RunAsGroupStrategyOptions `json:"runAsGroup" protobuf:"bytes,12,opt,name=runAsGroup"`


liggitt · 2017-09-24T01:54:30Z

pkg/apis/extensions/types.go

 	// RunAsUser is the strategy that will dictate the allowable RunAsUser values that may be set.
 	RunAsUser RunAsUserStrategyOptions
+	// RunAsGroup is the strategy that will dictate the allowable RunAsGroup values that may be set.
+	RunAsGroup RunAsGroupStrategyOptions


what's the default? (seems like it would have to be RunAsGroupStrategyRunAsAny for compatibility?)

yes thats my understanding.

then that should be populated in SetDefaults_PodSecurityPolicySpec

liggitt · 2018-02-27T21:44:50Z

pkg/apis/core/validation/validation.go

same question here, if RunAsGroup is non-nil, do we require RunAsUser to be non-nil? what if RunAsUser is set on the pod security context, and RunAsGroup is set on the container security context?

same RunAsGroup can be non nil and RunAsUser can be nil

Is that because the requirement for runasuser to be non-nil is docker specific? If other runtimes could allow that, it's fine to skip validating that here.

the validation is saying that if RunAsGroup is specified at all, then it should be a valid group id. Not sure i follow your comment. Its not validating whether its specified or not. The first if is validating whether its specified at all, the second if is validating that if its specified its a valid group id

I was wondering if we wanted to validate that both run as user and run as group were set, because the docker runtime will error if you set group without setting user

yes other implementations could allow that and the limitation is docker specific. for e.g. rkt allows setting both independently

There's a validation for it in dockershim - it might be prudent to push that up to the API validation for now.

tallclair

This is really hard to review. I suggest:

Split the PodSecurityPolicy changes into a separate PR
Put the generated files into a separate commit.

tallclair · 2018-02-27T23:22:04Z

staging/src/k8s.io/api/core/v1/types.go

Is this actually read anywhere?

no, this is not implemented, only the RunAsGroup field is. there is a feature gate to disable it

tallclair · 2018-02-27T23:30:47Z

pkg/kubelet/dockershim/security_context.go

nit: errors start with lowercase

RunAsGroup is not a string, I think you can just leave it out here.

tallclair · 2018-02-27T23:43:09Z

test/e2e/auth/pod_security_policy.go

Leave this out. It should be optional.

tallclair · 2018-02-27T23:43:13Z

test/e2e/framework/psp_util.go

tallclair · 2018-02-27T23:46:56Z

staging/src/k8s.io/api/extensions/v1beta1/types.go

AFAICT this is never being enforced?

only the securityContext is being enforced in Container and pod spec. PSP is not being enforced and hence i added the feature gates and disabled them by default. That was the comment from @liggitt which i have been taking care

tallclair · 2018-02-27T23:50:16Z

I don't see this making 1.10. If you disagree, please file an extension request.

liggitt · 2018-02-28T02:05:48Z

discussed with @tallclair and @krmayankk. Rather than merge API changes for PSP and runAsNonRootGroup before their implementation, we'd prefer limiting this PR to the single runAsGroup field in PodSecurityContext and SecurityContext, which is implemented all the way through CRI and has backing e2e tests.

These fields would still be feature gated and disabled by default, but would allow CRI implementations to integrate/test with this feature in the 1.10 timeframe (and have an e2e test that would exercise their integration).

The PSP and runAsNonRoot fields and enforcement would be continued in 1.11, and would be required to be completed before enabling the RunAsGroup feature by default.

A 1 day exception to remove the PSP and runAsNonRootGroup fields from this PR requested at https://groups.google.com/forum/#!msg/kubernetes-milestone-burndown/kq4zhEI_c-I/vLVpJlMMBAAJ

liggitt · 2018-02-28T17:51:00Z

staging/src/k8s.io/api/core/v1/types.go

this doc change is spurious with runAsNonRootGroup removed

liggitt · 2018-02-28T17:52:27Z

staging/src/k8s.io/api/core/v1/types.go

@pmorie is "Defaults to group specified in image metadata if unspecified." accurate? or does it default to gid 0?

I see this is described more fully in another field: "Defaults to group specified in image metadata if unspecified. If group is not specified in image metadata, the default is GID 0."

is that runtime-specific, or an API guarantee?

Pretty sure that's runtime specific. I would just say something along the lines of "Uses runtime default if unset".

tallclair · 2018-02-28T18:39:13Z

pkg/apis/extensions/validation/validation.go

Revert this file

tallclair · 2018-02-28T18:39:26Z

pkg/apis/extensions/validation/validation_test.go

Revert this file

tallclair · 2018-02-28T18:39:38Z

pkg/apis/extensions/validation/BUILD

Revert this file

tallclair · 2018-02-28T18:39:49Z

pkg/apis/extensions/v1beta1/conversion.go

tallclair · 2018-02-28T18:40:02Z

pkg/apis/extensions/types.go

Revert this file

still outstanding

tallclair · 2018-02-28T18:45:29Z

pkg/kubelet/dockershim/security_context.go

I don't think this should be an error.

tallclair · 2018-02-28T18:46:49Z

pkg/kubelet/dockershim/security_context.go

nit: can you just do

user = fmt.Sprintf("%s:%d", config.User, sc.GetRunAsGroup().Value)

?

tallclair · 2018-02-28T18:48:28Z

staging/src/k8s.io/api/core/v1/types.go

nit: prefer "The GID to run the container's init process"

(entrypoint is a docker term)

this matches RunAsUser doc... I'd leave it for now

tallclair · 2018-02-28T18:49:38Z

staging/src/k8s.io/api/core/v1/types.go

ditto re: entrypoint vs init

tallclair · 2018-02-28T18:51:41Z

staging/src/k8s.io/api/core/v1/types.go

Pretty sure that's runtime specific. I would just say something along the lines of "Uses runtime default if unset".

liggitt · 2018-02-28T21:03:46Z

pkg/apis/core/types.go

this should match the other RunAsGroup doc, not say the default is GID 0

liggitt · 2018-02-28T21:46:28Z

moving back into the milestone per granted exception

liggitt · 2018-02-28T21:46:35Z

/status approved-for-milestone

tallclair

/cc @yujuhong for CRI changes

I skimmed this and it looks ok (1 more file to revert). I'll make another pass after dinner and approve if everything looks ok.

tallclair · 2018-03-01T01:35:06Z

pkg/apis/extensions/types.go

please revert this file.

k8s-ci-robot · 2018-03-01T01:36:45Z

@tallclair: GitHub didn't allow me to request PR reviews from the following users: CRI, changes, for.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/cc @yujuhong for CRI changes

I skimmed this and it looks ok (1 more file to revert). I'll make another pass after dinner and approve if everything looks ok.

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.

liggitt · 2018-03-01T03:00:57Z

fyi, I pushed an update to the branch restoring the hack/gen-swagger-doc/example-output/definitions.html file which was unrelated to the changes in this PR:

$ git diff krmayankk/runas --numstat
5982    0       hack/gen-swagger-doc/example-output/definitions.html

$ git push --force-with-lease krmayankk HEAD:runas
To github.com:krmayankk/kubernetes.git
 + 1d5c33287c...7e31ffa0c9 HEAD -> runas (forced update)

API and test changes LGTM
/lgtm

liggitt · 2018-03-01T03:04:39Z

@tallclair has approval on pkg/kubelet changes

pkg approver needed for RunAsGroup feature gate

tallclair

Noticed one more issue with the validation. Everything else LGTM.

Please ping me on slack once you've fixed the validation.

tallclair · 2018-03-01T03:47:21Z

pkg/apis/core/validation/validation.go

The error handling here should match the error handling in the pod version - loop over the error messages.

liggitt · 2018-03-01T14:22:13Z

/lgtm

tallclair · 2018-03-01T17:42:36Z

/lgtm

liggitt · 2018-03-01T17:45:43Z

/assign @smarterclayton
Since you were most active in kubernetes/community#756
For approval of RunAsGroup feature gate

smarterclayton · 2018-03-01T20:56:26Z

/approve

Based on sig approval

k8s-ci-robot · 2018-03-01T20:57:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: krmayankk, liggitt, pmorie, smarterclayton, tallclair

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~api/OWNERS~~ [liggitt,smarterclayton]
~~docs/api-reference/OWNERS~~ [liggitt,smarterclayton]
~~pkg/OWNERS~~ [smarterclayton]
~~staging/src/k8s.io/api/OWNERS~~ [liggitt,smarterclayton]
~~test/e2e/node/OWNERS~~ [liggitt,smarterclayton,tallclair]

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

k8s-github-robot · 2018-03-01T22:41:13Z

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

k8s-github-robot · 2018-03-01T23:23:51Z

Automatic merge from submit-queue (batch tested with PRs 52077, 60456, 60591). If you want to cherry-pick this change to another branch, please follow the instructions here.

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 Sep 7, 2017

k8s-github-robot assigned dashpole and brendandburns Sep 7, 2017

k8s-github-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Sep 7, 2017

k8s-github-robot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Sep 7, 2017

krmayankk force-pushed the runas branch from fafb841 to efbda83 Compare September 7, 2017 09:08

k8s-ci-robot assigned pmorie and unassigned dashpole Sep 7, 2017

krmayankk force-pushed the runas branch from efbda83 to a457cb9 Compare September 8, 2017 08:07

cmluciano reviewed Sep 12, 2017

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 23, 2017

This was referenced Sep 23, 2017

Add a RunAsGroup field to SecurityContext #13868

Closed

Provide possibility to specify the group a container is run as #22179

Closed

liggitt reviewed Sep 24, 2017

View reviewed changes

krmayankk force-pushed the runas branch from 98a10af to 5a4aa0b Compare September 25, 2017 06:22

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 25, 2017

krmayankk force-pushed the runas branch from 5a4aa0b to be6f22e Compare September 25, 2017 06:33

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 26, 2017

krmayankk force-pushed the runas branch from be6f22e to 75f573a Compare September 26, 2017 07:25

liggitt reviewed Feb 27, 2018

View reviewed changes

tallclair reviewed Feb 27, 2018

View reviewed changes

liggitt reviewed Feb 28, 2018

View reviewed changes

staging/src/k8s.io/api/core/v1/types.go Outdated

Copy link
Copy Markdown

Member

liggitt Feb 28, 2018

Choose a reason for hiding this comment

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

this doc change is spurious with runAsNonRootGroup removed

liggitt reviewed Feb 28, 2018

View reviewed changes

tallclair reviewed Feb 28, 2018

View reviewed changes

liggitt reviewed Feb 28, 2018

View reviewed changes

pkg/apis/core/types.go Outdated

Copy link
Copy Markdown

Member

liggitt Feb 28, 2018

Choose a reason for hiding this comment

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

this should match the other RunAsGroup doc, not say the default is GID 0

tallclair reviewed Mar 1, 2018

View reviewed changes

pkg/apis/extensions/types.go Outdated

Copy link
Copy Markdown

Member

tallclair Mar 1, 2018

Choose a reason for hiding this comment

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

please revert this file.

tallclair reviewed Mar 1, 2018

View reviewed changes

API Changes for RunAsGroup and Implementation and e2e

b888415

krmayankk mentioned this pull request Mar 12, 2018

add UT for validatePSPRunAsUser #61030

Merged

This was referenced Mar 30, 2018

Add RunAsGroup support. containerd/cri#710

Merged

Fix dockershim CreateContainer error handling. #61965

Merged

krmayankk mentioned this pull request Apr 6, 2018

NonRootGroup API Changes and Implementation #62216

Closed

Quentin-M mentioned this pull request Apr 12, 2018

Kubernetes 1.10+ supports RunAsGroup / RunAsNonRootGroup prometheus-operator/prometheus-operator#1217

Closed

dtaniwaki mentioned this pull request Jun 6, 2018

Support runAsGroup jupyterhub/kubespawner#181

Merged

krmayankk mentioned this pull request Oct 18, 2018

add myself to node reviewers #69997

Merged

Conversation

krmayankk commented Sep 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-github-robot commented Sep 7, 2017

Uh oh!

dashpole commented Sep 7, 2017

Uh oh!

cmluciano left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krmayankk Sep 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liggitt Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tallclair left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

krmayankk commented Sep 7, 2017 •

edited

Loading

cmluciano left a comment •

edited

Loading

krmayankk Sep 25, 2017 •

edited

Loading

liggitt Feb 27, 2018 •

edited

Loading