Repository navigation
fix(elbv2): hold an ACM certificate in use while a listener serves it - #6056
Conversation
PR Summary by QodoKeep ACM certificates in use while ELBv2 listeners serve them
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
Code Review by Qodo
1. Stack updates that replace a certificate fail
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| requireListenerCertificates(region, protocol, certificates); | ||
| regionListeners.put(listenerArn, listener); | ||
| listeners.put(region, regionListeners); | ||
| markCertificatesInUse(listener); | ||
| }); |
There was a problem hiding this comment.
2. Stack updates that replace a certificate fail 🐞 Bug ≡ Correctness
A listener now registers as an ACM consumer when it is created (markCertificatesInUse). AcmCfnProvisioner.deletePriorOrUnwind deletes the superseded certificate during the certificate's own provision step, and CloudFormation runs that step, in dependency order, before the listener that refers to it is updated to the new ARN. In a stack where an AWS::ElasticLoadBalancingV2::Listener refers to an AWS::CertificateManager::Certificate, any createOnly change to the certificate (DomainName, SANs, KeyAlgorithm) now hits ResourceInUseException and the update rolls back. Before this PR the same update succeeded.
Agent Prompt
## Issue description
When a CloudFormation update replaces an ACM certificate that an ELBv2 listener in the same stack refers to, `AcmCfnProvisioner.deletePriorOrUnwind` deletes the old certificate before the listener has moved to the new ARN. Listeners now register as ACM InUseBy consumers, so that delete fails with ResourceInUseException and the stack update rolls back.
## Fix Focus Areas
- src/main/java/io/github/hectorvent/floci/services/cloudformation/provisioners/AcmCfnProvisioner.java[68-105]
- src/main/java/io/github/hectorvent/floci/services/cloudformation/CloudFormationService.java[2000-2073]
## Recommended Fix
Match real CloudFormation: delete the replaced physical resource in a cleanup phase that runs after every resource in the update has been provisioned. Record the prior ARN, for example in a cleanup list on the update context, instead of deleting it inline, and delete it once the dependents have been updated. Add a test that updates a stack with a listener referring to a replaced certificate and asserts the update succeeds and the old certificate is gone.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
pgermosen
added
acm
pgermosen
left a comment
pgermosen
left a comment
There was a problem hiding this comment.
@drakeo338 thanks for this. It conflicts with main now, though, and #5959, which just landed, already covers most of it: DeleteCertificate refuses a certificate a listener serves, reading the listeners' certificates live rather than registering them, and a re-import keeps InUseBy.
What main still lacks is the status. DeleteCertificate answers 409, where the ACM reference gives 400, as RevokeCertificate on main already does. Could you rebase and trim this down to that fix and its tests?
pgermosen
added
the
waiting-author
ACM answers ResourceInUseException with HTTP 400, not 409, so DeleteCertificate now does too, as RevokeCertificate already does. Refs floci-io#6040
The ELBv2 HTTPS data plane test still asserted 409, and the ACM CloudFormation provisioner test stubbed the refusal with 409.
282734e to
6ba353c
Compare
pgermosen
left a comment
pgermosen
left a comment
There was a problem hiding this comment.
@drakeo338 thanks for this. Since #5959 already holds the certificate while a listener serves it, I trimmed the branch down to your status fix: DeleteCertificate now answers 400 as the ACM reference gives, and I updated the two tests on main that still expected 409. The ELB Classic and CloudFront parts of #6040 stay open there.
pgermosen
removed
the
waiting-author
Summary
Refs #6040.
Covers the ELBv2 half of the registration list (listener create, modify, certificate add/remove, listener and load balancer delete) plus the 409 to 400 status fix. ELB Classic, CloudFront and the blank
SSLCertificateIdrefusal are not covered.A listener recorded its ACM certificate ARNs but never told ACM, so
DeleteCertificateremoved a certificate an HTTPS listener still served andInUseBystayed empty.CertificateReferenceResolvernow adds and releases the listener ARN as anInUseByconsumer, andDeleteCertificateanswersResourceInUseExceptionwith 400. Re-importing to an existing ARN used to resetInUseBy, so it now carries consumers over.Type of change
fix:)feat:)feat!:orfix!:)AWS Compatibility
Before: delete succeeded while a listener served the certificate. After: 400
ResourceInUseException, as the API reference documents.Checklist
./mvnw testpasses locally (only the affected test classes were run: 43 passed, 0 failed)