Raise error in Plan handler if drain options conflict

Signed-off-by: Brad Davidson <[email protected]>

Author: brandond

e2e/suite/job_generate_test.go

@@ -4,6 +4,7 @@ import (
 	"time"
 
 	batchv1 "k8s.io/api/batch/v1"
+	"k8s.io/utils/pointer"
 
 	"github.com/rancher/system-upgrade-controller/e2e/framework"
 	upgradeapiv1 "github.com/rancher/system-upgrade-controller/pkg/apis/upgrade.cattle.io/v1"
@@ -59,4 +60,61 @@ var _ = Describe("Job Generation", func() {
 			Expect(jobs[0].Status.Failed).To(BeNumerically("==", 0))
 		})
 	})
+
+	When("fails because of conflicting drain options", func() {
+		var (
+			err  error
+			plan *upgradeapiv1.Plan
+			jobs []batchv1.Job
+		)
+		BeforeEach(func() {
+			plan = e2e.NewPlan("fail-drain-options-", "library/alpine:3.11", []string{"sh", "-c"}, "exit 0")
+			plan.Spec.Version = "latest"
+			plan.Spec.Concurrency = 1
+			plan.Spec.NodeSelector = &metav1.LabelSelector{
+				MatchExpressions: []metav1.LabelSelectorRequirement{{
+					Key:      "node-role.kubernetes.io/master",
+					Operator: metav1.LabelSelectorOpDoesNotExist,
+				}},
+			}
+			plan.Spec.Drain = &upgradeapiv1.DrainSpec{
+				DisableEviction:    true,
+				DeleteLocalData:    pointer.Bool(true),
+				DeleteEmptydirData: pointer.Bool(true),
+				PodSelector: &metav1.LabelSelector{
+					MatchExpressions: []metav1.LabelSelectorRequirement{{
+						Key:      "app",
+						Values:   []string{"csi-attacher", "csi-provisioner"},
+						Operator: metav1.LabelSelectorOpNotIn,
+					}},
+				},
+			}
+			plan, err = e2e.CreatePlan(plan)
+			Expect(err).ToNot(HaveOccurred())
+
+			plan, err = e2e.WaitForPlanCondition(plan.Name, upgradeapiv1.PlanSpecValidated, 30*time.Second)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(upgradeapiv1.PlanSpecValidated.IsTrue(plan)).To(BeFalse())
+			Expect(upgradeapiv1.PlanSpecValidated.GetMessage(plan)).To(ContainSubstring("cannot specify both deleteEmptydirData and deleteLocalData"))
+
+			plan.Spec.Drain.DeleteLocalData = nil
+			plan, err = e2e.UpdatePlan(plan)
+			Expect(err).ToNot(HaveOccurred())
+
+			plan, err = e2e.WaitForPlanCondition(plan.Name, upgradeapiv1.PlanSpecValidated, 30*time.Second)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(upgradeapiv1.PlanSpecValidated.IsTrue(plan)).To(BeTrue())
+
+			jobs, err = e2e.WaitForPlanJobs(plan, 1, 120*time.Second)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(jobs).To(HaveLen(1))
+		})
+		It("should apply successfully after edit", func() {
+			Expect(jobs).To(HaveLen(1))
+			Expect(jobs[0].Status.Succeeded).To(BeNumerically("==", 1))
+			Expect(jobs[0].Status.Failed).To(BeNumerically("==", 0))
+			Expect(jobs[0].Spec.Template.Spec.InitContainers).To(HaveLen(1))
+			Expect(jobs[0].Spec.Template.Spec.InitContainers[0].Args).To(ContainSubstring("csi-attacher"))
+		})
+	})
 })

pkg/apis/upgrade.cattle.io/v1/types.go

@@ -15,6 +15,8 @@ import (
 var (
 	// PlanLatestResolved indicates that the latest version as per the spec has been determined.
 	PlanLatestResolved = condition.Cond("LatestResolved")
+	// PlanSpecValidated indicates that the plan spec has been validated.
+	PlanSpecValidated = condition.Cond("Validated")
 )
 
 // +genclient

pkg/upgrade/handle_upgrade.go

@@ -28,6 +28,16 @@ func (ctl *Controller) handlePlans(ctx context.Context) error {
 				return status, nil
 			}
 			logrus.Debugf("PLAN STATUS HANDLER: plan=%s/%s@%s, status=%+v", obj.Namespace, obj.Name, obj.ResourceVersion, status)
+
+			validated := upgradeapiv1.PlanSpecValidated
+			validated.CreateUnknownIfNotExists(obj)
+			if err := upgradeplan.Validate(obj); err != nil {
+				validated.SetError(obj, "Error", err)
+				return upgradeplan.DigestStatus(obj, secretsCache)
+			}
+			validated.False(obj)
+			validated.SetError(obj, "PlanIsValid", nil)
+
 			resolved := upgradeapiv1.PlanLatestResolved
 			resolved.CreateUnknownIfNotExists(obj)
 			if obj.Spec.Version == "" && obj.Spec.Channel == "" {
@@ -66,6 +76,9 @@ func (ctl *Controller) handlePlans(ctx context.Context) error {
 				return objects, status, nil
 			}
 			logrus.Debugf("PLAN GENERATING HANDLER: plan=%s/%s@%s, status=%+v", obj.Namespace, obj.Name, obj.ResourceVersion, status)
+			if !upgradeapiv1.PlanSpecValidated.IsTrue(obj) {
+				return objects, status, nil
+			}
 			if !upgradeapiv1.PlanLatestResolved.IsTrue(obj) {
 				return objects, status, nil
 			}

pkg/upgrade/plan/plan.go

@@ -31,6 +31,8 @@ const (
 )
 
 var (
+	ErrDrainDeleteConflict = fmt.Errorf("spec.drain cannot specify both deleteEmptydirData and deleteLocalData")
+
 	PollingInterval = func(defaultValue time.Duration) time.Duration {
 		if str, ok := os.LookupEnv("SYSTEM_UPGRADE_PLAN_POLLING_INTERVAL"); ok {
 			if d, err := time.ParseDuration(str); err != nil {
@@ -228,3 +230,13 @@ func sha256sum(s ...string) string {
 	}
 	return fmt.Sprintf("%x", h.Sum(nil))
 }
+
+// Validate performs validation of the plan spec, raising errors for any conflicting or invalid settings.
+func Validate(plan *upgradeapiv1.Plan) error {
+	if drainSpec := plan.Spec.Drain; drainSpec != nil {
+		if drainSpec.DeleteEmptydirData != nil && drainSpec.DeleteLocalData != nil {
+			return ErrDrainDeleteConflict
+		}
+	}
+	return nil
+}