-
-
Notifications
You must be signed in to change notification settings - Fork 151
BE: SR: Add compatibility w/ GCP SR #1153
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
base: main
Are you sure you want to change the base?
BE: SR: Add compatibility w/ GCP SR #1153
Conversation
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.
Hi magnusdriver! 👋
Welcome, and thank you for opening your first PR in the repo!
Please wait for triaging by our maintainers.
Please take a look at our contributing guide.
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 your contribution! I believe it would be more effective to implement this in a more generic manner by exposing the BEARER_SCOPE and BEARER_AUTH_CUSTOM_PROVIDER_CLASS parameters through SchemaRegistryAuth. Don't you mind to change it?
api/src/main/java/io/kafbat/ui/serdes/builtin/sr/MessageFormatter.java
Outdated
Show resolved
Hide resolved
I finally exposed the BEARER_AUTH_CUSTOM_PROVIDER_CLASS parameter. I didn't need the bearer scope for this use case. If you think it's still interesting to expose that parameter I'll add it. Maybe pass it to WebClientConfigurator.configureBearerTokenAuth as parameter for future use cases? |
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.
Left a few inline comments. Once we decide on a higher level of abstractions, we can dive deep into reviewing the concrete implementations. Right now, it's a nailed-down solution not taking future maintainability (or possible expansion towards other implementations) into account.
@@ -381,8 +381,13 @@ components: | |||
properties: | |||
compatibilityLevel: | |||
$ref: '#/components/schemas/Compatibility' | |||
required: |
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.
Why has compatibility become no longer required?
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'm trying to change it
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 finally added a new compatibilityConfigGcp schema detached from the existent compatibilityConfig. And I had to modify code in SchemaRegistryService class.
If this new modification is not ok then please let me know if you have any recommendation about how to address this as the only alternative I can think of is to add a new kafka-gcp-sr-api and a new GcpSchemaRegistryService class.
@@ -166,6 +178,11 @@ private static SchemaRegistryClient createSchemaRegistryClient(List<String> urls | |||
keyStorePassword); | |||
} | |||
|
|||
if (bearerAuthCustomProviderClass != null) { |
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.
Why is this called a class
? The implementation rather retrieves a token via custom implementation from GCP which is later used as a bearer token value, there are no "classes" in use per se.
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 use this to reference this class. And I changed the bearer token implementation for the WebClient using this class too. Sorry 🙏 .
What changes did you make? (Give an overview)
Added new cluster property "schemaRegistryAuth.bearerAuthCustomProviderClass" to enable compatibility with new GCP Schema Registries.
This property can be used in config.yaml file like this:
Or can be enabled with the env variable:
KAFKA_CLUSTERS_0_SCHEMA_REGISTRY_AUTH_BEARER_AUTH_CUSTOM_PROVIDER_CLASS="com.google.cloud.hosted.kafka.auth.GcpBearerAuthCredentialProvider"
or
KAFKA_CLUSTERS_0_SCHEMAREGISTRYAUTH_BEARERAUTHCUSTOMPROVIDERCLASS
is also valid.Important changes in code are:
bearerAuthCustomProviderClass
config property in kafbat-ui-apiproperty in SchemaRegistryService
It's needed to use
gcloud auth application-default login
to get the credentials to connect to GCP SchemaRegistries or run kafka-ui in a compute engine instance or GKE with a service account with the needed permissions.
Note: This functionality only works now with Avro schema types as these are the ones I work with.
It could work with Protobuf schemas, but not with JSON ones as GCP Schema Registries don't
support them.
How Has This Been Tested? (put an "x" (case-sensitive!) next to an item)
Manually (please, describe, if necessary)
Changes tested connecting to GCP and Confluent Schema Registries to check:
It was impossible to pass unit tests even before adding any change :'(
Checklist (put an "x" (case-sensitive!) next to all the items, otherwise the build will fail)
Check out Contributing and Code of Conduct