Repository: keikoproj/instance-manager
Pull request #109 (2020-05-16 23:19:41)

Spec validation
This adds some defaults and early failure around missing/invalid spec fields

- [x] BDD

codecov[bot]: # [Codecov](https://codecov.io/gh/keikoproj/instance-manager/pull/109?src=pr&el=h1) Report
> Merging [#109](https://codecov.io/gh/keikoproj/instance-manager/pull/109?src=pr&el=desc) into [master](https://codecov.io/gh/keikoproj/instance-manager/commit/71e192f513237524598d28247d8ff05efc1b28a0&el=desc) will **decrease** coverage by `0.03%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/keikoproj/instance-manager/pull/109/graphs/tree.svg?width=650&height=150&src=pr&token=IJbjmSBliL)](https://codecov.io/gh/keikoproj/instance-manager/pull/109?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #109      +/-   ##
==========================================
- Coverage   87.06%   87.02%   -0.04%     
==========================================
  Files          10       10              
  Lines         982      979       -3     
==========================================
- Hits          855      852       -3     
  Misses         91       91              
  Partials       36       36              
```


| [Impacted Files](https://codecov.io/gh/keikoproj/instance-manager/pull/109?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [controllers/provisioners/eks/upgrade.go](https://codecov.io/gh/keikoproj/instance-manager/pull/109/diff?src=pr&el=tree#diff-Y29udHJvbGxlcnMvcHJvdmlzaW9uZXJzL2Vrcy91cGdyYWRlLmdv) | `95.89% <100.00%> (-0.17%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/keikoproj/instance-manager/pull/109?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/keikoproj/instance-manager/pull/109?src=pr&el=footer). Last update [71e192f...be88a02](https://codecov.io/gh/keikoproj/instance-manager/pull/109?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/api/v1alpha1/instancegroup_types.go b/api/v1alpha1/instancegroup_types.go
index 628fa34..338379a 100644
--- a/api/v1alpha1/instancegroup_types.go
+++ b/api/v1alpha1/instancegroup_types.go
@@ -18,6 +18,7 @@ package v1alpha1
 import (
 	"strings"
 
+	"github.com/keikoproj/instance-manager/controllers/common"
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -44,13 +45,27 @@ const (
 	ReconcileReady ReconcileState = "Ready"
 	ReconcileErr   ReconcileState = "Error"
 
-	LifecycleStateNormal = "normal"
-	LifecycleStateSpot   = "spot"
+	LifecycleStateNormal      = "normal"
+	LifecycleStateSpot        = "spot"
+	CRDStrategyName           = "crd"
+	RollingUpdateStrategyName = "rollingupdate"
+	EKSProvisionerName        = "eks"
+	EKSManagedProvisionerName = "eks-managed"
 
 	NodesReady InstanceGroupConditionType = "NodesReady"
 )
 
 var (
+	Strategies   = []string{"crd", "rollingupdate", "managed"}
+	Provisioners = []string{"eks", "eks-managed"}
+
+	DefaultRollingUpdateStrategy = &RollingUpdateStrategy{
+		MaxUnavailable: &intstr.IntOrString{
+			Type:   intstr.Int,
+			IntVal: 1,
+		},
+	}
+
 	GroupVersionResource = schema.GroupVersionResource{
 		Group:    "instancemgr.keikoproj.io",
 		Version:  "v1alpha1",
@@ -247,6 +262,43 @@ func (c *EKSConfiguration) Validate() error {
 	}
 	return nil
 }
+func (s *InstanceGroupSpec) Validate() error {
+	if !common.ContainsEqualFold(Provisioners, s.Provisioner) {
+		return errors.Errorf("validation failed, provisioner '%v' is invalid", s.Provisioner)
+	}
+
+	if strings.EqualFold(s.Provisioner, EKSProvisionerName) {
+		if err := s.EKSSpec.EKSConfiguration.Validate(); err != nil {
+			return err
+		}
+	}
+
+	// TODO: Add validation for EKSManagedProvisioner
+
+	if s.AwsUpgradeStrategy.Type == "" {
+		s.AwsUpgradeStrategy.Type = RollingUpdateStrategyName
+	}
+
+	if !common.ContainsEqualFold(Strategies, s.AwsUpgradeStrategy.Type) {
+		return errors.Errorf("validation failed, strategy '%v' is invalid", s.AwsUpgradeStrategy.Type)
+	}
+
+	if strings.EqualFold(s.AwsUpgradeStrategy.Type, CRDStrategyName) && s.AwsUpgradeStrategy.CRDType == nil {
+		return errors.Errorf("validation failed, strategy.crd is required")
+	}
+
+	if strings.EqualFold(s.AwsUpgradeStrategy.Type, CRDStrategyName) && s.AwsUpgradeStrategy.CRDType != nil {
+		if err := s.AwsUpgradeStrategy.CRDType.Validate(); err != nil {
+			return err
+		}
+	}
+
+	if strings.EqualFold(s.AwsUpgradeStrategy.Type, RollingUpdateStrategyName) && s.AwsUpgradeStrategy.RollingUpdateType == nil {
+		s.AwsUpgradeStrategy.RollingUpdateType = DefaultRollingUpdateStrategy
+	}
+
+	return nil
+}
 func (c *EKSConfiguration) GetRoleName() string {
 	return c.ExistingRoleName
 }
diff --git a/controllers/common/utils.go b/controllers/common/utils.go
index befd351..1e4d23d 100644
--- a/controllers/common/utils.go
+++ b/controllers/common/utils.go
@@ -34,6 +34,16 @@ func ContainsString(slice []string, s string) bool {
 	return false
 }
 
+// ContainsString returns true if a given slice 'slice' contains string 's', otherwise return false
+func ContainsEqualFold(slice []string, s string) bool {
+	for _, item := range slice {
+		if strings.EqualFold(item, s) {
+			return true
+		}
+	}
+	return false
+}
+
 func StringSliceEquals(x, y []string) bool {
 	sort.Strings(x)
 	sort.Strings(y)
diff --git a/controllers/instancegroup_controller.go b/controllers/instancegroup_controller.go
index 3f28db0..4e69804 100644
--- a/controllers/instancegroup_controller.go
+++ b/controllers/instancegroup_controller.go
@@ -131,6 +131,13 @@ func (r *InstanceGroupReconciler) Reconcile(req ctrl.Request) (ctrl.Result, erro
 		return ctrl.Result{}, err
 	}
 
+	if err = instanceGroup.Spec.Validate(); err != nil {
+		r.Log.Error(err, "reconcile failed")
+		instanceGroup.SetState(v1alpha1.ReconcileErr)
+		r.Update(context.Background(), instanceGroup)
+		return ctrl.Result{}, err
+	}
+
 	// Add Finalizer if not present, and set the initial state
 	finalizerName := fmt.Sprintf("finalizers.%v.instancegroups.keikoproj.io", instanceGroup.Spec.Provisioner)
 	r.SetFinalizer(instanceGroup, finalizerName)
diff --git a/controllers/provisioners/eks/upgrade.go b/controllers/provisioners/eks/upgrade.go
index 8e83ebf..b256d8f 100644
--- a/controllers/provisioners/eks/upgrade.go
+++ b/controllers/provisioners/eks/upgrade.go
@@ -30,16 +30,12 @@ func (ctx *EksInstanceGroupContext) UpgradeNodes() error {
 	var (
 		instanceGroup = ctx.GetInstanceGroup()
 		strategy      = ctx.GetUpgradeStrategy()
+		strategyType  = strings.ToLower(strategy.GetType())
 	)
 
 	// process the upgrade strategy
-	switch strings.ToLower(strategy.GetType()) {
+	switch strategyType {
 	case kubeprovider.CRDStrategyName:
-		crdStrategy := strategy.GetCRDType()
-		if err := crdStrategy.Validate(); err != nil {
-			instanceGroup.SetState(v1alpha1.ReconcileErr)
-			return errors.Wrap(err, "failed to validate strategy spec")
-		}
 		ok, err := kubeprovider.ProcessCRDStrategy(ctx.KubernetesClient.KubeDynamic, instanceGroup)
 		if err != nil {
 			instanceGroup.SetState(v1alpha1.ReconcileErr)


Test patch:
diff --git a/controllers/provisioners/eks/upgrade_test.go b/controllers/provisioners/eks/upgrade_test.go
index 41ae00f..287cac8 100644
--- a/controllers/provisioners/eks/upgrade_test.go
+++ b/controllers/provisioners/eks/upgrade_test.go
@@ -33,20 +33,22 @@ import (
 
 func TestUpgradeCRDStrategyValidation(t *testing.T) {
 	var (
-		g       = gomega.NewGomegaWithT(t)
-		k       = MockKubernetesClientSet()
-		ig      = MockInstanceGroup()
-		cr      = MockCustomResourceSpec()
-		crd     = MockCustomResourceDefinition()
-		asgMock = NewAutoScalingMocker()
-		iamMock = NewIamMocker()
+		g   = gomega.NewGomegaWithT(t)
+		k   = MockKubernetesClientSet()
+		ig  = MockInstanceGroup()
+		cr  = MockCustomResourceSpec()
+		crd = MockCustomResourceDefinition()
 	)
 
-	w := MockAwsWorker(asgMock, iamMock)
-	ctx := MockContext(ig, k, w)
+	config := ig.GetEKSConfiguration()
 
 	// assume initial state of modifying
 	ig.SetState(v1alpha1.ReconcileModifying)
+	config.Subnets = []string{"subnet-1"}
+	config.Image = "ami-1234"
+	config.NodeSecurityGroups = []string{"sg-12323"}
+	config.InstanceType = "m5.large"
+	config.KeyPairName = "myKey"
 
 	// get custom resource yaml
 	crYAML, err := yaml.Marshal(cr.Object)
@@ -79,25 +81,25 @@ func TestUpgradeCRDStrategyValidation(t *testing.T) {
 		shouldErr     bool
 		expectedState v1alpha1.ReconcileState
 	}{
-		{input: valid, shouldErr: false, expectedState: v1alpha1.ReconcileModifying},
-		{input: missingName, shouldErr: true, expectedState: v1alpha1.ReconcileErr},
-		{input: missingSpec, shouldErr: true, expectedState: v1alpha1.ReconcileErr},
-		{input: missingConcurrency, shouldErr: false, expectedState: v1alpha1.ReconcileModifying},
-		{input: missingStatusPath, shouldErr: true, expectedState: v1alpha1.ReconcileErr},
-		{input: missingSuccessStr, shouldErr: true, expectedState: v1alpha1.ReconcileErr},
-		{input: missingFailureStr, shouldErr: true, expectedState: v1alpha1.ReconcileErr},
+		{input: valid, shouldErr: false},
+		{input: missingName, shouldErr: true},
+		{input: missingSpec, shouldErr: true},
+		{input: missingConcurrency, shouldErr: false},
+		{input: missingStatusPath, shouldErr: true},
+		{input: missingSuccessStr, shouldErr: true},
+		{input: missingFailureStr, shouldErr: true},
 	}
 
 	for i, tc := range tests {
-		t.Logf("#%v - %+v", i, tc.input.CRDType)
+		t.Logf("#%v - %+v", i, tc.input)
 		var errOccured bool
 		ig.SetUpgradeStrategy(tc.input)
-		err := ctx.UpgradeNodes()
+		err := ig.Spec.Validate()
 		if err != nil {
+			t.Log(err)
 			errOccured = true
 		}
 		g.Expect(errOccured).To(gomega.Equal(tc.shouldErr))
-		g.Expect(ctx.GetState()).To(gomega.Equal(tc.expectedState))
 	}
 	g.Expect(missingConcurrency.CRDType.ConcurrencyPolicy).To(gomega.Equal("forbid"))
 }