diff --git a/internal/server/polling/poll_test.go b/internal/server/polling/poll_test.go index 3a5e2e98..d2865ee6 100644 --- a/internal/server/polling/poll_test.go +++ b/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 diff --git a/internal/server/polling/server.go b/internal/server/polling/server.go index b877e35a..9aee0507 100644 --- a/internal/server/polling/server.go +++ b/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) diff --git a/internal/server/polling/terraform.go b/internal/server/polling/terraform.go index d20fe29c..dd803c64 100644 --- a/internal/server/polling/terraform.go +++ b/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 }