-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: add support for creating multiple secrets/secret_groups #157
Conversation
/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.
Generally good code, a few finer details and edge cases to consider.
modules/secrets/main.tf
Outdated
locals { | ||
secret_groups = flatten([ | ||
for secret_group in var.secrets : | ||
secret_group.existing_secret_group || secret_group.secret_group_name == null ? [] : [{ |
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.
secret_group_name is required, can it ever be null?
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.
when we set secret_group_name
to null, the secret will be created in the default
secret group.
modules/secrets/main.tf
Outdated
] : [ | ||
for secret in secret_group.secrets != null ? secret_group.secrets : [] : merge({ | ||
secret_group_id = secret_group.secret_group_name != null ? module.secrets_manager_groups[secret_group.secret_group_name].secret_group_id : null | ||
secret_group_name = secret_group.secret_group_name != null ? secret_group.secret_group_name : "default" |
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.
secret_group.secret_group_name is a required input, so it should never be null.
It is unclear what the intention is with the default
secret group in this code. The behaviour is not documented in the readme. Do the lower level modules even support passing the id as default in this way to create secrets in the default secret group? I think maybe the secret_group_id is intended to default to be default
(overloaded). It may be worth including a test for this code path.
Maybe a bigger question, secret_groups are the way to provide fine level access control over secrets. The use of default group may be considered a red flag for regulated environments, such as fscloud. Maybe default should just not be supported. @alex-reiff ?
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.
Yes, it supports passing default
as secret_group_id
.
This submodule was added to support managing secrets created by the DA. Issue, so I think it should add flexibility for choosing which secret group to use.
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 am not convinced about the support for null
, or the complexity of all the conditional logic.
default
is an abstraction for a guid. The culmination of this logic is unforeseen failure paths.
Consider this code block. We know that default
MUST exist. The logic processes default
and null in both existing and new groups blocks.
This results in
secret_group_name = null; existing_secret_group = true; the secret is created in the existing default secret group
secret_group_name = "default"; existing_secret_group = true; the secret is created in the existing default secret group
then
secret_group_name = null; existing_secret_group = false; the secret is created in the existing
default secret group... but existing_secret_group was false. Arguably this wrong, and potentially covers up and accidental error on inputs.
secret_group_name = "default"; existing_secret_group = false fails gracefully during plan with
╷
│ Error: Invalid value for variable
│
│ on ../../main.tf line 141, in module "secrets":
│ 141: secrets = var.secrets
│ ├────────────────
│ │ var.secrets is list of object with 5 elements
│
│ The `default` secret group already exist, set `existing_secret_group` flag to true.
│
│ This was checked by the validation rule at ../../modules/secrets/variables.tf:53,3-13.
The opinionated view is that the consumer should be required to pass a non-null string to the required field. When default
is required, it MUST be passed.
This avoids any confusion and creating secrets in the wrong secret group with different access rights.
/run pipeline |
/run pipeline |
modules/secrets/main.tf
Outdated
] : [ | ||
for secret in secret_group.secrets != null ? secret_group.secrets : [] : merge({ | ||
secret_group_id = secret_group.secret_group_name != null ? module.secrets_manager_groups[secret_group.secret_group_name].secret_group_id : null | ||
secret_group_name = secret_group.secret_group_name != null ? secret_group.secret_group_name : "default" |
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 am not convinced about the support for null
, or the complexity of all the conditional logic.
default
is an abstraction for a guid. The culmination of this logic is unforeseen failure paths.
Consider this code block. We know that default
MUST exist. The logic processes default
and null in both existing and new groups blocks.
This results in
secret_group_name = null; existing_secret_group = true; the secret is created in the existing default secret group
secret_group_name = "default"; existing_secret_group = true; the secret is created in the existing default secret group
then
secret_group_name = null; existing_secret_group = false; the secret is created in the existing
default secret group... but existing_secret_group was false. Arguably this wrong, and potentially covers up and accidental error on inputs.
secret_group_name = "default"; existing_secret_group = false fails gracefully during plan with
╷
│ Error: Invalid value for variable
│
│ on ../../main.tf line 141, in module "secrets":
│ 141: secrets = var.secrets
│ ├────────────────
│ │ var.secrets is list of object with 5 elements
│
│ The `default` secret group already exist, set `existing_secret_group` flag to true.
│
│ This was checked by the validation rule at ../../modules/secrets/variables.tf:53,3-13.
The opinionated view is that the consumer should be required to pass a non-null string to the required field. When default
is required, it MUST be passed.
This avoids any confusion and creating secrets in the wrong secret group with different access rights.
/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.
LGTM
🎉 This issue has been resolved in version 1.16.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Description
Issue: https://github.ibm.com/GoldenEye/issues/issues/9825
Release required?
x.x.X
)x.X.x
)X.x.x
)Release notes content
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