From 082301c09c1e21fdcafe1e3f6b1191cdface1a87 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wiktor=20Niesiob=C4=99dzki?= Date: Mon, 27 Mar 2023 07:49:19 +0000 Subject: [PATCH 1/4] Use unique bundle name for Cloud Function When cloud-function module is used multiple times within project and default `bundle_config.output_path` is used then all the instances try to use filename and result is undefined without guarantee to converge to desired state (i.e. multiple functions may share the same code). --- modules/cloud-function/README.md | 2 +- modules/cloud-function/main.tf | 4 ++-- modules/cloud-function/variables.tf | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/modules/cloud-function/README.md b/modules/cloud-function/README.md index ebf300fcc..d6edb6668 100644 --- a/modules/cloud-function/README.md +++ b/modules/cloud-function/README.md @@ -226,7 +226,7 @@ module "cf-http" { | name | description | type | required | default | |---|---|:---:|:---:|:---:| | [bucket_name](variables.tf#L26) | Name of the bucket that will be used for the function code. It will be created with prefix prepended if bucket_config is not null. | string | ✓ | | -| [bundle_config](variables.tf#L37) | Cloud function source folder and generated zip bundle paths. Output path defaults to '/tmp/bundle.zip' if null. | object({…}) | ✓ | | +| [bundle_config](variables.tf#L37) | Cloud function source folder and generated zip bundle paths. Output path defaults to '/tmp/bundle.zip' if null. | object({…}) | ✓ | | | [name](variables.tf#L94) | Name used for cloud function and associated resources. | string | ✓ | | | [project_id](variables.tf#L109) | Project id used for all resources. | string | ✓ | | | [bucket_config](variables.tf#L17) | Enable and configure auto-created bucket. Set fields to null to use defaults. | object({…}) | | null | diff --git a/modules/cloud-function/main.tf b/modules/cloud-function/main.tf index 35dba7294..708622343 100644 --- a/modules/cloud-function/main.tf +++ b/modules/cloud-function/main.tf @@ -267,8 +267,8 @@ resource "google_storage_bucket_object" "bundle" { data "archive_file" "bundle" { type = "zip" source_dir = var.bundle_config.source_dir - output_path = var.bundle_config.output_path - output_file_mode = "0666" + output_path = var.bundle_config.output_path != null ? var.bundle_config.output_path : "/tmp/bundle-${var.project_id}-${var.name}.zip" + output_file_mode = "0644" excludes = var.bundle_config.excludes } diff --git a/modules/cloud-function/variables.tf b/modules/cloud-function/variables.tf index 97a6217a6..5f4b28c94 100644 --- a/modules/cloud-function/variables.tf +++ b/modules/cloud-function/variables.tf @@ -38,7 +38,7 @@ variable "bundle_config" { description = "Cloud function source folder and generated zip bundle paths. Output path defaults to '/tmp/bundle.zip' if null." type = object({ source_dir = string - output_path = optional(string, "/tmp/bundle.zip") + output_path = optional(string) excludes = optional(list(string)) }) } From d105ed59d3d2e3ce41f2c13a3dc075dfd601faff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wiktor=20Niesiob=C4=99dzki?= Date: Mon, 27 Mar 2023 08:06:56 +0000 Subject: [PATCH 2/4] Resolve review comments --- modules/cloud-function/main.tf | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/cloud-function/main.tf b/modules/cloud-function/main.tf index 708622343..b5a861b2a 100644 --- a/modules/cloud-function/main.tf +++ b/modules/cloud-function/main.tf @@ -267,7 +267,7 @@ resource "google_storage_bucket_object" "bundle" { data "archive_file" "bundle" { type = "zip" source_dir = var.bundle_config.source_dir - output_path = var.bundle_config.output_path != null ? var.bundle_config.output_path : "/tmp/bundle-${var.project_id}-${var.name}.zip" + output_path = coalesce(var.bundle_config.output_path, "/tmp/bundle-${var.project_id}-${var.name}.zip") output_file_mode = "0644" excludes = var.bundle_config.excludes } From 9a0137bcfca61f564be191d38d8336d5c30a4500 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wiktor=20Niesiob=C4=99dzki?= Date: Mon, 27 Mar 2023 09:31:07 +0000 Subject: [PATCH 3/4] Add test veryfing multiple filenames are used by default --- tests/modules/cloud_function/fixture/main.tf | 8 +++---- .../cloud_function/fixture/variables.tf | 5 +++++ tests/modules/cloud_function/test_plan.py | 22 +++++++++++++++++++ 3 files changed, 31 insertions(+), 4 deletions(-) diff --git a/tests/modules/cloud_function/fixture/main.tf b/tests/modules/cloud_function/fixture/main.tf index 0096159fb..e9021610a 100644 --- a/tests/modules/cloud_function/fixture/main.tf +++ b/tests/modules/cloud_function/fixture/main.tf @@ -15,15 +15,15 @@ */ module "test" { + count = var.instance_count source = "../../../../modules/cloud-function" project_id = "my-project" - name = "test" + name = "test_${count.index}" bucket_name = var.bucket_name v2 = var.v2 bundle_config = { - source_dir = "bundle" - output_path = "bundle.zip" - excludes = null + source_dir = "bundle" + excludes = null } iam = { "roles/cloudfunctions.invoker" = ["allUsers"] diff --git a/tests/modules/cloud_function/fixture/variables.tf b/tests/modules/cloud_function/fixture/variables.tf index 192386276..fe97d9f8e 100644 --- a/tests/modules/cloud_function/fixture/variables.tf +++ b/tests/modules/cloud_function/fixture/variables.tf @@ -19,6 +19,11 @@ variable "bucket_name" { default = "test" } +variable "instance_count" { + type = number + default = 1 +} + variable "v2" { type = any default = false diff --git a/tests/modules/cloud_function/test_plan.py b/tests/modules/cloud_function/test_plan.py index 019d69ceb..940438aeb 100644 --- a/tests/modules/cloud_function/test_plan.py +++ b/tests/modules/cloud_function/test_plan.py @@ -22,6 +22,12 @@ def resources(plan_runner, version): _, resources = plan_runner(v2=v2) return resources +@pytest.fixture +def plan(plan_runner, version, count): + v2 = {'v1': 'false', 'v2': 'true'}[version] + plan, _ = plan_runner(v2=v2, instance_count=count) + return plan + @pytest.mark.parametrize('version', ['v1', 'v2']) def test_resource_count(resources): @@ -42,3 +48,19 @@ def test_iam(resources, version): assert len(bindings) == 1 assert bindings[0]['role'] == 'roles/cloudfunctions.invoker' assert bindings[0]['members'] == ['allUsers'] + + +@pytest.mark.parametrize('version', ['v1', 'v2']) +@pytest.mark.parametrize('count', [2]) +def test_multiple_functions(plan, version, count): + """Tests whether multiple use of functions result in use of multiple bundle files""" + + # data objects are only accessible in prior_state of the plan, they are not present in root_module + archives = [ + resource for child_module in plan['prior_state']["values"]["root_module"]["child_modules"] + for resource in child_module["resources"] + if resource["type"] == "archive_file" + ] + file_names = set([x["values"]["output_path"] for x in archives]) + + assert len(archives) == len(file_names) From 9005a51a9549a8978f77e2d42bfb1c552e898b60 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wiktor=20Niesiob=C4=99dzki?= Date: Mon, 27 Mar 2023 10:18:04 +0000 Subject: [PATCH 4/4] Use example testing instead custom test --- modules/cloud-function/README.md | 29 +++++++++++++++++++ .../examples/multiple_functions.yaml | 25 ++++++++++++++++ tests/modules/cloud_function/fixture/main.tf | 8 ++--- .../cloud_function/fixture/variables.tf | 5 ---- tests/modules/cloud_function/test_plan.py | 22 -------------- 5 files changed, 58 insertions(+), 31 deletions(-) create mode 100644 tests/modules/cloud_function/examples/multiple_functions.yaml diff --git a/modules/cloud-function/README.md b/modules/cloud-function/README.md index d6edb6668..4983f27f1 100644 --- a/modules/cloud-function/README.md +++ b/modules/cloud-function/README.md @@ -218,6 +218,35 @@ module "cf-http" { } } # tftest modules=1 resources=2 +``` + +### Multiple Cloud Functions within project + +When deploying multiple functions do not reuse `bundle_config.output_path` between instances as the result is undefined. Default `output_path` creates file in `/tmp` folder using project Id and function name to avoid name conflicts. + +```hcl +module "cf-http-one" { + source = "./fabric/modules/cloud-function" + project_id = "my-project" + name = "test-cf-http-one" + bucket_name = "test-cf-bundles" + bundle_config = { + source_dir = "fabric/assets" + } +} + +module "cf-http-two" { + source = "./fabric/modules/cloud-function" + project_id = "my-project" + name = "test-cf-http-two" + bucket_name = "test-cf-bundles" + bundle_config = { + source_dir = "fabric/assets" + } +} +# tftest modules=2 resources=4 inventory=multiple_functions.yaml + + ``` diff --git a/tests/modules/cloud_function/examples/multiple_functions.yaml b/tests/modules/cloud_function/examples/multiple_functions.yaml new file mode 100644 index 000000000..b9956db5b --- /dev/null +++ b/tests/modules/cloud_function/examples/multiple_functions.yaml @@ -0,0 +1,25 @@ +# Copyright 2023 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: + module.cf-http-one.google_storage_bucket_object.bundle: + source: /tmp/bundle-my-project-test-cf-http-one.zip + module.cf-http-two.google_storage_bucket_object.bundle: + source: /tmp/bundle-my-project-test-cf-http-two.zip + +counts: + google_cloudfunctions_function: 2 + google_storage_bucket_object: 2 + modules: 2 + resources: 4 diff --git a/tests/modules/cloud_function/fixture/main.tf b/tests/modules/cloud_function/fixture/main.tf index e9021610a..0096159fb 100644 --- a/tests/modules/cloud_function/fixture/main.tf +++ b/tests/modules/cloud_function/fixture/main.tf @@ -15,15 +15,15 @@ */ module "test" { - count = var.instance_count source = "../../../../modules/cloud-function" project_id = "my-project" - name = "test_${count.index}" + name = "test" bucket_name = var.bucket_name v2 = var.v2 bundle_config = { - source_dir = "bundle" - excludes = null + source_dir = "bundle" + output_path = "bundle.zip" + excludes = null } iam = { "roles/cloudfunctions.invoker" = ["allUsers"] diff --git a/tests/modules/cloud_function/fixture/variables.tf b/tests/modules/cloud_function/fixture/variables.tf index fe97d9f8e..192386276 100644 --- a/tests/modules/cloud_function/fixture/variables.tf +++ b/tests/modules/cloud_function/fixture/variables.tf @@ -19,11 +19,6 @@ variable "bucket_name" { default = "test" } -variable "instance_count" { - type = number - default = 1 -} - variable "v2" { type = any default = false diff --git a/tests/modules/cloud_function/test_plan.py b/tests/modules/cloud_function/test_plan.py index 940438aeb..019d69ceb 100644 --- a/tests/modules/cloud_function/test_plan.py +++ b/tests/modules/cloud_function/test_plan.py @@ -22,12 +22,6 @@ def resources(plan_runner, version): _, resources = plan_runner(v2=v2) return resources -@pytest.fixture -def plan(plan_runner, version, count): - v2 = {'v1': 'false', 'v2': 'true'}[version] - plan, _ = plan_runner(v2=v2, instance_count=count) - return plan - @pytest.mark.parametrize('version', ['v1', 'v2']) def test_resource_count(resources): @@ -48,19 +42,3 @@ def test_iam(resources, version): assert len(bindings) == 1 assert bindings[0]['role'] == 'roles/cloudfunctions.invoker' assert bindings[0]['members'] == ['allUsers'] - - -@pytest.mark.parametrize('version', ['v1', 'v2']) -@pytest.mark.parametrize('count', [2]) -def test_multiple_functions(plan, version, count): - """Tests whether multiple use of functions result in use of multiple bundle files""" - - # data objects are only accessible in prior_state of the plan, they are not present in root_module - archives = [ - resource for child_module in plan['prior_state']["values"]["root_module"]["child_modules"] - for resource in child_module["resources"] - if resource["type"] == "archive_file" - ] - file_names = set([x["values"]["output_path"] for x in archives]) - - assert len(archives) == len(file_names)