Move CNI template to helm by zaharidichev · Pull Request #3581 · linkerd/linkerd2

zaharidichev · 2019-10-15T14:39:32Z

This PR moves the cni installation logic from cni-template.go to a helm template. It also simplifies some of the logic around how configuration is being supplied to the plugin.

Fixes: #2921

Signed-off-by: zaharidichev [email protected]

alpeb

Nice refactor 👍

I think we can't have cni-plugin.yaml live under charts/linkerd2/templates because it will get picked up by bin/helm-build package when building the helm release or by anyone installing with helm directly with the source (helm install charts/linikerd2). That'll create the cni resources even if not using cni.

Probably the cleanest would be to create a separate chart in charts/cni to house this guy.

grampelberg · 2019-10-17T18:29:30Z

I'm a big +1 to having a separate chart. We should (as a general rule) be keeping individual charts as small as possible and introducing dependencies between them.

zaharidichev · 2019-10-17T19:08:15Z

@alpeb Thanks for pointing that out, was not aware. Will split it up!

zaharidichev · 2019-10-18T06:03:34Z

@alpeb Currently I am reusing the values.go for the cni template. Do we want to split that up as well and have cni_values.go with a corresponding yaml containing defaults?

alpeb · 2019-10-18T14:43:11Z

@alpeb Currently I am reusing the values.go for the cni template. Do we want to split that up as well and have cni_values.go with a corresponding yaml containing defaults?

Yeah, I think it would make sense to have all cleanly separated 👍 People could get confused by those CNI values in the default values.yaml

zaharidichev · 2019-10-21T09:22:41Z

@alpeb I split up the CNI parts further and added some tests around that. Let me know if that looks better

alpeb

Thanks @zaharidichev !

It all looks good to me 👍 Just one minor comment below. Also, you forgot to remove /charts/linkerd2/templates/cni-plugin.yaml ;-)

I'll try running the CNI integration tests and report back.

alpeb · 2019-10-21T14:38:00Z

charts/linkerd2-cni/README.md

+| Parameter                            | Description                                                           | Default                       |
+|--------------------------------------|-----------------------------------------------------------------------|-------------------------------|
+|`Namespace`                           | Control plane namespace                                               | `linkerd`|
+|`ControllerNamespaceLabel`            | Control plane label. Do not edit                                      |`linkerd.io/control-plane-component`|


I believe the default here should be linkerd.io/control-plane-ns

alpeb · 2019-10-21T15:50:49Z

cni-plugin/deployment/scripts/install-cni.sh

-sed -i s~__INBOUND_PORTS_TO_IGNORE__~"${INBOUND_PORTS_TO_IGNORE:=}"~g ${TMP_CONF}
-sed -i s~__OUTBOUND_PORTS_TO_IGNORE__~"${OUTBOUND_PORTS_TO_IGNORE:=}"~g ${TMP_CONF}
-sed -i s~__SIMULATE__~"${SIMULATE:=false}"~g ${TMP_CONF}
-sed -i s~__USE_WAIT_FLAG__~"${USE_WAIT_FLAG:=false}"~g ${TMP_CONF}


Nice riddance, those were indeed not necessary, along with the env vars! 👍

zaharidichev · 2019-10-21T17:54:23Z

@alpeb Thanks a lot! If I move it from a fork branch to an internal one the tests should be ran automatically, right ?

alpeb · 2019-10-21T17:59:35Z

@alpeb Thanks a lot! If I move it from a fork branch to an internal one the tests should be ran automatically, right ?

I'm afraid not. The CNI integration tests only run when merging to master, or when tagging master.

alpeb · 2019-10-21T20:29:10Z

It took me a while to run the integration tests because I had a misconfigured docker install 😬

