Refactor proxy injection to use Helm charts #3195

alpeb · 2019-08-05T15:36:57Z

New `patch` chart

A new chart /charts/patch was created, that generates the JSON patch
payload that is to be returned to the k8s API when doing the injection
through the proxy injector, and it's also leveraged by the linkerd inject --manual CLI.

Virtual File System changes

The VFS was used by linkerd install to access the old chart under
/chart. Now the proxy injection also uses the Helm charts to generate
the JSON patch (see above) so we've moved the VFS from cli/static to a
new common place under /pkg/charts/static, and the new root for the VFS is
now /charts.

linkerd install hasn't yet migrated to use the new charts (that'll
happen in #3127), so the only change in that regard was the creation of
/charts/chart which is a symlink pointing to /chart that
install.go now uses, so that the VFS contains both the old and new
charts, as a temporary measure.

You can see that /bin/Dockerfile-bin, /controller/Dockerfile and
/bin/build-cli-bin do now go generate pointing to the new location
(and the go generate annotation was moved from /cli/main.go to
pkg/charts/static/templates.go).

The symlink trick doesn't work when building the binaries through
Docker, so /bin/Dockerfile-bin replaces the symlink with an actual
copy of /chart.

Also note that in /controller/Dockerfile we now need to include the
prod tag in go install like we do in /bin/Dockerfile-bin so that
the proxy injector does use the VFS instead of the local file system.

Other changes

The common logic to parse a chart has been moved from install.go to
/pkg/charts/util.go.
The special ENV var in the proxy for "outbound router capacity" that
only applies to the Prometheus pod is now handled directly in the proxy
partial and all the associated go code could be removed.
The patch.go lib for generating the JSON patch in go along
with its tests patch_test.go are no longer needed.
Lots of functions in /pkg/inject/inject.go got removed/simplified
with their logic being moved into the charts themselves. As a
consequence lots of things in inject_test.go became irrelevant.
Moved template-values.go from /pkg/inject to pkg/charts as that
contains the go structs representation of the chart variables that
will be leveraged in Refactor CLI Install Command To Work With 2.5 Helm Chart #3127.

Testing

Don't forget to run /bin/helm.sh whenever you make changes to charts
;-)

Fixes #3128 New `patch` chart ================== A new chart `/charts/patch` was created, that generates the JSON patch payload that is to be returned to the k8s API when doing the injection through the proxy injector, and it's also leveraged by the `linkerd inject --manual` CLI. Virtual File System changes =========================== The VFS was used by `linkerd install` to access the old chart under `/chart`. Now the proxy injection also uses the Helm charts to generate the JSON patch (see above) so we've moved the VFS from `cli/static` to a new common place under `/pkg/charts/static`, and the new root for the VFS is now `/charts`. `linkerd install` hasn't yet migrated to use the new charts (that'll happen in #3127), so the only change in that regard was the creation of `/charts/chart` which is a symlink pointing to `/chart` that `install.go` now uses, so that the VFS contains both the old and new charts, as a temporary measure. You can see that `/bin/Dockerfile-bin`, `/controller/Dockerfile` and `/bin/build-cli-bin` do now `go generate` pointing to the new location (and the `go generate` annotation was moved from `/cli/main.go` to `pkg/charts/static/templates.go`). The symlink trick doesn't work when building the binaries through Docker, so `/bin/Dockerfile-bin` replaces the symlink with an actual copy of `/chart`. Also note that in `/controller/Dockerfile` we now need to include the `prod` tag in `go install` like we do in `/bin/Dockerfile-bin` so that the proxy injector does use the VFS instead of the local file system. Other changes =============== - The common logic to parse a chart has been moved from `install.go` to `/pkg/charts/util.go`. - The special ENV var in the proxy for "outbound router capacity" that only applies to the Prometheus pod is now handled directly in the proxy partial and all the associated go code could be removed. - The `patch.go` lib for generating the JSON patch in go along with its tests `patch_test.go` are no longer needed. - Lots of functions in `/pkg/inject/inject.go` got removed/simplified with their logic being moved into the charts themselves. As a consequence lots of things in `inject_test.go` became irrelevant. - Moved `template-values.go` from `/pkg/inject` to `pkg/charts` as that contains the go structs representation of the chart variables that will be leveraged in #3127. Testing ========= Don't forget to run `/bin/helm.sh` whenever you make changes to charts ;-) Signed-off-by: Alejandro Pedraza <alejandro@buoyant.io>

