Promote the shared injection check to the CLI and webhook by ihcsim · Pull Request #2555 · linkerd/linkerd2

ihcsim · 2019-03-25T04:22:20Z

This PR refactors the shouldInject logic by promoting it from the shared pkg/inject library to the CLI and webhook.

Performing this check earlier helps to separate the specialized logic to the CLI and webhook. Subsequent modification to this check to support configs override of existing meshed workloads will be confined to the relevant component.

This PR addresses point 1 to 3 of our previous discussion on configs override in 2500.

Signed-off-by: Ivan Sim [email protected]

cli/cmd/uninject.go

alpeb

All these refactorings certainly improve the quality of the code, thanks for taking care of this!
I added some comments below from a first review pass.

pkg/inject/report.go

pkg/inject/inject.go

alpeb · 2019-03-25T15:02:06Z

cli/cmd/inject.go

-	if !nonEmpty {
-		r := inject.Report{UnsupportedResource: true}
-		return bytes, []inject.Report{r}, nil
+	reports := []inject.Report{*report}


It appears we can deal with a single report (instead of having to always wrap in an single-item array) everywhere except in the inject_utils.go functions. That got evident in on of my branches, so I decided to tackle that later in a separate PR, but you can do that here as well if you find it's a natural change 😉

pkg/inject/report.go

alpeb · 2019-03-25T23:31:51Z

controller/proxy-injector/webhook_test.go

-				ns:       nsDisabled,
-				conf:     confNsDisabled(),
-				expected: false,
-			},


Is it enough to add the additional Injectable() test below to be able to re-enable these tests, or is it more complicated than that? If so we could track that as a separate item for the 2.3 test sprint to expedite this PR ⚡

These two test cases are no longer useful, as the results will always be true because GetPatch() no longer checks if injection should happen. Some of these tests need to be revisited, as they aren't testing anything webhook related, but not in this PR.

l5d-bot · 2019-03-26T18:00:08Z

Integration test results for d2d43ae: success 🎉
Log output: https://gist.github.com/dcc2d0ec285f93e83ef0c196abdb9d0d

l5d-bot · 2019-03-26T18:10:26Z

Integration test results for 0b11a25: success 🎉
Log output: https://gist.github.com/0948ff6e61fb92a2662bb9d738d3d25e

…webhook Performing this check earlier helps to separate the specialized logic to the CLI and webhook. Any subsequent modification of this check logic to support config override of existing meshed workload will be confined to the relevant component. The shared lib can then focus only on config overrides. Signed-off-by: Ivan Sim <[email protected]>

Signed-off-by: Ivan Sim <[email protected]>

alpeb

This looks and tests well for me

l5d-bot · 2019-03-26T21:48:45Z

Integration test results for 31c413c: success 🎉
Log output: https://gist.github.com/857c2a92cc367ee8e7b03763de032fe8

Signed-off-by: Ivan Sim <[email protected]>

l5d-bot · 2019-03-26T22:10:31Z

Integration test results for 3b45283: success 🎉
Log output: https://gist.github.com/59452101295f65f2640de191708e3a4e

klingerf

Nice, the approach seems a lot cleaner overall. But I think we've regressed on the logic that determines whether or not the webhook should inject a pod -- see my comment below.

cli/cmd/inject.go

pkg/inject/report.go

siggy

looks good! left a couple related comments...

pkg/inject/report.go

pkg/inject/report_test.go

l5d-bot · 2019-03-27T06:18:48Z

Integration test results for b312215: success 🎉
Log output: https://gist.github.com/1dfaa8b79cefd6216f95ac973254da52

l5d-bot · 2019-03-27T13:58:13Z

Integration test results for 3b45283: success 🎉
Log output: https://gist.github.com/4410a53006785c68bb7a5ff624f882cf

l5d-bot · 2019-03-27T16:17:38Z

Integration test results for 3fa3972: success 🎉
Log output: https://gist.github.com/a0769625d378838e7db793f2ecdc82ac

Signed-off-by: Ivan Sim <[email protected]>

l5d-bot · 2019-03-27T16:44:03Z

Integration test results for 3095a77: success 🎉
Log output: https://gist.github.com/11b639a776e6e46d69b31ac328b41620

pkg/inject/inject.go

pkg/inject/report.go

Signed-off-by: Ivan Sim <[email protected]>

l5d-bot · 2019-03-27T19:06:25Z

Integration test results for b340ec7: success 🎉
Log output: https://gist.github.com/e84829cf82ffd9c4a266eb9fb5480012

pkg/inject/report.go

Signed-off-by: Ivan Sim <[email protected]>

l5d-bot · 2019-03-27T20:54:43Z

Integration test results for 6b26976: success 🎉
Log output: https://gist.github.com/63d531588c1dcb8ed46b14dc3505a1e9

siggy

This change LGTM. I tested most of the permutations laid out in #2569. One thing to confirm: config.linkerd.io annotations are not honored at the namespace-level, correct?

👍 🚢

ihcsim · 2019-03-27T21:27:29Z

