Error out in instance controller when a parameter has an undefined pl…

…an trigger (#1336)

* Verify plans are referenced and referenced plans are defined
* error out in instance controller when a parameter has an undefined plan trigger

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
Signed-off-by: Thomas Runyon <runyontr@gmail.com>

pkg/apis/kudo/v1beta1/instance_types.go

@@ -174,6 +174,15 @@ const (
 	CleanupPlanName = "cleanup"
 )
 
+var (
+	ReservedPlanNames = []string{
+		DeployPlanName,
+		UpgradePlanName,
+		UpdatePlanName,
+		CleanupPlanName,
+	}
+)
+
 // IsTerminal returns true if the status is terminal (either complete, or in a nonrecoverable error)
 func (s ExecutionStatus) IsTerminal() bool {
 	return s == ExecutionComplete || s == ExecutionFatalError

pkg/controller/instance/instance_controller.go

@@ -594,24 +594,31 @@ func getPlanToBeExecuted(i *v1beta1.Instance, ov *v1beta1.OperatorVersion) (*str
 		log.Printf("Instance: instance %s/%s has updated parameters from %v to %v", i.Namespace, i.Name, instanceSnapshot.Parameters, i.Spec.Parameters)
 		paramDiff := kudov1beta1.ParameterDiff(instanceSnapshot.Parameters, i.Spec.Parameters)
 		paramDefinitions := kudov1beta1.GetParamDefinitions(paramDiff, ov)
-		plan := planNameFromParameters(paramDefinitions, ov)
-		if plan == nil {
-			return nil, &v1beta1.InstanceError{Err: fmt.Errorf("supposed to execute plan because instance %s/%s was updated but none of the deploy, update plans found in linked operatorVersion", i.Namespace, i.Name), EventName: kudo.String("PlanNotFound")}
+		plan, err := planNameFromParameters(paramDefinitions, ov)
+		if err != nil {
+			return nil, &v1beta1.InstanceError{Err: fmt.Errorf("supposed to execute plan because instance %s/%s was updated but no valid plan found: %v", i.Namespace, i.Name, err), EventName: kudo.String("PlanNotFound")}
 		}
 		return plan, nil
 	}
 	return nil, nil
 }
 
 // planNameFromParameters determines what plan to run based on params that changed and the related trigger plans
-func planNameFromParameters(params []v1beta1.Parameter, ov *v1beta1.OperatorVersion) *string {
+func planNameFromParameters(params []v1beta1.Parameter, ov *v1beta1.OperatorVersion) (*string, error) {
+	// TODO: if the params have different trigger plans, we always select first here which might not be ideal
 	for _, p := range params {
-		// TODO: if the params have different trigger plans, we always select first here which might not be ideal
-		if p.Trigger != "" && kudov1beta1.SelectPlan([]string{p.Trigger}, ov) != nil {
-			return kudo.String(p.Trigger)
+		if p.Trigger != "" {
+			if kudov1beta1.SelectPlan([]string{p.Trigger}, ov) != nil {
+				return kudo.String(p.Trigger), nil
+			}
+			return nil, fmt.Errorf("param %s defined trigger plan %s, but plan not defined in operatorversion", p.Name, p.Trigger)
 		}
 	}
-	return kudov1beta1.SelectPlan([]string{v1beta1.UpdatePlanName, v1beta1.DeployPlanName}, ov)
+	plan := kudov1beta1.SelectPlan([]string{v1beta1.UpdatePlanName, v1beta1.DeployPlanName}, ov)
+	if plan == nil {
+		return nil, fmt.Errorf("no default plan defined in operatorversion")
+	}
+	return plan, nil
 }
 
 // SaveSnapshot stores the current spec of Instance into the snapshot annotation

pkg/kudoctl/cmd/testdata/invalid-params.golden

@@ -1,6 +1,8 @@
 Warnings                                
+plan "validation" defined but not used  
 parameter "Cpus" defined but not used.  
 parameter "comma," defined but not used.
-Errors                                           
-parameter "Cpus" has a duplicate                 
-parameter "comma," contains invalid character ','
+Errors                                                            
+parameter "Cpus" has a duplicate                                  
+parameter "comma," contains invalid character ','                 
+plan "not-existing-plan" used in parameter "comma," is not defined

pkg/kudoctl/cmd/testdata/invalidzk/params.yaml

@@ -12,3 +12,4 @@ parameters:
   - name: comma,
     description: Amount of cpu to provide to Zookeeper pods
     default: "0.25"
+    trigger: not-existing-plan

pkg/kudoctl/cmd/verify/verify.go

@@ -6,6 +6,7 @@ import (
 
 	"github.com/kudobuilder/kudo/pkg/kudoctl/packages"
 	"github.com/kudobuilder/kudo/pkg/kudoctl/packages/verifier"
+	"github.com/kudobuilder/kudo/pkg/kudoctl/packages/verifier/plan"
 	"github.com/kudobuilder/kudo/pkg/kudoctl/packages/verifier/task"
 	"github.com/kudobuilder/kudo/pkg/kudoctl/packages/verifier/template"
 	"github.com/kudobuilder/kudo/pkg/version"
@@ -16,6 +17,7 @@ var verifiers = []verifier.PackageVerifier{
 	InvalidCharVerifier{";,"},
 	K8sVersionVerifier{},
 	task.ReferenceVerifier{},
+	plan.ReferenceVerifier{},
 	template.ParametersVerifier{},
 	template.ReferenceVerifier{},
 	template.RenderVerifier{},

pkg/kudoctl/packages/verifier/plan/verify_references.go

@@ -0,0 +1,60 @@
+package plan
+
+import (
+	"fmt"
+
+	"github.com/kudobuilder/kudo/pkg/apis/kudo/v1beta1"
+	"github.com/kudobuilder/kudo/pkg/kudoctl/packages"
+	"github.com/kudobuilder/kudo/pkg/kudoctl/packages/verifier"
+)
+
+// ReferenceVerifier verifies plans producing errors for plans referenced in param triggers that do not exist and warnings for plans which are not used in a param
+type ReferenceVerifier struct{}
+
+func (ReferenceVerifier) Verify(pf *packages.Files) verifier.Result {
+	res := verifier.NewResult()
+	res.Merge(plansNotDefined(pf))
+	res.Merge(plansDefinedNotUsed(pf))
+
+	return res
+}
+
+func plansDefinedNotUsed(pf *packages.Files) verifier.Result {
+	res := verifier.NewResult()
+	usedPlans := make(map[string]bool)
+
+	// Mark reserved plan names as used
+	for _, plan := range v1beta1.ReservedPlanNames {
+		usedPlans[plan] = true
+	}
+
+	// Mark plans in param triggers as used
+	for _, param := range pf.Params.Parameters {
+		if param.Trigger != "" {
+			usedPlans[param.Trigger] = true
+		}
+	}
+
+	for name := range pf.Operator.Plans {
+		if _, ok := usedPlans[name]; !ok {
+			res.AddWarnings(fmt.Sprintf("plan %q defined but not used", name))
+		}
+	}
+
+	return res
+}
+
+func plansNotDefined(pf *packages.Files) verifier.Result {
+	res := verifier.NewResult()
+	plans := pf.Operator.Plans
+
+	for _, param := range pf.Params.Parameters {
+		if param.Trigger != "" {
+			if _, ok := plans[param.Trigger]; !ok {
+				res.AddErrors(fmt.Sprintf("plan %q used in parameter %q is not defined", param.Trigger, param.Name))
+			}
+		}
+	}
+
+	return res
+}

pkg/kudoctl/packages/verifier/plan/verify_references_test.go

@@ -0,0 +1,76 @@
+package plan
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	"github.com/kudobuilder/kudo/pkg/apis/kudo/v1beta1"
+	"github.com/kudobuilder/kudo/pkg/kudoctl/packages"
+)
+
+func TestPlanReferenceVerifier(t *testing.T) {
+
+	resources := []string{"sally.yaml"}
+	tasks := []v1beta1.Task{{
+		Name: "thingOne",
+		Kind: "Apply",
+		Spec: v1beta1.TaskSpec{
+			ResourceTaskSpec: v1beta1.ResourceTaskSpec{Resources: resources},
+		},
+	}}
+
+	steps := []v1beta1.Step{{
+		Name:  "cat-in-hat",
+		Tasks: []string{"thingOne"},
+	}}
+
+	phases := []v1beta1.Phase{{
+		Name:     "parents leave",
+		Strategy: "serial",
+		Steps:    steps,
+	}}
+
+	plans := make(map[string]v1beta1.Plan)
+	plans["deploy"] = v1beta1.Plan{
+		Strategy: "serial",
+		Phases:   phases,
+	}
+	plans["used-plan"] = v1beta1.Plan{
+		Strategy: "serial",
+		Phases:   phases,
+	}
+	plans["unused-plan"] = v1beta1.Plan{
+		Strategy: "serial",
+		Phases:   phases,
+	}
+
+	operator := packages.OperatorFile{
+		Tasks: tasks,
+		Plans: plans,
+	}
+	params := packages.ParamsFile{
+		Parameters: packages.Parameter{
+			v1beta1.Parameter{
+				Name:    "PARAM1",
+				Trigger: "used-plan",
+			},
+			v1beta1.Parameter{
+				Name:    "PARAM2",
+				Trigger: "not-existing-plan",
+			},
+		},
+	}
+
+	pf := packages.Files{
+		Operator: &operator,
+		Params:   &params,
+	}
+	verifier := ReferenceVerifier{}
+	res := verifier.Verify(&pf)
+
+	assert.Equal(t, 1, len(res.Warnings))
+	assert.Equal(t, `plan "unused-plan" defined but not used`, res.Warnings[0])
+	assert.Equal(t, 1, len(res.Errors))
+	assert.Equal(t, `plan "not-existing-plan" used in parameter "PARAM2" is not defined`, res.Errors[0])
+}