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

Kms key bootstrap #2824

Closed
wants to merge 4 commits into from
Closed

Conversation

chrisst
Copy link
Contributor

@chrisst chrisst commented Jan 7, 2019

Add a method that will bootstrap a KMS keyring

It will either grab a well-known key or create a new one in the project
that is running the tests. This will allow reuse of KMS keys without needing
to create/delete them for all of the new resources that are starting to
support KMS integration.

Goal:
Since it looks like there is a strong push to get KMS support on many different resources in the GCP ecosystem I want to making testing those resources easier. This should help to speed up tests, reduce our project quota burden and simplify tests.

It will either grab a well-known key or create a new one in the project
that is running the tests. This will allow reuse of KMS keys without needing
to create/delete them for all of the new resources that are starting to
support KMS integration.
@ghost ghost added the size/l label Jan 7, 2019
@chrisst
Copy link
Contributor Author

chrisst commented Jan 7, 2019

This was in response to needing tests for #2772

@danawillow
Copy link
Contributor

Shouldn't this live inside MM?

@chrisst
Copy link
Contributor Author

chrisst commented Jan 7, 2019

Ah yeah, it should. I'll merge it upstream if you think the approach is sound.

@rileykarson
Copy link
Collaborator

Super high level question: Why not just require a CRYPTO_KEY_SELF_LINK env var set and fail the test if it's not provided? (Or skip, but I prefer fail so we notice it in CI if it's misconfigured) That seems a lot easier than handling creates and gets and stuff.

@chrisst
Copy link
Contributor Author

chrisst commented Jan 8, 2019

Summary of IRL conversation:
I don't think asking people to have to manually bootstrap their GCP environments with a keyring/key is good and using the ENV variable approach doesn't add a significant advantage over this approach.

Acceptance tests aren't skipped until the phase inside terraform core that
applies the configuration. Thus setup code needs to be also skipped.

This could also be moved to the pre-config step but due to variable scoping
things that are defined in pre-config aren't accesible later.
Copy link
Contributor

@danawillow danawillow left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me, just a few nits. Not approving because this PR should be in magic modules (we already copy this file over)

* testing KMS integration with other resources.
*
* This will either return an existing key or create one if it hasn't been created
* in the project yet. The motivation is because keyrings don't get deleted and we
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a really long sentence, can you clean it up a bit?

* to incur the overhead of creating a new project for each test that needs to use
* a KMS key.
**/
var SharedKeyRing = "tftest-shared-keyring-1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why "-1"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's for versioning. I added it for my own testing to make sure that new keys were getting created correctly when the constant changed. I left it because if we need to change it in future we can version bump to 2. I could check if the key regex support a 'v1' syntax.

}

if keyRing == nil {
t.Errorf("Unable to bootstrap KMS key. keyRing is nil!")
Copy link
Contributor

Choose a reason for hiding this comment

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

These two lines can be replaced with a t.Fatalf

Copy link
Collaborator

@rileykarson rileykarson left a comment

Choose a reason for hiding this comment

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

Seems sensible enough. Can other tests use this too?

*cloudkms.CryptoKey
}

func bootstrapKMSKey(t *testing.T) bootstrappedKMS {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have a slight preference for all this to live in another file, but I could be convinced either way.

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 was thinking it might be better off in a kms specific file or just a 'bootstrapping' file if we decide to use the same approach for project reuse

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds reasonable to me!

@chrisst
Copy link
Contributor Author

chrisst commented Jan 9, 2019

Duplicated by #2837

@chrisst chrisst closed this Jan 9, 2019
@ghost
Copy link

ghost commented Feb 8, 2019

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked and limited conversation to collaborators Feb 8, 2019
@chrisst chrisst deleted the kms-key-bootstrap branch July 8, 2019 22:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants