From cf0aae24f4cc8456c4cbcc3d21eac124992a8c68 Mon Sep 17 00:00:00 2001 From: Sarah French <15078782+SarahFrench@users.noreply.github.com> Date: Thu, 28 May 2026 10:37:39 +0100 Subject: [PATCH] fix: Stop our anti-`init -upgrade` check panicking when an overridden provider is in use. (#38635) Prior to this commit we wanted to validate the provider upgrade process using data that was only populated during backend initialisation (as that was the only place using that data previously). Now we enable the provider supply mode data to be available before backends are initialised. After making that available, we can update the check after getProvidersFromConfig that protects against the provider version being changed outside of a deliberate state migration process. If the state store provider is overridden then the upgrade process doesn't impact that provider anyway, so the check can be skipped. Also, this protects against errors if there was ever a built in provider supplying a state store implementation. --- internal/command/e2etest/init_test.go | 97 ++++++++++++- .../.terraform.lock.hcl | 6 + .../.terraform/terraform.tfstate | 17 +++ .../main.tf | 21 +++ .../outputs.tf | 3 + .../states/default/terraform.tfstate | 40 ++++++ internal/command/init.go | 10 -- internal/command/init_run.go | 21 ++- internal/command/init_test.go | 130 ++++++++++++++++++ internal/command/meta_backend.go | 11 +- internal/command/meta_config.go | 13 ++ 11 files changed, 346 insertions(+), 23 deletions(-) create mode 100644 internal/command/e2etest/testdata/initialized-directory-with-state-store-unmanaged/.terraform.lock.hcl create mode 100644 internal/command/e2etest/testdata/initialized-directory-with-state-store-unmanaged/.terraform/terraform.tfstate create mode 100644 internal/command/e2etest/testdata/initialized-directory-with-state-store-unmanaged/main.tf create mode 100644 internal/command/e2etest/testdata/initialized-directory-with-state-store-unmanaged/outputs.tf create mode 100644 internal/command/e2etest/testdata/initialized-directory-with-state-store-unmanaged/states/default/terraform.tfstate diff --git a/internal/command/e2etest/init_test.go b/internal/command/e2etest/init_test.go index 57ceec6214..dc2c0afdc8 100644 --- a/internal/command/e2etest/init_test.go +++ b/internal/command/e2etest/init_test.go @@ -14,7 +14,9 @@ import ( "github.com/google/go-cmp/cmp" + "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/e2e" + "github.com/hashicorp/terraform/internal/getproviders" ) func TestInitProviders(t *testing.T) { @@ -55,7 +57,6 @@ func TestInitProviders(t *testing.T) { if !strings.Contains(stdout, "Terraform has created a lock file") { t.Errorf("lock file notification is missing from output:\n%s", stdout) } - } func TestInitProvidersInternal(t *testing.T) { @@ -140,7 +141,6 @@ func TestInitProvidersVendored(t *testing.T) { if !strings.Contains(stdout, "- Installing hashicorp/null v1.0.0+local") { t.Errorf("provider download message is missing from output:\n%s", stdout) } - } func TestInitProvidersLocalOnly(t *testing.T) { @@ -425,7 +425,100 @@ func TestInitProviderWarnings(t *testing.T) { if !strings.Contains(stdout, "This provider is archived and no longer needed.") { t.Errorf("expected warning message is missing from output:\n%s", stdout) } +} + +// This is a regression test asserting that `terraform init -upgrade` doesn't error +// when the state storage provider is unmanaged by Terraform. The check to see if the +// state storage provider was affected by the upgrade process should be skipped when +// the provider is not managed by Terraform. Previously the check wasn't skipped and +// panicked due to how the provider was supplied. +// +// See the TestInit_getUpgradePlugins integration test for similar testing when using a +// dev_override provider. +func TestInitStateStoreUsingUnmanagedProvider(t *testing.T) { + if !canRunGoBuild { + // We're running in a separate-build-then-run context, so we can't + // currently execute this test which depends on being able to build + // new executable at runtime. + // + // (See the comment on canRunGoBuild's declaration for more information.) + t.Skip("can't run without building a new provider executable") + } + + // In temp dir create a plugin cache to be used in the test cases. + // The cache is supplied to commands using the -plugin-dir init flag. + // There are 2 versions of the simple6 provider: 0.0.1 and 2.0.0 + // This enables us to test an upgrade scenario where a newer provider version is available. + td := t.TempDir() + providerVersionOld := "0.0.1" + providerVersionNew := "2.0.0" + platform := getproviders.CurrentPlatform.String() + absolutePathToCache := filepath.Join(td, "cache") + simple6Provider := filepath.Join(td, "terraform-provider-simple6") + simple6ProviderExe := e2e.GoBuild("github.com/hashicorp/terraform/internal/provider-simple-v6/main", simple6Provider) + for _, v := range []string{providerVersionOld, providerVersionNew} { + dir := filepath.Join(absolutePathToCache, "registry.terraform.io/hashicorp", "simple6", v, platform) + if err := os.MkdirAll(dir, os.ModePerm); err != nil { + t.Fatal(err) + } + // Create an executable copy of the simple6ProviderExe file per version in the cache dir + data, err := os.ReadFile(simple6ProviderExe) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "terraform-provider-simple6"), data, 0755); err != nil { + t.Fatal(err) + } + } + t.Setenv(e2e.TestExperimentFlag, "true") + terraformBin := e2e.GoBuild("github.com/hashicorp/terraform", "terraform") + fixturePath := filepath.Join("testdata", "initialized-directory-with-state-store-unmanaged") + tf := e2e.NewBinary(t, terraformBin, fixturePath) + + // Assert the existing lockfile describes the older version of the provider. + lockFile := tf.Path(".terraform.lock.hcl") + buf, err := os.ReadFile(lockFile) + if err != nil { + t.Fatalf("unexpected error accessing lock file: %s", err) + } + buf = bytes.TrimSpace(buf) + + expectedLockFileContent := fmt.Sprintf(`# This file is maintained automatically by "terraform init". +# Manual edits may be lost in future updates. + +provider "registry.terraform.io/hashicorp/simple6" { + version = "%s" +}`, providerVersionOld) + if diff := cmp.Diff(expectedLockFileContent, string(buf)); diff != "" { + t.Errorf("unexpected difference in lock file content: %s", diff) + } + + // The simple6 provider is unmanaged + reattachConfig, _ := reattachedProviderForTest(t, addrs.NewDefaultProvider("simple6"), 6) + tf.AddEnv("TF_REATTACH_PROVIDERS=" + reattachConfig) + + // The init -upgrade process should succeed. + stdout, stderr, err := tf.Run( + "init", + "-upgrade", + "-enable-pluggable-state-storage-experiment", + fmt.Sprintf("-plugin-dir=%s", absolutePathToCache), + ) + if err != nil { + t.Fatalf("unexpected error: %s\nstdout: %s\nstderr: %s", err, stdout, stderr) + } + + // Lockfile should be unchanged because the only provider in the config is unmanaged. + buf, err = os.ReadFile(lockFile) + if err != nil { + t.Fatalf("unexpected error accessing lock file: %s", err) + } + buf = bytes.TrimSpace(buf) + + if diff := cmp.Diff(expectedLockFileContent, string(buf)); diff != "" { + t.Errorf("unexpected difference in lock file content: %s", diff) + } } // emptyConfigFileForTests creates a blank .terraformrc file in the requested diff --git a/internal/command/e2etest/testdata/initialized-directory-with-state-store-unmanaged/.terraform.lock.hcl b/internal/command/e2etest/testdata/initialized-directory-with-state-store-unmanaged/.terraform.lock.hcl new file mode 100644 index 0000000000..7a0db0a25a --- /dev/null +++ b/internal/command/e2etest/testdata/initialized-directory-with-state-store-unmanaged/.terraform.lock.hcl @@ -0,0 +1,6 @@ +# This file is maintained automatically by "terraform init". +# Manual edits may be lost in future updates. + +provider "registry.terraform.io/hashicorp/simple6" { + version = "0.0.1" +} diff --git a/internal/command/e2etest/testdata/initialized-directory-with-state-store-unmanaged/.terraform/terraform.tfstate b/internal/command/e2etest/testdata/initialized-directory-with-state-store-unmanaged/.terraform/terraform.tfstate new file mode 100644 index 0000000000..009467c3c7 --- /dev/null +++ b/internal/command/e2etest/testdata/initialized-directory-with-state-store-unmanaged/.terraform/terraform.tfstate @@ -0,0 +1,17 @@ +{ + "version": 3, + "terraform_version": "1.15.0", + "state_store": { + "type": "simple6_fs", + "provider": { + "version": "0.0.1", + "source": "registry.terraform.io/hashicorp/simple6", + "config": {} + }, + "config": { + "workspace_dir": "states" + }, + "hash": 3942813381, + "provider_supply_mode": "reattached" + } +} \ No newline at end of file diff --git a/internal/command/e2etest/testdata/initialized-directory-with-state-store-unmanaged/main.tf b/internal/command/e2etest/testdata/initialized-directory-with-state-store-unmanaged/main.tf new file mode 100644 index 0000000000..f20f555669 --- /dev/null +++ b/internal/command/e2etest/testdata/initialized-directory-with-state-store-unmanaged/main.tf @@ -0,0 +1,21 @@ +terraform { + required_providers { + simple6 = { + source = "registry.terraform.io/hashicorp/simple6" + } + } + + state_store "simple6_fs" { + provider "simple6" {} + + workspace_dir = "states" + } +} + +variable "name" { + default = "world" +} + +resource "terraform_data" "my-data" { + input = "hello ${var.name}" +} diff --git a/internal/command/e2etest/testdata/initialized-directory-with-state-store-unmanaged/outputs.tf b/internal/command/e2etest/testdata/initialized-directory-with-state-store-unmanaged/outputs.tf new file mode 100644 index 0000000000..b650136fa1 --- /dev/null +++ b/internal/command/e2etest/testdata/initialized-directory-with-state-store-unmanaged/outputs.tf @@ -0,0 +1,3 @@ +output "greeting" { + value = resource.terraform_data.my-data.output +} diff --git a/internal/command/e2etest/testdata/initialized-directory-with-state-store-unmanaged/states/default/terraform.tfstate b/internal/command/e2etest/testdata/initialized-directory-with-state-store-unmanaged/states/default/terraform.tfstate new file mode 100644 index 0000000000..4feaaed87a --- /dev/null +++ b/internal/command/e2etest/testdata/initialized-directory-with-state-store-unmanaged/states/default/terraform.tfstate @@ -0,0 +1,40 @@ +{ + "version": 4, + "terraform_version": "1.15.0", + "serial": 1, + "lineage": "9e13d881-e480-7a63-d47a-b4f5224e6743", + "outputs": { + "greeting": { + "value": "hello world", + "type": "string" + } + }, + "resources": [ + { + "mode": "managed", + "type": "terraform_data", + "name": "my-data", + "provider": "provider[\"terraform.io/builtin/terraform\"]", + "instances": [ + { + "schema_version": 0, + "attributes": { + "id": "d71fb368-2ba1-fb4c-5bd9-6a2b7f05d60c", + "input": { + "value": "hello world", + "type": "string" + }, + "output": { + "value": "hello world", + "type": "string" + }, + "triggers_replace": null + }, + "sensitive_attributes": [], + "identity_schema_version": 0 + } + ] + } + ], + "check_results": null +} \ No newline at end of file diff --git a/internal/command/init.go b/internal/command/init.go index ff5a64a4f7..e8b26ce095 100644 --- a/internal/command/init.go +++ b/internal/command/init.go @@ -8,7 +8,6 @@ import ( "fmt" "log" "maps" - "os" "reflect" "slices" "sort" @@ -32,7 +31,6 @@ import ( "github.com/hashicorp/terraform/internal/didyoumean" "github.com/hashicorp/terraform/internal/getproviders" "github.com/hashicorp/terraform/internal/getproviders/providerreqs" - "github.com/hashicorp/terraform/internal/getproviders/reattach" "github.com/hashicorp/terraform/internal/providercache" "github.com/hashicorp/terraform/internal/states" "github.com/hashicorp/terraform/internal/tfdiags" @@ -233,14 +231,6 @@ func (c *InitCommand) initBackend(ctx context.Context, root *configs.Module, ini } } - // Annotate state_store config representation with info about how the provider - // is supplied to Terraform. - isReattached, err := reattach.IsProviderReattached(root.StateStore.ProviderAddr, os.Getenv("TF_REATTACH_PROVIDERS")) - if err != nil { - panic(fmt.Sprintf("Unable to determine if provider %s is reattached while initializing the state store. This is a bug in Terraform and should be reported: %v", root.StateStore.ProviderAddr.ForDisplay(), err)) - } - root.StateStore.ProviderSupplyMode = getproviders.DetermineProviderSupplyMode(c.Meta.isProviderDevOverride(root.StateStore.ProviderAddr), isReattached, root.StateStore.ProviderAddr.IsBuiltIn()) - opts = &BackendOpts{ StateStoreConfig: root.StateStore, Locks: configLocks, diff --git a/internal/command/init_run.go b/internal/command/init_run.go index 22c95c9056..5636e8a6d0 100644 --- a/internal/command/init_run.go +++ b/internal/command/init_run.go @@ -167,6 +167,13 @@ func (c *InitCommand) run(initArgs *arguments.Init, view views.Init) int { return 1 } + if rootModEarly.StateStore != nil { // We know rootModEarly is not nil. + rootModEarly.StateStore.ProviderSupplyMode = c.Meta.getProviderSupplyModeForStateStore(rootModEarly) + if rootModEarly.StateStore.ProviderSupplyMode == getproviders.Unset { + panic("unset provider supply mode for state store") + } + } + if initArgs.Get { modsOutput, modsAbort, modsDiags := c.getModules(ctx, path, initArgs.TestsDirectory, rootModEarly, initArgs.Upgrade, view) diags = diags.Append(modsDiags) @@ -182,6 +189,12 @@ func (c *InitCommand) run(initArgs *arguments.Init, view views.Init) int { // With all of the modules (hopefully) installed, we can now try to load the // whole configuration tree. config, confDiags := c.loadConfigWithTests(path, initArgs.TestsDirectory) + if config != nil && config.Module != nil && config.Module.StateStore != nil { + config.Module.StateStore.ProviderSupplyMode = c.Meta.getProviderSupplyModeForStateStore(config.Module) + if config.Module.StateStore.ProviderSupplyMode == getproviders.Unset { + panic("unset provider supply mode for state store") + } + } // configDiags will be handled after: // - the version constraint check has happened // - and, the backend/state_store is initialised @@ -304,7 +317,13 @@ func (c *InitCommand) run(initArgs *arguments.Init, view views.Init) int { // The init command is not allowed to upgrade the provider used for state storage (unless we're reconfiguring the state store). // Unless users choose to reconfigure, they must upgrade the state store provider separately using `terraform state migrate -upgrade`. - if initArgs.Upgrade && !initArgs.Reconfigure && config.Module.StateStore != nil { + // We only check to see if the state store provider was upgraded if the provider supply mode is ManagedByTerraform; providers overridden + // using dev_override or unmanaged providers blocks any upgrade process impacting that provider. + // For more context, see: https://github.com/hashicorp/terraform/pull/38633 + if initArgs.Upgrade && + !initArgs.Reconfigure && + config.Module.StateStore != nil && + config.Module.StateStore.ProviderSupplyMode == getproviders.ManagedByTerraform { pAddr := config.Module.StateStore.ProviderAddr old := alteredPreviousLocks.Provider(pAddr) new := configLocks.Provider(pAddr) diff --git a/internal/command/init_test.go b/internal/command/init_test.go index 5e433d6e71..3715622f95 100644 --- a/internal/command/init_test.go +++ b/internal/command/init_test.go @@ -2610,6 +2610,136 @@ terraform { } }) + // A dev_override setting stops a provider being upgraded. That behaviour is tested elsewhere. + // This test is a regression test asserting that checks ensuring the state storage provider wasn't + // upgraded during `init -upgrade` doesn't panic when the state storage provider is a dev_override. + // + // An equivalent test for this scenario where the state storage provider is unmanaged is implemented + // as an E2E test, as that's the only place unmanaged providers can be used in tests. + t.Run("no errors if `init -upgrade` is run while the state store provider is a dev_override ", func(t *testing.T) { + // Create a temporary working directory and copy in test fixtures + td := t.TempDir() + t.Chdir(td) + + // Configuration uses a state store and has other provider requirements. + cfg := ` +terraform { + + required_providers { + test = { + source = "hashicorp/test" + version = "> 1.0.0" + } + } + state_store "test_store" { + provider "test" { + } + + value = "foobar" + } +}` + if err := os.WriteFile("main.tf", []byte(cfg), 0644); err != nil { + t.Fatalf("failed to write main.tf: %s", err) + } + + providerSource := newMockProviderSource(t, map[string][]string{ + // config requires > 1.0.0 + "test": {"1.2.3", "9.9.9"}, + }) + + // Mock provider to act as "hashicorp/test" + mockProvider := mockPluggableStateStorageProvider() + mockProviderAddress := addrs.NewDefaultProvider("test") + ui := new(cli.MockUi) + view, done := testView(t) + m := Meta{ + testingOverrides: metaOverridesForProvider(mockProvider), + Ui: ui, + View: view, + ProviderSource: providerSource, + AllowExperimentalFeatures: true, + + // THIS ALLOWS THE TEST TO MIMIC A SCENARIO WHERE THE PROVIDER IS A DEV OVERRIDE. + // The mock is still accessed via testingOverrides, but Terraform believes that it's a dev override due to + // its presence in the ProviderDevOverrides map. + ProviderDevOverrides: map[addrs.Provider]getproviders.PackageLocalDir{ + mockProviderAddress: ".", + }, + } + + // Make Terraform believe that we already have version 1.2.3 installed. + installFakeProviderPackages(t, &m, map[string][]string{ + "test": {"1.2.3"}, + }) + // Create a dependency lock file describing the hashicorp/test provider at version 1.2.3, to simulate a previous init with that version. + locks := depsfile.NewLocks() + locks.SetProvider( + addrs.NewDefaultProvider("test"), + getproviders.MustParseVersion("1.2.3"), + getproviders.MustParseVersionConstraints("> 1.0.0"), + []getproviders.Hash{ + getproviders.HashScheme1.New("wlbEC2mChQZ2hhgUhl6SeVLPP7fMqOFUZAQhQ9GIIno="), + }, + ) + if err := depsfile.SaveLocksToFile(locks, ".terraform.lock.hcl"); err != nil { + t.Fatalf("failed to write provider locks file: %s", err) + } + + c := &InitCommand{ + Meta: m, + } + + args := []string{ + "-upgrade=true", + "-enable-pluggable-state-storage-experiment", + } + code := c.Run(args) + if code != 0 { + t.Fatalf("expected code 0, but got code %d. \nOutput:\n%s", code, done(t).All()) + } + + // Assert that no providers were upgraded. + // + // Unlike other test scenarios, "test" v9.9.9 will not be installed in the cache. + // This is because the installation process skips installing providers that are + // dev_overrides. That logic is used in both upgrade and non-upgrade operations. + cacheDir := m.providerLocalCacheDir() + gotPackages := cacheDir.AllAvailablePackages() + wantPackages := map[addrs.Provider][]providercache.CachedProvider{ + mockProviderAddress: { + // No v9.9.9 entry + { + Provider: mockProviderAddress, + Version: getproviders.MustParseVersion("1.2.3"), + PackageDir: expectedPackageInstallPath("test", "1.2.3", false), + }, + }, + } + if diff := cmp.Diff(wantPackages, gotPackages); diff != "" { + t.Errorf("wrong cache directory contents after upgrade\n%s", diff) + } + + // The provider locks should not have changed. + locks, err := m.lockedDependencies() + if err != nil { + t.Fatalf("failed to get locked dependencies: %s", err) + } + gotProviderLocks := locks.AllProviders() + wantProviderLocks := map[addrs.Provider]*depsfile.ProviderLock{ + mockProviderAddress: depsfile.NewProviderLock( + mockProviderAddress, + getproviders.MustParseVersion("1.2.3"), + getproviders.MustParseVersionConstraints("> 1.0.0"), + []getproviders.Hash{ + getproviders.HashScheme1.New("wlbEC2mChQZ2hhgUhl6SeVLPP7fMqOFUZAQhQ9GIIno="), + }, + ), + } + if diff := cmp.Diff(gotProviderLocks, wantProviderLocks, depsfile.ProviderLockComparer); diff != "" { + t.Errorf("wrong version selections after upgrade\n%s", diff) + } + }) + t.Run("`init -upgrade -reconfigure` can be used to upgrade the state store provider", func(t *testing.T) { // Create a temporary working directory and copy in test fixtures td := t.TempDir() diff --git a/internal/command/meta_backend.go b/internal/command/meta_backend.go index ddfdba42cd..d20e805af9 100644 --- a/internal/command/meta_backend.go +++ b/internal/command/meta_backend.go @@ -14,7 +14,6 @@ import ( "fmt" "log" "maps" - "os" "path/filepath" "slices" "strconv" @@ -42,7 +41,6 @@ import ( "github.com/hashicorp/terraform/internal/didyoumean" "github.com/hashicorp/terraform/internal/getproviders" "github.com/hashicorp/terraform/internal/getproviders/providerreqs" - "github.com/hashicorp/terraform/internal/getproviders/reattach" "github.com/hashicorp/terraform/internal/plans" "github.com/hashicorp/terraform/internal/providers" "github.com/hashicorp/terraform/internal/states/statemgr" @@ -2094,19 +2092,12 @@ func (m *Meta) backend(configPath string, viewType arguments.ViewType) (backendr ViewType: viewType, } case root.StateStore != nil: - // Annotate state_store config representation with info about how the provider - // is supplied to Terraform. - isReattached, err := reattach.IsProviderReattached(root.StateStore.ProviderAddr, os.Getenv("TF_REATTACH_PROVIDERS")) - if err != nil { - panic(fmt.Sprintf("Unable to determine if provider %s is reattached while initializing the state store. This is a bug in Terraform and should be reported: %v", root.StateStore.ProviderAddr.ForDisplay(), err)) - } - root.StateStore.ProviderSupplyMode = getproviders.DetermineProviderSupplyMode(m.isProviderDevOverride(root.StateStore.ProviderAddr), isReattached, root.StateStore.ProviderAddr.IsBuiltIn()) - // Check the provider for state storage is present, either via the dependency lock file or // supplied via developer overrides, reattach config, or being built-in. // // Remember, the (Meta).backend method is used for non-init commands, so we expect dependency locks // to be present or for the provider to be otherwise available, e.g. via reattach config. + root.StateStore.ProviderSupplyMode = m.getProviderSupplyModeForStateStore(root) depsDiags := root.StateStore.VerifyDependencySelection(locks, root.ProviderRequirements, root.StateStore.ProviderSupplyMode) diags = diags.Append(depsDiags) if depsDiags.HasErrors() { diff --git a/internal/command/meta_config.go b/internal/command/meta_config.go index c476378ff1..7872adfac0 100644 --- a/internal/command/meta_config.go +++ b/internal/command/meta_config.go @@ -22,6 +22,8 @@ import ( "github.com/hashicorp/terraform/internal/configs" "github.com/hashicorp/terraform/internal/configs/configload" "github.com/hashicorp/terraform/internal/configs/configschema" + "github.com/hashicorp/terraform/internal/getproviders" + "github.com/hashicorp/terraform/internal/getproviders/reattach" "github.com/hashicorp/terraform/internal/initwd" "github.com/hashicorp/terraform/internal/registry" "github.com/hashicorp/terraform/internal/terraform" @@ -484,6 +486,17 @@ func (m *Meta) registryClient() *registry.Client { return registry.NewClient(m.Services, nil) } +func (m *Meta) getProviderSupplyModeForStateStore(config *configs.Module) getproviders.ProviderSupplyMode { + if config == nil || config.StateStore == nil { + return getproviders.Unset + } + isReattached, err := reattach.IsProviderReattached(config.StateStore.ProviderAddr, os.Getenv("TF_REATTACH_PROVIDERS")) + if err != nil { + panic(fmt.Sprintf("Unable to determine if provider %s is reattached while initializing the state store. This is a bug in Terraform and should be reported: %v", config.StateStore.ProviderAddr.ForDisplay(), err)) + } + return getproviders.DetermineProviderSupplyMode(m.isProviderDevOverride(config.StateStore.ProviderAddr), isReattached, config.StateStore.ProviderAddr.IsBuiltIn()) +} + // configValueFromCLI parses a configuration value that was provided in a // context in the CLI where only strings can be provided, such as on the // command line or in an environment variable, and returns the resulting