Skip to content

Adds WindowsOptions.RunAsUserName field#79489

Merged
k8s-ci-robot merged 3 commits intokubernetes:masterfrom
claudiubelu:feature/run-as-username
Jul 18, 2019
Merged

Adds WindowsOptions.RunAsUserName field#79489
k8s-ci-robot merged 3 commits intokubernetes:masterfrom
claudiubelu:feature/run-as-username

Conversation

@claudiubelu
Copy link
Copy Markdown
Contributor

@claudiubelu claudiubelu commented Jun 27, 2019

What type of PR is this?

/kind api-change

/sig windows

What this PR does / why we need it:

This adds windows configuration and wires up username in the podspec to the runtime interface.

#64009 added run_as_username to the container runtime interface, but did not hook it up in the Kubernetes v1.Container.SecurityContext.WindowsOptions.runAsUser field.

This PR also validates the RunAsUserName field, making sure that it valid, having the format DOMAIN\USER (case insensitive), where DOMAIN\ is optional and has to be a valid NetBios or DNS domain name. For more information about the restrictions on the DOMAIN and USER parts, look here: [1] [2]

Adds the WindowsRunAsUserName alpha feature gate. By default, it is disabled. If the feature gate is not enabled, the WindowsOptions.RunAsUserName field will be dropped from both the PodSecurityContext and container SecurityContext.

[1] https://support.microsoft.com/en-us/help/909264/naming-conventions-in-active-directory-for-computers-domains-sites-and
[2] https://docs.microsoft.com/en-us/powershell/module/microsoft.powershell.localaccounts/new-localuser?view=powershell-5.1

Which issue(s) this PR fixes:

Fixes: #73387

Special notes for your reviewer:

The original PR and discussions on it is here: #73609

Does this PR introduce a user-facing change?:

Add v1.Container.SecurityContext.WindowsOptions.RunAsUserName to the pod spec 

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/M Denotes a PR that changes 30-99 lines, ignoring generated files. sig/windows Categorizes an issue or PR as relevant to SIG Windows. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/kubelet sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/node Categorizes an issue or PR as relevant to SIG Node. labels Jun 27, 2019
@fejta-bot
Copy link
Copy Markdown

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

Copy link
Copy Markdown
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

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

There are no known limitation regarding the Windows username length or special character usage, so we only set an upper limit in order to prevent abuse.

this is supposed to target GMSA, correct?

is there a username cap for the existing GMSA support based on https://kubernetes.io/docs/tasks/configure-pod-container/configure-gmsa/#create-gmsa-credential-spec-resources

@claudiubelu
Copy link
Copy Markdown
Contributor Author

There are no known limitation regarding the Windows username length or special character usage, so we only set an upper limit in order to prevent abuse.

this is supposed to target GMSA, correct?

is there a username cap for the existing GMSA support based on https://kubernetes.io/docs/tasks/configure-pod-container/configure-gmsa/#create-gmsa-credential-spec-resources

Not quite, they're not quite the same: https://github.com/kubernetes/enhancements/blob/master/keps/sig-windows/20190418-windows-security-context.md#specification-of-both-gmsa-credspec-and-runasusername

@claudiubelu claudiubelu force-pushed the feature/run-as-username branch from 2812776 to b6d8043 Compare June 28, 2019 10:32
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 28, 2019
@claudiubelu
Copy link
Copy Markdown
Contributor Author

/cc @PatrickLang
/cc @michmike
/cc @adelina-t
/cc @liggitt
/cc @ddebroy

@neolit123
Copy link
Copy Markdown
Member

neolit123 commented Jun 28, 2019

Not quite, they're not quite the same: https://github.com/kubernetes/enhancements/blob/master/keps/sig-windows/20190418-windows-security-context.md#specification-of-both-gmsa-credspec-and-runasusername

the KEP does not seem to outline which Windows user account backend are we targeting if not GMSA?

RunAsUserName governs the local user identity used to log into the container

searching online the only tutorials for running Windows containers with non-root users, cover AD + GMSA only.
e.g. https://docs.microsoft.com/en-us/virtualization/windowscontainers/manage-containers/manage-serviceaccounts

@liggitt liggitt self-assigned this Jun 28, 2019
Copy link
Copy Markdown
Member

@liggitt liggitt left a comment

Choose a reason for hiding this comment

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

Needs a feature gate added, and corresponding logic in api/pod/util.go#dropDisabledFields

@ddebroy
Copy link
Copy Markdown
Contributor

ddebroy commented Jun 28, 2019

the KEP does not seem to outline which Windows user account backend are we targeting if not GMSA?

@neolit123 RunAsUserName only governs the user configured within the Windows container. It may be used, for example, to configure the user as Network Service or ContainerAdministrator instead of ContainerUser. The username specified through RunAsUserName does not have a direct relation with a user account backend (like GMSA).

@ddebroy
Copy link
Copy Markdown
Contributor

ddebroy commented Jun 28, 2019

@BCLAU for the dropDisabled... functions for username, you can follow the GMSA fields. Please also add corresponding tests in pkg/api/pod/util_test.go

Copy link
Copy Markdown
Contributor

@ddebroy ddebroy left a comment

Choose a reason for hiding this comment

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

Couple of nits around the comments for the WindowsOptions field.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 12, 2019
@claudiubelu claudiubelu force-pushed the feature/run-as-username branch from a61067a to 01176ff Compare July 12, 2019 21:23
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 12, 2019
@PatrickLang
Copy link
Copy Markdown
Contributor

/lgtm

I talked to @liggitt briefly on Slack. The validation regex looks ok to me and there's a test PR coming in with more validation.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 15, 2019
@claudiubelu claudiubelu force-pushed the feature/run-as-username branch from 01176ff to 175ffa7 Compare July 16, 2019 16:21
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 16, 2019
@liggitt
Copy link
Copy Markdown
Member

liggitt commented Jul 17, 2019

thanks, that's easier to follow. one question still pending at #79489 (comment), then lgtm

@claudiubelu claudiubelu force-pushed the feature/run-as-username branch from 175ffa7 to f01c790 Compare July 17, 2019 14:03
Copy link
Copy Markdown
Contributor Author

@claudiubelu claudiubelu left a comment

Choose a reason for hiding this comment

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

I didn't realize that I didn't press the Submit review button yesterday. Sorry. :)

@claudiubelu claudiubelu force-pushed the feature/run-as-username branch from f01c790 to a39e483 Compare July 17, 2019 14:36
jsturtevant and others added 3 commits July 17, 2019 15:03
Adds the field RunAsUserName in the WindowsSecurityContextOptions type,
which is used in PodSecurityContext and SecurityContext.

This field needs to allow for a valid set of usernames allowed for
Windows containers. It must have the format "U

This commit also validates the runAsUserName field, making sure that it valid,
having the format DOMAIN\USER (case insensitive), where DOMAIN\ is optional and
has to be a valid NetBios or DNS domain name.

For more information about the restrictions on the DOMAIN and USER parts, look here: [1] [2]

Adds the WindowsRunAsUserName alpha feature gate. By default, it is disabled.
If the feature gate is not enabled, the WindowsOptions.RunAsUserName field
will be dropped from both the PodSecurityContext and container
SecurityContext.

Co-Authored-By: Claudiu Belu <cbelu@cloudbasesolutions.com>

[1] https://support.microsoft.com/en-us/help/909264/naming-conventions-in-active-directory-for-computers-domains-sites-and
[2] https://docs.microsoft.com/en-us/powershell/module/microsoft.powershell.localaccounts/new-localuser?view=powershell-5.1
Co-Authored-By: Claudiu Belu <cbelu@cloudbasesolutions.com>
@claudiubelu claudiubelu force-pushed the feature/run-as-username branch from a39e483 to a8c78d1 Compare July 17, 2019 15:03
@liggitt
Copy link
Copy Markdown
Member

liggitt commented Jul 17, 2019

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 17, 2019
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bclau, derekwaynecarr, liggitt

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:

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 17, 2019
@claudiubelu
Copy link
Copy Markdown
Contributor Author

/test pull-kubernetes-e2e-gce-device-plugin-gpu

@liggitt
Copy link
Copy Markdown
Member

liggitt commented Jul 17, 2019

/retest

@PatrickLang
Copy link
Copy Markdown
Contributor

/lgtm

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

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. sig/windows Categorizes an issue or PR as relevant to SIG Windows. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

Status: API review completed, 1.16

Development

Successfully merging this pull request may close these issues.

Finish hooking up run_as_username on Windows

10 participants