Repository: aws-cloudformation/rain
Pull request #148 (2023-06-02 23:35:36)

Fix module for hook
A few bugfixes related to a hook project in the community registry extensions repo.



Patch:
diff --git a/cft/pkg/module.go b/cft/pkg/module.go
index b6668c9e..f80e71a5 100644
--- a/cft/pkg/module.go
+++ b/cft/pkg/module.go
@@ -47,12 +47,14 @@ func cloneAndReplaceProps(
 
 			found := false
 
-			_, moduleParam := s11n.GetMapValue(moduleParams, tprop.Value)
+			if moduleParams != nil {
+				_, moduleParam := s11n.GetMapValue(moduleParams, tprop.Value)
 
-			// Don't clone template props that are module parameters.
-			// Module params are used when we resolve Refs later
-			if moduleParam != nil {
-				continue
+				// Don't clone template props that are module parameters.
+				// Module params are used when we resolve Refs later
+				if moduleParam != nil {
+					continue
+				}
 			}
 
 			// Override anything hard coded into the module that is present in the parent template
@@ -121,52 +123,64 @@ func renamePropRefs(
 	//   SimpleProp: Val
 	//   RefParam: !Ref NameOfParam
 	//   RefResource: !Ref NameOfResource
+	//   GetAtt: !GetAtt ModuleExtension.Arn
 	//   Complex:
 	//     AnArray:
 	//       - Element0
 	//           A: B
 	//           C: !Ref D
 
+	config.Debugf("renamePropRefs parentName: %v, propName: %v, prop.Kind: %v", parentName, propName, prop.Kind)
+
 	if prop.Kind == yaml.ScalarNode {
+		refFoundInParams := false
 		if propName == "Ref" {
-			// Find the module parameter that matches the !Ref
-			_, param := s11n.GetMapValue(moduleParams, prop.Value)
-			if param != nil {
-				// We need to get the parameter value from the parent template.
-				// Module params are set by the parent template resource properties.
-
-				// Look for this property name in the parent template
-				_, parentVal := s11n.GetMapValue(templateProps, prop.Value)
-				if parentVal == nil {
-					return fmt.Errorf("did not find %v in parent template Resource Properties", prop.Value)
-				}
+			if moduleParams != nil {
+				// Find the module parameter that matches the !Ref
+				_, param := s11n.GetMapValue(moduleParams, prop.Value)
+				if param != nil {
+					// We need to get the parameter value from the parent template.
+					// Module params are set by the parent template resource properties.
+
+					// Look for this property name in the parent template
+					_, parentVal := s11n.GetMapValue(templateProps, prop.Value)
+					if parentVal == nil {
+						return fmt.Errorf("did not find %v in parent template Resource Properties", prop.Value)
+					}
 
-				config.Debugf("parentVal: %v", node.ToJson(parentVal))
+					config.Debugf("parentVal: %v", node.ToJson(parentVal))
 
-				// We can't just set prop.Value, since we would end up with Prop: !Ref Value instead of just Prop: Value
-				// Get the property's parent and set the entire map value for the property
+					// We can't just set prop.Value, since we would end up with Prop: !Ref Value instead of just Prop: Value
+					// Get the property's parent and set the entire map value for the property
 
-				// Get the map parent within the extension node we created
-				refMap := node.GetParent(prop, ext, nil)
-				if refMap.Value == nil {
-					return fmt.Errorf("could not find parent for %v", prop)
-				}
-				propParentPair := node.GetParent(refMap.Value, ext, nil)
+					// Get the map parent within the extension node we created
+					refMap := node.GetParent(prop, ext, nil)
+					if refMap.Value == nil {
+						return fmt.Errorf("could not find parent for %v", prop)
+					}
+					propParentPair := node.GetParent(refMap.Value, ext, nil)
 
-				config.Debugf("propParentPair.Value: %v", node.ToJson(propParentPair.Value))
+					config.Debugf("propParentPair.Value: %v", node.ToJson(propParentPair.Value))
 
-				// Create a new node to replace what's defined in the module
-				newValue := node.Clone(parentVal)
+					// Create a new node to replace what's defined in the module
+					newValue := node.Clone(parentVal)
 
-				config.Debugf("Setting %v to:\n%v", parentName, node.ToJson(newValue))
+					config.Debugf("Setting %v to:\n%v", parentName, node.ToJson(newValue))
 
-				node.SetMapValue(propParentPair.Value, parentName, newValue)
-			} else {
+					node.SetMapValue(propParentPair.Value, parentName, newValue)
+
+					refFoundInParams = true
+				}
+			}
+			if !refFoundInParams {
 				config.Debugf("Did not find param for %v", prop.Value)
 				// Look for a resource in the module
 				_, resource := s11n.GetMapValue(moduleResources, prop.Value)
 				if resource == nil {
-					return fmt.Errorf("did not find !Ref %v", prop.Value)
+					config.Debugf("did not find !Ref %v", prop.Value)
+					// If we can't find the Ref, leave it alone and assume it's
+					// expected to be in the parent template to be resolved at deploy time.
+					return nil
 				}
 				fixedName := rename(logicalId, prop.Value)
 				prop.Value = fixedName
@@ -174,12 +188,32 @@ func renamePropRefs(
 		}
 	} else if prop.Kind == yaml.SequenceNode {
 		config.Debugf("Sequence %v %v", propName, node.ToJson(prop))
+		if propName == "Fn::GetAtt" {
+			// Convert !GetAtt ModuleExtension.Property to !GetAtt LogicalId.Property
+			fixedName := rename(logicalId, prop.Content[0].Value)
+			prop.Content[0].Value = fixedName
+			config.Debugf("Fixed GetAtt: %v", fixedName)
+		} else {
+			// Recurse over array elements
+			for i, p := range prop.Content {
+				result := renamePropRefs(propName,
+					p.Value, prop.Content[i], ext, moduleParams, moduleResources, logicalId, templateProps)
+				if result != nil {
+					return result
+				}
+			}
+		}
+
 	} else if prop.Kind == yaml.MappingNode {
 		config.Debugf("Mapping %v %v", propName, node.ToJson(prop))
 		for i, p := range prop.Content {
 			if i%2 == 0 {
-				return renamePropRefs(propName,
+				config.Debugf("About to renamePropRefs for Mapping Content: %v", p.Value)
+				result := renamePropRefs(propName,
 					p.Value, prop.Content[i+1], ext, moduleParams, moduleResources, logicalId, templateProps)
+				if result != nil {
+					return result
+				}
 			}
 		}
 	} else {
@@ -457,13 +491,7 @@ func module(n *yaml.Node, root string, t cft.Template, parent node.NodePair) (bo
 	var path string
 
 	// Is this a local file or a URL?
-	if strings.HasPrefix(uri, "file://") {
-		// Read the local file
-		content, path, err = expectFile(n, root)
-		if err != nil {
-			return false, err
-		}
-	} else if strings.HasPrefix(uri, "https://") {
+	if strings.HasPrefix(uri, "https://") {
 		// Download the file and then parse it
 		resp, err := http.Get(uri)
 		if err != nil {
@@ -476,7 +504,11 @@ func module(n *yaml.Node, root string, t cft.Template, parent node.NodePair) (bo
 		}
 		content = []byte(body)
 	} else {
-		return false, errors.New("expected either file://path or https://path")
+		// Read the local file
+		content, path, err = expectFile(n, root)
+		if err != nil {
+			return false, err
+		}
 	}
 
 	// Parse the file
diff --git a/cft/pkg/util.go b/cft/pkg/util.go
index 1c184d69..dcd820b6 100644
--- a/cft/pkg/util.go
+++ b/cft/pkg/util.go
@@ -28,6 +28,7 @@ import (
 	"github.com/aws-cloudformation/rain/internal/aws"
 	"github.com/aws-cloudformation/rain/internal/aws/s3"
 	"github.com/aws-cloudformation/rain/internal/config"
+	"github.com/aws-cloudformation/rain/internal/node"
 	"gopkg.in/yaml.v3"
 )
 
@@ -175,15 +176,22 @@ func expectString(n *yaml.Node) (string, error) {
 }
 
 func expectFile(n *yaml.Node, root string) ([]byte, string, error) {
+
+	config.Debugf("expectFile: %v", node.ToJson(n))
+
 	path, err := expectString(n)
 	if err != nil {
 		return nil, "", err
 	}
 
+	config.Debugf("root: %v, path: %v", root, path)
+
 	if !filepath.IsAbs(path) {
 		path = filepath.Join(root, path)
 	}
 
+	config.Debugf("path: %v", path)
+
 	info, err := os.Stat(path)
 	if err != nil {
 		return nil, path, err


Test patch:
diff --git a/test/modules/bucket.yaml b/test/modules/bucket.yaml
index e92831b5..67c20103 100644
--- a/test/modules/bucket.yaml
+++ b/test/modules/bucket.yaml
@@ -73,6 +73,12 @@ Resources:
 
   AdditionalResource1:
     Type: AWS::S3::Bucket
+    Properties:
+      X: !Ref NotFound
+      A: !GetAtt ModuleExtension.Arn
+      B:
+        - !GetAtt ModuleExtension.Arn
+        - C: !GetAtt ModuleExtension.Arn
 
   AdditionalResource2:
     Type: AWS::S3::Bucket
diff --git a/test/modules/expect.yaml b/test/modules/expect.yaml
index 2b96cef2..69203d9d 100644
--- a/test/modules/expect.yaml
+++ b/test/modules/expect.yaml
@@ -80,6 +80,12 @@ Resources:
 
   ModuleExampleAdditionalResource1:
     Type: AWS::S3::Bucket
+    Properties:
+      X: !Ref NotFound
+      A: !GetAtt ModuleExample.Arn
+      B:
+        - !GetAtt ModuleExample.Arn
+        - C: !GetAtt ModuleExample.Arn
 
   ModuleExampleAdditionalResource2:
     Type: AWS::S3::Bucket
diff --git a/test/templates/module.yaml b/test/templates/module.yaml
index a97f81e1..d424188a 100644
--- a/test/templates/module.yaml
+++ b/test/templates/module.yaml
@@ -17,7 +17,7 @@ Resources:
 
   ModuleExample:
     #Type: !Rain::Module "https://raw.githubusercontent.com/ericzbeard/rain/modules/test/modules/bucket.yaml"
-    Type: !Rain::Module "file://../../modules/bucket.yaml"
+    Type: !Rain::Module "../modules/bucket.yaml"
     UpdateReplacePolicy: Delete
     Condition: ConditionA
     DependsOn: SecondResourceInOriginal