Support for secret store reference in Dapr components #7823
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7823 +/- ##
==========================================
+ Coverage 61.12% 61.20% +0.08%
==========================================
Files 523 523
Lines 27632 27683 +51
==========================================
+ Hits 16889 16944 +55
+ Misses 9251 9249 -2
+ Partials 1492 1490 -2 ☔ View full report in Codecov by Sentry. |
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
|
Wow thanks for sending this. This is a big improvement. I'll take a look at this tomorrow. Normally we might ask for an design document for an API change, but I think we can skip that in this case:
@radius-project/maintainers-radius holler if you have any concerns. |
There was a problem hiding this comment.
This is a great addition. Thanks!
Should we add a functional test or add this to an existing functional test so that we can test it out?
| "auth": map[string]any{ | ||
| "secretStore": secretStoreComponentName, | ||
| }, |
There was a problem hiding this comment.
Is this auth generated by Dapr since we create a Dapr object with a property called secretStoreComponentName?
There was a problem hiding this comment.
In this case, the auth.secretstore property is added here if the Applications.Dapr/* contains the secretStoreComponentName property.
The auth.secretstore property comes from the Dapr component spec
| // Nested object, has to be a secret reference | ||
| case map[string]any: | ||
| if secretKeyRef, ok := value["secretKeyRef"]; ok { | ||
| yamlItem["secretKeyRef"] = secretKeyRef | ||
| } else { | ||
| return unstructured.Unstructured{}, v1.NewClientErrInvalidRequest(fmt.Sprintf("Invalid metadata value for key %s in Dapr component %s", k, componentName)) | ||
| } |
There was a problem hiding this comment.
Could there be a case where it is not a secret reference even if it is a nested object? Do we need to do an additional check for this case to make sure that it is a secret reference?
There was a problem hiding this comment.
You can see the schema of Component here: https://github.com/dapr/dapr/blob/master/charts/dapr/crds/components.yaml#L48
Note that this is the output we're generating.
The reason why I'm mention it is that it shows what the schema is in Dapr that we're trying roughly match. The allowed keys are:
valuesecretKeyRefenvRef
envRef isn't relevant for us - it's a dev-time feature.
|
Hi @SoTrx - thanks for sending this PR. Looks like it works overall. One question based on your example: resource stateStore 'Applications.Dapr/stateStores@2023-10-01-preview' = {
name: 'statestore'
properties: {
environment: environment
application: application
resourceProvisioning: 'manual'
type: 'state.redis'
version: 'v1'
metadata: {
redisHost: {
secretKeyRef: {
name: 'redisHost'
key: 'redisHost'
}
}
redisPassword: ''
}
secretStoreComponentName: daprSecretStore.name
}
}It looks like the type of If I compare to the Dapr schema of Do you think it makes sense for us to change the design? Example: resource stateStore 'Applications.Dapr/stateStores@2023-10-01-preview' = {
name: 'statestore'
properties: {
environment: environment
application: application
resourceProvisioning: 'manual'
type: 'state.redis'
version: 'v1'
metadata: {
redisHost: {
secretKeyRef: {
name: 'redisHost'
key: 'redisHost'
}
}
redisPassword: {
value: ''
}
}
secretStoreComponentName: daprSecretStore.name
}
}This would be a breaking change, and we'd need to update our existing code, but it's probably a better design. What do you think? |
|
Thank you for your reviews @rynowak Going the other way and using strings only (with a magic prefix to reference secrets) might also seem tempting since it would be non-breaking. But this is a worse design and may lead to parsing/escaping issues or conflict with special values or even a company's own oddly defined pluggable components. resource stateStore 'Applications.Dapr/stateStores@2023-10-01-preview' = {
name: 'statestore'
properties: {
environment: environment
application: application
resourceProvisioning: 'manual'
type: 'state.redis'
version: 'v1'
metadata: {
redisHost: 'secretRef:://key,value'
redisPassword: ''
}
secretStoreComponentName: daprSecretStore.name
}
}Another approach could be treating the secret as its own resource. However, aside from the added complexity, this might encourage users to attempt using secrets from multiple secret stores within a single component, which, as far as I'm aware, Dapr does not support. So using only objects may be the best solution in my opinion. On a related note, if the goal is to match Dapr component schema, should we also change the Something like : resource stateStore 'Applications.Dapr/stateStores@2023-10-01-preview' = {
name: 'statestore'
properties: {
environment: environment
application: application
resourceProvisioning: 'manual'
type: 'state.redis'
version: 'v1'
metadata: {
redisHost: {
secretKeyRef: {
name: 'redisHost'
key: 'redisHost'
}
}
redisPassword: {
value: ''
}
}
auth: {
secretStore: daprSecretStore.name
}
}
}I'll make some functional tests with the curent implementation first as @ytimocin suggested before going forward with the changes we decide on. |
SoTrx
force-pushed
the
feat_dapr-secret-store
branch
3 times, most recently
from
3f7c87e to
a417124
Compare
|
The current implementation follow this schema : resource stateStore 'Applications.Dapr/stateStores@2023-10-01-preview' = {
name: 'statestore'
properties: {
environment: environment
application: application
resourceProvisioning: 'manual'
type: 'state.redis'
version: 'v1'
metadata: {
redisHost: {
secretKeyRef: {
name: 'redisHost'
key: 'redisHost'
}
}
redisPassword: {
value: ''
}
}
secretStoreComponentName: daprSecretStore.name
}
}Should I change secretStoreComponentName to auth.secretStore as well? Let me know if you need any further adjustments |
I'm on board with this 👍. I think it's good if we align to the Dapr component schema. Then it will be easy to update Radius as Dapr continues to add features. Taking a look now! Thanks for working through this. |
SoTrx
force-pushed
the
feat_dapr-secret-store
branch
3 times, most recently
from
408af4a to
a6799ac
Compare
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
rynowak
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
BREAKING CHANGE: All Dapr components metadata are now parsed as objects instead of their previous format. All string-based values will break Signed-off-by: SoTrx <[email protected]>
Signed-off-by: SoTrx <[email protected]>
…prrp Signed-off-by: SoTrx <[email protected]>
…ll Dapr components BREAKING CHANGE: The property `secretStoreComponentName` has been replaced with `auth.secretStore`. This change requires updates to any references to the old property. Signed-off-by: SoTrx <[email protected]>
Signed-off-by: SoTrx <[email protected]>
…z_generated_models_serde.go Signed-off-by: SoTrx <[email protected]>
ytimocin
force-pushed
the
feat_dapr-secret-store
branch
from
b1583ad to
883e043
Compare
ytimocin
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
* Dapr Resource Schema Update More context: radius-project/radius#7823 Signed-off-by: karishma-chawla <[email protected]> * Update env vars schema Related to: #1742 Signed-off-by: karishma-chawla <[email protected]> --------- Signed-off-by: karishma-chawla <[email protected]> Co-authored-by: karishma-chawla <[email protected]>
@rynowak missed this notification. I agree it's ok to skip design documents for such changes, but if a change changes an api then an overview of the updated schema, samples and doc updates should be needed regardless of the scope of change. |
Description
Hey,
This PR adds support for building Dapr components with an optional reference to a Dapr secret store.
The new property is named secretStoreComponentName. This name is totally arbitrary but was chosen to try to be as clear as possible.
A secret reference can be created using a secretKeyRef object, following the same naming convention as Dapr.
Example :
Type of change
Fixes: #7704