Repository: knative-sandbox/eventing-kafka
Pull request #209 (2020-11-20 04:00:48)

Defaulting to SASL plaintext wasn't working for consolidated channel
Add additional testcase, change how we default to plaintext SASL

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

This pull-request has been approved by: *<a href="https://github.com/knative-sandbox/eventing-kafka/pull/209#" title="Author self-approved">lberk</a>*

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

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

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

- ~~[OWNERS](https://github.com/knative-sandbox/eventing-kafka/blob/master/OWNERS)~~ [lberk]

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":[]} -->
knative-prow-robot: @lberk: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-sandbox-eventing-kafka-integration-test-channel-consolidated-tls | af41f3da15f4e77a93b13ca23e74ccccadd8a3b4 | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative-sandbox_eventing-kafka/209/pull-knative-sandbox-eventing-kafka-integration-test-channel-consolidated-tls/1329635796348571648) | `/test pull-knative-sandbox-eventing-kafka-integration-test-channel-consolidated-tls`

[Full PR test history](https://gubernator.knative.dev/pr/knative-sandbox_eventing-kafka/209). [Your PR dashboard](https://gubernator.knative.dev/pr/lberk).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
knative-prow-robot: @lberk: The following tests **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-sandbox-eventing-kafka-integration-test-channel-consolidated-tls | af41f3da15f4e77a93b13ca23e74ccccadd8a3b4 | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative-sandbox_eventing-kafka/209/pull-knative-sandbox-eventing-kafka-integration-test-channel-consolidated-tls/1329635796348571648) | `/test pull-knative-sandbox-eventing-kafka-integration-test-channel-consolidated-tls`
pull-knative-sandbox-eventing-kafka-integration-test-channel-consolidated-sasl | af41f3da15f4e77a93b13ca23e74ccccadd8a3b4 | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative-sandbox_eventing-kafka/209/pull-knative-sandbox-eventing-kafka-integration-test-channel-consolidated-sasl/1329635796369543168) | `/test pull-knative-sandbox-eventing-kafka-integration-test-channel-consolidated-sasl`

[Full PR test history](https://gubernator.knative.dev/pr/knative-sandbox_eventing-kafka/209). [Your PR dashboard](https://gubernator.knative.dev/pr/lberk).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
knative-prow-robot: @lberk: The following tests **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-sandbox-eventing-kafka-integration-test-channel-consolidated-sasl | af41f3da15f4e77a93b13ca23e74ccccadd8a3b4 | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative-sandbox_eventing-kafka/209/pull-knative-sandbox-eventing-kafka-integration-test-channel-consolidated-sasl/1329635796369543168) | `/test pull-knative-sandbox-eventing-kafka-integration-test-channel-consolidated-sasl`
pull-knative-sandbox-eventing-kafka-integration-test-channel-consolidated-tls | 6baedca3fa3bb1e34310f0b2761edd4e1a4a38ee | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative-sandbox_eventing-kafka/209/pull-knative-sandbox-eventing-kafka-integration-test-channel-consolidated-tls/1329636728746545152) | `/test pull-knative-sandbox-eventing-kafka-integration-test-channel-consolidated-tls`

[Full PR test history](https://gubernator.knative.dev/pr/knative-sandbox_eventing-kafka/209). [Your PR dashboard](https://gubernator.knative.dev/pr/lberk).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
codecov[bot]: # [Codecov](https://codecov.io/gh/knative-sandbox/eventing-kafka/pull/209?src=pr&el=h1) Report
> Merging [#209](https://codecov.io/gh/knative-sandbox/eventing-kafka/pull/209?src=pr&el=desc) (af41f3d) into [master](https://codecov.io/gh/knative-sandbox/eventing-kafka/commit/016b121f17a34c55e0a7528e9dd7849ca59b8868?el=desc) (016b121) will **decrease** coverage by `0.00%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/knative-sandbox/eventing-kafka/pull/209/graphs/tree.svg?width=650&height=150&src=pr&token=W765CT93QC)](https://codecov.io/gh/knative-sandbox/eventing-kafka/pull/209?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #209      +/-   ##
==========================================
- Coverage   76.04%   76.04%   -0.01%     
==========================================
  Files         112      112              
  Lines        4275     4274       -1     
==========================================
- Hits         3251     3250       -1     
  Misses        829      829              
  Partials      195      195              
```


| [Impacted Files](https://codecov.io/gh/knative-sandbox/eventing-kafka/pull/209?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/source/client.go](https://codecov.io/gh/knative-sandbox/eventing-kafka/pull/209/diff?src=pr&el=tree#diff-cGtnL3NvdXJjZS9jbGllbnQuZ28=) | `63.41% <100.00%> (-0.45%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative-sandbox/eventing-kafka/pull/209?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/knative-sandbox/eventing-kafka/pull/209?src=pr&el=footer). Last update [016b121...6baedca](https://codecov.io/gh/knative-sandbox/eventing-kafka/pull/209?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

knative-prow-robot: @lberk: The following tests **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-sandbox-eventing-kafka-integration-test-channel-consolidated-tls | 6baedca3fa3bb1e34310f0b2761edd4e1a4a38ee | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative-sandbox_eventing-kafka/209/pull-knative-sandbox-eventing-kafka-integration-test-channel-consolidated-tls/1329636728746545152) | `/test pull-knative-sandbox-eventing-kafka-integration-test-channel-consolidated-tls`
pull-knative-sandbox-eventing-kafka-integration-test-channel-consolidated-sasl | 6baedca3fa3bb1e34310f0b2761edd4e1a4a38ee | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative-sandbox_eventing-kafka/209/pull-knative-sandbox-eventing-kafka-integration-test-channel-consolidated-sasl/1329636728771710976) | `/test pull-knative-sandbox-eventing-kafka-integration-test-channel-consolidated-sasl`

[Full PR test history](https://gubernator.knative.dev/pr/knative-sandbox_eventing-kafka/209). [Your PR dashboard](https://gubernator.knative.dev/pr/lberk).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
knative-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-sandbox-eventing-kafka-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[pkg/source/client.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative-sandbox_eventing-kafka/209/pull-knative-sandbox-eventing-kafka-go-coverage/1329636728905928704/artifacts/line-cov.html#file0) | 71.1% | 70.7% | -0.4

matzew: /retest
knative-prow-robot: @lberk: The following tests **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-sandbox-eventing-kafka-integration-test-channel-consolidated-tls | 6baedca3fa3bb1e34310f0b2761edd4e1a4a38ee | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative-sandbox_eventing-kafka/209/pull-knative-sandbox-eventing-kafka-integration-test-channel-consolidated-tls/1329636728746545152) | `/test pull-knative-sandbox-eventing-kafka-integration-test-channel-consolidated-tls`
pull-knative-sandbox-eventing-kafka-integration-test-channel-consolidated-sasl | 6baedca3fa3bb1e34310f0b2761edd4e1a4a38ee | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative-sandbox_eventing-kafka/209/pull-knative-sandbox-eventing-kafka-integration-test-channel-consolidated-sasl/1329671492383805440) | `/test pull-knative-sandbox-eventing-kafka-integration-test-channel-consolidated-sasl`

[Full PR test history](https://gubernator.knative.dev/pr/knative-sandbox_eventing-kafka/209). [Your PR dashboard](https://gubernator.knative.dev/pr/lberk).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
knative-prow-robot: @lberk: The following tests **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-sandbox-eventing-kafka-integration-test-channel-consolidated-sasl | 6baedca3fa3bb1e34310f0b2761edd4e1a4a38ee | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative-sandbox_eventing-kafka/209/pull-knative-sandbox-eventing-kafka-integration-test-channel-consolidated-sasl/1329671492383805440) | `/test pull-knative-sandbox-eventing-kafka-integration-test-channel-consolidated-sasl`
pull-knative-sandbox-eventing-kafka-integration-test-channel-consolidated-tls | 6baedca3fa3bb1e34310f0b2761edd4e1a4a38ee | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative-sandbox_eventing-kafka/209/pull-knative-sandbox-eventing-kafka-integration-test-channel-consolidated-tls/1329671492362833920) | `/test pull-knative-sandbox-eventing-kafka-integration-test-channel-consolidated-tls`

