Domains

[siddhsuresh]

Greptile Summary

This PR introduces opt-in Ravion-managed domain/cert provisioning for ECS clusters and services, adds a new ravion_domain Terraform resource type backed by a custom provider, and refactors load_balancer_attachment from a typed Terraform variable to a JSON-decoded string to support control-plane ARN templating.

• ecs_cluster: new ravion_domains.tf allocates a wildcard ravion_domain for the cluster, binds the cert to the public ALB HTTPS listener as an SNI extra, and exposes domain outputs; module.yml adds UI inputs, dashboard metrics, and output declarations.
• ecs_service: new ravion_domains.tf implements Mode A (auto-FQDN under the cluster wildcard, with a two-phase cutover via ravion_auto_domain_status) and Mode B (per-service cert covering customer FQDNs); load_balancer_attachment variable replaced by load_balancer_attachment_json string with local decoding; module.yml added from scratch.
• Broad try(..., true) change: every variable validation in autoscaling, ecs_cluster, ecs_service, alb, nlb, and vpc is wrapped to suppress throws from complex-object attribute access.

Confidence Score: 2/5

Multiple blocking gaps exist between the module.yml control-plane definitions and the actual Terraform code — mismatched output names, mismatched input variable names, and conflicting provider version pins — that will cause apply failures or silent no-ops on every stack that uses the new Ravion domain wiring.

The module.yml files declare Ravion output names (ravion_managed_domains_enabled, ravion_public_alb_default_*) that simply do not exist in outputs.tf, and input names (ravion_parent_app_domain_id, ravion_auto_domain_listener_arn, etc.) that don't match the TF variables the resources actually read. Any downstream module referencing these outputs will receive nothing, and any upstream value the control plane passes using the module.yml keys will be silently ignored. Separately, the Ravion provider is pinned at = 0.4.3 in ecs_cluster and = 0.4.5 in ecs_service, which Terraform cannot satisfy in a shared workspace. Finally, when certificate_arns = [] (the new default set in module.yml), the ALB HTTPS listener is created with certificate_arn = null, which AWS rejects. Together these issues mean the primary new feature cannot be exercised in production without code fixes.

compute/ecs_cluster/module.yml and compute/ecs_service/module.yml (output/input name mismatches), compute/ecs_cluster/versions.tf and compute/ecs_service/versions.tf (conflicting provider version pins), networking/alb/locals.tf (null certificate_arn on HTTPS listener)

Important Files Changed

Filename	Overview
compute/ecs_cluster/versions.tf	Ravion provider pinned to = 0.4.3, conflicting with ecs_service pin of = 0.4.5; breaks any workspace using both modules
compute/ecs_service/versions.tf	Ravion provider pinned to = 0.4.5, conflicting with ecs_cluster pin of = 0.4.3
compute/ecs_cluster/module.yml	Declared Ravion output names don't match actual TF outputs; also declares private ALB Ravion outputs that have no corresponding TF resources
compute/ecs_service/module.yml	New file; UI input names (ravion_parent_app_domain_id, ravion_auto_domain_listener_arn, etc.) don't match TF variables; output names also diverge from outputs.tf
networking/alb/locals.tf	https_default_cert_arn can be null when certificate_arns is empty (now the default), causing HTTPS listener creation to fail at the AWS API level
networking/alb/listeners.tf	certificate_arn now references local.https_default_cert_arn (null-safe); SNI extras guard improved with length check; depends on ALB locals fix
compute/ecs_cluster/ravion_domains.tf	New file: allocates ravion_domain.cluster (wildcard cert) and binds it as SNI extra on public ALB HTTPS listener; preconditions guard required inputs
compute/ecs_service/ravion_domains.tf	New file implementing Mode A (auto-FQDN under cluster wildcard) and Mode B (custom cert) with a two-phase cutover mechanism via ravion_auto_domain_status data source
compute/ecs_service/locals.tf	Switches load_balancer_attachment from typed variable to JSON-decoded local; applies defaults post-decode; enable_auto_scaling guard simplified with try()
compute/ecs_service/variables.tf	load_balancer_attachment replaced with load_balancer_attachment_json string; wait_for_steady_state default changed from true to false; Ravion variables added
compute/autoscaling/variables.tf	All variable validations wrapped in try(..., true), silently swallowing errors for complex object inputs
compute/ecs_cluster/outputs.tf	Adds seven Ravion domain outputs (cluster domain id/fqdn/url, cert arn/status, aws account/region); names diverge from module.yml declarations

Sequence Diagram

sequenceDiagram
    participant CP as Control Plane
    participant Cluster as ecs_cluster module
    participant RavionAPI as Ravion Provider
    participant AWS as AWS (ACM + ALB)
    participant Service as ecs_service module

    CP->>Cluster: "use_ravion_managed_domains=true, ravion_aws_account_id"
    Cluster->>RavionAPI: "ravion_domain.cluster (wildcard cert, target=public ALB)"
    RavionAPI-->>AWS: "Issue ACM cert (*.cluster-fqdn + cluster-fqdn)"
    RavionAPI-->>Cluster: cert_arn, fqdn, id
    Cluster->>AWS: aws_lb_listener_certificate (SNI extra on HTTPS listener)
    Cluster-->>CP: ravion_cluster_domain_id, ravion_cluster_cert_arn, ...

    CP->>Service: cluster_parent_domain_id, cluster_https_listener_arn
    Service->>RavionAPI: data.ravion_auto_domain_status (check retirement)
    RavionAPI-->>Service: "retired=false"
    Service->>RavionAPI: ravion_domain.auto (child under cluster domain, Mode A)
    RavionAPI-->>Service: auto fqdn (svc-hash.cluster-fqdn)
    Service->>AWS: aws_lb_listener_rule.ravion (host_header to target group)

    Note over Service,AWS: Mode B (custom domains)
    CP->>Service: "domains=[api.example.com]"
    Service->>RavionAPI: ravion_domain.custom (cert covering customer FQDNs)
    RavionAPI-->>AWS: Attach cert to ALB listener via resource_arn
    Note over RavionAPI,AWS: Customer adds DNS CNAME records
    RavionAPI-->>Service: "retired=true (after DNS resolves)"
    Service->>RavionAPI: ravion_domain.auto count to 0 (destroy)

Loading

Comments Outside Diff (1)

1. compute/autoscaling/variables.tf, line 9-10 (link)

   try(..., true) silences all validation errors

   Wrapping every validation condition in try(..., true) means any expression that throws (e.g., a type mismatch or an unexpected attribute access on an incorrectly-shaped value) silently passes as valid instead of surfacing an error message. For the simple scalar checks like var.desired_capacity == null || var.desired_capacity >= 0, the null-guard short-circuit already prevented the throw without try(), so the wrapper is no-op but harmless there. The concern is the object-typed variables (mixed_instances_policy, instance_refresh, warm_pool, scaling_policies) where a malformed input object that causes an attribute-access error will now be accepted with no user feedback. This same pattern is applied throughout compute/ecs_cluster/variables.tf, compute/ecs_service/variables.tf, networking/alb/variables.tf, and networking/nlb/variables.tf.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: compute/autoscaling/variables.tf
   Line: 9-10
   
   Comment:
   **`try(..., true)` silences all validation errors**
   
   Wrapping every validation condition in `try(..., true)` means any expression that throws (e.g., a type mismatch or an unexpected attribute access on an incorrectly-shaped value) silently passes as valid instead of surfacing an error message. For the simple scalar checks like `var.desired_capacity == null || var.desired_capacity >= 0`, the null-guard short-circuit already prevented the throw without `try()`, so the wrapper is no-op but harmless there. The concern is the object-typed variables (`mixed_instances_policy`, `instance_refresh`, `warm_pool`, `scaling_policies`) where a malformed input object that causes an attribute-access error will now be accepted with no user feedback. This same pattern is applied throughout `compute/ecs_cluster/variables.tf`, `compute/ecs_service/variables.tf`, `networking/alb/variables.tf`, and `networking/nlb/variables.tf`.
   
   How can I resolve this? If you propose a fix, please make it concise.

Prompt To Fix All With AI

Fix the following 5 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 5
compute/ecs_cluster/versions.tf:17-20
**Conflicting provider version pins across modules**

`ecs_cluster` pins the Ravion provider at `= 0.4.3` while `ecs_service/versions.tf` pins it at `= 0.4.5`. Terraform/OpenTofu resolves a single provider version per workspace. Any stack that instantiates both modules (the typical pattern shown in the README) will immediately fail with `Error: Failed to query available provider packages — conflicting provider version constraints`. Both modules must use the same pinned version (or a shared `>= x.y` range).

