Introduce validating admission webhook (experimental) #1133

alenkacz · 2019-12-03T09:35:02Z

What this PR does / why we need it:
Please start reviewing by reading the user-facing documentation and reviewing also that kudobuilder/kudo.dev#110

Fixes #883

There are no integration tests (this is not possible to test via these) and no e2e tests right now. As this is experimental feature it's "OK" like that, but any suggestions in that space are appreciated, basically for that to be able to be test via current setup we would need pre-install cert-manager into kind cluster and start kudo with webhook (as harness takes care of starting kudo).

Also this is known as minimal implementation, all the following steps have been identified in these github issues component/webhook

kensipe

missing docs on setting up the cert manager
also... where is the link to the sequence diagram... super useful!

This is fantastic work!!! Really great. My request for change is really around some small nits and some addressing of the future of additional webhooks.

Makefile

cmd/manager/main.go

kensipe · 2019-12-03T21:52:59Z

cmd/manager/main.go

+			os.Exit(1)
+		}
+	}
+
 	// Start the Cmd
 	log.Info("Starting the Cmd.")


I'm curious why you changed CRD installation messages to past tense and after the event... and left all the manager manages as logs prior to the event?
also seems like something that should have been on separate pr.

hmm which messages? I don't see any change on this line

pkg/apis/kudo/v1beta1/instance_validator.go

kensipe · 2019-12-03T22:16:22Z

pkg/kudoctl/cmd/init.go

