Repository: aws-cloudformation/rain (go)

Issue:
When merging CFn files, replace ImportValues with Refs 
When merging CFn files, replace ImportValues (Fn::ImportValue) with Refs to original values. This will remove cross-stack references and help enable better analysis of merged files (e.g., using cfn-lint).

For example, imagine 2 CFn files, where one imports its VPC 

```
  "ExportsOutputRefVPCB9E5F0B4BD23A326": {
   "Value": {
    "Ref": "VPCB9E5F0B4"
   },
   "Export": {
    "Name": "beta-us-west-2-Vpc:ExportsOutputRefVPCB9E5F0B4BD23A326"
   }
  }
```

and another file uses the exported value

```
"VpcId": {
     "Fn::ImportValue": "beta-us-west-2-Vpc:ExportsOutputRefVPCB9E5F0B4BD23A326"
    }
```
   

We would like the merged file to replace Fn::ImportValue with a Ref, as in

```
"VpcId": {
     "Ref": "VPCB9E5F0B4"
    }
```
   


Pull request description:
When merging templates with Outputs, replace Imports that reference Exported Names
Fixes #564

Patch:
diff --git a/cft/format/pkl.go b/cft/format/pkl.go
index 565e3ffb..27fdd663 100644
--- a/cft/format/pkl.go
+++ b/cft/format/pkl.go
@@ -146,8 +146,12 @@ func writeResource(sb *strings.Builder, name string, resource *yaml.Node, basic
 				// Don't bother outputting type, since it's in the module
 				continue
 			}
-			// Basic rendering, without using module classes
-			w(sb, "    %s[\"%s\"]", indent, attrName)
+			if basic {
+				// Basic rendering, without using module classes
+				w(sb, "    %s[\"%s\"]", indent, attrName)
+			} else {
+				w(sb, "    %s%s ", indent, attrName)
+			}
 			switch attr.Kind {
 			case yaml.ScalarNode:
 				sb.WriteString(" = ")
diff --git a/internal/cmd/merge/merge.go b/internal/cmd/merge/merge.go
index d260e4ba..d66d20bc 100644
--- a/internal/cmd/merge/merge.go
+++ b/internal/cmd/merge/merge.go
@@ -8,6 +8,7 @@ import (
 	"github.com/aws-cloudformation/rain/cft/format"
 	"github.com/aws-cloudformation/rain/cft/parse"
 	"github.com/aws-cloudformation/rain/internal/config"
+	"github.com/aws-cloudformation/rain/internal/node"
 	"github.com/aws-cloudformation/rain/internal/ui"
 	"github.com/spf13/cobra"
 )
@@ -48,7 +49,7 @@ var Cmd = &cobra.Command{
 			}
 		}
 
-		config.Debugf("merged: %v", merged)
+		config.Debugf("merged: %v", node.ToSJson(merged.Node))
 
 		out := format.String(merged, format.Options{})
 		if outFn != "" {
diff --git a/internal/cmd/merge/outputs.go b/internal/cmd/merge/outputs.go
new file mode 100644
index 00000000..ca967361
--- /dev/null
+++ b/internal/cmd/merge/outputs.go
@@ -0,0 +1,71 @@
+package merge
+
+import (
+	"github.com/aws-cloudformation/rain/cft"
+	"github.com/aws-cloudformation/rain/cft/visitor"
+	"github.com/aws-cloudformation/rain/internal/config"
+	"github.com/aws-cloudformation/rain/internal/node"
+	"github.com/aws-cloudformation/rain/internal/s11n"
+	"gopkg.in/yaml.v3"
+)
+
+// mergeOutputImports looks for exported output values that are imported in this template.
+// This happens when we merge template A that has Outputs, and template B imports them.
+// Since we are merging the templates, the outputs and imports are not necessary.
+// If there are any, it replaces the Fn::Import nodes with the value.
+// Otherwise the template is returned as is
+func mergeOutputImports(t cft.Template) (cft.Template, error) {
+	outputs, err := t.GetSection(cft.Outputs)
+	if err != nil {
+		// This is expected if the template has no Outputs
+		config.Debugf("No outputs: %v", err)
+		return t, nil
+	}
+	exportMap := make(map[string]*yaml.Node)
+	for i := 0; i < len(outputs.Content); i += 2 {
+		name := outputs.Content[i].Value
+		val := outputs.Content[i+1]
+
+		config.Debugf("Checking %s: %s", name, node.ToSJson(val))
+
+		_, exp, _ := s11n.GetMapValue(val, "Export")
+		if exp != nil {
+			_, exportName, _ := s11n.GetMapValue(exp, "Name")
+			if exportName != nil {
+				// We found an export. Store the value for later when we go look for Fn::Import
+				_, exportVal, _ := s11n.GetMapValue(val, "Value")
+				if exportVal != nil {
+					exportMap[exportName.Value] = exportVal
+				} else {
+					config.Debugf("Unexpected: %s does not have an Export Value", name)
+				}
+			} else {
+				config.Debugf("Unexpected: %s does not have an Export Name", name)
+			}
+		}
+	}
+
+	if len(exportMap) > 0 {
+		config.Debugf("exportMap: %v", exportMap)
+
+		vf := func(n *visitor.Visitor) {
+			yamlNode := n.GetYamlNode()
+			if yamlNode.Kind == yaml.MappingNode {
+				if len(yamlNode.Content) == 2 && yamlNode.Content[0].Value == "Fn::Import" {
+					exportVal, ok := exportMap[yamlNode.Content[1].Value]
+					if ok {
+						// We found a match
+						config.Debugf("Replacing %s with %s", yamlNode.Content[1].Value, node.ToSJson(exportVal))
+						*yamlNode = *node.Clone(exportVal)
+					}
+				}
+			}
+
+		}
+
+		visitor := visitor.NewVisitor(t.Node)
+		visitor.Visit(vf)
+	}
+
+	return t, nil
+}
diff --git a/internal/cmd/merge/util.go b/internal/cmd/merge/util.go
index 09285636..10997d97 100644
--- a/internal/cmd/merge/util.go
+++ b/internal/cmd/merge/util.go
@@ -6,6 +6,7 @@ import (
 	"github.com/aws-cloudformation/rain/cft"
 	"github.com/aws-cloudformation/rain/cft/parse"
 	"github.com/aws-cloudformation/rain/internal/config"
+	"gopkg.in/yaml.v3"
 )
 
 func checkMerge(name string, dst, src map[string]interface{}) error {
@@ -145,6 +146,20 @@ func mergeTemplates(dstTemplate, srcTemplate cft.Template) (cft.Template, error)
 		}
 	}
 
-	config.Debugf("dst: %v", dst)
-	return parse.Map(dst)
+	config.Debugf("map dst: %v", dst)
+	retval, err := parse.Map(dst)
+	if err != nil {
+		return retval, err
+	}
+
+	// parse.Map does not actually return a correct cft.Template
+	// It's mostly used for unit tests except for here.
+
+	// Add the document node
+	docNode := &yaml.Node{Kind: yaml.DocumentNode, Content: make([]*yaml.Node, 0)}
+	docNode.Content = append(docNode.Content, retval.Node)
+	retval = cft.Template{Node: docNode}
+
+	// Merge Outputs with Fn::ImportValue
+	return mergeOutputImports(retval)
 }
diff --git a/scripts/integ.sh b/scripts/integ.sh
index bb5477fd..211a7426 100755
--- a/scripts/integ.sh
+++ b/scripts/integ.sh
@@ -48,6 +48,9 @@ set -eoux pipefail
 ./rain --profile rain pkg cft/pkg/tmpl/s3-template.yaml
 ./rain --profile rain pkg cft/pkg/tmpl/s3http-template.yaml
 
+# Make sure merge works
+./rain merge test/templates/merge-out-1.yaml test/templates/merge-out-2.yaml
+
 # Make sure build recommendations work
 ./internal/cmd/build/tmpl/scripts/validate.sh
 
@@ -56,4 +59,5 @@ set -eoux pipefail
 ./rain fmt test/templates/success.template --pkl --pkl-basic
 ./rain fmt test/templates/condition-stringlike.yaml --pkl > test/pkl/condition-stringlike.pkl
 pkl eval --project-dir test/pkl test/pkl/condition-stringlike.pkl -f yaml
+./rain --profile rain pkg test/webapp/webapp.pkl | cfn-lint
 


Test patch:
diff --git a/internal/cmd/merge/merge_test.go b/internal/cmd/merge/merge_test.go
index 3f13c44e..022beed5 100644
--- a/internal/cmd/merge/merge_test.go
+++ b/internal/cmd/merge/merge_test.go
@@ -3,6 +3,7 @@ package merge
 import (
 	"testing"
 
+	"github.com/aws-cloudformation/rain/cft/diff"
 	"github.com/aws-cloudformation/rain/cft/parse"
 	"github.com/google/go-cmp/cmp"
 )
@@ -313,3 +314,74 @@ func TestMergeTemplatesClash(t *testing.T) {
 		t.Fail()
 	}
 }
+
+// TestMergeOutputs tests merging templates where one has Fn::Import statements that reference Outputs in the other
+func TestMergeOutputs(t *testing.T) {
+
+	// Export a value
+	t1 := `
+Resources:
+  Bucket:
+    Type: AWS::S3::Bucket
+Outputs:
+  BucketName:
+    Value: !Ref Bucket
+    Export: 
+      Name: BucketNameExport
+`
+
+	// Reference the value from a different template
+	t2 := `
+Resources:
+  AccessLogsBucket:
+    Type: AWS::S3::Bucket
+    Properties:
+      BucketName: 
+        Fn::Sub:
+          - ${ParentBucket}-access-logs
+          - ParentBucket: 
+              Fn::Import: BucketNameExport
+`
+
+	// The merged template converts the Import to a Ref
+	expected := `
+Resources:
+  Bucket:
+    Type: AWS::S3::Bucket
+  AccessLogsBucket:
+    Type: AWS::S3::Bucket
+    Properties:
+      BucketName: !Sub
+        - ${ParentBucket}-access-logs
+        - ParentBucket: !Ref Bucket
+Outputs:
+  BucketName:
+    Value: !Ref Bucket
+    Export: 
+      Name: BucketNameExport
+`
+
+	template1, err := parse.String(t1)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	template2, err := parse.String(t2)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	expectedTemplate, err := parse.String(expected)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	merged, err := mergeTemplates(template1, template2)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if d := diff.New(expectedTemplate, merged); d.Mode() != "=" {
+		t.Errorf("%s", d.Format(true))
+	}
+}
diff --git a/test/pkl/condition-stringlike.pkl b/test/pkl/condition-stringlike.pkl
index 0a71957d..da85c5c1 100644
--- a/test/pkl/condition-stringlike.pkl
+++ b/test/pkl/condition-stringlike.pkl
@@ -10,15 +10,13 @@ Metadata {
 
 Resources {
     ["TempRole"] = new role.Role {
-        Type = "AWS::IAM::Role"
-        Metadata {
+        Metadata  {
         }
         RoleName = "TempRole"
     }
 
     ["TempManagedPolicy"] = new managedpolicy.ManagedPolicy {
-        Type = "AWS::IAM::ManagedPolicy"
-        Metadata {
+        Metadata  {
         }
         Roles {
             cfn.Ref("TempRole")
diff --git a/test/templates/merge-out-1.yaml b/test/templates/merge-out-1.yaml
new file mode 100644
index 00000000..54dbdbf2
--- /dev/null
+++ b/test/templates/merge-out-1.yaml
@@ -0,0 +1,9 @@
+Resources:
+  Bucket:
+    Type: AWS::S3::Bucket
+Outputs:
+  BucketName:
+    Value: !Ref Bucket
+    Export: 
+      Name: BucketNameExport
+
diff --git a/test/templates/merge-out-2.yaml b/test/templates/merge-out-2.yaml
new file mode 100644
index 00000000..b06ef134
--- /dev/null
+++ b/test/templates/merge-out-2.yaml
@@ -0,0 +1,10 @@
+Resources:
+  AccessLogsBucket:
+    Type: AWS::S3::Bucket
+    Properties:
+      BucketName: 
+        Fn::Sub:
+          - ${ParentBucket}-access-logs
+          - ParentBucket: 
+              Fn::Import: BucketNameExport
+