[Full PR test history](https://gubernator.knative.dev/pr/knative-sandbox_eventing-kafka/209). [Your PR dashboard](https://gubernator.knative.dev/pr/lberk).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->

Patch:
diff --git a/pkg/source/client.go b/pkg/source/client.go
index c4b275fc3..3056e7f9a 100644
--- a/pkg/source/client.go
+++ b/pkg/source/client.go
@@ -158,9 +158,7 @@ func UpdateSaramaConfigWithKafkaAuthConfig(saramaConf *sarama.Config, kafkaAuthC
 			saramaConf.Net.SASL.Handshake = true
 
 			// if SaslType is not provided we are defaulting to PLAIN
-			if kafkaAuthCfg.SASL.SaslType == "" {
-				kafkaAuthCfg.SASL.SaslType = sarama.SASLTypePlaintext
-			}
+			saramaConf.Net.SASL.Mechanism = sarama.SASLTypePlaintext
 
 			if kafkaAuthCfg.SASL.SaslType == sarama.SASLTypeSCRAMSHA256 {
 				saramaConf.Net.SASL.SCRAMClientGeneratorFunc = func() sarama.SCRAMClient { return &XDGSCRAMClient{HashGeneratorFcn: SHA256} }


Test patch:
diff --git a/pkg/source/client_test.go b/pkg/source/client_test.go
index 4ca06ef07..d2faeb968 100644
--- a/pkg/source/client_test.go
+++ b/pkg/source/client_test.go
@@ -55,35 +55,47 @@ func TestUpdateSaramaConfigWithKafkaAuthConfig(t *testing.T) {
 				SASL: &utils.KafkaSaslConfig{
 					User:     "my-user",
 					Password: "super-secret",
+					SaslType: sarama.SASLTypePlaintext,
 				},
 			},
 			enabledTLS:    false,
 			enabledSASL:   true,
-			salsMechanism: "",
+			salsMechanism: sarama.SASLTypePlaintext,
+		},
+		"Only SASL-PLAIN Auth (not specified, defaulted)": {
+			kafkaAuthCfg: &utils.KafkaAuthConfig{
+				SASL: &utils.KafkaSaslConfig{
+					User:     "my-user",
+					Password: "super-secret",
+				},
+			},
+			enabledTLS:    false,
+			enabledSASL:   true,
+			salsMechanism: sarama.SASLTypePlaintext,
 		},
 		"Only SASL-SCRAM-SHA-256 Auth": {
 			kafkaAuthCfg: &utils.KafkaAuthConfig{
 				SASL: &utils.KafkaSaslConfig{
 					User:     "my-user",
 					Password: "super-secret",
-					SaslType: "SCRAM-SHA-256",
+					SaslType: sarama.SASLTypeSCRAMSHA256,
 				},
 			},
 			enabledTLS:    false,
 			enabledSASL:   true,
-			salsMechanism: "SCRAM-SHA-256",
+			salsMechanism: sarama.SASLTypeSCRAMSHA256,
 		},
 		"Only SASL-SCRAM-SHA-512 Auth": {
 			kafkaAuthCfg: &utils.KafkaAuthConfig{
 				SASL: &utils.KafkaSaslConfig{
 					User:     "my-user",
 					Password: "super-secret",
-					SaslType: "SCRAM-SHA-512",
+					SaslType: sarama.SASLTypeSCRAMSHA512,
 				},
 			},
 			enabledTLS:    false,
 			enabledSASL:   true,
-			salsMechanism: "SCRAM-SHA-512",
+			salsMechanism: sarama.SASLTypeSCRAMSHA512,
 		},
 		"Only TLS Auth": {
 			kafkaAuthCfg: &utils.KafkaAuthConfig{
@@ -101,7 +113,7 @@ func TestUpdateSaramaConfigWithKafkaAuthConfig(t *testing.T) {
 				SASL: &utils.KafkaSaslConfig{
 					User:     "my-user",
 					Password: "super-secret",
-					SaslType: "SCRAM-SHA-512",
+					SaslType: sarama.SASLTypeSCRAMSHA512,
 				},
 				TLS: &utils.KafkaTlsConfig{
 					Cacert:   cert,
@@ -111,7 +123,7 @@ func TestUpdateSaramaConfigWithKafkaAuthConfig(t *testing.T) {
 			},
 			enabledTLS:    true,
 			enabledSASL:   true,
-			salsMechanism: "SCRAM-SHA-512",
+			salsMechanism: sarama.SASLTypeSCRAMSHA512,
 		},
 	}
 
@@ -128,7 +140,7 @@ func TestUpdateSaramaConfigWithKafkaAuthConfig(t *testing.T) {
 			}
 			if saslEnabled {
 				if tc.salsMechanism != string(config.Net.SASL.Mechanism) {
-					t.Errorf("SASL Mechanism is wrong")
+					t.Errorf("SASL Mechanism is wrong, want: %s vs got %s", tc.salsMechanism, string(config.Net.SASL.Mechanism))
 				}
 			}
 
diff --git a/test/e2e-tests.sh b/test/e2e-tests.sh
index ba2a54491..695921c58 100755
--- a/test/e2e-tests.sh
+++ b/test/e2e-tests.sh
@@ -349,17 +349,22 @@ function kafka_teardown() {
   kubectl delete namespace "${STRIMZI_KAFKA_NAMESPACE}"
 }
 
-function create_auth_secrets() {
+function create_tls_secrets() {
   echo "Creating TLS Kafka secret"
   STRIMZI_CRT=$(kubectl -n kafka get secret my-cluster-cluster-ca-cert --template='{{index .data "ca.crt"}}' | base64 --decode )
   TLSUSER_CRT=$(kubectl -n kafka get secret my-tls-user --template='{{index .data "user.crt"}}' | base64 --decode )
   TLSUSER_KEY=$(kubectl -n kafka get secret my-tls-user --template='{{index .data "user.key"}}' | base64 --decode )
-  SASL_PASSWD=$(kubectl -n kafka get secret my-sasl-user --template='{{index .data "password"}}' | base64 --decode )
 
   kubectl create secret --namespace knative-eventing generic strimzi-tls-secret \
     --from-literal=ca.crt="$STRIMZI_CRT" \
     --from-literal=user.crt="$TLSUSER_CRT" \
     --from-literal=user.key="$TLSUSER_KEY"
+}
+
+function create_sasl_secrets() {
+  echo "Creating SASL Kafka secret"
+  STRIMZI_CRT=$(kubectl -n kafka get secret my-cluster-cluster-ca-cert --template='{{index .data "ca.crt"}}' | base64 --decode )
+  SASL_PASSWD=$(kubectl -n kafka get secret my-sasl-user --template='{{index .data "password"}}' | base64 --decode )
 
   kubectl create secret --namespace knative-eventing generic strimzi-sasl-secret \
     --from-literal=ca.crt="$STRIMZI_CRT" \
@@ -437,12 +442,22 @@ function parse_flags() {
       TEST_CONSOLIDATED_CHANNEL=1
       return 1
       ;;
+    --consolidated-tls)
+      TEST_CONSOLIDATED_CHANNEL_TLS=1
+      return 1
+      ;;
+    --consolidated-sasl)
+      TEST_CONSOLIDATED_CHANNEL_SASL=1
+      return 1
+      ;;
   esac
   return 0
 }
 
 TEST_CONSOLIDATED_CHANNEL=${TEST_CONSOLIDATED_CHANNEL:-0}
 TEST_DISTRIBUTED_CHANNEL=${TEST_DISTRIBUTED_CHANNEL:-0}
+TEST_CONSOLIDATED_CHANNEL_TLS=${TEST_CONSOLIDATED_CHANNEL_TLS:-0}
+TEST_CONSOLIDATED_CHANNEL_SASL=${TEST_CONSOLIDATED_CHANNEL_SASL:-0}
 
 echo "e2e-tests.sh command line: $@"
 
@@ -453,6 +468,8 @@ initialize $@ --skip-istio-addon
 echo "e2e-tests.sh environment:"
 echo "TEST_CONSOLIDATED_CHANNEL: ${TEST_CONSOLIDATED_CHANNEL}"
 echo "TEST_DISTRIBUTED_CHANNEL: ${TEST_DISTRIBUTED_CHANNEL}"
+echo "TEST_CONSOLIDATED_CHANNEL_TLS: ${TEST_CONSOLIDATED_CHANNEL_TLS}"
+echo "TEST_CONSOLIDATED_CHANNEL_SASL: ${TEST_CONSOLIDATED_CHANNEL_SASL}"
 
 # If neither test was specified, run both
 if [[ $TEST_CONSOLIDATED_CHANNEL != 1 ]] && [[ $TEST_DISTRIBUTED_CHANNEL != 1 ]]; then
@@ -464,8 +481,15 @@ export SYSTEM_NAMESPACE
 
 if [[ $TEST_CONSOLIDATED_CHANNEL == 1 ]]; then
   test_consolidated_channel_plain || exit 1
-  create_auth_secrets
+fi
+
+if [[ $TEST_CONSOLIDATED_CHANNEL_TLS == 1 ]]; then
+  create_tls_secrets
   test_consolidated_channel_tls || exit 1
+fi
+
+if [[ $TEST_CONSOLIDATED_CHANNEL_SASL == 1 ]]; then
+  create_sasl_secrets
   test_consolidated_channel_sasl || exit 1
 fi