Just grouping all the exported methods together.

There are calls here that were already made in newProxyContainer(); not sure if there's a way to avoid that without making this snipped unnecessarily more complex.

Say shouldInject() returned false because HostNetwork is true, yet the user added config annotations unknowingly. This will inject the proxy when it shouldn't. I think shouldInject() needs to be split into different methods to be able to distinguish when pods should not be injected at all, and when they are already injected.

This might also help clarifying a bit the logic to decide when to inject/override inside GetPatch(), which is currently not obvious IMO.

Yes, definitely a bug in the if statement. As a matter of fact, it breaks linkerd inject, because the received YAML will already have a proxy spec, and if config annotations are present, the bug you described will occur, leading to some downstream complication in the YAML (i.e. the CLI calls GetPatch() once, and then the webhook calls it again when the deployment is created.) 💥

I'm not sure how splitting up the shouldInject() will help, as I imagine it will require some clever ways to chain these different methods together, to satisfy both the webhook and CLI code path.

What I ended up doing was to perform the shouldInject() checks earlier in both the CLI and webhhook. So if shouldInject() failed, we won't even call the GetPatch() function. This also helps to keep GetPatch() to focus only on generating JSON patches, and the decision on whether the configs need to be overridden.

The commit is at dde345f the HEAD of this branch. This seems to work for the webhook, but I'm now battling with how the CLI reports unit tests. Maybe you can take a look at that commit to see if it makes sense? Thanks!

What I ended up doing was to perform the shouldInject() checks earlier in both the CLI and webhhook. So if shouldInject() failed, we won't even call the GetPatch() function. This also helps to keep GetPatch() to focus only on generating JSON patches, and the decision on whether the configs need to be overridden.

Yes, this looks much better to me 👍
Note though that your shouldInject() functions will return false whenever there's already a proxy attached because healthcheck.HasExistingSidecars() doesn't distinguish between linkerd proxies and 3rd party proxies, so this will prevent config overriding. That should be an easy fix 😉

I think this code is unreachable as written. The shouldInject call returns false if conf.pod.spec is nil, and that causes this function to return immediately. GetPatch only returns ErrUnsupportedResourceKind if conf.pod.spec is nil, but it can't be nil by the time we call it.

If we've encountered an error, shouldn't we return nil, nil, err here instead? That's how we're handling errors further down in this function.

Rather than build the report slice multiple times inside this function, my preference to build it once, immediately after the report is returned. Something like:

diff --git a/cli/cmd/inject.go b/cli/cmd/inject.go
index 275ce53d..9822b46a 100644
--- a/cli/cmd/inject.go
+++ b/cli/cmd/inject.go
@@ -110,9 +110,10 @@ func (rt resourceTransformerInject) transform(bytes []byte) ([]byte, []inject.Re
 	if err != nil {
 		return nil, nil, err
 	}
+	reports := []inject.Report{*report}
 
 	if !shouldInject(conf) {
-		return bytes, []inject.Report{*report}, nil
+		return bytes, reports, nil
 	}
 
 	p, err := conf.GetPatch()
@@ -121,7 +122,7 @@ func (rt resourceTransformerInject) transform(bytes []byte) ([]byte, []inject.Re
 	}
 
 	if p.IsEmpty() {
-		return bytes, []inject.Report{*report}, nil
+		return bytes, reports, nil
 	}
 
 	p.AddCreatedByPodAnnotation(k8s.CreatedByAnnotationValue())
@@ -130,7 +131,7 @@ func (rt resourceTransformerInject) transform(bytes []byte) ([]byte, []inject.Re
 		return nil, nil, err
 	}
 	if patchJSON == nil {
-		return bytes, []inject.Report{*report}, nil
+		return bytes, reports, nil
 	}
 	log.Infof("patch generated for: %s", conf)
 	log.Debugf("patch: %s", patchJSON)
@@ -150,7 +151,7 @@ func (rt resourceTransformerInject) transform(bytes []byte) ([]byte, []inject.Re
 	if err != nil {
 		return nil, nil, err
 	}
-	return injectedYAML, []inject.Report{*report}, nil
+	return injectedYAML, reports, nil
 }
 
 func (resourceTransformerInject) generateReport(reports []inject.Report, output io.Writer) {

Mind changing this to log.Debugf? It's a bit too verbose to be at the info level.

You can set these two fields when you initialize the AdmissionResponse struct. In fact, I'd just return that struct directly:

diff --git a/controller/proxy-injector/webhook.go b/controller/proxy-injector/webhook.go
index 044d059e..7d167bb5 100644
--- a/controller/proxy-injector/webhook.go
+++ b/controller/proxy-injector/webhook.go
@@ -148,14 +148,12 @@ func (w *Webhook) inject(request *admissionv1beta1.AdmissionRequest) (*admission
 	log.Debugf("patch: %s", patchJSON)
 
 	patchType := admissionv1beta1.PatchTypeJSONPatch
-	admissionResponse := &admissionv1beta1.AdmissionResponse{
-		UID:     request.UID,
-		Allowed: true,
-	}
-	admissionResponse.Patch = patchJSON
-	admissionResponse.PatchType = &patchType
-
-	return admissionResponse, nil
+	return &admissionv1beta1.AdmissionResponse{
+		UID:       request.UID,
+		Allowed:   true,
+		Patch:     patchJSON,
+		PatchType: &patchType,
+	}, nil
 }
 
 func shouldInject(conf *inject.ResourceConfig) bool {

Related to my comment in the CLI code, it looks like this func is only ever called after a call to HasPayload, so it should never be the case the conf.pod.spec is nil. What if we make HasPayload package private (e.g. hasPayload), and just build it into these functions directly so that external packages don't have to do their own validation that the payload is present?

This func now duplicates a bunch of code that exists in pkg/inject/inject.go. Can you instead write it like:

func (r *Report) update(conf *ResourceConfig) {
	r.InjectDisabled = conf.InjectDisabled()
	r.HostNetwork = conf.PodUsingHostNetwork()
	r.Sidecar = conf.HasExistingProxy()
	r.UDP = checkUDPPorts(conf.pod.spec)
}

This is unused?

Rather than only doing the override if at least config.linkerd.io annotation is present, I think it would better to use something like k8s.IsMeshed to determine if the pod is inject, and then always override if inject, even if it doesn't contain override annotations. My thinking is that eventually we might allow users to change the contents of the proxy configmap after install, and we won't pick up the change if we're only looking for override annotations.