feat: option to create secret group for storing cert

[vkuma17]

Description

Enables option to create secret group in DA for storing cert instead of giving ID of a pre existing secret group.

Release required?

• No release
• Patch release (x.x.X)
• Minor release (x.X.x)
• Major release (X.x.x)

Release notes content

Default value of cert_secrets_group_id has changed from default to null because if secrets manager instance is created with addons flow, certificate can only be created in default group as that is the only secret group present and this is not a great practice. If null value is passed, DA will create a new secret group and hence your certificate might get deleted from existing secret group and will get recreated in the newly created secret group. If you want to avoid this you will have to manually change back cert_secrets_group_id value to your existing secret group ID.

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]

/run pipeline

[vkuma17]

/run pipeline

[shemau]

Specific comments on the code, but also. general observations.

Generally speaking this is a code pattern for DAs. If a DA is using secrets manager it is supporting secret groups. Is there an issue/epic to make sure a consistent pattern is adopted across all DAs?

I also have a concern about migration. Starting with an existing consumer using this feature by setting the ID. What happens? It seem likely that the new DA will drop the ID. Then it will default the new values which probably leads to it destroying the existing secret and creating a new one in the default group. Even if the code gets lucky and it does an update in place to move the secret, so that the secret CRN stays the same, it just changed who can read the secret. Likely the CRN will change impacting any external processes that are using it.

[vkuma17]

Specific comments on the code, but also. general observations.

Generally speaking this is a code pattern for DAs. If a DA is using secrets manager it is supporting secret groups. Is there an issue/epic to make sure a consistent pattern is adopted across all DAs?

I also have a concern about migration. Starting with an existing consumer using this feature by setting the ID. What happens? It seem likely that the new DA will drop the ID. Then it will default the new values which probably leads to it destroying the existing secret and creating a new one in the default group. Even if the code gets lucky and it does an update in place to move the secret, so that the secret CRN stays the same, it just changed who can read the secret. Likely the CRN will change impacting any external processes that are using it.

@shemau This is not a breaking change, there is always one secret group which has name and ID as default, currently default value of the cert_secret_group_id is default and new variables we are introducing have default values in accordance. I don't think existing secret will get destroyed and hence a new one will also not get created and hence CRN shouldn't change. Upgrade test has passed in PR.
@ocofaigh do you have any suggestion on the code pattern. Our secrets manager DA does support creating secret groups but it will create a list of groups and also ID's of them are not exposed. In either case, top DA will need some changes.

[shemau]

I still think there is a specific situation where a secret could be destroyed.

Let me try again.

A secret group ABCD has ID 1234. Currently 1234 is passed into DA. Secret group ABCD has the secret created in it.
Upgrade to new version of DA. The ID is discarded, since it is no longer required. The name is now default and create secret group is false (default values). The statefile has a secret in group 1234. The plan says the secret should be in group default. According to the sm_private_cert resource, secret_group_id does not force new, so an update in place happens, and the secret gets changed to the group with ID default. Secret groups purpose fine grain control of access, so the access to the secret just changed.

[vkuma17]

I still think there is a specific situation where a secret could be destroyed.

Let me try again.

A secret group ABCD has ID 1234. Currently 1234 is passed into DA. Secret group ABCD has the secret created in it. Upgrade to new version of DA. The ID is discarded, since it is no longer required. The name is now default and create secret group is false (default values). The statefile has a secret in group 1234. The plan says the secret should be in group default. According to the sm_private_cert resource, secret_group_id does not force new, so an update in place happens, and the secret gets changed to the group with ID default. Secret groups purpose fine grain control of access, so the access to the secret just changed.

Yes in that case, secret will get deleted from old secret group and will be recreated. Can we ask the consumer to use cert_secret_group_name accordingly to avoid breaking change?

[vkuma17]

/run pipeline

[vkuma17]

/run pipeline

[vkuma17]

@shemau i addressed review comments, need your suggestion on the migration here.

[vkuma17]

/run pipeline

[vkuma17]

/run pipeline

[shemau]

LGTM.

[terraform-ibm-modules-ops]

🎉 This PR is included in version 1.6.0 🎉

The release is available on:

• GitHub release
• v1.6.0

Your semantic-release bot 📦🚀