diff --git a/README.md b/README.md index 0217d85a4..0e8083021 100644 --- a/README.md +++ b/README.md @@ -457,13 +457,14 @@ Modules are not allowed to refer to themselves directly or in cycles. Module A can’t import Module A, and it can’t import Module B if that module imports Module A. -Modules support a basic form of looping/foreach by adding a `Map` attribute to -the module configuration. Special variables `$MapIndex` and `$MapValue` can be -used to refer to the index and list value. Logical Ids are auto-incremented by +Modules support a basic form of looping/foreach by either using the familiar +`Fn::ForEach` syntax, or with a shorthand by adding a `ForEach` attribute to +the module configuration. Special variables `$Identifier` and `$Index` can be +used to refer to the value and list index. With the shorthand, or if you don't +put the Identifier in the logical id, logical ids are auto-incremented by adding an integer starting at zero. Since this is a client-side-only feature, list values must be fully resolved scalars, not values that must be resolved at -deploy time. When deploy-time values are needed, `Fn::ForEach` is a better -design option. Local modules do not process `Fn::ForEach`. +deploy time. ``` Parameters: @@ -474,7 +475,7 @@ Parameters: Modules: Content: Source: ./map-module.yaml - Map: !Ref List + ForEach: !Ref List Properties: Name: !Sub my-bucket-$MapValue ``` @@ -506,15 +507,12 @@ It’s also possible to refer to elements within a `Map` using something like which resolves to a list of all of the `Arn` outputs from that module. When a module is processed, the first thing that happens is parsing of the -`Conditions` within the module. These conditions must be fully resolvable -client-side, since the package command does not have access to Parameters or -deploy-time values. These conditions are converted to a dictionary of boolean -values and the `Conditions` section is not emitted into the parent template. It -is not merged into the parent. Any resources marked with a false condition are -removed, and any property nodes with conditions are processed. Any values of -`!Ref AWS::NoValue` are removed. No evidence of conditions will remain in the -markup that is merged into the parent template, unless the condition is not -found in the module. +Conditions within the module. Any Resources, Modules, or Outputs marked with a +false condition are removed, and any property nodes with conditions are +processed. Any values of !Ref AWS::NoValue are removed. Any unresolved +conditions (for example, a condition that references a paramter in the parent +template, or something like AWS::Region) are emitted into the parent template, +prefixed with the module name. Much of the value of module output is in the smart handling of `Ref`, `Fn::GetAtt`, and `Fn::Sub`. For the most part, we want these to “just work” in @@ -612,11 +610,6 @@ Modules: Source: $def/a/b/bar.yaml ``` - - - - - ### Publish modules to CodeArtifact Rain integrates with AWS CodeArtifact to enable an experience similar to npm diff --git a/cft/cft.go b/cft/cft.go index c29d67a3c..8f9bc0c7b 100644 --- a/cft/cft.go +++ b/cft/cft.go @@ -257,6 +257,10 @@ func (t *Template) AddMappedModule(copiedConfig *ModuleConfig) { t.ModuleMaps = make(map[string]*ModuleConfig) } t.ModuleMaps[copiedConfig.Name] = copiedConfig + keyName := copiedConfig.OriginalName + copiedConfig.MapKey + // Also add the name if referenced by key + t.ModuleMaps[keyName] = copiedConfig + if t.ModuleMapNames == nil { t.ModuleMapNames = make(map[string][]string) } @@ -282,3 +286,9 @@ func (t *Template) AddResolvedModuleNode(n *yaml.Node) { func (t *Template) ModuleAlreadyResolved(n *yaml.Node) bool { return slices.Contains(t.ModuleResolved, n) } + +// IsRef returns true if the node is a 2-length Mapping node +// that starts with "Ref" +func IsRef(n *yaml.Node) bool { + return len(n.Content) == 2 && n.Content[0].Value == string(Ref) +} diff --git a/cft/module_config.go b/cft/module_config.go index 5c534c294..4e99293e5 100644 --- a/cft/module_config.go +++ b/cft/module_config.go @@ -2,7 +2,10 @@ package cft import ( "errors" + "fmt" + "strings" + "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" @@ -43,6 +46,9 @@ type ModuleConfig struct { // The root directory of the template that configures this module ParentRootDir string + + // If this module is wrapped in Fn::ForEach, this will be populated + FnForEach *FnForEach } func (c *ModuleConfig) Properties() map[string]any { @@ -53,7 +59,8 @@ func (c *ModuleConfig) Overrides() map[string]any { return node.DecodeMap(c.OverridesNode) } -// ResourceOverridesNode returns the Overrides node for the given resource if it exists +// ResourceOverridesNode returns the Overrides node for the +// given resource if it exists func (c *ModuleConfig) ResourceOverridesNode(name string) *yaml.Node { if c.OverridesNode == nil { return nil @@ -66,19 +73,59 @@ const ( Source string = "Source" Properties string = "Properties" Overrides string = "Overrides" - Map string = "Map" + ForEach string = "Fn::ForEach" ) // parseModuleConfig parses a single module configuration // from the Modules section in the template -func (t *Template) ParseModuleConfig(name string, n *yaml.Node) (*ModuleConfig, error) { - if n.Kind != yaml.MappingNode { - return nil, errors.New("not a mapping node") - } +func (t *Template) ParseModuleConfig( + name string, n *yaml.Node) (*ModuleConfig, error) { + m := &ModuleConfig{} m.Name = name m.Node = n + // Handle Fn::ForEach modules + if strings.HasPrefix(name, ForEach) && n.Kind == yaml.SequenceNode { + if len(n.Content) != 3 { + msg := "expected %s len 3, got %d" + return nil, fmt.Errorf(msg, name, len(n.Content)) + } + + m.FnForEach = &FnForEach{} + + loopName := strings.Replace(name, ForEach, "", 1) + loopName = strings.Replace(loopName, ":", "", -1) + m.FnForEach.LoopName = loopName + + m.Name = loopName // TODO: ? + + m.FnForEach.Identifier = n.Content[0].Value + m.FnForEach.Collection = n.Content[1] + outputKeyValue := n.Content[2] + + if outputKeyValue.Kind != yaml.MappingNode || + len(outputKeyValue.Content) != 2 || + outputKeyValue.Content[1].Kind != yaml.MappingNode { + msg := "invalid %s, expected OutputKey: OutputValue mapping" + return nil, fmt.Errorf(msg, name) + } + + m.FnForEach.OutputKey = outputKeyValue.Content[0].Value + m.Node = outputKeyValue.Content[1] + m.FnForEach.OutputValue = m.Node + n = m.Node + m.Map = m.FnForEach.Collection + + config.Debugf("ModuleConfig.FnForEach: %+v", m.FnForEach) + + } + + if n.Kind != yaml.MappingNode { + config.Debugf("ParseModuleConfig %s: %s", name, node.ToSJson(n)) + return nil, errors.New("not a mapping node") + } + content := n.Content for i := 0; i < len(content); i += 2 { attr := content[i].Value @@ -90,30 +137,41 @@ func (t *Template) ParseModuleConfig(name string, n *yaml.Node) (*ModuleConfig, m.PropertiesNode = val case Overrides: m.OverridesNode = val - case Map: + case "ForEach": m.Map = val } } - //err := t.ValidateModuleConfig(m) - //if err != nil { - // return nil, err - //} - return m, nil } -// ValidateModuleConfig makes sure the configuration does not -// break any rules, such as not having a Property with the -// same name as a Parameter. -//func (t *Template) ValidateModuleConfig(moduleConfig *ModuleConfig) error { -// props := moduleConfig.Properties() -// for key := range props { -// _, err := t.GetParameter(key) -// if err == nil { -// return fmt.Errorf("module %s in %s has Property %s with the same name as a template Parameter", -// moduleConfig.Name, moduleConfig.ParentRootDir, key) -// } -// } -// return nil -//} +type FnForEach struct { + LoopName string + Identifier string + Collection *yaml.Node + OutputKey string + OutputValue *yaml.Node +} + +// OutputKeyHasIdentifier returns true if the key uses the identifier +func (ff *FnForEach) OutputKeyHasIdentifier() bool { + dollar := "${" + ff.Identifier + "}" + amper := "&{" + ff.Identifier + "}" + if strings.Contains(ff.OutputKey, dollar) { + return true + } + if strings.Contains(ff.OutputKey, amper) { + return true + } + return false +} + +// ReplaceIdentifier replaces instance of the identifier in s for collection +// key k +func ReplaceIdentifier(s, k, identifier string) string { + dollar := "${" + identifier + "}" + amper := "&{" + identifier + "}" + s = strings.Replace(s, dollar, k, -1) + s = strings.Replace(s, amper, k, -1) + return s +} diff --git a/cft/module_config_test.go b/cft/module_config_test.go new file mode 100644 index 000000000..8e8c55aed --- /dev/null +++ b/cft/module_config_test.go @@ -0,0 +1,365 @@ +package cft + +import ( + "testing" + + "gopkg.in/yaml.v3" +) + +// unmarshalYaml is a helper function that unmarshals a YAML string and returns +// the mapping node +func unmarshalYaml(t *testing.T, yamlStr string) *yaml.Node { + node := &yaml.Node{} + err := yaml.Unmarshal([]byte(yamlStr), node) + if err != nil { + t.Fatalf("Failed to unmarshal test YAML: %v", err) + } + return node.Content[0] +} + +func TestModuleConfigProperties(t *testing.T) { + // Create a test ModuleConfig with a PropertiesNode + propertiesYaml := ` +foo: bar +baz: + qux: quux +numbers: + - 1 + - 2 + - 3 +` + config := ModuleConfig{ + Name: "TestModule", + PropertiesNode: unmarshalYaml(t, propertiesYaml), + } + + // Test the Properties method + props := config.Properties() + + // Check that the properties were decoded correctly + if props["foo"] != "bar" { + t.Errorf("Expected foo=bar, got %v", props["foo"]) + } + + // Check nested property + bazMap, ok := props["baz"].(map[string]interface{}) + if !ok { + t.Errorf("Expected baz to be a map, got %T", props["baz"]) + } else if bazMap["qux"] != "quux" { + t.Errorf("Expected baz.qux=quux, got %v", bazMap["qux"]) + } + + // Check array property + numbers, ok := props["numbers"].([]interface{}) + if !ok { + t.Errorf("Expected numbers to be an array, got %T", props["numbers"]) + } else if len(numbers) != 3 { + t.Errorf("Expected numbers to have 3 elements, got %d", len(numbers)) + } +} + +func TestModuleConfigOverrides(t *testing.T) { + // Create a test ModuleConfig with an OverridesNode + overridesYaml := ` +Resource1: + Properties: + Name: overridden +Resource2: + Metadata: + Comment: added +` + config := ModuleConfig{ + Name: "TestModule", + OverridesNode: unmarshalYaml(t, overridesYaml), + } + + // Test the Overrides method + overrides := config.Overrides() + + // Check that the overrides were decoded correctly + resource1, ok := overrides["Resource1"].(map[string]interface{}) + if !ok { + t.Errorf("Expected Resource1 to be a map, got %T", + overrides["Resource1"]) + } else { + props, ok := resource1["Properties"].(map[string]interface{}) + if !ok { + t.Errorf("Expected Resource1.Properties to be a map, got %T", + resource1["Properties"]) + } else if props["Name"] != "overridden" { + t.Errorf("Expected Resource1.Properties.Name=overridden, got %v", + props["Name"]) + } + } +} + +func TestResourceOverridesNode(t *testing.T) { + // Create a test ModuleConfig with an OverridesNode + overridesYaml := ` +Resource1: + Properties: + Name: overridden +Resource2: + Metadata: + Comment: added +` + config := ModuleConfig{ + Name: "TestModule", + OverridesNode: unmarshalYaml(t, overridesYaml), + } + + // Test ResourceOverridesNode for an existing resource + resource1Node := config.ResourceOverridesNode("Resource1") + if resource1Node == nil { + t.Errorf("Expected to get a node for Resource1, got nil") + } + + // Test ResourceOverridesNode for a non-existent resource + nonExistentNode := config.ResourceOverridesNode("NonExistentResource") + if nonExistentNode != nil { + t.Errorf("Expected to get nil for NonExistentResource, got %v", + nonExistentNode) + } + + // Test ResourceOverridesNode when OverridesNode is nil + configWithNilOverrides := ModuleConfig{ + Name: "TestModule", + OverridesNode: nil, + } + nilResult := configWithNilOverrides.ResourceOverridesNode("Resource1") + if nilResult != nil { + t.Errorf("Expected nil when OverridesNode is nil, got %v", nilResult) + } +} + +func TestParseModuleConfig(t *testing.T) { + // Create a test Template + template := &Template{} + + // Test case 1: Basic module configuration + basicModuleYaml := ` +Source: ./module.yaml +Properties: + Name: test +Overrides: + Resource1: + Properties: + Name: overridden +` + basicConfig, err := template.ParseModuleConfig("BasicModule", + unmarshalYaml(t, basicModuleYaml)) + if err != nil { + t.Errorf("ParseModuleConfig failed: %v", err) + } + + if basicConfig.Name != "BasicModule" { + t.Errorf("Expected Name=BasicModule, got %s", basicConfig.Name) + } + if basicConfig.Source != "./module.yaml" { + t.Errorf("Expected Source=./module.yaml, got %s", basicConfig.Source) + } + if basicConfig.PropertiesNode == nil { + t.Errorf("Expected PropertiesNode to be non-nil") + } + if basicConfig.OverridesNode == nil { + t.Errorf("Expected OverridesNode to be non-nil") + } + + // Test case 2: Module with Map + mapModuleYaml := ` +Source: ./module.yaml +ForEach: !Ref MyList +Properties: + Name: test-${MapValue} +` + mapConfig, err := template.ParseModuleConfig("MapModule", + unmarshalYaml(t, mapModuleYaml)) + if err != nil { + t.Errorf("ParseModuleConfig failed: %v", err) + } + + if mapConfig.Name != "MapModule" { + t.Errorf("Expected Name=MapModule, got %s", mapConfig.Name) + } + if mapConfig.Map == nil { + t.Errorf("Expected Map to be non-nil") + } + + // Test case 3: Invalid node type + invalidNodeYaml := `- not a mapping node` + _, err = template.ParseModuleConfig("InvalidModule", + unmarshalYaml(t, invalidNodeYaml)) + if err == nil { + t.Errorf("Expected error for invalid node type, got nil") + } +} + +func TestParseModuleConfigWithFnForEach(t *testing.T) { + // Create a test Template + template := &Template{} + + // Test case: Fn::ForEach module configuration + forEachYaml := ` +- item +- !Ref MyList +- OutputKey: + Source: ./module.yaml + Properties: + Name: !Sub test-${item} +` + forEachConfig, err := template.ParseModuleConfig("Fn::ForEach:LoopModule", + unmarshalYaml(t, forEachYaml)) + if err != nil { + t.Errorf("ParseModuleConfig failed for Fn::ForEach: %v", err) + } + + if forEachConfig.FnForEach == nil { + t.Errorf("Expected FnForEach to be non-nil") + } + + if forEachConfig.FnForEach.LoopName != "LoopModule" { + t.Errorf("Expected LoopName=LoopModule, got %s", + forEachConfig.FnForEach.LoopName) + } + + if forEachConfig.FnForEach.Identifier != "item" { + t.Errorf("Expected Identifier=item, got %s", + forEachConfig.FnForEach.Identifier) + } + + if forEachConfig.FnForEach.OutputKey != "OutputKey" { + t.Errorf("Expected OutputKey=OutputKey, got %s", + forEachConfig.FnForEach.OutputKey) + } + + // Test invalid Fn::ForEach (wrong number of elements) + invalidForEachYaml := ` +- item +- !Ref MyList +` + _, err = template.ParseModuleConfig("Fn::ForEach:InvalidLoop", + unmarshalYaml(t, invalidForEachYaml)) + if err == nil { + t.Errorf("Expected error for invalid Fn::ForEach, got nil") + } + + // Test invalid Fn::ForEach (invalid output mapping) + invalidOutputYaml := ` +- item +- !Ref MyList +- InvalidOutput +` + _, err = template.ParseModuleConfig("Fn::ForEach:InvalidOutput", + unmarshalYaml(t, invalidOutputYaml)) + if err == nil { + t.Errorf("Expected error for invalid output mapping, got nil") + } +} + +func TestFnForEachOutputKeyHasIdentifier(t *testing.T) { + // Test cases for OutputKeyHasIdentifier + testCases := []struct { + name string + identifier string + outputKey string + expected bool + }{ + { + name: "Dollar syntax present", + identifier: "item", + outputKey: "test-${item}", + expected: true, + }, + { + name: "Ampersand syntax present", + identifier: "item", + outputKey: "test-&{item}", + expected: true, + }, + { + name: "No identifier", + identifier: "item", + outputKey: "test-static", + expected: false, + }, + { + name: "Different identifier", + identifier: "item", + outputKey: "${different}", + expected: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + fnForEach := FnForEach{ + Identifier: tc.identifier, + OutputKey: tc.outputKey, + } + + result := fnForEach.OutputKeyHasIdentifier() + if result != tc.expected { + t.Errorf("Expected OutputKeyHasIdentifier()=%v, got %v", + tc.expected, result) + } + }) + } +} + +func TestFnForEachReplaceIdentifier(t *testing.T) { + // Test cases for ReplaceIdentifier + testCases := []struct { + name string + identifier string + input string + key string + expected string + }{ + { + name: "Replace dollar syntax", + identifier: "item", + input: "test-${item}-suffix", + key: "value1", + expected: "test-value1-suffix", + }, + { + name: "Replace ampersand syntax", + identifier: "item", + input: "test-&{item}-suffix", + key: "value1", + expected: "test-value1-suffix", + }, + { + name: "Replace multiple occurrences", + identifier: "item", + input: "${item}-middle-${item}", + key: "value1", + expected: "value1-middle-value1", + }, + { + name: "No replacement needed", + identifier: "item", + input: "static-string", + key: "value1", + expected: "static-string", + }, + { + name: "Mixed syntax", + identifier: "item", + input: "${item}-and-&{item}", + key: "value1", + expected: "value1-and-value1", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + + result := ReplaceIdentifier(tc.input, tc.key, tc.identifier) + if result != tc.expected { + t.Errorf("Expected ReplaceIdentifier()=%q, got %q", + tc.expected, result) + } + }) + } +} diff --git a/cft/pkg/conditions.go b/cft/pkg/conditions.go index 6b099d1a1..6d661bcee 100644 --- a/cft/pkg/conditions.go +++ b/cft/pkg/conditions.go @@ -2,19 +2,26 @@ package pkg import ( "fmt" + "slices" "github.com/aws-cloudformation/rain/cft" - "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" ) -// ProcessConditions evaluates conditions in the module and -// removes Modules and Resources that should be omitted by -// a Condition that evaluates to false. It then looks for -// Fn::If function calls that reference the condition and -// resolves them, removing the false item. +type EvalResult string + +const ( + Equals EvalResult = "==" + NotEquals EvalResult = "!=" + UnResolved EvalResult = "?" +) + +// ProcessConditions evaluates conditions in the module and removes Modules and +// Resources that should be omitted by a Condition that evaluates to false. It +// then looks for Fn::If function calls that reference the condition and +// resolves them, removing false nodes. func (module *Module) ProcessConditions() error { // If there are no conditions in the module, nothing to do if module.ConditionsNode == nil { @@ -27,21 +34,40 @@ func (module *Module) ProcessConditions() error { return err } - // Create a dictionary of condition names to boolean values - conditionValues := make(map[string]bool) + // Initialize the module's ConditionValues map if it doesn't exist + if module.ConditionValues == nil { + module.ConditionValues = make(map[string]bool) + } - // Evaluate each condition in the Conditions section - conditions := module.Conditions() - for condName, condValue := range conditions { - // Evaluate the condition expression - result, err := evalCond(condName, condValue, conditions, module) + unResolved := make([]string, 0) + resolved := make([]string, 0) + + // Evaluate each condition in the Conditions section in the order they + // appear in the YAML. This ensures that conditions that depend on other + // conditions are evaluated after their dependencies + for i := 0; i < len(module.ConditionsNode.Content); i += 2 { + name := module.ConditionsNode.Content[i].Value + valNode := module.ConditionsNode.Content[i+1] + + result, err := module.EvalCond(name, valNode) if err != nil { return err } - conditionValues[condName] = result + isResolved := true + switch result { + case Equals: + module.ConditionValues[name] = true + case NotEquals: + module.ConditionValues[name] = false + case UnResolved: + unResolved = append(unResolved, name) + isResolved = false + } + if isResolved { + resolved = append(resolved, name) + } } - // Process both Resources and Modules sections sections := []struct { name string node *yaml.Node @@ -57,7 +83,7 @@ func (module *Module) ProcessConditions() error { } // Filter items based on conditions - // We need to collect items to remove first to avoid modifying while iterating + // Collect items to remove first to avoid modifying while iterating var itemsToRemove []string for i := 0; i < len(section.node.Content); i += 2 { @@ -67,21 +93,29 @@ func (module *Module) ProcessConditions() error { // Check if this item has a Condition attribute _, conditionNode, _ := s11n.GetMapValue(itemNode, Condition) if conditionNode != nil { - var conditionResult bool - - if conditionNode.Kind == yaml.ScalarNode { - conditionName := conditionNode.Value - conditionResult = conditionValues[conditionName] - } else { - return fmt.Errorf("invalid Condition: %s", node.YamlStr(itemNode)) + var condResult bool + if conditionNode.Kind != yaml.ScalarNode { + return fmt.Errorf("invalid Condition: %s", + node.YamlStr(itemNode)) } + condName := conditionNode.Value + condResult, ok := module.ConditionValues[condName] + if !ok { + // Is this a condition that we could not + // fully resolve, or a condition that doesn't exist? - if !conditionResult { + if slices.Contains(unResolved, condName) { + // Prepend the module name to the condition + newName := module.Config.Name + conditionNode.Value + conditionNode.Value = newName - itemsToRemove = append(itemsToRemove, itemName) + } + } else { + if !condResult { + itemsToRemove = append(itemsToRemove, itemName) + } + node.RemoveFromMap(itemNode, Condition) } - - node.RemoveFromMap(itemNode, Condition) } } @@ -89,144 +123,228 @@ func (module *Module) ProcessConditions() error { for _, itemName := range itemsToRemove { err := node.RemoveFromMap(section.node, itemName) if err != nil { - return fmt.Errorf("error removing %s from %s section: %v", itemName, section.name, err) + return fmt.Errorf("error removing %s from %s section: %v", + itemName, section.name, err) } } // Process Fn::If functions in the remaining items - _, err := processFnIf(section.node, conditionValues) + _, err := module.ProcessFnIf(section.node, unResolved) if err != nil { - return fmt.Errorf("error processing Fn::If in %s section: %v", section.name, err) + return fmt.Errorf("error processing Fn::If in %s section: %v", + section.name, err) } } - node.RemoveFromMap(module.Node, string(cft.Conditions)) - - return nil -} - -// evalCond evaluates a CloudFormation condition expression and returns its boolean value -func evalCond(condName string, condValue interface{}, conditions map[string]interface{}, module *Module) (bool, error) { - - config.Debugf("evalCond %s %s:\n%s", module.Config.Name, condName, node.EncodeMap(condValue)) - - // Handle condition node based on its type - switch v := condValue.(type) { - case map[string]interface{}: - // Check for condition functions: Fn::And, Fn::Or, Fn::Not, Fn::Equals, etc. - if and, ok := v["Fn::And"]; ok { - return evaluateAnd(and, conditions, module) + if len(unResolved) == 0 { + node.RemoveFromMap(module.Node, string(cft.Conditions)) + } else { + // Only remove those that we fully resolved. + // Emit the rest into the parent template. + for _, name := range resolved { + node.RemoveFromMap(module.ConditionsNode, name) } - if or, ok := v["Fn::Or"]; ok { - return evaluateOr(or, conditions, module) + + // Ensure that the parent template has a Conditions section + t := module.ParentTemplate + tc, err := t.GetSection(cft.Conditions) + if err != nil { + tc, _ = t.AddMapSection(cft.Conditions) } - if not, ok := v["Fn::Not"]; ok { - return evaluateNot(not, conditions, module) + + // Record the existing conditions in the parent + tcNames := make(map[string]*yaml.Node) + for i := 0; i < len(tc.Content); i += 2 { + tcNames[tc.Content[i].Value] = tc.Content[i+1] } - if equals, ok := v["Fn::Equals"]; ok { - return evaluateEquals(equals) + + for i, condNode := range module.ConditionsNode.Content { + if i%2 == 0 { + // Prepend the module name to the condition + name := module.Config.Name + condNode.Value + val := module.ConditionsNode.Content[i+1] + if existing, ok := tcNames[name]; ok { + if node.YamlStr(existing) != node.YamlStr(val) { + msg := "module %s has an unresolved Condition " + + "%s that conflicts with a Condition in the parent" + return fmt.Errorf(msg, module.Config.Source, name) + } + } else { + nameNode := node.MakeScalar(name) + node.Append(tc, nameNode) + cloned := node.Clone(val) + node.Append(tc, cloned) + } + } } - if condition, ok := v["Condition"]; ok { - // Reference to another condition - conditionName, ok := condition.(string) - if !ok { - return false, fmt.Errorf("condition reference must be a string: %v", condition) + + } + + return nil +} + +// EvalCond evaluates a condition expression and returns its boolean value +func (module *Module) EvalCond( + name string, val *yaml.Node) (EvalResult, error) { + + // Handle mapping node (most condition functions) + if val.Kind == yaml.MappingNode && len(val.Content) >= 2 { + key := val.Content[0].Value + valueNode := val.Content[1] + + switch key { + case "Fn::And": + return module.EvalAnd(valueNode) + case "Fn::Or": + return module.EvalOr(valueNode) + case "Fn::Not": + return module.EvalNot(valueNode) + case "Fn::Equals": + return module.EvalEquals(valueNode) + case "Condition": + if valueNode.Kind != yaml.ScalarNode { + msg := "condition reference must be a string: %s" + return UnResolved, fmt.Errorf(msg, node.YamlStr(valueNode)) } + conditionName := valueNode.Value // Check if we've already evaluated this condition - if result, exists := conditions[conditionName]; exists { - boolResult, ok := result.(bool) - if ok { - return boolResult, nil + if res, exists := module.ConditionValues[conditionName]; exists { + if res { + return Equals, nil + } else { + return NotEquals, nil } - // If not already evaluated as boolean, recursively evaluate it - return evalCond(conditionName, result, conditions, module) } - return false, fmt.Errorf("referenced condition '%s' not found", conditionName) + msg := "referenced condition '%s' not found" + return UnResolved, fmt.Errorf(msg, conditionName) } - case string: + } else if val.Kind == yaml.ScalarNode { // This might be a direct condition reference - if result, exists := conditions[v]; exists { - return evalCond("", result, conditions, module) + if res, exists := module.ConditionValues[val.Value]; exists { + if res { + return Equals, nil + } else { + return NotEquals, nil + } } } // Default to false if we can't evaluate the condition - return false, fmt.Errorf("unable to evaluate condition '%s' in %s: unsupported format: %v", - condName, module.Config.Name, condValue) + msg := "unable to evaluate condition '%s' in %s: unsupported format: %s" + return UnResolved, + fmt.Errorf(msg, name, module.Config.Name, node.YamlStr(val)) } -// evaluateAnd evaluates an Fn::And condition -func evaluateAnd(andExpr interface{}, conditions map[string]interface{}, module *Module) (bool, error) { - andList, ok := andExpr.([]interface{}) - if !ok { - return false, fmt.Errorf("Fn::And requires a list of conditions") +// EvalAnd evaluates an Fn::And condition +func (module *Module) EvalAnd(n *yaml.Node) (EvalResult, error) { + if n.Kind != yaml.SequenceNode { + msg := "Fn::And requires a list of conditions: %s" + return UnResolved, fmt.Errorf(msg, node.YamlStr(n)) } + hasUnResolved := false + // All conditions must be true for And to be true - for _, cond := range andList { - result, err := evalCond("", cond, conditions, module) + for _, val := range n.Content { + result, err := module.EvalCond("", val) if err != nil { - return false, err + return UnResolved, err + } + if result == NotEquals { + return NotEquals, nil } - if !result { - return false, nil // Short circuit on first false condition + if result == UnResolved { + hasUnResolved = true } } - return true, nil + if !hasUnResolved { + return Equals, nil + } + // If they're not all Equals, we can't be sure + return UnResolved, nil } -// evaluateOr evaluates an Fn::Or condition -func evaluateOr(orExpr interface{}, conditions map[string]interface{}, module *Module) (bool, error) { - orList, ok := orExpr.([]interface{}) - if !ok { - return false, fmt.Errorf("Fn::Or requires a list of conditions") +// EvalOr evaluates an Fn::Or condition +func (module *Module) EvalOr(n *yaml.Node) (EvalResult, error) { + if n.Kind != yaml.SequenceNode { + msg := "Fn::Or requires a list of conditions: %s" + return UnResolved, fmt.Errorf(msg, node.YamlStr(n)) } + hasUnResolved := false + // Any condition being true makes Or true - for _, cond := range orList { - result, err := evalCond("", cond, conditions, module) + for _, val := range n.Content { + result, err := module.EvalCond("", val) if err != nil { - return false, err + return UnResolved, err } - if result { - return true, nil // Short circuit on first true condition + if result == Equals { + return Equals, nil } } - return false, nil + + if hasUnResolved { + return UnResolved, nil + } + + // All conditions are NotEquals + return NotEquals, nil } -// evaluateNot evaluates an Fn::Not condition -func evaluateNot(notExpr interface{}, conditions map[string]interface{}, module *Module) (bool, error) { - notList, ok := notExpr.([]interface{}) - if !ok || len(notList) != 1 { - return false, fmt.Errorf("Fn::Not requires exactly one condition") +// EvalNot evaluates an Fn::Not condition +func (module *Module) EvalNot(n *yaml.Node) (EvalResult, error) { + if n.Kind != yaml.SequenceNode || len(n.Content) != 1 { + msg := "Fn::Not requires exactly one condition: %s" + return UnResolved, fmt.Errorf(msg, node.YamlStr(n)) } - result, err := evalCond("", notList[0], conditions, module) + result, err := module.EvalCond("", n.Content[0]) if err != nil { - return false, err + return UnResolved, err + } + if result == Equals { + return NotEquals, nil + } else if result == NotEquals { + return Equals, nil + } else { + return result, nil } - return !result, nil } -// evaluateEquals evaluates an Fn::Equals condition -func evaluateEquals(equalsExpr interface{}) (bool, error) { - equalsList, ok := equalsExpr.([]interface{}) - if !ok || len(equalsList) != 2 { - return false, fmt.Errorf("Fn::Equals requires exactly two values") +// EvalEquals evaluates an Fn::Equals condition +func (module *Module) EvalEquals(n *yaml.Node) (EvalResult, error) { + if n.Kind != yaml.SequenceNode || len(n.Content) != 2 { + msg := "Fn::Equals requires exactly two values: %s" + return UnResolved, fmt.Errorf(msg, node.YamlStr(n)) } - // Resolve parameter references if needed - val1 := equalsList[0] - val2 := equalsList[1] + val1 := n.Content[0] + val2 := n.Content[1] + + val1Str := node.YamlStr(val1) + val2Str := node.YamlStr(val2) - // Compare the values - return val1 == val2, nil + if val1Str == val2Str { + return Equals, nil + } + + if val1.Kind == yaml.ScalarNode && val2.Kind == yaml.ScalarNode { + if val1.Value == val2.Value { + return Equals, nil + } else { + return NotEquals, nil + } + } + + return UnResolved, nil } -// processFnIf processes Fn::If functions in a node and its children +// ProcessFnIf processes Fn::If functions in a node and its children // Returns true if the node should be removed from its parent -func processFnIf(n *yaml.Node, conditionValues map[string]bool) (bool, error) { +func (module *Module) ProcessFnIf(n *yaml.Node, + unResolved []string) (bool, error) { + if n == nil { return false, nil } @@ -237,27 +355,31 @@ func processFnIf(n *yaml.Node, conditionValues map[string]bool) (bool, error) { if len(n.Content) >= 2 && n.Content[0].Value == "Fn::If" { value := n.Content[1] if value.Kind == yaml.SequenceNode && len(value.Content) == 3 { - condName := value.Content[0].Value - if condVal, exists := conditionValues[condName]; exists { + name := value.Content[0].Value + if condVal, exists := module.ConditionValues[name]; exists { // Get the appropriate value based on condition var replacement *yaml.Node if condVal { - replacement = node.Clone(value.Content[1]) // true value + replacement = node.Clone(value.Content[1]) } else { - replacement = node.Clone(value.Content[2]) // false value + replacement = node.Clone(value.Content[2]) } // Check if this is AWS::NoValue - if replacement.Kind == yaml.MappingNode && len(replacement.Content) >= 2 { + if replacement.Kind == yaml.MappingNode && + len(replacement.Content) >= 2 { if replacement.Content[0].Value == "Ref" && replacement.Content[1].Value == "AWS::NoValue" { - return true, nil // Signal that this node should be removed + return true, nil } } // Replace the entire node with the replacement *n = *replacement return false, nil + } else if slices.Contains(unResolved, name) { + newName := module.Config.Name + name + value.Content[0].Value = newName } } } @@ -273,7 +395,7 @@ func processFnIf(n *yaml.Node, conditionValues map[string]bool) (bool, error) { value := n.Content[i+1] // Process the value recursively - shouldRemove, err := processFnIf(value, conditionValues) + shouldRemove, err := module.ProcessFnIf(value, unResolved) if err != nil { return false, err } @@ -296,7 +418,7 @@ func processFnIf(n *yaml.Node, conditionValues map[string]bool) (bool, error) { // Process each item in the sequence i := 0 for i < len(n.Content) { - shouldRemove, err := processFnIf(n.Content[i], conditionValues) + shouldRemove, err := module.ProcessFnIf(n.Content[i], unResolved) if err != nil { return false, err } diff --git a/cft/pkg/conditions_test.go b/cft/pkg/conditions_test.go index 8c835914d..e4ae4a5e8 100644 --- a/cft/pkg/conditions_test.go +++ b/cft/pkg/conditions_test.go @@ -288,7 +288,7 @@ Resources: Type: AWS::S3::Bucket Condition: Test `, - wantErr: "Fn::Equals requires exactly two values", + wantErr: "Fn::Equals requires exactly two values: - only one value", }, { name: "invalid and condition", @@ -301,7 +301,7 @@ Resources: Type: AWS::S3::Bucket Condition: Test `, - wantErr: "Fn::And requires a list of conditions", + wantErr: "Fn::And requires a list of conditions: true", }, } diff --git a/cft/pkg/content.go b/cft/pkg/content.go index 2f827472c..0a5558e15 100644 --- a/cft/pkg/content.go +++ b/cft/pkg/content.go @@ -11,6 +11,8 @@ import ( "github.com/aws-cloudformation/rain/internal/config" ) +var contentCache map[string]*ModuleContent + type ModuleContent struct { Content []byte NewRootDir string @@ -42,7 +44,15 @@ func getModuleContent( baseUri string, uri string) (*ModuleContent, error) { - config.Debugf("getModuleContent root: %s, uri: %s", root, uri) + config.Debugf("getModuleContent root: %s, baseUri: %s, uri: %s", + root, baseUri, uri) + + cacheKey := fmt.Sprintf("%s/%s/%s", root, baseUri, uri) + + if cached, ok := contentCache[cacheKey]; ok { + config.Debugf("getModuleContent cache hit: %s", cacheKey) + return cached, nil + } var content []byte var err error @@ -83,16 +93,16 @@ func getModuleContent( // getModuleContent: root=cft/pkg/tmpl/awscli-modules, baseUri=, uri=package.zip/zip-module.yaml if strings.Contains(uri, ".zip/") { isZip = true - + // Extract the zip location and path within the zip zipIndex := strings.Index(uri, ".zip/") if zipIndex > 0 { - zipLocation := uri[:zipIndex+4] // Include the .zip part - zipPath := uri[zipIndex+5:] // Skip the .zip/ part - + zipLocation := uri[:zipIndex+4] // Include the .zip part + zipPath := uri[zipIndex+5:] // Skip the .zip/ part + zipLocation = resolveZipLocation(root, zipLocation) config.Debugf("Extracting from zip: %s, path: %s", zipLocation, zipPath) - + // Use DownloadFromZip directly - it can handle S3, HTTPS, and local files content, err = DownloadFromZip(zipLocation, "", zipPath) if err != nil { @@ -183,5 +193,11 @@ func getModuleContent( } } - return &ModuleContent{content, newRootDir, baseUri}, nil + retval := &ModuleContent{content, newRootDir, baseUri} + contentCache[cacheKey] = retval + return retval, nil +} + +func init() { + contentCache = make(map[string]*ModuleContent) } diff --git a/cft/pkg/intrinsics.go b/cft/pkg/intrinsics.go index e4d877b9f..a654ee204 100644 --- a/cft/pkg/intrinsics.go +++ b/cft/pkg/intrinsics.go @@ -16,9 +16,11 @@ import ( // FnJoin converts to a scalar if the join can be fully resolved func FnJoin(n *yaml.Node) error { + var err error vf := func(v *visitor.Visitor) { vn := v.GetYamlNode() + if vn.Kind != yaml.MappingNode { return } diff --git a/cft/pkg/maps.go b/cft/pkg/maps.go index cfa170b4d..a0fe1d2e1 100644 --- a/cft/pkg/maps.go +++ b/cft/pkg/maps.go @@ -1,3 +1,4 @@ +// ForEach (aka Map) processing for modules package pkg import ( @@ -13,18 +14,27 @@ import ( ) const ( - MI = "$MapIndex" - MV = "$MapValue" + MI = "$Index" + MV = "$Identifier" ) -func replaceMapStr(s string, index int, key string) string { +func replaceMapStr(s string, index int, key string, identifier string) string { + s = strings.Replace(s, "${"+MI+"}", fmt.Sprintf("%d", index), -1) + s = strings.Replace(s, "&{"+MI+"}", fmt.Sprintf("%d", index), -1) s = strings.Replace(s, MI, fmt.Sprintf("%d", index), -1) + s = strings.Replace(s, "${"+MV+"}", key, -1) + s = strings.Replace(s, "&{"+MV+"}", key, -1) s = strings.Replace(s, MV, key, -1) + + if identifier != "" { + s = cft.ReplaceIdentifier(s, key, identifier) + } + return s } -// mapPlaceholders looks for MapValue and MapIndex and replaces them -func mapPlaceholders(n *yaml.Node, index int, key string) { +// mapPlaceholders looks for Index and Identifier and replaces them +func mapPlaceholders(n *yaml.Node, index int, key string, identifier string) { vf := func(v *visitor.Visitor) { yamlNode := v.GetYamlNode() @@ -33,7 +43,8 @@ func mapPlaceholders(n *yaml.Node, index int, key string) { if len(content) == 2 { switch content[0].Value { case string(cft.Sub): - r := replaceMapStr(content[1].Value, index, key) + r := replaceMapStr(content[1].Value, + index, key, identifier) if parse.IsSubNeeded(r) { yamlNode.Value = r } else { @@ -41,25 +52,89 @@ func mapPlaceholders(n *yaml.Node, index int, key string) { } case string(cft.GetAtt): for _, getatt := range content[1].Content { - getatt.Value = replaceMapStr(getatt.Value, index, key) + getatt.Value = replaceMapStr(getatt.Value, index, key, + identifier) } } } } else if yamlNode.Kind == yaml.ScalarNode { - yamlNode.Value = replaceMapStr(yamlNode.Value, index, key) + yamlNode.Value = replaceMapStr(yamlNode.Value, index, key, + identifier) } } visitor.NewVisitor(n).Visit(vf) } -// processMaps duplicates module configuration in the template for each value in a CSV -func processMaps(originalContent []*yaml.Node, t *cft.Template, parentModule *Module) ([]*yaml.Node, error) { +// getMapKeys gets the CSV key values from either a hard-coded +// string or from a Ref. +func getMapKeys(moduleConfig *cft.ModuleConfig, t *cft.Template, + parentModule *Module) ([]string, error) { + + // The map is either a CSV or a Ref to a CSV that we can fully + // resolve + mapJson := node.ToSJson(moduleConfig.Map) + var keys []string + if moduleConfig.Map.Kind == yaml.ScalarNode { + keys = node.StringsFromNode(moduleConfig.Map) + } else if moduleConfig.Map.Kind == yaml.SequenceNode { + keys = node.StringsFromNode(moduleConfig.Map) + } else if moduleConfig.Map.Kind == yaml.MappingNode { + if cft.IsRef(moduleConfig.Map) { + r := moduleConfig.Map.Content[1].Value + // Look in the parent templates Parameters for the Ref + if !t.HasSection(cft.Parameters) { + msg := "module Map Ref no Parameters: %s" + return nil, fmt.Errorf(msg, mapJson) + } + params, _ := t.GetSection(cft.Parameters) + _, keysNode, _ := s11n.GetMapValue(params, r) + if keysNode == nil { + msg := "expected module Map Ref to a Parameter: %s" + return nil, fmt.Errorf(msg, mapJson) + } - content := make([]*yaml.Node, 0) + // Look at the parent module Properties + // TODO: Will this work in nested modules? + // Have those props been resolved? + if parentModule != nil { + if parentVal, ok := parentModule.Config.Properties()[r]; ok { + csv, ok := parentVal.(string) + if ok { + keys = strings.Split(csv, ",") + } else { + msg := "expected Map keys to be a CSV: %v" + return nil, fmt.Errorf(msg, parentVal) + } + } + } - // This will hold info about original mapped modules - moduleMaps := make(map[string]any) + if len(keys) == 0 { + _, d, _ := s11n.GetMapValue(keysNode, "Default") + if d == nil { + msg := "expected module Map Ref to a Default: %s" + return nil, fmt.Errorf(msg, mapJson) + } + keys = node.StringsFromNode(d) + } + } else { + msg := "expected module Map to be a Ref: %s" + return nil, fmt.Errorf(msg, mapJson) + } + } else { + return nil, fmt.Errorf("unexpected module Map Kind: %s", mapJson) + } + + return keys, nil +} + +// processMaps duplicates module configuration in the template for +// each value in a CSV. The external name for this is now "ForEach", +// but originally it was called "Map". +func processMaps(originalContent []*yaml.Node, + t *cft.Template, parentModule *Module) ([]*yaml.Node, error) { + + content := make([]*yaml.Node, 0) // Process Maps, which duplicate the module for each element in a list for i := 0; i < len(originalContent); i += 2 { @@ -69,90 +144,64 @@ func processMaps(originalContent []*yaml.Node, t *cft.Template, parentModule *Mo return nil, err } - if moduleConfig.Map != nil { - // The map is either a CSV or a Ref to a CSV that we can fully resolve - mapJson := node.ToSJson(moduleConfig.Map) - var keys []string - if moduleConfig.Map.Kind == yaml.ScalarNode { - keys = node.StringsFromNode(moduleConfig.Map) - } else if moduleConfig.Map.Kind == yaml.SequenceNode { - keys = node.StringsFromNode(moduleConfig.Map) - } else if moduleConfig.Map.Kind == yaml.MappingNode { - if len(moduleConfig.Map.Content) == 2 && moduleConfig.Map.Content[0].Value == "Ref" { - r := moduleConfig.Map.Content[1].Value - // Look in the parent templates Parameters for the Ref - if !t.HasSection(cft.Parameters) { - return nil, fmt.Errorf("module Map Ref no Parameters: %s", mapJson) - } - params, _ := t.GetSection(cft.Parameters) - _, keysNode, _ := s11n.GetMapValue(params, r) - if keysNode == nil { - return nil, fmt.Errorf("expected module Map Ref to a Parameter: %s", mapJson) - } + if moduleConfig.Map == nil { + content = append(content, originalContent[i]) + content = append(content, originalContent[i+1]) + continue + } - // Look at the parent module Properties - // TODO: Will this work in nested modules? Have those props been resolved? - if parentModule != nil { - if parentPropVal, ok := parentModule.Config.Properties()[r]; ok { - csv, ok := parentPropVal.(string) - if ok { - keys = strings.Split(csv, ",") - } else { - return nil, fmt.Errorf("expected Map keys to be a CSV: %v", parentPropVal) - } - } - } + keys, err := getMapKeys(moduleConfig, t, parentModule) + if err != nil { + return nil, err + } - if len(keys) == 0 { - _, d, _ := s11n.GetMapValue(keysNode, "Default") - if d == nil { - return nil, fmt.Errorf("expected module Map Ref to a Default: %s", mapJson) - } - keys = node.StringsFromNode(d) - } - } else { - return nil, fmt.Errorf("expected module Map to be a Ref: %s", mapJson) - } - } else { - return nil, fmt.Errorf("unexpected module Map Kind: %s", mapJson) + if len(keys) < 1 { + msg := "expected module Map to have items: %s" + mapErr := fmt.Errorf(msg, node.YamlStr(moduleConfig.Node)) + return nil, mapErr + } + + // Duplicate the config + for i, key := range keys { + mapName := fmt.Sprintf("%s%d", moduleConfig.Name, i) + + if moduleConfig.FnForEach != nil && + moduleConfig.FnForEach.OutputKeyHasIdentifier() { + // The OutputKey is something like A${Identifier}, + // which means we use the key instead of the + // array index to create the logical id + mapName = fmt.Sprintf("%s%s", moduleConfig.Name, key) } - if len(keys) < 1 { - mapErr := fmt.Errorf("expected module Map to have items: %s", mapJson) - return nil, mapErr + copiedNode := node.Clone(moduleConfig.Node) + node.RemoveFromMap(copiedNode, ForEach) + copiedConfig, err := t.ParseModuleConfig(mapName, copiedNode) + if err != nil { + return nil, err } - // Record the number of items in the map - moduleMaps[moduleConfig.Name] = len(keys) - - // Duplicate the config - for i, key := range keys { - mapName := fmt.Sprintf("%s%d", moduleConfig.Name, i) - copiedNode := node.Clone(moduleConfig.Node) - node.RemoveFromMap(copiedNode, "Map") - copiedConfig, err := t.ParseModuleConfig(mapName, copiedNode) - if err != nil { - return nil, err - } + // These values won't go into the YAML but we'll store them for + // later - // These values won't go into the YAML but we'll store them for later - copiedConfig.OriginalName = moduleConfig.Name - copiedConfig.IsMapCopy = true - copiedConfig.MapIndex = i - copiedConfig.MapKey = key + copiedConfig.OriginalName = moduleConfig.Name + copiedConfig.IsMapCopy = true + copiedConfig.MapIndex = i + copiedConfig.MapKey = key - // Add a reference to the template so we can find it later for Outputs - t.AddMappedModule(copiedConfig) + // Add a reference to the template so we can find it later for + // Outputs - // Replace $MapIndex and $MapValue - mapPlaceholders(copiedNode, i, key) + t.AddMappedModule(copiedConfig) - content = append(content, node.MakeScalar(mapName)) - content = append(content, copiedNode) + // Replace $Index and $Identifier + identifier := "" + if moduleConfig.FnForEach != nil { + identifier = moduleConfig.FnForEach.Identifier } - } else { - content = append(content, originalContent[i]) - content = append(content, originalContent[i+1]) + mapPlaceholders(copiedNode, i, key, identifier) + + content = append(content, node.MakeScalar(mapName)) + content = append(content, copiedNode) } } return content, nil diff --git a/cft/pkg/module.go b/cft/pkg/module.go index dc3acc1d9..9735c2e57 100644 --- a/cft/pkg/module.go +++ b/cft/pkg/module.go @@ -31,21 +31,22 @@ const ( Condition = "Condition" Default = "Default" Source = "Source" - Map = "Map" + ForEach = "ForEach" ) // Module represents a complete module, including parent config type Module struct { - Config *cft.ModuleConfig - ParametersNode *yaml.Node - ResourcesNode *yaml.Node - OutputsNode *yaml.Node - Node *yaml.Node - ParentTemplate *cft.Template - ConditionsNode *yaml.Node - ModulesNode *yaml.Node - Parsed *ParsedModule - ParentModule *Module + Config *cft.ModuleConfig + ParametersNode *yaml.Node + ResourcesNode *yaml.Node + OutputsNode *yaml.Node + Node *yaml.Node + ParentTemplate *cft.Template + ConditionsNode *yaml.Node + ConditionValues map[string]bool + ModulesNode *yaml.Node + Parsed *ParsedModule + ParentModule *Module } // Outputs returns the Outputs node as a map @@ -93,7 +94,7 @@ func processModulesSection(t *cft.Template, n *yaml.Node, originalContent := moduleSection.Content - // Duplicate module content that has a Map attribute + // Duplicate module content that has a ForEach attribute content, err := processMaps(originalContent, t, parentModule) if err != nil { return err @@ -216,17 +217,45 @@ func processModule( }, } + transforms, err := moduleAsTemplate.GetSection(cft.Transform) + if err == nil { + // The module has Transforms + parentTransforms, err := t.GetSection(cft.Transform) + if err != nil { + parentTransforms, _ = t.AddMapSection(cft.Transform) + } + merged := node.MakeSequence([]string{}) + both := []*yaml.Node{transforms, parentTransforms} + for _, tr := range both { + if tr.Kind == yaml.ScalarNode { + merged.Content = append(merged.Content, tr) + } else { + merged.Content = append(merged.Content, tr.Content...) + } + } + merged = node.Clone(merged) + *parentTransforms = *merged + } + err = processRainSection(moduleAsTemplate) if err != nil { return err } - err = processAddedSections(moduleAsTemplate, moduleAsTemplate.Node.Content[0], + // processAddedSections is where we recurse on sub-modules + err = processAddedSections(moduleAsTemplate, + moduleAsTemplate.Node.Content[0], parsedModule.RootDir, parsedModule.FS, m) if err != nil { return err } + // Process conditions again to emit sub-module conditions into the parent + err = m.ProcessConditions() + if err != nil { + return err + } + err = m.ValidateOverrides() if err != nil { return err @@ -243,12 +272,6 @@ func processModule( return err } - // Resolve any references to this module in the parent template - //err = m.Resolve(t.Node) - //if err != nil { - // return err - //} - fileRootDir := "" if parentModule != nil { fileRootDir = parentModule.Parsed.RootDir diff --git a/cft/pkg/module_test.go b/cft/pkg/module_test.go index 343be7552..4cde446c0 100644 --- a/cft/pkg/module_test.go +++ b/cft/pkg/module_test.go @@ -204,6 +204,20 @@ func TestAWSCLIModuleSameMod(t *testing.T) { runTest("awscli-modules/same-mod", t) } +// TestAWSCLIModuleCondUnRes tests the cond-unres AWS CLI module functionality +func TestAWSCLIModuleCondUnRes(t *testing.T) { + runTest("awscli-modules/cond-unres", t) +} + +// TestAWSCLIModuleCondConflict test a Condition conflict +func TestAWSCLIModuleCondConflict(t *testing.T) { + runFailTest("awscli-modules/cond-conflict", t) +} + +func TestAWSCLIModuleForEach(t *testing.T) { + runTest("awscli-modules/foreach", t) +} + func runTest(test string, t *testing.T) { // There should be 3 files for each test, for example: diff --git a/cft/pkg/outputs.go b/cft/pkg/outputs.go index 4f9b313e8..60a99979b 100644 --- a/cft/pkg/outputs.go +++ b/cft/pkg/outputs.go @@ -9,6 +9,7 @@ import ( "github.com/aws-cloudformation/rain/cft" "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" "github.com/aws-cloudformation/rain/internal/s11n" "gopkg.in/yaml.v3" @@ -90,7 +91,7 @@ func (module *Module) ProcessOutputs() error { // GetArrayIndexFromString extracts an integer array index from a string with // embedded brackets. For example, from "Content[1].Arn" it would return 1. // Returns an error if no valid index is found. -func GetArrayIndexFromString(s string) (int, error) { +func (module *Module) GetArrayIndexFromString(s string) (int, error) { // Find the opening bracket position start := strings.Index(s, "[") if start == -1 { @@ -109,6 +110,16 @@ func GetArrayIndexFromString(s string) (int, error) { // Convert to integer num, err := strconv.Atoi(numStr) if err != nil { + // This might be a map key instead of an index + maps := module.ParentTemplate.ModuleMaps + if maps != nil { + config.Debugf("maps: %v", maps) + name := s[0:start] + numStr + config.Debugf("Looking for ModuleMaps[%s]", name) + if cfg, ok := maps[name]; ok { + return cfg.MapIndex, nil + } + } return 0, fmt.Errorf("invalid array index in string %s: %w", s, err) } @@ -141,7 +152,7 @@ func (module *Module) CheckOutputGetAtt(s string, outputName string, outputVal a if mappedConfig, ok := module.ParentTemplate.ModuleMaps[module.Config.Name]; ok { fixedName := strings.Split(reffedModuleName, "[")[0] if mappedConfig.OriginalName == fixedName && tokens[1] == outputName { - idx, err := GetArrayIndexFromString(reffedModuleName) + idx, err := module.GetArrayIndexFromString(reffedModuleName) if err != nil { return nil, err } diff --git a/cft/pkg/outputs_test.go b/cft/pkg/outputs_test.go index 343e0f8fc..5469795bf 100644 --- a/cft/pkg/outputs_test.go +++ b/cft/pkg/outputs_test.go @@ -2,6 +2,8 @@ package pkg import ( "testing" + + "github.com/aws-cloudformation/rain/cft" ) func TestGetArrayIndexFromString(t *testing.T) { @@ -69,18 +71,20 @@ func TestGetArrayIndexFromString(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result, err := GetArrayIndexFromString(tt.input) - + m := &Module{} + m.ParentTemplate = &cft.Template{} + result, err := m.GetArrayIndexFromString(tt.input) + // Check error status if (err != nil) != tt.hasError { - t.Errorf("GetArrayIndexFromString(%q) error = %v, wantErr %v", + t.Errorf("GetArrayIndexFromString(%q) error = %v, wantErr %v", tt.input, err, tt.hasError) return } - + // If we expect no error, check the result value if !tt.hasError && result != tt.expected { - t.Errorf("GetArrayIndexFromString(%q) = %d, want %d", + t.Errorf("GetArrayIndexFromString(%q) = %d, want %d", tt.input, result, tt.expected) } }) diff --git a/cft/pkg/tmpl/awscli-modules/cond-conflict-template.yaml b/cft/pkg/tmpl/awscli-modules/cond-conflict-template.yaml new file mode 100644 index 000000000..a79ddd2be --- /dev/null +++ b/cft/pkg/tmpl/awscli-modules/cond-conflict-template.yaml @@ -0,0 +1,9 @@ +Conditions: + FooIsRegionUsEast1: + Fn::Equals: + - NotTheSameThing + - us-east-1 + +Modules: + Foo: + Source: cond-unres-module.yaml diff --git a/cft/pkg/tmpl/awscli-modules/cond-unres-expect.yaml b/cft/pkg/tmpl/awscli-modules/cond-unres-expect.yaml new file mode 100644 index 000000000..aec479897 --- /dev/null +++ b/cft/pkg/tmpl/awscli-modules/cond-unres-expect.yaml @@ -0,0 +1,35 @@ +Parameters: + P: + Type: String +Conditions: + FooIsRegionUsEast1: + Fn::Equals: + - Ref: AWS::Region + - us-east-1 + FooIsPTrue: + Fn::Equals: + - !Ref P + - true + FooSubIsParam: + Fn::Equals: + - !Ref P + - true + +Resources: + FooS3Bucket: + Type: AWS::S3::Bucket + Condition: FooIsRegionUsEast1 + FooA: + Type: A::B::C + Properties: + Name: + Fn::If: + - FooIsRegionUsEast1 + - a-east + - a-not-east + FooSubB: + Type: A::B::C + Condition: FooSubIsParam + FooC: + Type: D::E::F + Condition: FooIsPTrue diff --git a/cft/pkg/tmpl/awscli-modules/cond-unres-module.yaml b/cft/pkg/tmpl/awscli-modules/cond-unres-module.yaml new file mode 100644 index 000000000..f11d02cee --- /dev/null +++ b/cft/pkg/tmpl/awscli-modules/cond-unres-module.yaml @@ -0,0 +1,35 @@ +Parameters: + P: + Type: String + +Conditions: + IsRegionUsEast1: + Fn::Equals: + - Ref: AWS::Region + - us-east-1 + IsPTrue: + Fn::Equals: + - !Ref P + - true + +Modules: + Sub: + Source: cond-unres-submodule.yaml + Properties: + P: !Ref P + +Resources: + S3Bucket: + Type: AWS::S3::Bucket + Condition: IsRegionUsEast1 + A: + Type: A::B::C + Properties: + Name: + Fn::If: + - IsRegionUsEast1 + - a-east + - a-not-east + C: + Type: D::E::F + Condition: IsPTrue diff --git a/cft/pkg/tmpl/awscli-modules/cond-unres-submodule.yaml b/cft/pkg/tmpl/awscli-modules/cond-unres-submodule.yaml new file mode 100644 index 000000000..3c713a0c3 --- /dev/null +++ b/cft/pkg/tmpl/awscli-modules/cond-unres-submodule.yaml @@ -0,0 +1,14 @@ +Parameters: + P: + Type: String + +Conditions: + IsParam: + Fn::Equals: + - !Ref P + - true + +Resources: + B: + Condition: IsParam + Type: A::B::C diff --git a/cft/pkg/tmpl/awscli-modules/cond-unres-template.yaml b/cft/pkg/tmpl/awscli-modules/cond-unres-template.yaml new file mode 100644 index 000000000..a73ebf4d5 --- /dev/null +++ b/cft/pkg/tmpl/awscli-modules/cond-unres-template.yaml @@ -0,0 +1,9 @@ +Parameters: + P: + Type: String + +Modules: + Foo: + Source: cond-unres-module.yaml + Properties: + P: !Ref P diff --git a/cft/pkg/tmpl/awscli-modules/conditional-module.yaml b/cft/pkg/tmpl/awscli-modules/conditional-module.yaml index 987332819..c0299d868 100644 --- a/cft/pkg/tmpl/awscli-modules/conditional-module.yaml +++ b/cft/pkg/tmpl/awscli-modules/conditional-module.yaml @@ -5,15 +5,17 @@ Parameters: LambdaRoleName: Type: String - Description: If set, allow the lambda function to access this table Default: "" + HasLambda: + Type: Boolean + Default: false + Conditions: IfLambdaRoleIsSet: - Fn::Not: - - Fn::Equals: - - !Ref LambdaRoleName - - "" + Fn::Equals: + - !Ref HasLambda + - true Nested: Fn::And: diff --git a/cft/pkg/tmpl/awscli-modules/conditional-template.yaml b/cft/pkg/tmpl/awscli-modules/conditional-template.yaml index 3599ca93b..86088560b 100644 --- a/cft/pkg/tmpl/awscli-modules/conditional-template.yaml +++ b/cft/pkg/tmpl/awscli-modules/conditional-template.yaml @@ -4,6 +4,7 @@ Modules: Properties: TableName: table-a LambdaRoleName: my-role + HasLambda: true B: Source: ./conditional-module.yaml Properties: diff --git a/cft/pkg/tmpl/awscli-modules/foreach-expect.yaml b/cft/pkg/tmpl/awscli-modules/foreach-expect.yaml new file mode 100644 index 000000000..dc65143e8 --- /dev/null +++ b/cft/pkg/tmpl/awscli-modules/foreach-expect.yaml @@ -0,0 +1,30 @@ +Resources: + A0Foo: + Type: A::B::C + Properties: + Name: a-A + A1Foo: + Type: A::B::C + Properties: + Name: a-B + A2Foo: + Type: A::B::C + Properties: + Name: a-C + BAFoo: + Type: A::B::C + Properties: + Name: b-A + BBFoo: + Type: A::B::C + Properties: + Name: b-B + BCFoo: + Type: A::B::C + Properties: + Name: b-C +Outputs: + TestGetAttA: + Value: !GetAtt A0Foo.Arn + TestGetAttB: + Value: !GetAtt BBFoo.Arn diff --git a/cft/pkg/tmpl/awscli-modules/foreach-module.yaml b/cft/pkg/tmpl/awscli-modules/foreach-module.yaml new file mode 100644 index 000000000..2568077c5 --- /dev/null +++ b/cft/pkg/tmpl/awscli-modules/foreach-module.yaml @@ -0,0 +1,11 @@ +Parameters: + Name: + Type: String +Resources: + Foo: + Type: A::B::C + Properties: + Name: !Ref Name +Outputs: + Out1: + Value: !GetAtt Foo.Arn diff --git a/cft/pkg/tmpl/awscli-modules/foreach-template.yaml b/cft/pkg/tmpl/awscli-modules/foreach-template.yaml new file mode 100644 index 000000000..016254dbb --- /dev/null +++ b/cft/pkg/tmpl/awscli-modules/foreach-template.yaml @@ -0,0 +1,23 @@ +Modules: + Fn::ForEach::A: + - TheKey + - A,B,C + - A: + Source: foreach-module.yaml + Properties: + Name: a-${TheKey} + Fn::ForEach::B: + - TheKey + - A,B,C + - B&{TheKey}: + Source: foreach-module.yaml + Properties: + Name: b-${TheKey} +Outputs: + TestGetAttA: + Value: !GetAtt A[0].Out1 + TestGetAttB: + Value: !GetAtt B[B].Out1 + + + diff --git a/cft/pkg/tmpl/awscli-modules/map-expect.yaml b/cft/pkg/tmpl/awscli-modules/map-expect.yaml index a3b27c6db..627a9932f 100644 --- a/cft/pkg/tmpl/awscli-modules/map-expect.yaml +++ b/cft/pkg/tmpl/awscli-modules/map-expect.yaml @@ -2,6 +2,11 @@ Parameters: List: Type: CommaDelimitedList Default: A,B,C +Outputs: + TestSubArray: + Value: !Sub ${Content0Bucket.Arn} + TestUsingKey: + Value: !Sub ${Content0Bucket.Arn} Resources: Content0Bucket: Type: AWS::S3::Bucket diff --git a/cft/pkg/tmpl/awscli-modules/map-output-module.yaml b/cft/pkg/tmpl/awscli-modules/map-output-module.yaml index 6b8a91068..00f69e473 100644 --- a/cft/pkg/tmpl/awscli-modules/map-output-module.yaml +++ b/cft/pkg/tmpl/awscli-modules/map-output-module.yaml @@ -1,9 +1,9 @@ Modules: B: Source: ./map-module.yaml - Map: x,y + ForEach: x,y Properties: - Name: b-$MapValue + Name: b-$Identifier Outputs: BArns: Value: !GetAtt B[].Arn diff --git a/cft/pkg/tmpl/awscli-modules/map-template.yaml b/cft/pkg/tmpl/awscli-modules/map-template.yaml index d2a553d4e..7da1dfd4f 100644 --- a/cft/pkg/tmpl/awscli-modules/map-template.yaml +++ b/cft/pkg/tmpl/awscli-modules/map-template.yaml @@ -6,16 +6,16 @@ Parameters: Modules: Content: Source: ./map-module.yaml - Map: !Ref List + ForEach: !Ref List Properties: - Name: !Sub my-bucket-$MapValue + Name: !Sub my-bucket-$Identifier IndexTest: Source: ./map-index-module.yaml - Map: !Ref List + ForEach: !Ref List Properties: - ContentRef: !GetAtt Content[$MapIndex].Arn + ContentRef: !GetAtt Content[$Index].Arn NestedRef: - Obj: !GetAtt Content[$MapIndex].Arn + Obj: !GetAtt Content[$Index].Arn ListTest: Source: ./map-list-module.yaml Properties: @@ -24,3 +24,9 @@ Modules: Bar: Properties: ListOverride: !GetAtt Content[].Arn + +Outputs: + TestSubArray: + Value: !Sub ${Content[0].Arn} + TestUsingKey: + Value: !Sub ${Content[A].Arn} diff --git a/cft/pkg/tmpl/awscli-modules/transform-expect.yaml b/cft/pkg/tmpl/awscli-modules/transform-expect.yaml new file mode 100644 index 000000000..8c4dd11ce --- /dev/null +++ b/cft/pkg/tmpl/awscli-modules/transform-expect.yaml @@ -0,0 +1,5 @@ +Transform: + - AWS::LanguageExtensions + - MyMacro + - AWS::CodeDeployBlueGreen + diff --git a/cft/pkg/tmpl/awscli-modules/transform-module.yaml b/cft/pkg/tmpl/awscli-modules/transform-module.yaml new file mode 100644 index 000000000..9b6cd2de6 --- /dev/null +++ b/cft/pkg/tmpl/awscli-modules/transform-module.yaml @@ -0,0 +1,5 @@ +Transform: + - MyMacro + - AWS::CodeDeployBlueGreen + + diff --git a/cft/pkg/tmpl/awscli-modules/transform-template.yaml b/cft/pkg/tmpl/awscli-modules/transform-template.yaml new file mode 100644 index 000000000..639c95e64 --- /dev/null +++ b/cft/pkg/tmpl/awscli-modules/transform-template.yaml @@ -0,0 +1,5 @@ +Transform: AWS::LanguageExtensions + +Modules: + T: + Source: transform-module.yaml diff --git a/cft/pkg/tmpl/awscli-modules/vpc-module.yaml b/cft/pkg/tmpl/awscli-modules/vpc-module.yaml index f384467bc..8439876e3 100644 --- a/cft/pkg/tmpl/awscli-modules/vpc-module.yaml +++ b/cft/pkg/tmpl/awscli-modules/vpc-module.yaml @@ -11,11 +11,11 @@ Parameters: Modules: PublicSubnet: - Map: !Ref PublicCidrBlocks + ForEach: !Ref PublicCidrBlocks Source: ./subnet-module.yaml Properties: - SubnetCidrBlock: $MapValue - AZSelection: $MapIndex + SubnetCidrBlock: $Identifier + AZSelection: $Index VPCId: !Ref VPC InternetGatewayId: !Ref InternetGateway IsPublic: true @@ -24,11 +24,11 @@ Modules: DependsOn: GatewayAttachment PrivateSubnet: - Map: !Ref PrivateCidrBlocks + ForEach: !Ref PrivateCidrBlocks Source: ./subnet-module.yaml Properties: - SubnetCidrBlock: $MapValue - AZSelection: $MapIndex + SubnetCidrBlock: $Identifier + AZSelection: $Index VPCId: !Ref VPC InternetGatewayId: !Ref InternetGateway IsPublic: false diff --git a/docs/README.tmpl b/docs/README.tmpl index d086088fe..12bd67c9d 100644 --- a/docs/README.tmpl +++ b/docs/README.tmpl @@ -429,13 +429,14 @@ Modules are not allowed to refer to themselves directly or in cycles. Module A can’t import Module A, and it can’t import Module B if that module imports Module A. -Modules support a basic form of looping/foreach by adding a `Map` attribute to -the module configuration. Special variables `$MapIndex` and `$MapValue` can be -used to refer to the index and list value. Logical Ids are auto-incremented by +Modules support a basic form of looping/foreach by either using the familiar +`Fn::ForEach` syntax, or with a shorthand by adding a `ForEach` attribute to +the module configuration. Special variables `$Identifier` and `$Index` can be +used to refer to the value and list index. With the shorthand, or if you don't +put the Identifier in the logical id, logical ids are auto-incremented by adding an integer starting at zero. Since this is a client-side-only feature, list values must be fully resolved scalars, not values that must be resolved at -deploy time. When deploy-time values are needed, `Fn::ForEach` is a better -design option. Local modules do not process `Fn::ForEach`. +deploy time. ``` Parameters: @@ -446,7 +447,7 @@ Parameters: Modules: Content: Source: ./map-module.yaml - Map: !Ref List + ForEach: !Ref List Properties: Name: !Sub my-bucket-$MapValue ``` @@ -478,15 +479,12 @@ It’s also possible to refer to elements within a `Map` using something like which resolves to a list of all of the `Arn` outputs from that module. When a module is processed, the first thing that happens is parsing of the -`Conditions` within the module. These conditions must be fully resolvable -client-side, since the package command does not have access to Parameters or -deploy-time values. These conditions are converted to a dictionary of boolean -values and the `Conditions` section is not emitted into the parent template. It -is not merged into the parent. Any resources marked with a false condition are -removed, and any property nodes with conditions are processed. Any values of -`!Ref AWS::NoValue` are removed. No evidence of conditions will remain in the -markup that is merged into the parent template, unless the condition is not -found in the module. +Conditions within the module. Any Resources, Modules, or Outputs marked with a +false condition are removed, and any property nodes with conditions are +processed. Any values of !Ref AWS::NoValue are removed. Any unresolved +conditions (for example, a condition that references a paramter in the parent +template, or something like AWS::Region) are emitted into the parent template, +prefixed with the module name. Much of the value of module output is in the smart handling of `Ref`, `Fn::GetAtt`, and `Fn::Sub`. For the most part, we want these to “just work” in @@ -584,11 +582,6 @@ Modules: Source: $def/a/b/bar.yaml ``` - - - - - ### Publish modules to CodeArtifact Rain integrates with AWS CodeArtifact to enable an experience similar to npm diff --git a/docs/index.md b/docs/index.md index d95637255..868693b46 100644 --- a/docs/index.md +++ b/docs/index.md @@ -36,4 +36,4 @@ Rain is a command line tool for working with AWS CloudFormation templates and st * [rain tree](rain_tree.md) - Find dependencies of Resources and Outputs in a local template * [rain watch](rain_watch.md) - Display an updating view of a CloudFormation stack -###### Auto generated by spf13/cobra on 7-Apr-2025 +###### Auto generated by spf13/cobra on 17-Apr-2025 diff --git a/docs/rain_bootstrap.md b/docs/rain_bootstrap.md index c24646b64..d4a004bf8 100644 --- a/docs/rain_bootstrap.md +++ b/docs/rain_bootstrap.md @@ -33,4 +33,4 @@ rain bootstrap * [rain](index.md) - -###### Auto generated by spf13/cobra on 7-Apr-2025 +###### Auto generated by spf13/cobra on 17-Apr-2025 diff --git a/docs/rain_build.md b/docs/rain_build.md index 42e4d3d38..9c658d797 100644 --- a/docs/rain_build.md +++ b/docs/rain_build.md @@ -42,4 +42,4 @@ rain build [] or * [rain](index.md) - -###### Auto generated by spf13/cobra on 7-Apr-2025 +###### Auto generated by spf13/cobra on 17-Apr-2025 diff --git a/docs/rain_cat.md b/docs/rain_cat.md index 71601b858..4fce40b44 100644 --- a/docs/rain_cat.md +++ b/docs/rain_cat.md @@ -35,4 +35,4 @@ rain cat * [rain](index.md) - -###### Auto generated by spf13/cobra on 7-Apr-2025 +###### Auto generated by spf13/cobra on 17-Apr-2025 diff --git a/docs/rain_cc.md b/docs/rain_cc.md index 8efc1ec34..a6d943bdb 100644 --- a/docs/rain_cc.md +++ b/docs/rain_cc.md @@ -33,4 +33,4 @@ You must pass the --experimental (-x) flag to use this command, to acknowledge t * [rain cc rm](rain_cc_rm.md) - Delete a deployment created by cc deploy (Experimental!) * [rain cc state](rain_cc_state.md) - Download the state file for a template deployed with cc deploy -###### Auto generated by spf13/cobra on 7-Apr-2025 +###### Auto generated by spf13/cobra on 17-Apr-2025 diff --git a/docs/rain_cc_deploy.md b/docs/rain_cc_deploy.md index 21bdaef0c..f017b7666 100644 --- a/docs/rain_cc_deploy.md +++ b/docs/rain_cc_deploy.md @@ -40,4 +40,4 @@ rain cc deploy