feat: s2i builder env var interpolation (#991)

* feat: s2i builder env var interpolation

* nil env values disinclude from interpolate map

buildpacks/builder.go

@@ -6,7 +6,6 @@ import (
 	"fmt"
 	"io"
 	"os"
-	"regexp"
 	"runtime"
 	"strings"
 
@@ -97,15 +96,9 @@ func (builder *Builder) Build(ctx context.Context, f fn.Function) (err error) {
 		}
 	}
 
-	buildEnvs := make(map[string]string, len(f.BuildEnvs))
-	for _, env := range f.BuildEnvs {
-		val, set, err := processEnvValue(*env.Value)
-		if err != nil {
-			return err
-		}
-		if set {
-			buildEnvs[*env.Name] = val
-		}
+	buildEnvs, err := fn.Interpolate(f.BuildEnvs)
+	if err != nil {
+		return err
 	}
 
 	var isTrustedBuilderFunc = func(b string) bool {
@@ -167,31 +160,3 @@ type stdoutWrapper struct {
 func (s stdoutWrapper) Write(p []byte) (n int, err error) {
 	return s.impl.Write(p)
 }
-
-// build command supports only ENV values in from FOO=bar or FOO={{ env:LOCAL_VALUE }}
-var buildEnvRegex = regexp.MustCompile(`^{{\s*(\w+)\s*:(\w+)\s*}}$`)
-
-const (
-	ctxIdx = 1
-	valIdx = 2
-)
-
-// processEnvValue returns only value for ENV variable, that is defined in form FOO=bar or FOO={{ env:LOCAL_VALUE }}
-// if the value is correct, it is returned and the second return parameter is set to `true`
-// otherwise it is set to `false`
-// if the specified value is correct, but the required local variable is not set, error is returned as well
-func processEnvValue(val string) (string, bool, error) {
-	if strings.HasPrefix(val, "{{") {
-		match := buildEnvRegex.FindStringSubmatch(val)
-		if len(match) > valIdx && match[ctxIdx] == "env" {
-			if v, ok := os.LookupEnv(match[valIdx]); ok {
-				return v, true, nil
-			} else {
-				return "", false, fmt.Errorf("required local environment variable %q is not set", match[valIdx])
-			}
-		} else {
-			return "", false, nil
-		}
-	}
-	return val, true, nil
-}

function.go

@@ -222,6 +222,56 @@ func (f Function) Validate() error {
 	return errors.New(b.String())
 }
 
+var envPattern = regexp.MustCompile(`^{{\s*(\w+)\s*:(\w+)\s*}}$`)
+
+// Interpolate Env slice
+// Values with no special format are preserved as simple values.
+// Values which do include the interpolation format (begin with {{) but are not
+// keyed as "env" are also preserved as is.
+// Values properly formated as {{ env:NAME }} are interpolated (substituted)
+// with the value of the local environment variable "NAME", and an error is
+// returned if that environment variable does not exist.
+func Interpolate(ee []Env) (map[string]string, error) {
+	envs := make(map[string]string, len(ee))
+	for _, e := range ee {
+		// Assert non-nil name.
+		if e.Name == nil {
+			return envs, errors.New("env name may not be nil")
+		}
+		// Nil value indicates the resultant map should not include this env var.
+		if e.Value == nil {
+			continue
+		}
+		k, v := *e.Name, *e.Value
+
+		// Simple Values are preserved.
+		// If not prefixed by {{, no interpolation required (simple value)
+		if !strings.HasPrefix(v, "{{") {
+			envs[k] = v // no interpolation required.
+			continue
+		}
+
+		// Values not matching the interpolation pattern are preserved.
+		// If not in the form "{{ env:XYZ }}" then return the value as-is for
+		//                     0  1   2   3
+		// possible match and interpolation in different ways.
+		parts := envPattern.FindStringSubmatch(v)
+		if len(parts) <= 2 || parts[1] != "env" {
+			envs[k] = v
+			continue
+		}
+
+		// Properly formatted local env var references are interpolated.
+		localName := parts[2]
+		localValue, ok := os.LookupEnv(localName)
+		if !ok {
+			return envs, fmt.Errorf("expected environment variable '%v' not found", localName)
+		}
+		envs[k] = localValue
+	}
+	return envs, nil
+}
+
 // nameFromPath returns the default name for a Function derived from a path.
 // This consists of the last directory in the given path, if derivable (empty
 // paths, paths consisting of all slashes, etc. return the zero value "")

function_test.go

@@ -81,3 +81,57 @@ func TestFunction_NameDefault(t *testing.T) {
 		t.Fatalf("expected name 'testFunctionNameDefault', got '%v'", f.Name)
 	}
 }
+
+// Test_Interpolate ensures environment variable interpolation processes
+// environment variables by interpolating properly formatted references to
+// local environment variables, returning a final simple map structure.
+// Also ensures that nil value references are interpreted as meaning the
+// environment is to be disincluded from the resultant map, rathern than included
+// with an empty value.
+// TODO: Perhaps referring to a nonexistent local env var should be treated
+// as a "leave as is" (do not set) rather than "required" resulting in error?
+// TODO: What use case does a nil pointer in the Env struct serve?  Add it
+// explicitly here ore get rid of the nils.
+func Test_Interpolate(t *testing.T) {
+	defer WithEnvVar(t, "INTERPOLATE", "interpolated")()
+	cases := []struct {
+		Value    string
+		Expected string
+		Error    bool
+	}{
+		// Simple values are kept unchanged
+		{Value: "simple value", Expected: "simple value"},
+		// Properly referenced environment variables are interpolated
+		{Value: "{{ env:INTERPOLATE }}", Expected: "interpolated"},
+		// Other interpolation types other than "env" are left unchanged
+		{Value: "{{ other:TYPE }}", Expected: "{{ other:TYPE }}", Error: false},
+		// Properly formatted references to missing variables error
+		{Value: "{{ env:MISSING }}", Expected: "", Error: true},
+	}
+
+	name := "NAME" // default name for all tests
+	for _, c := range cases {
+		t.Logf("Value: %v\n", c.Value)
+		var (
+			envs    = []fn.Env{{Name: &name, Value: &c.Value}} // pre-interpolated
+			vv, err = fn.Interpolate(envs)                     // interpolated
+			v       = vv[name]                                 // final value
+		)
+		if c.Error && err == nil {
+			t.Fatal("expected error in Envs interpolation not received")
+		}
+		if v != c.Expected {
+			t.Fatalf("expected env value '%v' to be interpolated as '%v', but got '%v'", c.Value, c.Expected, v)
+		}
+	}
+
+	// Nil value should be treated as being disincluded from the resultant map.
+	envs := []fn.Env{{Name: &name}} // has a nil *Value ptr
+	vv, err := fn.Interpolate(envs)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(vv) != 0 {
+		t.Fatalf("expected envs with a nil value to not be included in interpolation result")
+	}
+}

s2i/builder.go

@@ -70,6 +70,19 @@ func (b *Builder) Build(ctx context.Context, f fn.Function) (err error) {
 	cfg.PreviousImagePullPolicy = api.DefaultPreviousImagePullPolicy
 	cfg.RuntimeImagePullPolicy = api.DefaultRuntimeImagePullPolicy
 	cfg.DockerConfig = s2idocker.GetDefaultDockerConfig()
+
+	// Environment variables
+	// Build Envs have local env var references interpolated then added to the
+	// config as an S2I EnvironmentList struct
+	buildEnvs, err := fn.Interpolate(f.BuildEnvs)
+	if err != nil {
+		return err
+	}
+	for k, v := range buildEnvs {
+		cfg.Environment = append(cfg.Environment, api.EnvironmentSpec{Name: k, Value: v})
+	}
+
+	// Validate the config
 	if errs := validation.ValidateConfig(cfg); len(errs) > 0 {
 		for _, e := range errs {
 			fmt.Fprintf(os.Stderr, "ERROR: %s\n", e)