From 14248311b1254fe8601b394f064e2fdd92dd4ced Mon Sep 17 00:00:00 2001 From: Luke Kingland <58986931+lkingland@users.noreply.github.com> Date: Tue, 26 Apr 2022 04:18:54 +0900 Subject: [PATCH] feat: s2i builder env var interpolation (#991) * feat: s2i builder env var interpolation * nil env values disinclude from interpolate map --- buildpacks/builder.go | 41 +++----------------------------- function.go | 50 +++++++++++++++++++++++++++++++++++++++ function_test.go | 54 +++++++++++++++++++++++++++++++++++++++++++ s2i/builder.go | 13 +++++++++++ 4 files changed, 120 insertions(+), 38 deletions(-) diff --git a/buildpacks/builder.go b/buildpacks/builder.go index 48ae8f461..7df589268 100644 --- a/buildpacks/builder.go +++ b/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 -} diff --git a/function.go b/function.go index 8f0990b31..6b821a30f 100644 --- a/function.go +++ b/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 "") diff --git a/function_test.go b/function_test.go index a86d665d7..d0f5e3603 100644 --- a/function_test.go +++ b/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") + } +} diff --git a/s2i/builder.go b/s2i/builder.go index dde3a4578..cf2738084 100644 --- a/s2i/builder.go +++ b/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)