Migrate registry/strategy Updates to webhooks #17

piotrmiskiewicz · 2019-04-08T07:17:31Z

This PR is a

Feature Implementation
Bug Fix
Documentation

What this PR does / why we need it:

Move logic from pkg/registry/*/strategy to webhooks

Which issue(s) this PR fixes
Resolves: kyma-project/kyma#2792

Please leave this checklist in the PR comment so that maintainers can ensure a good PR.

Merge Checklist:

New feature
- Tests
- Documentation
SVCat CLI flag
Server Flag for config
- Chart changes
- removing a flag by marking deprecated and hiding to avoid
  breaking the chart release and existing clients who provide a
  flag that will get an error when they try to update

CLAassistant · 2019-04-08T07:17:38Z

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

mszostok

In issue that you want to resolve is such comment

For validation take into account that we have already the static validation on CRD, so recheck what else needs to be done in ValidatingAdmissionWebhook.

Check also if we shouldn't remove the static validation from CRD and implement that in ValidatingAdmissionWebhook - take into account how user-friendly is the returned validation error.

Why in your pr it's not done? Right now we have duplicated logic, also the err msgs are not validated and I do not see any statement which approach was chosen and why. Base on my testing we should remove validation from CRDs

What's more right now you have registered validation which cannot work:

ServiceBinding (ValidateServiceBinding), under the hood, is doing:

	if create {
		allErrs = append(allErrs, validateServiceBindingCreate(binding)...)
	} else {
		allErrs = append(allErrs, validateServiceBindingUpdate(binding)...)
	}

both operation are validating status which is not provided on normal CREATE operation for ServiceBinding and it will not work.. Checking must be moved to status update handler. Validation for creating is not needed because user is not able to do that.

ServiceInstance (ValidateServiceInstanceUpdate), under the hood is validating the status entry, why it's not registered in webhook??

mszostok · 2019-04-10T09:45:31Z

pkg/apis/servicecatalog/v1beta1/validation/binding.go

@@ -0,0 +1,251 @@
+/*


as discussed. We can remove this copy and just adjust the imports in previous package.

removed the package and modified the old one (to use proper types/imports which makes API Server version not workig as discussed)

mszostok · 2019-04-11T09:18:29Z

pkg/webhook/servicecatalog/clusterservicebroker/mutation/handler.go

-		h.mutateOnUpdate(ctx, mutated)
+		originalObj := &sc.ClusterServiceBroker{}
+		if err := h.decoder.DecodeRaw(req.OldObject, originalObj); err != nil {
+			traced.Errorf("Could not decode request object: %v", err)


be more verbose, right now we cannot distinguish if it was problem with oldObj or req.Obj (line 59)

same comment for all other places

log message changed

mszostok · 2019-04-11T09:20:32Z

pkg/webhook/servicecatalog/clusterservicebroker/mutation/handler.go

@@ -106,6 +112,17 @@ func (h *CreateUpdateHandler) mutateOnCreate(ctx context.Context, sb *sc.Cluster
 	}
 }

-func (h *CreateUpdateHandler) mutateOnUpdate(ctx context.Context, obj *sc.ClusterServiceBroker) {
-	// TODO: implement logic from pkg/registry/servicecatalog/clusterservicebroker/strategy.go
+func (h *CreateUpdateHandler) mutateOnUpdate(ctx context.Context, oldSb, newSb *sc.ClusterServiceBroker) {


minior: sometimes you are using originalObj sometimes oldSb, please unify that

same comment for all other places

mszostok · 2019-04-11T09:22:36Z

pkg/webhook/servicecatalog/clusterservicebroker/mutation/handler.go

+
+	// Spec updates bump the generation so that we can distinguish between
+	// spec changes and other changes to the object.
+	if !apiequality.Semantic.DeepEqual(oldSb.Spec, newSb.Spec) {


did you check that behavior previously? In the webhook approach, this is not needed at all. Please remove that.

same comment for all other places

mszostok · 2019-04-11T09:24:01Z

pkg/webhook/servicecatalog/clusterservicebroker/mutation/handler_test.go

+				},
+			},
+		},
+		"Should override relist request": {


Suggested change

"Should override relist request": {

"Should restore previous relist request, when not provided (set to 0)": {

mszostok · 2019-04-11T09:44:10Z

pkg/webhook/servicecatalog/servicebinding/mutation/handler.go

@@ -97,10 +105,33 @@ func (h *CreateUpdateHandler) mutateOnCreate(ctx context.Context, req admission.
 	if utilfeature.DefaultFeatureGate.Enabled(scfeatures.OriginatingIdentity) {
 		setServiceBindingUserInfo(req, binding)
 	}
+
+	// Fill in the first entry set to "creating"?


this comment is not needed here

mszostok · 2019-04-11T09:44:45Z

pkg/webhook/servicecatalog/servicebinding/mutation/handler.go

@@ -84,7 +91,8 @@ func (h *CreateUpdateHandler) InjectDecoder(d *admission.Decoder) error {
 }

 func (h *CreateUpdateHandler) mutateOnCreate(ctx context.Context, req admission.Request, binding *sc.ServiceBinding) {
-	binding.Finalizers = []string{sc.FinalizerServiceCatalog}
+	// This feature was copied from Service Catalog registry: https://github.com/kubernetes-incubator/service-catalog/blob/master/pkg/registry/servicecatalog/binding/strategy.go


this comment is not needed here. The purpose of this comment is to have it in places where you copied a lot of logic, e.g. as you had for validation packages. In this place is more like adjustments because some logic is not ported e.g. as we have with recovering status entries, incrementing generation etc.

mszostok · 2019-04-11T09:46:18Z

pkg/webhook/servicecatalog/servicebinding/mutation/handler.go

+		if utilfeature.DefaultFeatureGate.Enabled(scfeatures.OriginatingIdentity) {
+			setServiceBindingUserInfo(req, newObj)
+		}
+		newObj.Generation = oldObj.Generation + 1


please remove it is done by api server

mszostok · 2019-04-11T09:57:03Z

pkg/webhook/servicecatalog/serviceinstance/mutation/handler.go

@@ -91,6 +97,8 @@ func (h *CreateUpdateHandler) Handle(ctx context.Context, req admission.Request)
 		return admission.Errored(http.StatusInternalServerError, err)
 	}

+	traced.Infof("subresource %v", req.SubResource)


should it still be here?

no, I forgot to remove it. fixed

mszostok · 2019-04-11T10:00:39Z

pkg/webhook/servicecatalog/servicebroker/validation/handler_status.go

+
+func (h *StatusUpdateHandler) Handle(ctx context.Context, req admission.Request) admission.Response {
+	traced := webhookutil.NewTracedLogger(req.UID)
+	traced.Infof("Start handling operation: %s for %s/%s: %q", req.Operation, req.Kind.Kind, req.SubResource, req.Name)


please be more precise. What kind of handler it is? It' will be hard to distinguish between validation sepc, mutation spec, and mutation status entries

enhanced message

adamwalach · 2019-04-16T06:07:04Z

charts/catalog/values.yaml

@@ -28,7 +28,7 @@ webhook:
    nodePort:
      # Available port in allowable range (e.g. 30000 - 32767 on minikube)
      # The TLS-enabled endpoint will be exposed here
-      securePort: 30443
+      securePort: 30444


Why was the securePort changed?

by accident, reverted

adamwalach · 2019-04-16T08:34:36Z

pkg/webhook/servicecatalog/clusterserviceplan/mutation/handler.go

@@ -38,7 +38,7 @@ var _ admission.Handler = &CreateUpdateHandler{}
 // Handle handles admission requests.
 func (h *CreateUpdateHandler) Handle(ctx context.Context, req admission.Request) admission.Response {
 	traced := webhookutil.NewTracedLogger(req.UID)
-	traced.Infof("Start handling operation: %s for %s: %q", req.Operation, req.Kind.Kind, req.Name)
+	traced.Infof("Start handling validation  operation: %s for %s: %q", req.Operation, req.Kind.Kind, req.Name)


should be "mutation" operation?

… (CRDs) solution (kubernetes-retired#2630) * Add basic validation to crds * Add webhook skeleton, remove api-server from chart, add webhoook server in chart, move PrepareForCreate login into webhook handler (kyma-incubator#2) * Add webhook skeleton, remove api-server from chart, add webhoook server in chart, move PrepareForCreate login into webhook handler * Add logger and GVK matcher * Add test coverage for webhook (kyma-incubator#6) * Add Status entry initialization in binding and instance controller (kyma-incubator#5) * Change fs to label selector (kyma-incubator#9) * Fix removing finalizer after switching to CRD /status sub-resource (kyma-incubator#8) * Add tests to webhooks (kyma-incubator#11) * Replace changevalidator with webhook (kyma-incubator#14) * Replace default service plan with webhook (kyma-incubator#10) * Add tests to webhooks - fix * Rewrite defaultServicePlan feature to webhook * Replace plugins by webhook (kyma-incubator#16) * Replace ServiceBinding plugin by webhook * Replace Broker plugins by webhook * Adjust webhooks to multi validation handlers * Service Catalog going towards to CRDs (kyma-incubator#18) * Migrate registry/strategy Updates to webhooks (kyma-incubator#17) * Use Update instead of updateReference method (kyma-incubator#19) * Replace tableconvertor with APC (kyma-incubator#20) * Fix svcat tests after the rebase with the upstream master branch * Pre delete jobs - remove CRD after delete helm release (kyma-incubator#21) * Apply fixes after executing `make verify` * Create docs about webhook implementation (kyma-incubator#24) * Change the securePort for the webhook server because colidates with old api-server * Change import paths to kubernetes-sigs, and rebase with master * Apply fixes after rebase Fixes: * makefile targets, * instance deprovision operation * entries under additionalPrinterColumns in crds.yaml * unit tests after rebase * Update documentation (kyma-incubator#40) * Update docs * Fix vendor after rebase with k8s 1.15 bump * Apply changes after review - remove the contrib/hack/crd folder - remove reference to Kyma project - rebase with current master - restore the image in chart - extract CRDs defintion to dedicated folder

piotrmiskiewicz force-pushed the migrate-registry2webhooks branch 4 times, most recently from 260fe11 to 6cf0d40 Compare April 9, 2019 11:58

piotrmiskiewicz marked this pull request as ready for review April 9, 2019 12:45

piotrmiskiewicz force-pushed the migrate-registry2webhooks branch from 6cf0d40 to 483463d Compare April 9, 2019 12:50

mszostok suggested changes Apr 11, 2019

View reviewed changes

mszostok mentioned this pull request Apr 11, 2019

Replace Service Catalog references store kyma-project/kyma#2836

Closed

piotrmiskiewicz force-pushed the migrate-registry2webhooks branch 3 times, most recently from e6dabe4 to 9f1c183 Compare April 12, 2019 14:20

adamwalach reviewed Apr 16, 2019

View reviewed changes

mszostok approved these changes Apr 16, 2019

View reviewed changes

Migrate registry/strategy Updates to webhooks

ff732c0

piotrmiskiewicz force-pushed the migrate-registry2webhooks branch from 0fb33b3 to ff732c0 Compare April 16, 2019 12:29

piotrmiskiewicz merged commit 043dfd8 into kyma-incubator:crd-prod-impl Apr 16, 2019

mszostok pushed a commit that referenced this pull request May 17, 2019

Migrate registry/strategy Updates to webhooks (#17)

56e38af

mszostok pushed a commit that referenced this pull request Jun 12, 2019

Migrate registry/strategy Updates to webhooks (#17)

bd04569

polskikiel pushed a commit that referenced this pull request Aug 7, 2019

Migrate registry/strategy Updates to webhooks (#17)

b570dc1

mszostok pushed a commit that referenced this pull request Aug 14, 2019

Migrate registry/strategy Updates to webhooks (#17)

8bdab53

mszostok pushed a commit that referenced this pull request Sep 11, 2019

Migrate registry/strategy Updates to webhooks (#17)

8b7c7c0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate registry/strategy Updates to webhooks #17

Migrate registry/strategy Updates to webhooks #17

piotrmiskiewicz commented Apr 8, 2019 •

edited

Loading

CLAassistant commented Apr 8, 2019

mszostok left a comment

mszostok Apr 10, 2019

piotrmiskiewicz Apr 12, 2019

mszostok Apr 11, 2019

piotrmiskiewicz Apr 12, 2019

mszostok Apr 11, 2019

piotrmiskiewicz Apr 12, 2019

mszostok Apr 11, 2019

piotrmiskiewicz Apr 12, 2019

mszostok Apr 11, 2019

piotrmiskiewicz Apr 12, 2019

mszostok Apr 11, 2019

piotrmiskiewicz Apr 12, 2019

mszostok Apr 11, 2019

piotrmiskiewicz Apr 12, 2019

mszostok Apr 11, 2019

piotrmiskiewicz Apr 12, 2019

mszostok Apr 11, 2019

piotrmiskiewicz Apr 12, 2019

mszostok Apr 11, 2019

piotrmiskiewicz Apr 12, 2019

adamwalach Apr 16, 2019 •

edited

Loading

piotrmiskiewicz Apr 16, 2019

adamwalach Apr 16, 2019

piotrmiskiewicz Apr 16, 2019

	"Should override relist request": {
	"Should restore previous relist request, when not provided (set to 0)": {

Migrate registry/strategy Updates to webhooks #17

Migrate registry/strategy Updates to webhooks #17

Conversation

piotrmiskiewicz commented Apr 8, 2019 • edited Loading

CLAassistant commented Apr 8, 2019

mszostok 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

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

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

adamwalach Apr 16, 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

piotrmiskiewicz commented Apr 8, 2019 •

edited

Loading

adamwalach Apr 16, 2019 •

edited

Loading