Skip to content
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: added functionality to allow for a kms key in an external account #230

Merged
merged 27 commits into from
Aug 13, 2024

Conversation

kierramarie
Copy link
Contributor

Description

An external kms key can be used now. If an api key for the external account is passed, new iam policies will be created to all EN and COS to communicate with the external kms instance.

Git Issue: #213

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.

@kierramarie
Copy link
Contributor Author

/run pipeline

@kierramarie
Copy link
Contributor Author

/run pipeline

@ocofaigh
Copy link
Member

@kierramarie ensure changes (including variable descriptions) are consistent with terraform-ibm-modules/terraform-ibm-secrets-manager#147

@kierramarie
Copy link
Contributor Author

@ocofaigh this da uses kms for en and cos. should I update the cos part as well to use the external kms (I have already mostly implemented this)?

@ocofaigh
Copy link
Member

ocofaigh commented Jul 1, 2024

@kierramarie yes since KMS key is supported for both EN and the COS bucket, cross account support needed for both

@kierramarie
Copy link
Contributor Author

/run pipeline

Copy link
Member

@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.

some comments

solutions/standard/variables.tf Outdated Show resolved Hide resolved
solutions/standard/main.tf Outdated Show resolved Hide resolved
solutions/standard/main.tf Outdated Show resolved Hide resolved
solutions/standard/main.tf Outdated Show resolved Hide resolved
solutions/standard/main.tf Outdated Show resolved Hide resolved
solutions/standard/main.tf Outdated Show resolved Hide resolved
solutions/standard/main.tf Outdated Show resolved Hide resolved
@kierramarie
Copy link
Contributor Author

/run pipeline

@kierramarie
Copy link
Contributor Author

/run pipeline

@Ak-sky
Copy link
Member

Ak-sky commented Jul 16, 2024

Need to wait for this PR to get merged.

@kierramarie
Copy link
Contributor Author

this pr is failing in the precommit but is fine when I run precommit locally.

@kierramarie
Copy link
Contributor Author

The PR is failing because:
basic test: "You can only have one instance of a Lite plan per service" error
upgrade da solution: "Unauthorized, service to service is not enabled" error, however upgrade works locally.
fscloud in schematics: "Unfortunately you don't have a permission to access requested resource" with resource: module.event_notification.module.event_notification.ibm_en_destination_cos.cos_en_destination[0]

@ocofaigh
Copy link
Member

@kierramarie To workaround the lite plan issue, you need to update the tests so they always create a new resource group, as you can have 1 lite plan per group. Here is how to achieve that -> https://github.com/terraform-ibm-modules/terraform-ibm-secrets-manager/blob/511d67cf6a936976cbdb093a366b9e4d36e61fb4/tests/pr_test.go#L62-L66

After you make that change, please re-run pipeline to see if you still face other errors

@kierramarie
Copy link
Contributor Author

/run pipeline

@kierramarie
Copy link
Contributor Author

/run pipeline

@kierramarie
Copy link
Contributor Author

@ocofaigh now just the fscloud test is failing with the same error with the "module.event_notification.module.event_notification.data.ibm_en_integrations.en_integrations[0]" resource.

@ocofaigh
Copy link
Member

@kierramarie pipeline is unblocked now if you want to get this one back ready

@kierramarie
Copy link
Contributor Author

/run pipeline

@kierramarie
Copy link
Contributor Author

/run pipeline

@Ak-sky
Copy link
Member

Ak-sky commented Aug 2, 2024

/run pipeline

@Ak-sky
Copy link
Member

Ak-sky commented Aug 2, 2024

/run pipeline

@kierramarie
Copy link
Contributor Author

/run pipeline

@kierramarie
Copy link
Contributor Author

/run pipeline

@kierramarie
Copy link
Contributor Author

/run pipeline

@ocofaigh ocofaigh merged commit 19bcf33 into main Aug 13, 2024
2 checks passed
@ocofaigh ocofaigh deleted the ksearle-ext-kms-key branch August 13, 2024 16:40
@terraform-ibm-modules-ops
Copy link
Contributor

🎉 This issue has been resolved in version 1.10.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.

4 participants