Closed turing85 closed 1 week ago
quarkus-bot[bot]
commented
1 year ago /cc @DavideD (hibernate-reactive), @Sanne (hibernate-orm,hibernate-reactive), @gavinking (hibernate-reactive), @gsmet (hibernate-orm), @mswatosh (db2), @yrodiere (hibernate-orm)
cquoss
commented
1 year ago The root cause here is that the setup is missing the msal4j dependency.
But this root cause never pops up since the time-out occurs here: https://github.com/agroal/agroal/blob/2a95e5bef7948635da4a2d0f88a53d5202868d9a/agroal-pool/src/main/java/io/agroal/pool/ConnectionPool.java#L279
I actually do not know how to fix this but dealing with it is a PITA. Whenever you observe this one has to drill down into the callable extracting the root cause.
Maybe s/o with better knowledge of this code can come up with s/th helpful here.
cquoss
commented
1 year ago What's also annoying here is the fact that one has to add all the extra deps here (json-smart, gson, jwt, msal4j). One would expect that quarkus-jdbc-mssql carries them along.
bertcarels
commented
1 year ago experienced this issue as well tried to configure mssql managed identity authentication for keycloak
I have made it work by adding the following dependencies in keycloak and doing a rebuild
<dependency>
<groupId>com.microsoft.azure</groupId>
<artifactId>msal4j</artifactId>
<version>1.13.3</version>
</dependency>
<dependency>
<groupId>com.azure</groupId>
<artifactId>azure-identity</artifactId>
<version>1.7.0</version>
</dependency>I tested it on Azure AKS and can confirm that both authentication with a) service principal b) managed identity are working
but I believe these dependencies need to be added on the quarkus jdbc mssql extension here: https://github.com/quarkusio/quarkus/blob/main/extensions/jdbc/jdbc-mssql/runtime/pom.xml
How could that be done? How do I propose that? Anyone can clarify? dependencies are documented here by Microsoft: https://learn.microsoft.com/en-us/sql/connect/jdbc/connecting-using-azure-active-directory-authentication?view=sql-server-ver16#client-setup-requirements
bertcarels
commented
1 year ago there's also an open issue on keycloak: https://github.com/keycloak/keycloak/issues/22830
yrodiere
commented
12 months ago But this root cause never pops up since the time-out occurs here: https://github.com/agroal/agroal/blob/2a95e5bef7948635da4a2d0f88a53d5202868d9a/agroal-pool/src/main/java/io/agroal/pool/ConnectionPool.java#L279
Hey @barreiro , it would seem that some exceptions are not propagated correctly by Agroal when using MSSQL and some optional dependencies are missing. Any idea what could be wrong exactly?
but I believe these dependencies need to be added on the quarkus jdbc mssql extension
I'm not sure they should be added to the Quarkus extension, because not all users will need them. Microsoft decided that their JDBC client should not depend on them, after all, and probably for the same reason.
We certainly could (should) document when they are needed though. Maybe here or there.
stephaneey
commented
11 months ago but I believe these dependencies need to be added on the quarkus jdbc mssql extension
I'm not sure they should be added to the Quarkus extension, because not all users will need them. Microsoft decided that their JDBC client should not depend on them, after all, and probably for the same reason.
Who don't need to leverage OAuth 2 flows nowadays? Relying on SQL authentication in 2023 isn't certainly the way to go for most organizations anymore. I think @bertcarels 'suggestion makes sense.
yrodiere
commented
11 months ago I think @bertcarels 'suggestion makes sense.
It certainly does, in his (and your) context.
In the context of people who don't need these features, the additional dependencies would represent a higher attack surface when it comes to security vulnerabilities, not to mention unwanted bloat.
It's a matter of finding the right balance between bloat and good out-of-the-box experience, for most users in most contexts.
Who don't need to leverage OAuth 2 flows nowadays?
I wouldn't know, as I don't personally use SQL Server in production environments. Microsoft obviously thinks some people don't need to leverage OAuth 2 flows, otherwise the feature would be built-in instead of requiring additional dependencies. I tend to trust Microsoft on that.
Regardless, the most pressing issue IMO is the fact that some errors get swallowed, making this issue a nightmare to debug. I hope @barreiro will be able to help on that front, and fix this in the next micro.
In the next minor, if someone more versed than I am in authentication practices (@maxandersen ? @geoand ? @gastaldi ?) decides we need out-of-the-box support for ActiveDirectory authentication with SQL Server, then sure let's go for more dependencies.
geoand
commented
11 months ago In the next minor, if someone more versed than I am in authentication practices (@maxandersen ? @geoand ? @gastaldi ?) decides we need out-of-the-box support for ActiveDirectory authentication with SQL Server, then sure let's go for more dependencies.
I'm not particularly well versed in auth practices, but OOTB support for AD at the expense of increased bloat, complexity etc is a -1 for me.
Having some kind optional support would be fine
Sanne
commented
11 months ago The Microsoft JDBC driver has a very deep dependency tree: it's not reasonable to pull in all the optional dependencies it presents, for example some of their dependency are highly contextual (e.g. only useful on certain old versions of Windows server) and last time I checked wouldn't work with native-image.
Also, it does not clearly define what is optional, this requires some domain knowledge
Admittedly this was the situation some years ago when I first looked into adding mssql support in Quarkus; perhaps things are improved by now: I'm just commenting here so that if anyone here wants to volunteer to try again and refine these choices, you know what to be careful with. I certainly did make some choices at the time, to make it work, and some might not be optimal today.
stephaneey
commented
11 months ago I understand the rationale and it is true that OAuth was not yet so dominant years ago, but nowadays, in Azure for instance, everything is 100% OIDC, and federated identities become mainstream, here again, relying on OIDC.
if anyone here wants to volunteer to try again and refine these choices, you know what to be careful with. I certainly did make some choices at the time, to make it work, and some might not be optimal today.
@bertcarels :)?
maxandersen
commented
1 week ago closing this as duplicate of https://github.com/quarkusio/quarkus/issues/36587
Describe the bug
When connecting to an azure-deployed MS SQL and using
the application fails to create a connection.
Expected behavior
Application connects, and database can be used.
Actual behavior
Application fails to connect with the following exception:
How to Reproduce?
pom.xml, notice thatquarkus.versionis set to2.12.0.Final(the first version failing the test)application.propertiesand configure the database-connection by settingdb.name,db.host,quarkus.datasource.usernameandquarkus.datasource.passwordpom.xml, changequarkus.versionfrom2.12.0.Finalto2.12.0.CR1Process finished with exit code 0