Repository navigation
Replies: 3 comments 8 replies
|
Thank you @yrodiere for following up on this.
That is correct we lack built-in support with operator based usage. We also officially support at least 1 database that lacks XA support.
That is correct - at least from a built-in perspective. Almost all of our existing code paths are either writing to a single datasource, or are read-only across all sources. There is a concern that custom integration could do anything under the transaction though.
This was where we had wanted heuristic mixed exceptions to help expose a situation that did not commit successfully. @keycloak/core should weigh in here as well as to the transactional expectations - and how we expect users choosing to run in non-xa mode (possibly because of non-xa datasource(s)) to recover.
No, we are just going to rely on the workaround introduced to support the legacy behavior.
Foremost we need to avoid hard errors, and to make users aware if things may be in an inconsistent state - we have a workaround for the former, and a log message on the latter. I'm open to additional changes, but it's hard to envision exactly what that would be. We have our own transaction start / commit as part of the request / response cycle outside of more typical quarkus handling. At the time we're starting the transaction we don't currently have captured what type of path we are dealing with (ready-only, write to one and what particular source, or write to multiple). Ignoring the possiblity of custom logic introducing additional writes, could identify all built-in logic that falls into the write to multiple category and make targeted logic changes there. |
We start a transaction at the beginning of the request processing cycle. Then there is handling when the response is complete to commit or rollback. See: So currently we are not using any transaction related annotations, nor quarkus level transaction management. It seems plausible that for the handful of built-in paths that could update multiple datasources that we could annotate those endpoints and pass that down somehow. Eveything else (nominally a single update, or all read-only case) we'd want to be free of warnings / errors. |
Okay, I think this rules out marking a transaction as read-only. There might be other solutions though, see below.
What would be the semantics of that annotation? IIUC we're talking of cases where datasources are read/write, so I'm not sure what's the point here... ?
Right, well in order to not get any warning / error, Narayana needs to be able to tell for sure that at most one non-XA datasoure is written to. |
That seems backwards, as this would give an "unsafe-by-default" sort of behavior. Better annotate endpoints that are known to be safe (e.g. with Two remarks though -- regardless of whether we go with safe-by-default or unsafe-by-default:
So, at this point is seems only two avenues are worth pursuing:
|
I'm relaying this from Keycloaks perspective - what is the least effort, and what matches our current assumptions. I don't mean to imply that this annotation would be something that Quarkus or Narayana would directly know about - since we only have 1 or two resource calls that are affected (without considering custom logic) so it is much more straight-forward for us to annotate those rather than everything else. The handling of that annotation would then presumably be in the logic I linked to above in our session handler. However if this seems too short-sighted, I'm fine with being safe by default. We definitely require number 2. The typical scenario there is an update to a federated resource, such as a user store. That update will eventually be made into just a single datasource, but that may be difficult to determine which datasource at the level at which we are starting the transaction. I believe many of these calls may require reads against the primary keycloak datasource as well. |
Sorry if I was dismissive; it's a decent workaround, it's just that we appear to approach the problem differently. I'm focusing on having warnings report actual problems nobody knows about, you're focusing on removing pesky, unnecessary warnings -- which is equally important and actually necessary to get to my goal. Implementation-wise, your solution will require Narayana to display warnings or not depending on the "endpoint context". Right now all Narayana offers is a global flag spanning all requests/threads. I suspect the change will not be straightforward, and may not be high-priority considering it's a workaround, but you can certainly try to file an issue to Narayana.
I think you don't need to know whether a datasource will be read-write or read-only at the time you start the transaction in the transaction manager, but a bit later, when the datasource joins the transaction (i.e. when you open a connection). Would that be easier? Note another constraint is that you can't upgrade from read-only to read-write after joining the transaction, so you'll have to look for that. It might be okay to open a second , concurrent connection to the same datasource with read-write access, but then you might run in classic connection pool issues when acquiring multiple connections (deadlocks). |
The semantics from Keycloak's perspective would be - with the annotation there would be a warning or hard error, depending upon the keycloak config, when more than 1 non-XA datasource is involved in the transaction. Without the annotation, no warning or error is expected. |
|
Now that we are finalizing our upgrade to 3.13 the thinking is that it will be simipler to not fight the direction of the safety check, and instead cut off support for multi-source scenarios with non-XA datasources. This should impact only a small amount of potential keyclaok usage, and we can assess any potential workarounds later if needed. |
|
That definitely looks like the easiest path if it's possible. Thank you for the update! |
|
This will be addressed in: |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Hey all, @shawkins,
Some time ago, a breaking change happened in Quarkus, causing exceptions at runtime, and this led to the discovery of unsafe paths in Keycloak. When going through these paths, Keycloak could involve multiple non-XA datasources in the same transaction, which could lead to a transaction being committed in some datasources, but rolled back in others.
The breaking change (causing the runtime exceptions) was reverted in Quarkus 3.8.5, which Keycloak upgraded to, so it no longer affects Keycloak, and a flag was added to Quarkus so that any unsafe paths can be reported as warnings instead of triggering exceptions.
However, as far as I know, the unsafe paths remain. We discussed this at length with @shawkins, and from what I understand:
Having a way to mark transactions as read-only -- and thus make Narayana understand that it's safe to involve multiple non-XA resources -- would help .
I wanted us to continue our conversation on this topic, especially with @shawkins:
Thank you,
Yoann (Hibernate, Quarkus)
CC: @gsmet, @Sanne, @mmusgrov
All reactions