Allow Unstructured callback from Defaulting Webhook #2363

pierDipi · 2021-11-26T11:00:23Z

Proposed Changes

Allow Defaulting webhook to register generic Unstructured callbacks so that implementers can create generic defaulting logic outside of the APIs package.

This will later be used in Eventing Kafka Broker.

Changes

Allow Unstructured callback from Defaulting Webhook

Release Note

Allow Unstructured callback from Defaulting Webhook

Signed-off-by: Pierangelo Di Pilato <[email protected]>

codecov · 2021-11-26T11:07:21Z

Codecov Report

Merging #2363 (b186600) into main (b35fcdd) will decrease coverage by 0.31%.
The diff coverage is 72.00%.

@@            Coverage Diff             @@
##             main    #2363      +/-   ##
==========================================
- Coverage   63.66%   63.35%   -0.32%     
==========================================
  Files         228      226       -2     
  Lines        9854     9855       +1     
==========================================
- Hits         6274     6244      -30     
- Misses       3305     3323      +18     
- Partials      275      288      +13

Impacted Files	Coverage Δ
metrics/config.go	`78.35% <60.00%> (+0.57%)`	⬆️
webhook/resourcesemantics/defaulting/user_info.go	`78.57% <66.66%> (-8.93%)`	⬇️
webhook/resourcesemantics/defaulting/defaulting.go	`78.09% <74.00%> (-1.54%)`	⬇️
webhook/resourcesemantics/defaulting/controller.go	`93.75% <75.00%> (-6.25%)`	⬇️
configmap/parse.go	`95.95% <0.00%> (-4.05%)`	⬇️
test/gcs/mock/mock.go	`90.32% <0.00%> (-2.16%)`	⬇️
apis/duck/v1/source_types.go	`45.28% <0.00%> (-0.72%)`	⬇️
codegen/cmd/injection-gen/generators/client.go	`0.91% <0.00%> (-0.03%)`	⬇️
hash/hash.go	`100.00% <0.00%> (ø)`
reconciler/retry.go	`100.00% <0.00%> (ø)`
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 608fc87...b186600. Read the comment docs.

pierDipi · 2021-11-26T11:10:00Z

/cc @dprotaso @julz

webhook/resourcesemantics/defaulting/defaulting.go

webhook/resourcesemantics/defaulting/defaulting_test.go

Signed-off-by: Pierangelo Di Pilato <[email protected]>

pierDipi · 2021-12-03T18:14:55Z

=== RUN   TestMetricsExport/OpenCensus
    e2e_test.go:194: Created exporter at localhost:12345
{"level":"info","ts":1638555035.7005816,"logger":"fallback","caller":"metrics/metrics_worker.go:76","msg":"Flushing the existing exporter before setting up the new exporter."}
{"level":"info","ts":1638555035.7940946,"logger":"fallback","caller":"metrics/opencensus_exporter.go:56","msg":"Created OpenCensus exporter with config:","config":{}}
{"level":"info","ts":1638555035.796918,"logger":"fallback","caller":"metrics/metrics_worker.go:91","msg":"Successfully updated the metrics exporter; old config: &{knative.dev/project testComponent prometheus 1000000000 <nil>  false 19090 0.0.0.0}; new config &{knative.dev/project testComponent opencensus 1000000000 <nil> localhost:12345 false 0 }"}
    e2e_test.go:232: Timeout reading input
    e2e_test.go:238: Unexpected OpenCensus exports (-want +got):
          []metrics.metricExtract(Inverse(Sort, []string{
        - 	"knative.dev/project/testComponent/global_export_counts<>:2",
        - 	"knative.dev/project/testComponent/resource_global_export_count<>:2",

#1672
/retest

Signed-off-by: Pierangelo Di Pilato <[email protected]>

pierDipi · 2021-12-03T19:10:37Z

2021/12/03 19:01:22 http: TLS handshake error from 127.0.0.1:36974: EOF
2021/12/03 19:01:22 http: TLS handshake error from 127.0.0.1:37248: EOF
2021/12/03 19:01:23 http: TLS handshake error from 127.0.0.1:37972: EOF
2021/12/03 19:01:23 http: TLS handshake error from 127.0.0.1:49456: tls: no certificates configured
2021/12/03 19:01:23 http: TLS handshake error from 127.0.0.1:49454: EOF
--- FAIL: TestConversionEmptyRequestBody (0.21s)
    logger.go:130: 2021-12-03T19:01:23.406Z	INFO	webhook/webhook.go:200	Informers have been synced, unblocking admission webhooks.
    logger.go:130: 2021-12-03T19:01:23.512Z	ERROR	webhook/webhook.go:150	failed to fetch secret	{"error": "secret \"webhook-certs\" not found"}
    webhook_integration_test.go:142: failed to get resp Get "https://0.0.0.0:38147/bazinga": remote error: tls: unrecognized name
    logger.go:130: 2021-12-03T19:01:23.513Z	INFO	webhook/webhook.go:242	Starting to fail readiness probes...
2021/12/03 19:01:23 http: TLS handshake error from 127.0.0.1:49644: EOF
2021/12/03 19:01:23 http: TLS handshake error from 127.0.0.1:34188: EOF
2021/12/03 19:01:24 http: TLS handshake error from 127.0.0.1:41514: EOF
FAIL

Flake in GH action #1509

Signed-off-by: Pierangelo Di Pilato <[email protected]>

pierDipi · 2021-12-14T12:59:00Z

@julz @markusthoemmes can you please review this PR since Dave is on vacation?

Signed-off-by: Pierangelo Di Pilato <[email protected]>

pierDipi · 2022-01-07T07:28:39Z

🙏 @dprotaso

dprotaso

some minor things

dprotaso · 2022-01-11T21:45:05Z

webhook/resourcesemantics/defaulting/user_info.go

@@ -50,3 +53,28 @@ func setUserInfoAnnotations(ctx context.Context, resource apis.HasSpec, groupNam
 		}
 	}
 }
+
+// setUserInfoAnnotationsUnstructured sets creator and updater annotations on a resource.
+func setUserInfoAnnotationsUnstructured(ctx context.Context, after *unstructured.Unstructured, before *unstructured.Unstructured, req *admissionv1.AdmissionRequest) {


UnstructuredContent() returns the whole object as a map so the equality.semantic.DeepEqual call isn't the same as the other helper - which is just comparing the spec when updating the UpdaterAnnotation.

I wonder if it's simpler to just wrap unstructured in a special type that returns the spec to satisfy apis.HasSpec and use the other helper.

Also unstructured objects conform to metav1.Object so there are methods for GetAnnotations, SetAnnotations etc.

Done in 8cc1192

I didn't want to expose this adapter to API users so it is only used internally.

dprotaso · 2022-01-12T01:15:55Z

webhook/resourcesemantics/defaulting/defaulting_test.go

+			r.Spec.FieldForCallbackDefaulting = "magic value"
+			// THIS IS NOT WHO IS CREATING IT, IT LIES!
+			r.Annotations = map[string]string{
+				"pkg.knative.dev/lastModifier": user2,
+			}


Should this be set given that the resource is being 'created' and not 'updated'?

if I understand your question, it was set

{ Operation: "replace", Path: "/spec/fieldDefaultingCallback", Value: "I'm a default", }

The problem is that the json tag was called differently from the field name of the struct.
I pushed a new commit to fix it.

Signed-off-by: Pierangelo Di Pilato <[email protected]>

dprotaso · 2022-01-14T14:08:43Z

/lgtm
/approve

knative-prow-robot · 2022-01-14T14:09:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, pierDipi

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

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

Add support for callback defaults

885e287

Signed-off-by: Pierangelo Di Pilato <[email protected]>

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Nov 26, 2021

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 26, 2021

knative-prow-robot requested review from dprotaso and julz November 26, 2021 11:10

dprotaso reviewed Dec 3, 2021

View reviewed changes

pierDipi added 3 commits December 3, 2021 18:58

Put unstr object in ctx and set user info

a8cc831

Signed-off-by: Pierangelo Di Pilato <[email protected]>

Move get callback at the top

7628c02

Signed-off-by: Pierangelo Di Pilato <[email protected]>

Panic when using delete verb

b2524a0

Signed-off-by: Pierangelo Di Pilato <[email protected]>

pierDipi force-pushed the KNATIVE_callback-defaults branch from fd777cc to b2524a0 Compare December 3, 2021 18:08

Split tests and add callback ctx tests

500de57

Signed-off-by: Pierangelo Di Pilato <[email protected]>

Set user info annotations

0bb0dbb

Signed-off-by: Pierangelo Di Pilato <[email protected]>

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 6, 2021

pierDipi requested a review from dprotaso December 6, 2021 11:13

Register Webhook Rules from callbacks

4162f86

Signed-off-by: Pierangelo Di Pilato <[email protected]>

dprotaso reviewed Jan 12, 2022

View reviewed changes

pierDipi added 2 commits January 12, 2022 09:26

Adapt unstructured objects to apis.HasSpec

8cc1192

Signed-off-by: Pierangelo Di Pilato <[email protected]>

Change json tag name to match struct field name

b186600

Signed-off-by: Pierangelo Di Pilato <[email protected]>

pierDipi force-pushed the KNATIVE_callback-defaults branch from 9b393b9 to b186600 Compare January 12, 2022 08:47

pierDipi requested a review from dprotaso January 13, 2022 12:09

knative-prow-robot assigned dprotaso Jan 14, 2022

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 14, 2022

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 14, 2022

knative-prow-robot merged commit 0a429cb into knative:main Jan 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow Unstructured callback from Defaulting Webhook #2363

Allow Unstructured callback from Defaulting Webhook #2363

pierDipi commented Nov 26, 2021

codecov bot commented Nov 26, 2021 •

edited

Loading

pierDipi commented Nov 26, 2021

pierDipi commented Dec 3, 2021

pierDipi commented Dec 3, 2021

pierDipi commented Dec 14, 2021

pierDipi commented Jan 7, 2022

dprotaso left a comment

dprotaso Jan 11, 2022

pierDipi Jan 12, 2022

dprotaso Jan 12, 2022

pierDipi Jan 12, 2022

dprotaso commented Jan 14, 2022

knative-prow-robot commented Jan 14, 2022

Allow Unstructured callback from Defaulting Webhook #2363

Allow Unstructured callback from Defaulting Webhook #2363

Conversation

pierDipi commented Nov 26, 2021

Changes

codecov bot commented Nov 26, 2021 • edited Loading

Codecov Report

pierDipi commented Nov 26, 2021

pierDipi commented Dec 3, 2021

pierDipi commented Dec 3, 2021

pierDipi commented Dec 14, 2021

pierDipi commented Jan 7, 2022

dprotaso left a comment

Choose a reason for hiding this comment

dprotaso Jan 11, 2022

Choose a reason for hiding this comment

pierDipi Jan 12, 2022

Choose a reason for hiding this comment

dprotaso Jan 12, 2022

Choose a reason for hiding this comment

pierDipi Jan 12, 2022

Choose a reason for hiding this comment

dprotaso commented Jan 14, 2022

knative-prow-robot commented Jan 14, 2022

codecov bot commented Nov 26, 2021 •

edited

Loading