fix(cloudformation): resolve dynamic references for all template values - #3301
fix(cloudformation): resolve dynamic references for all template values#3301omatheusmesmo wants to merge 1 commit into
Conversation
|
| Filename | Overview |
|---|---|
| src/main/java/io/github/hectorvent/floci/services/cloudformation/CloudFormationTemplateEngine.java | Routes scalar and recursively resolved textual values through the optional dynamic-reference resolver, fixing the prior scalar-resolution gap. |
| src/main/java/io/github/hectorvent/floci/services/cloudformation/provisioners/CfnDynamicReferences.java | Adds general dynamic-reference handling but incorrectly preserves unsupported ssm-secure references outside RDS master credentials. |
| src/main/java/io/github/hectorvent/floci/services/cloudformation/CloudFormationService.java | Wires dynamic-reference resolution into resource and output template engines. |
| src/main/java/io/github/hectorvent/floci/services/cloudformation/CloudFormationResourceProvisioner.java | Wires standalone provisioning while retaining the separate RDS secure-reference resolution stage. |
| src/test/java/io/github/hectorvent/floci/services/cloudformation/CloudFormationDynamicReferenceIntegrationTest.java | Adds end-to-end dynamic-reference coverage, but the previous repository-rule finding about raw HTTP management-plane validation remains outstanding. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[CloudFormation template value] --> B[Template engine resolve or resolveNode]
B --> C{Contains dynamic reference?}
C -->|No| D[Return original value]
C -->|SSM or Secrets Manager| E[General dynamic-reference resolver]
E --> F[Provision resolved property]
C -->|ssm-secure| G[Preserve reference verbatim]
G --> H{RDS master credential?}
H -->|Yes| I[RDS resolver with allowSsmSecure]
I --> F
H -->|No| J[Literal placeholder reaches property]
Reviews (5): Last reviewed commit: "fix(cloudformation): resolve dynamic ref..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (node.isTextual()) { | ||
| return resolveDynamicReferences(node.textValue()); | ||
| } | ||
| if (node.isNumber() || node.isBoolean()) { | ||
| return node; | ||
| } | ||
| if (node.isObject()) { | ||
| if (node.has("Ref") || node.has("Fn::Sub") || node.has("Fn::Join") || | ||
| node.has("Fn::Select") || node.has("Fn::If") || node.has("Fn::Base64") || | ||
| node.has("Fn::GetAtt") || node.has("Fn::ImportValue") || node.has("Fn::Split") || | ||
| node.has("Fn::GetAZs") || node.has("Fn::Cidr") || node.has("Fn::FindInMap")) { | ||
| return TextNode.valueOf(resolve(node)); | ||
| return resolveDynamicReferences(resolve(node)); |
There was a problem hiding this comment.
Dynamic references are resolved only through resolveNode, but many ordinary scalar properties still use resolve, which returns strings and intrinsic results without invoking the new resolver. For example, ProvisionContext.resolveOptional sends properties such as AWS::SNS::Topic.TopicName through engine.resolve, so {{resolve:ssm:...}} reaches the resource as literal text. This leaves the advertised all-property fix incomplete and violates the repository directive to preserve AWS-compatible CloudFormation behavior. Route scalar resolution through the same dynamic-reference stage and cover a directly resolved scalar property with a test.
Context Used: AGENTS.md (source)
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!
2777a3a to
fc39798
Compare
omatheusmesmo
changed the title
fc39798 to
f2c27de
Compare
| given() | ||
| .header("X-Amz-Target", "AmazonSSM.PutParameter") | ||
| .contentType(SSM_CONTENT_TYPE) | ||
| .body(""" | ||
| { | ||
| "Name": "/cfn-dynref/url", | ||
| "Value": "https://real.example.com", | ||
| "Type": "String", | ||
| "Overwrite": true | ||
| } | ||
| """) | ||
| .when() | ||
| .post("/") | ||
| .then() | ||
| .statusCode(200); |
There was a problem hiding this comment.
This integration test constructs raw SSM, Secrets Manager, CloudFormation, and Lambda HTTP requests. That violates the repository directive to prefer AWS SDK clients for management-plane validation, which ensures that tests exercise real client serialization and response handling. This repository requirement must be satisfied before merging. The same pattern also appears at lines 68–94, 99–160, and 164–216.
Context Used: AGENTS.md (source)
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!
f2c27de to
46bf29a
Compare
| * substituted regardless of which of the two a caller uses. | ||
| */ | ||
| public String resolve(JsonNode node) { | ||
| return resolveDynamicReferences(resolveIntrinsic(node)).asText(); |
There was a problem hiding this comment.
RDS ssm-secure credentials now fail before reaching the RDS-specific resolver. resolveOptional invokes engine.resolve, whose new general resolver disallows ssm-secure. The later call with allowSsmSecure=true is therefore never reached, so valid MasterUserPassword references for DB instances and clusters raise ValidationError.
|
Thanks, this is a real gap and the fix is the right shape: dynamic references only worked where a provisioner called the resolver by hand, so (blocking) The new general stage rejects the one case That is what CI shows: Your new javadoc is where the assumption shows: it says every other property reaches the resolver through the general path, but a master password is both. One direction if it helps: leave |
hectorvent
added
the
waiting-author
CloudFormationTemplateEngine#resolveNode now resolves {{resolve:ssm:...}}
and {{resolve:secretsmanager:...}} syntax in any textual result, not only
the RDS master-credential properties CfnDynamicReferences was previously
wired into. The dynamic-reference stage now lives inside resolve() itself,
so every scalar property resolution goes through it, including plain
property lookups such as ProvisionContext#resolveOptional that call
resolve() directly rather than through resolveNode. ssm-secure is left
verbatim at this general stage instead of rejected, since it is valid only
for RDS MasterUsername/MasterUserPassword, which resolve it separately
with the required permission.
* Fix floci-io#2213
Signed-off-by: Matheus Oliveira <matheus.6148@gmail.com>
46bf29a to
6bc36ac
Compare
| String replacement = "ssm-secure".equals(m.group(1)) | ||
| ? m.group(0) | ||
| : resolveDynamicRef(m.group(1), m.group(2), region, false); |
There was a problem hiding this comment.
The general resolver leaves every ssm-secure reference unchanged so the RDS credential path can resolve it later. However, non-RDS properties have no later validation stage, so an unsupported {{resolve:ssm-secure:...}} value is provisioned as a literal placeholder instead of producing the AWS-compatible validation error required by the repository's AWS compatibility directive. Restrict this pass-through to RDS master credentials or reject unresolved secure references before provisioning other properties.
Context Used: AGENTS.md (source)
2 participants
Summary
CloudFormation dynamic references (
{{resolve:ssm:...}},{{resolve:secretsmanager:...}}) were only resolved for the RDS MasterUsername/MasterUserPassword properties, which call CfnDynamicReferences directly. Every other property value goes through CloudFormationTemplateEngine#resolveNode, which had no dynamic-reference stage at all, so a value such as a Lambda Environment.Variables entry reached the deployed resource as the literal{{resolve:ssm:...}}text instead of the resolved value.Closes #2213
Type of change
fix:)feat:)feat!:orfix!:)AWS Compatibility
resolveNodenow runs the same CfnDynamicReferences resolution over every textual result it produces: a plain string literal carrying the syntax outright, and the text an intrinsic function (e.g. Fn::Sub) produces.ssm-securestays restricted to the RDS master-credential path, matching the existing behavior there. The RDS code path is unchanged, since it resolves MasterUsername/MasterUserPassword directly rather than through resolveNode.CloudFormationTemplateEngine: added an optional dynamicReferenceResolver collaborator (a UnaryOperator, following the same nullable-functional-collaborator pattern already used for importValueResolver) and applied it in resolveNode for both plain string values and the output of resolved intrinsics.CloudFormationService/CloudFormationResourceProvisioner: wired the new resolver into every place a CloudFormationTemplateEngine is constructed, backed by the existing CfnDynamicReferences bean.{{resolve:ssm:...}}and{{resolve:secretsmanager:...}}and asserts the deployed function has the resolved value, alongside a regression case for plain literal strings.Checklist
./mvnw testpasses locally