They are throwing some errors, could you please address those? It's actually not a big deal to run those tests after all, and they don't even require a cluster.
First do bin/docker-build-cni-plugin and then go test -v github.com/linkerd/linkerd2/cni-plugin/test -integration-tests.
That'll likely complain that the docker image is missing, I think because they're designed to run in master. To run from the branch, just tag the gcr.io/linkerd-io/cni-plugin image with the tag in the error message. After addressing that you'll see the real errors.

Feel free to ping me if you need any assistance.

alpeb

It all looks good to me 👍
Also tested fine in cluster.

alpeb · 2019-10-22T14:37:37Z

@zaharidichev this tested well using the CLI, but in order to do it through Helm, a new value in linkerd's values.yaml file, something like ProxyInit.Enabled will need to be added for the main chart to skip the init containers. I think that can be tackled in a separate PR.

zaharidichev · 2019-10-23T08:20:55Z

@alpeb there is already Values.NoInitContainer, which is not included in the values.yaml file but it defaults to false. Why is there te need for ProxyInit.Enabled in that case?

alpeb · 2019-10-23T14:22:35Z

You're right, I was thrown off by the absence of NoInitContainer in values.yaml.
After creating some docs explaining how to go about with Helm and CNI, we can add it there, pointing to the docs.

This tested fine in Minikube using the following commands:

(Minikube requires UseWaitFlag=true)
helm install -f ~/k8s/helm/linkerd-overrides.yaml --set UseWaitFlag=true charts/linkerd2-cni
helm install -f ~/k8s/helm/linkerd-overrides.yaml --set NoInitContainer=true --set InstallNamespace=false charts/linkerd

grampelberg · 2019-11-22T16:18:44Z

Aw man, I hate having PRs fall through the cracks. This looks good to me, @adleong or @ihcsim mind taking a look so we can get two 👍 ?

zaharidichev · 2019-11-22T16:38:15Z

@grampelberg yes its been a while :) the refactor I did here will align nicely with some other work I am doing atm so that's where my desire to get it merged comes from

ihcsim

@zaharidichev Thanks for cleaning this up 👍!

This branch works with the following commands on my GKE cluster:

# nodes should run Ubuntu, as COS uses read-only root filesystem
$ linkerd install-cni --dest-cni-bin-dir=/home/kubernetes/bin|k apply -f -
$ linkerd install --linkerd-cni-enabled|k apply -f -

$ helm install --name=linkerd2-cni --set DestCNIBinDir=/home/kubernetes/bin charts/linkerd2-cni
$ helm install --name=linkerd2 --set Identity.Issuer.CrtExpiry="Dec 9 16:10:37 2019" --set-file Identity.TrustAnchorsPEM=ca.crt --set-file Identity.Issuer.TLS.CrtPEM=issuer.crt --set-file Identity.Issuer.TLS.KeyPEM=issuer.key --set LinkerdVersion=git-57e39022 --set Proxy.Image.Version=git-57e39022 --set NoInitContainer=true --set InstallNamespace=false charts/linkerd2

See some comments on the README and variable names (in light of the Go convention) below.

charts/linkerd2-cni/Chart.yaml

charts/linkerd2-cni/README.md

charts/linkerd2-cni/templates/cni-plugin.yaml

cli/cmd/install-cni-plugin.go

charts/linkerd2-cni/Chart.yaml

pkg/charts/charts.go

ihcsim

LGTM 👍! See a comment below regarding the extra println().

cli/cmd/main_test.go

ihcsim · 2019-12-09T22:26:26Z

I just noticed the unresolved conflicts and fork. Please run the integration test on this fork, once the conflicts are resolved, before merging into master. Thanks.

Signed-off-by: zaharidichev <[email protected]>

Signed-off-by: Zahari Dichev <[email protected]>

zaharidichev force-pushed the zd/move-cni-to-helm branch from d8c22b0 to d23c125 Compare October 16, 2019 14:24

zaharidichev changed the title ~~WIP: Move CNI template to helm~~ Move CNI template to helm Oct 16, 2019

