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

API Changes for RunAsGroup #52077

Merged
merged 1 commit into from Mar 1, 2018

Conversation

@krmayankk
Copy link
Contributor

krmayankk commented Sep 7, 2017

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

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Sep 7, 2017

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

This comment has been minimized.

Copy link
Contributor

dashpole commented Sep 7, 2017

/unassign
/assign @pmorie

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

@krmayankk krmayankk force-pushed the krmayankk:runas branch from efbda83 to a457cb9 Sep 8, 2017

@cmluciano
Copy link
Member

cmluciano left a comment

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>
const (
// container must run as a particular gid.
RunAsGroupStrategyMustRunAs RunAsGroupStrategy = "MustRunAs"
// container must run as a non-root uid

This comment has been minimized.

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

This comment has been minimized.

@liggitt
@@ -514,3 +515,7 @@ func Convert_networking_NetworkPolicyList_To_v1beta1_NetworkPolicyList(in *netwo
func Convert_extensions_PodSecurityPolicySpec_To_v1beta1_PodSecurityPolicySpec(in *extensions.PodSecurityPolicySpec, out *extensionsv1beta1.PodSecurityPolicySpec, s conversion.Scope) error {
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 {

This comment has been minimized.

@liggitt

liggitt Sep 24, 2017

Member

why is this added as a manual conversion?

This comment has been minimized.

@krmayankk

krmayankk Sep 25, 2017

Author Contributor

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

This comment has been minimized.

@pmorie

pmorie Jan 4, 2018

Member

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

This comment has been minimized.

@liggitt

liggitt Feb 5, 2018

Member

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

This comment has been minimized.

@pmorie

pmorie Feb 11, 2018

Member

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

allErrs = append(allErrs, field.NotSupported(fldPath.Child("rule"), runAsGroup.Rule, supportedRunAsGroupRules.List()))
}

// validate range settings

This comment has been minimized.

@liggitt

liggitt Sep 24, 2017

Member

is it valid to specify ranges with RunAsGroupStrategyMustRunAsNonRoot or RunAsGroupStrategyRunAsAny?

This comment has been minimized.

@krmayankk

krmayankk Sep 25, 2017

Author Contributor

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

This comment has been minimized.

@pweil-

pweil- Sep 25, 2017

Member

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

This comment has been minimized.

@pmorie

pmorie Jan 4, 2018

Member

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

This comment has been minimized.

@krmayankk

krmayankk Jan 9, 2018

Author Contributor

added the check

// 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"`

This comment has been minimized.

@liggitt

liggitt Sep 24, 2017

Member

doesn't changing proto tags here break compatibility?

This comment has been minimized.

@krmayankk

krmayankk Sep 25, 2017

Author Contributor

fixing now

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

This comment has been minimized.

@liggitt

liggitt Sep 24, 2017

Member

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

This comment has been minimized.

@krmayankk

krmayankk Sep 25, 2017

Author Contributor

fixing now

// 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"`

This comment has been minimized.

@liggitt

liggitt Sep 24, 2017

Member

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

@@ -959,28 +959,30 @@ type PodSecurityPolicySpec struct {
SELinux SELinuxStrategyOptions `json:"seLinux" protobuf:"bytes,10,opt,name=seLinux"`
// 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"`

This comment has been minimized.

@liggitt

liggitt Sep 24, 2017

Member

proto tag

@@ -920,6 +920,8 @@ type PodSecurityPolicySpec struct {
SELinux SELinuxStrategyOptions
// 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

This comment has been minimized.

@liggitt

liggitt Sep 24, 2017

Member

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

This comment has been minimized.

@krmayankk

krmayankk Sep 25, 2017

Author Contributor

yes thats my understanding.

This comment has been minimized.

@liggitt

liggitt Feb 5, 2018

Member

then that should be populated in SetDefaults_PodSecurityPolicySpec

@krmayankk krmayankk force-pushed the krmayankk:runas branch from 98a10af to 5a4aa0b Sep 25, 2017

@krmayankk krmayankk force-pushed the krmayankk:runas branch from 5a4aa0b to be6f22e Sep 25, 2017

@krmayankk krmayankk force-pushed the krmayankk:runas branch from be6f22e to 75f573a Sep 26, 2017

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Mar 1, 2018

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

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 1, 2018

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Mar 1, 2018

@tallclair has approval on pkg/kubelet changes

pkg approver needed for RunAsGroup feature gate

@tallclair
Copy link
Member

tallclair left a comment

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

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

@@ -4921,6 +4927,12 @@ func ValidateSecurityContext(sc *core.SecurityContext, fldPath *field.Path) fiel
}
}

if sc.RunAsGroup != nil {
if validation.IsValidGroupID(*sc.RunAsGroup) != nil {

This comment has been minimized.

@tallclair

tallclair Mar 1, 2018

Member

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

@krmayankk krmayankk force-pushed the krmayankk:runas branch from 7e31ffa to 54161b0 Mar 1, 2018

@k8s-github-robot k8s-github-robot removed the lgtm label Mar 1, 2018

@krmayankk krmayankk force-pushed the krmayankk:runas branch from 54161b0 to b888415 Mar 1, 2018

@kubernetes kubernetes deleted a comment from krmayankk Mar 1, 2018

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Mar 1, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 1, 2018

@tallclair

This comment has been minimized.

Copy link
Member

tallclair commented Mar 1, 2018

/lgtm

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Mar 1, 2018

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

@smarterclayton

This comment has been minimized.

Copy link
Contributor

smarterclayton commented Mar 1, 2018

/approve

Based on sig approval

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Mar 1, 2018

[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

Needs approval from an approver in each of these files:

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

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Mar 1, 2018

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

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Mar 1, 2018

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-github-robot k8s-github-robot merged commit 16980f2 into kubernetes:master Mar 1, 2018

11 of 14 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce
Details
pull-kubernetes-e2e-gce Job triggered.
Details
pull-kubernetes-verify Job triggered.
Details
cla/linuxfoundation krmayankk authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details

k8s-github-robot pushed a commit that referenced this pull request Mar 21, 2018

Kubernetes Submit Queue
Merge pull request #61030 from krmayankk/utrunas
Automatic merge from submit-queue (batch tested with PRs 59536, 61104, 61030, 59013, 61169). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

add UT for validatePSPRunAsUser

This PR is the first in a series which will continue to finish the work started in #52077 to add RunAsGroup feature in Pod and PSP

This PR simply adds a UT for RunAsUser validation in PSP

@pmorie @tallclair

k8s-github-robot pushed a commit that referenced this pull request Mar 31, 2018

Kubernetes Submit Queue
Merge pull request #61965 from Random-Liu/fix-error-handling
Automatic merge from submit-queue (batch tested with PRs 61929, 61965). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Fix dockershim CreateContainer error handling.

Found this bug in CRI validation test kubernetes-sigs/cri-tools#282.

In #52077, we expect container creation to return error if `RunAsGroup` is specified without `RunAsUser` or `RunAsUsername`. However, the error returned is not handled.

@krmayankk This is only a corner case. Does this worth cherry-pick into 1.10?
@kubernetes/sig-node-bugs 

Signed-off-by: Lantao Liu <lantaol@google.com>

**Release note**:

```release-note
none
```

@dtaniwaki dtaniwaki referenced this pull request Jun 6, 2018

Merged

Support runAsGroup #181

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.