Do not allow EventSource / EventType to be deleted if there are bindings to them.

[vaikas]

Expected Behavior

If there's a binding, you should not be able to remove an event source since you can lose the bind container.

Actual Behavior

you can delete an event source even if there's a binding referring it.

Steps to Reproduce the Problem

1. create an event source / event type
2. create a binding referring those
3. delete event source
4. binding is left orphaned

Additional Info

[scothis]

I've had similar thoughts with regards to the Bus-Channel and Channel-Subscription relationships. We should employ the same strategy.

[vaikas]

I'm going to add OwnerReferences to the Bindings that point to event source and type so at least we'll get a cascading delete behaviour and don't leave orphaned objects.

[vaikas]

FYI, there's a bug in CRD finalizers wrt. cascading deletes that's been fixed in 1.11. I've added:

1. if an event source has been deleted, do not leave the binding in a bad state but remove the finalizer so the object will actually be deleted
2. set the ownerreferences.

[vaikas]

So, there's a few things we could do here and we should decide on which adventure we want to choose:

1. We should make EventType have an OwnerReference to EventSource. This is not doable in YAML however (because it requires a UUID which does not exist), so I'm going to follow up on this.
2. We need to decide if we want to
   A: delete bindings when an event type (or eventsource) goes away
   B: block deletion of event source if there are things still using it
   C: we allow deletion and leave bindings around
   D: Allow deletion, but do our own finalizers for EventSource / EventType that looks for bindings that have references to it, and while they do, do not allow it to be finalized

A we should be able to do with OwnerReference in Binding pointing to EventType (and then EventType -> EventSource). There's currently a bug in CRD finalizers and that is fixed in 1.11. This would require EventType / EventSource to have a finalizer set to have blockOwnerDeletion and us setting the OwnerReference as in (#100 )

B would be a little bit messy and would require us to write a webhook and during validation walk the bindings to see if there are any pointing to me. This seems racy.

C would require us to make a defensive copy somehow (shadow CRD?)

D is totally doable, but the one downside is that it would put the EventSource / EventType into a funky state for the lifetime of the last binding still referring to it. This means you could not delete it or recreate it (at least with the same name, etc.)

There are possibly other things, but to me these seem the obvious decisions that we should resolve as far as what behaviour we want the system to have.

[grantr]

We should make EventType have an OwnerReference to EventSource

I'm not sure this is the right ownership model. EventTypes seem like standalone objects in that they are useful even if no EventSources exist.

Say I have two pubsub EventSources, prod and dev. They're likely to emit the same EventTypes. Deleting the dev EventSource shouldn't cause the EventTypes used by prod to be deleted. Conversely, I shouldn't have to create identical pullrequest-prod and pullrequest-dev EventTypes.

We need to decide if we want to

I prefer D.

A: This makes deleting an EventSource or EventType extremely risky. Fat fingering that deletion ("was that kafka5 or kafka7 that was just decommissioned?") risks deleting huge portions of the feed-side routing map. Specifying --cascade=false when doing a delete eliminates that risk, but how many operators would know to do that? If they do, this is equivalent to C.

B: Agree with @vaikas-google that this is not ideal, plus it throws away the user's desired state which is not very declarative (see my thoughts on validation in webhooks at knative/serving#1223 (comment)).

C: This would work if we copy the EventSource parameters into the Bind parameters at Bind creation time. I'm not sure if that's a good idea, but I'm leaning against due to the possibility of surprising behavior ("why is my Bind is trying to unbind with the username of an employee who quit years ago?").

D: This is the Kubernetes standard way to block deletion of an object until it can be safely deleted. The funky state is at least standardized and documented. I share your concern about the difficulty of dealing with objects in this state, but that seems like a kubectl UI problem to me.

I think the ideal case is this:

EventType and EventSource resources both get a finalizer when any Bind resource references them. That finalizer is not removed until all Bind resources referencing them are individually deleted. The Bind controller never automatically deletes a Bind.

Yes, this allows EventSource and EventType resources to stick around for an indefinite period of time, but it prevents the fat finger destruction scenario above. In that case, the user realizes they accidentally deleted the wrong EventSource, and uses the kubectl undelete command to cancel the pending deletion (this command may not actually exist, but in theory it just removes the resource's deletionTimestamp).

There's currently a bug in CRD finalizers and that is fixed in 1.11.

What's the bug with CRD finalizers?

[vaikas]

Yeah, I think option D is a good one. Note that there is no kubectl undelete, so once an event source is deleted, it can not be further modified.

Issue with finalizers was that kubectl was overriding the finalizers.
https://github.com/kubernetes/kubernetes/blob/v1.10.2/pkg/kubectl/cmd/delete.go#L310

it's been changed in the head and will do the right thing in 1.11
https://github.com/kubernetes/kubernetes/blob/ed95abe72fb914e20052b4480be1f5e42c680ca1/pkg/kubectl/cmd/delete.go#L247

[grantr]

Note that there is no kubectl undelete, so once an event source is deleted, it can not be further modified.

Too bad! But at least it's possible to retrieve the associated state and transition it elsewhere at one's convenience in case of a fat-finger delete.

it's been changed in the head and will do the right thing in 1.11

👍 For anyone who looked and was confused, Cascade is set to true by default at https://github.com/kubernetes/kubernetes/blob/ed95abe72fb914e20052b4480be1f5e42c680ca1/pkg/kubectl/cmd/delete_flags.go#L135.

[n3wscott]

we use finalizers now to do this.

/close

[knative-prow-robot]

@n3wscott: Closing this issue.

Details

In response to this:

we use finalizers now to do this.

/close

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.