fix: SASL/PLAIN

[einarmo]

The previous fix did not work. This is now definitely verified to be working and also makes coherent sense.

I made a fix in the OAUTHBEARER PR to fix the SASL flow to send a message even when the final state was Finished(true). My solution instead broke the flow in a different way, making it so that we send the final message twice.

This should fix it. I didn't catch it before because my kafka configuration was wrong and didn't properly require plain auth, and this particular issue doesn't affect oauthbearer. This time I've verified that it fails with InvalidSaslState without this fix, and manages to connect with it.

• I've read the contributing section of the project CONTRIBUTING.md.
• Signed CLA (if not already signed).

[crepererum]

do you think we could extend the end2end tests accordingly? It seems that we already have some tests that try to do that, but it didn't seem to reproduce your issue:

rskafka/tests/client.rs

Lines 28 to 50 in e65584d

	#[tokio::test]
	async fn test_sasl() {
	maybe_start_logging();
	if env::var("TEST_INTEGRATION").is_err() {
	return;
	}
	if env::var("KAFKA_SASL_CONNECT").is_err() {
	eprintln!("Skipping sasl test.");
	return;
	}
	let test_cfg = maybe_skip_kafka_integration!();
	// Redpanda broker doesn't support SASL/PLAIN at this moment.
	if test_cfg.broker_impl != BrokerImpl::Kafka {
	return;
	}
	ClientBuilder::new(vec![env::var("KAFKA_SASL_CONNECT").unwrap()])
	.sasl_config(rskafka::client::SaslConfig::Plain(
	rskafka::client::Credentials::new("admin".to_string(), "admin-secret".to_string()),
	))
	.build()
	.await
	.unwrap();
	}

[einarmo]

I'll look into it. My tests were also against the bitnami image, so it should be possible to reproduce the issue there.

[einarmo]

Turns out you have to set KAFKA_SASL_CONNECT in integration tests.

Also the test just seems to hang before this patch, not entirely sure why. I made the test time out after 2 seconds, it passes in way less than that under normal circumstances. I ran

TEST_INTEGRATION="1" KAFKA_SASL_CONNECT="localhost:9097" KAFKA_CONNECT="localhost:9097" TEST_BROKER_IMPL="kafka" cargo test test_sasl

[einarmo]

Ah, because of the infinite backoff of course. Better solution is to disable that.

[einarmo]

This turned out to be a major hassle to get working.

The CI setup didn't configure any connection that allows SASL, so I added that which produced an error complaining about a missing jaas config. Looking into mounting that I found that doing so is essentially impossible in circleci without major changes to the build. Then I found that you were actually using very old versions of the bitnami image, maybe assuming that 3 pointed to the latest 3.* version of the image, which isn't actually the case here.

The fix was to update the kafka images and adding some more environment variables, which seems to work.

[crepererum]

Awesome. Thanks a lot for going the extra mile and also improve the testing. I really appreciate that!