Merge pull request juju#17470 from ycliuhw/implement-WatchRevisionsTo…

…Prune

juju#17470

This PR introduces a watcher that monitors when user secret revisions become obsolete or the auto-prune secret config is changed to `true`. Typically, non-latest secret revisions that no longer have any consumers will be marked as obsolete. These obsolete revisions are then automatically removed by the auto-prune workers for user secrets (if the auto-prune config is set to true).
The reason why the WatchObsoleteRevisionsToPrune was implemented as a multi-watcher is that it can be triggered in two different scenarios: 
- a user secret revision changed to obsolete and the secret auto-prune config is turned on;
- a user secret's auto-prune config changed from false to true;

Depends on juju#17592

## Checklist

- [x] Code style: imports ordered, good names, simple structure, etc
- [x] Comments saying why design decisions were made
- [x] Go unit tests, with comments saying what you're testing
- [ ] ~[Integration tests](https://github.com/juju/juju/tree/main/tests), with comments saying what you're testing~
- [ ] ~[doc.go](https://discourse.charmhub.io/t/readme-in-packages/451) added or updated in changed packages~

## QA steps

```
juju show-secret foo --revisions

cpp9fq7mp25c7823t5sg:
 revision: 4
 rotation: never
 owner: <model>
 name: foo
 created: 2024-06-19T08:33:44.742302307Z
 updated: 2024-06-19T08:33:53.684554916Z
 revisions:
 - revision: 1
 created: 2024-06-19T08:33:44.742302307Z
 updated: 0001-01-01T00:00:00Z
 - revision: 2
 created: 2024-06-19T08:33:51.156979444Z
 updated: 0001-01-01T00:00:00Z
 - revision: 3
 created: 2024-06-19T08:33:52.320925473Z
 updated: 0001-01-01T00:00:00Z
 - revision: 4
 created: 2024-06-19T08:33:53.684554916Z
 updated: 0001-01-01T00:00:00Z

juju grant-secret foo snappass-test

juju show-secret foo --revisions

cpp9fq7mp25c7823t5sg:
 revision: 4
 rotation: never
 owner: <model>
 name: foo
 created: 2024-06-19T08:33:44.742302307Z
 updated: 2024-06-19T08:33:53.684554916Z
 revisions:
 - revision: 1
 created: 2024-06-19T08:33:44.742302307Z
 updated: 0001-01-01T00:00:00Z
 - revision: 2
 created: 2024-06-19T08:33:51.156979444Z
 updated: 0001-01-01T00:00:00Z
 - revision: 3
 created: 2024-06-19T08:33:52.320925473Z
 updated: 0001-01-01T00:00:00Z
 - revision: 4
 created: 2024-06-19T08:33:53.684554916Z
 updated: 0001-01-01T00:00:00Z
 access:
 - target: application-snappass-test
 scope: model-45f2fbe8-f395-468b-8ab8-3bb6269e8b1c
 role: view

juju exec --unit snappass-test/0 -- secret-get cpp9fq7mp25c7823t5sg
token: "3"

juju update-secret foo --auto-prune

juju show-secret foo --revisions

cpp9fq7mp25c7823t5sg:
 revision: 4
 rotation: never
 owner: <model>
 name: foo
 created: 2024-06-19T08:33:44.742302307Z
 updated: 2024-06-19T08:34:53.390873807Z
 revisions:
 - revision: 4
 created: 2024-06-19T08:33:53.684554916Z
 updated: 0001-01-01T00:00:00Z
 access:
 - target: application-snappass-test
 scope: model-45f2fbe8-f395-468b-8ab8-3bb6269e8b1c
 role: view

juju update-secret foo token=4

juju show-secret foo --revisions

cpp9fq7mp25c7823t5sg:
 revision: 5
 rotation: never
 owner: <model>
 name: foo
 created: 2024-06-19T08:33:44.742302307Z
 updated: 2024-06-19T08:35:06.345225733Z
 revisions:
 - revision: 4
 created: 2024-06-19T08:33:53.684554916Z
 updated: 0001-01-01T00:00:00Z
 - revision: 5
 created: 2024-06-19T08:35:06.345225733Z
 updated: 0001-01-01T00:00:00Z
 access:
 - target: application-snappass-test
 scope: model-45f2fbe8-f395-468b-8ab8-3bb6269e8b1c
 role: view

juju exec --unit snappass-test/0 -- secret-get cpp9fq7mp25c7823t5sg --refresh
token: "4"

juju show-secret foo --revisions

cpp9fq7mp25c7823t5sg:
 revision: 5
 rotation: never
 owner: <model>
 name: foo
 created: 2024-06-19T08:33:44.742302307Z
 updated: 2024-06-19T08:35:06.345225733Z
 revisions:
 - revision: 5
 created: 2024-06-19T08:35:06.345225733Z
 updated: 0001-01-01T00:00:00Z
 access:
 - target: application-snappass-test
 scope: model-45f2fbe8-f395-468b-8ab8-3bb6269e8b1c
 role: view
```

## Documentation changes

No

## Links

**Jira card:** 
- JUJU-6005
- JUJU-4723

api/controller/usersecrets/client.go

@@ -47,6 +47,6 @@ func (c *Client) WatchRevisionsToPrune() (watcher.NotifyWatcher, error) {
 }
 
 // DeleteObsoleteUserSecrets deletes any obsolete user secret revisions.
-func (c *Client) DeleteObsoleteUserSecrets() error {
-	return c.facade.FacadeCall(context.TODO(), "DeleteObsoleteUserSecrets", nil, nil)
+func (c *Client) DeleteObsoleteUserSecretRevisions(ctx context.Context) error {
+	return c.facade.FacadeCall(ctx, "DeleteObsoleteUserSecretRevisions", nil, nil)
 }

api/controller/usersecrets/client_test.go

@@ -4,6 +4,8 @@
 package usersecrets_test
 
 import (
+	"context"
+
 	"github.com/juju/errors"
 	gc "gopkg.in/check.v1"
 
@@ -45,17 +47,17 @@ func (s *secretSuite) TestWatchRevisionsToPrune(c *gc.C) {
 	c.Assert(err, gc.ErrorMatches, "FAIL")
 }
 
-func (s *secretSuite) TestDeleteObsoleteUserSecrets(c *gc.C) {
+func (s *secretSuite) TestDeleteObsoleteUserSecretRevisions(c *gc.C) {
 	apiCaller := testing.APICallerFunc(func(objType string, version int, id, request string, arg, result interface{}) error {
 		c.Check(objType, gc.Equals, "UserSecretsManager")
 		c.Check(version, gc.Equals, 0)
 		c.Check(id, gc.Equals, "")
-		c.Check(request, gc.Equals, "DeleteObsoleteUserSecrets")
+		c.Check(request, gc.Equals, "DeleteObsoleteUserSecretRevisions")
 		c.Check(arg, gc.IsNil)
 		c.Assert(result, gc.IsNil)
 		return errors.New("boom")
 	})
 	client := usersecrets.NewClient(apiCaller)
-	err := client.DeleteObsoleteUserSecrets()
+	err := client.DeleteObsoleteUserSecretRevisions(context.Background())
 	c.Assert(err, gc.ErrorMatches, "boom")
 }

apiserver/facades/controller/usersecrets/package_test.go

@@ -21,15 +21,14 @@ func TestPackage(t *testing.T) {
 
 func NewTestAPI(
 	authorizer facade.Authorizer,
-	resources facade.Resources,
+	watcherRegistry facade.WatcherRegistry,
 	secretService SecretService,
 ) (*UserSecretsManager, error) {
 	if !authorizer.AuthController() {
 		return nil, apiservererrors.ErrPerm
 	}
 	return &UserSecretsManager{
-		authorizer:    authorizer,
-		resources:     resources,
-		secretService: secretService,
+		secretService:   secretService,
+		watcherRegistry: watcherRegistry,
 	}, nil
 }

apiserver/facades/controller/usersecrets/register.go

@@ -39,8 +39,7 @@ func NewUserSecretsManager(ctx facade.ModelContext) (*UserSecretsManager, error)
 	}
 
 	return &UserSecretsManager{
-		authorizer:    ctx.Auth(),
-		resources:     ctx.Resources(),
-		secretService: serviceFactory.Secret(backendConfigGetter),
+		watcherRegistry: ctx.WatcherRegistry(),
+		secretService:   serviceFactory.Secret(backendConfigGetter),
 	}, nil
 }

apiserver/facades/controller/usersecrets/secrets.go

@@ -8,34 +8,33 @@ import (
 
 	"github.com/juju/errors"
 
+	apiservererrors "github.com/juju/juju/apiserver/errors"
 	"github.com/juju/juju/apiserver/facade"
+	"github.com/juju/juju/apiserver/internal"
 	"github.com/juju/juju/rpc/params"
 )
 
 // UserSecretsManager is the implementation for the usersecrets facade.
 type UserSecretsManager struct {
-	authorizer facade.Authorizer
-	resources  facade.Resources
-
-	secretService SecretService
+	watcherRegistry facade.WatcherRegistry
+	secretService   SecretService
 }
 
 // WatchRevisionsToPrune returns a watcher for notifying when:
 //   - a secret revision owned by the model no longer
 //     has any consumers and should be pruned.
 func (s *UserSecretsManager) WatchRevisionsToPrune(ctx context.Context) (params.NotifyWatchResult, error) {
 	result := params.NotifyWatchResult{}
-	w, err := s.secretService.WatchObsoleteUserSecrets(ctx)
+	w, err := s.secretService.WatchObsoleteUserSecretsToPrune(ctx)
 	if err != nil {
 		return result, errors.Trace(err)
 	}
-	if _, ok := <-w.Changes(); ok {
-		result.NotifyWatcherId = s.resources.Register(w)
-	}
+	result.NotifyWatcherId, _, err = internal.EnsureRegisterWatcher(ctx, s.watcherRegistry, w)
+	result.Error = apiservererrors.ServerError(err)
 	return result, nil
 }
 
-// DeleteObsoleteUserSecrets deletes any obsolete user secret revisions.
-func (s *UserSecretsManager) DeleteObsoleteUserSecrets(ctx context.Context) error {
-	return s.secretService.DeleteObsoleteUserSecrets(ctx)
+// DeleteObsoleteUserSecretRevisions deletes any obsolete user secret revisions.
+func (s *UserSecretsManager) DeleteObsoleteUserSecretRevisions(ctx context.Context) error {
+	return s.secretService.DeleteObsoleteUserSecretRevisions(ctx)
 }

apiserver/facades/controller/usersecrets/secrets_test.go

@@ -21,12 +21,12 @@ type userSecretsSuite struct {
 	testing.IsolationSuite
 
 	authorizer *facademocks.MockAuthorizer
-	resources  *facademocks.MockResources
 
 	secretService *mocks.MockSecretService
 	watcher       *mocks.MockNotifyWatcher
 
-	facade *usersecrets.UserSecretsManager
+	facade          *usersecrets.UserSecretsManager
+	watcherRegistry *facademocks.MockWatcherRegistry
 }
 
 var _ = gc.Suite(&userSecretsSuite{})
@@ -35,30 +35,28 @@ func (s *userSecretsSuite) setup(c *gc.C) *gomock.Controller {
 	ctrl := gomock.NewController(c)
 
 	s.authorizer = facademocks.NewMockAuthorizer(ctrl)
-	s.resources = facademocks.NewMockResources(ctrl)
 	s.watcher = mocks.NewMockNotifyWatcher(ctrl)
 	s.secretService = mocks.NewMockSecretService(ctrl)
+	s.watcherRegistry = facademocks.NewMockWatcherRegistry(ctrl)
 
 	s.authorizer.EXPECT().AuthController().Return(true)
 
 	var err error
-	s.facade, err = usersecrets.NewTestAPI(
-		s.authorizer, s.resources,
-		s.secretService,
-	)
+	s.facade, err = usersecrets.NewTestAPI(s.authorizer, s.watcherRegistry, s.secretService)
 	c.Assert(err, jc.ErrorIsNil)
 	return ctrl
 }
 
 func (s *userSecretsSuite) TestWatchRevisionsToPrune(c *gc.C) {
 	defer s.setup(c).Finish()
 
-	s.secretService.EXPECT().WatchObsoleteUserSecrets(gomock.Any()).Return(s.watcher, nil)
-	s.resources.EXPECT().Register(s.watcher).Return("watcher-id")
+	s.secretService.EXPECT().WatchObsoleteUserSecretsToPrune(gomock.Any()).Return(s.watcher, nil)
 	ch := make(chan struct{}, 1)
 	ch <- struct{}{}
 	s.watcher.EXPECT().Changes().Return(ch)
 
+	s.watcherRegistry.EXPECT().Register(gomock.Any()).Return("watcher-id", nil)
+
 	result, err := s.facade.WatchRevisionsToPrune(context.Background())
 	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(result, jc.DeepEquals, params.NotifyWatchResult{
@@ -69,8 +67,7 @@ func (s *userSecretsSuite) TestWatchRevisionsToPrune(c *gc.C) {
 func (s *userSecretsSuite) TestDeleteRevisionsAutoPruneEnabled(c *gc.C) {
 	defer s.setup(c).Finish()
 
-	s.secretService.EXPECT().DeleteObsoleteUserSecrets(gomock.Any()).Return(nil)
-
-	err := s.facade.DeleteObsoleteUserSecrets(context.Background())
+	s.secretService.EXPECT().DeleteObsoleteUserSecretRevisions(gomock.Any()).Return(nil)
+	err := s.facade.DeleteObsoleteUserSecretRevisions(context.Background())
 	c.Assert(err, jc.ErrorIsNil)
 }

apiserver/facades/controller/usersecrets/service.go

@@ -13,6 +13,6 @@ import (
 // SecretService instances provide secret apis.
 type SecretService interface {
 	GetSecret(ctx context.Context, uri *secrets.URI) (*secrets.SecretMetadata, error)
-	DeleteObsoleteUserSecrets(ctx context.Context) error
-	WatchObsoleteUserSecrets(ctx context.Context) (watcher.NotifyWatcher, error)
+	DeleteObsoleteUserSecretRevisions(ctx context.Context) error
+	WatchObsoleteUserSecretsToPrune(ctx context.Context) (watcher.NotifyWatcher, error)
 }

apiserver/facades/schema.json

@@ -45413,9 +45413,9 @@
         "Schema": {
             "type": "object",
             "properties": {
-                "DeleteObsoleteUserSecrets": {
+                "DeleteObsoleteUserSecretRevisions": {
                     "type": "object",
-                    "description": "DeleteObsoleteUserSecrets deletes any obsolete user secret revisions."
+                    "description": "DeleteObsoleteUserSecretRevisions deletes any obsolete user secret revisions."
                 },
                 "WatchRevisionsToPrune": {
                     "type": "object",

core/watcher/watchertest/notify.go

@@ -115,6 +115,34 @@ func (c NotifyWatcherC) AssertChanges(duration time.Duration) {
 	}
 }
 
+// AssertNChanges fails if it does not receive n changes before a long time has passed.
+func (c NotifyWatcherC) AssertNChanges(n int) {
+	if n <= 1 {
+		c.Fatalf("n must be greater than 1")
+	}
+	received := 0
+	for {
+		select {
+		case _, ok := <-c.Watcher.Changes():
+			c.Check(ok, jc.IsTrue)
+			received++
+
+			if received < n {
+				continue
+			}
+			// Ensure we have no more changes.
+			c.AssertNoChange()
+			return
+		case <-time.After(testing.LongWait):
+			if received == 0 {
+				c.Fatalf("watcher did not send any changes")
+			} else {
+				c.Fatalf("watcher received %d changes, expected %d", received, n)
+			}
+		}
+	}
+}
+
 // AssertNoChange fails if it manages to read a value from Changes before a
 // short time has passed.
 func (c NotifyWatcherC) AssertNoChange() {

domain/secret/service/delete.go

@@ -15,10 +15,9 @@ import (
 	"github.com/juju/juju/internal/secrets/provider"
 )
 
-// DeleteObsoleteUserSecrets deletes any obsolete user secret revisions that are marked as auto-prune.
-func (s *SecretService) DeleteObsoleteUserSecrets(ctx context.Context) error {
-	// TODO(secrets)
-	return nil
+// DeleteObsoleteUserSecretRevisions deletes any obsolete user secret revisions that are marked as auto-prune.
+func (s *SecretService) DeleteObsoleteUserSecretRevisions(ctx context.Context) error {
+	return s.st.DeleteObsoleteUserSecretRevisions(ctx)
 }
 
 // DeleteSecret removes the specified secret.

domain/secret/service/delete_test.go

@@ -45,3 +45,13 @@ func (s *serviceSuite) TestDeleteSecretInternal(c *gc.C) {
 }
 
 // TODO(secrets) - add tests for backend when properly implemented
+
+func (s *serviceSuite) TestDeleteObsoleteUserSecretRevisions(c *gc.C) {
+	ctrl := gomock.NewController(c)
+	defer ctrl.Finish()
+
+	s.state = NewMockState(ctrl)
+	s.state.EXPECT().DeleteObsoleteUserSecretRevisions(gomock.Any()).Return(nil)
+	err := s.service(c).DeleteObsoleteUserSecretRevisions(context.Background())
+	c.Assert(err, jc.ErrorIsNil)
+}

domain/secret/service/package_test.go

@@ -12,7 +12,7 @@ import (
 //go:generate go run go.uber.org/mock/mockgen -typed -package service -destination state_mock_test.go github.com/juju/juju/domain/secret/service State
 //go:generate go run go.uber.org/mock/mockgen -typed -package service -destination provider_mock_test.go github.com/juju/juju/internal/secrets/provider SecretBackendProvider,SecretsBackend
 //go:generate go run go.uber.org/mock/mockgen -typed -package service -destination watcherfactory_mock_test.go github.com/juju/juju/domain/secret/service WatcherFactory
-//go:generate go run go.uber.org/mock/mockgen -typed -package service -destination watcher_mock_test.go github.com/juju/juju/core/watcher StringsWatcher
+//go:generate go run go.uber.org/mock/mockgen -typed -package service -destination watcher_mock_test.go github.com/juju/juju/core/watcher StringsWatcher,NotifyWatcher
 //go:generate go run go.uber.org/mock/mockgen -typed -package service -destination token_mock_test.go github.com/juju/juju/core/leadership Token
 
 func Test(t *testing.T) {