Enable replication connections by default in `pg_hba.conf` #222

myii · 2018-06-13T01:11:47Z

Upstream commit:

postgres/postgres@be37c21
9 Mar 2017
master REL_11_BETA1 REL_10_4 ... REL_10_BETA1

When installing version 10, default pg_hba.conf additionally contains:

# Allow replication connections from localhost, by a user with the
# replication privilege.
local   replication     all                                     peer
host    replication     all             127.0.0.1/32            md5
host    replication     all             ::1/128                 md5

Upstream commit: - postgres/postgres@be37c21 - 9 Mar 2017 - master REL_11_BETA1 REL_10_4 ... REL_10_BETA1

myii · 2018-06-16T20:45:17Z

@noelmcloughlin I've just added another commit, to separate the v10+ acls. Now these are only applied when the PostgreSQL server version is 10+. No spamming earlier versions.

Test output for version '9.6' (formatted):

[
    [u'local', u'all', u'all', u'peer'],
    [u'host', u'all', u'all', u'127.0.0.1/32', u'md5'],
    [u'host', u'all', u'all', u'::1/128', u'md5']
]

Test output for version '10' (formatted):

[
    [u'local', u'all', u'all', u'peer'],
    [u'host', u'all', u'all', u'127.0.0.1/32', u'md5'],
    [u'host', u'all', u'all', u'::1/128', u'md5'],
    [u'local', u'replication', u'all', u'peer'],
    [u'host', u'replication', u'all', u'127.0.0.1/32', u'md5'],
    [u'host', u'replication', u'all', u'::1/128', u'md5']
]

noelmcloughlin

Thanks @myii for the valuable PR. I just have some minor comments which are easy to resolve.

noelmcloughlin · 2018-06-17T10:43:27Z

postgres/defaults.yaml

@@ -2,7 +2,7 @@

 postgres:
  use_upstream_repo: True
-  version: '9.5'
+  version: '10'


Setting10 as "default" should be okay. Debian stretch ships with version 9.6 but the version parameter is paired with use_upstream_repo: True and version 10 seems to be widely available upstream, and for Debian stretch too. Approved.

noelmcloughlin · 2018-06-17T10:45:42Z

postgres/defaults.yaml

@@ -47,6 +47,12 @@ postgres:
    - ['host', 'all', 'all', '127.0.0.1/32', 'md5']
    # IPv6 local connections:
    - ['host', 'all', 'all', '::1/128', 'md5']
+  acls_v10:


Syntax and content looks fine. Consider changing identifier name from acls_v10 to acls_replication.

I'm not sure that having multiple settings for ACL is really that good idea.
I have checked PostgreSQL docs, and it seams the replication settings are supported starting at least from PG release 9.0.

So I think it would be no harm to include these ACL rules by default for any PG version.

noelmcloughlin · 2018-06-17T10:49:15Z

postgres/server/init.sls

@@ -107,6 +107,10 @@ postgresql-conf:
 {%- endif %}

 {%- set pg_hba_path = salt['file.join'](postgres.conf_dir, 'pg_hba.conf') %}
+{%- set acls = postgres.acls %}
+{%- if postgres.version|to_num >= 10 %}


Please use int instead of to_num (salt v2018), or even str_to_num (salt v2017). In this case the Jinja builtin filters are more portable. Not everybody uses latest Salt versions in production.

vutny

@noelmcloughlin @myii
I doubt about adding additional Pillar key for the replication case. Please see my comment below.

vutny · 2018-06-17T11:47:00Z

postgres/defaults.yaml

@@ -47,6 +47,12 @@ postgres:
    - ['host', 'all', 'all', '127.0.0.1/32', 'md5']
    # IPv6 local connections:
    - ['host', 'all', 'all', '::1/128', 'md5']
+  acls_v10:


I'm not sure that having multiple settings for ACL is really that good idea.
I have checked PostgreSQL docs, and it seams the replication settings are supported starting at least from PG release 9.0.

So I think it would be no harm to include these ACL rules by default for any PG version.

noelmcloughlin · 2018-06-17T12:14:26Z

Good catch @vutny. The purpose of the upstream commit was to "Enable replication connections by default in pg_hba.conf" so feature already existed. I too would prefer one [acls] block.

myii · 2018-06-17T12:56:15Z

