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: alternate approach for external crn support #547

Merged
merged 19 commits into from
Sep 11, 2023
Merged

Conversation

Aashiq-J
Copy link
Member

@Aashiq-J Aashiq-J commented Aug 24, 2023

Description

Issue: #510
In this alternate approach, we can pass the keys to be created and key_crn of already created keys in the list of keys. For example,

  "key_management": {
    "access_tags": [],
    "keys": [
      {
        "key_ring": "slz-slz-ring",
        "name": "slz-vsi-volume-key",
        "root_key": true
      },
      {
        "name": "sample",
        "crn": "xxxx-xxxx-xxxx-xxxx"
      }
    ],
    "name": "slz-kms",
    "resource_group": "slz-service-rg",
    "use_hs_crypto": false
  },

In this example, the slz-vsi-volume-key key will be created in slz-kms key-protect and sample will not be created as it is an existing key. And the user can use the sample key like rest of the keys in SLZ.

Disclaimer: If the user is using a existing_key_crn the user has to manage the auth policy for the access to the kms.

Release required?

  • No release
  • Patch release (x.x.X)
  • Minor release (x.X.x)
  • Major release (X.x.x)
Release notes content
  "key_management": {
      "keys": [
          {
              "key_ring": "slz-slz-ring",
              "name": "slz-slz-key",
              "root_key": true
          },
          {
              "name": "slz-atracker-key",
              "existing_key_crn": "xxxx-xxx-xxx"
          }
      ],
      "name": "slz-slz-kms",
      "resource_group": "slz-service-rg",
      "use_hs_crypto": false
  }

You can now use keys that you created outside this module or from different accounts. You specify the key CRN in the "existing_key_crn" field. When using an existing key crn, user must have an authentication policy that allows the block-storage, cloud-object-storage and secrets-manager to access the Key Management Service in the external account.

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.

Merge actions for mergers

  • When merging, use a relevant conventional commit message that is based on the PR contents and any release notes provided by the PR author. The commit message determines whether a new version of the module is needed, and if so, which semver increment to use (major, minor, or patch).
  • Merge by using "Squash and merge".

@Aashiq-J
Copy link
Member Author

/run pipeline

@Aashiq-J
Copy link
Member Author

/run pipeline

@Aashiq-J
Copy link
Member Author

/run pipeline

@Aashiq-J
Copy link
Member Author

/run pipeline

@Aashiq-J
Copy link
Member Author

/run pipeline

1 similar comment
@Aashiq-J
Copy link
Member Author

/run pipeline

@Aashiq-J Aashiq-J marked this pull request as ready for review August 30, 2023 13:57
@Aashiq-J
Copy link
Member Author

/run pipeline

3 similar comments
@Aashiq-J
Copy link
Member Author

/run pipeline

@Aashiq-J
Copy link
Member Author

/run pipeline

@Aashiq-J
Copy link
Member Author

/run pipeline

@Aashiq-J
Copy link
Member Author

/run pipeline

Copy link
Contributor

@SirSpidey SirSpidey left a comment

Choose a reason for hiding this comment

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

@Aashiq-J

I think these pieces of content would be helpful:

  • variables.tf: If you can't have a description for parts of the object, then include a comment on the existing_key_crn property. Idenitfy which properties are mutually exclusive (in other words, you don't identify both the name and the crn in the same object, right?)
  • Does this warrant an example? Or content in the readme for an existing example?
  • Good release notes (see below)

Perhaps a release note like this?

You can now use keys that you created outside this module (or is it from different accounts, or is it different instances). You specify the key CRN in the "existing_key_crn" field. The existing key must have an authentication policy that allows the service (which service?) to access the Key Management Service in the external account.

Copy link
Contributor

@toddgiguere toddgiguere left a comment

Choose a reason for hiding this comment

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

@Aashiq-J
The code changes look good from a terraform standpoint, one of the biggest changes here is that we are switching many of the key.id inputs to key.crn inputs for many of the provider calls.

Have we confirmed that all of the resource blocks are ok with crn instead of the id? Have we done a test with a cross-account key and verified that the resources (cos buckets, boot keys of vsi/cluster etc) are using the correct keys for encryption?

@Aashiq-J
Copy link
Member Author

Aashiq-J commented Sep 4, 2023

Hi, @toddgiguere , @SirSpidey

I have tested it and it works with crn. Maybe I can post some screenshots of the test.
I thought of adding a test for it, but to test it we need to add auth policies or create an automated way to add the auth policies in the kms account.

@Aashiq-J
Copy link
Member Author

Aashiq-J commented Sep 5, 2023

/run pipeline

@Aashiq-J
Copy link
Member Author

Aashiq-J commented Sep 5, 2023

/run pipeline

SirSpidey
SirSpidey previously approved these changes Sep 5, 2023
Copy link
Contributor

@SirSpidey SirSpidey left a comment

Choose a reason for hiding this comment

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

I realize now that both name and existing CRN are allowed. So I don't have any more comments.

@Aashiq-J
Copy link
Member Author

Aashiq-J commented Sep 6, 2023

/run pipeline

3 similar comments
@Aashiq-J
Copy link
Member Author

Aashiq-J commented Sep 6, 2023

/run pipeline

@Aashiq-J
Copy link
Member Author

Aashiq-J commented Sep 7, 2023

/run pipeline

@Aashiq-J
Copy link
Member Author

Aashiq-J commented Sep 7, 2023

/run pipeline

@Aashiq-J
Copy link
Member Author

Aashiq-J commented Sep 7, 2023

/run pipeline

@jor2
Copy link
Member

jor2 commented Sep 7, 2023

/run pipeline

2 similar comments
@Aashiq-J
Copy link
Member Author

Aashiq-J commented Sep 8, 2023

/run pipeline

@Aashiq-J
Copy link
Member Author

Aashiq-J commented Sep 8, 2023

/run pipeline

@Aashiq-J
Copy link
Member Author

Aashiq-J commented Sep 8, 2023

/run pipeline

@ocofaigh
Copy link
Member

ocofaigh commented Sep 8, 2023

@Aashiq-J probably no point in constantly rebasing and triggering pipeline as every pipeline run is provisioning resources in our account and adding to cost + quotas. Lets get the PR reviewed and approved and then trigger the pipeline. I was off when you guys talked about this so perhaps @toddgiguere you can give it a final review please?

@Aashiq-J
Copy link
Member Author

/run pipeline

@jojustin
Copy link
Member

I see Todd has approved this PR. Just merging this.

@jojustin jojustin merged commit 1c27943 into main Sep 11, 2023
@jojustin jojustin deleted the alt-approach branch September 11, 2023 11:58
@terraform-ibm-modules-ops
Copy link
Contributor

🎉 This PR is included in version 4.9.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.

7 participants