Skip to content

Commit

Permalink
Make the terraform harness only use the rwmutex if the plugin cache i…
Browse files Browse the repository at this point in the history 
…s enabled
  • Loading branch information
@toastwaffle
toastwaffle committed Feb 1, 2024
1 parent 8ea3c88 commit f00f122
Show file tree
Hide file tree
Showing 4 changed files with 89 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
64 changes: 46 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,10 @@ 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()
if h.UsePluginCache {
rwmutex.RLock()
defer rwmutex.RUnlock()
}

// The -detailed-exitcode flag will make terraform plan return:
// 0 - Succeeded, diff is empty (no changes)
Expand Down Expand Up @@ -535,8 +557,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 +583,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 f00f122

Please sign in to comment.