From d8e373ba34cde2b95c9ba612be9d19595431aae3 Mon Sep 17 00:00:00 2001 From: Ankitsinghsisodya Date: Sun, 5 Apr 2026 15:36:00 +0530 Subject: [PATCH 1/2] Warn when legacy .s2i/bin/assemble scaffolding is detected Prior to knative/func#3436, func wrote S2I scaffolding for go/python to /.s2i/bin/assemble. It now lives at .func/build/bin/assemble. The stale file can interfere with other builders (e.g. pack erroneously detects the assemble script). Add WarnIfLegacyS2IScaffolding helper in pkg/s2i that checks only scaffolded runtimes (go/python) and only the exact file func generated, so user-managed .s2i directories are not flagged. Call it from both the local Build() path and the remote PipelinesProvider.Run() path so the warning is emitted regardless of how the function is built. Fixes #3452 --- pkg/pipelines/tekton/pipelines_provider.go | 5 ++ pkg/s2i/builder.go | 2 + pkg/s2i/builder_test.go | 92 ++++++++++++++++++++++ pkg/s2i/scaffolder.go | 24 ++++++ 4 files changed, 123 insertions(+) diff --git a/pkg/pipelines/tekton/pipelines_provider.go b/pkg/pipelines/tekton/pipelines_provider.go index 540f995c0d..3be5764f36 100644 --- a/pkg/pipelines/tekton/pipelines_provider.go +++ b/pkg/pipelines/tekton/pipelines_provider.go @@ -33,6 +33,7 @@ import ( fnlabels "knative.dev/func/pkg/k8s/labels" "knative.dev/func/pkg/knative" "knative.dev/func/pkg/oci" + "knative.dev/func/pkg/s2i" "knative.dev/pkg/apis" ) @@ -113,6 +114,10 @@ func (pp *PipelinesProvider) Run(ctx context.Context, f fn.Function) (string, fn return "", f, err } + // Warn if the func-generated legacy .s2i/bin/assemble exists; it will be + // uploaded to the PVC and can interfere with the in-cluster build. + s2i.WarnIfLegacyS2IScaffolding(f, os.Stderr) + // Namespace is either a new namespace, specified as f.Namespace, or // the currently deployed namespace, recorded on f.Deploy.Namespace. // If neither exist, an error is returned (namespace is required) diff --git a/pkg/s2i/builder.go b/pkg/s2i/builder.go index 5b7793dbbb..3d9b0937c6 100644 --- a/pkg/s2i/builder.go +++ b/pkg/s2i/builder.go @@ -131,6 +131,8 @@ func (b *Builder) Build(ctx context.Context, f fn.Function, platforms []fn.Platf client = c } + WarnIfLegacyS2IScaffolding(f, os.Stderr) + // Link .s2iignore -> .funcignore funcignorePath := filepath.Join(f.Root, ".funcignore") s2iignorePath := filepath.Join(f.Root, ".s2iignore") diff --git a/pkg/s2i/builder_test.go b/pkg/s2i/builder_test.go index ba685f7c70..5c416bdc7a 100644 --- a/pkg/s2i/builder_test.go +++ b/pkg/s2i/builder_test.go @@ -1,11 +1,13 @@ package s2i_test import ( + "bytes" "context" "errors" "io" "os" "path/filepath" + "strings" "testing" "github.com/docker/docker/api/types" @@ -268,6 +270,96 @@ func TestBuildFail(t *testing.T) { } } +// Test_LegacyS2IAssembleWarning ensures that a warning is printed to stderr +// when the func-generated legacy .s2i/bin/assemble file is detected at the +// function root for a scaffolded runtime (go/python). +func Test_LegacyS2IAssembleWarning(t *testing.T) { + tests := []struct { + name string + runtime string + createFile bool + wantWarning bool + }{ + { + name: "scaffolded runtime with legacy assemble warns", + runtime: "go", + createFile: true, + wantWarning: true, + }, + { + name: "non-scaffolded runtime with .s2i/bin/assemble does not warn", + runtime: "node", + createFile: true, + wantWarning: false, + }, + { + name: "scaffolded runtime without legacy assemble does not warn", + runtime: "go", + createFile: false, + wantWarning: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + root, done := Mktemp(t) + defer done() + + // Initialize the function so middleware version detection succeeds + // (required for scaffolded runtimes like go/python). + f, err := fn.New().Init(fn.Function{ + Name: "test", + Root: root, + Runtime: tt.runtime, + Registry: "example.com/alice", + }) + if err != nil { + t.Fatal(err) + } + + if tt.createFile { + binDir := filepath.Join(root, ".s2i", "bin") + if err := os.MkdirAll(binDir, 0755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(binDir, "assemble"), []byte("#!/bin/bash"), 0700); err != nil { + t.Fatal(err) + } + } + + oldStderr := os.Stderr + r, w, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + os.Stderr = w + + i := &mockImpl{BuildFn: func(cfg *api.Config) (*api.Result, error) { return nil, nil }} + b := s2i.NewBuilder(s2i.WithImpl(i), s2i.WithDockerClient(mockDocker{})) + buildErr := b.Build(t.Context(), f, nil) + + w.Close() + os.Stderr = oldStderr + + var buf bytes.Buffer + if _, err := io.Copy(&buf, r); err != nil { + t.Fatal(err) + } + + if buildErr != nil { + t.Fatal(buildErr) + } + warned := strings.Contains(buf.String(), ".s2i/bin/assemble") + if tt.wantWarning && !warned { + t.Fatalf("expected a warning about .s2i/bin/assemble, got: %q", buf.String()) + } + if !tt.wantWarning && warned { + t.Fatalf("unexpected warning about .s2i/bin/assemble: %q", buf.String()) + } + }) + } +} + // mockImpl is a mock implementation of an S2I builder. type mockImpl struct { BuildFn func(*api.Config) (*api.Result, error) diff --git a/pkg/s2i/scaffolder.go b/pkg/s2i/scaffolder.go index e1a313c58e..23563b9025 100644 --- a/pkg/s2i/scaffolder.go +++ b/pkg/s2i/scaffolder.go @@ -3,6 +3,7 @@ package s2i import ( "context" "fmt" + "io" "os" "path/filepath" @@ -63,3 +64,26 @@ func (s Scaffolder) Scaffold(ctx context.Context, f fn.Function, path string) er } return nil } + +// WarnIfLegacyS2IScaffolding writes a warning to w when the func-generated +// legacy .s2i/bin/assemble file is present at the function root. +// Prior to https://github.com/knative/func/pull/3436, func wrote S2I +// scaffolding for go/python to /.s2i/bin/assemble; it now lives +// at .func/build/bin/assemble. The stale file can interfere with other +// builders (e.g. pack erroneously detects the assemble script). +// The check is intentionally scoped to scaffolded runtimes (go/python) and +// to the exact file func generated, so user-managed .s2i directories are not +// flagged. +func WarnIfLegacyS2IScaffolding(f fn.Function, w io.Writer) { + if !f.HasScaffolding() { + return + } + legacyAssemble := filepath.Join(f.Root, ".s2i", "bin", "assemble") + if _, err := os.Stat(legacyAssemble); err == nil { + fmt.Fprintf(w, "Warning: a '.s2i/bin/assemble' file was detected at the function root. "+ + "This appears to be leftover func-generated scaffolding from before it was moved to '%s/%s/bin/assemble'. "+ + "It may interfere with other builders. "+ + "If it was generated by func, consider removing '.s2i/' after verifying it is no longer needed.\n", + fn.RunDataDir, fn.BuildDir) + } +} From d2496ffb4987de6cff1bafb9c4cbe497b3531816 Mon Sep 17 00:00:00 2001 From: Ankitsinghsisodya Date: Fri, 10 Apr 2026 08:40:05 +0530 Subject: [PATCH 2/2] Move legacy .s2i warning into Client.Build Per review feedback on #3584, the warning helper now lives in pkg/functions as a standalone WarnIfLegacyS2IScaffolding and is invoked from Client.Build, so it fires for every build (and therefore every local deploy) regardless of which builder the user switched to. The Tekton pipelines provider keeps its own call for remote deploys (which don't go through Client.Build) and no longer needs to import pkg/s2i. The s2i builder's redundant call is removed. The test moves to pkg/functions and exercises the helper directly. --- pkg/functions/client.go | 7 ++ pkg/functions/function.go | 24 ++++++ pkg/functions/function_unit_test.go | 62 +++++++++++++++ pkg/pipelines/tekton/pipelines_provider.go | 4 +- pkg/s2i/builder.go | 2 - pkg/s2i/builder_test.go | 92 ---------------------- pkg/s2i/scaffolder.go | 24 ------ 7 files changed, 95 insertions(+), 120 deletions(-) diff --git a/pkg/functions/client.go b/pkg/functions/client.go index d8a7a11de7..75a5e8d3d7 100644 --- a/pkg/functions/client.go +++ b/pkg/functions/client.go @@ -691,6 +691,13 @@ func BuildWithPlatforms(pp []Platform) BuildOption { // not contain a populated Image. func (c *Client) Build(ctx context.Context, f Function, options ...BuildOption) (Function, error) { fmt.Fprintf(os.Stderr, "Building function image\n") + + // Warn if a func-generated legacy .s2i/bin/assemble file is left at the + // function root. This runs on every build (and therefore every deploy, + // which uses build under the hood) so users see the warning regardless + // of which builder they switched to. + WarnIfLegacyS2IScaffolding(f, os.Stderr) + ctx, cancel := context.WithCancel(ctx) defer cancel() diff --git a/pkg/functions/function.go b/pkg/functions/function.go index 07504c1103..f1b7253b55 100644 --- a/pkg/functions/function.go +++ b/pkg/functions/function.go @@ -3,6 +3,7 @@ package functions import ( "errors" "fmt" + "io" "os" "path/filepath" "reflect" @@ -601,6 +602,29 @@ func (f Function) HasScaffolding() bool { return f.Runtime == "go" || f.Runtime == "python" } +// WarnIfLegacyS2IScaffolding writes a warning to w when the func-generated +// legacy .s2i/bin/assemble file is present at the function root. +// Prior to https://github.com/knative/func/pull/3436, func wrote S2I +// scaffolding for go/python to /.s2i/bin/assemble; it now lives +// at .func/build/bin/assemble. The stale file can interfere with other +// builders (e.g. pack erroneously detects the assemble script). +// The check is intentionally scoped to scaffolded runtimes (go/python) and +// to the exact file func generated, so user-managed .s2i directories are not +// flagged. +func WarnIfLegacyS2IScaffolding(f Function, w io.Writer) { + if !f.HasScaffolding() { + return + } + legacyAssemble := filepath.Join(f.Root, ".s2i", "bin", "assemble") + if _, err := os.Stat(legacyAssemble); err == nil { + fmt.Fprintf(w, "Warning: a '.s2i/bin/assemble' file was detected at the function root. "+ + "This appears to be leftover func-generated scaffolding from before it was moved to '%s/%s/bin/assemble'. "+ + "It may interfere with other builders. "+ + "If it was generated by func, consider removing '.s2i/' after verifying it is no longer needed.\n", + RunDataDir, BuildDir) + } +} + // LabelsMap combines default labels with the labels slice provided. // It will the resulting slice with ValidateLabels and return a key/value map. // - key: EXAMPLE1 # Label directly from a value diff --git a/pkg/functions/function_unit_test.go b/pkg/functions/function_unit_test.go index 7a0715db9e..a5b19b7a03 100644 --- a/pkg/functions/function_unit_test.go +++ b/pkg/functions/function_unit_test.go @@ -1,9 +1,11 @@ package functions_test import ( + "bytes" "os" "path/filepath" "reflect" + "strings" "testing" "gopkg.in/yaml.v2" @@ -296,3 +298,63 @@ func expectedDefaultLabels(f fn.Function) map[string]string { fnlabels.FunctionRuntimeKey: f.Runtime, } } + +// TestWarnIfLegacyS2IScaffolding ensures the warning is emitted only when a +// func-generated legacy .s2i/bin/assemble file is present at the function root +// for a scaffolded runtime (go/python). +func TestWarnIfLegacyS2IScaffolding(t *testing.T) { + tests := []struct { + name string + runtime string + createFile bool + wantWarning bool + }{ + { + name: "scaffolded runtime with legacy assemble warns", + runtime: "go", + createFile: true, + wantWarning: true, + }, + { + name: "non-scaffolded runtime with .s2i/bin/assemble does not warn", + runtime: "node", + createFile: true, + wantWarning: false, + }, + { + name: "scaffolded runtime without legacy assemble does not warn", + runtime: "go", + createFile: false, + wantWarning: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + root, cleanup := Mktemp(t) + t.Cleanup(cleanup) + + if tt.createFile { + binDir := filepath.Join(root, ".s2i", "bin") + if err := os.MkdirAll(binDir, 0755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(binDir, "assemble"), []byte("#!/bin/bash"), 0700); err != nil { + t.Fatal(err) + } + } + + f := fn.Function{Root: root, Runtime: tt.runtime} + var buf bytes.Buffer + fn.WarnIfLegacyS2IScaffolding(f, &buf) + + warned := strings.Contains(buf.String(), ".s2i/bin/assemble") + if tt.wantWarning && !warned { + t.Fatalf("expected a warning about .s2i/bin/assemble, got: %q", buf.String()) + } + if !tt.wantWarning && warned { + t.Fatalf("unexpected warning about .s2i/bin/assemble: %q", buf.String()) + } + }) + } +} diff --git a/pkg/pipelines/tekton/pipelines_provider.go b/pkg/pipelines/tekton/pipelines_provider.go index 3be5764f36..c51fd198ed 100644 --- a/pkg/pipelines/tekton/pipelines_provider.go +++ b/pkg/pipelines/tekton/pipelines_provider.go @@ -33,7 +33,6 @@ import ( fnlabels "knative.dev/func/pkg/k8s/labels" "knative.dev/func/pkg/knative" "knative.dev/func/pkg/oci" - "knative.dev/func/pkg/s2i" "knative.dev/pkg/apis" ) @@ -116,7 +115,8 @@ func (pp *PipelinesProvider) Run(ctx context.Context, f fn.Function) (string, fn // Warn if the func-generated legacy .s2i/bin/assemble exists; it will be // uploaded to the PVC and can interfere with the in-cluster build. - s2i.WarnIfLegacyS2IScaffolding(f, os.Stderr) + // Remote deploy doesn't go through Client.Build, so we re-check here. + fn.WarnIfLegacyS2IScaffolding(f, os.Stderr) // Namespace is either a new namespace, specified as f.Namespace, or // the currently deployed namespace, recorded on f.Deploy.Namespace. diff --git a/pkg/s2i/builder.go b/pkg/s2i/builder.go index 3d9b0937c6..5b7793dbbb 100644 --- a/pkg/s2i/builder.go +++ b/pkg/s2i/builder.go @@ -131,8 +131,6 @@ func (b *Builder) Build(ctx context.Context, f fn.Function, platforms []fn.Platf client = c } - WarnIfLegacyS2IScaffolding(f, os.Stderr) - // Link .s2iignore -> .funcignore funcignorePath := filepath.Join(f.Root, ".funcignore") s2iignorePath := filepath.Join(f.Root, ".s2iignore") diff --git a/pkg/s2i/builder_test.go b/pkg/s2i/builder_test.go index 5c416bdc7a..ba685f7c70 100644 --- a/pkg/s2i/builder_test.go +++ b/pkg/s2i/builder_test.go @@ -1,13 +1,11 @@ package s2i_test import ( - "bytes" "context" "errors" "io" "os" "path/filepath" - "strings" "testing" "github.com/docker/docker/api/types" @@ -270,96 +268,6 @@ func TestBuildFail(t *testing.T) { } } -// Test_LegacyS2IAssembleWarning ensures that a warning is printed to stderr -// when the func-generated legacy .s2i/bin/assemble file is detected at the -// function root for a scaffolded runtime (go/python). -func Test_LegacyS2IAssembleWarning(t *testing.T) { - tests := []struct { - name string - runtime string - createFile bool - wantWarning bool - }{ - { - name: "scaffolded runtime with legacy assemble warns", - runtime: "go", - createFile: true, - wantWarning: true, - }, - { - name: "non-scaffolded runtime with .s2i/bin/assemble does not warn", - runtime: "node", - createFile: true, - wantWarning: false, - }, - { - name: "scaffolded runtime without legacy assemble does not warn", - runtime: "go", - createFile: false, - wantWarning: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - root, done := Mktemp(t) - defer done() - - // Initialize the function so middleware version detection succeeds - // (required for scaffolded runtimes like go/python). - f, err := fn.New().Init(fn.Function{ - Name: "test", - Root: root, - Runtime: tt.runtime, - Registry: "example.com/alice", - }) - if err != nil { - t.Fatal(err) - } - - if tt.createFile { - binDir := filepath.Join(root, ".s2i", "bin") - if err := os.MkdirAll(binDir, 0755); err != nil { - t.Fatal(err) - } - if err := os.WriteFile(filepath.Join(binDir, "assemble"), []byte("#!/bin/bash"), 0700); err != nil { - t.Fatal(err) - } - } - - oldStderr := os.Stderr - r, w, err := os.Pipe() - if err != nil { - t.Fatal(err) - } - os.Stderr = w - - i := &mockImpl{BuildFn: func(cfg *api.Config) (*api.Result, error) { return nil, nil }} - b := s2i.NewBuilder(s2i.WithImpl(i), s2i.WithDockerClient(mockDocker{})) - buildErr := b.Build(t.Context(), f, nil) - - w.Close() - os.Stderr = oldStderr - - var buf bytes.Buffer - if _, err := io.Copy(&buf, r); err != nil { - t.Fatal(err) - } - - if buildErr != nil { - t.Fatal(buildErr) - } - warned := strings.Contains(buf.String(), ".s2i/bin/assemble") - if tt.wantWarning && !warned { - t.Fatalf("expected a warning about .s2i/bin/assemble, got: %q", buf.String()) - } - if !tt.wantWarning && warned { - t.Fatalf("unexpected warning about .s2i/bin/assemble: %q", buf.String()) - } - }) - } -} - // mockImpl is a mock implementation of an S2I builder. type mockImpl struct { BuildFn func(*api.Config) (*api.Result, error) diff --git a/pkg/s2i/scaffolder.go b/pkg/s2i/scaffolder.go index 23563b9025..e1a313c58e 100644 --- a/pkg/s2i/scaffolder.go +++ b/pkg/s2i/scaffolder.go @@ -3,7 +3,6 @@ package s2i import ( "context" "fmt" - "io" "os" "path/filepath" @@ -64,26 +63,3 @@ func (s Scaffolder) Scaffold(ctx context.Context, f fn.Function, path string) er } return nil } - -// WarnIfLegacyS2IScaffolding writes a warning to w when the func-generated -// legacy .s2i/bin/assemble file is present at the function root. -// Prior to https://github.com/knative/func/pull/3436, func wrote S2I -// scaffolding for go/python to /.s2i/bin/assemble; it now lives -// at .func/build/bin/assemble. The stale file can interfere with other -// builders (e.g. pack erroneously detects the assemble script). -// The check is intentionally scoped to scaffolded runtimes (go/python) and -// to the exact file func generated, so user-managed .s2i directories are not -// flagged. -func WarnIfLegacyS2IScaffolding(f fn.Function, w io.Writer) { - if !f.HasScaffolding() { - return - } - legacyAssemble := filepath.Join(f.Root, ".s2i", "bin", "assemble") - if _, err := os.Stat(legacyAssemble); err == nil { - fmt.Fprintf(w, "Warning: a '.s2i/bin/assemble' file was detected at the function root. "+ - "This appears to be leftover func-generated scaffolding from before it was moved to '%s/%s/bin/assemble'. "+ - "It may interfere with other builders. "+ - "If it was generated by func, consider removing '.s2i/' after verifying it is no longer needed.\n", - fn.RunDataDir, fn.BuildDir) - } -}