Skip to content
This repository was archived by the owner on Jan 24, 2024. It is now read-only.

Conversation

@eolivelli
Copy link
Contributor

Motivation
When you enable JWT token authentication transactions do not work because transactions need broker-to-broker communication but there is currently no support for the SASL handshake.

Changes
Now when you enable authentication and you configure Token Authentication for Broker-To-Broker communications in Pulsar, we perform SASL PLAIN authentication while preparing the connection to other brokers.

Tests
I have extended existing SASL tests in order to use Transactions. I have copy/pasted some parts of the TransactionTest, but there was not enough code to make sense to share the test code.

@eolivelli
Copy link
Contributor Author

@Demogorgon314 @BewareMyPower
I have ported part of the KOP Proxy code (copy/paste) regarding broker-to-broker authentication.
One day if we accept the KOP Proxy here in the main repository we can share a single class for Proxy-To-Broker and for Broker-To-Broker communications.

https://github.com/datastax/starlight-for-kafka/blob/41b59fc67885db66846d945606db44062ff12f79/proxy/src/main/java/io/streamnative/pulsar/handlers/kop/ConnectionToBroker.java#L49

@eolivelli
Copy link
Contributor Author

@Demogorgon314
unfortunately the new test fails here in KOP repo

in my fork the test is passing.

This patch #849 fixes a problem with Multi-Tenant configuration and SASL, but it is not enough to make the test pass.

Today I will update #849 and address your comments, so that our codebase will be more one close to each other

@eolivelli
Copy link
Contributor Author

it looks like #849 is not needed.

in my fork I am missing the latest refactor about ReplicaManager, probably that's the cause.

can you help me in debugging the failure ?
@Demogorgon314

@Demogorgon314
Copy link
Member

Demogorgon314 commented Dec 2, 2021

@eolivelli
I found the reason why the test failed.

First of all, you need rebase master onto the current branch, then set conf.setKafkaTransactionCoordinatorEnabled(true); in SaslPlainTestBase since the TransactionCoordinator default is disabled.

Copy link
Member

@Demogorgon314 Demogorgon314 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Nice work!

@Demogorgon314 Demogorgon314 merged commit 2b89562 into streamnative:master Dec 2, 2021
BewareMyPower pushed a commit that referenced this pull request Dec 3, 2021
…brokers (#941)

**Motivation**
When you enable JWT token authentication transactions do not work because transactions need broker-to-broker communication but there is currently no support for the SASL handshake.

**Changes**
Now when you enable authentication and you configure Token Authentication for Broker-To-Broker communications in Pulsar, we perform SASL PLAIN authentication while preparing the connection to other brokers.

**Tests**
I have extended existing SASL tests in order to use Transactions. I have copy/pasted some parts of the TransactionTest, but there was not enough code to make sense to share the test code.
BewareMyPower pushed a commit that referenced this pull request Dec 3, 2021
…brokers (#941)

**Motivation**
When you enable JWT token authentication transactions do not work because transactions need broker-to-broker communication but there is currently no support for the SASL handshake.

**Changes**
Now when you enable authentication and you configure Token Authentication for Broker-To-Broker communications in Pulsar, we perform SASL PLAIN authentication while preparing the connection to other brokers.

**Tests**
I have extended existing SASL tests in order to use Transactions. I have copy/pasted some parts of the TransactionTest, but there was not enough code to make sense to share the test code.

Fix conflicts made by #864,
which changes the signature of `lookupBroker`.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants