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

Add encryption support #34

Merged

Conversation

06kellyjac
Copy link
Contributor

@06kellyjac 06kellyjac commented Feb 5, 2020

Resolves #27

This PR allows for users to specify the default_kms_key_name attribute to enable encryption.

Encryption can be enabled on a bucket by bucket basis similar to the existing settings: versioning and force_delete

The TF handles for empty strings because while an empty string is a valid input that results in a normal google managed key bucket, terraform will see this as a change every terraform apply.
There is no enable = false attribute option to have the encryption block ignored.
See the bottom of the PR for an example & plan demonstrating the issue

(Also bumps the version in requirements to match the compatibility section)


Example Usage:

Here bucket a will have CMEK encryption and bucket b will have the default google managed encryption

module "gcs_buckets" {
  source     = "./some/location/terraform-google-cloud-storage"
  project_id = local.project_id
  prefix     = "test"
  names      = [ "a", "b" ]

  encryption_key_names = {
    a: data.google_kms_crypto_key.key.self_link
  }
}

Resulting Plan:

An execution plan has been generated and is shown below.
Resource actions are indicated with the following symbols:
  + create

Terraform will perform the following actions:

  # module.gcs_buckets.google_storage_bucket.buckets[0] will be created
  + resource "google_storage_bucket" "buckets" {
      + bucket_policy_only = true
      + force_destroy      = false
      + id                 = (known after apply)
      + labels             = {
          + "name" = "test-eu-a"
        }
      + location           = "EU"
      + name               = "test-eu-a"
      + project            = "<REDACTED>"
      + self_link          = (known after apply)
      + storage_class      = "MULTI_REGIONAL"
      + url                = (known after apply)

      + encryption {
          + default_kms_key_name = "<REDACTED>"
        }

      + versioning {
          + enabled = false
        }
    }

  # module.gcs_buckets.google_storage_bucket.buckets[1] will be created
  + resource "google_storage_bucket" "buckets" {
      + bucket_policy_only = true
      + force_destroy      = false
      + id                 = (known after apply)
      + labels             = {
          + "name" = "test-eu-b"
        }
      + location           = "EU"
      + name               = "test-eu-b"
      + project            = "<REDACTED>"
      + self_link          = (known after apply)
      + storage_class      = "MULTI_REGIONAL"
      + url                = (known after apply)

      + versioning {
          + enabled = false
        }
    }

Example of the re-apply issue:

resource "google_storage_bucket" "bucket" {
  project  = local.project_id
  name     = "test-empty-encryption-block"
  location = "EU"

  encryption {
    default_kms_key_name = ""
  }
}

First plan:

An execution plan has been generated and is shown below.
Resource actions are indicated with the following symbols:
  + create

Terraform will perform the following actions:

  # google_storage_bucket.bucket will be created
  + resource "google_storage_bucket" "bucket" {
      + bucket_policy_only = (known after apply)
      + force_destroy      = false
      + id                 = (known after apply)
      + location           = "EU"
      + name               = "test-empty-encryption-block"
      + project            = "<REDACTED>"
      + self_link          = (known after apply)
      + storage_class      = "STANDARD"
      + url                = (known after apply)

      + encryption {}
    }

Every repeat plan:

An execution plan has been generated and is shown below.
Resource actions are indicated with the following symbols:
  ~ update in-place

Terraform will perform the following actions:

  # google_storage_bucket.bucket will be updated in-place
  ~ resource "google_storage_bucket" "bucket" {
        bucket_policy_only = false
        force_destroy      = false
        id                 = "test-empty-encryption-block"
        labels             = {}
        location           = "EU"
        name               = "test-empty-encryption-block"
        project            = "<REDACTED>"
        requester_pays     = false
        self_link          = "https://www.googleapis.com/storage/v1/b/test-empty-encryption-block"
        storage_class      = "STANDARD"
        url                = "gs://test-empty-encryption-block"

      + encryption {}
    }

The encryption block is dynamic to avoid adding an empty block.
An empty encryption block results in terraform expecting changes every apply.
Separate keys can be used for each bucket similar to versioning and force_destroy.
@06kellyjac
Copy link
Contributor Author

I've ran the docs and linting jobs from the Makefile but not the integration tests etc.

@morgante morgante self-requested a review February 5, 2020 15:45
Copy link
Contributor

@morgante morgante left a comment

Choose a reason for hiding this comment

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

Thanks for adding this! Just a few minor nits, but overall looks good/fine.

variables.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
Co-Authored-By: Morgante Pell <morgante.pell@morgante.net>
@06kellyjac
Copy link
Contributor Author

I pushed earlier but I didn't notice it failed because of the commit I made with the GH suggestion.
😅

encryption => encryption_key_names
@06kellyjac 06kellyjac force-pushed the 27_add_encryption_support branch from 9918fbe to 9914446 Compare February 5, 2020 18:40
@06kellyjac
Copy link
Contributor Author

Force pushed the update to the README I forgot to add with the make command

Copy link
Contributor

@morgante morgante left a comment

Choose a reason for hiding this comment

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

Thanks for the addition! Also, if you get a chance, it'd be helpful to add this to one of the examples/tests (but that's an optional follow-up).

@morgante morgante merged commit 32eff9b into terraform-google-modules:master Feb 5, 2020
@06kellyjac 06kellyjac deleted the 27_add_encryption_support branch February 5, 2020 19:33
@06kellyjac
Copy link
Contributor Author

I've already got most of an example done but it needs a couple tweaks.

I haven't looked much at the tests just yet.

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.

Add encryption support
2 participants