fix(pg_user): show diff on password mismatch (#2019)

CHANGELOG.md

@@ -11,6 +11,7 @@ nav_order: 1
 
 ## [MAJOR.MINOR.PATCH] - YYYY-MM-DD
 
+- Fix `pg_user`: throws an error when the password is modified directly in the service 
 - Enable `read_replica` service integration for `aiven_redis` and `aiven_valkey` resources
 - Add `disaster_recovery` service integration type support
 - Add `aiven_flink_jar_application`, `aiven_flink_jar_application_version` and `aiven_flink_jar_application_deployment` BETA resources

internal/schemautil/service_user.go

@@ -44,14 +44,9 @@ func ResourceServiceUserCreate(ctx context.Context, d *schema.ResourceData, m in
 	}
 
 	// Retry because the user may not be immediately available
-	err = RetryNotFound(ctx, func() error {
-		// ResourceServiceUserRead resets id each time it gets 404, setting/restoring it here.
-		d.SetId(BuildResourceID(projectName, serviceName, username))
-		err := ResourceServiceUserRead(ctx, d, m)
-		return ErrorFromDiagnostics(err)
-	})
-
-	return diag.FromErr(err)
+	// todo: Retry NotFound user might be not available immediately
+	d.SetId(BuildResourceID(projectName, serviceName, username))
+	return ResourceServiceUserRead(ctx, d, m)
 }
 
 func ResourceServiceUserUpdate(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Diagnostics {

internal/schemautil/wait.go

@@ -377,15 +377,5 @@ func WaitUntilNotFound(ctx context.Context, retryableFunc retryGo.RetryableFunc)
 	)
 }
 
-// retryNotFoundAttempts just a random number
-const retryNotFoundAttempts = 10
-
-func RetryNotFound(ctx context.Context, retryableFunc retryGo.RetryableFunc) error {
-	return retryGo.Do(
-		retryableFunc,
-		retryGo.Context(ctx),
-		retryGo.Delay(time.Second),
-		retryGo.Attempts(retryNotFoundAttempts),
-		retryGo.RetryIf(IsNotFound),
-	)
-}
+// RetryNotFoundAttempts just a random number
+const RetryNotFoundAttempts = 10

internal/sdkprovider/service/pg/pg_user.go

@@ -2,6 +2,7 @@ package pg
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"time"
 
@@ -108,12 +109,24 @@ func ResourcePGUserCreate(ctx context.Context, d schemautil.ResourceData, client
 		}
 	}
 
-	// Retry because the user may not be immediately available
-	return schemautil.RetryNotFound(ctx, func() error {
-		// ResourcePGUserRead resets id each time it gets 404, setting/restoring it here.
-		d.SetId(schemautil.BuildResourceID(projectName, serviceName, username))
-		return ResourcePGUserRead(ctx, d, client)
-	})
+	// Retries 404 and password not received
+	return retry.Do(
+		func() error {
+			// ResourcePGUserRead resets id each time it gets 404, setting/restoring it here.
+			d.SetId(schemautil.BuildResourceID(projectName, serviceName, username))
+			err := ResourcePGUserRead(ctx, d, client)
+			if err != nil {
+				return err
+			}
+			return validateUserPassword(d)
+		},
+		retry.Context(ctx),
+		retry.Delay(time.Second),
+		retry.Attempts(schemautil.RetryNotFoundAttempts),
+		retry.RetryIf(func(err error) bool {
+			return avngen.IsNotFound(err) || errors.Is(err, errPasswordNotReceived)
+		}),
+	)
 }
 
 func ResourcePGUserUpdate(ctx context.Context, d schemautil.ResourceData, client avngen.Client) error {
@@ -148,35 +161,31 @@ func ResourcePGUserUpdate(ctx context.Context, d schemautil.ResourceData, client
 		}
 	}
 
