Merge pull request juju#15815 from ycliuhw/fix/lp-2023364

juju#15815 This PR ensures we only mark obsolete revision if there is consumer revision change. ## 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 ```sh juju --show-log deploy hello-kubecon hello juju --show-log deploy nginx-ingress-integrator nginx juju --show-log integrate nginx hello juju --show-log trust nginx --scope=cluster juju exec --unit hello/0 -- secret-add --owner unit owned-by=hello/0 secret://42022b73-9739-4a66-890d-f4df231ce55f/cid6tkuffbas7af0e0eg juju exec --unit hello/0 -- secret-add owned-by=hello-app secret://42022b73-9739-4a66-890d-f4df231ce55f/cid6toeffbas7af0e0f0 juju exec --unit hello/0 -- secret-get cid6tkuffbas7af0e0eg owned-by: hello/0 juju exec --unit hello/0 -- secret-get cid6toeffbas7af0e0f0 owned-by: hello-app juju exec --unit hello/0 -- secret-info-get cid6tkuffbas7af0e0eg --format json {"cid6tkuffbas7af0e0eg":{"revision":1,"label":"","owner":"unit","rotation":"never"}} juju exec --unit hello/0 -- secret-info-get cid6toeffbas7af0e0f0 --format json {"cid6toeffbas7af0e0f0":{"revision":1,"label":"","owner":"application","rotation":"never"}} juju exec --unit hello/0 -- secret-grant cid6tkuffbas7af0e0eg -r 0 juju exec --unit hello/0 -- secret-grant cid6toeffbas7af0e0f0 -r 0 juju exec --unit nginx/0 -- secret-get cid6tkuffbas7af0e0eg owned-by: hello/0 juju exec --unit nginx/0 -- secret-get cid6toeffbas7af0e0f0 owned-by: hello-app juju:PRIMARY> db.secretRevisions.find().pretty() { "_id" : "42022b73-9739-4a66-890d-f4df231ce55f:cid6tkuffbas7af0e0eg/1", "txn-revno" : 2, "revision" : 1, "create-time" : ISODate("2023-06-27T05:08:36Z"), "update-time" : ISODate("2023-06-27T05:08:36Z"), "obsolete" : false, "data" : { }, "value-reference" : { "backend-id" : "42022b73-9739-4a66-890d-f4df231ce55f", "revision-id" : "cid6tkuffbas7af0e0eg-1" }, "pending-delete" : false, "owner-tag" : "unit-hello-0", "model-uuid" : "42022b73-9739-4a66-890d-f4df231ce55f" } { "_id" : "42022b73-9739-4a66-890d-f4df231ce55f:cid6toeffbas7af0e0f0/1", "txn-revno" : 2, "revision" : 1, "create-time" : ISODate("2023-06-27T05:08:51Z"), "update-time" : ISODate("2023-06-27T05:08:51Z"), "obsolete" : false, "data" : { }, "value-reference" : { "backend-id" : "42022b73-9739-4a66-890d-f4df231ce55f", "revision-id" : "cid6toeffbas7af0e0f0-1" }, "pending-delete" : false, "owner-tag" : "application-hello", "model-uuid" : "42022b73-9739-4a66-890d-f4df231ce55f" } juju exec --unit hello/0 -- secret-set cid6toeffbas7af0e0f0 owned-by=hello-app-updated juju show-status-log nginx/0 Time Type Status Message ... 27 Jun 2023 15:15:55+10:00 juju-unit executing running secret-changed hook for secret:cid6toeffbas7af0e0f0/2 27 Jun 2023 15:15:55+10:00 juju-unit idle 27 Jun 2023 15:16:53+10:00 juju-unit executing running action juju-exec 27 Jun 2023 15:16:55+10:00 juju-unit idle juju exec --unit nginx/0 -- secret-get cid6toeffbas7af0e0f0 owned-by: hello-app juju exec --unit nginx/0 -- secret-get cid6toeffbas7af0e0f0 --refresh owned-by: hello-app-updated db.secretRevisions.find().pretty() { "_id" : "42022b73-9739-4a66-890d-f4df231ce55f:cid6tkuffbas7af0e0eg/1", "txn-revno" : 2, "revision" : 1, "create-time" : ISODate("2023-06-27T05:08:36Z"), "update-time" : ISODate("2023-06-27T05:08:36Z"), "obsolete" : false, "data" : { }, "value-reference" : { "backend-id" : "42022b73-9739-4a66-890d-f4df231ce55f", "revision-id" : "cid6tkuffbas7af0e0eg-1" }, "pending-delete" : false, "owner-tag" : "unit-hello-0", "model-uuid" : "42022b73-9739-4a66-890d-f4df231ce55f" } { "_id" : "42022b73-9739-4a66-890d-f4df231ce55f:cid6toeffbas7af0e0f0/1", "txn-revno" : NumberLong(3), "revision" : 1, "create-time" : ISODate("2023-06-27T05:08:51Z"), "update-time" : ISODate("2023-06-27T05:08:51Z"), "obsolete" : true, "data" : { }, "value-reference" : { "backend-id" : "42022b73-9739-4a66-890d-f4df231ce55f", "revision-id" : "cid6toeffbas7af0e0f0-1" }, "pending-delete" : false, "owner-tag" : "application-hello", "model-uuid" : "42022b73-9739-4a66-890d-f4df231ce55f" } { "_id" : "42022b73-9739-4a66-890d-f4df231ce55f:cid6toeffbas7af0e0f0/2", "txn-revno" : 2, "revision" : 2, "create-time" : ISODate("2023-06-27T05:15:55Z"), "update-time" : ISODate("2023-06-27T05:15:55Z"), "obsolete" : false, "data" : { }, "value-reference" : { "backend-id" : "42022b73-9739-4a66-890d-f4df231ce55f", "revision-id" : "cid6toeffbas7af0e0f0-2" }, "pending-delete" : false, "owner-tag" : "application-hello", "model-uuid" : "42022b73-9739-4a66-890d-f4df231ce55f" } juju show-status-log hello/0 Time Type Status Message ... 27 Jun 2023 15:15:55+10:00 juju-unit executing running secret-changed hook for secret:cid6toeffbas7af0e0f0/2 27 Jun 2023 15:15:55+10:00 juju-unit idle 27 Jun 2023 15:16:13+10:00 juju-unit executing running action juju-exec 27 Jun 2023 15:16:13+10:00 juju-unit idle 27 Jun 2023 15:16:28+10:00 juju-unit executing running action juju-exec 27 Jun 2023 15:16:28+10:00 juju-unit idle 27 Jun 2023 15:18:16+10:00 juju-unit executing running secret-remove hook for secret:cid6toeffbas7af0e0f0/1 27 Jun 2023 15:18:16+10:00 juju-unit idle ``` ## Documentation changes No ## Bug reference https://bugs.launchpad.net/juju/+bug/2023364
SimonRichardson · Jun 27, 2023 · 47f9909 · 47f9909
2 parents 3778218 + 5197290
commit 47f9909
Show file tree

Hide file tree

Showing 3 changed files with 70 additions and 25 deletions.
diff --git a/state/package_test.go b/state/package_test.go
@@ -12,6 +12,7 @@ import (
 	gc "gopkg.in/check.v1"
 
 	"github.com/juju/juju/core/network"
+	"github.com/juju/juju/core/secrets"
 	coretesting "github.com/juju/juju/testing"
 )
 
@@ -113,3 +114,12 @@ func (st *State) ReadBackendRefCount(backendID string) (int, error) {
 	key := secretBackendRefCountKey(backendID)
 	return nsRefcounts.read(refCountCollection, key)
 }
+
+func (st *State) IsSecretRevisionObsolete(c *gc.C, uri *secrets.URI, rev int) bool {
+	col, closer := st.db().GetCollection(secretRevisionsC)
+	defer closer()
+	var doc secretRevisionDoc
+	err := col.FindId(secretRevisionKey(uri, rev)).One(&doc)
+	c.Assert(err, jc.ErrorIsNil)
+	return doc.Obsolete
+}
diff --git a/state/secrets.go b/state/secrets.go
@@ -1617,16 +1617,15 @@ func (st *State) SaveSecretConsumer(uri *secrets.URI, consumer names.Tag, metada
 					"current-revision": metadata.CurrentRevision,
 				}},
 			})
-		}
-
-		if localSecret {
-			// The consumer is tracking a new revision, which might result in the
-			// previous revision becoming obsolete.
-			obsoleteOps, err := st.markObsoleteRevisionOps(uri, consumer.String(), metadata.CurrentRevision)
-			if err != nil {
-				return nil, errors.Trace(err)
+			if localSecret && metadata.CurrentRevision > doc.CurrentRevision {
+				// The consumer is tracking a new revision, which might result in the
+				// previous revision becoming obsolete.
+				obsoleteOps, err := st.markObsoleteRevisionOps(uri, consumer.String(), metadata.CurrentRevision)
+				if err != nil {
+					return nil, errors.Trace(err)
+				}
+				ops = append(ops, obsoleteOps...)
 			}
-			ops = append(ops, obsoleteOps...)
 		}
 
 		return ops, nil
@@ -1683,16 +1682,17 @@ func (st *State) SaveSecretRemoteConsumer(uri *secrets.URI, consumer names.Tag,
 					"current-revision": metadata.CurrentRevision,
 				}},
 			})
+			if metadata.CurrentRevision > doc.CurrentRevision {
+				// The consumer is tracking a new revision, which might result in the
+				// previous revision becoming obsolete.
+				obsoleteOps, err := st.markObsoleteRevisionOps(uri, consumer.String(), metadata.CurrentRevision)
+				if err != nil {
+					return nil, errors.Trace(err)
+				}
+				ops = append(ops, obsoleteOps...)
+			}
 		}
 
