Fix tolerations not parsing its options correctly, add tests

Signed-off-by: Zsolt <zsolt.szeberenyi@figured.com>

commands/create_test.go

@@ -0,0 +1,26 @@
+package commands
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestCsvToMap(t *testing.T) {
+	d := []string{
+		"\"tolerations=key=foo,value=bar;key=foo2,value=bar2\",replicas=1",
+		"namespace=default",
+	}
+	r, err := csvToMap(d)
+
+	require.NoError(t, err)
+
+	require.Contains(t, r, "tolerations")
+	require.Equal(t, r["tolerations"], "key=foo,value=bar;key=foo2,value=bar2")
+
+	require.Contains(t, r, "replicas")
+	require.Equal(t, r["replicas"], "1")
+
+	require.Contains(t, r, "namespace")
+	require.Equal(t, r["namespace"], "default")
+}

docs/reference/buildx_create.md

@@ -139,13 +139,16 @@ Passes additional driver-specific options. Details for each driver:
   - `requests.memory` - Sets the request memory value specified in bytes or with a valid suffix. Example `requests.memory=500Mi`, `requests.memory=4G`
   - `limits.cpu` - Sets the limit CPU value specified in units of Kubernetes CPU. Example `limits.cpu=100m`, `limits.cpu=2`
   - `limits.memory` - Sets the limit memory value specified in bytes or with a valid suffix. Example `limits.memory=500Mi`, `limits.memory=4G`
