Introduce inject integration tests by siggy · Pull Request #2616 · linkerd/linkerd2

siggy · 2019-04-02T17:54:36Z

This change introduces integration tests for linkerd inject. The tests
perform CLI injection, with and without params, and validates the
output, including annotations.

Also add some known errors in logs to install_test.go.

TODO:

deploy uninjected and injected resources to a default and
auto-injected cluster
test creation and update

Part of #2459

Signed-off-by: Andrew Seigner [email protected]

l5d-bot · 2019-04-02T21:10:06Z

Integration test results for ce3c160: fail 😕
Log output: https://gist.github.com/b148efa90359f09f184bac1686fdffc6

l5d-bot · 2019-04-02T22:13:13Z

Integration test results for cac1276: success 🎉
Log output: https://gist.github.com/b64f81364b0bbdcee17ada414f1b7d31

l5d-bot · 2019-04-04T01:42:03Z

Integration test results for 2f77ba3: fail 😕
Log output: https://gist.github.com/e744e34e7ce82a3d738d3bc5464f1912

l5d-bot · 2019-04-04T02:05:35Z

Integration test results for f0c441d: fail 😕
Log output: https://gist.github.com/f86c46d10bc128b0df1f001f6ffc752c

l5d-bot · 2019-04-04T17:30:09Z

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

alpeb

According to this

linkerd2/test/inject/inject_test.go

Line 128 in 9cece5e

    
           o, err := TestHelper.Kubectl(patchedYAML, "--namespace", ns, "create", "-f", "-")

you're relying on the proxy-injector that one of the install_test.go runs set up to get the pods injected in TestAnnotationPermutations, right?

I got confused by that a little bit. I think it'd be better to disassociate tests more, by having install_test.go clean up the webhook after it's done, and setting a new one for this test (to also be cleaned when finished).

All the other changes looked good to me. I left below a couple of nits.

alpeb · 2019-04-04T17:43:32Z

test/inject/inject_test.go

+	patchJSON := []byte(`[
+		{"op": "remove", "path": "/spec/template/metadata/annotations/linkerd.io~1created-by"},
+		{"op": "remove", "path": "/spec/template/metadata/annotations/linkerd.io~1proxy-version"}
+	]`)


Nice leverage of json-patch here 😉

alpeb · 2019-04-04T17:45:23Z

test/inject/inject_test.go

@@ -0,0 +1,282 @@
+package get


Should be inject

alpeb · 2019-04-04T20:35:21Z

test/inject/inject_test.go

