Repository: aws-cloudformation/rain
Pull request #307 (2024-03-06 00:24:41)

Fix bug in module sequences


Patch:
diff --git a/cft/pkg/module.go b/cft/pkg/module.go
index 12f61212..95326cec 100644
--- a/cft/pkg/module.go
+++ b/cft/pkg/module.go
@@ -133,23 +133,21 @@ type refctx struct {
 
 func replaceProp(prop *yaml.Node, parentName string, v *yaml.Node, outNode *yaml.Node, sidx int) error {
 
-	// config.Debugf("replaceProp prop: %s, parentName: %s, v: %s, sidx: %d",
-	//  	node.ToSJson(prop), parentName, node.ToSJson(v), sidx)
-
 	if sidx > -1 {
 		// The node is a sequence element
 
 		newVal := node.Clone(v)
 
-		// TODO: Find the sequence in outNode and use the parent to replace it
-		// if v is a Mapping like a Ref
-
 		if v.Kind == yaml.MappingNode {
 			parentNode := node.GetParent(prop, outNode, nil)
-			//config.Debugf("replaceProp parentNode: %s", parentNode.String())
 			*parentNode.Value = *newVal
 		} else {
-			*prop = *newVal
+			parentNode := node.GetParent(prop, outNode, nil)
+			if parentNode.Key != nil {
+				*parentNode.Value = *newVal
+			} else {
+				*prop = *newVal
+			}
 		}
 		return nil
 	}
@@ -227,8 +225,6 @@ func resolveModuleRef(parentName string, prop *yaml.Node, sidx int, ctx *refctx)
 					prop.Value)
 			}
 
-			//config.Debugf("resolveModuleRef about to call replaceProp")
-
 			replaceProp(prop, parentName, parentVal, outNode, sidx)
 
 			refFoundInParams = true