-	return ResourcePGUserRead(ctx, d, client)
-}
-
-func ResourcePGUserRead(ctx context.Context, d schemautil.ResourceData, client avngen.Client) error {
-	projectName, serviceName, username, err := schemautil.SplitResourceID3(d.Id())
-	if err != nil {
-		return err
-	}
-
-	// User password might be Null https://api.aiven.io/doc/#tag/Service/operation/ServiceUserGet
-	// > Account password. A null value indicates a user overridden password.
-	var user *service.ServiceUserGetOut
-	err = retry.Do(
+	// Retries password not received
+	return retry.Do(
 		func() error {
-			user, err = client.ServiceUserGet(ctx, projectName, serviceName, username)
+			err := ResourcePGUserRead(ctx, d, client)
 			if err != nil {
-				return retry.Unrecoverable(err)
-			}
-			// The field is not nullable, so we compare to an empty string
-			if user.Password == "" {
-				return fmt.Errorf("password is not received from the API")
+				return err
 			}
-			return nil
+			return validateUserPassword(d)
 		},
 		retry.Context(ctx),
 		retry.Delay(time.Second),
-		retry.LastErrorOnly(true), // retry returns a list of errors by default
+		retry.Attempts(schemautil.RetryNotFoundAttempts),
+		retry.RetryIf(func(err error) bool {
+			return errors.Is(err, errPasswordNotReceived)
+		}),
 	)
+}
+
+func ResourcePGUserRead(ctx context.Context, d schemautil.ResourceData, client avngen.Client) error {
+	projectName, serviceName, username, err := schemautil.SplitResourceID3(d.Id())
+	if err != nil {
+		return err
+	}
 
+	user, err := client.ServiceUserGet(ctx, projectName, serviceName, username)
 	if err != nil {
 		return schemautil.ResourceReadHandleNotFound(err, d)
 	}
@@ -195,3 +204,15 @@ func ResourcePGUserRead(ctx context.Context, d schemautil.ResourceData, client a
 
 	return nil
 }
+
+// errPasswordNotReceived password is not received from the API:
+// 1. it was changed by TF but the API did not return it
+// 2. user has changed it in PG directly, so the API does not have it
+var errPasswordNotReceived = fmt.Errorf("password is not received from the API")
+
+func validateUserPassword(d schemautil.ResourceData) error {
+	if d.Get("password").(string) == "" {
+		return errPasswordNotReceived
+	}
+	return nil
+}

internal/sdkprovider/service/pg/pg_user_retries_test.go

@@ -14,7 +14,7 @@ import (
 	"github.com/aiven/terraform-provider-aiven/mocks"
 )
 
-func TestRetriesErrors(t *testing.T) {
+func TestCreateUpdateRetriesErrors(t *testing.T) {
 	ctx := context.Background()
 	client := mocks.NewMockClient(t)
 	projectName := "foo"
@@ -25,8 +25,8 @@ func TestRetriesErrors(t *testing.T) {
 	d.EXPECT().Get("project").Return(projectName)
 	d.EXPECT().Get("service_name").Return(serviceName)
 	d.EXPECT().Get("username").Return(username)
+	d.EXPECT().Get("password").Return(password).Once()
 	d.EXPECT().Get("pg_allow_replication").Return(true)
-	d.EXPECT().Get("password").Return(password)
 	d.EXPECT().SetId("foo/bar/baz")
 	d.EXPECT().Id().Return("foo/bar/baz")
 	d.EXPECT().IsNewResource().Return(true)
@@ -66,13 +66,15 @@ func TestRetriesErrors(t *testing.T) {
 		ServiceUserGet(ctx, projectName, serviceName, username).
 		Return(new(service.ServiceUserGetOut), nil).
 		Repeatability = retryPasswordFails
+	d.EXPECT().Get("password").Return("").Times(retryPasswordFails)
 
 	// Succeeds
 	userOut := &service.ServiceUserGetOut{Password: password}
 	client.EXPECT().
 		ServiceUserGet(ctx, projectName, serviceName, username).
 		Return(userOut, nil).
 		Once()
+	d.EXPECT().Get("password").Return(password).Once()
 
 	// Sets lots of things, so doesn't matter what
 	d.EXPECT().Set(mock.Anything, mock.Anything).Return(nil)
@@ -84,3 +86,31 @@ func TestRetriesErrors(t *testing.T) {
 	client.AssertNumberOfCalls(t, "ServiceUserCredentialsModify", 1)
 	client.AssertNumberOfCalls(t, "ServiceUserGet", retry404Fails+retryPasswordFails+1)
 }
+
+// TestReadDoesNotRetryEmptyPassword
+// When user resets user password in PG, the API returns an empty password,
+// because the password is not stored in the BE.
+// ReadContext must not retry empty password and the plan must show the diff.
+func TestReadDoesNotRetryEmptyPassword(t *testing.T) {
+	ctx := context.Background()
+	client := mocks.NewMockClient(t)
+	projectName := "foo"
+	serviceName := "bar"
+	username := "baz"
+	d := mocks.NewMockResourceData(t)
+	d.EXPECT().Id().Return("foo/bar/baz")
+
+	client.EXPECT().
+		ServiceUserGet(ctx, projectName, serviceName, username).
+		Return(&service.ServiceUserGetOut{Username: username, Type: "normal"}, nil)
+
+	d.EXPECT().Set("username", username).Return(nil)
+	d.EXPECT().Set("type", "normal").Return(nil)
+	d.EXPECT().Set("password", "").Return(nil) // Empty password!
+
+	err := ResourcePGUserRead(ctx, d, client)
+	require.NoError(t, err)
+
+	// No retries
+	client.AssertNumberOfCalls(t, "ServiceUserGet", 1)
+}