alpeb · 2019-08-05T15:37:53Z

cli/cmd/install.go

-	// Skip outbound port 443 to enable Kubernetes API access without the proxy.
-	// Once Kubernetes supports sidecar containers, this may be removed, as that
-	// will guarantee the proxy is running prior to control-plane startup.
-	configs.Proxy.IgnoreOutboundPorts = append(configs.Proxy.IgnoreOutboundPorts, &pb.Port{Port: 443})


This is now handled directly in the chart

alpeb · 2019-08-05T15:38:48Z

pkg/inject/inject.go

-	identityAPIPort = 8080
-
-	envTapDisabled = "LINKERD2_PROXY_TAP_DISABLED"
-	envTapSvcName  = "LINKERD2_PROXY_TAP_SVC_NAME"


All these consts are now in the charts

alpeb · 2019-08-05T15:39:21Z

controller/proxy-injector/webhook_test.go

 		}{
 			{
 				filename: "pod-inject-empty.yaml",
 				ns:       nsEnabled,
 				conf:     confNsEnabled(),
-				expected: true,


expected is true in all the test cases, so I removed it.

l5d-bot · 2019-08-05T15:57:15Z

Integration test results for 8cf5b42: fail 😕
Log output: https://gist.github.com/e589a79b25ff8ab16b5eb39fb26ba421

siggy

i'm going to dig into a more full review, but leaving this here for now as it was the first thing i encountered.

my quick remote build and install yielded an error message followed by a partial installation:

$ bin/docker-build # on a remote machine

$ bin/linkerd install | kubectl apply -f - # bin/go-run cli install yields the same issue
Error: open /patch/charts/partials-0.1.0.tgz: file does not exist
Usage:
  linkerd install [flags]
  linkerd install [command]
...
Use "linkerd install [command] --help" for more information about a command.

namespace/linkerd created
clusterrole.rbac.authorization.k8s.io/linkerd-linkerd-identity created
clusterrolebinding.rbac.authorization.k8s.io/linkerd-linkerd-identity created
serviceaccount/linkerd-identity created
clusterrole.rbac.authorization.k8s.io/linkerd-linkerd-controller created
clusterrolebinding.rbac.authorization.k8s.io/linkerd-linkerd-controller created
serviceaccount/linkerd-controller created
role.rbac.authorization.k8s.io/linkerd-heartbeat created
rolebinding.rbac.authorization.k8s.io/linkerd-heartbeat created
serviceaccount/linkerd-heartbeat created
serviceaccount/linkerd-web created
customresourcedefinition.apiextensions.k8s.io/serviceprofiles.linkerd.io created
customresourcedefinition.apiextensions.k8s.io/trafficsplits.split.smi-spec.io created
clusterrole.rbac.authorization.k8s.io/linkerd-linkerd-prometheus created
clusterrolebinding.rbac.authorization.k8s.io/linkerd-linkerd-prometheus created
serviceaccount/linkerd-prometheus created
serviceaccount/linkerd-grafana created
clusterrole.rbac.authorization.k8s.io/linkerd-linkerd-proxy-injector created
clusterrolebinding.rbac.authorization.k8s.io/linkerd-linkerd-proxy-injector created
serviceaccount/linkerd-proxy-injector created
secret/linkerd-proxy-injector-tls created
mutatingwebhookconfiguration.admissionregistration.k8s.io/linkerd-proxy-injector-webhook-config created
clusterrole.rbac.authorization.k8s.io/linkerd-linkerd-sp-validator created
clusterrolebinding.rbac.authorization.k8s.io/linkerd-linkerd-sp-validator created
serviceaccount/linkerd-sp-validator created
secret/linkerd-sp-validator-tls created
validatingwebhookconfiguration.admissionregistration.k8s.io/linkerd-sp-validator-webhook-config created
clusterrole.rbac.authorization.k8s.io/linkerd-linkerd-tap created
clusterrolebinding.rbac.authorization.k8s.io/linkerd-linkerd-tap created
clusterrolebinding.rbac.authorization.k8s.io/linkerd-linkerd-tap-auth-delegator created
serviceaccount/linkerd-tap created
rolebinding.rbac.authorization.k8s.io/linkerd-linkerd-tap-auth-reader created
secret/linkerd-tap-tls created
apiservice.apiregistration.k8s.io/v1alpha1.tap.linkerd.io created
podsecuritypolicy.policy/linkerd-linkerd-control-plane created
role.rbac.authorization.k8s.io/linkerd-psp created
rolebinding.rbac.authorization.k8s.io/linkerd-psp created
configmap/linkerd-config created
secret/linkerd-identity-issuer created
service/linkerd-identity created

it looks like the install command errors out after outputting service/linkerd-identity:

$ bin/go-run cli install
...
kind: Service
apiVersion: v1
metadata:
  name: linkerd-identity
  namespace: linkerd
  labels:
    linkerd.io/control-plane-component: identity
    linkerd.io/control-plane-ns: linkerd
  annotations:
    linkerd.io/created-by: linkerd/cli git-8cf5b428
spec:
  type: ClusterIP
  selector:
    linkerd.io/control-plane-component: identity
  ports:
  - name: grpc
    port: 8080
    targetPort: 8080
---
Error: open /Users/sig/code/linkerd2/charts/patch/charts/partials-0.1.0.tgz: no such file or directory
Usage:
  linkerd install [flags]
  linkerd install [command]

avoid requiring the build environments to have `helm` installed. Signed-off-by: Alejandro Pedraza <alejandro@buoyant.io>

alpeb · 2019-08-05T20:43:17Z

@siggy my last push fixes things so that dependencies are taken care of automatically without forcing the build environments to have helm to bring them up-to-date.

But in order to install linkerd through helm (helm install charts/linkerd2), given that doesn't go through our go code, people do need to run helm dep up charts/linkerd2 beforehand, for development. In prod that won't be necessary when the linkerd2 chart gets published in the official repo, and people would be able to just issue helm install stable/linkerd2.

l5d-bot · 2019-08-05T21:00:01Z

Integration test results for 41c64f1: fail 😕
Log output: https://gist.github.com/e862d4e7b82b26e38885c927f3a5c886

siggy

looks good! mostly nits and questions. 🚢 👍

mind adding a section to BUILD.md and/or TEST.md around when to use commands like bin/helm.sh and helm dep up charts/linkerd2? maybe documenting the typical dev flow of modifying templates and testing a helm install command?

cli/cmd/testdata/install_output.golden

pkg/charts/util.go

Signed-off-by: Alejandro Pedraza <alejandro@buoyant.io>

l5d-bot · 2019-08-06T14:19:07Z

Integration test results for 4250749: fail 😕
Log output: https://gist.github.com/345af8f4d41b24d7a28764ae96b0a7c4

alpeb · 2019-08-06T14:26:47Z

mind adding a section to BUILD.md and/or TEST.md around when to use commands like bin/helm.sh and helm dep up charts/linkerd2? maybe documenting the typical dev flow of modifying templates and testing a helm install command?

Thanks for the feedback! I've created #3199 in the Helm project to track Helm docs effort 👍

siggy

👍 🚢

Signed-off-by: Alejandro Pedraza <alejandro@buoyant.io>

ihcsim · 2019-08-06T16:25:36Z

@alpeb Opppss... looks like GH deleted this branch after I merged #3146, because this branch is set to be merged into the other feature branch. Can you reopen this PR? (GH doesn't offer me the option to reopen this PR.)

l5d-bot · 2019-08-06T16:40:57Z

Integration test results for 921b2d9: fail 😕
Log output: https://gist.github.com/7cfe885c66201a3c7ddad4d94ddd6d9f

(This replaces #3195 that had some branch issues) Fixes #3128 A new chart `/charts/patch` was created, that generates the JSON patch payload that is to be returned to the k8s API when doing the injection through the proxy injector, and it's also leveraged by the `linkerd inject --manual` CLI. The VFS was used by `linkerd install` to access the old chart under `/chart`. Now the proxy injection also uses the Helm charts to generate the JSON patch (see above) so we've moved the VFS from `cli/static` to a new common place under `/pkg/charts/static`, and the new root for the VFS is now `/charts`. `linkerd install` hasn't yet migrated to use the new charts (that'll happen in #3127), so the only change in that regard was the creation of `/charts/chart` which is a symlink pointing to `/chart` that `install.go` now uses, so that the VFS contains both the old and new charts, as a temporary measure. You can see that `/bin/Dockerfile-bin`, `/controller/Dockerfile` and `/bin/build-cli-bin` do now `go generate` pointing to the new location (and the `go generate` annotation was moved from `/cli/main.go` to `pkg/charts/static/templates.go`). The symlink trick doesn't work when building the binaries through Docker, so `/bin/Dockerfile-bin` replaces the symlink with an actual copy of `/chart`. Also note that in `/controller/Dockerfile` we now need to include the `prod` tag in `go install` like we do in `/bin/Dockerfile-bin` so that the proxy injector does use the VFS instead of the local file system. - The common logic to parse a chart has been moved from `install.go` to `/pkg/charts/util.go`. - The special ENV var in the proxy for "outbound router capacity" that only applies to the Prometheus pod is now handled directly in the proxy partial and all the associated go code could be removed. - The `patch.go` lib for generating the JSON patch in go along with its tests `patch_test.go` are no longer needed. - Lots of functions in `/pkg/inject/inject.go` got removed/simplified with their logic being moved into the charts themselves. As a consequence lots of things in `inject_test.go` became irrelevant. - Moved `template-values.go` from `/pkg/inject` to `pkg/charts` as that contains the go structs representation of the chart variables that will be leveraged in #3127. Don't forget to run `/bin/helm.sh` whenever you make changes to charts ;-) Signed-off-by: Alejandro Pedraza <alejandro@buoyant.io>

alpeb · 2019-08-06T17:52:24Z

This PR has been replaced with #3200

alpeb added area/controller area/cli size/large area/inject area/install labels Aug 5, 2019

alpeb requested review from siggy, ihcsim and kleimkuhler August 5, 2019 15:36

alpeb self-assigned this Aug 5, 2019

alpeb commented Aug 5, 2019

View reviewed changes

ihcsim mentioned this pull request Aug 5, 2019

Add Control Plane Helm Templates And Proxy Partials #3146

Merged

siggy reviewed Aug 5, 2019

View reviewed changes

Manually load the dependencies for the linkerd2 and patch charts to

41c64f1

avoid requiring the build environments to have `helm` installed. Signed-off-by: Alejandro Pedraza <alejandro@buoyant.io>

siggy approved these changes Aug 5, 2019

View reviewed changes

cli/cmd/testdata/install_output.golden Show resolved Hide resolved

pkg/charts/util.go Outdated Show resolved Hide resolved

pkg/charts/util.go Outdated Show resolved Hide resolved

Siggy's feedback

4250749

Signed-off-by: Alejandro Pedraza <alejandro@buoyant.io>

siggy approved these changes Aug 6, 2019

View reviewed changes

Fixed auto-injection

921b2d9

Signed-off-by: Alejandro Pedraza <alejandro@buoyant.io>

ihcsim closed this Aug 6, 2019

alpeb mentioned this pull request Aug 6, 2019

Refactor proxy injection to use Helm charts #3200

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor proxy injection to use Helm charts #3195

Refactor proxy injection to use Helm charts #3195

alpeb commented Aug 5, 2019

alpeb Aug 5, 2019

alpeb Aug 5, 2019

alpeb Aug 5, 2019

l5d-bot commented Aug 5, 2019

siggy left a comment •

edited

Loading

alpeb commented Aug 5, 2019

l5d-bot commented Aug 5, 2019

siggy left a comment

l5d-bot commented Aug 6, 2019

alpeb commented Aug 6, 2019

siggy left a comment

ihcsim commented Aug 6, 2019

l5d-bot commented Aug 6, 2019

alpeb commented Aug 6, 2019

Refactor proxy injection to use Helm charts #3195

Refactor proxy injection to use Helm charts #3195

Conversation

alpeb commented Aug 5, 2019

New patch chart

Virtual File System changes

Other changes

Testing

alpeb Aug 5, 2019

Choose a reason for hiding this comment

alpeb Aug 5, 2019

Choose a reason for hiding this comment

alpeb Aug 5, 2019

Choose a reason for hiding this comment

l5d-bot commented Aug 5, 2019

siggy left a comment • edited Loading

Choose a reason for hiding this comment

alpeb commented Aug 5, 2019

l5d-bot commented Aug 5, 2019

siggy left a comment

Choose a reason for hiding this comment

l5d-bot commented Aug 6, 2019

alpeb commented Aug 6, 2019

siggy left a comment

Choose a reason for hiding this comment

ihcsim commented Aug 6, 2019

l5d-bot commented Aug 6, 2019

alpeb commented Aug 6, 2019

New `patch` chart

siggy left a comment •

edited

Loading