From 94ce57e57f603e9e5ccc11036d4defa38534f182 Mon Sep 17 00:00:00 2001
From: Jan Janik <11janci@seznam.cz>
Date: Wed, 27 Mar 2019 22:05:20 +1300
Subject: [PATCH] =?UTF-8?q?Escape=20systemd=20special=20chars=20in=20?=
 =?UTF-8?q?=E2=80=94docker-env?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

---
 pkg/provision/buildroot.go                 | 15 ++++++++++++
 pkg/util/utils.go                          |  9 ++++++++
 pkg/util/utils_test.go                     | 19 +++++++++++++++
 test/integration/start_stop_delete_test.go | 27 +++++++++++++++-------
 4 files changed, 62 insertions(+), 8 deletions(-)

diff --git a/pkg/provision/buildroot.go b/pkg/provision/buildroot.go
index 50c701580da2..3e8c93edaa66 100644
--- a/pkg/provision/buildroot.go
+++ b/pkg/provision/buildroot.go
@@ -21,6 +21,7 @@ import (
 	"fmt"
 	"path"
 	"path/filepath"
+	"strings"
 	"text/template"
 	"time"
 
@@ -47,6 +48,9 @@ type BuildrootProvisioner struct {
 	provision.SystemdProvisioner
 }
 
+// for escaping systemd template specifiers (e.g. '%i'), which are not supported by minikube
+var systemdSpecifierEscaper = strings.NewReplacer("%", "%%")
+
 func init() {
 	provision.Register("Buildroot", &provision.RegisteredProvisioner{
 		New: NewBuildrootProvisioner,
@@ -64,6 +68,15 @@ func (p *BuildrootProvisioner) String() string {
 	return "buildroot"
 }
 
+// escapeSystemdDirectives escapes special characters in the input variables used to create the
+// systemd unit file, which would otherwise be interpreted as systemd directives. An example
+// are template specifiers (e.g. '%i') which are predefined variables that get evaluated dynamically
+// (see systemd man pages for more info). This is not supported by minikube, thus needs to be escaped.
+func escapeSystemdDirectives(engineConfigContext provision.EngineConfigContext) {
+	// escape '%' in Environment option so that it does not evaluate into a template specifier
+	engineConfigContext.EngineOptions.Env = util.ReplaceChars(engineConfigContext.EngineOptions.Env, systemdSpecifierEscaper)
+}
+
 // GenerateDockerOptions generates the *provision.DockerOptions for this provisioner
 func (p *BuildrootProvisioner) GenerateDockerOptions(dockerPort int) (*provision.DockerOptions, error) {
 	var engineCfg bytes.Buffer
@@ -127,6 +140,8 @@ WantedBy=multi-user.target
 		EngineOptions: p.EngineOptions,
 	}
 
+	escapeSystemdDirectives(engineConfigContext)
+
 	if err := t.Execute(&engineCfg, engineConfigContext); err != nil {
 		return nil, err
 	}
diff --git a/pkg/util/utils.go b/pkg/util/utils.go
index 7637e2a588b3..236dfacb2650 100644
--- a/pkg/util/utils.go
+++ b/pkg/util/utils.go
@@ -250,3 +250,12 @@ func TeePrefix(prefix string, r io.Reader, w io.Writer, logger func(format strin
 	}
 	return nil
 }
+
+// ReplaceChars returns a copy of the src slice with each string modified by the replacer
+func ReplaceChars(src []string, replacer *strings.Replacer) []string {
+	ret := make([]string, len(src))
+	for i, s := range src {
+		ret[i] = replacer.Replace(s)
+	}
+	return ret
+}
diff --git a/pkg/util/utils_test.go b/pkg/util/utils_test.go
index f7d1c7c372fe..377d1b567d97 100644
--- a/pkg/util/utils_test.go
+++ b/pkg/util/utils_test.go
@@ -197,3 +197,22 @@ func TestTeePrefix(t *testing.T) {
 		t.Errorf("log=%q, want: %q", gotLog, wantLog)
 	}
 }
+
+func TestReplaceChars(t *testing.T) {
+	testData := []struct {
+		src         []string
+		replacer    *strings.Replacer
+		expectedRes []string
+	}{
+		{[]string{"abc%def", "%Y%"}, strings.NewReplacer("%", "X"), []string{"abcXdef", "XYX"}},
+	}
+
+	for _, tt := range testData {
+		res := ReplaceChars(tt.src, tt.replacer)
+		for i, val := range res {
+			if val != tt.expectedRes[i] {
+				t.Fatalf("Expected '%s' but got '%s'", tt.expectedRes, res)
+			}
+		}
+	}
+}
diff --git a/test/integration/start_stop_delete_test.go b/test/integration/start_stop_delete_test.go
index 839283654d68..4f43d35db355 100644
--- a/test/integration/start_stop_delete_test.go
+++ b/test/integration/start_stop_delete_test.go
@@ -19,42 +19,44 @@ limitations under the License.
 package integration
 
 import (
-	"fmt"
 	"net"
 	"strings"
 	"testing"
 	"time"
 
 	"github.com/docker/machine/libmachine/state"
-	"k8s.io/minikube/pkg/minikube/constants"
 	"k8s.io/minikube/test/integration/util"
 )
 
 func TestStartStop(t *testing.T) {
 	tests := []struct {
-		name string
-		args []string
+		name         string
+		args         []string
+		assertCustom func(t *testing.T, r *util.MinikubeRunner)
 	}{
 		{"nocache_oldest", []string{
 			"--cache-images=false",
 			fmt.Sprintf("--kubernetes-version=%s", constants.OldestKubernetesVersion),
-		}},
+		}, null},
 		{"feature_gates_newest_cni", []string{
 			"--feature-gates",
 			"ServerSideApply=true",
 			"--network-plugin=cni",
 			"--extra-config=kubelet.network-plugin=cni",
 			fmt.Sprintf("--kubernetes-version=%s", constants.NewestKubernetesVersion),
-		}},
+		}, null},
 		{"containerd", []string{
 			"--container-runtime=containerd",
 			"--docker-opt containerd=/var/run/containerd/containerd.sock",
-		}},
+		}, null},
 		{"crio_ignore_preflights", []string{
 			"--container-runtime=crio",
 			"--extra-config",
 			"kubeadm.ignore-preflight-errors=SystemVerification",
-		}},
+		}, null},
+		{"docker_env_special_char", []string{
+			"--docker-env TEST_VAR=encoded%40space",
+		}, assertDockerEnv},
 	}
 
 	for _, test := range tests {
@@ -70,6 +72,10 @@ func TestStartStop(t *testing.T) {
 			r.Start(test.args...)
 			r.CheckStatus(state.Running.String())
 
+			if test.assertCustom != nil {
+				test.assertCustom(t, &r)
+			}
+
 			ip := r.RunCommand("ip", true)
 			ip = strings.TrimRight(ip, "\n")
 			if net.ParseIP(ip) == nil {
@@ -93,3 +99,8 @@ func TestStartStop(t *testing.T) {
 		})
 	}
 }
+
+func assertDockerEnv(t *testing.T, r *util.MinikubeRunner) {
+	out, _ := r.SSH("systemctl show --property=Environment docker")
+	t.Fatalf("========> %s", out)
+}