From 3f865370a67d57ec704de2eae625a8897fbcffe9 Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Sun, 22 Mar 2026 20:00:41 +0100 Subject: [PATCH 1/4] feat(credentials): add in-place credential rotation via upsert Introduces WithSecretName functional option to SaveCredentials, allowing callers to provide an existing secret path for in-place upsert instead of always generating a new UUID-based path. Updates CAS backend Update() to pass the existing secret name on rotation, eliminating orphaned credentials in all supported secret managers (Vault, AWS, GCP, Azure Key Vault). Signed-off-by: Miguel Martinez Trivino --- app/controlplane/pkg/biz/casbackend.go | 8 +- app/controlplane/pkg/biz/casbackend_test.go | 89 +++++++++++++++++- .../aws/mocks/SecretsManagerIface.go | 46 ++++++++-- pkg/credentials/aws/secretmanager.go | 40 ++++++-- pkg/credentials/aws/secretmanager_test.go | 72 ++++++++++++++- pkg/credentials/azurekv/keyvault.go | 13 ++- pkg/credentials/azurekv/keyvault_test.go | 52 ++++++++++- pkg/credentials/credentials.go | 29 +++++- pkg/credentials/gcp/secretmanager.go | 91 +++++++++++++------ pkg/credentials/gcp/secretmanager_test.go | 76 +++++++++++++++- pkg/credentials/mocks/ReaderWriter.go | 50 +++++++--- pkg/credentials/mocks/Writer.go | 50 +++++++--- pkg/credentials/vault/keyval.go | 12 ++- pkg/credentials/vault/keyval_test.go | 53 ++++++++++- 14 files changed, 591 insertions(+), 90 deletions(-) diff --git a/app/controlplane/pkg/biz/casbackend.go b/app/controlplane/pkg/biz/casbackend.go index 4e99773c0..0bc8066d4 100644 --- a/app/controlplane/pkg/biz/casbackend.go +++ b/app/controlplane/pkg/biz/casbackend.go @@ -1,5 +1,5 @@ // -// Copyright 2024-2025 The Chainloop Authors. +// Copyright 2024-2026 The Chainloop Authors. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -429,7 +429,11 @@ func (uc *CASBackendUseCase) Update(ctx context.Context, orgID, id string, descr credentialsUpdated := creds != nil // We want to rotate credentials if creds != nil { - secretName, err = uc.credsRW.SaveCredentials(ctx, orgID, creds) + var saveOpts []credentials.SaveOption + if before.SecretName != "" { + saveOpts = append(saveOpts, credentials.WithSecretName(before.SecretName)) + } + secretName, err = uc.credsRW.SaveCredentials(ctx, orgID, creds, saveOpts...) if err != nil { return nil, fmt.Errorf("storing the credentials: %w", err) } diff --git a/app/controlplane/pkg/biz/casbackend_test.go b/app/controlplane/pkg/biz/casbackend_test.go index 54afa8141..f049d8b04 100644 --- a/app/controlplane/pkg/biz/casbackend_test.go +++ b/app/controlplane/pkg/biz/casbackend_test.go @@ -1,5 +1,5 @@ // -// Copyright 2024-2025 The Chainloop Authors. +// Copyright 2024-2026 The Chainloop Authors. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -264,6 +264,93 @@ func (s *casBackendTestSuite) TestNewCASBackendUseCase() { } } +// TestUpdateRotatesCredentialsInPlace verifies that Update() passes the existing SecretName +// as a WithSecretName option so the credential store upserts in-place instead of +// creating a new entry. +func (s *casBackendTestSuite) TestUpdateRotatesCredentialsInPlace() { + ctx := context.Background() + existingSecretName := "org/existing-secret-path" + backendID := uuid.New() + newCreds := &credentials.OCIKeypair{Repo: "r", Username: "u", Password: "p"} + + tests := []struct { + name string + existingSecret string + wantWithSecretName bool // whether the SaveOption should carry the existing secret name + returnedSecretName string // what SaveCredentials mock returns + }{ + { + name: "existing secret name is forwarded as WithSecretName", + existingSecret: existingSecretName, + wantWithSecretName: true, + returnedSecretName: existingSecretName, + }, + { + name: "empty secret name generates a new path", + existingSecret: "", + wantWithSecretName: false, + returnedSecretName: "new-secret-path", + }, + } + + for _, tc := range tests { + s.Run(tc.name, func() { + s.resetMock() + + before := &biz.CASBackend{ + ID: backendID, + SecretName: tc.existingSecret, + Provider: backendType, + } + + // Step 1: FindByIDInOrg returns the existing backend (consumed once). + s.repo.On("FindByIDInOrg", ctx, s.validUUID, backendID).Return(before, nil).Once() + + // Step 2: SaveCredentials — capture SaveOption(s) to verify the secret name. + var capturedSecretName string + saveMatcher := mock.MatchedBy(func(_ interface{}) bool { return true }) + s.credsRW.On("SaveCredentials", ctx, s.validUUID.String(), saveMatcher, mock.Anything). + Run(func(args mock.Arguments) { + // args[3] is the first SaveOption when opts is non-empty. + if len(args) > 3 { + if opt, ok := args.Get(3).(credentials.SaveOption); ok { + o := credentials.ApplySaveOptions([]credentials.SaveOption{opt}) + capturedSecretName = o.SecretName + } + } + }).Return(tc.returnedSecretName, nil).Maybe() + + // Fallback for no-opts case (empty existing secret → no WithSecretName). + s.credsRW.On("SaveCredentials", ctx, s.validUUID.String(), saveMatcher). + Return(tc.returnedSecretName, nil).Maybe() + + // Step 3: repo.Update persists the change. + updatedBackend := &biz.CASBackend{ID: backendID, SecretName: tc.returnedSecretName, Provider: backendType} + s.repo.On("Update", ctx, mock.Anything).Return(updatedBackend, nil) + + // Steps 4–7: PerformValidation is called internally with new creds. + s.repo.On("FindByID", mock.Anything, backendID).Return(updatedBackend, nil) + s.credsRW.On("ReadCredentials", mock.Anything, mock.Anything, mock.Anything).Return(nil) + s.backendProvider.On("ValidateAndExtractCredentials", mock.Anything, mock.Anything).Return(nil, nil) + s.repo.On("UpdateValidationStatus", mock.Anything, backendID, biz.CASBackendValidationOK, mock.Anything).Return(nil) + + // Step 8: FindByIDInOrg reload after validation. + s.repo.On("FindByIDInOrg", ctx, s.validUUID, backendID).Return(updatedBackend, nil) + + got, err := s.useCase.Update(ctx, s.validUUID.String(), backendID.String(), nil, newCreds, nil, nil, nil) + s.Require().NoError(err) + s.Equal(tc.returnedSecretName, got.SecretName) + + if tc.wantWithSecretName { + s.Equal(existingSecretName, capturedSecretName, + "expected WithSecretName(%q) to be forwarded to SaveCredentials", existingSecretName) + } else { + s.Empty(capturedSecretName, "expected no WithSecretName when existingSecret is empty") + } + }) + } +} + // Run all the tests func TestCASBackend(t *testing.T) { suite.Run(t, new(casBackendTestSuite)) diff --git a/pkg/credentials/aws/mocks/SecretsManagerIface.go b/pkg/credentials/aws/mocks/SecretsManagerIface.go index b54cd46e2..aec03d6c9 100644 --- a/pkg/credentials/aws/mocks/SecretsManagerIface.go +++ b/pkg/credentials/aws/mocks/SecretsManagerIface.go @@ -1,5 +1,4 @@ -// -// Copyright 2023 The Chainloop Authors. +// Copyright 2023-2026 The Chainloop Authors. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -13,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -// Code generated by mockery v2.20.0. DO NOT EDIT. +// Code generated by mockery v3. DO NOT EDIT. package mocks @@ -62,6 +61,39 @@ func (_m *SecretsManagerIface) CreateSecret(ctx context.Context, params *secrets return r0, r1 } +// PutSecretValue provides a mock function with given fields: ctx, params, optFns +func (_m *SecretsManagerIface) PutSecretValue(ctx context.Context, params *secretsmanager.PutSecretValueInput, optFns ...func(*secretsmanager.Options)) (*secretsmanager.PutSecretValueOutput, error) { + _va := make([]interface{}, len(optFns)) + for _i := range optFns { + _va[_i] = optFns[_i] + } + var _ca []interface{} + _ca = append(_ca, ctx, params) + _ca = append(_ca, _va...) + ret := _m.Called(_ca...) + + var r0 *secretsmanager.PutSecretValueOutput + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, *secretsmanager.PutSecretValueInput, ...func(*secretsmanager.Options)) (*secretsmanager.PutSecretValueOutput, error)); ok { + return rf(ctx, params, optFns...) + } + if rf, ok := ret.Get(0).(func(context.Context, *secretsmanager.PutSecretValueInput, ...func(*secretsmanager.Options)) *secretsmanager.PutSecretValueOutput); ok { + r0 = rf(ctx, params, optFns...) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*secretsmanager.PutSecretValueOutput) + } + } + + if rf, ok := ret.Get(1).(func(context.Context, *secretsmanager.PutSecretValueInput, ...func(*secretsmanager.Options)) error); ok { + r1 = rf(ctx, params, optFns...) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + // DeleteSecret provides a mock function with given fields: ctx, params, optFns func (_m *SecretsManagerIface) DeleteSecret(ctx context.Context, params *secretsmanager.DeleteSecretInput, optFns ...func(*secretsmanager.Options)) (*secretsmanager.DeleteSecretOutput, error) { _va := make([]interface{}, len(optFns)) @@ -128,13 +160,11 @@ func (_m *SecretsManagerIface) GetSecretValue(ctx context.Context, params *secre return r0, r1 } -type mockConstructorTestingTNewSecretsManagerIface interface { +// NewSecretsManagerIface creates a new instance of SecretsManagerIface. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. +func NewSecretsManagerIface(t interface { mock.TestingT Cleanup(func()) -} - -// NewSecretsManagerIface creates a new instance of SecretsManagerIface. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. -func NewSecretsManagerIface(t mockConstructorTestingTNewSecretsManagerIface) *SecretsManagerIface { +}) *SecretsManagerIface { mock := &SecretsManagerIface{} mock.Mock.Test(t) diff --git a/pkg/credentials/aws/secretmanager.go b/pkg/credentials/aws/secretmanager.go index 4d504a0fd..022a6cb73 100644 --- a/pkg/credentials/aws/secretmanager.go +++ b/pkg/credentials/aws/secretmanager.go @@ -26,6 +26,7 @@ import ( "github.com/aws/aws-sdk-go-v2/aws" awscreds "github.com/aws/aws-sdk-go-v2/credentials" "github.com/aws/aws-sdk-go-v2/service/secretsmanager" + smtypes "github.com/aws/aws-sdk-go-v2/service/secretsmanager/types" "github.com/aws/aws-sdk-go-v2/service/sso/types" "github.com/aws/smithy-go" "github.com/chainloop-dev/chainloop/pkg/credentials" @@ -38,6 +39,7 @@ import ( type SecretsManagerIface interface { GetSecretValue(ctx context.Context, params *secretsmanager.GetSecretValueInput, optFns ...func(*secretsmanager.Options)) (*secretsmanager.GetSecretValueOutput, error) CreateSecret(ctx context.Context, params *secretsmanager.CreateSecretInput, optFns ...func(*secretsmanager.Options)) (*secretsmanager.CreateSecretOutput, error) + PutSecretValue(ctx context.Context, params *secretsmanager.PutSecretValueInput, optFns ...func(*secretsmanager.Options)) (*secretsmanager.PutSecretValueOutput, error) DeleteSecret(ctx context.Context, params *secretsmanager.DeleteSecretInput, optFns ...func(*secretsmanager.Options)) (*secretsmanager.DeleteSecretOutput, error) } @@ -79,10 +81,13 @@ func NewManager(opts *NewManagerOpts) (*Manager, error) { }, nil } -// Save Credentials, this is a generic function that can be used to save any type of credentials -// as long as they can be passed to json.Marshal -func (m *Manager) SaveCredentials(ctx context.Context, orgID string, creds any) (string, error) { - secretName := strings.Join([]string{m.secretPrefix, orgID, uuid.New().String()}, "/") +// SaveCredentials saves credentials. If opts includes WithSecretName, upserts at the given path. +func (m *Manager) SaveCredentials(ctx context.Context, orgID string, creds any, opts ...credentials.SaveOption) (string, error) { + o := credentials.ApplySaveOptions(opts) + secretName := o.SecretName + if secretName == "" { + secretName = strings.Join([]string{m.secretPrefix, orgID, uuid.New().String()}, "/") + } // Store the credentials as json key pairs c, err := json.Marshal(creds) @@ -90,10 +95,29 @@ func (m *Manager) SaveCredentials(ctx context.Context, orgID string, creds any) return "", fmt.Errorf("marshaling credentials to be stored: %w", err) } - if _, err = m.client.CreateSecret(ctx, &secretsmanager.CreateSecretInput{ - Name: aws.String(secretName), SecretString: aws.String(string(c)), - }); err != nil { - return "", fmt.Errorf("creating secret in AWS: %w", err) + if o.SecretName != "" { + // Upsert: try to update existing secret first + _, err = m.client.PutSecretValue(ctx, &secretsmanager.PutSecretValueInput{ + SecretId: aws.String(secretName), + SecretString: aws.String(string(c)), + }) + var notFoundErr *smtypes.ResourceNotFoundException + if errors.As(err, ¬FoundErr) { + // Secret does not exist yet, create it + _, err = m.client.CreateSecret(ctx, &secretsmanager.CreateSecretInput{ + Name: aws.String(secretName), + SecretString: aws.String(string(c)), + }) + } + } else { + _, err = m.client.CreateSecret(ctx, &secretsmanager.CreateSecretInput{ + Name: aws.String(secretName), + SecretString: aws.String(string(c)), + }) + } + + if err != nil { + return "", fmt.Errorf("saving secret in AWS: %w", err) } return secretName, nil diff --git a/pkg/credentials/aws/secretmanager_test.go b/pkg/credentials/aws/secretmanager_test.go index a20b24f95..afe05c402 100644 --- a/pkg/credentials/aws/secretmanager_test.go +++ b/pkg/credentials/aws/secretmanager_test.go @@ -1,5 +1,5 @@ // -// Copyright 2023 The Chainloop Authors. +// Copyright 2023-2026 The Chainloop Authors. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -24,6 +24,7 @@ import ( "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/service/secretsmanager" + smtypes "github.com/aws/aws-sdk-go-v2/service/secretsmanager/types" "github.com/aws/aws-sdk-go-v2/service/sso/types" "github.com/aws/smithy-go" "github.com/chainloop-dev/chainloop/pkg/credentials" @@ -202,6 +203,75 @@ func (s *testSuite) TestReadWriteCredentials() { } } +func (s *testSuite) TestSaveCredentialsUpsert() { + assert := assert.New(s.T()) + creds := &credentials.OCIKeypair{Repo: "r", Username: "u", Password: "p"} + existingSecretName := "org/existing-secret" + + testCases := []struct { + name string + secretName string // non-empty = WithSecretName upsert + awsNotFound bool // simulate ResourceNotFoundException on PutSecretValue + expectedError bool + }{ + { + name: "new secret — CreateSecret called", + secretName: "", + }, + { + name: "upsert existing — PutSecretValue succeeds", + secretName: existingSecretName, + }, + { + name: "upsert not found — fallback to CreateSecret", + secretName: existingSecretName, + awsNotFound: true, + }, + } + + for _, tc := range testCases { + s.Run(tc.name, func() { + initMockedManager(s) + m := s.mockedManager + mc, _ := m.client.(*mclient.SecretsManagerIface) + ctx := context.Background() + + var saveOpts []credentials.SaveOption + if tc.secretName != "" { + saveOpts = append(saveOpts, credentials.WithSecretName(tc.secretName)) + } + + switch { + case tc.secretName == "": + // New secret path: only CreateSecret is called. + mc.On("CreateSecret", ctx, mock.Anything).Return(nil, nil) + case tc.awsNotFound: + // Upsert path: PutSecretValue returns not-found, then CreateSecret. + mc.On("PutSecretValue", ctx, mock.Anything).Return(nil, &smtypes.ResourceNotFoundException{}) + mc.On("CreateSecret", ctx, mock.Anything).Return(nil, nil) + default: + // Upsert path: PutSecretValue succeeds directly. + mc.On("PutSecretValue", ctx, mock.Anything).Return(&secretsmanager.PutSecretValueOutput{}, nil) + } + + returned, err := m.SaveCredentials(ctx, orgID, creds, saveOpts...) + if tc.expectedError { + assert.Error(err) + return + } + assert.NoError(err) + + if tc.secretName != "" { + // In-place upsert must return the same path. + assert.Equal(tc.secretName, returned) + } else { + // New path must be auto-generated (non-empty and different from the existing one). + assert.NotEmpty(returned) + } + }) + } +} + // // Create a new secret, delete it and check it does not exist antymore func (s *testSuite) TestDeleteCredentials() { assert := assert.New(s.T()) diff --git a/pkg/credentials/azurekv/keyvault.go b/pkg/credentials/azurekv/keyvault.go index 711c6711b..2348923ec 100644 --- a/pkg/credentials/azurekv/keyvault.go +++ b/pkg/credentials/azurekv/keyvault.go @@ -1,5 +1,5 @@ // -// Copyright 2023 The Chainloop Authors. +// Copyright 2023-2026 The Chainloop Authors. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -120,9 +120,14 @@ func NewManager(opts *NewManagerOpts) (*Manager, error) { }, nil } -// SaveCredentials saves credentials -func (m *Manager) SaveCredentials(ctx context.Context, orgID string, creds any) (string, error) { - secretName := strings.Join([]string{m.secretPrefix, orgID, uuid.New().String()}, "-") +// SaveCredentials saves credentials. If opts includes WithSecretName, upserts at the given path. +func (m *Manager) SaveCredentials(ctx context.Context, orgID string, creds any, opts ...credentials.SaveOption) (string, error) { + o := credentials.ApplySaveOptions(opts) + secretName := o.SecretName + if secretName == "" { + secretName = strings.Join([]string{m.secretPrefix, orgID, uuid.New().String()}, "-") + } + // Store the credentials as json key pairs c, err := json.Marshal(creds) if err != nil { diff --git a/pkg/credentials/azurekv/keyvault_test.go b/pkg/credentials/azurekv/keyvault_test.go index 5b8986aa7..b5d4020fd 100644 --- a/pkg/credentials/azurekv/keyvault_test.go +++ b/pkg/credentials/azurekv/keyvault_test.go @@ -1,5 +1,5 @@ // -// Copyright 2023 The Chainloop Authors. +// Copyright 2023-2026 The Chainloop Authors. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -159,6 +159,56 @@ func (s *testSuite) TestSaveCredentials() { }) } +func (s *testSuite) TestSaveCredentialsUpsert() { + creds := &credentials.APICreds{Host: "myhost", Key: "mykey"} + existingSecretName := "org-existing-secret" + + testCases := []struct { + name string + secretName string // non-empty = WithSecretName upsert + }{ + { + name: "new secret — auto-generated name", + secretName: "", + }, + { + name: "upsert existing — SetSecret called with given name", + secretName: existingSecretName, + }, + } + + for _, tc := range testCases { + s.Run(tc.name, func() { + ctx := context.Background() + secretsRW := mocks.NewSecretsRW(s.T()) + + var capturedName string + secretsRW.On("SetSecret", ctx, mock.Anything, mock.Anything, mock.Anything). + Return(azsecrets.SetSecretResponse{}, nil). + Run(func(args mock.Arguments) { + capturedName = args.Get(1).(string) + }) + + m := &Manager{client: secretsRW, secretPrefix: "my-prefix"} + + var saveOpts []credentials.SaveOption + if tc.secretName != "" { + saveOpts = append(saveOpts, credentials.WithSecretName(tc.secretName)) + } + + returned, err := m.SaveCredentials(ctx, "my-org", creds, saveOpts...) + s.NoError(err) + s.Equal(capturedName, returned) + + if tc.secretName != "" { + s.Equal(tc.secretName, returned, "upsert must return the existing secret name unchanged") + } else { + s.NotEmpty(returned) + } + }) + } +} + func (s *testSuite) TestReadCredentials() { want := &credentials.APICreds{ Host: "myhost", diff --git a/pkg/credentials/credentials.go b/pkg/credentials/credentials.go index 53734337c..a62ee615d 100644 --- a/pkg/credentials/credentials.go +++ b/pkg/credentials/credentials.go @@ -1,5 +1,5 @@ // -// Copyright 2023 The Chainloop Authors. +// Copyright 2023-2026 The Chainloop Authors. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -34,8 +34,33 @@ type ReaderWriter interface { Writer } +// SaveOpts holds optional parameters for SaveCredentials. Exported for use by sub-packages. +type SaveOpts struct { + // SecretName when non-empty causes SaveCredentials to upsert at the given path + // instead of generating a new UUID-based path. + SecretName string +} + +// SaveOption is a functional option for SaveCredentials. +type SaveOption func(*SaveOpts) + +// WithSecretName instructs SaveCredentials to upsert at the provided path. +// An empty name is a no-op (auto-generate behavior is preserved). +func WithSecretName(name string) SaveOption { + return func(o *SaveOpts) { o.SecretName = name } +} + +// ApplySaveOptions applies the given options and returns the resulting SaveOpts. +func ApplySaveOptions(opts []SaveOption) SaveOpts { + o := SaveOpts{} + for _, opt := range opts { + opt(&o) + } + return o +} + type Writer interface { - SaveCredentials(ctx context.Context, org string, credentials any) (string, error) + SaveCredentials(ctx context.Context, org string, credentials any, opts ...SaveOption) (string, error) DeleteCredentials(ctx context.Context, credID string) error } diff --git a/pkg/credentials/gcp/secretmanager.go b/pkg/credentials/gcp/secretmanager.go index 601a18f0d..6469520ce 100644 --- a/pkg/credentials/gcp/secretmanager.go +++ b/pkg/credentials/gcp/secretmanager.go @@ -1,5 +1,5 @@ // -// Copyright 2023 The Chainloop Authors. +// Copyright 2023-2026 The Chainloop Authors. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -26,6 +26,8 @@ import ( secretmanager "cloud.google.com/go/secretmanager/apiv1" "cloud.google.com/go/secretmanager/apiv1/secretmanagerpb" "google.golang.org/api/option" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" "github.com/chainloop-dev/chainloop/pkg/credentials" "github.com/chainloop-dev/chainloop/pkg/servicelogger" @@ -83,42 +85,71 @@ func NewManager(opts *NewManagerOpts) (*Manager, error) { }, nil } -// SaveCredentials saves credentials -func (m *Manager) SaveCredentials(ctx context.Context, orgID string, creds any) (string, error) { +// SaveCredentials saves credentials. If opts includes WithSecretName, upserts at the given path. +func (m *Manager) SaveCredentials(ctx context.Context, orgID string, creds any, opts ...credentials.SaveOption) (string, error) { // store creds in key-value pair c, err := json.Marshal(creds) if err != nil { return "", fmt.Errorf("marshaling credentials to be stored: %w", err) } - secretID := strings.Join([]string{m.secretPrefix, orgID, uuid.Generate().String()}, "-") + o := credentials.ApplySaveOptions(opts) + secretID := o.SecretName + if secretID == "" { + secretID = strings.Join([]string{m.secretPrefix, orgID, uuid.Generate().String()}, "-") + } - // first create the secret itself - createSecretReq := &secretmanagerpb.CreateSecretRequest{ - Parent: fmt.Sprintf("projects/%s", m.projectID), - SecretId: secretID, - Secret: &secretmanagerpb.Secret{ - Replication: &secretmanagerpb.Replication{ - Replication: &secretmanagerpb.Replication_Automatic_{ - Automatic: &secretmanagerpb.Replication_Automatic{}, + parent := fmt.Sprintf("projects/%s/secrets/%s", m.projectID, secretID) + + if o.SecretName != "" { + // Upsert path: check if secret container exists + _, err = m.client.GetSecret(ctx, &secretmanagerpb.GetSecretRequest{Name: parent}) + if isNotFoundErr(err) { + // Secret does not exist, create the container first + _, err = m.client.CreateSecret(ctx, &secretmanagerpb.CreateSecretRequest{ + Parent: fmt.Sprintf("projects/%s", m.projectID), + SecretId: secretID, + Secret: &secretmanagerpb.Secret{ + Replication: &secretmanagerpb.Replication{ + Replication: &secretmanagerpb.Replication_Automatic_{ + Automatic: &secretmanagerpb.Replication_Automatic{}, + }, + }, + }, + }) + if err != nil { + return "", fmt.Errorf("creating secret in GCP: %w", err) + } + m.logger.Infow("msg", "created new secret for upsert", "secretID", secretID) + } else if err != nil { + return "", fmt.Errorf("checking secret existence in GCP: %w", err) + } + } else { + // New secret path: create the container + secret, err := m.client.CreateSecret(ctx, &secretmanagerpb.CreateSecretRequest{ + Parent: fmt.Sprintf("projects/%s", m.projectID), + SecretId: secretID, + Secret: &secretmanagerpb.Secret{ + Replication: &secretmanagerpb.Replication{ + Replication: &secretmanagerpb.Replication_Automatic_{ + Automatic: &secretmanagerpb.Replication_Automatic{}, + }, }, }, - }, - } - secret, err := m.client.CreateSecret(ctx, createSecretReq) - if err != nil { - return "", fmt.Errorf("creating secret in GCP: %w", err) + }) + if err != nil { + return "", fmt.Errorf("creating secret in GCP: %w", err) + } + // Use the fully-qualified name returned by CreateSecret + parent = secret.Name + m.logger.Infow("msg", "created new secret", "secretID", secretID) } - m.logger.Infow("msg", "created new secret", "secretID", secretID) - - // once the secret is created store it as the newest version - addSecretVersionReq := &secretmanagerpb.AddSecretVersionRequest{ - Parent: secret.Name, - Payload: &secretmanagerpb.SecretPayload{ - Data: c, - }, - } - v, err := m.client.AddSecretVersion(ctx, addSecretVersionReq) + + // Add a new version with the credential payload + v, err := m.client.AddSecretVersion(ctx, &secretmanagerpb.AddSecretVersionRequest{ + Parent: parent, + Payload: &secretmanagerpb.SecretPayload{Data: c}, + }) if err != nil { return "", fmt.Errorf("creating secret version in GCP: %w", err) } @@ -127,6 +158,12 @@ func (m *Manager) SaveCredentials(ctx context.Context, orgID string, creds any) return secretID, nil } +// isNotFoundErr returns true when the error is a gRPC NotFound status. +func isNotFoundErr(err error) bool { + s, ok := status.FromError(err) + return ok && s.Code() == codes.NotFound +} + // ReadCredentials reads the latest version of the credentials func (m *Manager) ReadCredentials(ctx context.Context, secretID string, creds any) error { getSecretRequest := secretmanagerpb.AccessSecretVersionRequest{ diff --git a/pkg/credentials/gcp/secretmanager_test.go b/pkg/credentials/gcp/secretmanager_test.go index ad238717a..ce0b2ad34 100644 --- a/pkg/credentials/gcp/secretmanager_test.go +++ b/pkg/credentials/gcp/secretmanager_test.go @@ -1,5 +1,5 @@ // -// Copyright 2023 The Chainloop Authors. +// Copyright 2023-2026 The Chainloop Authors. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -28,6 +28,8 @@ import ( "github.com/go-kratos/kratos/v2/log" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" ) const defaultOrgID = "test-org" @@ -107,6 +109,78 @@ func TestReadCredentials(t *testing.T) { assert.Equal(t, "key", creds.Key) } +func TestSaveCredentialsUpsert(t *testing.T) { + existingSecretName := "my-existing-secret" + + testCases := []struct { + name string + secretName string // non-empty = WithSecretName upsert + gcpNotFound bool // simulate NotFound on GetSecret (secret container absent) + }{ + { + name: "new secret — CreateSecret then AddSecretVersion", + secretName: "", + }, + { + name: "upsert existing — GetSecret succeeds, AddSecretVersion only", + secretName: existingSecretName, + }, + { + name: "upsert not found — CreateSecret then AddSecretVersion", + secretName: existingSecretName, + gcpNotFound: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + m := &Manager{projectID: defaultProjectID} + m.logger = servicelogger.ScopedHelper(log.NewStdLogger(io.Discard), "credentials/gcp-secrets-manager") + clientMock := gcpmocks.NewSecretsManagerInterface(t) + m.client = clientMock + + ociCreds := &credentials.OCIKeypair{Repo: "repo", Username: "user", Password: "password"} + + var saveOpts []credentials.SaveOption + if tc.secretName != "" { + saveOpts = append(saveOpts, credentials.WithSecretName(tc.secretName)) + } + + switch { + case tc.secretName == "": + // New path: CreateSecret returns a secret, then AddSecretVersion. + clientMock.On("CreateSecret", ctx, mock.Anything). + Return(&secretmanagerpb.Secret{Name: "projects/1234-5678-9012/secrets/generated-id"}, nil).Once() + clientMock.On("AddSecretVersion", ctx, mock.Anything). + Return(&secretmanagerpb.SecretVersion{Name: "…/versions/1"}, nil) + case tc.gcpNotFound: + // GetSecret returns not-found → CreateSecret → AddSecretVersion. + notFoundErr := status.Error(codes.NotFound, "secret not found") + clientMock.On("GetSecret", ctx, mock.Anything).Return(nil, notFoundErr).Once() + clientMock.On("CreateSecret", ctx, mock.Anything).Return(&secretmanagerpb.Secret{}, nil).Once() + clientMock.On("AddSecretVersion", ctx, mock.Anything). + Return(&secretmanagerpb.SecretVersion{Name: "…/versions/1"}, nil) + default: + // GetSecret succeeds (secret exists) → AddSecretVersion only. + clientMock.On("GetSecret", ctx, mock.Anything). + Return(&secretmanagerpb.Secret{Name: "projects/1234-5678-9012/secrets/" + tc.secretName}, nil).Once() + clientMock.On("AddSecretVersion", ctx, mock.Anything). + Return(&secretmanagerpb.SecretVersion{Name: "…/versions/2"}, nil) + } + + returned, err := m.SaveCredentials(ctx, defaultOrgID, ociCreds, saveOpts...) + assert.NoError(t, err) + + if tc.secretName != "" { + assert.Equal(t, tc.secretName, returned, "upsert must return the same secret name") + } else { + assert.NotEmpty(t, returned) + } + }) + } +} + func TestDeleteCredentials(t *testing.T) { m := &Manager{} m.logger = servicelogger.ScopedHelper(log.NewStdLogger(io.Discard), "credentials/gcp-secrets-manager") diff --git a/pkg/credentials/mocks/ReaderWriter.go b/pkg/credentials/mocks/ReaderWriter.go index b5c4d1748..3752512aa 100644 --- a/pkg/credentials/mocks/ReaderWriter.go +++ b/pkg/credentials/mocks/ReaderWriter.go @@ -1,10 +1,25 @@ -// Code generated by mockery v2.20.0. DO NOT EDIT. +// Code generated by mockery v3. DO NOT EDIT. + +// Copyright 2026 The Chainloop Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. package mocks import ( context "context" + credentials "github.com/chainloop-dev/chainloop/pkg/credentials" mock "github.com/stretchr/testify/mock" ) @@ -41,23 +56,30 @@ func (_m *ReaderWriter) ReadCredentials(ctx context.Context, secretName string, return r0 } -// SaveCredentials provides a mock function with given fields: ctx, org, _a2 -func (_m *ReaderWriter) SaveCredentials(ctx context.Context, org string, _a2 interface{}) (string, error) { - ret := _m.Called(ctx, org, _a2) +// SaveCredentials provides a mock function with given fields: ctx, org, _a2, opts +func (_m *ReaderWriter) SaveCredentials(ctx context.Context, org string, _a2 interface{}, opts ...credentials.SaveOption) (string, error) { + _va := make([]interface{}, len(opts)) + for _i := range opts { + _va[_i] = opts[_i] + } + var _ca []interface{} + _ca = append(_ca, ctx, org, _a2) + _ca = append(_ca, _va...) + ret := _m.Called(_ca...) var r0 string var r1 error - if rf, ok := ret.Get(0).(func(context.Context, string, interface{}) (string, error)); ok { - return rf(ctx, org, _a2) + if rf, ok := ret.Get(0).(func(context.Context, string, interface{}, ...credentials.SaveOption) (string, error)); ok { + return rf(ctx, org, _a2, opts...) } - if rf, ok := ret.Get(0).(func(context.Context, string, interface{}) string); ok { - r0 = rf(ctx, org, _a2) + if rf, ok := ret.Get(0).(func(context.Context, string, interface{}, ...credentials.SaveOption) string); ok { + r0 = rf(ctx, org, _a2, opts...) } else { r0 = ret.Get(0).(string) } - if rf, ok := ret.Get(1).(func(context.Context, string, interface{}) error); ok { - r1 = rf(ctx, org, _a2) + if rf, ok := ret.Get(1).(func(context.Context, string, interface{}, ...credentials.SaveOption) error); ok { + r1 = rf(ctx, org, _a2, opts...) } else { r1 = ret.Error(1) } @@ -65,13 +87,11 @@ func (_m *ReaderWriter) SaveCredentials(ctx context.Context, org string, _a2 int return r0, r1 } -type mockConstructorTestingTNewReaderWriter interface { +// NewReaderWriter creates a new instance of ReaderWriter. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. +func NewReaderWriter(t interface { mock.TestingT Cleanup(func()) -} - -// NewReaderWriter creates a new instance of ReaderWriter. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. -func NewReaderWriter(t mockConstructorTestingTNewReaderWriter) *ReaderWriter { +}) *ReaderWriter { mock := &ReaderWriter{} mock.Mock.Test(t) diff --git a/pkg/credentials/mocks/Writer.go b/pkg/credentials/mocks/Writer.go index 8ce555c38..f71ac54d2 100644 --- a/pkg/credentials/mocks/Writer.go +++ b/pkg/credentials/mocks/Writer.go @@ -1,10 +1,25 @@ -// Code generated by mockery v2.20.0. DO NOT EDIT. +// Code generated by mockery v3. DO NOT EDIT. + +// Copyright 2026 The Chainloop Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. package mocks import ( context "context" + credentials "github.com/chainloop-dev/chainloop/pkg/credentials" mock "github.com/stretchr/testify/mock" ) @@ -27,23 +42,30 @@ func (_m *Writer) DeleteCredentials(ctx context.Context, credID string) error { return r0 } -// SaveCredentials provides a mock function with given fields: ctx, org, _a2 -func (_m *Writer) SaveCredentials(ctx context.Context, org string, _a2 interface{}) (string, error) { - ret := _m.Called(ctx, org, _a2) +// SaveCredentials provides a mock function with given fields: ctx, org, _a2, opts +func (_m *Writer) SaveCredentials(ctx context.Context, org string, _a2 interface{}, opts ...credentials.SaveOption) (string, error) { + _va := make([]interface{}, len(opts)) + for _i := range opts { + _va[_i] = opts[_i] + } + var _ca []interface{} + _ca = append(_ca, ctx, org, _a2) + _ca = append(_ca, _va...) + ret := _m.Called(_ca...) var r0 string var r1 error - if rf, ok := ret.Get(0).(func(context.Context, string, interface{}) (string, error)); ok { - return rf(ctx, org, _a2) + if rf, ok := ret.Get(0).(func(context.Context, string, interface{}, ...credentials.SaveOption) (string, error)); ok { + return rf(ctx, org, _a2, opts...) } - if rf, ok := ret.Get(0).(func(context.Context, string, interface{}) string); ok { - r0 = rf(ctx, org, _a2) + if rf, ok := ret.Get(0).(func(context.Context, string, interface{}, ...credentials.SaveOption) string); ok { + r0 = rf(ctx, org, _a2, opts...) } else { r0 = ret.Get(0).(string) } - if rf, ok := ret.Get(1).(func(context.Context, string, interface{}) error); ok { - r1 = rf(ctx, org, _a2) + if rf, ok := ret.Get(1).(func(context.Context, string, interface{}, ...credentials.SaveOption) error); ok { + r1 = rf(ctx, org, _a2, opts...) } else { r1 = ret.Error(1) } @@ -51,13 +73,11 @@ func (_m *Writer) SaveCredentials(ctx context.Context, org string, _a2 interface return r0, r1 } -type mockConstructorTestingTNewWriter interface { +// NewWriter creates a new instance of Writer. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. +func NewWriter(t interface { mock.TestingT Cleanup(func()) -} - -// NewWriter creates a new instance of Writer. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. -func NewWriter(t mockConstructorTestingTNewWriter) *Writer { +}) *Writer { mock := &Writer{} mock.Mock.Test(t) diff --git a/pkg/credentials/vault/keyval.go b/pkg/credentials/vault/keyval.go index 64ebf69e2..274463e10 100644 --- a/pkg/credentials/vault/keyval.go +++ b/pkg/credentials/vault/keyval.go @@ -1,5 +1,5 @@ // -// Copyright 2023 The Chainloop Authors. +// Copyright 2023-2026 The Chainloop Authors. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -136,18 +136,22 @@ func validateReaderClient(kv *vault.KVv2, pathPrefix string) error { return nil } -func (m *Manager) SaveCredentials(ctx context.Context, orgID string, creds any) (string, error) { +func (m *Manager) SaveCredentials(ctx context.Context, orgID string, creds any, opts ...credentials.SaveOption) (string, error) { credsM, err := structToMap(creds) if err != nil { return "", fmt.Errorf("converting struct to map: %w", err) } - secretName := strings.Join([]string{m.secretPrefix, orgID, uuid.Generate().String()}, "/") + o := credentials.ApplySaveOptions(opts) + secretName := o.SecretName + if secretName == "" { + secretName = strings.Join([]string{m.secretPrefix, orgID, uuid.Generate().String()}, "/") + } m.logger.Infow("msg", "storing credentials", "path", secretName) _, err = m.client.Put(ctx, secretName, credsM) if err != nil { - return "", fmt.Errorf("creating secret in Vault: %w", err) + return "", fmt.Errorf("storing secret in Vault: %w", err) } return secretName, nil diff --git a/pkg/credentials/vault/keyval_test.go b/pkg/credentials/vault/keyval_test.go index 68e9df095..93c9fa12e 100644 --- a/pkg/credentials/vault/keyval_test.go +++ b/pkg/credentials/vault/keyval_test.go @@ -1,5 +1,5 @@ // -// Copyright 2023-2025 The Chainloop Authors. +// Copyright 2023-2026 The Chainloop Authors. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -129,6 +129,57 @@ func (s *testSuite) TestReadWriteCredentials() { assert.ErrorIs(err, credentials.ErrNotFound) } +// TestSaveCredentialsUpsert verifies that WithSecretName causes SaveCredentials to write +// to the provided path (overwriting if it already exists) instead of generating a new UUID. +func (s *testSuite) TestSaveCredentialsUpsert() { + assert := assert.New(s.T()) + require := require.New(s.T()) + + testCases := []struct { + name string + useExisting bool // whether to pass WithSecretName + expectedSamePath bool + }{ + {"new secret generates unique path", false, false}, + {"upsert at existing path returns same path", true, true}, + } + + opts := &vault.NewManagerOpts{AuthToken: defaultToken, Address: s.connectionString, SecretPrefix: "test-prefix"} + m, err := vault.NewManager(opts) + require.NoError(err) + + // First write to establish the path. + firstCreds := &credentials.OCIKeypair{Repo: "repo1", Username: "user1", Password: "pass1"} + existingPath, err := m.SaveCredentials(context.Background(), orgID, firstCreds) + require.NoError(err) + require.NotEmpty(existingPath) + + for _, tc := range testCases { + s.Run(tc.name, func() { + updatedCreds := &credentials.OCIKeypair{Repo: "repo2", Username: "user2", Password: "pass2"} + + var saveOpts []credentials.SaveOption + if tc.useExisting { + saveOpts = append(saveOpts, credentials.WithSecretName(existingPath)) + } + + returnedPath, err := m.SaveCredentials(context.Background(), orgID, updatedCreds, saveOpts...) + assert.NoError(err) + + if tc.expectedSamePath { + assert.Equal(existingPath, returnedPath, "upsert must return the same path") + + // Verify the credential was updated at the existing path. + got := &credentials.OCIKeypair{} + assert.NoError(m.ReadCredentials(context.Background(), returnedPath, got)) + assert.Equal(updatedCreds, got, "secret value must reflect the updated credentials") + } else { + assert.NotEqual(existingPath, returnedPath, "new write must generate a different path") + } + }) + } +} + // Create a new secret, delete it and check it does not exist antymore func (s *testSuite) TestDeleteCreds() { assert := assert.New(s.T()) From 28919cff774a81a917f98df5a5d301b39e45bfcc Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Sun, 22 Mar 2026 20:11:36 +0100 Subject: [PATCH 2/4] refactor(credentials): variadic ApplySaveOptions and atomic GCP upsert MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Change ApplySaveOptions signature from slice to variadic for idiomatic Go - Replace TOCTOU GetSecret→CreateSecret upsert in GCP with a single CreateSecret that tolerates AlreadyExists, eliminating the race condition - Add isAlreadyExistsErr helper alongside the removed isNotFoundErr - Update all call sites and tests accordingly Signed-off-by: Miguel Martinez Trivino --- app/controlplane/pkg/biz/casbackend_test.go | 2 +- pkg/credentials/aws/secretmanager.go | 4 +- pkg/credentials/azurekv/keyvault.go | 2 +- pkg/credentials/credentials.go | 2 +- pkg/credentials/gcp/secretmanager.go | 66 ++++++++------------- pkg/credentials/gcp/secretmanager_test.go | 14 ++--- pkg/credentials/vault/keyval.go | 2 +- 7 files changed, 37 insertions(+), 55 deletions(-) diff --git a/app/controlplane/pkg/biz/casbackend_test.go b/app/controlplane/pkg/biz/casbackend_test.go index f049d8b04..71786ce7d 100644 --- a/app/controlplane/pkg/biz/casbackend_test.go +++ b/app/controlplane/pkg/biz/casbackend_test.go @@ -314,7 +314,7 @@ func (s *casBackendTestSuite) TestUpdateRotatesCredentialsInPlace() { // args[3] is the first SaveOption when opts is non-empty. if len(args) > 3 { if opt, ok := args.Get(3).(credentials.SaveOption); ok { - o := credentials.ApplySaveOptions([]credentials.SaveOption{opt}) + o := credentials.ApplySaveOptions(opt) capturedSecretName = o.SecretName } } diff --git a/pkg/credentials/aws/secretmanager.go b/pkg/credentials/aws/secretmanager.go index 022a6cb73..d7bf4171e 100644 --- a/pkg/credentials/aws/secretmanager.go +++ b/pkg/credentials/aws/secretmanager.go @@ -1,5 +1,5 @@ // -// Copyright 2023-2025 The Chainloop Authors. +// Copyright 2023-2026 The Chainloop Authors. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -83,7 +83,7 @@ func NewManager(opts *NewManagerOpts) (*Manager, error) { // SaveCredentials saves credentials. If opts includes WithSecretName, upserts at the given path. func (m *Manager) SaveCredentials(ctx context.Context, orgID string, creds any, opts ...credentials.SaveOption) (string, error) { - o := credentials.ApplySaveOptions(opts) + o := credentials.ApplySaveOptions(opts...) secretName := o.SecretName if secretName == "" { secretName = strings.Join([]string{m.secretPrefix, orgID, uuid.New().String()}, "/") diff --git a/pkg/credentials/azurekv/keyvault.go b/pkg/credentials/azurekv/keyvault.go index 2348923ec..972b49fdc 100644 --- a/pkg/credentials/azurekv/keyvault.go +++ b/pkg/credentials/azurekv/keyvault.go @@ -122,7 +122,7 @@ func NewManager(opts *NewManagerOpts) (*Manager, error) { // SaveCredentials saves credentials. If opts includes WithSecretName, upserts at the given path. func (m *Manager) SaveCredentials(ctx context.Context, orgID string, creds any, opts ...credentials.SaveOption) (string, error) { - o := credentials.ApplySaveOptions(opts) + o := credentials.ApplySaveOptions(opts...) secretName := o.SecretName if secretName == "" { secretName = strings.Join([]string{m.secretPrefix, orgID, uuid.New().String()}, "-") diff --git a/pkg/credentials/credentials.go b/pkg/credentials/credentials.go index a62ee615d..e86345d1f 100644 --- a/pkg/credentials/credentials.go +++ b/pkg/credentials/credentials.go @@ -51,7 +51,7 @@ func WithSecretName(name string) SaveOption { } // ApplySaveOptions applies the given options and returns the resulting SaveOpts. -func ApplySaveOptions(opts []SaveOption) SaveOpts { +func ApplySaveOptions(opts ...SaveOption) SaveOpts { o := SaveOpts{} for _, opt := range opts { opt(&o) diff --git a/pkg/credentials/gcp/secretmanager.go b/pkg/credentials/gcp/secretmanager.go index 6469520ce..f0f9fc130 100644 --- a/pkg/credentials/gcp/secretmanager.go +++ b/pkg/credentials/gcp/secretmanager.go @@ -93,54 +93,38 @@ func (m *Manager) SaveCredentials(ctx context.Context, orgID string, creds any, return "", fmt.Errorf("marshaling credentials to be stored: %w", err) } - o := credentials.ApplySaveOptions(opts) + o := credentials.ApplySaveOptions(opts...) secretID := o.SecretName if secretID == "" { secretID = strings.Join([]string{m.secretPrefix, orgID, uuid.Generate().String()}, "-") } - parent := fmt.Sprintf("projects/%s/secrets/%s", m.projectID, secretID) + secretReq := &secretmanagerpb.CreateSecretRequest{ + Parent: fmt.Sprintf("projects/%s", m.projectID), + SecretId: secretID, + Secret: &secretmanagerpb.Secret{ + Replication: &secretmanagerpb.Replication{ + Replication: &secretmanagerpb.Replication_Automatic_{ + Automatic: &secretmanagerpb.Replication_Automatic{}, + }, + }, + }, + } + var parent string if o.SecretName != "" { - // Upsert path: check if secret container exists - _, err = m.client.GetSecret(ctx, &secretmanagerpb.GetSecretRequest{Name: parent}) - if isNotFoundErr(err) { - // Secret does not exist, create the container first - _, err = m.client.CreateSecret(ctx, &secretmanagerpb.CreateSecretRequest{ - Parent: fmt.Sprintf("projects/%s", m.projectID), - SecretId: secretID, - Secret: &secretmanagerpb.Secret{ - Replication: &secretmanagerpb.Replication{ - Replication: &secretmanagerpb.Replication_Automatic_{ - Automatic: &secretmanagerpb.Replication_Automatic{}, - }, - }, - }, - }) - if err != nil { - return "", fmt.Errorf("creating secret in GCP: %w", err) - } - m.logger.Infow("msg", "created new secret for upsert", "secretID", secretID) - } else if err != nil { - return "", fmt.Errorf("checking secret existence in GCP: %w", err) + // Upsert: attempt to create the container; AlreadyExists means it's already there. + parent = fmt.Sprintf("projects/%s/secrets/%s", m.projectID, secretID) + _, err = m.client.CreateSecret(ctx, secretReq) + if err != nil && !isAlreadyExistsErr(err) { + return "", fmt.Errorf("creating secret in GCP: %w", err) } } else { - // New secret path: create the container - secret, err := m.client.CreateSecret(ctx, &secretmanagerpb.CreateSecretRequest{ - Parent: fmt.Sprintf("projects/%s", m.projectID), - SecretId: secretID, - Secret: &secretmanagerpb.Secret{ - Replication: &secretmanagerpb.Replication{ - Replication: &secretmanagerpb.Replication_Automatic_{ - Automatic: &secretmanagerpb.Replication_Automatic{}, - }, - }, - }, - }) - if err != nil { - return "", fmt.Errorf("creating secret in GCP: %w", err) + // New secret: create the container and use the canonical name from the API response. + secret, createErr := m.client.CreateSecret(ctx, secretReq) + if createErr != nil { + return "", fmt.Errorf("creating secret in GCP: %w", createErr) } - // Use the fully-qualified name returned by CreateSecret parent = secret.Name m.logger.Infow("msg", "created new secret", "secretID", secretID) } @@ -158,10 +142,10 @@ func (m *Manager) SaveCredentials(ctx context.Context, orgID string, creds any, return secretID, nil } -// isNotFoundErr returns true when the error is a gRPC NotFound status. -func isNotFoundErr(err error) bool { +// isAlreadyExistsErr returns true when the error is a gRPC AlreadyExists status. +func isAlreadyExistsErr(err error) bool { s, ok := status.FromError(err) - return ok && s.Code() == codes.NotFound + return ok && s.Code() == codes.AlreadyExists } // ReadCredentials reads the latest version of the credentials diff --git a/pkg/credentials/gcp/secretmanager_test.go b/pkg/credentials/gcp/secretmanager_test.go index ce0b2ad34..672984332 100644 --- a/pkg/credentials/gcp/secretmanager_test.go +++ b/pkg/credentials/gcp/secretmanager_test.go @@ -115,14 +115,14 @@ func TestSaveCredentialsUpsert(t *testing.T) { testCases := []struct { name string secretName string // non-empty = WithSecretName upsert - gcpNotFound bool // simulate NotFound on GetSecret (secret container absent) + gcpNotFound bool // simulate secret container absent in GCP }{ { name: "new secret — CreateSecret then AddSecretVersion", secretName: "", }, { - name: "upsert existing — GetSecret succeeds, AddSecretVersion only", + name: "upsert existing — CreateSecret AlreadyExists, AddSecretVersion only", secretName: existingSecretName, }, { @@ -155,16 +155,14 @@ func TestSaveCredentialsUpsert(t *testing.T) { clientMock.On("AddSecretVersion", ctx, mock.Anything). Return(&secretmanagerpb.SecretVersion{Name: "…/versions/1"}, nil) case tc.gcpNotFound: - // GetSecret returns not-found → CreateSecret → AddSecretVersion. - notFoundErr := status.Error(codes.NotFound, "secret not found") - clientMock.On("GetSecret", ctx, mock.Anything).Return(nil, notFoundErr).Once() + // Upsert: secret container absent → CreateSecret succeeds → AddSecretVersion. clientMock.On("CreateSecret", ctx, mock.Anything).Return(&secretmanagerpb.Secret{}, nil).Once() clientMock.On("AddSecretVersion", ctx, mock.Anything). Return(&secretmanagerpb.SecretVersion{Name: "…/versions/1"}, nil) default: - // GetSecret succeeds (secret exists) → AddSecretVersion only. - clientMock.On("GetSecret", ctx, mock.Anything). - Return(&secretmanagerpb.Secret{Name: "projects/1234-5678-9012/secrets/" + tc.secretName}, nil).Once() + // Upsert: secret container exists → CreateSecret returns AlreadyExists → AddSecretVersion only. + alreadyExistsErr := status.Error(codes.AlreadyExists, "secret already exists") + clientMock.On("CreateSecret", ctx, mock.Anything).Return(nil, alreadyExistsErr).Once() clientMock.On("AddSecretVersion", ctx, mock.Anything). Return(&secretmanagerpb.SecretVersion{Name: "…/versions/2"}, nil) } diff --git a/pkg/credentials/vault/keyval.go b/pkg/credentials/vault/keyval.go index 274463e10..6321f4940 100644 --- a/pkg/credentials/vault/keyval.go +++ b/pkg/credentials/vault/keyval.go @@ -142,7 +142,7 @@ func (m *Manager) SaveCredentials(ctx context.Context, orgID string, creds any, return "", fmt.Errorf("converting struct to map: %w", err) } - o := credentials.ApplySaveOptions(opts) + o := credentials.ApplySaveOptions(opts...) secretName := o.SecretName if secretName == "" { secretName = strings.Join([]string{m.secretPrefix, orgID, uuid.Generate().String()}, "/") From d60d45c640d1c203449489177e7477eac507692c Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Sun, 22 Mar 2026 21:46:27 +0100 Subject: [PATCH 3/4] fix(test): pass mock.Anything for variadic opts in CAS backend integration test The SaveCredentials integration test expectation was registered with only 3 args. After the upsert change, Update() passes WithSecretName as a 4th variadic arg, causing the mock not to match. Add mock.Anything to cover the opts argument. Signed-off-by: Miguel Martinez Trivino --- app/controlplane/pkg/biz/casbackend_integration_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/controlplane/pkg/biz/casbackend_integration_test.go b/app/controlplane/pkg/biz/casbackend_integration_test.go index cc2f86a9f..281537e06 100644 --- a/app/controlplane/pkg/biz/casbackend_integration_test.go +++ b/app/controlplane/pkg/biz/casbackend_integration_test.go @@ -1,5 +1,5 @@ // -// Copyright 2024-2025 The Chainloop Authors. +// Copyright 2024-2026 The Chainloop Authors. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -410,7 +410,7 @@ func (s *CASBackendIntegrationTestSuite) TestUpdate() { creds := struct{}{} ctx := context.TODO() s.credsWriter.Mock = mock.Mock{} - s.credsWriter.On("SaveCredentials", ctx, s.orgNoBackend.ID, creds).Return("new-secret", nil) + s.credsWriter.On("SaveCredentials", ctx, s.orgNoBackend.ID, creds, mock.Anything).Return("new-secret", nil) s.credsWriter.On("ReadCredentials", ctx, "new-secret", mock.Anything).Return(nil) s.backendProvider.On("ValidateAndExtractCredentials", location, mock.Anything).Return(nil, nil) defaultB, err = s.CASBackend.Update(ctx, s.orgNoBackend.ID, defaultB.ID.String(), toPtrS(description), creds, nil, nil, nil) From 255d9495858ace18e7c30506a62f56df06a2f0ee Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Sun, 22 Mar 2026 22:30:25 +0100 Subject: [PATCH 4/4] fix(credentials): address PR review feedback - Rename WithSecretName to WithExistingSecret for clarity - Add nil guard in ApplySaveOptions to prevent runtime panic - GCP upsert: try AddSecretVersion first to avoid requiring secretmanager.secrets.create permission for existing secrets - Improve test assertion for empty-secret case to check opt presence - Add mockery v3 config for credentials package Signed-off-by: Miguel Martinez Trivino --- app/controlplane/pkg/biz/casbackend.go | 2 +- app/controlplane/pkg/biz/casbackend_test.go | 38 ++++++++++---------- pkg/credentials/.mockery.yml | 26 ++++++++++++++ pkg/credentials/aws/secretmanager.go | 2 +- pkg/credentials/aws/secretmanager_test.go | 4 +-- pkg/credentials/azurekv/keyvault.go | 2 +- pkg/credentials/azurekv/keyvault_test.go | 4 +-- pkg/credentials/credentials.go | 12 ++++--- pkg/credentials/gcp/secretmanager.go | 26 ++++++++++---- pkg/credentials/gcp/secretmanager_test.go | 40 +++++++++++---------- pkg/credentials/vault/keyval_test.go | 6 ++-- 11 files changed, 103 insertions(+), 59 deletions(-) create mode 100644 pkg/credentials/.mockery.yml diff --git a/app/controlplane/pkg/biz/casbackend.go b/app/controlplane/pkg/biz/casbackend.go index 0bc8066d4..e959444a2 100644 --- a/app/controlplane/pkg/biz/casbackend.go +++ b/app/controlplane/pkg/biz/casbackend.go @@ -431,7 +431,7 @@ func (uc *CASBackendUseCase) Update(ctx context.Context, orgID, id string, descr if creds != nil { var saveOpts []credentials.SaveOption if before.SecretName != "" { - saveOpts = append(saveOpts, credentials.WithSecretName(before.SecretName)) + saveOpts = append(saveOpts, credentials.WithExistingSecret(before.SecretName)) } secretName, err = uc.credsRW.SaveCredentials(ctx, orgID, creds, saveOpts...) if err != nil { diff --git a/app/controlplane/pkg/biz/casbackend_test.go b/app/controlplane/pkg/biz/casbackend_test.go index 71786ce7d..ef8f15030 100644 --- a/app/controlplane/pkg/biz/casbackend_test.go +++ b/app/controlplane/pkg/biz/casbackend_test.go @@ -265,7 +265,7 @@ func (s *casBackendTestSuite) TestNewCASBackendUseCase() { } // TestUpdateRotatesCredentialsInPlace verifies that Update() passes the existing SecretName -// as a WithSecretName option so the credential store upserts in-place instead of +// as a WithExistingSecret option so the credential store upserts in-place instead of // creating a new entry. func (s *casBackendTestSuite) TestUpdateRotatesCredentialsInPlace() { ctx := context.Background() @@ -274,22 +274,22 @@ func (s *casBackendTestSuite) TestUpdateRotatesCredentialsInPlace() { newCreds := &credentials.OCIKeypair{Repo: "r", Username: "u", Password: "p"} tests := []struct { - name string - existingSecret string - wantWithSecretName bool // whether the SaveOption should carry the existing secret name - returnedSecretName string // what SaveCredentials mock returns + name string + existingSecret string + wantWithExistingSecret bool // whether the SaveOption should carry the existing secret name + returnedSecretName string // what SaveCredentials mock returns }{ { - name: "existing secret name is forwarded as WithSecretName", - existingSecret: existingSecretName, - wantWithSecretName: true, - returnedSecretName: existingSecretName, + name: "existing secret name is forwarded as WithExistingSecret", + existingSecret: existingSecretName, + wantWithExistingSecret: true, + returnedSecretName: existingSecretName, }, { - name: "empty secret name generates a new path", - existingSecret: "", - wantWithSecretName: false, - returnedSecretName: "new-secret-path", + name: "empty secret name generates a new path", + existingSecret: "", + wantWithExistingSecret: false, + returnedSecretName: "new-secret-path", }, } @@ -308,11 +308,12 @@ func (s *casBackendTestSuite) TestUpdateRotatesCredentialsInPlace() { // Step 2: SaveCredentials — capture SaveOption(s) to verify the secret name. var capturedSecretName string + var optWasPassed bool saveMatcher := mock.MatchedBy(func(_ interface{}) bool { return true }) s.credsRW.On("SaveCredentials", ctx, s.validUUID.String(), saveMatcher, mock.Anything). Run(func(args mock.Arguments) { - // args[3] is the first SaveOption when opts is non-empty. if len(args) > 3 { + optWasPassed = true if opt, ok := args.Get(3).(credentials.SaveOption); ok { o := credentials.ApplySaveOptions(opt) capturedSecretName = o.SecretName @@ -320,7 +321,7 @@ func (s *casBackendTestSuite) TestUpdateRotatesCredentialsInPlace() { } }).Return(tc.returnedSecretName, nil).Maybe() - // Fallback for no-opts case (empty existing secret → no WithSecretName). + // Fallback for no-opts case (empty existing secret → no WithExistingSecret). s.credsRW.On("SaveCredentials", ctx, s.validUUID.String(), saveMatcher). Return(tc.returnedSecretName, nil).Maybe() @@ -341,11 +342,12 @@ func (s *casBackendTestSuite) TestUpdateRotatesCredentialsInPlace() { s.Require().NoError(err) s.Equal(tc.returnedSecretName, got.SecretName) - if tc.wantWithSecretName { + if tc.wantWithExistingSecret { + s.True(optWasPassed, "expected SaveOption to be passed to SaveCredentials") s.Equal(existingSecretName, capturedSecretName, - "expected WithSecretName(%q) to be forwarded to SaveCredentials", existingSecretName) + "expected WithExistingSecret(%q) to be forwarded to SaveCredentials", existingSecretName) } else { - s.Empty(capturedSecretName, "expected no WithSecretName when existingSecret is empty") + s.False(optWasPassed, "expected no SaveOption to be passed to SaveCredentials") } }) } diff --git a/pkg/credentials/.mockery.yml b/pkg/credentials/.mockery.yml new file mode 100644 index 000000000..f542504e6 --- /dev/null +++ b/pkg/credentials/.mockery.yml @@ -0,0 +1,26 @@ +all: false +formatter: goimports +include-auto-generated: false +log-level: info +recursive: false +require-template-schema-exists: true +template: testify +template-schema: "{{.Template}}.schema.json" +packages: + github.com/chainloop-dev/chainloop/pkg/credentials: + config: + dir: "{{.InterfaceDir}}/mocks" + filename: "{{.InterfaceName}}.go" + pkgname: mocks + structname: "{{.InterfaceName}}" + interfaces: + ReaderWriter: + Writer: + github.com/chainloop-dev/chainloop/pkg/credentials/aws: + config: + dir: "{{.InterfaceDir}}/mocks" + filename: "{{.InterfaceName}}.go" + pkgname: mocks + structname: "{{.InterfaceName}}" + interfaces: + SecretsManagerIface: diff --git a/pkg/credentials/aws/secretmanager.go b/pkg/credentials/aws/secretmanager.go index d7bf4171e..6e0f6f6a4 100644 --- a/pkg/credentials/aws/secretmanager.go +++ b/pkg/credentials/aws/secretmanager.go @@ -81,7 +81,7 @@ func NewManager(opts *NewManagerOpts) (*Manager, error) { }, nil } -// SaveCredentials saves credentials. If opts includes WithSecretName, upserts at the given path. +// SaveCredentials saves credentials. If opts includes WithExistingSecret, upserts at the given path. func (m *Manager) SaveCredentials(ctx context.Context, orgID string, creds any, opts ...credentials.SaveOption) (string, error) { o := credentials.ApplySaveOptions(opts...) secretName := o.SecretName diff --git a/pkg/credentials/aws/secretmanager_test.go b/pkg/credentials/aws/secretmanager_test.go index afe05c402..b5770aef4 100644 --- a/pkg/credentials/aws/secretmanager_test.go +++ b/pkg/credentials/aws/secretmanager_test.go @@ -210,7 +210,7 @@ func (s *testSuite) TestSaveCredentialsUpsert() { testCases := []struct { name string - secretName string // non-empty = WithSecretName upsert + secretName string // non-empty = WithExistingSecret upsert awsNotFound bool // simulate ResourceNotFoundException on PutSecretValue expectedError bool }{ @@ -238,7 +238,7 @@ func (s *testSuite) TestSaveCredentialsUpsert() { var saveOpts []credentials.SaveOption if tc.secretName != "" { - saveOpts = append(saveOpts, credentials.WithSecretName(tc.secretName)) + saveOpts = append(saveOpts, credentials.WithExistingSecret(tc.secretName)) } switch { diff --git a/pkg/credentials/azurekv/keyvault.go b/pkg/credentials/azurekv/keyvault.go index 972b49fdc..6d7a1a8c3 100644 --- a/pkg/credentials/azurekv/keyvault.go +++ b/pkg/credentials/azurekv/keyvault.go @@ -120,7 +120,7 @@ func NewManager(opts *NewManagerOpts) (*Manager, error) { }, nil } -// SaveCredentials saves credentials. If opts includes WithSecretName, upserts at the given path. +// SaveCredentials saves credentials. If opts includes WithExistingSecret, upserts at the given path. func (m *Manager) SaveCredentials(ctx context.Context, orgID string, creds any, opts ...credentials.SaveOption) (string, error) { o := credentials.ApplySaveOptions(opts...) secretName := o.SecretName diff --git a/pkg/credentials/azurekv/keyvault_test.go b/pkg/credentials/azurekv/keyvault_test.go index b5d4020fd..27494a16c 100644 --- a/pkg/credentials/azurekv/keyvault_test.go +++ b/pkg/credentials/azurekv/keyvault_test.go @@ -165,7 +165,7 @@ func (s *testSuite) TestSaveCredentialsUpsert() { testCases := []struct { name string - secretName string // non-empty = WithSecretName upsert + secretName string // non-empty = WithExistingSecret upsert }{ { name: "new secret — auto-generated name", @@ -193,7 +193,7 @@ func (s *testSuite) TestSaveCredentialsUpsert() { var saveOpts []credentials.SaveOption if tc.secretName != "" { - saveOpts = append(saveOpts, credentials.WithSecretName(tc.secretName)) + saveOpts = append(saveOpts, credentials.WithExistingSecret(tc.secretName)) } returned, err := m.SaveCredentials(ctx, "my-org", creds, saveOpts...) diff --git a/pkg/credentials/credentials.go b/pkg/credentials/credentials.go index e86345d1f..26ec8ef1a 100644 --- a/pkg/credentials/credentials.go +++ b/pkg/credentials/credentials.go @@ -34,7 +34,7 @@ type ReaderWriter interface { Writer } -// SaveOpts holds optional parameters for SaveCredentials. Exported for use by sub-packages. +// SaveOpts holds optional parameters for SaveCredentials. type SaveOpts struct { // SecretName when non-empty causes SaveCredentials to upsert at the given path // instead of generating a new UUID-based path. @@ -44,9 +44,9 @@ type SaveOpts struct { // SaveOption is a functional option for SaveCredentials. type SaveOption func(*SaveOpts) -// WithSecretName instructs SaveCredentials to upsert at the provided path. -// An empty name is a no-op (auto-generate behavior is preserved). -func WithSecretName(name string) SaveOption { +// WithExistingSecret instructs SaveCredentials to upsert at the provided existing +// secret path. An empty name is a no-op (auto-generate behavior is preserved). +func WithExistingSecret(name string) SaveOption { return func(o *SaveOpts) { o.SecretName = name } } @@ -54,7 +54,9 @@ func WithSecretName(name string) SaveOption { func ApplySaveOptions(opts ...SaveOption) SaveOpts { o := SaveOpts{} for _, opt := range opts { - opt(&o) + if opt != nil { + opt(&o) + } } return o } diff --git a/pkg/credentials/gcp/secretmanager.go b/pkg/credentials/gcp/secretmanager.go index f0f9fc130..b0c9cc784 100644 --- a/pkg/credentials/gcp/secretmanager.go +++ b/pkg/credentials/gcp/secretmanager.go @@ -85,7 +85,7 @@ func NewManager(opts *NewManagerOpts) (*Manager, error) { }, nil } -// SaveCredentials saves credentials. If opts includes WithSecretName, upserts at the given path. +// SaveCredentials saves credentials. If opts includes WithExistingSecret, upserts at the given path. func (m *Manager) SaveCredentials(ctx context.Context, orgID string, creds any, opts ...credentials.SaveOption) (string, error) { // store creds in key-value pair c, err := json.Marshal(creds) @@ -113,12 +113,24 @@ func (m *Manager) SaveCredentials(ctx context.Context, orgID string, creds any, var parent string if o.SecretName != "" { - // Upsert: attempt to create the container; AlreadyExists means it's already there. + // Upsert: try adding a new version first (only needs secretmanager.versions.add). parent = fmt.Sprintf("projects/%s/secrets/%s", m.projectID, secretID) - _, err = m.client.CreateSecret(ctx, secretReq) - if err != nil && !isAlreadyExistsErr(err) { + v, addErr := m.client.AddSecretVersion(ctx, &secretmanagerpb.AddSecretVersionRequest{ + Parent: parent, + Payload: &secretmanagerpb.SecretPayload{Data: c}, + }) + if addErr == nil { + m.logger.Infow("msg", "added new secret version", "secretID", secretID, "versionID", v.Name) + return secretID, nil + } + if !isNotFoundErr(addErr) { + return "", fmt.Errorf("adding secret version in GCP: %w", addErr) + } + // Secret container doesn't exist yet — create it and fall through to AddSecretVersion below. + if _, err = m.client.CreateSecret(ctx, secretReq); err != nil { return "", fmt.Errorf("creating secret in GCP: %w", err) } + m.logger.Infow("msg", "created new secret", "secretID", secretID) } else { // New secret: create the container and use the canonical name from the API response. secret, createErr := m.client.CreateSecret(ctx, secretReq) @@ -142,10 +154,10 @@ func (m *Manager) SaveCredentials(ctx context.Context, orgID string, creds any, return secretID, nil } -// isAlreadyExistsErr returns true when the error is a gRPC AlreadyExists status. -func isAlreadyExistsErr(err error) bool { +// isNotFoundErr returns true when the error is a gRPC NotFound status. +func isNotFoundErr(err error) bool { s, ok := status.FromError(err) - return ok && s.Code() == codes.AlreadyExists + return ok && s.Code() == codes.NotFound } // ReadCredentials reads the latest version of the credentials diff --git a/pkg/credentials/gcp/secretmanager_test.go b/pkg/credentials/gcp/secretmanager_test.go index 672984332..0fe9df4aa 100644 --- a/pkg/credentials/gcp/secretmanager_test.go +++ b/pkg/credentials/gcp/secretmanager_test.go @@ -76,8 +76,8 @@ func TestCreateCredentials(t *testing.T) { clientMock := gcpmocks.NewSecretsManagerInterface(t) m.client = clientMock - clientMock.On("CreateSecret", ctx, mock.Anything).Return(&secretmanagerpb.Secret{}, nil).Once() - clientMock.On("AddSecretVersion", ctx, mock.Anything).Return(&secretmanagerpb.SecretVersion{}, nil) + clientMock.On("CreateSecret", ctx, mock.Anything, mock.Anything).Return(&secretmanagerpb.Secret{}, nil).Once() + clientMock.On("AddSecretVersion", ctx, mock.Anything, mock.Anything).Return(&secretmanagerpb.SecretVersion{}, nil) _, err = m.SaveCredentials(ctx, defaultOrgID, creds) assert.NoError(t, err) @@ -114,19 +114,19 @@ func TestSaveCredentialsUpsert(t *testing.T) { testCases := []struct { name string - secretName string // non-empty = WithSecretName upsert + secretName string // non-empty = WithExistingSecret upsert gcpNotFound bool // simulate secret container absent in GCP }{ { - name: "new secret — CreateSecret then AddSecretVersion", + name: "new secret - CreateSecret then AddSecretVersion", secretName: "", }, { - name: "upsert existing — CreateSecret AlreadyExists, AddSecretVersion only", + name: "upsert existing - AddSecretVersion succeeds directly", secretName: existingSecretName, }, { - name: "upsert not found — CreateSecret then AddSecretVersion", + name: "upsert not found - AddSecretVersion returns NotFound then CreateSecret + AddSecretVersion", secretName: existingSecretName, gcpNotFound: true, }, @@ -144,27 +144,29 @@ func TestSaveCredentialsUpsert(t *testing.T) { var saveOpts []credentials.SaveOption if tc.secretName != "" { - saveOpts = append(saveOpts, credentials.WithSecretName(tc.secretName)) + saveOpts = append(saveOpts, credentials.WithExistingSecret(tc.secretName)) } switch { case tc.secretName == "": // New path: CreateSecret returns a secret, then AddSecretVersion. - clientMock.On("CreateSecret", ctx, mock.Anything). + clientMock.On("CreateSecret", ctx, mock.Anything, mock.Anything). Return(&secretmanagerpb.Secret{Name: "projects/1234-5678-9012/secrets/generated-id"}, nil).Once() - clientMock.On("AddSecretVersion", ctx, mock.Anything). - Return(&secretmanagerpb.SecretVersion{Name: "…/versions/1"}, nil) + clientMock.On("AddSecretVersion", ctx, mock.Anything, mock.Anything). + Return(&secretmanagerpb.SecretVersion{Name: "v1"}, nil).Once() case tc.gcpNotFound: - // Upsert: secret container absent → CreateSecret succeeds → AddSecretVersion. - clientMock.On("CreateSecret", ctx, mock.Anything).Return(&secretmanagerpb.Secret{}, nil).Once() - clientMock.On("AddSecretVersion", ctx, mock.Anything). - Return(&secretmanagerpb.SecretVersion{Name: "…/versions/1"}, nil) + // Upsert: AddSecretVersion returns NotFound, then CreateSecret, then AddSecretVersion again. + notFoundErr := status.Error(codes.NotFound, "secret not found") + clientMock.On("AddSecretVersion", ctx, mock.Anything, mock.Anything). + Return(nil, notFoundErr).Once() + clientMock.On("CreateSecret", ctx, mock.Anything, mock.Anything). + Return(&secretmanagerpb.Secret{}, nil).Once() + clientMock.On("AddSecretVersion", ctx, mock.Anything, mock.Anything). + Return(&secretmanagerpb.SecretVersion{Name: "v1"}, nil).Once() default: - // Upsert: secret container exists → CreateSecret returns AlreadyExists → AddSecretVersion only. - alreadyExistsErr := status.Error(codes.AlreadyExists, "secret already exists") - clientMock.On("CreateSecret", ctx, mock.Anything).Return(nil, alreadyExistsErr).Once() - clientMock.On("AddSecretVersion", ctx, mock.Anything). - Return(&secretmanagerpb.SecretVersion{Name: "…/versions/2"}, nil) + // Upsert: AddSecretVersion succeeds directly (no CreateSecret call). + clientMock.On("AddSecretVersion", ctx, mock.Anything, mock.Anything). + Return(&secretmanagerpb.SecretVersion{Name: "v2"}, nil).Once() } returned, err := m.SaveCredentials(ctx, defaultOrgID, ociCreds, saveOpts...) diff --git a/pkg/credentials/vault/keyval_test.go b/pkg/credentials/vault/keyval_test.go index 93c9fa12e..6ddb06321 100644 --- a/pkg/credentials/vault/keyval_test.go +++ b/pkg/credentials/vault/keyval_test.go @@ -129,7 +129,7 @@ func (s *testSuite) TestReadWriteCredentials() { assert.ErrorIs(err, credentials.ErrNotFound) } -// TestSaveCredentialsUpsert verifies that WithSecretName causes SaveCredentials to write +// TestSaveCredentialsUpsert verifies that WithExistingSecret causes SaveCredentials to write // to the provided path (overwriting if it already exists) instead of generating a new UUID. func (s *testSuite) TestSaveCredentialsUpsert() { assert := assert.New(s.T()) @@ -137,7 +137,7 @@ func (s *testSuite) TestSaveCredentialsUpsert() { testCases := []struct { name string - useExisting bool // whether to pass WithSecretName + useExisting bool // whether to pass WithExistingSecret expectedSamePath bool }{ {"new secret generates unique path", false, false}, @@ -160,7 +160,7 @@ func (s *testSuite) TestSaveCredentialsUpsert() { var saveOpts []credentials.SaveOption if tc.useExisting { - saveOpts = append(saveOpts, credentials.WithSecretName(existingPath)) + saveOpts = append(saveOpts, credentials.WithExistingSecret(existingPath)) } returnedPath, err := m.SaveCredentials(context.Background(), orgID, updatedCreds, saveOpts...)