aws-observability / terraform-aws-observability-accelerator

Open source project to help accelerate and ease observability setup on AWS environments
https://aws-observability.github.io/terraform-aws-observability-accelerator/
Apache License 2.0
288 stars 84 forks source link

[Bug]: NGINX config variable is incompatible with underlying patterns submodule #263

Closed araguiar closed 9 months ago

araguiar commented 9 months ago

Welcome to the AWS Observability Accelerator

AWS Observability Accelerator Release version

2.12.0

What is your environment, configuration and the example used?

Terraform 1.7.2

Error occurs when running terraform plan

module "eks_monitoring" {
  source = "github.com/aws-observability/terraform-aws-observability-accelerator//modules/eks-monitoring?ref=v2.12.0"
  ...
  nginx_config = {
    enable_alerting_rules  = true
    enable_recording_rules = true
    enable_dashboards      = var.enable_dashboards
    scrape_sample_limit    = 10000

    flux_gitrepository_name   = "aws-observability-accelerator"
    flux_gitrepository_url    = "https://github.com/aws-observability/aws-observability-accelerator"
    flux_gitrepository_branch = "v0.3.2"
    flux_kustomization_name   = "grafana-dashboards-nginx"
    flux_kustomization_path   = "./artifacts/grafana-operator-manifests/eks/nginx"

    grafana_dashboard_url = "https://raw.githubusercontent.com/aws-observability/aws-observability-accelerator/v0.2.0/artifacts/grafana-dashboards/eks/nginx/nginx.json"

    prometheus_metrics_endpoint = "/metrics"
  }
  ...
}

What did you do and What did you see instead?

I passed thenginx_config variable into the eks-monitoring module and the variable configuration does not match the underlying patterns/nginx modules input variable.

Error Seen

╷
│ Error: Invalid value for input variable
│ 
│   on .terraform/modules/eks_monitoring/modules/eks-monitoring/main.tf line 238, in module "nginx_monitoring":
│  238:   pattern_config = coalesce(var.nginx_config, local.nginx_pattern_config)
│ 
│ The given value is not suitable for
│ module.eks_monitoring.module.nginx_monitoring[0].var.pattern_config
│ declared at
│ .terraform/modules/eks_monitoring/modules/eks-monitoring/patterns/nginx/variables.tf:1,1-26:
│ map has no element for required attribute
│ "managed_prometheus_workspace_id".
╵

EKS Monitoring Variable Definition

https://github.com/aws-observability/terraform-aws-observability-accelerator/blob/v2.12.0/modules/eks-monitoring/variables.tf#L340

variable "nginx_config" {
  description = "Configuration object for NGINX monitoring"
  type = object({
    enable_alerting_rules  = bool
    enable_recording_rules = bool
    enable_dashboards      = bool
    scrape_sample_limit    = number

    flux_gitrepository_name   = string
    flux_gitrepository_url    = string
    flux_gitrepository_branch = string
    flux_kustomization_name   = string
    flux_kustomization_path   = string

    grafana_dashboard_url = string

    prometheus_metrics_endpoint = string
  })

  # defaults are pre-computed in locals.tf, provide a full definition to override
  default = null
}

Nginx Patterns Variable Definition

https://github.com/aws-observability/terraform-aws-observability-accelerator/blob/v2.12.0/modules/eks-monitoring/patterns/nginx/variables.tf#L1

variable "pattern_config" {
  description = "Configuration object for Java/JMX monitoring"
  type = object({
    enable_alerting_rules  = bool
    enable_recording_rules = bool
    scrape_sample_limit    = number

    enable_dashboards = bool

    flux_gitrepository_name   = string
    flux_gitrepository_url    = string
    flux_gitrepository_branch = string
    flux_kustomization_name   = string
    flux_kustomization_path   = string

    managed_prometheus_workspace_id = string <----- Mismatch is here
    prometheus_metrics_endpoint     = string

    grafana_dashboard_url = string
  })
  nullable = false
}

Implementing Template

https://github.com/aws-observability/terraform-aws-observability-accelerator/blob/v2.12.0/modules/eks-monitoring/main.tf#L234

module "nginx_monitoring" {
  source = "./patterns/nginx"
  count  = var.enable_nginx ? 1 : 0

  pattern_config = coalesce(var.nginx_config, local.nginx_pattern_config) <---- Passes incorrect parameter if var.nginx_config is set
}

Additional Information

The nginx pattern module should be using `merge` instead of `coalesce` so that internal configurations are not lost.
Implementing the configuration is painful as well since all I needed was to set the `scrape_sample_limit` without overriding anything else.

I will open a PR to allow optional configuration of all nginx_config variable elements and pass the correct parameters to the submodule.
bonclay7 commented 9 months ago

Hey there, awesome, thanks for raising this