Repository: knative-sandbox/eventing-kafka-broker (go)

Issue:
"kafka.triggers.eventing.knative.dev" finalizer is preventing Triggers from being deleted
**Describe the bug**

(using the Channel-based broker with an IMC channel)

The deletion of a Trigger pointing to a KafkaSink is always blocked by the `kafka.triggers.eventing.knative.dev` finalizer.

I also have a few questions:
- Why `kafka.triggers.eventing.knative.dev` and not `kafkasinks.eventing.knative.dev` to follow the usual Eventing scheme?
- Why is the finalizer on the Trigger object?

```yaml
apiVersion: eventing.knative.dev/v1
kind: Trigger
metadata:
  annotations:
    eventing.knative.dev/creator: myuser
    eventing.knative.dev/lastModifier: myuser
    kapp.k14s.io/identity: v1;myuser/eventing.knative.dev/Trigger/event-types-r1;eventing.knative.dev/v1
    kapp.k14s.io/original: '{"apiVersion":"eventing.knative.dev/v1","kind":"Trigger","metadata":{"labels":{"kapp.k14s.io/app":"1620119054995517000","kapp.k14s.io/association":"v1.f6f8256cbf0af308e897984262ffd937"},"name":"event-types-r1","namespace":"myuser"},"spec":{"broker":"event-types","filter":{"attributes":{"type":"dev.knative.source.github.push"}},"subscriber":{"ref":{"apiVersion":"eventing.knative.dev/v1alpha1","kind":"KafkaSink","name":"my-kafka-topic"}}}}'
    kapp.k14s.io/original-diff-md5: 7dc139d12a2acad49f0ad929abbd1d93
  creationTimestamp: "2021-05-04T09:04:24Z"
  deletionGracePeriodSeconds: 0
  deletionTimestamp: "2021-05-04T09:07:06Z"
  finalizers:
  - kafka.triggers.eventing.knative.dev
  generation: 2
  labels:
    eventing.knative.dev/broker: event-types
    kapp.k14s.io/app: "1620119054995517000"
    kapp.k14s.io/association: v1.f6f8256cbf0af308e897984262ffd937
  name: event-types-r1
  namespace: myuser
  resourceVersion: "210458603"
  selfLink: /apis/eventing.knative.dev/v1/namespaces/myuser/triggers/event-types-r1
  uid: aef097e5-4f88-43a5-96ba-f0291a56aa09
spec:
  broker: event-types
  filter:
    attributes:
      type: dev.knative.source.github.push
  subscriber:
    ref:
      apiVersion: eventing.knative.dev/v1alpha1
      kind: KafkaSink
      name: my-kafka-topic
      namespace: myuser
status:
  conditions:
  - lastTransitionTime: "2021-05-04T09:04:26Z"
    status: "True"
    type: BrokerReady
  - lastTransitionTime: "2021-05-04T09:04:26Z"
    status: "True"
    type: DependencyReady
  - lastTransitionTime: "2021-05-04T09:04:26Z"
    status: "True"
    type: Ready
  - lastTransitionTime: "2021-05-04T09:04:26Z"
    status: "True"
    type: SubscriberResolved
  - lastTransitionTime: "2021-05-04T09:04:26Z"
    status: "True"
    type: SubscriptionReady
  observedGeneration: 1
  subscriberUri: http://kafka-sink-ingress.knative-eventing.svc.cluster.local/myuser/my-kafka-topic
```

 **Expected behavior**

No finalizer (?)
If the finalizer is _really_ mandatory, at least the controller should behave properly and not block the deletion of objects. For example, it should give up and throw a "warning" Kubernetes event if an error is unrecoverable.

 **To Reproduce**

See previous manifest.

 **Knative release version**

v0.22.0

 **Additional context**


Pull request description:
[release-0.22] Do not add a finalizer on triggers without a broker
Fixes #863

_Note: I'm not backporting the E2E test since it uses new dependencies._

## Proposed Changes

- Do not add a finalizer on triggers without a broker

<!--
If this change has user-visible impact, follow the instructions below.
Examples include:

- :gift: Add new feature
- :bug: Fix bug
- :broom: Update or clean up current behavior
- :wastebasket: Remove feature or internal logic

Otherwise delete the rest of this template.
-->

**Release Note**

<!--
:page_facing_up: If this change has user-visible impact, write a release note in the block
below. Include the string "action required" if additional action is required of
users switching to the new release, for example in case of a breaking change.

Write as if you are speaking to users, not other Knative contributors. If this
change has no user-visible impact, no release-note is needed.
-->

```release-note
Do not add a finalizer on triggers without a broker.
```

/kind bug


Patch:
diff --git a/control-plane/pkg/reconciler/trigger/controller.go b/control-plane/pkg/reconciler/trigger/controller.go
index a9783a25c..2d03cd585 100644
--- a/control-plane/pkg/reconciler/trigger/controller.go
+++ b/control-plane/pkg/reconciler/trigger/controller.go
@@ -125,7 +125,7 @@ func filterTriggers(lister eventinglisters.BrokerLister) func(interface{}) bool
 
 		broker, err := lister.Brokers(trigger.Namespace).Get(trigger.Spec.Broker)
 		if err != nil {
-			return true // TODO decide what to do on NotFound/Found
+			return false
 		}
 
 		value, ok := broker.GetAnnotations()[apiseventing.BrokerClassKey]


Test patch:
diff --git a/control-plane/pkg/reconciler/trigger/controller_test.go b/control-plane/pkg/reconciler/trigger/controller_test.go
index b67feef99..4a4d2cbe5 100644
--- a/control-plane/pkg/reconciler/trigger/controller_test.go
+++ b/control-plane/pkg/reconciler/trigger/controller_test.go
@@ -54,5 +54,5 @@ func TestFilterTriggers(t *testing.T) {
 		},
 	})
 
-	assert.True(t, pass)
+	assert.False(t, pass)
 }