[Experimental] Spec non-compliance: `spec.subscriber` MUST be set

[evankanderson]

Describe the bug As part of https://github.com/knative/specs/pull/30 to simplify the user experience of Subscriptions, the spec was changed from requiring one of spec.subscriber or spec.reply to be set to requiring that spec.subscriber is set.

Expected behavior An admission validation error if a Subscription is created without a subscriber.

To Reproduce Create a Subscription like so:

apiVersion: messaging.knative.dev/v1
kind: Subscription
metadata:
  name: i-am-a-bad-bad-subscription
spec:
  channel:
    kind: Channel
    apiVersion: messaging.knative.dev/v1
    name: dont-care
  reply:
    uri: http://dev/null

Knative release version

0.26

Additional context Should probably get this in for 1.0

[salaboy]

@evankanderson is this a blocker for GA? Can you assign it to me? I can probably work on this one

[evankanderson]

I think it should be a blocker for GA (possibly with a flag to disable the validation until Oct 2022 because this is a v1 resource).

You can assign an issue to yourself with /assign (fyi)

/assign @salaboy

[lionelvillard]

I agree, this is a blocker for GA. @salaboy Beware this is potentially a breaking change, as @evankanderson hinted at it with adding a flag.

[salaboy]

@evankanderson @lionelvillard from the change in the spec I am assuming that spec.reply shouldn't be validated anymore. In other words, a missing reply will not cause validation to fail. Is this a correct assumption?

[lionelvillard]

Reopening since we decided to implement this as an experimental feature and there are followup tasks:

• [x] Alpha: feature disabled by default. At this stage, Knative eventing is conformant when the flag is enabled. (0.27)
• [ ] Beta: feature is enabled by default, can be disabled. At this stage, Knative eventing is conformant by default (0.28)
• [ ] GA. feature cannot be disabled. (0.29)

[lionelvillard]

@salaboy do you have some cycle to promote this feature to Beta?

[aliok]

For reference, this test here is run with no subscriber set but only with a reply set:

https://github.com/knative/eventing/blob/d4244d5b59a67ebfe7f78c412d59c16afc55319e/test/rekt/features/channel/features.go#L79-L101

So, it is actually compliant with the current spec but once subscriber is made mandatory, it will fail.

[aliok]

Same story here: https://github.com/knative/eventing/blob/d4244d5b59a67ebfe7f78c412d59c16afc55319e/test/rekt/features/channel/features.go#L57-L69

Used reply instead of subscriber as the subscriber and left subscriber blank.

[aliok]

I created new tests for the 2 cases I mentioned above. New tests use spec.subscriber properly instead of using spec.reply for subscriber information.

I kept the old tests (that only use spec.reply) as well.

PR: #6053

[salaboy]

@lionelvillard ok so to promote this to beta, we need to enable the flag by default, which can make some tests fail as mentioned by @aliok

I can send a PR for that, so this feature will be beta in 1.3 and we can make it final in 1.4

[github-actions[bot]]

This issue is stale because it has been open for 90 days with no activity. It will automatically close after 30 more days of inactivity. Reopen the issue with /reopen. Mark the issue as fresh by adding the comment /remove-lifecycle stale.

[pierDipi]

/remove-lifecycle stale /triage accepted

[aliok]

FYI, Parallel was using reply as subscriber and it is fixed here: https://github.com/knative/eventing/issues/6404

[pierDipi]

/assign /unassign salaboy

[pierDipi]

Feature promoted to beta in 1.7, so enabled by default.

[creydr]

Reopening since we decided to implement this as an experimental feature and there are followup tasks:

• [x] Alpha: feature disabled by default. At this stage, Knative eventing is conformant when the flag is enabled. (0.27)
• [ ] Beta: feature is enabled by default, can be disabled. At this stage, Knative eventing is conformant by default (0.28)
• [ ] GA. feature cannot be disabled. (0.29)

Last step from https://github.com/knative/eventing/issues/5756#issuecomment-976961730 is open. I can take over this and remove the flag so the validation cannot be disabled anymore.

/assign