-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Introduce Flyway specific user configuration #29628
Conversation
…r datasource, but DDL user for migration #24639
Thanks for your pull request! The title of your pull request does not follow our editorial rules. Could you have a look?
|
if (flywayRuntimeConfig.username.isPresent() && flywayRuntimeConfig.password.isPresent() | ||
&& flywayRuntimeConfig.jdbcUrl.isPresent()) { | ||
configure.dataSource(flywayRuntimeConfig.jdbcUrl.get(), flywayRuntimeConfig.username.get(), | ||
flywayRuntimeConfig.password.get()); |
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.
This assumes that all three have been set. We should warn (or perhaps even fail) if at least one but not all three are set.
Or perhaps the URL does not need to be set but can be taken from the corresponding DataSource.
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.
Yeah, we should have the following behavior:
- jdbc url can be set but is not mandatory. The default is the one from the datasource.
- if either user or password is defined, both should be defined.
- if a specific jdbc url is defined, I think we probably want the user and password to be mandatory as I don't think it's a good idea to use the credentials from the original datasource
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.
You can retrieve the original JDBC URL from dataSource.unwrap(AgroalDataSource.class).getConfiguration().connectionPoolConfiguration().connectionFactoryConfiguration().jdbcUrl();
(but you will have to deal with the SQLException - you can probably ignore it)
I think something similar to:
AgroalDataSource agroalDataSource;
try {
agroalDataSource = dataSource.unwrap(AgroalDataSource.class);
} catch (SQLException e) {
throw new RuntimeException("Unable to unwrap " + dataSource + " to AgroalDataSource", e);
}
configure.dataSource(
agroalDataSource.getConfiguration().connectionPoolConfiguration().connectionFactoryConfiguration().jdbcUrl(),
flywayRuntimeConfig.username.get(),
flywayRuntimeConfig.password.get());
Map<String, String> jdbcProperties = agroalDataSource.getConfiguration().connectionPoolConfiguration()
.connectionFactoryConfiguration().jdbcProperties().entrySet().stream().collect(
Collectors.toMap(
e -> e.getKey().toString(),
e -> e.getValue().toString()));
configure.jdbcProperties(jdbcProperties);
This is just to give you hints about the API.
I think you will need to differentiate:
- we have a jdbcUrl defined in Flyway config, we use it, together with jdbcProperties defined at the Flyway config level
In this case, user and password are mandatory and you use the ones defined in the Flyway config and throw an error if not defined.
- you don't have a jdbcUrl defined but a user and password
In this case, you use the jdbcUrl and jdbcProperties from the original datasource (see above about how to do it).
Maybe log a warning if jdbcProperties are set in the Flyway config.
- you don't have anything defined in the Flyway config
Just use the original datasource.
I'm wondering if maybe you should have a datasource @ConfigGroup
with something like:
quarkus.flyway.datasource.jdbcUrl
quarkus.flyway.datasource.jdbcProperties
quarkus.flyway.datasource.user
quarkus.flyway.datasource.password
# Flyway config properties | ||
quarkus.flyway.username=sa | ||
quarkus.flyway.password=sa | ||
quarkus.flyway.jdbcUrl=jdbc:h2:tcp://localhost/mem:init-sql-config;DB_CLOSE_DELAY=-1 |
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.
This one is not right, it should be quarkus.flyway.jdbc-url
.
if (flywayRuntimeConfig.username.isPresent() && flywayRuntimeConfig.password.isPresent() | ||
&& flywayRuntimeConfig.jdbcUrl.isPresent()) { | ||
configure.dataSource(flywayRuntimeConfig.jdbcUrl.get(), flywayRuntimeConfig.username.get(), | ||
flywayRuntimeConfig.password.get()); |
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.
Yeah, we should have the following behavior:
- jdbc url can be set but is not mandatory. The default is the one from the datasource.
- if either user or password is defined, both should be defined.
- if a specific jdbc url is defined, I think we probably want the user and password to be mandatory as I don't think it's a good idea to use the credentials from the original datasource
var = rs.getInt(1); | ||
} | ||
} | ||
assertEquals(100, var, "Different username and password was not executed"); |
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'm not sure I follow the rationale of this test and how it would fail if the new config stuff is not taken into account. Could you clarify your intent? (I might miss something obvious)
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.
This test if the connection was opened with new config and make a select and confirm the result.
* This includes e.g. accidentally setting quarkus.hibernate-orm.database.generation to drop-and-create. | ||
* If the application runs with a restricted database user, damage in such situations can be mitigated. | ||
* # Name of a custom database user for flyway to use. Must have DDL rights | ||
* quarkus.flyway.username=my_schema_owner |
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 think we should better clarify the intent rather than this.
I would go with something far more neutral such as:
Sets a specific user that will Flyway will use to connect to the datasource.
Allows to have a specific user that can apply DDL to the schema while the standard user used for the datasource may not.
(formatting will have to be fixed)
* quarkus.flyway.username=my_schema_owner | ||
* | ||
*/ | ||
|
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.
Can you remove the blank lines?
* This includes e.g. accidentally setting quarkus.hibernate-orm.database.generation to drop-and-create. | ||
* If the application runs with a restricted database user, damage in such situations can be mitigated. | ||
* # Name of a custom database user for flyway to use. Must have DDL rights | ||
* quarkus.flyway.username=my_schema_owner |
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.
No need for quarkus.flyway.username=my_schema_owner
, it will be automatically generated.
public Optional<String> password = Optional.empty(); | ||
|
||
/** | ||
* Set the jdbc url. |
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.
* Set the jdbc url. | |
* Set the JDBC URL if it is required for the JDBC URL to be different from the one from the datasource. |
* Set the jdbc url. | ||
* * quarkus.flyway.jdbcUrl= database:url | ||
*/ | ||
|
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.
Drop the blank line.
|
||
/** | ||
* Set the jdbc url. | ||
* * quarkus.flyway.jdbcUrl= database:url |
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.
You can remove this line.
if (flywayRuntimeConfig.username.isPresent() && flywayRuntimeConfig.password.isPresent() | ||
&& flywayRuntimeConfig.jdbcUrl.isPresent()) { | ||
configure.dataSource(flywayRuntimeConfig.jdbcUrl.get(), flywayRuntimeConfig.username.get(), | ||
flywayRuntimeConfig.password.get()); |
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.
You can retrieve the original JDBC URL from dataSource.unwrap(AgroalDataSource.class).getConfiguration().connectionPoolConfiguration().connectionFactoryConfiguration().jdbcUrl();
(but you will have to deal with the SQLException - you can probably ignore it)
I think something similar to:
AgroalDataSource agroalDataSource;
try {
agroalDataSource = dataSource.unwrap(AgroalDataSource.class);
} catch (SQLException e) {
throw new RuntimeException("Unable to unwrap " + dataSource + " to AgroalDataSource", e);
}
configure.dataSource(
agroalDataSource.getConfiguration().connectionPoolConfiguration().connectionFactoryConfiguration().jdbcUrl(),
flywayRuntimeConfig.username.get(),
flywayRuntimeConfig.password.get());
Map<String, String> jdbcProperties = agroalDataSource.getConfiguration().connectionPoolConfiguration()
.connectionFactoryConfiguration().jdbcProperties().entrySet().stream().collect(
Collectors.toMap(
e -> e.getKey().toString(),
e -> e.getValue().toString()));
configure.jdbcProperties(jdbcProperties);
This is just to give you hints about the API.
I think you will need to differentiate:
- we have a jdbcUrl defined in Flyway config, we use it, together with jdbcProperties defined at the Flyway config level
In this case, user and password are mandatory and you use the ones defined in the Flyway config and throw an error if not defined.
- you don't have a jdbcUrl defined but a user and password
In this case, you use the jdbcUrl and jdbcProperties from the original datasource (see above about how to do it).
Maybe log a warning if jdbcProperties are set in the Flyway config.
- you don't have anything defined in the Flyway config
Just use the original datasource.
I'm wondering if maybe you should have a datasource @ConfigGroup
with something like:
quarkus.flyway.datasource.jdbcUrl
quarkus.flyway.datasource.jdbcProperties
quarkus.flyway.datasource.user
quarkus.flyway.datasource.password
- As requested in quarkusio#29628 (comment)
The use case for this is to make it safer to run database migrations by configuring a dedicated db user for database migrations: DML-only user for datasource, but DDL user for migration
Closes #24639