This removes most of the code previously added in 491969d29d, because we
since learned that the hashicorp/helm provider signals deferral when any
unknown values are present in provider configuration even though in
practice it can sometimes successfully plan changes in spite of those
unknown values.
That therefore made the hashicorp/helm provider behavior worse under this
change than it was before, returning an error when no error was actually
warranted.
The ephemeral resources implementation landed later and was also
interacting with this change, and so this isn't a line-for-line revert of
the original change but still removes everything that was added in support
of handling provider deferral signals so that we'll be able to start fresh
with this later if we find a better way to handle it.
Signed-off-by: Martin Atkins <mart@degeneration.co.uk>
Previously the ReferenceTransformer just had a broad rule that any node
which implements GraphNodeDestroyer has its expression references
completely ignored.
Now that we're starting to allow dynamic expressions for destroy-related
settings like "prevent_destroy", we need to be able to represent the
dependencies implied by those expressions.
However, the assumption that configuration is mostly ignored when planning
destroy is load-bearing for minimizing awkward dependency problems in the
destroy-planning graph, so this introduces a new concept of "destroy
references" which means that we can implement only a small, curated subset
of references -- for now, just the ones from prevent_destroy -- that get
considered for any node type that implements GraphNodeDestroyer.
Having GraphNodeDestroyer effectively take priority over
GraphNodeReferencer seems like the least disruptive way to retrofit this
idea surgically as a small change to the previous unilateral rule against
any references at all, because in practice all of the destroy nodes embed
NodeAbstractResourceInstance and therefore implement GraphNodeReferencer,
so it is important that we continue to ignore that type's
GraphNodeReferencer implementation whenever it's embedded in something
that is also a GraphNodeDestroyer.
Signed-off-by: Martin Atkins <mart@degeneration.co.uk>
Previously we evaluated prevent_destroy expressions immediately inside the
config loader, thereby forcing it to always be a constant expression
producing a bool value.
Now the config loader just saves whatever expression it was given and we
let the language runtime deal with it instead, which means we can allow
references to dynamically-chosen values from elsewhere in the same module.
The language runtime's "validate" phase still performs a type check for
bool that's equivalent to what we used to do during config loading to make
sure that the "tofu validate" command can catch a similar subset of
problems as it used to be able to catch, but we have more information
available during the plan phase that allows us to produce more complete and
relevant error messages, so for any expression that we can't evaluate
with a nil evaluation context we'll now let the plan phase deal with the
checks instead.
The policy for handling annoying cases such as unknown values, ephemeral
values, sensitive values, and references to local symbols like count.index
is intentionally the most conservative choice to start, because future
versions of OpenTofu can allow more once we've got more experience but
cannot permit less if we find that we've made a mistake. Future changes
could potentially make these rules a little more liberal, once we have
learned from feedback on this initial functionality.
Signed-off-by: Martin Atkins <mart@degeneration.co.uk>
During the "refresh" and "plan" steps we build the marks for a managed
resource object's value as a combination of the marks from the input
(prior state or configuration, respectively) and the marks implied by the
provider schema.
However, the apply step was previously relying only on the marks from the
planned new state, without considering marks from the provider schema. That
meant that a sensitive attribute contained within a container that is
unknown during planning could not be marked as sensitive once the container
became known, because the corresponding value did not exist at all in the
planned new state and therefore could not carry a sensitive mark.
To fix this problem, this changes the apply step to match the strategy
already used in the refresh and plan steps: using combinePathValueMarks
to blend the dynamic marks with the static information from the schema,
so that the final value saved in the new state snapshot will have a full
set of sensitive markings for the next plan/apply round to rely on. Without
this the next plan/apply round would produce a spurious diff due to the
sensitivity of the nested attributes appearing to have changed.
This introduces a new test TestContext2Apply_sensitiveInsideUnknown which
covers the case where the sensitivity information comes from schema. The
preexisting test TestContext2Apply_additionalSensitiveFromState already
covered the case of dynamically-tracked sensitivity information, and
remains passing without modification after this change.
Signed-off-by: Martin Atkins <mart@degeneration.co.uk>
Previously the Go toolchain had no explicit support for "tools" and so we
used the typical Go community workaround of adding "tools.go" files (two,
for some reason) that existed only to trick the Go toolchain into
considering the tools as dependencies we could track in go.mod.
Go 1.24 introduced explicit support for tracking tools as part of go.mod,
and the ability to run those using "go tool" instead of "go run", and so
this commit switches us over to using that strategy for everything we were
previously managing in tools.go.
There are some intentional exceptions here:
- The protobuf-compile script can't use "go tool" or "go run" because the
tools in question are run only indirectly through protoc. However, we
do still use the "tool" directive in go.mod to tell the Go toolchain that
we depend on those tools, so that it'll track which versions we are
currently using as part of go.mod.
- Our golangci-lint Makefile target uses "go run" to run a specific
version of golangci-lint. We _intentionally_ don't consider that tool
to be a direct dependency of OpenTofu because it has a lot of indirect
dependencies that would pollute our go.mod file. Therefore that continues
to use "go run" after this commit.
- Both of our tools.go files previously referred to
github.com/nishanths/exhaustive , but nothing actually appears to be
using that tool in the current OpenTofu tree, so it's no longer a
dependency after this commit.
All of the dependencies we have _only_ for tools are now classified as
"indirect" in the go.mod file. This is the default behavior of the Go
toolchain and appears to be motivated by making it clearer that these
modules do not contribute anything to the runtime behavior of OpenTofu.
This also corrected a historical oddity in our go.mod where for some reason
the "indirect" dependencies had been split across two different "require"
directives; they are now all grouped together in a single directive.
Signed-off-by: Martin Atkins <mart@degeneration.co.uk>
This caused a bunch of mechanical changes to callers, of course. Expression
evaluation is a very cross-cutting concern, so updating everything all at
once would be a lot and so this stops at a mostly-arbitrary point wiring
a bunch of callers to pass in contexts without changing anything that has
lots of callers.
We'll continue pulling on this thread in later commits.
Signed-off-by: Martin Atkins <mart@degeneration.co.uk>
Continuing the ongoing work of getting context.Context wired in everywhere
we might want to generate OpenTelemetry traces, this completes all of the
provider-related methods of EvalContext.
Unfortunately there is still one remaining path not included here: the
EvalContext.EvaluationScope method needs to somehow arrange for contexts
to reach the provider-defined functions so that we can pass the context
to providers.Interface.CallFunction, which is tricky because that has to
get through the cty function API that wasn't designed for functions that
are backed by network calls. We'll deal with that in a subsequent commit
because it's likely to be a more invasive change than the
relatively-mechanical wiring updates included here.
Signed-off-by: Martin Atkins <mart@degeneration.co.uk>
It seems that a small number of providers are now able to return a special
signal when they find that they are unable to perform an operation due to
unknown values in the provider or resource configuration.
This is a uses that new signal to recommend a workaround in that situation,
giving a more actionable error message than would've been returned by the
provider otherwise.
We've not yet decided how OpenTofu might make use of these new signals in
the long term, and so this is intentionally implemented in a way where
most of the logic is centralized in the provider-related packages rather
than sprawled all over "package tofu".
It's likely that a future incarnation of this will plumb this idea in more
deeply, but this is just a temporary stop-gap to give slightly better
error messages in the meantime and so it's better to keep it relatively
contained for now until we have a longer-term plan for what OpenTofu Core
might do with this information.
Signed-off-by: Martin Atkins <mart@degeneration.co.uk>
Continuing our work to gradually plumb context.Context to everywhere that
we want to generate OpenTelemetry traces, this completes the call path
for most (but not all) of the gRPC requests to provider plugins, so that
we can add OpenTelemetry trace instrumentation in a future commit.
Unfortunately there are still a few providers.Interface callers left in
functions that don't have context.Context plumbed to them yet, and so
those are temporarily stubbed as context.TODO() here so we can more easily
find and complete them later.
The two gRPC implementations of providers.Interface were previously making
provider requests using a single context.Context established at the time
the provider process was started, but that isn't an appropriate context
to use for per-request concerns like tracing, so that context is now
unused and could potentially be removed in a future commit, but this change
already got pretty large and so I intend to deal with that separately
later.
This now exposes the gRPC provider calls to potential context cancellation
that they would previously observe only indirectly though the Stop method.
Since Stop is primarily used for graceful shutdown of ApplyResourceChange,
the changes here explicitly disconnect the cancellation signal for
ApplyResourceChange in particular, while letting the others get canceled
in the normal way since they are expected to be free of significant
side-effects. In future work we could consider removing Stop from the
internal API entirely and keeping it only as an implementation detail of
the gRPC implementation of this interface, with ApplyResourceChange
directly reacting to context cancellation and sending the gRPC Stop call
itself, but again that's too much change for this already-large commit.
The internal/legacy package currently contains some legacy code preserved
for the benefit of the backends, and unfortunately contains more than is
strictly necessary to support those callers, and so there was some dead
code there that also needed updating. provider_mock.go is removed entirely
because it's just an older copy of the similar file in package tofu. The
few calls to providers in schemas.go are updated to use
context.Background() rather than context.TODO() because we have no
intention of plumbing context.Context into that legacy code, and will
hopefully just delete it wholesale one day.
Signed-off-by: Martin Atkins <mart@degeneration.co.uk>
The graph node types related to resources and resource instances use a
bunch of helper functions in different combinations, rather than calling
directly into the provider API.
This commit plumbs context.Context through to the functions that _do_
eventually call methods directly on the provider object, leaving us just
one more step away from plumbing the context through to the actual gRPC
calls. The next step (in a future commit) will be to update the
providers.Interface methods to take context.Context arguments and then
have the gRPC-based implementations of that interface pass the context
through to the gRPC client stub calls, and then we should be pretty close
to being able to turn on OTel tracing instrumentation for our gRPC
client requests.
Signed-off-by: Martin Atkins <mart@degeneration.co.uk>
This continues our ongoing standardization on using evalCtx for parameters
of type EvalContext, so that we can use ctx for parameters of type
context.Context.
This commit is just a bunch of mechanical renames with no substantive
changes. Future commits will introduce additional context.Context params
to many of these functions.
Signed-off-by: Martin Atkins <mart@degeneration.co.uk>
This commit begins the introduction of OpenTelemetry tracing into the
implementations of individual graph node types in the language runtime.
It's tempting to just plumb this in centrally in ContextGraphWalker for all
graph nodes, but our intention is for the trace output to correspond as
much as possible to the OpenTofu user experience rather than its
implementation details, and the execution graph is _very much_ an
implementation detail with many aspects that have no direct manifestation
in the UI.
With that in mind, this focuses only on capturing traces for graph node
types that fit into one or more of the following categories:
- Corresponds to something we already announce in the UI output today.
- Can cause at least one provider gRPC (or other similar) request to occur
and so makes a good parent for later-added traces of those requests.
- Has an execution time that depends on something outside of OpenTofu's
direct control, such as interactive input which takes as long as the
user takes to answer the prompts. (Including these helps account for
this time separately from the time when OpenTofu was doing useful work.)
As long as our focus continues to be on echoing concepts we expect users
will already be familiar with from their use of OpenTofu, this probably
represents the deepest level of detail we ought to represent for the
purposes of describing OpenTofu's conceptual process. In a future commit
we'll add one extra level of nested spans beneath this one representing
directly the gRPC requests we're making to providers, since those spans
will represent the handoff to a potential sub-trace generated by the
provider plugin itself once we've established a mechanism for providers to
discover their trace parent.
Signed-off-by: Martin Atkins <mart@degeneration.co.uk>
We're intending to gradually improve all of the existing functions that
fail these checks as a separate project from other work, because fixing
for these particular lint rules tends to be too invasive to be safe or
sensible to combine with other work.
Therefore we'll temporarily disable these lints from the main lint run
and add a separate .golangci-complexity.yml that we can use to track our
progress towards eliminating those lint failures without continuing to
litter the code with nolint comments in the meantime.
This also removes all of the existing nolint comments for these linters so
that we can start fresh and review each one as part of our improvement
project.
We'll re-enable these linters (and remove .golangci-complexity.yml) once
each example has either been rewritten to pass the checks or we've
concluded that further decomposition would hurt readability and so added
"nolint" comments back in so we can review whether our lint rules are too
strict once we've got a bunch of examples to consider together.
Signed-off-by: Martin Atkins <mart@degeneration.co.uk>
Previously we made a very generic suggestion to use -target to split a
change into two parts as a workaround for the fact that count and for_each
must be known during planning. That works, but we didn't have enough
information available to tell the operator exactly what to target and so
anyone who is not an expert on the configuration they're working with tends
to get stuck unable to figure out exactly what they need to do.
The new -exclude option gives us an opportunity to do better here: we tend
to know for which object we're currently evaluating count or for_each, and
so we can mention that object directly in the error message when if we
recommend to use -exclude instead of -target.
Not all objects that support count/for_each will necessarily be directly
targetable, so we can still potentially recommend -target when we're
dealing with one of those objects. For example, as of this commit that
is true for for_each in a provider block, because there is not currently
any syntax for specifying a provider configuration as an addrs.Targetable.
Perhaps we'll introduce such a thing in the future, but that's outside the
scope of this change that's primarily focused on improving the messaging
for resource and module count/for_each.
Signed-off-by: Martin Atkins <mart@degeneration.co.uk>
