knative / eventing

Event-driven application platform for Kubernetes
https://knative.dev/docs/eventing
Apache License 2.0
1.41k stars 590 forks source link

Broker does not update apiVersion in ownerReference of the underlying Channel #4632

Closed antoineco closed 3 years ago

antoineco commented 3 years ago

Describe the bug

I found a few of those ownerReferences in Brokers that were created when Knative was v0.15.0.

$ kubectl get imc default-kne-trigger -o jsonpath='{.metadata.ownerReferences}'
[{"apiVersion":"eventing.knative.dev/v1alpha1","blockOwnerDeletion":true,"controller":true,"kind":"Broker","name":"default","uid":"b7274d06-29e8-4fef-8f47-15fe92063fde"}]

Those are invalid and do cause issues with garbage collection, because the eventing.knative.dev/v1alpha1 API doesn't exist anymore:

$ kubectl get brokers.v1.eventing.knative.dev
NAME      URL                                                                          AGE    READY   REASON
default   http://broker-ingress.knative-eventing.svc.cluster.local/antoineco/default   185d   True
$ kubectl get brokers.v1beta1.eventing.knative.dev
NAME      URL                                                                          AGE    READY   REASON
default   http://broker-ingress.knative-eventing.svc.cluster.local/antoineco/default   185d   True
$ kubectl get brokers.v1alpha1.eventing.knative.dev
error: the server doesn't have a resource type "brokers"

Expected behavior

Whatever the current "preferred version" is, the Broker reconciler should set that in the ownerReferences of all the objects (Channels) it owns, not only the ones created under the current Knative version.

$ kubectl get --raw /apis/eventing.knative.dev | jq .preferredVersion
{
  "groupVersion": "eventing.knative.dev/v1",
  "version": "v1"
}

To Reproduce

Find a time machine, return to Knative Eventing v0.15.0, create a Broker, fast forward to v0.19.0.

Knative release version

v0.19.2

Additional context

You don't actually need a time machine.

slinkydeveloper commented 3 years ago

/kind good-first-issue

If you want to pick this issue and you need any help with it, feel free to reach me out in this issue and I can help

lberk commented 3 years ago

@antoineco is this issue still relevant (v0.20 was released several weeks ago)

antoineco commented 3 years ago

Yes, if a Broker was created in the v1beta1 API version, the ownerReference will stay on that version, even after upgrading Knative.

We could also decide that v1beta1 -> v1 is not something we want to care about anymore, but we do have some fairly ancient Brokers at TriggerMesh that exhibit this behaviour.

antoineco commented 3 years ago

Closing otherwise this will rot.