Added support for Ubuntu 20.04 and 22.04 along with Postgresql 14, 15… #31
Conversation
| @@ -1,33 +0,0 @@ | |||
| --- | |||
There was a problem hiding this comment.
Why was this removed? Was it broken?
There was a problem hiding this comment.
I don't want to break compatibility for people using older versions of pg-auto-failover.
There was a problem hiding this comment.
I didn't think anyone would still be using it. I will add all of these back. I should have asked first, but either way now I know the answer.
| @@ -1,34 +0,0 @@ | |||
| --- | |||
| shell: > | ||
| su -c 'PATH="$PATH:{{ postgresql_cluster_bin_path }}" pg_autoctl -q show systemd --pgdata "{{ postgresql_cluster_data_dir }}"' postgres \ | ||
| | tee /etc/systemd/system/{{ postgresql_cluster_daemon }}.service | ||
| #- name: "create systemd config for cluster" |
| @@ -1,12 +0,0 @@ | |||
| --- | |||
| @@ -1,12 +0,0 @@ | |||
| --- | |||
| psql -p "{{ postgresql_cluster_port }}" -t -X -A -c "select rolpassword from pg_authid where rolname = '{{ item.name }}'" | ||
| become_user: "{{ postgresql_cluster_user }}" | ||
| loop: "{{ postgresql_cluster_users | default([]) | flatten(levels=1) }}" | ||
| when: not (postgresql_cluster_is_monitor | default('False') | bool) and (inventory_hostname == play_hosts[1]) |
There was a problem hiding this comment.
is play_hosts[1] the first host or the second?
There was a problem hiding this comment.
This runs on af-database-01 (af-monitor-01 is play_hosts[0]). This has always been the active node on deployment for me. Both nodes should be read-only anyway (but the monitor will not work for sure). The goal is to only run on one database node as it was easier for me to implement this solution than it was to try and deal with pulling the results and pushing the template to each machine individually using the previous method that was implemented when it was just md5 which was easily supported in Ansible.
There was a problem hiding this comment.
Okay, I understand that, but I'd prefer for this to not be decided by an index in play_hosts. Instead we should get the first non manager node instead by filtering the list of hosts in the inventory by postgresql_cluster_is_monitor and then using the first element in that list.
There was a problem hiding this comment.
Also, play_hosts seems to be deprecated:
https://docs.ansible.com/ansible/latest/reference_appendices/special_variables.html#term-play_hosts
| - name: "create user {{ user.name }}" | ||
| user: | ||
| name: "{{ user.name }}" | ||
| - name: "create user {{ item.name }}" |
There was a problem hiding this comment.
whenever we do an iteration across a tasks file, I actually prefer to have explicitly named loop variables so that there are no accidentaly collisions if we loop inside the task again. Also, "item" is just less self explanatory than "user"
| # ansible_ssh_port: "22" | ||
| ansible_ssh_user: "vagrant" | ||
| ansible_ssh_private_key_file: "{{ playbook_dir }}/test/simple/.vagrant/machines/monitor/virtualbox/private_key" | ||
| ansible_ssh_private_key_file: "/home/fred/code/wannabegeekster/pg_auto_failover_ansible/test/simple/.vagrant/machines/monitor/libvirt/private_key" |
There was a problem hiding this comment.
same for the other occurences in this file
There was a problem hiding this comment.
Apologize for that. Should have grepped for that for sure. It was very late after a very long day. I definitely did not want that in a public repo.
There was a problem hiding this comment.
no worries, that's what a code review is for.
| @@ -1,4 +1,4 @@ | |||
| "{{ postgresql_cluster_pg_bouncer_root_user | default('pgbounceradmin') }}" "{{ postgresql_cluster_pg_bouncer_root_password_hashed | default('md5' + ((postgresql_cluster_pg_bouncer_root_password + (postgresql_cluster_pg_bouncer_root_user | default('pgbounceradmin'))) | hash('md5'))) }}" | |||
There was a problem hiding this comment.
I have never used pg_bouncer before. Can I just login to the 6432 port with the pgbounceradmin user? I am not 100% sure what this account is for, but I can definitely do some more research and figure it out as well.
How is this user getting added to postgres as I don't see them being added anywhere. Unless I am missing something. For me to get the correct scram-sha-256 hashed password I have to pull it directly from Postgres.
I wil DMOR tonight and see what I can figure out here. Everything was working, so I didn't understand fully what this account was used for to be fair.
| {% for user in postgresql_cluster_users | default([]) %} | ||
| "{{ user.name }}" "{{ user.hashed_password | default('md5' + ((user.password + user.name) | hash('md5'))) }}" | ||
| {% endfor %} | ||
| {% for user in passsword_hashes.results | default([]) %} |
There was a problem hiding this comment.
i see we remove support for md5 altogether. Should we do that?
There was a problem hiding this comment.
It is recommended to remove MD5 support completely. MD5 support is disabled by default in Postgres 14 and above. It is highly insecure and IMHO poor practice to enable it as the Postgres team specifically disables it by default.
There was a problem hiding this comment.
okay. understood. Then we should get rid of it completely.
|
Thank you for the PR. I left some comments on it. Most of it is fine, but I am a bit worried about dropping support of old pg auto failover versions as well as some md5/scram-sha-256 confusions. Let's clear these things up! |
|
@WannaBeGeekster do you still want to pick this up? |
|
Apologize, have some health challenges, definitely want to still handle this, will hopefully have an update by next weekend. |
|
@WannaBeGeekster don't worry about it. Just checking in. Take care and thanks again for your willingness to pick this up! |
2 participants
… and 16
All tests run properly in Ubuntu 20.04 and 22.04. Several other changes were implemented. Including full support of scram-sha-256 for Postgres and pgbouncer.
All code has been scanned with steampunk spotter for best practices and to ensure compatibility with latest Ansible versions.