diff --git a/internal/controller/workspace/workspace.go b/internal/controller/workspace/workspace.go index 9829f86..50d76c9 100644 --- a/internal/controller/workspace/workspace.go +++ b/internal/controller/workspace/workspace.go @@ -98,7 +98,7 @@ func envVarFallback(envvar string, fallback string) string { var tfDir = envVarFallback("XP_TF_DIR", "/tf") type tfclient interface { - Init(ctx context.Context, cache bool, o ...terraform.InitOption) error + Init(ctx context.Context, o ...terraform.InitOption) error Workspace(ctx context.Context, name string) error Outputs(ctx context.Context) ([]terraform.Output, error) Resources(ctx context.Context) ([]string, error) @@ -126,11 +126,13 @@ func Setup(mgr ctrl.Manager, o controller.Options, timeout, pollJitter time.Dura } c := &connector{ - kube: mgr.GetClient(), - usage: resource.NewProviderConfigUsageTracker(mgr.GetClient(), &v1beta1.ProviderConfigUsage{}), - logger: o.Logger, - fs: fs, - terraform: func(dir string) tfclient { return terraform.Harness{Path: tfPath, Dir: dir} }, + kube: mgr.GetClient(), + usage: resource.NewProviderConfigUsageTracker(mgr.GetClient(), &v1beta1.ProviderConfigUsage{}), + logger: o.Logger, + fs: fs, + terraform: func(dir string, usePluginCache bool) tfclient { + return terraform.Harness{Path: tfPath, Dir: dir, UsePluginCache: usePluginCache} + }, } opts := []managed.ReconcilerOption{ @@ -164,7 +166,7 @@ type connector struct { usage resource.Tracker logger logging.Logger fs afero.Afero - terraform func(dir string) tfclient + terraform func(dir string, usePluginCache bool) tfclient } func (c *connector) Connect(ctx context.Context, mg resource.Managed) (managed.ExternalClient, error) { //nolint:gocyclo @@ -282,7 +284,7 @@ func (c *connector) Connect(ctx context.Context, mg resource.Managed) (managed.E pc.Spec.PluginCache = new(bool) *pc.Spec.PluginCache = true } - tf := c.terraform(dir) + tf := c.terraform(dir, *pc.Spec.PluginCache) if cr.Status.AtProvider.Checksum != "" { checksum, err := tf.GenerateChecksum(ctx) if err != nil { @@ -300,7 +302,7 @@ func (c *connector) Connect(ctx context.Context, mg resource.Managed) (managed.E o = append(o, terraform.WithInitArgs([]string{"-backend-config=" + filepath.Join(dir, tfBackendFile)})) } o = append(o, terraform.WithInitArgs(cr.Spec.ForProvider.InitArgs)) - if err := tf.Init(ctx, *pc.Spec.PluginCache, o...); err != nil { + if err := tf.Init(ctx, o...); err != nil { return nil, errors.Wrap(err, errInit) } return &external{tf: tf, kube: c.kube}, errors.Wrap(tf.Workspace(ctx, meta.GetExternalName(cr)), errWorkspace) diff --git a/internal/controller/workspace/workspace_test.go b/internal/controller/workspace/workspace_test.go index 3068e1b..9c0cff7 100644 --- a/internal/controller/workspace/workspace_test.go +++ b/internal/controller/workspace/workspace_test.go @@ -68,7 +68,7 @@ func (e *ErrFs) OpenFile(name string, flag int, perm os.FileMode) (afero.File, e } type MockTf struct { - MockInit func(ctx context.Context, cache bool, o ...terraform.InitOption) error + MockInit func(ctx context.Context, 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) @@ -79,8 +79,8 @@ type MockTf struct { MockGenerateChecksum func(ctx context.Context) (string, error) } -func (tf *MockTf) Init(ctx context.Context, cache bool, o ...terraform.InitOption) error { - return tf.MockInit(ctx, cache, o...) +func (tf *MockTf) Init(ctx context.Context, o ...terraform.InitOption) error { + return tf.MockInit(ctx, o...) } func (tf *MockTf) GenerateChecksum(ctx context.Context) (string, error) { @@ -124,7 +124,7 @@ func TestConnect(t *testing.T) { kube client.Client usage resource.Tracker fs afero.Afero - terraform func(dir string) tfclient + terraform func(dir string, usePluginCache bool) tfclient } type args struct { @@ -216,9 +216,9 @@ 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 { + terraform: func(_ string, _ bool) tfclient { return &MockTf{ - MockInit: func(ctx context.Context, cache bool, o ...terraform.InitOption) error { return nil }, + MockInit: func(ctx context.Context, o ...terraform.InitOption) error { return nil }, } }, }, @@ -255,9 +255,9 @@ func TestConnect(t *testing.T) { errs: map[string]error{filepath.Join(tfDir, string(uid), tfCreds): errBoom}, }, }, - terraform: func(_ string) tfclient { + terraform: func(_ string, _ bool) tfclient { return &MockTf{ - MockInit: func(ctx context.Context, cache bool, o ...terraform.InitOption) error { return nil }, + MockInit: func(ctx context.Context, o ...terraform.InitOption) error { return nil }, } }, }, @@ -294,9 +294,9 @@ func TestConnect(t *testing.T) { errs: map[string]error{filepath.Join(tfDir, string(uid), "subdir", tfCreds): errBoom}, }, }, - terraform: func(_ string) tfclient { + terraform: func(_ string, _ bool) tfclient { return &MockTf{ - MockInit: func(ctx context.Context, cache bool, o ...terraform.InitOption) error { return nil }, + MockInit: func(ctx context.Context, o ...terraform.InitOption) error { return nil }, } }, }, @@ -338,9 +338,9 @@ func TestConnect(t *testing.T) { errs: map[string]error{filepath.Join("/tmp", tfDir, string(uid), ".git-credentials"): errBoom}, }, }, - terraform: func(_ string) tfclient { + terraform: func(_ string, _ bool) tfclient { return &MockTf{ - MockInit: func(ctx context.Context, cache bool, o ...terraform.InitOption) error { return nil }, + MockInit: func(ctx context.Context, o ...terraform.InitOption) error { return nil }, } }, }, @@ -381,9 +381,9 @@ func TestConnect(t *testing.T) { errs: map[string]error{filepath.Join("/tmp", tfDir, string(uid)): errBoom}, }, }, - terraform: func(_ string) tfclient { + terraform: func(_ string, _ bool) tfclient { return &MockTf{ - MockInit: func(ctx context.Context, cache bool, o ...terraform.InitOption) error { return nil }, + MockInit: func(ctx context.Context, o ...terraform.InitOption) error { return nil }, } }, }, @@ -422,9 +422,9 @@ func TestConnect(t *testing.T) { errs: map[string]error{filepath.Join(tfDir, string(uid), tfConfig): errBoom}, }, }, - terraform: func(_ string) tfclient { + terraform: func(_ string, _ bool) tfclient { return &MockTf{ - MockInit: func(ctx context.Context, cache bool, o ...terraform.InitOption) error { return nil }, + MockInit: func(ctx context.Context, o ...terraform.InitOption) error { return nil }, } }, }, @@ -463,9 +463,9 @@ func TestConnect(t *testing.T) { errs: map[string]error{filepath.Join(tfDir, string(uid), "subdir", tfConfig): errBoom}, }, }, - terraform: func(_ string) tfclient { + terraform: func(_ string, _ bool) tfclient { return &MockTf{ - MockInit: func(ctx context.Context, cache bool, o ...terraform.InitOption) error { return nil }, + MockInit: func(ctx context.Context, o ...terraform.InitOption) error { return nil }, } }, }, @@ -499,9 +499,9 @@ func TestConnect(t *testing.T) { errs: map[string]error{filepath.Join(tfDir, string(uid), tfMain): errBoom}, }, }, - terraform: func(_ string) tfclient { + terraform: func(_ string, _ bool) tfclient { return &MockTf{ - MockInit: func(ctx context.Context, cache bool, o ...terraform.InitOption) error { return nil }, + MockInit: func(ctx context.Context, o ...terraform.InitOption) error { return nil }, } }, }, @@ -529,8 +529,8 @@ 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, cache bool, _ ...terraform.InitOption) error { return errBoom }} + terraform: func(_ string, _ bool) tfclient { + return &MockTf{MockInit: func(_ context.Context, _ ...terraform.InitOption) error { return errBoom }} }, }, args: args{ @@ -553,9 +553,9 @@ 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 { + terraform: func(_ string, _ bool) tfclient { return &MockTf{ - MockInit: func(ctx context.Context, cache bool, o ...terraform.InitOption) error { return nil }, + MockInit: func(ctx context.Context, o ...terraform.InitOption) error { return nil }, MockWorkspace: func(_ context.Context, _ string) error { return errBoom }, } }, @@ -579,7 +579,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 { + terraform: func(_ string, _ bool) tfclient { return &MockTf{ MockGenerateChecksum: func(ctx context.Context) (string, error) { return "", errBoom }, } @@ -613,7 +613,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 { + terraform: func(_ string, _ bool) tfclient { return &MockTf{ MockGenerateChecksum: func(ctx context.Context) (string, error) { return tfChecksum, nil }, MockWorkspace: func(_ context.Context, _ string) error { return nil }, @@ -649,9 +649,9 @@ 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 { + terraform: func(_ string, _ bool) tfclient { return &MockTf{ - MockInit: func(ctx context.Context, cache bool, o ...terraform.InitOption) error { return nil }, + MockInit: func(ctx context.Context, o ...terraform.InitOption) error { return nil }, MockGenerateChecksum: func(ctx context.Context) (string, error) { return tfChecksum, nil }, MockWorkspace: func(_ context.Context, _ string) error { return nil }, } @@ -688,9 +688,9 @@ 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 { + terraform: func(_ string, _ bool) tfclient { return &MockTf{ - MockInit: func(ctx context.Context, cache bool, o ...terraform.InitOption) error { + MockInit: func(ctx context.Context, o ...terraform.InitOption) error { args := terraform.InitArgsToString(o) if len(args) != 2 { return errors.New("two init args are expected") diff --git a/internal/terraform/terraform.go b/internal/terraform/terraform.go index 97708a2..97b452e 100644 --- a/internal/terraform/terraform.go +++ b/internal/terraform/terraform.go @@ -122,6 +122,9 @@ type Harness struct { // Dir in which to execute the terraform binary. Dir string + // Whether to use the terraform plugin cache + UsePluginCache bool + // TODO(negz): Harness is a subset of exec.Cmd. If callers need more insight // into what the underlying Terraform binary is doing (e.g. for debugging) // we could consider allowing them to attach io.Writers to Stdout and Stdin @@ -169,21 +172,25 @@ func InitArgsToString(o []InitOption) []string { var rwmutex = &sync.RWMutex{} // Init initializes a Terraform configuration. -func (h Harness) Init(ctx context.Context, cache bool, o ...InitOption) error { +func (h Harness) Init(ctx context.Context, o ...InitOption) error { args := append([]string{"init", "-input=false", "-no-color"}, InitArgsToString(o)...) cmd := exec.Command(h.Path, args...) //nolint:gosec cmd.Dir = h.Dir for _, e := range os.Environ() { if strings.Contains(e, "TF_PLUGIN_CACHE_DIR") { - if !cache { + if !h.UsePluginCache { continue } } cmd.Env = append(cmd.Env, e) } cmd.Env = append(cmd.Env, "TF_CLI_CONFIG_FILE=./.terraformrc") - rwmutex.Lock() - defer rwmutex.Unlock() + + if h.UsePluginCache { + rwmutex.Lock() + defer rwmutex.Unlock() + } + _, err := runCommand(ctx, cmd) return Classify(err) } @@ -238,8 +245,12 @@ func (h Harness) Workspace(ctx context.Context, name string) error { // is somewhat optimistic, but it shouldn't hurt to try. cmd = exec.Command(h.Path, "workspace", "new", "-no-color", name) //nolint:gosec cmd.Dir = h.Dir - rwmutex.RLock() - defer rwmutex.RUnlock() + + if h.UsePluginCache { + rwmutex.RLock() + defer rwmutex.RUnlock() + } + _, err := runCommand(ctx, cmd) return Classify(err) } @@ -266,8 +277,11 @@ func (h Harness) DeleteCurrentWorkspace(ctx context.Context) error { cmd = exec.Command(h.Path, "workspace", "delete", "-no-color", name) //nolint:gosec cmd.Dir = h.Dir - rwmutex.RLock() - defer rwmutex.RUnlock() + if h.UsePluginCache { + rwmutex.RLock() + defer rwmutex.RUnlock() + } + _, err = runCommand(ctx, cmd) if err == nil { // We successfully deleted the workspace; we're done. @@ -378,8 +392,11 @@ func (h Harness) Outputs(ctx context.Context) ([]Output, error) { outputs := map[string]output{} - rwmutex.RLock() - defer rwmutex.RUnlock() + if h.UsePluginCache { + rwmutex.RLock() + defer rwmutex.RUnlock() + } + out, err := runCommand(ctx, cmd) if jerr := json.Unmarshal(out, &outputs); jerr != nil { // If stdout doesn't appear to be the JSON we expected we try to extract @@ -424,8 +441,11 @@ func (h Harness) Resources(ctx context.Context) ([]string, error) { cmd := exec.Command(h.Path, "state", "list") //nolint:gosec cmd.Dir = h.Dir - rwmutex.RLock() - defer rwmutex.RUnlock() + if h.UsePluginCache { + rwmutex.RLock() + defer rwmutex.RUnlock() + } + out, err := runCommand(ctx, cmd) if err != nil { return nil, Classify(err) @@ -504,8 +524,9 @@ func (h Harness) Diff(ctx context.Context, o ...Option) (bool, error) { cmd := exec.Command(h.Path, args...) //nolint:gosec cmd.Dir = h.Dir - rwmutex.RLock() - defer rwmutex.RUnlock() + // Note: the terraform lock is not used (see the -lock=false flag above) and the rwmutex is + // intentionally not locked here to avoid excessive blocking. See + // https://github.com/upbound/provider-terraform/issues/239#issuecomment-1921732682 // The -detailed-exitcode flag will make terraform plan return: // 0 - Succeeded, diff is empty (no changes) @@ -535,8 +556,11 @@ func (h Harness) Apply(ctx context.Context, o ...Option) error { cmd := exec.Command(h.Path, args...) //nolint:gosec cmd.Dir = h.Dir - rwmutex.RLock() - defer rwmutex.RUnlock() + if h.UsePluginCache { + rwmutex.RLock() + defer rwmutex.RUnlock() + } + _, err := runCommand(ctx, cmd) return Classify(err) } @@ -558,8 +582,11 @@ func (h Harness) Destroy(ctx context.Context, o ...Option) error { cmd := exec.Command(h.Path, args...) //nolint:gosec cmd.Dir = h.Dir - rwmutex.RLock() - defer rwmutex.RUnlock() + if h.UsePluginCache { + rwmutex.RLock() + defer rwmutex.RUnlock() + } + _, err := runCommand(ctx, cmd) return Classify(err) } diff --git a/internal/terraform/terraform_harness_test.go b/internal/terraform/terraform_harness_test.go index b4d4398..f9bcaf7 100644 --- a/internal/terraform/terraform_harness_test.go +++ b/internal/terraform/terraform_harness_test.go @@ -439,9 +439,9 @@ func TestInitDiffApplyDestroy(t *testing.T) { } defer os.RemoveAll(dir) - tf := Harness{Path: tfBinaryPath, Dir: dir} + tf := Harness{Path: tfBinaryPath, Dir: dir, UsePluginCache: false} - err = tf.Init(tc.initArgs.ctx, false, tc.initArgs.o...) + err = tf.Init(tc.initArgs.ctx, tc.initArgs.o...) if diff := cmp.Diff(tc.want.init, err, test.EquateErrors()); diff != "" { t.Errorf("\n%s\ntf.Init(...): -want error, +got error:\n%s", tc.reason, diff) }