Skip to content

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

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from
Draft

feat: add guardrails to DA #282

wants to merge 14 commits into from

Conversation

alex-reiff
Copy link
Contributor

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?

  • No release
  • Patch release (x.x.X)
  • Minor release (x.X.x)
  • Major release (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:

/run pipeline

Checklist for reviewers

  • If relevant, a test for the change is included or updated with this PR.
  • If relevant, documentation for the change is included or updated with this PR.

For mergers

  • Use a conventional commit message to set the release level. Follow the guidelines.
  • Include information that users need to know about the PR in the commit message. The commit message becomes part of the GitHub release notes.
  • Use the Squash and merge option.

@alex-reiff alex-reiff requested a review from shemau as a code owner February 10, 2025 16:47
@alex-reiff
Copy link
Contributor Author

/run pipeline

@alex-reiff
Copy link
Contributor Author

/run pipeline

@alex-reiff
Copy link
Contributor Author

/run pipeline

@alex-reiff
Copy link
Contributor Author

/run pipeline

@alex-reiff
Copy link
Contributor Author

/run pipeline

@alex-reiff
Copy link
Contributor Author

/run pipeline

@alex-reiff
Copy link
Contributor Author

/run pipeline

@alex-reiff
Copy link
Contributor Author

/run pipeline

Copy link
Contributor

@ocofaigh ocofaigh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left some comments

@@ -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"
Copy link
Contributor

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" {
Copy link
Contributor

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
Copy link
Contributor

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

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"
Copy link
Contributor

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

default = "secrets_manager_group"
}

variable "access_group_ids" {
Copy link
Contributor

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 ?

Copy link
Contributor

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?

@alex-reiff
Copy link
Contributor Author

/run pipeline

@alex-reiff
Copy link
Contributor Author

/run pipeline

@alex-reiff
Copy link
Contributor Author

/run pipeline

@alex-reiff
Copy link
Contributor Author

/run pipeline

@alex-reiff
Copy link
Contributor Author

switching to draft while I work on incorporating acccess group creation into the secrets submodule

@alex-reiff alex-reiff marked this pull request as draft March 3, 2025 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants