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

Refactor proxy injection to use Helm charts #3200

Merged
merged 7 commits into from
Aug 7, 2019
Merged

Conversation

alpeb
Copy link
Member

@alpeb alpeb commented Aug 6, 2019

(This replaces #3195 that had some branch issues)

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 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
;-)

(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>
@l5d-bot
Copy link
Collaborator

l5d-bot commented Aug 6, 2019

Integration test results for 4e4ab8c: success 🎉
Log output: https://gist.github.com/26358324f6781f0fb05c26739f91dd0b

Signed-off-by: Alejandro Pedraza <alejandro@buoyant.io>
@l5d-bot
Copy link
Collaborator

l5d-bot commented Aug 6, 2019

Integration test results for 0bee203: fail 😕
Log output: https://gist.github.com/87589dab00544126f01c191708b0329d

charts/patch/templates/patch.json Show resolved Hide resolved
pkg/inject/inject.go Show resolved Hide resolved
pkg/inject/inject.go Show resolved Hide resolved
pkg/charts/charts.go Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
charts/chart Show resolved Hide resolved
pkg/inject/inject.go Outdated Show resolved Hide resolved
cli/cmd/testdata/install_output.golden Outdated Show resolved Hide resolved
Signed-off-by: Alejandro Pedraza <alejandro@buoyant.io>
@l5d-bot
Copy link
Collaborator

l5d-bot commented Aug 6, 2019

Integration test results for 199b602: success 🎉
Log output: https://gist.github.com/3423e9e34a7a7279dd0edee89177580b

Copy link
Member

@siggy siggy left a comment

Choose a reason for hiding this comment

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

lgtm modulo ivan's last question 🚢 👍

…bled

Signed-off-by: Alejandro Pedraza <alejandro@buoyant.io>
Signed-off-by: Alejandro Pedraza <alejandro@buoyant.io>
@l5d-bot
Copy link
Collaborator

l5d-bot commented Aug 7, 2019

Integration test results for 04085cd: success 🎉
Log output: https://gist.github.com/3a0f44fda485e9dc6578dc80c884c447

pkg/inject/inject.go Outdated Show resolved Hide resolved
Signed-off-by: Alejandro Pedraza <alejandro@buoyant.io>
@l5d-bot
Copy link
Collaborator

l5d-bot commented Aug 7, 2019

Integration test results for 7908e1a: fail 😕
Log output: https://gist.github.com/c633ebf9e2fb003e920cd68f2d7d2e32

Signed-off-by: Alejandro Pedraza <alejandro@buoyant.io>
@l5d-bot
Copy link
Collaborator

l5d-bot commented Aug 7, 2019

Integration test results for 9d29eeb: success 🎉
Log output: https://gist.github.com/d5f27bf9fd0b0b9eda466dd95cec9650

Copy link
Contributor

@ihcsim ihcsim left a comment

Choose a reason for hiding this comment

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

👍 Let's ship this!

@alpeb alpeb merged commit 3ae653a into master Aug 7, 2019
@alpeb alpeb deleted the alpeb/helm-injection3 branch August 7, 2019 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor Proxy Injection To Work With 2.5 Helm Chart
4 participants