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

Update windows.md to run as a windows service (in support of containerd) #1874

Merged
merged 2 commits into from
Apr 8, 2021

Conversation

jayunit100
Copy link
Contributor

quick update for antrea, as a windows service

@codecov-io
Copy link

codecov-io commented Feb 14, 2021

Codecov Report

❗ No coverage uploaded for pull request base (main@f93242a). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1874   +/-   ##
=======================================
  Coverage        ?   65.00%           
=======================================
  Files           ?      197           
  Lines           ?    17507           
  Branches        ?        0           
=======================================
  Hits            ?    11380           
  Misses          ?     4941           
  Partials        ?     1186           
Flag Coverage Δ
kind-e2e-tests 56.02% <0.00%> (?)
unit-tests 41.61% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

@jayunit100
Copy link
Contributor Author

@ruicao93 just a first draft of a patch for docs ,

@jayunit100 jayunit100 changed the title Update windows.md Update windows.md to run as a windows service (in support of containerd) Feb 14, 2021
docs/windows.md Outdated Show resolved Hide resolved
docs/windows.md Outdated Show resolved Hide resolved
docs/windows.md Show resolved Hide resolved
docs/windows.md Outdated Show resolved Hide resolved
docs/windows.md Outdated Show resolved Hide resolved
docs/windows.md Outdated Show resolved Hide resolved
docs/windows.md Outdated Show resolved Hide resolved
docs/windows.md Outdated Show resolved Hide resolved
@ruicao93
Copy link
Contributor

Fixes: #1889

@jayunit100 Thanks for your PR. Do you need me continue the PR if you have no time to address the comments?

Copy link
Contributor

@ruicao93 ruicao93 left a comment

Choose a reason for hiding this comment

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

Thanks Zhecheng.

  • Have you tried the service installation method?
  • Another thing we need to consider is that the agent config may need to be updated in some cases(e.g. new antrea-controller Pod is deployed). If the case happens, antrea-agent service will cannot start. [Windows] Always update agent configs on startup #1847

docs/windows.md Outdated Show resolved Hide resolved
docs/windows.md Outdated Show resolved Hide resolved
docs/windows.md Outdated Show resolved Hide resolved
docs/windows.md Outdated Show resolved Hide resolved
docs/windows.md Outdated

mv antrea-agent-windows-x86_64.exe c:/k/antrea/bin/antrea-agent.exe
Import-Module ./helper.psm1
Install-AntreaAgent -KubernetesVersion "v1.20.2" -KubernetesHome "c:/k" -KubeConfig "C:/etc/kubernetes/kubelet.conf" -AntreaVersion "${TAG}" -AntreaHome "c:/k/antrea"
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the requirement for the K8s version? And should we update the default K8s version in the existing powershell scripts? Maybe the scripts should use the value returned by https://storage.googleapis.com/kubernetes-release/release/stable.txt by default?

Copy link
Contributor

Choose a reason for hiding this comment

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

KubeConfig is important here as well, we've prototyped with this being the kubelet.conf but by default the node doesn't have access to everything it needs, is there a way that we can allow the node config to do what it needs to or is that a security problem and can we think of a way around it (avoiding manually creating the kubeconfig)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you @antoninbas and @perithompson . I am investigating these issues. Yes, on my testbed, kubelet.conf is not enough to get secret in Install-AntreaAgent().

Copy link
Contributor

@lzhecheng lzhecheng Mar 9, 2021

Choose a reason for hiding this comment

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

Discussed with @ruicao93. As for the K8s version, there's no requirement. So here v1.20.4 is just an example, users should keep it consistent with K8s version on other nodes. I changed it to <KubernetesVersion> and users should fill it up.
As for kubeconfig, it is users' responsibility to provide a valid kubeconfig. Whether using admin.conf or a customed one, it's up to the user. I changed it to <KubeconfigPath> and users should fill it up.

Copy link
Contributor

@perithompson perithompson Mar 9, 2021

Choose a reason for hiding this comment

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

understood, it's just incredibly difficult to automate this at node creation time so I was wondering if there was a way around this to help? Either way, have we got a method of generating this kubeconfig, either on Linux or on Windows that we could add to this document as there is no explanation of where the kubeconfig comes from.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have mentioned that kubeconfig can be copied from /etc/kubernetes/admin.conf on ControlPlane node. I suppose it is enough for users.

Copy link
Contributor

Choose a reason for hiding this comment

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

An alternative could be to provide a script to generate all the required kubeconfig files (2 for antrea-agent and 1 for kube-proxy) using the "admin" kubeconfig. This process would need to be done only once per cluster instead of on every Node. We could then use a DaemonSet to distribute and install the generated kubeconfig files to each Windows Node in the cluster. As a new Node joins the cluster, the files would be installed automatically. But maybe that's an overkill...

