From 4dbfd67ea95e9eea9778da7d7062b53fecbfe668 Mon Sep 17 00:00:00 2001 From: Lucas Bajolet Date: Thu, 24 Aug 2023 15:29:11 -0400 Subject: [PATCH 1/6] hcl2template: remove unused shouldContinue bool Not sure why this was defined and returned, but the value was set, but never used, as such this is not useful to keep in the code, so let's simplify this now. --- hcl2template/types.packer_config.go | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/hcl2template/types.packer_config.go b/hcl2template/types.packer_config.go index 819107e86b3..9ef8de242e4 100644 --- a/hcl2template/types.packer_config.go +++ b/hcl2template/types.packer_config.go @@ -380,7 +380,7 @@ func (cfg *PackerConfig) evaluateDatasources(skipExecution bool) hcl.Diagnostics // Now that most of our data sources have been started and executed, we can // try to execute the ones that depend on other data sources. for ref := range dependencies { - _, moreDiags, _ := cfg.recursivelyEvaluateDatasources(ref, dependencies, skipExecution, 0) + _, moreDiags := cfg.recursivelyEvaluateDatasources(ref, dependencies, skipExecution, 0) // Deduplicate diagnostics to prevent recursion messes. cleanedDiags := map[string]*hcl.Diagnostic{} for _, diag := range moreDiags { @@ -395,10 +395,9 @@ func (cfg *PackerConfig) evaluateDatasources(skipExecution bool) hcl.Diagnostics return diags } -func (cfg *PackerConfig) recursivelyEvaluateDatasources(ref DatasourceRef, dependencies map[DatasourceRef][]DatasourceRef, skipExecution bool, depth int) (map[DatasourceRef][]DatasourceRef, hcl.Diagnostics, bool) { +func (cfg *PackerConfig) recursivelyEvaluateDatasources(ref DatasourceRef, dependencies map[DatasourceRef][]DatasourceRef, skipExecution bool, depth int) (map[DatasourceRef][]DatasourceRef, hcl.Diagnostics) { var diags hcl.Diagnostics var moreDiags hcl.Diagnostics - shouldContinue := true if depth > 10 { // Add a comment about recursion. @@ -410,7 +409,7 @@ func (cfg *PackerConfig) recursivelyEvaluateDatasources(ref DatasourceRef, depen "other data sources, or your data sources have a cyclic " + "dependency. Please simplify your config to continue. ", }) - return dependencies, diags, false + return dependencies, diags } ds := cfg.Datasources[ref] @@ -421,11 +420,11 @@ func (cfg *PackerConfig) recursivelyEvaluateDatasources(ref DatasourceRef, depen // If this dependency is not in the map, it means we've already // launched and executed this datasource. Otherwise, it means // we still need to run it. RECURSION TIME!! - dependencies, moreDiags, shouldContinue = cfg.recursivelyEvaluateDatasources(dep, dependencies, skipExecution, depth) + dependencies, moreDiags = cfg.recursivelyEvaluateDatasources(dep, dependencies, skipExecution, depth) diags = append(diags, moreDiags...) if moreDiags.HasErrors() { diags = append(diags, moreDiags...) - return dependencies, diags, shouldContinue + return dependencies, diags } } } @@ -435,14 +434,14 @@ func (cfg *PackerConfig) recursivelyEvaluateDatasources(ref DatasourceRef, depen datasource, startDiags := cfg.startDatasource(cfg.parser.PluginConfig.DataSources, ref, true) if startDiags.HasErrors() { diags = append(diags, startDiags...) - return dependencies, diags, shouldContinue + return dependencies, diags } if skipExecution { placeholderValue := cty.UnknownVal(hcldec.ImpliedType(datasource.OutputSpec())) ds.value = placeholderValue cfg.Datasources[ref] = ds - return dependencies, diags, shouldContinue + return dependencies, diags } opts, _ := decodeHCL2Spec(ds.block.Body, cfg.EvalContext(DatasourceContext, nil), datasource) @@ -455,14 +454,14 @@ func (cfg *PackerConfig) recursivelyEvaluateDatasources(ref DatasourceRef, depen Subject: &cfg.Datasources[ref].block.DefRange, Severity: hcl.DiagError, }) - return dependencies, diags, shouldContinue + return dependencies, diags } ds.value = realValue cfg.Datasources[ref] = ds // remove ref from the dependencies map. delete(dependencies, ref) - return dependencies, diags, shouldContinue + return dependencies, diags } // getCoreBuildProvisioners takes a list of provisioner block, starts according From be2424949e2dd51d76cb30b6f402e51b621ffa8f Mon Sep 17 00:00:00 2001 From: Lucas Bajolet Date: Thu, 24 Aug 2023 16:09:25 -0400 Subject: [PATCH 2/6] hcl2template: simplify datasource evaluation Since datasources are recursively evaluated depending on their dependencies, we don't need to pre-execute those that depend on nothing, as the recursive traversal of the datasources will take care of that for us. --- hcl2template/types.packer_config.go | 46 ++--------------------------- 1 file changed, 3 insertions(+), 43 deletions(-) diff --git a/hcl2template/types.packer_config.go b/hcl2template/types.packer_config.go index 9ef8de242e4..39283e59075 100644 --- a/hcl2template/types.packer_config.go +++ b/hcl2template/types.packer_config.go @@ -311,8 +311,8 @@ func (cfg *PackerConfig) evaluateDatasources(skipExecution bool) hcl.Diagnostics // source in any of its input expressions. If so, skip evaluating it for // now, and add it to a list of datasources to evaluate again, later, // with the datasources in its context. - // This is essentially creating a very primitive DAG just for data - // source interdependencies. + dependencies[ref] = []DatasourceRef{} + block := ds.block body := block.Body attrs, _ := body.JustAttributes() @@ -330,51 +330,11 @@ func (cfg *PackerConfig) evaluateDatasources(skipExecution bool) hcl.Diagnostics Name: v[2].(hcl.TraverseAttr).Name, } log.Printf("The data source %#v depends on datasource %#v", ref, dependsOn) - if dependencies[ref] != nil { - dependencies[ref] = append(dependencies[ref], dependsOn) - } else { - dependencies[ref] = []DatasourceRef{dependsOn} - } + dependencies[ref] = append(dependencies[ref], dependsOn) skipFirstEval = true } } } - - // Now we have a list of data sources that depend on other data sources. - // Don't evaluate these; only evaluate data sources that we didn't - // mark as having dependencies. - if skipFirstEval { - continue - } - - datasource, startDiags := cfg.startDatasource(cfg.parser.PluginConfig.DataSources, ref, false) - diags = append(diags, startDiags...) - if diags.HasErrors() { - continue - } - - if skipExecution { - placeholderValue := cty.UnknownVal(hcldec.ImpliedType(datasource.OutputSpec())) - ds.value = placeholderValue - cfg.Datasources[ref] = ds - continue - } - - dsOpts, _ := decodeHCL2Spec(body, cfg.EvalContext(DatasourceContext, nil), datasource) - sp := packer.CheckpointReporter.AddSpan(ref.Type, "datasource", dsOpts) - realValue, err := datasource.Execute() - sp.End(err) - if err != nil { - diags = append(diags, &hcl.Diagnostic{ - Summary: err.Error(), - Subject: &cfg.Datasources[ref].block.DefRange, - Severity: hcl.DiagError, - }) - continue - } - - ds.value = realValue - cfg.Datasources[ref] = ds } // Now that most of our data sources have been started and executed, we can From 0c3adbf9acc40212c0b27c69bbfed1b50b70ae6f Mon Sep 17 00:00:00 2001 From: Lucas Bajolet Date: Thu, 24 Aug 2023 16:10:51 -0400 Subject: [PATCH 3/6] hcl2template: report localtion for cycle detection When a datasource fails to be evaluated because a cycle has been detected, we point out one of the links of the chain now so that users have a better idea of what to look at. --- hcl2template/types.packer_config.go | 1 + 1 file changed, 1 insertion(+) diff --git a/hcl2template/types.packer_config.go b/hcl2template/types.packer_config.go index 39283e59075..c9beb67bd0a 100644 --- a/hcl2template/types.packer_config.go +++ b/hcl2template/types.packer_config.go @@ -368,6 +368,7 @@ func (cfg *PackerConfig) recursivelyEvaluateDatasources(ref DatasourceRef, depen "sources. Either your data source depends on more than ten " + "other data sources, or your data sources have a cyclic " + "dependency. Please simplify your config to continue. ", + Subject: &(cfg.Datasources[ref]).block.DefRange, }) return dependencies, diags } From f4dcae25308be3e718533a27edff792f2d4a6c73 Mon Sep 17 00:00:00 2001 From: Lucas Bajolet Date: Thu, 24 Aug 2023 16:11:55 -0400 Subject: [PATCH 4/6] hcl2template: extract attr filter code from ds Datasources use their attribute's expressions to determine whether or not they depend on another datasource, in order to get the list of dependencies and execute them before executing a datasource. This code may be useful later on for figuring out the dependencies for any block, so we move this code to the utils.go file, and use this for datasources. --- hcl2template/types.packer_config.go | 31 ++++++++++------------------- hcl2template/utils.go | 20 +++++++++++++++++++ 2 files changed, 30 insertions(+), 21 deletions(-) diff --git a/hcl2template/types.packer_config.go b/hcl2template/types.packer_config.go index c9beb67bd0a..c344aa7b0b2 100644 --- a/hcl2template/types.packer_config.go +++ b/hcl2template/types.packer_config.go @@ -5,7 +5,6 @@ package hcl2template import ( "fmt" - "log" "sort" "strings" @@ -313,27 +312,17 @@ func (cfg *PackerConfig) evaluateDatasources(skipExecution bool) hcl.Diagnostics // with the datasources in its context. dependencies[ref] = []DatasourceRef{} - block := ds.block - body := block.Body - attrs, _ := body.JustAttributes() - - skipFirstEval := false - for _, attr := range attrs { - vars := attr.Expr.Variables() - for _, v := range vars { - // check whether the variable is a data source - if v.RootName() == "data" { - // construct, backwards, the data source type and name we - // need to evaluate before this one can be evaluated. - dependsOn := DatasourceRef{ - Type: v[1].(hcl.TraverseAttr).Name, - Name: v[2].(hcl.TraverseAttr).Name, - } - log.Printf("The data source %#v depends on datasource %#v", ref, dependsOn) - dependencies[ref] = append(dependencies[ref], dependsOn) - skipFirstEval = true - } + // Note: when looking at the expressions, we only need to care about + // attributes, as HCL2 expressions are not allowed in a block's labels. + vars := GetVarsByType(ds.block, "data") + for _, v := range vars { + // construct, backwards, the data source type and name we + // need to evaluate before this one can be evaluated. + dependsOn := DatasourceRef{ + Type: v[1].(hcl.TraverseAttr).Name, + Name: v[2].(hcl.TraverseAttr).Name, } + dependencies[ref] = append(dependencies[ref], dependsOn) } } diff --git a/hcl2template/utils.go b/hcl2template/utils.go index 98546a95456..a542a2467ac 100644 --- a/hcl2template/utils.go +++ b/hcl2template/utils.go @@ -186,3 +186,23 @@ func ConvertPluginConfigValueToHCLValue(v interface{}) (cty.Value, error) { } return buildValue, nil } + +func GetVarsByType(block *hcl.Block, topLevelLabels ...string) []hcl.Traversal { + attributes, _ := block.Body.JustAttributes() + + var vars []hcl.Traversal + + for _, attr := range attributes { + for _, variable := range attr.Expr.Variables() { + rootLabel := variable.RootName() + for _, label := range topLevelLabels { + if label == rootLabel { + vars = append(vars, variable) + break + } + } + } + } + + return vars +} From 5bae773a28d57f9130a0f4cb086fceac1674082c Mon Sep 17 00:00:00 2001 From: Lucas Bajolet Date: Fri, 25 Aug 2023 14:55:26 -0400 Subject: [PATCH 5/6] hcl2template: simplify startDatasource function --- hcl2template/types.datasource.go | 17 +++++++++-------- hcl2template/types.packer_config.go | 2 +- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/hcl2template/types.datasource.go b/hcl2template/types.datasource.go index 348d06f452c..23bc7c5900f 100644 --- a/hcl2template/types.datasource.go +++ b/hcl2template/types.datasource.go @@ -10,7 +10,6 @@ import ( "github.com/hashicorp/hcl/v2/hclsyntax" packersdk "github.com/hashicorp/packer-plugin-sdk/packer" hcl2shim "github.com/hashicorp/packer/hcl2template/shim" - "github.com/hashicorp/packer/packer" "github.com/zclconf/go-cty/cty" ) @@ -65,13 +64,15 @@ func (ds *Datasources) Values() (map[string]cty.Value, hcl.Diagnostics) { return res, diags } -func (cfg *PackerConfig) startDatasource(dataSourceStore packer.DatasourceStore, ref DatasourceRef, secondaryEvaluation bool) (packersdk.Datasource, hcl.Diagnostics) { +func (cfg *PackerConfig) startDatasource(ds DatasourceBlock) (packersdk.Datasource, hcl.Diagnostics) { var diags hcl.Diagnostics - block := cfg.Datasources[ref].block + block := ds.block + + dataSourceStore := cfg.parser.PluginConfig.DataSources if dataSourceStore == nil { diags = append(diags, &hcl.Diagnostic{ - Summary: "Unknown " + dataSourceLabel + " type " + ref.Type, + Summary: "Unknown " + dataSourceLabel + " type " + ds.Type, Subject: block.LabelRanges[0].Ptr(), Detail: "packer does not currently know any data source.", Severity: hcl.DiagError, @@ -79,9 +80,9 @@ func (cfg *PackerConfig) startDatasource(dataSourceStore packer.DatasourceStore, return nil, diags } - if !dataSourceStore.Has(ref.Type) { + if !dataSourceStore.Has(ds.Type) { diags = append(diags, &hcl.Diagnostic{ - Summary: "Unknown " + dataSourceLabel + " type " + ref.Type, + Summary: "Unknown " + dataSourceLabel + " type " + ds.Type, Subject: block.LabelRanges[0].Ptr(), Detail: fmt.Sprintf("known data sources: %v", dataSourceStore.List()), Severity: hcl.DiagError, @@ -89,7 +90,7 @@ func (cfg *PackerConfig) startDatasource(dataSourceStore packer.DatasourceStore, return nil, diags } - datasource, err := dataSourceStore.Start(ref.Type) + datasource, err := dataSourceStore.Start(ds.Type) if err != nil { diags = append(diags, &hcl.Diagnostic{ Summary: err.Error(), @@ -99,7 +100,7 @@ func (cfg *PackerConfig) startDatasource(dataSourceStore packer.DatasourceStore, } if datasource == nil { diags = append(diags, &hcl.Diagnostic{ - Summary: fmt.Sprintf("failed to start datasource plugin %q.%q", ref.Type, ref.Name), + Summary: fmt.Sprintf("failed to start datasource plugin %q.%q", ds.Type, ds.Name), Subject: &block.DefRange, Severity: hcl.DiagError, }) diff --git a/hcl2template/types.packer_config.go b/hcl2template/types.packer_config.go index c344aa7b0b2..10963226160 100644 --- a/hcl2template/types.packer_config.go +++ b/hcl2template/types.packer_config.go @@ -381,7 +381,7 @@ func (cfg *PackerConfig) recursivelyEvaluateDatasources(ref DatasourceRef, depen // If we've gotten here, then it means ref doesn't seem to have any further // dependencies we need to evaluate first. Evaluate it, with the cfg's full // data source context. - datasource, startDiags := cfg.startDatasource(cfg.parser.PluginConfig.DataSources, ref, true) + datasource, startDiags := cfg.startDatasource(ds) if startDiags.HasErrors() { diags = append(diags, startDiags...) return dependencies, diags From febaada14ec3a7d1c2b22c7ddccd5aeaa0682181 Mon Sep 17 00:00:00 2001 From: Lucas Bajolet Date: Fri, 25 Aug 2023 17:00:03 -0400 Subject: [PATCH 6/6] hcl2template: fix func to get vars from a config The previous implementation of the GetVarsByType function worked only on top-level attributes, ignoring the nested blocks in the structure. This implies that if a datasource depends on another through an expression within a nested block, we may not execute it first, and then executing this datasource before its dependent is possible, resulting in an error in the end. This commit is an attempt at making this more reliable for HCL configs, but only works on configs lifted from HCL files for now. We need to make this more reliable for later iterations. --- hcl2template/utils.go | 52 ++++++++++++++++++++++++++++++++----------- 1 file changed, 39 insertions(+), 13 deletions(-) diff --git a/hcl2template/utils.go b/hcl2template/utils.go index a542a2467ac..ce9486ed676 100644 --- a/hcl2template/utils.go +++ b/hcl2template/utils.go @@ -11,6 +11,7 @@ import ( "github.com/gobwas/glob" "github.com/hashicorp/hcl/v2" + "github.com/hashicorp/hcl/v2/hclsyntax" "github.com/hashicorp/packer/hcl2template/repl" hcl2shim "github.com/hashicorp/packer/hcl2template/shim" "github.com/zclconf/go-cty/cty" @@ -187,22 +188,47 @@ func ConvertPluginConfigValueToHCLValue(v interface{}) (cty.Value, error) { return buildValue, nil } +// GetVarsByType walks through a hcl body, and gathers all the Traversals that +// have a root type matching one of the specified top-level labels. +// +// This will only work on finite, expanded, HCL bodies. func GetVarsByType(block *hcl.Block, topLevelLabels ...string) []hcl.Traversal { - attributes, _ := block.Body.JustAttributes() - - var vars []hcl.Traversal - - for _, attr := range attributes { - for _, variable := range attr.Expr.Variables() { - rootLabel := variable.RootName() - for _, label := range topLevelLabels { - if label == rootLabel { - vars = append(vars, variable) - break - } + var travs []hcl.Traversal + + switch body := block.Body.(type) { + case *hclsyntax.Body: + travs = getVarsByTypeForHCLSyntaxBody(body) + default: + attrs, _ := body.JustAttributes() + for _, attr := range attrs { + travs = append(travs, attr.Expr.Variables()...) + } + } + + var rets []hcl.Traversal + for _, t := range travs { + varRootname := t.RootName() + for _, lbl := range topLevelLabels { + if varRootname == lbl { + rets = append(rets, t) + break } } } - return vars + return rets +} + +func getVarsByTypeForHCLSyntaxBody(body *hclsyntax.Body) []hcl.Traversal { + var rets []hcl.Traversal + + for _, attr := range body.Attributes { + rets = append(rets, attr.Expr.Variables()...) + } + + for _, block := range body.Blocks { + rets = append(rets, getVarsByTypeForHCLSyntaxBody(block.Body)...) + } + + return rets }