Domains

[siddhsuresh]

Greptile Summary

This PR introduces a _shared/ravion_cert_groups child module that centralises wildcard-cert provisioning and DNS routing for both ECS clusters (parent mode) and ECS services (leaf mode), replacing the older inline ravion_domain resources. It also wires in the ravion Terraform provider across the affected modules and converts the variable-validation patterns in autoscaling, ecs_cluster, ecs_service, alb, and nlb from var.X == null || ... to try(..., true).

• New _shared/ravion_cert_groups module — dispatches three cert-group kinds (ravion_auto, customer, inherit) with variant-aware DNS-record writes (Route53-Ravion, Route53 direct, Cloudflare) and listener-rule creation using sha256-derived priority buckets.
• Cluster integration — ecs_cluster/ravion_domains.tf instantiates the module in parent mode, issuing one wildcard ACM cert per group; the cert ARNs are appended to the public ALB's listener cert list via the new local.public_alb_effective_certificate_arns local.
• Service integration — ecs_service/ravion_domains.tf instantiates the module in leaf mode, receiving the cluster's ravion_certificate_groups output so inherit groups can nest under the right wildcard cert without duplicating state.

Confidence Score: 3/5

Two definite breakage paths need to be resolved before merging: the six module.yml output entries that have no backing Terraform output blocks will fail for any caller that references them, and inherit groups with a non-matching parent_group_name are silently dropped rather than erroring.

The module.yml / outputs.tf mismatch means the Ravion UI and any downstream module that reads the advertised cluster outputs will hit a plan-time error on every deployment. The silent drop of inherit groups with a mistyped parent_group_name produces a successful apply with zero domain allocations — a live service ends up with no URL and no diagnostic. Both issues are on the critical domain-provisioning path introduced by this PR.

compute/ecs_cluster/module.yml and compute/ecs_cluster/outputs.tf (output name mismatch); _shared/ravion_cert_groups/main.tf (silent inherit-group drop and unguarded domains[0] access).

Important Files Changed

Filename	Overview
_shared/ravion_cert_groups/main.tf	New shared module dispatching cert-group kinds (inherit, customer). Two issues: inherit groups with a mismatched parent_group_name are silently dropped, and aws_acm_certificate.customer will panic on domains[0] if the shared module is called without the ECS-service-level min-domains guard.
compute/ecs_cluster/module.yml	Adds 6 new Ravion outputs (ravion_managed_domains_enabled, ravion_cluster_fqdn, etc.) that have no corresponding output blocks in outputs.tf; the one block that was added (ravion_certificate_groups) is absent from module.yml — all 6 advertised outputs will fail at plan time.
_shared/ravion_cert_groups/parent.tf	New parent-mode logic issuing wildcard ACM certs per group and variant-dispatched DNS validation. Logic looks correct; tolist()[0] on domain_validation_options is safe for wildcard certs which produce exactly one validation option.
compute/ecs_cluster/ravion_domains.tf	New file wiring the shared cert-groups module in parent mode; straightforward delegation with correct mode and listener_arn gating.
compute/ecs_service/ravion_domains.tf	New file wiring the shared cert-groups module in leaf mode with routing target and listener ARN forwarded from cluster outputs.
compute/ecs_cluster/outputs.tf	Adds ravion_certificate_groups output but module.yml advertises six different output names that don't exist as Terraform output blocks.
compute/ecs_service/variables.tf	Adds ravion_certificate_groups and supporting domain-wiring variables with thorough per-kind validation; try() wrapping on existing validations consistent with broader PR pattern.
compute/autoscaling/variables.tf	Replaces null-guard short-circuit patterns with try(..., true) across all complex object validations; addresses OpenTofu evaluation-order issues with optional object attributes.
_shared/ravion_cert_groups/variables.tf	Shared module inputs well-documented; missing a min-domains validation for customer kind that the callers have but the module itself does not enforce.

Sequence Diagram

sequenceDiagram
    participant Cluster as ecs_cluster (parent mode)
    participant SharedModule as _shared/ravion_cert_groups
    participant ALB as AWS ALB
    participant ACM as AWS ACM
    participant DNS as DNS Provider (Route53/CF)
    participant Service as ecs_service (leaf mode)

    Cluster->>SharedModule: "mode=parent, cert_groups=[ravion_auto/customer]"
    SharedModule->>DNS: Lookup DnsProvider (platform_apex / customer)
    SharedModule->>ACM: "aws_acm_certificate (wildcard *.fqdn)"
    SharedModule->>DNS: Write validation records
    SharedModule->>ACM: aws_acm_certificate_validation
    SharedModule->>ALB: Attach wildcard cert (SNI)
    SharedModule-->>Cluster: "parent_groups output {parent_allocation_id, cert_arn, wildcard_fqdn}"

    Service->>SharedModule: "mode=leaf, cert_groups=[inherit/customer], cluster_groups=parent_groups"
    Note over SharedModule: inherit kind - nest under cluster parent allocation
    SharedModule->>DNS: Lookup DnsProvider (customer groups)
    SharedModule->>ACM: aws_acm_certificate (customer FQDNs)
    SharedModule->>DNS: Write routing + validation records
    SharedModule->>ALB: aws_lb_listener_rule (host-header per FQDN)
    SharedModule-->>Service: domain_fqdns, domain_allocation_ids

Loading

Comments Outside Diff (2)

1. compute/ecs_service/variables.tf, line 1032-1036 (link)

   ravion_service_slug is declared but never used

   var.ravion_service_slug is defined and documented as "the slug used to derive the service FQDN under the cluster wildcard", but ravion_domains.tf assigns slug = each.value (entries from var.ravion_domains) for every ravion_domain.this allocation — ravion_service_slug is never referenced. Any caller who sets this variable expecting it to influence domain naming will get silently incorrect FQDNs built from var.ravion_domains entries instead.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: compute/ecs_service/variables.tf
   Line: 1032-1036
   
   Comment:
   **`ravion_service_slug` is declared but never used**
   
   `var.ravion_service_slug` is defined and documented as "the slug used to derive the service FQDN under the cluster wildcard", but `ravion_domains.tf` assigns `slug = each.value` (entries from `var.ravion_domains`) for every `ravion_domain.this` allocation — `ravion_service_slug` is never referenced. Any caller who sets this variable expecting it to influence domain naming will get silently incorrect FQDNs built from `var.ravion_domains` entries instead.
   
   How can I resolve this? If you propose a fix, please make it concise.

2. compute/autoscaling/variables.tf, line 9-11 (link)

   try(..., true) wrapping silently swallows validation bugs

   Every validation condition across autoscaling/variables.tf, ecs_cluster/variables.tf, ecs_service/variables.tf, networking/alb/variables.tf, and networking/nlb/variables.tf is now wrapped in try(..., true). This catches not only expected null-dereference errors but also any accidental typos or wrong attribute names in the validation expression — all of which now return true (pass) instead of surfacing the problem. The original var.X == null || short-circuit pattern was both readable and safe; try buys very little here while hiding future expression bugs.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: compute/autoscaling/variables.tf
   Line: 9-11
   
   Comment:
   **`try(..., true)` wrapping silently swallows validation bugs**
   
   Every validation `condition` across `autoscaling/variables.tf`, `ecs_cluster/variables.tf`, `ecs_service/variables.tf`, `networking/alb/variables.tf`, and `networking/nlb/variables.tf` is now wrapped in `try(..., true)`. This catches not only expected null-dereference errors but also any accidental typos or wrong attribute names in the validation expression — all of which now return `true` (pass) instead of surfacing the problem. The original `var.X == null ||` short-circuit pattern was both readable and safe; `try` buys very little here while hiding future expression bugs.
   
   How can I resolve this? If you propose a fix, please make it concise.

Prompt To Fix All With AI

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

---

### Issue 1 of 3
compute/ecs_cluster/module.yml:552-557
**Six new `module.yml` outputs are never defined in `outputs.tf`**

`module.yml` now declares `ravion_managed_domains_enabled`, `ravion_cluster_domain_allocation_id`, `ravion_cluster_managed_domain_id`, `ravion_cluster_fqdn`, `ravion_cluster_certificate_arn`, and `ravion_dns_provider_id` as outputs, but none of these have a corresponding `output` block in `outputs.tf`. The only block that was actually added there is `ravion_certificate_groups`, which is itself absent from `module.yml`. Any caller (or the Ravion UI) that references any of these six advertised outputs will fail at plan time with `This object does not have an attribute named "..."`. The output that IS defined (`ravion_certificate_groups`) should be listed here, and the six phantom entries should either be removed or backed by real output blocks.

### Issue 2 of 3
_shared/ravion_cert_groups/main.tf:53-56
**`inherit` group silently vanishes when `parent_group_name` is not in `var.cluster_groups`**

The `contains(keys(var.cluster_groups), coalesce(g.parent_group_name, ""))` guard silently drops any `inherit` group whose `parent_group_name` doesn't match an entry in the map — no `ravion_domain`, no `aws_lb_listener_rule`, no error. A typo in the group name (e.g. `"default"` vs `"Default"`) produces a successful apply with zero domain allocations and the service silently gets no URL. The ECS-service-level validation only checks that `parent_group_name` is non-empty, not that it resolves. A `check` block or `precondition` that asserts every `inherit` group's `parent_group_name` exists in `var.cluster_groups` would surface the failure early.

### Issue 3 of 3
_shared/ravion_cert_groups/main.tf:199-202
**`aws_acm_certificate.customer` panics on `domains[0]` when the shared module is called with an empty `domains` list**

`aws_acm_certificate.customer` iterates `local.customer_groups` (all customer-kind groups) and accesses `each.value.domains[0]`. The shared module's `variables.tf` has no validation requiring at least one domain for `customer` kind — only the ECS-service-level caller enforces that constraint. A module consumer that bypasses or precedes that guard will hit an index-out-of-bounds error at plan time with no useful message. Adding a `validation` block on `var.cert_groups` in the shared module directly would make the invariant self-contained.

_{Reviews (3): Last reviewed commit: "feat(cert_groups): TF passes cert_group_..." | Re-trigger Greptile}

Greptile also left 2 inline comments on this PR.

[siddhsuresh]

@greptile

[greptile-apps]

Hash-based priority can collide, causing apply failures

substr(sha256(...), 0, 4) produces a 16-bit value (0–65535); % 49000 maps it to 0–48999. Because 65536 > 49000, the distribution is non-uniform (values 0–16535 appear twice), and — more critically — any two (var.name, domain-slug) combinations that hash to the same 4-hex prefix produce an identical listener-rule priority. When a second service hits the same priority on the same ALB listener, AWS rejects the aws_lb_listener_rule create with a DuplicatePriority error. The birthday bound is ~√(49000) ≈ 221 unique combinations before a 50% collision probability, but even a handful of services sharing a large domain list creates real risk at cluster scale.

Prompt To Fix With AI

This is a comment left during a code review.
Path: compute/ecs_service/ravion_domains.tf
Line: 38-45

Comment:
**Hash-based priority can collide, causing apply failures**

`substr(sha256(...), 0, 4)` produces a 16-bit value (0–65535); `% 49000` maps it to 0–48999. Because 65536 > 49000, the distribution is non-uniform (values 0–16535 appear twice), and — more critically — any two `(var.name, domain-slug)` combinations that hash to the same 4-hex prefix produce an identical listener-rule priority. When a second service hits the same priority on the same ALB listener, AWS rejects the `aws_lb_listener_rule` create with a `DuplicatePriority` error. The birthday bound is ~√(49000) ≈ 221 unique combinations before a 50% collision probability, but even a handful of services sharing a large domain list creates real risk at cluster scale.

How can I resolve this? If you propose a fix, please make it concise.

[greptile-apps]

try() without null guard silently swallows type errors

The previous var.instance_maintenance_policy == null || (condition) pattern was dropped; the refactored version relies entirely on try() to catch null-object access. While this works for the null case, try() returns true for any error — including a type mismatch where min_healthy_percentage or max_healthy_percentage is passed as a non-numeric value. Such inputs would silently pass validation instead of emitting the error_message. The same concern applies to the analogous try() wrapping used across compute/ecs_service/variables.tf and compute/ecs_cluster/variables.tf.

Prompt To Fix With AI

This is a comment left during a code review.
Path: compute/autoscaling/variables.tf
Line: 1392-1408

Comment:
**`try()` without null guard silently swallows type errors**

The previous `var.instance_maintenance_policy == null || (condition)` pattern was dropped; the refactored version relies entirely on `try()` to catch null-object access. While this works for the null case, `try()` returns `true` for *any* error — including a type mismatch where `min_healthy_percentage` or `max_healthy_percentage` is passed as a non-numeric value. Such inputs would silently pass validation instead of emitting the `error_message`. The same concern applies to the analogous `try()` wrapping used across `compute/ecs_service/variables.tf` and `compute/ecs_cluster/variables.tf`.

How can I resolve this? If you propose a fix, please make it concise.

[greptile-apps]

Redundant && var.enable_public_alb — local.enable_ravion_domain already requires var.enable_public_alb (defined in locals.tf), so this term is always true when the outer local is true and redundant when it's false.

Suggested change

	count = local.enable_ravion_domain ? 1 : 0

Prompt To Fix With AI

This is a comment left during a code review.
Path: compute/ecs_cluster/ravion_domains.tf
Line: 68

Comment:
Redundant `&& var.enable_public_alb` — `local.enable_ravion_domain` already requires `var.enable_public_alb` (defined in `locals.tf`), so this term is always `true` when the outer local is `true` and redundant when it's `false`.

```suggestion
  count             = local.enable_ravion_domain ? 1 : 0
```

How can I resolve this? If you propose a fix, please make it concise.

[greptile-apps]

ravion_dns_zone_id declared but never defined

module.yml advertises ravion_dns_zone_id: string as a module output, but no matching output "ravion_dns_zone_id" block exists in outputs.tf. Any caller that references module.ecs_cluster.ravion_dns_zone_id will fail with This object does not have an attribute named "ravion_dns_zone_id" during plan. The output block that was likely intended is ravion_dns_provider_id (which is defined in outputs.tf but not listed in module.yml), suggesting these two were renamed out of sync.

Prompt To Fix With AI

This is a comment left during a code review.
Path: compute/ecs_cluster/module.yml
Line: 539

Comment:
**`ravion_dns_zone_id` declared but never defined**

`module.yml` advertises `ravion_dns_zone_id: string` as a module output, but no matching `output "ravion_dns_zone_id"` block exists in `outputs.tf`. Any caller that references `module.ecs_cluster.ravion_dns_zone_id` will fail with `This object does not have an attribute named "ravion_dns_zone_id"` during plan. The output block that was likely intended is `ravion_dns_provider_id` (which *is* defined in `outputs.tf` but not listed in `module.yml`), suggesting these two were renamed out of sync.

How can I resolve this? If you propose a fix, please make it concise.

[greptile-apps]

given_id path silently leaves dns_provider_id null on domain resources

ravion_domain.this only passes var.ravion_dns_provider_id, not the given_id alternative. When a caller wires up the service using only ravion_dns_provider_given_id, dns_provider_id is null on every allocated domain resource while the data source lookup still succeeds — the domain allocation will be sent to the API without a provider reference. The cluster module resolved this by outputting ravion_dns_provider_id (the resolved opaque id), but there is no guard that validates the opaque id is actually present before creating domain resources.

Prompt To Fix With AI

This is a comment left during a code review.
Path: compute/ecs_service/ravion_domains.tf
Line: 54-60

Comment:
**`given_id` path silently leaves `dns_provider_id` null on domain resources**

`ravion_domain.this` only passes `var.ravion_dns_provider_id`, not the `given_id` alternative. When a caller wires up the service using only `ravion_dns_provider_given_id`, `dns_provider_id` is null on every allocated domain resource while the data source lookup still succeeds — the domain allocation will be sent to the API without a provider reference. The cluster module resolved this by outputting `ravion_dns_provider_id` (the resolved opaque id), but there is no guard that validates the opaque id is actually present before creating domain resources.

How can I resolve this? If you propose a fix, please make it concise.

[greptile-apps]

The ttl = 1 value and its inline comment are contradictory: 1 is Cloudflare's "automatic" TTL that is only meaningful when proxied = true. With proxied = false (the current value), a concrete TTL such as 60 seconds should be used — the same value the Ravion metadata record below uses for consistency.

Suggested change

	ttl = 1 # 1 == automatic, required when proxied = true; ALB doesn't accept CF proxying for arbitrary L7 so leave proxied false + ttl=60.
	proxied = false
	ttl = 60
	proxied = false

Prompt To Fix With AI

This is a comment left during a code review.
Path: compute/ecs_cluster/ravion_domains.tf
Line: 191-192

Comment:
The `ttl = 1` value and its inline comment are contradictory: `1` is Cloudflare's "automatic" TTL that is only meaningful when `proxied = true`. With `proxied = false` (the current value), a concrete TTL such as 60 seconds should be used — the same value the Ravion metadata record below uses for consistency.

```suggestion
  ttl     = 60
  proxied = false
```

How can I resolve this? If you propose a fix, please make it concise.

[greptile-apps]

Hash-based priority space is too small for collision-free allocation