It seems that currently we recommend running kube-proxy with the "admin" kubeconfig (/etc/kubernetes/admin.conf copied from the control-plane mode), which seems like too many permissions.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree it is an overkill. A simple script is enough to distribute kubeconfig to nodes. Then how about this? Admin kubeconfig is used to Install-AntreaAgent because such permission is required to get secret and kubelet one is used to install kube-proxy. I have updated this doc.

Copy link
Contributor

Choose a reason for hiding this comment

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

In the context of capi, when you create a node, you only have control of actions that happen on the host as an administrator, so in theory I guess you could get it from another host from the cluster but securing that is a nightmare too!

docs/windows.md Outdated Show resolved Hide resolved
docs/windows.md Outdated Show resolved Hide resolved
docs/windows.md Outdated Show resolved Hide resolved
@lzhecheng lzhecheng force-pushed the patch-14 branch 4 times, most recently from 13830c1 to 362abd4 Compare March 9, 2021 03:28
docs/windows.md Outdated Show resolved Hide resolved
@lzhecheng lzhecheng force-pushed the patch-14 branch 2 times, most recently from fd52d61 to 803689e Compare March 11, 2021 02:54
docs/windows.md Outdated
New-KubeProxyServiceInterface

mkdir "${KubernetesHome}/antrea/logs"
nssm install kube-proxy "${KubernetesHome}/kube-proxy.exe" "--proxy-mode=userspace --kubeconfig=$KubeletKubeConfig --log-dir=c:/var/log/kube-proxy --logtostderr=false --alsologtostderr"
Copy link
Contributor

Choose a reason for hiding this comment

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

What about use config file like KubeProxyKubeConfig instead of KubeletKubeConfig. It means let user specify a kubeconfig file used by kube-proxy. The kubelet configfile contains too many permissions.

For example, user could generate the KubeProxyKubeConfig use the kube-proxy service account.

Copy link
Contributor

Choose a reason for hiding this comment

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

@lzhecheng any thoughts on that, is there any upstream reference on running kube-proxy as a service?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

KubeletKubeConfig is changed to KubeProxyKubeconfigPath.

@antoninbas antoninbas added this to the Antrea v1.0 release milestone Mar 30, 2021
docs/windows.md Outdated Show resolved Hide resolved
docs/windows.md Outdated Show resolved Hide resolved
docs/windows.md Outdated Show resolved Hide resolved
docs/windows.md Show resolved Hide resolved
docs/windows.md Outdated Show resolved Hide resolved
docs/windows.md Outdated Show resolved Hide resolved
docs/windows.md Outdated
New-KubeProxyServiceInterface

mkdir "${KubernetesHome}/antrea/logs"
nssm install kube-proxy "${KubernetesHome}/kube-proxy.exe" "--proxy-mode=userspace --kubeconfig=$KubeletKubeConfig --log-dir=c:/var/log/kube-proxy --logtostderr=false --alsologtostderr"
Copy link
Contributor

Choose a reason for hiding this comment

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

@lzhecheng any thoughts on that, is there any upstream reference on running kube-proxy as a service?

docs/windows.md Show resolved Hide resolved
docs/windows.md Outdated Show resolved Hide resolved
docs/windows.md Outdated Show resolved Hide resolved
docs/windows.md Outdated

### Installation via wins (docker based runtimes)

Installing Antrea as a wins service gives you a lot of flexibility to manage it as a Pod, but currently this only works
Copy link
Contributor

Choose a reason for hiding this comment

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

Installing Antrea using wins..

Copy link
Contributor

Choose a reason for hiding this comment

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

I just feel "as wins service" might confuse people - thinking it means Windows service.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think @jayunit100 is referring to https://github.com/rancher/wins. Is it correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

I know. But not all readers understand what is a "wins service", so I suggest to rephrase the sentence as: "Installing Antrea using wins gives.."

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. I updated the sentence and added a link to the repo.

Co-authored-by: Zhecheng Li <[email protected]>
Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

LGTM. I would like to include this in the v1.0 release so these changes are part of the documentation for this release. We can refine them as needed in the future.

@antoninbas
Copy link
Contributor

/skip-all

@antoninbas
Copy link
Contributor

The "Verify docs and spelling" is failing because of an issue that has been fixed in "main". I will use admin privileges to merge.

@antoninbas antoninbas merged commit 3be5642 into antrea-io:main Apr 8, 2021
@antoninbas
Copy link
Contributor

Thanks again @lzhecheng for picking up this PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Antrea running as a service on windows
8 participants