Trigger subscriptions not updated on upgrade

mikehelmick commented 4 years ago

Describe the bug

On upgrade from 0.12 to 0.13, using a channel based broker config.

Triggers seem to be correctly upgraded from v1alpha1 to v1beta1, but the subscriptions on the underlying channel do not get upgraded correctly, and end up with invalid spec because of it.

Expected behavior

User facing model objects should continue to work on cluster upgrade.

To Reproduce

Using 0.12 w/ v1alpha1 broker configured for the in memory channel (possibly any channel) and a trigger against that broker.

Upgrade to 0.13

Observe failure of trigger status to not ready.

Knative release version

0.13

Additional context

Resource status on cluster after upgrade

❯ kubectl get triggers/funk-trigger-sayhello-cloudrun -oyaml
apiVersion: eventing.knative.dev/v1beta1
kind: Trigger
metadata:
  annotations:
    kubectl.kubernetes.io/last-applied-configuration: |
      {"apiVersion":"eventing.knative.dev/v1alpha1","kind":"Trigger","metadata":{"annotations":{},"name":"funk-trigger-sayhello-cloudrun","namespace":"default"},"spec":{"broker":"default","filter":{"attributes":{"type":"com.mikehelmick.eventutils.user.user"}},"subscriber":{"URI":"https://sayhello-9a687aac612347723895f45363a859e-byacsc76eq-uc.a.run.app/"}}}
  creationTimestamp: "2020-02-24T20:43:16Z"
  generation: 1
  labels:
    eventing.knative.dev/broker: default
  name: funk-trigger-sayhello-cloudrun
  namespace: default
  resourceVersion: "24139161"
  selfLink: /apis/eventing.knative.dev/v1beta1/namespaces/default/triggers/funk-trigger-sayhello-cloudrun
  uid: 707eecc6-2a94-4ff0-97aa-e9db3c71fc7b
spec:
  broker: default
  filter:
    attributes:
      type: com.mikehelmick.eventutils.user.user
  subscriber:
    uri: https://sayhello-9a687aac612347723895f45363a859e-byacsc76eq-uc.a.run.app/
status:
  conditions:
  - lastTransitionTime: "2020-03-09T19:56:02Z"
    status: "True"
    type: BrokerReady
  - lastTransitionTime: "2020-02-24T20:43:16Z"
    status: "True"
    type: DependencyReady
  - lastTransitionTime: "2020-03-09T19:56:02Z"
    message: 'Failed to resolve spec.reply: url missing in address of &ObjectReference{Kind:Broker,Namespace:default,Name:default,UID:,APIVersion:eventing.knative.dev/v1alpha1,ResourceVersion:,FieldPath:,}'
    reason: ReplyResolveFailed
    status: "False"
    type: Ready
  - lastTransitionTime: "2020-03-09T19:56:02Z"
    message: 'Failed to resolve spec.reply: url missing in address of &ObjectReference{Kind:Broker,Namespace:default,Name:default,UID:,APIVersion:eventing.knative.dev/v1alpha1,ResourceVersion:,FieldPath:,}'
    reason: ReplyResolveFailed
    status: "False"
    type: Subscribed
  - lastTransitionTime: "2020-02-24T20:43:16Z"
    status: "True"
    type: SubscriberResolved
  observedGeneration: 1
  subscriberUri: https://sayhello-9a687aac612347723895f45363a859e-byacsc76eq-uc.a.run.app/

❯ kubectl get subscriptions/default-funk-trigger-sayhe-707eecc6-2a94-4ff0-97aa-e9db3c71fc7b -oyaml
apiVersion: messaging.knative.dev/v1beta1
kind: Subscription
metadata:
  annotations:
    messaging.knative.dev/creator: system:serviceaccount:knative-eventing:eventing-controller
    messaging.knative.dev/lastModifier: system:serviceaccount:knative-eventing:eventing-controller
  creationTimestamp: "2020-02-24T20:43:16Z"
  deletionGracePeriodSeconds: 0
  deletionTimestamp: "2020-03-09T19:55:52Z"
  finalizers:
  - subscription-controller
  generation: 3
  labels:
    eventing.knative.dev/broker: default
    eventing.knative.dev/trigger: funk-trigger-sayhello-cloudrun
  name: default-funk-trigger-sayhe-707eecc6-2a94-4ff0-97aa-e9db3c71fc7b
  namespace: default
  ownerReferences:
  - apiVersion: eventing.knative.dev/v1alpha1
    blockOwnerDeletion: true
    controller: true
    kind: Trigger
    name: funk-trigger-sayhello-cloudrun
    uid: 707eecc6-2a94-4ff0-97aa-e9db3c71fc7b
  resourceVersion: "24139078"
  selfLink: /apis/messaging.knative.dev/v1beta1/namespaces/default/subscriptions/default-funk-trigger-sayhe-707eecc6-2a94-4ff0-97aa-e9db3c71fc7b
  uid: 2baf1503-43eb-410a-984e-063438d82322
spec:
  channel:
    apiVersion: messaging.knative.dev/v1alpha1
    kind: InMemoryChannel
    name: default-kne-trigger
  reply:
    ref:
      apiVersion: eventing.knative.dev/v1alpha1
      kind: Broker
      name: default
      namespace: default
  subscriber:
    uri: http://default-broker-filter.default.svc.cluster.local/triggers/default/funk-trigger-sayhello-cloudrun/707eecc6-2a94-4ff0-97aa-e9db3c71fc7b
