Initialize arrays of errors / error messages + minor refactoring (zalando#1701)

* init error arrays correctly
* avoid nilPointer when syncing connectionPooler
* getInfrastructureRoles should return error
* fix unit tests and return type for getInfrastructureRoles

Author: FxKu

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
 	}

pkg/cluster/resources.go

@@ -496,18 +496,17 @@ 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 {
-		return fmt.Errorf("could not delete all secrets: %v", errors)
+	if len(errors) > 0 {
+		return fmt.Errorf("could not delete all secrets: %v", strings.Join(errors, `', '`))
 	}
 
 	return nil

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)

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

pkg/controller/util.go

@@ -195,13 +195,12 @@ func (c *Controller) getInfrastructureRoleDefinitions() []*config.Infrastructure
 
 func (c *Controller) getInfrastructureRoles(
 	rolesSecrets []*config.InfrastructureRole) (
-	map[string]spec.PgUser, []error) {
-
-	var errors []error
-	var noRolesProvided = true
+	map[string]spec.PgUser, error) {
 
+	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
@@ -214,37 +213,39 @@ func (c *Controller) getInfrastructureRoles(
 	}
 
 	if noRolesProvided {
-		return nil, nil
+		return uniqRoles, nil
 	}
 
 	for _, secret := range rolesSecrets {
 		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 uniqRoles, fmt.Errorf(strings.Join(errors, `', '`))
+	}
+
+	return uniqRoles, nil
 }
 
 // Generate list of users representing one infrastructure role based on its

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",

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:
@@ -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, `', '`))
 		}
 	}