Use knative/pkg/webhooks by n3wscott · Pull Request #352 · knative/eventing

n3wscott · 2018-08-14T01:25:42Z

Moves to use the shared controller in /pkg.
Removes all eventing/pkg/webhook/ code because it is no longer needed 🎉

GenericCRD support (the interface) is needed for the pkg/webhook framework to operate on types generically. In testing this I found panics realated to types that lacked the method GetSpecJSON.

Found that spec.generation was missing and knative still uses that to track the generation until there is full support for /status updates for CRDs. Added generation to all specs

Release Note

Moving to use the common framework from knative/pkg/webhook to do type validation and defaulting.
action required: manually delete the pod that runs the current webhook with the following,
  $ kubectl delete deployment eventing-webhook -n knative-eventing
  $ kubectl delete service eventing-webhook -n knative-eventing

n3wscott · 2018-08-14T01:26:18Z

Grant for LGTM:
/cc @grantr

Evan for Approval:
/assign @evankanderson

n3wscott · 2018-08-14T01:28:40Z

/cc @mattmoor

🍾 👍 💯 🦃

n3wscott · 2018-08-14T02:22:10Z

/retest

n3wscott · 2018-08-14T14:22:52Z

/hold

n3wscott · 2018-08-14T15:45:23Z

depends on knative/pkg#44

n3wscott · 2018-08-14T17:20:25Z

/hold cancel

n3wscott · 2018-08-14T17:38:40Z

ISTIO!!!

/test pull-knative-eventing-integration-tests

grantr

Love that red ink! Thanks @n3wscott.

/lgtm

grantr · 2018-08-14T17:30:53Z

-	"go.uber.org/zap"
+	v1alpha1channels "github.com/knative/eventing/pkg/apis/channels/v1alpha1"
+	v1alpha1feeds "github.com/knative/eventing/pkg/apis/feeds/v1alpha1"
+	v1alpha1flows "github.com/knative/eventing/pkg/apis/flows/v1alpha1"


Why v1alpha1flows instead of flowsv1alpha1 as used in other controllers? I'm fine with this style, just curious if there's a reason.

It was not intentional. I can change to the normal way. Just got it mixed up

Were you going to change this?

grantr · 2018-08-14T17:50:28Z

+var _ apis.Validatable = (*Feed)(nil)
+var _ apis.Defaultable = (*Feed)(nil)
+var _ apis.Immutable = (*Feed)(nil)
+var _ webhook.GenericCRD = (*Feed)(nil)


No runtime.Object cast here?

so.. some egg on my face... webhook.GenericCRD has all of these

I could boil it down to just webhook.GenericCRD

grantr · 2018-08-14T17:53:23Z

-
-// TODO(n3wscott): these tests will go away when we move to new type of webhooks.
-
-func TestNewChannelNSBus(t *testing.T) {


Did these tests (and the tests for the other types) all make it over to the apis packages?

I did move them all as I was writing the other validation parts

I can confirm. I may have pushed for some additional tests, and @n3wscott said he'd add them after this PR.

n3wscott · 2018-08-14T18:20:27Z

/hold

evankanderson

Only a few minor comments, looking very good!

evankanderson · 2018-08-14T18:26:21Z

-		Port:             443,
-		SecretName:       "eventing-webhook-certs",
-		WebhookName:      "webhook.eventing.knative.dev",
+		ServiceName:    "webhook",


Changing the name of the webhook service is likely to mean that anyone with the existing webhook installed is going to run with two webhooks until they delete the old one by hand.

Put this in "release notes" -- steal the format from knative/serving if we don't have it.

done, and made a PR to update our template: #357

evankanderson · 2018-08-14T18:27:38Z

+    role: webhook
+  name: webhook
  namespace: knative-eventing
 spec:


Why are the Service and the Deployment for the webhook in separate files?

Can we combine these at the 400 level?

(But don't do this in this PR)

evankanderson · 2018-08-14T18:30:37Z

 var _ apis.Validatable = (*Channel)(nil)
 var _ apis.Defaultable = (*Channel)(nil)
 var _ apis.Immutable = (*Channel)(nil)
+var _ runtime.Object = (*Channel)(nil)


It sounds like you don't need this anymore?

evankanderson · 2018-08-14T18:32:07Z

 var _ apis.Validatable = (*Flow)(nil)
 var _ apis.Defaultable = (*Flow)(nil)
 var _ apis.Immutable = (*Flow)(nil)
+var _ runtime.Object = (*Flow)(nil)


Another runtime.Object that's unneeded.

evankanderson · 2018-08-14T18:32:48Z

-
-// TODO(n3wscott): these tests will go away when we move to new type of webhooks.
-
-func TestNewChannelNSBus(t *testing.T) {


I can confirm. I may have pushed for some additional tests, and @n3wscott said he'd add them after this PR.

n3wscott · 2018-08-14T18:37:16Z

this time for real, ISTIO!!!

/test pull-knative-eventing-integration-tests

evankanderson

/approve

evankanderson · 2018-08-14T20:06:00Z

-	"go.uber.org/zap"
+	v1alpha1channels "github.com/knative/eventing/pkg/apis/channels/v1alpha1"
+	v1alpha1feeds "github.com/knative/eventing/pkg/apis/feeds/v1alpha1"
+	v1alpha1flows "github.com/knative/eventing/pkg/apis/flows/v1alpha1"


Were you going to change this?

evankanderson · 2018-08-14T20:06:20Z

+    role: webhook
+  name: webhook
  namespace: knative-eventing
 spec:


(But don't do this in this PR)

knative-prow-robot · 2018-08-14T20:23:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson, grantr, n3wscott

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [evankanderson]

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

n3wscott · 2018-08-14T20:32:03Z

/test pull-knative-eventing-integration-tests

evankanderson · 2018-08-14T22:57:33Z

There's still the comment in cmd/webhook/main.go.

Also, can you update the PR description to cover the addition of Generation and the rationale for same?

n3wscott · 2018-08-14T23:22:18Z

/hold cancel

knative-metrics-robot · 2018-08-15T16:57:35Z

The following is the coverage report on pkg/.
Say /test pull-knative-eventing-go-coverage to run the coverage report again

File	Old Coverage	New Coverage	Delta
pkg/apis/feeds/v1alpha1/cluster_event_type_types.go	Do not exist	0.0%
pkg/apis/feeds/v1alpha1/event_source_types.go	Do not exist	0.0%
pkg/apis/flows/v1alpha1/flow_types.go	100.0%	98.4%	-1.6

n3wscott · 2018-08-15T17:18:18Z

/test pull-knative-eventing-integration-tests

evankanderson · 2018-08-15T17:23:12Z

/lgtm

…native#352) Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>

knative-prow-robot requested review from evankanderson and inlined August 14, 2018 01:25

knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 14, 2018

knative-prow-robot assigned evankanderson Aug 14, 2018

knative-prow-robot requested a review from grantr August 14, 2018 01:26

knative-prow-robot requested a review from mattmoor August 14, 2018 01:28

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 14, 2018

Scott Nichols added 11 commits August 14, 2018 10:16

Updating to k8s 1.11.

1c9a1bb

Staging the move to webhooks v2

aae9bf9

add type checking.

53c45d0

Moved flows and most of channel.

c20432d

Adding feeds objects to webhook.

6edb9b4

Use new webhook format from knative/pkg

9775e21

deleted fake classes after dep ensure.

8f91a32

eventing webhook.

74e59ce

Found some types missed GenericCRD support. Added.

dd42d7d

Updated serving and pkg dep.

6134284

Merged master, ran dep ensure.

a9bb0c7

n3wscott force-pushed the move-to-webhookv2 branch from d0d3e35 to a9bb0c7 Compare August 14, 2018 17:19

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 14, 2018

grantr approved these changes Aug 14, 2018

View reviewed changes

knative-prow-robot assigned grantr Aug 14, 2018

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

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 14, 2018

EventSource does not have spec.type. Removed.

103cf73

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 14, 2018

evankanderson reviewed Aug 14, 2018

View reviewed changes

Adding type checking to match all kinds.

ba6ed98

evankanderson reviewed Aug 14, 2018

View reviewed changes

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

We hack the generation, adding needed field to specs.

a39a548

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 14, 2018

Scott Nichols added 2 commits August 15, 2018 16:43

order the import for types correctly.

2918805

typo.

27bd46a

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 15, 2018

knative-prow-robot merged commit 4d4c03b into knative:master Aug 15, 2018

n3wscott mentioned this pull request Aug 17, 2018

Type is not a field on source. Removed. #380

Merged

grantr mentioned this pull request Aug 21, 2018

Add grantr to list of approvers #387

Merged

This was referenced Aug 27, 2018

Validate flow as told in the flow spec comments. #398

Merged

fixed up the event type webhook validation. #399

Merged

Nothing to validate with event sources. Removing todos. #400

Merged

Validate and default feeds. #401

Merged

matzew added a commit to matzew/eventing that referenced this pull request Dec 9, 2019

updates of the OLM for serving (knative#352)

5be1817

cardil pushed a commit to cardil/knative-eventing that referenced this pull request Sep 28, 2023

Adding description for the buildin event types of our duck sources (k…

95dfb61

…native#352) Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>


		// TODO(n3wscott): these tests will go away when we move to new type of webhooks.

		func TestNewChannelNSBus(t *testing.T) {

Conversation

n3wscott commented Aug 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

n3wscott commented Aug 14, 2018

Uh oh!

n3wscott commented Aug 14, 2018

Uh oh!

n3wscott commented Aug 14, 2018

Uh oh!

n3wscott commented Aug 14, 2018

Uh oh!

n3wscott commented Aug 14, 2018

Uh oh!

n3wscott commented Aug 14, 2018

Uh oh!

n3wscott commented Aug 14, 2018

Uh oh!

grantr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n3wscott commented Aug 14, 2018

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n3wscott commented Aug 14, 2018

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Aug 14, 2018

Uh oh!

n3wscott commented Aug 14, 2018

Uh oh!

evankanderson commented Aug 14, 2018

Uh oh!

n3wscott commented Aug 14, 2018

Uh oh!

knative-metrics-robot commented Aug 15, 2018

Uh oh!

n3wscott commented Aug 15, 2018

Uh oh!

evankanderson commented Aug 15, 2018

n3wscott commented Aug 14, 2018 •

edited

Loading