Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Backport release-0.14] Make the terraform harness only use the rwmutex if the plugin cache is enabled #242

Merged
merged 3 commits into from
Feb 6, 2024
Merged

[Backport release-0.14] Make the terraform harness only use the rwmutex if the plugin cache is enabled #242

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
541447b
Make the terraform harness only use the rwmutex if the plugin cache i…
toastwaffle Feb 1, 2024
e7f72cf
Remove plugin cache locking for `Diff()` (terraform plan)
toastwaffle Feb 1, 2024
a7418fe
Update comment to mention the -lock=false flag too
toastwaffle Feb 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 11 additions & 9 deletions internal/controller/workspace/workspace.go
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading