Fix regression of backend reinit detection when backend schema has required arguments (#2119)

Signed-off-by: Martin Atkins <mart@degeneration.co.uk>

internal/backend/init/init.go

@@ -54,6 +54,10 @@ func Init(services *disco.Disco) {
 	backendsLock.Lock()
 	defer backendsLock.Unlock()
 
+	// NOTE: Underscore-prefixed named are reserved for unit testing use via
+	// the RegisterTemp function. Do not add any underscore-prefixed names
+	// to the following table.
+
 	backends = map[string]backend.InitFn{
 		"local":  func(enc encryption.StateEncryption) backend.Backend { return backendLocal.New(enc) },
 		"remote": func(enc encryption.StateEncryption) backend.Backend { return backendRemote.New(services, enc) },
@@ -100,6 +104,10 @@ func Backend(name string) backend.InitFn {
 // This method sets this backend globally and care should be taken to do
 // this only before OpenTofu is executing to prevent odd behavior of backends
 // changing mid-execution.
+//
+// NOTE: Underscore-prefixed named are reserved for unit testing use via
+// the RegisterTemp function. Do not add any underscore-prefixed names
+// using this function.
 func Set(name string, f backend.InitFn) {
 	backendsLock.Lock()
 	defer backendsLock.Unlock()

internal/backend/init/testing.go

@@ -0,0 +1,204 @@
+// Copyright (c) The OpenTofu Authors
+// SPDX-License-Identifier: MPL-2.0
+// Copyright (c) 2023 HashiCorp, Inc.
+// SPDX-License-Identifier: MPL-2.0
+
+package init
+
+import (
+	"fmt"
+	"strings"
+
+	"github.com/opentofu/opentofu/internal/backend"
+	"github.com/opentofu/opentofu/internal/configs/configschema"
+	"github.com/opentofu/opentofu/internal/states/statemgr"
+	"github.com/opentofu/opentofu/internal/tfdiags"
+	"github.com/zclconf/go-cty/cty"
+)
+
+// RegisterTemp adds a new entry to the table of backends and returns a
+// function that will deregister it once called.
+//
+// This is essentially a workaround for the fact that the OpenTofu CLI
+// layer expects backends to always come from a centrally-maintained table
+// and doesn't have any way to directly pass an anonymous backend
+// implementation.
+//
+// The given name MUST start with an underscore, to ensure that it cannot
+// collide with any "real" backend. If the given name does not start with
+// an underscore then this function will panic. If we introduce plugin-based
+// backends in future then we might consider reserving part of the plugin
+// address namespace to represent temporary backends for test purposes only,
+// which would then replace this special underscore prefix as the differentiator.
+//
+// This is intended for unit tests that use [MockBackend], or a derivative
+// thereof. A typical usage pattern from a unit test would be:
+//
+//	// ("backendInit" represents _this_ package)
+//	t.Cleanup(backendInit.RegisterTemp("_test", func (enc encryption.StateEncryption) Backend {
+//		return &backendInit.MockBackend{
+//			// (and then whichever mock settings your test case needs)
+//		}
+//	}))
+//
+// Because this function modifies global state observable throughout the
+// program, any test using this function MUST NOT use t.Parallel. If a
+// single test needs to register multiple temporary backends for some reason
+// then it must select a different name for each one.
+func RegisterTemp(name string, f backend.InitFn) func() {
+	// FIXME: It would be better to add a map of backends to command.Meta's existing
+	// "testingOverrides" field, but at the time of writing direct calls to this
+	// package's func Backend are made from too many different places in the
+	// codebase to retrofit "testing overrides" without considerable risk to
+	// already-working code, so for now we compromise and just offer this helper
+	// function to hopefully help tests properly manage their temporary addition
+	// to the global backend table.
+
+	if !strings.HasPrefix(name, "_") {
+		panic("temporary backend name must begin with underscore")
+	}
+	backendsLock.Lock()
+	defer backendsLock.Unlock()
+
+	if _, exists := backends[name]; exists {
+		// If we get in here then it suggests one of the following mistakes in
+		// the calling code:
+		// - Using RegisterTemp in a test case that uses t.Parallel.
+		// - Forgetting to call the cleanup function in some other earlier test that
+		//   happened to choose the same temporary name.
+		// - Registering more than one temporary backend in a single test without
+		//   assigning each one a unique name.
+		panic(fmt.Sprintf("there is already a temporary backend named %q", name))
+	}
+
+	// The given init function is temporarily added to the global table, so that
+	// the CLI package can find it using the given (underscore-prefixed) name.
+	backends[name] = f
+
+	return func() {
+		backendsLock.Lock()
+		delete(backends, name)
+		backendsLock.Unlock()
+	}
+}
+
+// MockBackend is an implementation of [Backend] that largely just routes incoming
+// calls to a set of closures provided by a caller.
+//
+// This is included for testing purposes only. Use [RegisterTemp] to
+// temporarily add a MockBackend instance to the table of available backends
+// from a unit test function. Do not include MockBackend instances in the
+// initial static backend table.
+//
+// The mock automatically tracks the most recent call to each method for ease
+// of writing assertions in simple cases. If you need more complex tracking such
+// as a log of all calls then you can implement that inside your provided callback
+// functions.
+//
+// This implementation intentionally covers only the basic [Backend] interface,
+// and not any extension interfaces like [CLI] and [Enhanced]. Consider embedding
+// this into another type if you need to mock extension interfaces too, since
+// OpenTofu backend init uses type assertions to check for extension interfaces
+// and so having this type implement them would prevent its use in testing
+// situations that occur with non-extended backend implementations.
+type MockBackend struct {
+	// If you add support for new methods here in future, please preserve the
+	// alphabetical order by function name and the other naming suffix conventions
+	// for each field.
+
+	ConfigSchemaFn     func() *configschema.Block
+	ConfigSchemaCalled bool
+
+	ConfigureFn        func(configObj cty.Value) tfdiags.Diagnostics
+	ConfigureCalled    bool
+	ConfigureConfigObj cty.Value
+
+	DeleteWorkspaceFn     func(name string, force bool) error
+	DeleteWorkspaceCalled bool
+	DeleteWorkspaceName   string
+	DeleteWorkspaceForce  bool
+
+	PrepareConfigFn        func(configObj cty.Value) (cty.Value, tfdiags.Diagnostics)
+	PrepareConfigCalled    bool
+	PrepareConfigConfigObj cty.Value
+
+	StateMgrFn        func(workspace string) (statemgr.Full, error)
+	StateMgrCalled    bool
+	StateMgrWorkspace string
+
+	WorkspacesFn     func() ([]string, error)
+	WorkspacesCalled bool
+}
+
+var _ backend.Backend = (*MockBackend)(nil)
+
+// ConfigSchema implements Backend.
+func (m *MockBackend) ConfigSchema() *configschema.Block {
+	m.ConfigSchemaCalled = true
+
+	if m.ConfigSchemaFn == nil {
+		// Default behavior: return an empty schema
+		return &configschema.Block{}
+	}
+	return m.ConfigSchemaFn()
+}
+
+// Configure implements Backend.
+func (m *MockBackend) Configure(configObj cty.Value) tfdiags.Diagnostics {
+	m.ConfigureCalled = true
+	m.ConfigureConfigObj = configObj
+
+	if m.ConfigureFn == nil {
+		// Default behavior: do nothing at all, and report success
+		return nil
+	}
+	return m.ConfigureFn(configObj)
+}
+
+// DeleteWorkspace implements Backend.
+func (m *MockBackend) DeleteWorkspace(name string, force bool) error {
+	m.DeleteWorkspaceCalled = true
+	m.DeleteWorkspaceName = name
+	m.DeleteWorkspaceForce = force
+
+	if m.DeleteWorkspaceFn == nil {
+		// Default behavior: do nothing at all, and report success
+		return nil
+	}
+	return m.DeleteWorkspaceFn(name, force)
+}
+
+// PrepareConfig implements Backend.
+func (m *MockBackend) PrepareConfig(configObj cty.Value) (cty.Value, tfdiags.Diagnostics) {
+	m.PrepareConfigCalled = true
+	m.PrepareConfigConfigObj = configObj
+
+	if m.PrepareConfigFn == nil {
+		// Default behavior: just echo back the given config object and indicate success
+		return configObj, nil
+	}
+	return m.PrepareConfigFn(configObj)
+}
+
+// StateMgr implements Backend.
+func (m *MockBackend) StateMgr(workspace string) (statemgr.Full, error) {
+	m.StateMgrCalled = true
+	m.StateMgrWorkspace = workspace
+
+	if m.StateMgrFn == nil {
+		// Default behavior: fail as if there is no workspace of the given name
+		return nil, fmt.Errorf("no workspace named %q", workspace)
+	}
+	return m.StateMgrFn(workspace)
+}
+
+// Workspaces implements Backend.
+func (m *MockBackend) Workspaces() ([]string, error) {
+	m.WorkspacesCalled = true
+
+	if m.WorkspacesFn == nil {
+		// Default behavior: report no workspaces at all.
+		return nil, nil
+	}
+	return m.WorkspacesFn()
+}

internal/command/meta_backend.go

@@ -1361,7 +1361,12 @@ func (m *Meta) backendConfigNeedsMigration(c *configs.Backend, s *legacy.Backend
 	}
 	b := f(nil) // We don't need encryption here as it's only used for config/schema
 
-	schema := b.ConfigSchema()
+	// We use "NoneRequired" here because we're only evaluating the body written directly
+	// in the root module configuration, and we're intentionally not including any
+	// additional arguments passed on the command line (using -backend-config), so
+	// some of the required arguments might be satisfied from outside of the body we're
+	// evaluating here.
+	schema := b.ConfigSchema().NoneRequired()
 	givenVal, diags := c.Decode(schema)
 	if diags.HasErrors() {
 		log.Printf("[TRACE] backendConfigNeedsMigration: failed to decode given config; migration codepath must handle problem: %s", diags.Error())

internal/command/meta_backend_test.go

@@ -22,6 +22,7 @@ import (
 	"github.com/opentofu/opentofu/internal/addrs"
 	"github.com/opentofu/opentofu/internal/backend"
 	"github.com/opentofu/opentofu/internal/configs"
+	"github.com/opentofu/opentofu/internal/configs/configschema"
 	"github.com/opentofu/opentofu/internal/copy"
 	"github.com/opentofu/opentofu/internal/encryption"
 	"github.com/opentofu/opentofu/internal/plans"
@@ -682,6 +683,53 @@ func TestMetaBackend_configuredUnchangedWithStaticEvalVars(t *testing.T) {
 
 	defer testChdir(t, testFixturePath("backend-unchanged-vars"))()
 
+	// We'll use a mock backend here because we need to control the schema to
+	// make sure that we always have a required field for the ConfigOverride
+	// argument to populate. This is covering the regression caused by the first
+	// fix to the original bug, discussed here:
+	//    https://github.com/opentofu/opentofu/issues/2118
+	t.Cleanup(
+		backendInit.RegisterTemp("_test_local", func(enc encryption.StateEncryption) backend.Backend {
+			return &backendInit.MockBackend{
+				ConfigSchemaFn: func() *configschema.Block {
+					// The following is based on a subset of the normal "local"
+					// backend at the time of writing this test, but subsetted
+					// to only what we need and with all of the arguments
+					// marked as required (even though the real backend doesn't)
+					// so we can make sure that we handle required arguments
+					// properly.
+					return &configschema.Block{
+						Attributes: map[string]*configschema.Attribute{
+							"path": {
+								Type:     cty.String,
+								Required: true,
+								// We'll set this one in the root module, using early eval.
+							},
+							"workspace_dir": {
+								Type:     cty.String,
+								Required: true,
+								// We'll treat this one as if it were set with the -backend-config option to "tofu init"
+							},
+						},
+					}
+				},
+				WorkspacesFn: func() ([]string, error) {
+					return []string{"default"}, nil
+				},
+				StateMgrFn: func(workspace string) (statemgr.Full, error) {
+					// The migration-detection code actually fetches the state to
+					// decide if it's "empty" so it can avoid proposing to migrate
+					// an empty state, and so unfortunately we do need to have
+					// a relatively-realistic implementation of this. We'll
+					// just use the same filesystem-based implementation that
+					// the real local backend would use, but fixed to use our
+					// local-state.tfstate file from the test fixture.
+					return statemgr.NewFilesystem("local-state.tfstate", enc), nil
+				},
+			}
+		}),
+	)
+
 	// Setup the meta
 	m := testMetaBackend(t, nil)
 	// testMetaBackend normally sets migrateState on, because most of the tests

internal/command/testdata/backend-unchanged-vars/.terraform/terraform.tfstate

@@ -3,12 +3,12 @@
     "serial": 1,
     "lineage": "666f9301-7e65-4b19-ae23-71184bb19b03",
     "backend": {
-        "type": "local",
+        "type": "_test_local",
         "config": {
             "path": "local-state.tfstate",
             "workspace_dir": "doesnt-actually-matter-what-this-is"
         },
-        "hash": 4282859327
+        "hash": 117044127
     },
     "modules": [
         {

internal/command/testdata/backend-unchanged-vars/main.tf

@@ -4,7 +4,7 @@ variable "state_filename" {
 }
 
 terraform {
-  backend "local" {
+  backend "_test_local" {
     path = var.state_filename
   }
 }