Fix: legacy backend state reading and GC (#338)

Author: chivalryq

.github/workflows/e2e-test.yml

@@ -61,6 +61,9 @@ jobs:
           make configuration
         env:
           TERRAFORM_BACKEND_NAMESPACE: terraform
+      - name: dump controller logs
+        if: ${{ always() }}
+        run: kubectl logs deploy/terraform-controller -n terraform
 
       - name: Upload coverage report
         uses: codecov/codecov-action@v2

Makefile

@@ -267,7 +267,8 @@ custom: custom-credentials custom-provider
 
 
 configuration:
-	go test -coverprofile=e2e-coverage1.xml -v ./e2e/... -count=1
+	go test -coverprofile=e2e-coverage1.xml -v $(go list ./e2e/...|grep -v controllernamespace) -count=1
+	go test -v ./e2e/controllernamespace/...
 
 e2e-setup: install-chart alibaba
 

controllers/configuration/backend/backend.go

@@ -56,7 +56,7 @@ var backendInitFuncMap = map[string]backendInitFunc{
 }
 
 // ParseConfigurationBackend parses backend Conf from the v1beta2.Configuration
-func ParseConfigurationBackend(configuration *v1beta2.Configuration, k8sClient client.Client, credentials map[string]string) (Backend, error) {
+func ParseConfigurationBackend(configuration *v1beta2.Configuration, k8sClient client.Client, credentials map[string]string, controllerNSSpecified bool) (Backend, error) {
 	backend := configuration.Spec.Backend
 
 	var (
@@ -68,7 +68,7 @@ func ParseConfigurationBackend(configuration *v1beta2.Configuration, k8sClient c
 	switch {
 	case backend == nil || (backend.Inline == "" && backend.BackendType == ""):
 		// use the default k8s backend
-		return handleDefaultBackend(configuration, k8sClient)
+		return handleDefaultBackend(configuration, k8sClient, controllerNSSpecified)
 
 	case backend.Inline != "" && backend.BackendType != "":
 		return nil, errors.New("it's not allowed to set `spec.backend.inline` and `spec.backend.backendType` at the same time")
@@ -79,6 +79,8 @@ func ParseConfigurationBackend(configuration *v1beta2.Configuration, k8sClient c
 
 	case backend.BackendType != "":
 		// In this case, use the explicit custom backend
+		// we don't change backend secret suffix to UID of configuration here.
+		// If backend specified, it's user's responsibility to set the right secret suffix, to avoid conflict.
 		backendType, backendConf, err = handleExplicitBackend(backend)
 	}
 	if err != nil {
@@ -92,7 +94,7 @@ func ParseConfigurationBackend(configuration *v1beta2.Configuration, k8sClient c
 	return initFunc(k8sClient, backendConf, credentials)
 }
 
-func handleDefaultBackend(configuration *v1beta2.Configuration, k8sClient client.Client) (Backend, error) {
+func handleDefaultBackend(configuration *v1beta2.Configuration, k8sClient client.Client, controllerNSSpecified bool) (Backend, error) {
 	if configuration.Spec.Backend != nil {
 		if configuration.Spec.Backend.SecretSuffix == "" {
 			configuration.Spec.Backend.SecretSuffix = configuration.Name
@@ -104,7 +106,7 @@ func handleDefaultBackend(configuration *v1beta2.Configuration, k8sClient client
 			InClusterConfig: true,
 		}
 	}
-	return newDefaultK8SBackend(configuration.Spec.Backend.SecretSuffix, k8sClient, configuration.Namespace), nil
+	return newDefaultK8SBackend(configuration, k8sClient, controllerNSSpecified), nil
 }
 
 func handleInlineBackendHCL(hclCode string) (string, interface{}, error) {

controllers/configuration/backend/backend_test.go

@@ -13,8 +13,9 @@ import (
 
 func TestParseConfigurationBackend(t *testing.T) {
 	type args struct {
-		configuration *v1beta2.Configuration
-		credentials   map[string]string
+		configuration         *v1beta2.Configuration
+		credentials           map[string]string
+		controllerNSSpecified bool
 	}
 	type want struct {
 		backend Backend
@@ -299,17 +300,46 @@ terraform {
 				errMsg: "it's not allowed to set `spec.backend.inline` and `spec.backend.backendType` at the same time",
 			},
 		},
+		{
+			name: "backend is nil, specify controller namespace, generate backend with legacy secret suffix",
+			args: args{
+				configuration: &v1beta2.Configuration{
+					ObjectMeta: metav1.ObjectMeta{Name: "name", Namespace: "ns", UID: "xxxx-xxxx"},
+					Spec: v1beta2.ConfigurationSpec{
+						Backend: nil,
+					},
+				},
+				controllerNSSpecified: true,
+			},
+			want: want{
+				backend: &K8SBackend{
+					LegacySecretSuffix: "name",
+					SecretNS:           "ns",
+					SecretSuffix:       "xxxx-xxxx",
+					Client:             k8sClient,
+					HCLCode: `
+terraform {
+  backend "kubernetes" {
+    secret_suffix     = "xxxx-xxxx"
+    in_cluster_config = true
+    namespace         = "ns"
+  }
+}
+`,
+				},
+			},
+		},
 	}
 
 	for _, tc := range testcases {
 		t.Run(tc.name, func(t *testing.T) {
-			got, err := ParseConfigurationBackend(tc.args.configuration, k8sClient, tc.args.credentials)
+			got, err := ParseConfigurationBackend(tc.args.configuration, k8sClient, tc.args.credentials, tc.args.controllerNSSpecified)
 			if tc.want.errMsg != "" && !strings.Contains(err.Error(), tc.want.errMsg) {
 				t.Errorf("ValidConfigurationObject() error = %v, wantErr %v", err, tc.want.errMsg)
 				return
 			}
 			if !reflect.DeepEqual(tc.want.backend, got) {
-				t.Errorf("got %#v, want %#v", got, tc.want.backend)
+				t.Errorf("\ngot  %#v,\nwant %#v", got, tc.want.backend)
 			}
 		})
 	}

controllers/configuration/backend/kubernetes.go

@@ -21,12 +21,14 @@ import (
 	"fmt"
 	"os"
 
-	"github.com/oam-dev/terraform-controller/api/v1beta2"
-	"github.com/oam-dev/terraform-controller/controllers/util"
 	"github.com/pkg/errors"
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/klog/v2"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+
+	"github.com/oam-dev/terraform-controller/api/v1beta2"
+	"github.com/oam-dev/terraform-controller/controllers/util"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 const (
@@ -47,19 +49,31 @@ type K8SBackend struct {
 	SecretSuffix string
 	// SecretNS is the namespace of the Terraform backend secret
 	SecretNS string
+	// LegacySecretSuffix is the same as SecretSuffix, but only used when `--controller-namespace` is specified
+	LegacySecretSuffix string
 }
 
-func newDefaultK8SBackend(suffix string, client client.Client, namespace string) *K8SBackend {
+func newDefaultK8SBackend(configuration *v1beta2.Configuration, client client.Client, controllerNSSpecified bool) *K8SBackend {
 	ns := os.Getenv("TERRAFORM_BACKEND_NAMESPACE")
 	if ns == "" {
-		ns = namespace
+		ns = configuration.GetNamespace()
+	}
+
+	var (
+		suffix       = configuration.Spec.Backend.SecretSuffix
+		legacySuffix string
+	)
+	if controllerNSSpecified {
+		legacySuffix = suffix
+		suffix = string(configuration.GetUID())
 	}
 	hcl := renderK8SBackendHCL(suffix, ns)
 	return &K8SBackend{
-		Client:       client,
-		HCLCode:      hcl,
-		SecretSuffix: suffix,
-		SecretNS:     ns,
+		Client:             client,
+		HCLCode:            hcl,
+		SecretSuffix:       suffix,
+		SecretNS:           ns,
+		LegacySecretSuffix: legacySuffix,
 	}
 }
 
@@ -95,15 +109,23 @@ terraform {
 	return fmt.Sprintf(fmtStr, suffix, ns)
 }
 
-func (k K8SBackend) secretName() string {
+func (k *K8SBackend) secretName() string {
 	return fmt.Sprintf(TFBackendSecret, terraformWorkspace, k.SecretSuffix)
 }
 
+func (k *K8SBackend) legacySecretName() string {
+	return fmt.Sprintf(TFBackendSecret, terraformWorkspace, k.LegacySecretSuffix)
+}
+
 // GetTFStateJSON gets Terraform state json from the Terraform kubernetes backend
 func (k *K8SBackend) GetTFStateJSON(ctx context.Context) ([]byte, error) {
 	var s = v1.Secret{}
-	if err := k.Client.Get(ctx, client.ObjectKey{Name: k.secretName(), Namespace: k.SecretNS}, &s); err != nil {
-		return nil, errors.Wrap(err, "terraform state file backend secret is not generated")
+	// Try to get legacy secret first, if it doesn't exist, try to get new secret
+	err := k.Client.Get(ctx, client.ObjectKey{Name: k.legacySecretName(), Namespace: k.SecretNS}, &s)
+	if err != nil {
+		if err = k.Client.Get(ctx, client.ObjectKey{Name: k.secretName(), Namespace: k.SecretNS}, &s); err != nil {
+			return nil, errors.Wrap(err, "terraform state file backend secret is not generated")
+		}
 	}
 	tfStateData, ok := s.Data[TerraformStateNameInSecret]
 	if !ok {
@@ -119,8 +141,15 @@ func (k *K8SBackend) GetTFStateJSON(ctx context.Context) ([]byte, error) {
 
 // CleanUp will delete the Terraform kubernetes backend secret when deleting the configuration object
 func (k *K8SBackend) CleanUp(ctx context.Context) error {
-	klog.InfoS("Deleting the secret which stores Kubernetes backend", "Name", k.secretName())
+	klog.InfoS("Deleting the legacy secret which stores Kubernetes backend", "Name", k.legacySecretName())
 	var kubernetesBackendSecret v1.Secret
+	if err := k.Client.Get(ctx, client.ObjectKey{Name: k.legacySecretName(), Namespace: k.SecretNS}, &kubernetesBackendSecret); err == nil {
+		if err := k.Client.Delete(ctx, &kubernetesBackendSecret); err != nil {
+			return err
+		}
+	}
+
+	klog.InfoS("Deleting the secret which stores Kubernetes backend", "Name", k.secretName())
 	if err := k.Client.Get(ctx, client.ObjectKey{Name: k.secretName(), Namespace: k.SecretNS}, &kubernetesBackendSecret); err == nil {
 		if err := k.Client.Delete(ctx, &kubernetesBackendSecret); err != nil {
 			return err
@@ -131,8 +160,40 @@ func (k *K8SBackend) CleanUp(ctx context.Context) error {
 
 // HCL returns the backend hcl code string
 func (k *K8SBackend) HCL() string {
+	if k.LegacySecretSuffix != "" {
+		err := k.migrateLegacySecret()
+		if err != nil {
+			klog.ErrorS(err, "Failed to migrate legacy secret")
+		}
+	}
+
 	if k.HCLCode == "" {
 		k.HCLCode = renderK8SBackendHCL(k.SecretSuffix, k.SecretNS)
 	}
 	return k.HCLCode
 }
+
+// migrateLegacySecret will migrate the legacy secret to the new secret if the legacy secret exists
+// This is needed when the --controller-namespace is specified and restart the controller
+func (k *K8SBackend) migrateLegacySecret() error {
+	ctx := context.TODO()
+	s := v1.Secret{}
+	if err := k.Client.Get(ctx, client.ObjectKey{Name: k.legacySecretName(), Namespace: k.SecretNS}, &s); err == nil {
+		klog.InfoS("Migrating legacy secret to new secret", "LegacyName", k.legacySecretName(), "NewName", k.secretName(), "Namespace", k.SecretNS)
+		newSecret := v1.Secret{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      k.secretName(),
+				Namespace: k.SecretNS,
+			},
+			Data: s.Data,
+		}
+		err = k.Client.Create(ctx, &newSecret)
+		if err != nil {
+			return errors.Wrapf(err, "Fail to create new secret, Name: %s, Namespace: %s", k.secretName(), k.SecretNS)
+		} else if err = k.Client.Delete(ctx, &s); err != nil {
+			// Only delete the legacy secret if the new secret is successfully created
+			return errors.Wrapf(err, "Fail to delete legacy secret, Name: %s, Namespace: %s", k.legacySecretName(), k.SecretNS)
+		}
+	}
+	return nil
+}