-
Notifications
You must be signed in to change notification settings - Fork 1
feat: add guardrails to DA #282
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
base: main
Are you sure you want to change the base?
Conversation
/run pipeline |
/run pipeline |
/run pipeline |
/run pipeline |
/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.
left some comments
solutions/standard/variables.tf
Outdated
@@ -11,7 +11,7 @@ variable "ibmcloud_api_key" { | |||
variable "provider_visibility" { | |||
description = "Set the visibility value for the IBM terraform provider. Supported values are `public`, `private`, `public-and-private`. [Learn more](https://registry.terraform.io/providers/IBM-Cloud/ibm/latest/docs/guides/custom-service-endpoints)." | |||
type = string | |||
default = "private" | |||
default = "public" |
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.
what is the reason for changing to public?
@@ -80,6 +80,24 @@ variable "public_engine_enabled" { | |||
default = false | |||
} | |||
|
|||
variable "default_secret_group_name" { |
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 confusing - Isn't there a default group out of the box with every newly provisioned instance? Why are we creating another one (and also calling it default)
} | ||
|
||
module "iam_service_access_group" { | ||
count = var.existing_secrets_manager_crn == null ? 1 : 0 |
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 need a boolean to allow users to opt out of the access group creation if they want
solutions/standard/variables.tf
Outdated
variable "default_access_group_name" { | ||
type = string | ||
description = "Name to give the access group automatically created when provisioning a new Secrets Manager instance." | ||
default = "secrets_manager_group" |
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.
The name should probably have secrets reader in it since that is the role you are granting to this access group
solutions/standard/variables.tf
Outdated
default = "secrets_manager_group" | ||
} | ||
|
||
variable "access_group_ids" { |
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.
misleading name here. Perhaps go with access_group_users
or access_group_user_ids
?
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.
what happens if a user is added to the group outside of terraform (using accesshub for example). When terraform runs and finds an empty list, will it try to remove the users?
/run pipeline |
/run pipeline |
/run pipeline |
/run pipeline |
switching to draft while I work on incorporating acccess group creation into the secrets submodule |
Description
Add a default secrets group and access group to the secrets manager DA
Issue: https://github.ibm.com/GoldenEye/issues/issues/12243
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