ansible-collections / community.postgresql

Manage PostgreSQL with Ansible
https://galaxy.ansible.com/ui/repo/published/community/postgresql/
Other
111 stars 90 forks source link

Users configuration #722

Closed toydarian closed 3 months ago

toydarian commented 4 months ago
SUMMARY

PostgreSQL has the ability to set defaults for configuration parameters for roles. This means, that when that users logs in, the parameter for their session will be set to the default for their role instead of the global default.
For example work_mem could be set to a different default for a specific role.
The current state of the collection doesn't handle this in a good way, though it is possible to manage those using the community.postgresql.postgresql_query module. It would be better if those would be handled by the community.postgresql.postgresql_user module.
This is what these changes do. Configuration defaults can be handled when the user is created and will be managed in an idempotent manner. To provide backwards-compatibility, omitting the new parameter on the module will not change the current state.

Personal motivation:
I need this to manage settings for pg_audit for different users. But I think having this in the module is valuable enough, so I added it.
I also found a ticket on this.

Fixes #598

ISSUE TYPE
COMPONENT NAME

community.postgresql.postgresql_user

ADDITIONAL INFORMATION

I don't have more information than what is already mentioned in the summary.

betanummeric commented 4 months ago

I think in some cases it may be useful to only touch variables explicitly specified in the module arguments. With module arguments for example like

set_variables:
  search_path: foo
reset_variables:
- work_mem
reset_unspecified: false

would explicitly set search_path, explicitly reset work_mem and leave all other potentially configured things untouched.

toydarian commented 4 months ago

I think in some cases it may be useful to only touch variables explicitly specified in the module arguments. With module arguments for example like

set_variables:
  search_path: foo
reset_variables:
- work_mem
reset_unspecified: false

would explicitly set search_path, explicitly reset work_mem and leave all other potentially configured things untouched.

@betanummeric I'm not a big fan of that. If you had reset_unspecified: false you would need to list all variables to reset which can be a long list. If at all, I would specify a list of parameters not to reset, but I think, even that is overkill. I think the current implementation strikes a good balance between clarity and covering most use-cases as well as providing backwards-compatibility.

There are also other use-cases, for example IN DATABASE where you can set a default for a role only in a certain named database and then you have ALTER DATABASE to set defaults for a specific database, which are also not covered. If I tried to cover all of those, that would probably warrant a completely new module.

betanummeric commented 3 months ago

@toydarian

I'm not a big fan of that. If you had reset_unspecified: false you would need to list all variables to reset which can be a long list. If at all, I would specify a list of parameters not to reset, but I think, even that is overkill. I think the current implementation strikes a good balance between clarity and covering most use-cases as well as providing backwards-compatibility.

With reset_unspecified I meant to support these two modes, which is a common pattern across the collection: A) set the requested things and reset everything else, ensuring only requested things deviate from defaults B) set the requested things and leave the rest untouched

Mode A is useful when you have authoritative and comprehensive information for what shall be configured. Mode B is better when you only know how some parts shall be configured, leaving the rest up to others.

I'm using the collection to the extend possible only in mode B, integrated in some high-level automation. I'm doing this to step-wise introduce more automation to an already-existing database platform. When starting to use user-config, I would take a handful variables and either set or reset them, without needing to specify "long" lists.

As a user, I'd like to have the choice between these modes. We can make mode A the default (reset_unspecified: true).

I don't think we need a "don't reset these variables" list argument, this sounds too special.


There are also other use-cases, for example IN DATABASE where you can set a default for a role only in a certain named database and then you have ALTER DATABASE to set defaults for a specific database, which are also not covered. If I tried to cover all of those, that would probably warrant a completely new module.

alter role ... in database ... would be a feature for this postgresql_user module. It could be implemented with another argument holding a nested dict:

db_config:
  some_specific_database:
    maintenance_work_mem: '16MB'

alter database ... is a topic for the postgresql_db module.

No need to implement either in this PR.

toydarian commented 3 months ago

@betanummeric okay, now I understand your use-case. I incorporated the necessary changes.

toydarian commented 3 months ago

I think I addressed all comments now. @Andersson007 @betanummeric @hunleyd

toydarian commented 3 months ago

@hunleyd seems I forgot to update the example, fixed in 3f397d6

Andersson007 commented 3 months ago

@toydarian thanks a lot for working on this! The unit tests are failing:

01:06 _____ ERROR collecting tests/unit/plugins/modules/test_postgresql_user.py ______
01:06 ImportError while importing test module '/root/ansible_collections/community/postgresql/tests/unit/plugins/modules/test_postgresql_user.py'.
01:06 Hint: make sure your test modules/packages have valid Python names.
01:06 Traceback:
01:06 /tmp/ansible-test-b7_edotq/ansible/utils/collection_loader/_collection_finder.py:625: in load_module
01:06     exec(code_obj, module.__dict__)
01:06 tests/unit/plugins/modules/test_postgresql_user.py:12: in <module>
01:06     from plugins.modules.postgresql_user import parse_user_configuration, compare_user_configurations, _pg_quote_user
01:06 E   ImportError: No module named plugins.modules.postgresql_user
01:06 - generated xml file: /root/ansible_collections/community/postgresql/tests/output/junit/python2.7-modules-units.xml -
01:06 =========================== short test summary info ============================
01:06 ERROR tests/unit/plugins/modules/test_postgresql_user.py

We should make CI green before merging. As unit tests are optional, in case you have no time to deal with them right now, you could remove them and add later when you have time. So please make CI green however it works for you

toydarian commented 3 months ago

@Andersson007 about the unit-tests:
It seems they only fail for Python 2 and I can't really figure out why. I managed to fix the import-issue but now there is an assertion error and those tests work fine with Python 3. I disabled the failing test for Python 2.
The CI is green now.

Andersson007 commented 3 months ago

@toydarian thanks for the contribution! Much appreciated! @hunleyd @betanummeric thanks for reviewing! It was a great long discussion, but we did it!

@toydarian we have a forum group https://forum.ansible.com/g/PostgreSQLTeam and you're welcome to join. Also there's a Matrix channel mentioned in the description - welcome too if you're interested. There are plenty opportunities to contribute in this and other collections, so please let us know if there's nothing in the issue tracker that look interesting.