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

Issue:
Broker deletion is stuck when the referenced secret is gone
If using a `auth.secret.ref.name` and if that referenced secret is gone (regardless how it was removed), the deletion of the broker is stuck:

```
k delete -f ./015-broker-with-config.yaml 
broker.eventing.knative.dev "my-broken-kafka-broker" deleted
configmap "my-broken-kafka-broker-config" deleted
```

Especially in cases with _external_ topics, we should be more lenient, if the secret is not present, and not block the finalization 

Pull request:
Do not block finalization if secret for external topic is gone
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>

Fixes #2828

<!-- Please include the 'why' behind your changes if no issue exists -->

## Proposed Changes

- If secret for external managed topic is gone / not present, we do not block the finalization. The broker does anyways not need to perform any sorts of clean-ups on these externally managed topics

<!--
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

```

**Docs**

<!--
:book: If this change has user-visible impact, link to an issue or PR in
https://github.com/knative/docs.
-->


Patch:
diff --git a/control-plane/pkg/reconciler/broker/broker.go b/control-plane/pkg/reconciler/broker/broker.go
index fe24de10a..aef165f12 100644
--- a/control-plane/pkg/reconciler/broker/broker.go
+++ b/control-plane/pkg/reconciler/broker/broker.go
@@ -387,14 +387,20 @@ func (r *Reconciler) finalizeKind(ctx context.Context, broker *eventing.Broker)
 		return err
 	}
 
+	_, externalTopic := isExternalTopic(broker)
 	secret, err := security.Secret(ctx, &security.MTConfigMapSecretLocator{ConfigMap: brokerConfig, UseNamespaceInConfigmap: false}, r.SecretProviderFunc())
 	if err != nil {
-		return fmt.Errorf("failed to get secret: %w", err)
+
+		if externalTopic {
+			// we don't care
+			return nil
+		} else {
+			return fmt.Errorf("failed to get secret: %w", err)
+		}
 	}
 
 	// External topics are not managed by the broker,
 	// therefore we do not delete them
-	_, externalTopic := isExternalTopic(broker)
 	if !externalTopic {
 
 		// I do not like the use of `_`
diff --git a/go.mod b/go.mod
index b11b221b5..fc06e1f11 100644
--- a/go.mod
+++ b/go.mod
@@ -38,11 +38,11 @@ require (
 require (
 	github.com/kedacore/keda/v2 v2.8.1
 	gopkg.in/yaml.v3 v3.0.1
-	knative.dev/eventing v0.35.1-0.20221214083955-1048c6cfe143
-	knative.dev/eventing-kafka v0.35.1-0.20221209060617-bdb5df2114f2
+	knative.dev/eventing v0.35.1-0.20221216111651-57da6ae64e79
+	knative.dev/eventing-kafka v0.35.1-0.20221219133153-20e1b33541bc
 	knative.dev/hack v0.0.0-20221209013717-b9801b4f5a4d
 	knative.dev/pkg v0.0.0-20221209013515-911b435f02a1
-	knative.dev/reconciler-test v0.0.0-20221209013717-62896bcee56c
+	knative.dev/reconciler-test v0.0.0-20221216013452-b9ed31373dcc
 	sigs.k8s.io/controller-runtime v0.12.3
 )
 
diff --git a/go.sum b/go.sum
index 44fb2d1f7..70d373baa 100644
--- a/go.sum
+++ b/go.sum
@@ -1514,16 +1514,16 @@ k8s.io/utils v0.0.0-20200912215256-4140de9c8800/go.mod h1:jPW/WVKK9YHAvNhRxK0md/
 k8s.io/utils v0.0.0-20210819203725-bdf08cb9a70a/go.mod h1:jPW/WVKK9YHAvNhRxK0md/EJ228hCsBRufyofKtW8HA=
 k8s.io/utils v0.0.0-20221108210102-8e77b1f39fe2 h1:GfD9OzL11kvZN5iArC6oTS7RTj7oJOIfnislxYlqTj8=
 k8s.io/utils v0.0.0-20221108210102-8e77b1f39fe2/go.mod h1:OLgZIPagt7ERELqWJFomSt595RzquPNLL48iOWgYOg0=
-knative.dev/eventing v0.35.1-0.20221214083955-1048c6cfe143 h1:/L8eTVZdLFWfyzm2MqhNCqgOCktQCnD6z9Tq2UEXaF4=
-knative.dev/eventing v0.35.1-0.20221214083955-1048c6cfe143/go.mod h1:J5vUo5Eh2RZ7NkPMi1oQGBtjt0cJX1w9SJUAFyAMD4I=
-knative.dev/eventing-kafka v0.35.1-0.20221209060617-bdb5df2114f2 h1:uz8n4MQC3sdgGngRt0y4ruPmZtJXO3p951MH3EdGkRE=
-knative.dev/eventing-kafka v0.35.1-0.20221209060617-bdb5df2114f2/go.mod h1:JrJBms/yU0kwPo7B339VrTBPuyoXP71Y21divEe37AM=
+knative.dev/eventing v0.35.1-0.20221216111651-57da6ae64e79 h1:xNfiN1/EIbAPfXW5oGk4cddyTs22v31jR1fZ+u9WR5k=
+knative.dev/eventing v0.35.1-0.20221216111651-57da6ae64e79/go.mod h1:J5vUo5Eh2RZ7NkPMi1oQGBtjt0cJX1w9SJUAFyAMD4I=
+knative.dev/eventing-kafka v0.35.1-0.20221219133153-20e1b33541bc h1:di4Ayw59srC4/jDNX1iUQteH+9rDwNQgkobM2Xi1pNo=
+knative.dev/eventing-kafka v0.35.1-0.20221219133153-20e1b33541bc/go.mod h1:KllJmRMFymCnJ7wyITn2Fokb/6R/2JiuMfd7oTCWZeM=
 knative.dev/hack v0.0.0-20221209013717-b9801b4f5a4d h1:nMp1GE3iwrJzABi4xP0xSVOwaaZMJ8loT9S42d54wFM=
 knative.dev/hack v0.0.0-20221209013717-b9801b4f5a4d/go.mod h1:yk2OjGDsbEnQjfxdm0/HJKS2WqTLEFg/N6nUs6Rqx3Q=
 knative.dev/pkg v0.0.0-20221209013515-911b435f02a1 h1:CqZicZHNls+v1vs/BvGIzgLbJGN4XxT0TjcIFje0uyM=
 knative.dev/pkg v0.0.0-20221209013515-911b435f02a1/go.mod h1:IeUSNPPUpQnM35SjpnfCx0w5/V2RpEc+nmke6oPwpD0=
-knative.dev/reconciler-test v0.0.0-20221209013717-62896bcee56c h1:lYhJOMgrRY99Gv+EMs/Vf/2yubAKN60Mqo12IKy+pJs=
-knative.dev/reconciler-test v0.0.0-20221209013717-62896bcee56c/go.mod h1:Lr0DNmdAl34DdfapBRhzF04P0zMKntIE1Jsx8BmlIzM=
+knative.dev/reconciler-test v0.0.0-20221216013452-b9ed31373dcc h1:TsC12GD3wSsybkfbxuCsca78xBhHLZGgZuSL6Byyocc=
+knative.dev/reconciler-test v0.0.0-20221216013452-b9ed31373dcc/go.mod h1:Lr0DNmdAl34DdfapBRhzF04P0zMKntIE1Jsx8BmlIzM=
 pgregory.net/rapid v0.3.3 h1:jCjBsY4ln4Atz78QoBWxUEvAHaFyNDQg9+WU62aCn1U=
 pgregory.net/rapid v0.3.3/go.mod h1:UYpPVyjFHzYBGHIxLFoupi8vwk6rXNzRY9OMvVxFIOU=
 rsc.io/binaryregexp v0.2.0/go.mod h1:qTv7/COck+e2FymRvadv62gMdZztPaShugOCi3I+8D8=
diff --git a/vendor/knative.dev/eventing/pkg/apis/flows/v1/sequence_lifecycle.go b/vendor/knative.dev/eventing/pkg/apis/flows/v1/sequence_lifecycle.go
index 47401c64b..bc6559eda 100644
--- a/vendor/knative.dev/eventing/pkg/apis/flows/v1/sequence_lifecycle.go
+++ b/vendor/knative.dev/eventing/pkg/apis/flows/v1/sequence_lifecycle.go
@@ -17,7 +17,10 @@ limitations under the License.
 package v1
 
 import (
+	"time"
+
 	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	eventingduckv1 "knative.dev/eventing/pkg/apis/duck/v1"
 	messagingv1 "knative.dev/eventing/pkg/apis/messaging/v1"
@@ -93,13 +96,20 @@ func (ss *SequenceStatus) PropagateSubscriptionStatuses(subscriptions []*messagi
 				Namespace:  s.Namespace,
 			},
 		}
-		readyCondition := s.Status.GetCondition(messagingv1.SubscriptionConditionReady)
-		if readyCondition != nil {
+
+		if readyCondition := s.Status.GetCondition(messagingv1.SubscriptionConditionReady); readyCondition != nil {
 			ss.SubscriptionStatuses[i].ReadyCondition = *readyCondition
-			if readyCondition.Status != corev1.ConditionTrue {
+			if !readyCondition.IsTrue() {
 				allReady = false
 			}
 		} else {
+			ss.SubscriptionStatuses[i].ReadyCondition = apis.Condition{
+				Type:               apis.ConditionReady,
+				Status:             corev1.ConditionUnknown,
+				Reason:             "NoReady",
+				Message:            "Subscription does not have Ready condition",
+				LastTransitionTime: apis.VolatileTime{Inner: metav1.NewTime(time.Now())},
+			}
 			allReady = false
 		}
 
@@ -142,7 +152,13 @@ func (ss *SequenceStatus) PropagateChannelStatuses(channels []*eventingduckv1.Ch
 				allReady = false
 			}
 		} else {
-			ss.ChannelStatuses[i].ReadyCondition = apis.Condition{Type: apis.ConditionReady, Status: corev1.ConditionUnknown, Reason: "NoReady", Message: "Channel does not have Ready condition"}
+			ss.ChannelStatuses[i].ReadyCondition = apis.Condition{
+				Type:               apis.ConditionReady,
+				Status:             corev1.ConditionUnknown,
+				Reason:             "NoReady",
+				Message:            "Channel does not have Ready condition",
+				LastTransitionTime: apis.VolatileTime{Inner: metav1.NewTime(time.Now())},
+			}
 			allReady = false
 		}
 	}
diff --git a/vendor/modules.txt b/vendor/modules.txt
index e4007d79b..4381130d6 100644
--- a/vendor/modules.txt
+++ b/vendor/modules.txt
@@ -1221,7 +1221,7 @@ k8s.io/utils/net
 k8s.io/utils/pointer
 k8s.io/utils/strings/slices
 k8s.io/utils/trace
-# knative.dev/eventing v0.35.1-0.20221214083955-1048c6cfe143
+# knative.dev/eventing v0.35.1-0.20221216111651-57da6ae64e79
 ## explicit; go 1.18
 knative.dev/eventing/cmd/event_display
 knative.dev/eventing/pkg/apis/config
@@ -1374,7 +1374,7 @@ knative.dev/eventing/test/upgrade/prober/wathola/fetcher
 knative.dev/eventing/test/upgrade/prober/wathola/forwarder
 knative.dev/eventing/test/upgrade/prober/wathola/receiver
 knative.dev/eventing/test/upgrade/prober/wathola/sender
-# knative.dev/eventing-kafka v0.35.1-0.20221209060617-bdb5df2114f2
+# knative.dev/eventing-kafka v0.35.1-0.20221219133153-20e1b33541bc
 ## explicit; go 1.18
 knative.dev/eventing-kafka/pkg/apis/bindings
 knative.dev/eventing-kafka/pkg/apis/bindings/v1beta1
@@ -1553,7 +1553,7 @@ knative.dev/pkg/webhook/json
 knative.dev/pkg/webhook/resourcesemantics
 knative.dev/pkg/webhook/resourcesemantics/defaulting
 knative.dev/pkg/webhook/resourcesemantics/validation
-# knative.dev/reconciler-test v0.0.0-20221209013717-62896bcee56c
+# knative.dev/reconciler-test v0.0.0-20221216013452-b9ed31373dcc
 ## explicit; go 1.18
 knative.dev/reconciler-test/cmd/eventshub
 knative.dev/reconciler-test/pkg/environment


Test patch:
diff --git a/control-plane/pkg/reconciler/broker/broker_test.go b/control-plane/pkg/reconciler/broker/broker_test.go
index 3f141e33c..1a7c3fcb8 100644
--- a/control-plane/pkg/reconciler/broker/broker_test.go
+++ b/control-plane/pkg/reconciler/broker/broker_test.go
@@ -2150,6 +2150,54 @@ func brokerFinalization(t *testing.T, format string, env config.Env) {
 				testProber: probertesting.MockProber(prober.StatusNotReady),
 			},
 		},
+		{
+			Name: "Reconciled normal - with missing auth secret",
+			Objects: []runtime.Object{
+				NewDeletedBroker(
+					WithExternalTopic(ExternalTopicName),
+					WithBrokerConfig(KReference(BrokerConfig(bootstrapServers, 20, 5,
+						BrokerAuthConfig("secret-not-present-1"),
+					))),
+					BrokerConfigMapSecretAnnotation("secret-not-present-1"),
+				),
+				BrokerConfig(bootstrapServers, 20, 5, BrokerAuthConfig("secret-not-present-1")),
+				NewConfigMapFromContract(&contract.Contract{
+					Resources: []*contract.Resource{
+						{
+							Uid:     BrokerUUID,
+							Topics:  []string{BrokerTopic()},
+							Ingress: &contract.Ingress{Path: receiver.Path(BrokerNamespace, BrokerName)},
+						},
+					},
+					Generation: 1,
+				}, env.DataPlaneConfigMapNamespace, env.ContractConfigMapName, env.ContractConfigMapFormat),
+				NewService(),
+				BrokerReceiverPod(env.SystemNamespace, map[string]string{
+					"annotation_to_preserve": "value_to_preserve",
+				}),
+				BrokerDispatcherPod(env.SystemNamespace, map[string]string{
+					"annotation_to_preserve": "value_to_preserve",
+				}),
+			},
+			Key: testKey,
+			WantUpdates: []clientgotesting.UpdateActionImpl{
+				ConfigMapUpdate(env.DataPlaneConfigMapNamespace, env.ContractConfigMapName, env.ContractConfigMapFormat, &contract.Contract{
+					Resources:  []*contract.Resource{},
+					Generation: 2,
+				}),
+				BrokerReceiverPodUpdate(env.SystemNamespace, map[string]string{
+					base.VolumeGenerationAnnotationKey: "2",
+					"annotation_to_preserve":           "value_to_preserve",
+				}),
+				BrokerDispatcherPodUpdate(env.SystemNamespace, map[string]string{
+					base.VolumeGenerationAnnotationKey: "2",
+					"annotation_to_preserve":           "value_to_preserve",
+				}),
+			},
+			OtherTestData: map[string]interface{}{
+				testProber: probertesting.MockProber(prober.StatusNotReady),
+			},
+		},
 
 		{
 			Name: "Failed to delete topic",
diff --git a/test/e2e_new/broker_test.go b/test/e2e_new/broker_test.go
index 52f168a9f..04e82a883 100644
--- a/test/e2e_new/broker_test.go
+++ b/test/e2e_new/broker_test.go
@@ -84,6 +84,22 @@ func TestBrokerConfigMapDoesNotExist(t *testing.T) {
 	env.Test(ctx, t, features.BrokerConfigMapDoesNotExist())
 }
 
+func TestBrokerAuthSecretDoesNotExist(t *testing.T) {
+	// this test is observed to flake more when it is parallel
+	// t.Parallel()
+
+	ctx, env := global.Environment(
+		knative.WithKnativeNamespace(system.Namespace()),
+		knative.WithLoggingConfig,
+		knative.WithTracingConfig,
+		k8s.WithEventListener,
+		environment.WithPollTimings(PollInterval, PollTimeout),
+		environment.Managed(t),
+	)
+
+	env.Test(ctx, t, features.BrokerAuthSecretDoesNotExist())
+}
+
 func TestTriggerLatestOffset(t *testing.T) {
 	// this test is observed to flake more when it is parallel
 	// t.Parallel()
diff --git a/test/e2e_new/features/broker_deleted_recreated.go b/test/e2e_new/features/broker_deleted_recreated.go
index b3ff86701..69ba54ba2 100644
--- a/test/e2e_new/features/broker_deleted_recreated.go
+++ b/test/e2e_new/features/broker_deleted_recreated.go
@@ -27,6 +27,8 @@ import (
 	"knative.dev/reconciler-test/pkg/feature"
 	"knative.dev/reconciler-test/resources/svc"
 
+	kafkabroker "knative.dev/eventing-kafka-broker/control-plane/pkg/reconciler/broker"
+
 	"knative.dev/eventing-kafka-broker/test/e2e_new/features/featuressteps"
 	"knative.dev/eventing-kafka-broker/test/e2e_new/resources/configmap"
 	brokerconfigmap "knative.dev/eventing-kafka-broker/test/e2e_new/resources/configmap/broker"
@@ -97,6 +99,36 @@ func BrokerConfigMapDoesNotExist() *feature.Feature {
 	return f
 }
 
+// BrokerAuthSecretDoesNotExist tests that a broker can be deleted without the Secret existing.
+func BrokerAuthSecretDoesNotExist() *feature.Feature {
+	f := feature.NewFeatureNamed("delete broker with non existing Secret")
+
+	brokerName := feature.MakeRandomK8sName("broker")
+	configName := feature.MakeRandomK8sName("config")
+
+	f.Setup("create broker config", brokerconfigmap.Install(
+		configName,
+		brokerconfigmap.WithBootstrapServer(testpkg.BootstrapServersPlaintext),
+		brokerconfigmap.WithNumPartitions(1),
+		brokerconfigmap.WithReplicationFactor(1),
+		brokerconfigmap.WithAuthSecret("does-not-exist"),
+	))
+
+	f.Setup("install broker", broker.Install(
+		brokerName,
+		append(
+			broker.WithEnvConfig(),
+			broker.WithConfig(configName),
+			broker.WithAnnotations(
+				map[string]interface{}{
+					kafkabroker.ExternalTopicAnnotation: "my-topic",
+				}))...))
+
+	f.Assert("delete broker", featuressteps.DeleteBroker(brokerName))
+
+	return f
+}
+
 // BrokerCannotReachKafkaCluster tests that a broker can be deleted even when KafkaCluster is unreachable.
 func BrokerCannotReachKafkaCluster() *feature.Feature {
 	f := feature.NewFeatureNamed("delete broker with unreachable Kafka cluster")
diff --git a/test/e2e_new/resources/configmap/broker/broker_config.go b/test/e2e_new/resources/configmap/broker/broker_config.go
index 6b0df2495..cd94adb0e 100644
--- a/test/e2e_new/resources/configmap/broker/broker_config.go
+++ b/test/e2e_new/resources/configmap/broker/broker_config.go
@@ -61,3 +61,9 @@ func WithBootstrapServer(bootstrapServer string) manifest.CfgFn {
 		cfg["bootstrapServer"] = bootstrapServer
 	}
 }
+
+func WithAuthSecret(authSecret string) manifest.CfgFn {
+	return func(cfg map[string]interface{}) {
+		cfg["authSecret"] = authSecret
+	}
+}
diff --git a/test/e2e_new/resources/configmap/broker/broker_config.yaml b/test/e2e_new/resources/configmap/broker/broker_config.yaml
index 7648bd7ce..17a5970f9 100644
--- a/test/e2e_new/resources/configmap/broker/broker_config.yaml
+++ b/test/e2e_new/resources/configmap/broker/broker_config.yaml
@@ -27,3 +27,6 @@ data:
   {{ if .replicationFactor }}
   default.topic.replication.factor: "{{ .replicationFactor }}"
   {{ end }}
+  {{ if .authSecret }}
+  auth.secret.ref.name: "{{ .authSecret }}"
+  {{ end }}