Repository: crossplane-contrib/provider-terraform (go)

Issue:
Provider cache is only used for some workspaces and not all
### What happened?
The provider cache is only being used by some workspaces and not by others.

The problem appears to be that the Connect() function is deferring a call to os.Unsetenv("TF_PLUGIN_CACHE_DIR") which
is unsetting the env var for other threads that haven't yet called terraform init.  Those invocations of terraform init end up downloading local copies of the providers and using extra disk space.

Since the os.Setenv and os.Unsetenv primitives appear to function at the process level and not at the thread level, we cannot dynamically set and unset the environment variable for each thread.

The solution is to set the TF_PLUGIN_CACE_DIR env variable in the Dockerfile.  If there is a need to disable the cache
we can add a CLI option to unset the variable at runtime.


### How can we reproduce it?
Run a large number of workspaces that access a cloud provider.  Run "du -s /tf/*" and observe that some workspaces have a local copy of the provider while others do not.

### What environment did it happen in?
Crossplane version: 1.10.1


Pull request:
Refactor plugin cache implementation
* Remove setenv of TF_PLUGIN_CACHE_DIR for each Workspace connect
* Set TF_PLUGIN_CACHE_DIR env var in Dockerfile
* Unset TF_PLUGIN_CACHE_DIR in the CLI process environment when pluginCache is false
* Set TF_IN_AUTOMATION env var in Dockerfile

Signed-off-by: Bob Haddleton <bob.haddleton@nokia.com>

### Description of your changes

Fixes #139 

I have:

- [X] Read and followed Crossplane's [contribution process].
- [X] Run `make reviewable test` to ensure this PR is ready for review.

### How has this code been tested
Tested in a development cluster with 100+ workspaces

[contribution process]: https://git.io/fj2m9


Patch:
diff --git a/cluster/images/provider-terraform/Dockerfile b/cluster/images/provider-terraform/Dockerfile
index 107768c..9d32e28 100644
--- a/cluster/images/provider-terraform/Dockerfile
+++ b/cluster/images/provider-terraform/Dockerfile
@@ -4,17 +4,19 @@ ARG TARGETOS
 ARG TARGETARCH
 
 ENV TERRAFORM_VERSION=1.3.4
+ENV TF_IN_AUTOMATION=1
+ENV TF_PLUGIN_CACHE_DIR=/tf/plugin-cache
 
 ADD "bin/${TARGETOS}_${TARGETARCH}/provider" /usr/local/bin/crossplane-terraform-provider
 ADD .gitconfig .gitconfig
 
 RUN curl -s -L https://releases.hashicorp.com/terraform/${TERRAFORM_VERSION}/terraform_${TERRAFORM_VERSION}_${TARGETOS}_${TARGETARCH}.zip | \
   unzip -d /usr/local/bin - \
-  && chmod +x /usr/local/bin/terraform
+  && chmod +x /usr/local/bin/terraform \
+  && mkdir -p ${TF_PLUGIN_CACHE_DIR} \
+  && chown -R 2000 /tf
 # As of Crossplane v1.3.0 provider controllers run as UID 2000.
 # https://github.com/crossplane/crossplane/blob/v1.3.0/internal/controller/pkg/revision/deployment.go#L32
-RUN mkdir /tf
-RUN chown 2000 /tf
 
 USER 65532
 ENTRYPOINT ["crossplane-terraform-provider"]
