From 3029421dc9e9e5e40c19684f869c95889677f23c Mon Sep 17 00:00:00 2001 From: Felix Kunde Date: Tue, 23 Nov 2021 15:27:22 +0100 Subject: [PATCH 1/5] init error arrays correctly --- pkg/cluster/resources.go | 7 +++---- pkg/cluster/sync_test.go | 9 ++------- pkg/cluster/volumes.go | 2 +- pkg/controller/util.go | 9 +++------ pkg/util/users/users.go | 2 +- 5 files changed, 10 insertions(+), 19 deletions(-) diff --git a/pkg/cluster/resources.go b/pkg/cluster/resources.go index f078c6434..919e75153 100644 --- a/pkg/cluster/resources.go +++ b/pkg/cluster/resources.go @@ -496,17 +496,16 @@ func (c *Cluster) deleteEndpoint(role PostgresRole) error { func (c *Cluster) deleteSecrets() error { c.setProcessName("deleting secrets") - var errors []string - errorCount := 0 + errors := make([]string, 0) + for uid, secret := range c.Secrets { err := c.deleteSecret(uid, *secret) if err != nil { errors = append(errors, fmt.Sprintf("%v", err)) - errorCount++ } } - if errorCount > 0 { + if len(errors) > 0 { return fmt.Errorf("could not delete all secrets: %v", errors) } diff --git a/pkg/cluster/sync_test.go b/pkg/cluster/sync_test.go index c6e2a8357..3e3fc9318 100644 --- a/pkg/cluster/sync_test.go +++ b/pkg/cluster/sync_test.go @@ -196,17 +196,15 @@ func TestCheckAndSetGlobalPostgreSQLConfiguration(t *testing.T) { cluster.patroni = p mockPod := newMockPod("192.168.100.1") - // simulate existing config that differs with cluster.Spec + // simulate existing config that differs from cluster.Spec tests := []struct { subtest string - pod *v1.Pod patroni acidv1.Patroni pgParams map[string]string restartMaster bool }{ { subtest: "Patroni and Postgresql.Parameters differ - restart replica first", - pod: mockPod, patroni: acidv1.Patroni{ TTL: 30, // desired 20 }, @@ -218,7 +216,6 @@ func TestCheckAndSetGlobalPostgreSQLConfiguration(t *testing.T) { }, { subtest: "multiple Postgresql.Parameters differ - restart replica first", - pod: mockPod, patroni: acidv1.Patroni{ TTL: 20, }, @@ -230,7 +227,6 @@ func TestCheckAndSetGlobalPostgreSQLConfiguration(t *testing.T) { }, { subtest: "desired max_connections bigger - restart replica first", - pod: mockPod, patroni: acidv1.Patroni{ TTL: 20, }, @@ -242,7 +238,6 @@ func TestCheckAndSetGlobalPostgreSQLConfiguration(t *testing.T) { }, { subtest: "desired max_connections smaller - restart master first", - pod: mockPod, patroni: acidv1.Patroni{ TTL: 20, }, @@ -255,7 +250,7 @@ func TestCheckAndSetGlobalPostgreSQLConfiguration(t *testing.T) { } for _, tt := range tests { - requireMasterRestart, err := cluster.checkAndSetGlobalPostgreSQLConfiguration(tt.pod, tt.patroni, tt.pgParams) + requireMasterRestart, err := cluster.checkAndSetGlobalPostgreSQLConfiguration(mockPod, tt.patroni, tt.pgParams) assert.NoError(t, err) if requireMasterRestart != tt.restartMaster { t.Errorf("%s - %s: unexpect master restart strategy, got %v, expected %v", testName, tt.subtest, requireMasterRestart, tt.restartMaster) diff --git a/pkg/cluster/volumes.go b/pkg/cluster/volumes.go index 4962319ed..78533ef54 100644 --- a/pkg/cluster/volumes.go +++ b/pkg/cluster/volumes.go @@ -88,7 +88,7 @@ func (c *Cluster) syncUnderlyingEBSVolume() error { awsGp3 := aws.String("gp3") awsIo2 := aws.String("io2") - errors := []string{} + errors := make([]string, 0) for _, volume := range c.EBSVolumes { var modifyIops *int64 diff --git a/pkg/controller/util.go b/pkg/controller/util.go index 8aa891c09..3639b4123 100644 --- a/pkg/controller/util.go +++ b/pkg/controller/util.go @@ -197,9 +197,8 @@ func (c *Controller) getInfrastructureRoles( rolesSecrets []*config.InfrastructureRole) ( map[string]spec.PgUser, []error) { - var errors []error - var noRolesProvided = true - + errors := make([]error, 0) + noRolesProvided := true roles := []spec.PgUser{} uniqRoles := map[string]spec.PgUser{} @@ -230,9 +229,7 @@ func (c *Controller) getInfrastructureRoles( continue } - for _, r := range infraRoles { - roles = append(roles, r) - } + roles = append(roles, infraRoles...) } for _, r := range roles { diff --git a/pkg/util/users/users.go b/pkg/util/users/users.go index 3da933644..c6b6461fe 100644 --- a/pkg/util/users/users.go +++ b/pkg/util/users/users.go @@ -101,7 +101,7 @@ func (strategy DefaultUserSyncStrategy) ProduceSyncRequests(dbUsers spec.PgUserM // ExecuteSyncRequests makes actual database changes from the requests passed in its arguments. func (strategy DefaultUserSyncStrategy) ExecuteSyncRequests(requests []spec.PgSyncUserRequest, db *sql.DB) error { var reqretries []spec.PgSyncUserRequest - var errors []string + errors := make([]string, 0) for _, request := range requests { switch request.Kind { case spec.PGSyncUserAdd: From 203fd36d976e5775a80d05668234778a2a21a84a Mon Sep 17 00:00:00 2001 From: Felix Kunde Date: Tue, 23 Nov 2021 15:41:33 +0100 Subject: [PATCH 2/5] avoid nilPointer when syncing connectionPooler --- pkg/cluster/connection_pooler.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/cluster/connection_pooler.go b/pkg/cluster/connection_pooler.go index 5bde71458..c5c55350f 100644 --- a/pkg/cluster/connection_pooler.go +++ b/pkg/cluster/connection_pooler.go @@ -712,7 +712,8 @@ func (c *Cluster) syncConnectionPooler(oldSpec, newSpec *acidv1.Postgresql, Look if (!needSync && len(masterChanges) <= 0 && len(replicaChanges) <= 0) && ((!needConnectionPooler(&newSpec.Spec) && (c.ConnectionPooler == nil || !needConnectionPooler(&oldSpec.Spec))) || (c.ConnectionPooler != nil && needConnectionPooler(&newSpec.Spec) && - (c.ConnectionPooler[Master].LookupFunction || c.ConnectionPooler[Replica].LookupFunction))) { + ((c.ConnectionPooler[Master] != nil && c.ConnectionPooler[Master].LookupFunction) || + (c.ConnectionPooler[Replica] != nil && c.ConnectionPooler[Replica].LookupFunction)))) { c.logger.Debugln("syncing pooler is not required") return nil, nil } From 7edc14dfb34649b6e67485d6009e6de668d741b1 Mon Sep 17 00:00:00 2001 From: Felix Kunde Date: Wed, 24 Nov 2021 11:58:06 +0100 Subject: [PATCH 3/5] Update pkg/controller/util.go --- pkg/controller/util.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/controller/util.go b/pkg/controller/util.go index 3639b4123..41a324b19 100644 --- a/pkg/controller/util.go +++ b/pkg/controller/util.go @@ -229,7 +229,9 @@ func (c *Controller) getInfrastructureRoles( continue } - roles = append(roles, infraRoles...) + for _, r := range infraRoles { + roles = append(roles, r) + } } for _, r := range roles { From 4bbf1e9d24ae76156f063fa1beaee5c5b9fca889 Mon Sep 17 00:00:00 2001 From: Felix Kunde Date: Wed, 24 Nov 2021 12:16:21 +0100 Subject: [PATCH 4/5] getInfrastructureRoles should return error --- pkg/cluster/resources.go | 2 +- pkg/controller/util.go | 20 +++++++++++--------- pkg/util/users/users.go | 2 +- 3 files changed, 13 insertions(+), 11 deletions(-) diff --git a/pkg/cluster/resources.go b/pkg/cluster/resources.go index 919e75153..48a82ee04 100644 --- a/pkg/cluster/resources.go +++ b/pkg/cluster/resources.go @@ -506,7 +506,7 @@ func (c *Cluster) deleteSecrets() error { } if len(errors) > 0 { - return fmt.Errorf("could not delete all secrets: %v", errors) + return fmt.Errorf("could not delete all secrets: %v", strings.Join(errors, `', '`)) } return nil diff --git a/pkg/controller/util.go b/pkg/controller/util.go index 41a324b19..67b684856 100644 --- a/pkg/controller/util.go +++ b/pkg/controller/util.go @@ -195,9 +195,9 @@ func (c *Controller) getInfrastructureRoleDefinitions() []*config.Infrastructure func (c *Controller) getInfrastructureRoles( rolesSecrets []*config.InfrastructureRole) ( - map[string]spec.PgUser, []error) { + map[string]spec.PgUser, error) { - errors := make([]error, 0) + errors := make([]string, 0) noRolesProvided := true roles := []spec.PgUser{} uniqRoles := map[string]spec.PgUser{} @@ -220,30 +220,32 @@ func (c *Controller) getInfrastructureRoles( infraRoles, err := c.getInfrastructureRole(secret) if err != nil || infraRoles == nil { - c.logger.Debugf("Cannot get infrastructure role: %+v", *secret) + c.logger.Debugf("cannot get infrastructure role: %+v", *secret) if err != nil { - errors = append(errors, err) + errors = append(errors, fmt.Sprintf("%v", err)) } continue } - for _, r := range infraRoles { - roles = append(roles, r) - } + roles = append(roles, infraRoles...) } for _, r := range roles { if _, exists := uniqRoles[r.Name]; exists { - msg := "Conflicting infrastructure roles: roles[%s] = (%q, %q)" + msg := "conflicting infrastructure roles: roles[%s] = (%q, %q)" c.logger.Debugf(msg, r.Name, uniqRoles[r.Name], r) } uniqRoles[r.Name] = r } - return uniqRoles, errors + if len(errors) > 0 { + return nil, fmt.Errorf(strings.Join(errors, `', '`)) + } + + return uniqRoles, nil } // Generate list of users representing one infrastructure role based on its diff --git a/pkg/util/users/users.go b/pkg/util/users/users.go index c6b6461fe..821350bb7 100644 --- a/pkg/util/users/users.go +++ b/pkg/util/users/users.go @@ -138,7 +138,7 @@ func (strategy DefaultUserSyncStrategy) ExecuteSyncRequests(requests []spec.PgSy return err } } else { - return fmt.Errorf("could not execute sync requests for users: %v", errors) + return fmt.Errorf("could not execute sync requests for users: %v", strings.Join(errors, `', '`)) } } From d580684acb7d3c2825e5ebba8d3e45e4356be292 Mon Sep 17 00:00:00 2001 From: Felix Kunde Date: Wed, 24 Nov 2021 13:41:38 +0100 Subject: [PATCH 5/5] fix unit tests and return type for getInfrastructureRoles --- pkg/controller/util.go | 6 ++-- pkg/controller/util_test.go | 60 ++++++++----------------------------- 2 files changed, 16 insertions(+), 50 deletions(-) diff --git a/pkg/controller/util.go b/pkg/controller/util.go index 67b684856..563734ac9 100644 --- a/pkg/controller/util.go +++ b/pkg/controller/util.go @@ -200,7 +200,7 @@ func (c *Controller) getInfrastructureRoles( errors := make([]string, 0) noRolesProvided := true roles := []spec.PgUser{} - uniqRoles := map[string]spec.PgUser{} + uniqRoles := make(map[string]spec.PgUser) // To be compatible with the legacy implementation we need to return nil if // the provided secret name is empty. The equivalent situation in the @@ -213,7 +213,7 @@ func (c *Controller) getInfrastructureRoles( } if noRolesProvided { - return nil, nil + return uniqRoles, nil } for _, secret := range rolesSecrets { @@ -242,7 +242,7 @@ func (c *Controller) getInfrastructureRoles( } if len(errors) > 0 { - return nil, fmt.Errorf(strings.Join(errors, `', '`)) + return uniqRoles, fmt.Errorf(strings.Join(errors, `', '`)) } return uniqRoles, nil diff --git a/pkg/controller/util_test.go b/pkg/controller/util_test.go index d8e4c3782..a4ca17728 100644 --- a/pkg/controller/util_test.go +++ b/pkg/controller/util_test.go @@ -7,6 +7,7 @@ import ( b64 "encoding/base64" + "github.com/stretchr/testify/assert" "github.com/zalando/postgres-operator/pkg/spec" "github.com/zalando/postgres-operator/pkg/util/config" "github.com/zalando/postgres-operator/pkg/util/k8sutil" @@ -90,21 +91,21 @@ func TestClusterWorkerID(t *testing.T) { // not exist, or empty) and the old format. func TestOldInfrastructureRoleFormat(t *testing.T) { var testTable = []struct { - secretName spec.NamespacedName - expectedRoles map[string]spec.PgUser - expectedErrors []error + secretName spec.NamespacedName + expectedRoles map[string]spec.PgUser + expectedError error }{ { // empty secret name spec.NamespacedName{}, - nil, + map[string]spec.PgUser{}, nil, }, { // secret does not exist spec.NamespacedName{Namespace: v1.NamespaceDefault, Name: "null"}, map[string]spec.PgUser{}, - []error{fmt.Errorf(`could not get infrastructure roles secret default/null: NotFound`)}, + fmt.Errorf(`could not get infrastructure roles secret default/null: NotFound`), }, { spec.NamespacedName{ @@ -129,7 +130,7 @@ func TestOldInfrastructureRoleFormat(t *testing.T) { }, } for _, test := range testTable { - roles, errors := utilTestController.getInfrastructureRoles( + roles, err := utilTestController.getInfrastructureRoles( []*config.InfrastructureRole{ &config.InfrastructureRole{ SecretName: test.secretName, @@ -140,22 +141,9 @@ func TestOldInfrastructureRoleFormat(t *testing.T) { }, }) - if len(errors) != len(test.expectedErrors) { + if err != nil && err.Error() != test.expectedError.Error() { t.Errorf("expected error '%v' does not match the actual error '%v'", - test.expectedErrors, errors) - } - - for idx := range errors { - err := errors[idx] - expectedErr := test.expectedErrors[idx] - - if err != expectedErr { - if err != nil && expectedErr != nil && err.Error() == expectedErr.Error() { - continue - } - t.Errorf("expected error '%v' does not match the actual error '%v'", - expectedErr, err) - } + test.expectedError, err) } if !reflect.DeepEqual(roles, test.expectedRoles) { @@ -169,9 +157,8 @@ func TestOldInfrastructureRoleFormat(t *testing.T) { // corresponding secrets. Here we test the new format. func TestNewInfrastructureRoleFormat(t *testing.T) { var testTable = []struct { - secrets []spec.NamespacedName - expectedRoles map[string]spec.PgUser - expectedErrors []error + secrets []spec.NamespacedName + expectedRoles map[string]spec.PgUser }{ // one secret with one configmap { @@ -196,7 +183,6 @@ func TestNewInfrastructureRoleFormat(t *testing.T) { Flags: []string{"createdb"}, }, }, - nil, }, // multiple standalone secrets { @@ -224,7 +210,6 @@ func TestNewInfrastructureRoleFormat(t *testing.T) { MemberOf: []string{"new-test-inrole2"}, }, }, - nil, }, } for _, test := range testTable { @@ -239,27 +224,8 @@ func TestNewInfrastructureRoleFormat(t *testing.T) { }) } - roles, errors := utilTestController.getInfrastructureRoles(definitions) - if len(errors) != len(test.expectedErrors) { - t.Errorf("expected error does not match the actual error:\n%+v\n%+v", - test.expectedErrors, errors) - - // Stop and do not do any further checks - return - } - - for idx := range errors { - err := errors[idx] - expectedErr := test.expectedErrors[idx] - - if err != expectedErr { - if err != nil && expectedErr != nil && err.Error() == expectedErr.Error() { - continue - } - t.Errorf("expected error '%v' does not match the actual error '%v'", - expectedErr, err) - } - } + roles, err := utilTestController.getInfrastructureRoles(definitions) + assert.NoError(t, err) if !reflect.DeepEqual(roles, test.expectedRoles) { t.Errorf("expected roles output/the actual:\n%#v\n%#v",