Merge pull request juju#17698 from wallyworld/fix-user-secrets

juju#17698

A previous patch made secret updates idempotent

But there was a logic error updating user secrets. This fixes that by fixing the logic used to update the secret.

It also tweaks the bash test scripts to allow easier testing on k8s. If you specify bootstrap provider as k8s, it will default to using microk8s. 

## QA

./main.sh -v -p k8s secrets_k8s

## Links

Launchpad bug: https://bugs.launchpad.net/juju/+bug/2069238

Jira card: [JUJU-6225](https://warthogs.atlassian.net/browse/JUJU-6225)



[JUJU-6225]: https://warthogs.atlassian.net/browse/JUJU-6225?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ

apiserver/facades/client/secrets/secrets.go

@@ -471,7 +471,9 @@ func (s *SecretsAPI) updateSecret(backend provider.SecretsBackend, arg params.Up
 			}
 		}
 	}
-	if arg.AutoPrune == nil && arg.Description == nil && arg.Label == nil && len(arg.Content.Data) == 0 {
+
+	// There might be nothing to update if the checksums matched above.
+	if !arg.HasUpdate() {
 		return nil
 	}
 

apiserver/facades/client/secrets/secrets_test.go

@@ -563,6 +563,61 @@ func (s *SecretsSuite) TestUpdateSecretsSameChecksum(c *gc.C) {
 	s.assertUpdateSecrets(c, coresecrets.NewURI(), true, "7a38bf81f383f69433ad6e900d35b3e2385593f76a7b7ab5d4355b8ba41ee24b", false)
 }
 
+func (s *SecretsSuite) TestUpdateSecretJustContent(c *gc.C) {
+	defer s.setup(c).Finish()
+
+	s.expectAuthClient()
+	s.authorizer.EXPECT().HasPermission(permission.WriteAccess, coretesting.ModelTag).Return(nil)
+
+	uri := coresecrets.NewURI()
+	uriString := uri.String()
+
+	s.secretsState.EXPECT().GetSecret(uri).Return(&coresecrets.SecretMetadata{
+		URI:                    uri,
+		LatestRevision:         2,
+		LatestRevisionChecksum: "deadbeef",
+	}, nil)
+	s.secretsBackend.EXPECT().SaveContent(gomock.Any(), uri, 3, coresecrets.NewSecretValue(map[string]string{"foo": "bar"})).
+		Return("rev-id", nil)
+	s.secretsState.EXPECT().UpdateSecret(gomock.Any(), gomock.Any()).DoAndReturn(func(arg1 *coresecrets.URI, params state.UpdateSecretParams) (*coresecrets.SecretMetadata, error) {
+		c.Assert(arg1, gc.DeepEquals, uri)
+		c.Assert(params.ValueRef, gc.DeepEquals, &coresecrets.ValueRef{
+			BackendID:  "backend-id",
+			RevisionID: "rev-id",
+		})
+		c.Assert(params.Data, gc.IsNil)
+		c.Assert(params.Checksum, gc.Equals, "7a38bf81f383f69433ad6e900d35b3e2385593f76a7b7ab5d4355b8ba41ee24b")
+		result := &coresecrets.SecretMetadata{URI: uri, LatestRevision: 3}
+		if params.AutoPrune != nil {
+			result.AutoPrune = *params.AutoPrune
+		}
+		return result, nil
+	})
+
+	facade, err := apisecrets.NewTestAPI(s.authTag, s.authorizer, s.secretsState, s.secretConsumer,
+		adminBackendConfigGetter, backendConfigGetterForUserSecretsWrite(c),
+		func(cfg *provider.ModelBackendConfig) (provider.SecretsBackend, error) {
+			c.Assert(cfg.Config, jc.DeepEquals, provider.ConfigAttrs{"foo": cfg.BackendType})
+			return s.secretsBackend, nil
+		})
+	c.Assert(err, jc.ErrorIsNil)
+
+	result, err := facade.UpdateSecrets(params.UpdateUserSecretArgs{
+		Args: []params.UpdateUserSecretArg{
+			{
+				URI: uriString,
+				UpsertSecretArg: params.UpsertSecretArg{
+					Content: params.SecretContentParams{
+						Data: map[string]string{"foo": "bar"},
+					},
+				},
+			},
+		},
+	})
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(result.Results[0].Error, gc.IsNil)
+}
+
 func (s *SecretsSuite) TestRemoveSecrets(c *gc.C) {
 	defer s.setup(c).Finish()
 	s.expectAuthClient()

rpc/params/secrets.go

@@ -137,7 +137,7 @@ type UpdateUserSecretArg struct {
 
 // Validate validates the UpdateUserSecretArg.
 func (arg UpdateUserSecretArg) Validate() error {
-	if arg.AutoPrune == nil && arg.Description == nil && arg.Label == nil && len(arg.Content.Data) == 0 {
+	if !arg.HasUpdate() {
 		return errors.New("at least one attribute to update must be specified")
 	}
 	if arg.URI == "" && arg.ExistingLabel == "" {
@@ -149,6 +149,13 @@ func (arg UpdateUserSecretArg) Validate() error {
 	return nil
 }
 
+// HasUpdate returns true if arg contains at least one attribute to update.
+func (arg UpdateUserSecretArg) HasUpdate() bool {
+	return arg.AutoPrune != nil || arg.Description != nil || arg.Label != nil ||
+		arg.RotatePolicy != nil || arg.ExpireTime != nil ||
+		len(arg.Content.Data) != 0 || arg.Content.ValueRef != nil
+}
+
 // DeleteSecretArgs holds args for deleting secrets.
 type DeleteSecretArgs struct {
 	Args []DeleteSecretArg `json:"args"`

tests/includes/controller.sh

@@ -11,7 +11,7 @@ bootstrap_alt_controller() {
 	unset BOOTSTRAP_ADDITIONAL_ARGS
 
 	file="${TEST_DIR}/${name}.log"
-	juju_bootstrap "${BOOTSTRAP_CLOUD}" "${name}" "misc" "${file}"
+	juju_bootstrap "${BOOTSTRAP_CLOUD:-localhost}" "${name}" "misc" "${file}"
 
 	END_TIME=$(date +%s)
 	echo "====> Bootstrapped ${name} ($((END_TIME - START_TIME))s)"

tests/includes/juju.sh

@@ -72,8 +72,15 @@ bootstrap() {
 	"lxd")
 		cloud="${BOOTSTRAP_CLOUD:-localhost}"
 		;;
-	"vsphere" | "openstack" | "k8s" | "maas")
+	"vsphere" | "openstack" | "maas")
 		cloud="${BOOTSTRAP_CLOUD}"
+		if [[ -z ${cloud} ]]; then
+			echo "must specify cloud to bootstrap for provider ${BOOTSTRAP_PROVIDER}"
+			exit 1
+		fi
+		;;
+	"k8s")
+		cloud="${BOOTSTRAP_CLOUD:-microk8s}"
 		;;
 	"manual")
 		manual_name=${1}

tests/main.sh

@@ -6,7 +6,7 @@ export SHELLCHECK_OPTS="-e SC2230 -e SC2039 -e SC2028 -e SC2002 -e SC2005 -e SC2
 export BOOTSTRAP_REUSE_LOCAL="${BOOTSTRAP_REUSE_LOCAL:-}"
 export BOOTSTRAP_REUSE="${BOOTSTRAP_REUSE:-false}"
 export BOOTSTRAP_PROVIDER="${BOOTSTRAP_PROVIDER:-lxd}"
-export BOOTSTRAP_CLOUD="${BOOTSTRAP_CLOUD:-localhost}"
+export BOOTSTRAP_CLOUD="${BOOTSTRAP_CLOUD:-}"
 export BOOTSTRAP_SERIES="${BOOTSTRAP_SERIES:-}"
 export BOOTSTRAP_ARCH="${BOOTSTRAP_ARCH:-}"
 export BUILD_ARCH="${BUILD_ARCH:-}"

tests/suites/secrets_iaas/juju.sh

@@ -58,7 +58,7 @@ check_secrets() {
 	echo "Set different content for $secret_owned_by_easyrsa."
 	juju exec --unit easyrsa/0 -- secret-set "$secret_owned_by_easyrsa_id" foo=bar
 	check_contains "$(juju exec --unit easyrsa/0 -- secret-info-get "$secret_owned_by_easyrsa" --format json | jq ".${secret_owned_by_easyrsa_id}.revision")" '2'
-	check_contains "$(juju exec --unit easyrsa/0 -- secret-get "$secret_owned_by_easyrsa")" 'foo: bar'
+	check_contains "$(juju exec --unit easyrsa/0 -- secret-get --refresh "$secret_owned_by_easyrsa")" 'foo: bar'
 
 	echo "Checking: secret-revoke by relation ID"
 	juju exec --unit easyrsa/0 -- secret-revoke "$secret_owned_by_easyrsa" --relation "$relation_id"
@@ -91,7 +91,7 @@ run_user_secrets() {
 
 	# set same content again for revision 1.
 	juju --show-log update-secret "$secret_uri" owned-by="$model_name-1"
-	check_contains "$(juju --show-log show-secret "$secret_uri" --revisions | yq ".${secret_short_uri}.description")" 'info'
+	check_contains "$(juju --show-log show-secret "$secret_uri" --revisions | yq ".${secret_short_uri}.description")" 'this is a user secret'
 	check_contains "$(juju --show-log show-secret "$secret_uri" | yq ".${secret_short_uri}.revision")" '1'
 
 	# create a new revision 2.

tests/suites/secrets_k8s/k8s.sh

@@ -1,8 +1,6 @@
 run_secrets() {
 	echo
 
-	microk8s enable ingress
-
 	model_name='model-secrets-k8s'
 	juju --show-log add-model "$model_name" --config secret-backend=auto
 
@@ -103,8 +101,6 @@ run_secrets() {
 run_user_secrets() {
 	echo
 
-	microk8s enable ingress
-
 	model_name='model-user-secrets-k8s'
 	juju --show-log add-model "$model_name" --config secret-backend=auto
 	model_uuid=$(juju show-model $model_name --format json | jq -r ".[\"${model_name}\"][\"model-uuid\"]")

tests/suites/secrets_k8s/task.sh

@@ -6,6 +6,15 @@ test_secrets_k8s() {
 
 	set_verbosity
 
+	case "${BOOTSTRAP_PROVIDER:-}" in
+	"k8s")
+		microk8s enable ingress >/dev/null 2>&1 || true
+		;;
+	*)
+		echo "==> TEST SKIPPED: caas secrets tests, not a k8s provider"
+		;;
+	esac
+
 	echo "==> Checking for dependencies"
 	check_dependencies juju