Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Split apart defaulting and validation webhooks #5947

Merged
merged 1 commit into from
Nov 15, 2019
Merged

Split apart defaulting and validation webhooks #5947

merged 1 commit into from
Nov 15, 2019

Conversation

mattmoor
Copy link
Member

@mattmoor mattmoor commented Nov 5, 2019

Related: knative/pkg#848

/hold
until upstream PR lands.

@knative-prow-robot knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 5, 2019
@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Nov 5, 2019
@knative-prow-robot knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 5, 2019
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

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

@knative-prow-robot knative-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. area/API API objects and controllers labels Nov 5, 2019
@mattmoor
Copy link
Member Author

mattmoor commented Nov 6, 2019

/retest

@mattmoor mattmoor force-pushed the split-webhook branch 2 times, most recently from 5822768 to 87f8388 Compare November 6, 2019 02:51
vagababov
vagababov reviewed Nov 6, 2019
View reviewed changes
Copy link
Contributor

@vagababov vagababov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 6, 2019
@vagababov
Copy link
Contributor

I guess something stops working on downgrade...

@mattmoor
Copy link
Member Author

mattmoor commented Nov 9, 2019

Good catch, I forgot downgrade was back. Yeah, so downgrade is a problem without --prune because the webhook in 0.10 won't handle the registered path and we don't remove the webhook...

@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 9, 2019
mattmoor-sockpuppet
mattmoor-sockpuppet reviewed Nov 9, 2019
View reviewed changes
Copy link
Contributor

@mattmoor-sockpuppet mattmoor-sockpuppet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found go linting violations, please merge: mattmoor#106

@knative-test-reporter-robot
Copy link

The following jobs failed:

Test name Triggers Retries
pull-knative-serving-unit-tests 0/3

Failed non-flaky tests preventing automatic retry of pull-knative-serving-unit-tests:

pkg/activator/net.TestThrottlerWithError
pkg/activator/net.TestThrottlerWithError/both_requests_time_out

@mattmoor
Copy link
Member Author

mattmoor commented Nov 9, 2019

/retest

@knative-prow-robot knative-prow-robot added the area/test-and-release It flags unit/e2e/conformance/perf test issues for product features label Nov 9, 2019
mattmoor-sockpuppet
mattmoor-sockpuppet reviewed Nov 9, 2019
View reviewed changes
Copy link
Contributor

@mattmoor-sockpuppet mattmoor-sockpuppet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found go linting violations, please merge: mattmoor#107

@mattmoor
Copy link
Member Author

/retest

@mattmoor
Copy link
Member Author

Ok, I had a chance to debug what's happening here, but I haven't figured out how to fix this yet.

tl;dr Unfortunately for the downgrade test, our new reconciler-based webhook logic is far too effective.

As the downgraded webhook comes up it updates the validating webhook to use /. The validating webhook is left around, but as I have things configured it hits the same path as we used to serve the combined webhook on, so leaving it around should be safe (though folks should use a mechanism like --prune to clean this up).

If this were all, then things would be peachy, however, before we have a chance to shutdown the HEAD webhook it has already observed the change to its mutating webhook and reconciled it back to the path we use at HEAD (in this PR) which is /defaulting, which is a bad path on the old webhook service.

If you wait 10 minutes (🙄) the downgrade will fix itself, since that's the interval at which it ensures the webhook is registered, but by then the downgrade test (and probe) have long since failed.

If we simply wait to land this change until after the 0.11 cut, then it will effectively fix itself (for single-version downgrade), but I'd still like to see whether we can land at least parts of these changes.

cc @vagababov @dgerd

@mattmoor
Copy link
Member Author

I think I have a relatively surgical tweak to get most of this merged. I'll see how gross folks (and myself) find it when I've put some lipstick on it. 🤞

@knative knative deleted a comment from mattmoor-sockpuppet Nov 14, 2019
@mattmoor
Copy link
Member Author

/retest

@mattmoor mattmoor mentioned this pull request Nov 15, 2019
Closed
n3wscott
n3wscott reviewed Nov 15, 2019
View reviewed changes
cmd/webhook/main.go
@@ -136,7 +163,8 @@ func main() {

sharedmain.MainWithContext(ctx, "webhook",
certificates.NewController,
NewResourceAdmissionController,
NewDefaultingAdmissionController,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SO AWESOME

@n3wscott
Copy link
Contributor

/lgtm

Thanks for finding that issue with the webhooks and fixing it.

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 15, 2019
@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 15, 2019
@n3wscott
Copy link
Contributor

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 15, 2019
@mattmoor
Copy link
Member Author

/hold cancel

@knative-prow-robot knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 15, 2019
@mattmoor mattmoor mentioned this pull request Nov 15, 2019
Closed
@knative-prow-robot knative-prow-robot merged commit 916ab9f into knative:master Nov 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattmoor-sockpuppet mattmoor-sockpuppet mattmoor-sockpuppet left review comments

@n3wscott n3wscott n3wscott left review comments

@vagababov vagababov vagababov left review comments

@tcnghia tcnghia Awaiting requested review from tcnghia

Assignees

@vagababov vagababov

@n3wscott n3wscott

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/API API objects and controllers area/test-and-release It flags unit/e2e/conformance/perf test issues for product features cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@mattmoor @knative-prow-robot @vagababov @knative-test-reporter-robot @n3wscott @mattmoor-sockpuppet @googlebot