Add defaulting and validation infrastructure for DomainMappings #9886

julz · 2020-10-21T07:12:29Z

Part I-lost-count of #9713.

This commit adds the basic infrastructure for defaulting and validation and a minimal amount of defaulting/validation to make sure it all works. More validation/defaulting to follow in next PR.

/assign @mattmoor @vagababov

This commit adds the infrastructure for defaulting and validation and a minimal amount of defaulting/validation.

codecov · 2020-10-21T07:17:54Z

Codecov Report

Merging #9886 into master will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #9886      +/-   ##
==========================================
+ Coverage   87.78%   87.83%   +0.04%     
==========================================
  Files         181      183       +2     
  Lines        8591     8607      +16     
==========================================
+ Hits         7542     7560      +18     
+ Misses        800      799       -1     
+ Partials      249      248       -1

Impacted Files	Coverage Δ
pkg/apis/serving/v1alpha1/domainmapping_types.go	`100.00% <ø> (ø)`
...kg/apis/serving/v1alpha1/domainmapping_defaults.go	`100.00% <100.00%> (ø)`
.../apis/serving/v1alpha1/domainmapping_validation.go	`100.00% <100.00%> (ø)`
pkg/reconciler/revision/reconcile_resources.go	`80.72% <0.00%> (-2.41%)`	⬇️
pkg/queue/health/handler.go	`100.00% <0.00%> (ø)`
pkg/autoscaler/statforwarder/forwarder.go	`82.25% <0.00%> (+1.07%)`	⬆️
pkg/reconciler/configuration/configuration.go	`89.34% <0.00%> (+1.63%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 58563ba...822eab5. Read the comment docs.

markusthoemmes · 2020-10-21T07:34:23Z

cmd/domain-mapping-webhook/main.go

@@ -0,0 +1,86 @@
+package main


I wonder if we can fold controller's and webhooks into one pod with the leader-election work being done 🤔. It'd conflate scaling of one with the other, but maybe that's fine for extensions like this?

cc @mattmoor

is there a big advantage to doing that vs. keeping them separate? As you say it seems a shame to lose the ability to scale them independently (ideally the webhook doesn't need to have a cache of every ingress in the system for example, and the failure cases -> probes/PDBs are pretty different), plus when this lands they'll be separate anyway so seems easier to keep them that way and avoid future problems.

The short answer is yes, and it's how I've run mink ~forever for reasons we don't need to dive into here.

The longer answer is that I see this as a downstream distribution problem and I'd like to enable either or, and my bias is generally towards the most flexible (unopinionated) upstream option.

Totally see the point in mink fwiw where the goal is to have one simple thing to deploy, for this it seems a little more flexible to have them separate, especially since that's how this will eventually land anyway when we lose the alpha flag.

Ack, didn't want to block anything.

mattmoor · 2020-10-21T14:29:29Z

/lgtm
/approve

I'd be happy to have the other meta conversation separately, but I don't think we should block this on it. In particular, I wonder about it in the context of things like sample-controller and sample-source, which might loose a small army of these.

knative-prow-robot · 2020-10-21T14:29:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: julz, mattmoor

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~~ [mattmoor]

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

taragu · 2020-10-21T13:48:20Z

pkg/apis/serving/v1alpha1/domainmapping_types.go

@@ -48,6 +48,10 @@ type DomainMapping struct {

 // Verify that DomainMapping adheres to the appropriate interfaces.
 var (
+	// Check that Route may be validated and defaulted.


Possibly a copy and paste error?

Suggested change

// Check that Route may be validated and defaulted.

// Check that DomainMapping may be validated and defaulted.

hah, yeah, think so :) Will fix in next PR tomorrow since Im touching this anyway - thanks!

vagababov

I guess no Conversion webhook?

I have a few readability style nits, but I'll just send a pr with those, since I got late to the party :)

vagababov · 2020-10-21T17:41:39Z

config/domain-mapping/webhooks/validation.yaml

+  labels:
+    serving.knative.dev/release: devel
+webhooks:
+- admissionReviewVersions: ["v1", "v1beta1"]


Wasn't your API Alpha? :)

iiuc these are the versions of the AdmissionReview object we accept, not the version of the validated object we accept, see https://kubernetes.io/docs/reference/access-authn-authz/extensible-admission-controllers/#request

vagababov · 2020-10-21T17:44:08Z

pkg/apis/serving/v1alpha1/domainmapping_validation.go

+		errs = errs.Also(apis.ErrMissingField("name"))
+	}
+
+	if ref.Namespace != "" && ref.Namespace != md.Namespace {


Can ref.Namespace be empty? Given you default it?

mmmm dont know, maybe not :) tbh I just wanted to put some validation here to test stuff out, will swap this out for https://github.com/knative/pkg/blob/master/apis/duck/v1/knative_reference.go#L84 in next PR anyway when I fill in all the actual validation/defaulting :).

no Conversion webhook because v1alpha1 is the only supported version, so I think it doesn't make sense yet to allow any other version?

Add Defaulting and Validation for DomainMappings

5484858

This commit adds the infrastructure for defaulting and validation and a minimal amount of defaulting/validation.

knative-prow-robot assigned mattmoor and vagababov Oct 21, 2020

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. area/API API objects and controllers area/test-and-release It flags unit/e2e/conformance/perf test issues for product features labels Oct 21, 2020

knative-prow-robot requested review from yt3liu and ZhiminXiang October 21, 2020 07:12

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Oct 21, 2020

julz mentioned this pull request Oct 21, 2020

DomainMapping CRD (Tracking Issue) #9713

Closed

14 tasks

markusthoemmes reviewed Oct 21, 2020

View reviewed changes

julz added 3 commits October 21, 2020 10:09

lint and add copyright header

acd27c4

more copyright headers

496d0e7

lint again

822eab5

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 21, 2020

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 21, 2020

knative-prow-robot merged commit b796a84 into knative:master Oct 21, 2020

taragu reviewed Oct 21, 2020

View reviewed changes

vagababov reviewed Oct 21, 2020

View reviewed changes

vagababov mentioned this pull request Oct 21, 2020

Fix post review issues #9901

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add defaulting and validation infrastructure for DomainMappings #9886

Add defaulting and validation infrastructure for DomainMappings #9886

julz commented Oct 21, 2020

codecov bot commented Oct 21, 2020 •

edited

Loading

markusthoemmes Oct 21, 2020

julz Oct 21, 2020 •

edited

Loading

mattmoor Oct 21, 2020

julz Oct 21, 2020

markusthoemmes Oct 21, 2020

mattmoor commented Oct 21, 2020

knative-prow-robot commented Oct 21, 2020

taragu Oct 21, 2020

julz Oct 21, 2020

vagababov left a comment

vagababov Oct 21, 2020

julz Oct 21, 2020

vagababov Oct 21, 2020

julz Oct 21, 2020

julz Oct 21, 2020

	// Check that Route may be validated and defaulted.
	// Check that DomainMapping may be validated and defaulted.

Add defaulting and validation infrastructure for DomainMappings #9886

Add defaulting and validation infrastructure for DomainMappings #9886

Conversation

julz commented Oct 21, 2020

codecov bot commented Oct 21, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

julz Oct 21, 2020 • 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

mattmoor commented Oct 21, 2020

knative-prow-robot commented Oct 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vagababov 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

codecov bot commented Oct 21, 2020 •

edited

Loading

julz Oct 21, 2020 •

edited

Loading