@@ -380,9 +376,6 @@ func resolveModuleSub(parentName string, prop *yaml.Node, sidx int, ctx *refctx)
 // If sidx is > -1, this prop is in a sequence
 func renamePropRefs(parentName string, propName string, prop *yaml.Node, sidx int, ctx *refctx) error {
 
-	//config.Debugf("renamePropRefs parentName: %s, propName: %s, prop: %s, sidx: %d",
-	//	parentName, propName, node.ToSJson(prop), sidx)
-
 	logicalId := ctx.logicalId
 
 	// Properties:
@@ -418,10 +411,6 @@ func renamePropRefs(parentName string, propName string, prop *yaml.Node, sidx in
 			// Recurse over array elements
 			for i, p := range prop.Content {
 				// propName is blank so the next parentName is blank
-
-				//config.Debugf("About to call renamePropRefs, propName is %s", propName)
-
-				//result := renamePropRefs(parentName, p.Value, prop.Content[i], i, ctx)
 				result := renamePropRefs(propName, p.Value, prop.Content[i], i, ctx)
 				if result != nil {
 					return fmt.Errorf("recursing over array %s: %v", parentName, result)
@@ -433,23 +422,12 @@ func renamePropRefs(parentName string, propName string, prop *yaml.Node, sidx in
 		for i, p := range prop.Content {
 			if i%2 == 0 {
 
-				// TODO: If this Mapping is inside a Sequence, we need to be able
-				// to replace the entire mapping element.
-				//
-				// Prop:
-				//   - !Ref Foo
-				//
-				// Foo might resolve to a Sub, so the output should be:
-				//
-				// Prop:
-				//   - !Sub ${Foo}
-				//
-				// Right now what we get is:
-				//
-				// Prop:
-				//   - !Ref
-				//   Fn::Sub: ${Foo}
-				result := renamePropRefs(propName, p.Value, prop.Content[i+1], sidx, ctx)
+				// Don't pass sidx through if we're in a child node of the sequence
+				passSidx := sidx
+				if propName != "" {
+					passSidx = -1
+				}
+				result := renamePropRefs(propName, p.Value, prop.Content[i+1], passSidx, ctx)
 				if result != nil {
 					return fmt.Errorf("recursing over mapping node %s: %v", propName, result)
 				}
diff --git a/cft/pkg/tmpl/ref-false-expect.yaml b/cft/pkg/tmpl/ref-false-expect.yaml
new file mode 100644
index 00000000..ebdebb6a
--- /dev/null
+++ b/cft/pkg/tmpl/ref-false-expect.yaml
@@ -0,0 +1,16 @@
+Resources:
+  MyBucket1:
+    Type: AWS::S3::Bucket
+    Properties:
+      BucketName: foo
+      Parent:
+        - Child:
+            ShouldBeFalse: false
+        - OtherChild: false
+        - Subs:
+          - !Sub "b0false"
+          - !Sub "b1false"
+  MyBucket2:
+    Type: AWS::S3::Bucket
+    Properties:
+      BucketName: bar
diff --git a/cft/pkg/tmpl/ref-false-module.yaml b/cft/pkg/tmpl/ref-false-module.yaml
new file mode 100644
index 00000000..33bc3c0c
--- /dev/null
+++ b/cft/pkg/tmpl/ref-false-module.yaml
@@ -0,0 +1,20 @@
+Parameters:
+  Name:
+    Type: String
+  B:
+    Type: Boolean
+Resources:
+  Bucket1:
+    Type: AWS::S3::Bucket
+    Properties:
+      BucketName: !Ref Name
+      Parent:
+        - Child:
+            ShouldBeFalse: !Ref B
+        - OtherChild: !Ref B
+        - Subs:
+          - !Sub "b0${B}"
+          - !Sub "b1${B}"
+  Bucket2:
+    Type: AWS::S3::Bucket
+
diff --git a/cft/pkg/tmpl/ref-false-template.yaml b/cft/pkg/tmpl/ref-false-template.yaml
new file mode 100644
index 00000000..540574a5
--- /dev/null
+++ b/cft/pkg/tmpl/ref-false-template.yaml
@@ -0,0 +1,10 @@
+Resources:
+  My:
+    Type: !Rain::Module "./ref-false-module.yaml"
+    Properties:
+      Name: foo
+      B: false
+    Overrides:
+      Bucket2:
+        Properties:
+          BucketName: bar
diff --git a/cft/pkg/tmpl/sub-expect.yaml b/cft/pkg/tmpl/sub-expect.yaml
index 4df1c50a..5894602f 100644
--- a/cft/pkg/tmpl/sub-expect.yaml
+++ b/cft/pkg/tmpl/sub-expect.yaml
@@ -10,6 +10,9 @@ Resources:
       Z:
         - !Sub ${Foo}
         - !Ref MyBucket2
+        - ZZ:
+            ZZZ:
+              ZZZZ: !Sub ${Foo}
   MyBucket2:
     Type: AWS::S3::Bucket
     Properties:
diff --git a/cft/pkg/tmpl/sub-module.yaml b/cft/pkg/tmpl/sub-module.yaml
index e517d8ea..b088c597 100644
--- a/cft/pkg/tmpl/sub-module.yaml
+++ b/cft/pkg/tmpl/sub-module.yaml
@@ -15,6 +15,9 @@ Resources:
       Z:
         - !Ref SubName
         - !Ref Bucket2
+        - ZZ:
+            ZZZ:
+              ZZZZ: !Sub "${SubName}"
   Bucket2:
     Type: AWS::S3::Bucket
 
diff --git a/internal/node/node.go b/internal/node/node.go
index e163eeb5..3e7eb31c 100644
--- a/internal/node/node.go
+++ b/internal/node/node.go
@@ -66,7 +66,9 @@ func Clone(node *yaml.Node) *yaml.Node {
 //
 // In the above, if I want b's parent node pair, I get [Name, Map]
 // This allows us to ask "what is the parent's name",
-// which is useful for knowing the logical name of the resource a node belongs to
+// which is useful for knowing the logical name of the resource a node belongs to.
+//
+// For sequence elements that are maps, the Key will be nil
 func GetParent(node *yaml.Node, rootNode *yaml.Node, priorNode *yaml.Node) NodePair {
 	if node == rootNode {
 		config.Debugf("getParent node and rootNode are the same")
@@ -83,17 +85,13 @@ func GetParent(node *yaml.Node, rootNode *yaml.Node, priorNode *yaml.Node) NodeP
 	var pair NodePair
 
 	if rootNode.Kind == yaml.DocumentNode || rootNode.Kind == yaml.SequenceNode {
-		for i, n := range rootNode.Content {
+		for _, n := range rootNode.Content {
 			if n == node {
 				found = rootNode
 				before = priorNode
 				break
 			}
-			var prior *yaml.Node
-			if i > 0 {
-				prior = rootNode.Content[i-1]
-			}
-			pair = GetParent(node, n, prior)
+			pair = GetParent(node, n, nil)
 			if pair.Value != nil {
 				found = pair.Value
 				before = pair.Key
@@ -227,8 +225,6 @@ func SetMapValue(parent *yaml.Node, name string, val *yaml.Node) {
 
 // Set the value of a sequence element within the node
 func SetSequenceValue(parent *yaml.Node, val *yaml.Node, sidx int) {
-	config.Debugf("SetSequenceValue parent: %s, val: %s, sidx: %v",
-		ToSJson(parent), ToSJson(val), sidx)
 
 	if len(parent.Content) <= sidx {
 		return


Test patch:
diff --git a/cft/pkg/module_test.go b/cft/pkg/module_test.go
index 008856e2..3f8036bb 100644
--- a/cft/pkg/module_test.go
+++ b/cft/pkg/module_test.go
@@ -30,6 +30,10 @@ func TestMany(t *testing.T) {
 	runTest("many", t)
 }
 
+func TestRefFalse(t *testing.T) {
+	runTest("ref-false", t)
+}
+
 // TODO: This was broken in the refactor, come back to it later
 //func TestForeach(t *testing.T) {
 //	runTest("foreach", t)
diff --git a/internal/node/node_test.go b/internal/node/node_test.go
index 1355c1d1..d765035c 100644
--- a/internal/node/node_test.go
+++ b/internal/node/node_test.go
@@ -64,7 +64,7 @@ func TestGetParentFound(t *testing.T) {
 		Value: "ChildOfChild",
 	}
 
-	childValue.Content = make([]*yaml.Node, 2)
+	childValue.Content = make([]*yaml.Node, 4)
 	childValue.Content[0] = &yaml.Node{Kind: yaml.ScalarNode, Value: "ChildOfChildKey"}
 	childValue.Content[1] = childOfChild
 
@@ -75,6 +75,53 @@ func TestGetParentFound(t *testing.T) {
 	if pair.Key != childKey {
 		t.Errorf("childKey should have been found as key for childOfChild")
 	}
+
+	sequenceKey := &yaml.Node{Kind: yaml.ScalarNode, Value: "ChildSequence"}
+	childValue.Content[2] = sequenceKey
+	sequence := &yaml.Node{Kind: yaml.SequenceNode, Content: make([]*yaml.Node, 2)}
+	childValue.Content[3] = sequence
+
+	sequence.Content[0] = &yaml.Node{Kind: yaml.ScalarNode, Value: "Seq0"}
+	sequence.Content[1] = &yaml.Node{Kind: yaml.ScalarNode, Value: "Seq1"}
+
+	// For a sequence, the parent Key should be  ??
+	pair = node.GetParent(sequence.Content[0], parent, nil)
+	if pair.Key != sequenceKey {
+		t.Errorf("Seq0 pair Key should be sequenceKey")
+	}
+	if pair.Value != sequence {
+		t.Errorf("Seq0 pair Value should be sequence")
+	}
+
+	pair = node.GetParent(sequence.Content[1], parent, nil)
+	if pair.Key != sequenceKey {
+		t.Errorf("Seq1 pair Key should be sequenceKey")
+	}
+	if pair.Value != sequence {
+		t.Errorf("Seq1 pair Value should be sequence")
+	}
+
+	// Replace the sequence content with Maps
+	map0 := &yaml.Node{Kind: yaml.MappingNode, Content: make([]*yaml.Node, 2)}
+	map1 := &yaml.Node{Kind: yaml.MappingNode, Content: make([]*yaml.Node, 2)}
+	sequence.Content[0] = map0
+	sequence.Content[1] = map1
+
+	map0.Content[0] = &yaml.Node{Kind: yaml.ScalarNode, Value: "Ref"}
+	map0.Content[1] = &yaml.Node{Kind: yaml.ScalarNode, Value: "Foo"}
+	map1.Content[0] = &yaml.Node{Kind: yaml.ScalarNode, Value: "Ref"}
+	map1.Content[1] = &yaml.Node{Kind: yaml.ScalarNode, Value: "Bar"}
+
+	pair = node.GetParent(map0.Content[1], parent, nil)
+	if pair.Key != nil {
+		t.Errorf("Foo pair Key should be nil")
+	}
+
+	pair = node.GetParent(map1.Content[1], parent, nil)
+	if pair.Key != nil {
+		t.Errorf("Bar pair Key should be nil")
+	}
+
 }
 
 func TestRemoveFromMap(t *testing.T) {