-
Notifications
You must be signed in to change notification settings - Fork 2
add: kv (key_value) secret type #338
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
Conversation
|
/run pipeline |
|
/run pipeline |
|
/run pipeline |
|
/run pipeline |
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.
Ignore this comment: It is in the expected location.
The KV secret should probably be implemented using the same model as the other secrets manager keys. It does not require an engine, so I would expect the resource definition to be in the main secrets (terraform-ibm-secrets-manager-secret) module. This will then make it simple to use the existing model for managing multiple secrets (modules/secrets/main.tf) as the others.
variables.tf
Outdated
| } | ||
|
|
||
| variable "secret_kv_data" { | ||
| type = map(any) |
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.
This is a concern.
It clearly works here, but I wonder how it becomes accessible at higher levels. Specifically, if exposing from the DA, how is it documented?
Usually at the DA level, it is a list of objects, so it would become a list of maps of any.
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.
@Aditya-ranjan-16 Why are you using map(any) instead of map(string) ? Wont it always be a key value of strings? Or am I missing something?
@shemau FYI we don't support creating secrets through the Secrets Manager DA (on purpose) - only secret groups. It is however supported in the secrets submodule, and root level secrets-manager module so will need to be exposed in both places too after this is merged
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.
@ocofaigh the cloud docs example was using map(any) , but I will change it map(string) makes more sense
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 thought about the any/string as well. I figured it applied to the value part, since the key will always be a string. At this point you could have secrets that would be multi-part, eg ssh key with a private key and public key. I could not see a use case in the documentation, would it be
{
"private_key": "value",
"public_key": "value"
}
or
{
"key": { "private": "value", "public": "value"}
}
The issue specifically said to store JSON. jsondecode (https://developer.hashicorp.com/terraform/language/functions/jsondecode) returns a type of any (as far as I can tell); although it would also be possible to store as the string form.
I approved, because it seemed appropriate to follow the provider documentation and not become opinionated on the content.
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.
Planning failed. Terraform encountered an error while generating this plan.
╷
│ Error: Incorrect attribute value type
│
│ on ../../main.tf line 127, in resource "ibm_sm_kv_secret" "kv_secret":
│ 127: data = var.secret_kv_data
│
│ Inappropriate value for attribute "data": incorrect map element type: string required.
The function errors when values other than string are specified. So should be map(string).
The following are string (or equivalent) and work:
kv_data = { "key1" : "value", "key2" : "value2" }
kv_data = { "key1" : false, "key2" : true }
kv_data = { "key1" : 1, "key2" : 2 }
kv_data = { "key1" : "value", "key2" : true, "key3" : 4 }
It would be good to have an example, maybe complete, that uses the final format.
The following are fail with the error above:
kv_data = { "key1" : { "sub_key1" : "value1" }, "key2" : { "sub_key2" : "value2" } }
kv_data = { "key1" : [ "tag1" ], "key2" : [ "tag2" ] }
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.
LGTM.
I think we will want some additional documentation in secrets-manager to cover usage scenarios. eg. a simple key/value pair or a full out piece of JSON that someone might want to store (versus an arbitrary secret that contains a JSON structure).
ocofaigh
left a comment
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.
Can you please update the module readme with some supporting docs for the new secret type, just like we have for the other supported secret types
|
/run pipeline |
shemau
left a comment
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.
LGTM
Validated change request, reviewer is unavailble.
Description
https://github.ibm.com/GoldenEye/issues/issues/15195
Release required?
x.x.X)x.X.x)X.x.x)Release notes content
This PR adds a new secret type : kv (key_value)
Run the pipeline
If the CI pipeline doesn't run when you create the PR, the PR requires a user with GitHub collaborators access to run the pipeline.
Run the CI pipeline when the PR is ready for review and you expect tests to pass. Add a comment to the PR with the following text:
Checklist for reviewers
For mergers