Validation Webhook: Auto-Accept Status Updates #7010

sam-heilbron · 2022-08-23T04:23:07Z

Description

Auto-accept status updates, instead of requiring a full translation run

Context

The validation webhook executes a full translation run to determine whether a proposed change is valid. Status only changes will not affect the validity of the proposed change, and therefore it is both unnecessary and expensive to run translation for status only changes.

Checklist:

I included a concise, user-facing changelog (for details, see https://github.com/solo-io/go-utils/tree/master/changelogutils) which references the issue that is resolved.
If I updated APIs (our protos) or helm values, I ran make -B install-go-tools generated-code to ensure there will be no code diff
I followed guidelines laid out in the Gloo Edge contribution guide
I opened a draft PR or added the work in progress label if my PR is not ready for review
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works

…gloo into vwh/accept-status-updates

solo-changelog-bot · 2022-08-23T12:03:56Z

Issues linked to changelog:
#6990

EItanya · 2022-08-23T12:06:45Z

projects/gateway/pkg/services/k8sadmission/validating_admission_webhook.go

+		return false, &multierror.Error{Errors: []error{WrappedUnmarshalErr(err)}}
+	}
+
+	specsAreEqual := reflect.DeepEqual(newResource.Spec, oldResource.Spec)


proto.Equal?

Also, what about metadata, do we not care about label changes, etc.?

Yeah I was thinking about this and erred on the side of "if it's not a spec change, allow it". One example of a problematic change is:
You could change the label of a VirtualService, which is selected by a Gateway, and that would mean that the Gateway no longer selects any VS's and should reject the change.

You could change the label of a VirtualService, which is selected by a Gateway, and that would mean that the Gateway no longer selects any VS's and should reject the change.

wouldn't this be a reason for not skipping validation when there's a metadata-only change?

Yup! Just pushing up an update

projects/gateway/pkg/services/k8sadmission/validating_admission_webhook.go

github-actions · 2022-08-23T12:29:24Z

Visit the preview URL for this PR (updated for commit 40fe50c):

https://gloo-edge--pr7010-vwh-accept-status-up-44jybh9x.web.app

