From 7d623d351c4c7c5f84c8328c69e962fa6381804a Mon Sep 17 00:00:00 2001 From: Liel Chayoun Date: Tue, 6 Aug 2019 10:59:10 +0300 Subject: [PATCH 1/7] Update funcs.go --- helper/funcs.go | 1 + 1 file changed, 1 insertion(+) diff --git a/helper/funcs.go b/helper/funcs.go index 7a6b4c15173b..2a7a31b543a1 100644 --- a/helper/funcs.go +++ b/helper/funcs.go @@ -347,6 +347,7 @@ func CleanEnvVar(s string, r byte) string { b := []byte(s) for i, c := range b { switch { + case c == '-': case c == '_': case c == '.': case c >= 'a' && c <= 'z': From eb39720fbe9eb0788115c298888f570e7e2fe96b Mon Sep 17 00:00:00 2001 From: Liel Chayoun Date: Tue, 6 Aug 2019 11:00:06 +0300 Subject: [PATCH 2/7] Update funcs_test.go --- helper/funcs_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/helper/funcs_test.go b/helper/funcs_test.go index cff3e9c21cfe..a334ddde8457 100644 --- a/helper/funcs_test.go +++ b/helper/funcs_test.go @@ -135,9 +135,9 @@ func TestClearEnvVar(t *testing.T) { {"0sdf", "_sdf"}, {"asd0", "asd0"}, {"_asd", "_asd"}, - {"-asd", "_asd"}, + {"-asd", "-asd"}, {"asd.fgh", "asd.fgh"}, - {"A~!@#$%^&*()_+-={}[]|\\;:'\"<,>?/Z", "A______________________________Z"}, + {"A~!@#$%^&*()_+={}[]|\\;:'\"<,>?/Z", "A_____________________________Z"}, {"A\U0001f4a9Z", "A____Z"}, } for _, c := range cases { From d11b6509aa0f654e14920f75ffca251bb345cbc0 Mon Sep 17 00:00:00 2001 From: Liel Chayoun Date: Tue, 6 Aug 2019 11:59:31 +0300 Subject: [PATCH 3/7] Update env_test.go --- client/taskenv/env_test.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/client/taskenv/env_test.go b/client/taskenv/env_test.go index 531d8f168400..fc102944b8bc 100644 --- a/client/taskenv/env_test.go +++ b/client/taskenv/env_test.go @@ -650,17 +650,16 @@ func TestEnvironment_AppendHostEnvvars(t *testing.T) { } } -// TestEnvironment_DashesInTaskName asserts dashes in port labels are properly -// converted to underscores in environment variables. -// See: https://github.com/hashicorp/nomad/issues/2405 +// TestEnvironment_DashesInTaskName asserts dashes are kept in environment variables. +// See: https://github.com/hashicorp/nomad/issues/6079 func TestEnvironment_DashesInTaskName(t *testing.T) { a := mock.Alloc() task := a.Job.TaskGroups[0].Tasks[0] task.Env = map[string]string{"test-one-two": "three-four"} envMap := NewBuilder(mock.Node(), a, task, "global").Build().Map() - if envMap["test_one_two"] != "three-four" { - t.Fatalf("Expected test_one_two=three-four in TaskEnv; found:\n%#v", envMap) + if envMap["test-one-two"] != "three-four" { + t.Fatalf("Expected test-one-two=three-four in TaskEnv; found:\n%#v", envMap) } } From 9076cb95b41f52f129b2e256879d407ae803bfd5 Mon Sep 17 00:00:00 2001 From: lchayoun Date: Sun, 11 Aug 2019 12:51:42 +0300 Subject: [PATCH 4/7] allow dash in non generated environment variable names --- client/taskenv/env.go | 14 +++++++++++-- client/taskenv/env_test.go | 13 +++++++++--- helper/funcs.go | 6 ++++-- helper/funcs_test.go | 43 ++++++++++++++++++++++++++++++++++---- 4 files changed, 65 insertions(+), 11 deletions(-) diff --git a/client/taskenv/env.go b/client/taskenv/env.go index 5b42a17a7533..30c88f1b8e79 100644 --- a/client/taskenv/env.go +++ b/client/taskenv/env.go @@ -476,7 +476,12 @@ func (b *Builder) Build() *TaskEnv { // Clean keys (see #2405) cleanedEnv := make(map[string]string, len(envMap)) for k, v := range envMap { - cleanedK := helper.CleanEnvVar(k, '_') + var cleanedK string + if strings.HasPrefix(k, "NOMAD_") { + cleanedK = helper.CleanEnvVar(k, '_', "") + } else { + cleanedK = helper.CleanEnvVar(k, '_', "-") + } cleanedEnv[cleanedK] = v } @@ -484,7 +489,12 @@ func (b *Builder) Build() *TaskEnv { if deviceEnvs != nil { cleanedDeviceEnvs = make(map[string]string, len(deviceEnvs)) for k, v := range deviceEnvs { - cleanedK := helper.CleanEnvVar(k, '_') + var cleanedK string + if strings.HasPrefix(k, "NOMAD_") { + cleanedK = helper.CleanEnvVar(k, '_', "") + } else { + cleanedK = helper.CleanEnvVar(k, '_', "-") + } cleanedDeviceEnvs[cleanedK] = v } } diff --git a/client/taskenv/env_test.go b/client/taskenv/env_test.go index fc102944b8bc..faadec1d3f09 100644 --- a/client/taskenv/env_test.go +++ b/client/taskenv/env_test.go @@ -650,17 +650,24 @@ func TestEnvironment_AppendHostEnvvars(t *testing.T) { } } -// TestEnvironment_DashesInTaskName asserts dashes are kept in environment variables. -// See: https://github.com/hashicorp/nomad/issues/6079 +// TestEnvironment_DashesInTaskName asserts dashes in port labels are properly +// converted to underscores in environment variables. +// See: https://github.com/hashicorp/nomad/issues/2405 func TestEnvironment_DashesInTaskName(t *testing.T) { a := mock.Alloc() task := a.Job.TaskGroups[0].Tasks[0] - task.Env = map[string]string{"test-one-two": "three-four"} + task.Env = map[string]string{ + "test-one-two": "three-four", + "NOMAD_test_one_two": "three-five", + } envMap := NewBuilder(mock.Node(), a, task, "global").Build().Map() if envMap["test-one-two"] != "three-four" { t.Fatalf("Expected test-one-two=three-four in TaskEnv; found:\n%#v", envMap) } + if envMap["NOMAD_test_one_two"] != "three-five" { + t.Fatalf("Expected NOMAD_test_one_two=three-five in TaskEnv; found:\n%#v", envMap) + } } // TestEnvironment_UpdateTask asserts env vars and task meta are updated when a diff --git a/helper/funcs.go b/helper/funcs.go index 2a7a31b543a1..f17eb05c4c95 100644 --- a/helper/funcs.go +++ b/helper/funcs.go @@ -1,6 +1,7 @@ package helper import ( + "bytes" "crypto/sha512" "fmt" "regexp" @@ -343,11 +344,12 @@ func CopySliceInt(s []int) []int { // CleanEnvVar replaces all occurrences of illegal characters in an environment // variable with the specified byte. -func CleanEnvVar(s string, r byte) string { +// any extra characters will be considered valid +func CleanEnvVar(s string, r byte, e string) string { b := []byte(s) for i, c := range b { switch { - case c == '-': + case bytes.ContainsAny([]byte{c}, e): case c == '_': case c == '.': case c >= 'a' && c <= 'z': diff --git a/helper/funcs_test.go b/helper/funcs_test.go index a334ddde8457..8a16cc1d1e93 100644 --- a/helper/funcs_test.go +++ b/helper/funcs_test.go @@ -135,13 +135,14 @@ func TestClearEnvVar(t *testing.T) { {"0sdf", "_sdf"}, {"asd0", "asd0"}, {"_asd", "_asd"}, - {"-asd", "-asd"}, + {"-asd", "_asd"}, + {"NOMAD_aaa-asd", "NOMAD_aaa_asd"}, {"asd.fgh", "asd.fgh"}, - {"A~!@#$%^&*()_+={}[]|\\;:'\"<,>?/Z", "A_____________________________Z"}, + {"A~!@#$%^&*()_+-={}[]|\\;:'\"<,>?/Z", "A______________________________Z"}, {"A\U0001f4a9Z", "A____Z"}, } for _, c := range cases { - if output := CleanEnvVar(c.input, '_'); output != c.expected { + if output := CleanEnvVar(c.input, '_', ""); output != c.expected { t.Errorf("CleanEnvVar(%q, '_') -> %q != %q", c.input, output, c.expected) } } @@ -154,6 +155,40 @@ func BenchmarkCleanEnvVar(b *testing.B) { b.ReportAllocs() b.ResetTimer() for i := 0; i < b.N; i++ { - CleanEnvVar(in, replacement) + CleanEnvVar(in, replacement, "") + } +} + +func TestClearEnvVarWithExtraChars(t *testing.T) { + type testCase struct { + input string + expected string + } + cases := []testCase{ + {"asdf", "asdf"}, + {"ASDF", "ASDF"}, + {"0sdf", "_sdf"}, + {"asd0", "asd0"}, + {"_asd", "_asd"}, + {"-asd", "-asd"}, + {"asd.fgh", "asd.fgh"}, + {"A~!@#$%^&*()_+-={}[]|\\;:'\"<,>?/Z", "A_____________-________________Z"}, + {"A\U0001f4a9Z", "A____Z"}, + } + for _, c := range cases { + if output := CleanEnvVar(c.input, '_', "-"); output != c.expected { + t.Errorf("CleanEnvVar(%q, '_') -> %q != %q", c.input, output, c.expected) + } + } +} + +func BenchmarkCleanEnvVarWithExtraChars(b *testing.B) { + in := "NOMAD_ADDR_redis-cache" + replacement := byte('_') + b.SetBytes(int64(len(in))) + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + CleanEnvVar(in, replacement, "-") } } From 9ba97e21721850750749c2885f6fb8d2dd058123 Mon Sep 17 00:00:00 2001 From: lchayoun Date: Tue, 13 Aug 2019 08:49:38 +0300 Subject: [PATCH 5/7] allow dash in non generated environment variable names --- helper/funcs.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/helper/funcs.go b/helper/funcs.go index f17eb05c4c95..223c9f6e81b8 100644 --- a/helper/funcs.go +++ b/helper/funcs.go @@ -345,11 +345,11 @@ func CopySliceInt(s []int) []int { // CleanEnvVar replaces all occurrences of illegal characters in an environment // variable with the specified byte. // any extra characters will be considered valid -func CleanEnvVar(s string, r byte, e string) string { - b := []byte(s) +func CleanEnvVar(str string, replacement byte, extra string) string { + b := []byte(str) for i, c := range b { switch { - case bytes.ContainsAny([]byte{c}, e): + case bytes.ContainsAny([]byte{c}, extra): case c == '_': case c == '.': case c >= 'a' && c <= 'z': @@ -357,7 +357,7 @@ func CleanEnvVar(s string, r byte, e string) string { case i > 0 && c >= '0' && c <= '9': default: // Replace! - b[i] = r + b[i] = replacement } } return string(b) From 4d709ee3b41cd9009a49fad0f970a505a1a9e875 Mon Sep 17 00:00:00 2001 From: lchayoun Date: Tue, 13 Aug 2019 19:23:13 +0300 Subject: [PATCH 6/7] allow dash in non generated environment variable names - should only clean generate environment variables --- client/taskenv/env.go | 8 ++++---- helper/funcs.go | 9 +++------ helper/funcs_test.go | 39 ++------------------------------------- 3 files changed, 9 insertions(+), 47 deletions(-) diff --git a/client/taskenv/env.go b/client/taskenv/env.go index 30c88f1b8e79..f0b3381aec64 100644 --- a/client/taskenv/env.go +++ b/client/taskenv/env.go @@ -478,9 +478,9 @@ func (b *Builder) Build() *TaskEnv { for k, v := range envMap { var cleanedK string if strings.HasPrefix(k, "NOMAD_") { - cleanedK = helper.CleanEnvVar(k, '_', "") + cleanedK = helper.CleanEnvVar(k, '_') } else { - cleanedK = helper.CleanEnvVar(k, '_', "-") + cleanedK = k } cleanedEnv[cleanedK] = v } @@ -491,9 +491,9 @@ func (b *Builder) Build() *TaskEnv { for k, v := range deviceEnvs { var cleanedK string if strings.HasPrefix(k, "NOMAD_") { - cleanedK = helper.CleanEnvVar(k, '_', "") + cleanedK = helper.CleanEnvVar(k, '_') } else { - cleanedK = helper.CleanEnvVar(k, '_', "-") + cleanedK = k } cleanedDeviceEnvs[cleanedK] = v } diff --git a/helper/funcs.go b/helper/funcs.go index 223c9f6e81b8..7a6b4c15173b 100644 --- a/helper/funcs.go +++ b/helper/funcs.go @@ -1,7 +1,6 @@ package helper import ( - "bytes" "crypto/sha512" "fmt" "regexp" @@ -344,12 +343,10 @@ func CopySliceInt(s []int) []int { // CleanEnvVar replaces all occurrences of illegal characters in an environment // variable with the specified byte. -// any extra characters will be considered valid -func CleanEnvVar(str string, replacement byte, extra string) string { - b := []byte(str) +func CleanEnvVar(s string, r byte) string { + b := []byte(s) for i, c := range b { switch { - case bytes.ContainsAny([]byte{c}, extra): case c == '_': case c == '.': case c >= 'a' && c <= 'z': @@ -357,7 +354,7 @@ func CleanEnvVar(str string, replacement byte, extra string) string { case i > 0 && c >= '0' && c <= '9': default: // Replace! - b[i] = replacement + b[i] = r } } return string(b) diff --git a/helper/funcs_test.go b/helper/funcs_test.go index 8a16cc1d1e93..cff3e9c21cfe 100644 --- a/helper/funcs_test.go +++ b/helper/funcs_test.go @@ -136,13 +136,12 @@ func TestClearEnvVar(t *testing.T) { {"asd0", "asd0"}, {"_asd", "_asd"}, {"-asd", "_asd"}, - {"NOMAD_aaa-asd", "NOMAD_aaa_asd"}, {"asd.fgh", "asd.fgh"}, {"A~!@#$%^&*()_+-={}[]|\\;:'\"<,>?/Z", "A______________________________Z"}, {"A\U0001f4a9Z", "A____Z"}, } for _, c := range cases { - if output := CleanEnvVar(c.input, '_', ""); output != c.expected { + if output := CleanEnvVar(c.input, '_'); output != c.expected { t.Errorf("CleanEnvVar(%q, '_') -> %q != %q", c.input, output, c.expected) } } @@ -155,40 +154,6 @@ func BenchmarkCleanEnvVar(b *testing.B) { b.ReportAllocs() b.ResetTimer() for i := 0; i < b.N; i++ { - CleanEnvVar(in, replacement, "") - } -} - -func TestClearEnvVarWithExtraChars(t *testing.T) { - type testCase struct { - input string - expected string - } - cases := []testCase{ - {"asdf", "asdf"}, - {"ASDF", "ASDF"}, - {"0sdf", "_sdf"}, - {"asd0", "asd0"}, - {"_asd", "_asd"}, - {"-asd", "-asd"}, - {"asd.fgh", "asd.fgh"}, - {"A~!@#$%^&*()_+-={}[]|\\;:'\"<,>?/Z", "A_____________-________________Z"}, - {"A\U0001f4a9Z", "A____Z"}, - } - for _, c := range cases { - if output := CleanEnvVar(c.input, '_', "-"); output != c.expected { - t.Errorf("CleanEnvVar(%q, '_') -> %q != %q", c.input, output, c.expected) - } - } -} - -func BenchmarkCleanEnvVarWithExtraChars(b *testing.B) { - in := "NOMAD_ADDR_redis-cache" - replacement := byte('_') - b.SetBytes(int64(len(in))) - b.ReportAllocs() - b.ResetTimer() - for i := 0; i < b.N; i++ { - CleanEnvVar(in, replacement, "-") + CleanEnvVar(in, replacement) } } From 4244265a9d654abe094c38e5b0453e29e9b638ba Mon Sep 17 00:00:00 2001 From: lchayoun Date: Fri, 16 Aug 2019 11:11:47 +0300 Subject: [PATCH 7/7] allow dash in non generated environment variable names - should only clean generate environment variables --- client/taskenv/env.go | 25 ++++++------------------- 1 file changed, 6 insertions(+), 19 deletions(-) diff --git a/client/taskenv/env.go b/client/taskenv/env.go index f0b3381aec64..d0a1f7468f51 100644 --- a/client/taskenv/env.go +++ b/client/taskenv/env.go @@ -474,32 +474,19 @@ func (b *Builder) Build() *TaskEnv { } // Clean keys (see #2405) + prefixesToClean := [...]string{AddrPrefix, IpPrefix, PortPrefix, HostPortPrefix, MetaPrefix} cleanedEnv := make(map[string]string, len(envMap)) for k, v := range envMap { - var cleanedK string - if strings.HasPrefix(k, "NOMAD_") { - cleanedK = helper.CleanEnvVar(k, '_') - } else { - cleanedK = k - } - cleanedEnv[cleanedK] = v - } - - var cleanedDeviceEnvs map[string]string - if deviceEnvs != nil { - cleanedDeviceEnvs = make(map[string]string, len(deviceEnvs)) - for k, v := range deviceEnvs { - var cleanedK string - if strings.HasPrefix(k, "NOMAD_") { + cleanedK := k + for i := range prefixesToClean { + if strings.HasPrefix(k, prefixesToClean[i]) { cleanedK = helper.CleanEnvVar(k, '_') - } else { - cleanedK = k } - cleanedDeviceEnvs[cleanedK] = v } + cleanedEnv[cleanedK] = v } - return NewTaskEnv(cleanedEnv, cleanedDeviceEnvs, nodeAttrs) + return NewTaskEnv(cleanedEnv, deviceEnvs, nodeAttrs) } // Update task updates the environment based on a new alloc and task.