Skip to content

draft pr for discussion #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
ca-scribner wants to merge 2 commits into
base: 3.6
Choose a base branch
from
Draft

draft pr for discussion #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c69505e
refactor: update RemoveSecretsForAgent to reduce unnecessary api call…
ca-scribner Feb 27, 2025
dc54e98
feat: rework RemoveSecretsForAgent to handle missing secrets, fix oth…
ca-scribner Feb 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
249 changes: 156 additions & 93 deletions apiserver/common/secrets/secrets.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -609,67 +609,55 @@ func RemoveSecretsForAgent(
modelUUID string,
canDelete func(*coresecrets.URI) error,
) (params.ErrorResults, error) {
return removeSecrets(
removeState, adminConfigGetter, args,
modelUUID,
canDelete,
func(provider.SecretBackendProvider, provider.ModelBackendConfig, provider.SecretRevisions) error {
return nil
},
)
}
result := params.ErrorResults{
Results: make([]params.ErrorResult, len(args.Args)),
}

// RemoveUserSecrets removes the specified user supplied secrets.
// The secrets are removed from the state and backend, and the caller must have model admin access.
func RemoveUserSecrets(
removeState SecretsRemoveState, adminConfigGetter BackendAdminConfigGetter,
authTag names.Tag, args params.DeleteSecretArgs,
modelUUID string,
canDelete func(*coresecrets.URI) error,
) (params.ErrorResults, error) {
return removeSecrets(
removeState, adminConfigGetter, args, modelUUID, canDelete,
func(p provider.SecretBackendProvider, cfg provider.ModelBackendConfig, revs provider.SecretRevisions) error {
backend, err := p.NewBackend(&cfg)
if err != nil {
return errors.Trace(err)
}
for _, revId := range revs.RevisionIDs() {
if err = backend.DeleteContent(context.TODO(), revId); err != nil {
return errors.Trace(err)
}
}
if err := p.CleanupSecrets(&cfg, authTag, revs); err != nil {
return errors.Trace(err)
}
return nil
},
)
for i, arg := range args.Args {
uri, err := parseDeleteSecretArg(arg, removeState, modelUUID)
if err != nil {
result.Results[i].Error = apiservererrors.ServerError(err)
continue
}
if _, err := removeState.GetSecret(uri); err != nil {
// Check if the uri exists or not.
result.Results[i].Error = apiservererrors.ServerError(err)
continue
}
if err := canDelete(uri); err != nil {
result.Results[i].Error = apiservererrors.ServerError(err)
continue
}
if _, err = removeState.DeleteSecret(uri, arg.Revisions...); err != nil {
result.Results[i].Error = apiservererrors.ServerError(err)
continue
}
// TODO: Did I miss a cleanup from before?
}
return result, nil
}