Thanks for all of the feedback. When I get a chance, I'll rebase and let you know. ------------------ Sent from my BlackBerry

…

________________________________ From: N <[email protected]> Date: Sun, 17 Jun 2018 05:14:27 -0700 To: saltstack-formulas/postgres-formula<[email protected]> ReplyTo: saltstack-formulas/postgres-formula <[email protected]> Cc: Imran Iqbal<[email protected]>; Mention<[email protected]> Subject: Re: [saltstack-formulas/postgres-formula] Enable replication connections by default in `pg_hba.conf` (#222) Good catch @vutny<https://github.com/vutny>. The purpose of the upstream commit <postgres/postgres@be37c21> was to "Enable replication connections by default in pg_hba.conf" so feature already existed. I too would prefer one [acls] block. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#222 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AJwewS72t1qnlwvMQJREA_S1RxFJcLqXks5t9kgjgaJpZM4UlYZl>.

myii · 2018-06-18T01:29:38Z

@noelmcloughlin @vutny Rebased to remove the superfluous commit. Ready for final checks.

noelmcloughlin

Excellent work, thanks!!

aboe76 · 2018-06-19T06:23:44Z

Merged ! @myii @noelmcloughlin @vutny thanks for the work

myii · 2018-06-19T06:54:03Z

You're welcome.

myii added 2 commits June 13, 2018 02:07

Enable replication connections by default in pg_hba.conf

311cb3d

Upstream commit: - postgres/postgres@be37c21 - 9 Mar 2017 - master REL_11_BETA1 REL_10_4 ... REL_10_BETA1

Set default version to 10

9fdce93

aboe76 requested a review from noelmcloughlin June 16, 2018 19:02

myii force-pushed the PR_psql10_acls branch from f847974 to eb729ff Compare June 16, 2018 20:47

noelmcloughlin suggested changes Jun 17, 2018

View reviewed changes

vutny suggested changes Jun 17, 2018

View reviewed changes

myii force-pushed the PR_psql10_acls branch from eb729ff to 9fdce93 Compare June 17, 2018 23:37

noelmcloughlin approved these changes Jun 18, 2018

View reviewed changes

aboe76 merged commit aa91079 into saltstack-formulas:master Jun 19, 2018

myii deleted the PR_psql10_acls branch June 19, 2018 06:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable replication connections by default in `pg_hba.conf` #222

Enable replication connections by default in `pg_hba.conf` #222

myii commented Jun 13, 2018

myii commented Jun 16, 2018

noelmcloughlin left a comment

noelmcloughlin Jun 17, 2018

noelmcloughlin Jun 17, 2018

vutny Jun 17, 2018 •

edited

Loading

noelmcloughlin Jun 17, 2018

vutny left a comment

vutny Jun 17, 2018 •

edited

Loading

noelmcloughlin commented Jun 17, 2018

myii commented Jun 17, 2018 via email

myii commented Jun 18, 2018

noelmcloughlin left a comment

aboe76 commented Jun 19, 2018

myii commented Jun 19, 2018

Enable replication connections by default in pg_hba.conf #222

Enable replication connections by default in pg_hba.conf #222

Conversation

myii commented Jun 13, 2018

myii commented Jun 16, 2018

noelmcloughlin left a comment

Choose a reason for hiding this comment

noelmcloughlin Jun 17, 2018

Choose a reason for hiding this comment

noelmcloughlin Jun 17, 2018

Choose a reason for hiding this comment

vutny Jun 17, 2018 • edited Loading

Choose a reason for hiding this comment

noelmcloughlin Jun 17, 2018

Choose a reason for hiding this comment

vutny left a comment

Choose a reason for hiding this comment

vutny Jun 17, 2018 • edited Loading

Choose a reason for hiding this comment

noelmcloughlin commented Jun 17, 2018

myii commented Jun 17, 2018 via email

myii commented Jun 18, 2018

noelmcloughlin left a comment

Choose a reason for hiding this comment

aboe76 commented Jun 19, 2018

myii commented Jun 19, 2018

Enable replication connections by default in `pg_hba.conf` #222

Enable replication connections by default in `pg_hba.conf` #222

vutny Jun 17, 2018 •

edited

Loading

vutny Jun 17, 2018 •

edited

Loading