-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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 support for service registry service binding #21618
Conversation
Thanks for your pull request! The title of your pull request does not follow our editorial rules. Could you have a look?
|
@wtrocki @cescoffier @carlesarnal Here is the Binding extension for service registry. Proceed to comment. |
...time/src/main/java/io/quarkus/apicurio/registry/binding/ServiceRegistryBindingConverter.java
Outdated
Show resolved
Hide resolved
...time/src/main/java/io/quarkus/apicurio/registry/binding/ServiceRegistryBindingConverter.java
Outdated
Show resolved
Hide resolved
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 0937c8f
Failures⚙️ Initial JDK 11 Build #- Failing: extensions/apicurio-registry-avro/runtime
! Skipped: devtools/bom-descriptor-json docs extensions/apicurio-registry-avro/deployment and 8 more 📦 extensions/apicurio-registry-avro/runtime✖ |
0937c8f
to
edc2559
Compare
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building edc2559
Failures⚙️ Initial JDK 11 Build #- Failing: extensions/apicurio-registry-avro/runtime
! Skipped: devtools/bom-descriptor-json docs extensions/apicurio-registry-avro/deployment and 8 more 📦 extensions/apicurio-registry-avro/runtime✖ |
...time/src/main/java/io/quarkus/apicurio/registry/binding/ServiceRegistryBindingConverter.java
Outdated
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.
LGTM. I just think that maybe we should replace the combination of realm + auth URL by the token endpoint.
edc2559
to
1e1e5ee
Compare
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 1e1e5ee
Failures⚙️ Initial JDK 11 Build #- Failing: extensions/apicurio-registry-avro/runtime
! Skipped: devtools/bom-descriptor-json docs extensions/apicurio-registry-avro/deployment and 8 more 📦 extensions/apicurio-registry-avro/runtime✖ |
1e1e5ee
to
a1f0237
Compare
BuildProducer<ServiceProviderBuildItem> serviceProvider) { | ||
if (capabilities.isPresent(Capability.KUBERNETES_SERVICE_BINDING)) { | ||
serviceProvider.produce( | ||
new ServiceProviderBuildItem("io.quarkus.apicurio.registry.binding.ServiceRegistryBindingConverter", |
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.
Not related to that PR, but maybe we should have an SPI in the service binding extension with a dedicated build item (which will also about the capability check)
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 am not sure what you mean here. Can you elaborate a bit please?
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.
Imagine that instead to do:
if (capabilities.isPresent(Capability.KUBERNETES_SERVICE_BINDING)) {
serviceProvider.produce(
new ServiceProviderBuildItem("io.quarkus.apicurio.registry.binding.ServiceRegistryBindingConverter", ServiceRegistryBindingConverter.class.getName()));
You do:
producer.produce(new ServiceBindingConverter(ServiceRegistryBindingConverter.class));
Then the SBO processor would consumer this new build item and register the SPI provider.
Of course to avoid a dependency on the SBO extension deployment model, we would need to create an SPI module.
registryUrl = binding.getProperties().get("registryurl"); | ||
} | ||
if (registryUrl != null) { | ||
properties.put("mp.messaging.connector.smallrye-kafka.apicurio.registry.url", registryUrl); |
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 would use "kafka." instead of "mp.messaging.connector.smallrye-kafka."
So, bare clients using Kafka default config will have the apicurio URL configured too.
...time/src/main/java/io/quarkus/apicurio/registry/binding/ServiceRegistryBindingConverter.java
Show resolved
Hide resolved
Can you please squash the commits? |
5726377
to
cc74e12
Compare
I've implemented some feedback you all gave and have rerequested reviews. As an additional note : this works with native clients now, but it didn't before. The image is at |
Ok, it seems like the binding works, but whatever I did to inject the http client causes vertx's connections to drop and not reconnect. I'll roll that back on Monday, but in the meantime if anyone sees anything that might cause that please comment. |
...eployment/src/main/java/io/quarkus/apicurio/registry/avro/ApicurioRegistryAvroProcessor.java
Outdated
Show resolved
Hide resolved
Looks like all that's left for this to be ready is to use the Vert.x client, right? |
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.
It needs to use the vertx client.
@geoand Sadly no, when I've updated to use the vertx client I get different errors. For some reason the spi won't use the vertx client and is insisting on the jdk client.
|
That doesn't seem to be a Quarkus specific issue |
@secondsun check if you don't have a META-INF/services file declaring the wrong client. |
@cescoffier I don't have a META-INF/services file; I think it is coming in from a dependency. I will double check though. |
Even in the artifact providing OidcAuth? Look how the lookup is done and why it uses the wrong client. |
@secondsun this seems like a bug in the library providing OidcAuth, I'll take a look as soon as I can. |
@cescoffier I'll dig into that. @carlesarnal Do you have some time to take a look with me? You've helped with issues in the past and I think you're more familiar with this dependency. |
I can help, but I don't know Apicurio code at all. @carlesarnal has a lot more knowledge on that code. |
I think there's a bug in the underlying library but haven't had the time to go through it yet. I'll try to take a look tomorrow. |
Thanks guys, I'm taking a look today and will let you guys know what I find / send prs where I need to if I get lucky |
This code is not correct, it references the wrong client. Can you try using the Vert.x one? |
@cescoffier Above I've attatched a stack trace using the vertx client as you've suggested, I just hadn't committed it because it hasn't worked. |
cc74e12
to
d171f13
Compare
I've worked with @carlesarnal and he's fixed the underlying bug in the registry. I've tested his snapshot with this PR and it fixes all of my issues as well. When central catches up with things we can bump the version of the registry and this should be good to merge. Thanks guys. |
d171f13
to
5be0e25
Compare
@carlesarnal I saw the new registry was released, does that mean this is ready to merge? |
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.
LGTM, let's see what @carlesarnal and @geoand say.
<dependency> | ||
<groupId>org.apache.commons</groupId> | ||
<artifactId>commons-lang3</artifactId> | ||
</dependency> |
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.
Is this now needed for some reason?
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.
Yes. We use it to simplify parsing here : https://github.com/quarkusio/quarkus/pull/21618/files#diff-a5c8e493cddd3831ed7120b5c27d8f03412a724cb8ca09f0828a08d2c752012bR88
No, unfortunately, I was on PTO and we missed that release, I'll try to get the fix into the next one. I'll ping you once we do a release so we can update this. |
5be0e25
to
7ce16d2
Compare
@carlesarnal the registry has been updated and this can be merged but right? Cc @cescoffier |
LGTM, @geoand any comment before we click on the big red button? |
@cescoffier It looks like there is an outstanding change in your last review, can you clear that? |
Thanks! |
Sorry, PTO time for me. Yes, it has been updated and this should work just fine (I guess you already checked it). |
This pull request adds to the apicurio registry avro extension the ability to consume service bindings injected by the service binding operator. This work is being done as part of adding service registry binding to the rhoas operator and cli.
The SBO provides the following bindings
apicurio.auth.token.endpoint
property +apicurio.auth.client.id
property+apicurio.auth.realm
property+apicurio.registry.url
andmp.messaging.connector.smallrye-kafka.apicurio.registry.url
properties+== How to Test
=== Prerequisites
rhoas
Clioc
cli=== Cluster and Services Setup
Ensure that you have logged into your cluster with oc, and have logged into your Application Services account with
rhoas login
. Create a kafka instance withrhoas kafka create
, and a service registry instance withrhoas service-registry create
. Once your kafka and service registry entries are ready, create a kafka topic called movies. You can usehroas kafka topic create
. You will then executerhoas cluster connect
twice - connecting the kafka instance and your service registry instance separately with each run. When you connect the kafka instance, you will be prompted to update your kafka ACLs and provided a command to do so using a service account that was created. It is important you do this.We have provided a sample application(https://github.com/secondsun/kafka-avro-schema-quickstart/). This application is deployed to OpenShift as a container. One you have deployed the container, use
rhoas cluster bind
to first bind the kafka connection, then execute the command again to bind the service registry. Once the pod has restartted, use the route provided by openshift to navigate to/movies.html
. You should see a screen showing a random movie title.There is a version of the application already containerized with the extension here : quuay.io/secondsun/kafka-avro-schema-quickstart:latest
A video of the process is here :
https://www.youtube.com/watch?v=ul9JDjXLquw