Skip to content
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

Address a few inconsistencies in how we process root module input variables vs. called module input variables #29959

Merged
merged 4 commits into from Jan 10, 2022

Commits on Dec 20, 2021

  1. core: Remove TestContext2Validate_PlanGraphBuilder

    This test seems to be a holdover from the many-moons-ago switch from one
    graph for all operations to separate graphs for plan and apply. It is
    effectively just a copy of a subset of the content of the Context.Validate
    function and is a maintainability hazard because it tends to lag behind
    updates to that function unless changes there happen to make it fail.
    
    This test doesn't cover anything that the other validate context tests
    don't exercise as an implementation detail of calling Context.Validate,
    so I've just removed it with no replacement.
    apparentlymart committed Dec 20, 2021
    Copy the full SHA
    8e58f59 View commit details
    Browse the repository at this point in the history

Commits on Dec 21, 2021

  1. core: Handle root and child module input variables consistently

    Previously we had a significant discrepancy between these two situations:
    we wrote the raw root module variables directly into the EvalContext and
    then applied type conversions only at expression evaluation time, while
    for child modules we converted and validated the values while visiting
    the variable graph node and wrote only the _final_ value into the
    EvalContext.
    
    This confusion seems to have been the root cause for #29899, where
    validation rules for root module variables were being applied at the wrong
    point in the process, prior to type conversion.
    
    To fix that bug and also make similar mistakes less likely in the future,
    I've made the root module variable handling more like the child module
    variable handling in the following ways:
     - The "raw value" (exactly as given by the user) lives only in the graph
       node representing the variable, which mirrors how the _expression_
       for a child module variable lives in its graph node. This means that
       the flow for the two is the same except that there's no expression
       evaluation step for root module variables, because they arrive as
       constant values from the caller.
     - The set of variable values in the EvalContext is always only "final"
       values, after type conversion is complete. That in turn means we no
       longer need to do "just in time" conversion in
       evaluationStateData.GetInputVariable, and can just return the value
       exactly as stored, which is consistent with how we handle all other
       references between objects.
    
    This diff is noisier than I'd like because of how much it takes to wire
    a new argument (the raw variable values) through to the plan graph builder,
    but those changes are pretty mechanical and the interesting logic lives
    inside the plan graph builder itself, in NodeRootVariable, and
    the shared helper functions in eval_variable.go.
    
    While here I also took the opportunity to fix a historical API wart in
    EvalContext, where SetModuleCallArguments was built to take a set of
    variable values all at once but our current caller always calls with only
    one at a time. That is now just SetModuleCallArgument singular, to match
    with the new SetRootModuleArgument to deal with root module variables.
    apparentlymart committed Dec 21, 2021
    Copy the full SHA
    72bff83 View commit details
    Browse the repository at this point in the history

Commits on Dec 22, 2021

  1. core and backend: remove redundant handling of default variable values

    Previously we had three different layers all thinking they were
    responsible for substituting a default value for an unset root module
    variable:
     - the local backend, via logic in backend.ParseVariableValues
     - the context.Plan function (and other similar functions) trying to
       preprocess the input variables using
       terraform.mergeDefaultInputVariableValues .
     - the newer prepareFinalInputVariableValue, which aims to centralize all
       of the variable preparation logic so it can be common to both root and
       child module variables.
    
    The second of these was also trying to handle type constraint checking,
    which is also the responsibility of the central function and not something
    we need to handle so early.
    
    Only the last of these consistently handles both root and child module
    variables, and so is the one we ought to keep. The others are now
    redundant and are causing prepareFinalInputVariableValue to get a slightly
    corrupted view of the caller's chosen variable values.
    
    To rectify that, here we remove the two redundant layers altogether and
    have unset root variables pass through as cty.NilVal all the way to the
    central prepareFinalInputVariableValue function, which will then handle
    them in a suitable way which properly respects the "nullable" setting.
    
    This commit includes some test changes in the terraform package to make
    those tests no longer rely on the mergeDefaultInputVariableValues logic
    we've removed, and to instead explicitly set cty.NilVal for all unset
    variables to comply with our intended contract for PlanOpts.SetVariables,
    and similar. (This is so that we can more easily catch bugs in callers
    where they _don't_ correctly handle input variables; it allows us to
    distinguish between the caller explicitly marking a variable as unset vs.
    not describing it at all, where the latter is a bug in the caller.)
    apparentlymart committed Dec 22, 2021
    Copy the full SHA
    630500e View commit details
    Browse the repository at this point in the history
  2. core: More accurate error message for invalid variable values

    In earlier Terraform versions we had an extra validation step prior to
    the graph walk which tried to partially validate root module input
    variable values (just checking their type constraints) and then return
    error messages which specified as accurately as possible where the value
    had originally come from.
    
    We're now handling that sort of validation exclusively during the graph
    walk so that we can share the main logic between both root module and
    child module variable values, but previously that shared code wasn't
    able to generate such specific information about where the values had
    originated, because it was adapted from code originally written to only
    deal with child module variables.
    
    Here then we restore a similar level of detail as before, when we're
    processing root module variables. For child module variables, we use
    synthetic InputValue objects which state that the value was declared
    in the configuration, thus causing us to produce a similar sort of error
    message as we would've before which includes a source range covering
    the argument expression in the calling module block.
    apparentlymart committed Dec 22, 2021
    Copy the full SHA
    d1de8ba View commit details
    Browse the repository at this point in the history