📖 Windows Support CAEP #3616

jsturtevant · 2020-09-09T20:41:50Z

What this PR does / why we need it:
Add proposal for Windows support in CAPI and providers. Related to #2218

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

jsturtevant · 2020-09-09T20:50:42Z

/cc @marosset @michmike @ksubrmnn

benmoss · 2020-09-09T21:50:08Z

I would remove the

Fixes #2218

since this is just the proposal

vincepri · 2020-09-15T15:24:27Z

/assign @CecileRobertMichon @benmoss @devigned

docs/proposals/20200804-windows-support.md

CecileRobertMichon

The overall approach looks good to me, most comments are wording suggestions/asks for clarification

docs/proposals/20200804-windows-support.md

randomvariable · 2020-09-24T14:24:13Z

Didn't comment much on the PR, since did most of it in the google docs, so broadly supportive of this CAEP.

jsturtevant · 2020-09-24T16:19:01Z

I took a look at pull-cluster-api-verify-external-links — Job failed. . I didn't see any failures related to this document.

/test pull-cluster-api-verify-external-links

randomvariable · 2020-09-24T17:02:16Z

I took a look at pull-cluster-api-verify-external-links — Job failed. . I didn't see any failures related to this document.

Ignore that error, we're being rate limited by Github for lack of a token.

devigned

Overall lgtm. Just left some grammatical suggestions.

docs/proposals/20200804-windows-support.md

docs/book/src/developer/architecture/controllers/machine.md

docs/proposals/20200804-windows-support.md

randomvariable · 2020-10-14T12:08:25Z

@jsturtevant @michmike

For domain joining a Windows node I thought using pre/post kubeadm commands could be an option. @randomvariable at sig-windows meeting today you mentioned you have some ideas?

I think for the initial implementation, this is fine. I don't think we need to say much more than this right now.

There's going to be a security improvement that we would want to do, and that's where I think maybe having a separate controller that provides one-time domain join accounts and pre-created AD computer resources could be useful and what I'm thinking about implementing. I would however like to treat this as a subset of #3761 rather than blocking on this CAEP. This CAEP does not prohibit doing domain joins via the use of pre/post kubeadm commands, which is the main point.

jsturtevant · 2020-10-14T18:09:02Z

@CecileRobertMichon @randomvariable @ncdc This is ready for final review. Based on the meeting today I've removed the retry logic since it should be addressed by recent updates to kubeadm in k8s 1.19 (and backported to 1.17)

jayunit100 · 2020-10-15T17:51:16Z

docs/proposals/20200804-windows-support.md

+    - [Infrastructure provider implementation](#infrastructure-provider-implementation)
+    - [User Stories](#user-stories)
+      - [As an operator, I would like to create Windows OS worker nodes with the CAPI API.](#as-an-operator-i-would-like-to-create-windows-os-worker-nodes-with-the-capi-api)
+      - [As an operator, I would like to manage Windows OS worker nodes with the CAPI API.](#as-an-operator-i-would-like-to-manage-windows-os-worker-nodes-with-the-capi-api)


Seems like some type active directory awareness would be worth diving into.

- [As an operator, i'd like to federate my windows OS worker nodes with active directory]

or is that out of scope? Just a thought from what we think our customers would like to be able to do

(feel free to merge if its ready , dont wanna hold things up, maybe can iterate if this is in scope)

Yes definitely. I think we decided to move forward with this and address the AD via #3761

ref: #3616 (comment)

benmoss · 2020-10-19T18:48:04Z

/lgtm

CecileRobertMichon

/lgtm

k8s-ci-robot · 2020-10-19T18:56:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon, devigned

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:

~~OWNERS~~ [CecileRobertMichon]

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 · 2020-10-19T19:03:45Z

@jsturtevant: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-cluster-api-verify-external-links	`7147dcb`	link	`/test pull-cluster-api-verify-external-links`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

benmoss · 2020-10-19T19:08:38Z

/test pull-cluster-api-test-main

CecileRobertMichon · 2020-10-19T19:29:24Z

I didn't realize my lgtm would auto-apply the approve label, didn't mean for this to merge right away :/

vincepri · 2020-10-19T19:54:07Z

/milestone v0.4.0

No worries @CecileRobertMichon, have to investigate why that happened, might be a new behavior that we should take into account. If folks have more outstanding questions on this PR, feel free to comment or amend in a different PR.

benmoss · 2020-10-19T21:27:05Z

It looks like you might have used the "Approve" feature of the pull request review? I also didn't know that was the same as an /approve

Yeah, looks like that's it:
https://github.com/kubernetes/test-infra/blob/master/config/prow/plugins.yaml#L96
https://github.com/kubernetes/test-infra/blob/b08b9c59e07ba5c83677186300fb3afdea984812/prow/plugins/approve/approve.go#L560-L571

CecileRobertMichon · 2020-10-19T21:32:57Z

That looks like new behavior, didn't work that way in the past, eg. #3730 (review)

CecileRobertMichon · 2020-10-19T21:42:34Z

we're seeing the same thing in capz: kubernetes-sigs/cluster-api-provider-azure#1002 (review) (also new behavior) cc @devigned

CecileRobertMichon · 2020-10-19T21:58:46Z

The default changed 4 days ago because kubernetes/test-infra#19579 removed the deprecated option review_acts_as_approve (defaulted to false) in favor of ignore_review_state which defaults to false (but opposite behavior)

ravisantoshgudimetla

Apologies for delayed review @jsturtevant. I asked for some clarifications let me know if you want to discuss them on slack.

ravisantoshgudimetla · 2020-10-20T18:25:41Z

docs/proposals/20200804-windows-support.md

+      - [netbios names](#netbios-names)
+    - [Infrastructure provider implementation](#infrastructure-provider-implementation)
+    - [User Stories](#user-stories)
+      - [As an operator, I would like to create Windows OS worker nodes with the CAPI API.](#as-an-operator-i-would-like-to-create-windows-os-worker-nodes-with-the-capi-api)


ravisantoshgudimetla · 2020-10-20T18:31:31Z

docs/proposals/20200804-windows-support.md

+Another example is the the [sig-windows-tools](https://github.com/kubernetes-sigs/sig-windows-tools) which provide scripts for image configuration when using Kubeadm.
+
+Although the Linux implementation in image-builder uses Ansible for configuration, Windows isn't going to share
+the same configuration because [Ansible](https://docs.ansible.com/ansible/latest/user_guide/windows.html) requires [Windows specific modules](https://docs.ansible.com/ansible/latest/modules/list_of_windows_modules.html) to do the configuration. 


Can you please elaborate? Which windows specific modules are needed? We used ansible to configure Windows nodes in the past and we had to install modules on the host from which ansible is being run, a linux host in our case but nothing on the Windows except for enabling WinRM.

ravisantoshgudimetla · 2020-10-20T18:40:19Z

docs/proposals/20200804-windows-support.md

+By leveraging cloudbase-init, an infrastructure provider implementation will require only a few changes which include:
+
+- Make changes to their provider api to enable Windows OS infra machines ([example](https://github.com/ionutbalutoiu/cluster-api-provider-azure/commit/9c8daedac75959b141fec7ea909c2c1fd0bd484b))
+- Ensuring cloudbase-init is configured properly to read UserData which will contain the cloud-init script.  Users must configure 


We avoided infrastructure changes as on AWS as userData is automatically executed even on Windows VMs whereas in case of azure, we have a carry patch

But you're right in the sense we need to have consistency across cloud providers however we're a bit worried about the support of cloudbase-init across all the providers. What is the support statement like? Can we get some changes in if we need to immediately or in the long-term should we fork and attribute?

ravisantoshgudimetla · 2020-10-20T18:46:21Z

docs/proposals/20200804-windows-support.md

+
+Wins.exe is currently the [recommended way to use kubeadm](https://kubernetes.io/docs/tasks/administer-cluster/kubeadm/adding-windows-nodes/).  
+Limiting access to the named pipes that are required is one way to mitigate access.  Wins is currently 
+required during and after provisioning for running kube-proxy and the CNI daemonset.  


unrelated question: How are CNI and kube-proxy DS's container images being built for Windows and who owns building and supporting them?

ravisantoshgudimetla · 2020-10-20T18:58:40Z

docs/proposals/20200804-windows-support.md

+
+## Upgrade Strategy
+
+Nodes that use this pattern will require the infrastructure to be immutable as specified by CAPI documentation.


ravisantoshgudimetla · 2020-10-20T18:59:44Z

docs/proposals/20200804-windows-support.md

+Windows does not support Docker in Docker so end to end test can not be added for Windows specific behavior.
+If changes are required during development unit tests will be required.  
+
+For infrastructure providers the testing plan is left up to each infrastructure provider. It is recommended to leverage 


Can you add a link to jobs that need to be run for every infrastructure provider?

ravisantoshgudimetla · 2020-10-20T19:02:55Z

docs/proposals/20200804-windows-support.md

+In the future, when support for [Privileged Containers for Windows containers](https://github.com/kubernetes/enhancements/issues/1981) is merged, we might be able to revisit this proposal 
+and use privileged containers in place of wins.exe enabled containers. 
+
+Each infrastructure providers must provide their own `PreKubeadmCommands`/`PostKubeadmCommands` scripts that


Can you elaborate on what each infrastructure provider should do? What would be a sample PreKubeadmCommand for azure? Downloading Windows base image like server-core or something like that?

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 9, 2020

k8s-ci-robot requested review from detiber and ncdc September 9, 2020 20:41

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 9, 2020

k8s-ci-robot requested review from ksubrmnn, marosset and michmike September 9, 2020 20:50

jsturtevant mentioned this pull request Sep 10, 2020

Windows machine image kubernetes-sigs/image-builder#30

Closed

vincepri changed the title ~~Windows Support CAEP~~ 📖 Windows Support CAEP Sep 15, 2020

k8s-ci-robot assigned benmoss, CecileRobertMichon and devigned Sep 15, 2020

jsturtevant commented Sep 16, 2020

View reviewed changes

docs/proposals/20200804-windows-support.md Outdated Show resolved Hide resolved

jsturtevant commented Sep 16, 2020

View reviewed changes

docs/proposals/20200804-windows-support.md Show resolved Hide resolved

jsturtevant commented Sep 16, 2020

View reviewed changes

docs/proposals/20200804-windows-support.md Outdated Show resolved Hide resolved

ncdc reviewed Sep 16, 2020

View reviewed changes

jsturtevant force-pushed the capi-windows-proposal branch from 1a3c1cf to 0068599 Compare September 17, 2020 00:19

gab-satchi reviewed Sep 17, 2020

View reviewed changes

docs/proposals/20200804-windows-support.md Outdated Show resolved Hide resolved

jsturtevant mentioned this pull request Sep 22, 2020

Windows image configuration scripts and Azure builder kubernetes-sigs/image-builder#382

Merged

CecileRobertMichon reviewed Sep 22, 2020

View reviewed changes

marosset reviewed Sep 22, 2020

View reviewed changes

jsturtevant force-pushed the capi-windows-proposal branch 2 times, most recently from bae43ca to 7147dcb Compare September 23, 2020 19:35

devigned approved these changes Sep 25, 2020

View reviewed changes

jsturtevant mentioned this pull request Oct 13, 2020

Insulate users from kubeadm API version changes #2769

Closed

jsturtevant force-pushed the capi-windows-proposal branch 3 times, most recently from d62ba76 to 0c741f2 Compare October 13, 2020 22:13

CecileRobertMichon reviewed Oct 13, 2020

View reviewed changes

docs/book/src/developer/architecture/controllers/machine.md Outdated Show resolved Hide resolved

fabriziopandini reviewed Oct 14, 2020

View reviewed changes

docs/proposals/20200804-windows-support.md Outdated Show resolved Hide resolved

jsturtevant mentioned this pull request Oct 14, 2020

Test if experimentalRetryJoin still needed for K8s 1.19+ kubernetes-sigs/cluster-api-provider-azure#998

Closed

Windows proposal

4050406

jsturtevant force-pushed the capi-windows-proposal branch from 01e696c to 4050406 Compare October 14, 2020 18:00

jayunit100 suggested changes Oct 15, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 19, 2020

CecileRobertMichon approved these changes Oct 19, 2020

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 19, 2020

k8s-ci-robot merged commit c164748 into kubernetes-sigs:master Oct 19, 2020

k8s-ci-robot added this to the v0.4.0 milestone Oct 19, 2020

CecileRobertMichon mentioned this pull request Oct 19, 2020

Cluster API: set ignore_review_state to true kubernetes/test-infra#19623

Merged

ravisantoshgudimetla reviewed Oct 20, 2020

View reviewed changes

CecileRobertMichon mentioned this pull request Apr 5, 2021

Windows Support #2218

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

📖 Windows Support CAEP #3616

📖 Windows Support CAEP #3616

jsturtevant commented Sep 9, 2020 •

edited

Loading

jsturtevant commented Sep 9, 2020

benmoss commented Sep 9, 2020

vincepri commented Sep 15, 2020

CecileRobertMichon left a comment

randomvariable commented Sep 24, 2020

jsturtevant commented Sep 24, 2020

randomvariable commented Sep 24, 2020

devigned left a comment

randomvariable commented Oct 14, 2020

jsturtevant commented Oct 14, 2020

jayunit100 Oct 15, 2020

jayunit100 Oct 15, 2020

jsturtevant Oct 15, 2020

benmoss commented Oct 19, 2020

CecileRobertMichon left a comment

k8s-ci-robot commented Oct 19, 2020

k8s-ci-robot commented Oct 19, 2020 •

edited

Loading

benmoss commented Oct 19, 2020

CecileRobertMichon commented Oct 19, 2020

vincepri commented Oct 19, 2020

benmoss commented Oct 19, 2020 •

edited

Loading

CecileRobertMichon commented Oct 19, 2020

CecileRobertMichon commented Oct 19, 2020

CecileRobertMichon commented Oct 19, 2020

ravisantoshgudimetla left a comment

ravisantoshgudimetla Oct 20, 2020

ravisantoshgudimetla Oct 20, 2020

ravisantoshgudimetla Oct 20, 2020

ravisantoshgudimetla Oct 20, 2020

ravisantoshgudimetla Oct 20, 2020

ravisantoshgudimetla Oct 20, 2020

ravisantoshgudimetla Oct 20, 2020


		## Upgrade Strategy

		Nodes that use this pattern will require the infrastructure to be immutable as specified by CAPI documentation.

📖 Windows Support CAEP #3616

📖 Windows Support CAEP #3616

Conversation

jsturtevant commented Sep 9, 2020 • edited Loading

jsturtevant commented Sep 9, 2020

benmoss commented Sep 9, 2020

vincepri commented Sep 15, 2020

CecileRobertMichon left a comment

Choose a reason for hiding this comment

randomvariable commented Sep 24, 2020

jsturtevant commented Sep 24, 2020

randomvariable commented Sep 24, 2020

devigned left a comment

Choose a reason for hiding this comment

randomvariable commented Oct 14, 2020

jsturtevant commented Oct 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benmoss commented Oct 19, 2020

CecileRobertMichon left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 19, 2020

k8s-ci-robot commented Oct 19, 2020 • edited Loading

benmoss commented Oct 19, 2020

CecileRobertMichon commented Oct 19, 2020

vincepri commented Oct 19, 2020

benmoss commented Oct 19, 2020 • edited Loading

CecileRobertMichon commented Oct 19, 2020

CecileRobertMichon commented Oct 19, 2020

CecileRobertMichon commented Oct 19, 2020

ravisantoshgudimetla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsturtevant commented Sep 9, 2020 •

edited

Loading

k8s-ci-robot commented Oct 19, 2020 •

edited

Loading

benmoss commented Oct 19, 2020 •

edited

Loading