WIP: just one option exploration for fixing #2115

[vaikas]

Signed-off-by: Ville Aikas [email protected]

Changes

Currently when we create the validatingwebhookconfiguration, there's couple of wonky things with it:

1. Always includes status as a subresource and there are cases where we do not need it (#2115 )
2. Always includes all the verbs in the configuration even though they will later on get filtered, but the call to webhook is still made.

Also the API is widely used so I wanted to investigate what could be a smallest change using what we currently have to make this configurable.

So, the idea is that we have a way to customize additional validation logic via Callbacks, so if we utilize that struct we do not have to change the signature, but can piggyback off of it to provide ways to customize the actual webhook configuration without breaking any existing code.

Anyways, wanted to just put the smallest amount of code to see what folks think. obvs, need to write tests, etc. but just wanted to see what folks think before going too much deeper.

/kind

Fixes #

Release Note

Docs

[knative-prow[bot]]

@vaikas: The label(s) kind/<kind> cannot be applied, because the repository doesn't have them.

In response to this:

Signed-off-by: Ville Aikas [email protected]

Changes

Currently when we create the validatingwebhookconfiguration, there's couple of wonky things with it:

1. Always includes status as a subresource and there are cases where we do not need it (#2115 )
2. Always includes all the verbs in the configuration even though they will later on get filtered, but the call to webhook is still made.

Also the API is widely used so I wanted to investigate what could be a smallest change using what we currently have to make this configurable.

So, the idea is that we have a way to customize additional validation logic via Callbacks, so if we utilize that struct we do not have to change the signature, but can piggyback off of it to provide ways to customize the actual webhook configuration without breaking any existing code.

Anyways, wanted to just put the smallest amount of code to see what folks think. obvs, need to write tests, etc. but just wanted to see what folks think before going too much deeper.

/kind

Fixes #

Release Note

Docs

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.

[codecov[bot]]

Codecov Report

Merging #2547 (ebf094f) into main (2e08ca6) will decrease coverage by 0.09%. The diff coverage is 69.38%.

@@            Coverage Diff             @@
##             main    #2547      +/-   ##
==========================================
- Coverage   81.32%   81.22%   -0.10%     
==========================================
  Files         162      162              
  Lines        9739     9768      +29     
==========================================
+ Hits         7920     7934      +14     
- Misses       1582     1595      +13     
- Partials      237      239       +2     

Impacted Files	Coverage Δ
...k/resourcesemantics/validation/validation_admit.go	93.12% <ø> (ø)
...k/resourcesemantics/validation/reconcile_config.go	80.16% <63.88%> (-8.01%)	:arrow_down:
webhook/resourcesemantics/validation/controller.go	95.74% <84.61%> (-4.26%)	:arrow_down:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[vaikas]

Created a new New method. I created a new struct, take a looksie @dprotaso

[vaikas]

FYI. here's a working example demonstrating how it works with before / after. As in an answer to my question above :) https://github.com/sigstore/policy-controller/pull/165

[vaikas]

Not sure what the Tekton test is all about:

go: finding module for package knative.dev/pkg/pool
github.com/tektoncd/chains/pkg/chains imports
	github.com/sigstore/cosign/pkg/cosign imports
	knative.dev/pkg/pool: module knative.dev/pkg@latest found (v0.0.0-20220819090049-2e08ca63a922, replaced by /home/runner/work/pkg/pkg/upstream), but does not contain package knative.dev/pkg/pool

[vaikas]

UT failed with:

--- FAIL: TestDialTLSWithBackoffSuccess (0.32s)
FAIL network.TestDialTLSWithBackoffSuccess (0.32s)
PASS test/imports
PASS test/interactive.TestNewCommand (0.00s)
PASS test/interactive.TestAddArgs (0.00s)
PASS test/interactive.TestCommandStringer (0.00s)

/test unit-tests_pkg_main

[mattmoor]

/lgtm /approve /unhold

🎉 PKG cut last week, so we have another 5 to roll this back if we have regrets, but let's get it baking.

[knative-prow[bot]]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattmoor, vaikas

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:

• ~~webhook/OWNERS~~ [mattmoor,vaikas]

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

[vaikas]

/test unit-tests_pkg_main

[vaikas]

Oops, forgot to cut&paste the previous failures for retrying the unit tests:

--- FAIL: TestDialTLSWithBackoffSuccess (0.38s)
FAIL network.TestDialTLSWithBackoffSuccess (0.38s)
PASS test/imports
PASS test/helpers.TestAppendRandomString (0.00s)
PASS test/helpers.TestMakeK8sNamePrefix/abcd123 (0.00s)
PASS test/helpers.TestMakeK8sNamePrefix/AbCdef (0.00s)

[dprotaso]

Been dealing with the release and haven't had a chance to comment

I think rather than baking in SupportedVerbs into our go types we allow API types to declare OnCreate OnUpdate OnDelete methods as a way to signal the lifecycle events they care about

With the current solution you're sorta bleeding admissionregistration into various go types that don't really care about that