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.
#7 (closed)
IssueThis 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
.
#35 (closed)
Issuedashboard.tf
uses the deprecated hashicorp/template
provider. I have simply refactored to use the built-in templatefile
function instead.
#37 (closed)
IssueThis 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
}
}
#38 (closed)
IssueA 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)
Merge request reports
Activity
assigned to @rk725
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 thatgrant_sql_client_role_to_webapp_sa
defaults tofalse
, if someone starts using the new version of the module without addinggrant_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.
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 whenvar.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 befalse
to avoid the webapp service account having unnecessary permissions.This was my thinking behind the change, happy to tweak if needed.