Add integration tests for the instance admission controller (#1442)

Summary:
the tests discovered a bug in the instance admission controller (IAC) where IAC was trying to reset `PlanExecution.PlanName` when a plan becomes terminal. However, it wasn't doing this because:

1. it wasn't notified when `Instance.Status` was being updated (_Status_ is a sub-resource) and
2. even if it would, `Instance` resource **can not be changed** upon `Status` subresource updates (any resource is only allowed to modify itself from the webhook)

To fix this problem we would either need to merge `Status` back into the `Instance` resource or duplicate part of the `Status` information in the `Instance.Spec`. After long discussions, we decided to introduce a new `PlanExecution.Status` field which is populated by the instance controller and duplicates the corresponding `Status.PlanStatus` field.

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

config/crds/kudo.dev_instances.yaml

@@ -94,6 +94,9 @@ spec:
               properties:
                 planName:
                   type: string
+                status:
+                  description: ExecutionStatus captures the state of the rollout.
+                  type: string
                 uid:
                   description: UID is a type that holds unique ID values, including
                     UUIDs.  Because we don't ONLY use UUIDs, this is an alias to string.  Being

go.mod

@@ -9,6 +9,9 @@ require (
 	github.com/gosuri/uitable v0.0.4
 	github.com/kudobuilder/kuttl v0.1.0
 	github.com/manifoldco/promptui v0.6.0
+	github.com/onsi/ginkgo v1.11.0
+	github.com/onsi/gomega v1.8.1
+	github.com/prometheus/common v0.4.1
 	github.com/spf13/afero v1.2.2
 	github.com/spf13/cobra v0.0.5
 	github.com/spf13/pflag v1.0.5

pkg/apis/kudo/v1beta1/instance_types.go

@@ -47,6 +47,7 @@ type InstanceSpec struct {
 type PlanExecution struct {
 	PlanName string                `json:"planName,omitempty"`
 	UID      apimachinerytypes.UID `json:"uid,omitempty"`
+	Status   ExecutionStatus       `json:"status,omitempty"`
 
 	// Future PE options like Force: bool. Not needed for now
 }

pkg/apis/kudo/v1beta1/instance_types_helpers.go

@@ -66,6 +66,7 @@ func (i *Instance) UpdateInstanceStatus(planStatus *PlanStatus, updatedTimestamp
 		if v.Name == planStatus.Name {
 			planStatus.LastUpdatedTimestamp = updatedTimestamp
 			i.Status.PlanStatus[k] = *planStatus
+			i.Spec.PlanExecution.Status = planStatus.Status
 			i.Status.AggregatedStatus.Status = planStatus.Status
 		}
 	}

pkg/apis/kudo/v1beta1/instance_types_helpers_test.go

@@ -130,6 +130,8 @@ func TestInstance_ResetPlanStatus(t *testing.T) {
 	// we test that UID has changed. afterwards, we replace it with the old one and compare new
 	// plan status with the desired state
 	assert.NotEqual(t, instance.Status.PlanStatus["deploy"].UID, oldUID)
+	assert.Equal(t, instance.Spec.PlanExecution.Status, ExecutionPending)
+
 	oldPlanStatus := instance.Status.PlanStatus["deploy"]
 	statusCopy := oldPlanStatus.DeepCopy()
 	statusCopy.UID = testUUID

pkg/controller/instance/instance_controller.go

@@ -604,6 +604,7 @@ func fetchNewExecutionPlan(i *v1beta1.Instance, ov *v1beta1.OperatorVersion) (*s
 
 		i.Spec.PlanExecution.PlanName = v1beta1.CleanupPlanName
 		i.Spec.PlanExecution.UID = uuid.NewUUID()
+		i.Spec.PlanExecution.Status = v1beta1.ExecutionNeverRun
 	}
 
 	newPlanScheduled := func() bool {

pkg/kudoctl/cmd/testdata/deploy-kudo-ns.yaml.golden

@@ -488,6 +488,9 @@ spec:
               properties:
                 planName:
                   type: string
+                status:
+                  description: ExecutionStatus captures the state of the rollout.
+                  type: string
                 uid:
                   description: UID is a type that holds unique ID values, including
                     UUIDs.  Because we don't ONLY use UUIDs, this is an alias to string.  Being

pkg/kudoctl/cmd/testdata/deploy-kudo-sa.yaml.golden

@@ -488,6 +488,9 @@ spec:
               properties:
                 planName:
                   type: string
+                status:
+                  description: ExecutionStatus captures the state of the rollout.
+                  type: string
                 uid:
                   description: UID is a type that holds unique ID values, including
                     UUIDs.  Because we don't ONLY use UUIDs, this is an alias to string.  Being

pkg/kudoctl/cmd/testdata/deploy-kudo-webhook.yaml.golden

@@ -488,6 +488,9 @@ spec:
               properties:
                 planName:
                   type: string
+                status:
+                  description: ExecutionStatus captures the state of the rollout.
+                  type: string
                 uid:
                   description: UID is a type that holds unique ID values, including
                     UUIDs.  Because we don't ONLY use UUIDs, this is an alias to string.  Being

pkg/kudoctl/cmd/testdata/deploy-kudo.yaml.golden

@@ -488,6 +488,9 @@ spec:
               properties:
                 planName:
                   type: string
+                status:
+                  description: ExecutionStatus captures the state of the rollout.
+                  type: string
                 uid:
                   description: UID is a type that holds unique ID values, including
                     UUIDs.  Because we don't ONLY use UUIDs, this is an alias to string.  Being

pkg/kudoctl/kudoinit/prereq/webhook.go

@@ -59,7 +59,7 @@ func (k kudoWebHook) Install(client *kube.Client) error {
 	if err := installUnstructured(client.DynamicClient, certificate(k.opts.Namespace)); err != nil {
 		return err
 	}
-	if err := installAdmissionWebhook(client.KubeClient.AdmissionregistrationV1beta1(), instanceAdmissionWebhook(k.opts.Namespace)); err != nil {
+	if err := installAdmissionWebhook(client.KubeClient.AdmissionregistrationV1beta1(), InstanceAdmissionWebhook(k.opts.Namespace)); err != nil {
 		return err
 	}
 	return nil
@@ -79,7 +79,7 @@ func (k kudoWebHook) AsRuntimeObjs() []runtime.Object {
 		return make([]runtime.Object, 0)
 	}
 
-	av := instanceAdmissionWebhook(k.opts.Namespace)
+	av := InstanceAdmissionWebhook(k.opts.Namespace)
 	cert := certificate(k.opts.Namespace)
 	objs := []runtime.Object{&av}
 	for _, c := range cert {
@@ -160,7 +160,8 @@ func installAdmissionWebhook(client clientv1beta1.MutatingWebhookConfigurationsG
 	return err
 }
 
-func instanceAdmissionWebhook(ns string) admissionv1beta1.MutatingWebhookConfiguration {
+// InstanceAdmissionWebhook returns a MutatingWebhookConfiguration for the instance admission controller.
+func InstanceAdmissionWebhook(ns string) admissionv1beta1.MutatingWebhookConfiguration {
 	namespacedScope := admissionv1beta1.NamespacedScope
 	failedType := admissionv1beta1.Fail
 	equivalentType := admissionv1beta1.Equivalent

pkg/webhook/instance_admission.go

@@ -10,7 +10,6 @@ import (
 
 	"github.com/thoas/go-funk"
 	"k8s.io/api/admission/v1beta1"
-	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/uuid"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
@@ -120,12 +119,14 @@ func handleUpdate(ia *InstanceAdmission, req admission.Request) admission.Respon
 		return admission.Denied(err.Error())
 	}
 
-	// populate Instance.PlanExecution with the plan triggered by param change and evtl. a new UID
+	// populate Instance.PlanExecution with the plan triggered by param change and evtl. a new UID/Status
 	if triggered != nil {
 		new.Spec.PlanExecution.PlanName = *triggered
 		new.Spec.PlanExecution.UID = ""
+		new.Spec.PlanExecution.Status = ""
 		if *triggered != "" {
-			new.Spec.PlanExecution.UID = uuid.NewUUID() // if there is a new plan, generate new UID
+			new.Spec.PlanExecution.UID = uuid.NewUUID()                   // if there is a new plan, generate new UID
+			new.Spec.PlanExecution.Status = kudov1beta1.ExecutionNeverRun // and set status to NEVER_RUN
 		}
 
 		marshaled, err := json.Marshal(new)
@@ -183,7 +184,7 @@ func admitUpdate(old, new *kudov1beta1.Instance, ov *kudov1beta1.OperatorVersion
 	isPlanRetriggered := hadPlan && newPlan == oldPlan && newUID != oldUID
 	isPlanCancellation := hadPlan && newPlan == ""
 	isDeleting := new.IsDeleting() // a non-empty meta.deletionTimestamp is a signal to switch to the uninstalling life-cycle phase
-	isPlanTerminal := isTerminal(new, newPlan, new.Spec.PlanExecution.UID)
+	isPlanTerminal := new.Spec.PlanExecution.Status.IsTerminal()
 
 	parameterDefs, err := changedParameterDefinitions(old.Spec.Parameters, new.Spec.Parameters, ov)
 	if err != nil {
@@ -284,12 +285,6 @@ func admitUpdate(old, new *kudov1beta1.Instance, ov *kudov1beta1.OperatorVersion
 	}
 }
 
-// isTerminal returns true if passed plan exists, has the same uid and is terminal
-func isTerminal(i *kudov1beta1.Instance, plan string, uid types.UID) bool {
-	status := i.PlanStatus(plan)
-	return status != nil && status.UID == uid && status.Status.IsTerminal()
-}
-
 // triggeredByParameterUpdate determines what plan to run based on parameters that changed and the corresponding parameter trigger.
 func triggeredByParameterUpdate(params []kudov1beta1.Parameter, ov *kudov1beta1.OperatorVersion) (*string, error) {
 	// If no parameters were changed, we return an empty string so no plan would be triggered