Fix multiple longstanding issues

I have reviewed a few of the longstanding issues for this module and have put together this MR to try and move some of them forward.

Issue #7 (closed)

This is a problem in particular when the sql_instance_connection_name variable references a value which is only known after apply. This is always the case when we deploy our boilerplate workspaces for the first time and requires targeted apply steps to work around.

My resolution to this is to simply create a new boolean variable named grant_sql_client_role_to_webapp_sa and use this instead of the sql_instance_connection_name variable to determine whether to grant the SQL role. This will require a change to the boilerplate to add the new variable to the webapp module call and set it to true as the default is false.

Issue #35 (closed)

dashboard.tf uses the deprecated hashicorp/template provider. I have simply refactored to use the built-in templatefile function instead.

Issue #37 (closed)

This issue is because in Terraform 1.0 provider aliases are now to be specified in the terraform.required_provider[name].configuration_aliases list. In the comments of the issue there is a mention that by moving to the new syntax the aliased stackdriver provider becomes required when calling the module, even if you're not deploying the resources which use the provider (i.e. with monitoring disabled). However, I believe that this is just something we will have to accept as it is more important to move away from using the deprecated syntax. Also, there is a simple workaround of just assigning the default google provider to the stackdriver alias in the module call as follows.

module "webapp" {
  source = "git::https://gitlab.developers.cam.ac.uk/uis/devops/infra/terraform/gcp-cloud-run-app.git?ref=5.0.0"
  
  # ...

  providers = {
    google.stackdriver = google
  }
}

Issue #38 (closed)

A map cannot have a key which depends on a resource which is unknown. To resolve this issue I've refactored the map to always have known key names.

NOTE

https://gitlab.developers.cam.ac.uk/uis/devops/gcp-deploy-boilerplate/-/merge_requests/29 will be required once this MR is merged and a new 6.0.0 tag created.

Closes #7 (closed), #35 (closed), #37 (closed), & #38 (closed)

assigned to @rk725

marked this merge request as ready

changed the description

LGTM

approved this merge request

merged

mentioned in commit de5e93f4

requested review from @rh841

@rk725 remember to add changes to the changelog and create a new version (under 5.X as is just bugfixes) and a new release https://gitlab.developers.cam.ac.uk/uis/devops/infra/terraform/gcp-cloud-run-app/-/releases

@amc203 I'll update the changelog and create a release, no worries.

However, the change to use the new variable grant_sql_client_role_to_webapp_sa to grant the SQL perms to the webapp service account could be a breaking change. Given that grant_sql_client_role_to_webapp_sa defaults to false, if someone starts using the new version of the module without adding grant_sql_client_role_to_webapp_sa = true to their module call Terraform will try and remove the SQL permissions on the next apply. This could obviously break things if the user is not fully aware of what is happening.

Happy to go with 5.x if you'd rather but thought I'd mention this first.

oh, I see. What was the reason to change the default?

In that case, as it is a breaking change, let's bump it to the next major version.

It wasn't really "changing" the default value, more deciding on what the default value should be. Previously the module was trying to decide whether to grant the role based on count = (var.sql_instance_connection_name != "") ? 1 : 0. So, if you specify a sql connection you must want the role to be granted. Which made sense but was problematic when var.sql_instance_connection_name was "unknown until after apply", which is always the case on the first run of our boilerplate for a specific workspace, and required -target-ed apply steps to get the workspace up and running.

The simplest way I could see to resolve this was to just grant the role based on a boolean variable (grant_sql_client_role_to_webapp_sa), however, if you're not deploying SQL then the principal of least privilege would suggest the default for this new variable should be false to avoid the webapp service account having unnecessary permissions.

This was my thinking behind the change, happy to tweak if needed.

All looks good, thanks for explaining!