Fix: reconcile after configuration's hcl changed (#360)

Co-authored-by: caoxingming <caoxingming@jd.com>

Author: caoxingming

controllers/configuration_controller.go

@@ -213,7 +213,7 @@ func (r *ConfigurationReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 
 	var tfExecutionJob = &batchv1.Job{}
 	if err := meta.getApplyJob(ctx, r.Client, tfExecutionJob); err == nil {
-		if !meta.EnvChanged && tfExecutionJob.Status.Succeeded == int32(1) {
+		if !meta.EnvChanged && !meta.ConfigurationChanged && tfExecutionJob.Status.Succeeded == int32(1) {
 			err = meta.updateApplyStatus(ctx, r.Client, types.Available, types.MessageCloudResourceDeployed)
 			return ctrl.Result{}, err
 		}
@@ -635,24 +635,20 @@ func (r *ConfigurationReconciler) preCheck(ctx context.Context, configuration *v
 	}
 	meta.CompleteConfiguration, meta.Backend = completeConfiguration, backendConf
 
+	// Check whether configuration(hcl/json) is changed
+	if err := meta.CheckWhetherConfigurationChanges(ctx, k8sClient, configurationType); err != nil {
+		return err
+	}
+
 	if configuration.ObjectMeta.DeletionTimestamp.IsZero() {
 		if err := meta.storeTFConfiguration(ctx, k8sClient); err != nil {
 			return err
 		}
 	}
 
-	// Check whether configuration(hcl/json) is changed
-	if err := meta.CheckWhetherConfigurationChanges(ctx, k8sClient, configurationType); err != nil {
-		return err
-	}
-
 	if meta.ConfigurationChanged {
 		klog.InfoS("Configuration hanged, reloading...")
-		if err := meta.updateApplyStatus(ctx, k8sClient, types.ConfigurationReloading, types.ConfigurationReloadingAsHCLChanged); err != nil {
-			return err
-		}
-		// store configuration to ConfigMap
-		return meta.storeTFConfiguration(ctx, k8sClient)
+		return meta.updateApplyStatus(ctx, k8sClient, types.ConfigurationReloading, types.ConfigurationReloadingAsHCLChanged)
 	}
 
 	// Check whether env changes
@@ -1462,17 +1458,17 @@ func (meta *TFConfigurationMeta) storeTFConfiguration(ctx context.Context, k8sCl
 
 // CheckWhetherConfigurationChanges will check whether configuration is changed
 func (meta *TFConfigurationMeta) CheckWhetherConfigurationChanges(ctx context.Context, k8sClient client.Client, configurationType types.ConfigurationType) error {
-	var cm v1.ConfigMap
-	if err := k8sClient.Get(ctx, client.ObjectKey{Name: meta.ConfigurationCMName, Namespace: meta.ControllerNamespace}, &cm); err != nil {
-		return err
-	}
-
-	var configurationChanged bool
 	switch configurationType {
 	case types.ConfigurationHCL:
-		configurationChanged = cm.Data[types.TerraformHCLConfigurationName] != meta.CompleteConfiguration
-		meta.ConfigurationChanged = configurationChanged
-		if configurationChanged {
+		var cm v1.ConfigMap
+		if err := k8sClient.Get(ctx, client.ObjectKey{Name: meta.ConfigurationCMName, Namespace: meta.ControllerNamespace}, &cm); err != nil {
+			if kerrors.IsNotFound(err) {
+				return nil
+			}
+			return err
+		}
+		meta.ConfigurationChanged = cm.Data[types.TerraformHCLConfigurationName] != meta.CompleteConfiguration
+		if meta.ConfigurationChanged {
 			klog.InfoS("Configuration HCL changed", "ConfigMap", cm.Data[types.TerraformHCLConfigurationName],
 				"RenderedCompletedConfiguration", meta.CompleteConfiguration)
 		}

controllers/configuration_controller_test.go

@@ -659,6 +659,78 @@ func TestConfigurationReconcile(t *testing.T) {
 		WithObjects(namespace, secret, provider, configuration6).
 		Build()
 
+	// for case "Configuration changed, and reconcile"
+	appliedConfigurationCM := &corev1.ConfigMap{
+		ObjectMeta: v1.ObjectMeta{
+			Name:      "tf-a",
+			Namespace: req.Namespace,
+		},
+		Data: map[string]string{types.TerraformHCLConfigurationName: `Here is the original hcl
+
+terraform {
+  backend "kubernetes" {
+    secret_suffix     = "a"
+    in_cluster_config = true
+    namespace         = "b"
+  }
+}
+`,
+		},
+	}
+	varMap := map[string]string{"name": "abc"}
+	appliedEnvVariable := &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      fmt.Sprintf(TFVariableSecret, req.Name),
+			Namespace: req.Namespace,
+		},
+		Data: map[string][]byte{
+			"TF_VAR_name":             []byte(varMap["name"]),
+			"ALICLOUD_ACCESS_KEY":     []byte(ak.AccessKeyID),
+			"ALICLOUD_SECRET_KEY":     []byte(ak.AccessKeySecret),
+			"ALICLOUD_REGION":         []byte(provider.Spec.Region),
+			"ALICLOUD_SECURITY_TOKEN": []byte(""),
+		},
+		Type: corev1.SecretTypeOpaque,
+	}
+	appliedJobName := req.Name + "-" + string(TerraformApply)
+	appliedJob := &batchv1.Job{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      appliedJobName,
+			Namespace: req.Namespace,
+			UID:       "111",
+		},
+		Status: batchv1.JobStatus{
+			Succeeded: int32(1),
+		},
+	}
+	varData, _ := json.Marshal(varMap)
+	configuration7 := &v1beta2.Configuration{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "a",
+			Namespace: "b",
+		},
+		Spec: v1beta2.ConfigurationSpec{
+			HCL:      "Here is the changed hcl",
+			Variable: &runtime.RawExtension{Raw: varData},
+			ProviderReference: &crossplane.Reference{
+				Name:      "default",
+				Namespace: "default",
+			},
+			WriteConnectionSecretToReference: &crossplane.SecretReference{
+				Name:      "db-conn",
+				Namespace: "default",
+			},
+		},
+		Status: v1beta2.ConfigurationStatus{
+			Apply: v1beta2.ConfigurationApplyStatus{
+				State: types.Available,
+			},
+		},
+	}
+	r7 := &ConfigurationReconciler{}
+	r7.Client = fake.NewClientBuilder().WithScheme(s).WithObjects(secret, provider, backendSecret,
+		appliedJob, appliedEnvVariable, appliedConfigurationCM, configuration7).Build()
+
 	type args struct {
 		req reconcile.Request
 		r   *ConfigurationReconciler
@@ -726,6 +798,23 @@ func TestConfigurationReconcile(t *testing.T) {
 				}
 			},
 		},
+		{
+			name: "Configuration changed, and reconcile",
+			args: args{
+				req: req,
+				r:   r7,
+			},
+			check: func(t *testing.T, cc client.Client) {
+				job := &batchv1.Job{}
+				if err = cc.Get(context.TODO(), k8stypes.NamespacedName{Name: appliedJobName, Namespace: req.Namespace}, job); err != nil {
+					t.Error("Failed to retrieve the new job")
+				}
+				assert.Equal(t, job.Name, appliedJob.Name, "Not expected job name")
+				assert.Equal(t, job.Namespace, appliedJob.Namespace, "Not expected job namespace")
+				assert.NotEqual(t, job.UID, appliedJob.UID, "No new job created")
+				assert.NotEqual(t, job.Status.Succeeded, appliedJob.Status.Succeeded, "Not expected job status")
+			},
+		},
 	}
 
 	for _, tc := range testcases {
@@ -2510,10 +2599,36 @@ func TestCheckWhetherConfigurationChanges(t *testing.T) {
 					Namespace:           "b",
 					ControllerNamespace: "b",
 				},
-				configurationType: "xxx",
+				configurationType: "HCL",
 			},
 			want: want{
-				errMsg: "not found",
+				errMsg: "",
+			},
+		},
+		"configuration type is remote": {
+			args: args{
+				meta: &TFConfigurationMeta{
+					ConfigurationCMName: "aaa",
+					Namespace:           "b",
+					ControllerNamespace: "b",
+				},
+				configurationType: "Remote",
+			},
+			want: want{
+				errMsg: "",
+			},
+		},
+		"create configuration for the first time": {
+			args: args{
+				meta: &TFConfigurationMeta{
+					ConfigurationCMName: "aa",
+					Namespace:           "b",
+					ControllerNamespace: "b",
+				},
+				configurationType: "HCL",
+			},
+			want: want{
+				errMsg: "",
 			},
 		},
 	}
@@ -2525,6 +2640,11 @@ func TestCheckWhetherConfigurationChanges(t *testing.T) {
 					t.Errorf("CheckWhetherConfigurationChanges() error = %v, wantErr %v", err, tc.want.errMsg)
 				}
 			}
+
+			if tc.want.errMsg == "" {
+				assert.Nil(t, err)
+				assert.False(t, tc.args.meta.ConfigurationChanged)
+			}
 		})
 	}
 }