-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dag for local data eval #13155
base: main
Are you sure you want to change the base?
Dag for local data eval #13155
Changes from all commits
30aaccd
24caa29
b1459f7
061f0f7
0511eda
d416eb3
e6b4617
c4fce7e
72c9d06
91ed03b
d879bcd
bf6a402
9a56029
f242ba1
ac566c9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,12 +7,14 @@ import ( | |
"fmt" | ||
"os" | ||
"path/filepath" | ||
"reflect" | ||
|
||
"github.com/hashicorp/go-version" | ||
"github.com/hashicorp/hcl/v2" | ||
"github.com/hashicorp/hcl/v2/ext/dynblock" | ||
"github.com/hashicorp/hcl/v2/hclparse" | ||
packersdk "github.com/hashicorp/packer-plugin-sdk/packer" | ||
"github.com/hashicorp/packer/internal/dag" | ||
"github.com/hashicorp/packer/packer" | ||
"github.com/zclconf/go-cty/cty" | ||
) | ||
|
@@ -295,10 +297,224 @@ func filterVarsFromLogs(inputOrLocal Variables) { | |
} | ||
} | ||
|
||
func (cfg *PackerConfig) detectBuildPrereqDependencies() hcl.Diagnostics { | ||
var diags hcl.Diagnostics | ||
|
||
for _, ds := range cfg.Datasources { | ||
dependencies := GetVarsByType(ds.block, "data") | ||
dependencies = append(dependencies, GetVarsByType(ds.block, "local")...) | ||
|
||
for _, dep := range dependencies { | ||
// If something is locally aliased as `local` or `data`, we'll falsely | ||
// report it as a local variable, which is not necessarily what we | ||
// want to process here, so we continue. | ||
// | ||
// Note: this is kinda brittle, we should understand scopes to accurately | ||
// mark something from an expression as a reference to a local variable. | ||
// No real good solution for this now, besides maybe forbidding something | ||
// to be locally aliased as `local`. | ||
if len(dep) < 2 { | ||
continue | ||
} | ||
rs, err := NewRefStringFromDep(dep) | ||
if err != nil { | ||
diags = diags.Append(&hcl.Diagnostic{ | ||
Severity: hcl.DiagError, | ||
Summary: "failed to process datasource dependency", | ||
Detail: fmt.Sprintf("An error occurred while processing a dependency for data source %s: %s", | ||
ds.Name(), err), | ||
}) | ||
continue | ||
} | ||
|
||
err = ds.RegisterDependency(rs) | ||
if err != nil { | ||
diags = diags.Append(&hcl.Diagnostic{ | ||
Severity: hcl.DiagError, | ||
Summary: "failed to register datasource dependency", | ||
Detail: fmt.Sprintf("An error occurred while registering %q as a dependency for data source %s: %s", | ||
rs, ds.Name(), err), | ||
}) | ||
} | ||
} | ||
|
||
cfg.Datasources[ds.Ref()] = ds | ||
} | ||
|
||
for _, loc := range cfg.LocalBlocks { | ||
dependencies := FilterTraversalsByType(loc.Expr.Variables(), "data") | ||
dependencies = append(dependencies, FilterTraversalsByType(loc.Expr.Variables(), "local")...) | ||
|
||
for _, dep := range dependencies { | ||
// If something is locally aliased as `local` or `data`, we'll falsely | ||
// report it as a local variable, which is not necessarily what we | ||
// want to process here, so we continue. | ||
// | ||
// Note: this is kinda brittle, we should understand scopes to accurately | ||
// mark something from an expression as a reference to a local variable. | ||
// No real good solution for this now, besides maybe forbidding something | ||
// to be locally aliased as `local`. | ||
if len(dep) < 2 { | ||
continue | ||
} | ||
rs, err := NewRefStringFromDep(dep) | ||
if err != nil { | ||
diags = diags.Append(&hcl.Diagnostic{ | ||
Severity: hcl.DiagError, | ||
Summary: "failed to process local dependency", | ||
Detail: fmt.Sprintf("An error occurred while processing a dependency for local variable %s: %s", | ||
loc.LocalName, err), | ||
}) | ||
continue | ||
} | ||
|
||
err = loc.RegisterDependency(rs) | ||
if err != nil { | ||
diags = diags.Append(&hcl.Diagnostic{ | ||
Severity: hcl.DiagError, | ||
Summary: "failed to register local dependency", | ||
Detail: fmt.Sprintf("An error occurred while registering %q as a dependency for local variable %s: %s", | ||
rs, loc.LocalName, err), | ||
}) | ||
} | ||
} | ||
} | ||
|
||
return diags | ||
} | ||
|
||
func (cfg *PackerConfig) buildPrereqsDAG() (*dag.AcyclicGraph, error) { | ||
retGraph := dag.AcyclicGraph{} | ||
|
||
verticesMap := map[string]dag.Vertex{} | ||
|
||
// Do a first pass to create all the vertices | ||
for ref := range cfg.Datasources { | ||
// We keep a reference to the datasource separately from where it | ||
// is used to avoid getting bit by the loop semantics. | ||
// | ||
// This `ds` local variable is the same object for every loop | ||
// so if we directly use the address of this object, we'll end | ||
// up referencing the last node of the loop for each vertex, | ||
// leading to implicit cycles. | ||
// | ||
// However by capturing it locally in this loop, we have a | ||
// reference to the actual datasource block, so it ends-up being | ||
// the right instance for each vertex. | ||
ds := cfg.Datasources[ref] | ||
v := retGraph.Add(&ds) | ||
verticesMap[fmt.Sprintf("data.%s", ds.Name())] = v | ||
} | ||
// Note: locals being references to the objects already, we can safely | ||
// use the reference returned by the local loop. | ||
for _, local := range cfg.LocalBlocks { | ||
v := retGraph.Add(local) | ||
verticesMap[fmt.Sprintf("local.%s", local.LocalName)] = v | ||
} | ||
|
||
// Connect the vertices together | ||
// | ||
// Vertices that don't have dependencies will be connected to the | ||
// root vertex of the graph | ||
for _, ds := range cfg.Datasources { | ||
dsName := fmt.Sprintf("data.%s", ds.Name()) | ||
|
||
for _, dep := range ds.Dependencies { | ||
retGraph.Connect( | ||
dag.BasicEdge(verticesMap[dsName], | ||
verticesMap[dep.String()])) | ||
} | ||
} | ||
for _, loc := range cfg.LocalBlocks { | ||
locName := fmt.Sprintf("local.%s", loc.LocalName) | ||
|
||
for _, dep := range loc.dependencies { | ||
retGraph.Connect( | ||
dag.BasicEdge(verticesMap[locName], | ||
verticesMap[dep.String()])) | ||
} | ||
} | ||
|
||
if err := retGraph.Validate(); err != nil { | ||
return nil, err | ||
} | ||
|
||
return &retGraph, nil | ||
} | ||
|
||
func (cfg *PackerConfig) evaluateBuildPrereqs(skipDatasources bool) hcl.Diagnostics { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The logic in this parser seems minimally unit tested, I notice that we have the packer_tests that invoke a larger section of the code, but I feel like it may be worth adding more testing to the logic in the parser, maybe we could make these functions public instead of private and invoke them in unit tests, just small tests that make sure we skip things aliased as local for example There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree it is not unit tested, and to be frank there are a bunch of pre-requisites to make this unit-testable, at the moment we rely on the Also to be clear: that code is in a |
||
diags := cfg.detectBuildPrereqDependencies() | ||
if diags.HasErrors() { | ||
return diags | ||
} | ||
|
||
graph, err := cfg.buildPrereqsDAG() | ||
if err != nil { | ||
return diags.Append(&hcl.Diagnostic{ | ||
Severity: hcl.DiagError, | ||
Summary: "failed to prepare execution graph", | ||
Detail: fmt.Sprintf("An error occurred while building the graph for datasources/locals: %s", err), | ||
}) | ||
} | ||
|
||
walkFunc := func(v dag.Vertex) hcl.Diagnostics { | ||
var diags hcl.Diagnostics | ||
|
||
switch bl := v.(type) { | ||
case *DatasourceBlock: | ||
diags = cfg.evaluateDatasource(*bl, skipDatasources) | ||
case *LocalBlock: | ||
var val *Variable | ||
if cfg.LocalVariables == nil { | ||
cfg.LocalVariables = make(Variables) | ||
} | ||
val, diags = cfg.evaluateLocalVariable(bl) | ||
// Note: clumsy a bit, but we won't add the variable as `nil` here | ||
// unless no errors have been reported during evaluation. | ||
// | ||
// This prevents Packer from panicking down the line, as initialisation | ||
// doesn't stop if there are diags, so if `val` is nil, it crashes. | ||
if !diags.HasErrors() { | ||
cfg.LocalVariables[bl.LocalName] = val | ||
} | ||
default: | ||
diags = diags.Append(&hcl.Diagnostic{ | ||
Severity: hcl.DiagError, | ||
Summary: "unsupported DAG node type", | ||
Detail: fmt.Sprintf("A node of type %q was added to the DAG, but cannot be "+ | ||
"evaluated as it is unsupported. "+ | ||
"This is a Packer bug, please report it so we can investigate.", | ||
reflect.TypeOf(v).String()), | ||
}) | ||
} | ||
|
||
if diags.HasErrors() { | ||
return diags | ||
} | ||
|
||
return nil | ||
} | ||
|
||
for _, vtx := range graph.ReverseTopologicalOrder() { | ||
vtxDiags := walkFunc(vtx) | ||
if vtxDiags.HasErrors() { | ||
diags = diags.Extend(vtxDiags) | ||
return diags | ||
} | ||
} | ||
|
||
return nil | ||
} | ||
|
||
func (cfg *PackerConfig) Initialize(opts packer.InitializeOptions) hcl.Diagnostics { | ||
diags := cfg.InputVariables.ValidateValues() | ||
diags = append(diags, cfg.evaluateDatasources(opts.SkipDatasourcesExecution)...) | ||
diags = append(diags, cfg.evaluateLocalVariables(cfg.LocalBlocks)...) | ||
|
||
if opts.UseSequential { | ||
diags = diags.Extend(cfg.evaluateDatasources(opts.SkipDatasourcesExecution)) | ||
diags = diags.Extend(cfg.evaluateLocalVariables(cfg.LocalBlocks)) | ||
} else { | ||
diags = diags.Extend(cfg.evaluateBuildPrereqs(opts.SkipDatasourcesExecution)) | ||
} | ||
|
||
filterVarsFromLogs(cfg.InputVariables) | ||
filterVarsFromLogs(cfg.LocalVariables) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mention this is brittle, can you think of a scenario where this would cause valid dependencies to be ignored?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So on this, I don't have an example to be fair, I had tried to replicate a concern of mine with local aliases in a
for
expression, but the local aliases weren't returned by the hcl function, so it's mostly a non-concern at this point.I can remove this comment until we do encounter a real-life example that highlights this problem, but at the present time, it seems somewhat solid on the templates we have in the repo.