Merge pull request #1001 from weaveworks/delete-tf-before-source

Deleting the Terraform object before deleting its source

internal/server/polling/poll_test.go

@@ -3,6 +3,8 @@ package polling
 import (
 	"context"
 	"fmt"
+	bpconfig "github.com/weaveworks/tf-controller/internal/config"
+	"k8s.io/apimachinery/pkg/labels"
 	"testing"
 
 	"github.com/weaveworks/tf-controller/internal/git/provider/providerfakes"
@@ -197,10 +199,10 @@ func Test_poll_reconcile_objects(t *testing.T) {
 		expectToEqual(g, item.Spec.StoreReadablePlan, "human")
 		expectToEqual(g, item.Spec.ApprovePlan, "")
 		expectToEqual(g, item.Spec.Force, false)
-		expectToEqual(g, item.Spec.WriteOutputsToSecret.Name, fmt.Sprintf("test-secret-%d", idx+1))
-		expectToEqual(g, item.Labels["infra.weave.works/branch-planner"], "true")
+		g.Expect(item.Spec.WriteOutputsToSecret).To(gomega.BeNil()) // we don't need to use the output Secret of the plan
+		expectToEqual(g, item.Labels[bpconfig.LabelKey], bpconfig.LabelValue)
 		expectToEqual(g, item.Labels["test-label"], "abc")
-		expectToEqual(g, item.Labels["infra.weave.works/pr-id"], fmt.Sprint(idx+1))
+		expectToEqual(g, item.Labels[bpconfig.LabelPRIDKey], fmt.Sprint(idx+1))
 	}
 
 	// Check that the Source objects are created with all expected fields.
@@ -217,9 +219,9 @@ func Test_poll_reconcile_objects(t *testing.T) {
 	for idx, item := range srcList.Items[1:] {
 		expectToEqual(g, item.Name, fmt.Sprintf("%s-%d", source.Name, idx+1))
 		expectToEqual(g, item.Spec.Reference.Branch, fmt.Sprintf("test-branch-%d", idx+1))
-		expectToEqual(g, item.Labels["infra.weave.works/branch-planner"], "true")
+		expectToEqual(g, item.Labels[bpconfig.LabelKey], bpconfig.LabelValue)
 		expectToEqual(g, item.Labels["test-label"], "123")
-		expectToEqual(g, item.Labels["infra.weave.works/pr-id"], fmt.Sprint(idx+1))
+		expectToEqual(g, item.Labels[bpconfig.LabelPRIDKey], fmt.Sprint(idx+1))
 	}
 
 	// Check that branch Terraform objects are updated
@@ -234,16 +236,13 @@ func Test_poll_reconcile_objects(t *testing.T) {
 	tfList.Items = nil
 
 	expectToSucceed(g, k8sClient.List(context.TODO(), &tfList, &client.ListOptions{
-		Namespace: ns.Name,
+		Namespace:     ns.Name,
+		LabelSelector: labels.Set{bpconfig.LabelKey: bpconfig.LabelValue}.AsSelector(),
 	}))
 
-	for idx, item := range tfList.Items {
-		expectedSecretName := fmt.Sprintf("%s-%d", secretName, idx)
-		if idx == 0 {
-			expectedSecretName = secretName
-		}
+	for _, item := range tfList.Items {
 		expectToEqual(g, item.Labels["test-label"], "xyz")
-		expectToEqual(g, item.Spec.WriteOutputsToSecret.Name, expectedSecretName)
+		g.Expect(item.Spec.WriteOutputsToSecret).To(gomega.BeNil())
 	}
 
 	// Check that corresponding Terraform objects and Sources are deleted

internal/server/polling/server.go

@@ -244,7 +244,7 @@ func (s *Server) reconcile(ctx context.Context, original *infrav1.Terraform, sou
 		// If an error occurs, log it.
 		if !exist || pr.Closed {
 			log.Info("the PR either does not exist or has been closed, deleting corresponding Terraform object...", "PR ID", prId)
-			if err = s.deleteTerraform(ctx, tfPlannerObject); err != nil {
+			if err = s.deleteTerraformAndSource(ctx, tfPlannerObject); err != nil {
 				log.Error(err, "failed to delete Terraform object", "name", tfPlannerObject.Name, "namespace", tfPlannerObject.Namespace, "PR ID", prId)
 			} else {
 				log.Info("successfully deleted Terraform object", "name", tfPlannerObject.Name, "namespace", tfPlannerObject.Namespace, "PR ID", prId)

internal/server/polling/terraform.go

@@ -9,8 +9,10 @@ import (
 	sourcev1 "github.com/fluxcd/source-controller/api/v1"
 	bpconfig "github.com/weaveworks/tf-controller/internal/config"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	k8sLabels "k8s.io/apimachinery/pkg/labels"
+	"k8s.io/apimachinery/pkg/util/wait"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
@@ -98,13 +100,15 @@ func (s *Server) reconcileTerraform(ctx context.Context, originalTF *infrav1.Ter
 
 		spec.SourceRef.Name = source.Name
 		spec.SourceRef.Namespace = source.Namespace
+
+		// DestroyResourcesOnDeletion must be false, otherwise plan deletion will destroy resources
+		spec.DestroyResourcesOnDeletion = false
 		spec.PlanOnly = true
 		spec.StoreReadablePlan = "human"
 
-		// Relocate the output secret, so it's not shared between branches
-		if spec.WriteOutputsToSecret != nil && originalTF.Spec.WriteOutputsToSecret != nil {
-			spec.WriteOutputsToSecret.Name = s.createObjectName(originalTF.Spec.WriteOutputsToSecret.Name, prID)
-		}
+		// We don't need to examine or use the outputs of the plan
+		spec.WriteOutputsToSecret = nil
+
 		spec.ApprovePlan = ""
 		spec.Force = false
 
@@ -170,46 +174,59 @@ func (s *Server) createObjectName(name string, prID string) string {
 }
 
 func (s *Server) createLabels(labels map[string]string, originalName string, branch string, prID string) map[string]string {
-	if labels == nil {
-		labels = make(map[string]string)
+	resultLabels := make(map[string]string)
+	for k, v := range labels {
+		resultLabels[k] = v
 	}
 
-	labels[bpconfig.LabelKey] = bpconfig.LabelValue
-	labels[bpconfig.LabelPrimaryResourceKey] = originalName
-	labels[bpconfig.LabelPRIDKey] = prID
-
-	return labels
+	resultLabels[bpconfig.LabelKey] = bpconfig.LabelValue
+	resultLabels[bpconfig.LabelPrimaryResourceKey] = originalName
+	resultLabels[bpconfig.LabelPRIDKey] = prID
+	return resultLabels
 }
 
-func (s *Server) deleteTerraform(ctx context.Context, tf *infrav1.Terraform) error {
-	msg := fmt.Sprintf("Terraform %s in the namespace %s", tf.Name, tf.Namespace)
+func (s *Server) deleteTerraformAndSource(ctx context.Context, tf *infrav1.Terraform) error {
+	const (
+		pollInterval = 5 * time.Second // Interval to check the deletion status
+		pollTimeout  = 2 * time.Minute // Total time before timing out
+	)
 
-	if err := s.deleteSource(ctx, tf); err != nil {
-		s.log.Error(err, fmt.Sprintf("unable to delete Source for %s", msg))
+	tfMsg := fmt.Sprintf("Terraform %s in the namespace %s", tf.Name, tf.Namespace)
+
+	// Get source, but not yet delete it
+	source, err := s.getSource(ctx, tf)
+	if err != nil {
+		return fmt.Errorf("unable to get Source for %s: %w", tfMsg, err)
 	}
 
+	// Delete Terraform object
 	if err := s.clusterClient.Delete(ctx, tf); err != nil {
-		return fmt.Errorf("unable to delete %s: %w", msg, err)
+		return fmt.Errorf("unable to delete %s: %w", tfMsg, err)
 	}
 
-	s.log.Info(fmt.Sprintf("deleted %s", msg))
-
-	return nil
-}
+	// We have to wait for the Terraform object to be deleted before deleting the source
+	err = wait.PollImmediate(pollInterval, pollTimeout, func() (bool, error) {
+		if err := s.clusterClient.Get(ctx, client.ObjectKey{Name: tf.Name, Namespace: tf.Namespace}, tf); err != nil {
+			if errors.IsNotFound(err) {
+				return true, nil // Terraform object is deleted
+			}
+			return false, err // An error occurred
+		}
+		return false, nil // Terraform object still exists
+	})
 
-func (s *Server) deleteSource(ctx context.Context, tf *infrav1.Terraform) error {
-	source, err := s.getSource(ctx, tf)
 	if err != nil {
-		return fmt.Errorf("unable to get Source for Terraform %s in the namespace %s: %w", tf.Name, tf.Namespace, err)
+		return fmt.Errorf("error waiting for %s to be deleted: %w", tfMsg, err)
 	}
 
-	msg := fmt.Sprintf("Source %s in the namespace %s", source.Name, source.Namespace)
+	s.log.Info(fmt.Sprintf("deleted %s", tfMsg))
 
+	sourceMsg := fmt.Sprintf("Source %s in the namespace %s", source.Name, source.Namespace)
 	if err := s.clusterClient.Delete(ctx, source); err != nil {
-		return fmt.Errorf("unable to delete %s: %w", msg, err)
+		return fmt.Errorf("unable to delete %s: %w", sourceMsg, err)
 	}
 
-	s.log.Info(fmt.Sprintf("deleted %s", msg))
+	s.log.Info(fmt.Sprintf("deleted %s", sourceMsg))
 
 	return nil
 }