Meta Channel doesn't conform the spec's CRD spec.subscribers requirement

[aliok]

Describe the bug
As noted in #3050 (comment), Channel doesn't doesn't allow subscribers in the spec.

Spec says this:

Spec Requirements
v1alpha1 Spec: each channel CRD MUST contain an array of subscribers: spec.subscribable.subscribers
v1beta1 Spec: each channel CRD MUST contain an array of subscribers: spec.subscribers

When a channel.messaging.knative.dev is created, by default Knative creates an InMemoryChannel. When we create a subscription against that Channel, InMemoryChannel will have the subscribers in the status, but it is not propagated to channel.messaging.knative.dev. Other fields like status.address are propagated fine.

Expected behavior
I should be able to create a channel.messaging.knative.dev with spec.subscribable.subscribers and #3050 should pass.

To Reproduce

cat <<EOS |kubectl apply -f -
---
apiVersion: messaging.knative.dev/v1alpha1
kind: Channel
metadata:
  name: foo
spec:
  subscribable:
    subscribers:
    - UID: "1234"
      ReplyUri: "foo.bar"
EOS

ends up in

Error from server (BadRequest): error when creating "channel_v1alpha1.yaml": admission webhook "validation.webhook.eventing.knative.dev" denied the request: validation failed: must not set the field(s): spec.subscribable.subscribers

Knative release version
0.14

Additional context
#3050

[vaikas]

So, not allowing the setting of subscribers is definitely intentional. Without this it's impossible to have a source of truth that's reliable. If we allow modifications to both channel as well as the backing channel, hilarity will ensue in trying to understand what the actual intent is.

Not propagating the status seems like a bug as they should be propagated to the Channel (from the backing channel, one way sync only so there's no problem like we have with two sources of modifiable truth).

I'd suggest we tackle the documentation for the Channel either as "non-conformant" Channel or talk about more in the context of a convenience factory method.
@grantr
@lionelvillard

[aliok]

So, for this issue, is it enough to change the spec statement

each channel CRD MUST contain an array of subscribers: spec.subscribers

to

other than channels.messaging.knative.dev, each channel CRD MUST contain an array of subscribers: spec.subscribers

?

[matzew]

Right, setting the subscribers is not for the spec, I added a comment here: #3050 (comment)

[matzew]

I'd suggest we tackle the documentation for the Channel either as "non-conformant" Channel or talk about more in the context of a convenience factory method.

@vaikas I think I would lean towards the later ?

[vaikas]

That was my knee jerk reaction as well.

[lberk]

ok I'll try and tackle this as part of reviewing's @aliok's #3016
/unassign @aliok
/assign @lberk

[aliok]

/reopen

There's somethings mixed here. The PR #3088 is great (thanks @lberk ). It fixes #3037 and not this issue.

Reopening this and closing that one.

The tickets were named very similar.

[knative-prow-robot]

@aliok: Reopened this issue.

In response to this:

/reopen

There's somethings mixed here. The PR #3088 is great (thanks @lberk ). It fixes #3037 and not this issue.

Reopening this and closing that one.

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.

[lberk]

@aliok As @vaikas mentioned in his comment:

So, not allowing the setting of subscribers is definitely intentional. Without this it's impossible to have a source of truth that's reliable. If we allow modifications to both channel as well as the backing channel, hilarity will ensue in trying to understand what the actual intent is.

That's why we adjusted the channel to properly reflect the subscribers status in the 'meta' channel. As stated, it is by design that we do not allow setting the subscriber field directly, if we did, we'd end up in a reconciler war between the backingChannel and the Channel itself.

If you still feel this issue is keeping open, then we'd need to discuss actually amending the spec for the Channel as specifically non-conformant.

[matzew]

discuss actually amending the spec for the Channel as specifically non-conformant.

not sure if we need to be that extreme ;-)

but how about saying, or "specifying" that the abstract channel is not supporting this, b/c it is handled by the "backing implementation(s)" ?

@lberk thoughts?

[aliok]

That's why we adjusted the channel to properly reflect the subscribers status in the 'meta' channel. As stated, it is by design that we do not allow setting the subscriber field directly, if we did, we'd end up in a reconciler war between the backingChannel and the Channel itself.

That's #3037

If you still feel this issue is keeping open, then we'd need to discuss actually amending the spec for the Channel as specifically non-conformant.

Either we keep this open and change the spec and close this OR we close this create a new ticket for spec change

[lberk]

I'm open to whatever the colour of the bikeshed you'd prefer in terms of phrasing it, but yes, the point being if we'd like to further address the issue (which I assume so because it was reopened), then we need to tackle in the spec

[lberk]

Either we keep this open and change the spec and close this OR we close this create a new ticket for spec change

no need for more issues & extra churn, lets just open a PR and reference this issue

[aliok]

/close

Fixed by #3132

[knative-prow-robot]

@aliok: Closing this issue.

In response to this:

/close

Fixed by #3132

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.