Use pkg/webhook along with build webhook implementation #379

shashwathi · 2018-09-27T19:21:56Z

Address #378

Proposed Changes

Do not use custom build webhook implementation
Implement validate and default methods for all build CRDs and use pkg/wehbook controller.

FYI
I have not added any tests in this PR. If this change does not get 👎 then I will add tests to this PR and also delete custom webhook code.

Goal of this PR is to get conversation started about this change

Lot of validation in build CRDs included calling k8s API to validate object exist like
secrets, build templates. I have not included that change here because I noticed that Serving did not have any similar validations.
My question is does build CRD want that validation still?

Thoughts or concerns @imjasonh @mattmoor @pivotal-nader-ziada ?

knative-prow-robot · 2018-09-27T19:22:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: shashwathi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [shashwathi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

imjasonh

Looking good so far, I think the biggest pain will be moving the full validation into the controller itself.

For now, we might be able to get away with having two webhook admission controllers, this one which does simple checks and uses knative/pkg webhook setup, and our existing one which does more thorough checks and can be phased into the real controller in a future PR.

WDYT?

pkg/apis/build/v1alpha1/build_defaults.go

imjasonh · 2018-09-27T19:46:16Z

pkg/apis/build/v1alpha1/build_template_validation.go

+		return err
+	}
+	return nil
+	// return validatePlaceholders(tmpl.TemplateSpec().Steps)


I think this was erroneously commented out.

It was intentional. validatePlaceholders validates the attached template steps and template is obtained by calling kube api. This type of validation wont be in pkg/webhook. For now lets keep this is build webhook controller itself.

imjasonh · 2018-09-27T19:47:07Z

pkg/apis/build/v1alpha1/build_validation.go

+	}
+	if bs.Template != nil && len(bs.Steps) > 0 {
+		return apis.ErrMissingField("b.spec.template").Also(apis.ErrMissingField("b.spec.steps"))
+		//return validationError("TemplateAndSteps", "build cannot specify both template and steps")


These return validationError lines can just be deleted.

imjasonh · 2018-09-27T19:48:46Z

pkg/apis/build/v1alpha1/build_validation.go

+		return err
+	}
+
+	// Need kubeclient to fetch template


Yeah I think the current thinking is that we'll only do simple validation in the webhook, and leave it up to the controller to do full validation with resolved objects like the template, service account, secrets, etc.

I'm not sure there's a good story yet for surfacing validation errors outside of webhook checks though, we'd probably need to have some new condition type to express the Invalid state.

imjasonh · 2018-09-27T19:49:10Z

pkg/apis/build/v1alpha1/build_validation.go

+
+// ValidateSecrets for build
+func (bs *BuildSpec) ValidateSecrets() *apis.FieldError {
+	// TODO: move this logic into set defaults


+1 good call

shashwathi · 2018-09-28T20:41:57Z

@imjasonh

I like the idea of 2 controllers for now. Updated code to split the validation between the 2 webhooks. Added tests and cleaned up bunch of common dependencies.

shashwathi · 2018-09-28T20:53:22Z

/assign @imjasonh
/assign @pivotal-nader-ziada

mattmoor · 2018-09-30T13:45:13Z

cmd/webhook/main.go

+		Options: pkgoptions,
+		Handlers: map[schema.GroupVersionKind]pkgwebhook.GenericCRD{
+			v1alpha1.SchemeGroupVersion.WithKind("Build"):                &v1alpha1.Build{},
+			v1alpha1.SchemeGroupVersion.WithKind("ClusterBuildTemplate"): &v1alpha1.ClusterBuildTemplate{},


Can you confirm that the common infrastructure works properly with cluster-scoped resources, since AFAIK this is the first one we're trying :)

Cool 👍
I have added e2e test for build with template and build with cluster template as well. Going forward we will have better coverage for these features

mattmoor · 2018-09-30T13:48:15Z

pkg/apis/build/v1alpha1/build_defaults.go

+)
+
+// DefaultTime is 10min
+const DefaultTime = 10 * time.Minute


nit: DefaultTimeout

pkg/apis/build/v1alpha1/build_defaults_test.go

imjasonh · 2018-10-01T17:24:49Z

pkg/apis/build/v1alpha1/build_template_defaults.go

+package v1alpha1
+
+// SetDefaults for build template
+func (b *BuildTemplate) SetDefaults() {


Can we just put this into build_template_types.go ? This separate file doesn't add much.

imjasonh · 2018-10-01T17:26:33Z

pkg/apis/build/v1alpha1/build_template_validation_test.go

+		reason: "long name",
+		tmpl: &BuildTemplate{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: "*********************************************************************************************",


strings.Repeat("x", maxLength+1) is self-documenting and self-updating if we update maxLength.

I missed my TODO there. Good catch

imjasonh · 2018-10-01T17:26:51Z

pkg/apis/build/v1alpha1/build_template_validation.go

+		}
+	}
+
+	if len(name) > 63 {


Can we extract this to a const?

imjasonh · 2018-10-01T17:30:06Z

pkg/apis/build/v1alpha1/build_template_validation.go

+	seen := map[string]struct{}{}
+	for _, p := range params {
+		if _, ok := seen[p.Name]; ok {
+			return apis.ErrMultipleOneOf("ParamName")


I don't think MultipleOneOf is the correct error, I think that error is intended to communicate "you can specify X or Y, but not both"

https://godoc.org/github.com/knative/pkg/apis#ErrMultipleOneOf

(ErrMissingOneOf is "you must specify X or Y, but not neither")

Maybe ErrInvalidKeyName? https://godoc.org/github.com/knative/pkg/apis#ErrInvalidKeyName

Yeah pick the error with closest match I guess. I am okay with ErrInvalidKeyName

imjasonh · 2018-10-01T17:32:12Z

pkg/apis/build/v1alpha1/clutser_build_template_validation_test.go

@@ -0,0 +1,111 @@
+package v1alpha1


typo in filename: cluster_build_template_validation_test.go

imjasonh · 2018-10-01T17:35:21Z

pkg/apis/build/v1alpha1/clutser_build_template_validation_test.go

+		reason string // if "", expect success.
+	}{{
+		desc: "Single named step",
+		tmpl: &ClusterBuildTemplate{


I think we can condense both of these test tables into one, and avoid duplication:

func TestValidateBuildTemplate(t *testing.T) { for _, c := range []struct { desc string spec BuildTemplateSpec reason string } {{ ... }} { t.Run("namespaced-"+c.desc, func(t *testing.T) { tmpl := &BuildTemplate{Spec: c.spec}, tmpl.Validate() ... }) t.Run("cluster-namespaced-"+c.desc, func(t *testing.T) { ctmpl := &ClusterBuildTemplate{Spec: c.spec}, ctmpl.Validate() ... }) } }

knative-metrics-robot · 2018-10-01T19:22:53Z

The following is the coverage report on pkg/.
Say /test pull-knative-build-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/build/v1alpha1/build_defaults.go	Do not exist	100.0%
pkg/apis/build/v1alpha1/build_template_types.go	33.3%	66.7%	33.3
pkg/apis/build/v1alpha1/build_template_validation.go	Do not exist	94.4%
pkg/apis/build/v1alpha1/build_types.go	77.8%	88.9%	11.1
pkg/apis/build/v1alpha1/build_validation.go	Do not exist	96.6%
pkg/apis/build/v1alpha1/cluster_build_template_default.go	Do not exist	0.0%
pkg/apis/build/v1alpha1/cluster_build_template_validation.go	Do not exist	100.0%
pkg/webhook/build.go	89.6%	89.7%	0.1
pkg/webhook/template_common.go	71.4%	100.0%	28.6
pkg/webhook/webhook.go	25.6%	23.4%	-2.2

imjasonh

/lgtm

* Add validation & defaut for btmpl * add validation for cluster build template * Add validation and defaults for build * Use pkg webhook * compiles * Add webhook package * Move tests * Add tests * webhook test * cleanup unused code * Revert previous change * Add e2e tests for build with template and cluster template * Fix typo * Fix test typo * Increase coverage * Address comments * Add tests * Rename file to generic name

Shash Reddy added 4 commits September 27, 2018 09:28

Add validation & defaut for btmpl

97bea78

add validation for cluster build template

2280b2f

Add validation and defaults for build

7b7ecf3

Use pkg webhook

4a462d7

knative-prow-robot added the do-not-merge/work-in-progress label Sep 27, 2018

knative-prow-robot requested review from imjasonh and mattmoor September 27, 2018 19:22

knative-prow-robot added the size/L label Sep 27, 2018

knative-prow-robot added the approved label Sep 27, 2018

shashwathi changed the title ~~[WIP] Use pkg/webhook instead og build webhook iimplmentation~~ [WIP] Use pkg/webhook instead of build webhook implementation Sep 27, 2018

imjasonh reviewed Sep 27, 2018

View reviewed changes

compiles

432b2a4

knative-prow-robot added size/XL and removed size/L labels Sep 28, 2018

Add webhook package

a89dc49

knative-prow-robot added size/XXL and removed size/XL labels Sep 28, 2018

Shash Reddy added 3 commits September 28, 2018 15:14

Move tests

817a39c

Add tests

cba4a11

webhook test

496374f

shashwathi changed the title ~~[WIP] Use pkg/webhook instead of build webhook implementation~~ Use pkg/webhook instead of build webhook implementation Sep 28, 2018

knative-prow-robot removed the do-not-merge/work-in-progress label Sep 28, 2018

Shash Reddy added 2 commits September 28, 2018 16:47

cleanup unused code

2c47a6c

Revert previous change

58c05fe

knative-prow-robot assigned imjasonh and nader-ziada Sep 28, 2018

shashwathi changed the title ~~Use pkg/webhook instead of build webhook implementation~~ Use pkg/webhook along with build webhook implementation Sep 28, 2018

mattmoor reviewed Sep 30, 2018

View reviewed changes

Shash Reddy added 4 commits October 1, 2018 09:35

Add e2e tests for build with template and cluster template

dddca9c

Fix typo

0c21d5d

Fix test typo

e328233

Increase coverage

64f2949

imjasonh reviewed Oct 1, 2018

View reviewed changes

Shash Reddy added 3 commits October 1, 2018 14:25

Address comments

80c7adc

Add tests

d2381d0

Rename file to generic name

864523c

imjasonh approved these changes Oct 1, 2018

View reviewed changes

knative-prow-robot added the lgtm label Oct 1, 2018

knative-prow-robot merged commit 96c3637 into knative:master Oct 1, 2018

shashwathi deleted the wref branch October 1, 2018 20:14

imjasonh mentioned this pull request Oct 2, 2018

Default Build timeout should be set in the webhook #382

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use pkg/webhook along with build webhook implementation #379

Use pkg/webhook along with build webhook implementation #379

shashwathi commented Sep 27, 2018

knative-prow-robot commented Sep 27, 2018

imjasonh left a comment

imjasonh Sep 27, 2018

shashwathi Sep 28, 2018

imjasonh Sep 27, 2018

imjasonh Sep 27, 2018

imjasonh Sep 27, 2018

shashwathi commented Sep 28, 2018

shashwathi commented Sep 28, 2018

mattmoor Sep 30, 2018

shashwathi Oct 1, 2018

mattmoor Sep 30, 2018

imjasonh Oct 1, 2018

imjasonh Oct 1, 2018

shashwathi Oct 1, 2018

imjasonh Oct 1, 2018

imjasonh Oct 1, 2018

shashwathi Oct 1, 2018

imjasonh Oct 1, 2018

imjasonh Oct 1, 2018

knative-metrics-robot commented Oct 1, 2018

imjasonh left a comment

Use pkg/webhook along with build webhook implementation #379

Use pkg/webhook along with build webhook implementation #379

Conversation

shashwathi commented Sep 27, 2018

Proposed Changes

knative-prow-robot commented Sep 27, 2018

imjasonh 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

shashwathi commented Sep 28, 2018

shashwathi commented Sep 28, 2018

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

knative-metrics-robot commented Oct 1, 2018

imjasonh left a comment

Choose a reason for hiding this comment