Merge pull request #118 from 1Password/fix/internal-server-conflict

Retry CLI request in case of 409 Conflict error

docs/index.md

@@ -35,19 +35,12 @@ provider "onepassword" {
 - `token` (String) A valid token for your 1Password Connect API. Can also be sourced from OP_CONNECT_TOKEN. Either this or `service_account_token` must be set.
 - `url` (String) The HTTP(S) URL where your 1Password Connect API can be found. Must be provided through the OP_CONNECT_HOST environment variable if this attribute is not set. Can be omitted, if service_account_token is set.
 
-## Known Service Accounts limitation:
-Users might encounter the following error if they create, update, or delete too many items simultaneously in the same 1Password vault.
+## Use with service accounts:
+Retry mechanism is implemented when using provider with service accounts. Each retry fast forwards to the [service account rate limit](https://developer.1password.com/docs/service-accounts/rate-limits/).
 
-\```
-op error: (409) Conflict: Internal server conflict
-\```
-
-The 1Password Terraform provider handles each resource separately. As a result, each request to perform a create, update, or delete operation using CLI to create an additional parallel request. Too many parallel requests might result in one or more race conditions.
-
-You can avoid receiving the 409 error in one of the following ways:
-1. Use `depends_on` in your resource definition to make sure the provider makes requests sequentially.
-2. After receiving the `409` error, run `terraform apply` again. You might need to run this multiple times until it applies all the changes.
-3. Use a Connect server.
-4. Put items in the different vaults.
+It's recommended to limit the number of parallel resource operations. It can be done by using `-parallelism=n` flag when running `terraform apply`, where `n` is the number of parallel resource operations (the default is `10`).
+```
+terraform apply `-parallelism=n`
+```
 
-This will be addressed in the future release.
+The reason of having retry mechanism is that 1Password doesn't allow parallel modification on the items located in the same vault.

onepassword/cli/op.go

@@ -78,6 +78,12 @@ func (op *OP) GetItemByTitle(ctx context.Context, title string, vaultUuid string
 }
 
 func (op *OP) CreateItem(ctx context.Context, item *onepassword.Item, vaultUuid string) (*onepassword.Item, error) {
+	return op.withRetry(func() (*onepassword.Item, error) {
+		return op.create(ctx, item, vaultUuid)
+	})
+}
+
+func (op *OP) create(ctx context.Context, item *onepassword.Item, vaultUuid string) (*onepassword.Item, error) {
 	if item.Vault.ID != "" && item.Vault.ID != vaultUuid {
 		return nil, errors.New("item payload contains vault id that does not match vault uuid")
 	}
@@ -108,6 +114,12 @@ func (op *OP) CreateItem(ctx context.Context, item *onepassword.Item, vaultUuid
 }
 
 func (op *OP) UpdateItem(ctx context.Context, item *onepassword.Item, vaultUuid string) (*onepassword.Item, error) {
+	return op.withRetry(func() (*onepassword.Item, error) {
+		return op.update(ctx, item, vaultUuid)
+	})
+}
+
+func (op *OP) update(ctx context.Context, item *onepassword.Item, vaultUuid string) (*onepassword.Item, error) {
 	if item.Vault.ID != "" && item.Vault.ID != vaultUuid {
 		return nil, errors.New("item payload contains vault id that does not match vault uuid")
 	}
@@ -127,12 +139,26 @@ func (op *OP) UpdateItem(ctx context.Context, item *onepassword.Item, vaultUuid
 }
 
 func (op *OP) DeleteItem(ctx context.Context, item *onepassword.Item, vaultUuid string) error {
+	_, err := op.withRetry(func() (*onepassword.Item, error) {
+		return op.delete(ctx, item, vaultUuid)
+	})
+	if err != nil {
+		return err
+	}
+	return nil
+}
+
+func (op *OP) delete(ctx context.Context, item *onepassword.Item, vaultUuid string) (*onepassword.Item, error) {
 	if item.Vault.ID != "" && item.Vault.ID != vaultUuid {
-		return errors.New("item payload contains vault id that does not match vault uuid")
+		return nil, errors.New("item payload contains vault id that does not match vault uuid")
 	}
 	item.Vault.ID = vaultUuid
 
-	return op.execJson(ctx, nil, nil, p("item"), p("delete"), p(item.ID), f("vault", vaultUuid))
+	err := op.execJson(ctx, nil, nil, p("item"), p("delete"), p(item.ID), f("vault", vaultUuid))
+	if err != nil {
+		return nil, err
+	}
+	return item, err
 }
 
 func (op *OP) execJson(ctx context.Context, dst any, stdin []byte, args ...opArg) error {
@@ -177,3 +203,30 @@ func (op *OP) execRaw(ctx context.Context, stdin []byte, args ...opArg) ([]byte,
 
 	return result, nil
 }
+
+func (op *OP) withRetry(action func() (*onepassword.Item, error)) (*onepassword.Item, error) {
+	attempt := 0
+	item, err := action()
+	if err != nil {
+		// retry if there is 409 Conflict error
+		if strings.Contains(err.Error(), "409") {
+			// make 3 retry attempts to successfully finish the operation
+			for attempt < 3 {
+				waitBeforeRetry(attempt)
+				item, err = action()
+				if err != nil {
+					attempt++
+					continue
+				}
+				break
+			}
+		}
+		// return error if operation did not succeed after retries
+		// or error is other than 409
+		if err != nil {
+			return nil, err
+		}
+	}
+
+	return item, nil
+}

onepassword/cli/op_test.go

@@ -0,0 +1,62 @@
+package cli
+
+import (
+	"errors"
+	"testing"
+
+	"github.com/1Password/connect-sdk-go/onepassword"
+)
+
+func TestWithRetry(t *testing.T) {
+	op := &OP{}
+	tests := map[string]struct {
+		fakeAction func() (*onepassword.Item, error)
+		validate   func(item *onepassword.Item, err error)
+	}{
+		"should fail when error other than 409": {
+			fakeAction: func() (*onepassword.Item, error) {
+				return nil, errors.New("failed to perform action")
+			},
+			validate: func(item *onepassword.Item, err error) {
+				if err == nil {
+					t.Error("Action should fail when error is other than 409")
+				}
+				if item != nil {
+					t.Error("Item should be nil when error is other than 409")
+				}
+			},
+		},
+		"should fail when error is 409": {
+			fakeAction: func() (*onepassword.Item, error) {
+				return nil, errors.New("409 Conflict error")
+			},
+			validate: func(item *onepassword.Item, err error) {
+				if err == nil {
+					t.Error("Action should fail when error is 409")
+				}
+				if item != nil {
+					t.Error("Item should be nil when error is 409")
+				}
+			},
+		},
+		"should succeed": {
+			fakeAction: func() (*onepassword.Item, error) {
+				return &onepassword.Item{}, nil
+			},
+			validate: func(item *onepassword.Item, err error) {
+				if err != nil {
+					t.Errorf("Action should succeed, but got an error: %s", err.Error())
+				}
+				if item == nil {
+					t.Error("Item should not be nil")
+				}
+			},
+		},
+	}
+
+	for description, test := range tests {
+		t.Run(description, func(t *testing.T) {
+			test.validate(op.withRetry(test.fakeAction))
+		})
+	}
+}

onepassword/cli/utils.go

@@ -1,9 +1,13 @@
 package cli
 
 import (
+	"crypto/rand"
 	"fmt"
+	"math"
+	"math/big"
 	"regexp"
 	"strings"
+	"time"
 
 	"github.com/1Password/connect-sdk-go/onepassword"
 )
@@ -70,3 +74,19 @@ func passwordRecipeToString(recipe *onepassword.GeneratorRecipe) string {
 	}
 	return str
 }
+
+// waitBeforeRetry waits some amount of time based on retryAttempt
+// it implements 'exponential backoff with jitter' algorithm
+func waitBeforeRetry(retryAttempts int) {
+	randInt, err := rand.Int(rand.Reader, big.NewInt(100))
+	if err != nil {
+		randInt = big.NewInt(0)
+	}
+	randPercentage := float64(randInt.Int64()) / 100
+	jitter := (1.0 + randPercentage) / 2
+
+	exp := math.Pow(2, float64(retryAttempts))
+	retryTimeMilliseconds := 100 + 500*exp*jitter
+	wait := time.Duration(retryTimeMilliseconds) * time.Millisecond
+	time.Sleep(wait)
+}

templates/index.md.tmpl

@@ -20,19 +20,12 @@ To use a service account token, you must install [1Password CLI](https://develop
 
 {{ .SchemaMarkdown | trimspace }}
 
-## Known Service Accounts limitation:
-Users might encounter the following error if they create, update, or delete too many items simultaneously in the same 1Password vault.
+## Use with service accounts:
+Retry mechanism is implemented when using provider with service accounts. Each retry fast forwards to the [service account rate limit](https://developer.1password.com/docs/service-accounts/rate-limits/).
 
-\```
-op error: (409) Conflict: Internal server conflict
-\```
+It's recommended to limit the number of parallel resource operations. It can be done by using `-parallelism=n` flag when running `terraform apply`, where `n` is the number of parallel resource operations (the default is `10`).
+```
+terraform apply `-parallelism=n`
+```
 
-The 1Password Terraform provider handles each resource separately. As a result, each request to perform a create, update, or delete operation using CLI to create an additional parallel request. Too many parallel requests might result in one or more race conditions.
-
-You can avoid receiving the 409 error in one of the following ways:
-1. Use `depends_on` in your resource definition to make sure the provider makes requests sequentially.
-2. After receiving the `409` error, run `terraform apply` again. You might need to run this multiple times until it applies all the changes.
-3. Use a Connect server.
-4. Put items in the different vaults.
-
-This will be addressed in the future release.
+The reason of having retry mechanism is that 1Password doesn't allow parallel modification on the items located in the same vault.