feat: refactor secret service and secret backend service to use the new atomic run #18106

ycliuhw · 2024-09-17T08:59:37Z

This PR refactored secret and secretbackend domain packages to run state calls in the same transaction using the new atomic run.
No feature changes.

Checklist

Code style: imports ordered, good names, simple structure, etc
Comments saying why design decisions were made
Go unit tests, with comments saying what you're testing
~~Integration tests, with comments saying what you're testing~~
~~doc.go added or updated in changed packages~~

QA steps

unit tests and bash tests.

go test -race -v ./domain/secret/... -test.timeout=600s -check.v

go test -race -v ./domain/secretbackend/... -test.timeout=600s -check.v

bash -ex ./main.sh -v -c aws -p ec2 -x output.txt -s '""' secrets_iaas test_secrets_vault

bash -ex ./main.sh -v -c microk8s -p k8s -x output.txt -s '""' secrets_k8s test_user_secrets

Documentation changes

No

Links

Jira card: JUJU-6268

…ice;

…end service;

…ed methods;

wallyworld

The permission checks can't be done inside the txn; to clarify - the calls to check leadership must be done outside the txn. The rest of the db lookups can occur inside the txn.

domain/secret/service/access.go

domain/secret/service/service_test.go

domain/secretbackend/state/package_test.go

domain/secret/service/consume.go

domain/secret/service/service.go

…inside transaction;

wallyworld

A few questions to discuss.

domain/secret/service/access.go

domain/secret/service/exportimport.go

domain/secret/service/service.go

domain/secretbackend/state/state_test.go

domain/secretbackend/state/package_test.go

wallyworld

Thanks for progress. Couple of additional comments.

domain/secretbackend/state/package_test.go

domain/secretbackend/service/modelsecretbackendservice.go

domain/secret/types.go

domain/secret/service/exportimport.go

domain/secret/service/service.go

wallyworld

Thank you. Just a few small things and a couple of things to follow up on. We can land this and make progress.

domain/secret/service/exportimport.go

domain/secret/service/service.go

domain/secret/types.go

domain/secret/state/state.go

domain/secret/state/types.go

SimonRichardson

I want @manadart to approve this before landing.

manadart

Some comments based on a once over.

There is a lot to be done here to get what I would view as correct use of RunAtomic.

This patch is too big. I would attack one service method at a time rather than trying to do everything at once and needing rounds of follow-ups.

manadart · 2024-09-30T17:39:05Z

domain/secret/state/state.go

