Add validation webhook for service profiles #2623

alpeb · 2019-04-02T23:21:56Z

Fixes #2075

(This supersedes #2508 because it was easier to redo this in master than
rebasing that PR. It addresses all the feedback received in #2508)

The new webhook endpoint is created in a new container in the
linkerd-controller pod.

The proxy injector webhook was refactored to extract-out the scaffolding
to be reused by the validation webhook (and eventually by any other
admission webhooks we'd like to create in the future). The new common
stuff now reside under controller/webhook.

The validation webhook simply sets up the endpoint server and the
webhook config using pkg/webhook, and calls the old
profiles.Validate to perform the actual validation.

I Also got rid of the unused -no-init-container flag in the proxy injector.

Todo in a follow-up PRs: remove the SP check from the CLI check.

l5d-bot · 2019-04-02T23:35:09Z

Integration test results for 956d8f3: success 🎉
Log output: https://gist.github.com/cd89e384c5564be90b0b410ea3851971

siggy

This webhook package is really slick! Most of my feedback is questions to help my own understanding of how this all works. Thanks!

cli/cmd/install.go

controller/sp-validator/tmpl/validating_webhook_configuration.go

controller/webhook/config.go

siggy · 2019-04-03T01:25:45Z

controller/webhook/server.go

+		}
+		return admissionReview
+	}
+	log.Infof("received admission review request %s", admissionReview.Request.UID)


consider logging the namespace/name rather than UID as a more user-friendly message.

curiously when i turned on debug logging, i wasn't seeing the name in the debug output, any idea what's up?

$ bin/linkerd logs --control-plane-component controller --container sp-validator ... linkerd linkerd-controller-c689f4fd9-hpqf6 sp-validator time="2019-04-03T01:22:47Z" level=debug msg="admission request: &AdmissionRequest{UID:fd3716f1-55ae-11e9-adbd-025000000001,Kind:linkerd.io/v1alpha1, Kind=ServiceProfile,Resource:linkerd.io/v1alpha1, Resource=serviceprofiles,SubResource:,Name:,Namespace:linkerd,Operation:CREATE,UserInfo:k8s_io_api_authentication_v1.UserInfo{Username:docker-for-desktop,UID:,Groups:[system:masters system:authenticated],Extra:map[string]ExtraValue{},},Object:k8s_io_apimachinery_pkg_runtime.RawExtension{Raw:*[123

I've found that Request.Name is always sent empty by k8s. From their godoc:

// Name is the name of the object as presented in the request. On a CREATE operation, the client may omit name and // rely on the server to generate the name. If that is the case, this method will return the empty string. // +optional Name string `json:"name,omitempty" protobuf:"bytes,5,opt,name=name"`

controller/webhook/launcher.go

siggy · 2019-04-03T01:32:29Z

controller/webhook/launcher.go

+	}
+	log.Infof("created webhook configuration: %s", selfLink)
+
+	s, err := NewServer(k8sAPI, *addr, config.WebhookServiceName, *controllerNamespace, rootCA, config.Handler)


if config.Handler only exists to get passed through Launch() into NewServer(), consider just making it a separate param on Launch(). similar question for MetricsPort and others.

I kinda did that to avoid having to pass so many arguments, but I think I can reach a compromise in my next push 👍

controller/sp-validator/webhook_ops.go

siggy · 2019-04-03T01:39:53Z

controller/webhook/server.go