diff --git a/internal/controller/workspace/workspace.go b/internal/controller/workspace/workspace.go
index fcddd3e..0049679 100644
--- a/internal/controller/workspace/workspace.go
+++ b/internal/controller/workspace/workspace.go
@@ -53,25 +53,23 @@ const (
 	errGetPC        = "cannot get ProviderConfig"
 	errGetCreds     = "cannot get credentials"
 
-	errMkdir             = "cannot make Terraform configuration directory"
-	errMkPluginCacheDir  = "cannot make Terraform plugin cache directory"
-	errRemoteModule      = "cannot get remote Terraform module"
-	errSetGitCredDir     = "cannot set GIT_CRED_DIR environment variable"
-	errSetPluginCacheDir = "cannot set TF_PLUGIN_CACHE_DIR environment variable"
-	errWriteCreds        = "cannot write Terraform credentials"
-	errWriteGitCreds     = "cannot write .git-credentials to /tmp dir"
-	errWriteConfig       = "cannot write Terraform configuration " + tfConfig
-	errWriteMain         = "cannot write Terraform configuration " + tfMain
-	errInit              = "cannot initialize Terraform configuration"
-	errWorkspace         = "cannot select Terraform workspace"
-	errResources         = "cannot list Terraform resources"
-	errDiff              = "cannot diff (i.e. plan) Terraform configuration"
-	errOutputs           = "cannot list Terraform outputs"
-	errOptions           = "cannot determine Terraform options"
-	errApply             = "cannot apply Terraform configuration"
-	errDestroy           = "cannot destroy Terraform configuration"
-	errVarFile           = "cannot get tfvars"
-	errDeleteWorkspace   = "cannot delete Terraform workspace"
+	errMkdir           = "cannot make Terraform configuration directory"
+	errRemoteModule    = "cannot get remote Terraform module"
+	errSetGitCredDir   = "cannot set GIT_CRED_DIR environment variable"
+	errWriteCreds      = "cannot write Terraform credentials"
+	errWriteGitCreds   = "cannot write .git-credentials to /tmp dir"
+	errWriteConfig     = "cannot write Terraform configuration " + tfConfig
+	errWriteMain       = "cannot write Terraform configuration " + tfMain
+	errInit            = "cannot initialize Terraform configuration"
+	errWorkspace       = "cannot select Terraform workspace"
+	errResources       = "cannot list Terraform resources"
+	errDiff            = "cannot diff (i.e. plan) Terraform configuration"
+	errOutputs         = "cannot list Terraform outputs"
+	errOptions         = "cannot determine Terraform options"
+	errApply           = "cannot apply Terraform configuration"
+	errDestroy         = "cannot destroy Terraform configuration"
+	errVarFile         = "cannot get tfvars"
+	errDeleteWorkspace = "cannot delete Terraform workspace"
 
 	gitCredentialsFilename = ".git-credentials"
 )
