From fa254ccd7845436b380301cb0ff4bc89db9821d0 Mon Sep 17 00:00:00 2001 From: Chris Bandy Date: Thu, 27 Feb 2025 10:47:16 -0600 Subject: [PATCH 1/3] Remove handling of HBA role/group membership We don't use it, and how to quote/escape it has been underspecified for years. --- internal/postgres/hba.go | 6 ------ internal/postgres/hba_test.go | 4 ++-- 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/internal/postgres/hba.go b/internal/postgres/hba.go index 3163b3307b..a4ab340c68 100644 --- a/internal/postgres/hba.go +++ b/internal/postgres/hba.go @@ -116,12 +116,6 @@ func (hba *HostBasedAuthentication) Replication() *HostBasedAuthentication { return hba } -// Role makes hba match connections by users that are members of a specific role. -func (hba *HostBasedAuthentication) Role(name string) *HostBasedAuthentication { - hba.user = "+" + hba.quote(name) - return hba -} - // SameNetwork makes hba match connection attempts from IP addresses in any // subnet to which the server is directly connected. func (hba *HostBasedAuthentication) SameNetwork() *HostBasedAuthentication { diff --git a/internal/postgres/hba_test.go b/internal/postgres/hba_test.go index 7457b7f649..a165c2f536 100644 --- a/internal/postgres/hba_test.go +++ b/internal/postgres/hba_test.go @@ -52,8 +52,8 @@ func TestHostBasedAuthentication(t *testing.T) { User("KD6-3.7").Method("scram-sha-256"). String()) - assert.Equal(t, `hostssl "data" +"admin" all md5 clientcert="verify-ca"`, - NewHBA().TLS().Database("data").Role("admin"). + assert.Equal(t, `hostssl "data" all all md5 clientcert="verify-ca"`, + NewHBA().TLS().Database("data"). Method("md5").Options(map[string]string{"clientcert": "verify-ca"}). String()) From 955bc816666fb310309db0c0893f3c5c00281256 Mon Sep 17 00:00:00 2001 From: Chris Bandy Date: Wed, 26 Feb 2025 16:48:57 -0600 Subject: [PATCH 2/3] Calculate Postgres HBA rules in the controller The controller assigned mandatory and default rules but did nothing with rules defined in the spec. The patroni.DynamicConfiguration function was interpreting its schemaless fields while also combining Postgres HBA rules from elsewhere. Its tests were long and complicated. 1. Postgres HBA rules are now extracted from the schemaless Patroni field in their own function with its own tests. 2. The PostgresCluster controller now creates a single set of HBA rules based on all the fields of the PostgresCluster spec. 3. The DynamicConfiguration function is simpler (19 lines, 18% smaller) and easier to test. --- .../controller/postgrescluster/cluster.go | 2 +- .../controller/postgrescluster/controller.go | 7 +- .../controller/postgrescluster/patroni.go | 2 +- .../controller/postgrescluster/postgres.go | 30 ++++ .../postgrescluster/postgres_test.go | 36 +++++ internal/patroni/config.go | 29 +--- internal/patroni/config_test.go | 152 ++---------------- internal/patroni/postgres.go | 27 ++++ internal/patroni/postgres_test.go | 85 ++++++++++ internal/patroni/reconcile.go | 2 +- internal/patroni/reconcile_test.go | 11 +- internal/postgres/hba.go | 43 +++++ internal/postgres/hba_test.go | 66 ++++++++ 13 files changed, 314 insertions(+), 178 deletions(-) diff --git a/internal/controller/postgrescluster/cluster.go b/internal/controller/postgrescluster/cluster.go index 4cd62f60c8..ead4881b1e 100644 --- a/internal/controller/postgrescluster/cluster.go +++ b/internal/controller/postgrescluster/cluster.go @@ -30,7 +30,7 @@ import ( // files (etc) that apply to the entire cluster. func (r *Reconciler) reconcileClusterConfigMap( ctx context.Context, cluster *v1beta1.PostgresCluster, - pgHBAs postgres.HBAs, pgParameters *postgres.ParameterSet, + pgHBAs *postgres.OrderedHBAs, pgParameters *postgres.ParameterSet, ) (*corev1.ConfigMap, error) { clusterConfigMap := &corev1.ConfigMap{ObjectMeta: naming.ClusterConfigMap(cluster)} clusterConfigMap.SetGroupVersionKind(corev1.SchemeGroupVersion.WithKind("ConfigMap")) diff --git a/internal/controller/postgrescluster/controller.go b/internal/controller/postgrescluster/controller.go index 4de285e559..bbe141c0b4 100644 --- a/internal/controller/postgrescluster/controller.go +++ b/internal/controller/postgrescluster/controller.go @@ -33,8 +33,6 @@ import ( "github.com/crunchydata/postgres-operator/internal/initialize" "github.com/crunchydata/postgres-operator/internal/kubernetes" "github.com/crunchydata/postgres-operator/internal/logging" - "github.com/crunchydata/postgres-operator/internal/pgbouncer" - "github.com/crunchydata/postgres-operator/internal/pgmonitor" "github.com/crunchydata/postgres-operator/internal/pki" "github.com/crunchydata/postgres-operator/internal/postgres" "github.com/crunchydata/postgres-operator/internal/registration" @@ -231,10 +229,7 @@ func (r *Reconciler) Reconcile( } } - pgHBAs := postgres.NewHBAs() - pgmonitor.PostgreSQLHBAs(ctx, cluster, &pgHBAs) - pgbouncer.PostgreSQL(cluster, &pgHBAs) - + pgHBAs := r.generatePostgresHBAs(ctx, cluster) pgParameters := r.generatePostgresParameters(ctx, cluster, backupsSpecFound) otelConfig := collector.NewConfigForPostgresPod(ctx, cluster, pgParameters) diff --git a/internal/controller/postgrescluster/patroni.go b/internal/controller/postgrescluster/patroni.go index 5242169be6..af3a3b8cca 100644 --- a/internal/controller/postgrescluster/patroni.go +++ b/internal/controller/postgrescluster/patroni.go @@ -173,7 +173,7 @@ func (r *Reconciler) reconcilePatroniDistributedConfiguration( func (r *Reconciler) reconcilePatroniDynamicConfiguration( ctx context.Context, cluster *v1beta1.PostgresCluster, instances *observedInstances, - pgHBAs postgres.HBAs, pgParameters *postgres.ParameterSet, + pgHBAs *postgres.OrderedHBAs, pgParameters *postgres.ParameterSet, ) error { if !patroni.ClusterBootstrapped(cluster) { // Patroni has not yet bootstrapped. Dynamic configuration happens through diff --git a/internal/controller/postgrescluster/postgres.go b/internal/controller/postgrescluster/postgres.go index 25ffeefc99..162192f308 100644 --- a/internal/controller/postgrescluster/postgres.go +++ b/internal/controller/postgrescluster/postgres.go @@ -33,6 +33,7 @@ import ( "github.com/crunchydata/postgres-operator/internal/patroni" "github.com/crunchydata/postgres-operator/internal/pgaudit" "github.com/crunchydata/postgres-operator/internal/pgbackrest" + "github.com/crunchydata/postgres-operator/internal/pgbouncer" "github.com/crunchydata/postgres-operator/internal/pgmonitor" "github.com/crunchydata/postgres-operator/internal/postgis" "github.com/crunchydata/postgres-operator/internal/postgres" @@ -41,6 +42,35 @@ import ( "github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1" ) +// generatePostgresHBAs produces the HBA rules for cluster that incorporates, +// from highest to lowest precedence: +// 1. mandatory rules determined by controllers +// 2. rules in cluster.spec.patroni.dynamicConfiguration +// 3. default rules, when none were in cluster.spec +func (*Reconciler) generatePostgresHBAs( + ctx context.Context, cluster *v1beta1.PostgresCluster, +) *postgres.OrderedHBAs { + builtin := postgres.NewHBAs() + pgmonitor.PostgreSQLHBAs(ctx, cluster, &builtin) + pgbouncer.PostgreSQL(cluster, &builtin) + + // Postgres processes HBA rules in order. Start with mandatory rules + // so connections are matched against them first. + result := new(postgres.OrderedHBAs) + result.Append(builtin.Mandatory...) + + // Append any rules specified in the the Patroni section. + before := result.Length() + result.AppendUnstructured(patroni.PostgresHBAs(cluster.Spec.Patroni)...) + + // When there are no specified rules, include the recommended defaults. + if result.Length() == before { + result.Append(builtin.Default...) + } + + return result +} + // generatePostgresParameters produces the parameter set for cluster that // incorporates, from highest to lowest precedence: // 1. mandatory values determined by controllers diff --git a/internal/controller/postgrescluster/postgres_test.go b/internal/controller/postgrescluster/postgres_test.go index f6da644a09..4f967d1088 100644 --- a/internal/controller/postgrescluster/postgres_test.go +++ b/internal/controller/postgrescluster/postgres_test.go @@ -34,6 +34,42 @@ import ( "github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1" ) +func TestGeneratePostgresHBAs(t *testing.T) { + ctx := context.Background() + reconciler := &Reconciler{} + + builtin := reconciler.generatePostgresHBAs(ctx, v1beta1.NewPostgresCluster()).AsStrings() + assert.Assert(t, len(builtin) > 0, + "expected an empty cluster to have some builtin rules") + + defaults := builtin[len(builtin)-1:] + assert.Assert(t, len(defaults) > 0, + "expected at least one default rule") + + required := builtin[:len(builtin)-len(defaults)] + assert.Assert(t, len(required) > 0, + "expected at least one mandatory rule") + + t.Run("Patroni", func(t *testing.T) { + cluster := v1beta1.NewPostgresCluster() + require.UnmarshalInto(t, &cluster.Spec.Patroni, `{ + dynamicConfiguration: { + postgresql: { pg_hba: [ "first custom", "another" ] }, + }, + }`) + + result := reconciler.generatePostgresHBAs(ctx, cluster).AsStrings() + assert.Assert(t, cmp.Len(result, len(required)+2), + "expected two rules from the Patroni section and no defaults") + + // mandatory rules should be first + assert.DeepEqual(t, result[:len(required)], required) + + // specified rules should be last and in their original order + assert.DeepEqual(t, result[len(required):], []string{`first custom`, `another`}) + }) +} + func TestGeneratePostgresParameters(t *testing.T) { ctx := context.Background() reconciler := &Reconciler{} diff --git a/internal/patroni/config.go b/internal/patroni/config.go index 2174607c63..bee17bbb94 100644 --- a/internal/patroni/config.go +++ b/internal/patroni/config.go @@ -39,7 +39,7 @@ const ( // clusterYAML returns Patroni settings that apply to the entire cluster. func clusterYAML( cluster *v1beta1.PostgresCluster, - pgHBAs postgres.HBAs, parameters *postgres.ParameterSet, patroniLogStorageLimit int64, + pgHBAs *postgres.OrderedHBAs, parameters *postgres.ParameterSet, patroniLogStorageLimit int64, ) (string, error) { root := map[string]any{ // The cluster identifier. This value cannot change during the cluster's @@ -208,7 +208,7 @@ func clusterYAML( // and returns a value that can be marshaled to JSON. func DynamicConfiguration( spec *v1beta1.PostgresClusterSpec, - pgHBAs postgres.HBAs, parameters *postgres.ParameterSet, + pgHBAs *postgres.OrderedHBAs, parameters *postgres.ParameterSet, ) map[string]any { // Copy the entire configuration before making any changes. root := make(map[string]any) @@ -239,32 +239,13 @@ func DynamicConfiguration( postgresql[k] = v } } - root["postgresql"] = postgresql - if m := parameters.AsMap(); m != nil { postgresql["parameters"] = m } - - // Copy the "postgresql.pg_hba" section after any mandatory values. - hba := make([]string, 0, len(pgHBAs.Mandatory)) - for i := range pgHBAs.Mandatory { - hba = append(hba, pgHBAs.Mandatory[i].String()) + if pgHBAs != nil { + postgresql["pg_hba"] = pgHBAs.AsStrings() } - if section, ok := postgresql["pg_hba"].([]any); ok { - for i := range section { - // any pg_hba values that are not strings will be skipped - if value, ok := section[i].(string); ok { - hba = append(hba, value) - } - } - } - // When the section is missing or empty, include the recommended defaults. - if len(hba) == len(pgHBAs.Mandatory) { - for i := range pgHBAs.Default { - hba = append(hba, pgHBAs.Default[i].String()) - } - } - postgresql["pg_hba"] = hba + root["postgresql"] = postgresql // Enabling `pg_rewind` allows a former primary to automatically rejoin the // cluster even if it has commits that were not sent to a replica. In other diff --git a/internal/patroni/config_test.go b/internal/patroni/config_test.go index d5ce0eb81d..5386454a47 100644 --- a/internal/patroni/config_test.go +++ b/internal/patroni/config_test.go @@ -32,7 +32,7 @@ func TestClusterYAML(t *testing.T) { cluster.Namespace = "some-namespace" cluster.Name = "cluster-name" - data, err := clusterYAML(cluster, postgres.HBAs{}, postgres.NewParameterSet(), 0) + data, err := clusterYAML(cluster, nil, nil, 0) assert.NilError(t, err) assert.Equal(t, data, strings.TrimSpace(` # Generated by postgres-operator. DO NOT EDIT. @@ -41,8 +41,6 @@ bootstrap: dcs: loop_wait: 10 postgresql: - parameters: {} - pg_hba: [] use_pg_rewind: false use_slots: false ttl: 30 @@ -91,7 +89,7 @@ watchdog: cluster.Name = "cluster-name" cluster.Spec.PostgresVersion = 14 - data, err := clusterYAML(cluster, postgres.HBAs{}, postgres.NewParameterSet(), 0) + data, err := clusterYAML(cluster, nil, nil, 0) assert.NilError(t, err) assert.Equal(t, data, strings.TrimSpace(` # Generated by postgres-operator. DO NOT EDIT. @@ -100,8 +98,6 @@ bootstrap: dcs: loop_wait: 10 postgresql: - parameters: {} - pg_hba: [] use_pg_rewind: true use_slots: false ttl: 30 @@ -159,7 +155,7 @@ watchdog: Level: &logLevel, } - data, err := clusterYAML(cluster, postgres.HBAs{}, postgres.NewParameterSet(), 1000) + data, err := clusterYAML(cluster, nil, nil, 1000) assert.NilError(t, err) assert.Equal(t, data, strings.TrimSpace(` # Generated by postgres-operator. DO NOT EDIT. @@ -168,8 +164,6 @@ bootstrap: dcs: loop_wait: 10 postgresql: - parameters: {} - pg_hba: [] use_pg_rewind: true use_slots: false ttl: 30 @@ -230,10 +224,16 @@ func TestDynamicConfiguration(t *testing.T) { return out } + rules := func(in ...string) *postgres.OrderedHBAs { + out := new(postgres.OrderedHBAs) + out.AppendUnstructured(in...) + return out + } + for _, tt := range []struct { name string spec string - hbas postgres.HBAs + hbas *postgres.OrderedHBAs params *postgres.ParameterSet expected map[string]any }{ @@ -243,7 +243,6 @@ func TestDynamicConfiguration(t *testing.T) { "loop_wait": int32(10), "ttl": int32(30), "postgresql": map[string]any{ - "pg_hba": []string{}, "use_pg_rewind": true, "use_slots": false, }, @@ -263,7 +262,6 @@ func TestDynamicConfiguration(t *testing.T) { "ttl": int32(30), "retry_timeout": int64(5), "postgresql": map[string]any{ - "pg_hba": []string{}, "use_pg_rewind": true, "use_slots": false, }, @@ -285,7 +283,6 @@ func TestDynamicConfiguration(t *testing.T) { "loop_wait": int32(8), "ttl": int32(99), "postgresql": map[string]any{ - "pg_hba": []string{}, "use_pg_rewind": true, "use_slots": false, }, @@ -304,7 +301,6 @@ func TestDynamicConfiguration(t *testing.T) { "loop_wait": int32(10), "ttl": int32(30), "postgresql": map[string]any{ - "pg_hba": []string{}, "use_pg_rewind": true, "use_slots": false, }, @@ -326,7 +322,6 @@ func TestDynamicConfiguration(t *testing.T) { "loop_wait": int32(10), "ttl": int32(30), "postgresql": map[string]any{ - "pg_hba": []string{}, "use_pg_rewind": true, "use_slots": "input", }, @@ -359,142 +354,28 @@ func TestDynamicConfiguration(t *testing.T) { "another": "5", "unrelated": "default", }, - "pg_hba": []string{}, - "use_pg_rewind": true, - "use_slots": false, - }, - }, - }, - { - name: "postgresql.pg_hba: wrong-type is ignored", - spec: `{ - patroni: { - dynamicConfiguration: { - postgresql: { - pg_hba: true, - }, - }, - }, - }`, - expected: map[string]any{ - "loop_wait": int32(10), - "ttl": int32(30), - "postgresql": map[string]any{ - "pg_hba": []string{}, - "use_pg_rewind": true, - "use_slots": false, - }, - }, - }, - { - name: "postgresql.pg_hba: default when no input", - spec: `{ - patroni: { - dynamicConfiguration: { - postgresql: { - pg_hba: null, - }, - }, - }, - }`, - hbas: postgres.HBAs{ - Default: []*postgres.HostBasedAuthentication{ - postgres.NewHBA().Local().Method("peer"), - }, - }, - expected: map[string]any{ - "loop_wait": int32(10), - "ttl": int32(30), - "postgresql": map[string]any{ - "pg_hba": []string{ - "local all all peer", - }, "use_pg_rewind": true, "use_slots": false, }, }, }, { - name: "postgresql.pg_hba: no default when input", + name: "HBA pass through", spec: `{ patroni: { dynamicConfiguration: { postgresql: { - pg_hba: [custom], + pg_hba: [calculated, elsewhere], }, }, }, }`, - hbas: postgres.HBAs{ - Default: []*postgres.HostBasedAuthentication{ - postgres.NewHBA().Local().Method("peer"), - }, - }, + hbas: rules("function args"), expected: map[string]any{ "loop_wait": int32(10), "ttl": int32(30), "postgresql": map[string]any{ - "pg_hba": []string{ - "custom", - }, - "use_pg_rewind": true, - "use_slots": false, - }, - }, - }, - { - name: "postgresql.pg_hba: mandatory before others", - spec: `{ - patroni: { - dynamicConfiguration: { - postgresql: { - pg_hba: [custom], - }, - }, - }, - }`, - hbas: postgres.HBAs{ - Mandatory: []*postgres.HostBasedAuthentication{ - postgres.NewHBA().Local().Method("peer"), - }, - }, - expected: map[string]any{ - "loop_wait": int32(10), - "ttl": int32(30), - "postgresql": map[string]any{ - "pg_hba": []string{ - "local all all peer", - "custom", - }, - "use_pg_rewind": true, - "use_slots": false, - }, - }, - }, - { - name: "postgresql.pg_hba: ignore non-string types", - spec: `{ - patroni: { - dynamicConfiguration: { - postgresql: { - pg_hba: [1, true, custom, {}, []], - }, - }, - }, - }`, - hbas: postgres.HBAs{ - Mandatory: []*postgres.HostBasedAuthentication{ - postgres.NewHBA().Local().Method("peer"), - }, - }, - expected: map[string]any{ - "loop_wait": int32(10), - "ttl": int32(30), - "postgresql": map[string]any{ - "pg_hba": []string{ - "local all all peer", - "custom", - }, + "pg_hba": []string{"function args"}, "use_pg_rewind": true, "use_slots": false, }, @@ -515,7 +396,6 @@ func TestDynamicConfiguration(t *testing.T) { "loop_wait": int32(10), "ttl": int32(30), "postgresql": map[string]any{ - "pg_hba": []string{}, "use_pg_rewind": true, "use_slots": false, }, @@ -550,7 +430,6 @@ func TestDynamicConfiguration(t *testing.T) { "parameters": map[string]string{ "restore_command": "mandatory", }, - "pg_hba": []string{}, "use_pg_rewind": true, "use_slots": false, }, @@ -590,7 +469,6 @@ func TestDynamicConfiguration(t *testing.T) { "parameters": map[string]string{ "restore_command": "mandatory", }, - "pg_hba": []string{}, "use_pg_rewind": true, "use_slots": false, }, @@ -632,7 +510,6 @@ func TestDynamicConfiguration(t *testing.T) { "parameters": map[string]string{ "restore_command": "mandatory", }, - "pg_hba": []string{}, "use_pg_rewind": true, "use_slots": false, }, @@ -665,7 +542,6 @@ func TestDynamicConfiguration(t *testing.T) { "parameters": map[string]string{ "encryption_key_command": "echo one", }, - "pg_hba": []string{}, "use_pg_rewind": bool(true), "use_slots": bool(false), }, diff --git a/internal/patroni/postgres.go b/internal/patroni/postgres.go index cb686312fa..519fc30c04 100644 --- a/internal/patroni/postgres.go +++ b/internal/patroni/postgres.go @@ -12,6 +12,33 @@ import ( "github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1" ) +// PostgresHBAs returns the HBA rules in spec, if any. +func PostgresHBAs(spec *v1beta1.PatroniSpec) []string { + var result []string + + if spec != nil { + // DynamicConfiguration lacks an OpenAPI schema, so it may contain any type + // at any depth. Navigate the object and skip HBA values that aren't string. + // + // Patroni expects a list of strings: + // https://github.com/patroni/patroni/blob/v4.0.0/patroni/validator.py#L1170 + // + if root := spec.DynamicConfiguration; root != nil { + if postgresql, ok := root["postgresql"].(map[string]any); ok { + if section, ok := postgresql["pg_hba"].([]any); ok { + for i := range section { + if value, ok := section[i].(string); ok { + result = append(result, value) + } + } + } + } + } + } + + return result +} + // PostgresParameters returns the Postgres parameters in spec, if any. func PostgresParameters(spec *v1beta1.PatroniSpec) *postgres.ParameterSet { result := postgres.NewParameterSet() diff --git a/internal/patroni/postgres_test.go b/internal/patroni/postgres_test.go index 16fdc30fdf..becd1b1743 100644 --- a/internal/patroni/postgres_test.go +++ b/internal/patroni/postgres_test.go @@ -13,6 +13,91 @@ import ( "github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1" ) +func TestPostgresHBAs(t *testing.T) { + t.Run("Zero", func(t *testing.T) { + result := PostgresHBAs(nil) + + assert.Assert(t, result == nil) + }) + + t.Run("NoDynamicConfig", func(t *testing.T) { + spec := new(v1beta1.PatroniSpec) + result := PostgresHBAs(spec) + + assert.Assert(t, result == nil) + }) + + t.Run("NoPostgreSQL", func(t *testing.T) { + spec := new(v1beta1.PatroniSpec) + require.UnmarshalInto(t, spec, `{ + dynamicConfiguration: {}, + }`) + + result := PostgresHBAs(spec) + assert.Assert(t, result == nil) + + t.Run("WrongType", func(t *testing.T) { + require.UnmarshalInto(t, spec, `{ + dynamicConfiguration: { + postgresql: asdf, + }, + }`) + + result := PostgresHBAs(spec) + assert.Assert(t, result == nil) + }) + }) + + t.Run("NoHBAs", func(t *testing.T) { + spec := new(v1beta1.PatroniSpec) + require.UnmarshalInto(t, spec, `{ + dynamicConfiguration: { + postgresql: { + use_pg_rewind: true, + }, + }, + }`) + + result := PostgresHBAs(spec) + assert.Assert(t, result == nil) + + t.Run("WrongType", func(t *testing.T) { + require.UnmarshalInto(t, spec, `{ + dynamicConfiguration: { + postgresql: { + pg_hba: asdf, + }, + }, + }`) + + result := PostgresHBAs(spec) + assert.Assert(t, result == nil) + }) + }) + + t.Run("HBAs", func(t *testing.T) { + spec := new(v1beta1.PatroniSpec) + require.UnmarshalInto(t, spec, `{ + dynamicConfiguration: { + postgresql: { + pg_hba: [ + "host all all all trust", + true, + "total garbage, yikes", + 123, + ], + }, + }, + }`) + + result := PostgresHBAs(spec) + assert.DeepEqual(t, result, []string{ + "host all all all trust", + "total garbage, yikes", + }) + }) +} + func TestPostgresParameters(t *testing.T) { t.Run("Zero", func(t *testing.T) { result := PostgresParameters(nil) diff --git a/internal/patroni/reconcile.go b/internal/patroni/reconcile.go index 394a33d6d5..a8de99f028 100644 --- a/internal/patroni/reconcile.go +++ b/internal/patroni/reconcile.go @@ -29,7 +29,7 @@ func ClusterBootstrapped(postgresCluster *v1beta1.PostgresCluster) bool { // ClusterConfigMap populates the shared ConfigMap with fields needed to run Patroni. func ClusterConfigMap(ctx context.Context, inCluster *v1beta1.PostgresCluster, - inHBAs postgres.HBAs, + inHBAs *postgres.OrderedHBAs, inParameters *postgres.ParameterSet, outClusterConfigMap *corev1.ConfigMap, patroniLogStorageLimit int64, diff --git a/internal/patroni/reconcile_test.go b/internal/patroni/reconcile_test.go index 9a82dfde2d..729bd6573d 100644 --- a/internal/patroni/reconcile_test.go +++ b/internal/patroni/reconcile_test.go @@ -14,7 +14,6 @@ import ( "github.com/crunchydata/postgres-operator/internal/naming" "github.com/crunchydata/postgres-operator/internal/pki" - "github.com/crunchydata/postgres-operator/internal/postgres" "github.com/crunchydata/postgres-operator/internal/testing/cmp" "github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1" ) @@ -24,20 +23,18 @@ func TestClusterConfigMap(t *testing.T) { ctx := context.Background() cluster := new(v1beta1.PostgresCluster) - pgHBAs := postgres.HBAs{} - pgParameters := postgres.NewParameterSet() - cluster.Default() + config := new(corev1.ConfigMap) - assert.NilError(t, ClusterConfigMap(ctx, cluster, pgHBAs, pgParameters, config, 0)) + assert.NilError(t, ClusterConfigMap(ctx, cluster, nil, nil, config, 0)) // The output of clusterYAML should go into config. - data, _ := clusterYAML(cluster, pgHBAs, pgParameters, 0) + data, _ := clusterYAML(cluster, nil, nil, 0) assert.DeepEqual(t, config.Data["patroni.yaml"], data) // No change when called again. before := config.DeepCopy() - assert.NilError(t, ClusterConfigMap(ctx, cluster, pgHBAs, pgParameters, config, 0)) + assert.NilError(t, ClusterConfigMap(ctx, cluster, nil, nil, config, 0)) assert.DeepEqual(t, config, before) } diff --git a/internal/postgres/hba.go b/internal/postgres/hba.go index a4ab340c68..b6bf690074 100644 --- a/internal/postgres/hba.go +++ b/internal/postgres/hba.go @@ -6,6 +6,7 @@ package postgres import ( "fmt" + "slices" "strings" ) @@ -151,3 +152,45 @@ func (hba *HostBasedAuthentication) String() string { return strings.TrimSpace(fmt.Sprintf("%s %s %s %s %s %s", hba.origin, hba.database, hba.user, hba.address, hba.method, hba.options)) } + +// OrderedHBAs is an append-only sequence of pg_hba.conf lines. +type OrderedHBAs struct { + records []string +} + +// Append renders and adds pg_hba.conf lines to o. Nil pointers are ignored. +func (o *OrderedHBAs) Append(hbas ...*HostBasedAuthentication) { + o.records = slices.Grow(o.records, len(hbas)) + + for _, hba := range hbas { + if hba != nil { + o.records = append(o.records, hba.String()) + } + } +} + +// AppendUnstructured trims and adds unvalidated pg_hba.conf lines to o. +// Empty lines and lines that are entirely control characters are omitted. +func (o *OrderedHBAs) AppendUnstructured(hbas ...string) { + o.records = slices.Grow(o.records, len(hbas)) + + for _, hba := range hbas { + hba = strings.TrimFunc(hba, func(r rune) bool { + // control characters, space, and backslash + return r > '~' || r < '!' || r == '\\' + }) + if len(hba) > 0 { + o.records = append(o.records, hba) + } + } +} + +// AsStrings returns a copy of o as a slice. +func (o *OrderedHBAs) AsStrings() []string { + return slices.Clone(o.records) +} + +// Length returns the number of records in o. +func (o *OrderedHBAs) Length() int { + return len(o.records) +} diff --git a/internal/postgres/hba_test.go b/internal/postgres/hba_test.go index a165c2f536..c15cf5cc77 100644 --- a/internal/postgres/hba_test.go +++ b/internal/postgres/hba_test.go @@ -60,3 +60,69 @@ func TestHostBasedAuthentication(t *testing.T) { assert.Equal(t, `hostnossl all all all reject`, NewHBA().NoSSL().Method("reject").String()) } + +func TestOrderedHBAs(t *testing.T) { + ordered := new(OrderedHBAs) + + // The zero value is empty. + assert.Equal(t, ordered.Length(), 0) + assert.Assert(t, cmp.Len(ordered.AsStrings(), 0)) + + // Append can be called without arguments. + ordered.Append() + ordered.AppendUnstructured() + assert.Assert(t, cmp.Len(ordered.AsStrings(), 0)) + + // Append adds to the end of the slice. + ordered.Append(NewHBA()) + assert.Equal(t, ordered.Length(), 1) + assert.DeepEqual(t, ordered.AsStrings(), []string{ + `all all all`, + }) + + // AppendUnstructured adds to the end of the slice. + ordered.AppendUnstructured("could be anything, really") + assert.Equal(t, ordered.Length(), 2) + assert.DeepEqual(t, ordered.AsStrings(), []string{ + `all all all`, + `could be anything, really`, + }) + + // Append and AppendUnstructured do not have a separate order. + ordered.Append(NewHBA().User("zoro")) + assert.Equal(t, ordered.Length(), 3) + assert.DeepEqual(t, ordered.AsStrings(), []string{ + `all all all`, + `could be anything, really`, + `all "zoro" all`, + }) + + t.Run("NilPointersIgnored", func(t *testing.T) { + rules := new(OrderedHBAs) + rules.Append( + NewHBA(), nil, + NewHBA(), nil, + ) + assert.DeepEqual(t, rules.AsStrings(), []string{ + `all all all`, + `all all all`, + }) + }) + + t.Run("SpecialCharactersStripped", func(t *testing.T) { + rules := new(OrderedHBAs) + rules.AppendUnstructured( + " \n\t things \n\n\n", + `with # comment`, + " \n\t \\\\ \f", // entirely special characters + `trailing slashes \\\`, + "multiple \\\n lines okay", + ) + assert.DeepEqual(t, rules.AsStrings(), []string{ + `things`, + `with # comment`, + `trailing slashes`, + "multiple \\\n lines okay", + }) + }) +} From 7b40de56be065b4aa3bb45a34f72161ca809c5e4 Mon Sep 17 00:00:00 2001 From: Chris Bandy Date: Mon, 3 Mar 2025 16:47:08 -0600 Subject: [PATCH 3/3] FIXUP: review, Mar 3 --- internal/controller/postgrescluster/postgres.go | 2 +- internal/postgres/hba.go | 4 ---- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/internal/controller/postgrescluster/postgres.go b/internal/controller/postgrescluster/postgres.go index 162192f308..a2f6fb8611 100644 --- a/internal/controller/postgrescluster/postgres.go +++ b/internal/controller/postgrescluster/postgres.go @@ -59,7 +59,7 @@ func (*Reconciler) generatePostgresHBAs( result := new(postgres.OrderedHBAs) result.Append(builtin.Mandatory...) - // Append any rules specified in the the Patroni section. + // Append any rules specified in the Patroni section. before := result.Length() result.AppendUnstructured(patroni.PostgresHBAs(cluster.Spec.Patroni)...) diff --git a/internal/postgres/hba.go b/internal/postgres/hba.go index b6bf690074..875a476ee2 100644 --- a/internal/postgres/hba.go +++ b/internal/postgres/hba.go @@ -160,8 +160,6 @@ type OrderedHBAs struct { // Append renders and adds pg_hba.conf lines to o. Nil pointers are ignored. func (o *OrderedHBAs) Append(hbas ...*HostBasedAuthentication) { - o.records = slices.Grow(o.records, len(hbas)) - for _, hba := range hbas { if hba != nil { o.records = append(o.records, hba.String()) @@ -172,8 +170,6 @@ func (o *OrderedHBAs) Append(hbas ...*HostBasedAuthentication) { // AppendUnstructured trims and adds unvalidated pg_hba.conf lines to o. // Empty lines and lines that are entirely control characters are omitted. func (o *OrderedHBAs) AppendUnstructured(hbas ...string) { - o.records = slices.Grow(o.records, len(hbas)) - for _, hba := range hbas { hba = strings.TrimFunc(hba, func(r rune) bool { // control characters, space, and backslash