This change LGTM. I tested most of the permutations laid out in #2569. One thing to confirm: config.linkerd.io annotations are not honored at the namespace-level, correct?

Correct. We moved it out of 2.3 into #2499.

ihcsim requested review from alpeb and klingerf March 25, 2019 04:22

ihcsim commented Mar 25, 2019

View reviewed changes

cli/cmd/uninject.go Show resolved Hide resolved

ihcsim self-assigned this Mar 25, 2019

ihcsim added area/inject priority/P0 Release Blocker labels Mar 25, 2019

alpeb reviewed Mar 25, 2019

View reviewed changes

ihcsim force-pushed the isim/proxy-injection-check branch 2 times, most recently from d2d43ae to 0b11a25 Compare March 26, 2019 17:55

alpeb mentioned this pull request Mar 26, 2019

Add validation webhook for service profiles #2508

Closed

siggy mentioned this pull request Mar 26, 2019

Injection integration tests #2569

Closed

1 task

Ivan Sim added 4 commits March 26, 2019 14:27

Alejandro's feedback

627319c

Signed-off-by: Ivan Sim <[email protected]>

Ensure non-workload resources' metadata can be unmarshalled

e1eab33

Signed-off-by: Ivan Sim <[email protected]>

Resolve rebase error

31c413c

Signed-off-by: Ivan Sim <[email protected]>

ihcsim force-pushed the isim/proxy-injection-check branch from 0b11a25 to 31c413c Compare March 26, 2019 21:32

alpeb approved these changes Mar 26, 2019

View reviewed changes

Update comment of 'default' case in the 'switch'

3b45283

Signed-off-by: Ivan Sim <[email protected]>

klingerf reviewed Mar 26, 2019

View reviewed changes

cli/cmd/inject.go Show resolved Hide resolved

pkg/inject/report.go Outdated Show resolved Hide resolved

siggy reviewed Mar 26, 2019

View reviewed changes

pkg/inject/report.go Outdated Show resolved Hide resolved

pkg/inject/report_test.go Outdated Show resolved Hide resolved

ihcsim force-pushed the isim/proxy-injection-check branch from 774c8b7 to b312215 Compare March 27, 2019 06:00

ihcsim force-pushed the isim/proxy-injection-check branch from b312215 to 3b45283 Compare March 27, 2019 13:46

ihcsim force-pushed the isim/proxy-injection-check branch from 10f0545 to 3fa3972 Compare March 27, 2019 16:05

Support the inject disable annotation on workload originated from CLI

3095a77

Signed-off-by: Ivan Sim <[email protected]>

ihcsim force-pushed the isim/proxy-injection-check branch from 3fa3972 to 3095a77 Compare March 27, 2019 16:32

alpeb reviewed Mar 27, 2019

View reviewed changes

pkg/inject/inject.go Outdated Show resolved Hide resolved

alpeb reviewed Mar 27, 2019

View reviewed changes

pkg/inject/inject.go Outdated Show resolved Hide resolved

alpeb reviewed Mar 27, 2019

View reviewed changes

pkg/inject/report.go Outdated Show resolved Hide resolved

Ivan Sim added 2 commits March 27, 2019 11:01

Alejandro's feedback

7dd31ff

Signed-off-by: Ivan Sim <[email protected]>

Additional unit tests for inject annotation

b340ec7

Signed-off-by: Ivan Sim <[email protected]>

ihcsim force-pushed the isim/proxy-injection-check branch from feede5c to b340ec7 Compare March 27, 2019 18:51

siggy reviewed Mar 27, 2019

View reviewed changes

pkg/inject/report.go Outdated Show resolved Hide resolved

Siggy's feedback

6b26976

Signed-off-by: Ivan Sim <[email protected]>

siggy approved these changes Mar 27, 2019

View reviewed changes

ihcsim merged commit ea07dd3 into master Mar 27, 2019

ihcsim deleted the isim/proxy-injection-check branch March 27, 2019 21:51

Conversation

ihcsim commented Mar 25, 2019

Uh oh!

Uh oh!

alpeb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alpeb Mar 25, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alpeb Mar 25, 2019

Choose a reason for hiding this comment

Uh oh!

ihcsim Mar 26, 2019

Choose a reason for hiding this comment

Uh oh!

l5d-bot commented Mar 26, 2019

Uh oh!

l5d-bot commented Mar 26, 2019

Uh oh!

alpeb left a comment

Choose a reason for hiding this comment

Uh oh!

l5d-bot commented Mar 26, 2019

Uh oh!

l5d-bot commented Mar 26, 2019

Uh oh!

klingerf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

siggy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

l5d-bot commented Mar 27, 2019

Uh oh!

l5d-bot commented Mar 27, 2019

Uh oh!

l5d-bot commented Mar 27, 2019

Uh oh!

l5d-bot commented Mar 27, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

l5d-bot commented Mar 27, 2019

Uh oh!

Uh oh!

l5d-bot commented Mar 27, 2019

Uh oh!

siggy left a comment

Choose a reason for hiding this comment

Uh oh!

ihcsim commented Mar 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ihcsim commented Mar 27, 2019 •

edited

Loading