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

PostgreSQL: Restore role to its original value between migrations instead of resetting it #2035

Closed
danieljamesscott opened this issue Jun 5, 2018 · 15 comments

Comments

@danieljamesscott
Copy link

@danieljamesscott danieljamesscott commented Jun 5, 2018

Which version and edition of Flyway are you using?

5.1.1

If this is not the latest version, can you reproduce the issue with the latest one as well?

(Many bugs are fixed in newer releases and upgrading will often resolve the issue)

Which client are you using? (Command-line, Java API, Maven plugin, Gradle plugin)

Java API

Which database are you using (type & version)?

PostgreSQL 9.6

Which operating system are you using?

Linux

What did you do?

(Please include the content causing the issue, any relevant configuration settings, the SQL statement that failed (if relevant) and the command you ran.)

I need to be able to run 'set role $rolename' for all flyway migrations, including the initial metadata table creation. This is to ensure that the created objects are owned by the 'correct' user. We are using vault for database credential management, so the user running the migrations is dynamic and will change over time, leading to inconsistent ownership of objects.

What did you expect to see?

Possibly a flyway configuration option, or callback support to call 'set role'. I'm not sure if there's a callback which is run before the initial metadata table creation?

What did you see instead?

I saw a few issues related to this, including a call to 'RESET ROLE' which appears to be conflicting with my attempts to use connectInitSql on the pool, or the beforeMigration callback.

pradheeps@5671442#diff-082a3414ab12680389b61424a632705cR90

@axelfontaine
Copy link
Contributor

@axelfontaine axelfontaine commented Jun 5, 2018

This should be achievable using a combination of callbacks.

For the initial schema history table creation, use the beforeMigrate.sql callback and then for the individual migrations use the beforeEachMigrate.sql callback.

@danieljamesscott
Copy link
Author

@danieljamesscott danieljamesscott commented Jun 5, 2018

Thanks, but would it be possible to add this in a cleaner way? I believe that this will become more of a problem as people start using dynamic users more.

@axelfontaine
Copy link
Contributor

@axelfontaine axelfontaine commented Jun 5, 2018

I am not sure a dedicated setting would be pulling its conceptual weight at this point, especially considering the easy workaround available.

But the future will tell. If this does prove to be a major pain point down the road, we'll gladly reconsider this decision.

@danieljamesscott
Copy link
Author

@danieljamesscott danieljamesscott commented Jun 5, 2018

Sure. Thanks.

Can we make it possible to disable the 'RESET ROLE' call? It seems a little strange to do a hard reset. If someone has changed role, then it should be their responsibility to change it back when they're done, right?

@axelfontaine
Copy link
Contributor

@axelfontaine axelfontaine commented Jun 5, 2018

Can we make it possible to disable the 'RESET ROLE' call? It seems a little strange to do a hard reset. If someone has changed role, then it should be their responsibility to change it back when they're done, right?

No, as this would potentially produce different results. For example if V1 changes the role and V2 does not then running V1 + V2 together will produce a different result than running V1 and then later V2.

@danieljamesscott
Copy link
Author

@danieljamesscott danieljamesscott commented Jun 5, 2018

Yes, but if a user is messing with roles in the migration, then they should ensure that they do the reset, no?

By the same argument, if I connect with user1 and run v1 + v2, that produces a different result than when I connect with user1 and run v1 and then connect with user2 and run v2.

@danieljamesscott
Copy link
Author

@danieljamesscott danieljamesscott commented Jun 5, 2018

Would you accept a PR to make execution of Connection.doRestoreOriginalState configurable with a new configuration option in Configuration?

@axelfontaine
Copy link
Contributor

@axelfontaine axelfontaine commented Jun 6, 2018

Would you accept a PR to make execution of Connection.doRestoreOriginalState configurable with a new configuration option in Configuration?

No, for the conceptual weight reasons I outlined above.

@danieljamesscott
Copy link
Author

@danieljamesscott danieljamesscott commented Jun 7, 2018

I'm not sure what's going on, but setting the role in the 'beforeMigrate' callback does not result in flyway_schema_history being owned by my role. It is owned by the connecting role. This appears to have stopped working around 5.0.7.

@axelfontaine
Copy link
Contributor

@axelfontaine axelfontaine commented Jun 7, 2018

I saw that you mentioned this in the PR comments. Can you debug and figure out why?

@axelfontaine axelfontaine reopened this Jun 13, 2018
@axelfontaine axelfontaine added this to the Flyway 5.2.0 milestone Jun 21, 2018
@axelfontaine
Copy link
Contributor

@axelfontaine axelfontaine commented Jun 21, 2018

I am starting to warm for the option of adding a postgresql.role configuration property to handle this cleanly.

More background info: https://dba.stackexchange.com/questions/77704/proxy-authentication-for-postgesql

@axelfontaine
Copy link
Contributor

@axelfontaine axelfontaine commented Sep 22, 2018

I am starting to think that an even better solution would be to expose the initSqls property of the data source to all clients. This would allow for a generic way to do things like set role before any other Flyway code runs.

This should then be combined with replacing the reset role calls with an initial call to read the current user and individual calls before each migration to reset the current user to the one we read before.

Feedback welcome.

@danieljamesscott
Copy link
Author

@danieljamesscott danieljamesscott commented Sep 22, 2018

This is exactly what I was asking for in the original issue report! 😃

@axelfontaine axelfontaine changed the title Vault integration and set role support PostgreSQL: Restore role to its original value between migrations instead of resetting it Sep 25, 2018
axelfontaine added a commit to flyway/flywaydb.org that referenced this issue Sep 25, 2018
@axelfontaine
Copy link
Contributor

@axelfontaine axelfontaine commented Sep 25, 2018

This has now been implemented. It should give you exactly what you need when used in combination with #1324 .

dohrayme pushed a commit to dohrayme/flyway that referenced this issue Feb 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants