Merge pull request #34 from hashicorp/fix-hcl-flag

fix: update variables on HCL flag changes

CHANGELOG.md

@@ -2,6 +2,7 @@
 
 * Add `sshKeyID` to CR spec, so users can reference modules in private git repos (#25)
 * Always update Sensitive variables when a Run is triggered and before the Run is executed (#22)
+* Fix: update variables when HCL flag changes (#33)
 
 ## 0.1.2 (April 16, 2020)
 

operator/pkg/controller/workspace/mocks/test

@@ -0,0 +1 @@
+secrets

operator/pkg/controller/workspace/tfc_variable.go

@@ -66,54 +66,85 @@ func (t *TerraformCloudClient) deleteVariablesFromTFC(specTFCVariables []*tfc.Va
 	return nil
 }
 
-func (t *TerraformCloudClient) UpdateSensitiveBeforeRun(workspace string, specTFCVariables []*tfc.Variable) error {
-	workspaceVariables, err := t.listVariables(workspace)
-	if err != nil {
-		return err
-	}
-
+func (t *TerraformCloudClient) createVariablesOnTFC(workspace *tfc.Workspace, specTFCVariables []*tfc.Variable, workspaceVariables []*tfc.Variable) (bool, error) {
+	updated := false
 	for _, v := range specTFCVariables {
-		if !v.Sensitive {
+		index := find(workspaceVariables, v.Key)
+		if index < 0 {
+			err := t.CreateTerraformVariable(workspace, v)
+			if err != nil {
+				return false, err
+			}
+			updated = true
 			continue
 		}
+	}
+	return updated, nil
+}
+
+func checkIfVariableChanged(specVariable *tfc.Variable, workspaceVariable *tfc.Variable) bool {
+	if specVariable.Value != workspaceVariable.Value {
+		return true
+	}
+	if specVariable.HCL != workspaceVariable.HCL {
+		return true
+	}
+	if !specVariable.Sensitive && workspaceVariable.Sensitive {
+		return true
+	}
+	return false
+}
+
+func getNonSensitiveVariablesToUpdate(specTFCVariables []*tfc.Variable, workspaceVariables []*tfc.Variable) []*tfc.Variable {
+	variablesToUpdate := []*tfc.Variable{}
+	for _, v := range specTFCVariables {
 		index := find(workspaceVariables, v.Key)
-		err := t.checkAndRetrieveIfSensitive(v)
-		if err != nil {
-			return err
+		if index < 0 || workspaceVariables[index].Sensitive {
+			continue
 		}
-		err = t.UpdateTerraformVariable(workspaceVariables[index], v.Value)
-		if err != nil {
-			return err
+		if checkIfVariableChanged(v, workspaceVariables[index]) {
+			v.ID = workspaceVariables[index].ID
+			variablesToUpdate = append(variablesToUpdate, v)
 		}
 	}
-
-	return nil
+	return variablesToUpdate
 }
 
-func (t *TerraformCloudClient) updateVariablesOnTFC(workspace *tfc.Workspace, specTFCVariables []*tfc.Variable, workspaceVariables []*tfc.Variable) (bool, error) {
-	updated := false
+func getSensitiveVariablesToUpdate(specTFCVariables []*tfc.Variable, workspaceVariables []*tfc.Variable, secretsMountPath string) ([]*tfc.Variable, error) {
+	variablesToUpdate := []*tfc.Variable{}
 	for _, v := range specTFCVariables {
 		index := find(workspaceVariables, v.Key)
 		if index < 0 {
-			err := t.CreateTerraformVariable(workspace, v)
-			if err != nil {
-				return false, err
-			}
-			updated = true
 			continue
 		}
-		if v.Value != workspaceVariables[index].Value {
-			t.checkAndRetrieveIfSensitive(v)
-			err := t.UpdateTerraformVariable(workspaceVariables[index], v.Value)
-			if err != nil {
-				return false, err
-			}
-			if !v.Sensitive {
-				updated = true
+		if workspaceVariables[index].Sensitive {
+			if err := checkAndRetrieveIfSensitive(v, secretsMountPath); err != nil {
+				return nil, err
 			}
+			v.ID = workspaceVariables[index].ID
+			v.Sensitive = true
+			variablesToUpdate = append(variablesToUpdate, v)
 		}
 	}
-	return updated, nil
+	return variablesToUpdate, nil
+}
+
+func generateUpdateVariableList(specTFCVariables []*tfc.Variable, workspaceVariables []*tfc.Variable, secretsMountPath string) ([]*tfc.Variable, error) {
+	updateList := []*tfc.Variable{}
+
+	nonSensitiveVariablesToUpdate := getNonSensitiveVariablesToUpdate(specTFCVariables, workspaceVariables)
+	if len(nonSensitiveVariablesToUpdate) == 0 {
+		return updateList, nil
+	}
+
+	sensitiveVariablesToUpdate, err := getSensitiveVariablesToUpdate(specTFCVariables, workspaceVariables, secretsMountPath)
+	if err != nil {
+		return nonSensitiveVariablesToUpdate, err
+	}
+
+	updateList = append(nonSensitiveVariablesToUpdate, sensitiveVariablesToUpdate...)
+
+	return updateList, nil
 }
 
 // CheckVariables creates, updates, or deletes variables as needed
@@ -130,7 +161,21 @@ func (t *TerraformCloudClient) CheckVariables(workspace string, specTFCVariables
 		return false, err
 	}
 
-	return t.updateVariablesOnTFC(tfcWorkspace, specTFCVariables, workspaceVariables)
+	createdVariables, err := t.createVariablesOnTFC(tfcWorkspace, specTFCVariables, workspaceVariables)
+	if err != nil {
+		return false, err
+	}
+
+	variablesToUpdate, err := generateUpdateVariableList(specTFCVariables, workspaceVariables, t.SecretsMountPath)
+	if err != nil || len(variablesToUpdate) == 0 {
+		return false, err
+	}
+
+	if err = t.UpdateTerraformVariables(variablesToUpdate); err != nil {
+		return false, err
+	}
+
+	return createdVariables || len(variablesToUpdate) > 0, nil
 }
 
 func find(tfcVariables []*tfc.Variable, key string) int {
@@ -164,23 +209,29 @@ func (t *TerraformCloudClient) DeleteVariable(variable *tfc.Variable) error {
 	return nil
 }
 
-// UpdateTerraformVariable updates a variable
-func (t *TerraformCloudClient) UpdateTerraformVariable(variable *tfc.Variable, newValue string) error {
-	options := tfc.VariableUpdateOptions{
-		Key:       &variable.Key,
-		Value:     &newValue,
-		Sensitive: &variable.Sensitive,
+// UpdateTerraformVariables updates a list of variable
+func (t *TerraformCloudClient) UpdateTerraformVariables(variables []*tfc.Variable) error {
+	if len(variables) == 0 {
+		return nil
 	}
-	_, err := t.Client.Variables.Update(context.TODO(), variable.ID, options)
-	if err != nil {
-		return err
+	for _, v := range variables {
+		options := tfc.VariableUpdateOptions{
+			Key:       &v.Key,
+			Value:     &v.Value,
+			HCL:       &v.HCL,
+			Sensitive: &v.Sensitive,
+		}
+		_, err := t.Client.Variables.Update(context.TODO(), v.ID, options)
+		if err != nil {
+			return err
+		}
 	}
 	return nil
 }
 
-func (t *TerraformCloudClient) checkAndRetrieveIfSensitive(variable *tfc.Variable) error {
+func checkAndRetrieveIfSensitive(variable *tfc.Variable, secretsMountPath string) error {
 	if variable.Sensitive {
-		filePath := fmt.Sprintf("%s/%s", t.SecretsMountPath, variable.Key)
+		filePath := fmt.Sprintf("%s/%s", secretsMountPath, variable.Key)
 		data, err := ioutil.ReadFile(filePath)
 		if err != nil {
 			return fmt.Errorf("could not get secret, %s", err)
@@ -193,7 +244,7 @@ func (t *TerraformCloudClient) checkAndRetrieveIfSensitive(variable *tfc.Variabl
 
 // CreateTerraformVariable creates a Terraform variable based on key and value
 func (t *TerraformCloudClient) CreateTerraformVariable(workspace *tfc.Workspace, variable *tfc.Variable) error {
-	t.checkAndRetrieveIfSensitive(variable)
+	checkAndRetrieveIfSensitive(variable, t.SecretsMountPath)
 	options := tfc.VariableCreateOptions{
 		Key:       &variable.Key,
 		Value:     &variable.Value,

operator/pkg/controller/workspace/tfc_variable_test.go

@@ -0,0 +1,177 @@
+package workspace
+
+import (
+	"testing"
+
+	tfc "github.com/hashicorp/go-tfe"
+	"github.com/stretchr/testify/assert"
+)
+
+const (
+	secretsMount = "mocks"
+)
+
+var (
+	specVariables = []*tfc.Variable{
+		{
+			Key:       "test",
+			HCL:       false,
+			Value:     "needs update",
+			Sensitive: true,
+		},
+		{
+			Key:       "test2",
+			HCL:       false,
+			Sensitive: true,
+		},
+	}
+	workspaceVariables = []*tfc.Variable{
+		{
+			Key:       "test",
+			HCL:       true,
+			Value:     "test",
+			Sensitive: false,
+		},
+		{
+			Key:       "test2",
+			HCL:       false,
+			Sensitive: true,
+		},
+	}
+)
+
+func TestShouldReturn1VariableNeedsUpdate(t *testing.T) {
+	updatedVariables := getNonSensitiveVariablesToUpdate(specVariables, workspaceVariables)
+	assert.Len(t, updatedVariables, 1)
+	assert.Equal(t, updatedVariables[0].HCL, specVariables[0].HCL)
+	assert.Equal(t, updatedVariables[0].Value, specVariables[0].Value)
+	assert.Equal(t, updatedVariables[0].Sensitive, specVariables[0].Sensitive)
+}
+
+func TestShouldReturnNoVariableNeedsUpdateWhenChangingSensitive(t *testing.T) {
+	specVariables := []*tfc.Variable{
+		{
+			Key:       "test",
+			Sensitive: false,
+		},
+	}
+	workspaceVariables := []*tfc.Variable{
+		{
+			Key:       "test",
+			Sensitive: true,
+		},
+	}
+	updatedVariables := getNonSensitiveVariablesToUpdate(specVariables, workspaceVariables)
+	assert.Len(t, updatedVariables, 0)
+}
+
+func TestShouldReturnNoVariableNeedsUpdate(t *testing.T) {
+	specVariables := []*tfc.Variable{
+		{
+			Key:       "test2",
+			HCL:       false,
+			Value:     "test2",
+			Sensitive: false,
+		},
+	}
+	workspaceVariables := []*tfc.Variable{
+		{
+			Key:       "test2",
+			HCL:       false,
+			Value:     "test2",
+			Sensitive: false,
+		},
+	}
+	updatedVariables := getNonSensitiveVariablesToUpdate(specVariables, workspaceVariables)
+	assert.Len(t, updatedVariables, 0)
+}
+
+func TestShouldGetSensitiveVariablesForUpdate(t *testing.T) {
+	specVariables := []*tfc.Variable{
+		{
+			Key:       "test",
+			Sensitive: true,
+		},
+	}
+	workspaceVariables := []*tfc.Variable{
+		{
+			Key:       "test",
+			Sensitive: true,
+		},
+	}
+	update, err := getSensitiveVariablesToUpdate(specVariables, workspaceVariables, secretsMount)
+	assert.NoError(t, err)
+	assert.Len(t, update, 1)
+	assert.Equal(t, update[0].Key, specVariables[0].Key)
+}
+
+func TestShouldUpdateVariables(t *testing.T) {
+	specVariables := []*tfc.Variable{
+		{
+			Key:       "test",
+			Sensitive: true,
+			HCL:       false,
+		},
+		{
+			Key:       "test2",
+			Value:     `{hello="world"}`,
+			Sensitive: false,
+			HCL:       true,
+		},
+	}
+	workspaceVariables := []*tfc.Variable{
+		{
+			Key:       "test",
+			Sensitive: true,
+			HCL:       false,
+		},
+		{
+			Key:       "test2",
+			Value:     "hello world",
+			Sensitive: false,
+			HCL:       false,
+		},
+	}
+	update, err := generateUpdateVariableList(specVariables, workspaceVariables, secretsMount)
+	assert.NoError(t, err)
+	assert.Len(t, update, 2)
+	assert.False(t, update[0].Sensitive)
+	assert.Equal(t, update[0].Key, specVariables[1].Key)
+	assert.Equal(t, update[0].Value, specVariables[1].Value)
+	assert.Equal(t, update[0].HCL, specVariables[1].HCL)
+	assert.True(t, update[1].Sensitive)
+	assert.Equal(t, update[1].Key, specVariables[0].Key)
+	assert.Equal(t, update[1].HCL, specVariables[0].HCL)
+}
+
+func TestShouldNotUpdateVariables(t *testing.T) {
+	specVariables := []*tfc.Variable{
+		{
+			Key:       "test",
+			Sensitive: true,
+			HCL:       false,
+		},
+		{
+			Key:       "test2",
+			Value:     `{hello="world"}`,
+			Sensitive: false,
+			HCL:       true,
+		},
+	}
+	workspaceVariables := []*tfc.Variable{
+		{
+			Key:       "test",
+			Sensitive: true,
+			HCL:       false,
+		},
+		{
+			Key:       "test2",
+			Value:     `{hello="world"}`,
+			Sensitive: false,
+			HCL:       true,
+		},
+	}
+	update, err := generateUpdateVariableList(specVariables, workspaceVariables, secretsMount)
+	assert.NoError(t, err)
+	assert.Len(t, update, 0)
+}

operator/pkg/controller/workspace/workspace_controller.go

@@ -234,11 +234,6 @@ func (r *ReconcileWorkspace) Reconcile(request reconcile.Request) (reconcile.Res
 	if updatedTerraform || updatedVariables || instance.Status.RunID == "" {
 		r.reqLogger.Info("Starting run because template changed", "Organization", organization, "Name", workspace, "Namespace", request.Namespace)
 
-		if err := r.tfclient.UpdateSensitiveBeforeRun(workspace, specTFCVariables); err != nil {
-			r.reqLogger.Error(err, "Could not update Sensitive Varaibles before Run")
-			return reconcile.Result{}, err
-		}
-
 		if err := r.tfclient.CreateRun(instance, terraform); err != nil {
 			r.reqLogger.Error(err, "Could not run new Terraform configuration")
 			return reconcile.Result{}, err