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

Describe the bug As noted in https://github.com/knative/eventing/pull/3050#issuecomment-619645369, 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 https://github.com/knative/eventing/pull/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 https://github.com/knative/eventing/pull/3050

About this issue

Original URL
State: closed
Created 4 years ago
Comments: 15 (15 by maintainers)

Most upvoted comments

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

lberk on May 8, 2020

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 on May 8, 2020

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?

matzew on May 8, 2020

@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.

lberk on May 8, 2020