From 23395a102299025aff0d4e55e29f6d92fc4df2ef Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 16 Dec 2021 15:57:47 -0800 Subject: [PATCH] providercache: Discard lock entries for unused providers Previously we would only ever add new lock entries or update existing ones. However, it's possible that over time a module may _cease_ using a particular provider, at which point we ought to remove it from the lock file so that operations won't fail when seeing that the provider cache directory is inconsistent with the lock file. Now the provider installer (EnsureProviderVersions) will remove any lock file entries that relate to providers not included in the given requirements, which therefore makes the resulting lock file properly match the set of packages the installer wrote into the cache. This does potentially mean that someone could inadvertently defeat the lock by removing a provider dependency, running "terraform init", then undoing that removal, and finally running "terraform init" again. However, that seems relatively unlikely compared to the likelihood of removing a provider and keeping it removed, and in the event it _did_ happen the changes to the lock entry for that provider would be visible in the diff of the provider lock file as usual, and so could be noticed in code review just as for any other change to dependencies. --- internal/command/init_test.go | 23 ++++ .../testdata/init-provider-now-unused/main.tf | 3 + internal/depsfile/locks.go | 17 +++ internal/depsfile/locks_test.go | 78 +++++++++++ internal/providercache/installer.go | 16 ++- internal/providercache/installer_test.go | 124 ++++++++++++++++++ .../docs/language/files/dependency-lock.mdx | 52 ++++++++ 7 files changed, 311 insertions(+), 2 deletions(-) create mode 100644 internal/command/testdata/init-provider-now-unused/main.tf diff --git a/internal/command/init_test.go b/internal/command/init_test.go index a96e0e9d6..d23625cec 100644 --- a/internal/command/init_test.go +++ b/internal/command/init_test.go @@ -2203,6 +2203,11 @@ provider "registry.terraform.io/hashicorp/test" { "zh:e919b507a91e23a00da5c2c4d0b64bcc7900b68d43b3951ac0f6e5d80387fbdc", ] } +`) + + emptyUpdatedLockFile := strings.TrimSpace(` +# This file is maintained automatically by "terraform init". +# Manual edits may be lost in future updates. `) cases := []struct { @@ -2223,6 +2228,15 @@ provider "registry.terraform.io/hashicorp/test" { ok: true, want: updatedLockFile, }, + { + desc: "unused provider", + fixture: "init-provider-now-unused", + providers: map[string][]string{"test": {"1.2.3"}}, + input: inputLockFile, + args: []string{}, + ok: true, + want: emptyUpdatedLockFile, + }, { desc: "readonly", fixture: "init-provider-lock-file", @@ -2232,6 +2246,15 @@ provider "registry.terraform.io/hashicorp/test" { ok: true, want: inputLockFile, }, + { + desc: "unused provider readonly", + fixture: "init-provider-now-unused", + providers: map[string][]string{"test": {"1.2.3"}}, + input: inputLockFile, + args: []string{"-lockfile=readonly"}, + ok: false, + want: inputLockFile, + }, { desc: "conflict", fixture: "init-provider-lock-file", diff --git a/internal/command/testdata/init-provider-now-unused/main.tf b/internal/command/testdata/init-provider-now-unused/main.tf new file mode 100644 index 000000000..e1f1ec18b --- /dev/null +++ b/internal/command/testdata/init-provider-now-unused/main.tf @@ -0,0 +1,3 @@ +# Intentionally blank, but intended to be used in a test case which +# uses an input lock file which already had an entry for the hashicorp/test +# provider, and should therefore detect it as no longer used. diff --git a/internal/depsfile/locks.go b/internal/depsfile/locks.go index 0dec32a1d..ebd5a632a 100644 --- a/internal/depsfile/locks.go +++ b/internal/depsfile/locks.go @@ -98,6 +98,23 @@ func (l *Locks) SetProvider(addr addrs.Provider, version getproviders.Version, c return new } +// RemoveProvider removes any existing lock file entry for the given provider. +// +// If the given provider did not already have a lock entry, RemoveProvider is +// a no-op. +// +// Only lockable providers can be passed to this method. If you pass a +// non-lockable provider address then this function will panic. Use +// function ProviderIsLockable to determine whether a particular provider +// should participate in the version locking mechanism. +func (l *Locks) RemoveProvider(addr addrs.Provider) { + if !ProviderIsLockable(addr) { + panic(fmt.Sprintf("Locks.RemoveProvider with non-lockable provider %s", addr)) + } + + delete(l.providers, addr) +} + // SetProviderOverridden records that this particular Terraform process will // not pay attention to the recorded lock entry for the given provider, and // will instead access that provider's functionality in some other special diff --git a/internal/depsfile/locks_test.go b/internal/depsfile/locks_test.go index 387270568..e82f9e117 100644 --- a/internal/depsfile/locks_test.go +++ b/internal/depsfile/locks_test.go @@ -3,6 +3,7 @@ package depsfile import ( "testing" + "github.com/google/go-cmp/cmp" "github.com/hashicorp/terraform/internal/addrs" "github.com/hashicorp/terraform/internal/getproviders" ) @@ -138,3 +139,80 @@ func TestLocksEqualProviderAddress(t *testing.T) { equalProviderAddressBothWays(t, a, b) }) } + +func TestLocksProviderSetRemove(t *testing.T) { + beepProvider := addrs.NewDefaultProvider("beep") + boopProvider := addrs.NewDefaultProvider("boop") + v2 := getproviders.MustParseVersion("2.0.0") + v2EqConstraints := getproviders.MustParseVersionConstraints("2.0.0") + v2GtConstraints := getproviders.MustParseVersionConstraints(">= 2.0.0") + hash := getproviders.HashScheme("test").New("1") + + locks := NewLocks() + if got, want := len(locks.AllProviders()), 0; got != want { + t.Fatalf("fresh locks object already has providers") + } + + locks.SetProvider(boopProvider, v2, v2EqConstraints, []getproviders.Hash{hash}) + { + got := locks.AllProviders() + want := map[addrs.Provider]*ProviderLock{ + boopProvider: { + addr: boopProvider, + version: v2, + versionConstraints: v2EqConstraints, + hashes: []getproviders.Hash{hash}, + }, + } + if diff := cmp.Diff(want, got, ProviderLockComparer); diff != "" { + t.Fatalf("wrong providers after SetProvider boop\n%s", diff) + } + } + + locks.SetProvider(beepProvider, v2, v2GtConstraints, []getproviders.Hash{hash}) + { + got := locks.AllProviders() + want := map[addrs.Provider]*ProviderLock{ + boopProvider: { + addr: boopProvider, + version: v2, + versionConstraints: v2EqConstraints, + hashes: []getproviders.Hash{hash}, + }, + beepProvider: { + addr: beepProvider, + version: v2, + versionConstraints: v2GtConstraints, + hashes: []getproviders.Hash{hash}, + }, + } + if diff := cmp.Diff(want, got, ProviderLockComparer); diff != "" { + t.Fatalf("wrong providers after SetProvider beep\n%s", diff) + } + } + + locks.RemoveProvider(boopProvider) + { + got := locks.AllProviders() + want := map[addrs.Provider]*ProviderLock{ + beepProvider: { + addr: beepProvider, + version: v2, + versionConstraints: v2GtConstraints, + hashes: []getproviders.Hash{hash}, + }, + } + if diff := cmp.Diff(want, got, ProviderLockComparer); diff != "" { + t.Fatalf("wrong providers after RemoveProvider boop\n%s", diff) + } + } + + locks.RemoveProvider(beepProvider) + { + got := locks.AllProviders() + want := map[addrs.Provider]*ProviderLock{} + if diff := cmp.Diff(want, got, ProviderLockComparer); diff != "" { + t.Fatalf("wrong providers after RemoveProvider beep\n%s", diff) + } + } +} diff --git a/internal/providercache/installer.go b/internal/providercache/installer.go index 0a2032223..e753a73c0 100644 --- a/internal/providercache/installer.go +++ b/internal/providercache/installer.go @@ -162,8 +162,8 @@ func (i *Installer) EnsureProviderVersions(ctx context.Context, locks *depsfile. // We'll work with a copy of the given locks, so we can modify it and // return the updated locks without affecting the caller's object. - // We'll add or replace locks in here during our work so that the final - // locks file reflects what the installer has selected. + // We'll add, replace, or remove locks in here during our work so that the + // final locks file reflects what the installer has selected. locks = locks.DeepCopy() if cb := evts.PendingProviders; cb != nil { @@ -562,6 +562,18 @@ NeedProvider: cb(authResults) } + // Finally, if the lock structure contains locks for any providers that + // are no longer needed by this configuration, we'll remove them. This + // is important because we will not have installed those providers + // above and so a lock file still containing them would make the working + // directory invalid: not every provider in the lock file is available + // for use. + for providerAddr := range locks.AllProviders() { + if _, ok := reqs[providerAddr]; !ok { + locks.RemoveProvider(providerAddr) + } + } + if len(errs) > 0 { return locks, InstallerError{ ProviderErrors: errs, diff --git a/internal/providercache/installer_test.go b/internal/providercache/installer_test.go index e5a8a5e61..f0e04f5d8 100644 --- a/internal/providercache/installer_test.go +++ b/internal/providercache/installer_test.go @@ -824,6 +824,130 @@ func TestEnsureProviderVersions(t *testing.T) { } }, }, + "remove no-longer-needed provider from lock file": { + Source: getproviders.NewMockSource( + []getproviders.PackageMeta{ + { + Provider: beepProvider, + Version: getproviders.MustParseVersion("1.0.0"), + TargetPlatform: fakePlatform, + Location: beepProviderDir, + }, + }, + nil, + ), + LockFile: ` + provider "example.com/foo/beep" { + version = "1.0.0" + constraints = ">= 1.0.0" + hashes = [ + "h1:2y06Ykj0FRneZfGCTxI9wRTori8iB7ZL5kQ6YyEnh84=", + ] + } + provider "example.com/foo/obsolete" { + version = "2.0.0" + constraints = ">= 2.0.0" + hashes = [ + "no:irrelevant", + ] + } + `, + Mode: InstallNewProvidersOnly, + Reqs: getproviders.Requirements{ + beepProvider: getproviders.MustParseVersionConstraints(">= 1.0.0"), + }, + Check: func(t *testing.T, dir *Dir, locks *depsfile.Locks) { + if allCached := dir.AllAvailablePackages(); len(allCached) != 1 { + t.Errorf("wrong number of cache directory entries; want only one\n%s", spew.Sdump(allCached)) + } + if allLocked := locks.AllProviders(); len(allLocked) != 1 { + t.Errorf("wrong number of provider lock entries; want only one\n%s", spew.Sdump(allLocked)) + } + + gotLock := locks.Provider(beepProvider) + wantLock := depsfile.NewProviderLock( + beepProvider, + getproviders.MustParseVersion("1.0.0"), + getproviders.MustParseVersionConstraints(">= 1.0.0"), + []getproviders.Hash{"h1:2y06Ykj0FRneZfGCTxI9wRTori8iB7ZL5kQ6YyEnh84="}, + ) + if diff := cmp.Diff(wantLock, gotLock, depsfile.ProviderLockComparer); diff != "" { + t.Errorf("wrong lock entry\n%s", diff) + } + + gotEntry := dir.ProviderLatestVersion(beepProvider) + wantEntry := &CachedProvider{ + Provider: beepProvider, + Version: getproviders.MustParseVersion("1.0.0"), + PackageDir: filepath.Join(dir.BasePath(), "example.com/foo/beep/1.0.0/bleep_bloop"), + } + if diff := cmp.Diff(wantEntry, gotEntry); diff != "" { + t.Errorf("wrong cache entry\n%s", diff) + } + }, + WantEvents: func(inst *Installer, dir *Dir) map[addrs.Provider][]*testInstallerEventLogItem { + return map[addrs.Provider][]*testInstallerEventLogItem{ + noProvider: { + { + Event: "PendingProviders", + Args: map[addrs.Provider]getproviders.VersionConstraints{ + beepProvider: getproviders.MustParseVersionConstraints(">= 1.0.0"), + }, + }, + { + Event: "ProvidersFetched", + Args: map[addrs.Provider]*getproviders.PackageAuthenticationResult{ + beepProvider: nil, + }, + }, + }, + // Note: intentionally no entries for example.com/foo/obsolete + // here, because it's no longer needed and therefore not + // installed. + beepProvider: { + { + Event: "QueryPackagesBegin", + Provider: beepProvider, + Args: struct { + Constraints string + Locked bool + }{">= 1.0.0", true}, + }, + { + Event: "QueryPackagesSuccess", + Provider: beepProvider, + Args: "1.0.0", + }, + { + Event: "FetchPackageMeta", + Provider: beepProvider, + Args: "1.0.0", + }, + { + Event: "FetchPackageBegin", + Provider: beepProvider, + Args: struct { + Version string + Location getproviders.PackageLocation + }{"1.0.0", beepProviderDir}, + }, + { + Event: "FetchPackageSuccess", + Provider: beepProvider, + Args: struct { + Version string + LocalDir string + AuthResult string + }{ + "1.0.0", + filepath.Join(dir.BasePath(), "example.com/foo/beep/1.0.0/bleep_bloop"), + "unauthenticated", + }, + }, + }, + } + }, + }, "failed install of a non-existing built-in provider": { Source: getproviders.NewMockSource( []getproviders.PackageMeta{}, diff --git a/website/docs/language/files/dependency-lock.mdx b/website/docs/language/files/dependency-lock.mdx index 2f5f092b4..b2b358bda 100644 --- a/website/docs/language/files/dependency-lock.mdx +++ b/website/docs/language/files/dependency-lock.mdx @@ -363,3 +363,55 @@ both `zh:` and `h1:` checksums for each of them in the lock file, thus avoiding the case where Terraform will learn about a `h1:` equivalent only at a later time. See the `terraform providers lock` documentation for more information on this command. + +### Providers that are no longer required + +If you remove the last dependency on a particular provider from your +configuration, then `terraform init` will remove any existing lock file entry +for that provider. + +```diff +--- .terraform.lock.hcl 2020-10-07 16:12:07.539570634 -0700 ++++ .terraform.lock.hcl 2020-10-07 16:12:15.267487237 -0700 +@@ -6,26 +6,6 @@ + ] + } + +-provider "registry.terraform.io/hashicorp/azurerm" { +- version = "2.30.0" +- constraints = "~> 2.12" +- hashes = [ +- "h1:FJwsuowaG5CIdZ0WQyFZH9r6kIJeRKts9+GcRsTz1+Y=", +- "h1:c/ntSXrDYM1mUir2KufijYebPcwKqS9CRGd3duDSGfY=", +- "h1:yre4Ph76g9H84MbuhZ2z5MuldjSA4FsrX6538O7PCcY=", +- "zh:04f0a50bb2ba92f3bea6f0a9e549ace5a4c13ef0cbb6975494cac0ef7d4acb43", +- "zh:2082e12548ebcdd6fd73580e83f626ed4ed13f8cdfd51205d8696ffe54f30734", +- "zh:246bcc449e9a92679fb30f3c0a77f05513886565e2dcc66b16c4486f51533064", +- "zh:24de3930625ac9014594d79bfa42d600eca65e9022b9668b54bfd0d924e21d14", +- "zh:2a22893a576ff6f268d9bf81cf4a56406f7ba79f77826f6df51ee787f6d2840a", +- "zh:2b27485e19c2aaa9f15f29c4cff46154a9720647610171e30fc6c18ddc42ec28", +- "zh:435f24ce1fb2b63f7f02aa3c84ac29c5757cd29ec4d297ed0618423387fe7bd4", +- "zh:7d99725923de5240ff8b34b5510569aa4ebdc0bdb27b7bac2aa911a8037a3893", +- "zh:7e3b5d0af3b7411dd9dc65ec9ab6caee8c191aee0fa7f20fc4f51716e67f50c0", +- "zh:da0af4552bef5a29b88f6a0718253f3bf71ce471c959816eb7602b0dadb469ca", +- ] +-} +- + provider "registry.terraform.io/newrelic/newrelic" { + version = "2.1.2" + constraints = "~> 2.1.1" +``` + +If you add a new requirement for the same provider at a later date and run +`terraform init` again, Terraform will treat it as if it were +[an entirely new provider](#dependency-on-a-new-provider) +and so will not necessarily select the same version that was previously +selected and will not be able to verify that the checksums remained unchanged. + +-> **Note:** In Terraform v1.0 and earlier, `terraform init` does not +automatically remove now-unneeded providers from the lock file, and instead +just ignores them. If you removed a provider dependency while using an +earlier version of Terraform and then upgraded to Terraform v1.1 or later +then you may see the error "missing or corrupted provider plugins", referring to +the stale lock file entries. If so, run `terraform init` with the new Terraform +version to tidy those unneeded entries and then retry the previous operation.