Couple fixes for env var replacements and tests

caddy/parse/parsing.go

@@ -69,9 +69,7 @@ func (p *parser) addresses() error {
 	var expectingAnother bool
 
 	for {
-		tkn := p.Val()
-
-		tkn = getValFromEnv(tkn)
+		tkn := replaceEnvVars(p.Val())
 
 		// special case: import directive replaces tokens during parse-time
 		if tkn == "import" && p.isNewLine() {
@@ -243,7 +241,7 @@ func (p *parser) directive() error {
 		} else if p.Val() == "}" && nesting == 0 {
 			return p.Err("Unexpected '}' because no matching opening brace")
 		}
-		p.tokens[p.cursor].text = getValFromEnv(p.tokens[p.cursor].text)
+		p.tokens[p.cursor].text = replaceEnvVars(p.tokens[p.cursor].text)
 		p.block.Tokens[dir] = append(p.block.Tokens[dir], p.tokens[p.cursor])
 	}
 
@@ -306,6 +304,31 @@ func standardAddress(str string) (host, port string, err error) {
 	return
 }
 
+// replaceEnvVars replaces environment variables that appear in the token
+// and understands both the Unix $SYNTAX and Windows %SYNTAX%.
+func replaceEnvVars(s string) string {
+	s = replaceEnvReferences(s, "{%", "%}")
+	s = replaceEnvReferences(s, "{$", "}")
+	return s
+}
+
+// replaceEnvReferences performs the actual replacement of env variables
+// in s, given the placeholder start and placeholder end strings.
+func replaceEnvReferences(s, refStart, refEnd string) string {
+	index := strings.Index(s, refStart)
+	for index != -1 {
+		endIndex := strings.Index(s, refEnd)
+		if endIndex != -1 {
+			ref := s[index : endIndex+len(refEnd)]
+			s = strings.Replace(s, ref, os.Getenv(ref[len(refStart):len(ref)-len(refEnd)]), -1)
+		} else {
+			return s
+		}
+		index = strings.Index(s, refStart)
+	}
+	return s
+}
+
 type (
 	// serverBlock associates tokens with a list of addresses
 	// and groups tokens by directive name.
@@ -330,26 +353,3 @@ func (sb serverBlock) HostList() []string {
 	}
 	return sbHosts
 }
-
-func getValFromEnv(s string) string {
-	s = replaceEnvReferences(s, "{$", "}")
-	s = replaceEnvReferences(s, "{%", "%}")
-	return s
-}
-
-func replaceEnvReferences(s, refStart, refEnd string) string {
-	index := strings.Index(s, refStart)
-	for index != -1 {
-		endIndex := strings.Index(s, refEnd)
-		if endIndex != -1 {
-			ref := s[index : endIndex+len(refEnd)]
-			s = strings.Replace(s, ref, os.Getenv(ref[len(refStart):len(ref)-len(refEnd)]), -1)
-		} else {
-			return s
-		}
-
-		index = strings.Index(s, refStart)
-	}
-
-	return s
-}

caddy/parse/parsing_test.go

@@ -368,75 +368,76 @@ func TestParseAll(t *testing.T) {
 func TestEnvironmentReplacement(t *testing.T) {
 	setupParseTests()
 
-	os.Setenv("MY_PORT", "8080")
-	os.Setenv("MY_ADDRESS", "servername.com")
-	os.Setenv("MY_ADDRESS2", "127.0.0.1")
-
-	for i, test := range []struct {
-		input     string
-		addresses [][]address // addresses per server block, in order
-	}{
-		{`{$MY_ADDRESS}`, [][]address{
-			{{"servername.com", ""}},
-		}},
+	os.Setenv("PORT", "8080")
+	os.Setenv("ADDRESS", "servername.com")
+	os.Setenv("FOOBAR", "foobar")
+
+	// basic test; unix-style env vars
+	p := testParser(`{$ADDRESS}`)
+	blocks, _ := p.parseAll()
+	if actual, expected := blocks[0].Addresses[0].Host, "servername.com"; expected != actual {
+		t.Errorf("Expected host to be '%s' but was '%s'", expected, actual)
+	}
 
-		{`{$MY_ADDRESS}:{$MY_PORT}`, [][]address{
-			[]address{{"servername.com", "8080"}},
-		}},
+	// multiple vars per token
+	p = testParser(`{$ADDRESS}:{$PORT}`)
+	blocks, _ = p.parseAll()
+	if actual, expected := blocks[0].Addresses[0].Host, "servername.com"; expected != actual {
+		t.Errorf("Expected host to be '%s' but was '%s'", expected, actual)
+	}
+	if actual, expected := blocks[0].Addresses[0].Port, "8080"; expected != actual {
+		t.Errorf("Expected port to be '%s' but was '%s'", expected, actual)
+	}
 
-		{`{$MY_ADDRESS2}:1234 {
-		  }
-		  localhost:{$MY_PORT} {
-		  }`, [][]address{
-			[]address{{"127.0.0.1", "1234"}},
-			[]address{{"localhost", "8080"}},
-		}},
+	// windows-style var and unix style in same token
+	p = testParser(`{%ADDRESS%}:{$PORT}`)
+	blocks, _ = p.parseAll()
+	if actual, expected := blocks[0].Addresses[0].Host, "servername.com"; expected != actual {
+		t.Errorf("Expected host to be '%s' but was '%s'", expected, actual)
+	}
+	if actual, expected := blocks[0].Addresses[0].Port, "8080"; expected != actual {
+		t.Errorf("Expected port to be '%s' but was '%s'", expected, actual)
+	}
 
-		{`{%MY_ADDRESS%}`, [][]address{
-			{{"servername.com", ""}},
-		}},
+	// reverse order
+	p = testParser(`{$ADDRESS}:{%PORT%}`)
+	blocks, _ = p.parseAll()
+	if actual, expected := blocks[0].Addresses[0].Host, "servername.com"; expected != actual {
+		t.Errorf("Expected host to be '%s' but was '%s'", expected, actual)
+	}
+	if actual, expected := blocks[0].Addresses[0].Port, "8080"; expected != actual {
+		t.Errorf("Expected port to be '%s' but was '%s'", expected, actual)
+	}
 
-		{`{%MY_ADDRESS%}:{%MY_PORT%}`, [][]address{
-			[]address{{"servername.com", "8080"}},
-		}},
+	// env var in server block body as argument
+	p = testParser(":{%PORT%}\ndir1 {$FOOBAR}")
+	blocks, _ = p.parseAll()
+	if actual, expected := blocks[0].Addresses[0].Port, "8080"; expected != actual {
+		t.Errorf("Expected port to be '%s' but was '%s'", expected, actual)
+	}
+	if actual, expected := blocks[0].Tokens["dir1"][1].text, "foobar"; expected != actual {
+		t.Errorf("Expected argument to be '%s' but was '%s'", expected, actual)
+	}
 
-		{`{%MY_ADDRESS2%}:1234 {
-		  }
-		  localhost:{%MY_PORT%} {
-		  }`, [][]address{
-			[]address{{"127.0.0.1", "1234"}},
-			[]address{{"localhost", "8080"}},
-		}},
-	} {
-		p := testParser(test.input)
-		blocks, err := p.parseAll()
+	// combined windows env vars in argument
+	p = testParser(":{%PORT%}\ndir1 {%ADDRESS%}/{%FOOBAR%}")
+	blocks, _ = p.parseAll()
+	if actual, expected := blocks[0].Tokens["dir1"][1].text, "servername.com/foobar"; expected != actual {
+		t.Errorf("Expected argument to be '%s' but was '%s'", expected, actual)
+	}
 
-		if err != nil {
-			t.Errorf("Test %d: Expected no error, but got: %v", i, err)
-		}
+	// malformed env var (windows)
+	p = testParser(":1234\ndir1 {%ADDRESS}")
+	blocks, _ = p.parseAll()
+	if actual, expected := blocks[0].Tokens["dir1"][1].text, "{%ADDRESS}"; expected != actual {
+		t.Errorf("Expected host to be '%s' but was '%s'", expected, actual)
+	}
 
-		if len(blocks) != len(test.addresses) {
-			t.Errorf("Test %d: Expected %d server blocks, got %d",
-				i, len(test.addresses), len(blocks))
-			continue
-		}
-		for j, block := range blocks {
-			if len(block.Addresses) != len(test.addresses[j]) {
-				t.Errorf("Test %d: Expected %d addresses in block %d, got %d",
-					i, len(test.addresses[j]), j, len(block.Addresses))
-				continue
-			}
-			for k, addr := range block.Addresses {
-				if addr.Host != test.addresses[j][k].Host {
-					t.Errorf("Test %d, block %d, address %d: Expected host to be '%s', but was '%s'",
-						i, j, k, test.addresses[j][k].Host, addr.Host)
-				}
-				if addr.Port != test.addresses[j][k].Port {
-					t.Errorf("Test %d, block %d, address %d: Expected port to be '%s', but was '%s'",
-						i, j, k, test.addresses[j][k].Port, addr.Port)
-				}
-			}
-		}
+	// malformed (non-existent) env var (unix)
+	p = testParser(`:{$PORT$}`)
+	blocks, _ = p.parseAll()
+	if actual, expected := blocks[0].Addresses[0].Port, ""; expected != actual {
+		t.Errorf("Expected port to be '%s' but was '%s'", expected, actual)
 	}
 }