Skip to content
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

Client Side Tracing & OAuth implementation #66

Merged
merged 19 commits into from
Mar 15, 2024
Merged

Client Side Tracing & OAuth implementation #66

merged 19 commits into from
Mar 15, 2024

Conversation

SravanThotakura05
Copy link
Collaborator

@SravanThotakura05 SravanThotakura05 commented Mar 12, 2024

Implemented Client Side tracing & OAuth

  1. Client Side Tracing
    a. Trace messages are now generated for messages consumed and published within the extension
    b. Context Propagation for existing trace ID will be included in next release

  2. OAuth
    a. Implemented Client Credential flow
    b. Users now configure refresh interval to fetch access token before expiry
    c. Refer to https://quarkus.io/guides/security-openid-connect-client-reference for supported configuration

  3. Liveness mode is now set to true by default to handle pod restart in Kubernetes. Earlier liveness is set to true when messages are received from queue. This is causing the pod to restart since liveness is not set true within specified threshold

@SravanThotakura05 SravanThotakura05 marked this pull request as ready for review March 12, 2024 09:49
void startup(@Observes StartupEvent event) {
Multi.createFrom().ticks().every(duration)
.emitOn(Infrastructure.getDefaultWorkerPool())
// .filter(aLong -> {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented it as we will not receive refresh token in client_credentials OAuth flow


Function<SyntheticCreationalContext<MessagingService>, MessagingService> function = recorder.init(config, shutdown);

additionalBeanBuildItemBuildProducer.produce(AdditionalBeanBuildItem.unremovableOf(OidcProvider.class));
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ozangunalp is there a way we can produce this bean only if oidc properties are configured by user. I see some warnings/errors thrown by oidc-client library when no configuration is available(in case of basic authentication or any other type of authentication). This is not blocking message flow.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the easier option is to make it do nothing when oauth authentication is not configured.

@SravanThotakura05 SravanThotakura05 changed the title Distributed Tracing & OAuth implementation Client Side Tracing & OAuth implementation Mar 12, 2024
Copy link
Collaborator

@ozangunalp ozangunalp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For OAuth:
@SravanThotakura05 Overall we need a Quarkus integration test to properly check the token retrieval and refresh. I've made a try here:
https://github.com/ozangunalp/solace-quarkus/pull/new/tracing

For Tracing if there is a "from app to broker to app" propagation test with proper parent traces, it's ok for me.

@SravanThotakura05
Copy link
Collaborator Author

SravanThotakura05 commented Mar 13, 2024

5fa5d97

For OAuth: @SravanThotakura05 Overall we need a Quarkus integration test to properly check the token retrieval and refresh. I've made a try here: https://github.com/ozangunalp/solace-quarkus/pull/new/tracing

For Tracing if there is a "from app to broker to app" propagation test with proper parent traces, it's ok for me.

@ozangunalp i have added your test and it runs fine with initial access token. I have made a small change where we need to update access token only if solace service is connected otherwise messaging service will thrown an exception. For the same reason the access token is not refreshed in tests(Thread.sleep(5000)) since Mutiny encountered this exception.
Now, i see access token is refreshed for every 5s in logs. Link to commit - 5fa5d97

Regarding tracing tests, this file includes a processor test in which app publishes messages to broker and the same is consumed by the application. Do we need to enhance it further?

https://github.com/SolaceLabs/solace-quarkus/pull/66/files#diff-29b18880764c5cb3347ba37608419a1caa8ba605d69bbf9408daa37861aba000

@ozangunalp
Copy link
Collaborator

ozangunalp commented Mar 14, 2024

@ozangunalp i have added your test and it runs fine with initial access token. I have made a small change where we need to update access token only if solace service is connected otherwise messaging service will thrown an exception. For the same reason the access token is not refreshed in tests(Thread.sleep(5000)) since Mutiny encountered this exception.

Oh, I understand what you mean now. It is ok for me.

Token refresh needs more tests in general but I think it is good for a first version. It is already great that we have an end-to-end test for this with Keycloak etc.

@SravanThotakura05 SravanThotakura05 merged commit f1e53a9 into main Mar 15, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OAuth implementation Integrate client side OpenTelemetry tracing in Incoming and Outgoing channels
2 participants