+// Start starts the https server
+func (s *Server) Start() {
+	log.Infof("listening at %s", s.Server.Addr)
+	if err := s.ListenAndServeTLS("", ""); err != nil {


is ListenAndServeTLS eventually going to take params?

Nope. The certs are added in s's setup above in the NewServer function.

olix0r · 2019-04-03T02:15:17Z

What happens when --ha is used and multiple controller instances are spawned simultaneously?

olix0r · 2019-04-03T02:23:35Z

I think our options are:

Wait on this until Create Mutating Webhooks during install #2628 is done and proceed with this container in the controller pod.
Create a new deploy with a new service account with a dedicated serviceaccount that has the ClusterRoleBinding to create service accounts. This deploy should not be scaled with HA.

grampelberg · 2019-04-03T15:40:15Z

Create a new deploy with a new service account with a dedicated serviceaccount that has the ClusterRoleBinding to create service accounts. This deploy should not be scaled with HA.

I like this idea in general. The controller pod is getting a little large and hard to reason about. Are there any downsides?

alpeb · 2019-04-03T16:07:27Z

@olix0r great catch 👍
The second option also sounds better to me. Of course #2628 will still have to be done.

grampelberg · 2019-04-03T16:08:28Z

+1 to #2628 as well =)

ihcsim

Some thoughts around the structure of the code. (I will review the webhook handlers separately, but looking good so far).

Also, instead of having the different proxy-injector and sp-validator packages with different webhook.go files and a separate webhook package, how about just put everything into the webhook package and name the different webhook files after their component? I see the case for a separate webhook package only if it's a lib in pkg.

Something like:

controller/webhook/proxy_injector.go
controller/webhook/proxy_injector_ops.go
controller/webhook/sp_validator.go
controller/webhook/sp_validator_ops.go
controller/webhook/tmpl

ihcsim · 2019-04-03T17:44:30Z

controller/cmd/proxy-injector/main.go

-	if err := s.Shutdown(); err != nil {
-		log.Error(err)
-	}
+	webhook.Launch(&webhook.Config{


I find it hard to follow (and sometimes write tests for) objects that are half-created. IIUC, the other half of the webhook.Config is in the launcher. I know there is a bit of a trade-off here, but personally, I'm ok with duplicating the flags code if it helps to make things clearer.

TIOLI: Along with my comment below on removing the ConfigOps interface and Ops struct, this part can look like:

// configure and parse flags code // ... config := NewMWC() // or config := NewVWC() webhook.Launch(config)

ihcsim · 2019-04-03T18:36:11Z

controller/proxy-injector/webhook_ops.go

+)
+
+// Ops satisfies the ConfigOps interface for managing MutatingWebhook configs
+type Ops struct{}


TIOLI: I wonder if we can reduce the amount of code by just creating a MWC and a VWC, and not have the ConfigOps interface and Ops struct.

What about this?

type MWC struct { // fields } func NewMWC(/* args */) *MWC { /* code */ } func (m *MWC) Create() { /* code /* } func (m *MWC) Get() { /* code */ } func (m *MWC) Delete() { /* code */ }

And the same for the VWC.

I think if we move all the code into one package as suggested above, we don't need an ConfigOps interface.

ihcsim

The webhook handlers LGTM. Just two comments on the admission response and VWC.

controller/sp-validator/webhook.go

controller/sp-validator/tmpl/validating_webhook_configuration.go

ihcsim · 2019-04-03T21:17:30Z

What happens when --ha is used and multiple controller instances are spawned simultaneously?

If we ever made the proxy injector a required component, we can consider having just one admission controller webhook (and rename the proxy injector to a more generic admission controller or something). The k8s repo has a similar example with one webhook, but multiple handlers.

grampelberg · 2019-04-03T21:19:18Z

@ihcsim is there a reason to keep them separate right now? Should we write up an issue to fix that in the 2.4 cycle?

ihcsim · 2019-04-03T21:23:30Z

Right now, the proxy injector webhook is an optional component. AFAIK, this validating webhook is required. I can create an issue for it in 2.4.

grampelberg · 2019-04-03T21:27:41Z

The webhook feels like a nice to have (though with minimal impact), I can go either way.

alpeb · 2019-04-03T23:06:16Z

@ihcsim thanks for your reviews, I'm still going through them. One comment though:

how about just put everything into the webhook package and name the different webhook files after their component

If we go down the route of having separate deployments for the proxy-injector and the validator, it would make sense to keep them under different directories under /controller, don't you agree?

ihcsim · 2019-04-04T19:20:04Z

If we go down the route of having separate deployments for the proxy-injector and the validator, it would make sense to keep them under different directories under /controller

I don't think the kind of workloads we deploy to k8s should have any inference on the code packaging structure. Per stand-up, let's make this a TIOLI.

klingerf

Great, this approach looks good to me overall, but I suspect it will change somewhat once you rebase against master and move the profile validator to a separate deployment. Will give it a more thorough look once that's pushed.

chart/templates/controller.yaml

l5d-bot · 2019-04-05T01:43:33Z

Integration test results for 2ecb804: fail 😕
Log output: https://gist.github.com/23b6cadc499a3d71241a8a1865a0a6de

l5d-bot · 2019-04-05T02:06:47Z

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

klingerf

Update looks great! Just had a few bits of miscellaneous feedback, but otherwise looks good to me.

klingerf · 2019-04-05T17:25:37Z

chart/templates/proxy_injector.yaml

@@ -78,6 +78,7 @@ rules:
 - apiGroups: [""]
  resources: ["namespaces"]
  verbs: ["list", "get", "watch"]
+# TODO: this is for creating the webhook config. Can be removed when #2176 lands.


Does this comment belong on line 75, above the privs for "mutatingwebhookconfigurations"? This comment also gets printed in the install YAML that the CLI emits, so I'd call it semi-user-facing. I'm inclined to omit TODOs from our YAML files, unless you feel strongly that we need them.

It actually belongs in both places. The pod listing ability is needed more precisely because of #2559.
I tried using Helm comments ({{- /* comment */ -}}) but for some reason that didn't fly. I'll remove anyways these comments and I'll make a note instead 😉

chart/templates/sp_validator.yaml

controller/webhook/launcher.go

siggy

looking good! a few more comments...

Add sp-validator here (note that proxy-injector is added dynamically when --proxy-auto-inject is set):

linkerd2/test/install_test.go

Lines 36 to 50 in a3bba0e

    
           linkerdSvcs = []string{ 
        
           	"linkerd-controller-api", 
        
           	"linkerd-destination", 
        
           	"linkerd-grafana", 
        
           	"linkerd-identity", 
        
           	"linkerd-prometheus", 
        
           	"linkerd-web", 
        
           } 
        
           linkerdDeployReplicas = map[string]deploySpec{ 
        
           	"linkerd-controller": {1, []string{"destination", "public-api", "tap"}}, 
        
           	"linkerd-grafana":    {1, []string{}}, 
        
           	"linkerd-identity":   {1, []string{"identity"}}, 
        
           	"linkerd-prometheus": {1, []string{}}, 
        
           	"linkerd-web":        {1, []string{"web"}},

controller/cmd/sp-validator/main.go

controller/webhook/launcher.go

controller/webhook/server.go

siggy · 2019-04-05T18:35:40Z

controller/webhook/server.go

+	}
+
+	if len(data) == 0 {
+		return


is this an error or some condition that should be logged?

I believe it's just a sanity check against empty payloads. I'll add some logging.

alpeb · 2019-04-05T20:16:43Z

Alright, thanks for all the feedback 🙏
The latest push applies all the suggestions, and rebases against master.

klingerf

⭐️ Looks great, thanks for making those updates!

Fixes #2075 (This supersedes #2508 because it was easier to redo this in master than rebasing that PR. It addresses all the feedback received in #2508) The new webhook endpoint is created in a new container in the linkerd-controller pod. The proxy injector webhook was refactored to extract-out the scaffolding to be reused by the validation webhook (and eventually by any other admission webhooks we'd like to create in the future). The new common stuff now reside under controller/webhook. The validation webhook simply sets up the endpoint server and the webhook config using pkg/webhook, and calls the old profiles.Validate to perform the actual validation. I Also got rid of the unused -no-init-container flag in the proxy injector. Todo in a follow-up PRs: remove the SP check from the CLI check. Signed-off-by: Alejandro Pedraza <[email protected]>

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

l5d-bot · 2019-04-05T21:23:38Z

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

Add validation webhook for service profiles Fixes linkerd#2075 Todo in a follow-up PRs: remove the SP check from the CLI check. Signed-off-by: Alejandro Pedraza <[email protected]> Signed-off-by: [email protected] <[email protected]>

alpeb mentioned this pull request Apr 2, 2019

Add validation webhook for service profiles #2508

Closed

alpeb self-assigned this Apr 2, 2019

alpeb added the priority/P1 Planned for Release label Apr 2, 2019

siggy reviewed Apr 3, 2019

View reviewed changes

ihcsim reviewed Apr 3, 2019

View reviewed changes

controller/sp-validator/webhook.go Outdated Show resolved Hide resolved

controller/sp-validator/tmpl/validating_webhook_configuration.go Show resolved Hide resolved

klingerf reviewed Apr 4, 2019

View reviewed changes

chart/templates/controller.yaml Outdated Show resolved Hide resolved

alpeb force-pushed the alpeb/serviceprofile-validation2 branch from 956d8f3 to b7f7baf Compare April 5, 2019 01:35

alpeb force-pushed the alpeb/serviceprofile-validation2 branch from 2ecb804 to 5998454 Compare April 5, 2019 01:52

klingerf reviewed Apr 5, 2019

View reviewed changes

siggy reviewed Apr 5, 2019

View reviewed changes

alpeb force-pushed the alpeb/serviceprofile-validation2 branch from 2a6379e to d2acce5 Compare April 5, 2019 20:15

klingerf approved these changes Apr 5, 2019

View reviewed changes

alpeb added 7 commits April 5, 2019 15:54

Address @siggy's feedback

24db1f7

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

Addressed feedback, rebase, separate validator into its own service

8f9242c

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

Update deps

2e01b49

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

Address all feedback

1a75985

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

Update test

37c1b70

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

Another rebase

bf35a14

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

alpeb force-pushed the alpeb/serviceprofile-validation2 branch from 833a167 to bf35a14 Compare April 5, 2019 21:01

alpeb merged commit edb2250 into master Apr 5, 2019

alpeb deleted the alpeb/serviceprofile-validation2 branch April 5, 2019 21:10

alpeb mentioned this pull request Apr 23, 2019

Replace client.Corev1() calls with k8s.API calls in webhook.go #2566

Closed

	linkerdSvcs = []string{
	"linkerd-controller-api",
	"linkerd-destination",
	"linkerd-grafana",
	"linkerd-identity",
	"linkerd-prometheus",
	"linkerd-web",
	}

	linkerdDeployReplicas = map[string]deploySpec{
	"linkerd-controller": {1, []string{"destination", "public-api", "tap"}},
	"linkerd-grafana": {1, []string{}},
	"linkerd-identity": {1, []string{"identity"}},
	"linkerd-prometheus": {1, []string{}},
	"linkerd-web": {1, []string{"web"}},

Add validation webhook for service profiles #2623

Add validation webhook for service profiles #2623

Conversation

alpeb commented Apr 2, 2019

l5d-bot commented Apr 2, 2019

siggy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olix0r commented Apr 3, 2019

olix0r commented Apr 3, 2019

grampelberg commented Apr 3, 2019

alpeb commented Apr 3, 2019

grampelberg commented Apr 3, 2019

ihcsim left a comment

Choose a reason for hiding this comment

ihcsim Apr 3, 2019 • edited Loading

Choose a reason for hiding this comment

ihcsim Apr 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ihcsim left a comment

Choose a reason for hiding this comment

ihcsim commented Apr 3, 2019

grampelberg commented Apr 3, 2019

ihcsim commented Apr 3, 2019

grampelberg commented Apr 3, 2019

alpeb commented Apr 3, 2019

ihcsim commented Apr 4, 2019

klingerf left a comment

Choose a reason for hiding this comment

l5d-bot commented Apr 5, 2019

l5d-bot commented Apr 5, 2019

klingerf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siggy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpeb commented Apr 5, 2019

klingerf left a comment

Choose a reason for hiding this comment

l5d-bot commented Apr 5, 2019

ihcsim Apr 3, 2019 •

edited

Loading

ihcsim Apr 3, 2019 •

edited

Loading