SecretStores advertise supported Features(). #2069

tmacam · 2022-09-12T22:52:28Z

Description

This PR is aimed at addressing issue #2047.

In the Secret API Documentation it is stated:

If a secret store has support for multiple keys in a secret, a JSON payload is returned with the key names as fields and their respective values.

In case of a secret store that only has name/value semantics, a JSON payload is returned with the name of the secret as the field and the value of the secret as the value.

There are two classes of secret stores but there isn't a way to tell them apart at run-time. This limits the ability of conformance tests to verify the behavior of secret stores supporting multiple keys.

We address this by augmenting SecretStores with the ability to advetise Features. This is similar to what PubSub and StateStores do. Feature MULTIPLE_KEY_VALUES_PER_SECRET was added and is advertised by Hashicorp Vault (default behaviour) and by Local File SecretStore (depending on its configuration).

Updated tests to account to new method and ensure expected behavior.

Issue reference

Please reference the issue this PR will close: #2047

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

Code compiles correctly
Created/updated tests
Extended the documentation / Created issue in the Update SecretStore API documentation to account for Features feature docs#2787

mcandeia · 2022-09-12T23:13:59Z

secretstores/feature.go

@@ -0,0 +1,33 @@
+/*
+Copyright 2021 The Dapr Authors


mcandeia · 2022-09-12T23:14:50Z

secretstores/feature.go

+)
+
+// Feature names a feature that can be implemented by Secret Store components.
+type Feature string


nit: invert the declaring order: Define type feature first then define by its values below

I was following the example from state/feature.go and pubsub/feature.go but yeah, you are right, it would look and read better inverted. Fixed in next revision.

mcandeia · 2022-09-12T23:16:06Z

secretstores/alicloud/parameterstore/parameterstore_test.go

+	s.Init(m)
+	t.Run("no features are advertised", func(t *testing.T) {
+		f := s.Features()
+		assert.Equal(t, 0, len(f))


nit:

Suggested change

assert.Equal(t, 0, len(f))

assert.Empty(t, f)

I am pushing a revision with this change.

mcandeia · 2022-09-12T23:16:18Z

secretstores/aws/parameterstore/parameterstore_test.go

+	// Yes, we are skipping initialization as feature retrieval doesn't depend on it.
+	t.Run("no features are advertised", func(t *testing.T) {
+		f := s.Features()
+		assert.Equal(t, 0, len(f))


Suggested change

assert.Equal(t, 0, len(f))

assert.Empty(t, f)

mcandeia · 2022-09-12T23:16:31Z

secretstores/aws/secretmanager/secretmanager_test.go

+	s := smSecretStore{}
+	t.Run("no features are advertised", func(t *testing.T) {
+		f := s.Features()
+		assert.Equal(t, 0, len(f))


nit

Suggested change

assert.Equal(t, 0, len(f))

assert.Empty(t, f)

mcandeia · 2022-09-12T23:17:03Z

secretstores/gcp/secretmanager/secretmanager_test.go

+	// Yes, we are skipping initialization as feature retrieval doesn't depend on it.
+	t.Run("no features are advertised", func(t *testing.T) {
+		f := s.Features()
+		assert.Equal(t, 0, len(f))


nit:

Suggested change

assert.Equal(t, 0, len(f))

assert.Empty(t, f)

mcandeia · 2022-09-12T23:18:45Z

secretstores/hashicorp/vault/vault.go

-		json:   jsoniter.ConfigFastest,
+		client:   &http.Client{},
+		logger:   logger,
+		features: []secretstores.Feature{secretstores.FeatureMultipleKeyValuesPerSecret},


is there a reason to keep the features as a property? I mean, if not, I think you can return straight the feature array on line 512

Not really. I thought this would be more aligned to what I saw on other implementations (state store ones) but I guess simpler is better here, I will update the code.

mcandeia · 2022-09-12T23:19:15Z

secretstores/huaweicloud/csms/csms_test.go

+	// Yes, we are skipping initialization as feature retrieval doesn't depend on it.
+	t.Run("no features are advertised", func(t *testing.T) {
+		f := s.Features()
+		assert.Equal(t, 0, len(f))


nit

Suggested change

assert.Equal(t, 0, len(f))

assert.Empty(t, f)

mcandeia · 2022-09-12T23:19:28Z

secretstores/kubernetes/kubernetes_test.go

+	// Yes, we are skipping initialization as feature retrieval doesn't depend on it.
+	t.Run("no features are advertised", func(t *testing.T) {
+		f := s.Features()
+		assert.Equal(t, 0, len(f))


nit:

Suggested change

assert.Equal(t, 0, len(f))

assert.Empty(t, f)

mcandeia · 2022-09-12T23:19:41Z

secretstores/local/env/envstore_test.go

+	// Yes, we are skipping initialization as feature retrieval doesn't depend on it.
+	t.Run("no features are advertised", func(t *testing.T) {
+		f := s.Features()
+		assert.Equal(t, 0, len(f))


nit:

Suggested change

assert.Equal(t, 0, len(f))

assert.Empty(t, f)

This PR is aimed at addressing issue dapr#2047. In the [Secret API Documentation](https://docs.dapr.io/reference/api/secrets_api/#response-body) it is stated: > If a secret store has support for multiple keys in a secret, a JSON payload is returned with the key names as fields and their respective values. > > In case of a secret store that only has name/value semantics, a JSON payload is returned with the name of the secret as the field and the value of the secret as the value. There are two classes of secret stores but there isn't a way to tell them apart at run-time. This limits the ability of conformance tests to verify the behavior of secret stores supporting multiple keys. We address this by augmenting SecretStores with the ability to advetise `Features`. This is similar to what PubSub and StateStores do. Feature `MULTIPLE_KEY_VALUES_PER_SECRET` was added and is advertised by Hashicorp Vault (default behaviour) and by Local File SecretStore (depending on its configuration). Updated tests to account to new method and ensure expected behavior. Fixes dapr#2047 Signed-off-by: Tiago Alves Macambira <tmacam@burocrata.org>

codecov · 2022-09-13T22:09:03Z

Codecov Report

Merging #2069 (cdf58c7) into master (80ae331) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2069      +/-   ##
==========================================
+ Coverage   37.74%   37.79%   +0.04%     
==========================================
  Files         192      192              
  Lines       24062    24090      +28     
==========================================
+ Hits         9083     9105      +22     
- Misses      14209    14215       +6     
  Partials      770      770

Impacted Files	Coverage Δ
...etstores/alicloud/parameterstore/parameterstore.go	`91.26% <100.00%> (+0.17%)`	⬆️
secretstores/aws/parameterstore/parameterstore.go	`84.84% <100.00%> (+0.31%)`	⬆️
secretstores/aws/secretmanager/secretmanager.go	`48.83% <100.00%> (+1.21%)`	⬆️
secretstores/azure/keyvault/keyvault.go	`37.93% <100.00%> (+1.08%)`	⬆️
secretstores/gcp/secretmanager/secretmanager.go	`42.72% <100.00%> (+1.06%)`	⬆️
secretstores/hashicorp/vault/vault.go	`34.65% <100.00%> (+2.42%)`	⬆️
secretstores/huaweicloud/csms/csms.go	`71.62% <100.00%> (+0.78%)`	⬆️
secretstores/kubernetes/kubernetes.go	`20.00% <100.00%> (+3.01%)`	⬆️
secretstores/local/env/envstore.go	`83.33% <100.00%> (+1.51%)`	⬆️
secretstores/local/file/filestore.go	`64.70% <100.00%> (+2.20%)`	⬆️
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

berndverst · 2022-09-13T22:31:37Z

@tmacam can you please raise a docs issue to add this as a column in the SecretStores table?

tmacam · 2022-09-13T22:47:10Z

@tmacam can you please raise a docs issue to add this as a column in the SecretStores table?

Done: created issue dapr/docs#2787 and mentioned that table specifically at it.

This PR is aimed at addressing issue dapr#2047. In the [Secret API Documentation](https://docs.dapr.io/reference/api/secrets_api/#response-body) it is stated: > If a secret store has support for multiple keys in a secret, a JSON payload is returned with the key names as fields and their respective values. > > In case of a secret store that only has name/value semantics, a JSON payload is returned with the name of the secret as the field and the value of the secret as the value. There are two classes of secret stores but there isn't a way to tell them apart at run-time. This limits the ability of conformance tests to verify the behavior of secret stores supporting multiple keys. We address this by augmenting SecretStores with the ability to advetise `Features`. This is similar to what PubSub and StateStores do. Feature `MULTIPLE_KEY_VALUES_PER_SECRET` was added and is advertised by Hashicorp Vault (default behaviour) and by Local File SecretStore (depending on its configuration). Updated tests to account to new method and ensure expected behavior. Fixes dapr#2047 Signed-off-by: Tiago Alves Macambira <tmacam@burocrata.org> Signed-off-by: Tiago Alves Macambira <tmacam@burocrata.org> Co-authored-by: Bernd Verst <4535280+berndverst@users.noreply.github.com>

PR dapr/components-contrib#2069 added Features() as an API to secret stores. Unfortunatelly, Dapr Metadata APIs would not contain any information regarding Secret Store features or capabilities in its output. This PR addresses this. Signed-off-by: Tiago Alves Macambira <tmacam@burocrata.org>

* Expose SecretStore features through metadata APIs. PR dapr/components-contrib#2069 added Features() as an API to secret stores. Unfortunatelly, Dapr Metadata APIs would not contain any information regarding Secret Store features or capabilities in its output. This PR addresses this. Signed-off-by: Tiago Alves Macambira <tmacam@burocrata.org> * PubSubs features are not reported by Metadata API. While fixing the previous commit I noticed PubSubs features are also not output by Metadata API. Addressing that and adding tests. Signed-off-by: Tiago Alves Macambira <tmacam@burocrata.org> * Fix PubSub mock initialization Signed-off-by: Tiago Alves Macambira <tmacam@burocrata.org> Signed-off-by: Tiago Alves Macambira <tmacam@burocrata.org> Co-authored-by: Artur Souza <artursouza.ms@outlook.com> Co-authored-by: Dapr Bot <56698301+dapr-bot@users.noreply.github.com>

tmacam force-pushed the SecretStoreFeature_I2047 branch from a4c06a3 to 8bf16c7 Compare September 12, 2022 23:00

tmacam marked this pull request as ready for review September 12, 2022 23:04

tmacam requested review from a team as code owners September 12, 2022 23:04

tmacam mentioned this pull request Sep 12, 2022

Update SecretStore API documentation to account for Features feature dapr/docs#2787

Closed

mcandeia suggested changes Sep 12, 2022

View reviewed changes

tmacam force-pushed the SecretStoreFeature_I2047 branch from 8bf16c7 to 2fe9bc9 Compare September 13, 2022 00:17

ItalyPaleAle approved these changes Sep 13, 2022

View reviewed changes

Merge branch 'master' into SecretStoreFeature_I2047

cdf58c7

berndverst approved these changes Sep 13, 2022

View reviewed changes

berndverst added this to the v1.9 milestone Sep 13, 2022

berndverst added automerge documentation required This issue needs documentation labels Sep 13, 2022

artursouza merged commit 8eec2a8 into dapr:master Sep 13, 2022

tmacam deleted the SecretStoreFeature_I2047 branch September 20, 2022 18:45

This was referenced Sep 20, 2022

Expose SecretStore and PubSub features through Metadata APIs. dapr/dapr#5208

Merged

PubSub and SecretStore features are not exposed by Metadata API dapr/dapr#5211

Closed

tmacam mentioned this pull request Oct 21, 2022

REQUEST: New membership for @tmacam dapr/community#246

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SecretStores advertise supported Features(). #2069

SecretStores advertise supported Features(). #2069

tmacam commented Sep 12, 2022 •

edited

Loading

mcandeia Sep 12, 2022

mcandeia Sep 12, 2022

tmacam Sep 13, 2022

mcandeia Sep 12, 2022

tmacam Sep 13, 2022

mcandeia Sep 12, 2022

mcandeia Sep 12, 2022

mcandeia Sep 12, 2022

mcandeia Sep 12, 2022

tmacam Sep 13, 2022 •

edited

Loading

mcandeia Sep 12, 2022

mcandeia Sep 12, 2022

mcandeia Sep 12, 2022

codecov bot commented Sep 13, 2022

berndverst commented Sep 13, 2022

tmacam commented Sep 13, 2022

SecretStores advertise supported Features(). #2069

SecretStores advertise supported Features(). #2069

Conversation

tmacam commented Sep 12, 2022 • edited Loading

Description

Issue reference

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmacam Sep 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 13, 2022

Codecov Report

berndverst commented Sep 13, 2022

tmacam commented Sep 13, 2022

tmacam commented Sep 12, 2022 •

edited

Loading

tmacam Sep 13, 2022 •

edited

Loading