zaharidichev force-pushed the zd/move-cni-to-helm branch 3 times, most recently from 718d858 to 05aeeaa Compare October 16, 2019 16:18

alpeb reviewed Oct 17, 2019

View reviewed changes

zaharidichev force-pushed the zd/move-cni-to-helm branch from 05aeeaa to d110219 Compare October 19, 2019 13:58

alpeb assigned zaharidichev Oct 21, 2019

alpeb reviewed Oct 21, 2019

View reviewed changes

alpeb approved these changes Oct 22, 2019

View reviewed changes

zaharidichev force-pushed the zd/move-cni-to-helm branch from 69e1c45 to e344e06 Compare October 29, 2019 08:34

zaharidichev requested review from adleong and ihcsim November 4, 2019 14:58

zaharidichev requested a review from grampelberg November 22, 2019 09:57

grampelberg assigned ihcsim and unassigned zaharidichev Dec 2, 2019

zaharidichev mentioned this pull request Dec 8, 2019

Remove experimental label from cni plugin #3796

Closed

ihcsim reviewed Dec 9, 2019

View reviewed changes

zaharidichev force-pushed the zd/move-cni-to-helm branch from 950a722 to 1d25bfb Compare December 9, 2019 12:36

ihcsim mentioned this pull request Dec 9, 2019

Update CI/CD to build CNI chart #3801

Closed

ihcsim approved these changes Dec 9, 2019

View reviewed changes

cli/cmd/main_test.go Outdated Show resolved Hide resolved

zaharidichev added 7 commits December 10, 2019 10:20

Create helm chart for the CNI plugin

5acae68

Signed-off-by: zaharidichev <[email protected]>

Add helm install tests for the CNI plugin

cd71bef

Signed-off-by: zaharidichev <[email protected]>

Add readme for the CNI helm chart

2a553ad

Signed-off-by: zaharidichev <[email protected]>

Fix integration tests

2d34f19

Signed-off-by: zaharidichev <[email protected]>

Remove old cni-plugin.yaml

61be07b

Signed-off-by: zaharidichev <[email protected]>

Add trace partial template

e68e1c5

Signed-off-by: zaharidichev <[email protected]>

Address more comments

f33a581

Signed-off-by: Zahari Dichev <[email protected]>

zaharidichev force-pushed the zd/move-cni-to-helm branch from 1d25bfb to f33a581 Compare December 10, 2019 08:21

zaharidichev merged commit 0313f10 into linkerd:master Dec 10, 2019

Conversation

zaharidichev commented Oct 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alpeb left a comment

Choose a reason for hiding this comment

Uh oh!

grampelberg commented Oct 17, 2019

Uh oh!

zaharidichev commented Oct 17, 2019

Uh oh!

zaharidichev commented Oct 18, 2019

Uh oh!

alpeb commented Oct 18, 2019

Uh oh!

zaharidichev commented Oct 21, 2019

Uh oh!

alpeb left a comment

Choose a reason for hiding this comment

Uh oh!

alpeb Oct 21, 2019

Choose a reason for hiding this comment

Uh oh!

alpeb Oct 21, 2019

Choose a reason for hiding this comment

Uh oh!

zaharidichev commented Oct 21, 2019

Uh oh!

alpeb commented Oct 21, 2019

Uh oh!

alpeb commented Oct 21, 2019

Uh oh!

alpeb left a comment

Choose a reason for hiding this comment

Uh oh!

alpeb commented Oct 22, 2019

Uh oh!

zaharidichev commented Oct 23, 2019

Uh oh!

alpeb commented Oct 23, 2019

Uh oh!

grampelberg commented Nov 22, 2019

Uh oh!

zaharidichev commented Nov 22, 2019

Uh oh!

ihcsim left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ihcsim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ihcsim commented Dec 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

zaharidichev commented Oct 15, 2019 •

edited

Loading

ihcsim left a comment •

edited

Loading