bug: docker build arg fix

pkg/build/pipeline/CiTemplateReadService.go

@@ -115,6 +115,9 @@ func (impl *CiTemplateReadServiceImpl) extractBuildConfigBean(templateOverride *
 			impl.Logger.Errorw("error occurred while parsing ci build config", "err", err)
 		}
 	}
+	// rows saved before args were stripped on write may still carry them; never serve them,
+	// so API clients can't echo them back and builds can't pick them up
+	ciBuildConfigBean.ClearDockerBuildArgs()
 	ciBuildConfigBean.GitMaterialId = templateOverride.GitMaterialId
 	ciBuildConfigBean.BuildContextGitMaterialId = templateOverride.BuildContextGitMaterialId
 	return ciBuildConfigBean, nil

pkg/build/pipeline/CiTemplateReadService_test.go

@@ -0,0 +1,175 @@
+/*
+ * Copyright (c) 2024. Devtron Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package pipeline
+
+import (
+	"testing"
+
+	"github.com/devtron-labs/devtron/internal/sql/repository/pipelineConfig"
+	"github.com/devtron-labs/devtron/internal/util"
+	bean2 "github.com/devtron-labs/devtron/pkg/build/pipeline/bean"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+type fakeCiTemplateOverrideRepository struct {
+	pipelineConfig.CiTemplateOverrideRepository
+	overrides []*pipelineConfig.CiTemplateOverride
+}
+
+func (f *fakeCiTemplateOverrideRepository) FindByAppId(int) ([]*pipelineConfig.CiTemplateOverride, error) {
+	return f.overrides, nil
+}
+
+func (f *fakeCiTemplateOverrideRepository) FindByCiPipelineIds([]int) ([]*pipelineConfig.CiTemplateOverride, error) {
+	return f.overrides, nil
+}
+
+func (f *fakeCiTemplateOverrideRepository) FindByCiPipelineId(int) (*pipelineConfig.CiTemplateOverride, error) {
+	return f.overrides[0], nil
+}
+
+type fakeCiTemplateRepository struct {
+	pipelineConfig.CiTemplateRepository
+	template *pipelineConfig.CiTemplate
+}
+
+func (f *fakeCiTemplateRepository) FindByAppId(int) (*pipelineConfig.CiTemplate, error) {
+	return f.template, nil
+}
+
+func (f *fakeCiTemplateRepository) FindByAppIds([]int) ([]*pipelineConfig.CiTemplate, error) {
+	return []*pipelineConfig.CiTemplate{f.template}, nil
+}
+
+// the #2983 override row: args and target platform the editor showed as empty
+const staleDockerOverrideMetadata = `{"dockerfileRelativePath":"Dockerfile","dockerfileContent":"","args":{"AWS_ACCESS_KEY_ID":"AKIA-stale","AWS_SECRET_ACCESS_KEY":"stale-secret"},"targetPlatform":"linux/arm64,linux/amd64","dockerBuildOptions":{"network":"host"},"buildContext":"ctx","useBuildx":false,"buildxProvenanceMode":"","buildxDriverImage":""}`
+
+func overrideRow(buildType bean2.CiBuildType, metadata string) *pipelineConfig.CiTemplateOverride {
+	return &pipelineConfig.CiTemplateOverride{
+		Id: 11, CiPipelineId: 21, GitMaterialId: 31, BuildContextGitMaterialId: 32, DockerfilePath: "legacy/Dockerfile",
+		DockerRegistryId: "reg", DockerRepository: "repo", Active: true, CiBuildConfigId: 41,
+		CiBuildConfig: &pipelineConfig.CiBuildConfig{Id: 41, Type: string(buildType), BuildMetadata: metadata},
+	}
+}
+
+func newReadService(t *testing.T, templates *fakeCiTemplateRepository, overrides *fakeCiTemplateOverrideRepository) *CiTemplateReadServiceImpl {
+	logger, err := util.NewSugardLogger()
+	require.NoError(t, err)
+	return NewCiTemplateReadServiceImpl(logger, templates, overrides)
+}
+
+// every exported read that goes through extractBuildConfigBean
+func readOverrideBuildConfigs(t *testing.T, svc *CiTemplateReadServiceImpl) map[string]*bean2.CiBuildConfigBean {
+	byPipelineId, err := svc.FindTemplateOverrideByCiPipelineId(21)
+	require.NoError(t, err)
+	byAppId, err := svc.FindTemplateOverrideByAppId(1)
+	require.NoError(t, err)
+	require.Len(t, byAppId, 1)
+	byPipelineIds, err := svc.FindTemplateOverrideByCiPipelineIds([]int{21})
+	require.NoError(t, err)
+	require.Len(t, byPipelineIds, 1)
+	return map[string]*bean2.CiBuildConfigBean{
+		"FindTemplateOverrideByCiPipelineId":  byPipelineId.CiBuildConfig,
+		"FindTemplateOverrideByAppId":         byAppId[0].CiBuildConfig,
+		"FindTemplateOverrideByCiPipelineIds": byPipelineIds[0].CiBuildConfig,
+	}
+}
+
+func TestCiTemplateReadService_OverrideNeverServesDockerBuildArgs(t *testing.T) {
+	for _, buildType := range []bean2.CiBuildType{bean2.SELF_DOCKERFILE_BUILD_TYPE, bean2.MANAGED_DOCKERFILE_BUILD_TYPE} {
+		t.Run(string(buildType)+"/stored args are dropped, the rest of the override is served", func(t *testing.T) {
+			svc := newReadService(t, nil, &fakeCiTemplateOverrideRepository{overrides: []*pipelineConfig.CiTemplateOverride{overrideRow(buildType, staleDockerOverrideMetadata)}})
+			for read, cfg := range readOverrideBuildConfigs(t, svc) {
+				require.NotNil(t, cfg.DockerBuildConfig, read)
+				assert.Nil(t, cfg.DockerBuildConfig.Args, read)
+				assert.Equal(t, buildType, cfg.CiBuildType, read)
+				assert.Equal(t, 41, cfg.Id, read)
+				assert.Equal(t, 31, cfg.GitMaterialId, read)
+				assert.Equal(t, 32, cfg.BuildContextGitMaterialId, read)
+				assert.Equal(t, "Dockerfile", cfg.DockerBuildConfig.DockerfilePath, read)
+				assert.Equal(t, "linux/arm64,linux/amd64", cfg.DockerBuildConfig.TargetPlatform, read)
+				assert.Equal(t, map[string]string{"network": "host"}, cfg.DockerBuildConfig.DockerBuildOptions, read)
+				assert.Equal(t, "ctx", cfg.DockerBuildConfig.BuildContext, read)
+			}
+		})
+	}
+
+	t.Run("override without args is served unchanged", func(t *testing.T) {
+		svc := newReadService(t, nil, &fakeCiTemplateOverrideRepository{overrides: []*pipelineConfig.CiTemplateOverride{
+			overrideRow(bean2.SELF_DOCKERFILE_BUILD_TYPE, `{"dockerfileRelativePath":"Dockerfile","targetPlatform":"linux/amd64"}`)}})
+		for read, cfg := range readOverrideBuildConfigs(t, svc) {
+			assert.Nil(t, cfg.DockerBuildConfig.Args, read)
+			assert.Equal(t, "linux/amd64", cfg.DockerBuildConfig.TargetPlatform, read)
+		}
+	})
+
+	t.Run("override without a ci_build_config row falls back to a dockerfile config without args", func(t *testing.T) {
+		row := overrideRow(bean2.SELF_DOCKERFILE_BUILD_TYPE, "")
+		row.CiBuildConfig = nil
+		svc := newReadService(t, nil, &fakeCiTemplateOverrideRepository{overrides: []*pipelineConfig.CiTemplateOverride{row}})
+		for read, cfg := range readOverrideBuildConfigs(t, svc) {
+			assert.Equal(t, bean2.SELF_DOCKERFILE_BUILD_TYPE, cfg.CiBuildType, read)
+			assert.Equal(t, "legacy/Dockerfile", cfg.DockerBuildConfig.DockerfilePath, read)
+			assert.Nil(t, cfg.DockerBuildConfig.Args, read)
+		}
+	})
+
+	t.Run("regression: buildpack override keeps its buildpack env args", func(t *testing.T) {
+		svc := newReadService(t, nil, &fakeCiTemplateOverrideRepository{overrides: []*pipelineConfig.CiTemplateOverride{
+			overrideRow(bean2.BUILDPACK_BUILD_TYPE, `{"builderId":"b","args":{"BP_ENV":"x"}}`)}})
+		for read, cfg := range readOverrideBuildConfigs(t, svc) {
+			assert.Nil(t, cfg.DockerBuildConfig, read)
+			assert.Equal(t, map[string]string{"BP_ENV": "x"}, cfg.BuildPackConfig.Args, read)
+		}
+	})
+
+	t.Run("regression: ci job (skip build) override", func(t *testing.T) {
+		svc := newReadService(t, nil, &fakeCiTemplateOverrideRepository{overrides: []*pipelineConfig.CiTemplateOverride{overrideRow(bean2.SKIP_BUILD_TYPE, "")}})
+		for read, cfg := range readOverrideBuildConfigs(t, svc) {
+			assert.Equal(t, bean2.SKIP_BUILD_TYPE, cfg.CiBuildType, read)
+			assert.Nil(t, cfg.DockerBuildConfig, read)
+		}
+	})
+}
+
+// Regression: app level build config owns the args and must keep serving them.
+func TestCiTemplateReadService_AppLevelKeepsDockerBuildArgs(t *testing.T) {
+	t.Run("app level build config row", func(t *testing.T) {
+		template := &pipelineConfig.CiTemplate{Id: 1, AppId: 1, CiBuildConfig: &pipelineConfig.CiBuildConfig{
+			Type: string(bean2.SELF_DOCKERFILE_BUILD_TYPE), BuildMetadata: `{"dockerfileRelativePath":"Dockerfile","args":{"APP":"1"},"targetPlatform":"linux/amd64"}`,
+		}}
+		svc := newReadService(t, &fakeCiTemplateRepository{template: template}, nil)
+		byAppId, err := svc.FindByAppId(1)
+		require.NoError(t, err)
+		assert.Equal(t, map[string]string{"APP": "1"}, byAppId.CiBuildConfig.DockerBuildConfig.Args)
+		assert.Equal(t, "linux/amd64", byAppId.CiBuildConfig.DockerBuildConfig.TargetPlatform)
+		byAppIds, err := svc.FindByAppIds([]int{1})
+		require.NoError(t, err)
+		assert.Equal(t, map[string]string{"APP": "1"}, byAppIds[1].CiBuildConfig.DockerBuildConfig.Args)
+	})
+	t.Run("legacy app level template without build config row", func(t *testing.T) {
+		template := &pipelineConfig.CiTemplate{Id: 1, AppId: 1, DockerfilePath: "Dockerfile", Args: `{"LEGACY":"x"}`, TargetPlatform: "linux/arm64"}
+		svc := newReadService(t, &fakeCiTemplateRepository{template: template}, nil)
+		byAppId, err := svc.FindByAppId(1)
+		require.NoError(t, err)
+		assert.Equal(t, map[string]string{"LEGACY": "x"}, byAppId.CiBuildConfig.DockerBuildConfig.Args)
+		byAppIds, err := svc.FindByAppIds([]int{1})
+		require.NoError(t, err)
+		assert.Equal(t, map[string]string{"LEGACY": "x"}, byAppIds[1].CiBuildConfig.DockerBuildConfig.Args)
+	})
+}

pkg/build/pipeline/bean/CiBuildConfig.go

@@ -41,6 +41,15 @@ type CiBuildConfigBean struct {
 	PipelineType              string             `json:"pipelineType"`
 }
 
+// ClearDockerBuildArgs drops docker build args from a CI pipeline's build config override.
+// Build args are not part of the override: an overridden pipeline builds with the app level
+// args, with the pipeline's own docker args applied on top at trigger time.
+func (b *CiBuildConfigBean) ClearDockerBuildArgs() {
+	if b != nil && b.DockerBuildConfig != nil {
+		b.DockerBuildConfig.Args = nil
+	}
+}
+
 type DockerBuildConfig struct {
 	DockerfilePath         string              `json:"dockerfileRelativePath,omitempty"`
 	DockerfileContent      string              `json:"dockerfileContent"`

pkg/build/pipeline/bean/CiBuildConfig_test.go

@@ -0,0 +1,98 @@
+/*
+ * Copyright (c) 2024. Devtron Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package bean
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestCiBuildConfigBean_ClearDockerBuildArgs(t *testing.T) {
+	t.Run("nil receiver is a no-op", func(t *testing.T) {
+		var b *CiBuildConfigBean
+		assert.NotPanics(t, func() { b.ClearDockerBuildArgs() })
+	})
+
+	t.Run("nil docker build config (buildpack / skip build) is a no-op", func(t *testing.T) {
+		for _, buildType := range []CiBuildType{BUILDPACK_BUILD_TYPE, SKIP_BUILD_TYPE} {
+			b := &CiBuildConfigBean{
+				CiBuildType:     buildType,
+				BuildPackConfig: &BuildPackConfig{BuilderId: "builder", Args: map[string]string{"BP_KEY": "v"}},
+			}
+			assert.NotPanics(t, func() { b.ClearDockerBuildArgs() }, buildType)
+			assert.Nil(t, b.DockerBuildConfig, buildType)
+			// buildpack env args are part of the override ("build the container image" section), never touched
+			assert.Equal(t, map[string]string{"BP_KEY": "v"}, b.BuildPackConfig.Args, buildType)
+		}
+	})
+
+	argShapes := map[string]map[string]string{
+		"nil args":       nil,
+		"empty args":     {},
+		"populated args": {"AWS_ACCESS_KEY_ID": "AKIA...", "AWS_SECRET_ACCESS_KEY": "secret", "FOO": "bar"},
+	}
+	for _, buildType := range []CiBuildType{SELF_DOCKERFILE_BUILD_TYPE, MANAGED_DOCKERFILE_BUILD_TYPE} {
+		for name, args := range argShapes {
+			t.Run(string(buildType)+"/"+name+" are cleared, every other field kept", func(t *testing.T) {
+				b := &CiBuildConfigBean{
+					Id:                        7,
+					GitMaterialId:             3,
+					BuildContextGitMaterialId: 4,
+					UseRootBuildContext:       true,
+					CiBuildType:               buildType,
+					PipelineType:              "CI_BUILD",
+					DockerBuildConfig: &DockerBuildConfig{
+						DockerfilePath:     "Dockerfile",
+						DockerfileContent:  "FROM alpine",
+						Args:               args,
+						TargetPlatform:     "linux/arm64,linux/amd64",
+						Language:           "go",
+						DockerBuildOptions: map[string]string{"network": "host"},
+						BuildContext:       "./ctx",
+						UseBuildx:          true,
+					},
+				}
+				b.ClearDockerBuildArgs()
+
+				assert.Nil(t, b.DockerBuildConfig.Args)
+				assert.Equal(t, 7, b.Id)
+				assert.Equal(t, 3, b.GitMaterialId)
+				assert.Equal(t, 4, b.BuildContextGitMaterialId)
+				assert.True(t, b.UseRootBuildContext)
+				assert.Equal(t, buildType, b.CiBuildType)
+				assert.Equal(t, "CI_BUILD", b.PipelineType)
+				d := b.DockerBuildConfig
+				assert.Equal(t, "Dockerfile", d.DockerfilePath)
+				assert.Equal(t, "FROM alpine", d.DockerfileContent)
+				// target platform IS part of the override and must survive
+				assert.Equal(t, "linux/arm64,linux/amd64", d.TargetPlatform)
+				assert.Equal(t, "go", d.Language)
+				assert.Equal(t, map[string]string{"network": "host"}, d.DockerBuildOptions)
+				assert.Equal(t, "./ctx", d.BuildContext)
+				assert.True(t, d.UseBuildx)
+			})
+		}
+	}
+
+	t.Run("idempotent", func(t *testing.T) {
+		b := &CiBuildConfigBean{CiBuildType: SELF_DOCKERFILE_BUILD_TYPE, DockerBuildConfig: &DockerBuildConfig{Args: map[string]string{"A": "1"}}}
+		b.ClearDockerBuildArgs()
+		b.ClearDockerBuildArgs()
+		assert.Nil(t, b.DockerBuildConfig.Args)
+	})
+}

pkg/build/trigger/HandlerService.go

@@ -1137,8 +1137,9 @@ func (impl *HandlerServiceImpl) buildWfRequestForCiPipeline(pipeline *pipelineCo
 		}
 		ciBuildConfigBean = templateOverrideBean.CiBuildConfig
 		// updating args coming from ciBaseBuildConfigEntity because it is not part of Ci override
-		if ciBuildConfigBean != nil && ciBuildConfigBean.DockerBuildConfig != nil && ciBaseBuildConfigBean != nil && ciBaseBuildConfigBean.DockerBuildConfig != nil {
-			ciBuildConfigBean.DockerBuildConfig.Args = ciBaseBuildConfigBean.DockerBuildConfig.Args
+		if err := adapter.InheritAppLevelDockerBuildArgs(ciBuildConfigBean, ciTemplate, ciBaseBuildConfigBean); err != nil {
+			impl.Logger.Errorw("error occurred while reading app level docker build args", "ciTemplateId", ciTemplate.Id, "err", err)
+			return nil, errors.New("error while parsing ci build config")
 		}
 		templateOverride := templateOverrideBean.CiTemplateOverride
 		checkoutPath = templateOverride.GitMaterial.CheckoutPath

pkg/pipeline/CiTemplateService.go

@@ -50,6 +50,9 @@ func (impl CiTemplateServiceImpl) Save(ciTemplateBean *bean.CiTemplateBean) erro
 	ciTemplateOverrideId := 0
 
 	buildConfig := ciTemplateBean.CiBuildConfig
+	if ciTemplateOverride != nil {
+		buildConfig.ClearDockerBuildArgs()
+	}
 	err := impl.CiBuildConfigService.Save(ciTemplateId, ciTemplateOverrideId, buildConfig, ciTemplateBean.UserId)
 	if err != nil {
 		impl.Logger.Errorw("error occurred while saving ci build config", "config", buildConfig, "err", err)
@@ -86,6 +89,8 @@ func (impl CiTemplateServiceImpl) Update(ciTemplateBean *bean.CiTemplateBean) er
 		ciTemplateId = ciTemplate.Id
 	} else {
 		ciTemplateOverrideId = ciTemplateOverride.Id
+		// the full build_metadata is rewritten from the request, so args echoed back by a client are dropped here
+		ciBuildConfig.ClearDockerBuildArgs()
 	}
 	_, err := impl.CiBuildConfigService.UpdateOrSave(ciTemplateId, ciTemplateOverrideId, ciBuildConfig, ciTemplateBean.UserId)
 	if err != nil {