-  - `nodeselector="label1=value1,label2=value2"` - Sets the kv of `Pod` nodeSelector. No Defaults. Example `nodeselector=kubernetes.io/arch=arm64`
+  - `"nodeselector=label1=value1,label2=value2"` - Sets the kv of `Pod` nodeSelector. No Defaults. Example `nodeselector=kubernetes.io/arch=arm64`
-  - `tolerations="key=foo,value=bar;key=foo2,operator=exists;key=foo3,effect=NoSchedule"` - Sets the `Pod` tolerations. Accepts the same values as the kube manifest tolerations. Key-value pairs are separated by `,`, tolerations are separated by `;`. No Defaults. Example `tolerations=operator=exists`
+  - `"tolerations=key=foo,value=bar;key=foo2,operator=exists;key=foo3,effect=NoSchedule"` - Sets the `Pod` tolerations. Accepts the same values as the kube manifest tolerations. Key-value pairs are separated by `,`, tolerations are separated by `;`. No Defaults. Example `tolerations=operator=exists`
   - `rootless=(true|false)` - Run the container as a non-root user without `securityContext.privileged`. Needs Kubernetes 1.19 or later. [Using Ubuntu host kernel is recommended](https://github.com/moby/buildkit/blob/master/docs/rootless.md). Defaults to false.
   - `loadbalance=(sticky|random)` - Load-balancing strategy. If set to "sticky", the pod is chosen using the hash of the context path. Defaults to "sticky"
   - `qemu.install=(true|false)` - Install QEMU emulation for multi platforms support.
   - `qemu.image=IMAGE` - Sets the QEMU emulation image. Defaults to `tonistiigi/binfmt:latest`
 
+Note: When using quoted values for example for the `nodeselector` or `tolerations` options, ensure that quotes are escaped 
+correctly for your shell.
+
 ### <a name="leave"></a> Remove a node from a builder (--leave)
 
 The `--leave` flag changes the action of the command to remove a node from a

driver/kubernetes/factory.go

@@ -68,6 +68,39 @@ func (f *factory) New(ctx context.Context, cfg driver.InitConfig) (driver.Driver
 		clientset:  clientset,
 	}
 
+	deploymentOpt, loadbalance, namespace, err := f.processDriverOpts(deploymentName, namespace, cfg)
+	if nil != err {
+		return nil, err
+	}
+
+	d.deployment, d.configMaps, err = manifest.NewDeployment(deploymentOpt)
+	if err != nil {
+		return nil, err
+	}
+
+	d.minReplicas = deploymentOpt.Replicas
+
+	d.deploymentClient = clientset.AppsV1().Deployments(namespace)
+	d.podClient = clientset.CoreV1().Pods(namespace)
+	d.configMapClient = clientset.CoreV1().ConfigMaps(namespace)
+
+	switch loadbalance {
+	case LoadbalanceSticky:
+		d.podChooser = &podchooser.StickyPodChooser{
+			Key:        cfg.ContextPathHash,
+			PodClient:  d.podClient,
+			Deployment: d.deployment,
+		}
+	case LoadbalanceRandom:
+		d.podChooser = &podchooser.RandomPodChooser{
+			PodClient:  d.podClient,
+			Deployment: d.deployment,
+		}
+	}
+	return d, nil
+}
+
+func (f *factory) processDriverOpts(deploymentName string, namespace string, cfg driver.InitConfig) (*manifest.DeploymentOpt, string, string, error) {
 	deploymentOpt := &manifest.DeploymentOpt{
 		Name:          deploymentName,
 		Image:         bkimage.DefaultImage,
@@ -81,6 +114,7 @@ func (f *factory) New(ctx context.Context, cfg driver.InitConfig) (driver.Driver
 	deploymentOpt.Qemu.Image = bkimage.QemuImage
 
 	loadbalance := LoadbalanceSticky
+	var err error
 
 	for k, v := range cfg.DriverOpts {
 		switch k {
@@ -93,7 +127,7 @@ func (f *factory) New(ctx context.Context, cfg driver.InitConfig) (driver.Driver
 		case "replicas":
 			deploymentOpt.Replicas, err = strconv.Atoi(v)
 			if err != nil {
-				return nil, err
+				return nil, "", "", err
 			}
 		case "requests.cpu":
 			deploymentOpt.RequestsCPU = v
@@ -106,7 +140,7 @@ func (f *factory) New(ctx context.Context, cfg driver.InitConfig) (driver.Driver
 		case "rootless":
 			deploymentOpt.Rootless, err = strconv.ParseBool(v)
 			if err != nil {
-				return nil, err
+				return nil, "", "", err
 			}
 			if _, isImage := cfg.DriverOpts["image"]; !isImage {
 				deploymentOpt.Image = bkimage.DefaultRootlessImage
@@ -122,17 +156,10 @@ func (f *factory) New(ctx context.Context, cfg driver.InitConfig) (driver.Driver
 			}
 			deploymentOpt.NodeSelector = s
 		case "tolerations":
-			u, err := strconv.Unquote(v)
+			ts := strings.Split(v, ";")
-			if nil != err {
-				return nil, err
-			}
-			ts := strings.Split(u, ";")
 			deploymentOpt.Tolerations = []corev1.Toleration{}
 			for i := range ts {
 				kvs := strings.Split(ts[i], ",")
-				if len(kvs) == 0 {
-					return nil, errors.Errorf("invalid tolaration %q", v)
-				}
 
 				t := corev1.Toleration{}
 
@@ -151,12 +178,12 @@ func (f *factory) New(ctx context.Context, cfg driver.InitConfig) (driver.Driver
 						case "tolerationSeconds":
 							c, err := strconv.Atoi(kv[1])
 							if nil != err {
-								return nil, err
+								return nil, "", "", err
 							}
 							c64 := int64(c)
 							t.TolerationSeconds = &c64
 						default:
-							return nil, errors.Errorf("invalid tolaration %q", v)
+							return nil, "", "", errors.Errorf("invalid tolaration %q", v)
 						}
 					}
 				}
@@ -168,48 +195,24 @@ func (f *factory) New(ctx context.Context, cfg driver.InitConfig) (driver.Driver
 			case LoadbalanceSticky:
 			case LoadbalanceRandom:
 			default:
-				return nil, errors.Errorf("invalid loadbalance %q", v)
+				return nil, "", "", errors.Errorf("invalid loadbalance %q", v)
 			}
 			loadbalance = v
 		case "qemu.install":
 			deploymentOpt.Qemu.Install, err = strconv.ParseBool(v)
 			if err != nil {
-				return nil, err
+				return nil, "", "", err
 			}
 		case "qemu.image":
 			if v != "" {
 				deploymentOpt.Qemu.Image = v
 			}
 		default:
-			return nil, errors.Errorf("invalid driver option %s for driver %s", k, DriverName)
+			return nil, "", "", errors.Errorf("invalid driver option %s for driver %s", k, DriverName)
-		}
 		}
-
-	d.deployment, d.configMaps, err = manifest.NewDeployment(deploymentOpt)
-	if err != nil {
-		return nil, err
 	}
 
-	d.minReplicas = deploymentOpt.Replicas
+	return deploymentOpt, loadbalance, namespace, nil
-
-	d.deploymentClient = clientset.AppsV1().Deployments(namespace)
-	d.podClient = clientset.CoreV1().Pods(namespace)
-	d.configMapClient = clientset.CoreV1().ConfigMaps(namespace)
-
-	switch loadbalance {
-	case LoadbalanceSticky:
-		d.podChooser = &podchooser.StickyPodChooser{
-			Key:        cfg.ContextPathHash,
-			PodClient:  d.podClient,
-			Deployment: d.deployment,
-		}
-	case LoadbalanceRandom:
-		d.podChooser = &podchooser.RandomPodChooser{
-			PodClient:  d.podClient,
-			Deployment: d.deployment,
-		}
-	}
-	return d, nil
 }
 
 func (f *factory) AllowsInstances() bool {

driver/kubernetes/factory_test.go

@@ -0,0 +1,230 @@
+package kubernetes
+
+import (
+	"testing"
+
+	"github.com/docker/buildx/driver"
+	"github.com/docker/buildx/driver/bkimage"
+	"github.com/stretchr/testify/require"
+	v1 "k8s.io/api/core/v1"
+	"k8s.io/client-go/rest"
+)
+
+type mockKubeClientConfig struct {
+	clientConfig *rest.Config
+	namespace    string
+}
+
+func (r *mockKubeClientConfig) ClientConfig() (*rest.Config, error) {
+	return r.clientConfig, nil
+}
+
+func (r *mockKubeClientConfig) Namespace() (string, bool, error) {
+	return r.namespace, true, nil
+}
+
+func TestFactory_processDriverOpts(t *testing.T) {
+	kcc := mockKubeClientConfig{
+		clientConfig: &rest.Config{},
+	}
+
+	cfg := driver.InitConfig{
+		Name:             "buildx_buildkit_test",
+		KubeClientConfig: &kcc,
+	}
+	f := factory{}
+
+	t.Run(
+		"ValidOptions", func(t *testing.T) {
+			cfg.DriverOpts = map[string]string{
+				"namespace":       "test-ns",
+				"image":           "test:latest",
+				"replicas":        "2",
+				"requests.cpu":    "100m",
+				"requests.memory": "32Mi",
+				"limits.cpu":      "200m",
+				"limits.memory":   "64Mi",
+				"rootless":        "true",
+				"nodeselector":    "selector1=value1,selector2=value2",
+				"tolerations":     "key=tolerationKey1,value=tolerationValue1,operator=Equal,effect=NoSchedule,tolerationSeconds=60;key=tolerationKey2,operator=Exists",
+				"loadbalance":     "random",
+				"qemu.install":    "true",
+				"qemu.image":      "qemu:latest",
+			}
+			ns := "test"
+
+			r, loadbalance, ns, err := f.processDriverOpts(cfg.Name, ns, cfg)
+
+			nodeSelectors := map[string]string{
+				"selector1": "value1",
+				"selector2": "value2",
+			}
+
+			ts := int64(60)
+			tolerations := []v1.Toleration{
+				{
+					Key:               "tolerationKey1",
+					Operator:          v1.TolerationOpEqual,
+					Value:             "tolerationValue1",
+					Effect:            v1.TaintEffectNoSchedule,
+					TolerationSeconds: &ts,
+				},
+				{
+					Key:      "tolerationKey2",
+					Operator: v1.TolerationOpExists,
+				},
+			}
+
+			require.NoError(t, err)
+
+			require.Equal(t, "test-ns", ns)
+			require.Equal(t, "test:latest", r.Image)
+			require.Equal(t, 2, r.Replicas)
+			require.Equal(t, "100m", r.RequestsCPU)
+			require.Equal(t, "32Mi", r.RequestsMemory)
+			require.Equal(t, "200m", r.LimitsCPU)
+			require.Equal(t, "64Mi", r.LimitsMemory)
+			require.True(t, r.Rootless)
+			require.Equal(t, nodeSelectors, r.NodeSelector)
+			require.Equal(t, tolerations, r.Tolerations)
+			require.Equal(t, LoadbalanceRandom, loadbalance)
+			require.True(t, r.Qemu.Install)
+			require.Equal(t, "qemu:latest", r.Qemu.Image)
+		},
+	)
+
+	t.Run(
+		"NoOptions", func(t *testing.T) {
+			cfg.DriverOpts = map[string]string{}
+
+			r, loadbalance, ns, err := f.processDriverOpts(cfg.Name, "test", cfg)
+
+			require.NoError(t, err)
+
+			require.Equal(t, "test", ns)
+			require.Equal(t, bkimage.DefaultImage, r.Image)
+			require.Equal(t, 1, r.Replicas)
+			require.Equal(t, "", r.RequestsCPU)
+			require.Equal(t, "", r.RequestsMemory)
+			require.Equal(t, "", r.LimitsCPU)
+			require.Equal(t, "", r.LimitsMemory)
+			require.False(t, r.Rootless)
+			require.Empty(t, r.NodeSelector)
+			require.Empty(t, r.Tolerations)
+			require.Equal(t, LoadbalanceSticky, loadbalance)
+			require.False(t, r.Qemu.Install)
+			require.Equal(t, bkimage.QemuImage, r.Qemu.Image)
+		},
+	)
+
+	t.Run(
+		"RootlessOverride", func(t *testing.T) {
+			cfg.DriverOpts = map[string]string{
+				"rootless":    "true",
+				"loadbalance": "sticky",
+			}
+
+			r, loadbalance, ns, err := f.processDriverOpts(cfg.Name, "test", cfg)
+
+			require.NoError(t, err)
+
+			require.Equal(t, "test", ns)
+			require.Equal(t, bkimage.DefaultRootlessImage, r.Image)
+			require.Equal(t, 1, r.Replicas)
+			require.Equal(t, "", r.RequestsCPU)
+			require.Equal(t, "", r.RequestsMemory)
+			require.Equal(t, "", r.LimitsCPU)
+			require.Equal(t, "", r.LimitsMemory)
+			require.True(t, r.Rootless)
+			require.Empty(t, r.NodeSelector)
+			require.Empty(t, r.Tolerations)
+			require.Equal(t, LoadbalanceSticky, loadbalance)
+			require.False(t, r.Qemu.Install)
+			require.Equal(t, bkimage.QemuImage, r.Qemu.Image)
+		},
+	)
+
+	t.Run(
+		"InvalidReplicas", func(t *testing.T) {
+			cfg.DriverOpts = map[string]string{
+				"replicas": "invalid",
+			}
+
+			_, _, _, err := f.processDriverOpts(cfg.Name, "test", cfg)
+
+			require.Error(t, err)
+		},
+	)
+
+	t.Run(
+		"InvalidRootless", func(t *testing.T) {
+			cfg.DriverOpts = map[string]string{
+				"rootless": "invalid",
+			}
+
+			_, _, _, err := f.processDriverOpts(cfg.Name, "test", cfg)
+
+			require.Error(t, err)
+		},
+	)
+
+	t.Run(
+		"InvalidTolerationKeyword", func(t *testing.T) {
+			cfg.DriverOpts = map[string]string{
+				"tolerations": "key=foo,value=bar,invalid=foo2",
+			}
+
+			_, _, _, err := f.processDriverOpts(cfg.Name, "test", cfg)
+
+			require.Error(t, err)
+		},
+	)
+
+	t.Run(
+		"InvalidTolerationSeconds", func(t *testing.T) {
+			cfg.DriverOpts = map[string]string{
+				"tolerations": "key=foo,value=bar,tolerationSeconds=invalid",
+			}
+
+			_, _, _, err := f.processDriverOpts(cfg.Name, "test", cfg)
+
+			require.Error(t, err)
+		},
+	)
+
+	t.Run(
+		"InvalidLoadBalance", func(t *testing.T) {
+			cfg.DriverOpts = map[string]string{
+				"loadbalance": "invalid",
+			}
+
+			_, _, _, err := f.processDriverOpts(cfg.Name, "test", cfg)
+
+			require.Error(t, err)
+		},
+	)
+
+	t.Run(
+		"InvalidQemuInstall", func(t *testing.T) {
+			cfg.DriverOpts = map[string]string{
+				"qemu.install": "invalid",
+			}
+
+			_, _, _, err := f.processDriverOpts(cfg.Name, "test", cfg)
+
+			require.Error(t, err)
+		},
+	)
+
+	t.Run(
+		"InvalidOption", func(t *testing.T) {
+			cfg.DriverOpts = map[string]string{
+				"invalid": "foo",
+			}
+
+			_, _, _, err := f.processDriverOpts(cfg.Name, "test", cfg)
+
+			require.Error(t, err)
+		},
+	)
+}