Skip to content

Commit

Permalink
[Backport release-0.14] Make the terraform harness only use the rwmut…
Browse files Browse the repository at this point in the history 
…ex if the plugin cache is enabled (#242)

* Make the terraform harness only use the rwmutex if the plugin cache is enabled

(cherry picked from commit f00f122)

* Remove plugin cache locking for `Diff()` (terraform plan)

(cherry picked from commit e7276c1)

* Update comment to mention the -lock=false flag too

(cherry picked from commit 110097b)

---------

Co-authored-by: Samuel Littley <slittley@thoughtmachine.net>
  • Loading branch information
@github-actions @toastwaffle
github-actions[bot] and toastwaffle committed Feb 6, 2024
1 parent 8ea3c88 commit a342156
Show file tree
Hide file tree
Showing 4 changed files with 88 additions and 59 deletions.
20 changes: 11 additions & 9 deletions internal/controller/workspace/workspace.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand All @@ -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)
Expand Down
60 changes: 30 additions & 30 deletions internal/controller/workspace/workspace_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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) {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 },
}
},
},
Expand Down Expand Up @@ -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 },
}
},
},
Expand Down Expand Up @@ -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 },
}
},
},
Expand Down Expand Up @@ -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 },
}
},
},
Expand Down Expand Up @@ -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 },
}
},
},
Expand Down Expand Up @@ -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 },
}
},
},
Expand Down Expand Up @@ -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 },
}
},
},
Expand Down Expand Up @@ -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 },
}
},
},
Expand Down Expand Up @@ -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{
Expand All @@ -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 },
}
},
Expand All @@ -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 },
}
Expand Down Expand Up @@ -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 },
Expand Down Expand Up @@ -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 },
}
Expand Down Expand Up @@ -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")
Expand Down
63 changes: 45 additions & 18 deletions internal/terraform/terraform.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand All @@ -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.
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
}
Expand All @@ -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)
}
Expand Down
4 changes: 2 additions & 2 deletions internal/terraform/terraform_harness_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down

0 comments on commit a342156

Please sign in to comment.