Allow valid object references for Subscription's channel by nachocano · Pull Request #1283 · knative/eventing

nachocano · 2019-05-28T19:03:37Z

Fixes #1283
Helps with #1216

Proposed Changes

Allow to create subscriptions where their spec.channel does not just allow the Channel kind (^Channel$), but also other kind of channels. Similarly with spec.reply.channel. Also removed the check for apiVersion equals to eventing.knative.dev/v1alpha1.
This is needed for the different channels CRDs. E.g., InMemoryChannel, KafkaChannel, NATSSChannel, etc.
We just validate the channel configured is a valid object reference in the webhook, and perform more checks in the subscription controller. There, we check whether the channel CRD has the messaging.knative.dev/subscribable label set to true. If no, then we return an error.
If this is approved, we should add the label to the Channel spec.

Release Note

Subscriptions can now have different Channel kinds as part of their spec.channel. E.g., "InMemoryChannel", "KafkaChannel", etc.

We will do the check in the subscription controller

nachocano · 2019-05-28T19:12:06Z

/assign @Harwayne @vaikas-google @grantr

Harwayne

What happens if the spec.channel is not a subscribable? For example, a ClusterRole. What error is seen by the user?

nachocano · 2019-05-28T23:41:28Z

What happens if the spec.channel is not a subscribable? For example, a ClusterRole. What error is seen by the user?

Current version does not shown an error. I was planning on doing it in a follow up PR, but I think I should do it here before this is merged. As we cannot check for Subscribable, I'll add the label check with the CRD informer/lister in the controller, instead of the webhook. If you guys have a strong opinion of doing it in the webhook, please do let me know.
Marking this as WIP to avoid any preliminary merge

knative-metrics-robot · 2019-05-28T23:42:01Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/eventing/v1alpha1/subscription_validation.go	100.0%	96.6%	-3.4
pkg/reconciler/subscription/subscription.go	Do not exist	78.5%

Harwayne · 2019-05-29T00:04:35Z

What happens if the spec.channel is not a subscribable? For example, a ClusterRole. What error is seen by the user?

Current version does not shown an error. I was planning on doing it in a follow up PR, but I think I should do it here before this is merged. As we cannot check for Subscribable, I'll add the label check with the CRD informer/lister in the controller, instead of the webhook. If you guys have a strong opinion of doing it in the webhook, please do let me know.
Marking this as WIP to avoid any preliminary merge

The easier thing may be to add a status condition, and mark it as failing if r.syncPhysicalChannel returns an error. I think this will be necessary in the long run, whereas the CRD with a label is a nice-to-have.

nachocano · 2019-05-29T00:28:27Z

The easier thing may be to add a status condition, and mark it as failing if r.syncPhysicalChannel returns an error. I think this will be necessary in the long run, whereas the CRD with a label is a nice-to-have.

The code already does that. See here. I think I added those in a previous PR.
It seems that r.syncPhysicalChannel is not returning an error in this case. Will debug it further.

knative-metrics-robot · 2019-05-29T01:44:58Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/eventing/v1alpha1/subscription_validation.go	100.0%	96.6%	-3.4
pkg/reconciler/subscription/subscription.go	Do not exist	78.5%

updating apiextensions lib.

knative-metrics-robot · 2019-05-29T16:19:48Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/eventing/v1alpha1/subscription_validation.go	100.0%	96.6%	-3.4
pkg/reconciler/reconciler.go	Do not exist	3.6%
pkg/reconciler/subscription/subscription.go	Do not exist	74.7%

knative-metrics-robot · 2019-05-29T16:31:27Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/eventing/v1alpha1/subscription_validation.go	100.0%	96.6%	-3.4
pkg/reconciler/reconciler.go	Do not exist	3.6%
pkg/reconciler/subscription/subscription.go	Do not exist	74.7%

knative-metrics-robot · 2019-05-29T17:57:17Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/eventing/v1alpha1/subscription_validation.go	100.0%	96.6%	-3.4
pkg/reconciler/reconciler.go	Do not exist	3.6%
pkg/reconciler/subscription/subscription.go	Do not exist	74.3%