-		// The consumer is tracking a new revision, which might result in the
-		// previous revision becoming obsolete.
-		obsoleteOps, err := st.markObsoleteRevisionOps(uri, consumer.String(), metadata.CurrentRevision)
-		if err != nil {
-			return nil, errors.Trace(err)
-		}
-		ops = append(ops, obsoleteOps...)
-
 		return ops, nil
 	}
 	return st.db().Run(buildTxn)

diff --git a/state/secrets_test.go b/state/secrets_test.go
@@ -1364,23 +1364,58 @@ func (s *SecretsSuite) TestSaveSecretConsumer(c *gc.C) {
 		},
 	}
 	uri := secrets.NewURI()
-	_, err := s.store.CreateSecret(uri, cp)
+	md, err := s.store.CreateSecret(uri, cp)
 	c.Assert(err, jc.ErrorIsNil)
-	md := &secrets.SecretConsumerMetadata{
+	c.Assert(md.LatestRevision, gc.Equals, 1)
+	c.Assert(s.State.IsSecretRevisionObsolete(c, uri, 1), jc.IsFalse)
+
+	cmd := &secrets.SecretConsumerMetadata{
 		Label:           "foobar",
-		CurrentRevision: 666,
+		CurrentRevision: md.LatestRevision,
+		LatestRevision:  md.LatestRevision,
 	}
-	err = s.State.SaveSecretConsumer(uri, names.NewUnitTag("mariadb/0"), md)
+	err = s.State.SaveSecretConsumer(uri, names.NewUnitTag("mariadb/0"), cmd)
 	c.Assert(err, jc.ErrorIsNil)
 	md2, err := s.State.GetSecretConsumer(uri, names.NewUnitTag("mariadb/0"))
 	c.Assert(err, jc.ErrorIsNil)
-	c.Assert(md2, jc.DeepEquals, md)
-	md.CurrentRevision = 668
-	err = s.State.SaveSecretConsumer(uri, names.NewUnitTag("mariadb/0"), md)
+	c.Assert(md2, jc.DeepEquals, cmd)
+	c.Assert(md2.LatestRevision, gc.Equals, 1)
+	c.Assert(md2.CurrentRevision, gc.Equals, 1)
+	c.Assert(s.State.IsSecretRevisionObsolete(c, uri, 1), jc.IsFalse)
+
+	// secret revison ++, but not obsolete.
+	md, err = s.store.UpdateSecret(uri, state.UpdateSecretParams{
+		LeaderToken: &fakeToken{},
+		Data:        map[string]string{"foo": "bar", "baz": "qux"},
+	})
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(md.LatestRevision, gc.Equals, 2)
+	c.Assert(s.State.IsSecretRevisionObsolete(c, uri, 1), jc.IsFalse)
+	c.Assert(s.State.IsSecretRevisionObsolete(c, uri, 2), jc.IsFalse)
+
+	// consumer latest revison ++, but not obsolete.
+	cmd.LatestRevision = md.LatestRevision
+	err = s.State.SaveSecretConsumer(uri, names.NewUnitTag("mariadb/0"), cmd)
 	c.Assert(err, jc.ErrorIsNil)
 	md2, err = s.State.GetSecretConsumer(uri, names.NewUnitTag("mariadb/0"))
 	c.Assert(err, jc.ErrorIsNil)
-	c.Assert(md2, jc.DeepEquals, md)
+	c.Assert(md2, jc.DeepEquals, cmd)
+	c.Assert(md2.LatestRevision, gc.Equals, 2)
+	c.Assert(md2.CurrentRevision, gc.Equals, 1)
+	c.Assert(s.State.IsSecretRevisionObsolete(c, uri, 1), jc.IsFalse)
+	c.Assert(s.State.IsSecretRevisionObsolete(c, uri, 2), jc.IsFalse)
+
+	// consumer current revison ++, then obsolete.
+	cmd.CurrentRevision = md.LatestRevision
+	err = s.State.SaveSecretConsumer(uri, names.NewUnitTag("mariadb/0"), cmd)
+	c.Assert(err, jc.ErrorIsNil)
+	md2, err = s.State.GetSecretConsumer(uri, names.NewUnitTag("mariadb/0"))
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(md2, jc.DeepEquals, cmd)
+	c.Assert(md2.LatestRevision, gc.Equals, 2)
+	c.Assert(md2.CurrentRevision, gc.Equals, 2)
+	c.Assert(s.State.IsSecretRevisionObsolete(c, uri, 1), jc.IsTrue)
+	c.Assert(s.State.IsSecretRevisionObsolete(c, uri, 2), jc.IsFalse)
 }
 
 func (s *SecretsSuite) TestSaveSecretConsumerConcurrent(c *gc.C) {