### Issue 2 of 5
compute/ecs_cluster/module.yml:622-629
**module.yml output declarations don't match actual Terraform outputs**

The seven outputs declared here (`ravion_managed_domains_enabled`, `ravion_public_alb_default_url`, `ravion_public_alb_default_fqdn`, `ravion_public_alb_default_cert_arn`, `ravion_private_alb_default_url`, `ravion_private_alb_default_fqdn`, `ravion_private_alb_default_cert_arn`) do not exist in `compute/ecs_cluster/outputs.tf`. The actual TF outputs are `ravion_cluster_domain_id`, `ravion_cluster_domain_fqdn`, `ravion_cluster_domain_url`, `ravion_cluster_cert_arn`, `ravion_cluster_cert_status`, `ravion_aws_account_id`, and `ravion_aws_region`. Any control-plane reference to these declared-but-absent outputs will resolve to nothing, silently breaking downstream wiring. There are also no private-ALB Ravion domain resources in the TF code, so the three `ravion_private_alb_*` entries can never be populated.

### Issue 3 of 5
compute/ecs_service/module.yml:101-127
**module.yml input names don't match Terraform variable names**

Several UI inputs defined here map to non-existent TF variables:

- `ravion_listener_arn` → no matching TF variable (the TF variable is `cluster_https_listener_arn`, which serves both Mode A and Mode B).
- `ravion_parent_app_domain_id` → TF variable is `cluster_parent_domain_id`.
- `ravion_auto_domain_listener_arn` → TF variable is `cluster_https_listener_arn`.
- `ravion_auto_domain_alb_dns_name` → TF variable is `cluster_alb_dns_name`.
- `ravion_auto_domain_alb_zone_id` → TF variable is `cluster_alb_zone_id`.

When the control plane passes these UI input values to Terraform, the misnamed keys will be silently ignored, leaving the actual TF variables unset (defaulting to `null`) and breaking the Ravion domain wiring at plan/apply time. The output section has the same issue — `ravion_domains`, `ravion_cert_id`, `ravion_cert_arn`, `ravion_cert_status`, `ravion_auto_domain_enabled`, `ravion_auto_domain_fqdn`, `ravion_auto_domain_url` are declared but the actual TF outputs are `ravion_domain_id`, `ravion_domain_fqdn`, `ravion_domain_url`, `ravion_custom_domain_id`, and `ravion_custom_domain_cert_arn`.

### Issue 4 of 5
networking/alb/locals.tf:32
**HTTPS listener created with `null` certificate_arn when `certificate_arns = []`**

`https_default_cert_arn` is `null` when `certificate_arns` is empty. `compute/ecs_cluster/module.yml` now sets `default: []` for `public_alb_certificate_arns`, so a user who sets `use_ravion_managed_domains = true` and leaves `public_alb_certificate_arns` at its default will trigger this path. The `aws_lb_listener.https` resource then passes `certificate_arn = null` to AWS, which requires a certificate for HTTPS listeners and will reject the call with a validation error. The Ravion wildcard cert is wired only as an SNI extra via `aws_lb_listener_certificate` (after the listener exists), so it cannot fill the listener's mandatory default-cert slot. The HTTPS listener needs at least one cert at creation time.

### Issue 5 of 5
compute/autoscaling/variables.tf:9-10
**`try(..., true)` silences all validation errors**

Wrapping every validation condition in `try(..., true)` means any expression that throws (e.g., a type mismatch or an unexpected attribute access on an incorrectly-shaped value) silently passes as valid instead of surfacing an error message. For the simple scalar checks like `var.desired_capacity == null || var.desired_capacity >= 0`, the null-guard short-circuit already prevented the throw without `try()`, so the wrapper is no-op but harmless there. The concern is the object-typed variables (`mixed_instances_policy`, `instance_refresh`, `warm_pool`, `scaling_policies`) where a malformed input object that causes an attribute-access error will now be accepted with no user feedback. This same pattern is applied throughout `compute/ecs_cluster/variables.tf`, `compute/ecs_service/variables.tf`, `networking/alb/variables.tf`, and `networking/nlb/variables.tf`.

_{Reviews (1): Last reviewed commit: "ecs_service: bring auto-FQDN back when u..." | Re-trigger Greptile}

Greptile also left 4 inline comments on this PR.