Add validation of managed labels/annotations #134

erikgb · 2022-01-27T17:24:15Z

Closes #128. Part of #47.

This adds webhook validation of managed labels and annotations.

Tested: Added unit tests for validations. Ran e2e-tests, but no additions/modifications in this PR.

k8s-ci-robot · 2022-01-27T17:24:23Z

Hi @erikgb. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

adrianludwin

I'll only be able to look at this tomorrow.

Ryan, if you have a few minutes to check this out and like it, feel free to lgtm/approve without me! Otherwise I'll get it tomorrow.

/cc @rjbez17

erikgb · 2022-01-27T17:49:36Z

FYI: I removed some refactoring/cleanup that can be done in a separate PR. Ready for review.

internal/hierarchyconfig/validator_test.go

internal/hierarchyconfig/validator.go

adrianludwin

I think this is sufficient testing. We generally don't have e2e tests (which is what I think you mean by "integration tests"?) for webhook validation except to make sure that it's set up at all. Unit tests are more than enough to test whether the logic is correct.

Otherwise, this lgtm apart from some small requests. Thanks!

internal/hierarchyconfig/validator.go

internal/hierarchyconfig/validator_test.go

adrianludwin

Sorry four last nits! LMK if you disagree with them.

adrianludwin · 2022-01-28T15:43:06Z

internal/hierarchyconfig/validator.go

+	allErrs := validateManagedMeta(req.hc)
+	if len(allErrs) > 0 {
+		gk := schema.GroupKind{Group: api.GroupVersion.Group, Kind: "HierarchyConfiguration"}
+		err := apierrors.NewInvalid(gk, req.hc.Name, allErrs)
+		return webhooks.DenyFromAPIError(err)
+	}
+


Can we shorten this even more? I like the lower-level function to encapsulate as much information as possible so that the top-level function is easier to read, e.g:

if denied := validateManagedMeta(req.hc); denied != nil { return denied }

Now the caller (handle()) doesn't have to know anything about the type of information coming out of validateManagedMeta other than that it's ready to be returned to its caller (Handle()).

There reason I left the allErrs there, is to make it more obvious how to include more field errors in the same response. And to avoid more refactoring if/when more fields are added to HierarchyConfiguration that require validation in this webhook.

If we were using the high-level webhook framework provided by controller-runtime, Error/field.ErrorList is the "contract" between your webhook implementation and controller-runtime. That is another reason for keeping it as suggested IMO.

So here I do not agree with you, and would like to keep it as is. But if you insist..... 😉

Interesting. Let's leave it as-is for now, but generally I prefer to keep the current state of the code as readable as possible, and save the flexibility until we actually use them in the PRs. But especially if you'll be adding that shortly, it's fine to go in like this.

internal/hierarchyconfig/validator_test.go

adrianludwin · 2022-01-28T15:48:21Z

internal/hierarchyconfig/validator_test.go

+	defer func() {
+		_ = config.SetManagedMeta(nil, nil)
+	}()


Why not defer config.SetManagedMeta(nil, nil)? Is the anonymous function adding anything?

Just my IDE complaining about unhandled error, but in this case it's "wrong". 😅 Fixed.

internal/hierarchyconfig/validator_test.go

Part of kubernetes-retired#47. This adds validation of managed labels and annotations. Tested: Added unit tests for validations. Ran e2e-tests, but no additions/modifications to those in this PR.

adrianludwin

/lgtm
/approve

adrianludwin · 2022-01-28T16:23:40Z

internal/hierarchyconfig/validator.go

+	allErrs := validateManagedMeta(req.hc)
+	if len(allErrs) > 0 {
+		gk := schema.GroupKind{Group: api.GroupVersion.Group, Kind: "HierarchyConfiguration"}
+		err := apierrors.NewInvalid(gk, req.hc.Name, allErrs)
+		return webhooks.DenyFromAPIError(err)
+	}
+


Interesting. Let's leave it as-is for now, but generally I prefer to keep the current state of the code as readable as possible, and save the flexibility until we actually use them in the PRs. But especially if you'll be adding that shortly, it's fine to go in like this.

adrianludwin · 2022-01-28T16:24:33Z

/ok-to-test

k8s-ci-robot · 2022-01-28T16:24:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adrianludwin, erikgb

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

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

k8s-ci-robot requested a review from adrianludwin January 27, 2022 17:24

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 27, 2022

erikgb force-pushed the feat/label-annotation-validation branch from b9c5dcf to 4f5145d Compare January 27, 2022 17:26

k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Jan 27, 2022

k8s-ci-robot requested a review from rjbez17 January 27, 2022 17:32

adrianludwin reviewed Jan 27, 2022

View reviewed changes

erikgb force-pushed the feat/label-annotation-validation branch from 4f5145d to 31d8df5 Compare January 27, 2022 17:48

erikgb commented Jan 27, 2022

View reviewed changes

internal/hierarchyconfig/validator_test.go Outdated Show resolved Hide resolved

erikgb commented Jan 27, 2022

View reviewed changes

internal/hierarchyconfig/validator.go Outdated Show resolved Hide resolved

adrianludwin reviewed Jan 28, 2022

View reviewed changes

erikgb mentioned this pull request Jan 28, 2022

Should have more user-friendly options to set managed labels/annotations #135

Closed

erikgb force-pushed the feat/label-annotation-validation branch 3 times, most recently from 1b71be0 to 3e31634 Compare January 28, 2022 15:36

adrianludwin reviewed Jan 28, 2022

View reviewed changes

Add validation of managed labels/annotations

7db7393

Part of kubernetes-retired#47. This adds validation of managed labels and annotations. Tested: Added unit tests for validations. Ran e2e-tests, but no additions/modifications to those in this PR.

erikgb force-pushed the feat/label-annotation-validation branch from 3e31634 to 7db7393 Compare January 28, 2022 16:18

adrianludwin reviewed Jan 28, 2022

View reviewed changes

k8s-ci-robot assigned adrianludwin Jan 28, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 28, 2022

k8s-ci-robot merged commit 22c4260 into kubernetes-retired:master Jan 28, 2022

adrianludwin added this to the release-v1.0 milestone Mar 29, 2022

Add validation of managed labels/annotations #134

Add validation of managed labels/annotations #134

Uh oh!

Conversation

erikgb commented Jan 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Jan 27, 2022

Uh oh!

adrianludwin left a comment

Choose a reason for hiding this comment

Uh oh!

erikgb commented Jan 27, 2022

Uh oh!

Uh oh!

Uh oh!

adrianludwin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adrianludwin left a comment

Choose a reason for hiding this comment

Uh oh!

adrianludwin Jan 28, 2022

Choose a reason for hiding this comment

Uh oh!

erikgb Jan 28, 2022

Choose a reason for hiding this comment

Uh oh!

adrianludwin Jan 28, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adrianludwin Jan 28, 2022

Choose a reason for hiding this comment

Uh oh!

erikgb Jan 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adrianludwin left a comment

Choose a reason for hiding this comment

Uh oh!

adrianludwin Jan 28, 2022

Choose a reason for hiding this comment

Uh oh!

adrianludwin commented Jan 28, 2022

Uh oh!

k8s-ci-robot commented Jan 28, 2022

Uh oh!

Uh oh!

erikgb commented Jan 27, 2022 •

edited

Loading

erikgb Jan 28, 2022 •

edited

Loading