Merge pull request #13266 from olemarkus/validate-taints

Validate taints in IG spec

pkg/apis/kops/util/BUILD.bazel

@@ -4,6 +4,7 @@ go_library(
     name = "go_default_library",
     srcs = [
         "labels.go",
+        "taints.go",
         "versions.go",
     ],
     importpath = "k8s.io/kops/pkg/apis/kops/util",

pkg/apis/kops/util/taints.go

@@ -0,0 +1,58 @@
+/*
+Copyright 2020 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package util
+
+import (
+	"fmt"
+	"strings"
+)
+
+// parseTaint takes a string and returns a map of its value
+// it mimics the function from https://github.com/kubernetes/kubernetes/blob/master/pkg/util/taints/taints.go
+// but returns a map instead of a v1.Taint
+func ParseTaint(st string) (map[string]string, error) {
+	taint := make(map[string]string)
+
+	var key string
+	var value string
+	var effect string
+
+	parts := strings.Split(st, ":")
+	switch len(parts) {
+	case 1:
+		key = parts[0]
+	case 2:
+		effect = parts[1]
+
+		partsKV := strings.Split(parts[0], "=")
+		if len(partsKV) > 2 {
+			return taint, fmt.Errorf("invalid taint spec: %v", st)
+		}
+		key = partsKV[0]
+		if len(partsKV) == 2 {
+			value = partsKV[1]
+		}
+	default:
+		return taint, fmt.Errorf("invalid taint spec: %v", st)
+	}
+
+	taint["key"] = key
+	taint["value"] = value
+	taint["effect"] = effect
+
+	return taint, nil
+}

pkg/apis/kops/validation/instancegroup.go

@@ -24,9 +24,11 @@ import (
 
 	"github.com/aws/aws-sdk-go/aws/arn"
 	"github.com/aws/aws-sdk-go/service/ec2"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 
 	"k8s.io/kops/pkg/apis/kops"
+	"k8s.io/kops/pkg/apis/kops/util"
 	"k8s.io/kops/upup/pkg/fi"
 	"k8s.io/kops/upup/pkg/fi/cloudup/awsup"
 )
@@ -157,6 +159,21 @@ func ValidateInstanceGroup(g *kops.InstanceGroup, cloud fi.Cloud, strict bool) f
 
 	allErrs = append(allErrs, IsValidValue(field.NewPath("spec", "updatePolicy"), g.Spec.UpdatePolicy, []string{kops.UpdatePolicyAutomatic, kops.UpdatePolicyExternal})...)
 
+	taintKeys := sets.NewString()
+	for i, taint := range g.Spec.Taints {
+		path := field.NewPath("spec", "taints").Index(i)
+		parts, err := util.ParseTaint(taint)
+		if err != nil {
+			allErrs = append(allErrs, field.Invalid(path, taint, "invalid taint value"))
+		}
+		key := parts["key"]
+		if taintKeys.Has(key) {
+			allErrs = append(allErrs, field.Forbidden(path, fmt.Sprintf("cannot add multiple taints with key %q", key)))
+		} else {
+			taintKeys.Insert(key)
+		}
+	}
+
 	return allErrs
 }
 

pkg/apis/kops/validation/instancegroup_test.go

@@ -307,6 +307,34 @@ func TestIGCloudLabelIsIGName(t *testing.T) {
 	}
 }
 
+func TestValidTaints(t *testing.T) {
+	grid := []struct {
+		taints   []string
+		expected []string
+	}{
+		{
+			taints: []string{
+				"nvidia.com/gpu:NoSchedule",
+			},
+		},
+		{
+			taints: []string{
+				"nvidia.com/gpu:NoSchedule",
+				"nvidia.com/gpu=1:NoSchedule",
+			},
+			expected: []string{"Forbidden::spec.taints[1]"},
+		},
+	}
+
+	for _, g := range grid {
+		ig := createMinimalInstanceGroup()
+
+		ig.Spec.Taints = g.taints
+		errs := ValidateInstanceGroup(ig, nil, true)
+		testErrors(t, g.taints, errs, g.expected)
+	}
+}
+
 func TestIGUpdatePolicy(t *testing.T) {
 	const unsupportedValueError = "Unsupported value::spec.updatePolicy"
 	for _, test := range []struct {

pkg/testutils/cluster.go

@@ -33,6 +33,9 @@ func BuildMinimalCluster(clusterName string) *kops.Cluster {
 		{Name: "subnet-us-mock-1a", Zone: "us-mock-1a", CIDR: "172.20.1.0/24", Type: kops.SubnetTypePrivate},
 	}
 
+	c.Spec.ContainerRuntime = "containerd"
+	c.Spec.Containerd = &kops.ContainerdConfig{}
+
 	c.Spec.MasterPublicName = fmt.Sprintf("api.%v", clusterName)
 	c.Spec.MasterInternalName = fmt.Sprintf("internal.api.%v", clusterName)
 	c.Spec.KubernetesAPIAccess = []string{"0.0.0.0/0"}

upup/pkg/fi/cloudup/populate_instancegroup_spec.go

@@ -18,6 +18,7 @@ package cloudup
 
 import (
 	"fmt"
+	"strings"
 
 	"github.com/aws/aws-sdk-go/service/ec2"
 	"github.com/blang/semver/v4"
@@ -186,7 +187,15 @@ func PopulateInstanceGroupSpec(cluster *kops.Cluster, input *kops.InstanceGroup,
 					ig.Spec.NodeLabels = make(map[string]string)
 				}
 				ig.Spec.NodeLabels["kops.k8s.io/gpu"] = "1"
-				ig.Spec.Taints = append(ig.Spec.Taints, "nvidia.com/gpu:NoSchedule")
+				hasNvidiaTaint := false
+				for _, taint := range ig.Spec.Taints {
+					if strings.HasPrefix(taint, "nvidia.com/gpu") {
+						hasNvidiaTaint = true
+					}
+				}
+				if !hasNvidiaTaint {
+					ig.Spec.Taints = append(ig.Spec.Taints, "nvidia.com/gpu:NoSchedule")
+				}
 			}
 		}
 	}

upup/pkg/fi/cloudup/populate_instancegroup_spec_test.go

@@ -80,6 +80,49 @@ func TestPopulateInstanceGroup_Image_Required(t *testing.T) {
 	expectErrorFromPopulateInstanceGroup(t, cluster, g, channel, "unable to determine default image for InstanceGroup nodes")
 }
 
+func TestPopulateInstanceGroup_AddTaintsCollision(t *testing.T) {
+	_, cluster := buildMinimalCluster()
+	input := buildMinimalNodeInstanceGroup()
+	input.Spec.Taints = []string{"nvidia.com/gpu:NoSchedule"}
+	input.Spec.MachineType = "g4dn.xlarge"
+	cluster.Spec.Containerd.NvidiaGPU = &kopsapi.NvidiaGPUConfig{Enabled: fi.Bool(true)}
+
+	channel := &kopsapi.Channel{}
+
+	cloud, err := BuildCloud(cluster)
+	if err != nil {
+		t.Fatalf("error from BuildCloud: %v", err)
+	}
+	output, err := PopulateInstanceGroupSpec(cluster, input, cloud, channel)
+	if err != nil {
+		t.Fatalf("error from PopulateInstanceGropuSpec: %v", err)
+	}
+	if len(output.Spec.Taints) != 1 {
+		t.Errorf("Expected only 1 taint, got %d", len(output.Spec.Taints))
+	}
+}
+
+func TestPopulateInstanceGroup_AddTaints(t *testing.T) {
+	_, cluster := buildMinimalCluster()
+	input := buildMinimalNodeInstanceGroup()
+	input.Spec.MachineType = "g4dn.xlarge"
+	cluster.Spec.Containerd.NvidiaGPU = &kopsapi.NvidiaGPUConfig{Enabled: fi.Bool(true)}
+
+	channel := &kopsapi.Channel{}
+
+	cloud, err := BuildCloud(cluster)
+	if err != nil {
+		t.Fatalf("error from BuildCloud: %v", err)
+	}
+	output, err := PopulateInstanceGroupSpec(cluster, input, cloud, channel)
+	if err != nil {
+		t.Fatalf("error from PopulateInstanceGropuSpec: %v", err)
+	}
+	if len(output.Spec.Taints) != 1 {
+		t.Errorf("Expected only 1 taint, got %d", len(output.Spec.Taints))
+	}
+}
+
 func expectErrorFromPopulateInstanceGroup(t *testing.T, cluster *kopsapi.Cluster, g *kopsapi.InstanceGroup, channel *kopsapi.Channel, message string) {
 	cloud, err := BuildCloud(cluster)
 	if err != nil {

upup/pkg/fi/cloudup/template_functions.go

@@ -293,7 +293,7 @@ func (tf *TemplateFunctions) AddTo(dest template.FuncMap, secretStore fi.SecretS
 
 	dest["ArchitectureOfAMI"] = tf.architectureOfAMI
 
-	dest["ParseTaint"] = parseTaint
+	dest["ParseTaint"] = util.ParseTaint
 
 	dest["UsesInstanceIDForNodeName"] = func() bool {
 		return nodeup.UsesInstanceIDForNodeName(tf.Cluster)
@@ -763,42 +763,6 @@ func (tf *TemplateFunctions) architectureOfAMI(amiID string) string {
 	return "arm64"
 }
 
-// parseTaint takes a string and returns a map of its value
-// it mimics the function from https://github.com/kubernetes/kubernetes/blob/master/pkg/util/taints/taints.go
-// but returns a map instead of a v1.Taint
-func parseTaint(st string) (map[string]string, error) {
-	taint := make(map[string]string)
-
-	var key string
-	var value string
-	var effect string
-
-	parts := strings.Split(st, ":")
-	switch len(parts) {
-	case 1:
-		key = parts[0]
-	case 2:
-		effect = parts[1]
-
-		partsKV := strings.Split(parts[0], "=")
-		if len(partsKV) > 2 {
-			return taint, fmt.Errorf("invalid taint spec: %v", st)
-		}
-		key = partsKV[0]
-		if len(partsKV) == 2 {
-			value = partsKV[1]
-		}
-	default:
-		return taint, fmt.Errorf("invalid taint spec: %v", st)
-	}
-
-	taint["key"] = key
-	taint["value"] = value
-	taint["effect"] = effect
-
-	return taint, nil
-}
-
 func karpenterInstanceTypes(cloud awsup.AWSCloud, ig kops.InstanceGroupSpec) ([]string, error) {
 	var mixedInstancesPolicy *kops.MixedInstancesPolicySpec