knative-metrics-robot · 2019-05-29T19:04:51Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/eventing/v1alpha1/subscription_validation.go	100.0%	96.6%	-3.4
pkg/reconciler/reconciler.go	Do not exist	3.6%
pkg/reconciler/subscription/subscription.go	Do not exist	79.7%

…acker.

nachocano · 2019-05-29T19:17:02Z

@vaikas-google @Harwayne ready for a second pass.

knative-metrics-robot · 2019-05-29T19:24:29Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/eventing/v1alpha1/subscription_validation.go	100.0%	96.6%	-3.4
pkg/reconciler/reconciler.go	Do not exist	3.6%
pkg/reconciler/subscription/subscription.go	Do not exist	79.8%

vaikas · 2019-05-29T20:30:10Z

 	Subscribers []ChannelSubscriberSpec `json:"subscribers,omitempty" patchStrategy:"merge" patchMergeKey:"uid"`
 }

+// Subscribable is an Implementable "duck type".


I think we should create a new Duck Type that looks something like this:

type Channelable struct {
metav1.TypeMeta ...
metav1.ObjectMeta ...
Spec ChannelSpec ...
Status Addressable ...

}

So that we have one Duck type that allows us to create Subscriptions to it as well get the Addressable part, since a Channel is both so rather than muck with addressable and subscribable, we have a proper Channelable type that handles spec / status. I think Addressable might not be enough, since we might also need some kind of Subscriptions statuses wired through. If you think this is reasonable, let's create an issue to track this, unless we think this change might be useful to do now.
@Harwayne @n3wscott

I agree we need this to make things cleaner.
IMO we can track it and do it in a follow up, but waiting to hear thoughts from the guys...

fine with an issue, we need to relax this constraint to be able to use new channels, please generate an issue to track this work and I'm good.

knative-metrics-robot · 2019-05-29T21:30:09Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/eventing/v1alpha1/subscription_validation.go	100.0%	96.6%	-3.4
pkg/reconciler/reconciler.go	Do not exist	3.6%
pkg/reconciler/subscription/subscription.go	Do not exist	79.8%

vaikas · 2019-05-29T22:19:46Z

/lgtm
/approve

knative-prow-robot · 2019-05-29T22:19:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nachocano, vaikas-google

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~~ [vaikas-google]

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

Co-authored-by: Markus Thömmes <markusthoemmes@me.com>

* Rollback certificate algorithm changes (knative#1281) (knative#1283) Co-authored-by: Markus Thömmes <markusthoemmes@me.com> * mute noisy metrics * [SRVKS-790] Patch subresource to unblock webhooks on 4.9 (knative#1361) Co-authored-by: Markus Thömmes <markusthoemmes@me.com> Co-authored-by: Stavros Kontopoulos <skontopo@redhat.com>

* add patch of missing versions Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Patch 0.23 with missing API versions Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * remove the patch, it's not needed in other versions Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Using pre-built image from OCP registry (#2) Signed-off-by: Matthias Wessendorf <mwessend@redhat.com> * Apply ko publish patch to here ... (#3) Signed-off-by: Matthias Wessendorf <mwessend@redhat.com> * One more thing.... (#4) Signed-off-by: Matthias Wessendorf <mwessend@redhat.com> * Add logs to understand rekt progress Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Adding back vendor patches (#5) * Rollback certificate algorithm changes (knative#1281) (knative#1283) Co-authored-by: Markus Thömmes <markusthoemmes@me.com> * mute noisy metrics * [SRVKS-790] Patch subresource to unblock webhooks on 4.9 (knative#1361) Co-authored-by: Markus Thömmes <markusthoemmes@me.com> Co-authored-by: Stavros Kontopoulos <skontopo@redhat.com> Co-authored-by: Matthias Wessendorf <mwessend@redhat.com> Co-authored-by: Markus Thömmes <markusthoemmes@me.com> Co-authored-by: Stavros Kontopoulos <skontopo@redhat.com>

nachocano added 9 commits May 23, 2019 16:03

removing validations of Kind Channel and apiVersion.

db8ab30

We will do the check in the subscription controller

adding subscribable type check

fb2505f

more on subscription

efcca93

Merge remote-tracking branch 'upstream/master' into subscription-channel

0c133b9

setting namespace to find the channel

1358fc6

removing pattern

ad8c164

Removing unnecessary changes for now.

0e2cf6b

Merge remote-tracking branch 'upstream/master' into subscription-channel

6166c8a

updates

3abb5f4

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label May 28, 2019

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 28, 2019

knative-prow-robot requested review from Harwayne and vaikas May 28, 2019 19:03

knative-prow-robot assigned grantr, Harwayne and vaikas May 28, 2019

vaikas reviewed May 28, 2019

View reviewed changes

Comment thread pkg/apis/eventing/v1alpha1/subscription_types.go

nachocano added 2 commits May 28, 2019 16:06

Merge remote-tracking branch 'upstream/master' into subscription-channel

5974ce0

better documentation

95ff2c3

Harwayne reviewed May 28, 2019

View reviewed changes

nachocano changed the title ~~Allow valid object references for Subscription's channel~~ [WIP] Allow valid object references for Subscription's channel May 28, 2019

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 28, 2019

Merge remote-tracking branch 'upstream/master' into subscription-channel

a99bc09

adding custom resource definition lister in subscription controller.

825bc7b

updating apiextensions lib.

giving permissions

190b448

nachocano added 3 commits May 29, 2019 11:45

adding some UTs

f6dd4dd

updates to test

96283ac

adding label

2135fd7

commenting the tracker code for now, until we have a cluster-level tr…

9383841

…acker.

nachocano changed the title ~~[WIP] Allow valid object references for Subscription's channel~~ Allow valid object references for Subscription's channel May 29, 2019

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 29, 2019

vaikas reviewed May 29, 2019

View reviewed changes

nachocano added 2 commits May 29, 2019 14:13

cosmetics

1684ce4

Merge remote-tracking branch 'upstream/master' into subscription-channel

d48db2f

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 29, 2019

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 29, 2019

vaikas mentioned this pull request May 29, 2019

Create "Channelable" Duck Type #1292

Closed

knative-prow-robot merged commit 06e0227 into knative:master May 29, 2019

nachocano mentioned this pull request May 30, 2019

Subscription.Reply to non-Channels #925

Closed

adrcunha mentioned this pull request Jul 24, 2019

Release notes tooling knative/test-infra#214

Closed

rhuss mentioned this pull request Sep 20, 2019

Rename subscription.reply.channel to subscription.reply.sink #1936

Closed

matzew added a commit to matzew/eventing that referenced this pull request Jun 4, 2021

Rollback certificate algorithm changes (knative#1281) (knative#1283)

e7b9776

Co-authored-by: Markus Thömmes <markusthoemmes@me.com>

matzew added a commit to matzew/eventing that referenced this pull request Sep 3, 2021

Rollback certificate algorithm changes (knative#1281) (knative#1283)

8a71f1b

Co-authored-by: Markus Thömmes <markusthoemmes@me.com>

Conversation

nachocano commented May 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

nachocano commented May 28, 2019

Uh oh!

Uh oh!

Harwayne left a comment

Choose a reason for hiding this comment

Uh oh!

nachocano commented May 28, 2019

Uh oh!

knative-metrics-robot commented May 28, 2019

Uh oh!

Harwayne commented May 29, 2019

Uh oh!

nachocano commented May 29, 2019

Uh oh!

knative-metrics-robot commented May 29, 2019

Uh oh!

knative-metrics-robot commented May 29, 2019

Uh oh!

knative-metrics-robot commented May 29, 2019

Uh oh!

knative-metrics-robot commented May 29, 2019

Uh oh!

knative-metrics-robot commented May 29, 2019

Uh oh!

nachocano commented May 29, 2019

Uh oh!

knative-metrics-robot commented May 29, 2019

Uh oh!

vaikas May 29, 2019

Choose a reason for hiding this comment

Uh oh!

nachocano May 29, 2019

Choose a reason for hiding this comment

Uh oh!

vaikas May 29, 2019

Choose a reason for hiding this comment

Uh oh!

vaikas May 29, 2019

Choose a reason for hiding this comment

Uh oh!

knative-metrics-robot commented May 29, 2019

Uh oh!

vaikas commented May 29, 2019

Uh oh!

knative-prow-robot commented May 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

nachocano commented May 28, 2019 •

edited

Loading