Fix OAuthCredentials (and connecting to SR via direct connections) #290

rohitsanj · 2025-01-09T21:55:41Z

Summary of Changes

Closes #126

Changes related to fixing OAuth credentials:

Adds a new credential option for logicalClusterId -- this is used when connecting to CCloud Kafka/SR over OAuth. See this docs page.
Rename identityPool to identityPoolId for clarity
Fix JAAS config construction by adding a semicolon at the end.

Once I fixed the above, I noticed that Kafka requests were going through fine but the SR requests were all failing with a 401. After some investigation, I found that we need to implement the httpAuthenticationHeaders method for each Credentials type. This is then used as part of the SR proxy implementation.

Implementing httpAuthenticationHeaders is simple enough for BasicCredentials (or ApiKeyAndSecret) since it's only a matter of base64 encoding the username:password string, however, for OAuthCredentials, this is likely much more complex given the breadth of configs available.

The remaining changes in the PR:

The SR proxy implementation now uses the cached SchemaRegistryClient instance to issue the request instead of constructing the full HTTP request ourselves.

Any additional details or context that should be provided?

I tested the OAuth credentials against Confluent Cloud by connecting to a Kafka cluster. Followed this guide to setup Okta as an OAuth provider.

Pull request checklist

Please check if your PR fulfills the following (if applicable):

Tests:
- Added new
- Updated existing
- Deleted existing
Have you validated this change locally against a running instance of the Quarkus dev server?
```
make quarkus-dev
```
Have you validated this change against a locally running native executable?
```
make mvn-package-native && ./target/ide-sidecar-*-runner
```

rohitsanj · 2025-01-13T17:25:26Z

src/main/java/io/confluent/idesidecar/restapi/clients/ClientConfigurator.java

-    // CCloud requires the logical cluster ID to be set in the properties, so examine the URL
-    var logicalId = CCloud.SchemaRegistryIdentifier
-        .parse(srUri)
-        .map(CCloud.LsrcId.class::cast)
-        .map(CCloud.LsrcId::id)
-        .orElse(null);
-
    // Add any properties for SR credentials (if defined)
    var options = connection
        .getSchemaRegistryOptions()
-        .withRedact(redact)
-        .withLogicalClusterId(logicalId);
+        .withRedact(redact);


The Confluent Cloud logical cluster id for a schema registry will need to be provided by the user as part of the credentials config in case of OAuth credentials. We cannot infer the "logical" cluster id from the Schema Registry URI which may or may not contain the cluster id in its first part. Moreover, a given physical cluster id may host multiple logical cluster ids.

rohitsanj · 2025-01-13T17:57:42Z

src/main/java/io/confluent/idesidecar/restapi/connections/DirectConnectionState.java

-      // and getting all subjects.
-      srClient.getAllSubjects();
+      // and getting all schema types
+      srClient.getSchemaTypes();


getSchemaTypes is much less expensive than a getAllSubjects call. This is kind of unrelated to the PR's changes.

flippingbits

Great work, @rohitsanj! I have a few minor comments. Otherwise, your PR looks good to me!

src/generated/resources/openapi.json

src/main/java/io/confluent/idesidecar/restapi/application/ProxyProcessorBeanProducers.java

flippingbits · 2025-01-13T21:00:39Z

src/main/java/io/confluent/idesidecar/restapi/credentials/OAuthCredentials.java

+  @Schema(description = "Additional property that can be added in the request header to identify "
+      + "the logical cluster ID to connect to. For example, this may be a Confluent Cloud " +
+      "Kafka or Schema Registry cluster ID.")
+  @JsonProperty(value = "logical_cluster_id")
+  @Null
+  String logicalClusterId,


Do you know if the logicalClusterId is needed for clusters other than CCloud ones? If not, what about modeling this in a bit more generic way so that we can maybe support other extension_s as well without having to introduce new fields to the OAuthCredentials?

Hm, yeah it is definitely CCloud specific, but so is the other identityPoolId field.

If not, what about modeling this in a bit more generic way so that we can maybe support other extension_s as well without having to introduce new fields to the OAuthCredentials?

Hm, would it be fine to revisit this at a later point? For now I imagine that we will have some client-side logic that will only surface these fields for direct connections to Confluent Cloud.

Thanks for the clarification! From my POV, it's fine to add these two fields now because we probably won't need to support any other custom JAAS configs in the near/mid term.

Hm, would it be fine to revisit this at a later point?

I don't think we can remove them at a later point as they'll be part of the API contract and Direct Connection form. Removing them would have an impact on existing Direction Connections of users and be a breaking change.

For now I imagine that we will have some client-side logic that will only surface these fields for direct connections to Confluent Cloud.

Maybe make it clear that these fields are only relevant for Direct Connections that target CCloud clusters by, for instance, adding the prefix ccloud_ to their names?

I don't think we can remove them at a later point as they'll be part of the API contract and Direct Connection form. Removing them would have an impact on existing Direction Connections of users and be a breaking change.

Makes sense. Thanks for mentioning.

adding the prefix ccloud_ to their names?

Prefixing ccloud_ sounds good. 👍

sonarqube-confluent · 2025-01-14T18:53:38Z

69.30% Coverage on New Code (is less than 80.00%)

Analysis Details

4 Issues

0 Bugs
0 Vulnerabilities
4 Code Smells

Coverage and Duplications

69.30% Coverage (80.20% Estimated after merge)
No duplication information (0.60% Estimated after merge)

Project ID: ide-sidecar

View in SonarQube

flippingbits

Thanks for the update, @rohitsanj! LGTM.

Fix OAuthCredentials

7d621ff

This comment has been minimized.

Sign in to view

rohitsanj added 2 commits January 10, 2025 11:06

Fix Oauth tests

1e257b2

Add openapi changes

6dc4b82

This comment has been minimized.

Sign in to view

rohitsanj added 2 commits January 10, 2025 17:40

Refactor Schema Registry proxy to use SR client

6976236

refactor

d8f2b7c

This comment has been minimized.

Sign in to view

rohitsanj added 2 commits January 12, 2025 18:48

Fix tests

302413d

Fix content type

df6ed23

This comment has been minimized.

Sign in to view

rohitsanj commented Jan 13, 2025

View reviewed changes

Add javadocs and clean up

48cb0a5

rohitsanj changed the title ~~Fix OAuthCredentials~~ Fix OAuthCredentials (and connecting to SR via direct connections) Jan 13, 2025

This comment has been minimized.

Sign in to view

rohitsanj marked this pull request as ready for review January 13, 2025 18:43

rohitsanj requested a review from a team as a code owner January 13, 2025 18:43

flippingbits reviewed Jan 13, 2025

View reviewed changes

rohitsanj added 2 commits January 13, 2025 13:27

Merge branch 'main' into fix-oauth-direct

60df7a1

fix typo

b868f93

This comment has been minimized.

Sign in to view

flippingbits mentioned this pull request Jan 14, 2025

Add OAuth 2.0 authN to direct connections #126

Closed

Prefix ccloud to the oauth extensions

bfd623c

flippingbits self-requested a review January 14, 2025 19:19

flippingbits approved these changes Jan 14, 2025

View reviewed changes

rohitsanj merged commit 85fca10 into main Jan 14, 2025
2 checks passed

rohitsanj deleted the fix-oauth-direct branch January 14, 2025 20:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix OAuthCredentials (and connecting to SR via direct connections) #290

Fix OAuthCredentials (and connecting to SR via direct connections) #290

rohitsanj commented Jan 9, 2025 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

rohitsanj Jan 13, 2025

rohitsanj Jan 13, 2025

This comment has been minimized.

flippingbits left a comment

flippingbits Jan 13, 2025

rohitsanj Jan 13, 2025

flippingbits Jan 14, 2025

rohitsanj Jan 14, 2025

This comment has been minimized.

sonarqube-confluent bot commented Jan 14, 2025

flippingbits left a comment

Fix OAuthCredentials (and connecting to SR via direct connections) #290

Fix OAuthCredentials (and connecting to SR via direct connections) #290

Conversation

rohitsanj commented Jan 9, 2025 • edited Loading

Summary of Changes

Any additional details or context that should be provided?

Pull request checklist

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

rohitsanj Jan 13, 2025

Choose a reason for hiding this comment

rohitsanj Jan 13, 2025

Choose a reason for hiding this comment

This comment has been minimized.

flippingbits left a comment

Choose a reason for hiding this comment

flippingbits Jan 13, 2025

Choose a reason for hiding this comment

rohitsanj Jan 13, 2025

Choose a reason for hiding this comment

flippingbits Jan 14, 2025

Choose a reason for hiding this comment

rohitsanj Jan 14, 2025

Choose a reason for hiding this comment

This comment has been minimized.

sonarqube-confluent bot commented Jan 14, 2025

Analysis Details

4 Issues

Coverage and Duplications

flippingbits left a comment

Choose a reason for hiding this comment

rohitsanj commented Jan 9, 2025 •

edited

Loading