From fe8d4a5679cee66c555379d64e0b45243f17c425 Mon Sep 17 00:00:00 2001 From: Aleksandr Rybolovlev Date: Tue, 23 Apr 2024 09:40:04 +0200 Subject: [PATCH] Update variables reconciliation logic to reduce number of API calls (#390) --- .../ENHANCEMENTS-390-20240419-102720.yaml | 6 + .../unreleased/NOTES-390-20240419-102736.yaml | 6 + api/v1alpha2/workspace_helpers.go | 46 +++ api/v1alpha2/workspace_types.go | 17 + api/v1alpha2/zz_generated.deepcopy.go | 20 ++ charts/terraform-cloud-operator/README.md | 8 + .../terraform-cloud-operator/README.md.gotmpl | 8 + .../crds/app.terraform.io_workspaces.yaml | 28 ++ .../bases/app.terraform.io_workspaces.yaml | 28 ++ controllers/workspace_controller.go | 5 +- controllers/workspace_controller_variables.go | 320 ++++++++---------- .../workspace_controller_variables_test.go | 31 +- docs/api-reference.md | 18 + 13 files changed, 329 insertions(+), 212 deletions(-) create mode 100644 .changes/unreleased/ENHANCEMENTS-390-20240419-102720.yaml create mode 100644 .changes/unreleased/NOTES-390-20240419-102736.yaml diff --git a/.changes/unreleased/ENHANCEMENTS-390-20240419-102720.yaml b/.changes/unreleased/ENHANCEMENTS-390-20240419-102720.yaml new file mode 100644 index 00000000..147eccd5 --- /dev/null +++ b/.changes/unreleased/ENHANCEMENTS-390-20240419-102720.yaml @@ -0,0 +1,6 @@ +kind: ENHANCEMENTS +body: '`Workspace`: Update variables reconciliation logic to reduce the number of + API calls.' +time: 2024-04-19T10:27:20.790343+02:00 +custom: + PR: "390" diff --git a/.changes/unreleased/NOTES-390-20240419-102736.yaml b/.changes/unreleased/NOTES-390-20240419-102736.yaml new file mode 100644 index 00000000..65fd7e45 --- /dev/null +++ b/.changes/unreleased/NOTES-390-20240419-102736.yaml @@ -0,0 +1,6 @@ +kind: NOTES +body: The `Workspace` CRD has been changed. Please follow the Helm chart instructions + on how to upgrade it. +time: 2024-04-19T10:27:36.644376+02:00 +custom: + PR: "390" diff --git a/api/v1alpha2/workspace_helpers.go b/api/v1alpha2/workspace_helpers.go index 2120ca2c..c28ff40d 100644 --- a/api/v1alpha2/workspace_helpers.go +++ b/api/v1alpha2/workspace_helpers.go @@ -3,6 +3,52 @@ package v1alpha2 +import ( + "github.com/hashicorp/terraform-cloud-operator/internal/slice" +) + func (w *Workspace) IsCreationCandidate() bool { return w.Status.WorkspaceID == "" } + +// AddOrUpdateVariableStatus adds a given variable to the status if it does not exist there; otherwise, it updates it. +func (s *WorkspaceStatus) AddOrUpdateVariableStatus(variable VariableStatus) { + for i, v := range s.Variables { + if v.Name == variable.Name && v.Category == variable.Category { + s.Variables[i].ID = variable.ID + s.Variables[i].VersionID = variable.VersionID + s.Variables[i].ValueID = variable.ValueID + s.Variables[i].Category = variable.Category + return + } + } + + s.Variables = append(s.Variables, VariableStatus{ + Name: variable.Name, + ID: variable.ID, + VersionID: variable.VersionID, + ValueID: variable.ValueID, + Category: variable.Category, + }) +} + +// GetVariableStatus returns a given variable from the status if it exists there; otherwise, nil. +func (s *WorkspaceStatus) GetVariableStatus(variable VariableStatus) *VariableStatus { + for _, v := range s.Variables { + if v.Name == variable.Name && v.Category == variable.Category { + return &v + } + } + + return nil +} + +// DeleteVariableStatus deletes a given variable from the status. +func (s *WorkspaceStatus) DeleteVariableStatus(variable VariableStatus) { + for i, v := range s.Variables { + if v.Name == variable.Name && v.Category == variable.Category { + s.Variables = slice.RemoveFromSlice(s.Variables, i) + return + } + } +} diff --git a/api/v1alpha2/workspace_types.go b/api/v1alpha2/workspace_types.go index b3a138a5..abfa51b2 100644 --- a/api/v1alpha2/workspace_types.go +++ b/api/v1alpha2/workspace_types.go @@ -598,6 +598,19 @@ type RunStatus struct { OutputRunID string `json:"outputRunID,omitempty"` } +type VariableStatus struct { + // Name of the variable. + Name string `json:"name"` + // ID of the variable. + ID string `json:"id"` + // VersionID is a hash of the variable on the TFC end. + VersionID string `json:"versionID"` + // ValueID is a hash of the variable on the CRD end. + ValueID string `json:"valueID"` + // Category of the variable. + Category string `json:"category"` +} + // WorkspaceStatus defines the observed state of Workspace. type WorkspaceStatus struct { // Workspace ID that is managed by the controller. @@ -624,6 +637,10 @@ type WorkspaceStatus struct { //+kubebuilder:validation:Pattern:="^\\d{1}\\.\\d{1,2}\\.\\d{1,2}$" //+optional TerraformVersion string `json:"terraformVersion,omitempty"` + // Workspace variables. + // + //+optional + Variables []VariableStatus `json:"variables,omitempty"` } //+kubebuilder:object:root=true diff --git a/api/v1alpha2/zz_generated.deepcopy.go b/api/v1alpha2/zz_generated.deepcopy.go index bbacb6c0..cb29376c 100644 --- a/api/v1alpha2/zz_generated.deepcopy.go +++ b/api/v1alpha2/zz_generated.deepcopy.go @@ -862,6 +862,21 @@ func (in *Variable) DeepCopy() *Variable { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *VariableStatus) DeepCopyInto(out *VariableStatus) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new VariableStatus. +func (in *VariableStatus) DeepCopy() *VariableStatus { + if in == nil { + return nil + } + out := new(VariableStatus) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *VersionControl) DeepCopyInto(out *VersionControl) { *out = *in @@ -1082,6 +1097,11 @@ func (in *WorkspaceStatus) DeepCopyInto(out *WorkspaceStatus) { *out = new(PlanStatus) **out = **in } + if in.Variables != nil { + in, out := &in.Variables, &out.Variables + *out = make([]VariableStatus, len(*in)) + copy(*out, *in) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new WorkspaceStatus. diff --git a/charts/terraform-cloud-operator/README.md b/charts/terraform-cloud-operator/README.md index d6d3aee4..2583caf5 100644 --- a/charts/terraform-cloud-operator/README.md +++ b/charts/terraform-cloud-operator/README.md @@ -84,6 +84,14 @@ For a more detailed explanation, please refer to the [FAQ](../../docs/faq.md#gen #### Upgrade recommendations + - `2.3.0` to `2.4.0` + + - The `Workspace` CRD has been changed: + + ```console + $ kubectl replace -f https://raw.githubusercontent.com/hashicorp/terraform-cloud-operator/v2.4.0/charts/terraform-cloud-operator/crds/app.terraform.io_workspaces.yaml + ``` + - `2.2.0` to `2.3.0` - The `Workspace` CRD has been changed: diff --git a/charts/terraform-cloud-operator/README.md.gotmpl b/charts/terraform-cloud-operator/README.md.gotmpl index e1199662..5ac0bdd6 100644 --- a/charts/terraform-cloud-operator/README.md.gotmpl +++ b/charts/terraform-cloud-operator/README.md.gotmpl @@ -84,6 +84,14 @@ For a more detailed explanation, please refer to the [FAQ](../../docs/faq.md#gen #### Upgrade recommendations + - `2.3.0` to `2.4.0` + + - The `Workspace` CRD has been changed: + + ```console + $ kubectl replace -f https://raw.githubusercontent.com/hashicorp/terraform-cloud-operator/v2.4.0/charts/terraform-cloud-operator/crds/app.terraform.io_workspaces.yaml + ``` + - `2.2.0` to `2.3.0` - The `Workspace` CRD has been changed: diff --git a/charts/terraform-cloud-operator/crds/app.terraform.io_workspaces.yaml b/charts/terraform-cloud-operator/crds/app.terraform.io_workspaces.yaml index 7c0a53e3..80386762 100644 --- a/charts/terraform-cloud-operator/crds/app.terraform.io_workspaces.yaml +++ b/charts/terraform-cloud-operator/crds/app.terraform.io_workspaces.yaml @@ -758,6 +758,34 @@ spec: description: Workspace last update timestamp. format: int64 type: integer + variables: + description: Workspace variables. + items: + properties: + category: + description: Category of the variable. + type: string + id: + description: ID of the variable. + type: string + name: + description: Name of the variable. + type: string + valueID: + description: ValueID is a hash of the variable on the CRD end. + type: string + versionID: + description: VersionID is a hash of the variable on the TFC + end. + type: string + required: + - category + - id + - name + - valueID + - versionID + type: object + type: array workspaceID: description: Workspace ID that is managed by the controller. type: string diff --git a/config/crd/bases/app.terraform.io_workspaces.yaml b/config/crd/bases/app.terraform.io_workspaces.yaml index af588ac1..ec53a1e8 100644 --- a/config/crd/bases/app.terraform.io_workspaces.yaml +++ b/config/crd/bases/app.terraform.io_workspaces.yaml @@ -755,6 +755,34 @@ spec: description: Workspace last update timestamp. format: int64 type: integer + variables: + description: Workspace variables. + items: + properties: + category: + description: Category of the variable. + type: string + id: + description: ID of the variable. + type: string + name: + description: Name of the variable. + type: string + valueID: + description: ValueID is a hash of the variable on the CRD end. + type: string + versionID: + description: VersionID is a hash of the variable on the TFC + end. + type: string + required: + - category + - id + - name + - valueID + - versionID + type: object + type: array workspaceID: description: Workspace ID that is managed by the controller. type: string diff --git a/controllers/workspace_controller.go b/controllers/workspace_controller.go index 2e10504e..15b3cc3d 100644 --- a/controllers/workspace_controller.go +++ b/controllers/workspace_controller.go @@ -11,6 +11,8 @@ import ( "os" "strconv" + "github.com/go-logr/logr" + tfc "github.com/hashicorp/go-tfe" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" @@ -22,9 +24,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/predicate" - "github.com/go-logr/logr" - - tfc "github.com/hashicorp/go-tfe" appv1alpha2 "github.com/hashicorp/terraform-cloud-operator/api/v1alpha2" "github.com/hashicorp/terraform-cloud-operator/version" ) diff --git a/controllers/workspace_controller_variables.go b/controllers/workspace_controller_variables.go index c43a4c3e..c0f0bd96 100644 --- a/controllers/workspace_controller_variables.go +++ b/controllers/workspace_controller_variables.go @@ -5,185 +5,163 @@ package controllers import ( "context" + "crypto/sha256" + "encoding/gob" + "encoding/hex" "fmt" + tfc "github.com/hashicorp/go-tfe" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" - "github.com/google/go-cmp/cmp" - "github.com/google/go-cmp/cmp/cmpopts" - tfc "github.com/hashicorp/go-tfe" appv1alpha2 "github.com/hashicorp/terraform-cloud-operator/api/v1alpha2" ) -// HELPERS - -// getVariablesToCreate returns a map of variables that need to be created, i.e. exist in manifest, but absent in workspace. -func getVariablesToCreate(instanceVariables, workspaceVariables map[string]tfc.Variable) map[string]tfc.Variable { - return varDifference(instanceVariables, workspaceVariables) -} - -// getVariablesToDelete returns a map of variables that need to be deleted, i.e. exist in workspace, but absent in manifest. -func getVariablesToDelete(instanceVariables, workspaceVariables map[string]tfc.Variable) map[string]tfc.Variable { - return varDifference(workspaceVariables, instanceVariables) -} - -// getVariablesToUpdate returns a map of variables that exist in both -// the manifest and workspace but have differing values. -// -// NOTE We cannot read the value of sensitive variables from the -// Terraform Cloud API, so sensitive variables will always need to be updated. -func getVariablesToUpdate(instanceVariables, workspaceVariables map[string]tfc.Variable) map[string]tfc.Variable { - variables := make(map[string]tfc.Variable) - - if len(instanceVariables) == 0 || len(workspaceVariables) == 0 { - return variables - } - - for ik, iv := range instanceVariables { - if wv, ok := workspaceVariables[ik]; ok { - if !isNoLongerSensitive(iv.Sensitive, wv.Sensitive) && !cmp.Equal(iv, wv, cmpopts.IgnoreFields(tfc.Variable{}, "ID", "VersionID", "Workspace")) { - iv.ID = wv.ID - variables[iv.Key] = iv - } - } +// variableValueID calculates a hash of a variable. +func variableValueID(v tfc.Variable) string { + hash := sha256.New() + hv := appv1alpha2.Variable{ + Name: v.Key, + Description: v.Description, + HCL: v.HCL, + Sensitive: v.Sensitive, + Value: v.Value, } + gob.NewEncoder(hash).Encode(hv) - return variables + return hex.EncodeToString(hash.Sum(nil)) } -// getVariablesRequiringRecreate returns a map of variables requiring to recreate. -func getVariablesRequiringRecreate(instanceVariables, workspaceVariables map[string]tfc.Variable) map[string]tfc.Variable { - variables := make(map[string]tfc.Variable) - - if len(instanceVariables) == 0 || len(workspaceVariables) == 0 { - return variables - } - - // When the "Sensitive" attribute changes from true to false we need to delete and recreate a variable - for ik, iv := range instanceVariables { - if wv, ok := workspaceVariables[ik]; ok { - if isNoLongerSensitive(iv.Sensitive, wv.Sensitive) { - iv.ID = wv.ID - variables[iv.Key] = iv - } - } +func createWorkspaceVariable(ctx context.Context, w *workspaceInstance, variable tfc.Variable) error { + w.log.Info("Reconcile Variables", "msg", fmt.Sprintf("creating %s variable %s", variable.Category, variable.Key)) + v, err := w.tfClient.Client.Variables.Create(ctx, w.instance.Status.WorkspaceID, tfc.VariableCreateOptions{ + Key: &variable.Key, + Value: &variable.Value, + Description: &variable.Description, + Category: &variable.Category, + HCL: &variable.HCL, + Sensitive: &variable.Sensitive, + }) + if err != nil { + w.log.Error(err, "Reconcile Variables", "msg", fmt.Sprintf("failed to create %s variable %s", variable.Category, variable.Key)) + return err } - return variables -} + w.log.Info("Reconcile Variables", "msg", fmt.Sprintf("successfully created %s variable %s", variable.Category, variable.Key)) + w.instance.Status.AddOrUpdateVariableStatus(appv1alpha2.VariableStatus{ + Name: v.Key, + ID: v.ID, + VersionID: v.VersionID, + ValueID: variableValueID(variable), + Category: string(v.Category), + }) -// isNoLongerSensitive verifies either the attribute 'Sensitive' of the variable changed from 'true' to 'false'. -func isNoLongerSensitive(instanceSensitive, workspaceSensitive bool) bool { - return !instanceSensitive && workspaceSensitive + return nil } -// varDifference returns a map that contains the elements of map `a` that are not in map `b`. -// It compares only the names of the variables, not their content. -func varDifference(a, b map[string]tfc.Variable) map[string]tfc.Variable { - variables := make(map[string]tfc.Variable) - - for k, v := range a { - if _, ok := b[k]; !ok { - variables[k] = v +func updateWorkspaceVariable(ctx context.Context, w *workspaceInstance, specVariable, workspaceVariable tfc.Variable) error { + // If a workspace variable is marked as sensitive, it cannot be updated to become non-sensitive. + // In such cases, the variable must be removed and a new one created. + // This condition addresses the scenario where a variable is non-sensitive in the specification but sensitive in the workspace. + // | spec.Sensitive | workspace.Sensitive | AND | + // | 0(!= 1) | 0 | 0 | + // | 0(!= 1) | 1 | 1 | + // | 1(!= 0) | 0 | 0 | + // | 1(!= 0) | 1 | 0 | + if !specVariable.Sensitive && workspaceVariable.Sensitive { + w.log.Info("Reconcile Variables", "msg", fmt.Sprintf("updating %s variable %s due to sensitivity changes", specVariable.Category, specVariable.Key)) + if err := deleteWorkspaceVariable(ctx, w, workspaceVariable); err != nil { + return err } - } - - return variables -} - -func (r *WorkspaceReconciler) createWorkspaceVariables(ctx context.Context, w *workspaceInstance, variables map[string]tfc.Variable, category tfc.CategoryType) error { - for _, v := range variables { - _, err := w.tfClient.Client.Variables.Create(ctx, w.instance.Status.WorkspaceID, tfc.VariableCreateOptions{ - Key: &v.Key, - Description: &v.Description, - Value: &v.Value, - HCL: &v.HCL, - Sensitive: &v.Sensitive, - Category: &category, - }) - if err != nil { + if err := createWorkspaceVariable(ctx, w, workspaceVariable); err != nil { return err } - } + w.log.Info("Reconcile Variables", "msg", fmt.Sprintf("successfully updated %s variable %s", specVariable.Category, specVariable.Key)) - return nil -} - -// getWorkspaceVariables returns a list of all variables associated with the workspace. -func (r *WorkspaceReconciler) getWorkspaceVariables(ctx context.Context, w *workspaceInstance, workspace *tfc.Workspace) ([]*tfc.Variable, error) { - if workspace.Variables == nil { - return []*tfc.Variable{}, nil - } - - v, err := w.tfClient.Client.Variables.List(ctx, w.instance.Status.WorkspaceID, &tfc.VariableListOptions{}) - if err != nil { - return nil, err + return nil } - return v.Items, nil -} -func (r *WorkspaceReconciler) updateWorkspaceVariables(ctx context.Context, w *workspaceInstance, variables map[string]tfc.Variable) error { - for _, v := range variables { - _, err := w.tfClient.Client.Variables.Update(ctx, w.instance.Status.WorkspaceID, v.ID, tfc.VariableUpdateOptions{ - Key: &v.Key, - Description: &v.Description, - Value: &v.Value, - HCL: &v.HCL, - Sensitive: &v.Sensitive, - Category: &v.Category, + vID := variableValueID(specVariable) + statusVariable := w.instance.Status.GetVariableStatus(appv1alpha2.VariableStatus{ + Name: specVariable.Key, + Category: string(specVariable.Category), + }) + // Update a variable if one of three conditions is true: + // - A variable is not in Status, indicating that the variable is present in the workspace but not managed by the operator yet. + // - A variable's Status and workspace VersionID do not match, indicating that the variable has been updated outside of the operator. + // - A variable's Status and workspace ValueID do not match, indicating that the variable has been updated via the spec. + if statusVariable == nil || statusVariable.VersionID != workspaceVariable.VersionID || statusVariable.ValueID != vID { + w.log.Info("Reconcile Variables", "msg", fmt.Sprintf("updating %s variable %s", specVariable.Category, specVariable.Key)) + v, err := w.tfClient.Client.Variables.Update(ctx, w.instance.Status.WorkspaceID, workspaceVariable.ID, tfc.VariableUpdateOptions{ + Key: &specVariable.Key, + Value: &specVariable.Value, + Description: &specVariable.Description, + Category: &specVariable.Category, + HCL: &specVariable.HCL, + Sensitive: &specVariable.Sensitive, }) if err != nil { + w.log.Error(err, "Reconcile Variables", "msg", fmt.Sprintf("failed to update %s variable %s", specVariable.Category, specVariable.Key)) return err } + + w.log.Info("Reconcile Variables", "msg", fmt.Sprintf("successfully updated %s variable %s", specVariable.Category, specVariable.Key)) + w.instance.Status.AddOrUpdateVariableStatus(appv1alpha2.VariableStatus{ + Name: v.Key, + ID: v.ID, + VersionID: v.VersionID, + ValueID: vID, + Category: string(v.Category), + }) } return nil } -// updateWorkspaceSensitiveVariables treats a special case when the attribute 'Sensitive' of the variable changed from 'true' to 'false'. -// In this case, we have to delete the variable and create a new one. -func (r *WorkspaceReconciler) updateWorkspaceSensitiveVariables(ctx context.Context, w *workspaceInstance, variables map[string]tfc.Variable, category tfc.CategoryType) error { - err := r.deleteWorkspaceVariables(ctx, w, variables) - if err != nil { - return err - } - err = r.createWorkspaceVariables(ctx, w, variables, category) +func deleteWorkspaceVariable(ctx context.Context, w *workspaceInstance, variable tfc.Variable) error { + w.log.Info("Reconcile Variables", "msg", fmt.Sprintf("deleteing %s variable %s", variable.Category, variable.Key)) + err := w.tfClient.Client.Variables.Delete(ctx, w.instance.Status.WorkspaceID, variable.ID) if err != nil { + w.log.Error(err, "Reconcile Variables", "msg", fmt.Sprintf("failed to delete %s variable %s", variable.Category, variable.Key)) return err } + w.log.Info("Reconcile Variables", "msg", fmt.Sprintf("successfully deleted %s variable %s", variable.Category, variable.Key)) + w.instance.Status.DeleteVariableStatus(appv1alpha2.VariableStatus{Name: variable.Key, Category: string(variable.Category)}) + return nil } -func (r *WorkspaceReconciler) deleteWorkspaceVariables(ctx context.Context, w *workspaceInstance, variables map[string]tfc.Variable) error { - workspaceID := w.instance.Status.WorkspaceID - for _, v := range variables { - err := w.tfClient.Client.Variables.Delete(ctx, workspaceID, v.ID) - if err != nil { - return err - } +// getWorkspaceVariables returns a list of all variables associated with the workspace. +func (r *WorkspaceReconciler) getWorkspaceVariables(ctx context.Context, w *workspaceInstance, workspace *tfc.Workspace) ([]*tfc.Variable, error) { + if workspace.Variables == nil { + return []*tfc.Variable{}, nil } - return nil + w.log.Info("Reconcile Variables", "msg", "getting workspace variables") + v, err := w.tfClient.Client.Variables.List(ctx, w.instance.Status.WorkspaceID, &tfc.VariableListOptions{}) + if err != nil { + w.log.Error(err, "Reconcile Variables", "msg", "failed to get workspace variables") + return nil, err + } + w.log.Info("Reconcile Variables", "msg", "successfully got workspace variables") + + return v.Items, nil } // getVariablesByCategory returns a map of all instance variables by type. -func (r *WorkspaceReconciler) getVariablesByCategory(ctx context.Context, w *workspaceInstance, category tfc.CategoryType) map[string]tfc.Variable { +func (r *WorkspaceReconciler) getVariablesByCategory(ctx context.Context, w *workspaceInstance, category tfc.CategoryType) (map[string]tfc.Variable, error) { variables := make(map[string]tfc.Variable) - var instanceVariables []appv1alpha2.Variable - switch category { - case tfc.CategoryEnv: - instanceVariables = w.instance.Spec.EnvironmentVariables - case tfc.CategoryTerraform: - instanceVariables = w.instance.Spec.TerraformVariables + specVariables := w.instance.Spec.TerraformVariables + if category == tfc.CategoryEnv { + specVariables = w.instance.Spec.EnvironmentVariables } - if len(instanceVariables) == 0 { - return variables + if len(specVariables) == 0 { + return variables, nil } - for _, v := range instanceVariables { + for _, v := range specVariables { value := v.Value if v.ValueFrom != nil { var err error @@ -200,22 +178,22 @@ func (r *WorkspaceReconciler) getVariablesByCategory(ctx context.Context, w *wor } if err != nil { w.log.Error(err, "Reconcile Variables", "msg", fmt.Sprintf("failed to get value for the variable %s", v.Name)) - r.Recorder.Event(&w.instance, corev1.EventTypeWarning, "ReconcileVariables", "Failed to get value for a variable") - continue + r.Recorder.Event(&w.instance, corev1.EventTypeWarning, "ReconcileVariables", fmt.Sprintf("Failed to get value for the variable %s", v.Name)) + return nil, err } } variables[v.Name] = tfc.Variable{ Key: v.Name, - Description: v.Description, Value: value, + Description: v.Description, + Category: category, HCL: v.HCL, Sensitive: v.Sensitive, - Category: category, } } - return variables + return variables, nil } // getWorkspaceVariablesByCategory returns a map of all workspace variables by type. @@ -224,15 +202,7 @@ func getWorkspaceVariablesByCategory(workspaceVariables []*tfc.Variable, categor for _, v := range workspaceVariables { if v.Category == category { - variables[v.Key] = tfc.Variable{ - ID: v.ID, - Key: v.Key, - Description: v.Description, - Value: v.Value, - HCL: v.HCL, - Sensitive: v.Sensitive, - Category: category, - } + variables[v.Key] = *v } } @@ -240,48 +210,42 @@ func getWorkspaceVariablesByCategory(workspaceVariables []*tfc.Variable, categor } func (r *WorkspaceReconciler) reconcileVariablesByCategory(ctx context.Context, w *workspaceInstance, variables []*tfc.Variable, category tfc.CategoryType) error { - instanceVariables := r.getVariablesByCategory(ctx, w, category) workspaceVariables := getWorkspaceVariablesByCategory(variables, category) + specVariables, err := r.getVariablesByCategory(ctx, w, category) + if err != nil { + return err + } - if len(instanceVariables) == 0 && len(workspaceVariables) == 0 { + if len(specVariables) == 0 && len(workspaceVariables) == 0 { w.log.Info("Reconcile Variables", "msg", fmt.Sprintf("there are no %s variables both in spec and workspace", category)) return nil } - workspaceID := w.instance.Status.WorkspaceID - - daleteVariables := getVariablesToDelete(instanceVariables, workspaceVariables) - if len(daleteVariables) > 0 { - w.log.Info("Reconcile Variables", "msg", fmt.Sprintf("deleting %d %s variables from the workspace ID %s", len(daleteVariables), category, workspaceID)) - err := r.deleteWorkspaceVariables(ctx, w, daleteVariables) - if err != nil { - return err - } - } - - createVariables := getVariablesToCreate(instanceVariables, workspaceVariables) - if len(createVariables) > 0 { - w.log.Info("Reconcile Variables", "msg", fmt.Sprintf("creating %d new %s variables to the workspace ID %s", len(createVariables), category, workspaceID)) - err := r.createWorkspaceVariables(ctx, w, createVariables, category) - if err != nil { - return err - } - } - - updateVariables := getVariablesToUpdate(instanceVariables, workspaceVariables) - if len(updateVariables) > 0 { - w.log.Info("Reconcile Variables", "msg", fmt.Sprintf("updating %d %s variables in the workspace ID %s", len(updateVariables), category, workspaceID)) - err := r.updateWorkspaceVariables(ctx, w, updateVariables) - if err != nil { - return err + w.log.Info("Reconcile Variables", "msg", fmt.Sprintf("there are %d %s variables in spec", len(specVariables), category)) + w.log.Info("Reconcile Variables", "msg", fmt.Sprintf("there are %d %s variables in workspace", len(workspaceVariables), category)) + + // Let's consider spec and workspace variables as two sets. + // The comparison of these sets allows us to identify variables needing creation, deletion, or updating: + // - The set difference of spec and workspace gives variables for creation. + // - The set difference of workspace and spec gives variables for deletion. + // - Intersection of spec and workspace sets gives variables for updating. + // Iterating over the spec set and comparing it with the workspace set provides creation and update candidates. + // Updated variables are removed from the workspace set, leaving only deletion candidates. + for sk, sv := range specVariables { + if wv, ok := workspaceVariables[sk]; ok { + if err := updateWorkspaceVariable(ctx, w, sv, wv); err != nil { + return err + } + delete(workspaceVariables, sk) + } else { + if err := createWorkspaceVariable(ctx, w, sv); err != nil { + return err + } } } - recreateVariables := getVariablesRequiringRecreate(instanceVariables, workspaceVariables) - if len(recreateVariables) > 0 { - w.log.Info("Reconcile Variables", "msg", fmt.Sprintf("making %d %s variables no sensitive in the workspace ID %s", len(recreateVariables), category, workspaceID)) - err := r.updateWorkspaceSensitiveVariables(ctx, w, recreateVariables, category) - if err != nil { + for _, wv := range workspaceVariables { + if err := deleteWorkspaceVariable(ctx, w, wv); err != nil { return err } } @@ -297,12 +261,10 @@ func (r *WorkspaceReconciler) reconcileVariables(ctx context.Context, w *workspa return err } - // Reconcile Terraform Variables if err = r.reconcileVariablesByCategory(ctx, w, workspaceVariables, tfc.CategoryTerraform); err != nil { return err } - // Reconcile Environment Variables if err = r.reconcileVariablesByCategory(ctx, w, workspaceVariables, tfc.CategoryEnv); err != nil { return err } diff --git a/controllers/workspace_controller_variables_test.go b/controllers/workspace_controller_variables_test.go index f411235b..7d26b0e0 100644 --- a/controllers/workspace_controller_variables_test.go +++ b/controllers/workspace_controller_variables_test.go @@ -20,7 +20,7 @@ import ( appv1alpha2 "github.com/hashicorp/terraform-cloud-operator/api/v1alpha2" ) -var _ = Describe("Workspace controller", Ordered, func() { +var _ = Describe("Workspace controller", Label("Variables"), Ordered, func() { var ( instance *appv1alpha2.Workspace namespacedName = types.NamespacedName{ @@ -361,35 +361,6 @@ var _ = Describe("Workspace controller", Ordered, func() { }) }) -var _ = Describe("Makes Sensitive Workspace Variables No Sensitive", func() { - Context("Update Variables", func() { - It("Instance No Sensitive, Workspace No Sensitive", func() { - instanceVariable := false - workspaceVariable := false - r := isNoLongerSensitive(instanceVariable, workspaceVariable) - Expect(r).Should(BeFalse()) - }) - It("Instance No Sensitive, Workspace Sensitive", func() { - instanceVariable := false - workspaceVariable := true - r := isNoLongerSensitive(instanceVariable, workspaceVariable) - Expect(r).Should(BeTrue()) - }) - It("Instance Sensitive, Workspace No Sensitive", func() { - instanceVariable := true - workspaceVariable := false - r := isNoLongerSensitive(instanceVariable, workspaceVariable) - Expect(r).Should(BeFalse()) - }) - It("Instance Sensitive, Workspace Sensitive", func() { - instanceVariable := true - workspaceVariable := true - r := isNoLongerSensitive(instanceVariable, workspaceVariable) - Expect(r).Should(BeFalse()) - }) - }) -}) - // compareVars compares two slices of variables and returns 'true' if they are equal and 'false' otherwise. It ignores field 'ID'. func compareVars(aVars, bVars []tfc.Variable) bool { if len(aVars) != len(bVars) { diff --git a/docs/api-reference.md b/docs/api-reference.md index f01ceb7d..65effaaa 100644 --- a/docs/api-reference.md +++ b/docs/api-reference.md @@ -638,6 +638,24 @@ _Appears in:_ | `valueFrom` _[ValueFrom](#valuefrom)_ | Source for the variable's value. Cannot be used if value is not empty. | +#### VariableStatus + + + + + +_Appears in:_ +- [WorkspaceStatus](#workspacestatus) + +| Field | Description | +| --- | --- | +| `name` _string_ | Name of the variable. | +| `id` _string_ | ID of the variable. | +| `versionID` _string_ | VersionID is a hash of the variable on the TFC end. | +| `valueID` _string_ | ValueID is a hash of the variable on the CRD end. | +| `category` _string_ | Category of the variable. | + + #### VersionControl