Merge pull request #212 from 1Password/vzt/fix-panic-when-reading-file

Fix panic when reading file

internal/controller/deployment_controller.go

@@ -203,13 +203,13 @@ func (r *DeploymentReconciler) handleApplyingDeployment(ctx context.Context, dep
 
 	item, err := op.GetOnePasswordItemByPath(ctx, r.OpClient, annotations[op.ItemPathAnnotation])
 	if err != nil {
-		return fmt.Errorf("Failed to retrieve item: %v", err)
+		return fmt.Errorf("failed to retrieve item: %w", err)
 	}
 
 	// Create owner reference.
 	gvk, err := apiutil.GVKForObject(deployment, r.Scheme)
 	if err != nil {
-		return fmt.Errorf("could not to retrieve group version kind: %v", err)
+		return fmt.Errorf("could not to retrieve group version kind: %w", err)
 	}
 	ownerRef := &metav1.OwnerReference{
 		APIVersion: gvk.GroupVersion().String(),

internal/controller/onepassworditem_controller.go

@@ -173,13 +173,13 @@ func (r *OnePasswordItemReconciler) handleOnePasswordItem(ctx context.Context, r
 
 	item, err := op.GetOnePasswordItemByPath(ctx, r.OpClient, resource.Spec.ItemPath)
 	if err != nil {
-		return fmt.Errorf("Failed to retrieve item: %v", err)
+		return fmt.Errorf("failed to retrieve item: %w", err)
 	}
 
 	// Create owner reference.
 	gvk, err := apiutil.GVKForObject(resource, r.Scheme)
 	if err != nil {
-		return fmt.Errorf("could not to retrieve group version kind: %v", err)
+		return fmt.Errorf("could not to retrieve group version kind: %w", err)
 	}
 	ownerRef := &metav1.OwnerReference{
 		APIVersion: gvk.GroupVersion().String(),

internal/controller/onepassworditem_controller_test.go

@@ -2,6 +2,7 @@ package controller
 
 import (
 	"context"
+	"fmt"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 
@@ -303,6 +304,54 @@ var _ = Describe("OnePasswordItem controller", func() {
 			}, timeout, interval).Should(BeTrue())
 			Expect(secret.Type).Should(Equal(v1.SecretType(customType)))
 		})
+
+		It("Should handle 1Password Item with a file and populate secret correctly", func() {
+			ctx := context.Background()
+			spec := onepasswordv1.OnePasswordItemSpec{
+				ItemPath: item1.Path,
+			}
+
+			key := types.NamespacedName{
+				Name:      "item-with-file",
+				Namespace: namespace,
+			}
+
+			toCreate := &onepasswordv1.OnePasswordItem{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      key.Name,
+					Namespace: key.Namespace,
+				},
+				Spec: spec,
+			}
+
+			fileContent := []byte("dummy-cert-content")
+			item := item1.ToModel()
+			item.Files = []model.File{
+				{
+					ID:          "file-id-123",
+					Name:        "server.crt",
+					ContentPath: fmt.Sprintf("/v1/vaults/%s/items/%s/files/file-id-123/content", item.VaultID, item.ID),
+				},
+			}
+			item.Files[0].SetContent(fileContent)
+
+			mockGetItemByIDFunc.Return(item, nil)
+			mockGetItemByIDFunc.On("GetFileContent", item.VaultID, item.ID, "file-id-123").Return(fileContent, nil)
+
+			By("Creating a new OnePasswordItem with file successfully")
+			Expect(k8sClient.Create(ctx, toCreate)).Should(Succeed())
+
+			createdSecret := &v1.Secret{}
+			Eventually(func() bool {
+				err := k8sClient.Get(ctx, key, createdSecret)
+				if err != nil {
+					return false
+				}
+				return true
+			}, timeout, interval).Should(BeTrue())
+
+			Expect(createdSecret.Data).Should(HaveKeyWithValue("server.crt", fileContent))
+		})
 	})
 
 	Context("Unhappy path", func() {

pkg/kubernetessecrets/kubernetes_secrets_builder.go

@@ -120,7 +120,7 @@ func BuildKubernetesSecretData(fields []model.ItemField, files []model.File) map
 	for _, file := range files {
 		content, err := file.Content()
 		if err != nil {
-			log.Error(err, "Could not load contents of file %s", file.Name)
+			log.Error(err, fmt.Sprintf("Could not load contents of file %s", file.Name))
 			continue
 		}
 		if content != nil {

pkg/onepassword/client/connect/connect.go

@@ -2,7 +2,9 @@ package connect
 
 import (
 	"context"
+	"errors"
 	"fmt"
+	"time"
 
 	"github.com/1Password/connect-sdk-go/connect"
 	"github.com/1Password/connect-sdk-go/onepassword"
@@ -30,7 +32,7 @@ func NewClient(config Config) *Connect {
 func (c *Connect) GetItemByID(ctx context.Context, vaultID, itemID string) (*model.Item, error) {
 	connectItem, err := c.client.GetItemByUUID(itemID, vaultID)
 	if err != nil {
-		return nil, fmt.Errorf("1Password Connect error: %w", err)
+		return nil, fmt.Errorf("failed to GetItemByID using 1Password Connect: %w", err)
 	}
 
 	var item model.Item
@@ -42,7 +44,7 @@ func (c *Connect) GetItemsByTitle(ctx context.Context, vaultID, itemTitle string
 	// Get all items in the vault with the specified title
 	connectItems, err := c.client.GetItemsByTitle(itemTitle, vaultID)
 	if err != nil {
-		return nil, fmt.Errorf("1Password Connect error: %w", err)
+		return nil, fmt.Errorf("failed to GetItemsByTitle using 1Password Connect: %w", err)
 	}
 
 	items := make([]model.Item, len(connectItems))
@@ -55,21 +57,38 @@ func (c *Connect) GetItemsByTitle(ctx context.Context, vaultID, itemTitle string
 	return items, nil
 }
 
+// GetFileContent retrieves the content of a file from a 1Password item.
+// As the Connect has a delay when synchronizing files and returns a 500 error in this case, this function implements a retry mechanism.
 func (c *Connect) GetFileContent(ctx context.Context, vaultID, itemID, fileID string) ([]byte, error) {
-	bytes, err := c.client.GetFileContent(&onepassword.File{
-		ContentPath: fmt.Sprintf("/v1/vaults/%s/items/%s/files/%s/content", vaultID, itemID, fileID),
-	})
-	if err != nil {
-		return nil, fmt.Errorf("1Password Connect error: %w", err)
+	const maxRetries = 5
+	const delay = 1 * time.Second
+
+	var lastErr error
+	for i := 0; i < maxRetries; i++ {
+		bytes, err := c.client.GetFileContent(&onepassword.File{
+			ContentPath: fmt.Sprintf("/v1/vaults/%s/items/%s/files/%s/content", vaultID, itemID, fileID),
+		})
+		if err == nil {
+			return bytes, nil
+		}
+
+		var connectErr *onepassword.Error
+		if errors.As(err, &connectErr) && connectErr.StatusCode == 500 {
+			lastErr = err
+			time.Sleep(delay)
+			continue
+		}
+
+		return nil, fmt.Errorf("failed to GetFileContent using 1Password Connect: %w", err)
 	}
 
-	return bytes, nil
+	return nil, fmt.Errorf("failed to GetFileContent using 1Password Connect after %d retries: %w", maxRetries, lastErr)
 }
 
 func (c *Connect) GetVaultsByTitle(ctx context.Context, vaultQuery string) ([]model.Vault, error) {
 	connectVaults, err := c.client.GetVaultsByTitle(vaultQuery)
 	if err != nil {
-		return nil, fmt.Errorf("1Password Connect error: %w", err)
+		return nil, fmt.Errorf("failed to GetVaultsByTitle using 1Password Connect: %w", err)
 	}
 
 	var vaults []model.Vault

pkg/onepassword/client/sdk/sdk.go

@@ -37,7 +37,7 @@ func NewClient(ctx context.Context, config Config) (*SDK, error) {
 func (s *SDK) GetItemByID(ctx context.Context, vaultID, itemID string) (*model.Item, error) {
 	sdkItem, err := s.client.Items().Get(ctx, vaultID, itemID)
 	if err != nil {
-		return nil, fmt.Errorf("1Password sdk error: %w", err)
+		return nil, fmt.Errorf("failed to GetItemsByTitle using 1Password SDK: %w", err)
 	}
 
 	var item model.Item
@@ -49,7 +49,7 @@ func (s *SDK) GetItemsByTitle(ctx context.Context, vaultID, itemTitle string) ([
 	// Get all items in the vault
 	sdkItems, err := s.client.Items().List(ctx, vaultID)
 	if err != nil {
-		return nil, fmt.Errorf("1Password sdk error: %w", err)
+		return nil, fmt.Errorf("failed to GetItemsByTitle using 1Password SDK: %w", err)
 	}
 
 	// Filter items by title
@@ -70,7 +70,7 @@ func (s *SDK) GetFileContent(ctx context.Context, vaultID, itemID, fileID string
 		ID: fileID,
 	})
 	if err != nil {
-		return nil, fmt.Errorf("1Password sdk error: %w", err)
+		return nil, fmt.Errorf("failed to GetFileContent using 1Password SDK: %w", err)
 	}
 
 	return bytes, nil
@@ -80,7 +80,7 @@ func (s *SDK) GetVaultsByTitle(ctx context.Context, title string) ([]model.Vault
 	// List all vaults
 	sdkVaults, err := s.client.Vaults().List(ctx)
 	if err != nil {
-		return nil, fmt.Errorf("1Password sdk error: %w", err)
+		return nil, fmt.Errorf("failed to GetVaultsByTitle using 1Password SDK: %w", err)
 	}
 
 	// Filter vaults by title

pkg/onepassword/items.go

@@ -33,11 +33,12 @@ func GetOnePasswordItemByPath(ctx context.Context, opClient opclient.Client, pat
 		return nil, fmt.Errorf("faield to 'GetItemByID' for vaultID='%s' and itemID='%s': %w", vaultID, itemID, err)
 	}
 
-	for _, file := range item.Files {
-		_, err := opClient.GetFileContent(ctx, vaultID, itemID, file.ID)
+	for i, file := range item.Files {
+		content, err := opClient.GetFileContent(ctx, vaultID, itemID, file.ID)
 		if err != nil {
 			return nil, err
 		}
+		item.Files[i].SetContent(content)
 	}
 
 	return item, nil

pkg/onepassword/model/item.go

@@ -70,6 +70,15 @@ func (i *Item) FromSDKItem(item *sdk.Item) {
 		})
 	}
 
+	// Items of 'Document' category keeps file information in the Document field.
+	if item.Category == sdk.ItemCategoryDocument {
+		i.Files = append(i.Files, File{
+			ID:   item.Document.ID,
+			Name: item.Document.Name,
+			Size: int(item.Document.Size),
+		})
+	}
+
 	i.CreatedAt = item.CreatedAt
 }
 

pkg/onepassword/secret_update_handler.go

@@ -123,7 +123,7 @@ func (h *SecretUpdateHandler) updateKubernetesSecrets(ctx context.Context) (map[
 
 		item, err := GetOnePasswordItemByPath(ctx, h.opClient, OnePasswordItemPath)
 		if err != nil {
-			log.Error(err, "failed to retrieve 1Password item at path \"%s\" for secret \"%s\"", secret.Annotations[ItemPathAnnotation], secret.Name)
+			log.Error(err, fmt.Sprintf("failed to retrieve 1Password item at path %s for secret %s", secret.Annotations[ItemPathAnnotation], secret.Name))
 			continue
 		}
 
@@ -136,7 +136,7 @@ func (h *SecretUpdateHandler) updateKubernetesSecrets(ctx context.Context) (map[
 				secret.Annotations[VersionAnnotation] = itemVersion
 				secret.Annotations[ItemPathAnnotation] = itemPathString
 				if err := h.client.Update(ctx, &secret); err != nil {
-					log.Error(err, "failed to update secret %s annotations to version %d: %s", secret.Name, itemVersion, err)
+					log.Error(err, fmt.Sprintf("failed to update secret %s annotations to version %s", secret.Name, itemVersion))
 					continue
 				}
 				continue
@@ -147,7 +147,7 @@ func (h *SecretUpdateHandler) updateKubernetesSecrets(ctx context.Context) (map[
 			secret.Data = kubeSecrets.BuildKubernetesSecretData(item.Fields, item.Files)
 			log.V(logs.DebugLevel).Info(fmt.Sprintf("New secret path: %v and version: %v", secret.Annotations[ItemPathAnnotation], secret.Annotations[VersionAnnotation]))
 			if err := h.client.Update(ctx, &secret); err != nil {
-				log.Error(err, "failed to update secret %s to version %d: %s", secret.Name, itemVersion, err)
+				log.Error(err, fmt.Sprintf("failed to update secret %s to version %s", secret.Name, itemVersion))
 				continue
 			}
 			if updatedSecrets[secret.Namespace] == nil {
@@ -218,7 +218,7 @@ func isSecretSetForAutoRestart(secret *corev1.Secret, deployment *appsv1.Deploym
 
 	restartDeploymentBool, err := utils.StringToBool(restartDeployment)
 	if err != nil {
-		log.Error(err, "Error parsing %v annotation on Secret %v. Must be true or false. Defaulting to false.", RestartDeploymentsAnnotation, secret.Name)
+		log.Error(err, fmt.Sprintf("Error parsing %s annotation on Secret %s. Must be true or false. Defaulting to false.", RestartDeploymentsAnnotation, secret.Name))
 		return false
 	}
 	return restartDeploymentBool
@@ -233,7 +233,7 @@ func isDeploymentSetForAutoRestart(deployment *appsv1.Deployment, setForAutoRest
 
 	restartDeploymentBool, err := utils.StringToBool(restartDeployment)
 	if err != nil {
-		log.Error(err, "Error parsing %v annotation on Deployment %v. Must be true or false. Defaulting to false.", RestartDeploymentsAnnotation, deployment.Name)
+		log.Error(err, fmt.Sprintf("Error parsing %s annotation on Deployment %s. Must be true or false. Defaulting to false.", RestartDeploymentsAnnotation, deployment.Name))
 		return false
 	}
 	return restartDeploymentBool
@@ -248,7 +248,7 @@ func (h *SecretUpdateHandler) isNamespaceSetToAutoRestart(namespace *corev1.Name
 
 	restartDeploymentBool, err := utils.StringToBool(restartDeployment)
 	if err != nil {
-		log.Error(err, "Error parsing %v annotation on Namespace %v. Must be true or false. Defaulting to false.", RestartDeploymentsAnnotation, namespace.Name)
+		log.Error(err, fmt.Sprintf("Error parsing %s annotation on Namespace %s. Must be true or false. Defaulting to false.", RestartDeploymentsAnnotation, namespace.Name))
 		return false
 	}
 	return restartDeploymentBool