@@ -98,6 +99,7 @@ func newInitCmd(fs afero.Fs, out io.Writer) *cobra.Command {
 	f.BoolVar(&i.crdOnly, "crd-only", false, "Add only KUDO CRDs to your cluster")
 	f.BoolVarP(&i.wait, "wait", "w", false, "Block until KUDO manager is running and ready to receive requests")
 	f.Int64Var(&i.timeout, "wait-timeout", 300, "Wait timeout to be used")
+	f.BoolVar(&i.enableValidation, "enable-validation", false, "When set to true, validation webhook will be deployed alongside KUDO controller")


as we discussed... I think this should be more clear and likely more granular... perhaps --web-hooks=InstanceValidator
with a comma separated list... and perhaps --web-hooks=All for convince.
Not a blocker for me.. just think we will have to address this at some point.

kensipe · 2019-12-03T22:17:47Z

pkg/kudoctl/cmd/init.go

@@ -149,6 +151,14 @@ func (initCmd *initCmd) run() error {
 			}
 			mans = append(mans, prereq...)

+			if opts.EnableValidation {
+				prereq, err := cmdInit.WebhookManifests(opts.Namespace)


WebhookManifests likely needs to passed a options with a slice... or another parameter of slice of options.

pkg/kudoctl/cmd/init/webhook.go

ANeumann82 · 2019-12-04T12:48:59Z

pkg/kudoctl/cmd/init/webhook.go

+
+func installAdmissionWebhook(client clientv1beta1.ValidatingWebhookConfigurationsGetter, webhook v1beta1.ValidatingWebhookConfiguration) error {
+	_, err := client.ValidatingWebhookConfigurations().Create(&webhook)
+	if kerrors.IsAlreadyExists(err) {


In this case, should we Get the webhook configuration and deep-compare if it matches the expected configuration? Thinking about changes in the webhook, for example when we want to validate operatorVersions in addition to instances

this is along the lines we handle all other resources right now. If we want to change it, I would do it for all and in another PR... but in general I agree :)

Co-Authored-By: Ken Sipe <[email protected]>

zen-dog

I left a few nits around naming and what not. The only reason to request changes is explained here.

zen-dog · 2019-12-04T13:55:02Z

cmd/manager/main.go

+	if err != nil {
+		return err
+	}
+	validator, isValidator := obj.(kudo.Validator)


nit: these variable are called ok by convention

Suggested change

validator, isValidator := obj.(kudo.Validator)

validator, ok := obj.(kudo.Validator)

I guess this method is adopted from the controller-runtime but since we're the one calling it, you could pass kudo.Validatior directly and avoid the casting?

yeah I need runtime.object here to be able to get GVK

it's ok then ;)

Right right, it's ok now... 😝

zen-dog · 2019-12-04T14:04:00Z

cmd/manager/main.go

+		log.Infof("skip registering a validating webhook, admission.Validator interface is not implemented", "GVK", gvk)
+		return nil
+	}
+	vwh := kudo.ValidatingWebhookFor(validator)


Given multiple hook kinds (mutating, conversion) and the fact that Go doesn't allow you to overload functions, you probably want to have that in name.

cmd/manager/main.go

zen-dog · 2019-12-04T14:08:27Z

cmd/manager/main.go

+	}
+	vwh := kudo.ValidatingWebhookFor(validator)
+	if vwh != nil {
+		path := generateValidatePath(gvk)


All of that is adopted from the controller-runtime. It might not be ideal but I'd rather keep it this way: easier to bump once cr changes things.

zen-dog · 2019-12-04T14:09:14Z

go.sum

@@ -277,6 +277,7 @@ github.com/google/go-cmp v0.3.0/go.mod h1:8QqcDgzrUqlUb/G2PQTWiueGozuR1884gddMyw
 github.com/google/gofuzz v0.0.0-20161122191042-44d81051d367/go.mod h1:HP5RmnzzSNb993RKQDq4+1A4ia9nllfqcQFTQJedwGI=
 github.com/google/gofuzz v1.0.0 h1:A8PeW59pxE9IoFRqBp37U+mSNaQoZ46F1f0f863XSXw=
 github.com/google/gofuzz v1.0.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg=
+github.com/google/martian v2.1.0+incompatible h1:/CP5g8u/VJHijgedC/Legn3BAbAaWPgecwXBIDzw5no=


No reason to introduce another logger, I guess?

pkg/apis/kudo/v1beta1/instance_validator.go

pkg/kudoctl/cmd/init/webhook.go

cmd/manager/main.go

kensipe

there are a few minor changes I would still prefer to see... but I'm ok even without.
nice work!

kensipe · 2019-12-04T20:52:13Z

pkg/kudoctl/cmd/init.go

@@ -98,6 +99,7 @@ func newInitCmd(fs afero.Fs, out io.Writer) *cobra.Command {
 	f.BoolVar(&i.crdOnly, "crd-only", false, "Add only KUDO CRDs to your cluster")
 	f.BoolVarP(&i.wait, "wait", "w", false, "Block until KUDO manager is running and ready to receive requests")
 	f.Int64Var(&i.timeout, "wait-timeout", 300, "Wait timeout to be used")
+	f.StringVar(&i.webhooks, "webhook", "", "List of webhooks exposed, when empty, no webhook server will be started (the only webhook right now is InstanceValidation)")


Suggested change

f.StringVar(&i.webhooks, "webhook", "", "List of webhooks exposed, when empty, no webhook server will be started (the only webhook right now is InstanceValidation)")

f.StringVar(&i.webhooks, "webhook", "", "List of webhooks to install separated by commas")

"list of webhooks exposed" isn't phrased corrected. "to be exposed" is closer. It is odd to add the "(...)" to a cli output or to specify limitations that will need to be edited and hopefully not forgotten.

The info I expect a user to be concerned with... is... is it the name of the class? does it include the package name? where can I find the mapping of strings -> webhooks.

hmm I really wanted to help the user to know what to do here because if you know what are the supported options you have to go to the docs or code (?) and that's not very nice experience. I think the CLI should help you do this right. I rephrased it a bit.

Yes we will need to update it with every new webhook but this is the best documentation we have and the best documentation we can get to the user so I think we should be fine with it and just really take care of this docs and keep it up to date. We won't certainly add new webhook every week...

kensipe · 2019-12-04T20:55:51Z

pkg/kudoctl/cmd/init.go

@@ -118,13 +120,16 @@ func (initCmd *initCmd) validate(flags *flag.FlagSet) error {
 	if flags.Changed("wait-timeout") && !initCmd.wait {
 		return errors.New("wait-timeout is only useful when using the flag '--wait'")
 	}
+	if initCmd.webhooks != "" && initCmd.webhooks != "InstanceValidation" {
+		return errors.New("webhooks can be only empty or contain a single string 'InstanceValidation'. No other webhooks supported right now")


again... I know we are looking to land this and it is limited to 1 WH... but we know we will add more. Couldn't we have a map of webhooks which contains 1 currently and have some infra code here that doesn't need to change as we add more in?

As far as the error message is concerned... certainly "no other webhooks are supported" should be the last sentence... "right now" is implied.

I find that premature optimization - I don't see the advantage of writing it now rather then later :) it's the same job and later we'll have more information because we'll really be at the point when adding new webhook and having multiple. And we'll need to change this code anyway otherwise the validation would be failing. Whether it should be map or slice, I would prefer to leave that decision for later, when we really need to maintain multiple. I choose the easiest option for now

I removed the right now though, thanks

pkg/kudoctl/cmd/init/webhook.go

go.mod

zen-dog

We should mention it somewhere that running plans are not interruptable anymore.

alenkacz added 5 commits November 30, 2019 09:12

WIP

231db59

Merge branch 'master' into av/webhook

f0721d5

Small refactoring of init tests as a preparation

b768149

Fix tests

ac1c3bf

Revert makefile

285c10e

alenkacz requested review from gerred, kensipe, nfnt and zen-dog as code owners December 3, 2019 09:35

alenkacz added 2 commits December 3, 2019 13:19

Fixes

55a893b

Fix makefile

8712590

kensipe requested changes Dec 3, 2019

View reviewed changes

ANeumann82 reviewed Dec 4, 2019

View reviewed changes

pkg/kudoctl/cmd/init/webhook.go Outdated Show resolved Hide resolved

ANeumann82 reviewed Dec 4, 2019

View reviewed changes

alenkacz and others added 2 commits December 4, 2019 16:02

Update cmd/manager/main.go

fc12fe8

Co-Authored-By: Ken Sipe <[email protected]>

PR review comments

1f4664e

zen-dog requested changes Dec 4, 2019

View reviewed changes

Accept webhooks as a flag for init

021d64d

mpereira reviewed Dec 4, 2019

View reviewed changes

cmd/manager/main.go Outdated Show resolved Hide resolved

alenkacz added 2 commits December 4, 2019 19:15

Review comments

97961a7

Make linter happy

5876758

kensipe approved these changes Dec 4, 2019

View reviewed changes

kensipe reviewed Dec 4, 2019

View reviewed changes

pkg/kudoctl/cmd/init/webhook.go Outdated Show resolved Hide resolved

kensipe and others added 5 commits December 4, 2019 15:09

updating imports

3ea53ed

PR review comments

7be7f92

Merge branch 'av/webhook' of github.com:kudobuilder/kudo into av/webhook

d92acd9

It's ok now :D

3ce028e

Fix arg parsing

286c2da

zen-dog reviewed Dec 5, 2019

View reviewed changes

go.mod Show resolved Hide resolved

alenkacz mentioned this pull request Dec 5, 2019

Design a solution for interrupting plans #1140

Open

zen-dog approved these changes Dec 5, 2019

View reviewed changes

alenkacz added release-highlight do-not-merge/hold labels Dec 6, 2019

Also create

6fc2d8a

alenkacz removed the do-not-merge/hold label Dec 6, 2019

Merge

596dac7

alenkacz merged commit da27754 into master Dec 6, 2019

alenkacz deleted the av/webhook branch December 6, 2019 17:29

porridge added release/highlight This PR is a highlight for the next release and removed release-highlight labels Mar 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce validating admission webhook (experimental) #1133

Introduce validating admission webhook (experimental) #1133

alenkacz commented Dec 3, 2019

kensipe left a comment

kensipe Dec 3, 2019

alenkacz Dec 4, 2019

kensipe Dec 3, 2019

kensipe Dec 3, 2019

ANeumann82 Dec 4, 2019

alenkacz Dec 4, 2019

zen-dog left a comment •

edited

Loading

zen-dog Dec 4, 2019

zen-dog Dec 4, 2019

alenkacz Dec 4, 2019 •

edited

Loading

zen-dog Dec 4, 2019 •

edited

Loading

alenkacz Dec 5, 2019

zen-dog Dec 4, 2019

zen-dog Dec 4, 2019

zen-dog Dec 4, 2019

kensipe left a comment

kensipe Dec 4, 2019

alenkacz Dec 5, 2019

kensipe Dec 4, 2019

alenkacz Dec 5, 2019

zen-dog left a comment

	validator, isValidator := obj.(kudo.Validator)
	validator, ok := obj.(kudo.Validator)

	f.StringVar(&i.webhooks, "webhook", "", "List of webhooks exposed, when empty, no webhook server will be started (the only webhook right now is InstanceValidation)")
	f.StringVar(&i.webhooks, "webhook", "", "List of webhooks to install separated by commas")

Introduce validating admission webhook (experimental) #1133

Introduce validating admission webhook (experimental) #1133

Conversation

alenkacz commented Dec 3, 2019

kensipe 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

zen-dog left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alenkacz Dec 4, 2019 • edited Loading

Choose a reason for hiding this comment

zen-dog Dec 4, 2019 • edited Loading

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

kensipe 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

zen-dog left a comment

Choose a reason for hiding this comment

zen-dog left a comment •

edited

Loading

alenkacz Dec 4, 2019 •

edited

Loading

zen-dog Dec 4, 2019 •

edited

Loading