Fix deep copy and don't modify original state (#919)

pkg/controller/instance/plan_execution_engine.go

@@ -9,8 +9,6 @@ import (
 
 	"k8s.io/apimachinery/pkg/types"
 
-	"errors"
-
 	"github.com/kudobuilder/kudo/pkg/apis/kudo/v1alpha1"
 	kudoengine "github.com/kudobuilder/kudo/pkg/engine"
 	"github.com/kudobuilder/kudo/pkg/util/health"
@@ -61,18 +59,12 @@ func executePlan(plan *activePlan, metadata *engineMetadata, c client.Client, en
 		return plan.PlanStatus, nil
 	}
 
-	// we don't want to modify the original state, and State does not contain any pointer, so shallow copy is enough
-	newState := &(*plan.PlanStatus)
+	// we don't want to modify the original state -> need to do deepcopy
+	newState := plan.PlanStatus.DeepCopy()
 
 	// render kubernetes resources needed to execute this plan
-	planResources, err := prepareKubeResources(plan, metadata, enhancer)
+	planResources, err := prepareKubeResources(plan, newState, metadata, enhancer)
 	if err != nil {
-		var exErr *executionError
-		if errors.As(err, &exErr) {
-			newState.Status = v1alpha1.ExecutionFatalError
-		} else {
-			newState.Status = v1alpha1.ErrorStatus
-		}
 		return newState, err
 	}
 
@@ -231,7 +223,8 @@ func patchExistingObject(newResource runtime.Object, existingResource runtime.Ob
 
 // prepareKubeResources takes all resources in all tasks for a plan and renders them with the right parameters
 // it also takes care of applying KUDO specific conventions to the resources like commond labels
-func prepareKubeResources(plan *activePlan, meta *engineMetadata, renderer kubernetesObjectEnhancer) (*planResources, error) {
+// newState gets modified with possible state changes as a result of this method
+func prepareKubeResources(plan *activePlan, newState *v1alpha1.PlanStatus, meta *engineMetadata, renderer kubernetesObjectEnhancer) (*planResources, error) {
 	configs := make(map[string]interface{})
 	configs["OperatorName"] = meta.operatorName
 	configs["Name"] = meta.instanceName
@@ -243,7 +236,7 @@ func prepareKubeResources(plan *activePlan, meta *engineMetadata, renderer kuber
 	}
 
 	for _, phase := range plan.spec.Phases {
-		phaseState, _ := getPhaseFromStatus(phase.Name, plan.PlanStatus)
+		phaseState, _ := getPhaseFromStatus(phase.Name, newState)
 		perStepResources := make(map[string][]runtime.Object)
 		result.PhaseResources[phase.Name] = phaseResources{
 			StepResources: perStepResources,
@@ -265,6 +258,7 @@ func prepareKubeResources(plan *activePlan, meta *engineMetadata, renderer kuber
 						if resource, ok := plan.templates[res]; ok {
 							templatedYaml, err := engine.Render(resource, configs)
 							if err != nil {
+								newState.Status = v1alpha1.ExecutionFatalError
 								phaseState.Status = v1alpha1.ExecutionFatalError
 								stepState.Status = v1alpha1.ExecutionFatalError
 
@@ -274,6 +268,7 @@ func prepareKubeResources(plan *activePlan, meta *engineMetadata, renderer kuber
 							}
 							resourcesAsString[res] = templatedYaml
 						} else {
+							newState.Status = v1alpha1.ExecutionFatalError
 							phaseState.Status = v1alpha1.ExecutionFatalError
 							stepState.Status = v1alpha1.ExecutionFatalError
 
@@ -291,6 +286,7 @@ func prepareKubeResources(plan *activePlan, meta *engineMetadata, renderer kuber
 					})
 
 					if err != nil {
+						newState.Status = v1alpha1.ErrorStatus
 						phaseState.Status = v1alpha1.ErrorStatus
 						stepState.Status = v1alpha1.ErrorStatus
 
@@ -299,6 +295,7 @@ func prepareKubeResources(plan *activePlan, meta *engineMetadata, renderer kuber
 					}
 					resources = append(resources, resourcesWithConventions...)
 				} else {
+					newState.Status = v1alpha1.ErrorStatus
 					phaseState.Status = v1alpha1.ErrorStatus
 					stepState.Status = v1alpha1.ErrorStatus
 

pkg/controller/instance/plan_execution_engine_test.go

@@ -118,6 +118,37 @@ func TestExecutePlan(t *testing.T) {
 	}
 }
 
+func TestExecutePlanWithEnhancerError(t *testing.T) {
+	testClient := fake.NewFakeClientWithScheme(scheme.Scheme)
+	newStatus, err := executePlan(&activePlan{
+		name: "test",
+		PlanStatus: &v1alpha1.PlanStatus{
+			Status: v1alpha1.ExecutionPending,
+			Name:   "test",
+			Phases: []v1alpha1.PhaseStatus{{Name: "phase", Status: v1alpha1.ExecutionPending, Steps: []v1alpha1.StepStatus{{Status: v1alpha1.ExecutionPending, Name: "step"}}}},
+		},
+		spec: &v1alpha1.Plan{
+			Strategy: "serial",
+			Phases: []v1alpha1.Phase{
+				{Name: "phase", Strategy: "serial", Steps: []v1alpha1.Step{{Name: "step", Tasks: []string{"task"}}}},
+			},
+		},
+		tasks:     map[string]v1alpha1.TaskSpec{"task": {Resources: []string{"job"}}},
+		templates: map[string]string{"job": getResourceAsString(getJob("job1", "default"))},
+	}, &engineMetadata{
+		instanceName:        "Instance",
+		instanceNamespace:   "default",
+		operatorVersion:     "ov-1.0",
+		operatorName:        "operator",
+		resourcesOwner:      getJob("pod2", "default"),
+		operatorVersionName: "ovname",
+	}, testClient, &errKubernetesObjectEnhancer{})
+
+	if err == nil || newStatus == nil || newStatus.Status != v1alpha1.ErrorStatus || newStatus.Phases[0].Status != v1alpha1.ErrorStatus || newStatus.Phases[0].Steps[0].Status != v1alpha1.ErrorStatus {
+		t.Fatalf("Expecting to get an error with fatal error in status for error in using enhancer, got %w, %v", err, newStatus)
+	}
+}
+
 func getJob(name string, namespace string) *batchv1.Job {
 	job := &batchv1.Job{
 		TypeMeta: metav1.TypeMeta{
@@ -166,3 +197,9 @@ func (k *testKubernetesObjectEnhancer) applyConventionsToTemplates(templates map
 	}
 	return result, nil
 }
+
+type errKubernetesObjectEnhancer struct{}
+
+func (k *errKubernetesObjectEnhancer) applyConventionsToTemplates(templates map[string]string, metadata ExecutionMetadata) ([]runtime.Object, error) {
+	return nil, errors.New("Always error")
+}