terraform-google-modules / terraform-example-foundation

Shows how the CFT modules can be composed to build a secure cloud foundation
https://cloud.google.com/architecture/security-foundations
Apache License 2.0
1.22k stars 714 forks source link

chore(monitoring): remove unused monitoring project #1200

Closed eeaton closed 4 months ago

eeaton commented 5 months ago

Earlier versions of the blueprint created a monitoring project per environment, with the assumption that this could be used for creating an environment-wide metrics scoping project which would then be used by other blueprints (Enterprise Application Blueprint, MLOps blueprint). However, we haven't identified any tangible use cases for the environment wide monitoring projects, and other blueprints do not require them. The useful scope for monitoring projects will vary based on how teams monitoring related groups of workloads/applications, and requires signficicant planning and design specific to their operations, so we've decided the generic empty monitoring project does not provide significant value to a general foundation pattern.

Summary of changes: ~remove unusedmonitoring projects and other references in code ~change bootstrap steps that configure monitoring_workspace_users as a required group. Move this to optional ~remove references to the monitoring projects and related inputs/outputs in Readme text

eeaton commented 4 months ago

Thanks for catching the instance in terraform.example.tfvars, I've updated that.

Re:

Since the value of monitoring_workspace_users is not used in the code, would not be better to remove it from the optional_groups and related resources?

Initially my thought was to leave it in optional_groups, because several of the groups in optional_groups are referenced in tests and outputs but aren't implemented in any IAM roles. This is something I plan to fix in v5, because the written guide recommends all the groups.

However, now that I think about it again, your suggestion makes sense. Even when we add more useful IAM bindings in v5, I don't expect we will have any use for the monitoring_workspace_users group so it's better to remove. I'll make an additional commit to remove it from tests and outputs and optional_groups etc.

KonStg commented 3 months ago

There are some residual variables on https://github.com/terraform-google-modules/terraform-example-foundation/blob/ceeead24430999d89ab18da71e9a520389a15d92/2-environments/modules/env_baseline/variables.tf#L54

monitoring_budget_amount
monitoring_alert_spent_percents
monitoring_alert_pubsub_topic
monitoring_budget_alert_spend_basis
eeaton commented 3 months ago

Thanks for the catch @KonStg . I've raised an additional PR #1281 to clean up the remaining variable definitons.