feat: limit substitutions to double-braces (#598)

supporting substitution of $FOO or ${FOO} invites confusion and makes it
hard to understand the behavior of a stackerfile by looking at it,
because it can accidentally hard-code a value that is expected to be a
shell variable in a run section.

using double brackets is visually different and has the benefit of being
invalid shell syntax, so you know that it is intended to be a stacker
placeholder.

Now if you specify --substitute FOO=bar and have $FOO or ${FOO} in your
stackerfile, stacker will fail the operation and print errors.

It first collects all instances of the error then prints them at the
end, so you shouldn't have to run it over and over to get them all.

Note that the previously documented behavior of allowing ${FOO:default}
was not tested and did not work. That documentation is changed to
reflect real life.

Test changes:

To avoid confusion about what substitution syntax we are testing, where
possible, use quoted heredocs for stackerfiles in tests.

This replaces use of e.g. $BUSYBOX_OCI, which was getting substituted by
the shell before cat wrote it to a file, with ${{BUSYBOX_OCI}} in a
quoted heredoc, so bash doesn't try to substitute it (which would fail),
and instead stacker will see it and substitute it itself.
This means adding a lot of --substitute args around where they were not
previously required.

A couple of places still use the previous approach so we can use the
shell to sub in a file sha, for example.

Signed-off-by: Michael McCracken <mikmccra@cisco.com>

doc/stacker_yaml.md

@@ -11,20 +11,36 @@ processed in stacker as:
 
     1 2 3
 
-That is, variables of the form `$FOO` or `${FOO}` are supported, and variables
-with `${FOO:default}` a default value will evaluate to their default if not
-specified on the command line. It is an error to specify a `${FOO}` style
-without a default; to make the default an empty string, use `${FOO:}`.
+In order to avoid conflict with bash or POSIX shells in the `run` section, 
+only placeholders with two braces are supported, e.g. `${{FOO}}`.
+Placeholders with a default value like `${{FOO:default}}` will evaluate to their default if not
+specified on the command line or in a substitution file.
 
-In addition to substitutions provided on the command line, the following
-variables are also available with their values from either command
+Using a `${{FOO}}` placeholder without a default will result in an error if
+there is no substitution provided. If you want an empty string in that case, use
+an empty default: `${{FOO:}}`.
+
+In order to avoid confusion, it is also an error if a placeholder in the shell
+style (`$FOO` or `${FOO}`) is found when the same key has been provided as a
+substitution either via the command line (e.g. `--substitute FOO=bar`) or in a
+substitution file. An error will be printed that explains how to rewrite it:
+
+   error "A=B" was provided as a substitution and unsupported placeholder "${A}" was found. Replace "${A}" with "${{A}}" to use the substitution.
+
+Substitutions can also be specified in a yaml file given with the argument
+`--substitute-file`, with any number of key: value pairs:
+
+    FOO: bar
+    BAZ: bat
+
+In addition to substitutions provided on the command line or a file, the
+following variables are also available with their values from either command
 line flags or stacker-config file.
 
     STACKER_STACKER_DIR config name 'stacker_dir', cli flag '--stacker-dir'-
     STACKER_ROOTFS_DIR  config name 'rootfs_dir', cli flag '--roots-dir'
     STACKER_OCI_DIR     config name 'oci_dir', cli flag '--oci-dir'
 
-
 The stacker build environment will have the following environment variables
 available for reference:
 

pkg/types/stackerfile.go

@@ -58,28 +58,78 @@ func (sf *Stackerfile) Len() int {
 }
 
 func substitute(content string, substitutions []string) (string, error) {
+	// replace all placeholders where we have a substitution provided
+	sub_usage := []int{}
+	unsupported_messages := []string{}
 	for _, subst := range substitutions {
 		membs := strings.SplitN(subst, "=", 2)
 		if len(membs) != 2 {
 			return "", errors.Errorf("invalid substition %s", subst)
 		}
-
-		from := fmt.Sprintf("$%s", membs[0])
 		to := membs[1]
 
-		log.Debugf("substituting %s to %s", from, to)
+		// warn on finding unsupported placeholders matching provided
+		// substitution keys:
+		nobracket := fmt.Sprintf("$%s", membs[0])
+		onebracket := fmt.Sprintf("${%s}", membs[0])
+		bad_content := content
+		for _, unsupp_placeholder := range []string{nobracket, onebracket} {
+			if strings.Contains(content, unsupp_placeholder) {
+				msg := fmt.Sprintf("%q was provided as a substitution "+
+					"and unsupported placeholder %q was found. "+
+					"Replace %q with \"${{%s}}\" to use the substitution.\n", subst,
+					unsupp_placeholder, unsupp_placeholder, membs[0])
+				unsupported_messages = append(unsupported_messages, msg)
+			}
+		}
 
-		content = strings.Replace(content, from, to, -1)
+		// the ${FOO:bar} syntax never worked! but since we documented it
+		// previously, let's warn on it too:
+		bad_re, err := regexp.Compile(fmt.Sprintf(`\$\{%s(:[^\}]*)?\}`, membs[0]))
+		if err != nil {
+			return "", err
+		}
+		bad_matches := bad_re.FindAllString(bad_content, -1)
+		for _, bad_match := range bad_matches {
+			msg := fmt.Sprintf("%q was provided as a substitution "+
+				"and unsupported placeholder %q was found. "+
+				"Replace %q with \"${{%s}\" to use the substitution.\n", subst,
+				bad_match, bad_match, bad_match[2:])
+			unsupported_messages = append(unsupported_messages, msg)
+		}
 
+		nmatches := 0
 		re, err := regexp.Compile(fmt.Sprintf(`\$\{\{%s(:[^\}]*)?\}\}`, membs[0]))
 		if err != nil {
 			return "", err
 		}
 
+		matches := re.FindAllString(content, -1)
 		content = re.ReplaceAllString(content, to)
+
+		if matches != nil {
+			nmatches += len(matches)
+		}
+		sub_usage = append(sub_usage, nmatches)
+	}
+
+	for i, numused := range sub_usage {
+		if numused > 0 {
+			log.Debugf("substitution: %q was used %d times", substitutions[i], numused)
+		} else {
+			log.Debugf("substitution: %q was NOT used", substitutions[i])
+		}
+	}
+
+	if len(unsupported_messages) > 0 {
+		for _, msg := range unsupported_messages {
+			log.Errorf(msg)
+		}
+		return "", errors.Errorf("%d instances of unsupported placeholders found. Review log for how to update.", len(unsupported_messages))
 	}
 
-	// now, anything that's left we can just use its value
+	// now, anything that's left was not provided in substitutions but should
+	// have a default value. Not having a default here is an error.
 	re := regexp.MustCompile(`\$\{\{[^\}]*\}\}`)
 	for {
 		indexes := re.FindAllStringIndex(content, -1)

pkg/types/types_test.go

@@ -80,27 +80,39 @@ third:
 }
 
 func TestSubstitute(t *testing.T) {
-	s := "$ONE $TWO ${{TWO}} ${{TWO:}} ${{TWO:3}} ${{TWO2:22}} ${{THREE:3}}"
-	result, err := substitute(s, []string{"ONE=1", "TWO=2"})
+	// test supported double bracket syntax with default values
+	s := "${{ONE}} ${{TWO:}} ${{TWO:3}} ${{TWO2:22}} ${{THREE:3}} ${{NOTHING:}}"
+	result, err := substitute(s, []string{"ONE=1", "TWO=2", "BLEH=BLAH"})
 	if err != nil {
 		t.Fatalf("failed substitutition: %s", err)
 	}
 
-	expected := "1 2 2 2 2 22 3"
+	expected := "1 2 2 22 3 "
 	if result != expected {
-		t.Fatalf("bad substitution result, expected %s got %s", expected, result)
+		t.Fatalf("bad substitution result, expected '%s' got '%s'", expected, result)
 	}
 
-	// ${PRODUCT} is ok
-	s = "$PRODUCT ${PRODUCT//x} ${{PRODUCT}}"
-	result, err = substitute(s, []string{"PRODUCT=foo"})
-	if err != nil {
-		t.Fatalf("failed substitution: %s", err)
+	// Test that unsupported single bracket syntax is passed through
+	// (there are warnings printed to help debug)
+	_, err = substitute("${A} ${A:Z}", []string{"A=B"})
+	if err == nil {
+		t.Fatalf("substitution with unsupported placeholders should have failed but didn't")
 	}
 
-	expected = "foo ${PRODUCT//x} foo"
-	if result != expected {
-		t.Fatalf("bad substitution result, expected %s got %s", expected, result)
+	// $PRODUCT or ${PRODUCT} are NOT valid placeholders, but also not errors.
+	// we warn if you provide PRODUCT and we see those invalid placeholders
+	// we do not warn if you have e.g. $DONTWARN but it isn't a provided sub
+	s = "$DONTWARN $PRODUCT ${PRODUCT} ${PRODUCT:def} ${PRODUCT//x} ${{PRODUCT}} ${{PRODUCT:bar}}"
+	_, err = substitute(s, []string{"PRODUCT=foo"})
+	if err == nil {
+		t.Fatalf("substitution with unsupported placeholders should have failed but didn't")
+	}
+
+	// a double bracket placeholder with no default value must be provided
+	s = "${{NOT_SET}}"
+	_, err = substitute(s, []string{})
+	if err == nil {
+		t.Fatalf("expected error for unset variable!")
 	}
 }
 

test/annotations-namespace.bats

@@ -9,29 +9,29 @@ function teardown() {
 }
 
 @test "namespace arg works" {
-    cat > stacker.yaml <<EOF
+    cat > stacker.yaml <<"EOF"
 thing:
     from:
         type: oci
-        url: $BUSYBOX_OCI
+        url: ${{BUSYBOX_OCI}}
     run: ls
 EOF
-    stacker build --annotations-namespace=namespace.example
+    stacker build --annotations-namespace=namespace.example --substitute BUSYBOX_OCI=${BUSYBOX_OCI}
     [ "$status" -eq 0 ]
     manifest=$(cat oci/index.json | jq -r .manifests[0].digest | cut -f2 -d:)
     namespace=$(cat oci/blobs/sha256/$manifest | jq -r .annotations | cut -f1 -d:)
     [[ "$namespace" == *"namespace.example"* ]]
 }
 
 @test "default namespace arg works" {
-    cat > stacker.yaml <<EOF
+    cat > stacker.yaml <<"EOF"
 thing:
     from:
         type: oci
-        url: $BUSYBOX_OCI
+        url: ${{BUSYBOX_OCI}}
     run: ls
 EOF
-    stacker build
+    stacker build --substitute BUSYBOX_OCI=${BUSYBOX_OCI}
     [ "$status" -eq 0 ]
     manifest=$(cat oci/index.json | jq -r .manifests[0].digest | cut -f2 -d:)
     namespace=$(cat oci/blobs/sha256/$manifest | jq -r .annotations | cut -f1 -d:)

test/annotations.bats

@@ -9,16 +9,16 @@ function teardown() {
 }
 
 @test "annotations work" {
-    cat > stacker.yaml <<EOF
+    cat > stacker.yaml <<"EOF"
 thing:
     from:
         type: oci
-        url: $BUSYBOX_OCI
+        url: ${{BUSYBOX_OCI}}
     run: ls
     annotations:
       a.b.c.key: val
 EOF
-    stacker build 
+    stacker build --substitute BUSYBOX_OCI=${BUSYBOX_OCI}
     [ "$status" -eq 0 ]
     manifest=$(cat oci/index.json | jq -r .manifests[0].digest | cut -f2 -d:)
     cat oci/blobs/sha256/$manifest | jq .

test/args.bats

@@ -9,11 +9,11 @@ function teardown() {
 }
 
 @test "workdir args" {
-    cat > stacker.yaml <<EOF
+    cat > stacker.yaml <<"EOF"
 parent:
     from:
         type: oci
-        url: $BUSYBOX_OCI
+        url: ${{BUSYBOX_OCI}}
 child:
     from:
         type: built
@@ -24,7 +24,7 @@ EOF
   # check defaults
   tmpdir=$(mktemp -d)
   chmod -R a+rwx $tmpdir
-  stacker --work-dir $tmpdir build
+  stacker --work-dir $tmpdir build --substitute BUSYBOX_OCI=${BUSYBOX_OCI}
   [ -d $tmpdir ]
   [ -d $tmpdir/.stacker ]
   [ -d $tmpdir/roots ]
@@ -36,7 +36,7 @@ EOF
   chmod -R a+rwx $tmpdir
   stackerdir=$(mktemp -d)
   chmod -R a+rwx $stackerdir
-  stacker --work-dir $tmpdir --stacker-dir $stackerdir build
+  stacker --work-dir $tmpdir --stacker-dir $stackerdir build --substitute BUSYBOX_OCI=${BUSYBOX_OCI}
   [ -d $tmpdir ]
   [ ! -d $tmpdir/.stacker ]
   [ -d $tmpdir/roots ]

test/asterisk.bats

@@ -9,16 +9,16 @@ function teardown() {
 }
 
 @test "wildcards work in run section" {
-    cat > stacker.yaml <<EOF
+    cat > stacker.yaml <<"EOF"
 a:
     from:
         type: oci
-        url: $BUSYBOX_OCI
+        url: ${{BUSYBOX_OCI}}
     run: |
         mkdir /mybin
         cp /bin/* /mybin
 EOF
-    stacker build
+    stacker build --substitute BUSYBOX_OCI=${BUSYBOX_OCI}
     umoci unpack --image oci:a dest
     [ "$status" -eq 0 ]
 

test/atomfs.bats

@@ -29,15 +29,15 @@ function basic_test() {
     require_privilege priv
     local verity_arg=$1
 
-    cat > stacker.yaml <<EOF
+    cat > stacker.yaml <<"EOF"
 test:
     from:
         type: oci
-        url: $BUSYBOX_OCI
+        url: ${{BUSYBOX_OCI}}
     run: |
         touch /hello
 EOF
-    stacker build --layer-type=squashfs $verity_arg
+    stacker build --layer-type=squashfs $verity_arg --substitute BUSYBOX_OCI=${BUSYBOX_OCI}
     mkdir mountpoint
     stacker internal-go atomfs mount test-squashfs mountpoint
 
@@ -63,11 +63,11 @@ EOF
 
 @test "mount + umount + mount a tree of images works" {
     require_privilege priv
-    cat > stacker.yaml <<EOF
+    cat > stacker.yaml <<"EOF"
 base:
     from:
         type: oci
-        url: $BUSYBOX_OCI
+        url: ${{BUSYBOX_OCI}}
     run: touch /base
 a:
     from:
@@ -85,7 +85,7 @@ c:
         tag: base
     run: touch /c
 EOF
-    stacker build --layer-type=squashfs
+    stacker build --layer-type=squashfs --substitute BUSYBOX_OCI=${BUSYBOX_OCI}
 
     mkdir a
     stacker internal-go atomfs mount a-squashfs a
@@ -137,15 +137,15 @@ EOF
 
 @test "bad existing verity device is rejected" {
     require_privilege priv
-    cat > stacker.yaml <<EOF
+    cat > stacker.yaml <<"EOF"
 test:
     from:
         type: oci
-        url: $BUSYBOX_OCI
+        url: ${{BUSYBOX_OCI}}
     run: |
         touch /hello
 EOF
-    stacker build --layer-type=squashfs
+    stacker build --layer-type=squashfs --substitute BUSYBOX_OCI=${BUSYBOX_OCI}
 
     manifest=$(cat oci/index.json | jq -r .manifests[0].digest | cut -f2 -d:)
     first_layer_hash=$(cat oci/blobs/sha256/$manifest | jq -r .layers[0].digest | cut -f2 -d:)