_{(expires Tue, 30 Aug 2022 18:52:06 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

projects/gateway/pkg/services/k8sadmission/validating_admission_webhook.go

jenshu · 2022-08-23T13:45:26Z

projects/gateway/pkg/services/k8sadmission/validating_admission_webhook.go

+
+	specsAreEqual := reflect.DeepEqual(newResource, oldResource)
+	if specsAreEqual {
+		logger.Debugf("Skipping validatation. Reason: status only update")


add resource name/namespace/kind here too?

jenshu · 2022-08-23T13:46:00Z

projects/gateway/pkg/services/k8sadmission/validating_admission_webhook.go

+
+	specsAreEqual := reflect.DeepEqual(newResource, oldResource)
+	if specsAreEqual {
+		logger.Debugf("Skipping validatation. Reason: status only update")


Suggested change

logger.Debugf("Skipping validatation. Reason: status only update")

logger.Debugf("Skipping validation. Reason: status only update")

jenshu · 2022-08-23T13:58:57Z

projects/gateway/pkg/services/k8sadmission/validating_admission_webhook.go

+		return false, &multierror.Error{Errors: []error{WrappedUnmarshalErr(err)}}
+	}
+
+	// A status-only update will only differ by status and generation, so we nil both fields before comparing


does resourceVersion not change on status updates?

Yeah good call. I tried adding a kube2e test, but it's tricky to test, given that we still update generation and other metadata fields, so its hard to identify if the auto-accept of status does anything. Ill keep working on one siince I think it will be necessary to prove out/ensure we don't have a regression.

…gloo into vwh/accept-status-updates

kdorosh · 2022-08-23T14:35:34Z

projects/gateway/pkg/services/k8sadmission/validating_admission_webhook.go

+	// For update requests, we check to see if this is a status update
+	// If it is, we do not need to validate the resource
+	var newResource, oldResource crdv1.Resource
+	if err := json.Unmarshal(admissionRequest.Object.Raw, &newResource); err != nil {
+		return false, &multierror.Error{Errors: []error{WrappedUnmarshalErr(err)}}
+	}
+	if err := json.Unmarshal(admissionRequest.OldObject.Raw, &oldResource); err != nil {
+		return false, &multierror.Error{Errors: []error{WrappedUnmarshalErr(err)}}
+	}


rather than do it this way... should we hash it instead and compare hashes? that is what hashing was intended for

i see we hash the spec, but i mean hash the whole resource with our generated hash functions.. just like our emitters

this should ignore the status but also ignore any fields on the spec we want opted out here as well (which could cause similar issues)

If we do that and there are new fields on the CRD that our current version of gloo doesnt care about then it will still generate validations.

but hashing is faster agreed

Yeah I think that's cleaner. I think I'll need to pass in the typed resource into the function for the 'oldResource'. I'll update!

sam-heilbron · 2022-08-23T15:55:21Z

/kick https://storage.googleapis.com/solo-public-build-logs/logs.html?buildid=f78e19e9-dcd9-4065-9a5b-d14f4bff3bf2 consul works as expected

projects/gateway/pkg/services/k8sadmission/validating_admission_webhook.go

EItanya

These changes make sense to me!

jenshu · 2022-08-23T18:15:09Z

LGTM as well

sam-heilbron and others added 2 commits August 23, 2022 00:22

skip status only changes

34bd2cc

Merge branch 'master' into vwh/accept-status-updates

8bb817e

github-actions bot added the keep pr updated signals bulldozer to keep pr up to date with base branch label Aug 23, 2022

add changelog

fd8c123

sam-heilbron marked this pull request as ready for review August 23, 2022 12:03

Merge branch 'vwh/accept-status-updates' of ssh:https://github.com/solo-io/…

2c8c63f

…gloo into vwh/accept-status-updates

bad log

5cb41b5

EItanya reviewed Aug 23, 2022

View reviewed changes

nfuden reviewed Aug 23, 2022

View reviewed changes

projects/gateway/pkg/services/k8sadmission/validating_admission_webhook.go Show resolved Hide resolved

nfuden reviewed Aug 23, 2022

View reviewed changes

projects/gateway/pkg/services/k8sadmission/validating_admission_webhook.go Outdated Show resolved Hide resolved

sam-heilbron added 2 commits August 23, 2022 08:35

handle metadata changes

01c9e3f

add tests for metadata only changes

7eedef8

sam-heilbron requested review from EItanya, jenshu and nfuden August 23, 2022 13:00

Merge refs/heads/master into vwh/accept-status-updates

b2ffa0a

jenshu reviewed Aug 23, 2022

View reviewed changes

nfuden and others added 3 commits August 23, 2022 10:08

k8sadmission: Update to have logger with all useful fields (#7013)

041147f

handle metadata changes

c39617a

Merge branch 'vwh/accept-status-updates' of ssh:https://github.com/solo-io/…

f5b2849

…gloo into vwh/accept-status-updates

sam-heilbron requested a review from jenshu August 23, 2022 14:29

kdorosh reviewed Aug 23, 2022

View reviewed changes

simplify equality check

7933115

sam-heilbron requested a review from kdorosh August 23, 2022 15:10

Simplify tests

947f112

jenshu reviewed Aug 23, 2022

View reviewed changes

projects/gateway/pkg/services/k8sadmission/validating_admission_webhook.go Show resolved Hide resolved

jenshu reviewed Aug 23, 2022

View reviewed changes

projects/gateway/pkg/services/k8sadmission/validating_admission_webhook.go Show resolved Hide resolved

EItanya reviewed Aug 23, 2022

View reviewed changes

nfuden previously approved these changes Aug 23, 2022

View reviewed changes

add readme to webhook

40fe50c

sam-heilbron dismissed nfuden’s stale review via 40fe50c August 23, 2022 18:27

sam-heilbron requested a review from nfuden August 23, 2022 18:27

nfuden approved these changes Aug 23, 2022

View reviewed changes

sam-heilbron requested a review from ashishb-solo August 23, 2022 18:37

sam-heilbron mentioned this pull request Aug 23, 2022

[1.12] Validation Webhook: Auto-Accept Status Updates #7016

Merged

8 tasks

soloio-bulldozer bot merged commit 7fff0fc into master Aug 23, 2022

soloio-bulldozer bot deleted the vwh/accept-status-updates branch August 23, 2022 19:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validation Webhook: Auto-Accept Status Updates #7010

Validation Webhook: Auto-Accept Status Updates #7010

sam-heilbron commented Aug 23, 2022 •

edited

Loading

solo-changelog-bot bot commented Aug 23, 2022

EItanya Aug 23, 2022

EItanya Aug 23, 2022

sam-heilbron Aug 23, 2022

jenshu Aug 23, 2022

sam-heilbron Aug 23, 2022

github-actions bot commented Aug 23, 2022 •

edited

Loading

jenshu Aug 23, 2022

jenshu Aug 23, 2022

jenshu Aug 23, 2022

sam-heilbron Aug 23, 2022

kdorosh Aug 23, 2022

kdorosh Aug 23, 2022

kdorosh Aug 23, 2022

nfuden Aug 23, 2022

nfuden Aug 23, 2022

sam-heilbron Aug 23, 2022

sam-heilbron commented Aug 23, 2022

EItanya left a comment

jenshu commented Aug 23, 2022

	logger.Debugf("Skipping validatation. Reason: status only update")
	logger.Debugf("Skipping validation. Reason: status only update")

Validation Webhook: Auto-Accept Status Updates #7010

Validation Webhook: Auto-Accept Status Updates #7010

Conversation

sam-heilbron commented Aug 23, 2022 • edited Loading

Description

Context

Checklist:

solo-changelog-bot bot commented Aug 23, 2022

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

github-actions bot commented Aug 23, 2022 • 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

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

sam-heilbron commented Aug 23, 2022

EItanya left a comment

Choose a reason for hiding this comment

jenshu commented Aug 23, 2022

sam-heilbron commented Aug 23, 2022 •

edited

Loading

github-actions bot commented Aug 23, 2022 •

edited

Loading