Enabling Validating and Mutating Webhook's for KCP#818
Merged
Conversation
sttts
reviewed
Apr 4, 2022
sttts
reviewed
Apr 4, 2022
sttts
reviewed
Apr 4, 2022
sttts
reviewed
Apr 4, 2022
sttts
reviewed
Apr 4, 2022
sttts
reviewed
Apr 4, 2022
sttts
reviewed
Apr 4, 2022
shawn-hurley
force-pushed
the
add-conformance-webhook-test
branch
2 times, most recently
from
b0b3fc0 to
16b708f
Compare
shawn-hurley
changed the title
ncdc
reviewed
Apr 6, 2022
shawn-hurley
force-pushed
the
add-conformance-webhook-test
branch
3 times, most recently
from
e139ced to
9fe6e2d
Compare
shawn-hurley
force-pushed
the
add-conformance-webhook-test
branch
from
9fe6e2d to
0a26674
Compare
sttts
reviewed
Apr 20, 2022
sttts
reviewed
Apr 20, 2022
sttts
reviewed
Apr 20, 2022
| // Determine the type of request, is it api binding or not. | ||
| if workspace, isAPIBinding := p.getAPIBindingWorkspace(attr, lcluster); isAPIBinding { | ||
| wh := p.restrictToLogicalCluster(hooks, workspace) | ||
| whAccessor = append(whAccessor, wh...) |
Member
There was a problem hiding this comment.
I think we want to replace the hooks with wh, not just add them, at least that's true for status updates (= status subresource). cc @ncdc
Author
There was a problem hiding this comment.
a cluster-admin couldn't have a webhook for a bound resource in their cluster then? at least that was the use case that I was thinking of
Member
There was a problem hiding this comment.
I don't think this is a use case we'd want to enable, at least not without further discussion (i.e. let's get this PR ready + merged without this, and consider it for the future).
In the typical (non admin) use case, an API provider provides APIs, and that includes validating + mutating webhooks. API consumers must not be able to change validating/mutating behaviors, or at least I don't think they should be able to (?). Allowing cluster-admins to do so would make things much harder for the API provider to debug when things go wrong.
shawn-hurley
force-pushed
the
add-conformance-webhook-test
branch
2 times, most recently
from
e280866 to
4d14315
Compare
shawn-hurley
changed the title
shawn-hurley
force-pushed
the
add-conformance-webhook-test
branch
4 times, most recently
from
9f138df to
e0c9fca
Compare
sttts
reviewed
Apr 27, 2022
sttts
reviewed
Apr 27, 2022
shawn-hurley
commented
Apr 27, 2022
shawn-hurley
commented
Apr 27, 2022
shawn-hurley
force-pushed
the
add-conformance-webhook-test
branch
3 times, most recently
from
66404b9 to
0414709
Compare
sttts
reviewed
Apr 27, 2022
sttts
reviewed
Apr 27, 2022
sttts
reviewed
Apr 29, 2022
| }, wait.ForeverTestTimeout, 100*time.Millisecond) | ||
|
|
||
| //Avoid race condition where webhook informer is not updated before the call to create was made. | ||
| t.Log("Verify webhook is eventually called") |
Member
There was a problem hiding this comment.
here we also have the race about the two eventually calls.
Member
|
For this to land, also for @ncdc to pick this up in review later today:
|
shawn-hurley
force-pushed
the
add-conformance-webhook-test
branch
from
2c034f2 to
4b9439f
Compare
…ng webhooks * adding a test case for validating webhook * add a generic kcp webhook dispatcher for validationg and mutating webhooks. * Adding e2e tests for new plugins * admission/webhooks: add index by workspace for bindings * Adding indexer to the generic webhook struct
shawn-hurley
force-pushed
the
add-conformance-webhook-test
branch
from
4b9439f to
dfee47b
Compare
Author
sttts
reviewed
Apr 29, 2022
shawn-hurley
force-pushed
the
add-conformance-webhook-test
branch
from
dfee47b to
bea27fc
Compare
sttts
approved these changes
Apr 30, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adding a conformance test to prove kcp-dev/kubernetes#52 is working as expected
Another follow on PR is going to need to test this using the APIBindings type to pass webhooks along.
Related issue(s)
Fixes #