func getSecretURIForLabel(secretsState ListSecretsState, modelUUID string, label string) (*coresecrets.URI, error) {
results, err := secretsState.ListSecrets(state.SecretsFilter{
Label: &label,
OwnerTags: []names.Tag{names.NewModelTag(modelUUID)},
})
if err != nil {
return nil, errors.Trace(err)
// parseDeleteSecretArg parses arguments for secret deletion, returning the secret URI
func parseDeleteSecretArg(arg params.DeleteSecretArg, removeState SecretsRemoveState, modelUUID string) (*coresecrets.URI, error) {
if arg.URI == "" && arg.Label == "" {
return nil, errors.New("must specify either URI or label")
}
if len(results) == 0 {
return nil, errors.NotFoundf("secret %q", label)
if arg.URI != "" {
return coresecrets.ParseURI(arg.URI)
}
if len(results) > 1 {
return nil, errors.NotFoundf("more than 1 secret with label %q", label)
if arg.Label != "" {
return getSecretURIForLabel(removeState, modelUUID, arg.Label)
}
return results[0].URI, nil
return nil, errors.New("must specify either URI or label")
}

func removeSecrets(
// RemoveUserSecrets removes the specified user supplied secrets.
// The secrets are removed from the state and backend, and the caller must have model admin access.
func RemoveUserSecrets(
removeState SecretsRemoveState, adminConfigGetter BackendAdminConfigGetter,
args params.DeleteSecretArgs,
authTag names.Tag, args params.DeleteSecretArgs,
modelUUID string,
canDelete func(*coresecrets.URI) error,
removeFromBackend func(provider.SecretBackendProvider, provider.ModelBackendConfig, provider.SecretRevisions) error,
) (params.ErrorResults, error) {
result := params.ErrorResults{
Results: make([]params.ErrorResult, len(args.Args)),
Expand All @@ -683,68 +671,120 @@ func removeSecrets(
return result, errors.Trace(err)
}

removeFromExternal := func(uri *coresecrets.URI, revisions ...int) error {
externalRevs := make(map[string]provider.SecretRevisions)
gatherExternalRevs := func(valRef *coresecrets.ValueRef) {
if valRef == nil {
// Internal secret, nothing to do here.
return
}
if _, ok := externalRevs[valRef.BackendID]; !ok {
externalRevs[valRef.BackendID] = provider.SecretRevisions{}
}
externalRevs[valRef.BackendID].Add(uri, valRef.RevisionID)
}
removeFromExternal := func(uri *coresecrets.URI, revisions ...int) ([]int, []error) {
// TODO: Is this the right way to prototype this variable?
var revs []*coresecrets.SecretRevisionMetadata
if len(revisions) == 0 {
// Remove all revisions.
revs, err := removeState.ListSecretRevisions(uri)
revs, err = removeState.ListSecretRevisions(uri)
if err != nil {
return errors.Trace(err)
}
for _, rev := range revs {
gatherExternalRevs(rev.ValueRef)
return nil, []error{errors.Trace(err)}
}
} else {
for _, rev := range revisions {
revs = make([]*coresecrets.SecretRevisionMetadata, len(revisions))
for i, rev := range revisions {
revMeta, err := removeState.GetSecretRevision(uri, rev)
if err != nil {
return errors.Trace(err)
return nil, []error{errors.Trace(err)}
}
gatherExternalRevs(revMeta.ValueRef)
revs[i] = revMeta
}
}

for backendID, r := range externalRevs {
backendCfg, ok := cfgInfo.Configs[backendID]
if !ok {
return errors.NotFoundf("secret backend %q", backendID)
var revisionsDeleted []int
var errorsEncountered []error
providersToCleanUp := make(map[string]provider.SecretRevisions)

deleteRevisionFromBackend := func(revisionId string, backendId string) error {
p, err := GetProvider(cfgInfo.Configs[backendId].BackendType)
if err != nil {
return errors.Trace(err)
}
provider, err := GetProvider(backendCfg.BackendType)

// delete secret in backend
backendCfg := cfgInfo.Configs[backendId]
backend, err := p.NewBackend(&backendCfg)
if err != nil {
return errors.Trace(err)
}
if err := removeFromBackend(provider, backendCfg, r); err != nil {

if err = backend.DeleteContent(context.TODO(), revisionId); err != nil {
return errors.Trace(err)
}
return nil
}
return nil
}

for i, arg := range args.Args {
if arg.URI == "" && arg.Label == "" {
result.Results[i].Error = apiservererrors.ServerError(errors.New("must specify either URI or label"))
continue
for _, rev := range revs {
// TODO: This returns on the first error, but we're called with multiple revisions. Should we instead
// keep a list of any errors encountered and return that?
backendId := rev.ValueRef.BackendID
revisionId := rev.ValueRef.RevisionID

for {
err := deleteRevisionFromBackend(revisionId, backendId)
if err == nil {
break
}
// Capture the any non NotFound error and go to the next revision
if err == nil || !errors.Is(err, errors.NotFound) {
errorsEncountered = append(errorsEncountered, errors.Trace(err))
break
}

// NotFound could be because:
// 1. The backend is draining and the secret was moved to the new backend before we accessed it.
// 2. The secret is actually missing from the backend.
// Check if the revision has moved to a new backend
// TODO: I'm pretty sure rev.Revision is the same Revision as needed by GetSecretRevision?
updatedRev, err := removeState.GetSecretRevision(uri, rev.Revision)
if err != nil {
errorsEncountered = append(errorsEncountered, errors.Trace(err))
break
}

// If the backend changed, try to delete the secret from the new backend.
if backendId != updatedRev.ValueRef.BackendID {
backendId = updatedRev.ValueRef.BackendID
revisionId = updatedRev.ValueRef.RevisionID
continue
}

// Otherwise, the revision really is missing from the backend and we move on.
// We tolerate this because our goal is to have that revision removed anyway.
// TODO: Log? In the calling function, if removeState.GetSecret(uri) returns an error
// that is added to the return. But in here, we might encounter an error for each revision.
// TODO: Should we clean up this provider even though we didn't delete the revision? Maybe it got
// deleted some other way and left configurations dangling in the provider?
break
}

// Log this revision to be cleaned up in the provider
if _, ok := providersToCleanUp[backendId]; !ok {
providersToCleanUp[backendId] = provider.SecretRevisions{}
}
providersToCleanUp[backendId].Add(uri, rev.ValueRef.RevisionID)
// TODO: Should we document a revision as deleted here, or only if the cleanup is successful?
revisionsDeleted = append(revisionsDeleted, rev.Revision)
}

var (
uri *coresecrets.URI
err error
)
if arg.URI != "" {
uri, err = coresecrets.ParseURI(arg.URI)
} else {
uri, err = getSecretURIForLabel(removeState, modelUUID, arg.Label)
// Clean up all providers we've touched
for backendId, secretRevisions := range providersToCleanUp {
backendCfg := cfgInfo.Configs[backendId]
p, err := GetProvider(cfgInfo.Configs[backendId].BackendType)
if err != nil {
errorsEncountered = append(errorsEncountered, errors.Trace(err))
}

if err := p.CleanupSecrets(&backendCfg, authTag, secretRevisions); err != nil {
errorsEncountered = append(errorsEncountered, errors.Trace(err))
}
}

return revisionsDeleted, errorsEncountered
}

for i, arg := range args.Args {
uri, err := parseDeleteSecretArg(arg, removeState, modelUUID)
if err != nil {
result.Results[i].Error = apiservererrors.ServerError(err)
continue
Expand All @@ -758,15 +798,38 @@ func removeSecrets(
result.Results[i].Error = apiservererrors.ServerError(err)
continue
}
if err := removeFromExternal(uri, arg.Revisions...); err != nil {

revisionsDeleted, errors := removeFromExternal(uri, arg.Revisions...)
if len(errors) > 0 {
// TODO: How do we surface these errors? This gets called somewhere that expectes a single error
// We remove the secret from the backend first.
result.Results[i].Error = apiservererrors.ServerError(err)
continue
// TODO: Can't continue here. We still need to `DeleteSecret` below for any revisionsDeleted
//continue
}
if _, err = removeState.DeleteSecret(uri, arg.Revisions...); err != nil {
if _, err = removeState.DeleteSecret(uri, revisionsDeleted...); err != nil {
// TODO: This could error on top of removeFromExternal reutrning some errors. Need to combine or something
result.Results[i].Error = apiservererrors.ServerError(err)
continue
}
}
return result, nil

}

func getSecretURIForLabel(secretsState ListSecretsState, modelUUID string, label string) (*coresecrets.URI, error) {
results, err := secretsState.ListSecrets(state.SecretsFilter{
Label: &label,
OwnerTags: []names.Tag{names.NewModelTag(modelUUID)},
})
if err != nil {
return nil, errors.Trace(err)
}
if len(results) == 0 {
return nil, errors.NotFoundf("secret %q", label)
}
if len(results) > 1 {
return nil, errors.NotFoundf("more than 1 secret with label %q", label)
}
return results[0].URI, nil
}
51 changes: 29 additions & 22 deletions apiserver/common/secrets/secrets_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -813,10 +813,12 @@ func (s *secretsSuite) TestRemoveSecretsForSecretOwnersWithRevisions(c *gc.C) {
s.PatchValue(&secrets.GetProvider, func(string) (provider.SecretBackendProvider, error) { return mockprovider, nil })

removeState.EXPECT().GetSecret(&expectURI).Return(&coresecrets.SecretMetadata{}, nil)
removeState.EXPECT().GetSecretRevision(&expectURI, 666).Return(&coresecrets.SecretRevisionMetadata{
Revision: 666,
ValueRef: &coresecrets.ValueRef{BackendID: "backend-id", RevisionID: "rev-666"},
}, nil)
// TODO: Test fails because we don't call GetSecretRevision during RemoveSecretsForAgent anymore.
// I don't think we ever needed to, and the new code path doesn't at all. Remove this?
//removeState.EXPECT().GetSecretRevision(&expectURI, 666).Return(&coresecrets.SecretRevisionMetadata{
// Revision: 666,
// ValueRef: &coresecrets.ValueRef{BackendID: "backend-id", RevisionID: "rev-666"},
//}, nil)
removeState.EXPECT().DeleteSecret(&expectURI, []int{666}).Return([]coresecrets.ValueRef{{
BackendID: "backend-id",
RevisionID: "rev-666",
Expand Down Expand Up @@ -867,15 +869,17 @@ func (s *secretsSuite) TestRemoveSecretsForSecretOwners(c *gc.C) {
s.PatchValue(&secrets.GetProvider, func(string) (provider.SecretBackendProvider, error) { return mockprovider, nil })

removeState.EXPECT().GetSecret(&expectURI).Return(&coresecrets.SecretMetadata{}, nil)
removeState.EXPECT().ListSecretRevisions(&expectURI).Return(
[]*coresecrets.SecretRevisionMetadata{
{
Revision: 666,
ValueRef: &coresecrets.ValueRef{BackendID: "backend-id", RevisionID: "rev-666"},
},
},
nil,
)
// TODO: Test fails because we don't call ListSecretRevisions during RemoveSecretsForAgent anymore.
// I don't think we ever needed to, and the new code path doesn't at all. Remove this?
//removeState.EXPECT().ListSecretRevisions(&expectURI).Return(
// []*coresecrets.SecretRevisionMetadata{
// {
// Revision: 666,
// ValueRef: &coresecrets.ValueRef{BackendID: "backend-id", RevisionID: "rev-666"},
// },
// },
// nil,
//)
removeState.EXPECT().DeleteSecret(&expectURI, []int{}).Return([]coresecrets.ValueRef{{
BackendID: "backend-id",
RevisionID: "rev-666",
Expand Down Expand Up @@ -918,6 +922,7 @@ func ptr[T any](v T) *T {
return &v
}

// TODO: Rename to include Agent in the name?
func (s *secretsSuite) TestRemoveSecretsByLabel(c *gc.C) {
ctrl := gomock.NewController(c)
defer ctrl.Finish()
Expand All @@ -935,15 +940,17 @@ func (s *secretsSuite) TestRemoveSecretsByLabel(c *gc.C) {
URI: uri,
}}, nil)
removeState.EXPECT().GetSecret(&expectURI).Return(&coresecrets.SecretMetadata{}, nil)
removeState.EXPECT().ListSecretRevisions(&expectURI).Return(
[]*coresecrets.SecretRevisionMetadata{
{
Revision: 666,
ValueRef: &coresecrets.ValueRef{BackendID: "backend-id", RevisionID: "rev-666"},
},
},
nil,
)
// TODO: Test fails because we don't call ListSecretRevisions during RemoveSecretsForAgent anymore.
// I don't think we ever needed to, and the new code path doesn't at all. Remove this?
//removeState.EXPECT().ListSecretRevisions(&expectURI).Return(
// []*coresecrets.SecretRevisionMetadata{
// {
// Revision: 666,
// ValueRef: &coresecrets.ValueRef{BackendID: "backend-id", RevisionID: "rev-666"},
// },
// },
// nil,
//)
removeState.EXPECT().DeleteSecret(&expectURI, []int{}).Return([]coresecrets.ValueRef{{
BackendID: "backend-id",
RevisionID: "rev-666",
Expand Down