Merge pull request juju#17181 from wallyworld/secret-consumer-logic

juju#17181

This PR add state and service logic for getting and saving secret consumer info. This is needed so that units can consumer secrets with the revision tracked.

The DDL needed to be changed as well. There were some unnecessary tables for application consumer which are not needed. It's also not necessary to use a uuid for the secret consumer tables as this is a join table and we just need the secret and unit uuids. And the associated triggers are removed since we'll be using a different approach to fire the relevant watchers.

The secret consumer table had a FK for the secret id. But for cross model secrets, the id does not exist in the model so the FK needs to be removed. But we have checks in the txn that the secret exists where needed (for local secrets).

In mongo, we needed to denormalise the latest revision and stick it in the consumer record. We don't do that now so there's a bit of fallout. The LatestRevision attr on SecretConsumerMetadata is not used any more except for old state code. For local secrets, we can look up the latest revision when needed. From model secrets, we'll need to add a table to store them. For now, just local secrets will work.

A small driveby improvement - rename the Revision attribute of the SecretRevisionInfo struct to LatestRevision.

## QA steps

bootstrap

```
$ juju add-secret mysecret --info "some secret" foo=bar
secret:cob1il8r4jm2b5mh01ng
$ juju exec -u controller/0 -- secret-get cob1il8r4jm2b5mh01ng
foo: bar
```

## Links

**Jira card:** JUJU-5855

api/agent/secretsmanager/client.go

@@ -142,8 +142,8 @@ func (c *Client) GetConsumerSecretsRevisionInfo(unitName string, uris []string)
 			return nil, errors.Annotatef(err, "finding latest info for secret %q", uris[i])
 		}
 		info[uris[i]] = coresecrets.SecretRevisionInfo{
-			Revision: latest.Revision,
-			Label:    latest.Label,
+			LatestRevision: latest.Revision,
+			Label:          latest.Label,
 		}
 	}
 	return info, err

apiserver/facades/agent/secretsmanager/secrets.go

@@ -21,6 +21,7 @@ import (
 	"github.com/juju/juju/core/permission"
 	coresecrets "github.com/juju/juju/core/secrets"
 	corewatcher "github.com/juju/juju/core/watcher"
+	secreterrors "github.com/juju/juju/domain/secret/errors"
 	secretservice "github.com/juju/juju/domain/secret/service"
 	"github.com/juju/juju/internal/secrets"
 	secretsprovider "github.com/juju/juju/internal/secrets/provider"
@@ -409,36 +410,36 @@ func (s *SecretsManagerAPI) GetConsumerSecretsRevisionInfo(ctx context.Context,
 		return params.SecretConsumerInfoResults{}, errors.Errorf("expected unit tag for consumer %q, got %T", consumerTag, consumerTag)
 	}
 	for i, uri := range args.URIs {
-		data, err := s.getSecretConsumerInfo(ctx, unitConsumer, uri)
+		data, latestRevision, err := s.getSecretConsumerInfo(ctx, unitConsumer, uri)
 		if err != nil {
 			result.Results[i].Error = apiservererrors.ServerError(err)
 			continue
 		}
 		result.Results[i] = params.SecretConsumerInfoResult{
-			Revision: data.LatestRevision,
+			Revision: latestRevision,
 			Label:    data.Label,
 		}
 	}
 	return result, nil
 }
 
-func (s *SecretsManagerAPI) getSecretConsumerInfo(ctx context.Context, unitTag names.UnitTag, uriStr string) (*coresecrets.SecretConsumerMetadata, error) {
+func (s *SecretsManagerAPI) getSecretConsumerInfo(ctx context.Context, unitTag names.UnitTag, uriStr string) (*coresecrets.SecretConsumerMetadata, int, error) {
 	uri, err := coresecrets.ParseURI(uriStr)
 	if err != nil {
-		return nil, errors.Trace(err)
+		return nil, 0, errors.Trace(err)
 	}
 	// We only check read permissions for local secrets.
 	// For CMR secrets, the remote model manages the permissions.
 	if uri.IsLocal(s.modelUUID) {
 		canRead, err := s.canRead(ctx, uri, unitTag)
 		if err != nil {
-			return nil, errors.Trace(err)
+			return nil, 0, errors.Trace(err)
 		}
 		if !canRead {
-			return nil, apiservererrors.ErrPerm
+			return nil, 0, apiservererrors.ErrPerm
 		}
 	}
-	return s.secretsConsumer.GetSecretConsumer(ctx, uri, unitTag.Id())
+	return s.secretsConsumer.GetSecretConsumerAndLatest(ctx, uri, unitTag.Id())
 }
 
 func secretOwnersFromAuthTag(authTag names.Tag, leadershipChecker leadership.Checker) ([]secretservice.CharmSecretOwner, error) {
@@ -583,7 +584,7 @@ func (s *SecretsManagerAPI) getRemoteSecretContent(ctx context.Context, uri *cor
 
 	unitName := s.authTag.Id()
 	consumerInfo, err := s.secretsConsumer.GetSecretConsumer(ctx, uri, unitName)
-	if err != nil && !errors.Is(err, errors.NotFound) {
+	if err != nil && !errors.Is(err, secreterrors.SecretConsumerNotFound) {
 		return nil, nil, false, errors.Trace(err)
 	}
 	var wantRevision int
@@ -623,7 +624,6 @@ func (s *SecretsManagerAPI) getRemoteSecretContent(ctx context.Context, uri *cor
 	}
 	if refresh || labelToUpdate != nil {
 		if refresh {
-			consumerInfo.LatestRevision = latestRevision
 			consumerInfo.CurrentRevision = latestRevision
 		}
 		if labelToUpdate != nil {

apiserver/facades/agent/secretsmanager/secrets_test.go

@@ -479,11 +479,10 @@ func (s *SecretsManagerSuite) TestGetConsumerSecretsRevisionInfoHavingConsumerLa
 
 	s.expectSecretAccessQuery(1)
 	uri := coresecrets.NewURI()
-	s.secretsConsumer.EXPECT().GetSecretConsumer(gomock.Any(), uri, "mariadb/0").Return(
+	s.secretsConsumer.EXPECT().GetSecretConsumerAndLatest(gomock.Any(), uri, "mariadb/0").Return(
 		&coresecrets.SecretConsumerMetadata{
-			LatestRevision: 666,
-			Label:          "label",
-		}, nil)
+			Label: "label",
+		}, 666, nil)
 
 	results, err := s.facade.GetConsumerSecretsRevisionInfo(context.Background(), params.GetSecretConsumerInfoArgs{
 		ConsumerTag: "unit-mariadb/0",
@@ -503,10 +502,10 @@ func (s *SecretsManagerSuite) TestGetConsumerSecretsRevisionInfoHavingNoConsumer
 
 	s.expectSecretAccessQuery(1)
 	uri := coresecrets.NewURI()
-	s.secretsConsumer.EXPECT().GetSecretConsumer(gomock.Any(), uri, "mariadb/0").Return(
+	s.secretsConsumer.EXPECT().GetSecretConsumerAndLatest(gomock.Any(), uri, "mariadb/0").Return(
 		&coresecrets.SecretConsumerMetadata{
-			LatestRevision: 666,
-		}, nil)
+			CurrentRevision: 665,
+		}, 666, nil)
 
 	results, err := s.facade.GetConsumerSecretsRevisionInfo(context.Background(), params.GetSecretConsumerInfoArgs{
 		ConsumerTag: "unit-mariadb/0",
@@ -525,11 +524,11 @@ func (s *SecretsManagerSuite) TestGetConsumerSecretsRevisionInfoForPeerUnitsAcce
 
 	s.expectSecretAccessQuery(1)
 	uri := coresecrets.NewURI()
-	s.secretsConsumer.EXPECT().GetSecretConsumer(gomock.Any(), uri, "mariadb/0").Return(
+	s.secretsConsumer.EXPECT().GetSecretConsumerAndLatest(gomock.Any(), uri, "mariadb/0").Return(
 		&coresecrets.SecretConsumerMetadata{
-			LatestRevision: 666,
-			Label:          "owner-label",
-		}, nil)
+			CurrentRevision: 665,
+			Label:           "owner-label",
+		}, 666, nil)
 
 	results, err := s.facade.GetConsumerSecretsRevisionInfo(context.Background(), params.GetSecretConsumerInfoArgs{
 		ConsumerTag: "unit-mariadb/0",
@@ -1075,7 +1074,6 @@ func (s *SecretsManagerSuite) TestGetSecretContentCrossModelExistingConsumerRefr
 		}, 666, true, nil)
 
 	s.secretsConsumer.EXPECT().SaveSecretConsumer(gomock.Any(), uri, "mariadb/0", &coresecrets.SecretConsumerMetadata{
-		LatestRevision:  666,
 		CurrentRevision: 666,
 	})
 
@@ -1121,7 +1119,7 @@ func (s *SecretsManagerSuite) TestGetSecretContentCrossModelNewConsumer(c *gc.C)
 	s.remoteClient.EXPECT().Close().Return(nil)
 
 	s.crossModelState.EXPECT().GetToken(names.NewApplicationTag("mariadb")).Return("token", nil)
-	s.secretsConsumer.EXPECT().GetSecretConsumer(gomock.Any(), uri, "mariadb/0").Return(nil, errors.NotFoundf(""))
+	s.secretsConsumer.EXPECT().GetSecretConsumer(gomock.Any(), uri, "mariadb/0").Return(nil, secreterrors.SecretConsumerNotFound)
 	s.secretService.EXPECT().ProcessSecretConsumerLabel(gomock.Any(), "mariadb/0", uri, "", gomock.Any()).Return(uri, nil, nil)
 
 	s.remoteClient.EXPECT().GetSecretAccessScope(uri, "token", 0).Return("scope-token", nil)
@@ -1145,7 +1143,6 @@ func (s *SecretsManagerSuite) TestGetSecretContentCrossModelNewConsumer(c *gc.C)
 		}, 666, true, nil)
 
 	s.secretsConsumer.EXPECT().SaveSecretConsumer(gomock.Any(), uri, "mariadb/0", &coresecrets.SecretConsumerMetadata{
-		LatestRevision:  666,
 		CurrentRevision: 666,
 	})
 

apiserver/facades/agent/secretsmanager/service.go

@@ -23,6 +23,7 @@ type SecretTriggers interface {
 // SecretsConsumer instances provide secret consumer apis.
 type SecretsConsumer interface {
 	GetSecretConsumer(ctx context.Context, uri *secrets.URI, unitName string) (*secrets.SecretConsumerMetadata, error)
+	GetSecretConsumerAndLatest(ctx context.Context, uri *secrets.URI, unitName string) (*secrets.SecretConsumerMetadata, int, error)
 	GetURIByConsumerLabel(ctx context.Context, label string, unitName string) (*secrets.URI, error)
 	SaveSecretConsumer(ctx context.Context, uri *secrets.URI, unitName string, md *secrets.SecretConsumerMetadata) error
 	GetConsumedRevision(

apiserver/facades/client/secrets/secrets.go

@@ -436,11 +436,11 @@ func (s *SecretsAPI) secretURI(ctx context.Context, uriStr, label string) (*core
 	if uriStr != "" {
 		return coresecrets.ParseURI(uriStr)
 	}
-	md, err := s.secretService.GetUserSecretByLabel(ctx, label)
+	uri, err := s.secretService.GetUserSecretURIByLabel(ctx, label)
 	if err != nil {
 		return nil, errors.Annotatef(err, "getting user secret for label %q", label)
 	}
-	return md.URI, nil
+	return uri, nil
 }
 
 // RemoveSecrets isn't on the v1 API.

apiserver/facades/client/secrets/secrets_test.go

@@ -413,9 +413,7 @@ func (s *SecretsSuite) assertUpdateSecrets(c *gc.C, uri *coresecrets.URI, isInte
 	if uri == nil {
 		existingLabel = "my-secret"
 		uri = coresecrets.NewURI()
-		s.secretService.EXPECT().GetUserSecretByLabel(gomock.Any(), "my-secret").Return(&coresecrets.SecretMetadata{
-			URI: uri,
-		}, nil)
+		s.secretService.EXPECT().GetUserSecretURIByLabel(gomock.Any(), "my-secret").Return(uri, nil)
 	} else {
 		uriString = uri.String()
 	}
@@ -662,9 +660,7 @@ func (s *SecretsSuite) TestGrantSecretByName(c *gc.C) {
 	s.authorizer.EXPECT().HasPermission(permission.WriteAccess, coretesting.ModelTag).Return(nil)
 
 	uri := coresecrets.NewURI()
-	s.secretService.EXPECT().GetUserSecretByLabel(gomock.Any(), "my-secret").Return(&coresecrets.SecretMetadata{
-		URI: uri,
-	}, nil)
+	s.secretService.EXPECT().GetUserSecretURIByLabel(gomock.Any(), "my-secret").Return(uri, nil)
 	s.secretService.EXPECT().GrantSecretAccess(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(
 		func(_ context.Context, arg *coresecrets.URI, params secretservice.SecretAccessParams) error {
 			c.Assert(arg, gc.DeepEquals, uri)

apiserver/facades/client/secrets/service.go

@@ -21,7 +21,7 @@ type SecretService interface {
 	// View and fetch secrets.
 
 	GetSecret(ctx context.Context, uri *secrets.URI) (*secrets.SecretMetadata, error)
-	GetUserSecretByLabel(ctx context.Context, label string) (*secrets.SecretMetadata, error)
+	GetUserSecretURIByLabel(ctx context.Context, label string) (*secrets.URI, error)
 	GetSecretValue(context.Context, *secrets.URI, int) (secrets.SecretValue, *secrets.ValueRef, error)
 	ListSecrets(ctx context.Context, uri *secrets.URI,
 		revisions domainsecret.Revisions,

apiserver/facades/controller/crossmodelsecrets/crossmodelsecrets.go

@@ -246,9 +246,8 @@ func (s *CrossModelSecretsAPI) updateConsumedRevision(ctx stdcontext.Context, se
 		if consumerInfo == nil {
 			consumerInfo = &coresecrets.SecretConsumerMetadata{}
 		}
-		consumerInfo.LatestRevision = md.LatestRevision
 		consumerInfo.CurrentRevision = md.LatestRevision
-		if err := secretService.SaveSecretRemoteConsumer(ctx, uri, consumer.Id(), consumerInfo); err != nil {
+		if err := secretService.SaveSecretRemoteConsumer(ctx, uri, md.LatestRevision, consumer.Id(), consumerInfo); err != nil {
 			return 0, errors.Trace(err)
 		}
 	}

apiserver/facades/controller/crossmodelsecrets/crossmodelsecrets_test.go

@@ -181,9 +181,8 @@ func (s *CrossModelSecretsSuite) assertGetSecretContentInfo(c *gc.C, newConsumer
 	s.secretService.EXPECT().GetSecret(gomock.Any(), uri).Return(&coresecrets.SecretMetadata{
 		LatestRevision: 667,
 	}, nil)
-	s.secretService.EXPECT().SaveSecretRemoteConsumer(gomock.Any(), uri, "remote-app/666", &coresecrets.SecretConsumerMetadata{
+	s.secretService.EXPECT().SaveSecretRemoteConsumer(gomock.Any(), uri, 667, "remote-app/666", &coresecrets.SecretConsumerMetadata{
 		CurrentRevision: 667,
-		LatestRevision:  667,
 	}).Return(nil)
 	s.secretService.EXPECT().GetSecretAccess(gomock.Any(), uri, consumer).Return(coresecrets.RoleView, nil)
 	s.secretService.EXPECT().GetSecretValue(gomock.Any(), uri, 667).Return(

apiserver/facades/controller/crossmodelsecrets/service.go

@@ -16,7 +16,7 @@ type SecretService interface {
 	GetSecret(context.Context, *secrets.URI) (*secrets.SecretMetadata, error)
 	GetSecretValue(context.Context, *secrets.URI, int) (secrets.SecretValue, *secrets.ValueRef, error)
 	GetSecretRemoteConsumer(ctx context.Context, uri *secrets.URI, unitName string) (*secrets.SecretConsumerMetadata, error)
-	SaveSecretRemoteConsumer(ctx context.Context, uri *secrets.URI, unitName string, md *secrets.SecretConsumerMetadata) error
+	SaveSecretRemoteConsumer(ctx context.Context, uri *secrets.URI, latestRevision int, unitName string, md *secrets.SecretConsumerMetadata) error
 	GetSecretAccess(ctx context.Context, uri *secrets.URI, consumer secretservice.SecretAccessor) (secrets.SecretRole, error)
 	GetSecretAccessScope(ctx context.Context, uri *secrets.URI, accessor secretservice.SecretAccessor) (secretservice.SecretAccessScope, error)
 }

core/secrets/secret.go

@@ -236,14 +236,16 @@ type SecretConsumerMetadata struct {
 	// CurrentRevision is current revision the
 	// consumer wants to read.
 	CurrentRevision int
+
+	// TODO(secrets) - this will be removed
 	// LatestRevision is the latest secret revision.
 	LatestRevision int
 }
 
 // SecretRevisionInfo holds info used to read a secret vale.
 type SecretRevisionInfo struct {
-	Revision int
-	Label    string
+	LatestRevision int
+	Label          string
 }
 
 // Filter is used when querying secrets.

domain/schema/model.go

@@ -23,9 +23,6 @@ const (
 	tableSecretRotation
 	tableSecretRevisionObsolete
 	tableSecretRevisionExpire
-	tableSecretApplicationConsumerCurrentRevision
-	tableSecretUnitConsumerCurrentRevision
-	tableSecretRemoteApplicationConsumerCurrentRevision
 	tableSecretRemoteUnitConsumerCurrentRevision
 )
 
@@ -68,12 +65,6 @@ func ModelDDL() *schema.Schema {
 			"secret_revision", "uuid", "obsolete", tableSecretRevisionObsolete),
 		changeLogTriggersForTableOnColumn(
 			"secret_revision_expire", "revision_uuid", "next_expire_time", tableSecretRevisionExpire),
-		changeLogTriggersForTableOnColumn(
-			"secret_application_consumer", "uuid", "current_revision", tableSecretApplicationConsumerCurrentRevision),
-		changeLogTriggersForTableOnColumn(
-			"secret_unit_consumer", "uuid", "current_revision", tableSecretUnitConsumerCurrentRevision),
-		changeLogTriggersForTableOnColumn(
-			"secret_remote_application_consumer", "uuid", "current_revision", tableSecretRemoteApplicationConsumerCurrentRevision),
 		changeLogTriggersForTableOnColumn(
 			"secret_remote_unit_consumer", "uuid", "current_revision", tableSecretRemoteUnitConsumerCurrentRevision),
 
@@ -140,10 +131,8 @@ INSERT INTO change_log_namespace VALUES
     (10, 'secret_rotation', 'Secret rotation changes based on UUID'),
     (11, 'secret_revision', 'Secret revision obsolete changes based on UUID'),
     (12, 'secret_revision_expire', 'Secret revision next expire time changes based on UUID'),
-    (13, 'secret_application_consumer', 'Secret application consumer current revision changes based on UUID'),
-    (14, 'secret_unit_consumer', 'Secret unit consumer current revision changes based on UUID'),
-    (15, 'secret_remote_application_consumer', 'Secret remote application consumer current revision changes based on UUID'),
-    (16, 'secret_remote_unit_consumer', 'Secret remote unit consumer current revision changes based on UUID');
+    (13, 'secret_unit_consumer', 'Secret unit consumer current revision changes based on UUID'),
+    (14, 'secret_remote_unit_consumer', 'Secret remote unit consumer current revision changes based on UUID');
 `)
 }