Skip to content

Conversation

@vkuma17
Copy link
Contributor

@vkuma17 vkuma17 commented Apr 30, 2025

Description

Add support for setting up Secrets Manager in a Kubernetes Service cluster. AKA the steps documented here: https://cloud.ibm.com/docs/containers?topic=containers-secrets-mgr

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.

@vkuma17
Copy link
Contributor Author

vkuma17 commented Apr 30, 2025

/run pipeline

@vkuma17
Copy link
Contributor Author

vkuma17 commented Apr 30, 2025

/run pipeline

@vkuma17
Copy link
Contributor Author

vkuma17 commented Apr 30, 2025

/run pipeline

toddgiguere
toddgiguere previously approved these changes May 1, 2025
@toddgiguere
Copy link
Member

@ocofaigh @daniel-butler-irl The code here looks correct so I've approved, but please have a look at the variable names in the solution/catalog to make sure they look ok as well.

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 a few comments. We also now need to update the ibm_catalog.json and add mappings for the new inputs since secrets manager DA is marked as a dependent DA

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.

As discussed on slack, I think OCP DA should create the secret group that the secrets manager integration will use. I would use the cluster ID in the name of the secret group. And use https://github.com/terraform-ibm-modules/terraform-ibm-secrets-manager-secret-group to create it, as this also supports creating an associated access group for the secret group.
We should also support using existing group, but default to create new one.

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.

@Vipin654 Left one more comment. Are you planning to add secret group support in this PR?

@vkuma17
Copy link
Contributor Author

vkuma17 commented May 2, 2025

@Vipin654 Left one more comment. Are you planning to add secret group support in this PR?

Yes @ocofaigh. I will add secrets group creation in this PR only.

@knee-na
Copy link

knee-na commented May 2, 2025

I added comments in this PR where possible for the Readme file. Other content suggestions are included in this comment for the Readme file:

| <a name="input_cluster_config_endpoint_type"></a> [cluster\_config\_endpoint\_type](#input\_cluster\_config\_endpoint\_type) | Specify which type of endpoint to use for for cluster config access: 'default', 'private', 'vpe', 'link'. 'default' value will use the default endpoint of the cluster. | `string` | `"default"` | no |

Specify which type of endpoint to use for cluster config access: 'default', 'private', 'vpe', 'link'. A 'default' value uses the default endpoint of the cluster.

| <a name="input_cluster_name"></a> [cluster\_name](#input\_cluster\_name) | The name that will be assigned to the provisioned cluster | `string` | n/a | yes |

The name that is assigned to the provisioned cluster.

| <a name="input_cluster_ready_when"></a> [cluster\_ready\_when](#input\_cluster\_ready\_when) | The cluster is ready when one of the following: MasterNodeReady (not recommended), OneWorkerNodeReady, Normal, IngressReady | `string` | `"IngressReady"` | no |

The cluster is ready based on one of the following:

| <a name="input_vpc_id"></a> [vpc\_id](#input\_vpc\_id) | Id of the VPC instance where this cluster will be provisioned | `string` | n/a | yes |

ID of the VPC instance where this cluster is provisioned.

| <a name="input_vpc_subnets"></a> [vpc\_subnets](#input\_vpc\_subnets) | Metadata that describes the VPC's subnets. Obtain this information from the VPC where this cluster will be created | <pre>map(list(object({<br/> id = string<br/> zone = string<br/> cidr_block = string<br/> })))</pre> | n/a | yes |

Metadata that describes the VPC's subnets. Obtain this information from the VPC where this cluster is created

| <a name="output_cluster_crn"></a> [cluster\_crn](#output\_cluster\_crn) | CRN for the created cluster |

CRN for the cluster

| <a name="output_cluster_id"></a> [cluster\_id](#output\_cluster\_id) | ID of cluster created |

ID of the cluster

| <a name="output_cluster_name"></a> [cluster\_name](#output\_cluster\_name) | Name of the created cluster |

Name of the cluster

@vkuma17
Copy link
Contributor Author

vkuma17 commented May 15, 2025

/run pipeline

@vkuma17
Copy link
Contributor Author

vkuma17 commented May 15, 2025

/run pipeline

2 similar comments
@vkuma17
Copy link
Contributor Author

vkuma17 commented May 15, 2025

/run pipeline

@vkuma17
Copy link
Contributor Author

vkuma17 commented May 15, 2025

/run pipeline

@vkuma17
Copy link
Contributor Author

vkuma17 commented May 17, 2025

/run pipeline

@vkuma17
Copy link
Contributor Author

vkuma17 commented May 18, 2025

/run pipeline

@vkuma17 vkuma17 requested a review from ocofaigh May 19, 2025 08:15
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.

see latest comments

# decide the iam endpoint depending upon the IBMCLOUD_IAM_API_ENDPOINT env variable set by the user and
# whether provider visibility is public or private
iam_cloud_endpoint="${IBMCLOUD_IAM_API_ENDPOINT:-"iam.cloud.ibm.com"}"
IBMCLOUD_IAM_API_ENDPOINT=${iam_cloud_endpoint#https://}
Copy link
Contributor

Choose a reason for hiding this comment

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

why are you stripping https:// from IBMCLOUD_IAM_API_ENDPOINT ? You are adding it back again later, so I see no point in this logic. The default value of IBMCLOUD_IAM_API_ENDPOINT should be https://iam.cloud.ibm.com

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess if we need to use private endpoint for IAM, then removing https:// is needed so that we can later add private. I took the same logic from reset_api_key script.

https://github.com/terraform-ibm-modules/terraform-ibm-base-ocp-vpc/blob/sm-integration/scripts/reset_iks_api_key.sh#L34

@vkuma17
Copy link
Contributor Author

vkuma17 commented May 19, 2025

/run pipeline

@ocofaigh
Copy link
Contributor

/run pipeline

@ocofaigh ocofaigh merged commit 21e9aaf into main May 19, 2025
2 checks passed
@ocofaigh ocofaigh deleted the sm-integration branch May 19, 2025 20:00
@terraform-ibm-modules-ops
Copy link
Contributor

🎉 This PR is included in version 3.48.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants