From 469362a4efe7ce59a37ae828091ab705ae70ff5c Mon Sep 17 00:00:00 2001 From: Jordan Singer Date: Wed, 6 Dec 2023 10:13:51 -0600 Subject: [PATCH 1/3] validate fields for property refs and produce config errors repoty --- pkg/engine2/cli.go | 12 ++ pkg/engine2/engine.go | 13 ++ pkg/engine2/enginetesting/mock_solution.go | 5 + pkg/engine2/enginetesting/test_solution.go | 4 + .../operational_eval/vertex_path_expand.go | 2 +- .../operational_eval/vertex_property.go | 12 ++ pkg/engine2/solution_context/decisions.go | 36 +++- pkg/engine2/solution_context/interface.go | 1 + pkg/engine2/solution_context/memory_record.go | 8 + .../properties/any_property.go | 7 +- .../properties/bool_property.go | 8 +- .../properties/bool_property_test.go | 26 ++- .../properties/float_property.go | 8 +- .../properties/float_property_test.go | 27 ++- .../properties/int_property.go | 8 +- .../properties/int_property_test.go | 27 ++- .../properties/list_property.go | 24 +-- .../properties/list_property_test.go | 26 ++- .../properties/map_property.go | 19 +- .../properties/map_property_test.go | 27 ++- pkg/knowledge_base2/properties/property.go | 29 +++ .../properties/property_test.go | 173 ++++++++++++++++++ .../properties/resource_property.go | 11 +- .../properties/set_property.go | 25 ++- .../properties/set_property_test.go | 26 ++- .../properties/string_property.go | 31 +++- .../properties/string_property_test.go | 49 ++++- pkg/knowledge_base2/reader/properties.go | 3 + pkg/knowledge_base2/resource_template.go | 4 +- pkg/templates/aws/resources/ecr_image.yaml | 1 + 30 files changed, 572 insertions(+), 80 deletions(-) create mode 100644 pkg/knowledge_base2/properties/property_test.go diff --git a/pkg/engine2/cli.go b/pkg/engine2/cli.go index b53b9f8ab..6b9748b70 100644 --- a/pkg/engine2/cli.go +++ b/pkg/engine2/cli.go @@ -287,6 +287,18 @@ func (em *EngineMain) RunEngine(cmd *cobra.Command, args []string) error { }, ) + configErrors := em.Engine.getPropertyValidation(context.Solutions[0]) + if len(configErrors) > 0 { + configErrorData, err := json.Marshal(configErrors) + if err != nil { + return errors.Errorf("failed to marshal config errors: %s", err.Error()) + } + files = append(files, &io.RawFile{ + FPath: "config-errors.json", + Content: configErrorData, + }) + } + err = io.OutputTo(files, architectureEngineCfg.outputDir) if err != nil { return errors.Errorf("failed to write output files: %s", err.Error()) diff --git a/pkg/engine2/engine.go b/pkg/engine2/engine.go index b94355954..ae620c385 100644 --- a/pkg/engine2/engine.go +++ b/pkg/engine2/engine.go @@ -43,3 +43,16 @@ func (e *Engine) Run(context *EngineContext) error { context.Solutions = append(context.Solutions, solutionCtx) return err } + +func (e *Engine) getPropertyValidation(ctx solution_context.SolutionContext) []solution_context.PropertyValidationDecision { + decisions := ctx.GetDecisions().GetRecords() + validationDecisions := make([]solution_context.PropertyValidationDecision, 0) + for _, decision := range decisions { + if validation, ok := decision.(solution_context.PropertyValidationDecision); ok { + if validation.Error != nil { + validationDecisions = append(validationDecisions, validation) + } + } + } + return validationDecisions +} diff --git a/pkg/engine2/enginetesting/mock_solution.go b/pkg/engine2/enginetesting/mock_solution.go index f0c2fac37..3a5407747 100644 --- a/pkg/engine2/enginetesting/mock_solution.go +++ b/pkg/engine2/enginetesting/mock_solution.go @@ -31,6 +31,11 @@ func (m *MockSolution) RecordDecision(d solution_context.SolveDecision) { m.Called(d) } +func (m *MockSolution) GetDecisions() solution_context.DecisionRecords { + args := m.Called() + return args.Get(0).(solution_context.DecisionRecords) +} + func (m *MockSolution) DataflowGraph() construct.Graph { args := m.Called() return args.Get(0).(construct.Graph) diff --git a/pkg/engine2/enginetesting/test_solution.go b/pkg/engine2/enginetesting/test_solution.go index db50f7565..52eae8b3f 100644 --- a/pkg/engine2/enginetesting/test_solution.go +++ b/pkg/engine2/enginetesting/test_solution.go @@ -58,6 +58,10 @@ func (sol *TestSolution) Constraints() *constraints.Constraints { func (sol *TestSolution) RecordDecision(d solution_context.SolveDecision) {} +func (sol *TestSolution) GetDecisions() solution_context.DecisionRecords { + return nil +} + func (sol *TestSolution) DataflowGraph() construct.Graph { return sol.dataflow } diff --git a/pkg/engine2/operational_eval/vertex_path_expand.go b/pkg/engine2/operational_eval/vertex_path_expand.go index b1591cb81..a88cb4478 100644 --- a/pkg/engine2/operational_eval/vertex_path_expand.go +++ b/pkg/engine2/operational_eval/vertex_path_expand.go @@ -268,7 +268,7 @@ func (v *pathExpandVertex) addDepsFromProps( continue } // if this dependency could pass validation for the resources property, consider it as a dependent vertex - if err := prop.Validate(resource, dep); err == nil { + if err := prop.Validate(resource, dep, solution_context.DynamicCtx(eval.Solution)); err == nil { changes.addEdge(v.Key(), Key{Ref: ref}) } } diff --git a/pkg/engine2/operational_eval/vertex_property.go b/pkg/engine2/operational_eval/vertex_property.go index aa98c2e37..cbba7c2d9 100644 --- a/pkg/engine2/operational_eval/vertex_property.go +++ b/pkg/engine2/operational_eval/vertex_property.go @@ -137,6 +137,18 @@ func (v *propertyVertex) Evaluate(eval *Evaluator) error { return eval.AddResources(res) } + // Now that the vertex is evaluated, we will check it for validity and record our decision + val, err := res.GetProperty(v.Ref.Property) + if err != nil { + return fmt.Errorf("error while validating resource property: could not get property %s on resource %s: %w", v.Ref.Property, v.Ref.Resource, err) + } + err = v.Template.Validate(res, val, solution_context.DynamicCtx(eval.Solution)) + eval.Solution.RecordDecision(solution_context.PropertyValidationDecision{ + Resource: v.Ref.Resource, + Property: v.Template, + Value: val, + Error: err, + }) return nil } diff --git a/pkg/engine2/solution_context/decisions.go b/pkg/engine2/solution_context/decisions.go index 1fa5c241b..82ea43d6d 100644 --- a/pkg/engine2/solution_context/decisions.go +++ b/pkg/engine2/solution_context/decisions.go @@ -1,6 +1,8 @@ package solution_context import ( + "fmt" + construct "github.com/klothoplatform/klotho/pkg/construct2" knowledgebase "github.com/klothoplatform/klotho/pkg/knowledge_base2" ) @@ -18,6 +20,7 @@ type ( // FindDecision(decision SolveDecision) []KV // // FindContext returns the various decisions (the what) for a given context (the why) // FindContext(key string, value any) []SolveDecision + GetRecords() []SolveDecision } SolveDecision interface { @@ -50,14 +53,35 @@ type ( Value any } - ResourceConfigurationError struct { + PropertyValidationDecision struct { Resource construct.ResourceId Property knowledgebase.Property + Value any + Error error } ) -func (d AddResourceDecision) internal() {} -func (d AddDependencyDecision) internal() {} -func (d RemoveResourceDecision) internal() {} -func (d RemoveDependencyDecision) internal() {} -func (d SetPropertyDecision) internal() {} +func (d AddResourceDecision) internal() {} +func (d AddDependencyDecision) internal() {} +func (d RemoveResourceDecision) internal() {} +func (d RemoveDependencyDecision) internal() {} +func (d SetPropertyDecision) internal() {} +func (d PropertyValidationDecision) internal() {} + +func (d PropertyValidationDecision) MarshalJSON() ([]byte, error) { + if d.Value != nil { + stringVal := `{ + "resource": "%s", + "property": "%s", + "value": "%s", + "error": "%s" + }` + return []byte(fmt.Sprintf(stringVal, d.Resource, d.Property.Details().Path, d.Value, d.Error)), nil + } + stringVal := `{ + "resource": "%s", + "property": "%s", + "error": "%s" + }` + return []byte(fmt.Sprintf(stringVal, d.Resource, d.Property.Details().Path, d.Error)), nil +} diff --git a/pkg/engine2/solution_context/interface.go b/pkg/engine2/solution_context/interface.go index 1bb4ccb09..a76716fac 100644 --- a/pkg/engine2/solution_context/interface.go +++ b/pkg/engine2/solution_context/interface.go @@ -14,6 +14,7 @@ type ( KnowledgeBase() knowledgebase.TemplateKB Constraints() *constraints.Constraints RecordDecision(d SolveDecision) + GetDecisions() DecisionRecords DataflowGraph() construct.Graph DeploymentGraph() construct.Graph diff --git a/pkg/engine2/solution_context/memory_record.go b/pkg/engine2/solution_context/memory_record.go index 3f43ac2d2..1e8237aaf 100644 --- a/pkg/engine2/solution_context/memory_record.go +++ b/pkg/engine2/solution_context/memory_record.go @@ -14,3 +14,11 @@ type ( func (m *MemoryRecord) AddRecord(context []KV, decision SolveDecision) { m.records = append(m.records, record{context: context, decision: decision}) } + +func (m *MemoryRecord) GetRecords() []SolveDecision { + var decisions []SolveDecision + for _, record := range m.records { + decisions = append(decisions, record.decision) + } + return decisions +} diff --git a/pkg/knowledge_base2/properties/any_property.go b/pkg/knowledge_base2/properties/any_property.go index 23fcda309..02709d458 100644 --- a/pkg/knowledge_base2/properties/any_property.go +++ b/pkg/knowledge_base2/properties/any_property.go @@ -1,6 +1,8 @@ package properties import ( + "fmt" + construct "github.com/klothoplatform/klotho/pkg/construct2" knowledgebase "github.com/klothoplatform/klotho/pkg/knowledge_base2" ) @@ -107,7 +109,10 @@ func (a *AnyProperty) Type() string { return "any" } -func (a *AnyProperty) Validate(resource *construct.Resource, value any) error { +func (a *AnyProperty) Validate(resource *construct.Resource, value any, ctx knowledgebase.DynamicContext) error { + if a.Required && value == nil { + return fmt.Errorf(knowledgebase.ErrRequiredProperty, a.Path, resource.ID) + } return nil } diff --git a/pkg/knowledge_base2/properties/bool_property.go b/pkg/knowledge_base2/properties/bool_property.go index 2b86cc834..d270bac99 100644 --- a/pkg/knowledge_base2/properties/bool_property.go +++ b/pkg/knowledge_base2/properties/bool_property.go @@ -83,7 +83,13 @@ func (b *BoolProperty) Type() string { return "bool" } -func (b *BoolProperty) Validate(resource *construct.Resource, value any) error { +func (b *BoolProperty) Validate(resource *construct.Resource, value any, ctx knowledgebase.DynamicContext) error { + if value == nil { + if b.Required { + return fmt.Errorf(knowledgebase.ErrRequiredProperty, b.Path, resource.ID) + } + return nil + } if _, ok := value.(bool); !ok { return fmt.Errorf("invalid bool value %v", value) } diff --git a/pkg/knowledge_base2/properties/bool_property_test.go b/pkg/knowledge_base2/properties/bool_property_test.go index 29a2f1434..a903a8c0e 100644 --- a/pkg/knowledge_base2/properties/bool_property_test.go +++ b/pkg/knowledge_base2/properties/bool_property_test.go @@ -4,8 +4,10 @@ import ( "testing" construct "github.com/klothoplatform/klotho/pkg/construct2" + "github.com/klothoplatform/klotho/pkg/engine2/enginetesting" knowledgebase "github.com/klothoplatform/klotho/pkg/knowledge_base2" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" ) func Test_SetBoolProperty(t *testing.T) { @@ -268,10 +270,12 @@ func Test_BoolPropertyType(t *testing.T) { func Test_BoolPropertyValidate(t *testing.T) { tests := []struct { - name string - property *BoolProperty - value any - wantErr bool + name string + property *BoolProperty + testResources []*construct.Resource + mockKBCalls []mock.Call + value any + wantErr bool }{ { name: "bool property", @@ -298,7 +302,19 @@ func Test_BoolPropertyValidate(t *testing.T) { t.Run(tt.name, func(t *testing.T) { assert := assert.New(t) resource := &construct.Resource{} - err := tt.property.Validate(resource, tt.value) + graph := construct.NewGraph() + for _, r := range tt.testResources { + graph.AddVertex(r) + } + mockKB := &enginetesting.MockKB{} + for _, call := range tt.mockKBCalls { + mockKB.On(call.Method, call.Arguments...).Return(call.ReturnArguments...) + } + ctx := knowledgebase.DynamicValueContext{ + Graph: graph, + KnowledgeBase: mockKB, + } + err := tt.property.Validate(resource, tt.value, ctx) assert.Equal(tt.wantErr, err != nil) }) } diff --git a/pkg/knowledge_base2/properties/float_property.go b/pkg/knowledge_base2/properties/float_property.go index 905673bb2..8d87ea839 100644 --- a/pkg/knowledge_base2/properties/float_property.go +++ b/pkg/knowledge_base2/properties/float_property.go @@ -91,7 +91,13 @@ func (f *FloatProperty) Type() string { return "float" } -func (f *FloatProperty) Validate(resource *construct.Resource, value any) error { +func (f *FloatProperty) Validate(resource *construct.Resource, value any, ctx knowledgebase.DynamicContext) error { + if value == nil { + if f.Required { + return fmt.Errorf(knowledgebase.ErrRequiredProperty, f.Path, resource.ID) + } + return nil + } floatVal, ok := value.(float64) if !ok { return fmt.Errorf("invalid int value %v", value) diff --git a/pkg/knowledge_base2/properties/float_property_test.go b/pkg/knowledge_base2/properties/float_property_test.go index ba876fda4..82a0a4517 100644 --- a/pkg/knowledge_base2/properties/float_property_test.go +++ b/pkg/knowledge_base2/properties/float_property_test.go @@ -4,8 +4,11 @@ import ( "testing" construct "github.com/klothoplatform/klotho/pkg/construct2" + "github.com/klothoplatform/klotho/pkg/engine2/enginetesting" + knowledgebase "github.com/klothoplatform/klotho/pkg/knowledge_base2" knowledgebase2 "github.com/klothoplatform/klotho/pkg/knowledge_base2" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" ) func Test_SetFloatProperty(t *testing.T) { @@ -196,10 +199,12 @@ func Test_ParseFloatValue(t *testing.T) { func Test_FloatProperty_Validate(t *testing.T) { tests := []struct { - name string - property *FloatProperty - value any - wantErr bool + name string + property *FloatProperty + testResources []*construct.Resource + mockKBCalls []mock.Call + value any + wantErr bool }{ { name: "float value", @@ -225,7 +230,19 @@ func Test_FloatProperty_Validate(t *testing.T) { t.Run(test.name, func(t *testing.T) { assert := assert.New(t) resource := &construct.Resource{} - err := test.property.Validate(resource, test.value) + graph := construct.NewGraph() + for _, r := range test.testResources { + graph.AddVertex(r) + } + mockKB := &enginetesting.MockKB{} + for _, call := range test.mockKBCalls { + mockKB.On(call.Method, call.Arguments...).Return(call.ReturnArguments...) + } + ctx := knowledgebase.DynamicValueContext{ + Graph: graph, + KnowledgeBase: mockKB, + } + err := test.property.Validate(resource, test.value, ctx) if test.wantErr { assert.Error(err) return diff --git a/pkg/knowledge_base2/properties/int_property.go b/pkg/knowledge_base2/properties/int_property.go index 10459fc1f..c8d18ae44 100644 --- a/pkg/knowledge_base2/properties/int_property.go +++ b/pkg/knowledge_base2/properties/int_property.go @@ -84,7 +84,13 @@ func (i *IntProperty) Type() string { return "int" } -func (i *IntProperty) Validate(resource *construct.Resource, value any) error { +func (i *IntProperty) Validate(resource *construct.Resource, value any, ctx knowledgebase.DynamicContext) error { + if value == nil { + if i.Required { + return fmt.Errorf(knowledgebase.ErrRequiredProperty, i.Path, resource.ID) + } + return nil + } intVal, ok := value.(int) if !ok { return fmt.Errorf("invalid int value %v", value) diff --git a/pkg/knowledge_base2/properties/int_property_test.go b/pkg/knowledge_base2/properties/int_property_test.go index 4235e9be7..5b1f5f7db 100644 --- a/pkg/knowledge_base2/properties/int_property_test.go +++ b/pkg/knowledge_base2/properties/int_property_test.go @@ -4,8 +4,11 @@ import ( "testing" construct "github.com/klothoplatform/klotho/pkg/construct2" + "github.com/klothoplatform/klotho/pkg/engine2/enginetesting" + knowledgebase "github.com/klothoplatform/klotho/pkg/knowledge_base2" knowledgebase2 "github.com/klothoplatform/klotho/pkg/knowledge_base2" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" ) func Test_SetIntPropertyProperty(t *testing.T) { @@ -188,10 +191,12 @@ func Test_IntProperty_Validate(t *testing.T) { upperBound := 10 lowerBound := 0 tests := []struct { - name string - property *IntProperty - value any - wantErr bool + name string + property *IntProperty + testResources []*construct.Resource + mockKBCalls []mock.Call + value any + wantErr bool }{ { name: "int property", @@ -252,7 +257,19 @@ func Test_IntProperty_Validate(t *testing.T) { t.Run(test.name, func(t *testing.T) { assert := assert.New(t) resource := &construct.Resource{} - err := test.property.Validate(resource, test.value) + graph := construct.NewGraph() + for _, r := range test.testResources { + graph.AddVertex(r) + } + mockKB := &enginetesting.MockKB{} + for _, call := range test.mockKBCalls { + mockKB.On(call.Method, call.Arguments...).Return(call.ReturnArguments...) + } + ctx := knowledgebase.DynamicValueContext{ + Graph: graph, + KnowledgeBase: mockKB, + } + err := test.property.Validate(resource, test.value, ctx) if test.wantErr { assert.Error(err) return diff --git a/pkg/knowledge_base2/properties/list_property.go b/pkg/knowledge_base2/properties/list_property.go index cb5e805f4..31da2ba11 100644 --- a/pkg/knowledge_base2/properties/list_property.go +++ b/pkg/knowledge_base2/properties/list_property.go @@ -167,7 +167,14 @@ func (l *ListProperty) Type() string { return "list" } -func (l *ListProperty) Validate(resource *construct.Resource, value any) error { +func (l *ListProperty) Validate(resource *construct.Resource, value any, ctx knowledgebase.DynamicContext) error { + if value == nil { + if l.Required { + return fmt.Errorf(knowledgebase.ErrRequiredProperty, l.Path, resource.ID) + } + return nil + } + listVal, ok := value.([]any) if !ok { return fmt.Errorf("invalid map value %v", value) @@ -183,20 +190,15 @@ func (l *ListProperty) Validate(resource *construct.Resource, value any) error { } } var errs error - - for _, v := range listVal { - if len(l.Properties) != 0 { - m := MapProperty{Properties: l.Properties} - err := m.Validate(resource, v) - if err != nil { - errs = errors.New(errs.Error() + "\n" + err.Error()) - } - } else { - err := l.ItemProperty.Validate(resource, v) + // Only validate values if its a primitive list, otherwise let the sub properties handle their own validation + if l.ItemProperty != nil { + for _, v := range listVal { + err := l.ItemProperty.Validate(resource, v, ctx) if err != nil { errs = errors.New(errs.Error() + "\n" + err.Error()) } } + } if errs != nil { return errs diff --git a/pkg/knowledge_base2/properties/list_property_test.go b/pkg/knowledge_base2/properties/list_property_test.go index 9aadcc7a1..4833f529a 100644 --- a/pkg/knowledge_base2/properties/list_property_test.go +++ b/pkg/knowledge_base2/properties/list_property_test.go @@ -4,8 +4,10 @@ import ( "testing" construct "github.com/klothoplatform/klotho/pkg/construct2" + "github.com/klothoplatform/klotho/pkg/engine2/enginetesting" knowledgebase "github.com/klothoplatform/klotho/pkg/knowledge_base2" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" ) func Test_ListProperty_Set(t *testing.T) { @@ -285,10 +287,12 @@ func Test_ListProperty_Validate(t *testing.T) { minLength := 1 maxLength := 2 tests := []struct { - name string - property *ListProperty - value any - wantErr bool + name string + property *ListProperty + testResources []*construct.Resource + mockKBCalls []mock.Call + value any + wantErr bool }{ { name: "list property", @@ -346,7 +350,19 @@ func Test_ListProperty_Validate(t *testing.T) { t.Run(tt.name, func(t *testing.T) { assert := assert.New(t) resource := &construct.Resource{} - err := tt.property.Validate(resource, tt.value) + graph := construct.NewGraph() + for _, r := range tt.testResources { + graph.AddVertex(r) + } + mockKB := &enginetesting.MockKB{} + for _, call := range tt.mockKBCalls { + mockKB.On(call.Method, call.Arguments...).Return(call.ReturnArguments...) + } + ctx := knowledgebase.DynamicValueContext{ + Graph: graph, + KnowledgeBase: mockKB, + } + err := tt.property.Validate(resource, tt.value, ctx) if tt.wantErr { assert.Error(err) return diff --git a/pkg/knowledge_base2/properties/map_property.go b/pkg/knowledge_base2/properties/map_property.go index 25319ae50..ee8a9b424 100644 --- a/pkg/knowledge_base2/properties/map_property.go +++ b/pkg/knowledge_base2/properties/map_property.go @@ -176,7 +176,13 @@ func (m *MapProperty) Type() string { return "map" } -func (m *MapProperty) Validate(resource *construct.Resource, value any) error { +func (m *MapProperty) Validate(resource *construct.Resource, value any, ctx knowledgebase.DynamicContext) error { + if value == nil { + if m.Required { + return fmt.Errorf(knowledgebase.ErrRequiredProperty, m.Path, resource.ID) + } + return nil + } mapVal, ok := value.(map[string]any) if !ok { return fmt.Errorf("invalid map value %v", value) @@ -192,18 +198,13 @@ func (m *MapProperty) Validate(resource *construct.Resource, value any) error { } } var errs error + // Only validate values if its a primitive map, otherwise let the sub properties handle their own validation if m.KeyProperty != nil && m.ValueProperty != nil { for k, v := range mapVal { - if err := m.KeyProperty.Validate(resource, k); err != nil { + if err := m.KeyProperty.Validate(resource, k, ctx); err != nil { errs = errors.Join(errs, fmt.Errorf("invalid key %v for map property type %s: %w", k, m.KeyProperty.Type(), err)) } - if err := m.ValueProperty.Validate(resource, v); err != nil { - errs = errors.Join(errs, fmt.Errorf("invalid value %v for map property type %s: %w", v, m.ValueProperty.Type(), err)) - } - } - } else { - for _, v := range mapVal { - if err := m.ValueProperty.Validate(resource, v); err != nil { + if err := m.ValueProperty.Validate(resource, v, ctx); err != nil { errs = errors.Join(errs, fmt.Errorf("invalid value %v for map property type %s: %w", v, m.ValueProperty.Type(), err)) } } diff --git a/pkg/knowledge_base2/properties/map_property_test.go b/pkg/knowledge_base2/properties/map_property_test.go index 9fa59d32c..744636d21 100644 --- a/pkg/knowledge_base2/properties/map_property_test.go +++ b/pkg/knowledge_base2/properties/map_property_test.go @@ -4,8 +4,11 @@ import ( "testing" construct "github.com/klothoplatform/klotho/pkg/construct2" + "github.com/klothoplatform/klotho/pkg/engine2/enginetesting" + knowledgebase "github.com/klothoplatform/klotho/pkg/knowledge_base2" knowledgebase2 "github.com/klothoplatform/klotho/pkg/knowledge_base2" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" ) func Test_SetMapProperty(t *testing.T) { @@ -221,10 +224,12 @@ func Test_MapProperty_Type(t *testing.T) { func Test_MapProperty_Validate(t *testing.T) { tests := []struct { - name string - property *MapProperty - value any - wantErr bool + name string + property *MapProperty + testResources []*construct.Resource + mockKBCalls []mock.Call + value any + wantErr bool }{ { name: "valid map property", @@ -273,7 +278,19 @@ func Test_MapProperty_Validate(t *testing.T) { t.Run(test.name, func(t *testing.T) { assert := assert.New(t) resource := &construct.Resource{} - err := test.property.Validate(resource, test.value) + graph := construct.NewGraph() + for _, r := range test.testResources { + graph.AddVertex(r) + } + mockKB := &enginetesting.MockKB{} + for _, call := range test.mockKBCalls { + mockKB.On(call.Method, call.Arguments...).Return(call.ReturnArguments...) + } + ctx := knowledgebase.DynamicValueContext{ + Graph: graph, + KnowledgeBase: mockKB, + } + err := test.property.Validate(resource, test.value, ctx) if test.wantErr { assert.Error(err) return diff --git a/pkg/knowledge_base2/properties/property.go b/pkg/knowledge_base2/properties/property.go index 500ea8ce8..ffc67dffd 100644 --- a/pkg/knowledge_base2/properties/property.go +++ b/pkg/knowledge_base2/properties/property.go @@ -47,6 +47,35 @@ func ParsePropertyRef(value any, ctx knowledgebase.DynamicContext, data knowledg return construct.PropertyRef{}, fmt.Errorf("invalid property reference value %v", value) } +func ValidatePropertyRef(value construct.PropertyRef, propertyType string, ctx knowledgebase.DynamicContext) (refVal any, err error) { + resource, err := ctx.DAG().Vertex(value.Resource) + if err != nil { + return nil, fmt.Errorf("error getting resource %s, while validating property ref: %w", value.Resource, err) + } + if resource == nil { + return nil, fmt.Errorf("resource %s does not exist", value.Resource) + } + rt, err := ctx.KB().GetResourceTemplate(value.Resource) + if err != nil { + return nil, err + } + prop := rt.GetProperty(value.Property) + if prop == nil { + return nil, fmt.Errorf("property %s does not exist on resource %s", value.Property, value.Resource) + } + if prop.Type() != propertyType { + return nil, fmt.Errorf("property %s on resource %s is not of type %s", value.Property, value.Resource, propertyType) + } + if prop.Details().DeployTime { + return nil, nil + } + propVal, err := resource.GetProperty(value.Property) + if err != nil { + return nil, fmt.Errorf("error getting property %s on resource %s, while validating property ref: %w", value.Property, value.Resource, err) + } + return propVal, nil +} + func (p *PropertyValidityCheck) Validate(value any, properties construct.Properties) error { var buff bytes.Buffer data := ValidityCheckData{ diff --git a/pkg/knowledge_base2/properties/property_test.go b/pkg/knowledge_base2/properties/property_test.go new file mode 100644 index 000000000..d737d76e2 --- /dev/null +++ b/pkg/knowledge_base2/properties/property_test.go @@ -0,0 +1,173 @@ +package properties + +import ( + "testing" + + construct "github.com/klothoplatform/klotho/pkg/construct2" + "github.com/klothoplatform/klotho/pkg/engine2/enginetesting" + knowledgebase "github.com/klothoplatform/klotho/pkg/knowledge_base2" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" +) + +func Test_ValidatePropertyRef(t *testing.T) { + tests := []struct { + name string + propertyType string + ref construct.PropertyRef + testResources []*construct.Resource + mockKBCalls []mock.Call + expect any + wantErr bool + }{ + { + name: "string property ref, existing resource with value", + propertyType: "string", + ref: construct.PropertyRef{ + Resource: construct.ResourceId{Name: "test"}, + Property: "test", + }, + testResources: []*construct.Resource{ + { + ID: construct.ResourceId{Name: "test"}, + Properties: map[string]any{ + "test": "testval", + }, + }, + }, + mockKBCalls: []mock.Call{ + { + Method: "GetResourceTemplate", + Arguments: mock.Arguments{ + construct.ResourceId{Name: "test"}, + }, + ReturnArguments: mock.Arguments{ + &knowledgebase.ResourceTemplate{ + Properties: knowledgebase.Properties{ + "test": &StringProperty{PropertyDetails: knowledgebase.PropertyDetails{Path: "test", Name: "test"}}, + }, + }, nil, + }, + }, + }, + expect: "testval", + }, + { + name: "string property is deploy time returns nil", + propertyType: "string", + ref: construct.PropertyRef{ + Resource: construct.ResourceId{Name: "test"}, + Property: "test", + }, + testResources: []*construct.Resource{ + { + ID: construct.ResourceId{Name: "test"}, + Properties: map[string]any{ + "test": "testval", + }, + }, + }, + mockKBCalls: []mock.Call{ + { + Method: "GetResourceTemplate", + Arguments: mock.Arguments{ + construct.ResourceId{Name: "test"}, + }, + ReturnArguments: mock.Arguments{ + &knowledgebase.ResourceTemplate{ + Properties: knowledgebase.Properties{ + "test": &StringProperty{PropertyDetails: knowledgebase.PropertyDetails{ + Path: "test", + Name: "test", + DeployTime: true, + }}, + }, + }, nil, + }, + }, + }, + }, + { + name: "no resource, throws err", + propertyType: "string", + ref: construct.PropertyRef{ + Resource: construct.ResourceId{Name: "test"}, + Property: "test", + }, + mockKBCalls: []mock.Call{ + { + Method: "GetResourceTemplate", + Arguments: mock.Arguments{ + construct.ResourceId{Name: "test"}, + }, + ReturnArguments: mock.Arguments{ + &knowledgebase.ResourceTemplate{ + Properties: knowledgebase.Properties{ + "test": &StringProperty{PropertyDetails: knowledgebase.PropertyDetails{ + Path: "test", + Name: "test", + DeployTime: true, + }}, + }, + }, nil, + }, + }, + }, + wantErr: true, + }, + { + name: "no property throws error", + propertyType: "string", + ref: construct.PropertyRef{ + Resource: construct.ResourceId{Name: "test"}, + Property: "test", + }, + testResources: []*construct.Resource{ + { + ID: construct.ResourceId{Name: "test"}, + Properties: map[string]any{ + "test": "testval", + }, + }, + }, + mockKBCalls: []mock.Call{ + { + Method: "GetResourceTemplate", + Arguments: mock.Arguments{ + construct.ResourceId{Name: "test"}, + }, + ReturnArguments: mock.Arguments{ + &knowledgebase.ResourceTemplate{ + Properties: knowledgebase.Properties{}, + }, nil, + }, + }, + }, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert := assert.New(t) + graph := construct.NewGraph() + for _, r := range tt.testResources { + graph.AddVertex(r) + } + mockKB := &enginetesting.MockKB{} + for _, call := range tt.mockKBCalls { + mockKB.On(call.Method, call.Arguments...).Return(call.ReturnArguments...) + } + ctx := knowledgebase.DynamicValueContext{ + Graph: graph, + KnowledgeBase: mockKB, + } + val, err := ValidatePropertyRef(tt.ref, tt.propertyType, ctx) + if tt.wantErr { + assert.Error(err) + } else { + assert.NoError(err) + assert.Equal(tt.expect, val) + } + }) + } +} diff --git a/pkg/knowledge_base2/properties/resource_property.go b/pkg/knowledge_base2/properties/resource_property.go index 5d559cfea..754358570 100644 --- a/pkg/knowledge_base2/properties/resource_property.go +++ b/pkg/knowledge_base2/properties/resource_property.go @@ -3,7 +3,6 @@ package properties import ( "fmt" - "github.com/klothoplatform/klotho/pkg/collectionutil" construct "github.com/klothoplatform/klotho/pkg/construct2" knowledgebase "github.com/klothoplatform/klotho/pkg/knowledge_base2" ) @@ -126,12 +125,18 @@ func (r *ResourceProperty) Type() string { return "resource" } -func (r *ResourceProperty) Validate(resource *construct.Resource, value any) error { +func (r *ResourceProperty) Validate(resource *construct.Resource, value any, ctx knowledgebase.DynamicContext) error { + if value == nil { + if r.Required { + return fmt.Errorf(knowledgebase.ErrRequiredProperty, r.Path, resource.ID) + } + return nil + } id, ok := value.(construct.ResourceId) if !ok { return fmt.Errorf("invalid resource value %v", value) } - if !collectionutil.Contains(r.AllowedTypes, id) { + if r.AllowedTypes != nil && len(r.AllowedTypes) > 0 && !r.AllowedTypes.MatchesAny(id) { return fmt.Errorf("resource value %v does not match allowed types %s", value, r.AllowedTypes) } return nil diff --git a/pkg/knowledge_base2/properties/set_property.go b/pkg/knowledge_base2/properties/set_property.go index 94a1ccb80..c0abd2328 100644 --- a/pkg/knowledge_base2/properties/set_property.go +++ b/pkg/knowledge_base2/properties/set_property.go @@ -156,7 +156,13 @@ func (s *SetProperty) Type() string { return "set" } -func (s *SetProperty) Validate(resource *construct.Resource, value any) error { +func (s *SetProperty) Validate(resource *construct.Resource, value any, ctx knowledgebase.DynamicContext) error { + if value == nil { + if s.Required { + return fmt.Errorf(knowledgebase.ErrRequiredProperty, s.Path, resource.ID) + } + return nil + } setVal, ok := value.(set.HashedSet[string, any]) if !ok { return fmt.Errorf("could not validate set property: invalid set value %v", value) @@ -172,14 +178,17 @@ func (s *SetProperty) Validate(resource *construct.Resource, value any) error { } } - var errs error - for _, item := range setVal.ToSlice() { - if err := s.ItemProperty.Validate(resource, item); err != nil { - errs = errors.Join(errs, fmt.Errorf("invalid item %v: %v", item, err)) + // Only validate values if its a primitive list, otherwise let the sub properties handle their own validation + if s.ItemProperty != nil { + var errs error + for _, item := range setVal.ToSlice() { + if err := s.ItemProperty.Validate(resource, item, ctx); err != nil { + errs = errors.Join(errs, fmt.Errorf("invalid item %v: %v", item, err)) + } + } + if errs != nil { + return errs } - } - if errs != nil { - return errs } return nil } diff --git a/pkg/knowledge_base2/properties/set_property_test.go b/pkg/knowledge_base2/properties/set_property_test.go index 709138338..61fbc91c6 100644 --- a/pkg/knowledge_base2/properties/set_property_test.go +++ b/pkg/knowledge_base2/properties/set_property_test.go @@ -5,9 +5,11 @@ import ( "testing" construct "github.com/klothoplatform/klotho/pkg/construct2" + "github.com/klothoplatform/klotho/pkg/engine2/enginetesting" knowledgebase "github.com/klothoplatform/klotho/pkg/knowledge_base2" "github.com/klothoplatform/klotho/pkg/set" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" ) func Test_SetSetProperty(t *testing.T) { @@ -465,10 +467,12 @@ func Test_SetValidate(t *testing.T) { minLength := 1 maxLength := 2 tests := []struct { - name string - property *SetProperty - value any - expected bool + name string + property *SetProperty + testResources []*construct.Resource + mockKBCalls []mock.Call + value any + expected bool }{ { name: "set property", @@ -570,7 +574,19 @@ func Test_SetValidate(t *testing.T) { t.Run(tt.name, func(t *testing.T) { assert := assert.New(t) resource := &construct.Resource{} - actual := tt.property.Validate(resource, tt.value) + graph := construct.NewGraph() + for _, r := range tt.testResources { + graph.AddVertex(r) + } + mockKB := &enginetesting.MockKB{} + for _, call := range tt.mockKBCalls { + mockKB.On(call.Method, call.Arguments...).Return(call.ReturnArguments...) + } + ctx := knowledgebase.DynamicValueContext{ + Graph: graph, + KnowledgeBase: mockKB, + } + actual := tt.property.Validate(resource, tt.value, ctx) if tt.expected { assert.NoError(actual) } else { diff --git a/pkg/knowledge_base2/properties/string_property.go b/pkg/knowledge_base2/properties/string_property.go index b94241919..606e37f97 100644 --- a/pkg/knowledge_base2/properties/string_property.go +++ b/pkg/knowledge_base2/properties/string_property.go @@ -92,15 +92,34 @@ func (s *StringProperty) Type() string { return "string" } -func (s *StringProperty) Validate(resource *construct.Resource, value any) error { +func (s *StringProperty) Validate(resource *construct.Resource, value any, ctx knowledgebase.DynamicContext) error { + if value == nil { + if s.Required { + return fmt.Errorf(knowledgebase.ErrRequiredProperty, s.Path, resource.ID) + } + return nil + } stringVal, ok := value.(string) if !ok { - return fmt.Errorf("could not validate property: invalid string value %v", value) - } - if s.AllowedValues != nil { - if !collectionutil.Contains(s.AllowedValues, stringVal) { - return fmt.Errorf("value %s is not allowed. allowed values are %s", stringVal, s.AllowedValues) + propertyRef, ok := value.(construct.PropertyRef) + if !ok { + return fmt.Errorf("could not validate property: invalid string value %v", value) } + refVal, err := ValidatePropertyRef(propertyRef, s.Type(), ctx) + if err != nil { + return err + } + if refVal == nil { + return nil + } + stringVal, ok = refVal.(string) + if !ok { + return fmt.Errorf("could not validate property: invalid string value %v", value) + } + } + + if s.AllowedValues != nil && len(s.AllowedValues) > 0 && !collectionutil.Contains(s.AllowedValues, stringVal) { + return fmt.Errorf("value %s is not allowed. allowed values are %s", stringVal, s.AllowedValues) } if s.SanitizeTmpl != nil { diff --git a/pkg/knowledge_base2/properties/string_property_test.go b/pkg/knowledge_base2/properties/string_property_test.go index 0d130034a..c6add3b38 100644 --- a/pkg/knowledge_base2/properties/string_property_test.go +++ b/pkg/knowledge_base2/properties/string_property_test.go @@ -4,8 +4,10 @@ import ( "testing" construct "github.com/klothoplatform/klotho/pkg/construct2" + "github.com/klothoplatform/klotho/pkg/engine2/enginetesting" knowledgebase "github.com/klothoplatform/klotho/pkg/knowledge_base2" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" ) func Test_SetStringProperty(t *testing.T) { @@ -361,6 +363,8 @@ func Test_StringValidate(t *testing.T) { name string property *StringProperty sanitizeTemplate string + testResources []*construct.Resource + mockKBCalls []mock.Call value any expected bool }{ @@ -396,6 +400,37 @@ func Test_StringValidate(t *testing.T) { value: "TEST", expected: true, }, + { + name: "Valid property ref as string", + property: &StringProperty{ + PropertyDetails: knowledgebase.PropertyDetails{ + Path: "test", + }, + }, + testResources: []*construct.Resource{ + { + ID: construct.ResourceId{Name: "blah"}, + Properties: construct.Properties{"test": "blah"}, + }, + }, + value: construct.PropertyRef{Resource: construct.ResourceId{Name: "blah"}, Property: "test"}, + mockKBCalls: []mock.Call{ + { + Method: "GetResourceTemplate", + Arguments: mock.Arguments{ + construct.ResourceId{Name: "blah"}, + }, + ReturnArguments: mock.Arguments{ + &knowledgebase.ResourceTemplate{ + Properties: knowledgebase.Properties{ + "test": &StringProperty{PropertyDetails: knowledgebase.PropertyDetails{Path: "test", Name: "test"}}, + }, + }, nil, + }, + }, + }, + expected: true, + }, { name: "string not in allowed values", property: &StringProperty{ @@ -440,7 +475,19 @@ func Test_StringValidate(t *testing.T) { tt.property.SanitizeTmpl = tmpl } resource := &construct.Resource{} - actual := tt.property.Validate(resource, tt.value) + graph := construct.NewGraph() + for _, r := range tt.testResources { + graph.AddVertex(r) + } + mockKB := &enginetesting.MockKB{} + for _, call := range tt.mockKBCalls { + mockKB.On(call.Method, call.Arguments...).Return(call.ReturnArguments...) + } + ctx := knowledgebase.DynamicValueContext{ + Graph: graph, + KnowledgeBase: mockKB, + } + actual := tt.property.Validate(resource, tt.value, ctx) if tt.expected { assert.NoError(actual) } else { diff --git a/pkg/knowledge_base2/reader/properties.go b/pkg/knowledge_base2/reader/properties.go index 98224cf59..5a2b53c39 100644 --- a/pkg/knowledge_base2/reader/properties.go +++ b/pkg/knowledge_base2/reader/properties.go @@ -198,6 +198,9 @@ var fieldConversion = map[string]func(val reflect.Value, p *Property, kp knowled if !ok { return fmt.Errorf("invalid sanitize template") } + if sanitizeTmpl == "" { + return nil + } // generate random uuid as the name of the template name := uuid.New().String() tmpl, err := knowledgebase.NewSanitizationTmpl(name, sanitizeTmpl) diff --git a/pkg/knowledge_base2/resource_template.go b/pkg/knowledge_base2/resource_template.go index 5bf1a07a3..38043097c 100644 --- a/pkg/knowledge_base2/resource_template.go +++ b/pkg/knowledge_base2/resource_template.go @@ -81,7 +81,7 @@ type ( // GetDefaultValue returns the default value for the property, pertaining to the specific data being passed in for execution GetDefaultValue(ctx DynamicContext, data DynamicValueData) (any, error) // Validate ensures the value is valid for the property and returns an error if it is not - Validate(resource *construct.Resource, value any) error + Validate(resource *construct.Resource, value any, ctx DynamicContext) error // SubProperties returns the sub properties of the property, if any. This is used for properties that are complex structures, // such as lists, sets, or maps SubProperties() Properties @@ -141,6 +141,8 @@ type ( ) const ( + ErrRequiredProperty = "required property %s is not set on resource %s" + Compute Functionality = "compute" Cluster Functionality = "cluster" Storage Functionality = "storage" diff --git a/pkg/templates/aws/resources/ecr_image.yaml b/pkg/templates/aws/resources/ecr_image.yaml index 8ce6c0992..e742f8773 100644 --- a/pkg/templates/aws/resources/ecr_image.yaml +++ b/pkg/templates/aws/resources/ecr_image.yaml @@ -4,6 +4,7 @@ display_name: ECR Image properties: BaseImage: type: string + required: true Tag: type: string Repo: From a93cd6f82ade498c19a9ed356bff946a6ef857b7 Mon Sep 17 00:00:00 2001 From: Jordan Singer Date: Thu, 7 Dec 2023 09:19:36 -0600 Subject: [PATCH 2/3] pr comments and returning different error types based on failure --- cmd/engine/main.go | 10 ++++++++-- pkg/engine2/cli.go | 7 +++++-- pkg/engine2/engine.go | 14 ++++++++++++-- pkg/engine2/errors.go | 11 +++++++++++ pkg/knowledge_base2/properties/float_property.go | 6 +++--- pkg/knowledge_base2/properties/string_property.go | 2 +- pkg/templates/aws/resources/rds_instance.yaml | 8 ++++++++ 7 files changed, 48 insertions(+), 10 deletions(-) create mode 100644 pkg/engine2/errors.go diff --git a/cmd/engine/main.go b/cmd/engine/main.go index d298f63e9..ad6ab8f91 100644 --- a/cmd/engine/main.go +++ b/cmd/engine/main.go @@ -14,7 +14,13 @@ func main() { em.AddEngineCli(root) err := root.Execute() if err != nil { - fmt.Printf("Error: %v\n", err) - os.Exit(1) + switch err.(type) { + case engine.ConfigValidationError: + fmt.Printf("Error: %v\n", err) + os.Exit(2) + default: + fmt.Printf("Error: %v\n", err) + os.Exit(1) + } } } diff --git a/pkg/engine2/cli.go b/pkg/engine2/cli.go index 6b9748b70..3e30013e9 100644 --- a/pkg/engine2/cli.go +++ b/pkg/engine2/cli.go @@ -287,14 +287,14 @@ func (em *EngineMain) RunEngine(cmd *cobra.Command, args []string) error { }, ) - configErrors := em.Engine.getPropertyValidation(context.Solutions[0]) + configErrors, configErr := em.Engine.getPropertyValidation(context.Solutions[0]) if len(configErrors) > 0 { configErrorData, err := json.Marshal(configErrors) if err != nil { return errors.Errorf("failed to marshal config errors: %s", err.Error()) } files = append(files, &io.RawFile{ - FPath: "config-errors.json", + FPath: "config_errors.json", Content: configErrorData, }) } @@ -303,5 +303,8 @@ func (em *EngineMain) RunEngine(cmd *cobra.Command, args []string) error { if err != nil { return errors.Errorf("failed to write output files: %s", err.Error()) } + if configErr != nil { + return ConfigValidationError{Err: configErr} + } return nil } diff --git a/pkg/engine2/engine.go b/pkg/engine2/engine.go index ae620c385..ec43483a9 100644 --- a/pkg/engine2/engine.go +++ b/pkg/engine2/engine.go @@ -1,6 +1,8 @@ package engine2 import ( + "errors" + construct "github.com/klothoplatform/klotho/pkg/construct2" "github.com/klothoplatform/klotho/pkg/engine2/constraints" "github.com/klothoplatform/klotho/pkg/engine2/solution_context" @@ -44,7 +46,7 @@ func (e *Engine) Run(context *EngineContext) error { return err } -func (e *Engine) getPropertyValidation(ctx solution_context.SolutionContext) []solution_context.PropertyValidationDecision { +func (e *Engine) getPropertyValidation(ctx solution_context.SolutionContext) ([]solution_context.PropertyValidationDecision, error) { decisions := ctx.GetDecisions().GetRecords() validationDecisions := make([]solution_context.PropertyValidationDecision, 0) for _, decision := range decisions { @@ -54,5 +56,13 @@ func (e *Engine) getPropertyValidation(ctx solution_context.SolutionContext) []s } } } - return validationDecisions + var errs error + for _, decision := range validationDecisions { + for _, c := range ctx.Constraints().Resources { + if c.Target == decision.Resource && c.Property == decision.Property.Details().Path { + errs = errors.Join(errs, decision.Error) + } + } + } + return validationDecisions, errs } diff --git a/pkg/engine2/errors.go b/pkg/engine2/errors.go new file mode 100644 index 000000000..0f1d33650 --- /dev/null +++ b/pkg/engine2/errors.go @@ -0,0 +1,11 @@ +package engine2 + +type ( + ConfigValidationError struct { + Err error + } +) + +func (e ConfigValidationError) Error() string { + return e.Err.Error() +} diff --git a/pkg/knowledge_base2/properties/float_property.go b/pkg/knowledge_base2/properties/float_property.go index 8d87ea839..4b926ceab 100644 --- a/pkg/knowledge_base2/properties/float_property.go +++ b/pkg/knowledge_base2/properties/float_property.go @@ -100,13 +100,13 @@ func (f *FloatProperty) Validate(resource *construct.Resource, value any, ctx kn } floatVal, ok := value.(float64) if !ok { - return fmt.Errorf("invalid int value %v", value) + return fmt.Errorf("invalid float value %v", value) } if f.MinValue != nil && floatVal < *f.MinValue { - return fmt.Errorf("int value %f is less than lower bound %f", value, *f.MinValue) + return fmt.Errorf("float value %f is less than lower bound %f", value, *f.MinValue) } if f.MaxValue != nil && floatVal > *f.MaxValue { - return fmt.Errorf("int value %f is greater than upper bound %f", value, *f.MaxValue) + return fmt.Errorf("float value %f is greater than upper bound %f", value, *f.MaxValue) } return nil } diff --git a/pkg/knowledge_base2/properties/string_property.go b/pkg/knowledge_base2/properties/string_property.go index 606e37f97..e58980d43 100644 --- a/pkg/knowledge_base2/properties/string_property.go +++ b/pkg/knowledge_base2/properties/string_property.go @@ -129,7 +129,7 @@ func (s *StringProperty) Validate(resource *construct.Resource, value any, ctx k return err } if oldVal != stringVal { - return fmt.Errorf("value %s was sanitized to %s", oldVal, stringVal) + return fmt.Errorf("value %s did not pass sanitization rules. suggested value: %s", oldVal, stringVal) } } return nil diff --git a/pkg/templates/aws/resources/rds_instance.yaml b/pkg/templates/aws/resources/rds_instance.yaml index 74587f763..0617f5704 100644 --- a/pkg/templates/aws/resources/rds_instance.yaml +++ b/pkg/templates/aws/resources/rds_instance.yaml @@ -35,6 +35,14 @@ properties: DatabaseName: type: string default_value: main + sanitize: | + {{ . + | replace `^[^[:alpha:]]+` "" + | replace `--+` "-" + | replace `-$` "" + | replace `[^[:alnum:]-]+` "-" + | length 1 63 + }} IamDatabaseAuthenticationEnabled: type: bool default_value: true From df2bfdc41c5572577e6247143aa94c3e9662e6de Mon Sep 17 00:00:00 2001 From: Jordan Singer Date: Thu, 7 Dec 2023 11:48:04 -0600 Subject: [PATCH 3/3] update tests now that validity of resource property is fixed --- pkg/engine2/solution_context/decisions.go | 26 +++---- .../testdata/ecs_rds.dataflow-viz.yaml | 2 +- pkg/engine2/testdata/ecs_rds.expect.yaml | 10 +-- pkg/engine2/testdata/ecs_rds.iac-viz.yaml | 6 +- .../testdata/k8s_api.dataflow-viz.yaml | 10 +-- pkg/engine2/testdata/k8s_api.expect.yaml | 73 ++++++++++--------- pkg/engine2/testdata/k8s_api.iac-viz.yaml | 30 ++++---- 7 files changed, 78 insertions(+), 79 deletions(-) diff --git a/pkg/engine2/solution_context/decisions.go b/pkg/engine2/solution_context/decisions.go index 82ea43d6d..7e5758194 100644 --- a/pkg/engine2/solution_context/decisions.go +++ b/pkg/engine2/solution_context/decisions.go @@ -1,7 +1,7 @@ package solution_context import ( - "fmt" + "encoding/json" construct "github.com/klothoplatform/klotho/pkg/construct2" knowledgebase "github.com/klothoplatform/klotho/pkg/knowledge_base2" @@ -70,18 +70,16 @@ func (d PropertyValidationDecision) internal() {} func (d PropertyValidationDecision) MarshalJSON() ([]byte, error) { if d.Value != nil { - stringVal := `{ - "resource": "%s", - "property": "%s", - "value": "%s", - "error": "%s" - }` - return []byte(fmt.Sprintf(stringVal, d.Resource, d.Property.Details().Path, d.Value, d.Error)), nil + return json.Marshal(map[string]any{ + "resource": d.Resource, + "property": d.Property.Details().Path, + "value": d.Value, + "error": d.Error, + }) } - stringVal := `{ - "resource": "%s", - "property": "%s", - "error": "%s" - }` - return []byte(fmt.Sprintf(stringVal, d.Resource, d.Property.Details().Path, d.Error)), nil + return json.Marshal(map[string]any{ + "resource": d.Resource, + "property": d.Property.Details().Path, + "error": d.Error, + }) } diff --git a/pkg/engine2/testdata/ecs_rds.dataflow-viz.yaml b/pkg/engine2/testdata/ecs_rds.dataflow-viz.yaml index 801d089c0..63130dfb2 100755 --- a/pkg/engine2/testdata/ecs_rds.dataflow-viz.yaml +++ b/pkg/engine2/testdata/ecs_rds.dataflow-viz.yaml @@ -12,6 +12,6 @@ resources: parent: vpc/vpc-0 ecs_service/ecs_service_0 -> rds_instance/rds-instance-2: - path: aws:ecs_task_definition:ecs_service_0,aws:iam_role:ecs_service_0-rds-instance-2 + path: aws:ecs_task_definition:ecs_service_0,aws:iam_role:ecs_service_0-execution-role diff --git a/pkg/engine2/testdata/ecs_rds.expect.yaml b/pkg/engine2/testdata/ecs_rds.expect.yaml index aa5311ccb..608fb38ae 100755 --- a/pkg/engine2/testdata/ecs_rds.expect.yaml +++ b/pkg/engine2/testdata/ecs_rds.expect.yaml @@ -34,7 +34,7 @@ resources: rds-instance-2_RDS_ENDPOINT: aws:rds_instance:rds-instance-2#Endpoint rds-instance-2_RDS_PASSWORD: aws:rds_instance:rds-instance-2#Password rds-instance-2_RDS_USERNAME: aws:rds_instance:rds-instance-2#Username - ExecutionRole: aws:iam_role:ecs_service_0-rds-instance-2 + ExecutionRole: aws:iam_role:ecs_service_0-execution-role Image: aws:ecr_image:ecs_service_0-image LogGroup: aws:log_group:ecs_service_0-log-group Memory: "512" @@ -46,12 +46,12 @@ resources: Region: aws:region:region-0 RequiresCompatibilities: - FARGATE - TaskRole: aws:iam_role:ecs_service_0-rds-instance-2 + TaskRole: aws:iam_role:ecs_service_0-execution-role aws:ecr_image:ecs_service_0-image: Context: . Dockerfile: ecs_service_0-image.Dockerfile Repo: aws:ecr_repo:ecr_repo-0 - aws:iam_role:ecs_service_0-rds-instance-2: + aws:iam_role:ecs_service_0-execution-role: AssumeRolePolicyDoc: Statement: - Action: @@ -209,11 +209,11 @@ edges: aws:ecs_service:ecs_service_0 -> aws:subnet:vpc-0:subnet-0: aws:ecs_service:ecs_service_0 -> aws:subnet:vpc-0:subnet-1: aws:ecs_task_definition:ecs_service_0 -> aws:ecr_image:ecs_service_0-image: - aws:ecs_task_definition:ecs_service_0 -> aws:iam_role:ecs_service_0-rds-instance-2: + aws:ecs_task_definition:ecs_service_0 -> aws:iam_role:ecs_service_0-execution-role: aws:ecs_task_definition:ecs_service_0 -> aws:log_group:ecs_service_0-log-group: aws:ecs_task_definition:ecs_service_0 -> aws:region:region-0: aws:ecr_image:ecs_service_0-image -> aws:ecr_repo:ecr_repo-0: - aws:iam_role:ecs_service_0-rds-instance-2 -> aws:rds_instance:rds-instance-2: + aws:iam_role:ecs_service_0-execution-role -> aws:rds_instance:rds-instance-2: aws:nat_gateway:subnet-2:subnet-0-route_table-nat_gateway -> aws:elastic_ip:subnet-0-route_table-nat_gateway-elastic_ip: aws:nat_gateway:subnet-2:subnet-0-route_table-nat_gateway -> aws:subnet:vpc-0:subnet-2: aws:subnet:vpc-0:subnet-2 -> aws:availability_zone:region-0:availability_zone-0: diff --git a/pkg/engine2/testdata/ecs_rds.iac-viz.yaml b/pkg/engine2/testdata/ecs_rds.iac-viz.yaml index 7e272c986..f29d858b6 100755 --- a/pkg/engine2/testdata/ecs_rds.iac-viz.yaml +++ b/pkg/engine2/testdata/ecs_rds.iac-viz.yaml @@ -59,8 +59,8 @@ resources: ecr_image/ecs_service_0-image: ecr_image/ecs_service_0-image -> ecr_repo/ecr_repo-0: - iam_role/ecs_service_0-rds-instance-2: - iam_role/ecs_service_0-rds-instance-2 -> rds_instance/rds-instance-2: + iam_role/ecs_service_0-execution-role: + iam_role/ecs_service_0-execution-role -> rds_instance/rds-instance-2: log_group/ecs_service_0-log-group: @@ -84,7 +84,7 @@ resources: ecs_task_definition/ecs_service_0: ecs_task_definition/ecs_service_0 -> ecr_image/ecs_service_0-image: - ecs_task_definition/ecs_service_0 -> iam_role/ecs_service_0-rds-instance-2: + ecs_task_definition/ecs_service_0 -> iam_role/ecs_service_0-execution-role: ecs_task_definition/ecs_service_0 -> log_group/ecs_service_0-log-group: ecs_task_definition/ecs_service_0 -> region/region-0: diff --git a/pkg/engine2/testdata/k8s_api.dataflow-viz.yaml b/pkg/engine2/testdata/k8s_api.dataflow-viz.yaml index 5a612b6cc..e1a2a5a47 100755 --- a/pkg/engine2/testdata/k8s_api.dataflow-viz.yaml +++ b/pkg/engine2/testdata/k8s_api.dataflow-viz.yaml @@ -4,11 +4,11 @@ resources: parent: eks_cluster/eks_cluster-0 - load_balancer/rest-api-4-integbcc77100: + load_balancer/rest-api-4-integ6897f0b9: parent: vpc/vpc-0 - load_balancer/rest-api-4-integbcc77100 -> kubernetes:pod:eks_cluster-0/pod2: - path: aws:load_balancer_listener:rest_api_4_integration_0-pod2,aws:target_group:rest-api-4-integbcc77100,kubernetes:target_group_binding:restapi4integration0-pod2,kubernetes:service:restapi4integration0-pod2 + load_balancer/rest-api-4-integ6897f0b9 -> kubernetes:pod:eks_cluster-0/pod2: + path: aws:load_balancer_listener:rest_api_4_integration_0-eks_cluster-0,aws:target_group:rest-api-4-integ6897f0b9,kubernetes:target_group_binding:restapi4integration0-ekscluster-0,kubernetes:service:restapi4integration0-pod2 kubernetes:helm_chart:eks_cluster-0/metricsserver: @@ -31,7 +31,7 @@ resources: aws:api_integration:rest_api_4/rest_api_4_integration_0: parent: rest_api/rest_api_4 - aws:api_integration:rest_api_4/rest_api_4_integration_0 -> load_balancer/rest-api-4-integbcc77100: - path: aws:vpc_link:rest_api_4_integration_0-pod2 + aws:api_integration:rest_api_4/rest_api_4_integration_0 -> load_balancer/rest-api-4-integ6897f0b9: + path: aws:vpc_link:rest_api_4_integration_0-eks_cluster-0 diff --git a/pkg/engine2/testdata/k8s_api.expect.yaml b/pkg/engine2/testdata/k8s_api.expect.yaml index 2000c7f9c..a43e5e6df 100755 --- a/pkg/engine2/testdata/k8s_api.expect.yaml +++ b/pkg/engine2/testdata/k8s_api.expect.yaml @@ -111,26 +111,26 @@ resources: Resource: aws:api_resource:rest_api_4:api_resource-0 RestApi: aws:rest_api:rest_api_4 Route: /{proxy+} - Target: aws:load_balancer:rest-api-4-integbcc77100 + Target: aws:load_balancer:rest-api-4-integ6897f0b9 Type: HTTP_PROXY Uri: aws:api_integration:rest_api_4:rest_api_4_integration_0#LbUri - VpcLink: aws:vpc_link:rest_api_4_integration_0-pod2 - aws:vpc_link:rest_api_4_integration_0-pod2: - Target: aws:load_balancer:rest-api-4-integbcc77100 - aws:load_balancer:rest-api-4-integbcc77100: + VpcLink: aws:vpc_link:rest_api_4_integration_0-eks_cluster-0 + aws:vpc_link:rest_api_4_integration_0-eks_cluster-0: + Target: aws:load_balancer:rest-api-4-integ6897f0b9 + aws:load_balancer:rest-api-4-integ6897f0b9: Scheme: internal Subnets: - aws:subnet:vpc-0:subnet-0 - aws:subnet:vpc-0:subnet-1 Type: network - aws:load_balancer_listener:rest_api_4_integration_0-pod2: + aws:load_balancer_listener:rest_api_4_integration_0-eks_cluster-0: DefaultActions: - - TargetGroup: aws:target_group:rest-api-4-integbcc77100 + - TargetGroup: aws:target_group:rest-api-4-integ6897f0b9 Type: forward - LoadBalancer: aws:load_balancer:rest-api-4-integbcc77100 + LoadBalancer: aws:load_balancer:rest-api-4-integ6897f0b9 Port: 80 Protocol: TCP - aws:target_group:rest-api-4-integbcc77100: + aws:target_group:rest-api-4-integ6897f0b9: HealthCheck: Enabled: true HealthyThreshold: 5 @@ -143,19 +143,19 @@ resources: Protocol: TCP TargetType: ip Vpc: aws:vpc:vpc-0 - kubernetes:target_group_binding:restapi4integration0-pod2: + kubernetes:target_group_binding:restapi4integration0-ekscluster-0: Object: apiVersion: elbv2.k8s.aws/v1beta1 kind: TargetGroupBinding metadata: labels: - KLOTHO_ID_LABEL: restapi4integration0-pod2 - name: restapi4integration0-pod2 + KLOTHO_ID_LABEL: restapi4integration0-ekscluster-0 + name: restapi4integration0-ekscluster-0 spec: serviceRef: name: restapi4integration0-pod2 port: 80 - targetGroupARN: aws:target_group:rest-api-4-integbcc77100#Arn + targetGroupARN: aws:target_group:rest-api-4-integ6897f0b9#Arn kubernetes:helm_chart:eks_cluster-0:aws-load-balancer-controller: Chart: aws-load-balancer-controller Cluster: aws:eks_cluster:eks_cluster-0 @@ -285,12 +285,12 @@ resources: - ec2.amazonaws.com Version: "2012-10-17" ManagedPolicies: + - arn:aws:iam::aws:policy/AmazonEKSWorkerNodePolicy + - arn:aws:iam::aws:policy/AmazonEC2ContainerRegistryReadOnly - arn:aws:iam::aws:policy/AmazonEKS_CNI_Policy - arn:aws:iam::aws:policy/AWSCloudMapFullAccess - arn:aws:iam::aws:policy/CloudWatchAgentServerPolicy - arn:aws:iam::aws:policy/AmazonSSMManagedInstanceCore - - arn:aws:iam::aws:policy/AmazonEKSWorkerNodePolicy - - arn:aws:iam::aws:policy/AmazonEC2ContainerRegistryReadOnly aws:iam_role:pod2: AssumeRolePolicyDoc: Statement: @@ -503,27 +503,27 @@ resources: ToPort: 0 IngressRules: - CidrBlocks: - - 10.0.128.0/18 - Description: Allow ingress traffic from ip addresses within the subnet subnet-0 + - 0.0.0.0/0 + Description: Allows ingress traffic from the EKS control plane + FromPort: 9443 + Protocol: TCP + ToPort: 9443 + - Description: Allow ingress traffic from within the same security group FromPort: 0 Protocol: "-1" + Self: true ToPort: 0 - CidrBlocks: - - 10.0.192.0/18 - Description: Allow ingress traffic from ip addresses within the subnet subnet-1 + - 10.0.128.0/18 + Description: Allow ingress traffic from ip addresses within the subnet subnet-0 FromPort: 0 Protocol: "-1" ToPort: 0 - CidrBlocks: - - 0.0.0.0/0 - Description: Allows ingress traffic from the EKS control plane - FromPort: 9443 - Protocol: TCP - ToPort: 9443 - - Description: Allow ingress traffic from within the same security group + - 10.0.192.0/18 + Description: Allow ingress traffic from ip addresses within the subnet subnet-1 FromPort: 0 Protocol: "-1" - Self: true ToPort: 0 Vpc: aws:vpc:vpc-0 aws:route_table:subnet-0-route_table: @@ -609,16 +609,17 @@ edges: aws:api_resource:rest_api_4:api_resource-0 -> aws:api_integration:rest_api_4:rest_api_4_integration_0: aws:api_resource:rest_api_4:api_resource-0 -> aws:api_method:rest_api_4:rest_api_4_integration_0_method: aws:api_method:rest_api_4:rest_api_4_integration_0_method -> aws:api_integration:rest_api_4:rest_api_4_integration_0: - aws:api_integration:rest_api_4:rest_api_4_integration_0 -> aws:vpc_link:rest_api_4_integration_0-pod2: - aws:vpc_link:rest_api_4_integration_0-pod2 -> aws:load_balancer:rest-api-4-integbcc77100: - aws:load_balancer:rest-api-4-integbcc77100 -> aws:load_balancer_listener:rest_api_4_integration_0-pod2: - aws:load_balancer:rest-api-4-integbcc77100 -> aws:subnet:vpc-0:subnet-0: - aws:load_balancer:rest-api-4-integbcc77100 -> aws:subnet:vpc-0:subnet-1: - aws:load_balancer_listener:rest_api_4_integration_0-pod2 -> aws:target_group:rest-api-4-integbcc77100: - aws:target_group:rest-api-4-integbcc77100 -> kubernetes:target_group_binding:restapi4integration0-pod2: - kubernetes:target_group_binding:restapi4integration0-pod2 -> aws:eks_cluster:eks_cluster-0: - kubernetes:target_group_binding:restapi4integration0-pod2 -> kubernetes:helm_chart:eks_cluster-0:aws-load-balancer-controller: - kubernetes:target_group_binding:restapi4integration0-pod2 -> kubernetes:service:restapi4integration0-pod2: + aws:api_integration:rest_api_4:rest_api_4_integration_0 -> aws:vpc_link:rest_api_4_integration_0-eks_cluster-0: + aws:vpc_link:rest_api_4_integration_0-eks_cluster-0 -> aws:load_balancer:rest-api-4-integ6897f0b9: + aws:load_balancer:rest-api-4-integ6897f0b9 -> aws:load_balancer_listener:rest_api_4_integration_0-eks_cluster-0: + aws:load_balancer:rest-api-4-integ6897f0b9 -> aws:subnet:vpc-0:subnet-0: + aws:load_balancer:rest-api-4-integ6897f0b9 -> aws:subnet:vpc-0:subnet-1: + aws:load_balancer_listener:rest_api_4_integration_0-eks_cluster-0 -> aws:target_group:rest-api-4-integ6897f0b9: + aws:target_group:rest-api-4-integ6897f0b9 -> kubernetes:target_group_binding:restapi4integration0-ekscluster-0: + kubernetes:target_group_binding:restapi4integration0-ekscluster-0 -> aws:eks_cluster:eks_cluster-0: + ? kubernetes:target_group_binding:restapi4integration0-ekscluster-0 -> kubernetes:helm_chart:eks_cluster-0:aws-load-balancer-controller + : + kubernetes:target_group_binding:restapi4integration0-ekscluster-0 -> kubernetes:service:restapi4integration0-pod2: kubernetes:helm_chart:eks_cluster-0:aws-load-balancer-controller -> aws:eks_cluster:eks_cluster-0: kubernetes:helm_chart:eks_cluster-0:aws-load-balancer-controller -> aws:region:region-0: ? kubernetes:helm_chart:eks_cluster-0:aws-load-balancer-controller -> kubernetes:service_account:eks_cluster-0:aws-load-balancer-controller diff --git a/pkg/engine2/testdata/k8s_api.iac-viz.yaml b/pkg/engine2/testdata/k8s_api.iac-viz.yaml index 30e808559..f9fe7cc36 100755 --- a/pkg/engine2/testdata/k8s_api.iac-viz.yaml +++ b/pkg/engine2/testdata/k8s_api.iac-viz.yaml @@ -48,9 +48,9 @@ resources: aws:api_resource:rest_api_4/api_resource-0: aws:api_resource:rest_api_4/api_resource-0 -> rest_api/rest_api_4: - load_balancer/rest-api-4-integbcc77100: - load_balancer/rest-api-4-integbcc77100 -> aws:subnet:vpc-0/subnet-0: - load_balancer/rest-api-4-integbcc77100 -> aws:subnet:vpc-0/subnet-1: + load_balancer/rest-api-4-integ6897f0b9: + load_balancer/rest-api-4-integ6897f0b9 -> aws:subnet:vpc-0/subnet-0: + load_balancer/rest-api-4-integ6897f0b9 -> aws:subnet:vpc-0/subnet-1: iam_role/aws-load-balancer-controller: iam_role/aws-load-balancer-controller -> iam_oidc_provider/eks_cluster-0: @@ -84,8 +84,8 @@ resources: aws:api_method:rest_api_4/rest_api_4_integration_0_method -> aws:api_resource:rest_api_4/api_resource-0: aws:api_method:rest_api_4/rest_api_4_integration_0_method -> rest_api/rest_api_4: - vpc_link/rest_api_4_integration_0-pod2: - vpc_link/rest_api_4_integration_0-pod2 -> load_balancer/rest-api-4-integbcc77100: + vpc_link/rest_api_4_integration_0-eks_cluster-0: + vpc_link/rest_api_4_integration_0-eks_cluster-0 -> load_balancer/rest-api-4-integ6897f0b9: kubernetes:service_account:eks_cluster-0/aws-load-balancer-controller: kubernetes:service_account:eks_cluster-0/aws-load-balancer-controller -> eks_cluster/eks_cluster-0: @@ -115,9 +115,9 @@ resources: aws:api_integration:rest_api_4/rest_api_4_integration_0 -> aws:api_method:rest_api_4/rest_api_4_integration_0_method: aws:api_integration:rest_api_4/rest_api_4_integration_0 -> aws:api_resource:rest_api_4/api_resource-0: aws:api_integration:rest_api_4/rest_api_4_integration_0 -> rest_api/rest_api_4: - aws:api_integration:rest_api_4/rest_api_4_integration_0 -> vpc_link/rest_api_4_integration_0-pod2: + aws:api_integration:rest_api_4/rest_api_4_integration_0 -> vpc_link/rest_api_4_integration_0-eks_cluster-0: - target_group/rest-api-4-integbcc77100: + target_group/rest-api-4-integ6897f0b9: kubernetes:helm_chart:eks_cluster-0/aws-load-balancer-controller: kubernetes:helm_chart:eks_cluster-0/aws-load-balancer-controller -> eks_cluster/eks_cluster-0: @@ -155,11 +155,11 @@ resources: aws:api_deployment:rest_api_4/api_deployment-0 -> aws:api_method:rest_api_4/rest_api_4_integration_0_method: aws:api_deployment:rest_api_4/api_deployment-0 -> rest_api/rest_api_4: - kubernetes:target_group_binding/restapi4integration0-pod2: - kubernetes:target_group_binding/restapi4integration0-pod2 -> eks_cluster/eks_cluster-0: - kubernetes:target_group_binding/restapi4integration0-pod2 -> target_group/rest-api-4-integbcc77100: - kubernetes:target_group_binding/restapi4integration0-pod2 -> kubernetes:helm_chart:eks_cluster-0/aws-load-balancer-controller: - kubernetes:target_group_binding/restapi4integration0-pod2 -> kubernetes:service/restapi4integration0-pod2: + kubernetes:target_group_binding/restapi4integration0-ekscluster-0: + kubernetes:target_group_binding/restapi4integration0-ekscluster-0 -> eks_cluster/eks_cluster-0: + kubernetes:target_group_binding/restapi4integration0-ekscluster-0 -> target_group/rest-api-4-integ6897f0b9: + kubernetes:target_group_binding/restapi4integration0-ekscluster-0 -> kubernetes:helm_chart:eks_cluster-0/aws-load-balancer-controller: + kubernetes:target_group_binding/restapi4integration0-ekscluster-0 -> kubernetes:service/restapi4integration0-pod2: kubernetes:manifest/fluent-bit: kubernetes:manifest/fluent-bit -> eks_cluster/eks_cluster-0: @@ -191,9 +191,9 @@ resources: route_table_association/subnet-0-subnet-0-route_table -> route_table/subnet-0-route_table: route_table_association/subnet-0-subnet-0-route_table -> aws:subnet:vpc-0/subnet-0: - load_balancer_listener/rest_api_4_integration_0-pod2: - load_balancer_listener/rest_api_4_integration_0-pod2 -> load_balancer/rest-api-4-integbcc77100: - load_balancer_listener/rest_api_4_integration_0-pod2 -> target_group/rest-api-4-integbcc77100: + load_balancer_listener/rest_api_4_integration_0-eks_cluster-0: + load_balancer_listener/rest_api_4_integration_0-eks_cluster-0 -> load_balancer/rest-api-4-integ6897f0b9: + load_balancer_listener/rest_api_4_integration_0-eks_cluster-0 -> target_group/rest-api-4-integ6897f0b9: iam_role_policy_attachment/aws-load-balancer-controller-iam_policy-0: iam_role_policy_attachment/aws-load-balancer-controller-iam_policy-0 -> iam_policy/iam_policy-0: