From 1e9c04ee05e4ac2b5cfbbdcd2d6c57a4ffbef30f Mon Sep 17 00:00:00 2001 From: Volodymyr Zotov Date: Tue, 8 Jul 2025 16:41:49 -0500 Subject: [PATCH 1/8] Fix logging the error so it doesn't panic --- pkg/kubernetessecrets/kubernetes_secrets_builder.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/kubernetessecrets/kubernetes_secrets_builder.go b/pkg/kubernetessecrets/kubernetes_secrets_builder.go index 3105d2a..dfb282f 100644 --- a/pkg/kubernetessecrets/kubernetes_secrets_builder.go +++ b/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 { From 5980e7e63a326289590a68a3e62d1f376082f0df Mon Sep 17 00:00:00 2001 From: Volodymyr Zotov Date: Tue, 8 Jul 2025 16:42:38 -0500 Subject: [PATCH 2/8] Wrap the error to not lose child errors --- internal/controller/deployment_controller.go | 4 ++-- internal/controller/onepassworditem_controller.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/controller/deployment_controller.go b/internal/controller/deployment_controller.go index f031620..b299cec 100644 --- a/internal/controller/deployment_controller.go +++ b/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(), diff --git a/internal/controller/onepassworditem_controller.go b/internal/controller/onepassworditem_controller.go index 3dfd046..078af65 100644 --- a/internal/controller/onepassworditem_controller.go +++ b/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(), From 24d3f6f043127258597c221e771506d990fad753 Mon Sep 17 00:00:00 2001 From: Volodymyr Zotov Date: Tue, 8 Jul 2025 16:44:09 -0500 Subject: [PATCH 3/8] Retry when getting content file via Connect Connect has a delay when synchronizing files and returns a 500 error in this case, this function implements a retry mechanism. In this case we handle retries in the code and do not print redundunt errors in the POD logs. --- pkg/onepassword/client/connect/connect.go | 37 +++++++++++++++++------ 1 file changed, 28 insertions(+), 9 deletions(-) diff --git a/pkg/onepassword/client/connect/connect.go b/pkg/onepassword/client/connect/connect.go index f528e5c..fc967a5 100644 --- a/pkg/onepassword/client/connect/connect.go +++ b/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("GetItemByID 1Password Connect error: %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("GetItemsByTitle 1Password Connect error: %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("GetFileContent 1Password Connect error: %w", err) } - return bytes, nil + return nil, fmt.Errorf("GetFileContent failed after retries: %w", 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("GetVaultsByTitle 1Password Connect error: %w", err) } var vaults []model.Vault From 9cee6595d5be1632537fc5e02fd7b7abe74f9aff Mon Sep 17 00:00:00 2001 From: Volodymyr Zotov Date: Tue, 8 Jul 2025 16:46:10 -0500 Subject: [PATCH 4/8] Set file content after fetching it --- pkg/onepassword/items.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/onepassword/items.go b/pkg/onepassword/items.go index fb660fd..534c000 100644 --- a/pkg/onepassword/items.go +++ b/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 From b35acb7d13248b8dbfbc0635a2e308994eba77ec Mon Sep 17 00:00:00 2001 From: Volodymyr Zotov Date: Tue, 8 Jul 2025 16:57:59 -0500 Subject: [PATCH 5/8] Add test case for item with file --- .../onepassworditem_controller_test.go | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/internal/controller/onepassworditem_controller_test.go b/internal/controller/onepassworditem_controller_test.go index 58cf9f2..701de59 100644 --- a/internal/controller/onepassworditem_controller_test.go +++ b/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() { From 6ef0da2d17283f5703059db31afbdf005651d302 Mon Sep 17 00:00:00 2001 From: Volodymyr Zotov Date: Thu, 10 Jul 2025 14:03:57 -0500 Subject: [PATCH 6/8] Add file from document --- pkg/onepassword/model/item.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/pkg/onepassword/model/item.go b/pkg/onepassword/model/item.go index ebf13e4..9f4af44 100644 --- a/pkg/onepassword/model/item.go +++ b/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 } From dbd7408fac9d33d38f95d63d337cc774a042a4db Mon Sep 17 00:00:00 2001 From: Volodymyr Zotov Date: Thu, 10 Jul 2025 15:40:53 -0500 Subject: [PATCH 7/8] Update errors text --- pkg/onepassword/client/connect/connect.go | 10 +++++----- pkg/onepassword/client/sdk/sdk.go | 8 ++++---- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/pkg/onepassword/client/connect/connect.go b/pkg/onepassword/client/connect/connect.go index fc967a5..0c77a3d 100644 --- a/pkg/onepassword/client/connect/connect.go +++ b/pkg/onepassword/client/connect/connect.go @@ -32,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("GetItemByID 1Password Connect error: %w", err) + return nil, fmt.Errorf("failed to GetItemByID using 1Password Connect: %w", err) } var item model.Item @@ -44,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("GetItemsByTitle 1Password Connect error: %w", err) + return nil, fmt.Errorf("failed to GetItemsByTitle using 1Password Connect: %w", err) } items := make([]model.Item, len(connectItems)) @@ -79,16 +79,16 @@ func (c *Connect) GetFileContent(ctx context.Context, vaultID, itemID, fileID st continue } - return nil, fmt.Errorf("GetFileContent 1Password Connect error: %w", err) + return nil, fmt.Errorf("failed to GetFileContent using 1Password Connect: %w", err) } - return nil, fmt.Errorf("GetFileContent failed after retries: %w", lastErr) + 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("GetVaultsByTitle 1Password Connect error: %w", err) + return nil, fmt.Errorf("failed to GetVaultsByTitle using 1Password Connect: %w", err) } var vaults []model.Vault diff --git a/pkg/onepassword/client/sdk/sdk.go b/pkg/onepassword/client/sdk/sdk.go index 6c90700..997f67c 100644 --- a/pkg/onepassword/client/sdk/sdk.go +++ b/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 From 1d613eac2b4a36f838c627821fc61549681e8c63 Mon Sep 17 00:00:00 2001 From: Volodymyr Zotov Date: Thu, 10 Jul 2025 15:46:02 -0500 Subject: [PATCH 8/8] Fix logging errors so it won't panic --- pkg/onepassword/secret_update_handler.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/onepassword/secret_update_handler.go b/pkg/onepassword/secret_update_handler.go index 1042740..a1e0e1e 100644 --- a/pkg/onepassword/secret_update_handler.go +++ b/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