Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add OAuthBearer authentication support for rdkafka #21058

Conversation

andrewstanovsky
Copy link
Contributor

Description

Adds support for OAuthBearer authentication flow for rdkafka client when Kafka server expects/supports it. The 'tokenProvider' is supposed to be set via nconf programatically.

Breaking Changes

server/routerlicious/packages/services-ordering-rdkafka/src/rdkafkaBase.ts introduces connect() method breaking change by changing its return value from 'void' to 'Promise'. This is needed for tokenProvider's call convenience and overall maintainability/readability of the code.

Reviewer Guidance

This change has been verified with FRS and three types of Kafka server:

  • HDI Kafka w/ SSL auth
  • Event Hubs w/ connection string
  • Event Hubs w/ managed identity

Another change that is not directly related to the main topic is fix for incorrect node-rdkafka's feature filtering

		const rdKafkaHasSSLEnabled = kafka.features.filter((feature) =>
			feature.toLowerCase().includes("ssl"),
		);

The rdKafkaHasSSLEnabled will have an empty array if ssl is not included. Which later would be checked with the following line:

		if (!rdKafkaHasSSLEnabled) {

The problem with this line is that it will always be false whether rdKafkaHasSSLEnabled contains ssl entry or it's completely empty. Empty array is still not null/undefined, so the condition will be false.

I've changed it to an array of lower case values from the original one of rdkafka (redundant step, since it always contains lower case features, but to make sure it's always lower case and it doesn't change the previous logic).

		const lcKafkaFeatures = kafka.features.map((s) => s.toLowerCase());

And new way of checking it traverses this new array and checks it with the includes() method:

		if (!lcKafkaFeatures.includes("ssl")) {

@github-actions github-actions bot added area: server Server related issues (routerlicious) base: main PRs targeted against main branch labels May 13, 2024
@andrewstanovsky andrewstanovsky merged commit 65e26c1 into microsoft:main May 15, 2024
29 checks passed
@andrewstanovsky andrewstanovsky deleted the astanovsky/rdkafka_oauthbearer_auth branch May 15, 2024 18:47
kekachmar pushed a commit to kekachmar/FluidFramework that referenced this pull request May 21, 2024
## Description

Adds support for OAuthBearer authentication flow for rdkafka client when
Kafka server expects/supports it. The 'tokenProvider' is supposed to be
set via nconf programatically.

## Breaking Changes


server/routerlicious/packages/services-ordering-rdkafka/src/rdkafkaBase.ts
introduces connect() method breaking change by changing its return value
from 'void' to 'Promise<void>'. This is needed for tokenProvider's call
convenience and overall maintainability/readability of the code.

## Reviewer Guidance

This change has been verified with FRS and three types of Kafka server:

* HDI Kafka w/ SSL auth
* Event Hubs w/ connection string
* Event Hubs w/ managed identity


Another change that is not directly related to the main topic is fix for
incorrect node-rdkafka's feature filtering
```
		const rdKafkaHasSSLEnabled = kafka.features.filter((feature) =>
			feature.toLowerCase().includes("ssl"),
		);
```
The `rdKafkaHasSSLEnabled` will have an empty array if `ssl` is not
included. Which later would be checked with the following line:
```
		if (!rdKafkaHasSSLEnabled) {
```
The problem with this line is that it will always be false whether
`rdKafkaHasSSLEnabled` contains `ssl` entry or it's completely empty.
Empty array is still not null/undefined, so the condition will be false.

I've changed it to an array of lower case values from the original one
of rdkafka (redundant step, since it always contains lower case
features, but to make sure it's always lower case and it doesn't change
the previous logic).
```
		const lcKafkaFeatures = kafka.features.map((s) => s.toLowerCase());
```

And new way of checking it traverses this new array and checks it with
the `includes()` method:

```
		if (!lcKafkaFeatures.includes("ssl")) {
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: server Server related issues (routerlicious) base: main PRs targeted against main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants