Skip to content

breaking change to acm cert module, change to single domains array#66

Merged
flybayer merged 1 commit into
mainfrom
bb-certrename
Jul 17, 2026
Merged

breaking change to acm cert module, change to single domains array#66
flybayer merged 1 commit into
mainfrom
bb-certrename

Conversation

@flybayer

@flybayer flybayer commented Jul 17, 2026

Copy link
Copy Markdown
Member

Greptile Summary

This PR makes a breaking change to the ACM certificate module by replacing the separate domain_name (string) and subject_alternative_names (list) inputs with a single ordered domains list, where the first element is the primary domain and the rest become SANs. All consumers — Terraform module, UI definition, docs, and tests — are updated consistently.

  • certificate.tf uses var.domains[0] for the primary domain and slice(var.domains, 1, length(var.domains)) for SANs; safe because the new domains validation enforces a non-empty list with unique, non-blank entries.
  • rvn-acm-certificate-definition.yml consolidates the two separate UI inputs into a single string_array field, bumps to v0.4.0 (correctly labeled BREAKING), and wires domains directly into the stack template variables.
  • Tests are updated and now assert both primary domain assignment and SAN population separately for the multi-domain case.

Confidence Score: 4/5

Safe to merge; the breaking change is well-scoped and all call sites within the repo are updated.

The Terraform logic, validation, UI definition, and tests are all consistent. The only gap is that the single-domain test run doesn't assert that subject_alternative_names is null, leaving a small blind spot if the null-branch condition in certificate.tf regresses.

security/acm_certificate/tests/basic.tftest.hcl — the single-domain case could use an explicit SAN null assertion.

Important Files Changed

Filename Overview
security/acm_certificate/certificate.tf Replaces domain_name/subject_alternative_names with var.domains[0] and a slice() call; logic is correct and safe given the validation guard on domains
security/acm_certificate/variables.tf Removes domain_name and subject_alternative_names; new domains variable includes length > 0, non-empty string, and case-insensitive uniqueness validations
security/acm_certificate/rvn-acm-certificate-definition.yml Merges two separate UI inputs into a single string_array domains field, bumps to v0.4.0, and moves the stack block to before readme; stack template variables are updated correctly
security/acm_certificate/tests/basic.tftest.hcl Test suite updated to use domains; multi-domain test asserts both primary domain and SANs separately, but no assertion verifies subject_alternative_names is null when only one domain is provided
security/acm_certificate/README.md Docs updated to reflect new domains API, source URL corrected from ravion-modules to modules, version ref updated to [email protected]
cdn/cloudfront/rvn-cloudfront-definition.yml Version bumped to 0.3.2; certificate usage guidance updated to describe the new ordered-domains approach
README.md Module table description updated to mention ordered domains; cosmetic change only

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["domains input\n(list of FQDNs)"] --> B{length > 1?}
    B -- yes --> C["domain_name = domains[0]\nsubject_alternative_names = domains[1..]"]    
    B -- no --> D["domain_name = domains[0]\nsubject_alternative_names = null"]
    C --> E["aws_acm_certificate.this\n(DNS validation)"]
    D --> E
    E --> F{route53_validation\n_records_creation_enabled?}
    F -- true --> G["aws_route53_record.validation\n(CNAME per domain)"]
    F -- false --> H["Output: validation_records\n(for manual DNS)"]
    G --> I{certificate_validation\n_wait_enabled?}
    H --> I
    I -- true --> J["aws_acm_certificate_validation\n(wait for ISSUED)"]
    I -- false --> K["Apply completes\n(certificate PENDING)"]
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A["domains input\n(list of FQDNs)"] --> B{length > 1?}
    B -- yes --> C["domain_name = domains[0]\nsubject_alternative_names = domains[1..]"]    
    B -- no --> D["domain_name = domains[0]\nsubject_alternative_names = null"]
    C --> E["aws_acm_certificate.this\n(DNS validation)"]
    D --> E
    E --> F{route53_validation\n_records_creation_enabled?}
    F -- true --> G["aws_route53_record.validation\n(CNAME per domain)"]
    F -- false --> H["Output: validation_records\n(for manual DNS)"]
    G --> I{certificate_validation\n_wait_enabled?}
    H --> I
    I -- true --> J["aws_acm_certificate_validation\n(wait for ISSUED)"]
    I -- false --> K["Apply completes\n(certificate PENDING)"]
Loading

Comments Outside Diff (1)

  1. security/acm_certificate/tests/basic.tftest.hcl, line 44-61 (link)

    P2 No assertion that single-domain produces null SANs

    defaults_no_route53_no_wait exercises the single-domain path (global domains = ["api.example.com"]) but never asserts that aws_acm_certificate.this.subject_alternative_names is null. In certificate.tf the SAN argument is length(var.domains) > 1 ? slice(...) : null, so if a regression ever swapped the condition the test would still pass silently.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: security/acm_certificate/tests/basic.tftest.hcl
    Line: 44-61
    
    Comment:
    **No assertion that single-domain produces null SANs**
    
    `defaults_no_route53_no_wait` exercises the single-domain path (global `domains = ["api.example.com"]`) but never asserts that `aws_acm_certificate.this.subject_alternative_names` is `null`. In `certificate.tf` the SAN argument is `length(var.domains) > 1 ? slice(...) : null`, so if a regression ever swapped the condition the test would still pass silently.
    
    How can I resolve this? If you propose a fix, please make it concise.

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
security/acm_certificate/tests/basic.tftest.hcl:44-61
**No assertion that single-domain produces null SANs**

`defaults_no_route53_no_wait` exercises the single-domain path (global `domains = ["api.example.com"]`) but never asserts that `aws_acm_certificate.this.subject_alternative_names` is `null`. In `certificate.tf` the SAN argument is `length(var.domains) > 1 ? slice(...) : null`, so if a regression ever swapped the condition the test would still pass silently.

Reviews (1): Last reviewed commit: "breaking change to acm cert module, chan..." | Re-trigger Greptile

@github-actions

Copy link
Copy Markdown

Ravion Module Publish Plan

Dry run only. No Ravion API mutations were made.

Module Current Version New Version Description
rvn-acm-certificate 0.3.1 0.4.0 BREAKING: Replace domain_name and subject_alternative_names with domains. Put the primary domain first and all subject alternative names after it.
rvn-cloudfront 0.3.1 0.3.2 Clarify how to order domains when requesting the selected ACM certificate.

Diffs

rvn-acm-certificate n/a -> 0.4.0

--- remote
+++ compiled
-description: ACM certificate requests an AWS Certificate Manager public certificate with DNS validation.
+description: Requests an AWS Certificate Manager public certificate with DNS validation.
 name: ACM Certificate
 type: rvn-acm-certificate

rvn-acm-certificate 0.3.1 -> 0.4.0

--- remote
+++ compiled
         pattern: ^[a-z0-9]([a-z0-9-]{0,62}[a-z0-9])?$
     required: true
     type: string
-  - description: Primary fully qualified domain name for the ACM certificate.
-    id: domain_name
+  - add_button_label: Add domain
+    description: Fully qualified domain names for the certificate. The first domain is primary and the remaining domains are subject alternative names.
+    id: domains
     immutable: true
-    label: Domain name
+    label: Domains
     placeholder: api.example.com
     required: true
-    type: string
-  - add_button_label: Add secondary domain
-    description: Additional fully qualified domain names to include on the certificate.
-    id: subject_alternative_names
-    immutable: true
-    label: Subject alternative names
-    placeholder: www.example.com
     type: string_array
   - id: section_validation
     label: DNS validation
@@
     type: string
     values: $values:ravion/execution_environments
 readme: |
-  ACM certificate requests an AWS Certificate Manager public certificate with DNS validation.
+  Requests an AWS Certificate Manager public certificate with DNS validation.
 
   ## Overview
 
@@
 
   ## Certificate domains
 
-  The required Domain name field is the primary fully qualified domain name for the certificate, such as api.example.com.
-
-  Subject alternative names add more FQDNs to the same certificate. Do not repeat the primary domain name in this list. ACM rejects invalid duplicate domain combinations.
+  Add every fully qualified domain name to the required Domains list. The first domain, such as api.example.com, is the certificate's primary domain. Every remaining domain becomes a subject alternative name. Domain names must be unique.
 
   ## DNS validation
 
@@
   | Region | Yes | None | AWS Region for ACM. Use us-east-1 for CloudFront. |
   | Terraform execution environment | No | None | Optional runner environment override for the stack pipeline. |
   | Name slug | Yes | Project, environment, and module given IDs | Name prefix used for certificate tags. |
-  | Domain name | Yes | None | Primary FQDN for the certificate. |
-  | Subject alternative names | No | Empty list | Additional FQDNs on the certificate. |
+  | Domains | Yes | None | Ordered FQDNs. The first is primary and the remainder are subject alternative names. |
   | Create Route53 validation records | No | false | Creates validation CNAMEs in Route53. |
   | Route53 hosted zone ID | When Route53 records are enabled | None | Public hosted zone used for validation records. |
   | Wait for certificate issuance | No | false | Waits until ACM issues the certificate during apply. |
@@
 
   - [AWS Certificate Manager DNS validation](https://docs.aws.amazon.com/acm/latest/userguide/dns-validation.html)
   - [AWS Certificate Manager certificates for CloudFront](https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/cnames-and-https-requirements.html)
-  - [Source module](https://github.com/flightcontrolhq/modules/tree/[email protected]/security/acm_certificate)
+  - [Source module](https://github.com/flightcontrolhq/modules/tree/[email protected]/security/acm_certificate)
 stack:
   pipelines:
     change:
@@
         base_path: security/acm_certificate
         branch: main
         execution_environment_id: << module.input.execution_environment_id >>
-        ref: [email protected]
+        ref: [email protected]
         repo: https://github.com/flightcontrolhq/modules
         stack_id: <<stack.id>>
         terraform_variables:
           ...overrides: << module.input.advanced_terraform_variables >>
           certificate_validation_wait_enabled: << module.input.certificate_validation_wait_enabled >>
-          domain_name: << module.input.domain_name >>
+          domains: << module.input.domains >>
           name: << module.input.name >>
           region: << module.input.aws_region >>
           route53_validation_records_creation_enabled: << module.input.route53_validation_records_creation_enabled >>
           route53_zone_id: << module.input.route53_zone_id >>
-          subject_alternative_names: << module.input.subject_alternative_names || [] >>
           tags:
             ...overrides: << module.input.tags >>
             EnvironmentGivenId: <<environment.given_id>>

rvn-cloudfront 0.3.1 -> 0.3.2

--- remote
+++ compiled
 
   The default configuration is safe for dynamic apps: responses are cached at the edge only when your app returns Cache-Control headers, and the full viewer request, including the Host header, cookies, and query strings, is forwarded to the origin. You get TLS termination close to users, HTTP/2 and HTTP/3, connection reuse to the origin, optional WAF, and per-path caching for static assets.
 
-  Terraform source: [flightcontrolhq/modules/cdn/cloudfront](https://github.com/flightcontrolhq/modules/tree/[email protected]/cdn/cloudfront)
+  Terraform source: [flightcontrolhq/modules/cdn/cloudfront](https://github.com/flightcontrolhq/modules/tree/[email protected]/cdn/cloudfront)
 
   ## Use cases
 
@@
 
   ## Certificates
 
-  Domain aliases require a certificate issued in us-east-1, because CloudFront is a global service. Select an ACM Certificate module instance in the ACM certificate field; its certificate ARN is mapped automatically. Request the certificate with the primary domain and any additional aliases as subject alternative names.
+  Domain aliases require a certificate issued in us-east-1, because CloudFront is a global service. Select an ACM Certificate module instance in the ACM certificate field; its certificate ARN is mapped automatically. In that certificate's Domains list, put the primary domain first and add any other aliases after it.
 
   This certificate is for viewers connecting to CloudFront. The load balancer keeps its own regional certificate for the origin leg.
 
@@
         base_path: cdn/cloudfront
         branch: main
         execution_environment_id: << module.input.execution_environment_id >>
-        ref: [email protected]
+        ref: [email protected]
         repo: https://github.com/flightcontrolhq/modules
         stack_id: <<stack.id>>
         terraform_variables:

@flybayer
flybayer merged commit 54b812e into main Jul 17, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant