[BUG] Namespace Reconciler Bugs found while migrating.

[n3wscott]

Related to #734, #1069

While moving the namespace reconciler to pkg/controller I found the following bugs:

1. The namespace reconciler does not watch the resources it creates. The controller will not heal if one of those components are deleted.

2. The namespace reconciler does not clean up brokers it had created as a result of a namespace annotation. The resources should be deleted when the label is removed.

3. The namespace reconciler does not have a unit test for a conflicting default broker.

4. There is not enough events created in this reconciler, see Move namespace controller to pkg/controller style. #1069 (comment)

[shashwathi]

/assign

[Harwayne]

The namespace reconciler does not watch the resources it creates. The controller will not heal if one of those components are deleted.

The old reconciler did. This was missed during migration. I think #1127 fixed this for the new reconciler.

The namespace reconciler does not clean up brokers it had created as a result of a namespace annotation. The resources should be deleted when the label is removed.

I don't agree. Deleting all the created resources means all Brokers in the namespace stop working, as their ServiceAccount and RBAC permissions are deleted out from under them. I'm in favor of the current behavior, namely annotating creates things, removing the annotations modifies nothing, but allows users to modify the objects.

The namespace reconciler does not have a unit test for a conflicting default broker.

The behavior of not replacing an existing Broker has been useful. It allows the usage of non-default ClusterChannelProvisioners. We should have a unit test, but I think it should allow the existing one to continue to exist, not replace it.

[n3wscott]

I don't agree. Deleting all the created resources means all Brokers in the namespace stop working, as their ServiceAccount and RBAC permissions are deleted out from under them.

I think this is WAI if they stop working because the service account that was created for them is removed because the injected accounts and roles are removed. It could be a security risk to let the continue to use that service account. Trust the operator.

The behavior of not replacing an existing Broker has been useful.

I agree, I was asking for a test to confirm that it does not inject on-top of the existing broker. The existing broker should remain untouched.

[evankanderson]

Can we use OwnerReferences to prevent the deletion of the RBAC and ServiceAccount for additional Brokers? I.e. have the Broker add at least the OwnerReference for ServiceAccount, and possibly also for RoleBindings (though that sounds scarier)?

Testing that we don't take over a pre-existing Broker seems useful. This would even allow the pattern of annotating the namespace and then deliberately removing the OwnerReference to free the resources from the grasp of the controller.

[n3wscott]

We can do that but my opinion is that if something is using the injected account and creds they are doing it wrong. I would expect to be able to disable eventing if I can enable it.

It is a tricky thing, injected pods from istio are not removed, but you can also turn that behavior for future pods. Is there an example of this elsewhere in Kubernetes?

[n3wscott]

I see both arguments, but using the injected service account is like watching YouTube from iMessage when YouTube is blocked... It works but not by intent.

[akashrv]

The label is "knative-eventing-injection:enabled", and not "knative-eventing:enabled."
In that case isn't the current behavior of not to delete anything when the label is removed, correct behavior?
And when the label is present, make sure that there is always a default broker. If broker gets deleted, re-create it.

[shashwathi]

Can we use OwnerReferences to prevent the deletion of the RBAC and ServiceAccount for additional Brokers?

Yes I like this idea. This provides more guarantee for additional brokers in the namespace that SA and rolebinding wont be deleted.

removing the annotations modifies nothing, but allows users to modify the objects.

I am not in favor of this design. Removing the annotation or explicitly disabling should undo the action of creating resources. Otherwise user has to manually track the resources created and explicitly delete them.

In that case isn't the current behavior of not to delete anything when the label is removed, correct behavior?

In ideal case if adding label has effect of creating A,B resources then removing the label should delete the resources. If namespace has only default broker then there should one-stop way to disable broker and related resources and current design does not provide that.

In this example I understand the implications of deleting the resources can stop functionality of additional brokers so we could consider

• additional brokers in namespace should add themselves as owners of SA and RB resources( like Evan suggested)
• additional brokers should have their own SA and RB resources. ( I can see this idea might be a burden on user and duplication of resources). This definitely provides most guarantee for brokers about SA and RB resources.

[grantr]

I don't think we should remove anything when the label is removed, least of all the Broker.

The default injected Broker is not just a kick-the-tires construct; it's intended for use in production deployments as well (I'm predicting that most production deployments will have a single Broker per namespace). Brokers are stateful workloads and may have unrecoverable data and configuration (in Trigger specs) associated with their existence. It strikes me as excessively dangerous to favor behavior that automatically deletes data in response to an edit to a seemingly unrelated resource (the Namespace).

I agree with @akashrv that a reasonable, not-reading-the-docs interpretation of the label knative-eventing-injection is that injection is a one-way process, and that the label has no effect after injection has occurred and may be safely deleted. The fact that this interpretation is reasonable makes me even more concerned about unintended consequences.

The problem of deleting all resources associated with an injected Broker is trivially solvable by documenting the list of 3 resources that are created upon injection. If that's not enough, we can label all auto-injected resources and document a single command that will list those resources by label and delete them. This just doesn't seem like a case we need to optimize for, especially when the optimization is dangerous for production workloads.