Merge pull request #15709 from wallyworld/hook-secret-revison

#15709

When creating the hook content for secret hooks, we were only setting the revision if the hook was secret-remove. However, it also needs to be set for secret-expired.

The logic was a little wrong - it has been changed so that if the hook info has revision set, it will always be used for any secret hook. We were also incorrectly setting revision for the secret-changed hook, so this has been fixed.

## QA steps

Deploy a charm and create a secret which expires in a minute.
Echo the JUJU_SECRET_REVISION env var in the secret-expired hook and see that it is set correctly.

## Bug reference

https://bugs.launchpad.net/juju/+bug/2023120

worker/uniter/hook/hook.go

@@ -58,6 +58,11 @@ type Info struct {
 	SecretLabel string `yaml:"secret-label,omitempty"`
 }
 
+// SecretHookRequiresRevision returns true if the hook context needs a secret revision.
+func SecretHookRequiresRevision(kind hooks.Kind) bool {
+	return kind == hooks.SecretRemove || kind == hooks.SecretExpired
+}
+
 // Validate returns an error if the info is not valid.
 func (hi Info) Validate() error {
 	switch hi.Kind {
@@ -108,7 +113,7 @@ func (hi Info) Validate() error {
 		if _, err := secrets.ParseURI(hi.SecretURI); err != nil {
 			return errors.Errorf("invalid secret URI %q", hi.SecretURI)
 		}
-		if (hi.Kind == hooks.SecretRemove || hi.Kind == hooks.SecretExpired) && hi.SecretRevision <= 0 {
+		if SecretHookRequiresRevision(hi.Kind) && hi.SecretRevision <= 0 {
 			return errors.Errorf("%q hook requires a secret revision", hi.Kind)
 		}
 		return nil

worker/uniter/operation/runhook.go

@@ -52,7 +52,7 @@ func (rh *runHook) String() string {
 	case rh.info.Kind.IsStorage():
 		suffix = fmt.Sprintf(" (%s)", rh.info.StorageId)
 	case rh.info.Kind.IsSecret():
-		if rh.info.SecretRevision == 0 {
+		if rh.info.SecretRevision == 0 || !hook.SecretHookRequiresRevision(rh.info.Kind) {
 			suffix = fmt.Sprintf(" (%s)", rh.info.SecretURI)
 		} else {
 			suffix = fmt.Sprintf(" (%s/%d)", rh.info.SecretURI, rh.info.SecretRevision)
@@ -121,7 +121,7 @@ func RunningHookMessage(hookName string, info hook.Info) string {
 	}
 	if info.Kind.IsSecret() {
 		revMsg := ""
-		if info.SecretRevision > 0 {
+		if info.SecretRevision > 0 && hook.SecretHookRequiresRevision(info.Kind) {
 			revMsg = fmt.Sprintf("/%d", info.SecretRevision)
 		}
 		return fmt.Sprintf("running %s hook for %s%s", hookName, info.SecretURI, revMsg)

worker/uniter/runner/context/context.go

@@ -1795,3 +1795,10 @@ func (ctx *HookContext) SecretURI() (string, error) {
 func (ctx *HookContext) SecretLabel() string {
 	return ctx.secretLabel
 }
+
+// SecretRevision returns the secret revision for secret hooks.
+// This is not yet used by any hook commands - it is exported
+// for tests to use.
+func (ctx *HookContext) SecretRevision() int {
+	return ctx.secretRevision
+}

worker/uniter/runner/context/contextfactory.go

@@ -294,7 +294,7 @@ func (f *contextFactory) HookContext(hookInfo hook.Info) (*HookContext, error) {
 	if hookInfo.Kind.IsSecret() {
 		ctx.secretURI = hookInfo.SecretURI
 		ctx.secretLabel = hookInfo.SecretLabel
-		if hookInfo.Kind == hooks.SecretRemove {
+		if hook.SecretHookRequiresRevision(hookInfo.Kind) {
 			ctx.secretRevision = hookInfo.SecretRevision
 		}
 		if ctx.secretLabel == "" {

worker/uniter/runner/context/contextfactory_test.go

@@ -317,14 +317,18 @@ func (s *ContextFactorySuite) TestNewHookContextWithStorage(c *gc.C) {
 
 func (s *ContextFactorySuite) TestSecretHookContext(c *gc.C) {
 	hi := hook.Info{
-		Kind:        hooks.SecretRotate,
-		SecretURI:   "secret:9m4e2mr0ui3e8a215n4g",
-		SecretLabel: "label",
+		// Kind can be any secret hook kind.
+		// Whatever attributes are set below will
+		// be added to the context.
+		Kind:           hooks.SecretExpired,
+		SecretURI:      "secret:9m4e2mr0ui3e8a215n4g",
+		SecretLabel:    "label",
+		SecretRevision: 666,
 	}
 	ctx, err := s.factory.HookContext(hi)
 	c.Assert(err, jc.ErrorIsNil)
 	s.AssertCoreContext(c, ctx)
-	s.AssertSecretContext(c, ctx, hi.SecretURI, hi.SecretLabel)
+	s.AssertSecretContext(c, ctx, hi.SecretURI, hi.SecretLabel, hi.SecretRevision)
 	s.AssertNotWorkloadContext(c, ctx)
 	s.AssertNotActionContext(c, ctx)
 	s.AssertNotRelationContext(c, ctx)

worker/uniter/runner/context/util_test.go

@@ -369,10 +369,11 @@ func (s *HookContextSuite) AssertNotWorkloadContext(c *gc.C, ctx *runnercontext.
 	c.Assert(workloadName, gc.Equals, "")
 }
 
-func (s *HookContextSuite) AssertSecretContext(c *gc.C, ctx *runnercontext.HookContext, secretURI, label string) {
+func (s *HookContextSuite) AssertSecretContext(c *gc.C, ctx *runnercontext.HookContext, secretURI, label string, revision int) {
 	uri, _ := ctx.SecretURI()
 	c.Assert(uri, gc.Equals, secretURI)
 	c.Assert(ctx.SecretLabel(), gc.Equals, label)
+	c.Assert(ctx.SecretRevision(), gc.Equals, revision)
 }
 
 func (s *HookContextSuite) AssertNotSecretContext(c *gc.C, ctx *runnercontext.HookContext) {

worker/uniter/secrets/resolver_test.go

@@ -282,7 +282,7 @@ func (s *changeSecretsSuite) TestNextOpNoneExisting(c *gc.C) {
 	}
 	op, err := s.resolver.NextOp(localState, s.remoteState, s.opFactory)
 	c.Assert(err, jc.ErrorIsNil)
-	c.Assert(op.String(), gc.Equals, "run secret-changed (secret:9m4e2mr0ui3e8a215n4g/666) hook")
+	c.Assert(op.String(), gc.Equals, "run secret-changed (secret:9m4e2mr0ui3e8a215n4g) hook")
 }
 
 func (s *changeSecretsSuite) TestNextOpUpdatedRevision(c *gc.C) {
@@ -302,7 +302,7 @@ func (s *changeSecretsSuite) TestNextOpUpdatedRevision(c *gc.C) {
 	}
 	op, err := s.resolver.NextOp(localState, s.remoteState, s.opFactory)
 	c.Assert(err, jc.ErrorIsNil)
-	c.Assert(op.String(), gc.Equals, "run secret-changed (secret:9m4e2mr0ui3e8a215n4g/666) hook")
+	c.Assert(op.String(), gc.Equals, "run secret-changed (secret:9m4e2mr0ui3e8a215n4g) hook")
 }
 
 func (s *changeSecretsSuite) TestNextOpNone(c *gc.C) {