Skip to content

Commit 2c57498

Browse files
FxKuFxKu
authored
skip db user actions when its secret failed to sync on update (#2969)
* skip db user actions when its secret failed to sync on update * need to add new pgUser field to e2e test * lets collect errors of syncSecret so we still get status updateFailed
1 parent 1af4c50 commit 2c57498

File tree

5 files changed

+69
-20
lines changed

5 files changed

+69
-20
lines changed

‎e2e/tests/test_e2e.py‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1003,7 +1003,8 @@ def verify_role():
10031003
"Origin": 2,
10041004
"IsDbOwner": False,
10051005
"Deleted": False,
1006-
"Rotated": False
1006+
"Rotated": False,
1007+
"Degraded": False,
10071008
})
10081009
return True
10091010
except:

‎pkg/cluster/sync.go‎

Lines changed: 44 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1059,40 +1059,52 @@ func (c *Cluster) syncStandbyClusterConfiguration() error {
10591059
func (c *Cluster) syncSecrets() error {
10601060
c.logger.Debug("syncing secrets")
10611061
c.setProcessName("syncing secrets")
1062+
errors := make([]string, 0)
10621063
generatedSecrets := c.generateUserSecrets()
10631064
retentionUsers := make([]string, 0)
10641065
currentTime := time.Now()
10651066

10661067
for secretUsername, generatedSecret := range generatedSecrets {
1067-
secret, err := c.KubeClient.Secrets(generatedSecret.Namespace).Create(context.TODO(), generatedSecret, metav1.CreateOptions{})
1068+
pgUserDegraded := false
1069+
createdSecret, err := c.KubeClient.Secrets(generatedSecret.Namespace).Create(context.TODO(), generatedSecret, metav1.CreateOptions{})
10681070
if err == nil {
1069-
c.Secrets[secret.UID] = secret
1070-
c.logger.Infof("created new secret %s, namespace: %s, uid: %s", util.NameFromMeta(secret.ObjectMeta), generatedSecret.Namespace, secret.UID)
1071+
c.Secrets[createdSecret.UID] = createdSecret
1072+
c.logger.Infof("created new secret %s, namespace: %s, uid: %s", util.NameFromMeta(createdSecret.ObjectMeta), generatedSecret.Namespace, createdSecret.UID)
10711073
continue
10721074
}
10731075
if k8sutil.ResourceAlreadyExists(err) {
1074-
if err = c.updateSecret(secretUsername, generatedSecret, &retentionUsers, currentTime); err != nil {
1075-
c.logger.Warningf("syncing secret %s failed: %v", util.NameFromMeta(secret.ObjectMeta), err)
1076+
updatedSecret, err := c.updateSecret(secretUsername, generatedSecret, &retentionUsers, currentTime)
1077+
if err == nil {
1078+
c.Secrets[updatedSecret.UID] = updatedSecret
1079+
continue
10761080
}
1081+
errors = append(errors, fmt.Sprintf("syncing secret %s failed: %v", util.NameFromMeta(updatedSecret.ObjectMeta), err))
1082+
pgUserDegraded = true
10771083
} else {
1078-
return fmt.Errorf("could not create secret for user %s: in namespace %s: %v", secretUsername, generatedSecret.Namespace, err)
1084+
errors = append(errors, fmt.Sprintf("could not create secret for user %s: in namespace %s: %v", secretUsername, generatedSecret.Namespace, err))
1085+
pgUserDegraded = true
10791086
}
1087+
c.updatePgUser(secretUsername, pgUserDegraded)
10801088
}
10811089

10821090
// remove rotation users that exceed the retention interval
10831091
if len(retentionUsers) > 0 {
10841092
err := c.initDbConn()
10851093
if err != nil {
1086-
return fmt.Errorf("could not init db connection: %v", err)
1094+
errors = append(errors, fmt.Sprintf("could not init db connection: %v", err))
10871095
}
10881096
if err = c.cleanupRotatedUsers(retentionUsers, c.pgDb); err != nil {
1089-
return fmt.Errorf("error removing users exceeding configured retention interval: %v", err)
1097+
errors = append(errors, fmt.Sprintf("error removing users exceeding configured retention interval: %v", err))
10901098
}
10911099
if err := c.closeDbConn(); err != nil {
1092-
c.logger.Errorf("could not close database connection after removing users exceeding configured retention interval: %v", err)
1100+
errors = append(errors, fmt.Sprintf("could not close database connection after removing users exceeding configured retention interval: %v", err))
10931101
}
10941102
}
10951103

1104+
if len(errors) > 0 {
1105+
return fmt.Errorf("%v", strings.Join(errors, `', '`))
1106+
}
1107+
10961108
return nil
10971109
}
10981110

@@ -1105,7 +1117,7 @@ func (c *Cluster) updateSecret(
11051117
secretUsername string,
11061118
generatedSecret *v1.Secret,
11071119
retentionUsers *[]string,
1108-
currentTime time.Time) error {
1120+
currentTime time.Time) (*v1.Secret, error) {
11091121
var (
11101122
secret *v1.Secret
11111123
err error
@@ -1115,7 +1127,7 @@ func (c *Cluster) updateSecret(
11151127

11161128
// get the secret first
11171129
if secret, err = c.KubeClient.Secrets(generatedSecret.Namespace).Get(context.TODO(), generatedSecret.Name, metav1.GetOptions{}); err != nil {
1118-
return fmt.Errorf("could not get current secret: %v", err)
1130+
return generatedSecret, fmt.Errorf("could not get current secret: %v", err)
11191131
}
11201132
c.Secrets[secret.UID] = secret
11211133

@@ -1211,24 +1223,22 @@ func (c *Cluster) updateSecret(
12111223
if updateSecret {
12121224
c.logger.Infof("%s", updateSecretMsg)
12131225
if secret, err = c.KubeClient.Secrets(secret.Namespace).Update(context.TODO(), secret, metav1.UpdateOptions{}); err != nil {
1214-
return fmt.Errorf("could not update secret %s: %v", secretName, err)
1226+
return secret, fmt.Errorf("could not update secret %s: %v", secretName, err)
12151227
}
1216-
c.Secrets[secret.UID] = secret
12171228
}
12181229

12191230
if changed, _ := c.compareAnnotations(secret.Annotations, generatedSecret.Annotations, nil); changed {
12201231
patchData, err := metaAnnotationsPatch(generatedSecret.Annotations)
12211232
if err != nil {
1222-
return fmt.Errorf("could not form patch for secret %q annotations: %v", secret.Name, err)
1233+
return secret, fmt.Errorf("could not form patch for secret %q annotations: %v", secret.Name, err)
12231234
}
12241235
secret, err = c.KubeClient.Secrets(secret.Namespace).Patch(context.TODO(), secret.Name, types.MergePatchType, []byte(patchData), metav1.PatchOptions{})
12251236
if err != nil {
1226-
return fmt.Errorf("could not patch annotations for secret %q: %v", secret.Name, err)
1237+
return secret, fmt.Errorf("could not patch annotations for secret %q: %v", secret.Name, err)
12271238
}
1228-
c.Secrets[secret.UID] = secret
12291239
}
12301240

1231-
return nil
1241+
return secret, nil
12321242
}
12331243

12341244
func (c *Cluster) rotatePasswordInSecret(
@@ -1334,6 +1344,23 @@ func (c *Cluster) rotatePasswordInSecret(
13341344
return updateSecretMsg, nil
13351345
}
13361346

1347+
func (c *Cluster) updatePgUser(secretUsername string, degraded bool) {
1348+
for key, pgUser := range c.pgUsers {
1349+
if pgUser.Name == secretUsername {
1350+
pgUser.Degraded = degraded
1351+
c.pgUsers[key] = pgUser
1352+
return
1353+
}
1354+
}
1355+
for key, pgUser := range c.systemUsers {
1356+
if pgUser.Name == secretUsername {
1357+
pgUser.Degraded = degraded
1358+
c.systemUsers[key] = pgUser
1359+
return
1360+
}
1361+
}
1362+
}
1363+
13371364
func (c *Cluster) syncRoles() (err error) {
13381365
c.setProcessName("syncing roles")
13391366

‎pkg/cluster/sync_test.go‎

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,12 @@ import (
1212

1313
v1 "k8s.io/api/core/v1"
1414
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
15+
"k8s.io/apimachinery/pkg/runtime"
1516
"k8s.io/apimachinery/pkg/types"
17+
k8stesting "k8s.io/client-go/testing"
1618

1719
"github.com/golang/mock/gomock"
20+
"github.com/pkg/errors"
1821
"github.com/sirupsen/logrus"
1922
"github.com/stretchr/testify/assert"
2023
"github.com/zalando/postgres-operator/mocks"
@@ -50,6 +53,16 @@ func newFakeK8sSyncClient() (k8sutil.KubernetesClient, *fake.Clientset) {
5053
}
5154

5255
func newFakeK8sSyncSecretsClient() (k8sutil.KubernetesClient, *fake.Clientset) {
56+
// add a reactor that checks namespace existence before creating secrets
57+
clientSet.PrependReactor("create", "secrets", func(action k8stesting.Action) (bool, runtime.Object, error) {
58+
createAction := action.(k8stesting.CreateAction)
59+
secret := createAction.GetObject().(*v1.Secret)
60+
if secret.Namespace != "default" {
61+
return true, nil, errors.New("namespace does not exist")
62+
}
63+
return false, nil, nil
64+
})
65+
5366
return k8sutil.KubernetesClient{
5467
SecretsGetter: clientSet.CoreV1(),
5568
}, clientSet
@@ -810,7 +823,7 @@ func TestUpdateSecret(t *testing.T) {
810823
},
811824
Spec: acidv1.PostgresSpec{
812825
Databases: map[string]string{dbname: dbowner},
813-
Users: map[string]acidv1.UserFlags{appUser: {}, "bar": {}, dbowner: {}},
826+
Users: map[string]acidv1.UserFlags{appUser: {}, "bar": {}, dbowner: {}, "not-exist.test_user": {}},
814827
UsersIgnoringSecretRotation: []string{"bar"},
815828
UsersWithInPlaceSecretRotation: []string{dbowner},
816829
Streams: []acidv1.Stream{
@@ -842,6 +855,7 @@ func TestUpdateSecret(t *testing.T) {
842855
PasswordRotationInterval: 1,
843856
PasswordRotationUserRetention: 3,
844857
},
858+
EnableCrossNamespaceSecret: true,
845859
Resources: config.Resources{
846860
ClusterLabels: map[string]string{"application": "spilo"},
847861
ClusterNameLabel: "cluster-name",
@@ -864,7 +878,9 @@ func TestUpdateSecret(t *testing.T) {
864878

865879
allUsers := make(map[string]spec.PgUser)
866880
for _, pgUser := range cluster.pgUsers {
867-
allUsers[pgUser.Name] = pgUser
881+
if !pgUser.Degraded {
882+
allUsers[pgUser.Name] = pgUser
883+
}
868884
}
869885
for _, systemUser := range cluster.systemUsers {
870886
allUsers[systemUser.Name] = systemUser

‎pkg/spec/types.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@ type PgUser struct {
5858
IsDbOwner bool `yaml:"is_db_owner"`
5959
Deleted bool `yaml:"deleted"`
6060
Rotated bool `yaml:"rotated"`
61+
Degraded bool `yaml:"degraded"`
6162
}
6263

6364
func (user *PgUser) Valid() bool {

‎pkg/util/users/users.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,10 @@ func (strategy DefaultUserSyncStrategy) ProduceSyncRequests(dbUsers spec.PgUserM
4848
if newUser.Deleted {
4949
continue
5050
}
51+
// when the secret of the user could not be created or updated skip any database actions
52+
if newUser.Degraded {
53+
continue
54+
}
5155
dbUser, exists := dbUsers[name]
5256
if !exists {
5357
reqs = append(reqs, spec.PgSyncUserRequest{Kind: spec.PGSyncUserAdd, User: newUser})

0 commit comments

Comments
 (0)