status:
  conditions:
  - lastTransitionTime: "2020-02-24T20:43:16Z"
    status: "True"
    type: AddedToChannel
  - lastTransitionTime: "2020-02-24T20:43:17Z"
    status: "True"
    type: ChannelReady
  - lastTransitionTime: "2020-03-09T19:55:52Z"
    message: 'Failed to resolve spec.reply: url missing in address of &ObjectReference{Kind:Broker,Namespace:default,Name:default,UID:,APIVersion:eventing.knative.dev/v1alpha1,ResourceVersion:,FieldPath:,}'
    reason: ReplyResolveFailed
    status: "False"
    type: Ready
  - lastTransitionTime: "2020-03-09T19:55:52Z"
    message: 'Failed to resolve spec.reply: url missing in address of &ObjectReference{Kind:Broker,Namespace:default,Name:default,UID:,APIVersion:eventing.knative.dev/v1alpha1,ResourceVersion:,FieldPath:,}'
    reason: ReplyResolveFailed
    status: "False"
    type: Resolved
  observedGeneration: 1
  physicalSubscription:
    replyURI: http://default-broker.default.svc.cluster.local
    subscriberURI: http://default-broker-filter.default.svc.cluster.local/triggers/default/funk-trigger-sayhello-cloudrun/707eecc6-2a94-4ff0-97aa-e9db3c71fc7b

mikehelmick commented 4 years ago

/cc @vaikas @grantr

grantr commented 4 years ago

Broker yaml as provided by @mikehelmick:

apiVersion: eventing.knative.dev/v1beta1
kind: Broker
metadata:
  annotations:
    eventing.knative.dev/creator: system:serviceaccount:knative-eventing:eventing-controller
    eventing.knative.dev/lastModifier: system:serviceaccount:knative-eventing:eventing-controller
  creationTimestamp: "2020-03-09T19:55:52Z"
  finalizers:
  - brokers.eventing.knative.dev
  generation: 1
  labels:
    eventing.knative.dev/namespaceInjected: "true"
  name: default
  namespace: default
  ownerReferences:
  - apiVersion: v1
    blockOwnerDeletion: true
    controller: true
    kind: Namespace
    name: default
    uid: 7eb92d1b-ab05-4ee2-9078-791396cae7b5
  resourceVersion: "24139162"
  selfLink: /apis/eventing.knative.dev/v1beta1/namespaces/default/brokers/default
  uid: 8bc23156-092a-4491-8fb7-325af7284660
spec:
  config:
    apiVersion: v1
    kind: ConfigMap
    name: config-br-default-channel
    namespace: knative-eventing
status:
  address:
    url: http://default-broker.default.svc.cluster.local
  conditions:
  - lastTransitionTime: "2020-03-09T19:55:52Z"
    status: "True"
    type: Addressable
  - lastTransitionTime: "2020-03-09T19:56:02Z"
    status: "True"
    type: FilterReady
  - lastTransitionTime: "2020-03-09T19:55:59Z"
    status: "True"
    type: IngressReady
  - lastTransitionTime: "2020-03-09T19:56:02Z"
    status: "True"
    type: Ready
  - lastTransitionTime: "2020-03-09T19:55:52Z"
    status: "True"
    type: TriggerChannelReady
  observedGeneration: 1

grantr commented 4 years ago

Is the default-kne-trigger InMemoryChannel a v1alpha1 or a v1beta1? If the channel doesn't exist anymore, we can probably assume it was the same as the one used by config-br-default-channel, which still uses v1alpha1.

The error on the Subscription looks like it's the Subscription controller failing to find a valid Addressable URL in the object referenced by the reply ref (the Broker). The reply reference is referencing the Broker with eventing.knative.dev/v1alpha1, but the Broker is v1beta1.

I see three potential bugs here:

The Trigger controller isn't creating Subscriptions with the correct Broker API version in references. The Broker reference is v1alpha1 but the Broker is v1beta1.
The Broker conversion webhook may be clobbering status.address.url when converting v1beta1 Brokers to v1alpha1.
The Broker default channel is IMC v1alpha1 but should be v1beta1.

vaikas commented 4 years ago

One interesting bit here is that the Subscription is marked as deleted, has: deletionTimestamp: "2020-03-09T19:55:52Z"

as far as the reference being v1alpha1 or v1beta1 should be fine because it will be converted on the fly, so they should be equivalent. As far as clobbering, I just checked (but if @mikehelmick could check this also) like so: Here's my v1beta1 broker status: status: address: url: http://default-broker.test-broker-6.svc.cluster.local

And with v1alpha I get: kubectl -n test-broker-6 get brokers.v1alpha1.eventing.knative.dev -oyaml status: address: url: http://default-broker.test-broker-6.svc.cluster.local

They look the same, and tracing the types looks good to me as well. @mikehelmick would you mind grabbing the broker with the full path brokers.v1alpha1.eventing.knative.dev as above?

Also tyring to figure out what the reasons for the .1 release were, were there bugs with the .13 that forced us to .13.1? I seem to recall there were, with trigger?

mikehelmick commented 4 years ago

I retested this going from 12.0 to 13.1 and the .1 release patch doesn't have this issue.

mikehelmick commented 4 years ago

and verified this works with 12.0 to 13.0 as well.

Going to close this since I can't reproduce it. Thanks for the help / investigation here.

vaikas commented 4 years ago

Thanks for checking Mike! :)

knative / eventing

Trigger subscriptions not updated on upgrade #2740