Skip to content

Add possibility to override LRAClient & LRARoutes#10498

Closed
johbossle wants to merge 2 commits into
apache:camel-3.xfrom
johbossle:camel-3.x
Closed

Add possibility to override LRAClient & LRARoutes#10498
johbossle wants to merge 2 commits into
apache:camel-3.xfrom
johbossle:camel-3.x

Conversation

@johbossle
Copy link
Copy Markdown
Contributor

Description

The Saga EIP pattern is using rest based communication for interaction with the lra-coordinator (e.g. narayana). Although there are ways to secure the communication with the coordinator, this is not supported by the underlying code. Also, it is not possible to easily override some implementations, since they are created by intenral constructors.

This change is targeting, that one can override (influence) the creation of the LRAClient and LRASagaRoutes in the LRASagaService. With those "extensions", it is then possible to override some default behavior and for example you can set the HttpClient and use your own adjusted instance with an Authenticator.

Target

  • I checked that the commit is targeting the correct branch (note that Camel 3 uses camel-3.x, whereas Camel 4 uses the main branch)

Tracking

  • If this is a large change, bug fix, or code improvement, I checked there is a JIRA issue filed for the change (usually before you start working on it).
    I do consider this change as a small change, since the default behavior is not touched.

Apache Camel coding standards and style

  • I checked that each commit in the pull request has a meaningful subject line and body.

  • I have run mvn clean install -DskipTests locally and I have committed all auto-generated changes

@github-actions
Copy link
Copy Markdown
Contributor

🌟 Thank you for your contribution to the Apache Camel project! 🌟

⚠️ Please note that the changes on this PR may be tested automatically.

If necessary Apache Camel Committers may access logs and test results in the job summaries!

Copy link
Copy Markdown
Contributor

@oscerd oscerd left a comment

Choose a reason for hiding this comment

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

LGTM, can we please have this on main too? @ffang could you have a look too?

@github-actions
Copy link
Copy Markdown
Contributor

Components test results:

Total Tested Failed ❌ Passed ✅
1 1 0 1

@github-actions
Copy link
Copy Markdown
Contributor

🚫 There are (likely) no changes in core core to be tested in this PR

Copy link
Copy Markdown
Contributor

@zhfeng zhfeng left a comment

Choose a reason for hiding this comment

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

Thanks @johbossle - I can understand the changes with LRAClient but why do you need to add the extra routes in LRASagaRoutes?

IIRC the rest configurations and direct:lra-compensation and direct:lra-completion in
LRASagaRoutes are very important. If we override LRASagaRoutes and miss these routes in some cases, the LRA can not run correctly.

So can we think about avoiding miss these core routes?

And yeah, we should have these changes on main too.

@johbossle
Copy link
Copy Markdown
Contributor Author

@zhfeng For me, there were two reasons to allow the override of LRARoutes:

  • keep it in analogy with the creation process of LRAClient (I'm personally not a big fan of hard-coded constructor calls without possibility to influence anything. It makes it harder to test, less open for other use-cases, ...)
  • Get at least a a possibility to influence the verification of the requests.
    Nevertheless, I do understand the risk of missing the routes and the importance of those. But if you are overriding, you hopefully know what you are doing. You could also manipulate the CamelContext and remove the routes etc.

To continue, I do see now the following possibilities:

  • In general, we do not want to allow to override the LRASagaRoutes creation
  • We simply do it as proposed right now
  • We split this aspect into an extra PR and realize the topic here - maybe in consequence also with having a protected method for verifyRequest.

What way should we follow?

@zhfeng
Copy link
Copy Markdown
Contributor

zhfeng commented Jun 27, 2023

From my own point of view, LRASagaRoutes is an internal implementation and can not be override. And I'm open to the discussion, so feel free to raise another PR for the LRASagaRoutes.

Also +1 for override LRAClient but I'm not sure narayana-lra-coordinator supports SSL right now?

@johbossle
Copy link
Copy Markdown
Contributor Author

Then let us keep the LRARoutes internal and kick it out of the PR.
As far as I know, there is the possibility to secure narayana. See also https://github.com/jbosstm/quickstart/blob/main/rts/lra-examples/lra-jwt/wildfly/README.md

Should I open a new PR for only allowing to override the LRAClient?
To support the use-case end-to-end, I would also suggest to allow a more easy way to add authentication header without overriding/delegating the HttpClient-Implementation. But for that extension, I would open an extra jira ticket to work on.

@zhfeng
Copy link
Copy Markdown
Contributor

zhfeng commented Jun 27, 2023

Yeah, that would be nice to have a new PR and definely a JIRA ticket is needed.

@johbossle
Copy link
Copy Markdown
Contributor Author

As agreed we will close this PR and open a new one, containing the agreed changes.

@johbossle johbossle closed this Jun 27, 2023
@johbossle johbossle deleted the camel-3.x branch June 27, 2023 08:31
@mmusgrov
Copy link
Copy Markdown

Also +1 for override LRAClient but I'm not sure narayana-lra-coordinator supports SSL right now?

Configuring JAX-RS to use SSL is done at the server level (WildFly, Quarkus or wherever you're deploying your JAX-RS resources to), ie the coordinator doesn't have to do anything special (it uses jakarta.ws.rs.client.ClientBuilder.newClient() which is supplied by the JAX-RS server).

@zhfeng
Copy link
Copy Markdown
Contributor

zhfeng commented Jun 27, 2023

Thanks @mmusgrov for the clarifying - Is there any example or document about these server configuration? I think it could be very helpful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants