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: add autokey plus migration #156

Conversation

romanini-ciandt
Copy link
Member

@romanini-ciandt romanini-ciandt commented Sep 26, 2024

The goal of this PR is to provide instructions for customers to migrate the Terraform Key Handle state from terraform-google-autokey to autokey submodule in this repo

@romanini-ciandt romanini-ciandt changed the title feat: add autokey plus migration [WIP] feat: add autokey plus migration Sep 27, 2024
@romanini-ciandt romanini-ciandt marked this pull request as ready for review September 27, 2024 21:39
@romanini-ciandt romanini-ciandt requested a review from a team as a code owner September 27, 2024 21:39
@g-swap
Copy link

g-swap commented Sep 30, 2024

Reviewing the PR

@romanini-ciandt romanini-ciandt requested a review from g-swap October 1, 2024 17:33
Copy link

@tdbhacks tdbhacks left a comment

Choose a reason for hiding this comment

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

Nice!! Left two minor comments.

I assume this is hard / infeasible / not worth to test with an automated test, do you mind maybe sharing a sample step by step example log of this process?

scripts/create_autokey_tfvars_file.sh Outdated Show resolved Hide resolved
Comment on lines +44 to +47
lifecycle {
ignore_changes = [name]
}

Copy link

Choose a reason for hiding this comment

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

Is this needed because the import script will import the existing keyhandles into TF resources that include the random suffix in the name, but we need to ignore the diffs on apply?

Or is it unrelated and more of a general autokey module improvement, to address what we discussed offline re: users changing the keyhandle name and then running into issues when they try to change it back?

Copy link
Member Author

Choose a reason for hiding this comment

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

A short answer would be:
It is to address issues on both (when imported or not), but the change was motivated to support the import process. It would impact 100% of the imports and just eventual use cases for not imported.

More context about why:
I added this lifecycle to avoid resource replaces when importing existing key handles. When we import, this submodule will try to append a random suffix and a replacement will be forced.

But also, as we discussed offline, this change would avoid errors even when the user is not importing.
Example: The key Handle for secret manager can not be recreated, just 1 instance per project is allowed by GCP. In this case, if the name is changed in tf, terraform will "destroy" (not actually destroy, but remove it from tf state) and the attempt to create a new resource will raise an error.

So, I chose this lifecycle approach, but I also see two others alternatives:

  1. Add a boolean flag in input variables, so users can control if to append the random suffix into key Handles or not. But the con in this approach is that all the key handles would need to have the same behavior (imported or not), and also we wouldn't be preventing the secret manager issue I mentioned above.
  2. Add a new key handle tf resource specifically for import process. The con in this approach is that, beyond the tf resource duplication, we would need to specify in .tfvars file different variables names for imported and non-imported key handles

Sorry for the length of the comment... but is it clear?
Let me know what you think, or if we should change the approach here :)

Copy link

Choose a reason for hiding this comment

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

Gotcha, thanks for confirming! This all makes sense to me and seems reasonable, I'll defer to the CFT team and see if they disagree :) Thank you!

@romanini-ciandt
Copy link
Member Author

Nice!! Left two minor comments.

I assume this is hard / infeasible / not worth to test with an automated test, do you mind maybe sharing a sample step by step example log of this process?

@tdbhacks here is the step by step being executed:

  • Getting the existing Autokey state from terraform-google-autokey module
    image

  • Creating the .tfvars file
    image
    image

  • Importing the existing Autokey state from terraform-google-autokey module using autokey submodule
    image
    image
    image

  • terraform apply after the import
    image
    Note that 6 resources were added. None of them were key handles because at this point they're already imported. These are the other terraform resources that autokey submodule have (service agent activation, assign permissions to sa, Autokey config, random, etc.)

  • Cleaning your local environment
    image

Copy link

@tdbhacks tdbhacks left a comment

Choose a reason for hiding this comment

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

Awesome, thanks Leo!!

Copy link

@erlanderlo erlanderlo left a comment

Choose a reason for hiding this comment

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

one minor comment to consider, so I'll leave it up to you to address.

scripts/export_autokey_env_vars.sh Show resolved Hide resolved
@romanini-ciandt
Copy link
Member Author

@apeabody could you PTAL?

Copy link
Contributor

@apeabody apeabody left a comment

Choose a reason for hiding this comment

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

Approved by @tdbhacks & @erlanderlo

@apeabody apeabody merged commit 661c103 into terraform-google-modules:master Oct 15, 2024
4 checks passed
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.

5 participants