From 2a3eeecb186c558f452b723ca3a4eba001a4c858 Mon Sep 17 00:00:00 2001 From: Eric Beard Date: Wed, 30 Oct 2024 16:44:23 -0700 Subject: [PATCH 1/2] Fix bugs in Constants an Sub processing --- cft/graph/util.go | 2 +- cft/parse/parse_sub.go | 8 ++- cft/parse/parse_sub_test.go | 6 +-- cft/pkg/constants.go | 94 +++++++++++++++++++++++++++++++++++ cft/pkg/constants_test.go | 32 ++++++++++-- cft/pkg/module.go | 2 +- cft/pkg/pkg.go | 72 +++------------------------ internal/cmd/cc/resolve.go | 4 +- test/templates/constants.yaml | 2 + 9 files changed, 145 insertions(+), 77 deletions(-) diff --git a/cft/graph/util.go b/cft/graph/util.go index f78ac32e..4e7a27c7 100644 --- a/cft/graph/util.go +++ b/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 diff --git a/cft/parse/parse_sub.go b/cft/parse/parse_sub.go index b5ecbce1..0c9790c7 100644 --- a/cft/parse/parse_sub.go +++ b/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) diff --git a/cft/parse/parse_sub_test.go b/cft/parse/parse_sub_test.go index fabc1da8..f429498e 100644 --- a/cft/parse/parse_sub_test.go +++ b/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) } diff --git a/cft/pkg/constants.go b/cft/pkg/constants.go index a9b34397..a6183c0c 100644 --- a/cft/pkg/constants.go +++ b/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) +} diff --git a/cft/pkg/constants_test.go b/cft/pkg/constants_test.go index bcd7b234..4122c54d 100644 --- a/cft/pkg/constants_test.go +++ b/cft/pkg/constants_test.go @@ -5,6 +5,8 @@ import ( "github.com/aws-cloudformation/rain/cft/diff" "github.com/aws-cloudformation/rain/cft/parse" + "github.com/aws-cloudformation/rain/internal/config" + "github.com/aws-cloudformation/rain/internal/node" "gopkg.in/yaml.v3" ) @@ -32,6 +34,8 @@ Resources: Type: AWS::S3::Bucket Properties: BucketName: !Sub "pre-${Prefix}-${Rain::Test1}-suffix" + Foo: !Sub ${Rain::Test1} + Bar: !Sub ${!leavemealone} ` expect := ` Parameters: @@ -50,11 +54,12 @@ 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 + config.Debug = true p, err := parse.String(source) if err != nil { @@ -71,6 +76,9 @@ Resources: t.Fatal(err) } + config.Debugf("tmpl: %s", node.ToSJson(tmpl.Node)) + config.Debugf("et: %s", node.ToSJson(et.Node)) + d := diff.New(tmpl, et) if d.Mode() != "=" { t.Errorf("Output does not match expected: %v", d.Format(true)) @@ -90,3 +98,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) + } + } +} diff --git a/cft/pkg/module.go b/cft/pkg/module.go index 97e235c0..04f30270 100644 --- a/cft/pkg/module.go +++ b/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 } diff --git a/cft/pkg/pkg.go b/cft/pkg/pkg.go index 51b9010a..a4c7e79b 100644 --- a/cft/pkg/pkg.go +++ b/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 diff --git a/internal/cmd/cc/resolve.go b/internal/cmd/cc/resolve.go index 53010cbd..f9ca19d9 100644 --- a/internal/cmd/cc/resolve.go +++ b/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 } diff --git a/test/templates/constants.yaml b/test/templates/constants.yaml index 20174188..c7eaacdf 100644 --- a/test/templates/constants.yaml +++ b/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} From baf9606e74a78a857d3be28e385feaa8dcdf415e Mon Sep 17 00:00:00 2001 From: Eric Beard Date: Wed, 30 Oct 2024 16:47:05 -0700 Subject: [PATCH 2/2] Remove debug from test --- cft/pkg/constants_test.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/cft/pkg/constants_test.go b/cft/pkg/constants_test.go index 4122c54d..25b5bae6 100644 --- a/cft/pkg/constants_test.go +++ b/cft/pkg/constants_test.go @@ -5,8 +5,6 @@ import ( "github.com/aws-cloudformation/rain/cft/diff" "github.com/aws-cloudformation/rain/cft/parse" - "github.com/aws-cloudformation/rain/internal/config" - "github.com/aws-cloudformation/rain/internal/node" "gopkg.in/yaml.v3" ) @@ -59,7 +57,7 @@ Resources: Bar: ${!leavemealone} ` - config.Debug = true + //config.Debug = true p, err := parse.String(source) if err != nil { @@ -76,9 +74,6 @@ Resources: t.Fatal(err) } - config.Debugf("tmpl: %s", node.ToSJson(tmpl.Node)) - config.Debugf("et: %s", node.ToSJson(et.Node)) - d := diff.New(tmpl, et) if d.Mode() != "=" { t.Errorf("Output does not match expected: %v", d.Format(true))