New `linkerd inject` default and advanced modes #2721

alpeb · 2019-04-19T18:42:34Z

Fixes #2710 and #2711 (made more sense to tackle both at once)

This changes the default behavior of linkerd inject to not inject the
proxy but just the linkerd.io/inject: enabled annotation for the
auto-injector to pick it up (regardless of any namespace annotation).

A new --advanced mode was added, which behaves as before, injecting
the proxy in the command output.

The unit tests are running with --advanced to avoid any changes in the
fixtures.

I still need to add a new test for non-advanced mode and fix the
integration tests.

l5d-bot · 2019-04-19T19:24:30Z

Integration test results for 49e6ead: fail 😕
Log output: https://gist.github.com/fec4cbeeaebd8859eb619eb4bfd94dde

pkg/inject/inject.go

siggy

We shouldn't necessarily block merging this on #2715, but I think we should block an edge release on this until #2715 merges.

~~With --advanced, I'm getting some unexpected output:~~

linkerd.io/proxy-version: ""
...
image: 'gcr.io/linkerd-io/proxy:'

alpeb · 2019-04-19T22:16:35Z

We shouldn't necessarily block merging this on #2715, but I think we should block an edge release on this until #2715 merges.

Yeah I was kinda waiting for #2723 to be finish before tackling #2715, but it seems multi-stage install still needs quite a few more pieces. I'll do #2715 on Monday, should be a quickie.

As for the empty "proxy-version", how can I reproduce that?

siggy · 2019-04-19T22:49:03Z

As for the empty "proxy-version", how can I reproduce that?

My mistake, I was diffing linkerd inject - against bin/go-run cli inject --advanced -. The behavior I reported already exists on master.

dadjeibaah

This is looking great. Thanks for adding this. Just had one TIOLI. What do you think about changing inject.NewResourceConfig so it accepts the injectProxy as a configuration parameter. That way there will be no need to change GetPatch's signature in install, inject and webhook methods. Something like:

diff --git a/cli/cmd/inject.go b/cli/cmd/inject.go
index a4d0f10c..fd3b46d7 100644
--- a/cli/cmd/inject.go
+++ b/cli/cmd/inject.go
@@ -132,7 +132,7 @@ func uninjectAndInject(inputs []io.Reader, errWriter, outWriter io.Writer, trans
 }
 
 func (rt resourceTransformerInject) transform(bytes []byte) ([]byte, []inject.Report, error) {
-	conf := inject.NewResourceConfig(rt.configs, inject.OriginCLI)
+	conf := inject.NewResourceConfig(rt.configs, rt.injectProxy, inject.OriginCLI)
 	if len(rt.proxyOutboundCapacity) > 0 {
 		conf = conf.WithProxyOutboundCapacity(rt.proxyOutboundCapacity)
 	}
@@ -158,7 +158,7 @@ func (rt resourceTransformerInject) transform(bytes []byte) ([]byte, []inject.Re
 		conf.AppendPodAnnotations(rt.overrideAnnotations)
 	}
 
-	p, err := conf.GetPatch(bytes, rt.injectProxy)
+	p, err := conf.GetPatch(bytes)
 	if err != nil {
 		return nil, nil, err
 	}
diff --git a/controller/proxy-injector/webhook.go b/controller/proxy-injector/webhook.go
index 3ada3a74..ecad4e5a 100644
--- a/controller/proxy-injector/webhook.go
+++ b/controller/proxy-injector/webhook.go
@@ -61,7 +61,7 @@ func Inject(api *k8s.API,
 	resourceConfig.AppendPodAnnotations(map[string]string{
 		pkgK8s.CreatedByAnnotation: fmt.Sprintf("linkerd/proxy-injector %s", version.Version),
 	})
-	p, err := resourceConfig.GetPatch(request.Object.Raw, true)
+	p, err := resourceConfig.GetPatch(request.Object.Raw)
 	if err != nil {
 		return nil, err
 	}
diff --git a/controller/proxy-injector/webhook_test.go b/controller/proxy-injector/webhook_test.go
index 481f507a..27163faa 100644
--- a/controller/proxy-injector/webhook_test.go
+++ b/controller/proxy-injector/webhook_test.go
@@ -119,7 +119,7 @@ func TestGetPatch(t *testing.T) {
 					t.Fatal(err)
 				}
 
-				p, err := fullConf.GetPatch(fakeReq.Object.Raw, true)
+				p, err := fullConf.GetPatch(fakeReq.Object.Raw)
 				if err != nil {
 					t.Fatalf("Unexpected PatchForAdmissionRequest error: %s", err)
 				}
@@ -153,7 +153,7 @@ func TestGetPatch(t *testing.T) {
 
 		fakeReq := getFakeReq(deployment)
 		conf := confNsDisabled().WithKind(fakeReq.Kind.Kind)
-		p, err := conf.GetPatch(fakeReq.Object.Raw, true)
+		p, err := conf.GetPatch(fakeReq.Object.Raw)
 		if err != nil {
 			t.Fatalf("Unexpected PatchForAdmissionRequest error: %s", err)
 		}
diff --git a/pkg/inject/inject.go b/pkg/inject/inject.go
index 986791e9..1c47db85 100644
--- a/pkg/inject/inject.go
+++ b/pkg/inject/inject.go
@@ -105,6 +105,7 @@ type ResourceConfig struct {
 	proxyOutboundCapacity  map[string]uint
 	ownerRetriever         OwnerRetrieverFunc
 	origin                 Origin
+	injectProxy            bool
 
 	workload struct {
 		obj      runtime.Object
@@ -124,11 +125,12 @@ type ResourceConfig struct {
 }
 
 // NewResourceConfig creates and initializes a ResourceConfig
-func NewResourceConfig(configs *config.All, origin Origin) *ResourceConfig {
+func NewResourceConfig(configs *config.All, injectProxy bool, origin Origin) *ResourceConfig {
 	config := &ResourceConfig{
 		configs:               configs,
 		proxyOutboundCapacity: map[string]uint{},
 		origin:                origin,
+		injectProxy:           injectProxy,
 	}
 
 	config.pod.meta = &metav1.ObjectMeta{}
@@ -194,13 +196,13 @@ func (conf *ResourceConfig) ParseMetaAndYAML(bytes []byte) (*Report, error) {
 
 // GetPatch returns the JSON patch containing the proxy and init containers specs, if any.
 // If injectProxy is false, only the config.linkerd.io annotations are set.
-func (conf *ResourceConfig) GetPatch(bytes []byte, injectProxy bool) (*Patch, error) {
+func (conf *ResourceConfig) GetPatch(bytes []byte) (*Patch, error) {
 	patch := NewPatch(conf.workload.metaType.Kind)
 	if conf.pod.spec != nil {
 		if len(conf.pod.meta.Annotations) == 0 {
 			patch.addPodAnnotationsRoot()
 		}
-		if injectProxy {
+		if conf.injectProxy {
 			conf.injectObjectMeta(patch)
 			conf.injectPodSpec(patch)
 		}

cli/cmd/inject.go

alpeb · 2019-04-22T17:51:16Z

My latest push contains a new test for CLI config overrides, one new test for non-advanced mode, and adds --advanced there where injection without the auto-injector is made. In #2715 the auto-injector will become mandatory, so we might adjust the integration tests accordingly by removing that flag wherever possible.

l5d-bot · 2019-04-22T18:15:37Z

Integration test results for 7cff6f6: success 🎉
Log output: https://gist.github.com/7d0a26f3f89bf93dfbdf7cfd94f64ac8

siggy

this all looks good! i ran into an auto-inject issue, reported in #2731, but i don't think that's related to this branch.

This changes the default behavior of `linkerd inject` to not inject the proxy but just the `linkerd.io/inject: enabled` annotation for the auto-injector to pick it up (regardless of any namespace annotation). A new `--advanced` mode was added, which behaves as before, injecting the proxy in the command output. The unit tests are running with `--advanced` to avoid any changes in the fixtures. I still need to add a new test for non-advanced mode and fix the integration tests. Signed-off-by: Alejandro Pedraza <[email protected]>

…tion tests to account for CLI options overridding Signed-off-by: Alejandro Pedraza <[email protected]>

Signed-off-by: Alejandro Pedraza <[email protected]>

Added --advanced there where injection without the auto-injector is made. In #2715 the auto-injector will become mandatory, so we might adjust the integration tests accordingly by removing that flag wherever possible. Signed-off-by: Alejandro Pedraza <[email protected]>

Signed-off-by: Alejandro Pedraza <[email protected]>

l5d-bot · 2019-04-23T15:16:13Z

Integration test results for 3626214: success 🎉
Log output: https://gist.github.com/aaf0cc5e6081d727de4bb8388e3341e5

adleong · 2019-04-23T22:21:41Z

The flag name of "advanced" is not very descriptive of what this flag actually does (although the flag help text does a good job of explaining it). What do you think of something like "manual" instead of "advanced"? "Advanced" sounds aspirational; as if you should strive to become an advanced user so that you can use inject in advanced mode. But, in reality, no one should be using this flag unless they have a very specific usecase for it.

adleong · 2019-04-23T22:25:21Z

The existence of this flag means that we can't assume that meshed pods have been injected by the auto-injector and we still have to support both ways that a pod can become meshed. Is the plan to eventually phase this flag out? That would yield a smaller surface area to support and let us make more assumptions about meshed pods.

ihcsim

So without the --advanced flag, all the proxy flags (--admin-port, --control-port) will have no effects, right? I wonder if we should just add a comment in those flags description like (advanced mode only)?

ihcsim · 2019-04-23T22:15:16Z

cli/cmd/inject.go

@@ -93,6 +95,10 @@ sub-folders, or coming from stdin.`,
 	}

 	flags := options.flagSet(pflag.ExitOnError)
+	flags.BoolVar(
+		&advancedOption, "advanced", advancedOption,
+		"Add the proxy container in the ouput (as opposed to just adding the annotations for the auto-injector to do it) (default false)",


typo: ouput

Can we include the implication of using this flag (i.e. the proxy injector will skip the YAML output, hence, auto-injection and config override won't work)? How about Include the proxy sidecar container spec in the YAML output. Note that proxy configuration annotations aren't supported.?

I think it's important to mention the auto-injector, so we could put something like

Include the proxy sidecar container spec in the YAML output (the auto-injector won't pick it up, so config annotations aren't supported) (default false)

A bit long still...

Or we can just create a ticket in website to document it.

alpeb · 2019-04-23T22:52:05Z

I do like the --manual name better.

Is the plan to eventually phase this flag out? That would yield a smaller surface area to support and let us make more assumptions about meshed pods.

I believe not. The idea with --manual is to support advanced cases where we need the injected yaml before it gets sent to the cluster, like in CD pipelines where something like kustomize would modify some proxy parameter.

So without the --advanced flag, all the proxy flags (--admin-port, --control-port) will have no effects, right? I wonder if we should just add a comment in those flags description like (advanced mode only)?

They do have an effect, being transformed into override annotations.

adleong · 2019-04-23T23:01:37Z

I may be missing some context here but by my reading of #2576 the original problem that this is trying to solve is that there's a discrepancy between the specs of manually vs automatically injected deployments. I'm unclear on how changing the default behavior to automatic fixes this.

ihcsim · 2019-04-23T23:27:39Z

Essentially, we are forcing all workloads to be auto-injected by the proxy injector. This will make all CLI-injected workload consistent with the 2.3 auto-injected workload, where the source of truth lies in the pod YAML, not the parent's YAML.

The CLI manual mode exists (as @grampelberg puts in) a break-the-glass measure. The implication there is that workload will bypass the mutating webhook, hence, pod mutation and config annotations aren't supported.

That would yield a smaller surface area to support

Both the CLI manual mode and proxy injector use the same code path to inject the proxy. The difference lies in the "API boundary" (string vs. k8s admission types). So in a way, the surface area is small.

and let us make more assumptions about meshed pods.

Let us know if you think we have overlooked anything.

Signed-off-by: Alejandro Pedraza <[email protected]>

adleong · 2019-04-23T23:45:53Z

Thanks for the additional context, @ihcsim! This is very helpful.

My concern about surface area is that when debugging (or even when developing features) there isn't a consistent source of truth for the injected YAML because it may be in the parent's YAML if manual injection was used or in the pod YAML otherwise. This is essentially the problem statement from #2576.

But this change does make it so that the vast majority of users will use automatic injection which may make support easier in most cases. We should just keep in mind that keeping manual injection around means that we always have to be aware that there are two potential sources of truth.

Signed-off-by: Alejandro Pedraza <[email protected]>

l5d-bot · 2019-04-24T13:58:58Z

Integration test results for 944812d: success 🎉
Log output: https://gist.github.com/9ca0f2266a451554e6ed0333cfbc7b0d

alpeb force-pushed the alpeb/inject-advanced-flag branch from e09616d to 49e6ead Compare April 19, 2019 18:45

ihcsim assigned alpeb Apr 19, 2019

ihcsim added the area/inject label Apr 19, 2019

ihcsim reviewed Apr 19, 2019

View reviewed changes

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

siggy reviewed Apr 19, 2019

View reviewed changes

dadjeibaah reviewed Apr 19, 2019

View reviewed changes

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

alpeb force-pushed the alpeb/inject-advanced-flag branch from 49e6ead to 7cff6f6 Compare April 22, 2019 17:49

siggy mentioned this pull request Apr 22, 2019

auto-injector setting inconsistent parent label #2731

Closed

siggy approved these changes Apr 22, 2019

View reviewed changes

siggy mentioned this pull request Apr 22, 2019

Consolidate Differences In Runtime YAML of Auto-Inject And Manual-Injected Workloads #2576

Closed

alpeb added 5 commits April 23, 2019 09:48

Move injectPodAnnotations() before injectPodSpec() and enhanced injec…

6c18ea5

…tion tests to account for CLI options overridding Signed-off-by: Alejandro Pedraza <[email protected]>

Add test for non-advanced mode

845cacb

Signed-off-by: Alejandro Pedraza <[email protected]>

Rebase-related changes

3626214

Signed-off-by: Alejandro Pedraza <[email protected]>

alpeb force-pushed the alpeb/inject-advanced-flag branch from 7cff6f6 to 3626214 Compare April 23, 2019 14:55

ihcsim reviewed Apr 23, 2019

View reviewed changes

ihcsim approved these changes Apr 23, 2019

View reviewed changes

Change --advanced help text

5ec0891

Signed-off-by: Alejandro Pedraza <[email protected]>

Rename --advanced flag to --manual

944812d

Signed-off-by: Alejandro Pedraza <[email protected]>

alpeb merged commit 62d9a80 into master Apr 24, 2019

alpeb deleted the alpeb/inject-advanced-flag branch April 24, 2019 14:05

alpeb mentioned this pull request Apr 24, 2019

Introduce a new CLI inject --advanced flag #2711

Closed

ihcsim mentioned this pull request Jun 16, 2019

Remove Auto-Inject resource config's origin field (2.5) #2948

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New `linkerd inject` default and advanced modes #2721

New `linkerd inject` default and advanced modes #2721

alpeb commented Apr 19, 2019 •

edited

Loading

l5d-bot commented Apr 19, 2019

siggy left a comment •

edited

Loading

alpeb commented Apr 19, 2019

siggy commented Apr 19, 2019 •

edited

Loading

dadjeibaah left a comment

alpeb commented Apr 22, 2019

l5d-bot commented Apr 22, 2019

siggy left a comment

l5d-bot commented Apr 23, 2019

adleong commented Apr 23, 2019

adleong commented Apr 23, 2019

ihcsim left a comment

ihcsim Apr 23, 2019

alpeb Apr 23, 2019

ihcsim Apr 23, 2019

alpeb commented Apr 23, 2019

adleong commented Apr 23, 2019

ihcsim commented Apr 23, 2019 •

edited

Loading

adleong commented Apr 23, 2019

l5d-bot commented Apr 24, 2019

New linkerd inject default and advanced modes #2721

New linkerd inject default and advanced modes #2721

Conversation

alpeb commented Apr 19, 2019 • edited Loading

l5d-bot commented Apr 19, 2019

siggy left a comment • edited Loading

Choose a reason for hiding this comment

alpeb commented Apr 19, 2019

siggy commented Apr 19, 2019 • edited Loading

dadjeibaah left a comment

Choose a reason for hiding this comment

alpeb commented Apr 22, 2019

l5d-bot commented Apr 22, 2019

siggy left a comment

Choose a reason for hiding this comment

l5d-bot commented Apr 23, 2019

adleong commented Apr 23, 2019

adleong commented Apr 23, 2019

ihcsim left a comment

Choose a reason for hiding this comment

ihcsim Apr 23, 2019

Choose a reason for hiding this comment

alpeb Apr 23, 2019

Choose a reason for hiding this comment

ihcsim Apr 23, 2019

Choose a reason for hiding this comment

alpeb commented Apr 23, 2019

adleong commented Apr 23, 2019

ihcsim commented Apr 23, 2019 • edited Loading

adleong commented Apr 23, 2019

l5d-bot commented Apr 24, 2019

New `linkerd inject` default and advanced modes #2721

New `linkerd inject` default and advanced modes #2721

alpeb commented Apr 19, 2019 •

edited

Loading

siggy left a comment •

edited

Loading

siggy commented Apr 19, 2019 •

edited

Loading

ihcsim commented Apr 23, 2019 •

edited

Loading