From 082c63dfc5376abb420e77f50e8eb72c3fe84c59 Mon Sep 17 00:00:00 2001 From: Ludovico Magnocavallo Date: Thu, 22 Dec 2022 12:27:09 +0100 Subject: [PATCH] refactor GCS module and tests (#1066) --- modules/gcs/README.md | 57 ++++++++--------- modules/gcs/main.tf | 44 +++++++------- modules/gcs/variables.tf | 84 +++++++++++++++++--------- tests/modules/gcs/common.tfvars | 13 ++++ tests/modules/gcs/fixture/main.tf | 29 --------- tests/modules/gcs/fixture/variables.tf | 77 ----------------------- tests/modules/gcs/iam.tfvars | 3 + tests/modules/gcs/iam.yaml | 30 +++++++++ tests/modules/gcs/prefix.tfvars | 1 + tests/modules/gcs/prefix.yaml | 44 ++++++++++++++ tests/modules/gcs/test_plan.py | 54 ----------------- tests/modules/gcs/tftest.yaml | 20 ++++++ 12 files changed, 215 insertions(+), 241 deletions(-) create mode 100644 tests/modules/gcs/common.tfvars delete mode 100644 tests/modules/gcs/fixture/main.tf delete mode 100644 tests/modules/gcs/fixture/variables.tf create mode 100644 tests/modules/gcs/iam.tfvars create mode 100644 tests/modules/gcs/iam.yaml create mode 100644 tests/modules/gcs/prefix.tfvars create mode 100644 tests/modules/gcs/prefix.yaml delete mode 100644 tests/modules/gcs/test_plan.py create mode 100644 tests/modules/gcs/tftest.yaml diff --git a/modules/gcs/README.md b/modules/gcs/README.md index acffd2e1..439b4522 100644 --- a/modules/gcs/README.md +++ b/modules/gcs/README.md @@ -1,4 +1,5 @@ # Google Cloud Storage Module + ## Example ```hcl @@ -41,12 +42,10 @@ module "bucket" { iam = { "roles/storage.admin" = ["group:storage@example.com"] } - retention_policy = { retention_period = 100 is_locked = true } - logging_config = { log_bucket = var.bucket log_object_prefix = null @@ -63,32 +62,26 @@ module "bucket" { project_id = "myproject" prefix = "test" name = "my-bucket" - iam = { "roles/storage.admin" = ["group:storage@example.com"] } - - lifecycle_rule = { - action = { - type = "SetStorageClass" - storage_class = "STANDARD" - } - condition = { - age = 30 - created_before = null - with_state = null - matches_storage_class = null - num_newer_versions = null - custom_time_before = null - days_since_custom_time = null - days_since_noncurrent_time = null - noncurrent_time_before = null + lifecycle_rules = { + lr-0 = { + action = { + type = "SetStorageClass" + storage_class = "STANDARD" + } + condition = { + age = 30 + } } } } # tftest modules=1 resources=2 ``` + ### Minimal example with GCS notifications + ```hcl module "bucket-gcs-notification" { source = "./fabric/modules/gcs" @@ -112,23 +105,23 @@ module "bucket-gcs-notification" { | name | description | type | required | default | |---|---|:---:|:---:|:---:| -| [name](variables.tf#L89) | Bucket name suffix. | string | ✓ | | -| [project_id](variables.tf#L117) | Bucket project id. | string | ✓ | | -| [cors](variables.tf#L17) | CORS configuration for the bucket. Defaults to null. | object({…}) | | null | +| [name](variables.tf#L116) | Bucket name suffix. | string | ✓ | | +| [project_id](variables.tf#L145) | Bucket project id. | string | ✓ | | +| [cors](variables.tf#L17) | CORS configuration for the bucket. Defaults to null. | object({…}) | | null | | [encryption_key](variables.tf#L28) | KMS key that will be used for encryption. | string | | null | | [force_destroy](variables.tf#L34) | Optional map to set force destroy keyed by name, defaults to false. | bool | | false | | [iam](variables.tf#L40) | IAM bindings in {ROLE => [MEMBERS]} format. | map(list(string)) | | {} | | [labels](variables.tf#L46) | Labels to be attached to all buckets. | map(string) | | {} | -| [lifecycle_rule](variables.tf#L52) | Bucket lifecycle rule. | object({…}) | | null | -| [location](variables.tf#L74) | Bucket location. | string | | "EU" | -| [logging_config](variables.tf#L80) | Bucket logging configuration. | object({…}) | | null | -| [notification_config](variables.tf#L94) | GCS Notification configuration. | object({…}) | | null | -| [prefix](variables.tf#L107) | Optional prefix used to generate the bucket name. | string | | null | -| [retention_policy](variables.tf#L122) | Bucket retention policy. | object({…}) | | null | -| [storage_class](variables.tf#L131) | Bucket storage class. | string | | "MULTI_REGIONAL" | -| [uniform_bucket_level_access](variables.tf#L141) | Allow using object ACLs (false) or not (true, this is the recommended behavior) , defaults to true (which is the recommended practice, but not the behavior of storage API). | bool | | true | -| [versioning](variables.tf#L147) | Enable versioning, defaults to false. | bool | | false | -| [website](variables.tf#L153) | Bucket website. | object({…}) | | null | +| [lifecycle_rules](variables.tf#L52) | Bucket lifecycle rule. | map(object({…})) | | {} | +| [location](variables.tf#L101) | Bucket location. | string | | "EU" | +| [logging_config](variables.tf#L107) | Bucket logging configuration. | object({…}) | | null | +| [notification_config](variables.tf#L121) | GCS Notification configuration. | object({…}) | | null | +| [prefix](variables.tf#L135) | Optional prefix used to generate the bucket name. | string | | null | +| [retention_policy](variables.tf#L150) | Bucket retention policy. | object({…}) | | null | +| [storage_class](variables.tf#L159) | Bucket storage class. | string | | "MULTI_REGIONAL" | +| [uniform_bucket_level_access](variables.tf#L169) | Allow using object ACLs (false) or not (true, this is the recommended behavior) , defaults to true (which is the recommended practice, but not the behavior of storage API). | bool | | true | +| [versioning](variables.tf#L175) | Enable versioning, defaults to false. | bool | | false | +| [website](variables.tf#L181) | Bucket website. | object({…}) | | null | ## Outputs diff --git a/modules/gcs/main.tf b/modules/gcs/main.tf index 960b23d2..68b77077 100644 --- a/modules/gcs/main.tf +++ b/modules/gcs/main.tf @@ -75,22 +75,25 @@ resource "google_storage_bucket" "bucket" { } dynamic "lifecycle_rule" { - for_each = var.lifecycle_rule == null ? [] : [""] + for_each = var.lifecycle_rules + iterator = rule content { action { - type = var.lifecycle_rule.action["type"] - storage_class = var.lifecycle_rule.action["storage_class"] + type = rule.value.action.type + storage_class = rule.value.action.storage_class } condition { - age = var.lifecycle_rule.condition["age"] - created_before = var.lifecycle_rule.condition["created_before"] - with_state = var.lifecycle_rule.condition["with_state"] - matches_storage_class = var.lifecycle_rule.condition["matches_storage_class"] - num_newer_versions = var.lifecycle_rule.condition["num_newer_versions"] - custom_time_before = var.lifecycle_rule.condition["custom_time_before"] - days_since_custom_time = var.lifecycle_rule.condition["days_since_custom_time"] - days_since_noncurrent_time = var.lifecycle_rule.condition["days_since_noncurrent_time"] - noncurrent_time_before = var.lifecycle_rule.condition["noncurrent_time_before"] + age = rule.value.condition.age + created_before = rule.value.condition.created_before + custom_time_before = rule.value.condition.custom_time_before + days_since_custom_time = rule.value.condition.days_since_custom_time + days_since_noncurrent_time = rule.value.condition.days_since_noncurrent_time + matches_prefix = rule.value.condition.matches_prefix + matches_storage_class = rule.value.condition.matches_storage_class + matches_suffix = rule.value.condition.matches_suffix + noncurrent_time_before = rule.value.condition.noncurrent_time_before + num_newer_versions = rule.value.condition.num_newer_versions + with_state = rule.value.condition.with_state } } } @@ -104,15 +107,14 @@ resource "google_storage_bucket_iam_binding" "bindings" { } resource "google_storage_notification" "notification" { - count = local.notification ? 1 : 0 - bucket = google_storage_bucket.bucket.name - payload_format = var.notification_config.payload_format - topic = google_pubsub_topic.topic[0].id - event_types = var.notification_config.event_types - custom_attributes = var.notification_config.custom_attributes - - depends_on = [google_pubsub_topic_iam_binding.binding] - + count = local.notification ? 1 : 0 + bucket = google_storage_bucket.bucket.name + payload_format = var.notification_config.payload_format + topic = google_pubsub_topic.topic[0].id + custom_attributes = var.notification_config.custom_attributes + event_types = var.notification_config.event_types + object_name_prefix = var.notification_config.object_name_prefix + depends_on = [google_pubsub_topic_iam_binding.binding] } resource "google_pubsub_topic_iam_binding" "binding" { count = local.notification ? 1 : 0 diff --git a/modules/gcs/variables.tf b/modules/gcs/variables.tf index 2e151723..58aec4df 100644 --- a/modules/gcs/variables.tf +++ b/modules/gcs/variables.tf @@ -17,10 +17,10 @@ variable "cors" { description = "CORS configuration for the bucket. Defaults to null." type = object({ - origin = list(string) - method = list(string) - response_header = list(string) - max_age_seconds = number + origin = optional(list(string)) + method = optional(list(string)) + response_header = optional(list(string)) + max_age_seconds = optional(number) }) default = null } @@ -49,26 +49,53 @@ variable "labels" { default = {} } -variable "lifecycle_rule" { +variable "lifecycle_rules" { description = "Bucket lifecycle rule." - type = object({ + type = map(object({ action = object({ type = string - storage_class = string + storage_class = optional(string) }) condition = object({ - age = number - created_before = string - with_state = string - matches_storage_class = list(string) - num_newer_versions = string - custom_time_before = string - days_since_custom_time = string - days_since_noncurrent_time = string - noncurrent_time_before = string + age = optional(number) + created_before = optional(string) + custom_time_before = optional(string) + days_since_custom_time = optional(number) + days_since_noncurrent_time = optional(number) + matches_prefix = optional(list(string)) + matches_storage_class = optional(list(string)) # STANDARD, MULTI_REGIONAL, REGIONAL, NEARLINE, COLDLINE, ARCHIVE, DURABLE_REDUCED_AVAILABILITY + matches_suffix = optional(list(string)) + noncurrent_time_before = optional(string) + num_newer_versions = optional(number) + with_state = optional(string) # "LIVE", "ARCHIVED", "ANY" }) - }) - default = null + })) + default = {} + nullable = false + validation { + condition = alltrue([ + for k, v in var.lifecycle_rules : v.action != null && v.condition != null + ]) + error_message = "Lifecycle rules action and condition cannot be null." + } + validation { + condition = alltrue([ + for k, v in var.lifecycle_rules : contains( + ["Delete", "SetStorageClass", "AbortIncompleteMultipartUpload"], + v.action.type + ) + ]) + error_message = "Lifecycle rules action type has unsupported value." + } + validation { + condition = alltrue([ + for k, v in var.lifecycle_rules : + v.action.type != "SetStorageClass" + || + v.action.storage_class != null + ]) + error_message = "Lifecycle rules with action type SetStorageClass require a storage class." + } } variable "location" { @@ -81,7 +108,7 @@ variable "logging_config" { description = "Bucket logging configuration." type = object({ log_bucket = string - log_object_prefix = string + log_object_prefix = optional(string) }) default = null } @@ -94,12 +121,13 @@ variable "name" { variable "notification_config" { description = "GCS Notification configuration." type = object({ - enabled = bool - payload_format = string - topic_name = string - sa_email = string - event_types = list(string) - custom_attributes = map(string) + enabled = bool + payload_format = string + topic_name = string + sa_email = string + event_types = optional(list(string)) + custom_attributes = optional(map(string)) + object_name_prefix = optional(string) }) default = null } @@ -123,7 +151,7 @@ variable "retention_policy" { description = "Bucket retention policy." type = object({ retention_period = number - is_locked = bool + is_locked = optional(bool) }) default = null } @@ -153,8 +181,8 @@ variable "versioning" { variable "website" { description = "Bucket website." type = object({ - main_page_suffix = string - not_found_page = string + main_page_suffix = optional(string) + not_found_page = optional(string) }) default = null } diff --git a/tests/modules/gcs/common.tfvars b/tests/modules/gcs/common.tfvars new file mode 100644 index 00000000..5bab53b2 --- /dev/null +++ b/tests/modules/gcs/common.tfvars @@ -0,0 +1,13 @@ +force_destroy = true +labels = { environment = "test" } +logging_config = { + log_bucket = "foo" +} +name = "test" +project_id = "test-project" +retention_policy = { + retention_period = 5 + is_locked = false +} +storage_class = "MULTI_REGIONAL" +versioning = true diff --git a/tests/modules/gcs/fixture/main.tf b/tests/modules/gcs/fixture/main.tf deleted file mode 100644 index ea2e994f..00000000 --- a/tests/modules/gcs/fixture/main.tf +++ /dev/null @@ -1,29 +0,0 @@ -/** - * Copyright 2022 Google LLC - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -module "test" { - source = "../../../../modules/gcs" - project_id = "my-project" - uniform_bucket_level_access = var.uniform_bucket_level_access - force_destroy = var.force_destroy - iam = var.iam - labels = var.labels - logging_config = var.logging_config - name = "bucket-a" - prefix = var.prefix - retention_policy = var.retention_policy - versioning = var.versioning -} diff --git a/tests/modules/gcs/fixture/variables.tf b/tests/modules/gcs/fixture/variables.tf deleted file mode 100644 index 455d9a4b..00000000 --- a/tests/modules/gcs/fixture/variables.tf +++ /dev/null @@ -1,77 +0,0 @@ -/** - * Copyright 2022 Google LLC - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -variable "uniform_bucket_level_access" { - type = bool - default = false -} - -variable "force_destroy" { - type = bool - default = true -} - -variable "iam" { - type = map(list(string)) - default = {} -} - -variable "labels" { - type = map(string) - default = { environment = "test" } -} - -variable "logging_config" { - type = object({ - log_bucket = string - log_object_prefix = string - }) - default = { - log_bucket = "foo" - log_object_prefix = null - } -} - -variable "prefix" { - type = string - default = null -} - -variable "project_id" { - type = string - default = "my-project" -} - -variable "retention_policy" { - type = object({ - retention_period = number - is_locked = bool - }) - default = { - retention_period = 5 - is_locked = false - } -} - -variable "storage_class" { - type = string - default = "MULTI_REGIONAL" -} - -variable "versioning" { - type = bool - default = true -} diff --git a/tests/modules/gcs/iam.tfvars b/tests/modules/gcs/iam.tfvars new file mode 100644 index 00000000..cfb3a014 --- /dev/null +++ b/tests/modules/gcs/iam.tfvars @@ -0,0 +1,3 @@ +iam = { + "roles/storage.admin" = ["user:a@example.org"] +} diff --git a/tests/modules/gcs/iam.yaml b/tests/modules/gcs/iam.yaml new file mode 100644 index 00000000..8a85a4bd --- /dev/null +++ b/tests/modules/gcs/iam.yaml @@ -0,0 +1,30 @@ +# Copyright 2022 Google LLC +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +values: + google_storage_bucket.bucket: + name: test + + google_storage_bucket_iam_binding.bindings["roles/storage.admin"]: + bucket: test + condition: [] + members: + - user:a@example.org + role: roles/storage.admin + +counts: + google_storage_bucket: 1 + google_storage_bucket_iam_binding: 1 + modules: 0 + resources: 2 diff --git a/tests/modules/gcs/prefix.tfvars b/tests/modules/gcs/prefix.tfvars new file mode 100644 index 00000000..0031d561 --- /dev/null +++ b/tests/modules/gcs/prefix.tfvars @@ -0,0 +1 @@ +prefix = "foo" diff --git a/tests/modules/gcs/prefix.yaml b/tests/modules/gcs/prefix.yaml new file mode 100644 index 00000000..6baee4a1 --- /dev/null +++ b/tests/modules/gcs/prefix.yaml @@ -0,0 +1,44 @@ +# Copyright 2022 Google LLC +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +values: + google_storage_bucket.bucket: + force_destroy: true + labels: + environment: test + location: EU + logging: + - log_bucket: foo + name: foo-test + project: test-project + retention_policy: + - is_locked: false + retention_period: 5 + storage_class: MULTI_REGIONAL + uniform_bucket_level_access: true + versioning: + - enabled: true + +counts: + google_storage_bucket: 1 + modules: 0 + resources: 1 + +outputs: + bucket: __missing__ + id: foo-test + name: foo-test + notification: null + topic: null + url: __missing__ diff --git a/tests/modules/gcs/test_plan.py b/tests/modules/gcs/test_plan.py deleted file mode 100644 index 22775a58..00000000 --- a/tests/modules/gcs/test_plan.py +++ /dev/null @@ -1,54 +0,0 @@ -# Copyright 2022 Google LLC -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. - -def test_buckets(plan_runner): - "Test bucket resources." - _, resources = plan_runner() - assert len(resources) == 1 - r = resources[0] - assert r['type'] == 'google_storage_bucket' - assert r['values']['name'] == 'bucket-a' - assert r['values']['project'] == 'my-project' - - -def test_prefix(plan_runner): - "Test bucket name when prefix is set." - _, resources = plan_runner(prefix='foo') - assert resources[0]['values']['name'] == 'foo-bucket-a' - - -def test_config_values(plan_runner): - "Test that variables set the correct attributes on buckets." - variables = dict( - uniform_bucket_level_access='true', - force_destroy='true', - versioning='true' - ) - _, resources = plan_runner(**variables) - assert len(resources) == 1 - r = resources[0] - assert r['values']['uniform_bucket_level_access'] is True - assert r['values']['force_destroy'] is True - assert r['values']['versioning'] == [{'enabled': True}] - assert r['values']['logging'] == [{'log_bucket': 'foo'}] - assert r['values']['retention_policy'] == [ - {'is_locked': False, 'retention_period': 5} - ] - - -def test_iam(plan_runner): - "Test bucket resources with iam roles and members." - iam = '{ "roles/storage.admin" = ["user:a@b.com"] }' - _, resources = plan_runner(iam=iam) - assert len(resources) == 2 diff --git a/tests/modules/gcs/tftest.yaml b/tests/modules/gcs/tftest.yaml new file mode 100644 index 00000000..22337d18 --- /dev/null +++ b/tests/modules/gcs/tftest.yaml @@ -0,0 +1,20 @@ +# Copyright 2022 Google LLC +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +module: modules/gcs +common_tfvars: + - common.tfvars +tests: + prefix: + iam: