diff --git a/internal/controller/postgrescluster/patroni.go b/internal/controller/postgrescluster/patroni.go index 1c5ac93ee..fb6df0a6a 100644 --- a/internal/controller/postgrescluster/patroni.go +++ b/internal/controller/postgrescluster/patroni.go @@ -204,14 +204,9 @@ func (r *Reconciler) reconcilePatroniDynamicConfiguration( return r.PodExec(ctx, pod.Namespace, pod.Name, naming.ContainerDatabase, stdin, stdout, stderr, command...) } - var configuration map[string]any - if cluster.Spec.Patroni != nil { - configuration = cluster.Spec.Patroni.DynamicConfiguration - } - configuration = patroni.DynamicConfiguration(cluster, configuration, pgHBAs, pgParameters) - return errors.WithStack( - patroni.Executor(exec).ReplaceConfiguration(ctx, configuration)) + patroni.Executor(exec).ReplaceConfiguration(ctx, + patroni.DynamicConfiguration(&cluster.Spec, pgHBAs, pgParameters))) } // generatePatroniLeaderLeaseService returns a v1.Service that exposes the diff --git a/internal/patroni/config.go b/internal/patroni/config.go index 65b1f8d23..64645ec2d 100644 --- a/internal/patroni/config.go +++ b/internal/patroni/config.go @@ -179,13 +179,8 @@ func clusterYAML( // Patroni has not yet bootstrapped. Populate the "bootstrap.dcs" field to // facilitate it. When Patroni is already bootstrapped, this field is ignored. - var configuration map[string]any - if cluster.Spec.Patroni != nil { - configuration = cluster.Spec.Patroni.DynamicConfiguration - } - root["bootstrap"] = map[string]any{ - "dcs": DynamicConfiguration(cluster, configuration, pgHBAs, pgParameters), + "dcs": DynamicConfiguration(&cluster.Spec, pgHBAs, pgParameters), // Missing here is "users" which runs *after* "post_bootstrap". It is // not possible to use roles created by the former in the latter. @@ -200,20 +195,19 @@ func clusterYAML( // DynamicConfiguration combines configuration with some PostgreSQL settings // and returns a value that can be marshaled to JSON. func DynamicConfiguration( - cluster *v1beta1.PostgresCluster, - configuration map[string]any, + spec *v1beta1.PostgresClusterSpec, pgHBAs postgres.HBAs, pgParameters postgres.Parameters, ) map[string]any { // Copy the entire configuration before making any changes. - root := make(map[string]any, len(configuration)) - for k, v := range configuration { - root[k] = v + root := make(map[string]any) + if spec.Patroni != nil && spec.Patroni.DynamicConfiguration != nil { + root = spec.Patroni.DynamicConfiguration.DeepCopy() } - root["ttl"] = *cluster.Spec.Patroni.LeaderLeaseDurationSeconds - root["loop_wait"] = *cluster.Spec.Patroni.SyncPeriodSeconds + // NOTE: These are always populated due to [v1beta1.PatroniSpec.Default] + root["ttl"] = *spec.Patroni.LeaderLeaseDurationSeconds + root["loop_wait"] = *spec.Patroni.SyncPeriodSeconds - // Copy the "postgresql" section before making any changes. postgresql := map[string]any{ // TODO(cbandy): explain this. requires an archive, perhaps. "use_slots": false, @@ -221,12 +215,13 @@ func DynamicConfiguration( // When TDE is configured, override the pg_rewind binary name to point // to the wrapper script. - if config.FetchKeyCommand(&cluster.Spec) != "" { + if config.FetchKeyCommand(spec) != "" { postgresql["bin_name"] = map[string]any{ "pg_rewind": "/tmp/pg_rewind_tde.sh", } } + // Copy the "postgresql" section over the above defaults. if section, ok := root["postgresql"].(map[string]any); ok { for k, v := range section { postgresql[k] = v @@ -300,15 +295,12 @@ func DynamicConfiguration( // Recent versions of `pg_rewind` can run with limited permissions granted // by Patroni to the user defined in "postgresql.authentication.rewind". // PostgreSQL v10 and earlier require superuser access over the network. - postgresql["use_pg_rewind"] = cluster.Spec.PostgresVersion > 10 - - if cluster.Spec.Standby != nil && cluster.Spec.Standby.Enabled { - // Copy the "standby_cluster" section before making any changes. - standby := make(map[string]any) - if section, ok := root["standby_cluster"].(map[string]any); ok { - for k, v := range section { - standby[k] = v - } + postgresql["use_pg_rewind"] = spec.PostgresVersion > 10 + + if spec.Standby != nil && spec.Standby.Enabled { + standby, _ := root["standby_cluster"].(map[string]any) + if standby == nil { + standby = make(map[string]any) } // Unset any previous value for restore_command - we will set it later if needed @@ -316,16 +308,16 @@ func DynamicConfiguration( // Populate replica creation methods based on options provided in the standby spec: methods := []string{} - if cluster.Spec.Standby.Host != "" { - standby["host"] = cluster.Spec.Standby.Host - if cluster.Spec.Standby.Port != nil { - standby["port"] = *cluster.Spec.Standby.Port + if spec.Standby.Host != "" { + standby["host"] = spec.Standby.Host + if spec.Standby.Port != nil { + standby["port"] = *spec.Standby.Port } methods = append([]string{basebackupCreateReplicaMethod}, methods...) } - if cluster.Spec.Standby.RepoName != "" { + if spec.Standby.RepoName != "" { // Append pgbackrest as the first choice when creating the standby methods = append([]string{pgBackRestCreateReplicaMethod}, methods...) diff --git a/internal/patroni/config_test.go b/internal/patroni/config_test.go index 38ade680b..eb8b12918 100644 --- a/internal/patroni/config_test.go +++ b/internal/patroni/config_test.go @@ -17,7 +17,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/yaml" - "github.com/crunchydata/postgres-operator/internal/initialize" "github.com/crunchydata/postgres-operator/internal/postgres" "github.com/crunchydata/postgres-operator/internal/testing/cmp" "github.com/crunchydata/postgres-operator/internal/testing/require" @@ -229,8 +228,7 @@ func TestDynamicConfiguration(t *testing.T) { for _, tt := range []struct { name string - cluster *v1beta1.PostgresCluster - input map[string]any + spec string hbas postgres.HBAs params postgres.Parameters expected map[string]any @@ -250,13 +248,17 @@ func TestDynamicConfiguration(t *testing.T) { }, { name: "top-level passes through", - input: map[string]any{ - "retry_timeout": 5, - }, + spec: `{ + patroni: { + dynamicConfiguration: { + retry_timeout: 5, + }, + }, + }`, expected: map[string]any{ "loop_wait": int32(10), "ttl": int32(30), - "retry_timeout": 5, + "retry_timeout": float64(5), "postgresql": map[string]any{ "parameters": map[string]any{}, "pg_hba": []string{}, @@ -267,18 +269,16 @@ func TestDynamicConfiguration(t *testing.T) { }, { name: "top-level: spec overrides input", - cluster: &v1beta1.PostgresCluster{ - Spec: v1beta1.PostgresClusterSpec{ - Patroni: &v1beta1.PatroniSpec{ - LeaderLeaseDurationSeconds: initialize.Int32(99), - SyncPeriodSeconds: initialize.Int32(8), + spec: `{ + patroni: { + leaderLeaseDurationSeconds: 99, + syncPeriodSeconds: 8, + dynamicConfiguration: { + loop_wait: 3, + ttl: nope, }, }, - }, - input: map[string]any{ - "loop_wait": 3, - "ttl": "nope", - }, + }`, expected: map[string]any{ "loop_wait": int32(8), "ttl": int32(99), @@ -292,9 +292,13 @@ func TestDynamicConfiguration(t *testing.T) { }, { name: "postgresql: wrong-type is ignored", - input: map[string]any{ - "postgresql": true, - }, + spec: `{ + patroni: { + dynamicConfiguration: { + postgresql: true, + }, + }, + }`, expected: map[string]any{ "loop_wait": int32(10), "ttl": int32(30), @@ -308,12 +312,16 @@ func TestDynamicConfiguration(t *testing.T) { }, { name: "postgresql: defaults and overrides", - input: map[string]any{ - "postgresql": map[string]any{ - "use_pg_rewind": "overridden", - "use_slots": "input", + spec: `{ + patroni: { + dynamicConfiguration: { + postgresql: { + use_pg_rewind: overidden, + use_slots: input, + }, + }, }, - }, + }`, expected: map[string]any{ "loop_wait": int32(10), "ttl": int32(30), @@ -327,11 +335,15 @@ func TestDynamicConfiguration(t *testing.T) { }, { name: "postgresql.parameters: wrong-type is ignored", - input: map[string]any{ - "postgresql": map[string]any{ - "parameters": true, + spec: `{ + patroni: { + dynamicConfiguration: { + postgresql: { + parameters: true, + }, + }, }, - }, + }`, expected: map[string]any{ "loop_wait": int32(10), "ttl": int32(30), @@ -345,21 +357,25 @@ func TestDynamicConfiguration(t *testing.T) { }, { name: "postgresql.parameters: input passes through", - input: map[string]any{ - "postgresql": map[string]any{ - "parameters": map[string]any{ - "something": "str", - "another": 5, + spec: `{ + patroni: { + dynamicConfiguration: { + postgresql: { + parameters: { + something: str, + another: 5, + }, + }, }, }, - }, + }`, expected: map[string]any{ "loop_wait": int32(10), "ttl": int32(30), "postgresql": map[string]any{ "parameters": map[string]any{ "something": "str", - "another": 5, + "another": float64(5), }, "pg_hba": []string{}, "use_pg_rewind": true, @@ -369,14 +385,18 @@ func TestDynamicConfiguration(t *testing.T) { }, { name: "postgresql.parameters: input overrides default", - input: map[string]any{ - "postgresql": map[string]any{ - "parameters": map[string]any{ - "something": "str", - "another": 5, + spec: `{ + patroni: { + dynamicConfiguration: { + postgresql: { + parameters: { + something: str, + another: 5, + }, + }, }, }, - }, + }`, params: postgres.Parameters{ Default: parameters(map[string]string{ "something": "overridden", @@ -389,7 +409,7 @@ func TestDynamicConfiguration(t *testing.T) { "postgresql": map[string]any{ "parameters": map[string]any{ "something": "str", - "another": 5, + "another": float64(5), "unrelated": "default", }, "pg_hba": []string{}, @@ -400,14 +420,18 @@ func TestDynamicConfiguration(t *testing.T) { }, { name: "postgresql.parameters: mandatory overrides input", - input: map[string]any{ - "postgresql": map[string]any{ - "parameters": map[string]any{ - "something": "str", - "another": 5, + spec: `{ + patroni: { + dynamicConfiguration: { + postgresql: { + parameters: { + something: str, + another: 5, + }, + }, }, }, - }, + }`, params: postgres.Parameters{ Mandatory: parameters(map[string]string{ "something": "overrides", @@ -420,7 +444,7 @@ func TestDynamicConfiguration(t *testing.T) { "postgresql": map[string]any{ "parameters": map[string]any{ "something": "overrides", - "another": 5, + "another": float64(5), "unrelated": "setting", }, "pg_hba": []string{}, @@ -431,13 +455,17 @@ func TestDynamicConfiguration(t *testing.T) { }, { name: "postgresql.parameters: mandatory shared_preload_libraries", - input: map[string]any{ - "postgresql": map[string]any{ - "parameters": map[string]any{ - "shared_preload_libraries": "given", + spec: `{ + patroni: { + dynamicConfiguration: { + postgresql: { + parameters: { + shared_preload_libraries: given, + }, + }, }, }, - }, + }`, params: postgres.Parameters{ Mandatory: parameters(map[string]string{ "shared_preload_libraries": "mandatory", @@ -458,13 +486,17 @@ func TestDynamicConfiguration(t *testing.T) { }, { name: "postgresql.parameters: mandatory shared_preload_libraries wrong-type is ignored", - input: map[string]any{ - "postgresql": map[string]any{ - "parameters": map[string]any{ - "shared_preload_libraries": 1, + spec: `{ + patroni: { + dynamicConfiguration: { + postgresql: { + parameters: { + shared_preload_libraries: 1, + }, + }, }, }, - }, + }`, params: postgres.Parameters{ Mandatory: parameters(map[string]string{ "shared_preload_libraries": "mandatory", @@ -485,13 +517,17 @@ func TestDynamicConfiguration(t *testing.T) { }, { name: "postgresql.parameters: shared_preload_libraries order", - input: map[string]any{ - "postgresql": map[string]any{ - "parameters": map[string]any{ - "shared_preload_libraries": "given, citus, more", + spec: `{ + patroni: { + dynamicConfiguration: { + postgresql: { + parameters: { + shared_preload_libraries: "given, citus, more", + }, + }, }, }, - }, + }`, params: postgres.Parameters{ Mandatory: parameters(map[string]string{ "shared_preload_libraries": "mandatory", @@ -512,11 +548,15 @@ func TestDynamicConfiguration(t *testing.T) { }, { name: "postgresql.pg_hba: wrong-type is ignored", - input: map[string]any{ - "postgresql": map[string]any{ - "pg_hba": true, + spec: `{ + patroni: { + dynamicConfiguration: { + postgresql: { + pg_hba: true, + }, + }, }, - }, + }`, expected: map[string]any{ "loop_wait": int32(10), "ttl": int32(30), @@ -530,11 +570,15 @@ func TestDynamicConfiguration(t *testing.T) { }, { name: "postgresql.pg_hba: default when no input", - input: map[string]any{ - "postgresql": map[string]any{ - "pg_hba": nil, + spec: `{ + patroni: { + dynamicConfiguration: { + postgresql: { + pg_hba: null, + }, + }, }, - }, + }`, hbas: postgres.HBAs{ Default: []*postgres.HostBasedAuthentication{ postgres.NewHBA().Local().Method("peer"), @@ -555,11 +599,15 @@ func TestDynamicConfiguration(t *testing.T) { }, { name: "postgresql.pg_hba: no default when input", - input: map[string]any{ - "postgresql": map[string]any{ - "pg_hba": []any{"custom"}, + spec: `{ + patroni: { + dynamicConfiguration: { + postgresql: { + pg_hba: [custom], + }, + }, }, - }, + }`, hbas: postgres.HBAs{ Default: []*postgres.HostBasedAuthentication{ postgres.NewHBA().Local().Method("peer"), @@ -580,11 +628,15 @@ func TestDynamicConfiguration(t *testing.T) { }, { name: "postgresql.pg_hba: mandatory before others", - input: map[string]any{ - "postgresql": map[string]any{ - "pg_hba": []any{"custom"}, + spec: `{ + patroni: { + dynamicConfiguration: { + postgresql: { + pg_hba: [custom], + }, + }, }, - }, + }`, hbas: postgres.HBAs{ Mandatory: []*postgres.HostBasedAuthentication{ postgres.NewHBA().Local().Method("peer"), @@ -606,11 +658,15 @@ func TestDynamicConfiguration(t *testing.T) { }, { name: "postgresql.pg_hba: ignore non-string types", - input: map[string]any{ - "postgresql": map[string]any{ - "pg_hba": []any{1, true, "custom", map[string]string{}, []string{}}, + spec: `{ + patroni: { + dynamicConfiguration: { + postgresql: { + pg_hba: [1, true, custom, {}, []], + }, + }, }, - }, + }`, hbas: postgres.HBAs{ Mandatory: []*postgres.HostBasedAuthentication{ postgres.NewHBA().Local().Method("peer"), @@ -632,11 +688,15 @@ func TestDynamicConfiguration(t *testing.T) { }, { name: "standby_cluster: input passes through", - input: map[string]any{ - "standby_cluster": map[string]any{ - "primary_slot_name": "str", + spec: `{ + patroni: { + dynamicConfiguration: { + standby_cluster: { + primary_slot_name: str, + }, + }, }, - }, + }`, expected: map[string]any{ "loop_wait": int32(10), "ttl": int32(30), @@ -653,20 +713,20 @@ func TestDynamicConfiguration(t *testing.T) { }, { name: "standby_cluster: repo only", - cluster: &v1beta1.PostgresCluster{ - Spec: v1beta1.PostgresClusterSpec{ - Standby: &v1beta1.PostgresStandbySpec{ - Enabled: true, - RepoName: "repo", - }, + spec: `{ + standby: { + enabled: true, + repoName: repo, }, - }, - input: map[string]any{ - "standby_cluster": map[string]any{ - "restore_command": "overridden", - "unrelated": "input", + patroni: { + dynamicConfiguration: { + standby_cluster: { + restore_command: overridden, + unrelated: input, + }, + }, }, - }, + }`, params: postgres.Parameters{ Mandatory: parameters(map[string]string{ "restore_command": "mandatory", @@ -692,23 +752,23 @@ func TestDynamicConfiguration(t *testing.T) { }, { name: "standby_cluster: basebackup for streaming", - cluster: &v1beta1.PostgresCluster{ - Spec: v1beta1.PostgresClusterSpec{ - Standby: &v1beta1.PostgresStandbySpec{ - Enabled: true, - Host: "0.0.0.0", - Port: initialize.Int32(5432), - }, + spec: `{ + standby: { + enabled: true, + host: 0.0.0.0, + port: 5432, }, - }, - input: map[string]any{ - "standby_cluster": map[string]any{ - "host": "overridden", - "port": int32(0000), - "restore_command": "overridden", - "unrelated": "input", + patroni: { + dynamicConfiguration: { + standby_cluster: { + host: overridden, + port: 0000, + restore_command: overridden, + unrelated: input, + }, + }, }, - }, + }`, params: postgres.Parameters{ Mandatory: parameters(map[string]string{ "restore_command": "mandatory", @@ -735,24 +795,24 @@ func TestDynamicConfiguration(t *testing.T) { }, { name: "standby_cluster: both repo and streaming", - cluster: &v1beta1.PostgresCluster{ - Spec: v1beta1.PostgresClusterSpec{ - Standby: &v1beta1.PostgresStandbySpec{ - Enabled: true, - Host: "0.0.0.0", - Port: initialize.Int32(5432), - RepoName: "repo", - }, + spec: `{ + standby: { + enabled: true, + host: 0.0.0.0, + port: 5432, + repoName: repo, }, - }, - input: map[string]any{ - "standby_cluster": map[string]any{ - "host": "overridden", - "port": int32(9999), - "restore_command": "overridden", - "unrelated": "input", + patroni: { + dynamicConfiguration: { + standby_cluster: { + host: overridden, + port: 9999, + restore_command: overridden, + unrelated: input, + }, + }, }, - }, + }`, params: postgres.Parameters{ Mandatory: parameters(map[string]string{ "restore_command": "mandatory", @@ -780,25 +840,25 @@ func TestDynamicConfiguration(t *testing.T) { }, { name: "tde enabled", - cluster: &v1beta1.PostgresCluster{ - Spec: v1beta1.PostgresClusterSpec{ - Patroni: &v1beta1.PatroniSpec{ - DynamicConfiguration: map[string]any{ - "postgresql": map[string]any{ - "parameters": map[string]any{ - "encryption_key_command": "echo test", - }, + spec: `{ + patroni: { + dynamicConfiguration: { + postgresql: { + parameters: { + encryption_key_command: echo test, }, }, }, }, - }, + }`, expected: map[string]any{ "loop_wait": int32(10), "ttl": int32(30), "postgresql": map[string]any{ - "bin_name": map[string]any{"pg_rewind": string("/tmp/pg_rewind_tde.sh")}, - "parameters": map[string]any{}, + "bin_name": map[string]any{"pg_rewind": string("/tmp/pg_rewind_tde.sh")}, + "parameters": map[string]any{ + "encryption_key_command": "echo test", + }, "pg_hba": []string{}, "use_pg_rewind": bool(true), "use_slots": bool(false), @@ -807,15 +867,13 @@ func TestDynamicConfiguration(t *testing.T) { }, } { t.Run(tt.name, func(t *testing.T) { - cluster := tt.cluster - if cluster == nil { - cluster = new(v1beta1.PostgresCluster) - } + cluster := new(v1beta1.PostgresCluster) + assert.NilError(t, yaml.Unmarshal([]byte(tt.spec), &cluster.Spec)) if cluster.Spec.PostgresVersion == 0 { cluster.Spec.PostgresVersion = 14 } cluster.Default() - actual := DynamicConfiguration(cluster, tt.input, tt.hbas, tt.params) + actual := DynamicConfiguration(&cluster.Spec, tt.hbas, tt.params) assert.DeepEqual(t, tt.expected, actual) }) }