From 64621312014d9d749790bfee13cb459895a0cd5f Mon Sep 17 00:00:00 2001 From: ericzzzzzzz <102683393+ericzzzzzzz@users.noreply.github.com> Date: Wed, 21 Aug 2024 11:54:07 -0400 Subject: [PATCH] fix: keep the original template if template expansion fails (#9503) (#9504) * fix: keep the original template if template expansion fails --- integration/diagnose_test.go | 61 ++++++++++++++++++- .../diagnose/not-all-envs-set/skaffold.yaml | 19 ++++++ pkg/skaffold/tags/templates.go | 17 +++--- pkg/skaffold/tags/templates_test.go | 12 ++-- 4 files changed, 91 insertions(+), 18 deletions(-) create mode 100644 integration/testdata/diagnose/not-all-envs-set/skaffold.yaml diff --git a/integration/diagnose_test.go b/integration/diagnose_test.go index ccd2c22008f..504f31af130 100644 --- a/integration/diagnose_test.go +++ b/integration/diagnose_test.go @@ -25,6 +25,9 @@ import ( "text/template" "github.com/GoogleContainerTools/skaffold/v2/integration/skaffold" + "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/schema/latest" + "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/schema/util" + "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/yaml" "github.com/GoogleContainerTools/skaffold/v2/testutil" ) @@ -83,12 +86,12 @@ func TestDiagnose(t *testing.T) { configContents, err := os.ReadFile(filepath.Join(test.dir, "skaffold.yaml")) t.CheckNoError(err) templ, err := os.ReadFile(filepath.Join(test.dir, "diagnose.tmpl")) + t.CheckNoError(err) tmpDir.Write("skaffold.yaml", string(configContents)) args := []string{"--yaml-only", "--output", tmpDir.Path(test.outputFile), "-f", tmpDir.Path("skaffold.yaml")} args = append(args, test.args...) skaffold.Diagnose(args...). InDir(test.dir).RunOrFail(t.T) - t.CheckNoError(err) outTemplate := template.Must(template.New("tmpl").Parse(string(templ))) cwd, err := filepath.Abs(test.dir) t.CheckNoError(err) @@ -96,7 +99,6 @@ func TestDiagnose(t *testing.T) { outTemplate.Execute(expected, map[string]string{"Root": cwd}) outputPath := tmpDir.Path(test.outputFile) - t.CheckNoError(err) out, err := os.ReadFile(outputPath) t.CheckNoError(err) t.CheckDeepEqual(expected.String(), string(out), testutil.YamlObj(t.T)) @@ -104,6 +106,61 @@ func TestDiagnose(t *testing.T) { } } +// During the schema upgrade(v2beta28->v2beta29), Skaffold injects setTemplate fields into the configuration if a legacy Helm deployer is configured. +// These injected fields contain templates, and we want to ensure that when expanding them with Go templates, the original field values remain unchanged +// when environment variables are not set. This is important because users who use the skaffold diagnose command on the old schema +// with Helm might not be aware of the existence of these templated fields, leading to templating failures. +func TestDiagnoseTemplatingNotAllEnvsSet(t *testing.T) { + tests := []struct { + description string + dir string + outputFile string + args []string + envs map[string]string + }{ + { + description: "apply replacements to templates in skaffold.yaml", + dir: "testdata/diagnose/not-all-envs-set", + outputFile: "abc.txt", + args: []string{"--enable-templating"}, + envs: map[string]string{"AAA": "aaa"}, + }, + } + + for _, test := range tests { + MarkIntegrationTest(t, CanRunWithoutGcp) + testutil.Run(t, test.description, func(t *testutil.T) { + if test.envs != nil { + for k, v := range test.envs { + t.Setenv(k, v) + } + } + tmpDir := testutil.NewTempDir(t.T) + configContents, err := os.ReadFile(filepath.Join(test.dir, "skaffold.yaml")) + t.CheckNoError(err) + tmpDir.Write("skaffold.yaml", string(configContents)) + outputPath := tmpDir.Path(test.outputFile) + args := []string{"--yaml-only", "--output", outputPath, "-f", tmpDir.Path("skaffold.yaml")} + args = append(args, test.args...) + skaffold.Diagnose(args...). + InDir(test.dir).RunOrFail(t.T) + out, err := os.ReadFile(outputPath) + t.CheckNoError(err) + var conf latest.SkaffoldConfig + yaml.Unmarshal(out, &conf) + // templates unchanged + t.CheckDeepEqual(conf.Deploy.LegacyHelmDeploy.Releases[0].SetValueTemplates, util.FlatMap{"image.repository": "{{.IMAGE_REPO_test_image}}", + "image.tag": "{{.IMAGE_TAG_test_image}}@{{.IMAGE_DIGEST_test_image}}", + }) + cwd, err := filepath.Abs(test.dir) + t.CheckNoError(err) + + // templates successfully expanded. + t.CheckDeepEqual(conf.Deploy.LegacyHelmDeploy.Releases[0].ValuesFiles[0], cwd+"/aaa/test-values.yaml") + }) + } +} + func folders(root string) ([]string, error) { var folders []string diff --git a/integration/testdata/diagnose/not-all-envs-set/skaffold.yaml b/integration/testdata/diagnose/not-all-envs-set/skaffold.yaml new file mode 100644 index 00000000000..406166ccdb7 --- /dev/null +++ b/integration/testdata/diagnose/not-all-envs-set/skaffold.yaml @@ -0,0 +1,19 @@ +apiVersion: skaffold/v2beta26 +kind: Config +metadata: + name: cio-cloud-project-mgmt-api +build: + artifacts: + - image: test-image +deploy: + helm: + releases: + - name: test-release + artifactOverrides: + image: test-image + valuesFiles: + - '{{.AAA}}/test-values.yaml' + remoteChart: "oci://test-chart" + version: 1.21.2 + imageStrategy: + helm: {} \ No newline at end of file diff --git a/pkg/skaffold/tags/templates.go b/pkg/skaffold/tags/templates.go index 11838717870..b142b83b5d7 100644 --- a/pkg/skaffold/tags/templates.go +++ b/pkg/skaffold/tags/templates.go @@ -17,7 +17,6 @@ limitations under the License. package tags import ( - "fmt" "reflect" "slices" "strings" @@ -118,13 +117,13 @@ func expandTemplate(v reflect.Value) error { switch v.Kind() { case reflect.String: updated, err := util.ExpandEnvTemplate(v.String(), nil) - if strings.Contains(updated, "") { - return fmt.Errorf("environment variables missing for template keys") - } if err != nil { return err } - v.SetString(updated) + // we want to keep the original template if expanding fails, otherwise update the value with expanded result. + if !strings.Contains(updated, "") { + v.SetString(updated) + } case reflect.Ptr: return expandTemplate(v.Elem()) case reflect.Slice, reflect.Array: @@ -142,13 +141,13 @@ func expandTemplate(v reflect.Value) error { } } else if vv.Kind() == reflect.String { updated, err := util.ExpandEnvTemplate(vv.String(), nil) - if strings.Contains(updated, "") { - return fmt.Errorf("environment variables missing for template keys") - } if err != nil { return err } - v.SetMapIndex(key, reflect.ValueOf(updated)) + // we want to keep the original template if expanding fails, otherwise update the value with expanded result. + if !strings.Contains(updated, "") { + v.SetMapIndex(key, reflect.ValueOf(updated)) + } } } } diff --git a/pkg/skaffold/tags/templates_test.go b/pkg/skaffold/tags/templates_test.go index 0e25c27899c..66f079c3bee 100644 --- a/pkg/skaffold/tags/templates_test.go +++ b/pkg/skaffold/tags/templates_test.go @@ -75,11 +75,10 @@ func TestApplyTemplates(t *testing.T) { envs: map[string]string{"SECOND": "second", "THIRD": "third"}, }, { - name: "Map of strings, no value", - input: testStruct{MapString: map[string]string{"first": "first", "second": "{{.SECOND}}", "third": "{{.THIRD}}"}}, - want: testStruct{MapString: map[string]string{"first": "first", "second": "{{.SECOND}}", "third": "{{.THIRD}}"}}, - wantErr: true, - envs: map[string]string{}, + name: "Map of strings, keep the original template", + input: testStruct{MapString: map[string]string{"first": "first", "second": "{{.SECOND}}", "third": "{{.THIRD}}"}}, + want: testStruct{MapString: map[string]string{"first": "first", "second": "{{.SECOND}}", "third": "{{.THIRD}}"}}, + envs: map[string]string{}, }, { name: "Map of pointers to strings", @@ -108,14 +107,13 @@ func TestApplyTemplates(t *testing.T) { wantErr: false, }, { - name: "string not found - ", + name: "string not found - keep the original template", input: testStruct{ SimpleString: "{{ .ENV_VAR }}", }, want: testStruct{ SimpleString: "{{ .ENV_VAR }}", }, - wantErr: true, }, { name: "invalid template",