From a2bc0869af049c2b16c37c06f20d7e400218f651 Mon Sep 17 00:00:00 2001 From: Robert Kaussow Date: Fri, 15 Nov 2024 21:45:29 +0100 Subject: [PATCH] fix: use StringMap for `build_args` instead of StringSlice (#277) BREAKING CHANGE: The type of the `build_args` property was changed from `list` to `map` to support the use of secrets for certain build arguments. During this refactoring, the `build_args_from_env` behavior was also changed to achieve the intended behavior. Environment variable names passed to this list will be used as build arguments if the environment variable is set. --- docker/docker.go | 95 +++++++++++--------- docker/docker_test.go | 197 ++++++++++++++++++++++++++++++++++++++++++ docs/data/data.yaml | 31 ++++++- plugin/impl.go | 7 ++ plugin/plugin.go | 12 +-- 5 files changed, 292 insertions(+), 50 deletions(-) create mode 100644 docker/docker_test.go diff --git a/docker/docker.go b/docker/docker.go index f2777e6..60983d0 100644 --- a/docker/docker.go +++ b/docker/docker.go @@ -2,6 +2,7 @@ package docker import ( "fmt" + "maps" "os" "strings" "time" @@ -23,33 +24,33 @@ type Registry struct { // Build defines Docker build parameters. type Build struct { - Ref string // Git commit ref - Branch string // Git repository branch - Containerfile string // Docker build Containerfile - Context string // Docker build context - TagsAuto bool // Docker build auto tag - TagsSuffix string // Docker build tags with suffix - Tags cli.StringSlice // Docker build tags - ExtraTags cli.StringSlice // Docker build tags including registry - Platforms cli.StringSlice // Docker build target platforms - Args cli.StringSlice // Docker build args - ArgsEnv cli.StringSlice // Docker build args from env - Target string // Docker build target - Pull bool // Docker build pull - CacheFrom []string // Docker build cache-from - CacheTo string // Docker build cache-to - Compress bool // Docker build compress - Repo string // Docker build repository - NoCache bool // Docker build no-cache - AddHost cli.StringSlice // Docker build add-host - Quiet bool // Docker build quiet - Output string // Docker build output folder - NamedContext cli.StringSlice // Docker build named context - Labels cli.StringSlice // Docker build labels - Provenance string // Docker build provenance attestation - SBOM string // Docker build sbom attestation - Secrets []string // Docker build secrets - Dryrun bool // Docker build dryrun + Ref string // Git commit ref + Branch string // Git repository branch + Containerfile string // Docker build Containerfile + Context string // Docker build context + TagsAuto bool // Docker build auto tag + TagsSuffix string // Docker build tags with suffix + Tags cli.StringSlice // Docker build tags + ExtraTags cli.StringSlice // Docker build tags including registry + Platforms cli.StringSlice // Docker build target platforms + Args map[string]string // Docker build args + ArgsEnv cli.StringSlice // Docker build args from env + Target string // Docker build target + Pull bool // Docker build pull + CacheFrom []string // Docker build cache-from + CacheTo string // Docker build cache-to + Compress bool // Docker build compress + Repo string // Docker build repository + NoCache bool // Docker build no-cache + AddHost cli.StringSlice // Docker build add-host + Quiet bool // Docker build quiet + Output string // Docker build output folder + NamedContext cli.StringSlice // Docker build named context + Labels cli.StringSlice // Docker build labels + Provenance string // Docker build provenance attestation + SBOM string // Docker build sbom attestation + Secrets []string // Docker build secrets + Dryrun bool // Docker build dryrun } // helper function to create the docker login command. @@ -100,10 +101,12 @@ func (b *Build) Run() *plugin_exec.Cmd { "-f", b.Containerfile, } - defaultBuildArgs := []string{ - fmt.Sprintf("DOCKER_IMAGE_CREATED=%s", time.Now().Format(time.RFC3339)), + defaultBuildArgs := map[string]string{ + "DOCKER_IMAGE_CREATED": time.Now().Format(time.RFC3339), } + maps.Copy(b.Args, defaultBuildArgs) + args = append(args, b.Context) if !b.Dryrun && b.Output == "" && len(b.Tags.Value()) > 0 { args = append(args, "--push") @@ -130,11 +133,11 @@ func (b *Build) Run() *plugin_exec.Cmd { } for _, arg := range b.ArgsEnv.Value() { - b.addProxyValue(arg) + b.addArgFromEnv(arg) } - for _, arg := range append(defaultBuildArgs, b.Args.Value()...) { - args = append(args, "--build-arg", arg) + for key, value := range b.Args { + args = append(args, "--build-arg", fmt.Sprintf("%s=%s", key, value)) } for _, host := range b.AddHost.Value() { @@ -203,9 +206,19 @@ func (b *Build) AddProxyBuildArgs() { func (b *Build) addProxyValue(key string) { value := b.getProxyValue(key) - if len(value) > 0 && !b.hasProxyBuildArg(key) { - b.Args = *cli.NewStringSlice(append(b.Args.Value(), fmt.Sprintf("%s=%s", key, value))...) - b.Args = *cli.NewStringSlice(append(b.Args.Value(), fmt.Sprintf("%s=%s", strings.ToUpper(key), value))...) + if value != "" && !b.hasProxyBuildArg(key) { + b.Args[key] = value + b.Args[strings.ToUpper(key)] = value + } +} + +func (b *Build) addArgFromEnv(key string) { + if value, ok := b.Args[key]; ok && value != "" { + return + } + + if value, ok := os.LookupEnv(key); ok && value != "" { + b.Args[key] = value } } @@ -215,7 +228,7 @@ func (b *Build) addProxyValue(key string) { func (b *Build) getProxyValue(key string) string { value := os.Getenv(key) - if len(value) > 0 { + if value != "" { return value } @@ -224,12 +237,12 @@ func (b *Build) getProxyValue(key string) string { // helper function that looks to see if a proxy value was set in the build args. func (b *Build) hasProxyBuildArg(key string) bool { - keyUpper := strings.ToUpper(key) + if _, ok := b.Args[key]; ok { + return true + } - for _, s := range b.Args.Value() { - if strings.HasPrefix(s, key) || strings.HasPrefix(s, keyUpper) { - return true - } + if _, ok := b.Args[strings.ToUpper(key)]; ok { + return true } return false diff --git a/docker/docker_test.go b/docker/docker_test.go new file mode 100644 index 0000000..eb29acc --- /dev/null +++ b/docker/docker_test.go @@ -0,0 +1,197 @@ +package docker + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestHasProxyBuildArg(t *testing.T) { + tests := []struct { + name string + args map[string]string + key string + expected bool + }{ + { + name: "lowercase key exists", + args: map[string]string{"http_proxy": "http://proxy.example.com"}, + key: "http_proxy", + expected: true, + }, + { + name: "uppercase key exists", + args: map[string]string{"HTTP_PROXY": "http://proxy.example.com"}, + key: "http_proxy", + expected: true, + }, + { + name: "key does not exist", + args: map[string]string{}, + key: "http_proxy", + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + b := &Build{Args: tt.args} + result := b.hasProxyBuildArg(tt.key) + assert.Equal(t, tt.expected, result) + }) + } +} + +func TestGetProxyValue(t *testing.T) { + tests := []struct { + name string + envVars map[string]string + key string + expected string + }{ + { + name: "lowercase env var exists", + envVars: map[string]string{ + "http_proxy": "http://proxy.local", + }, + key: "http_proxy", + expected: "http://proxy.local", + }, + { + name: "uppercase env var exists", + envVars: map[string]string{ + "HTTP_PROXY": "http://proxy.upper.local", + }, + key: "http_proxy", + expected: "http://proxy.upper.local", + }, + { + name: "both cases exist, lowercase preferred", + envVars: map[string]string{ + "http_proxy": "http://proxy.lower.local", + "HTTP_PROXY": "http://proxy.upper.local", + }, + key: "http_proxy", + expected: "http://proxy.lower.local", + }, + { + name: "no env vars exist", + envVars: map[string]string{}, + key: "http_proxy", + expected: "", + }, + { + name: "different proxy type", + envVars: map[string]string{ + "https_proxy": "https://secure.proxy.local", + "HTTPS_PROXY": "https://secure.proxy.upper.local", + }, + key: "https_proxy", + expected: "https://secure.proxy.local", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Set test environment variables + for k, v := range tt.envVars { + t.Setenv(k, v) + } + + b := &Build{} + result := b.getProxyValue(tt.key) + assert.Equal(t, tt.expected, result) + }) + } +} + +func TestAddArgFromEnv(t *testing.T) { + tests := []struct { + name string + envVars map[string]string + key string + existing map[string]string + expected map[string]string + }{ + { + name: "add new env var", + envVars: map[string]string{ + "NEW_VAR": "test_value", + }, + key: "NEW_VAR", + expected: map[string]string{ + "NEW_VAR": "test_value", + }, + }, + { + name: "empty env var", + envVars: map[string]string{}, + key: "MISSING_VAR", + expected: map[string]string{}, + }, + { + name: "env var with empty value", + envVars: map[string]string{ + "EMPTY_VAR": "", + }, + key: "EMPTY_VAR", + expected: map[string]string{}, + }, + { + name: "multiple env vars", + envVars: map[string]string{ + "VAR1": "value1", + "VAR2": "value2", + }, + key: "VAR1", + expected: map[string]string{ + "VAR1": "value1", + }, + }, + { + name: "special characters in value", + envVars: map[string]string{ + "SPECIAL_VAR": "!@#$%^&*()", + }, + key: "SPECIAL_VAR", + expected: map[string]string{ + "SPECIAL_VAR": "!@#$%^&*()", + }, + }, + { + name: "preserve existing args", + envVars: map[string]string{ + "TEST_VAR": "new_value", + }, + key: "TEST_VAR", + existing: map[string]string{ + "TEST_VAR": "old_value", + }, + expected: map[string]string{ + "TEST_VAR": "old_value", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Clear environment before each test + for k := range tt.envVars { + t.Setenv(k, "") + } + + // Set test environment variables + for k, v := range tt.envVars { + t.Setenv(k, v) + } + + b := &Build{Args: make(map[string]string)} + if tt.existing != nil { + b.Args = tt.existing + } + + b.addArgFromEnv(tt.key) + assert.Equal(t, tt.expected, b.Args) + }) + } +} diff --git a/docs/data/data.yaml b/docs/data/data.yaml index 37b1f5c..b01c7d6 100644 --- a/docs/data/data.yaml +++ b/docs/data/data.yaml @@ -34,13 +34,38 @@ properties: - name: build_args description: | - Custom build arguments for the build. - type: list + Custom build arguments for the build. Example: + + ```yaml + steps: + - name: Build + image: quay.io/thegeeklab/wp-docker-buildx + settings: + repo: example/repo + build_args: + FOO: bar + API_KEY: + from_secret: API_KEY + ``` + type: map required: false - name: build_args_from_env description: | - Forward environment variables as custom arguments to the build. + Forward environment variables to the build as build arguments. If the same key + already exists in `build_args`, it will not be overwritten. Example: + + ```yaml + steps: + - name: Build + image: quay.io/thegeeklab/wp-docker-buildx + environment: + CUSTOM_ENVIRONMENT: custom_value + settings: + repo: example/repo + build_args_from_env: + - CUSTOM_ENVIRONMENT + ``` type: list required: false diff --git a/plugin/impl.go b/plugin/impl.go index 6fb0b70..6f706aa 100644 --- a/plugin/impl.go +++ b/plugin/impl.go @@ -210,6 +210,13 @@ func (p *Plugin) FlagsFromContext() error { p.Settings.Build.Secrets = secrets.Get() + args, ok := p.Context.Generic("args").(*plugin_types.StringMapFlag) + if !ok { + return fmt.Errorf("%w: failed to read args input", ErrTypeAssertionFailed) + } + + p.Settings.Build.Args = args.Get() + return nil } diff --git a/plugin/plugin.go b/plugin/plugin.go index dc933e5..a5cdf8e 100644 --- a/plugin/plugin.go +++ b/plugin/plugin.go @@ -227,12 +227,12 @@ func Flags(settings *Settings, category string) []cli.Flag { Destination: &settings.Build.ExtraTags, Category: category, }, - &cli.StringSliceFlag{ - Name: "args", - EnvVars: []string{"PLUGIN_BUILD_ARGS"}, - Usage: "custom build arguments for the build", - Destination: &settings.Build.Args, - Category: category, + &cli.GenericFlag{ + Name: "args", + EnvVars: []string{"PLUGIN_BUILD_ARGS"}, + Usage: "custom build arguments for the build", + Value: &plugin_types.StringMapFlag{}, + Category: category, }, &cli.StringSliceFlag{ Name: "args-from-env",