Validate autoscaling annotations on Services and Configs. #9358

markusthoemmes · 2020-09-10T13:51:31Z

Proposed Changes

This validates that no autoscaling annotations are put onto top-level metadata in Services and Configurations. Further, it adds advice into the error message to guide users to put the annotations into the right spot.

Despite explicitly documenting this, we're still seeing users continuously struggling here. This is an attempt to frontload the error determination.

Happy to bin this if i'm completely off, haven't discussed this with anybody before cooking it up 😅

Release Note

The absence of autoscaling annotations in both Service and Configuration's top-level metadata is now validated. This gives users an actionable error message but might potentially cause old (faulty) YAML to start to fail to be accepted now.

/assign @mattmoor @dprotaso

knative-prow-robot

@markusthoemmes: 0 warnings.

In response to this:

Proposed Changes

This validates that no autoscaling annotations are put onto top-level metadata in Services and Configurations. Further, it adds advice into the error message to guide users to put the annotations into the right spot.

Despite explicitly documenting this, we're still seeing users continuously struggling here. This is an attempt to frontload the error determination.

Happy to bin this if i'm completely off, haven't discussed this with anybody before cooking it up 😅

Release Note
NONE
/assign @mattmoor @dprotaso

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

knative-prow-robot · 2020-09-10T13:51:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: markusthoemmes

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:

~~pkg/apis/OWNERS~~ [markusthoemmes]

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

pkg/apis/serving/metadata_validation.go

vagababov · 2020-09-10T14:19:17Z

pkg/apis/serving/metadata_validation.go

