Merge pull request juju#18457 from manadart/dqlite-secrets-caveat

juju#18457

This change is mechanical only and serves as preparation for a patch to follow.

The secrets service was using a `canManage` function, which included a logical branch that asserted application leadership of unit accessors for application-owned secrets.

The issue with this is that the point-in-time check may become invalid later, during execution of the logic requiring said check.

In preparation for using `WithLease` to ensure checked conditions for the duration of the operations predicated on them, we swap `canManage` for `getManagementCaveat`. Right now, the caveat is always permissive if the point-in-time check is satisfied, but we will replace this later with usage of `WithLease`.

Sundry nips and tucks accompany including usage of the new errors package for code under change. Formatting changes are in the first of the two commits. Looking at the second in isolation clarifies material change.

## QA steps

Passing tests.

domain/leaseservice.go

@@ -27,7 +27,9 @@ func NewLeaseService(leaseChecker lease.ModelLeaseManagerGetter) *LeaseService {
 // function returns.
 // The context must be passed to the closure function to ensure that the
 // cancellation is propagated to the closure.
-func (s *LeaseService) WithLease(ctx context.Context, leaseName, holderName string, fn func(context.Context) error) error {
+func (s *LeaseService) WithLease(
+	ctx context.Context, leaseName, holderName string, fn func(context.Context) error,
+) error {
 	// Holding the lease is quite a complex operation, so we need to ensure that
 	// the context is not cancelled before we start the operation.
 	if err := ctx.Err(); err != nil {

domain/leaseservice_test.go

@@ -33,17 +33,19 @@ func (s *leaseServiceSuite) TestWithLease(c *gc.C) {
 	done := make(chan struct{})
 
 	// Force the lease wait to be triggered.
-	s.leaseChecker.EXPECT().WaitUntilExpired(gomock.Any(), "leaseName", gomock.Any()).DoAndReturn(func(ctx context.Context, leaseName string, start chan<- struct{}) error {
-		close(start)
-
-		// Don't return until the lease function is done.
-		select {
-		case <-done:
-		case <-time.After(testing.LongWait):
-			c.Fatalf("lease function not done")
-		}
-		return nil
-	})
+	s.leaseChecker.EXPECT().WaitUntilExpired(gomock.Any(), "leaseName", gomock.Any()).DoAndReturn(
+		func(ctx context.Context, leaseName string, start chan<- struct{}) error {
+			close(start)
+
+			// Don't return until the lease function is done.
+			select {
+			case <-done:
+			case <-time.After(testing.LongWait):
+				c.Fatalf("lease function not done")
+			}
+			return nil
+		},
+	)
 
 	// Check we correctly hold the lease.
 	s.leaseChecker.EXPECT().Token("leaseName", "holderName").Return(s.token)
@@ -64,9 +66,11 @@ func (s *leaseServiceSuite) TestWithLease(c *gc.C) {
 func (s *leaseServiceSuite) TestWithLeaseWaitReturnsError(c *gc.C) {
 	defer s.setupMocks(c).Finish()
 
-	s.leaseChecker.EXPECT().WaitUntilExpired(gomock.Any(), "leaseName", gomock.Any()).DoAndReturn(func(ctx context.Context, leaseName string, start chan<- struct{}) error {
-		return fmt.Errorf("not holding lease")
-	})
+	s.leaseChecker.EXPECT().WaitUntilExpired(gomock.Any(), "leaseName", gomock.Any()).DoAndReturn(
+		func(ctx context.Context, leaseName string, start chan<- struct{}) error {
+			return fmt.Errorf("not holding lease")
+		},
+	)
 
 	service := NewLeaseService(s.modelLeaseManager)
 
@@ -87,19 +91,21 @@ func (s *leaseServiceSuite) TestWithLeaseWaitHasLeaseChange(c *gc.C) {
 
 	// Cause the start to be triggered right away, but ensure that the
 	// lease has changed.
-	s.leaseChecker.EXPECT().WaitUntilExpired(gomock.Any(), "leaseName", gomock.Any()).DoAndReturn(func(ctx context.Context, leaseName string, start chan<- struct{}) error {
-		close(start)
+	s.leaseChecker.EXPECT().WaitUntilExpired(gomock.Any(), "leaseName", gomock.Any()).DoAndReturn(
+		func(ctx context.Context, leaseName string, start chan<- struct{}) error {
+			close(start)
 
-		select {
-		case <-running:
-		case <-time.After(testing.LongWait):
-			c.Fatalf("lease function not running")
-		}
+			select {
+			case <-running:
+			case <-time.After(testing.LongWait):
+				c.Fatalf("lease function not running")
+			}
 
-		close(done)
+			close(done)
 
-		return fmt.Errorf("not holding lease")
-	})
+			return fmt.Errorf("not holding lease")
+		},
+	)
 
 	// Check we correctly hold the lease.
 	s.leaseChecker.EXPECT().Token("leaseName", "holderName").Return(s.token)
@@ -145,16 +151,18 @@ func (s *leaseServiceSuite) TestWithLeaseFailsOnWaitCheck(c *gc.C) {
 
 	// Cause the start to be triggered right away, but ensure that the
 	// lease has changed.
-	s.leaseChecker.EXPECT().WaitUntilExpired(gomock.Any(), "leaseName", gomock.Any()).DoAndReturn(func(ctx context.Context, leaseName string, start chan<- struct{}) error {
-		close(start)
-
-		select {
-		case <-done:
-		case <-time.After(testing.LongWait):
-		}
-
-		return nil
-	})
+	s.leaseChecker.EXPECT().WaitUntilExpired(gomock.Any(), "leaseName", gomock.Any()).DoAndReturn(
+		func(ctx context.Context, leaseName string, start chan<- struct{}) error {
+			close(start)
+
+			select {
+			case <-done:
+			case <-time.After(testing.LongWait):
+			}
+
+			return nil
+		},
+	)
 
 	// Fail the lease check.
 	s.leaseChecker.EXPECT().Token("leaseName", "holderName").Return(s.token)

domain/secret/service/access.go

@@ -129,10 +129,14 @@ func (s *SecretService) getSecretAccess(ctx context.Context, uri *secrets.URI, a
 // satisfying [secreterrors.InvalidSecretPermissionChange] is returned.
 // It returns [secreterrors.PermissionDenied] if the secret cannot be managed by the accessor.
 func (s *SecretService) GrantSecretAccess(ctx context.Context, uri *secrets.URI, params SecretAccessParams) error {
-	if err := s.canManage(ctx, uri, params.Accessor, params.LeaderToken); err != nil {
+	withCaveat, err := s.getManagementCaveat(ctx, uri, params.Accessor, params.LeaderToken)
+	if err != nil {
 		return errors.Trace(err)
 	}
-	return s.secretState.GrantAccess(ctx, uri, grantParams(params))
+
+	return withCaveat(ctx, func(innerCtx context.Context) error {
+		return s.secretState.GrantAccess(innerCtx, uri, grantParams(params))
+	})
 }
 
 func grantParams(in SecretAccessParams) domainsecret.GrantParams {
@@ -168,7 +172,8 @@ func grantParams(in SecretAccessParams) domainsecret.GrantParams {
 // RevokeSecretAccess revokes access to the secret for the specified subject.
 // It returns an error satisfying [secreterrors.SecretNotFound] if the secret is not found.
 func (s *SecretService) RevokeSecretAccess(ctx context.Context, uri *secrets.URI, params SecretAccessParams) error {
-	if err := s.canManage(ctx, uri, params.Accessor, params.LeaderToken); err != nil {
+	withCaveat, err := s.getManagementCaveat(ctx, uri, params.Accessor, params.LeaderToken)
+	if err != nil {
 		return errors.Trace(err)
 	}
 
@@ -186,29 +191,42 @@ func (s *SecretService) RevokeSecretAccess(ctx context.Context, uri *secrets.URI
 		p.SubjectTypeID = domainsecret.SubjectModel
 	}
 
-	return s.secretState.RevokeAccess(ctx, uri, p)
+	return withCaveat(ctx, func(innerCtx context.Context) error {
+		return s.secretState.RevokeAccess(innerCtx, uri, p)
+	})
 }
 
-// canManage checks that the accessor can manage the secret.
-// If the request is for a secret owned by an application, the unit must be the leader.
-func (s *SecretService) canManage(
+// getManagementCaveat returns a function within which an operation can be
+// executed if the caveat remains satisfied.
+// If the secret is unit-owned and the unit can manage it, the caveat is always
+// permissive.
+// If the secret is application-owned, the unit must be, and remain the leader
+// of that application.
+// If the caveat can never be satisfied, an error is returned - the input
+// accessor can never manage the input secret.
+func (s *SecretService) getManagementCaveat(
 	ctx context.Context,
 	uri *secrets.URI, assessor SecretAccessor,
 	leaderToken leadership.Token,
-) error {
+) (func(context.Context, func(context.Context) error) error, error) {
 	hasRole, err := s.getSecretAccess(ctx, uri, assessor)
 	if err != nil {
 		// Typically not found error.
-		return errors.Trace(err)
+		return nil, errors.Trace(err)
 	}
 	if hasRole.Allowed(secrets.RoleManage) {
-		return nil
+		return func(ctx context.Context, fn func(context.Context) error) error {
+			return fn(ctx)
+		}, nil
 	}
 	// Units can manage app owned secrets if they are the leader.
 	if assessor.Kind == UnitAccessor {
 		if leaderToken == nil {
-			return secreterrors.PermissionDenied
+			return nil, secreterrors.PermissionDenied
 		}
+
+		// TODO (manadart 2024-11-29): This section will be updated to
+		// return a WithLease wrapper in a patch to follow.
 		if err := leaderToken.Check(); err == nil {
 			appName, _ := names.UnitApplication(assessor.ID)
 			hasRole, err = s.getSecretAccess(ctx, uri, SecretAccessor{
@@ -217,14 +235,17 @@ func (s *SecretService) canManage(
 			})
 			if err != nil {
 				// Typically not found error.
-				return errors.Trace(err)
+				return nil, errors.Trace(err)
 			}
 			if hasRole.Allowed(secrets.RoleManage) {
-				return nil
+				return func(ctx context.Context, fn func(context.Context) error) error {
+					return fn(ctx)
+				}, nil
 			}
 		}
 	}
-	return fmt.Errorf("%q is not allowed to manage this secret%w", assessor.ID, errors.Hide(secreterrors.PermissionDenied))
+	return nil, fmt.Errorf(
+		"%q is not allowed to manage this secret %w", assessor.ID, errors.Hide(secreterrors.PermissionDenied))
 }
 
 // canRead checks that the accessor can read the secret.

domain/secret/service/access_test.go

@@ -14,7 +14,7 @@ import (
 	domainsecret "github.com/juju/juju/domain/secret"
 )
 
-func (s *serviceSuite) TestCanManageOwnerUnit(c *gc.C) {
+func (s *serviceSuite) TestGetManagementCaveatOwnerUnit(c *gc.C) {
 	ctrl := s.setupMocks(c)
 	defer ctrl.Finish()
 
@@ -27,14 +27,14 @@ func (s *serviceSuite) TestCanManageOwnerUnit(c *gc.C) {
 
 	token := NewMockToken(ctrl)
 
-	err := s.service.canManage(context.Background(), uri, SecretAccessor{
+	_, err := s.service.getManagementCaveat(context.Background(), uri, SecretAccessor{
 		Kind: UnitAccessor,
 		ID:   "mariadb/0",
 	}, token)
 	c.Assert(err, jc.ErrorIsNil)
 }
 
-func (s *serviceSuite) TestCanManageLeaderUnitAppSecret(c *gc.C) {
+func (s *serviceSuite) TestGetManagementCaveatLeaderUnitAppSecret(c *gc.C) {
 	ctrl := s.setupMocks(c)
 	defer ctrl.Finish()
 
@@ -52,14 +52,14 @@ func (s *serviceSuite) TestCanManageLeaderUnitAppSecret(c *gc.C) {
 	token := NewMockToken(ctrl)
 	token.EXPECT().Check().Return(nil)
 
-	err := s.service.canManage(context.Background(), uri, SecretAccessor{
+	_, err := s.service.getManagementCaveat(context.Background(), uri, SecretAccessor{
 		Kind: UnitAccessor,
 		ID:   "mariadb/0",
 	}, token)
 	c.Assert(err, jc.ErrorIsNil)
 }
 
-func (s *serviceSuite) TestCanManageUserSecrets(c *gc.C) {
+func (s *serviceSuite) TestGetManagementCaveatUserSecrets(c *gc.C) {
 	ctrl := s.setupMocks(c)
 	defer ctrl.Finish()
 
@@ -72,7 +72,7 @@ func (s *serviceSuite) TestCanManageUserSecrets(c *gc.C) {
 
 	token := NewMockToken(ctrl)
 
-	err := s.service.canManage(context.Background(), uri, SecretAccessor{
+	_, err := s.service.getManagementCaveat(context.Background(), uri, SecretAccessor{
 		Kind: ModelAccessor,
 		ID:   "model-uuid",
 	}, token)

domain/secret/service/delete.go

@@ -6,17 +6,16 @@ package service
 import (
 	"context"
 
-	"github.com/juju/errors"
-
 	"github.com/juju/juju/core/secrets"
 	"github.com/juju/juju/domain"
+	"github.com/juju/juju/internal/errors"
 )
 
 // DeleteObsoleteUserSecretRevisions deletes any obsolete user secret revisions that are marked as auto-prune.
 func (s *SecretService) DeleteObsoleteUserSecretRevisions(ctx context.Context) error {
 	deletedRevisionIDs, err := s.secretState.DeleteObsoleteUserSecretRevisions(ctx)
 	if err != nil {
-		return errors.Trace(err)
+		return errors.Capture(err)
 	}
 	if err = s.secretBackendState.RemoveSecretBackendReference(ctx, deletedRevisionIDs...); err != nil {
 		// We don't want to error out if we can't remove the backend reference.
@@ -29,13 +28,19 @@ func (s *SecretService) DeleteObsoleteUserSecretRevisions(ctx context.Context) e
 // If revisions is nil or the last remaining revisions are removed.
 // It returns [secreterrors.PermissionDenied] if the secret cannot be managed by the accessor.
 func (s *SecretService) DeleteSecret(ctx context.Context, uri *secrets.URI, params DeleteSecretParams) error {
-	if err := s.canManage(ctx, uri, params.Accessor, params.LeaderToken); err != nil {
-		return errors.Trace(err)
-	}
-	if err := s.secretState.RunAtomic(ctx, func(ctx domain.AtomicContext) error {
-		return s.secretState.DeleteSecret(ctx, uri, params.Revisions)
-	}); err != nil {
-		return errors.Annotatef(err, "deleting secret %q", uri.ID)
+	withCaveat, err := s.getManagementCaveat(ctx, uri, params.Accessor, params.LeaderToken)
+	if err != nil {
+		return errors.Capture(err)
 	}
-	return nil
+
+	return withCaveat(ctx, func(innerCtx context.Context) error {
+		// TODO (manadart 2024-11-29): This context naming is nasty,
+		// but will be removed with RunAtomic.
+		if err := s.secretState.RunAtomic(innerCtx, func(innerInnerCtx domain.AtomicContext) error {
+			return s.secretState.DeleteSecret(innerInnerCtx, uri, params.Revisions)
+		}); err != nil {
+			return errors.Errorf("deleting secret %q: %w", uri.ID, err)
+		}
+		return nil
+	})
 }