@@ -93,7 +91,7 @@ func envVarFallback(envvar string, fallback string) string {
 var tfDir = envVarFallback("XP_TF_DIR", "/tf")
 
 type tfclient interface {
-	Init(ctx context.Context, o ...terraform.InitOption) error
+	Init(ctx context.Context, cache bool, o ...terraform.InitOption) error
 	Workspace(ctx context.Context, name string) error
 	Outputs(ctx context.Context) ([]terraform.Output, error)
 	Resources(ctx context.Context) ([]string, error)
@@ -261,20 +259,8 @@ func (c *connector) Connect(ctx context.Context, mg resource.Managed) (managed.E
 		pc.Spec.PluginCache = new(bool)
 		*pc.Spec.PluginCache = true
 	}
-	if *pc.Spec.PluginCache {
-		pluginCacheDir, err := filepath.Abs(filepath.Join(tfDir, "plugin-cache"))
-		if err != nil {
-			return nil, errors.Wrap(err, errMkPluginCacheDir)
-		}
-		if err := c.fs.MkdirAll(pluginCacheDir, 0700); err != nil {
-			return nil, errors.Wrap(err, errMkPluginCacheDir)
-		}
-		if err := os.Setenv("TF_PLUGIN_CACHE_DIR", pluginCacheDir); err != nil {
-			return nil, errors.Wrap(err, errSetPluginCacheDir)
-		}
-		defer os.Unsetenv("TF_PLUGIN_CACHE_DIR") // nolint: errcheck
-	}
-	if err := tf.Init(ctx, o...); err != nil {
+
+	if err := tf.Init(ctx, *pc.Spec.PluginCache, o...); err != nil {
 		return nil, errors.Wrap(err, errInit)
 	}
 
diff --git a/internal/terraform/terraform.go b/internal/terraform/terraform.go
index ac5fb36..437a232 100644
--- a/internal/terraform/terraform.go
+++ b/internal/terraform/terraform.go
@@ -23,6 +23,7 @@ import (
 	"context"
 	"encoding/json"
 	"io/ioutil"
+	"os"
 	"os/exec"
 	"path/filepath"
 	"regexp"
@@ -124,7 +125,7 @@ func WithInitArgs(v []string) InitOption {
 var sem = semaphore.NewWeighted(int64(1))
 
 // Init initializes a Terraform configuration.
-func (h Harness) Init(ctx context.Context, o ...InitOption) error {
+func (h Harness) Init(ctx context.Context, cache bool, o ...InitOption) error {
 	io := &initOptions{}
 	for _, fn := range o {
 		fn(io)
@@ -133,6 +134,14 @@ func (h Harness) Init(ctx context.Context, o ...InitOption) error {
 	args := append([]string{"init", "-input=false", "-no-color"}, io.args...)
 	cmd := exec.CommandContext(ctx, h.Path, args...) //nolint:gosec
 	cmd.Dir = h.Dir
+	if !cache {
+		for _, e := range os.Environ() {
+			if strings.Contains(e, "TF_PLUGIN_CACHE_DIR") {
+				continue
+			}
+			cmd.Env = append(cmd.Env, e)
+		}
+	}
 	err := sem.Acquire(ctx, 1)
 	if err != nil {
 		return errors.Wrap(err, errSemAcquire)


Test patch:
diff --git a/internal/controller/workspace/workspace_test.go b/internal/controller/workspace/workspace_test.go
index 472253d..0cefc6a 100644
--- a/internal/controller/workspace/workspace_test.go
+++ b/internal/controller/workspace/workspace_test.go
@@ -61,7 +61,7 @@ func (e *ErrFs) OpenFile(name string, flag int, perm os.FileMode) (afero.File, e
 }
 
 type MockTf struct {
-	MockInit                   func(ctx context.Context, o ...terraform.InitOption) error
+	MockInit                   func(ctx context.Context, cache bool, o ...terraform.InitOption) error
 	MockWorkspace              func(ctx context.Context, name string) error
 	MockOutputs                func(ctx context.Context) ([]terraform.Output, error)
 	MockResources              func(ctx context.Context) ([]string, error)
@@ -71,8 +71,8 @@ type MockTf struct {
 	MockDeleteCurrentWorkspace func(ctx context.Context) error
 }
 
-func (tf *MockTf) Init(ctx context.Context, o ...terraform.InitOption) error {
-	return tf.MockInit(ctx, o...)
+func (tf *MockTf) Init(ctx context.Context, cache bool, o ...terraform.InitOption) error {
+	return tf.MockInit(ctx, cache, o...)
 }
 
 func (tf *MockTf) Workspace(ctx context.Context, name string) error {
@@ -206,7 +206,7 @@ func TestConnect(t *testing.T) {
 				fs:    afero.Afero{Fs: afero.NewMemMapFs()},
 				terraform: func(_ string) tfclient {
 					return &MockTf{
-						MockInit: func(ctx context.Context, o ...terraform.InitOption) error { return nil },
+						MockInit: func(ctx context.Context, cache bool, o ...terraform.InitOption) error { return nil },
 					}
 				},
 			},
@@ -245,7 +245,7 @@ func TestConnect(t *testing.T) {
 				},
 				terraform: func(_ string) tfclient {
 					return &MockTf{
-						MockInit: func(ctx context.Context, o ...terraform.InitOption) error { return nil },
+						MockInit: func(ctx context.Context, cache bool, o ...terraform.InitOption) error { return nil },
 					}
 				},
 			},
@@ -284,7 +284,7 @@ func TestConnect(t *testing.T) {
 				},
 				terraform: func(_ string) tfclient {
 					return &MockTf{
-						MockInit: func(ctx context.Context, o ...terraform.InitOption) error { return nil },
+						MockInit: func(ctx context.Context, cache bool, o ...terraform.InitOption) error { return nil },
 					}
 				},
 			},
@@ -328,7 +328,7 @@ func TestConnect(t *testing.T) {
 				},
 				terraform: func(_ string) tfclient {
 					return &MockTf{
-						MockInit: func(ctx context.Context, o ...terraform.InitOption) error { return nil },
+						MockInit: func(ctx context.Context, cache bool, o ...terraform.InitOption) error { return nil },
 					}
 				},
 			},
@@ -371,7 +371,7 @@ func TestConnect(t *testing.T) {
 				},
 				terraform: func(_ string) tfclient {
 					return &MockTf{
-						MockInit: func(ctx context.Context, o ...terraform.InitOption) error { return nil },
+						MockInit: func(ctx context.Context, cache bool, o ...terraform.InitOption) error { return nil },
 					}
 				},
 			},
@@ -391,51 +391,6 @@ func TestConnect(t *testing.T) {
 			},
 			want: errors.Wrap(errBoom, errWriteGitCreds),
 		},
-		"PluginCacheMkdirError": {
-			reason: "We should return any error encountered while creating the plugin cache directory",
-			fields: fields{
-				kube: &test.MockClient{
-					MockGet: test.NewMockGetFn(nil, func(obj client.Object) error {
-						if pc, ok := obj.(*v1alpha1.ProviderConfig); ok {
-							pc.Spec.Credentials = []v1alpha1.ProviderCredentials{{
-								Filename: ".git-credentials",
-								Source:   xpv1.CredentialsSourceNone,
-							}}
-							pc.Spec.PluginCache = new(bool)
-							*pc.Spec.PluginCache = true
-						}
-						return nil
-					}),
-				},
-				usage: resource.TrackerFn(func(_ context.Context, _ resource.Managed) error { return nil }),
-				fs: afero.Afero{
-					Fs: &ErrFs{
-						Fs:   afero.NewMemMapFs(),
-						errs: map[string]error{filepath.Join(tfDir, "plugin-cache"): errBoom},
-					},
-				},
-				terraform: func(_ string) tfclient {
-					return &MockTf{
-						MockInit: func(ctx context.Context, o ...terraform.InitOption) error { return nil },
-					}
-				},
-			},
-			args: args{
-				mg: &v1alpha1.Workspace{
-					ObjectMeta: metav1.ObjectMeta{UID: uid},
-					Spec: v1alpha1.WorkspaceSpec{
-						ResourceSpec: xpv1.ResourceSpec{
-							ProviderConfigReference: &xpv1.Reference{},
-						},
-						ForProvider: v1alpha1.WorkspaceParameters{
-							Module: "github.com/crossplane/rocks",
-							Source: v1alpha1.ModuleSourceInline,
-						},
-					},
-				},
-			},
-			want: errors.Wrap(errBoom, errMkPluginCacheDir),
-		},
 		"WriteConfigError": {
 			reason: "We should return any error encountered while writing our crossplane-provider-config.tf file",
 			fields: fields{
@@ -457,7 +412,7 @@ func TestConnect(t *testing.T) {
 				},
 				terraform: func(_ string) tfclient {
 					return &MockTf{
-						MockInit: func(ctx context.Context, o ...terraform.InitOption) error { return nil },
+						MockInit: func(ctx context.Context, cache bool, o ...terraform.InitOption) error { return nil },
 					}
 				},
 			},
@@ -498,7 +453,7 @@ func TestConnect(t *testing.T) {
 				},
 				terraform: func(_ string) tfclient {
 					return &MockTf{
-						MockInit: func(ctx context.Context, o ...terraform.InitOption) error { return nil },
+						MockInit: func(ctx context.Context, cache bool, o ...terraform.InitOption) error { return nil },
 					}
 				},
 			},
@@ -534,7 +489,7 @@ func TestConnect(t *testing.T) {
 				},
 				terraform: func(_ string) tfclient {
 					return &MockTf{
-						MockInit: func(ctx context.Context, o ...terraform.InitOption) error { return nil },
+						MockInit: func(ctx context.Context, cache bool, o ...terraform.InitOption) error { return nil },
 					}
 				},
 			},
@@ -563,7 +518,7 @@ func TestConnect(t *testing.T) {
 				usage: resource.TrackerFn(func(_ context.Context, _ resource.Managed) error { return nil }),
 				fs:    afero.Afero{Fs: afero.NewMemMapFs()},
 				terraform: func(_ string) tfclient {
-					return &MockTf{MockInit: func(_ context.Context, _ ...terraform.InitOption) error { return errBoom }}
+					return &MockTf{MockInit: func(_ context.Context, cache bool, _ ...terraform.InitOption) error { return errBoom }}
 				},
 			},
 			args: args{
@@ -588,7 +543,7 @@ func TestConnect(t *testing.T) {
 				fs:    afero.Afero{Fs: afero.NewMemMapFs()},
 				terraform: func(_ string) tfclient {
 					return &MockTf{
-						MockInit:      func(ctx context.Context, o ...terraform.InitOption) error { return nil },
+						MockInit:      func(ctx context.Context, cache bool, o ...terraform.InitOption) error { return nil },
 						MockWorkspace: func(_ context.Context, _ string) error { return errBoom },
 					}
 				},
@@ -615,7 +570,7 @@ func TestConnect(t *testing.T) {
 				fs:    afero.Afero{Fs: afero.NewMemMapFs()},
 				terraform: func(_ string) tfclient {
 					return &MockTf{
-						MockInit:      func(ctx context.Context, o ...terraform.InitOption) error { return nil },
+						MockInit:      func(ctx context.Context, cache bool, o ...terraform.InitOption) error { return nil },
 						MockWorkspace: func(_ context.Context, _ string) error { return nil },
 					}
 				},