feat: add wait for values to be created and updated on 1Password side (#3238)

pkg/provider/onepassword/fake/fake.go

@@ -123,7 +123,17 @@ func (mockClient *OnePasswordMockClient) GetItemsByTitle(itemUUID, vaultUUID str
 // CreateItem will call a validation function if set.
 func (mockClient *OnePasswordMockClient) CreateItem(i *onepassword.Item, s string) (*onepassword.Item, error) {
 	if mockClient.CreateItemValidateFunc != nil {
-		return mockClient.CreateItemValidateFunc(i, s)
+		item, err := mockClient.CreateItemValidateFunc(i, s)
+		if err == nil {
+			mockClient.MockItems[i.Vault.ID] = append(mockClient.MockItems[i.Vault.ID], *item)
+			if mockClient.MockItemFields[i.Vault.ID] == nil {
+				mockClient.MockItemFields[i.Vault.ID] = make(map[string][]*onepassword.ItemField)
+			}
+
+			mockClient.MockItemFields[i.Vault.ID][i.ID] = item.Fields
+		}
+
+		return item, err
 	}
 	return &onepassword.Item{}, nil
 }
@@ -131,7 +141,19 @@ func (mockClient *OnePasswordMockClient) CreateItem(i *onepassword.Item, s strin
 // UpdateItem will call a validation function if set.
 func (mockClient *OnePasswordMockClient) UpdateItem(i *onepassword.Item, s string) (*onepassword.Item, error) {
 	if mockClient.UpdateItemValidateFunc != nil {
-		return mockClient.UpdateItemValidateFunc(i, s)
+		updatedItem, err := mockClient.UpdateItemValidateFunc(i, s)
+
+		if err == nil {
+			for index, item := range mockClient.MockItems[i.Vault.ID] {
+				if item.ID == updatedItem.ID {
+					mockClient.MockItems[i.Vault.ID][index] = *updatedItem
+					mockClient.MockItemFields[i.Vault.ID][updatedItem.ID] = updatedItem.Fields
+					break
+				}
+			}
+		}
+
+		return updatedItem, err
 	}
 	return &onepassword.Item{}, nil
 }

pkg/provider/onepassword/onepassword.go

@@ -20,6 +20,7 @@ import (
 	"fmt"
 	"net/url"
 	"sort"
+	"time"
 
 	"github.com/1Password/connect-sdk-go/connect"
 	"github.com/1Password/connect-sdk-go/onepassword"
@@ -45,8 +46,6 @@ const (
 	errOnePasswordStoreAtLeastOneVault            = "must be at least one vault: spec.provider.onepassword.vaults"
 	errOnePasswordStoreInvalidConnectHost         = "unable to parse URL: spec.provider.onepassword.connectHost: %w"
 	errOnePasswordStoreNonUniqueVaultNumbers      = "vault order numbers must be unique"
-	errFetchK8sSecret                             = "could not fetch ConnectToken Secret: %w"
-	errMissingToken                               = "missing Secret Token"
 	errGetVault                                   = "error finding 1Password Vault: %w"
 
 	errGetItem               = "error finding 1Password Item: %w"
@@ -59,7 +58,6 @@ const (
 	// custom error messages.
 	errKeyNotFoundMsg       = "key not found in 1Password Vaults"
 	errNoVaultsMsg          = "no vaults found"
-	errNoChangesMsg         = "no changes made to 1Password Item"
 	errExpectedOneItemMsg   = "expected one 1Password Item matching"
 	errExpectedOneFieldMsg  = "expected one 1Password ItemField matching"
 	errExpectedOneFieldMsgF = "%w: '%s' in '%s', got %d"
@@ -86,8 +84,10 @@ type ProviderOnePassword struct {
 }
 
 // https://github.com/external-secrets/external-secrets/issues/644
-var _ esv1beta1.SecretsClient = &ProviderOnePassword{}
-var _ esv1beta1.Provider = &ProviderOnePassword{}
+var (
+	_ esv1beta1.SecretsClient = &ProviderOnePassword{}
+	_ esv1beta1.Provider      = &ProviderOnePassword{}
+)
 
 // Capabilities return the provider supported capabilities (ReadOnly, WriteOnly, ReadWrite).
 func (provider *ProviderOnePassword) Capabilities() esv1beta1.SecretStoreCapabilities {
@@ -251,9 +251,9 @@ func (provider *ProviderOnePassword) createItem(val []byte, ref esv1beta1.PushSe
 // the value is different, the value is updated. If the label exists and the
 // value is the same, nothing is done.
 func updateFieldValue(fields []*onepassword.ItemField, label, newVal string) ([]*onepassword.ItemField, error) {
-	// This will always iterate over all items
-	// but its done to ensure that two fields with the same label
-	// exist resulting in undefined behavior
+	// This will always iterate over all items.
+	// This is done to ensure that two fields with the same label
+	// exist resulting in undefined behavior.
 	var (
 		found bool
 		index int
@@ -270,9 +270,9 @@ func updateFieldValue(fields []*onepassword.ItemField, label, newVal string) ([]
 	if !found {
 		return append(fields, generateNewItemField(label, newVal)), nil
 	}
-	if field := fields[index]; newVal != field.Value {
-		field.Value = newVal
-		fields[index] = field
+
+	if fields[index].Value != newVal {
+		fields[index].Value = newVal
 	}
 
 	return fields, nil
@@ -289,7 +289,7 @@ func generateNewItemField(label, newVal string) *onepassword.ItemField {
 	return field
 }
 
-func (provider *ProviderOnePassword) PushSecret(_ context.Context, secret *corev1.Secret, ref esv1beta1.PushSecretData) error {
+func (provider *ProviderOnePassword) PushSecret(ctx context.Context, secret *corev1.Secret, ref esv1beta1.PushSecretData) error {
 	val, ok := secret.Data[ref.GetSecretKey()]
 	if !ok {
 		return ErrKeyNotFound
@@ -301,7 +301,9 @@ func (provider *ProviderOnePassword) PushSecret(_ context.Context, secret *corev
 		if err = provider.createItem(val, ref); err != nil {
 			return fmt.Errorf(errCreateItem, err)
 		}
-		return nil
+
+		err = provider.waitForFunc(ctx, provider.waitForItemToExist(title))
+		return err
 	} else if err != nil {
 		return err
 	}
@@ -319,6 +321,11 @@ func (provider *ProviderOnePassword) PushSecret(_ context.Context, secret *corev
 	if _, err = provider.client.UpdateItem(providerItem, providerItem.Vault.ID); err != nil {
 		return fmt.Errorf(errUpdateItem, err)
 	}
+
+	if err := provider.waitForFunc(ctx, provider.waitForLabelToBeUpdated(title, label, val)); err != nil {
+		return fmt.Errorf("failed waiting for label update: %w", err)
+	}
+
 	return nil
 }
 
@@ -580,6 +587,57 @@ func (provider *ProviderOnePassword) getAllForVault(vaultID string, ref esv1beta
 	return nil
 }
 
+// waitForFunc will wait for OnePassword to _actually_ create/update the secret. OnePassword returns immediately after
+// the initial create/update which makes the next call for the same item create/update a new item with the same name. Hence, we'll
+// wait for the item to exist or be updated on OnePassword's side as well.
+// Ideally we could do bulk operations and handle data with one submit, but that would require re-writing the entire
+// push secret controller. For now, this is sufficient.
+func (provider *ProviderOnePassword) waitForFunc(ctx context.Context, fn func() error) error {
+	// check every .5 seconds
+	tick := time.NewTicker(500 * time.Millisecond)
+	defer tick.Stop()
+	done, cancel := context.WithTimeout(ctx, 5*time.Second)
+	defer cancel()
+
+	var err error
+	for {
+		select {
+		case <-tick.C:
+			if err = fn(); err == nil {
+				return nil
+			}
+		case <-done.Done():
+			return fmt.Errorf("timeout to wait for function to run successfully; last error was: %w", err)
+		}
+	}
+}
+
+func (provider *ProviderOnePassword) waitForItemToExist(title string) func() error {
+	return func() error {
+		_, err := provider.findItem(title)
+
+		return err
+	}
+}
+
+func (provider *ProviderOnePassword) waitForLabelToBeUpdated(title, label string, val []byte) func() error {
+	return func() error {
+		item, err := provider.findItem(title)
+		if err != nil {
+			return err
+		}
+
+		for _, field := range item.Fields {
+			// we found the label with the right value
+			if field.Label == label && field.Value == string(val) {
+				return nil
+			}
+		}
+
+		return fmt.Errorf("label %s no found on value with title %s", title, label)
+	}
+}
+
 func countFieldsWithLabel(fieldLabel string, fields []*onepassword.ItemField) int {
 	count := 0
 	for _, field := range fields {

pkg/provider/onepassword/onepassword_test.go

@@ -2047,7 +2047,7 @@ func TestProviderOnePasswordPushSecret(t *testing.T) {
 				},
 			},
 			updateValidateFunc: func(item *onepassword.Item, s string) (*onepassword.Item, error) {
-				validateItem(t, &onepassword.Item{
+				expectedItem := &onepassword.Item{
 					Vault: onepassword.ItemVault{
 						ID: vaultName,
 					},
@@ -2063,8 +2063,9 @@ func TestProviderOnePasswordPushSecret(t *testing.T) {
 							Type:  onepassword.FieldTypeConcealed,
 						},
 					},
-				}, item)
-				return nil, nil
+				}
+				validateItem(t, expectedItem, item)
+				return expectedItem, nil
 			},
 			existingItems: []onepassword.Item{
 				{