From 7858cd71f5ee737c5beff0a2d8bb4e2167fb2444 Mon Sep 17 00:00:00 2001 From: Jordan Singer Date: Mon, 4 Dec 2023 14:52:46 -0600 Subject: [PATCH] pr comments --- .../operational_eval/dependency_capture.go | 5 +- pkg/engine2/operational_eval/graph.go | 3 - .../operational_eval/vertex_property.go | 6 +- .../operational_configuration.go | 3 +- .../operational_rule/operational_step.go | 25 +++---- .../properties/any_property.go | 2 +- .../properties/bool_property.go | 2 +- .../properties/float_property.go | 2 +- .../properties/int_property.go | 2 +- .../properties/list_property.go | 2 +- .../properties/map_property.go | 2 +- .../properties/resource_property.go | 2 +- .../properties/set_property.go | 2 +- .../properties/string_property.go | 2 +- pkg/knowledge_base2/reader/embed.go | 3 + pkg/knowledge_base2/reader/properties.go | 74 ++++++++++--------- pkg/knowledge_base2/resource_template.go | 2 +- 17 files changed, 68 insertions(+), 71 deletions(-) diff --git a/pkg/engine2/operational_eval/dependency_capture.go b/pkg/engine2/operational_eval/dependency_capture.go index fd6a63aa4..564ec71b2 100644 --- a/pkg/engine2/operational_eval/dependency_capture.go +++ b/pkg/engine2/operational_eval/dependency_capture.go @@ -182,9 +182,8 @@ func (ctx *fauxConfigContext) ExecutePropertyRule( if propRule.Value != nil { ctx.ExecuteValue(propRule.Value, data) } - if len(propRule.Step.Resources) > 0 { - errs = errors.Join(errs, ctx.executeOpStep(data, propRule.Step)) - } + errs = errors.Join(errs, ctx.executeOpStep(data, propRule.Step)) + return errs } diff --git a/pkg/engine2/operational_eval/graph.go b/pkg/engine2/operational_eval/graph.go index cbfe7c742..66dcdad2c 100644 --- a/pkg/engine2/operational_eval/graph.go +++ b/pkg/engine2/operational_eval/graph.go @@ -144,9 +144,6 @@ func (eval *Evaluator) resourceVertices( ) (graphChanges, error) { changes := newChanges() var errs error - if res.ID.Type == "iam_role" { - res.ID.Type = "iam_role" - } addProp := func(prop knowledgebase.Property) error { vertex := &propertyVertex{ Ref: construct.PropertyRef{Resource: res.ID, Property: prop.Details().Path}, diff --git a/pkg/engine2/operational_eval/vertex_property.go b/pkg/engine2/operational_eval/vertex_property.go index f9b44fbac..0600d5130 100644 --- a/pkg/engine2/operational_eval/vertex_property.go +++ b/pkg/engine2/operational_eval/vertex_property.go @@ -35,7 +35,7 @@ func (prop *propertyVertex) Dependencies(eval *Evaluator) (graphChanges, error) // Template can be nil when checking for dependencies from a propertyVertex when adding an edge template if prop.Template != nil { - _, _ = prop.Template.GetDefaultValue(propCtx.inner, resData) + _, _ = prop.Template.GetDefaultValue(propCtx, resData) details := prop.Template.Details() if opRule := details.OperationalRule; opRule != nil { if err := propCtx.ExecutePropertyRule(resData, *opRule); err != nil { @@ -286,12 +286,12 @@ func (v *propertyVertex) Ready(eval *Evaluator) (ReadyPriority, error) { return ReadyNow, nil } ptype := v.Template.Type() - if strings.Contains(ptype, "list") || strings.Contains(ptype, "set") { + if strings.HasPrefix(ptype, "list") || strings.HasPrefix(ptype, "set") { // never sure when a list/set is ready - it'll just be appended to by edges through // `v.EdgeRules` return NotReadyHigh, nil } - if strings.Contains(ptype, "map") && len(v.Template.SubProperties()) == 0 { + if strings.HasPrefix(ptype, "map") && len(v.Template.SubProperties()) == 0 { // maps without sub-properties (ie, not objects) are also appended to by edges return NotReadyHigh, nil } diff --git a/pkg/engine2/operational_rule/operational_configuration.go b/pkg/engine2/operational_rule/operational_configuration.go index 0db22858e..a4a880854 100644 --- a/pkg/engine2/operational_rule/operational_configuration.go +++ b/pkg/engine2/operational_rule/operational_configuration.go @@ -17,7 +17,6 @@ func (ctx OperationalRuleContext) HandleConfigurationRule(config knowledgebase.C if err != nil { return fmt.Errorf("resource %s not found: %w", res, err) } - action := "add" resolvedField := config.Config.Field err = dyn.ExecuteDecode(config.Config.Field, ctx.Data, &resolvedField) @@ -26,7 +25,7 @@ func (ctx OperationalRuleContext) HandleConfigurationRule(config knowledgebase.C } config.Config.Field = resolvedField - err = solution_context.ConfigureResource(ctx.Solution, resource, config.Config, ctx.Data, action) + err = solution_context.ConfigureResource(ctx.Solution, resource, config.Config, ctx.Data, "add") if err != nil { return err } diff --git a/pkg/engine2/operational_rule/operational_step.go b/pkg/engine2/operational_rule/operational_step.go index 582da7202..72948a260 100644 --- a/pkg/engine2/operational_rule/operational_step.go +++ b/pkg/engine2/operational_rule/operational_step.go @@ -270,25 +270,20 @@ func (ctx OperationalRuleContext) SetField(resource, fieldResource *construct.Re return nil } - removeCurrentValue := func(res any) error { - switch val := res.(type) { - case construct.ResourceId: - if val != fieldResource.ID { - return removeResource(val) - } - case construct.PropertyRef: - if val.Resource != fieldResource.ID { - return removeResource(val.Resource) - } - } - return nil - } - propVal, err := resource.GetProperty(path) if err != nil { zap.S().Debugf("property %s not found on resource %s", path, resource.ID) } - err = removeCurrentValue(propVal) + switch val := propVal.(type) { + case construct.ResourceId: + if val != fieldResource.ID { + err = removeResource(val) + } + case construct.PropertyRef: + if val.Resource != fieldResource.ID { + err = removeResource(val.Resource) + } + } if err != nil { return err } diff --git a/pkg/knowledge_base2/properties/any_property.go b/pkg/knowledge_base2/properties/any_property.go index 89ab0af8d..386105a9c 100644 --- a/pkg/knowledge_base2/properties/any_property.go +++ b/pkg/knowledge_base2/properties/any_property.go @@ -40,7 +40,7 @@ func (a *AnyProperty) Clone() knowledgebase.Property { return &clone } -func (a *AnyProperty) GetDefaultValue(ctx knowledgebase.DynamicValueContext, data knowledgebase.DynamicValueData) (any, error) { +func (a *AnyProperty) GetDefaultValue(ctx knowledgebase.DynamicContext, data knowledgebase.DynamicValueData) (any, error) { if a.DefaultValue == nil { return nil, nil } diff --git a/pkg/knowledge_base2/properties/bool_property.go b/pkg/knowledge_base2/properties/bool_property.go index 2a047b80d..bda5ef853 100644 --- a/pkg/knowledge_base2/properties/bool_property.go +++ b/pkg/knowledge_base2/properties/bool_property.go @@ -47,7 +47,7 @@ func (b *BoolProperty) Details() *knowledgebase.PropertyDetails { return &b.PropertyDetails } -func (b *BoolProperty) GetDefaultValue(ctx knowledgebase.DynamicValueContext, data knowledgebase.DynamicValueData) (any, error) { +func (b *BoolProperty) GetDefaultValue(ctx knowledgebase.DynamicContext, data knowledgebase.DynamicValueData) (any, error) { if b.DefaultValue == nil { return nil, nil } diff --git a/pkg/knowledge_base2/properties/float_property.go b/pkg/knowledge_base2/properties/float_property.go index 280ad09e9..92a3f3bd6 100644 --- a/pkg/knowledge_base2/properties/float_property.go +++ b/pkg/knowledge_base2/properties/float_property.go @@ -50,7 +50,7 @@ func (f *FloatProperty) Clone() knowledgebase.Property { return &clone } -func (f *FloatProperty) GetDefaultValue(ctx knowledgebase.DynamicValueContext, data knowledgebase.DynamicValueData) (any, error) { +func (f *FloatProperty) GetDefaultValue(ctx knowledgebase.DynamicContext, data knowledgebase.DynamicValueData) (any, error) { if f.DefaultValue == nil { return nil, nil } diff --git a/pkg/knowledge_base2/properties/int_property.go b/pkg/knowledge_base2/properties/int_property.go index 0546fa9a7..30a59b212 100644 --- a/pkg/knowledge_base2/properties/int_property.go +++ b/pkg/knowledge_base2/properties/int_property.go @@ -49,7 +49,7 @@ func (i *IntProperty) Clone() knowledgebase.Property { return &clone } -func (i *IntProperty) GetDefaultValue(ctx knowledgebase.DynamicValueContext, data knowledgebase.DynamicValueData) (any, error) { +func (i *IntProperty) GetDefaultValue(ctx knowledgebase.DynamicContext, data knowledgebase.DynamicValueData) (any, error) { if i.DefaultValue == nil { return nil, nil } diff --git a/pkg/knowledge_base2/properties/list_property.go b/pkg/knowledge_base2/properties/list_property.go index 3d6501a56..8ebe089e2 100644 --- a/pkg/knowledge_base2/properties/list_property.go +++ b/pkg/knowledge_base2/properties/list_property.go @@ -97,7 +97,7 @@ func (l *ListProperty) Clone() knowledgebase.Property { return &clone } -func (list *ListProperty) GetDefaultValue(ctx knowledgebase.DynamicValueContext, data knowledgebase.DynamicValueData) (any, error) { +func (list *ListProperty) GetDefaultValue(ctx knowledgebase.DynamicContext, data knowledgebase.DynamicValueData) (any, error) { if list.DefaultValue == nil { return nil, nil } diff --git a/pkg/knowledge_base2/properties/map_property.go b/pkg/knowledge_base2/properties/map_property.go index 2ff4e5cec..501b5a8eb 100644 --- a/pkg/knowledge_base2/properties/map_property.go +++ b/pkg/knowledge_base2/properties/map_property.go @@ -79,7 +79,7 @@ func (m *MapProperty) Clone() knowledgebase.Property { return &clone } -func (m *MapProperty) GetDefaultValue(ctx knowledgebase.DynamicValueContext, data knowledgebase.DynamicValueData) (any, error) { +func (m *MapProperty) GetDefaultValue(ctx knowledgebase.DynamicContext, data knowledgebase.DynamicValueData) (any, error) { if m.DefaultValue == nil { return nil, nil } diff --git a/pkg/knowledge_base2/properties/resource_property.go b/pkg/knowledge_base2/properties/resource_property.go index 686df6c3d..2a6285e61 100644 --- a/pkg/knowledge_base2/properties/resource_property.go +++ b/pkg/knowledge_base2/properties/resource_property.go @@ -59,7 +59,7 @@ func (r *ResourceProperty) Clone() knowledgebase.Property { return &clone } -func (r *ResourceProperty) GetDefaultValue(ctx knowledgebase.DynamicValueContext, data knowledgebase.DynamicValueData) (any, error) { +func (r *ResourceProperty) GetDefaultValue(ctx knowledgebase.DynamicContext, data knowledgebase.DynamicValueData) (any, error) { if r.DefaultValue == nil { return nil, nil } diff --git a/pkg/knowledge_base2/properties/set_property.go b/pkg/knowledge_base2/properties/set_property.go index b5d2b60bc..85534573f 100644 --- a/pkg/knowledge_base2/properties/set_property.go +++ b/pkg/knowledge_base2/properties/set_property.go @@ -82,7 +82,7 @@ func (s *SetProperty) Clone() knowledgebase.Property { return &clone } -func (s *SetProperty) GetDefaultValue(ctx knowledgebase.DynamicValueContext, data knowledgebase.DynamicValueData) (any, error) { +func (s *SetProperty) GetDefaultValue(ctx knowledgebase.DynamicContext, data knowledgebase.DynamicValueData) (any, error) { if s.DefaultValue == nil { return nil, nil } diff --git a/pkg/knowledge_base2/properties/string_property.go b/pkg/knowledge_base2/properties/string_property.go index 8531e6203..1f2935c71 100644 --- a/pkg/knowledge_base2/properties/string_property.go +++ b/pkg/knowledge_base2/properties/string_property.go @@ -51,7 +51,7 @@ func (s *StringProperty) Clone() knowledgebase.Property { return &clone } -func (s *StringProperty) GetDefaultValue(ctx knowledgebase.DynamicValueContext, data knowledgebase.DynamicValueData) (any, error) { +func (s *StringProperty) GetDefaultValue(ctx knowledgebase.DynamicContext, data knowledgebase.DynamicValueData) (any, error) { if s.DefaultValue == nil { return nil, nil } diff --git a/pkg/knowledge_base2/reader/embed.go b/pkg/knowledge_base2/reader/embed.go index c6880b20d..e2736c49d 100644 --- a/pkg/knowledge_base2/reader/embed.go +++ b/pkg/knowledge_base2/reader/embed.go @@ -26,6 +26,9 @@ func NewKBFromFs(resources, edges, models fs.FS) (*knowledgebase.KnowledgeBase, } kbModels[name] = kbModel } + if errs != nil { + return nil, errs + } kb.Models = kbModels templates, err := TemplatesFromFs(resources, readerModels) if err != nil { diff --git a/pkg/knowledge_base2/reader/properties.go b/pkg/knowledge_base2/reader/properties.go index 9aab373c6..aba6e4d6a 100644 --- a/pkg/knowledge_base2/reader/properties.go +++ b/pkg/knowledge_base2/reader/properties.go @@ -13,8 +13,11 @@ import ( ) type ( + // Properties defines the structure of properties defined in yaml as a part of a template. Properties map[string]*Property + // Property defines the structure of a property defined in yaml as a part of a template. + // these fields must be exactly the union of all the fields in the different property types. Property struct { Name string `json:"name" yaml:"name"` // Type defines the type of the property @@ -97,45 +100,46 @@ func (p *Property) Convert() (knowledgebase.Property, error) { srcField := srcVal.Field(i) fieldName := srcVal.Type().Field(i).Name dstField := dstVal.FieldByName(fieldName) - if dstField.IsValid() && dstField.CanSet() { - // Skip nil pointers - if (srcField.Kind() == reflect.Ptr || srcField.Kind() == reflect.Interface) && srcField.IsNil() { - continue + if !dstField.IsValid() || !dstField.CanSet() { + continue + } + // Skip nil pointers + if (srcField.Kind() == reflect.Ptr || srcField.Kind() == reflect.Interface) && srcField.IsNil() { + continue + } + // Handle sub properties so we can recurse down the tree + if fieldName == "Properties" { + properties, ok := srcField.Interface().(Properties) + if !ok { + return nil, fmt.Errorf("invalid properties") } - // Handle sub properties so we can recurse down the tree - if fieldName == "Properties" { - properties, ok := srcField.Interface().(Properties) - if !ok { - return nil, fmt.Errorf("invalid properties") - } - var errs error - props := knowledgebase.Properties{} - for name, prop := range properties { - propertyType, err := prop.Convert() - if err != nil { - errs = fmt.Errorf("%w\n%s", errs, err.Error()) - continue - } - props[name] = propertyType - } - if errs != nil { - return nil, errs + var errs error + props := knowledgebase.Properties{} + for name, prop := range properties { + propertyType, err := prop.Convert() + if err != nil { + errs = fmt.Errorf("%w\n%s", errs, err.Error()) + continue } - dstField.Set(reflect.ValueOf(props)) - continue + props[name] = propertyType } + if errs != nil { + return nil, errs + } + dstField.Set(reflect.ValueOf(props)) + continue + } - if dstField.Type() == srcField.Type() { - dstField.Set(srcField) - } else { - if conversion, found := fieldConversion[fieldName]; found { - err := conversion(srcField, p, propertyType) - if err != nil { - return nil, err - } - } else { - return nil, fmt.Errorf("invalid property type %s", fieldName) + if dstField.Type() == srcField.Type() { + dstField.Set(srcField) + } else { + if conversion, found := fieldConversion[fieldName]; found { + err := conversion(srcField, p, propertyType) + if err != nil { + return nil, err } + } else { + return nil, fmt.Errorf("invalid property type %s", fieldName) } } } @@ -151,7 +155,6 @@ func setChildPaths(property *Property, currPath string) { path := currPath + "." + name child.Path = path setChildPaths(child, path) - property.Properties[name] = child } } @@ -172,6 +175,7 @@ func (p *Property) Clone() *Property { return &cloned } +// fieldConversion is a map providing functionality on how to convert inputs into our internal types if they are not inherently the same structure var fieldConversion = map[string]func(val reflect.Value, p *Property, kp knowledgebase.Property) error{ "SanitizeTmpl": func(val reflect.Value, p *Property, kp knowledgebase.Property) error { sanitizeTmpl, ok := val.Interface().(string) diff --git a/pkg/knowledge_base2/resource_template.go b/pkg/knowledge_base2/resource_template.go index 25d1a19ac..859349e5d 100644 --- a/pkg/knowledge_base2/resource_template.go +++ b/pkg/knowledge_base2/resource_template.go @@ -79,7 +79,7 @@ type ( // Type returns the string representation of the type of the property, as it should appear in the resource template Type() string // GetDefaultValue returns the default value for the property, pertaining to the specific data being passed in for execution - GetDefaultValue(ctx DynamicValueContext, data DynamicValueData) (any, error) + 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(value any, properties construct.Properties) error // SubProperties returns the sub properties of the property, if any. This is used for properties that are complex structures,