From a5bfb4b0e422e1bd2065ed8ead2e12904bb8db7d Mon Sep 17 00:00:00 2001 From: Sofiyan-Shaikh <183908419+Sofiyan-Shaikh@users.noreply.github.com> Date: Mon, 18 May 2026 02:06:12 +0530 Subject: [PATCH] fix(deploy): don't write func.yaml before remote pipeline runs --- cmd/deploy.go | 11 ++- cmd/deploy_test.go | 72 +++++++++++++-- pkg/functions/function.go | 37 +++++--- pkg/pipelines/tekton/pipelines_provider.go | 49 ++++++++++- .../tekton/pipelines_provider_test.go | 87 +++++++++++++++++++ 5 files changed, 228 insertions(+), 28 deletions(-) diff --git a/cmd/deploy.go b/cmd/deploy.go index f7d7284647..f97236ef6b 100644 --- a/cmd/deploy.go +++ b/cmd/deploy.go @@ -318,12 +318,11 @@ func runDeploy(cmd *cobra.Command, newClient ClientFactory) (err error) { // Deploy if cfg.Remote { - // Write func.yaml before the pipeline uploads sources to the PVC, - // so that the on-cluster deploy step sees the latest config - // (e.g. --image-pull-secret, --service-account, --deployer). - if err = f.Write(); err != nil { - return - } + // NOTE: func.yaml is intentionally NOT written here. The remote + // pipeline's source upload injects the in-memory func.yaml (with + // any CLI overrides) into the tar stream, so the on-cluster deploy + // step sees the latest config without dirtying the working tree + // before a successful deploy (issue #3679). var url string // Invoke a remote build/push/deploy pipeline // Returned is the function with fields like Registry, f.Deploy.Image & diff --git a/cmd/deploy_test.go b/cmd/deploy_test.go index 540cb4e15f..bdffdb3bf7 100644 --- a/cmd/deploy_test.go +++ b/cmd/deploy_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "os" "path/filepath" "reflect" "strings" @@ -1912,9 +1913,12 @@ func TestDeploy_ImagePullSecretFromEnv(t *testing.T) { } } -// TestDeploy_ImagePullSecretRemote ensures that when deploying remotely, -// func.yaml is written to disk before the pipeline starts, so the on-cluster -// deploy step picks up the --image-pull-secret value. +// TestDeploy_ImagePullSecretRemote ensures that when deploying remotely the +// --image-pull-secret value reaches the pipeline via the in-memory function, +// and that func.yaml is NOT written to disk before the pipeline runs (the +// on-disk invariant from issue #3679). The actual byte-level injection into +// the upload tar stream is covered by TestSourcesAsTarStream; the mock +// pipeline provider here bypasses sourcesAsTarStream entirely. func TestDeploy_ImagePullSecretRemote(t *testing.T) { root := FromTempDirectory(t) @@ -1926,14 +1930,19 @@ func TestDeploy_ImagePullSecretRemote(t *testing.T) { pipelinesProvider := mock.NewPipelinesProvider() pipelinesProvider.RunFn = func(f fn.Function) (string, fn.Function, error) { - // Inside the pipeline Run, func.yaml on disk should already - // have the image pull secret written. + // The in-memory function passed to the pipeline must carry the + // CLI override so the on-cluster deploy step picks it up. + if f.Deploy.ImagePullSecret != "my-remote-secret" { + t.Fatalf("expected in-memory function to have imagePullSecret 'my-remote-secret', got '%v'", f.Deploy.ImagePullSecret) + } + // func.yaml on disk must NOT have been mutated before the pipeline + // runs: it should still reflect the pristine init state. diskFn, err := fn.NewFunction(root) if err != nil { t.Fatalf("failed to load func.yaml during pipeline Run: %v", err) } - if diskFn.Deploy.ImagePullSecret != "my-remote-secret" { - t.Fatalf("expected func.yaml on disk to have imagePullSecret 'my-remote-secret', got '%v'", diskFn.Deploy.ImagePullSecret) + if diskFn.Deploy.ImagePullSecret != "" { + t.Fatalf("expected func.yaml on disk to be unmodified before the pipeline runs, but it has imagePullSecret '%v'", diskFn.Deploy.ImagePullSecret) } f.Deploy.Namespace = "default" if f.Deploy.Image, err = f.ImageName(); err != nil { @@ -1956,6 +1965,55 @@ func TestDeploy_ImagePullSecretRemote(t *testing.T) { } } +// TestDeploy_RemoteDeployFailureKeepsWorkTreeClean reproduces issue #3679: +// a failed `func deploy --remote` must NOT mutate func.yaml on disk. The +// on-disk func.yaml is supposed to reflect deployed state only; a one-off +// CLI flag (here --image-pull-secret) on a failed deploy should leave the +// working tree clean so the user can retry with different flags. +// +// On current code this FAILS because cmd/deploy.go calls f.Write() before +// the pipeline runs (regression introduced by #3663). +func TestDeploy_RemoteDeployFailureKeepsWorkTreeClean(t *testing.T) { + root := FromTempDirectory(t) + + f := fn.Function{Runtime: "go", Root: root, Registry: TestRegistry} + if _, err := fn.New().Init(f); err != nil { + t.Fatal(err) + } + + // Snapshot func.yaml exactly as it sits on disk after init. + funcYamlPath := filepath.Join(root, fn.FunctionFile) + before, err := os.ReadFile(funcYamlPath) + if err != nil { + t.Fatal(err) + } + + // A pipeline that fails, simulating any remote deploy failure. + pipelinesProvider := mock.NewPipelinesProvider() + pipelinesProvider.RunFn = func(f fn.Function) (string, fn.Function, error) { + return "", f, errors.New("simulated remote pipeline failure") + } + + cmd := NewDeployCmd(NewTestClient( + fn.WithPipelinesProvider(pipelinesProvider), + fn.WithRegistry(TestRegistry), + )) + cmd.SetArgs([]string{"--remote", "--image-pull-secret=my-remote-secret", "--namespace=default"}) + if err := cmd.Execute(); err == nil { + t.Fatal("expected the remote deploy to fail, but it succeeded") + } + + after, err := os.ReadFile(funcYamlPath) + if err != nil { + t.Fatal(err) + } + + if string(before) != string(after) { + t.Fatalf("issue #3679 reproduced: func.yaml on disk was mutated by a FAILED "+ + "remote deploy.\n--- before ---\n%s\n--- after ---\n%s", before, after) + } +} + // Test_ValidateBuilder tests that the builder validation accepts the // set of known builders, and spot-checks an error is thrown for unknown. func Test_ValidateBuilder(t *testing.T) { diff --git a/pkg/functions/function.go b/pkg/functions/function.go index 09d2506273..0581904143 100644 --- a/pkg/functions/function.go +++ b/pkg/functions/function.go @@ -459,9 +459,9 @@ func (f Function) Write() (err error) { return } - // Write + // Serialize the function to its on-disk func.yaml representation var bb []byte - if bb, err = yaml.Marshal(&f); err != nil { + if bb, err = f.MarshalYAML(); err != nil { return } // TODO: open existing file for writing, such that existing permissions @@ -472,18 +472,7 @@ func (f Function) Write() (err error) { } defer rwFile.Close() - schemaURI := funcYamlSchemaURI() - - // Write schema header - schemaHeader := fmt.Sprintf(`# $schema: %s -# yaml-language-server: $schema=%s -`, schemaURI, schemaURI) - - if _, err = rwFile.WriteString(schemaHeader); err != nil { - return err - } - - // Write function data + // Write function data (schema header + serialized struct) if _, err = rwFile.Write(bb); err != nil { return err } @@ -507,6 +496,26 @@ func (f Function) Write() (err error) { return } +// MarshalYAML returns the function serialized into its on-disk func.yaml +// representation: the schema header followed by the YAML-marshaled struct. +// This is the single source of truth for func.yaml's byte content, shared +// by Write (persisting to disk after a successful deploy) and by the remote +// pipeline source upload (injecting the in-memory config into the tar stream +// without dirtying the working tree). +func (f Function) MarshalYAML() ([]byte, error) { + bb, err := yaml.Marshal(&f) + if err != nil { + return nil, err + } + + schemaURI := funcYamlSchemaURI() + schemaHeader := fmt.Sprintf(`# $schema: %s +# yaml-language-server: $schema=%s +`, schemaURI, schemaURI) + + return append([]byte(schemaHeader), bb...), nil +} + func funcYamlSchemaURI() string { var ( ref = "main" diff --git a/pkg/pipelines/tekton/pipelines_provider.go b/pkg/pipelines/tekton/pipelines_provider.go index 15182ecc12..d0f515871f 100644 --- a/pkg/pipelines/tekton/pipelines_provider.go +++ b/pkg/pipelines/tekton/pipelines_provider.go @@ -299,7 +299,40 @@ func sourcesAsTarStream(f fn.Function) *io.PipeReader { go func() { tw := tar.NewWriter(pw) - err := tw.WriteHeader(&tar.Header{ + // Serialize the in-memory function (with any one-off CLI overrides + // applied) and inject it into the stream as func.yaml, rather than + // reading the on-disk file. This lets the on-cluster deploy step see + // the latest config without dirtying the user's working tree before + // the deploy succeeds (issue #3679). + funcYamlBytes, err := f.MarshalYAML() + if err != nil { + _ = pw.CloseWithError(fmt.Errorf("error while serializing func.yaml for tar stream: %w", err)) + return + } + + writeFuncYaml := func(mode int64) error { + hdr := &tar.Header{ + Typeflag: tar.TypeReg, + Name: path.Join("source", fn.FunctionFile), + Size: int64(len(funcYamlBytes)), + Mode: mode, + ModTime: time.Now(), + Uid: nobodyID, + Gid: nobodyID, + Uname: "nobody", + Gname: "nobody", + } + if err := tw.WriteHeader(hdr); err != nil { + return fmt.Errorf("cannot write func.yaml header to tar stream: %w", err) + } + if _, err := tw.Write(funcYamlBytes); err != nil { + return fmt.Errorf("cannot write func.yaml content to tar stream: %w", err) + } + return nil + } + funcYamlInjected := false + + err = tw.WriteHeader(&tar.Header{ Typeflag: tar.TypeDir, Name: "source/", Mode: 0777, @@ -330,6 +363,16 @@ func sourcesAsTarStream(f fn.Function) *io.PipeReader { return nil } + // Replace the on-disk func.yaml with the in-memory serialization + // so CLI overrides reach the on-cluster deploy step (issue #3679). + if relp == fn.FunctionFile && fi.Mode().IsRegular() { + if err := writeFuncYaml(int64(fi.Mode().Perm())); err != nil { + return err + } + funcYamlInjected = true + return nil + } + lnk := "" if fi.Mode()&fs.ModeSymlink != 0 { lnk, err = os.Readlink(p) @@ -381,6 +424,10 @@ func sourcesAsTarStream(f fn.Function) *io.PipeReader { } return nil }) + if err == nil && !funcYamlInjected { + // Function was never persisted to disk: inject func.yaml explicitly. + err = writeFuncYaml(0o644) + } if err != nil { _ = pw.CloseWithError(fmt.Errorf("error while creating tar stream from sources: %w", err)) } else { diff --git a/pkg/pipelines/tekton/pipelines_provider_test.go b/pkg/pipelines/tekton/pipelines_provider_test.go index 4d65c99c1e..6b7e254ce0 100644 --- a/pkg/pipelines/tekton/pipelines_provider_test.go +++ b/pkg/pipelines/tekton/pipelines_provider_test.go @@ -81,6 +81,93 @@ func TestSourcesAsTarStream(t *testing.T) { } } +// readFuncYamlFromStream returns the content of source/func.yaml from the +// tar stream, or fails if it is absent. +func readFuncYamlFromStream(t *testing.T, rc io.ReadCloser) []byte { + t.Helper() + tr := tar.NewReader(rc) + for { + hdr, err := tr.Next() + if err != nil { + if errors.Is(err, io.EOF) { + break + } + t.Fatal(err) + } + if hdr.Name == "source/"+fn.FunctionFile { + b, err := io.ReadAll(tr) + if err != nil { + t.Fatal(err) + } + return b + } + } + t.Fatalf("source/%s missing from tar stream", fn.FunctionFile) + return nil +} + +// TestSourcesAsTarStream_FuncYamlInjection reproduces the fix for issue +// #3679: the func.yaml placed into the upload tar stream must reflect the +// in-memory function (with CLI overrides applied), NOT the stale on-disk +// file, and streaming must not mutate the on-disk file. +func TestSourcesAsTarStream_FuncYamlInjection(t *testing.T) { + t.Run("overrides on-disk func.yaml", func(t *testing.T) { + root := t.TempDir() + + // Stale on-disk func.yaml (as if from a previous deploy). + onDisk := fn.Function{Root: root, Runtime: "go", Name: "fn"} + onDisk.Deploy.ImagePullSecret = "stale-on-disk" + if err := onDisk.Write(); err != nil { + t.Fatal(err) + } + before, err := os.ReadFile(filepath.Join(root, fn.FunctionFile)) + if err != nil { + t.Fatal(err) + } + + // In-memory function with a one-off CLI override. + f := fn.Function{Root: root, Runtime: "go", Name: "fn"} + f.Deploy.ImagePullSecret = "from-cli" + + rc := sourcesAsTarStream(f) + t.Cleanup(func() { _ = rc.Close() }) + + streamed := readFuncYamlFromStream(t, rc) + if !strings.Contains(string(streamed), "imagePullSecret: from-cli") { + t.Fatalf("tar func.yaml did not reflect in-memory override; got:\n%s", streamed) + } + if strings.Contains(string(streamed), "stale-on-disk") { + t.Fatalf("tar func.yaml contains stale on-disk value; got:\n%s", streamed) + } + + // The on-disk file must be untouched by streaming. + after, err := os.ReadFile(filepath.Join(root, fn.FunctionFile)) + if err != nil { + t.Fatal(err) + } + if string(before) != string(after) { + t.Fatalf("sourcesAsTarStream mutated on-disk func.yaml.\nbefore:\n%s\nafter:\n%s", before, after) + } + }) + + t.Run("injects when func.yaml absent on disk", func(t *testing.T) { + root := t.TempDir() + f := fn.Function{Root: root, Runtime: "go", Name: "fn"} + f.Deploy.ImagePullSecret = "from-cli" + + rc := sourcesAsTarStream(f) + t.Cleanup(func() { _ = rc.Close() }) + + streamed := readFuncYamlFromStream(t, rc) + if !strings.Contains(string(streamed), "imagePullSecret: from-cli") { + t.Fatalf("tar func.yaml not injected for never-saved function; got:\n%s", streamed) + } + if _, err := os.Stat(filepath.Join(root, fn.FunctionFile)); !os.IsNotExist(err) { + t.Fatalf("sourcesAsTarStream created func.yaml on disk; err=%v", err) + } + }) +} + func Test_createPipelinePersistentVolumeClaim(t *testing.T) { type mockType func(ctx context.Context, name, namespaceOverride string, labels map[string]string, annotations map[string]string, accessMode corev1.PersistentVolumeAccessMode, resourceRequest resource.Quantity, storageClass string) (err error)