+			case "", k8s.ProxyInjectDisabled:
+				switch podAnnotation {
+				case k8s.ProxyInjectEnabled:
+					shouldBeInjected = true


Why favoring a switch here instead of a simple if?

It's completely subjective. I tried writing this both ways, and I find the switch slightly more readable than the if.

Here they are side-by-side, let me know what you think:

shouldBeInjected := false if nsAnnotation == "" || nsAnnotation == k8s.ProxyInjectDisabled { shouldBeInjected = podAnnotation == k8s.ProxyInjectEnabled } else if nsAnnotation == k8s.ProxyInjectEnabled { shouldBeInjected = (podAnnotation == "" || podAnnotation == k8s.ProxyInjectEnabled) }

shouldBeInjected := false switch nsAnnotation { case "", k8s.ProxyInjectDisabled: switch podAnnotation { case k8s.ProxyInjectEnabled: shouldBeInjected = true } case k8s.ProxyInjectEnabled: switch podAnnotation { case "", k8s.ProxyInjectEnabled: shouldBeInjected = true } }

The outer switch looks good. I was gonna say ifs are better in the inner conditions, but not so much for the second one where an or is involved. But it's definitely more readable keeping all switches for consistency-sake. I'm gonna steal this idiom

alpeb · 2019-04-04T21:25:37Z

Actually I'm not sure about my previous comment anymore. All the integration tests rely on the setup done by install_test.go. So in this case a simple comment might suffice, clarifying that the control plane is configured for auto-injection at this point.

siggy · 2019-04-04T21:43:41Z

@alpeb Yup there's an assumption with the integration tests that install_test.go always runs first: https://github.com/linkerd/linkerd2/blob/38f504beb14cf216443ec7a6bad147d25c0d308e/TEST.md#running-tests

Agree that inject_test.go is still a little special, in that is specifically requires a control-plane with auto-injection. I've added a comment above TestAnnotationPermutations calling this out.

This change introduces integration tests for `linkerd inject`. The tests perform CLI injection, with and without params, and validates the output, including annotations. Also add some known errors in logs to `install_test.go`. TODO: - deploy uninjected and injected resources to a default and auto-injected cluster - test creation and update Part of #2459 Signed-off-by: Andrew Seigner <[email protected]>

Signed-off-by: Andrew Seigner <[email protected]>

l5d-bot · 2019-04-04T22:16:29Z

Integration test results for 88332c8: success 🎉
Log output: https://gist.github.com/887722bf46fce667d3bc99eec13c8576

alpeb

Looks good to me 👍

This change introduces integration tests for `linkerd inject`. The tests perform CLI injection, with and without params, and validates the output, including annotations. Also add some known errors in logs to `install_test.go`. TODO: - deploy uninjected and injected resources to a default and auto-injected cluster - test creation and update Part of linkerd#2459 Signed-off-by: Andrew Seigner <[email protected]> Signed-off-by: [email protected] <[email protected]>

siggy added the area/test label Apr 2, 2019

siggy self-assigned this Apr 2, 2019

siggy force-pushed the siggy/inject-integration branch 3 times, most recently from 851af62 to dd7cacb Compare April 2, 2019 19:49

siggy changed the title ~~Introduce inject integration tests~~ DO NOT REVIEW: Introduce inject integration tests Apr 2, 2019

siggy force-pushed the siggy/inject-integration branch from dd7cacb to ce3c160 Compare April 2, 2019 20:56

siggy force-pushed the siggy/inject-integration branch from ce3c160 to cac1276 Compare April 2, 2019 22:04

linkerd deleted a comment from l5d-bot Apr 2, 2019

siggy changed the title ~~DO NOT REVIEW: Introduce inject integration tests~~ Introduce inject integration tests Apr 2, 2019

admc mentioned this pull request Apr 2, 2019

Linkerd 2.3 release test plan #2459

Closed

25 tasks

siggy force-pushed the siggy/inject-integration branch from c9e7b2e to 2f77ba3 Compare April 4, 2019 01:15

siggy force-pushed the siggy/inject-integration branch 2 times, most recently from b2e39b3 to f0c441d Compare April 4, 2019 01:49

siggy requested review from alpeb and ihcsim April 4, 2019 17:07

siggy force-pushed the siggy/inject-integration branch from f0c441d to 9cece5e Compare April 4, 2019 17:15

alpeb reviewed Apr 4, 2019

View reviewed changes

siggy added 2 commits April 4, 2019 14:49

review feedback

88332c8

Signed-off-by: Andrew Seigner <[email protected]>

siggy force-pushed the siggy/inject-integration branch from cfbac81 to 88332c8 Compare April 4, 2019 21:49

alpeb approved these changes Apr 5, 2019

View reviewed changes

siggy merged commit 2f80add into master Apr 5, 2019

siggy deleted the siggy/inject-integration branch April 5, 2019 18:42

Conversation

siggy commented Apr 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

l5d-bot commented Apr 2, 2019

Uh oh!

l5d-bot commented Apr 2, 2019

Uh oh!

l5d-bot commented Apr 4, 2019

Uh oh!

l5d-bot commented Apr 4, 2019

Uh oh!

l5d-bot commented Apr 4, 2019

Uh oh!

alpeb left a comment

Choose a reason for hiding this comment

Uh oh!

alpeb Apr 4, 2019

Choose a reason for hiding this comment

Uh oh!

alpeb Apr 4, 2019

Choose a reason for hiding this comment

Uh oh!

alpeb Apr 4, 2019

Choose a reason for hiding this comment

Uh oh!

siggy Apr 4, 2019

Choose a reason for hiding this comment

Uh oh!

alpeb Apr 4, 2019

Choose a reason for hiding this comment

Uh oh!

alpeb commented Apr 4, 2019

Uh oh!

siggy commented Apr 4, 2019

Uh oh!

l5d-bot commented Apr 4, 2019

Uh oh!

alpeb left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

siggy commented Apr 2, 2019 •

edited

Loading