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

Add "MayRunAs" value among other GroupStrategies #65135

Merged
merged 1 commit into from Sep 30, 2018

Conversation

@stlaz
Contributor

stlaz commented Jun 15, 2018

What this PR does / why we need it:
Adds "MayRunAs" value among other group strategies. This strategy
allows to define a certain range of GIDs for FSGroupStrategy and
SupplementalGroupStrategy in a PSP.

This new strategy works similarly to the "MustRunAs" one, except that
when no GID is specified in a pod/container security context then no
GID is generated for the respective containers.

Which issue(s) this PR fixes
Resolves #56173

Release note:

PodSecurityPolicy objects now support a `MayRunAs` rule for `fsGroup` and `supplementalGroups` options. This allows specifying ranges of allowed GIDs for pods/containers without forcing a default GID the way `MustRunAs` does. This means that a container to which such a policy applies to won't use any fsGroup/supplementalGroup GID if not explicitly specified, yet a specified GID must still fall in the GID range according to the policy.
@php-coder

This comment has been minimized.

Show comment
Hide comment
@php-coder

php-coder Jun 15, 2018

Contributor

/sig auth
/ok-to-test

PTAL @kubernetes/sig-auth-api-reviews

CC @simo5

Contributor

php-coder commented Jun 15, 2018

/sig auth
/ok-to-test

PTAL @kubernetes/sig-auth-api-reviews

CC @simo5

@php-coder

This comment has been minimized.

Show comment
Hide comment
@php-coder

php-coder Jun 15, 2018

Contributor

@stlaz One more place to update:

supplementalGroupsRules := []policy.SupplementalGroupsStrategyType{
policy.SupplementalGroupsStrategyRunAsAny,
policy.SupplementalGroupsStrategyMustRunAs,
}
psp.SupplementalGroups.Rule = supplementalGroupsRules[c.Rand.Intn(len(supplementalGroupsRules))]
fsGroupRules := []policy.FSGroupStrategyType{
policy.FSGroupStrategyMustRunAs,
policy.FSGroupStrategyRunAsAny,
}

Contributor

php-coder commented Jun 15, 2018

@stlaz One more place to update:

supplementalGroupsRules := []policy.SupplementalGroupsStrategyType{
policy.SupplementalGroupsStrategyRunAsAny,
policy.SupplementalGroupsStrategyMustRunAs,
}
psp.SupplementalGroups.Rule = supplementalGroupsRules[c.Rand.Intn(len(supplementalGroupsRules))]
fsGroupRules := []policy.FSGroupStrategyType{
policy.FSGroupStrategyMustRunAs,
policy.FSGroupStrategyRunAsAny,
}

@php-coder

This comment has been minimized.

Show comment
Hide comment
@php-coder

php-coder Jun 15, 2018

Contributor

@kubernetes/sig-auth-api-reviews Do we need to update PSP in extensions API group?

Contributor

php-coder commented Jun 15, 2018

@kubernetes/sig-auth-api-reviews Do we need to update PSP in extensions API group?

@php-coder

This comment has been minimized.

Show comment
Hide comment
@php-coder

php-coder Jun 15, 2018

Contributor

/assign @tallclair

Contributor

php-coder commented Jun 15, 2018

/assign @tallclair

@stlaz

This comment has been minimized.

Show comment
Hide comment
@stlaz

stlaz Jun 15, 2018

Contributor

Updated the patch to fix verify-basel failure and to address the PR comments.

Contributor

stlaz commented Jun 15, 2018

Updated the patch to fix verify-basel failure and to address the PR comments.

@stlaz stlaz changed the title from [WIP] Add "MayRunAs" value among other GroupStrategies to Add "MayRunAs" value among other GroupStrategies Jun 18, 2018

@stlaz

This comment has been minimized.

Show comment
Hide comment
@stlaz

stlaz Jun 18, 2018

Contributor

This change introduces backward incompatibility since the "MayRunAs" value to the groups is unknown to previous API versions. If we convert this to "MustRunAs" internally, we wouldn't be able to later get the information that it's actually "MayRunAs".
What would be the preferred way to handle such an issue? Is the rollback desirable here? Does @tallclair have an input on this?

Contributor

stlaz commented Jun 18, 2018

This change introduces backward incompatibility since the "MayRunAs" value to the groups is unknown to previous API versions. If we convert this to "MustRunAs" internally, we wouldn't be able to later get the information that it's actually "MayRunAs".
What would be the preferred way to handle such an issue? Is the rollback desirable here? Does @tallclair have an input on this?

@krmayankk

This comment has been minimized.

Show comment
Hide comment
@krmayankk

krmayankk Jun 20, 2018

Contributor

@tallclair @php-coder i believe the MayRunAs would also be needed by RunAsGroup field. I will include that in the PSP changes for RunAsGroup in my next PR.

Contributor

krmayankk commented Jun 20, 2018

@tallclair @php-coder i believe the MayRunAs would also be needed by RunAsGroup field. I will include that in the PSP changes for RunAsGroup in my next PR.

@php-coder

This comment has been minimized.

Show comment
Hide comment
@php-coder

php-coder Jun 26, 2018

Contributor

PTAL @tallclair

Contributor

php-coder commented Jun 26, 2018

PTAL @tallclair

@tallclair

This change introduces backward incompatibility since the "MayRunAs" value to the groups is unknown to previous API versions.

Backwards incompatibility means that a resource that was valid in a previous version is no longer valid (or behaves differently), which isn't the case here. What you're refering to is forwards compatibility, and that's not something Kubernetes provides, so this is a non-issue.

@stlaz

This comment has been minimized.

Show comment
Hide comment
@stlaz

stlaz Jul 17, 2018

Contributor

