From 4a9d46f9df972215ba2eee180c84966e7537e2fc Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 6 Nov 2024 12:55:51 -0500 Subject: [PATCH 1/4] avoid unknowns in OpenEphemeralResource Ephemeral resources can't be opened if the configuration contains unknown values. --- .../ephemeral/ephemeral_resources.go | 5 +++++ internal/terraform/node_resource_ephemeral.go | 21 +++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/internal/resources/ephemeral/ephemeral_resources.go b/internal/resources/ephemeral/ephemeral_resources.go index 0034579d0822..186bbd6c8460 100644 --- a/internal/resources/ephemeral/ephemeral_resources.go +++ b/internal/resources/ephemeral/ephemeral_resources.go @@ -212,6 +212,11 @@ type resourceInstanceInternal struct { func (r *resourceInstanceInternal) close(ctx context.Context) tfdiags.Diagnostics { var diags tfdiags.Diagnostics + // if the resource could not be opened, there will not be anything to close either + if r.impl == nil { + return diags + } + // Stop renewing, if indeed we are. If we previously saw any errors during // renewing then they finally get returned here, to be reported along with // any errors during close. diff --git a/internal/terraform/node_resource_ephemeral.go b/internal/terraform/node_resource_ephemeral.go index b57de5ed26d7..916ba4c9f428 100644 --- a/internal/terraform/node_resource_ephemeral.go +++ b/internal/terraform/node_resource_ephemeral.go @@ -73,6 +73,27 @@ func ephemeralResourceOpen(ctx EvalContext, inp ephemeralResourceInput) (*provid } unmarkedConfigVal, configMarks := configVal.UnmarkDeepWithPaths() + if !unmarkedConfigVal.IsWhollyKnown() { + log.Printf("[DEBUG] ehpemeralResourceOpen: configuration for %s contains unknown values, cannot open resource", inp.addr) + + // We don't know what the result will be, but we need to keep the + // configured attributes for consistent evaluation. We can use the same + // technique we used for data sources to create the plan-time value. + unknownResult := objchange.PlannedDataResourceObject(schema, unmarkedConfigVal) + // add back any configured marks + unknownResult = unknownResult.MarkWithPaths(configMarks) + // and mark the entire value as ephemeral, since it's coming from an ephemeral context. + unknownResult = unknownResult.Mark(marks.Ephemeral) + + // The state of ephemerals all comes from the registered instances, so + // we still need to register something so evaluation doesn't fail. + ephemerals.RegisterInstance(ctx.StopCtx(), inp.addr, ephemeral.ResourceInstanceRegistration{ + Value: unknownResult, + ConfigBody: config.Config, + }) + return nil, diags + } + validateResp := provider.ValidateEphemeralResourceConfig(providers.ValidateEphemeralResourceConfigRequest{ TypeName: inp.addr.Resource.Resource.Type, Config: unmarkedConfigVal, From 02bbd750c4da1bdc33fca3b3147f963c5d1edc8a Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 6 Nov 2024 13:17:10 -0500 Subject: [PATCH 2/4] test for unknown ephemeral configs An Ephemeral resource should not be opened at all if there are any unknowns in the configuration. --- .../terraform/context_plan_ephemeral_test.go | 81 +++++++++++++++++++ 1 file changed, 81 insertions(+) diff --git a/internal/terraform/context_plan_ephemeral_test.go b/internal/terraform/context_plan_ephemeral_test.go index 5e0af42b9659..44265798bd70 100644 --- a/internal/terraform/context_plan_ephemeral_test.go +++ b/internal/terraform/context_plan_ephemeral_test.go @@ -652,3 +652,84 @@ ephemeral "ephem_resource" "data" { }) } } + +func TestContext2Apply_ephemeralUnknownPlan(t *testing.T) { + m := testModuleInline(t, map[string]string{ + "main.tf": ` +resource "test_instance" "test" { +} + +ephemeral "ephem_resource" "data" { + input = test_instance.test.id + lifecycle { + postcondition { + condition = self.value != nil + error_message = "should return a value" + } + } +} + +locals { + value = ephemeral.ephem_resource.data.value +} + +// create a sink for the ephemeral value to test +provider "sink" { + test_string = local.value +} + +// we need a resource to ensure the sink provider is configured +resource "sink_object" "empty" { +} +`, + }) + + ephem := &testing_provider.MockProvider{ + GetProviderSchemaResponse: &providers.GetProviderSchemaResponse{ + EphemeralResourceTypes: map[string]providers.Schema{ + "ephem_resource": { + Block: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "value": { + Type: cty.String, + Computed: true, + }, + "input": { + Type: cty.String, + Required: true, + }, + }, + }, + }, + }, + }, + } + + sink := simpleMockProvider() + sink.GetProviderSchemaResponse.ResourceTypes = map[string]providers.Schema{ + "sink_object": {Block: simpleTestSchema()}, + } + sink.ConfigureProviderFn = func(req providers.ConfigureProviderRequest) (resp providers.ConfigureProviderResponse) { + if req.Config.GetAttr("test_string").IsKnown() { + t.Error("sink provider config should not be known in this test") + } + return resp + } + + p := testProvider("test") + + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("ephem"): testProviderFuncFixed(ephem), + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + addrs.NewDefaultProvider("sink"): testProviderFuncFixed(sink), + }, + }) + + _, diags := ctx.Plan(m, nil, DefaultPlanOpts) + assertNoDiagnostics(t, diags) + + if ephem.OpenEphemeralResourceCalled { + t.Error("OpenEphemeralResourceCalled called when config was not known") + } +} From c2a1070c23c1219c34887578376529546c1f1987 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 6 Nov 2024 13:56:20 -0500 Subject: [PATCH 3/4] create a ui hook for unknown ephemeral config This is a stopgap to give users some feedback when an ephemeral resource contains unknowns and cannot be opened. There is no json hook implementation so that the machine-readable spec is not bound to the new output until further review. It doesn't really fit with the current json ui model to have messages about what isn't going to happen. --- internal/command/views/hook_json.go | 7 +++++++ internal/command/views/hook_ui.go | 20 ++++++++++++++----- internal/terraform/node_resource_ephemeral.go | 17 +++++++++++----- 3 files changed, 34 insertions(+), 10 deletions(-) diff --git a/internal/command/views/hook_json.go b/internal/command/views/hook_json.go index 9ac061f2929d..4d05849bd9ac 100644 --- a/internal/command/views/hook_json.go +++ b/internal/command/views/hook_json.go @@ -167,6 +167,13 @@ func (h *jsonHook) PostRefresh(id terraform.HookResourceIdentity, dk addrs.Depos } func (h *jsonHook) PreEphemeralOp(id terraform.HookResourceIdentity, action plans.Action) (terraform.HookAction, error) { + // this uses the same plans.Read action as a data source to indicate that + // the ephemeral resource can't be processed until apply, so there is no + // progress hook + if action == plans.Read { + return terraform.HookActionContinue, nil + } + h.view.Hook(json.NewEphemeralOpStart(id.Addr, action)) progress := resourceProgress{ addr: id.Addr, diff --git a/internal/command/views/hook_ui.go b/internal/command/views/hook_ui.go index bc22260e233b..4a6644c8cbff 100644 --- a/internal/command/views/hook_ui.go +++ b/internal/command/views/hook_ui.go @@ -351,6 +351,12 @@ func (h *UiHook) PreEphemeralOp(rId terraform.HookResourceIdentity, action plans var operation string var op uiResourceOp switch action { + case plans.Read: + // FIXME: this uses the same semantics as data sources, where "read" + // means deferred until apply, but because data sources don't implement + // hooks, and the meaning of Read is overloaded, we can't rely on any + // existing hooks + operation = "Configuration unknown, deferring..." case plans.Open: operation = "Opening..." op = uiResourceOpen @@ -367,6 +373,15 @@ func (h *UiHook) PreEphemeralOp(rId terraform.HookResourceIdentity, action plans return terraform.HookActionContinue, nil } + h.println(fmt.Sprintf( + h.view.colorize.Color("[reset][bold]%s: %s"), + rId.Addr, operation, + )) + + if action == plans.Read { + return terraform.HookActionContinue, nil + } + uiState := uiResourceState{ Address: key, Op: op, @@ -379,11 +394,6 @@ func (h *UiHook) PreEphemeralOp(rId terraform.HookResourceIdentity, action plans h.resources[key] = uiState h.resourcesLock.Unlock() - h.println(fmt.Sprintf( - h.view.colorize.Color("[reset][bold]%s: %s"), - rId.Addr, operation, - )) - go h.stillRunning(uiState) return terraform.HookActionContinue, nil diff --git a/internal/terraform/node_resource_ephemeral.go b/internal/terraform/node_resource_ephemeral.go index 916ba4c9f428..3453f77dc86a 100644 --- a/internal/terraform/node_resource_ephemeral.go +++ b/internal/terraform/node_resource_ephemeral.go @@ -51,6 +51,11 @@ func ephemeralResourceOpen(ctx EvalContext, inp ephemeralResourceInput) (*provid return nil, diags } + rId := HookResourceIdentity{ + Addr: inp.addr, + ProviderAddr: inp.providerConfig.Provider, + } + ephemerals := ctx.EphemeralResources() allInsts := ctx.InstanceExpander() keyData := allInsts.GetResourceInstanceRepetitionData(inp.addr) @@ -91,6 +96,13 @@ func ephemeralResourceOpen(ctx EvalContext, inp ephemeralResourceInput) (*provid Value: unknownResult, ConfigBody: config.Config, }) + + ctx.Hook(func(h Hook) (HookAction, error) { + // ephemeral resources aren't stored in the plan, so use a hook to + // give some feedback to the user that this can't be opened + return h.PreEphemeralOp(rId, plans.Read) + }) + return nil, diags } @@ -104,11 +116,6 @@ func ephemeralResourceOpen(ctx EvalContext, inp ephemeralResourceInput) (*provid return nil, diags } - rId := HookResourceIdentity{ - Addr: inp.addr, - ProviderAddr: inp.providerConfig.Provider, - } - ctx.Hook(func(h Hook) (HookAction, error) { return h.PreEphemeralOp(rId, plans.Open) }) From c562e0c57448b8887315c2f85baf9ffa443916ea Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 7 Nov 2024 08:19:37 -0500 Subject: [PATCH 4/4] Update internal/terraform/context_plan_ephemeral_test.go Co-authored-by: Radek Simko --- internal/terraform/context_plan_ephemeral_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/terraform/context_plan_ephemeral_test.go b/internal/terraform/context_plan_ephemeral_test.go index 44265798bd70..1762e467fd59 100644 --- a/internal/terraform/context_plan_ephemeral_test.go +++ b/internal/terraform/context_plan_ephemeral_test.go @@ -665,7 +665,7 @@ ephemeral "ephem_resource" "data" { postcondition { condition = self.value != nil error_message = "should return a value" - } + } } }