Repository: knative-sandbox/eventing-kafka-broker
Pull request #1200 (2021-09-03 07:03:00)

Separate security config function
Fixes #

<!-- Please include the 'why' behind your changes if no issue exists -->

## Proposed Changes

- Divide `NewOptionFromSecret` into 2 functions that return a single value, from a single function that returns 2 values
- Improve code readability
-

<!--
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.
-->


knative-prow-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative-sandbox/eventing-kafka-broker/pull/1200#" title="Author self-approved">aliok</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **markusthoemmes** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @markusthoemmes` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative-sandbox%2Feventing-kafka-broker).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/knative-sandbox/eventing-kafka-broker/blob/main/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["markusthoemmes"]} -->
aliok: cc @pierDipi @matzew 
knative-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-sandbox-eventing-kafka-broker-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[control-plane/pkg/reconciler/broker/broker.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative-sandbox_eventing-kafka-broker/1200/pull-knative-sandbox-eventing-kafka-broker-go-coverage/1433686957409439744/artifacts/line-cov.html#file0) | 86.2% | 85.8% | -0.4
[control-plane/pkg/reconciler/sink/kafka_sink.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative-sandbox_eventing-kafka-broker/1200/pull-knative-sandbox-eventing-kafka-broker-go-coverage/1433686957409439744/artifacts/line-cov.html#file1) | 82.6% | 82.6% | 0.1
[control-plane/pkg/security/config.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative-sandbox_eventing-kafka-broker/1200/pull-knative-sandbox-eventing-kafka-broker-go-coverage/1433686957409439744/artifacts/line-cov.html#file2) | 70.0% | 63.6% | -6.4


Patch:
diff --git a/control-plane/pkg/reconciler/broker/broker.go b/control-plane/pkg/reconciler/broker/broker.go
index 5bcd72fb5..409d53272 100644
--- a/control-plane/pkg/reconciler/broker/broker.go
+++ b/control-plane/pkg/reconciler/broker/broker.go
@@ -105,11 +105,10 @@ func (r *Reconciler) reconcileKind(ctx context.Context, broker *eventing.Broker)
 
 	logger.Debug("config resolved", zap.Any("config", topicConfig))
 
-	securityOption, secret, err := security.NewOptionFromSecret(ctx, &security.MTConfigMapSecretLocator{ConfigMap: brokerConfig}, r.SecretProviderFunc())
+	secret, err := security.Secret(ctx, &security.MTConfigMapSecretLocator{ConfigMap: brokerConfig}, r.SecretProviderFunc())
 	if err != nil {
-		return fmt.Errorf("failed to create security (auth) option: %w", err)
+		return fmt.Errorf("failed to get secret: %w", err)
 	}
-
 	if secret != nil {
 		logger.Debug("Secret reference",
 			zap.String("apiVersion", secret.APIVersion),
@@ -119,6 +118,9 @@ func (r *Reconciler) reconcileKind(ctx context.Context, broker *eventing.Broker)
 		)
 	}
 
+	// get security option for Sarama with secret info in it
+	securityOption := security.NewOptionFromSecret(secret)
+
 	if err := r.TrackSecret(secret, broker); err != nil {
 		return fmt.Errorf("failed to track secret: %w", err)
 	}
@@ -279,11 +281,21 @@ func (r *Reconciler) finalizeKind(ctx context.Context, broker *eventing.Broker)
 		return fmt.Errorf("failed to resolve broker config: %w", err)
 	}
 
-	authProvider := &security.MTConfigMapSecretLocator{ConfigMap: brokerConfig}
-	securityOption, _, err := security.NewOptionFromSecret(ctx, authProvider, r.SecretProviderFunc())
+	secret, err := security.Secret(ctx, &security.MTConfigMapSecretLocator{ConfigMap: brokerConfig}, r.SecretProviderFunc())
 	if err != nil {
-		return fmt.Errorf("failed to create security (auth) option: %w", err)
+		return fmt.Errorf("failed to get secret: %w", err)
 	}
+	if secret != nil {
+		logger.Debug("Secret reference",
+			zap.String("apiVersion", secret.APIVersion),
+			zap.String("name", secret.Name),
+			zap.String("namespace", secret.Namespace),
+			zap.String("kind", secret.Kind),
+		)
+	}
+
+	// get security option for Sarama with secret info in it
+	securityOption := security.NewOptionFromSecret(secret)
 
 	topic, err := r.ClusterAdmin.DeleteTopic(kafka.Topic(TopicPrefix, broker), topicConfig.BootstrapServers, securityOption)
 	if err != nil {
diff --git a/control-plane/pkg/reconciler/sink/kafka_sink.go b/control-plane/pkg/reconciler/sink/kafka_sink.go
index 72ea7475d..b5911ce2b 100644
--- a/control-plane/pkg/reconciler/sink/kafka_sink.go
+++ b/control-plane/pkg/reconciler/sink/kafka_sink.go
@@ -80,10 +80,21 @@ func (r *Reconciler) reconcileKind(ctx context.Context, ks *eventing.KafkaSink)
 		ks.GetStatus().Annotations = make(map[string]string, 1)
 	}
 
-	securityOption, secret, err := security.NewOptionFromSecret(ctx, &SecretLocator{KafkaSink: ks}, r.SecretProviderFunc())
+	secret, err := security.Secret(ctx, &SecretLocator{KafkaSink: ks}, r.SecretProviderFunc())
 	if err != nil {
-		return fmt.Errorf("failed to create auth option: %w", err)
+		return fmt.Errorf("failed to get secret: %w", err)
 	}
+	if secret != nil {
+		logger.Debug("Secret reference",
+			zap.String("apiVersion", secret.APIVersion),
+			zap.String("name", secret.Name),
+			zap.String("namespace", secret.Namespace),
+			zap.String("kind", secret.Kind),
+		)
+	}
+
+	// get security option for Sarama with secret info in it
+	securityOption := security.NewOptionFromSecret(secret)
 
 	if err := r.TrackSecret(secret, ks); err != nil {
 		return fmt.Errorf("failed to track secret: %w", err)
@@ -262,10 +273,22 @@ func (r *Reconciler) finalizeKind(ctx context.Context, ks *eventing.KafkaSink) e
 	// 	- https://cwiki.apache.org/confluence/display/KAFKA/KIP-286%3A+producer.send%28%29+should+not+block+on+metadata+update
 
 	if ks.GetStatus().Annotations[base.TopicOwnerAnnotation] == ControllerTopicOwner {
-		securityOption, _, err := security.NewOptionFromSecret(ctx, &SecretLocator{KafkaSink: ks}, r.SecretProviderFunc())
+		secret, err := security.Secret(ctx, &SecretLocator{KafkaSink: ks}, r.SecretProviderFunc())
 		if err != nil {
-			return fmt.Errorf("failed to create security (auth) option: %w", err)
+			return fmt.Errorf("failed to get secret: %w", err)
+		}
+		if secret != nil {
+			logger.Debug("Secret reference",
+				zap.String("apiVersion", secret.APIVersion),
+				zap.String("name", secret.Name),
+				zap.String("namespace", secret.Namespace),
+				zap.String("kind", secret.Kind),
+			)
 		}
+
+		// get security option for Sarama with secret info in it
+		securityOption := security.NewOptionFromSecret(secret)
+
 		topic, err := r.ClusterAdmin.DeleteTopic(ks.Spec.Topic, ks.Spec.BootstrapServers, securityOption)
 		if err != nil {
 			return err
diff --git a/control-plane/pkg/security/config.go b/control-plane/pkg/security/config.go
index 335541e0d..6027e19e0 100644
--- a/control-plane/pkg/security/config.go
+++ b/control-plane/pkg/security/config.go
@@ -46,31 +46,37 @@ type SecretLocator interface {
 // SecretProviderFunc provides a secret given a namespace/name pair.
 type SecretProviderFunc func(ctx context.Context, namespace, name string) (*corev1.Secret, error)
 
-func NewOptionFromSecret(ctx context.Context, config SecretLocator, secretProviderFunc SecretProviderFunc) (ConfigOption, *corev1.Secret, error) {
+func NewOptionFromSecret(secret *corev1.Secret) ConfigOption {
+	if secret == nil {
+		return NoOp
+	}
+	return secretData(secret.Data)
+}
 
+func Secret(ctx context.Context, config SecretLocator, secretProviderFunc SecretProviderFunc) (*corev1.Secret, error) {
 	name, ok, err := config.SecretName()
 	if err != nil {
-		return nil, nil, err
+		return nil, err
 	}
 	if !ok {
-		// No auth config, return a no-op config option.
-		return NoOp, nil, nil
+		// No auth config, will later use a no-op config option.
+		return nil, nil
 	}
 	ns, ok, err := config.SecretNamespace()
 	if err != nil {
-		return nil, nil, err
+		return nil, err
 	}
 	if !ok {
-		// No auth config, return a no-op config option.
-		return NoOp, nil, nil
+		// No auth config, will later use a no-op config option.
+		return nil, nil
 	}
 
 	secret, err := secretProviderFunc(ctx, ns, name)
 	if err != nil {
-		return nil, nil, err
+		return nil, err
 	}
 
-	return secretData(secret.Data), secret, nil
+	return secret, nil
 }
 
 // DefaultSecretProviderFunc is a secret provider that uses the local cache for getting the secret and when the secret


Test patch:
diff --git a/.github/workflows/kind-e2e.yaml b/.github/workflows/kind-e2e.yaml
index 1319ca5e5..23ddd3421 100644
--- a/.github/workflows/kind-e2e.yaml
+++ b/.github/workflows/kind-e2e.yaml
@@ -4,9 +4,6 @@ on:
   pull_request:
     branches: [ 'main', 'release-*' ]
 
-  schedule:
-    - cron: '0 */6 * * *'
-
 defaults:
   run:
     shell: bash
diff --git a/control-plane/pkg/security/config_test.go b/control-plane/pkg/security/config_test.go
index c7df3e949..16c555eba 100644
--- a/control-plane/pkg/security/config_test.go
+++ b/control-plane/pkg/security/config_test.go
@@ -50,16 +50,14 @@ func (sp *SecretProviderFuncMock) F(ctx context.Context, namespace, name string)
 	return sp.secret, sp.err
 }
 
-func TestNewOptionFromSecret(t *testing.T) {
+func TestSecret(t *testing.T) {
 
 	tests := []struct {
 		name               string
 		ctx                context.Context
 		config             SecretLocator
 		secretProviderFunc SecretProviderFunc
-		want               ConfigOption
 		wantSecret         *corev1.Secret
-		wantConfigOption   bool
 		wantErr            bool
 	}{
 		{
@@ -87,7 +85,6 @@ func TestNewOptionFromSecret(t *testing.T) {
 				wantNamespace: "my-ns",
 				t:             t,
 			}).F,
-			wantConfigOption: true,
 			wantSecret: &corev1.Secret{
 				Data: map[string][]byte{
 					ProtocolKey: []byte(ProtocolPlaintext),
@@ -120,21 +117,17 @@ func TestNewOptionFromSecret(t *testing.T) {
 				wantNamespace: "my-ns",
 				t:             t,
 			}).F,
-			wantConfigOption: false,
-			wantSecret:       nil,
-			wantErr:          true,
+			wantSecret: nil,
+			wantErr:    true,
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			configOption, secret, err := NewOptionFromSecret(tt.ctx, tt.config, tt.secretProviderFunc)
+			secret, err := Secret(tt.ctx, tt.config, tt.secretProviderFunc)
 			if (err != nil) != tt.wantErr {
-				t.Errorf("NewOptionFromSecret() error = %v, wantErr %v", err, tt.wantErr)
+				t.Errorf("Secret() error = %v, wantErr %v", err, tt.wantErr)
 				return
 			}
-			if (configOption != nil) != tt.wantConfigOption {
-				t.Errorf("NewOptionFromSecret() configOption want %v got %p", tt.wantConfigOption, configOption)
-			}
 			if diff := cmp.Diff(tt.wantSecret, secret); diff != "" {
 				t.Errorf("NewOptionFromSecret() secret = %v, want %v diff: %s", secret, tt.wantSecret, diff)
 			}