Thank you @tallclair for your comments, my apologies that it took me so long to react in the PR accordingly.
I added a bunch of tests to podsecuritypolicy/provider_test.go and removed most of mayrunas_test.go::TestMayRunAsGenerate (kept only one to test the behavior is correct).

Contributor

stlaz commented Jul 17, 2018

Thank you @tallclair for your comments, my apologies that it took me so long to react in the PR accordingly.
I added a bunch of tests to podsecuritypolicy/provider_test.go and removed most of mayrunas_test.go::TestMayRunAsGenerate (kept only one to test the behavior is correct).

@tallclair

A few nits, and please fix the test. Everything else lgtm.

@tallclair

This comment has been minimized.

Show comment
Hide comment
@tallclair

tallclair Jul 17, 2018

Member

/assign @erictune
For sig-apps-api approval. Why does the policy group require apps approval? Should we change it to sig-auth-api-approvers, or just api-approvers? /cc @liggitt

Member

tallclair commented Jul 17, 2018

/assign @erictune
For sig-apps-api approval. Why does the policy group require apps approval? Should we change it to sig-auth-api-approvers, or just api-approvers? /cc @liggitt

@liggitt

This comment has been minimized.

Show comment
Hide comment
@liggitt

liggitt Aug 24, 2018

Member

/hold
for resolution of requiring a group to match all ranges. LGTM otherwise

Member

liggitt commented Aug 24, 2018

/hold
for resolution of requiring a group to match all ranges. LGTM otherwise

@liggitt liggitt modified the milestones: v1.12, v1.13 Sep 5, 2018

@k8s-ci-robot k8s-ci-robot added sig/architecture and removed lgtm labels Sep 6, 2018

@stlaz

This comment has been minimized.

Show comment
Hide comment
@stlaz

stlaz Sep 6, 2018

Contributor

/retest

Contributor

stlaz commented Sep 6, 2018

/retest

@stlaz

This comment has been minimized.

Show comment
Hide comment
@stlaz

stlaz Sep 6, 2018

Contributor

The "fall in all groups" issue should now be resolved, a positive multi-ranges test was added on top of the mayrunas tests.

Contributor

stlaz commented Sep 6, 2018

The "fall in all groups" issue should now be resolved, a positive multi-ranges test was added on top of the mayrunas tests.

@liggitt

This comment has been minimized.

Show comment
Hide comment
@liggitt

liggitt Sep 6, 2018

Member

thanks. both tallclair and I are out of the office at the moment, but will pick this up when master reopens for 1.13.

Member

liggitt commented Sep 6, 2018

thanks. both tallclair and I are out of the office at the moment, but will pick this up when master reopens for 1.13.

@stlaz

This comment has been minimized.

Show comment
Hide comment
@stlaz

stlaz Sep 7, 2018

Contributor

Thank you. I'm not sure exactly why the verify test does not pass since make verify seems to be running fine on my system. The only guess I have was my upgrade of golang from 1.10 to 1.11 🤔

Contributor

stlaz commented Sep 7, 2018

Thank you. I'm not sure exactly why the verify test does not pass since make verify seems to be running fine on my system. The only guess I have was my upgrade of golang from 1.10 to 1.11 🤔

@stlaz

This comment has been minimized.

Show comment
Hide comment
@stlaz

stlaz Sep 14, 2018

Contributor

/retest

Contributor

stlaz commented Sep 14, 2018

/retest

@stlaz

This comment has been minimized.

Show comment
Hide comment
@stlaz

stlaz Sep 25, 2018

Contributor

@tallclair Test cases for multiple groups added.

Contributor

stlaz commented Sep 25, 2018

@tallclair Test cases for multiple groups added.

@tallclair

A couple nits, but LGTM. Please squash.

Add "MayRunAs" value among other GroupStrategies
Adds "MayRunAs" value among other group strategies. This strategy
allows to define a certain range of GIDs for FSGroupStrategy and
SupplementalGroupStrategy in a PSP.

This new strategy works similarly to the "MustRunAs" one, except that
when no GID is specified in a pod/container security context then no
GID is generated for the respective containers.

Resolves #56173
@stlaz

This comment has been minimized.

Show comment
Hide comment
@stlaz

stlaz Sep 27, 2018

Contributor

Comments addressed + squashed.

Contributor

stlaz commented Sep 27, 2018

Comments addressed + squashed.

@tallclair

This comment has been minimized.

Show comment
Hide comment
@tallclair

tallclair Sep 27, 2018

Member

/lgtm
/hold cancel

Member

tallclair commented Sep 27, 2018

/lgtm
/hold cancel

@k8s-ci-robot k8s-ci-robot added lgtm and removed do-not-merge/hold labels Sep 27, 2018

@liggitt

This comment has been minimized.

Show comment
Hide comment
@liggitt

liggitt Sep 30, 2018

Member

/approve

Member

liggitt commented Sep 30, 2018

/approve

@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Sep 30, 2018

Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, stlaz, 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

Contributor

k8s-ci-robot commented Sep 30, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, stlaz, 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-ci-robot k8s-ci-robot merged commit 0b3a5cd into kubernetes:master Sep 30, 2018

18 checks passed

cla/linuxfoundation stlaz authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
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-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
@yue9944882

This comment has been minimized.

Show comment
Hide comment
@yue9944882

yue9944882 Oct 11, 2018

Contributor

hi folks, why not syncing these new internal definitions to the external repo? i'm afraid currently we can't specify the new strategy w/ client-go. also the "MayRunAs" will be an undefined value when deserialized into external types.

ref: #69704

Contributor

yue9944882 commented Oct 11, 2018

hi folks, why not syncing these new internal definitions to the external repo? i'm afraid currently we can't specify the new strategy w/ client-go. also the "MayRunAs" will be an undefined value when deserialized into external types.

ref: #69704

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment