Proxy init and sidecar containers auto-injection

[ihcsim]

Per discussion with @grampelberg, here's a PR that contains a proposed implementation for the proxy init and side containers auto-injection feature.

Example usage can be found in this gist.

The following is a summary of the changes in this PR:

Control Plane

1. Add the proxy-injector pod to the control plane.
2. The proxy-injector creates the webhook's mutating webhook configuration by using the CA controller's trust anchor as its CA bundle.
3. The mutating webhook configuration ignores namespaces that are labelled with linkerd.io/auto-inject: disabled or linkerd.io/auto-inject: completed.
4. The webhook is triggered by CREATE events of Deployments.
5. The webhook ignores Deployment with pods that are labelled with linkerd.io/auto-inject: disabled or linkerd.io/auto-inject: completed.
6. A TLS secret named proxy-injector-service-tls-linkerd-io is created by the CA controller to enable TLS communication between the API server and the webhook.
7. Declare a new config map to store the proxy init and sidecar container specs used for the auto-injection.

CLI

1. Update cli/install/template.go with new Deployment, Service, RBAC etc. specs.
2. Make some of the inject proxy options available to the install command because cli/install/template.go now contains proxy container specs.
3. New install option to enable or disable proxy auto-injection.
   • Auto-injection only works when the --tls=optional and proxy-auto-inject options are set.

CA Controller

1. Add a new handler to create a new TLS secret when a new mutating webhook configuration is created. The Service name defined in the webhook configuration will be used as the TLS identity.

RBAC

1. CA cluster role is updated with permissions to get, list and watch mutating webhook configuration.
2. Add a new cluster role with permission for the controller to create, get, list and watch mutating webhook configuration. This cluster role also has permission to get, list and watch config maps. (I tried to limit the config map permissions to namespace-scoped, but I think the usage of informers.Lister().Get() requires global scope?)

(I can submit a PR for usage documentation once this PR is reviewed.)

[grampelberg]

Holy cow, look at those tests!

@ihcsim why did you put the inject label on the deployment instead of the pod itself?

[ihcsim]

why did you put the inject label on the deployment instead of the pod itself?

I don't remember why I did it that way 😆 I don't see why it needs to be at the deployment level. I had to change the mutating webhook configuration CREATE rule from pod to deployment so that the webhook can inject the other needed labels into /spec/template/metadata/labels. Maybe it's a leftover from the code change effort.

Holy cow, look at those tests!

Not perfect though because the webhook gets only as far as generating the JSON patch. Whether the JSON patch can be applied or not is beyond the webhook's scope.

[grampelberg]

I'd love to see if it works at the pod level, that way we get all the other resource types for free.

[klingerf]

@ihcsim Great work! I was able to run this locally using the gist you provided, and it's awesome.

I didn't make it all the way through on this pass, but I left a bunch of miscellaneous comments below. I admittedly don't know a lot about admission controllers, so some of my feedback may not make sense. And I'll spend some more time looking at this over the next day or two.

From an architectural perspective, how would you feel about running the proxy-injector service in a separate proxy-injector pod? I think my preference would be to not modify the containers running in the existing controller pod, since this feature is an add on. Similarly, I couldn't quite follow the changes to the ca controller. Is is the case that we'll receive a new MutatingWebhookConfiguration event for each new deployment that's created, and we need to watch those events in order to propagate certs to those deployments? And that replaces the Pod watch events, since the pods no longer have injected YAML that tells us they're part of the data plane? If so, it seems like we should be running either the Pod watcher or the MutatingWebhookConfiguration watcher, but not both -- see my comment in that file about gating that behavior behind a flag to the ca server.

[klingerf]

Maybe this is leftover debug logging? If you want to keep in it, I'd change to log.Debugf, and fix the issue with string substitution (%s, not s%).

[klingerf]

This method seems like an alias for k8s.NewClientSet -- why not just call that method directly from proxy-injector/main.go and remove this file?

[ihcsim]

Yes, this is no longer needed. It was introduced in some of my earlier tests to polymorphically return a fake k8s client. I will remove it.

[klingerf]

I don't think you're ever hitting this code path, since GetObjects is only called by the tap server and the public-api server, and neither of those should be requesting MWCs. I recommend removing these two lines and the getMutatingWebhookConfigurations method below entirely.

[ihcsim]

👍 It's one of those I will add them for completeness sake but don't know what it does changes. I will remove them.

[klingerf]

You could use a duration flag here instead of a string flag, and that would remove the need to call ParseDuration later in this file. Duration flags will also automatically reject invalid durations.

volumeMountsWaitTime := flag.Duration("volume-mounts-wait", 3*time.Minute, "maximum wait time for the secret volumes to mount before the timeout expires")

[klingerf]

This is super minor, but the convention in the rest of our controller mains is to define flags as local variables within the main func itself, rather than as global variables that are configured via an init func. So for consistency with those, I'd do something like:

func main() {
	port := flag.String("port", "443", "port that this webhook admission server listens on")
	kubeconfig := flag.String("kubeconfig", "", "path to kubeconfig")
	controllerNamespace := flag.String("controller-namespace", "linkerd", "namespace in which Linkerd is installed")
	certFile := flag.String("tls-cert-file", "/var/linkerd-io/identity/certificate.crt", "location of the webhook server's TLS cert file")
	keyFile := flag.String("tls-key-file", "/var/linkerd-io/identity/private-key.p8", "location of the webhook server's TLS private key file")
	trustAnchorsPath := flag.String("trust-anchors-path", "/var/linkerd-io/trust-anchors/trust-anchors.pem", "path to the CA trust anchors PEM file used to create the mutating webhook configuration")
	volumeMountsWaitTime := flag.Duration("volume-mounts-wait", 3*time.Minute, "maximum wait time for the secret volumes to mount before the timeout expires")
	webhookServiceName := flag.String("webhook-service", "proxy-injector.linkerd.io", "name of the admission webhook")
	flags.ConfigureAndParse()

[klingerf]

Related to my comments in controller/k8s/api.go -- I'm pretty sure you can revert all of the changes to this file. Since we're not exposing resources of type MWC through linkerd stat and linkerd tap, they don't need to be added here.

[klingerf]

The logrus log level is global, so there's no need to re-set the level each time you import it. And at this point in your main func, the log level was already set by calling flags.ConfigureAndParse(). So I recommend removing this call and all of the SetLogLevel funcs and tests that you've added in the injector package.

[klingerf]

We should pass in a ready channel to the sync call here so that we know when the caches are synced. That way we can also run an admin server and configure a readiness check that won't succeed until caches are synced.

If you add the following flag above:

	metricsAddr := flag.String("metrics-addr", ":9993", "address to serve scrapable metrics on")

Then here you can do:

	ready := make(chan struct{})
	s.SyncAPI(ready)

	go admin.StartServer(*metricsAddr, ready)

[klingerf]

Ahhhh, related to my other comment about logging -- I now see that these structs embed the logger, which is why you're explicitly setting the level. My preference would be to remove the embedding of Logger and rely on global log instead.

[klingerf]

I think we need to gate this behavior behind a flag to the ca service, contingent on whether or not linkerd was installed with the --proxy-auto-inject set. Otherwise, all TLS deployments will fire up a watch on MutatingWebhookConfigurations, regardless of whether or not the admission controller is running.

[ihcsim]

Absolutely. I forgot about the scenario where --tls=optional is enabled but not --proxy-auto-inject.

[ihcsim]

@klingerf Thanks for the feedback.

From an architectural perspective, how would you feel about running the proxy-injector service in a separate proxy-injector pod?

No objections. That was my initial plan. I actually misread a comment in the other older PR, thinking that the intent was to run the injector as a container in the controller pod.

Is is the case that we'll receive a new MutatingWebhookConfiguration event for each new deployment that's created..

The MutatingWebhookConfiguration is created just once when the injector starts up. It contains information used by the k8s API server to communicate with the admission webhook (which conceptually, is made up of a TLS server and the actual webhook that does the spec mutation). The configuration has nothing to do with any other new "application" deployments.

(Tangent: The only other times the MutatingWebhookConfiguration needs to be re-created is when the CA controller's trust-anchors.pem changed because it uses the trust-anchors.pem as its CA bundle. This is how the k8s API server knows it can trust the cert presented by the webhook TLS server. Currently, I am doing re-creating the MutatingWebhookConfiguration manually.)

and we need to watch those events in order to propagate certs to those deployments? And that replaces the Pod watch events, since the pods no longer have injected YAML that tells us they're part of the data plane?

Nothing about how LD2 is currently doing automatic certs propagation has changed. If it does, it's unintentional and I screwed up!

My changes to the CA controller is simply my way of telling the CA controller, "hey, I need a CA cert for my webhook's TLS server. btw, the API server will be talking to my TLS server as https://proxy-injector-service-name.controller-namespace:443. You can find the values of proxy-injector-service-name and controller-namespace in the MutatingWebhookConfiguration".

Open to more feedback and suggestions...

[klingerf]

@ihcsim Ok great, thanks for the additional feedback! I definitely wasn't understanding the intent of the ca change, but now I get it. I think the watch approach that you've taken works, but see my comment below.

Also, I haven't fully thought this through, but if we wanted to avoid setting up a watch on all MWCs and requiring the additional RBAC privileges, it seems possible that the ca service could provide an endpoint that the proxy-injector service could call on startup, prompting the ca service to create the secret on demand. And maybe that could be generalized for other use cases in the future... although I can't think of one off the top of my head.

[ihcsim]

From an architectural perspective, how would you feel about running the proxy-injector service in a separate proxy-injector pod?

@klingerf To clarify, the intent is to move the injector completely out of the control plane, right? So I'll have to roll a new Docker image for the injector, update the build scripts and move the code to the project root level?

[klingerf]

@ihcsim Thanks for checking -- I just meant a separate pod definition in the install YAML. It's no problem for the pod to use the same controller image and rely on these existing build system. But we should add another service account for the new pod with the proper RBAC permissions. I think something like this in template.go would work:

diff --git a/cli/install/template.go b/cli/install/template.go
index e4956792..4bcefe55 100644
--- a/cli/install/template.go
+++ b/cli/install/template.go
@@ -229,7 +229,40 @@ spec:
             path: /ready
             port: 9998
           failureThreshold: 7
-      {{if and .EnableTLS .ProxyAutoInjectEnabled }}
+
+{{- if and .EnableTLS .ProxyAutoInjectEnabled }}
+### Service Account Proxy Injector ###
+---
+kind: ServiceAccount
+apiVersion: v1
+metadata:
+  name: linkerd-proxy-injector
+  namespace: {{.Namespace}}
+
+### Controller RBAC ###
+... RBAC for injector here
+
+---
+kind: Deployment
+apiVersion: extensions/v1beta1
+metadata:
+  name: proxy-injector
+  namespace: {{.Namespace}}
+  labels:
+    {{.ControllerComponentLabel}}: proxy-injector
+  annotations:
+    {{.CreatedByAnnotation}}: {{.CliVersion}}
+spec:
+  replicas: {{.ControllerReplicas}}
+  template:
+    metadata:
+      labels:
+        {{.ControllerComponentLabel}}: proxy-injector
+      annotations:
+        {{.CreatedByAnnotation}}: {{.CliVersion}}
+    spec:
+      serviceAccount: linkerd-proxy-injector
+      containers:
       - name: proxy-injector
         image: {{.ControllerImage}}
         imagePullPolicy: {{.ImagePullPolicy}}
@@ -255,7 +288,6 @@ spec:
         secret:
           secretName: {{.ProxyInjectorTLSSecret}}
           optional: true
-      {{ end }}
 
 ### Web ###
 ---

[klingerf]

And thanks for addressing my other feedback! I'm going to be offline for a few days, but will re-review as soon as I get back.

[ihcsim]

@klingerf I have moved the proxy-injector into a separate deployment in the control plane. The interesting thing now is that I see two TLS secrets being generated:

$ kubectl -n linkerd get secret
...
proxy-injector-deployment-tls-linkerd-io              Opaque                                2         45m
proxy-injector-service-tls-linkerd-io                 Opaque                                2         45m

As previously mentioned, the proxy-injector-service-tls-linkerd-io secret is generated by the MWC event handler defined in the CertificateController. The TLS cert in this secret uses the webhook's Service DNS name as its subject name.

Certificate:
    Data:
        Version: 3 (0x2)
        Serial Number: 84 (0x54)
    Signature Algorithm: ecdsa-with-SHA256
        Issuer: CN=Cluster-local Managed Pod CA
        ....
        X509v3 extensions:
            X509v3 Subject Alternative Name: critical
                DNS:proxy-injector.linkerd.svc

This is how the k8s API server communicates with the webhook.

My preference is that ca supports SAN. Based on what I see, I don't think this is possible, right?

Will chat more when you're back.

[klingerf]

@ihcsim Thanks for making those updates! This is looking really good. I unfortunately didn't get all the way through reviewing again today, but I wanted to send a couple of comments before I come back to it tomorrow.

[klingerf]

This is super minor, but the convention for the rest of our cluster roles in this file is to use the name linkerd-{{.Namespace}}- + deployment name. And since the deployment name is "proxy-injector", I'd prefer for this role name to be:

  name: linkerd-{{.Namespace}}-proxy-injector

[ihcsim]

Got it. Yeah, I had to make some assumption about the naming convention.

[klingerf]

Same comment here as above:

  name: linkerd-{{.Namespace}}-proxy-injector

[klingerf]

And here:

  name: linkerd-{{.Namespace}}-proxy-injector

[klingerf]

In the event that the webhook config exists, how would you feel about updating it every time the proxy-injector process starts? I followed your gist and ran into this issue:

2018/10/01 04:09:54 http: TLS handshake error from 172.17.0.4:55728: remote error: tls: bad certificate

I think a possible workaround for that would be to force update the mwc each time this process starts. Something along the lines of:

diff --git a/controller/cmd/proxy-injector/main.go b/controller/cmd/proxy-injector/main.go
index a29310e6..342f3f75 100644
--- a/controller/cmd/proxy-injector/main.go
+++ b/controller/cmd/proxy-injector/main.go
@@ -44,19 +44,11 @@ func main() {
 
 	webhookConfig := injector.NewWebhookConfig(k8sClient, *controllerNamespace, *webhookServiceName, *trustAnchorsPath)
 
-	exist, err := webhookConfig.Exist()
+	obj, err := webhookConfig.CreateOrUpdate()
 	if err != nil {
-		log.Fatalf("failed to access the mutating webhook configurations resource: ", err)
-	}
-
-	if !exist {
-		log.Info("creating mutating webhook configuration")
-		webhookConfig, err := webhookConfig.Create()
-		if err != nil {
-			log.Fatal("faild to create the mutating webhook configuration: ", err)
-		}
-		log.Info("created mutating webhook configuration: ", webhookConfig.ObjectMeta.SelfLink)
+		log.Fatalf("failed to create or update mutating webhook configuration: %s", err)
 	}
+	log.Infof("created or updated mutating webhook configuration: ", obj.ObjectMeta.SelfLink)
 
 	log.Infof("waiting for the tls secrets to mount (wait at most %s)", volumeMountsWaitTime)
 	if err := waitForMounts(*volumeMountsWaitTime, *certFile, *keyFile); err != context.Canceled {

[ihcsim]

Yup, that will work. There is still a chance that the ca trust-anchors.pem MWC's CA bundle will go out-of-sync with the proxy-injector's TLS secret, triggering the TLS handshake error. At least, with your suggested approach, we know the workaround to the TLS error is to delete the proxy-injector pod, and let it update the MWC's CA bundle 👍

[klingerf]

Typo here: faild => failed

[klingerf]

Related to my previous comment, I think we still need to check the name of the MWC here to make sure that the update pertains to the MWC that is being created by the proxy-injector process:

diff --git a/controller/ca/controller.go b/controller/ca/controller.go
index 34514f95..223c5ff9 100644
--- a/controller/ca/controller.go
+++ b/controller/ca/controller.go
@@ -186,9 +186,11 @@ func (c *CertificateController) handlePodUpdate(oldObj, newObj interface{}) {
 
 func (c *CertificateController) handleMWCAdd(obj interface{}) {
 	mwc := obj.(*v1beta1.MutatingWebhookConfiguration)
-	log.Debugf("enqueuing secret write for mutating webhook configuration %q", mwc.ObjectMeta.Name)
-	for _, webhook := range mwc.Webhooks {
-		c.queue.Add(fmt.Sprintf("%s.%s.%s", webhook.ClientConfig.Service.Name, pkgK8s.Service, webhook.ClientConfig.Service.Namespace))
+	if mwc.Name == pkgK8s.ProxyInjectorWebhookConfig {
+		log.Debugf("enqueuing secret write for mutating webhook configuration %q", mwc.ObjectMeta.Name)
+		for _, webhook := range mwc.Webhooks {
+			c.queue.Add(fmt.Sprintf("%s.%s.%s", webhook.ClientConfig.Service.Name, pkgK8s.Service, webhook.ClientConfig.Service.Namespace))
+		}
 	}
 }

[klingerf]

@ihcsim The latest set of updates looks great. And I had a chance to review the rest of the code in the injector package, and left some comments. I'm pretty sure we can run the proxy-injector process with fewer RBAC permissions, but would be interested to hear your feedback on it.

[klingerf]

Passing this as an argument to NewAPI will cause the watch on MWCs to be established automatically, so you only want to provide the MWC argument when proxyAutoInject is true. Something like:

	var k8sAPI *k8s.API
	if *proxyAutoInject {
		k8sAPI = k8s.NewAPI(k8sClient, k8s.Pod, k8s.RS, k8s.MWC)
	} else {
		k8sAPI = k8s.NewAPI(k8sClient, k8s.Pod, k8s.RS)
	}

[klingerf]

This is minor, but can I bug you to wrap all of your comments at 80 characters?

[klingerf]

I think this would be clearer as:

		switch status {
		case k8sPkg.ProxyAutoInjectDisabled, k8sPkg.ProxyAutoInjectCompleted:
			return true
		}

[klingerf]

We should move checkSidecars to a public HasExistingSidecars function in a separate package that can be shared between the proxy-injector and the cli. How about creating a new file at pkg/healthcheck/sidecar.go? It's at least loosely related to health checking. And then here:

	return healthcheck.HasExistingSidecars(&deployment.Spec.Template.Spec)

[ihcsim]

Sounds good.

Yeah, I was hesitant to change the CLI code too much as I wasn't sure if those changes will be necessary/accepted etc. So I decided to let you bring them up :-)

[klingerf]

Rather than reading this configmap from the API each time we need to inject, couldn't we volume mount it into the proxy-injector container and read it from disk when the process starts up? That would remove the need to watch all configmaps in all namespaces, and it would allow us to scope the RBAC permissions for this process more tightly.

[ihcsim]

Love it! Must admit that I got a bit API happy there.

[klingerf]

You're using the Kubernetes API here to retrieve the full namespace object given the namespace name, but the only field you're ever accessing on the object is the name, on line 125. So it seems like you don't need to use the Kubernetes API at all for this. I'd change as follows:

@@ -106,11 +107,6 @@ func (w *Webhook) inject(request *admissionv1beta1.AdmissionRequest) (*admission
        }
        log.Infof("resource namespace: %s", ns)
 
-       namespace, err := w.k8sAPI.NS().Lister().Get(ns)
-       if err != nil {
-               return nil, err
-       }
-
        if w.ignore(&deployment) {
                log.Infof("ignoring deployment %s", deployment.ObjectMeta.Name)
                return &admissionv1beta1.AdmissionResponse{
@@ -122,7 +118,7 @@ func (w *Webhook) inject(request *admissionv1beta1.AdmissionRequest) (*admission
        identity := &k8sPkg.TLSIdentity{
                Name:                deployment.ObjectMeta.Name,
                Kind:                strings.ToLower(request.Kind.Kind),
-               Namespace:           namespace.ObjectMeta.GetName(),
+               Namespace:           ns,
                ControllerNamespace: w.controllerNamespace,
        }
        proxy, proxyInit, err := w.containersSpec(identity)

And that would allow you to remove the "namespaces" RBAC privileges for this process.

[klingerf]

Same comment here as above -- since this process is already running in the controller namespace, it should be able to volume mount the ProxyInjectorSidecarConfig configmap and read it on startup, rather than needing to retrieve it from the API.

[klingerf]

I think if you make the configmap and namespace changes that I suggest in this file, then this process should no longer need access to the Kubernetes API, and you can remove this field.

[klingerf]

If you make the changes I suggest in webhook.go, I'm pretty sure you can remove the "configmaps" and "namespaces" RBAC rules here.

[klingerf]

Based on the create or update changes that you made in your last commit, I think the new set of MWC verbs you're using is:

  verbs: ["get", "create", "update"]

But I haven't tested those yet.

[ihcsim]

Ooopss... rebase fail.... works on my machine ™️

[ihcsim]

@klingerf I have pushed all the changes. I also rebased my branch with master to fix the CI failures. If the code looks good, I can help put together some documentation for the website; probably gonna re-use the examples in my gist with less text.

[klingerf]

@ihcsim Thanks for making those updates! This branch is looking really really good. I left just a few additional small comments, mostly around the tests (yay tests btw!).

Once the comments are addressed I think it needs one more master rebase to resolve conflicts from #1731, but after that it should hopefully be good to go.

If the code looks good, I can help put together some documentation for the website; probably gonna re-use the examples in my gist with less text.

Updating the docs would be totally awesome. I've been using the gist you provided, and it's super helpful.

Thanks again for bearing with me and addressing all of my feedback on this branch. I'm excited to see this feature added!

[klingerf]

The function name and the comment here don't match -- the comment should be renamed from NewFakeClient to NewClient.

[klingerf]

I don't think we should do this as part of this pull request, but I wanted to note it while I'm thinking about it.

I'm worried that over time the inject YAML in this configmap will start to drift from the YAML that's produced by inject.go. I think a more future proof solution would be to somehow use the inject.go codepath to produce the YAML that goes into this configmap, rather than duplicating the logic from inject.go inside this template.

But like I said, that feels like something that can be done in a follow-up pull request.

[ihcsim]

Agreed. As a matter of fact, I took that approach when I first started this POC, where I tried to extract out the code in injectPodSpec(), to be shared between install and inject. But the scope of the changes to the code and test went beyond my comfort level. We can open a task for the follow-up PR.

[klingerf]

I think it should say 'fake/data' instead of a 'test/data' for the folder path in this comment. Same goes for the rest of the comments in this file.

[klingerf]

This factory func uses yaml.Unmarshal, but the comment says that it requires valid JSON. I'd just change the comment to s/JSON/YAML/. Same goes for the rest of the factory funcs in this file.

[klingerf]

It feels a bit brittle to compare JSON byte slices for an exact match, since two JSON objects could be effectively equal but with different key ordering. I'd be inclined to just remove the JSON marshaling from this test, and compare the object slices directly. Something like:

diff --git a/controller/proxy-injector/patch_test.go b/controller/proxy-injector/patch_test.go
index 5a47c22a..40026bd5 100644
--- a/controller/proxy-injector/patch_test.go
+++ b/controller/proxy-injector/patch_test.go
@@ -1,7 +1,6 @@
 package injector
 
 import (
-	"encoding/json"
 	"reflect"
 	"testing"
 
@@ -53,11 +52,6 @@ func TestPatch(t *testing.T) {
 		k8sPkg.CreatedByAnnotation: createdBy,
 	})
 
-	actual, err := json.Marshal(patch.patchOps)
-	if err != nil {
-		t.Fatal("Unexpected error: ", err)
-	}
-
 	expectedOps := []*patchOp{
 		&patchOp{Op: "add", Path: patchPathContainer, Value: sidecar},
 		&patchOp{Op: "add", Path: patchPathInitContainerRoot, Value: []*v1.Container{}},
@@ -71,12 +65,8 @@ func TestPatch(t *testing.T) {
 		}},
 		&patchOp{Op: "add", Path: patchPathPodAnnotation, Value: map[string]string{k8sPkg.CreatedByAnnotation: createdBy}},
 	}
-	expected, err := json.Marshal(expectedOps)
-	if err != nil {
-		t.Fatal("Unexpected error: ", err)
-	}
 
-	if !reflect.DeepEqual(actual, expected) {
-		t.Errorf("Content mismatch\nExpected: %s\nActual: %s", expected, actual)
+	if !reflect.DeepEqual(expectedOps, patch.patchOps) {
+		t.Errorf("Content mismatch\nExpected: %s\nActual: %s", expectedOps, patch.patchOps)
 	}
 }

[klingerf]

Super minor, but the rest of our controller mains accept an -addr flag instead of a -port flag. For consistency with those mains, I'd change this to:

	addr := flag.String("addr", ":443", "address to serve on")

And then update NewWebhookServer accordingly.

[klingerf]

Looks like this const is unused -- let's remove it.

[klingerf]

Totally optional, but you could condense this test a bit by using a table-driven test like you're doing in TestIgnore below.

[ihcsim]

@klingerf I have updated all the tests per your feedback and rebased this branch with master. The config map is also updated to include the recent proxy resource requests options.

Appreciate all your feedback!

[klingerf]

@ihcsim Ok, great, thanks for making those updates. I opened #1748 to track using one code path for CLI inject and admission controller inject. And to that end, I did a bit more rigorous testing of all of the CLI options for configuring admission controller inject, and found a few discrepancies. They're called out in my comments below. Once these are fixed this branch should be good to go! Thanks again for bearing with me through all of this feedback.

[klingerf]

IgnoreInboundPorts and IgnoreOutboundPorts needs to be formatted as a list of comma-separate ints, but using a slice means that we'll end up printing [ and ] characters around the ports in the YAML, which won't work.

Here's one way to go about fixing it, but would also be open to other implementations:

diff --git a/cli/cmd/install.go b/cli/cmd/install.go
index b344441f..0df9c4fd 100644
--- a/cli/cmd/install.go
+++ b/cli/cmd/install.go
@@ -6,6 +6,7 @@ import (
 	"io"
 	"io/ioutil"
 	"os"
+	"strings"
 	"text/template"
 
 	"github.com/linkerd/linkerd2/cli/install"
@@ -41,8 +42,8 @@ type installConfig struct {
 	TLSIdentityVolumeSpecFileName    string
 	InboundPort                      uint
 	OutboundPort                     uint
-	IgnoreInboundPorts               []uint
-	IgnoreOutboundPorts              []uint
+	IgnoreInboundPorts               string
+	IgnoreOutboundPorts              string
 	ProxyAutoInjectEnabled           bool
 	ProxyAutoInjectLabel             string
 	ProxyUID                         int64
@@ -111,6 +112,17 @@ func validateAndBuildConfig(options *installOptions) (*installConfig, error) {
 	if err := validate(options); err != nil {
 		return nil, err
 	}
+	ignoreInboundPorts := []string{
+		fmt.Sprintf("%d", options.proxyControlPort),
+		fmt.Sprintf("%d", options.proxyMetricsPort),
+	}
+	for _, p := range options.ignoreInboundPorts {
+		ignoreInboundPorts = append(ignoreInboundPorts, fmt.Sprintf("%d", p))
+	}
+	ignoreOutboundPorts := []string{}
+	for _, p := range options.ignoreOutboundPorts {
+		ignoreOutboundPorts = append(ignoreOutboundPorts, fmt.Sprintf("%d", p))
+	}
 	return &installConfig{
 		Namespace:                        controlPlaneNamespace,
 		ControllerImage:                  fmt.Sprintf("%s/controller:%s", options.dockerRegistry, options.linkerdVersion),
@@ -137,8 +149,8 @@ func validateAndBuildConfig(options *installOptions) (*installConfig, error) {
 		TLSIdentityVolumeSpecFileName:    k8s.TLSIdentityVolumeSpecFileName,
 		InboundPort:                      options.inboundPort,
 		OutboundPort:                     options.outboundPort,
-		IgnoreInboundPorts:               options.ignoreInboundPorts,
-		IgnoreOutboundPorts:              options.ignoreOutboundPorts,
+		IgnoreInboundPorts:               strings.Join(ignoreInboundPorts, ","),
+		IgnoreOutboundPorts:              strings.Join(ignoreOutboundPorts, ","),
 		ProxyAutoInjectEnabled:           options.proxyAutoInject,
 		ProxyAutoInjectLabel:             k8s.ProxyAutoInjectLabel,
 		ProxyUID:                         options.proxyUID,
diff --git a/cli/cmd/install_test.go b/cli/cmd/install_test.go
index 9823e27f..4e8ed7bf 100644
--- a/cli/cmd/install_test.go
+++ b/cli/cmd/install_test.go
@@ -57,8 +57,8 @@ func TestRender(t *testing.T) {
 		ProxyInjectorSidecarConfig:       "ProxyInjectorSidecarConfig",
 		ProxySpecFileName:                "ProxySpecFileName",
 		ProxyInitSpecFileName:            "ProxyInitSpecFileName",
-		IgnoreInboundPorts:               []uint{1, 2, 3},
-		IgnoreOutboundPorts:              []uint{2, 3, 4},
+		IgnoreInboundPorts:               "4190,4191,1,2,3",
+		IgnoreOutboundPorts:              "2,3,4",
 		ProxyResourceRequestCPU:          "RequestCPU",
 		ProxyResourceRequestMemory:       "RequestMemory",
 	}
diff --git a/cli/cmd/testdata/install_output.golden b/cli/cmd/testdata/install_output.golden
index 51e7b5ad..91c3fa5d 100644
--- a/cli/cmd/testdata/install_output.golden
+++ b/cli/cmd/testdata/install_output.golden
@@ -1226,9 +1226,9 @@ data:
     - --proxy-uid
     - 2102
     - --inbound-ports-to-ignore
-    - 4190,4191,[1 2 3]
+    - 4190,4191,1,2,3
     - --outbound-ports-to-ignore
-    - [2 3 4]
+    - 2,3,4
     image: ProxyInitImage
     imagePullPolicy: IfNotPresent
     name: linkerd-init
diff --git a/cli/install/template.go b/cli/install/template.go
index 9b25ecaa..99e6e3b1 100644
--- a/cli/install/template.go
+++ b/cli/install/template.go
@@ -822,9 +822,11 @@ data:
     - {{.OutboundPort}}
     - --proxy-uid
     - {{.ProxyUID}}
+    {{- if ne (len .IgnoreInboundPorts) 0}}
     - --inbound-ports-to-ignore
-    - {{.ProxyControlPort}},{{.ProxyMetricsPort}}{{ if .IgnoreInboundPorts }},{{.IgnoreInboundPorts}}{{end}}
-    {{- if .IgnoreOutboundPorts}}
+    - {{.IgnoreInboundPorts}}
+    {{- end}}
+    {{- if ne (len .IgnoreOutboundPorts) 0}}
     - --outbound-ports-to-ignore
     - {{.IgnoreOutboundPorts}}
     {{- end}}

[klingerf]

The brackets here for inbound and outbound ports to ignore aren't going to be parseable by the proxy-init go process, unfortunately. I think this needs to be:

    - --inbound-ports-to-ignore
    - 4190,4191,1,2,3
    - --outbound-ports-to-ignore
    - 2,3,4

[klingerf]

This timeout is configurable via the --proxy-bind-timeout flag, so we need to reflect the value of that flag when rendering this template. Something like:

diff --git a/cli/cmd/install.go b/cli/cmd/install.go
index 0df9c4fd..b77f2c77 100644
--- a/cli/cmd/install.go
+++ b/cli/cmd/install.go
@@ -57,6 +57,7 @@ type installConfig struct {
 	ProxyImage                       string
 	ProxyResourceRequestCPU          string
 	ProxyResourceRequestMemory       string
+	ProxyBindTimeout                 string
 }
 
 type installOptions struct {
@@ -164,6 +165,7 @@ func validateAndBuildConfig(options *installOptions) (*installConfig, error) {
 		ProxyImage:                       options.taggedProxyImage(),
 		ProxyResourceRequestCPU:          options.proxyCpuRequest,
 		ProxyResourceRequestMemory:       options.proxyMemoryRequest,
+		ProxyBindTimeout:                 options.proxyBindTimeout,
 	}, nil
 }
 
diff --git a/cli/cmd/install_test.go b/cli/cmd/install_test.go
index 4e8ed7bf..9c9ead1c 100644
--- a/cli/cmd/install_test.go
+++ b/cli/cmd/install_test.go
@@ -61,6 +61,7 @@ func TestRender(t *testing.T) {
 		IgnoreOutboundPorts:              "2,3,4",
 		ProxyResourceRequestCPU:          "RequestCPU",
 		ProxyResourceRequestMemory:       "RequestMemory",
+		ProxyBindTimeout:                 "1m",
 	}
 
 	testCases := []struct {
diff --git a/cli/cmd/testdata/install_output.golden b/cli/cmd/testdata/install_output.golden
index 91c3fa5d..537a6e23 100644
--- a/cli/cmd/testdata/install_output.golden
+++ b/cli/cmd/testdata/install_output.golden
@@ -1243,7 +1243,7 @@ data:
     - name: LINKERD2_PROXY_LOG
       value: warn,linkerd2_proxy=info
     - name: LINKERD2_PROXY_BIND_TIMEOUT
-      value: 10s
+      value: 1m
     - name: LINKERD2_PROXY_CONTROL_URL
       value: tcp://proxy-api.Namespace.svc.cluster.local:123
     - name: LINKERD2_PROXY_CONTROL_LISTENER
diff --git a/cli/install/template.go b/cli/install/template.go
index 99e6e3b1..93712f3d 100644
--- a/cli/install/template.go
+++ b/cli/install/template.go
@@ -844,7 +844,7 @@ data:
     - name: LINKERD2_PROXY_LOG
       value: warn,linkerd2_proxy=info
     - name: LINKERD2_PROXY_BIND_TIMEOUT
-      value: 10s
+      value: {{.ProxyBindTimeout}}
     - name: LINKERD2_PROXY_CONTROL_URL
       value: tcp://proxy-api.{{.Namespace}}.svc.cluster.local:{{.ProxyAPIPort}}
     - name: LINKERD2_PROXY_CONTROL_LISTENER

[klingerf]

I missed this earlier, but my preference is to call this "linkerd-proxy-injector-webhook-config", since MWCs are not namespaced. Prefixing "linkerd-" is the approach that we take for ClusterRoles and ClusterRoleBindings, which are also not namespaced, so that cluster admins can easily identify that they were created by the linkerd install.

[ihcsim]

@klingerf Done! We'll probably also need another issue to extent the webhook to support other workload like statefulset, daemonset and pods. WDYT?

[klingerf]

⭐️ 🎉 Nice work!! Thanks for addressing all of my feedback. This looks good to me. I opened #1751 to track adding support for non-deployment workloads.

[dendle]

Thanks for the great work @ihcsim