+func ValidateHasNoAutoscalingAnnotation(annotations map[string]string) *apis.FieldError {
+	for key := range annotations {
+		if strings.HasPrefix(key, autoscaling.GroupName) {
+			return apis.ErrGeneric(noAutoscalingMsg, apis.CurrentField)


I wish we had warnings -- it's not really an error to set them here, it's just they are useless.

pkg/apis/serving/metadata_validation.go

markusthoemmes · 2020-09-10T14:47:34Z

Reworked, error message now looks like this:

Error from server (BadRequest): error when creating "STDIN": admission webhook "validation.webhook.serving.knative.dev" denied the request: validation failed: invalid key name "autoscaling.knative.dev/class": metadata.annotations
autoscaling annotations must be put under "spec.template.metadata.annotations" to work

vagababov · 2020-09-10T14:48:37Z

pkg/apis/serving/metadata_validation.go

 	for key := range annotations {
 		if strings.HasPrefix(key, autoscaling.GroupName) {
-			return apis.ErrGeneric(noAutoscalingMsg, apis.CurrentField)
+			return apis.ErrInvalidKeyName(key, apis.CurrentField, `autoscaling annotations must be put under "spec.template.metadata.annotations" to work`)


well if we're not going to aggregate them with Also, then errs is not required.

missed that, sorry 😂

vagababov

/lgtm
/hold
I'll defer to Matt for final lgtm.

vagababov · 2020-09-10T15:00:22Z

pkg/apis/serving/metadata_validation_test.go

+		name:       "no offender",
+		annotation: map[string]string{"foo": "bar"},
+	}, {
+		name:       "only offender",


several bad annotations case?

knative-metrics-robot · 2020-09-10T15:03:07Z

The following is the coverage report on the affected files.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/serving/metadata_validation.go	98.4%	98.5%	0.1

julz

Probably should add something to the release note section in the description for this one to make super sure we remember to add a warning, since existing valid yaml may become invalid (eg if you put annotations in both places) after this

markusthoemmes · 2020-09-14T14:51:13Z

@julz hmm, fair point. We might want to only apply this on create for that reason 🤔

vagababov · 2020-09-14T16:18:02Z

Yeah, it's easy to check if we're in update or not for a few versions.

vagababov · 2020-09-14T16:18:12Z

And probably log warning in case of update.

julz · 2020-09-14T16:42:52Z

I think this may already only apply on update due to the if !apis.IsInStatusUpdate(ctx) { (?), but it still (imo) warrants a release note -- even if it only applies on create, it means existing local yaml files that worked before (e.g. sample apps, gitops configs) may cause an error when applied to a new cluster.

markusthoemmes · 2020-09-16T10:36:01Z

@julz added a release note.

julz · 2020-09-16T10:45:02Z

/lgtm

mattmoor

Should we also do Route?

Otherwise LGTM

markusthoemmes · 2020-09-16T14:24:09Z

I'm not as concerned about route as I've never seen a user try to add autoscaling annotations there 😂

mattmoor · 2020-09-16T15:07:45Z

/lgtm
/unhold

dprotaso · 2020-09-16T21:09:42Z

/retest

mattmoor · 2020-09-16T22:01:42Z

/retest

knative-test-reporter-robot · 2020-09-16T22:22:08Z

The following jobs failed:

Test name	Triggers	Retries
pull-knative-serving-integration-tests	2020-09-16 17:57:34.987 +0000 UTC 2020-09-16 18:21:34.946 +0000 UTC 2020-09-16 18:41:05.12 +0000 UTC 2020-09-16 19:08:05.097 +0000 UTC 2020-09-16 21:57:35.25 +0000 UTC 2020-09-16 22:22:05.256 +0000 UTC	3/3
pull-knative-serving-upgrade-tests	2020-09-16 17:47:35.171 +0000 UTC 2020-09-16 17:59:35.086 +0000 UTC 2020-09-16 18:11:35.222 +0000 UTC 2020-09-16 18:34:05.13 +0000 UTC	3/3

Job pull-knative-serving-integration-tests expended all 3 retries without success.

dprotaso · 2020-09-17T00:26:10Z

Is installing istio broken?
/tmp/tmp.qT9nj7tHbj/third_party/istio-stable/install-istio.sh: line 20: return: can only return' from a function or sourced script`

markusthoemmes · 2020-09-17T14:32:13Z

/retest

Validate autoscaling annotations on Services and Configs.

4ea3e29

knative-prow-robot assigned dprotaso and mattmoor Sep 10, 2020

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Sep 10, 2020

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/API API objects and controllers labels Sep 10, 2020

knative-prow-robot reviewed Sep 10, 2020

View reviewed changes

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

knative-prow-robot requested review from evankanderson and vaikas September 10, 2020 13:51

vagababov reviewed Sep 10, 2020

View reviewed changes

Rework error messages.

fc1af86

vagababov reviewed Sep 10, 2020

View reviewed changes

Aggregate all offending values.

3583ed3

vagababov reviewed Sep 10, 2020

View reviewed changes

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 10, 2020

knative-prow-robot assigned vagababov Sep 10, 2020

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

julz reviewed Sep 10, 2020

View reviewed changes

knative-prow-robot assigned julz Sep 16, 2020

mattmoor reviewed Sep 16, 2020

View reviewed changes

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 16, 2020

knative-prow-robot merged commit cadb89f into knative:master Sep 17, 2020

navidshaikh mentioned this pull request Sep 18, 2020

autoscaling annotations must be put under "spec.template.metadata.annotations" to work knative/client#1020

Closed

Validate autoscaling annotations on Services and Configs. #9358

Validate autoscaling annotations on Services and Configs. #9358

Uh oh!

Conversation

markusthoemmes commented Sep 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

Uh oh!

knative-prow-robot commented Sep 10, 2020

Uh oh!

Uh oh!

vagababov Sep 10, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

markusthoemmes commented Sep 10, 2020

Uh oh!

vagababov Sep 10, 2020

Choose a reason for hiding this comment

Uh oh!

markusthoemmes Sep 10, 2020

Choose a reason for hiding this comment

Uh oh!

vagababov left a comment

Choose a reason for hiding this comment

Uh oh!

vagababov Sep 10, 2020

Choose a reason for hiding this comment

Uh oh!

knative-metrics-robot commented Sep 10, 2020

Uh oh!

julz left a comment

Choose a reason for hiding this comment

Uh oh!

markusthoemmes commented Sep 14, 2020

Uh oh!

vagababov commented Sep 14, 2020

Uh oh!

vagababov commented Sep 14, 2020

Uh oh!

julz commented Sep 14, 2020

Uh oh!

markusthoemmes commented Sep 16, 2020

Uh oh!

julz commented Sep 16, 2020

Uh oh!

mattmoor left a comment

Choose a reason for hiding this comment

Uh oh!

markusthoemmes commented Sep 16, 2020

Uh oh!

mattmoor commented Sep 16, 2020

Uh oh!

dprotaso commented Sep 16, 2020

Uh oh!

mattmoor commented Sep 16, 2020

Uh oh!

knative-test-reporter-robot commented Sep 16, 2020

Uh oh!

dprotaso commented Sep 17, 2020

Uh oh!

markusthoemmes commented Sep 17, 2020

Uh oh!

Uh oh!

markusthoemmes commented Sep 10, 2020 •

edited

Loading