-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature specs: Applications.Core/secretStores
extended use cases
#57
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Will Tsai <[email protected]>
Signed-off-by: Will Tsai <[email protected]>
Signed-off-by: Will Tsai <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just have the one question around naming conventions for types.
Signed-off-by: Will <[email protected]>
Signed-off-by: Will Tsai <[email protected]>
Signed-off-by: Will Tsai <[email protected]>
Signed-off-by: Will Tsai <[email protected]>
This pull request is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
This pull request has been closed due to inactivity. Feel free to reopen if you are still working on it. |
resource azurekeyvaultsecrets 'Applications.Core/secretStores@2023-10-01-preview' = { | ||
name: 'azurekeyvaultsecrets' | ||
properties:{ | ||
application: application | ||
type: 'generic' // this can change based on detailed tech design | ||
data: { | ||
'name': { | ||
value: secret1 | ||
} | ||
'version': { | ||
value: 1 | ||
} | ||
'encoding': { | ||
value: 'base64' | ||
} | ||
'alias': { | ||
value: secretalias | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't tell how it references from azure keyvault, could you please elaborate on that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kachawla yes, good catch, in this case I missed adding the reference to an external resource name, so I've added resource: 'secret-app-existing-secret'
to the example. Hopefully that clears it up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be helpful to use a more real example instead of a string like secret-app-existing-secret
. For example, if a user wanted to reference an azure keyvault, would they need to provide a fully qualified resource id?
username: '' | ||
resources: [ | ||
{ id: cosmosAccount.id } | ||
] | ||
secrets: { | ||
+ connectionString: { | ||
+ valueFrom: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should revisit the overall plan for provisioning types we plan to support on portable resources before implementing this change. There are some changes with UDT that haven't been solidified yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added this as a note to the feature spec
|
||
### Feature 2: Add functionality to reference `Applications.Core/secretStores` in `Applications.Core/*` resources | ||
<!-- One or two sentence summary --> | ||
Add the ability for developers to reference values from their `Applications.Core/secretStores` resources in their core resources (namely `Applications.Core/extenders` and `Applications.Core/volumes`) so that secrets can be securely managed for use in their application to authenticate into those resources. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Should extender be bucketed under portable resources (feature 3)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I bucketed it under Core resources because of its Application.Core
namespace, do you think it's misnamed in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd bucket these based on the user's goal/task. I think there are three scenarios:
- Application code needs to read secrets as env-vars.
- Application code needs to read secrets as files.
- Radius needs to operate on secrets for infrastructural purposes (unrelated to user-code).
What's good about this framing is that the user has already decided what they need to accomplish. We can talk clearly about needs and wants without getting stuck in implementation.
|
||
> The TLS certificate data secret in `Applications.Core/gateways` is referenced today by `tls: { certificateFrom: secretstore.id }`. As a part of implementation we should evaluate if the `valueFrom: { secretRef: { ... } }` pattern proposed here is an acceptable deviation from the previous pattern implemented for `gateways`. | ||
|
||
**Risk: use of secrets in core and portable resources.** This pattern of referencing and leveraging secrets in core and portable resources might not be a common or desirable pattern for users. We will validate this by opening up this feature spec for discussion with the community. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if I follow this, are there alternatives that we think might be more desirable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, there are no more desirable alternatives that we can think of, but I just added this as a risk so that we can explicitly call it out and source feedback on it
This pull request is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
This pull request has been closed due to inactivity. Feel free to reopen if you are still working on it. |
@willtsai looks like this got auto closed. Are there any pending updates or is this PR mainly pending approval at this point? |
Signed-off-by: Will Tsai <[email protected]>
Signed-off-by: Will <[email protected]>
Signed-off-by: Will Tsai <[email protected]>
Signed-off-by: Will Tsai <[email protected]>
Signed-off-by: Will Tsai <[email protected]>
|
||
As an operator, I can define an `Applications.Core/secretStores` resource and deploy it along with an Environment so that the developers I support can securely leverage secrets I manage on their behalf for use in their application resources. | ||
|
||
As an operator, I can reference an `Applications.Core/secretStores` in my Recipes so that I no longer have to store secrets as plain text in the `secrets` field of my Recipe's `output` object for authentication, etc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now it reads like it could be two things: a recipe containing a single instance of a secret used by all applications (simple), just want to make sure we don't paint ourselves into a corner if there's any plans for a recipe containing a class of secret that resolves to an application instance specific secret(complex).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Further thought I think the complex case isn't one we're trying to solve.
|
||
As an operator, I can define an `Applications.Core/secretStores` resource and deploy it along with an Environment so that the developers I support can securely leverage secrets I manage on their behalf for use in their application resources. | ||
|
||
As an operator, I can reference an `Applications.Core/secretStores` in my Recipes so that I no longer have to store secrets as plain text in the `secrets` field of my Recipe's `output` object for authentication, etc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this document include the design for how users will do this?
|
||
As a developer, I can reference an `Applications.Core/secretStores` in my `Applications.Core/containers` resource definition so that Radius will inject secrets as environment variables into my container at deploy time so that credentials can be provided to the container for authentication, etc. | ||
|
||
As a developer, I can reference an `Applications.Core/secretStores` in my `Applications.Extenders` or `Applications.Volumes` resource definition so that I no longer have to store secrets as plain text in the `properties.secrets` field of my resource for authentication, etc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this document include the design for each of the places we need to update?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the volumes scenario is different. The use-case for extenders belongs in the next paragraph.
As an application developer, I cannot reference a `secretStores` resource in the `Applications.Core/containers`, `Applications.Core/extenders`, `Applications.Core/volumes`, `Applications.Datastores/*`, and `Applications.Messaging/*` resource types to securely manage secrets for use in my application. Instead, I'm having to store my secrets in plain text within the `properties.secrets` field for each resource. This is a critical gap in the secret management capabilities of Radius that is hindering my ability to securely manage secrets for use in my containers. | ||
|
||
## Desired user experience outcome | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can simplify this into three cases:
- Secrets are stored and then mounted as environment-variables in a container.
- Secrets are stored and then mounted as files-on-disk in a container.
- Secrets are stored and used internally by the Radius infrastructure or some other piece of automation (not application code).
name: 'authcreds' | ||
properties:{ | ||
application: application | ||
// today the type is an enum, see https://github.com/radius-project/radius/pull/7816 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this something addressed by the proposal?
+ valueFrom: { | ||
+ secretRef: { | ||
+ source: authcreds.id | ||
+ key: 'username' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does the user get authcreds.id
?
} | ||
``` | ||
|
||
Step 7: Developer deploys the resources to Radius and the secrets required are either injected into the container as environment variables or used as credentials for authentication into the extender, volume, datastore, or messaging resource at deploy time. If the resource is deployed using a Recipe, the secrets are securely passed as outputs to Radius by the Recipe for use in provisioning and deploying the resource. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm really confused about this walkthrough because the steps are numbered 1-7, but they appear unrelated, or to address different usecases.
|
||
### Feature 1: Add functionality to reference `Applications.Core/secretStores` in `Applications.Core/containers` resources | ||
<!-- One or two sentence summary --> | ||
Add the ability for developers to reference values from their `Applications.Core/secretStores` resources in their `Applications.Core/containers` resource definitions under the `properties.container.env` field so that secrets can be injected as environment variables into their container at deploy time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we do this already?
|
||
### Feature 2: Add functionality to reference `Applications.Core/secretStores` in `Applications.Core/*` resources | ||
<!-- One or two sentence summary --> | ||
Add the ability for developers to reference values from their `Applications.Core/secretStores` resources in their core resources (namely `Applications.Core/extenders` and `Applications.Core/volumes`) so that secrets can be securely managed for use in their application to authenticate into those resources. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd bucket these based on the user's goal/task. I think there are three scenarios:
- Application code needs to read secrets as env-vars.
- Application code needs to read secrets as files.
- Radius needs to operate on secrets for infrastructural purposes (unrelated to user-code).
What's good about this framing is that the user has already decided what they need to accomplish. We can talk clearly about needs and wants without getting stuck in implementation.
### Feature 5: Add functionality to reference `Applications.Core/secretStores` in Recipes | ||
<!-- One or two sentence summary --> | ||
Add the ability for operators to reference values from their `Applications.Core/secretStores` resources in their Recipe's `output` object so that secrets can be securely passed as outputs to Radius that can be used in provisioning and deploying the resource using the Recipe. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels like this design doc is trying to accomplish a lot. Which of these should we do first?
+ password: { | ||
+ valueFrom: { | ||
+ secretRef: { | ||
+ source: authcreds.id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed this before. I don't believe bicep would allow you to reference a resource that is not defined within this file.
+ password = { | ||
+ valueFrom = { | ||
+ secretRef = { | ||
+ source = authcreds.id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
|
||
**Assumption: operators can create and deploy `Applications.Core/secretStores` resources.** We assume that operators create and manage secrets on behalf of developers, which are encapsulated in `Applications.Core/secretStores` resources. The developers can subsequently reference these resources in their application resources to securely manage secrets for use in their applications. We will validate this assumption by opening up this feature spec for discussion with the community. | ||
|
||
**Assumption: the `Applications.secretStores` resource type can be referenced as a parameter to a Bicep resource.** We assume that the `Applications.secretStores` resource type can be referenced as a parameter to a Bicep resource so that secrets can be passed as object parameters into a Radius Bicep resource. We will validate this assumption by investigating the feasibility of this approach during tech design. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah this assumption answers my confusion above. I don't think this assumption is technically feasible. Also I'm not fully clear on the value we are trying to provide by enabling this as a feature.
This pull request is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
This pull request has been closed due to inactivity. Feel free to reopen if you are still working on it. |
Signed-off-by: Will <[email protected]>
Here are the feature specs for extending the use cases of
Applications.Core/secretStores