The expression (parseint(substr(sha256("${var.name}:${d}"), 0, 4), 16) % 49000) + 1000 uses only the first 4 hex characters (16-bit, 0–65535 range), then takes % 49000, collapsing to 49,000 buckets. By the birthday problem, the probability of at least one collision reaches ~50% around 261 (service, domain) pairs sharing the same ALB listener. A collision causes aws_lb_listener_rule creation to fail with a duplicate-priority error that is silent until apply time.

Prompt To Fix With AI

This is a comment left during a code review.
Path: compute/ecs_service/ravion_domains.tf
Line: 43-50

Comment:
**Hash-based priority space is too small for collision-free allocation**

The expression `(parseint(substr(sha256("${var.name}:${d}"), 0, 4), 16) % 49000) + 1000` uses only the first 4 hex characters (16-bit, 0–65535 range), then takes `% 49000`, collapsing to 49,000 buckets. By the birthday problem, the probability of at least one collision reaches ~50% around 261 (service, domain) pairs sharing the same ALB listener. A collision causes `aws_lb_listener_rule` creation to fail with a duplicate-priority error that is silent until apply time.

How can I resolve this? If you propose a fix, please make it concise.

[siddhsuresh]

@greptile

[greptile-apps]

Six new module.yml outputs are never defined in outputs.tf

module.yml now declares ravion_managed_domains_enabled, ravion_cluster_domain_allocation_id, ravion_cluster_managed_domain_id, ravion_cluster_fqdn, ravion_cluster_certificate_arn, and ravion_dns_provider_id as outputs, but none of these have a corresponding output block in outputs.tf. The only block that was actually added there is ravion_certificate_groups, which is itself absent from module.yml. Any caller (or the Ravion UI) that references any of these six advertised outputs will fail at plan time with This object does not have an attribute named "...". The output that IS defined (ravion_certificate_groups) should be listed here, and the six phantom entries should either be removed or backed by real output blocks.

Prompt To Fix With AI

This is a comment left during a code review.
Path: compute/ecs_cluster/module.yml
Line: 552-557

Comment:
**Six new `module.yml` outputs are never defined in `outputs.tf`**

`module.yml` now declares `ravion_managed_domains_enabled`, `ravion_cluster_domain_allocation_id`, `ravion_cluster_managed_domain_id`, `ravion_cluster_fqdn`, `ravion_cluster_certificate_arn`, and `ravion_dns_provider_id` as outputs, but none of these have a corresponding `output` block in `outputs.tf`. The only block that was actually added there is `ravion_certificate_groups`, which is itself absent from `module.yml`. Any caller (or the Ravion UI) that references any of these six advertised outputs will fail at plan time with `This object does not have an attribute named "..."`. The output that IS defined (`ravion_certificate_groups`) should be listed here, and the six phantom entries should either be removed or backed by real output blocks.

How can I resolve this? If you propose a fix, please make it concise.

[greptile-apps]

inherit group silently vanishes when parent_group_name is not in var.cluster_groups

The contains(keys(var.cluster_groups), coalesce(g.parent_group_name, "")) guard silently drops any inherit group whose parent_group_name doesn't match an entry in the map — no ravion_domain, no aws_lb_listener_rule, no error. A typo in the group name (e.g. "default" vs "Default") produces a successful apply with zero domain allocations and the service silently gets no URL. The ECS-service-level validation only checks that parent_group_name is non-empty, not that it resolves. A check block or precondition that asserts every inherit group's parent_group_name exists in var.cluster_groups would surface the failure early.

Prompt To Fix With AI

This is a comment left during a code review.
Path: _shared/ravion_cert_groups/main.tf
Line: 53-56

Comment:
**`inherit` group silently vanishes when `parent_group_name` is not in `var.cluster_groups`**

The `contains(keys(var.cluster_groups), coalesce(g.parent_group_name, ""))` guard silently drops any `inherit` group whose `parent_group_name` doesn't match an entry in the map — no `ravion_domain`, no `aws_lb_listener_rule`, no error. A typo in the group name (e.g. `"default"` vs `"Default"`) produces a successful apply with zero domain allocations and the service silently gets no URL. The ECS-service-level validation only checks that `parent_group_name` is non-empty, not that it resolves. A `check` block or `precondition` that asserts every `inherit` group's `parent_group_name` exists in `var.cluster_groups` would surface the failure early.

How can I resolve this? If you propose a fix, please make it concise.