Skip to content
This repository has been archived by the owner on Nov 14, 2020. It is now read-only.

Making possible to specify default 'role' parameter for the user #150

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

wimi
Copy link

@wimi wimi commented Jun 22, 2020

This PR adds possibility to set the default role parameter for the user.
This is effectively the following statement on database level:
ALTER ROLE role_name SET role TO role_parameter_value

This is my first PR on github, so apologies if something is not filled in - just let me know and I'll fix this.

As for the code, it is my first golang code as well (yay!) - so I tried to stick to style of other parts of the code as closely as possible.

Looking at the code, it might be a good idea to somehow unify handling of default parameters - if there will be more. I'm willing to grab the subject in the future.

@wimi
Copy link
Author

wimi commented Jun 26, 2020

@cyrilgdn Not sure if I have to create Issue ticket for that PR as well...
So, ping! Hope you get this notification.

We'd really love to have this feature in the provider :)

@cyrilgdn
Copy link
Contributor

cyrilgdn commented Jul 3, 2020

Hi @wimi ,

Thanks for opening this PR!

To be honest I don't know what's the role parameter is for a user :) ? Could you explain to me what it does ? (I don't find the documentation)

I need to think a bit about this because it's not the first time we want to configure a role parameter (there's also #132 ), maybe we could define some kind of map of parameters.

@wimi
Copy link
Author

wimi commented Jul 3, 2020

Hi @cyrilgdn!

Role parameter allows you to change the identifier of the currently logged in user (https://www.postgresql.org/docs/current/sql-set-role.html). This is useful when you want to implement seamless user/password rotation scenario. If you need more details about it, here is a blog where it is described: https://www.jannikarndt.de/blog/2018/08/rotating_postgresql_passwords_with_no_downtime/

I've been looking at the code and currently there are already two parameters set for the user: search_path and statement_timeout. Each of those have its own property in TF Role resource. That's why I went the same route with role attribute - as an additional property in Role.

I was also thinking about implementing a map of parameters for role, but in this case we'd have legacy properties search_path/statement_timeout and new "role_properties" map. We could implement some sanity check to allow using only legacy approach or new one, but not both at the same time. This approach will certainly take longer to implement than what I did so far though :) I am also not 100% sure about the data types of each of parameter, that might be a little bit problematic as well.

So maybe instead of implementing generic parameters map we could still have individual properties - added on a "as needed basis", but with some generic handling for these in the code. Right now each of those parameters seem to be handled by very similar code. If you agree with than - I can take a look at how we can generalize this and make code more reusable for future extensibility.

@wimi
Copy link
Author

wimi commented Jul 4, 2020

Hi @cyrilgdn,

Actually, I think I implemented a rough idea of setting a map of default parameters for role. It is still in early WIP phase, but I think it already has a structure we can discuss. It also coexists with parameters already set by standalone role properties.

Can we discuss my idea? Here is a draft PR for that: #155

I am really interested in pushing this subject - as we'd like to have this feature this way or another.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants