-
Notifications
You must be signed in to change notification settings - Fork 3
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
Initial limited support for direct connections #116
Conversation
Fixes #96 This adds the initial support for a new type of connection resources, called `DIRECT` connections, that represent a local or remote Kafka cluster and/or Schema Registry. The extension might create a direct connection to talk to a locally running CP installation, or a remote Apache Kafka cluster with a remote Schema Registry, or even a CCloud cluster with Schema Registry. The user may have to provide credentials to successfully authenticate and connect to each remote service/cluster. The direction connections are not necessarily the constructs we want to expose. It is likely that near-term versions of the VS Code extension will allow users to connect to local or remote Kafka and SR clusters, but it may also expose to users different kinds of connections that are all represented in the sidecar as direct connections. For example, maybe the VS Code extension allows users to connect to “Confluent Platform”, “Apache Kafka”, and “WarpStream” (in addition to CCloud and local AK+SR), and these might all be customized facades on top of direct connections. This is the first step in full support for direct connections, and as such only limited functionality is available in this PR: * CRUDL connection resources with `type=DIRECT`, with validation of fields for each type of connection. * A direct connection contains zero or one `kafka_cluster` object and zero or one `schema_registry` objects, allowing it to connect to a (remote or local) Kafka cluster and/or a (remote or local) Schema Registry. * GraphQL support for the Kafka cluster and SR referenced by direct connections. The following changes will be made to this PR before it is to be merged, likely through subsequent PRs based on this PR: # Add basic credentials to the `kafka_cluster` and `schema_registry` definitions, and use this to connect to downstream resources. All secrets in credentials will be write-only: the Connection REST API endpoins must always mask secrets. # Update direct connection status by showing authn success against Kafka cluster and/or SR. This may mean we evolve the current `status` section of the connection resources to better work with non-CCloud connections, where connectivity to Kafka cluster and SR are independent. # Support using direction connections in Kafka REST API and Message Consumer API endpoints # Support using direction connections in SR API endpoints # Local connections can define SR URL via same `schema_registry` field as direct, superseding the existing `local_config`, which will still be supported for a while so that the extension can migrate. A local connection is invalid if it has both `local_config` and `schema_registry` objects. A few enhancements may come after this initial functionality is merged: # Add an `PLATFORM` (or `MDS`) connection type that connects to a local or remote MDS and dynamically discovers the registered clusters. # Potentially add custom client properties to `kafka_cluster` and `schema_registry`, allowing users to override some client connection properties. # Improved validation logic for resource types, potentially using javax.validation. # Allow direct connections to connect to Flink cluster and Kafka Connect cluster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great start, thank you @rhauch! I have some minor comments/questions and the rest are minor suggestions for fixes.
src/main/java/io/confluent/idesidecar/restapi/models/graph/DirectKafkaCluster.java
Outdated
Show resolved
Hide resolved
src/main/java/io/confluent/idesidecar/restapi/models/graph/DirectKafkaCluster.java
Outdated
Show resolved
Hide resolved
src/main/java/io/confluent/idesidecar/restapi/models/graph/DirectKafkaCluster.java
Outdated
Show resolved
Hide resolved
src/main/java/io/confluent/idesidecar/restapi/models/graph/DirectKafkaCluster.java
Outdated
Show resolved
Hide resolved
src/main/java/io/confluent/idesidecar/restapi/models/graph/RealDirectFetcher.java
Outdated
Show resolved
Hide resolved
src/test/java/io/confluent/idesidecar/restapi/resources/ConnectionsResourceTest.java
Outdated
Show resolved
Hide resolved
src/test/java/io/confluent/idesidecar/restapi/resources/ConnectionsResourceTest.java
Show resolved
Hide resolved
src/main/java/io/confluent/idesidecar/restapi/models/ConnectionSpec.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, @rhauch! I have only a few minor comments/suggestions.
src/main/java/io/confluent/idesidecar/restapi/models/ConnectionSpec.java
Outdated
Show resolved
Hide resolved
src/main/java/io/confluent/idesidecar/restapi/models/ConnectionSpec.java
Outdated
Show resolved
Hide resolved
} | ||
} | ||
|
||
void isNotAllowed(List<Error> errors, String path, String what, String when) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got used to using names that start with is
for methods that return a boolean value. What about renaming this method to something like addNotAllowedError
or similar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed all of the methods to check*(...)
, which I think is a decent combination of readable that avoids name patterns that are used for getters.
src/main/java/io/confluent/idesidecar/restapi/models/ConnectionSpec.java
Outdated
Show resolved
Hide resolved
The primary reason to transition to Jakarta APIs from `javax` is due to the transfer of Java EE to the Eclipse Foundation, where it became Jakarta EE. The older `javax` packages within Java EE have been deprecated. Jakarta EE uses the `jakarta` namespace, which is incompatible with the legacy javax namespace. Mixing the two can cause issues. Several projects and frameworks… have already adopted Jakarta. Additionally, upgrading to Jakarta allows you to keep up with the latest changes and modernize your applications. https://github.com/jakartaee/platform/blob/main/namespace/mappings.adoc#mapping-javax-to-jakarta
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! LGTM 🙌
Waiting for final approval from @flippingbits, since they had some questions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update. LGTM, @rhauch!
#112 was recently merged. Locally, I did the following to verify
All is good, and I did some basic testing with the native image, confirming that the Connections API works as expected for local, CCloud and the new direct connections. The local and CCloud connections should not be affected by this PR build, other than validating create and update of connection resources, which have good coverage with new tests. Based upon this, I'm merging this PR. |
Summary of Changes
Resolves #96
This adds the initial but limited support for a new type of connection resources, called
DIRECT
connections, that represent a local or remote Kafka cluster and/or Schema Registry. The extension might create a direct connection to talk to a locally running CP installation, or a remote Apache Kafka cluster with a remote Schema Registry, or even a CCloud cluster with Schema Registry. The user may have to provide credentials to successfully authenticate and connect to each remote service/cluster.The direction connections are not necessarily the constructs we want to expose. It is likely that near-term versions of the VS Code extension will allow users to connect to local or remote Kafka and SR clusters, but it may also expose to users different kinds of connections that are all represented in the sidecar as direct connections. For example, maybe the VS Code extension allows users to connect to “Confluent Platform”, “Apache Kafka”, and “WarpStream” (in addition to CCloud and local AK+SR), and these might all be customized facades on top of direct connections.
This is the first step in full support for direct connections, and as such only limited functionality is available in this PR:
type=DIRECT
, with validation of fields for each type of connection.kafka_cluster
object and zero or oneschema_registry
objects, allowing it to connect to a (remote or local) Kafka cluster and/or a (remote or local) Schema Registry.Once this PR is merged, the following will handled as followups to complete the support for direct connections:
status
to work for local, CCloud and direct connections #123After that, we will have more improvements to better support Confluent Platform:
Other improvements may include:
local_config
to sameschema_registry
used in direct connections #130Also, we avoided using
javax
and instead usedjakarta
imports due to the transfer of Java EE to the Eclipse Foundation, where it became Jakarta EE. The olderjavax
packages within Java EE have been deprecated. Jakarta EE uses thejakarta
namespace, which is incompatible with the legacy javax namespace. Mixing the two can cause issues. Several projects and frameworks… have already adopted Jakarta. Additionally, upgrading to Jakarta allows you to keep up with the latest changes and modernize your applications.https://github.com/jakartaee/platform/blob/main/namespace/mappings.adoc#mapping-javax-to-jakarta
Pull request checklist
Please check if your PR fulfills the following (if applicable):