Merge pull request #756 from mumoshu/disallow-controller-taint

Explicitly disallow tainting controller nodes

Author: mumoshu

core/controlplane/config/config.go

@@ -85,8 +85,6 @@ func NewDefaultCluster() *Cluster {
 				Enabled: false,
 			},
 		},
-
-		Taints: model.Taints{},
 		Dex: model.Dex{
 			Enabled:         false,
 			Url:             "https://dex.example.com",
@@ -691,7 +689,6 @@ type Experimental struct {
 	Dex                         model.Dex                      `yaml:"dex"`
 	DisableSecurityGroupIngress bool                           `yaml:"disableSecurityGroupIngress"`
 	NodeMonitorGracePeriod      string                         `yaml:"nodeMonitorGracePeriod"`
-	Taints                      model.Taints                   `yaml:"taints"`
 	model.UnknownKeys           `yaml:",inline"`
 }
 
@@ -1479,10 +1476,6 @@ func (e EtcdSettings) Valid() error {
 }
 
 func (c Experimental) Valid() error {
-	if err := c.Taints.Valid(); err != nil {
-		return err
-	}
-
 	if err := c.NodeDrainer.Valid(); err != nil {
 		return err
 	}

core/controlplane/config/templates/cloud-config-worker

@@ -257,7 +257,7 @@ coreos:
         --rkt-stage1-image=coreos.com/rkt/stage1-coreos \
         {{if .NodeLabels.Enabled}}--node-labels {{.NodeLabels.String}} \
         {{end}}--register-node=true \
-        {{if .Experimental.Taints}}--register-with-taints={{.Experimental.Taints.String}}\
+        {{if .Taints}}--register-with-taints={{.Taints.String}}\
         {{end}}--allow-privileged=true \
         {{if .NodeStatusUpdateFrequency}}--node-status-update-frequency={{.NodeStatusUpdateFrequency}} \
         {{end}}--pod-manifest-path=/etc/kubernetes/manifests \

core/controlplane/config/templates/cluster.yaml

@@ -1245,11 +1245,6 @@ addons:
 #       username: "admin"
 #       userID: "08a8684b-db88-4b73-90a9-3cd1661f5466"
 #
-#   # Kubernetes node taints to be added to controller nodes
-#   taints:
-#     - key: dedicated
-#       value: search
-#       effect: NoSchedule
 #   plugins:
 #     rbac:
 #       enabled: true

core/nodepool/config/config.go

@@ -332,6 +332,10 @@ func (c ProvidedConfig) valid() error {
 		return err
 	}
 
+	if err := c.NodeSettings.Validate(); err != nil {
+		return err
+	}
+
 	clusterNamePlaceholder := "<my-cluster-name>"
 	nestedStackNamePlaceHolder := "<my-nested-stack-name>"
 	replacer := strings.NewReplacer(clusterNamePlaceholder, "", nestedStackNamePlaceHolder, "")

core/root/config/config.go

@@ -91,7 +91,7 @@ func ConfigFromBytes(data []byte) (*Config, error) {
 		if np == nil {
 			return nil, fmt.Errorf("Empty nodepool definition found at index %d", i)
 		}
-		if err := np.Experimental.Taints.Valid(); err != nil {
+		if err := np.Taints.Valid(); err != nil {
 			return nil, fmt.Errorf("invalid taints for node pool at index %d: %v", i, err)
 		}
 

model/controller.go

@@ -38,6 +38,7 @@ func NewDefaultController() Controller {
 		},
 		NodeSettings: NodeSettings{
 			NodeLabels: NodeLabels{},
+			Taints:     Taints{},
 		},
 	}
 }
@@ -80,6 +81,9 @@ func (c Controller) Validate() error {
 	if err := c.IAMConfig.Validate(); err != nil {
 		return err
 	}
+	if len(c.Taints) > 0 {
+		return errors.New("`controller.taints` must not be specified because tainting controller nodes breaks the cluster")
+	}
 	return nil
 }
 

model/node_settings.go

@@ -2,4 +2,12 @@ package model
 
 type NodeSettings struct {
 	NodeLabels NodeLabels `yaml:"nodeLabels"`
+	Taints     Taints     `yaml:"taints"`
+}
+
+func (s NodeSettings) Validate() error {
+	if err := s.Taints.Valid(); err != nil {
+		return err
+	}
+	return nil
 }

test/integration/maincluster_test.go

@@ -131,7 +131,6 @@ func TestMainClusterConfig(t *testing.T) {
 				Enabled:      false,
 				DrainTimeout: 5,
 			},
-			Taints: model.Taints{},
 		}
 
 		actual := c.Experimental
@@ -1194,10 +1193,6 @@ experimental:
   plugins:
     rbac:
       enabled: true
-  taints:
-    - key: reservation
-      value: spot
-      effect: NoSchedule
 cloudWatchLogging:
   enabled: true
 worker:
@@ -1285,9 +1280,6 @@ worker:
 								Enabled: true,
 							},
 						},
-						Taints: model.Taints{
-							{Key: "reservation", Value: "spot", Effect: "NoSchedule"},
-						},
 					}
 
 					actual := c.Experimental
@@ -1398,9 +1390,6 @@ worker:
 							Enabled:      false,
 							DrainTimeout: 0,
 						},
-						Taints: model.Taints{
-							{Key: "reservation", Value: "spot", Effect: "NoSchedule"},
-						},
 					}
 					p := c.NodePools[0]
 					if reflect.DeepEqual(expected, p.Experimental) {
@@ -1410,11 +1399,19 @@ worker:
 					expectedNodeLabels := model.NodeLabels{
 						"kube-aws.coreos.com/role": "worker",
 					}
-
 					actualNodeLabels := c.NodePools[0].NodeLabels
 					if !reflect.DeepEqual(expectedNodeLabels, actualNodeLabels) {
 						t.Errorf("worker node labels didn't match: expected=%v, actual=%v", expectedNodeLabels, actualNodeLabels)
 					}
+
+					expectedTaints := model.Taints{
+						{Key: "reservation", Value: "spot", Effect: "NoSchedule"},
+					}
+					actualTaints := c.NodePools[0].Taints
+					if !reflect.DeepEqual(expectedTaints, actualTaints) {
+						t.Errorf("worker node taints didn't match: expected=%v, actual=%v", expectedTaints, actualTaints)
+					}
+
 				},
 			},
 		},
@@ -3616,6 +3613,17 @@ controller:
 			configYaml:           kubeAwsSettings.withClusterName("my.cluster").minimumValidClusterYaml(),
 			expectedErrorMessage: "clusterName(=my.cluster) is malformed. It must consist only of alphanumeric characters, colons, or hyphens",
 		},
+		{
+			context: "WithControllerTaint",
+			configYaml: minimalValidConfigYaml + `
+controller:
+  taints:
+  - key: foo
+    value: bar
+    effect: NoSchedule
+`,
+			expectedErrorMessage: "`controller.taints` must not be specified because tainting controller nodes breaks the cluster",
+		},
 		{
 			context: "WithElasticFileSystemIdInSpecificNodePoolWithManagedSubnets",
 			configYaml: mainClusterYaml + `