Implement support for TLS config, OAuth credentials and setup CP integration tests #201

rohitsanj · 2024-12-03T01:56:34Z

Summary of Changes

Add cp-demo testcontainers, along with a script to set it up from scratch -- works locally and on CI.
Added make targets to start/stop cp-demo.
Added make targets to store/load the Docker images into/from Semaphore cache. (I noticed we weren't caching confluent-local, so I've added that too.)
Bring @rhauch's implementation of mTLS and OAuth credentials from Add mTLS and OAuth 2.0 support for direct connections to Kafka and SR #211 and tweaked as follows. Mutual TLS is no longer a "credentials" type, but rather a sub-field called ssl under ConnectionSpec.KafkaClusterConfig as well as ConnectionSpec.SchemaRegistryConfig. The ssl config may be set along with any credentials type (basic, apikey/secret, oauth) and can even be set in the absence of creds.
The field ssl holds all configs related to TLS. (h/t @flippingbits for this section)
- It can be used to disable the usage of TLS via the boolean field enabled, in which case the sidecar uses an unencrypted but potentially authenticated channel to interact with the Kafka cluster.
- It accepts a keystore via the field keystore, a truststore via the field truststore, and allows to disable hostname verification via the boolean field verify_hostname.
- If ssl is set to null, the sidecar uses an encrypted channel to interact with the Kafka cluster, verifies the hostname, and does not take any custom keystore/truststore into account.

Any additional details or context that should be provided?

Follow up items:

Add a new Credentials type called MutualTLSCredentials starting with a single field such as derived that is always set to true. The SSL configuration for mTLS will still need to be passed in the kafka_cluster.ssl (or schema_registry.ssl) object. -- Add MutualTLSCredentials as a placeholder Credentials type #237
The OAuth credentials type is untested, we can add integration tests for this in a follow up PR. -- Add integration tests for OAuth credentials against cp-demo #238
~~Fix pom.xml configuration to ensure we run integration tests just once, not twice.~~ (Fixed in Fix coverage reporting and push to Sonarqube #224)
Replace ZK with KRaft, start just one Kafka broker -- Modernize CPDemoTestEnvironment to use KRaft instead of to-be-deprecated ZK #239

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
```

confluent-cla-assistant · 2024-12-03T01:56:43Z

🎉 All Contributor License Agreements have been signed. Ready to merge.
_{Please push an empty commit if you would like to re-run the checks to verify CLA status for all contributors.}

src/test/java/io/confluent/idesidecar/restapi/util/CPDemoTestEnvironment.java

Resolves #125 #126 Adds preliminary support to allow Kafka and SR clients to use mTLS and OAuth 2.0 to authenticate for direct connections. These changes have been unit tested to verify the expected client configurations are generated. However, those expected configurations have NOT yet been tested against CCloud or CP via integration tests.

…s' into rohitsanj/int-test-cp # Conflicts: # src/generated/resources/openapi.yaml # src/main/java/io/confluent/idesidecar/restapi/credentials/MutualTLSCredentials.java

# Conflicts: # src/main/java/io/confluent/idesidecar/restapi/messageviewer/MessageViewerContext.java # src/main/java/io/confluent/idesidecar/restapi/proxy/ProxyContext.java

rhauch

Thanks, @rohitsanj. I took a quick first pass, and wanted to share these questions/suggestions sooner than later.

src/main/java/io/confluent/idesidecar/restapi/models/ConnectionSpec.java

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

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

src/main/resources/application.yml

src/test/java/io/confluent/idesidecar/restapi/cache/ClientConfiguratorStaticTest.java

src/main/java/io/confluent/idesidecar/restapi/proxy/clusters/ClusterProxyContext.java

rohitsanj

Some notes

src/main/java/io/confluent/idesidecar/restapi/proxy/ProxyHttpClient.java

src/main/java/io/confluent/idesidecar/restapi/proxy/ProxyRequestProcessor.java

...java/io/confluent/idesidecar/restapi/proxy/clusters/processors/ClusterStrategyProcessor.java

src/test/java/io/confluent/idesidecar/restapi/util/SidecarClient.java

src/test/java/io/confluent/idesidecar/restapi/util/cpdemo/Constants.java

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

…ection cache

src/main/java/io/confluent/idesidecar/restapi/proxy/ConnectionProcessor.java

flippingbits

Thanks for the updates, @rohitsanj! 🎉 I have a few questions and comments. Otherwise, your PR looks good to me. I feel you're very close to being able to merge it into main.

src/generated/resources/openapi.json

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

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

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

src/test/java/io/confluent/idesidecar/restapi/kafkarest/RecordsV3ErrorsSuite.java

src/test/java/io/confluent/idesidecar/restapi/kafkarest/RecordsV3WithoutSRSuite.java

src/test/java/io/confluent/idesidecar/restapi/resources/ConnectionsResourceTest.java

flippingbits · 2024-12-19T11:39:39Z

src/test/java/io/confluent/idesidecar/restapi/resources/ConnectionsResourceTest.java

+                .withDetail("Kafka cluster truststore path is required and may not be blank")
+        ),
+        new TestInput(
+            "Direct spec is invalid with SSL having keystore only",


Can't we rely on the built-in/system certs in the absence of the truststore?
AFAIK, that's how you'd use mTLS with, for instance, Confluent Cloud.

Sounds good -- I'd imagine the built-in/system certs will be picked up automatically by the Java runtime without us having to configure anything.

Or would we need to use the stored Preferences to add any tls_pem_paths to the trust store?

We should definitely test if that works with the native executable.

Sounds good -- I've already created #235 for supporting mTLS against Confluent Cloud, so testing would be taken care of as well.

For now, I've changed the constraints to allow passing keystore options without truststore.

rohitsanj · 2024-12-19T21:53:04Z

@flippingbits Thanks so much for the review! I've made some updates based on our offline conversation.

rohitsanj · 2024-12-19T22:06:56Z

Seems there's quite a few branches in the ConnectionSpec validation logic that have not been covered with tests. Given how large this PR has gotten, we can add tests in a follow up PR -- #241

rohitsanj · 2024-12-19T22:11:26Z

The two bugs reported by Sonarqube were false positives and I've resolved them with comments.

flippingbits

Thanks for the update, @rohitsanj! This looks great. I have two more very minor comments. Otherwise, your PR looks good to me.

src/generated/resources/openapi.json

flippingbits

Thanks for the great work, @rohitsanj! LGTM, pending the results of the CI pipeline.

rohitsanj · 2024-12-19T23:07:48Z

For those following: @rhauch is fine with us merging this.

sonarqube-confluent · 2024-12-19T23:34:01Z

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

Analysis Details

8 Issues

0 Bugs
0 Vulnerabilities
8 Code Smells

Coverage and Duplications

57.90% Coverage (80.10% Estimated after merge)
No duplication information (0.60% Estimated after merge)

Project ID: ide-sidecar

View in SonarQube

flippingbits reviewed Dec 3, 2024

View reviewed changes

src/test/java/io/confluent/idesidecar/restapi/util/CPDemoTestEnvironment.java Show resolved Hide resolved

rhauch mentioned this pull request Dec 5, 2024

Add initial partial support for connecting to Confluent Platform MDS #205

Closed

5 tasks

rhauch and others added 15 commits December 7, 2024 10:10

Add cp-demo testcontainer classes

a6c8861

Make some progress

5f0a651

Works with schemaless records tests (finally)

95371d9

Tweak healthchecks

1ab7070

Allow reusing containers across test runs

f7377d0

wip stuff

30ccfb9

wip

58e6c16

Merge remote-tracking branch 'origin/mtls-oauth-for-direct-connection…

e0f8ee6

…s' into rohitsanj/int-test-cp # Conflicts: # src/generated/resources/openapi.yaml # src/main/java/io/confluent/idesidecar/restapi/credentials/MutualTLSCredentials.java

more wip

d09b03e

more wip

2640bd4

Merge branch 'refs/heads/main' into rohitsanj/int-test-cp

14624a9

# Conflicts: # src/main/java/io/confluent/idesidecar/restapi/messageviewer/MessageViewerContext.java # src/main/java/io/confluent/idesidecar/restapi/proxy/ProxyContext.java

Setup tests for basic auth and mTLS

e706a00

add tests

6bfc205

airlock-confluentinc bot force-pushed the rohitsanj/int-test-cp branch from 2146156 to 6bfc205 Compare December 10, 2024 21:53

rhauch reviewed Dec 10, 2024

View reviewed changes

rohitsanj commented Dec 10, 2024

View reviewed changes

rohitsanj added 5 commits December 11, 2024 13:22

address feedback and add tests

1b0e9cf

add connection spec tests

cf7e76c

Add scripts and mechanisms to start/stop/cleanup cp-demo

b08790d

Shutdown cp-demo if running on CI

e701bc6

Fix all containers running check

b66844f

rohitsanj changed the title ~~[WIP] Add cp-demo testcontainer classes~~ Add cp-demo testcontainer classes Dec 12, 2024

rohitsanj added 3 commits December 11, 2024 19:52

Cache docker images and add TestLifecycle annotation to LocalIT

ce6133a

only run docker load if file exists

4ef926a

Use fully qualified name to avoid name conflicts in AbstractIT's conn…

a9ead3d

…ection cache

This comment has been minimized.

Sign in to view

rohitsanj commented Dec 19, 2024

View reviewed changes

src/main/java/io/confluent/idesidecar/restapi/proxy/ConnectionProcessor.java Outdated Show resolved Hide resolved

try combine.self="override"

d2912a7

This comment has been minimized.

Sign in to view

set id

bdcc3cd

This comment has been minimized.

Sign in to view

flippingbits reviewed Dec 19, 2024

View reviewed changes

Merge branch 'main' into rohitsanj/int-test-cp

252bd0f

rohitsanj added 2 commits December 19, 2024 13:39

Address reviewer feedback

616f8dc

Merge branch 'main' into rohitsanj/int-test-cp

41eda6a

rohitsanj requested a review from flippingbits December 19, 2024 21:49

This comment has been minimized.

Sign in to view

Harden shouldThrowNotImplementedForUnsupportedSchemaDetails

70d25a7

flippingbits reviewed Dec 19, 2024

View reviewed changes

src/generated/resources/openapi.json Outdated Show resolved Hide resolved

src/generated/resources/openapi.json Outdated Show resolved Hide resolved

fix extra space and remove unnecessary changes from openapi files

1b41998

rohitsanj requested a review from flippingbits December 19, 2024 22:43

flippingbits approved these changes Dec 19, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

Merge branch 'main' into rohitsanj/int-test-cp

45f83b3

rohitsanj merged commit 0ed73d1 into main Dec 19, 2024
2 checks passed

rohitsanj deleted the rohitsanj/int-test-cp branch December 19, 2024 23:57

rohitsanj mentioned this pull request Jan 2, 2025

Add integration tests for direct connections #161

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement support for TLS config, OAuth credentials and setup CP integration tests #201

Implement support for TLS config, OAuth credentials and setup CP integration tests #201

rohitsanj commented Dec 3, 2024 •

edited

Loading

confluent-cla-assistant bot commented Dec 3, 2024

rhauch left a comment

rohitsanj left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

flippingbits left a comment

flippingbits Dec 19, 2024

rohitsanj Dec 19, 2024 •

edited

Loading

flippingbits Dec 19, 2024

rohitsanj Dec 19, 2024 •

edited

Loading

rohitsanj Dec 19, 2024

rohitsanj commented Dec 19, 2024

rohitsanj commented Dec 19, 2024

This comment has been minimized.

rohitsanj commented Dec 19, 2024

flippingbits left a comment

flippingbits left a comment

This comment has been minimized.

rohitsanj commented Dec 19, 2024

sonarqube-confluent bot commented Dec 19, 2024

Implement support for TLS config, OAuth credentials and setup CP integration tests #201

Implement support for TLS config, OAuth credentials and setup CP integration tests #201

Conversation

rohitsanj commented Dec 3, 2024 • edited Loading

Summary of Changes

Any additional details or context that should be provided?

Pull request checklist

confluent-cla-assistant bot commented Dec 3, 2024

rhauch left a comment

Choose a reason for hiding this comment

rohitsanj left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

flippingbits left a comment

Choose a reason for hiding this comment

flippingbits Dec 19, 2024

Choose a reason for hiding this comment

rohitsanj Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

flippingbits Dec 19, 2024

Choose a reason for hiding this comment

rohitsanj Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

rohitsanj Dec 19, 2024

Choose a reason for hiding this comment

rohitsanj commented Dec 19, 2024

rohitsanj commented Dec 19, 2024

This comment has been minimized.

rohitsanj commented Dec 19, 2024

flippingbits left a comment

Choose a reason for hiding this comment

flippingbits left a comment

Choose a reason for hiding this comment

This comment has been minimized.

rohitsanj commented Dec 19, 2024

sonarqube-confluent bot commented Dec 19, 2024

Analysis Details

8 Issues

Coverage and Duplications

rohitsanj commented Dec 3, 2024 •

edited

Loading

rohitsanj Dec 19, 2024 •

edited

Loading

rohitsanj Dec 19, 2024 •

edited

Loading