WIP: Introduce mutating webhook for sidecar injection. by Bedotech · Pull Request #1423 · linkerd/linkerd2

Bedotech · 2018-08-08T13:18:52Z

For manage deployment with linkerd2 with other tools like helm is really
important to make the pod injection at the lower level we can.

#561 Using MutatingWebhook (new k8s 1.9) we can modify a pod before the pod
is created.

So far i have created:

new container called 'sidecar-injection'
update install profile for add 'sidecar-injection'
create for now fake webhook for test integration

This PR is really WIP, before we have to solve:

Ho we issue certificate for webhook in k8s
Link the webhook inject handler with existing api for sidecar
injection.
code review.

Signed-off-by: Mattia Rossi [email protected]

rmars

🎉 thank you for undertaking this, @Bedotech!

This PR is a good starting point for us to start to think about and flesh out some of the design tradeoffs we'll need to make.

I discussed this with the team and the main design considerations we'd like to start thinking about are:

how we can integrate this into the install experience such that the user can do minimal work. Can the cert setup be included as part of linkerd install? How much work would it be to get there from here? Is it worth doing it as a follow up, or can we get it integrated now?
Can we use our existing TLS bootstrapping?
can the webhook server be another subcomponent in the cmd/ directory? this way we can build and maintain fewer docker images

We'd also like to run the TLS setup we decide on by @briansmith before it merges.

I also had a couple of starting smaller review points. A lot of the code here is likely to change as we think through this together, but this is a great start!

rmars · 2018-08-10T18:20:18Z

doc/sidecar-injection.md

+
+## Setup
+
+Once you have create the secret you can run `linkerd install`, for automatic


As mentioned above, eventually we'll want to find a way to have this be run from linkerd install (seeing if gating the admisison controller feature behind a flag is possible)

Ok i think this can be done after we can effectively inject

rmars · 2018-08-10T18:56:19Z

sidecar-injection/webhook.go

+}
+
+func init() {
+	_ = corev1.AddToScheme(runtimeScheme)


I think we can omit the _ = if we're just throwing out all these results anyway

rmars · 2018-08-10T18:57:38Z

sidecar-injection/main.go

+	"os/signal"
+	"syscall"
+
+	"github.com/golang/glog"


note that we use sirupsen/logrus for logging instead of glog- we'll need to move these over.

rmars · 2018-08-10T20:53:45Z

sidecar-injection/webhook.go

+
+func updateAnnotation(target map[string]string, added map[string]string) (patch []patchOperation) {
+	for key, value := range added {
+		if target == nil || target[key] == "" {


should this second || target[key] be here? if target[key] doesn't exist, do we want to re-initialize the entire annotation map?

rmars · 2018-08-10T21:07:47Z

sidecar-injection/webhook.go

+		} else {
+			patch = append(patch, patchOperation{
+				Op:    "replace",
+				Path:  "/metadata/annotations/" + key,


Do we need to escape the key here?

// the Path of the JSON patch is a JSON pointer value // so we need to escape any "/"s in the key we add to the annotation // https://tools.ietf.org/html/rfc6901 func escapeJSONPointer(s string) string { esc := strings.Replace(s, "~", "~0", -1) esc = strings.Replace(esc, "/", "~1", -1) return esc }

rmars · 2018-08-11T00:34:28Z

doc/sidecar-injection.md

@@ -0,0 +1,64 @@
+++


😻 thank you for adding documentation!

ihcsim · 2018-08-15T18:51:13Z

sidecar-injection/main.go

+
+	// get command line parameters
+	flag.IntVar(&parameters.port, "port", 443, "Webhook server port.")
+	flag.StringVar(&parameters.certFile, "tlsCertFile", "/etc/webhook/certs/cert.pem", "File containing the x509 Certificate for HTTPS.")


I think we should follow the convention used by the other controllers' flags i.e. tls-cert-file, instead of tlsCertFile.

ihcsim · 2018-08-15T18:52:36Z

sidecar-injection/webhook.go

+
+	var body []byte
+	if r.Body != nil {
+		if data, err := ioutil.ReadAll(r.Body); err == nil {


I think we should handle the case of err !=nil. Otherwise, the empty body error below will be returned to the user which can be misleading.

ihcsim · 2018-08-20T05:17:20Z

sidecar-injection/webhook.go

+}
+
+func addContainer(target, added []corev1.Container, basePath string) (patch []patchOperation) {
+	first := len(target) == 0


Is it possible for target's length to be 0?

ihcsim · 2018-08-20T05:19:49Z

sidecar-injection/webhook.go

+		admissionResponse = whsvr.mutate(&ar)
+	}
+
+	admissionReview := v1beta1.AdmissionReview{}


Might not need this and re-use ar from line 279. The AdmissionReview type has both a Request and a Response fields.

For manage deployment with linkerd2 with other tools like helm is really important to make the pod injection at the lower level we can. Using MutatingWebhook (new k8s 1.9) we can modify a pod before the pod is created. So far i have created: * new container called 'sidecar-injection' * update install profile for add 'sidecar-injection' * create for now fake webhook for test integration This PR is really WIP, before we have to solve: * Ho we issue certificate for webhook in k8s * Link the webhook inject handler with existing api for sidecar injection. * code review. Signed-off-by: Mattia Rossi <[email protected]>

Trye to solve the SSL problem with .sh script that sign a certificate using kubectl.

briansmith · 2018-08-21T23:38:53Z

bin/webhook-create-signed-cert.sh

+https://kubernetes.io/docs/tasks/tls/managing-tls-in-a-cluster for
+detailed explantion and additional instructions.
+
+The server key/cert k8s CA cert are stored in a k8s secret.


I don't think this will work as a long-term solution:

The private key for the certificate shouldn't ever be outside the cluster, so this script should be run from within the controller, not by the user.

The CertificateSigningRequest API isn't always enabled and even when enabled isn't always configured in a usable state. For example and in particular, I don't think minikube enables this in its default configuration.

AFAICT, using the CertificateSigningRequest API isn't necessary because the webhook configuration YAML lets you specify the CA to be trusted for the webhook.

The way I was thinking to do this is as follows:

conduit install installs the webhook along with the rest of the control plane but it won't work yet because the object properties that indicates the webhook's CA won't be written yet.

When the CA starts up, (and in the future, whenever the root CA certificates change) the CA replaces the webhooks configuration with one that has its own certificate as the trusted CA certificate. This requires the CA to have edit properties for the webhook configuration object.

After step 2 the webhook should work.

Yeah, this is really the first implementation, if you look at Istio the setup and creation of the certificate is made at the setup time using k8s api.

The point 1 or 2 are prerequisite.

Only one point, i have made all test against clean minikube environment and all is working , so i think the CertificateSigningRequest it's enabled by default.

... creation of the certificate is made at the setup time using k8s api.

Not 100% sure, from what I see in the citadel code, they may no longer be relying on the k8s api to generate and sign CSR. Can you confirm?

References:

security/cmd/istio_ca/main.go#L427-L454)

security/pkg/pki/ca/ca.go#L174-L194

security/pkg/pki/util/generate_cert.go#L107-L114

cppforlife · 2018-09-21T21:41:10Z

following package https://github.com/knative/pkg/tree/master/webhook might be handy to use for implementation. it got extracted out of few knative components recently cc @mattmoor

here it's being used: https://github.com/knative/serving/blob/279424145446338b96c4e5aad9a2700db6b0ea77/cmd/webhook/main.go#L78

klingerf · 2018-10-15T20:34:03Z

@Bedotech Thanks for your hard work on this. Auto injection support was added in #1714, so I'm going to close out this PR. Thanks again for doing so much initial investigation.

grampelberg requested a review from klingerf August 8, 2018 15:16

grampelberg assigned Bedotech Aug 8, 2018

grampelberg requested review from rmars and removed request for klingerf August 9, 2018 17:52

rmars reviewed Aug 11, 2018

View reviewed changes

ihcsim reviewed Aug 15, 2018

View reviewed changes

ihcsim reviewed Aug 20, 2018

View reviewed changes

Mattia Rossi added 2 commits August 20, 2018 19:01

Update mutating webhook, adding some docs.

a720829

Trye to solve the SSL problem with .sh script that sign a certificate using kubectl.

Bedotech force-pushed the add-mutating-webhook-for-sidecar-injection branch from a1ac10c to a720829 Compare August 20, 2018 17:01

briansmith reviewed Aug 21, 2018

View reviewed changes

ihcsim mentioned this pull request Sep 28, 2018

Proxy init and sidecar containers auto-injection #1714

Merged

klingerf closed this Oct 15, 2018


		## Setup

		Once you have create the secret you can run `linkerd install`, for automatic

Conversation

Bedotech commented Aug 8, 2018

Uh oh!

rmars left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ihcsim Aug 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cppforlife commented Sep 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

klingerf commented Oct 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ihcsim Aug 20, 2018 •

edited

Loading

cppforlife commented Sep 21, 2018 •

edited

Loading