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

Kubelet & API changes for Windows GMSA support #75459

Merged
merged 4 commits into from May 21, 2019

Conversation

@wk8
Copy link
Member

commented Mar 19, 2019

What type of PR is this?
/kind feature

What this PR does / why we need it:
This patch comprises the kubelet & API changes outlined in the Windows GMSA KEP
(https://github.com/kubernetes/enhancements/blob/master/keps/sig-windows/20181221-windows-group-managed-service-accounts-for-container-identity.md)
to add GMSA support to Windows workloads.

This essentially means moving the GMSA cred spec names & contents
from annotations to dedicated API fields.
More specifically, this is affecting PodSecurityContext and SecurityContext
structs, adding a WindowsOptions object to both.

Updated tests.

Special notes for your reviewer:
Still a WIP, will also update the webhook
(https://github.com/kubernetes-sigs/windows-gmsa)
to ensure all is in order before merging.

Does this PR introduce a user-facing change?:

API changes and deprecating the use of special annotations for Windows GMSA support (version beta)

@wk8 wk8 changed the title Kubelet & API changes for GMSA support - version beta Kubelet & API changes for Windows GMSA support - version beta Mar 19, 2019

@wk8

This comment has been minimized.

Copy link
Member Author

commented Mar 19, 2019

Putting on hold until the webhook is upated too.

@wk8

This comment has been minimized.

Copy link
Member Author

commented Mar 19, 2019

/hold

@wk8

This comment has been minimized.

Copy link
Member Author

commented Mar 19, 2019

/sig windows

@ddebroy
Copy link
Member

left a comment

Overall looks good. Once comment about removing the feature flag. While the link to the KEP is useful, can you update the PR description please with the API objects changed: PodSecurityContext and SecurityContext) and the new fields introduced?

pkg/features/kube_features.go Outdated Show resolved Hide resolved
@ddebroy

This comment has been minimized.

Copy link
Member

commented Mar 19, 2019

@wk8 wk8 force-pushed the wk8:wk8/gmsa_beta branch from 3782b17 to 335b5d0 Mar 19, 2019

@michmike

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

/milestone v1.15
/sig windows

@liggitt
Copy link
Member

left a comment

thanks for the update. you can squash the Review comment: renaming GMSA go variables files into the right commits.

found a couple potential NPEs in pkg/securitycontext/util.go

the update to dropDisabledFields() (#75459 (comment)) is still outstanding as well

looking good otherwise

pkg/apis/core/validation/validation.go Outdated Show resolved Hide resolved
pkg/securitycontext/util.go Outdated Show resolved Hide resolved

@liggitt liggitt moved this from In progress to Changes requested in API Reviews May 15, 2019

@wk8 wk8 force-pushed the wk8:wk8/gmsa_beta branch from c74cdad to 2002833 May 15, 2019

@k8s-ci-robot k8s-ci-robot added size/XXL and removed size/XL labels May 16, 2019

wk8 added 3 commits May 16, 2019
API changes for Windows GMSA support
This patch comprises the API changes outlined in the Windows GMSA KEP
(https://github.com/kubernetes/enhancements/blob/master/keps/sig-windows/20181221-windows-group-managed-service-accounts-for-container-identity.md)
to add GMSA support to Windows workloads.

It includes validation, as well as dropping fields if the `WindowsGMSA` feature
flag is not set, both with unit tests.

Signed-off-by: Jean Rouge <rougej+github@gmail.com>
GRPC changes for GMSA support
Signed-off-by: Jean Rouge <rougej+github@gmail.com>
Auto-generated changes for Windows GMSA support
This patch comprises the auto-generated changes for the API changes outlined in
the Windows GMSA KEP
(https://github.com/kubernetes/enhancements/blob/master/keps/sig-windows/20181221-windows-group-managed-service-accounts-for-container-identity.md)
to add GMSA support to Windows workloads.

Signed-off-by: Jean Rouge <rougej+github@gmail.com>

@wk8 wk8 force-pushed the wk8:wk8/gmsa_beta branch from 1432a2a to 2e4850d May 16, 2019

@wk8

This comment has been minimized.

Copy link
Member Author

commented May 16, 2019

@liggitt @ddebroy : amended as requested, including #75459 (comment), could you please take another look?

Kubelet & implementation changes for Windows GMSA support
This patch comprises the kubelet changes outlined in the Windows GMSA KEP
(https://github.com/kubernetes/enhancements/blob/master/keps/sig-windows/20181221-windows-group-managed-service-accounts-for-container-identity.md)
to add GMSA support to Windows workloads.

Updated tests.

Signed-off-by: Jean Rouge <rougej+github@gmail.com>

@wk8 wk8 force-pushed the wk8:wk8/gmsa_beta branch from 2e4850d to b39d8f4 May 17, 2019

@liggitt liggitt moved this from Changes requested to In progress in API Reviews May 17, 2019

@liggitt

This comment has been minimized.

Copy link
Member

commented May 17, 2019

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label May 17, 2019

@liggitt liggitt moved this from In progress to Completed, 1.15 in API Reviews May 17, 2019

@liggitt

This comment has been minimized.

Copy link
Member

commented May 17, 2019

/priority important-soon

@liggitt

This comment has been minimized.

Copy link
Member

commented May 17, 2019

/cc @yujuhong

for cri approval

@k8s-ci-robot k8s-ci-robot requested a review from yujuhong May 17, 2019

@@ -637,6 +637,9 @@ message WindowsContainerSecurityContext {
// exist in the container image and be resolved there by the runtime;
// otherwise, the runtime MUST return error.
string run_as_username = 1;

// The contents of the GMSA credential spec to use to run this container.
string credential_spec = 2;

This comment has been minimized.

Copy link
@yujuhong

yujuhong May 17, 2019

Member

The API change LGTM. We may need to change the comment to make it clear if we ever supports non-GMSA spects.

This comment has been minimized.

Copy link
@wk8

wk8 May 20, 2019

Author Member

@yujuhong : sorry if I'm misunderstanding, but that field's name was requested to not include "GMSA" in #75459 (comment), and the comment makes it clear it's for GMSA for now; so not sure what else we should do here?

This comment has been minimized.

Copy link
@liggitt

liggitt May 20, 2019

Member

I think this is fine for now. The field name matches the oci field name, and the description is reasonably consistent with the oci doc.

@PatrickLang

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

@yujuhong - can you approve or find one from SIG-Node?

@yujuhong

This comment has been minimized.

Copy link
Member

commented May 21, 2019

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, wk8, yujuhong

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 ae2a162 into kubernetes:master May 21, 2019

20 checks passed

cla/linuxfoundation wk8 authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

SIG-Windows automation moved this from In Progress+Review to Done (v1.15) May 21, 2019

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.