Merge pull request #577 from ericzbeard/fix-572

Fix bugs in Constants and Sub processing

cft/graph/util.go

@@ -13,7 +13,7 @@ func getRefs(t map[string]interface{}) []string {
 }
 
 func parseSubString(refs []string, substr string) []string {
-	words, err := parse.ParseSub(substr)
+	words, err := parse.ParseSub(substr, false)
 	if err != nil {
 		config.Debugf("Unable to parse Sub %s: %v", substr, err)
 		return refs

cft/parse/parse_sub.go

@@ -49,7 +49,7 @@ const (
 //	SubWord { T: STR, W: "-123" }
 //
 // Invalid syntax like "${AAA" returns an error
-func ParseSub(sub string) ([]SubWord, error) {
+func ParseSub(sub string, leaveBang bool) ([]SubWord, error) {
 	words := make([]SubWord, 0)
 	state := READSTR
 	var last rune
@@ -111,6 +111,12 @@ func ParseSub(sub string) ([]SubWord, error) {
 			if state == READLIT {
 				// Don't write the ! to the string
 				state = READSTR
+				if leaveBang {
+					// Unless we actually want it
+					// The cc command doesn't want it
+					// The Rain Constants parser wants it
+					buf += string(r)
+				}
 			} else {
 				// This is a ! somewhere not related to a LITERAL
 				buf += string(r)

cft/parse/parse_sub_test.go

@@ -48,7 +48,7 @@ func TestParseSub(t *testing.T) {
 	}
 
 	for sub, expect := range cases {
-		words, err := parse.ParseSub(sub)
+		words, err := parse.ParseSub(sub, false)
 		if err != nil {
 			t.Fatal(err)
 		}
@@ -65,14 +65,14 @@ func TestParseSub(t *testing.T) {
 
 	// Invalid strings should fail
 	sub := "${AAA"
-	words, err := parse.ParseSub(sub)
+	words, err := parse.ParseSub(sub, false)
 	if err == nil {
 		t.Fatalf("\"%s\": should have failed, got %v", sub, words)
 	}
 
 	// Rain constants
 	sub = "${Rain::Test}"
-	words, err = parse.ParseSub(sub)
+	words, err = parse.ParseSub(sub, false)
 	if err != nil {
 		t.Fatal(err)
 	}

cft/pkg/constants.go

@@ -3,8 +3,11 @@ package pkg
 import (
 	"fmt"
 
+	"github.com/aws-cloudformation/rain/cft/parse"
+	"github.com/aws-cloudformation/rain/cft/visitor"
 	"github.com/aws-cloudformation/rain/internal/config"
 	"github.com/aws-cloudformation/rain/internal/node"
+	"gopkg.in/yaml.v3"
 )
 
 // rainConstant parses a !Rain::Constant node
@@ -25,3 +28,94 @@ func rainConstant(ctx *directiveContext) (bool, error) {
 
 	return true, nil
 }
+
+// replaceConstants replaces ${Rain::ConstantName} in a single scalar node
+// If the constant name is not found in the map created from the Rain section
+// In the template, an error is returned
+func replaceConstants(n *yaml.Node, constants map[string]*yaml.Node) error {
+	if n.Kind != yaml.ScalarNode {
+		return fmt.Errorf("expected n to be a ScalarNode")
+	}
+
+	// Parse every scalar as if it was a Sub. Look for ${Rain::X}
+
+	retval := ""
+	words, err := parse.ParseSub(n.Value, true)
+	if err != nil {
+		return err
+	}
+	for _, w := range words {
+		switch w.T {
+		case parse.STR:
+			retval += w.W
+		case parse.REF:
+			retval += fmt.Sprintf("${%s}", w.W)
+		case parse.AWS:
+			retval += fmt.Sprintf("${AWS::%s}", w.W)
+		case parse.GETATT:
+			retval += fmt.Sprintf("${%s}", w.W)
+		case parse.RAIN:
+			val, ok := constants[w.W]
+			if !ok {
+				return fmt.Errorf("did not find Rain constant %s", w.W)
+			}
+			retval += val.Value
+		}
+	}
+
+	config.Debugf("Replacing %s with %s", n.Value, retval)
+	n.Value = retval
+
+	return nil
+}
+
+func IsSubNeeded(s string) bool {
+
+	words, err := parse.ParseSub(s, true)
+	if err != nil {
+		config.Debugf("error in IsSubNeeded: %v", err)
+		return true
+	}
+	for _, w := range words {
+		switch w.T {
+		case parse.STR:
+			// Ignore this
+		default:
+			// Anything else means it's needed
+			return true
+		}
+	}
+	return false
+}
+
+// replaceTemplateConstants scans the entire template looking for Sub strings
+// and replaces all instances of ${Rain::ConstantName} if that name exists
+// in the Rain/Constants section of the template
+func replaceTemplateConstants(templateNode *yaml.Node, constants map[string]*yaml.Node) {
+
+	config.Debugf("Constants: %v", constants)
+
+	vf := func(n *visitor.Visitor) {
+		yamlNode := n.GetYamlNode()
+		if yamlNode.Kind == yaml.MappingNode {
+			if len(yamlNode.Content) == 2 && yamlNode.Content[0].Value == "Fn::Sub" {
+				config.Debugf("About to replace constants in %s", yamlNode.Content[1].Value)
+				err := replaceConstants(yamlNode.Content[1], constants)
+				if err != nil {
+					config.Debugf("%v", err)
+				}
+
+				// Remove unnecessary Subs
+				// Parse the value again and see if it has any non-words
+				if !IsSubNeeded(yamlNode.Content[1].Value) {
+					config.Debugf("Sub is not needed for %s", yamlNode.Content[1].Value)
+					*yamlNode = yaml.Node{Kind: yaml.ScalarNode, Value: yamlNode.Content[1].Value}
+				}
+
+			}
+		}
+	}
+
+	visitor := visitor.NewVisitor(templateNode)
+	visitor.Visit(vf)
+}

cft/pkg/constants_test.go

@@ -32,6 +32,8 @@ Resources:
     Type: AWS::S3::Bucket
     Properties:
       BucketName: !Sub "pre-${Prefix}-${Rain::Test1}-suffix" 
+      Foo: !Sub ${Rain::Test1}
+      Bar: !Sub ${!leavemealone}
 `
 	expect := `
 Parameters:
@@ -50,8 +52,9 @@ Resources:
   Bucket3:
     Type: AWS::S3::Bucket
     Properties:
-      BucketName: 
-        Fn::Sub: pre-${Prefix}-ezbeard-rain-test-constants-suffix
+      BucketName: !Sub pre-${Prefix}-ezbeard-rain-test-constants-suffix
+      Foo: ezbeard-rain-test-constants
+      Bar: ${!leavemealone}
 `
 
 	//config.Debug = true
@@ -90,3 +93,21 @@ func TestReplaceConstants(t *testing.T) {
 		t.Fatalf("Expected Foo, got %s", n.Value)
 	}
 }
+
+func TestIsSubNeeded(t *testing.T) {
+	cases := make(map[string]bool)
+	cases["ABC"] = false
+	cases["${A}bc"] = true
+	cases["${Rain::Something}"] = true
+	cases[""] = false
+	cases["${Abc.Def"] = true
+	cases["${!saml:sub}"] = false
+	cases["${!Literal}-abc"] = false
+	cases["$foo$bar"] = false
+
+	for k, v := range cases {
+		if IsSubNeeded(k) != v {
+			t.Errorf("IsSubNeeded(%s) should be %v", k, v)
+		}
+	}
+}

cft/pkg/module.go

@@ -395,7 +395,7 @@ func resolveModuleSub(parentName string, prop *yaml.Node, sidx int, ctx *refctx)
 
 	refFoundInParams := false
 
-	words, err := parse.ParseSub(prop.Value)
+	words, err := parse.ParseSub(prop.Value, true)
 	if err != nil {
 		return err
 	}

cft/pkg/pkg.go

@@ -59,7 +59,7 @@ type transformContext struct {
 func transform(ctx *transformContext) (bool, error) {
 	changed := false
 
-	config.Debugf("transform: %s", node.ToSJson(ctx.nodeToTransform))
+	// config.Debugf("transform: %s", node.ToSJson(ctx.nodeToTransform))
 
 	// registry is a map of functions defined in rain.go
 	for path, fn := range registry {
@@ -79,70 +79,6 @@ func transform(ctx *transformContext) (bool, error) {
 	return changed, nil
 }
 
-// replaceConstants replaces ${Rain::ConstantName} in a single scalar node
-// If the constant name is not found in the map created from the Rain section
-// In the template, an error is returned
-func replaceConstants(n *yaml.Node, constants map[string]*yaml.Node) error {
-	if n.Kind != yaml.ScalarNode {
-		return fmt.Errorf("expected n to be a ScalarNode")
-	}
-
-	// Parse every scalar as if it was a Sub. Look for ${Rain::X}
-
-	retval := ""
-	words, err := parse.ParseSub(n.Value)
-	if err != nil {
-		return err
-	}
-	for _, w := range words {
-		switch w.T {
-		case parse.STR:
-			retval += w.W
-		case parse.REF:
-			retval += fmt.Sprintf("${%s}", w.W)
-		case parse.AWS:
-			retval += fmt.Sprintf("${AWS::%s}", w.W)
-		case parse.GETATT:
-			retval += fmt.Sprintf("${%s}", w.W)
-		case parse.RAIN:
-			val, ok := constants[w.W]
-			if !ok {
-				return fmt.Errorf("did not find Rain constant %s", w.W)
-			}
-			retval += val.Value
-		}
-	}
-
-	config.Debugf("Replacing %s with %s", n.Value, retval)
-	n.Value = retval
-
-	return nil
-}
-
-// replaceTemplateConstants scans the entire template looking for Sub strings
-// and replaces all instances of ${Rain::ConstantName} if that name exists
-// in the Rain/Constants section of the template
-func replaceTemplateConstants(templateNode *yaml.Node, constants map[string]*yaml.Node) {
-
-	config.Debugf("Constants: %v", constants)
-
-	vf := func(n *visitor.Visitor) {
-		yamlNode := n.GetYamlNode()
-		if yamlNode.Kind == yaml.MappingNode {
-			if len(yamlNode.Content) == 2 && yamlNode.Content[0].Value == "Fn::Sub" {
-				config.Debugf("About to replace constants in %s", yamlNode.Content[1].Value)
-				err := replaceConstants(yamlNode.Content[1], constants)
-				if err != nil {
-					config.Debugf("%v", err)
-				}
-			}
-		}
-	}
-
-	visitor := visitor.NewVisitor(templateNode)
-	visitor.Visit(vf)
-}
-
 // Template returns t with assets included as per AWS CLI packaging rules
 // and any Rain:: functions used.
 // rootDir must be passed in so that any included assets can be loaded from the same directory
@@ -156,9 +92,11 @@ func Template(t cft.Template, rootDir string, fs *embed.FS) (cft.Template, error
 	// First look for a Rain section and store constants
 	t.Constants = make(map[string]*yaml.Node)
 	rainNode, err := t.GetSection(cft.Rain)
+	hasRainNode := false
 	if err != nil {
 		config.Debugf("Unable to get Rain section: %v", err)
 	} else {
+		hasRainNode = true
 		config.Debugf("Rain node: %s", node.ToSJson(rainNode))
 		_, c, _ := s11n.GetMapValue(rainNode, "Constants")
 		if c != nil {
@@ -260,7 +198,9 @@ func Template(t cft.Template, rootDir string, fs *embed.FS) (cft.Template, error
 	v.Visit(replaceAnchors)
 
 	// Look for ${Rain::ConstantName} in all Sub strings
-	replaceTemplateConstants(templateNode, constants)
+	if hasRainNode {
+		replaceTemplateConstants(templateNode, constants)
+	}
 
 	// Marshal and Unmarshal to resolve new line/column numbers
 

internal/cmd/cc/resolve.go

@@ -361,7 +361,7 @@ func resolveSub(n *yaml.Node, resource *Resource) (string, error) {
 		// Ref for single strings like ${MyParam} or ${MyBucket}
 		// Map values
 		// ${!Literal}
-		words, err := parse.ParseSub(n.Value)
+		words, err := parse.ParseSub(n.Value, false)
 		if err != nil {
 			return "", err
 		}
@@ -377,7 +377,7 @@ func resolveSub(n *yaml.Node, resource *Resource) (string, error) {
 		if n.Content[1].Kind != yaml.MappingNode {
 			return "", fmt.Errorf("expected Sub %s Content[1] to be a Mapping", sub)
 		}
-		words, err := parse.ParseSub(sub)
+		words, err := parse.ParseSub(sub, false)
 		if err != nil {
 			return "", err
 		}

test/templates/constants.yaml

@@ -22,4 +22,6 @@ Resources:
     Type: AWS::S3::Bucket
     Properties:
       BucketName: !Sub "pre-${Prefix}-${Rain::Test1}-suffix" 
+      Foo: !Sub ${Rain::Test1}
+      Bar: !Sub ${!leavemealone}