-	}
-
-	err = db.Txn(ctx, func(ctx context.Context, tx *sqlair.TX) error {
+	return domain.Run(ctx, func(ctx context.Context, tx *sqlair.TX) error {
 		if err := st.createSecret(ctx, tx, version, uri, secret, st.checkUserSecretLabelExists); err != nil {


The closure passed here does validation that should be invoked at the service layer.

manadart · 2024-09-30T17:44:04Z

domain/secret/state/state.go

 	label := ""
 	if secret.Label != nil {
 		label = *secret.Label
 	}
-	err = db.Txn(ctx, func(ctx context.Context, tx *sqlair.TX) error {
+	return domain.Run(ctx, func(ctx context.Context, tx *sqlair.TX) error {


The UUID by name retrieval in this block should be done up in the service.

manadart · 2024-09-30T17:45:58Z

domain/secret/state/state.go

 	label := ""
 	if secret.Label != nil {
 		label = *secret.Label
 	}
-	err = db.Txn(ctx, func(ctx context.Context, tx *sqlair.TX) error {
+	return domain.Run(ctx, func(ctx context.Context, tx *sqlair.TX) error {
 		dbSecretOwner := secretUnitOwner{SecretID: uri.ID, Label: label}

 		selectUnitUUID := `SELECT &unit.uuid FROM unit WHERE name=$unit.name`


manadart · 2024-09-30T17:46:48Z

domain/secret/state/state.go

 }

 // UpdateSecret creates a secret with the specified parameters, returning an
 // error satisfying [secreterrors.SecretNotFound] if the secret does not exist.
 // It also returns an error satisfying [secreterrors.SecretLabelAlreadyExists]
 // if the secret owner already has a secret with the same label.
 func (st State) UpdateSecret(
-	ctx context.Context, uri *coresecrets.URI, secret domainsecret.UpsertSecretParams,
+	ctx domain.AtomicContext, uri *coresecrets.URI, secret domainsecret.UpsertSecretParams,
 ) error {
 	if !secret.HasUpdate() {


Validation for the service layer.

manadart · 2024-09-30T17:55:00Z

domain/secret/state/state.go

+		}
+		revisionResult = make([][]*domainsecret.SecretRevision, len(secrets))
+		for i, secret := range secrets {
+			revisionResult[i], err = st.listSecretRevisionsWithData(ctx, tx, secret.URI)


Potato. Surely we can get this all at once.

ycliuhw added 6 commits September 17, 2024 18:54

feat: change some secret state methods to accept atomic context;

b8f3c05

build: generate mocks for secret service;

fe86d27

feat: make state calls in same transaction if possible in secret serv…

a791be5

…ice;

feat: change some secret backend state methods to accept atomic context;

8571ab2

build: generate mocks for secret backend service;

c72bb47

feat: make state calls in same transaction if possible in secret back…

f1fc3ec

…end service;

ycliuhw added do not merge Even if a PR has been approved, do not merge the PR! 4.0 labels Sep 17, 2024

ycliuhw changed the title ~~Refactor secret service atomic~~ feat: refactor secret service and secret backend service to use the new atomic run Sep 17, 2024

ycliuhw requested review from wallyworld and hpidcock September 18, 2024 07:36

ycliuhw removed the do not merge Even if a PR has been approved, do not merge the PR! label Sep 18, 2024

ycliuhw marked this pull request as ready for review September 18, 2024 07:36

feat: make state calls in same transaction for secret consuming relat…

c6bdbf0

…ed methods;

ycliuhw force-pushed the refactor-secret-service-atomic branch from 8ae4db2 to c6bdbf0 Compare September 18, 2024 23:07

wallyworld requested changes Sep 19, 2024

View reviewed changes

ycliuhw added 2 commits September 19, 2024 16:42

fix: we should not run atomic transaction for permission checks;

4ce35a4

fix: change those atomic helper methods to func;

358ba9d

ycliuhw force-pushed the refactor-secret-service-atomic branch from 18f6983 to 358ba9d Compare September 19, 2024 07:00

feat: do leadership check outside of transaction but do access check …

cc9f0cf

…inside transaction;

wallyworld reviewed Sep 23, 2024

View reviewed changes

feat: add new methods for secret export for improving performance;

cac00ee

wallyworld reviewed Sep 25, 2024

View reviewed changes

feat: export secret information in same transaction;

41d4a61

wallyworld approved these changes Sep 26, 2024

View reviewed changes

feat: import secret in same transaction;

d623783

ycliuhw force-pushed the refactor-secret-service-atomic branch from bcd4bf0 to 0e59fb7 Compare September 26, 2024 23:21

build: re-generate mocks for secretsmanager facade package;

82e2835

ycliuhw force-pushed the refactor-secret-service-atomic branch from 0e59fb7 to 82e2835 Compare September 26, 2024 23:25

fix: remove unused test helper funcs in domain secret package;

a8e1f1a

SimonRichardson requested changes Sep 27, 2024

View reviewed changes

ycliuhw requested a review from manadart September 30, 2024 05:42

manadart reviewed Sep 30, 2024

View reviewed changes

hpidcock added the has merge conflicts label Oct 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: refactor secret service and secret backend service to use the new atomic run #18106

feat: refactor secret service and secret backend service to use the new atomic run #18106

ycliuhw commented Sep 17, 2024 •

edited by jira bot

Loading

wallyworld left a comment •

edited

Loading

wallyworld left a comment

wallyworld left a comment

wallyworld left a comment

SimonRichardson left a comment

manadart left a comment

manadart Sep 30, 2024

manadart Sep 30, 2024

manadart Sep 30, 2024

manadart Sep 30, 2024

manadart Sep 30, 2024

feat: refactor secret service and secret backend service to use the new atomic run #18106

Are you sure you want to change the base?

feat: refactor secret service and secret backend service to use the new atomic run #18106

Conversation

ycliuhw commented Sep 17, 2024 • edited by jira bot Loading

Checklist

QA steps

Documentation changes

Links

wallyworld left a comment • edited Loading

Choose a reason for hiding this comment

wallyworld left a comment

Choose a reason for hiding this comment

wallyworld left a comment

Choose a reason for hiding this comment

wallyworld left a comment

Choose a reason for hiding this comment

SimonRichardson left a comment

Choose a reason for hiding this comment

manadart left a comment

Choose a reason for hiding this comment

manadart Sep 30, 2024

Choose a reason for hiding this comment

manadart Sep 30, 2024

Choose a reason for hiding this comment

manadart Sep 30, 2024

Choose a reason for hiding this comment

manadart Sep 30, 2024

Choose a reason for hiding this comment

manadart Sep 30, 2024

Choose a reason for hiding this comment

ycliuhw commented Sep 17, 2024 •

edited by jira bot

Loading

wallyworld left a comment •

edited

Loading