Repository: keikoproj/instance-manager
Pull request #273 (2021-04-15 06:17:21)

feat: conditional default values

codecov[bot]: # [Codecov](https://codecov.io/gh/keikoproj/instance-manager/pull/273?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=keikoproj) Report
> Merging [#273](https://codecov.io/gh/keikoproj/instance-manager/pull/273?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=keikoproj) (9369f61) into [master](https://codecov.io/gh/keikoproj/instance-manager/commit/c0eb25d65856ca1261f70465c56950f4bfc0cdc3?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=keikoproj) (c0eb25d) will **decrease** coverage by `1.70%`.
> The diff coverage is `71.05%`.

[![Impacted file tree graph](https://codecov.io/gh/keikoproj/instance-manager/pull/273/graphs/tree.svg?width=650&height=150&src=pr&token=IJbjmSBliL&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=keikoproj)](https://codecov.io/gh/keikoproj/instance-manager/pull/273?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=keikoproj)

```diff
@@            Coverage Diff             @@
##           master     #273      +/-   ##
==========================================
- Coverage   87.82%   86.11%   -1.71%     
==========================================
  Files          17       17              
  Lines        2267     2341      +74     
==========================================
+ Hits         1991     2016      +25     
- Misses        173      220      +47     
- Partials      103      105       +2     
```


| [Impacted Files](https://codecov.io/gh/keikoproj/instance-manager/pull/273?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=keikoproj) | Coverage Δ | |
|---|---|---|
| [controllers/provisioners/config.go](https://codecov.io/gh/keikoproj/instance-manager/pull/273/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=keikoproj#diff-Y29udHJvbGxlcnMvcHJvdmlzaW9uZXJzL2NvbmZpZy5nbw==) | `55.97% <71.05%> (-14.94%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/keikoproj/instance-manager/pull/273?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=keikoproj).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=keikoproj)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/keikoproj/instance-manager/pull/273?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=keikoproj). Last update [c0eb25d...9369f61](https://codecov.io/gh/keikoproj/instance-manager/pull/273?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=keikoproj). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=keikoproj).

eytan-avisror: Very cool. had some questions when you get the chance.. I may need to think about the edge cases of conflicts in default between multiple matching condtionals

Patch:
diff --git a/controllers/provisioners/config.go b/controllers/provisioners/config.go
index 49584a8..0a6923a 100644
--- a/controllers/provisioners/config.go
+++ b/controllers/provisioners/config.go
@@ -17,16 +17,16 @@ package provisioners
 
 import (
 	"fmt"
+	"k8s.io/apimachinery/pkg/labels"
 	"strings"
 
 	"github.com/ghodss/yaml"
-	"github.com/pkg/errors"
-	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-	runtime "k8s.io/apimachinery/pkg/runtime"
-
 	"github.com/keikoproj/instance-manager/api/v1alpha1"
 	"github.com/keikoproj/instance-manager/controllers/common"
+	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	runtime "k8s.io/apimachinery/pkg/runtime"
 )
 
 var (
@@ -46,9 +46,25 @@ var (
 type ProvisionerConfiguration struct {
 	Boundaries    ResourceFieldBoundary
 	Defaults      map[string]interface{}
+	Conditionals  []Conditional
 	InstanceGroup *v1alpha1.InstanceGroup
 }
 
+type SelectableAnnotations struct {
+	Annotations map[string]string
+}
+func (a *SelectableAnnotations) Has(label string) (exists bool) {
+	if _, ok := a.Annotations[label]; ok {
+		return true
+	}
+	return false
+}
+
+// Get returns the value for the provided label.
+func (a *SelectableAnnotations) Get(label string) (value string) {
+	return a.Annotations[label]
+}
+
 func NewProvisionerConfiguration(config *corev1.ConfigMap, instanceGroup *v1alpha1.InstanceGroup) (*ProvisionerConfiguration, error) {
 	var c = &ProvisionerConfiguration{}
 	c.InstanceGroup = &v1alpha1.InstanceGroup{}
@@ -70,10 +86,16 @@ type ResourceFieldBoundary struct {
 	Shared     SharedBoundaries `yaml:"shared,omitempty"`
 }
 
+type Conditional struct {
+	AnnotationSelector string                 `yaml:"annotationSelector,omitempty"`
+	Defaults   map[string]interface{} `yaml:"defaults,omitempty"`
+}
+
 func (c *ProvisionerConfiguration) Unmarshal(cm *corev1.ConfigMap) error {
 	var (
-		boundariesPath = common.FieldPath("data.boundaries")
-		defaultsPath   = common.FieldPath("data.defaults")
+		boundariesPath   = common.FieldPath("data.boundaries")
+		defaultsPath     = common.FieldPath("data.defaults")
+		conditionalsPath = common.FieldPath("data.conditionals")
 	)
 
 	config, err := runtime.DefaultUnstructuredConverter.ToUnstructured(cm)
@@ -103,6 +125,14 @@ func (c *ProvisionerConfiguration) Unmarshal(cm *corev1.ConfigMap) error {
 		}
 	}
 
+	if conditionals, ok, _ := unstructured.NestedString(config, conditionalsPath...); ok {
+		var conditionalConfig = make([]Conditional, 0)
+		err := yaml.Unmarshal([]byte(conditionals), &conditionalConfig)
+		if err != nil {
+			return errors.Wrap(err, "failed to convert conditionals to unstructured")
+		}
+		c.Conditionals = conditionalConfig
+	}
 	return nil
 }
 
@@ -130,9 +160,26 @@ func (c *ProvisionerConfiguration) SetDefaults() error {
 
 func (c *ProvisionerConfiguration) setRestrictedFields(unstructuredInstanceGroup map[string]interface{}) error {
 	// apply restricted paths to instance group
+	var applicableConditionals, err = getMatchingConditionals(c.InstanceGroup, c.Conditionals)
+	if err != nil {
+		return err
+	}
 	for _, pathStr := range c.Boundaries.Restricted {
 		path := common.FieldPath(pathStr)
 		// if a default value exists for the path, set it on the instance group
+		var setField = false
+		var field interface{}
+		for _, conditional := range applicableConditionals {
+			if field, setField, _ = unstructured.NestedFieldCopy(conditional.Defaults, path...); setField {
+				err := unstructured.SetNestedField(unstructuredInstanceGroup, field, path...)
+				if err != nil {
+					errors.Wrap(err, "failed to set nested field")
+				}
+			}
+		}
+		if setField {
+			continue
+		}
 		if field, ok, _ := unstructured.NestedFieldCopy(c.Defaults, path...); ok {
 			// default value exists for restricted path
 			err := unstructured.SetNestedField(unstructuredInstanceGroup, field, path...)
@@ -151,14 +198,38 @@ func isConflict(defaultVal, resourceVal interface{}) bool {
 	return false
 }
 
-func (c *ProvisionerConfiguration) setSharedFields(obj map[string]interface{}) error {
+func getMatchingConditionals(ig *v1alpha1.InstanceGroup, conditionals []Conditional) ([]Conditional,error) {
+	var applicableConditionals = make([]Conditional, 0)
+	var annotationLabels = &SelectableAnnotations{Annotations: ig.Annotations}
+	for _, conditional := range conditionals {
+		selector, err := labels.Parse(conditional.AnnotationSelector)
+		if err != nil {
+			return nil, err;
+		}
+		if selector.Matches(annotationLabels){
+			applicableConditionals = append(applicableConditionals, conditional)
+		}
+	}
+	return applicableConditionals, nil
+}
 
+func (c *ProvisionerConfiguration) setSharedFields(obj map[string]interface{}) error {
+	var applicableConditionals, err = getMatchingConditionals(c.InstanceGroup, c.Conditionals)
+	if err != nil {
+		return err
+	}
 	for _, pathStr := range c.Boundaries.Shared.Replace {
 		var (
 			defaultVal  = common.FieldValue(pathStr, c.Defaults)
 			resourceVal = common.FieldValue(pathStr, obj)
 		)
 
+		for _, conditional := range applicableConditionals {
+			conditionalValue := common.FieldValue(pathStr, conditional.Defaults)
+			if conditionalValue != nil {
+				defaultVal = conditionalValue
+			}
+		}
 		if defaultVal == nil {
 			continue
 		}
@@ -180,6 +251,20 @@ func (c *ProvisionerConfiguration) setSharedFields(obj map[string]interface{}) e
 			resourceVal = common.FieldValue(pathStr, obj)
 		)
 
+		for _, conditional := range applicableConditionals {
+			conditionalValue := common.FieldValue(pathStr, conditional.Defaults)
+			if conditionalValue != nil {
+				if isConflict(conditionalValue, resourceVal) {
+					merge := Merge(conditionalValue, resourceVal, pathStr, false)
+					if err := common.SetFieldValue(pathStr, obj, merge); err != nil {
+						return errors.Wrap(err, "failed to merge field")
+					}
+					resourceVal = merge
+					continue
+				}
+			}
+		}
+
 		if defaultVal == nil {
 			continue
 		}
@@ -202,6 +287,13 @@ func (c *ProvisionerConfiguration) setSharedFields(obj map[string]interface{}) e
 			resourceVal = common.FieldValue(pathStr, obj)
 		)
 
+		for _, conditional := range applicableConditionals {
+			conditionalValue := common.FieldValue(pathStr, conditional.Defaults)
+			if conditionalValue != nil {
+				defaultVal = conditionalValue
+			}
+		}
+
 		if defaultVal == nil {
 			continue
 		}
diff --git a/docs/EKS.md b/docs/EKS.md
index 1279270..8f9eeb4 100644
--- a/docs/EKS.md
+++ b/docs/EKS.md
@@ -397,7 +397,7 @@ spec:
       # you can also reference "All" to suspend all processes
 ```
 
-## GitOps/Platform support, boundaries and default values
+## GitOps/Platform support, boundaries, default and conditional values
 
 In order to support use-cases around GitOps or platform management, the controller allows operators to define 'boundaries' of configurations into `restricted` and `shared` configurations, along with the default values to enforce.
 
@@ -545,6 +545,64 @@ This also makes upgrades easier across a managed cluster, an operator can now si
 
 Individual namespaces can opt-out by adding the annotation `instancemgr.keikoproj.io/config-excluded=true`, this is useful for system namespaces which may need to override a global restrictive configuration, e.g. subnet, while keeping the boundary as is for other namespaces - adding this annotation to a namespace will opt-out all instancegroups under the namespace from using the cluster configuration.
 
+
+### Conditional defaults
+For more complex setups, such as clusters that have InstanceGroups that have different architectures, operating systems, etc - it might be 
+desirable to conditionally apply default values. Conditional default values can be added, as seen in the example below:
+```yaml
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: instance-manager
+  namespace: instance-manager
+data:
+  boundaries: |
+    shared:
+      merge:
+      - spec.eks.configuration.labels
+      mergeOverride:
+      - spec.eks.configuration.tags
+      replace:
+      - spec.eks.strategy
+    restricted:
+    - spec.eks.configuration.image
+  defaults: |
+    spec:
+      provisioner: eks
+      eks:
+        configuration:
+          image: ami-x86linux
+  conditionals: |
+    - annotation: 'instancemgr.keikoproj.io/os-family = windows'
+      defaults:
+       spec:
+         eks:
+           configuration:
+             image: ami-windows
+    - annotationSelector: 'instancemgr.keikoproj.io/arch = arm64,instancemgr.keikoproj.io/os-family = bottlerocket'
+      defaults:
+       spec:
+         eks:
+           configuration:
+             image: ami-bottlerocketArm
+```
+
+The `annotationSelector` field accepts a selector string, which follows the semantics of the Kubernetes [field selector](https://kubernetes.io/docs/concepts/overview/working-with-objects/field-selectors/). 
+This selector is applied against the annotations of InstanceGroup objects. When a selector matches an InstanceGroup, the `defaults` associated with it are applied to the InstangeGroup, taking precedence over the non-conditional 
+default values. Conditional rules are applied in-order from the beginning of the list to the end - meaning that the last applicable rule will apply in case of conflicts. 
+
+The following operators are supported:
+```
+<selector-syntax>         ::= <requirement> | <requirement> "," <selector-syntax>
+<requirement>             ::= [!] KEY [ <set-based-restriction> | <exact-match-restriction> ]
+<set-based-restriction>   ::= "" | <inclusion-exclusion> <value-set>
+<inclusion-exclusion>     ::= <inclusion> | <exclusion>
+<exclusion>               ::= "notin"
+<inclusion>               ::= "in"
+<value-set>               ::= "(" <values> ")"
+<values>                  ::= VALUE | VALUE "," <values>
+<exact-match-restriction> ::= ["="|"=="|"!="] VALUE
+```
 ## Annotations
 
 | Annotation Key | Object | Annotation Value | Purpose |


Test patch:
diff --git a/controllers/provisioners/config_test.go b/controllers/provisioners/config_test.go
index c733dc1..caa1fa9 100644
--- a/controllers/provisioners/config_test.go
+++ b/controllers/provisioners/config_test.go
@@ -104,6 +104,15 @@ func TestSetDefaultsRestricted(t *testing.T) {
     - spec.eks.configuration.instanceType
     - spec.strategy`
 
+	mockConditionals := `
+- annotationSelector: "instancemgr.keikoproj.io/os-family=windows"
+  defaults:
+    spec:
+      eks:
+        configuration:
+          image: ami-22222222222
+`
+
 	mockDefaults := `
 spec:
   strategy:
@@ -124,7 +133,7 @@ spec:
         value: taint-value
         effect: NoSchedule`
 
-	cm := MockConfigMap(MockConfigData("boundaries", mockBoundaries, "defaults", mockDefaults))
+	cm := MockConfigMap(MockConfigData("boundaries", mockBoundaries, "defaults", mockDefaults, "conditionals", mockConditionals))
 	cr := MockResource()
 	cr.Spec.EKSSpec.EKSConfiguration.EksClusterName = "someCluster"
 	cr.Spec.EKSSpec.EKSConfiguration.NodeSecurityGroups = []string{"sg-000000000000"}
@@ -160,6 +169,382 @@ spec:
 	g.Expect(c.InstanceGroup.Spec.EKSSpec.EKSConfiguration.KeyPairName).To(gomega.Equal("TestKeyPair"))
 }
 
+func TestSetDefaultsWithRestrictedConditional(t *testing.T) {
+	var (
+		g = gomega.NewGomegaWithT(t)
+	)
+
+	// Restricted fields are always replaced with default values
+
+	mockBoundaries := `
+    restricted:
+    - spec.eks.configuration.keyPairName
+    - spec.eks.configuration.taints
+    - spec.eks.configuration.labels
+    - spec.eks.configuration.securityGroups
+    - spec.eks.configuration.instanceType
+    - spec.eks.configuration.image
+    - spec.strategy`
+
+	mockConditionals := `
+- annotationSelector: "instancemgr.keikoproj.io/os-family=windows"
+  defaults:
+    spec:
+      eks:
+        configuration:
+          image: ami-22222222222
+- annotationSelector: "instancemgr.keikoproj.io/arch=arm64"
+  defaults:
+    spec:
+      eks:
+        configuration:
+          image: ami-33333333333
+`
+
+	mockDefaults := `
+spec:
+  strategy:
+    type: rollingUpdate
+    rollingUpdate:
+      maxUnavailable: 30%
+  eks:
+    configuration:
+      keyPairName: TestKeyPair
+      image: ami-025bf02d663404bbc
+      securityGroups:
+      - sg-123456789012
+      instanceType: m5.large
+      labels:
+        label-key: label-value
+      taints:
+      - key: taint-key
+        value: taint-value
+        effect: NoSchedule`
+
+	cm := MockConfigMap(MockConfigData("boundaries", mockBoundaries, "defaults", mockDefaults, "conditionals", mockConditionals))
+	cr := MockResource()
+	cr.Annotations = make(map[string]string)
+	cr.Annotations["instancemgr.keikoproj.io/os-family"] = "windows"
+	cr.Annotations["instancemgr.keikoproj.io/arch"] = "arm64"
+	cr.Spec.EKSSpec.EKSConfiguration.EksClusterName = "someCluster"
+	cr.Spec.EKSSpec.EKSConfiguration.NodeSecurityGroups = []string{"sg-000000000000"}
+	cr.Spec.EKSSpec.EKSConfiguration.InstanceType = "m5.xlarge"
+	cr.Spec.EKSSpec.EKSConfiguration.Labels = MockLabels("other-label-key", "other-label-value")
+	cr.Spec.EKSSpec.EKSConfiguration.Taints = []corev1.Taint{MockTaint("other-taint-key", "other-taint-value", "NoExecute")}
+	cr.Spec.AwsUpgradeStrategy = v1alpha1.AwsUpgradeStrategy{Type: "crd"}
+	c, err := NewProvisionerConfiguration(cm, cr)
+	g.Expect(err).NotTo(gomega.HaveOccurred())
+
+	err = c.SetDefaults()
+	g.Expect(err).NotTo(gomega.HaveOccurred())
+
+	// All restricted fields must be overwritten with default value even if the user sets them
+	g.Expect(c.InstanceGroup.Spec.AwsUpgradeStrategy).To(gomega.Equal(v1alpha1.AwsUpgradeStrategy{
+		Type: "rollingUpdate",
+		RollingUpdateType: &v1alpha1.RollingUpdateStrategy{
+			MaxUnavailable: &intstr.IntOrString{Type: intstr.String, StrVal: "30%"},
+		},
+	}))
+	g.Expect(c.InstanceGroup.Spec.EKSSpec.EKSConfiguration.NodeSecurityGroups).To(gomega.Equal([]string{"sg-123456789012"}))
+	g.Expect(c.InstanceGroup.Spec.EKSSpec.EKSConfiguration.InstanceType).To(gomega.Equal("m5.large"))
+	g.Expect(c.InstanceGroup.Spec.EKSSpec.EKSConfiguration.Labels).To(gomega.Equal(MockLabels("label-key", "label-value")))
+	g.Expect(c.InstanceGroup.Spec.EKSSpec.EKSConfiguration.Taints).To(gomega.Equal([]corev1.Taint{MockTaint("taint-key", "taint-value", "NoSchedule")}))
+	g.Expect(c.InstanceGroup.Spec.EKSSpec.EKSConfiguration.Image).To(gomega.Equal("ami-33333333333"))
+
+	// Fields without defaults should stay as provided
+	g.Expect(c.InstanceGroup.Spec.EKSSpec.EKSConfiguration.EksClusterName).To(gomega.Equal("someCluster"))
+
+	// Fields with defaults are used when CR does not provide it
+	g.Expect(c.InstanceGroup.Spec.EKSSpec.EKSConfiguration.KeyPairName).To(gomega.Equal("TestKeyPair"))
+}
+
+func TestSetDefaultsWithSharedConditionalReplace(t *testing.T) {
+	var (
+		g = gomega.NewGomegaWithT(t)
+	)
+
+	// Restricted fields are always replaced with default values
+
+	mockBoundaries := `
+    shared:
+      replace: 
+      - spec.eks.configuration.tags`
+
+	mockConditionals := `
+- annotationSelector: "instancemgr.keikoproj.io/os-family=windows"
+  defaults:
+    spec:
+      eks:
+        configuration:
+          securityGroups:
+          - sg-923456789012
+          tags:
+          - key: tag-A
+            value: value-A
+          - key: tag-B
+            value: value-B`
+
+	mockDefaults := `
+spec:
+  strategy:
+    type: rollingUpdate
+    rollingUpdate:
+      maxUnavailable: 30%
+  eks:
+    configuration:
+      keyPairName: TestKeyPair
+      image: ami-025bf02d663404bbc
+      instanceType: m5.large
+      labels:
+        label-key: label-value
+      taints:
+      - key: taint-key
+        value: taint-value
+        effect: NoSchedule
+`
+
+	cm := MockConfigMap(MockConfigData("boundaries", mockBoundaries, "defaults", mockDefaults, "conditionals", mockConditionals))
+	cr := MockResource()
+	cr.Annotations = make(map[string]string)
+	cr.Annotations["instancemgr.keikoproj.io/os-family"] = "windows"
+	cr.Annotations["instancemgr.keikoproj.io/arch"] = "arm64"
+	cr.Spec.EKSSpec.EKSConfiguration.EksClusterName = "someCluster"
+	cr.Spec.EKSSpec.EKSConfiguration.Tags = []map[string]string{
+		MockTag("tag-A", "value-D"),
+		MockTag("tag-C", "value-C"),
+	}
+	cr.Spec.EKSSpec.EKSConfiguration.NodeSecurityGroups = []string{"sg-000000000000"}
+	cr.Spec.AwsUpgradeStrategy = v1alpha1.AwsUpgradeStrategy{Type: "crd"}
+	c, err := NewProvisionerConfiguration(cm, cr)
+	g.Expect(err).NotTo(gomega.HaveOccurred())
+
+	err = c.SetDefaults()
+	g.Expect(err).NotTo(gomega.HaveOccurred())
+	//Should not replace
+	g.Expect(c.InstanceGroup.Spec.EKSSpec.EKSConfiguration.NodeSecurityGroups).To(gomega.Equal([]string{"sg-000000000000"}))
+	g.Expect(c.InstanceGroup.Spec.EKSSpec.EKSConfiguration.Tags).To(gomega.Equal([]map[string]string{
+		MockTag("tag-A", "value-D"),
+		MockTag("tag-C", "value-C"),
+	}))
+}
+
+func TestSetDefaultsWithSharedConditionalMergeOverride(t *testing.T) {
+	var (
+		g = gomega.NewGomegaWithT(t)
+	)
+
+	// Restricted fields are always replaced with default values
+
+	mockBoundaries := `
+    shared:
+      mergeOverride: 
+      - spec.eks.configuration.tags`
+
+	mockConditionals := `
+- annotationSelector: "instancemgr.keikoproj.io/arch in (arm64),instancemgr.keikoproj.io/os-family in (windows)"
+  defaults:
+    spec:
+      eks:
+        configuration:
+          tags:
+          - key: tag-A
+            value: value-A
+          - key: tag-B
+            value: value-B`
+	mockDefaults := `
+spec:
+  strategy:
+    type: rollingUpdate
+    rollingUpdate:
+      maxUnavailable: 30%
+  eks:
+    configuration:
+      keyPairName: TestKeyPair
+      image: ami-025bf02d663404bbc
+      instanceType: m5.large
+      labels:
+        label-key: label-value
+      taints:
+      - key: taint-key
+        value: taint-value
+        effect: NoSchedule
+`
+
+	cm := MockConfigMap(MockConfigData("boundaries", mockBoundaries, "defaults", mockDefaults, "conditionals", mockConditionals))
+	cr := MockResource()
+	cr.Annotations = make(map[string]string)
+	cr.Annotations["instancemgr.keikoproj.io/os-family"] = "windows"
+	cr.Annotations["instancemgr.keikoproj.io/arch"] = "arm64"
+	cr.Spec.EKSSpec.EKSConfiguration.EksClusterName = "someCluster"
+	cr.Spec.EKSSpec.EKSConfiguration.Tags = []map[string]string{
+		MockTag("tag-A", "value-D"),
+		MockTag("tag-C", "value-C"),
+	}
+	cr.Spec.EKSSpec.EKSConfiguration.NodeSecurityGroups = []string{"sg-000000000000"}
+	cr.Spec.AwsUpgradeStrategy = v1alpha1.AwsUpgradeStrategy{Type: "crd"}
+	c, err := NewProvisionerConfiguration(cm, cr)
+	g.Expect(err).NotTo(gomega.HaveOccurred())
+
+	err = c.SetDefaults()
+	g.Expect(err).NotTo(gomega.HaveOccurred())
+	//Should not replace
+	g.Expect(c.InstanceGroup.Spec.EKSSpec.EKSConfiguration.NodeSecurityGroups).To(gomega.Equal([]string{"sg-000000000000"}))
+	g.Expect(c.InstanceGroup.Spec.EKSSpec.EKSConfiguration.Tags).To(gomega.Equal([]map[string]string{
+		MockTag("tag-A", "value-D"),
+		MockTag("tag-B", "value-B"),
+		MockTag("tag-C", "value-C"),
+	}))
+}
+
+func TestSetDefaultsWithInvalidConditionalYAML(t *testing.T) {
+	var (
+		g = gomega.NewGomegaWithT(t)
+	)
+
+	// Restricted fields are always replaced with default values
+
+	mockBoundaries := `
+    shared:
+      mergeOverride: 
+      - spec.eks.configuration.tags`
+
+	mockConditionals := `
+- annotationSelector: "instancemgr.keikoproj.io/os-family=windows"
+  defaults:
+    spec:
+      eks:
+        # invalid TAB character on following line
+		configuration:
+          tags:
+          - key: tag-A
+            value: value-A
+          - key: tag-B
+            value: value-B`
+	mockDefaults := `
+spec:
+  strategy:
+    type: rollingUpdate
+    rollingUpdate:
+      maxUnavailable: 30%
+  eks:
+    configuration:
+      keyPairName: TestKeyPair
+      image: ami-025bf02d663404bbc
+      instanceType: m5.large
+      labels:
+        label-key: label-value
+      taints:
+      - key: taint-key
+        value: taint-value
+        effect: NoSchedule
+`
+
+	cm := MockConfigMap(MockConfigData("boundaries", mockBoundaries, "defaults", mockDefaults, "conditionals", mockConditionals))
+	cr := MockResource()
+	_, err := NewProvisionerConfiguration(cm, cr)
+	g.Expect(err).To(gomega.HaveOccurred())
+}
+
+func TestSetDefaultsWithInvalidConditionalSelector(t *testing.T) {
+	var (
+		g = gomega.NewGomegaWithT(t)
+	)
+
+	// Restricted fields are always replaced with default values
+
+	mockBoundaries := `
+    shared:
+      mergeOverride: 
+      - spec.eks.configuration.tags`
+
+	mockConditionals := `
+- annotationSelector: "instancemgr.keikoproj.io/os-family==f==windows"
+  defaults:
+    spec:
+      eks:
+        configuration:
+          tags:
+          - key: tag-A
+            value: value-A
+          - key: tag-B
+            value: value-B`
+	mockDefaults := `
+spec:
+  strategy:
+    type: rollingUpdate
+    rollingUpdate:
+      maxUnavailable: 30%
+  eks:
+    configuration:
+      keyPairName: TestKeyPair
+      image: ami-025bf02d663404bbc
+      instanceType: m5.large
+      labels:
+        label-key: label-value
+      taints:
+      - key: taint-key
+        value: taint-value
+        effect: NoSchedule
+`
+
+	cm := MockConfigMap(MockConfigData("boundaries", mockBoundaries, "defaults", mockDefaults, "conditionals", mockConditionals))
+	cr := MockResource()
+	c, _ := NewProvisionerConfiguration(cm, cr)
+	err := c.SetDefaults()
+	g.Expect(err).To(gomega.HaveOccurred())
+}
+
+func TestSetDefaultsWithSharedConditionalMerge(t *testing.T) {
+	var (
+		g = gomega.NewGomegaWithT(t)
+	)
+
+	mockBoundaries := `
+    shared:
+      merge:
+      - spec.eks.configuration.securityGroups`
+
+	mockConditionals := `
+- annotationSelector: "instancemgr.keikoproj.io/os-family=windows"
+  defaults:
+    spec:
+      eks:
+        configuration:
+          securityGroups:
+          - sg-923456789012`
+
+	mockDefaults := `
+spec:
+  strategy:
+    type: rollingUpdate
+    rollingUpdate:
+      maxUnavailable: 30%
+  eks:
+    configuration:
+      securityGroups:
+      - sg-1234`
+
+	cm := MockConfigMap(MockConfigData("boundaries", mockBoundaries, "defaults", mockDefaults, "conditionals", mockConditionals))
+	cr := MockResource()
+	cr.Annotations = make(map[string]string)
+	cr.Annotations["instancemgr.keikoproj.io/os-family"] = "windows"
+	cr.Annotations["instancemgr.keikoproj.io/arch"] = "arm64"
+	cr.Spec.EKSSpec.EKSConfiguration.EksClusterName = "someCluster"
+	cr.Spec.EKSSpec.EKSConfiguration.NodeSecurityGroups = []string{"sg-000000000000"}
+	cr.Spec.AwsUpgradeStrategy = v1alpha1.AwsUpgradeStrategy{Type: "crd"}
+	c, err := NewProvisionerConfiguration(cm, cr)
+	g.Expect(err).NotTo(gomega.HaveOccurred())
+
+	err = c.SetDefaults()
+	g.Expect(err).NotTo(gomega.HaveOccurred())
+	//Should not replace
+	g.Expect(c.InstanceGroup.Spec.EKSSpec.EKSConfiguration.NodeSecurityGroups).To(gomega.Equal([]string{
+		"sg-1234",
+		"sg-923456789012",
+		"sg-000000000000",
+	}))
+}
+
 func TestSetDefaultsSharedMergeOverride(t *testing.T) {
 	var (
 		g = gomega.NewGomegaWithT(t)