diff --git a/cmd/build.go b/cmd/build.go index f7f4e25822..7a958845b9 100644 --- a/cmd/build.go +++ b/cmd/build.go @@ -153,13 +153,17 @@ func runBuild(cmd *cobra.Command, _ []string, newClient ClientFactory) (err erro cfg buildConfig f fn.Function ) - if cfg, err = newBuildConfig().Prompt(); err != nil { + cfg = newBuildConfig() + if f, err = fn.NewFunction(cfg.Path); err != nil { // Read in the Function + return + } + if cfg, err = cfg.Prompt(f); err != nil { return wrapPromptError(err, "build") } if err = cfg.Validate(cmd); err != nil { // Perform any pre-validation return wrapValidateError(err, "build") } - if f, err = fn.NewFunction(cfg.Path); err != nil { // Read in the Function + if f, err = fn.NewFunction(cfg.Path); err != nil { // The prompt may change the path return } if !f.Initialized() { @@ -300,8 +304,9 @@ func (c buildConfig) Configure(f fn.Function) fn.Function { // Prompt the user with value of config members, allowing for interactive changes. // Skipped if not in an interactive terminal (non-TTY), or if --confirm false (agree to -// all prompts) was set (default). -func (c buildConfig) Prompt() (buildConfig, error) { +// all prompts) was set (default). f is the function being configured, which +// need not be on the local filesystem. +func (c buildConfig) Prompt(f fn.Function) (buildConfig, error) { // If there is no registry nor explicit image name defined, the // Registry prompt is shown whether or not we are in confirm mode. // Otherwise, it is only shown if in confirm mode @@ -309,10 +314,6 @@ func (c buildConfig) Prompt() (buildConfig, error) { // value and will always use the value from the config (flag or env variable). // This is not strictly correct and will be fixed when Global Config: Function // Context is available (PR#1416) - f, err := fn.NewFunction(c.Path) - if err != nil { - return c, err - } // Check if function exists first if !f.Initialized() { @@ -330,7 +331,7 @@ func (c buildConfig) Prompt() (buildConfig, error) { err := survey.AskOne( &survey.Input{Message: "Registry for function images:", Default: c.Registry}, &c.Registry, - survey.WithValidator(NewRegistryValidator(c.Path))) + survey.WithValidator(NewRegistryValidator(f))) if err != nil { return c, fn.ErrRegistryRequired } @@ -377,7 +378,7 @@ func (c buildConfig) Prompt() (buildConfig, error) { }, } - err = survey.Ask(qs, &c) + err := survey.Ask(qs, &c) if err != nil { return c, err } diff --git a/cmd/config_git_set.go b/cmd/config_git_set.go index 9b5998e595..6523d286b5 100644 --- a/cmd/config_git_set.go +++ b/cmd/config_git_set.go @@ -148,7 +148,7 @@ func newConfigGitSetConfig(_ *cobra.Command) (c configGitSetConfig) { func (c configGitSetConfig) Prompt(f fn.Function) (configGitSetConfig, error) { var err error - if c.buildConfig, err = c.buildConfig.Prompt(); err != nil { + if c.buildConfig, err = c.buildConfig.Prompt(f); err != nil { return c, err } diff --git a/cmd/deploy.go b/cmd/deploy.go index b8b8273efd..2b0d5144c4 100644 --- a/cmd/deploy.go +++ b/cmd/deploy.go @@ -1,6 +1,7 @@ package cmd import ( + "context" "errors" "fmt" "io" @@ -13,7 +14,6 @@ import ( "github.com/spf13/cobra" "k8s.io/apimachinery/pkg/api/resource" "knative.dev/client/pkg/util" - "knative.dev/func/cmd/common" "knative.dev/func/pkg/builders" "knative.dev/func/pkg/config" "knative.dev/func/pkg/deployers" @@ -81,6 +81,9 @@ DESCRIPTION eliminating the need for a local container engine. To trigger deployment of a git repository instead of local source, combine with '--git-url': '{{rootCmdUse}} deploy --remote --git-url=git.example.com/alice/f.git' + The function is then read from the repository, so no local copy is + needed. Choose the revision with '--git-branch' and the directory within + the repository with '--git-dir'. Domain When deploying, a function's route is automatically generated using the @@ -261,34 +264,25 @@ EXAMPLES func runDeploy(cmd *cobra.Command, newClient ClientFactory) (err error) { var ( - cfg deployConfig - f fn.Function + cfg deployConfig + f fn.Function // the function to deploy + local fn.Function // the function at cfg.Path, if any ) // Initialize config first cfg = newDeployConfig(cmd) - // Create function object to check if initialized - if f, err = fn.NewFunction(cfg.Path); err != nil { + // Load the function at path. It is the function to deploy unless the + // source is a git repository, in which case it only records the outcome. + if local, err = fn.NewFunction(cfg.Path); err != nil { return } - - // Check if function exists BEFORE prompting for config - if !f.Initialized() { - if !cfg.Remote || f.Build.Git.URL == "" { - // Only error if this is not a fully remote build - return NewErrNotInitializedFromPath(f.Root, "deploy") - } else { - // TODO: this case is not supported because the pipeline - // implementation requires the function's name, which is in the - // remote repository. We should inspect the remote repository. - // For now, give a more helpful error. - return errors.New("please ensure the function's source is also available locally") - } + if f, err = cfg.function(cmd.Context(), local); err != nil { + return } // Now that we know function exists, proceed with prompting - if cfg, err = cfg.Prompt(); err != nil { + if cfg, err = cfg.Prompt(f); err != nil { if errors.Is(err, fn.ErrRegistryRequired) { return NewErrRegistryRequired(err, "deploy") } @@ -297,6 +291,12 @@ func runDeploy(cmd *cobra.Command, newClient ClientFactory) (err error) { if err = cfg.Validate(cmd); err != nil { return wrapValidateError(err, "deploy") } + // The prompt may have made the source a git repository + if cfg.Remote && cfg.GitURL != "" && f.Root != "" { + if f, err = cfg.function(cmd.Context(), local); err != nil { + return + } + } // Warn if registry changed but registryInsecure is still true warnRegistryInsecureChange(cmd.OutOrStderr(), cfg.Registry, f) @@ -439,6 +439,24 @@ func runDeploy(cmd *cobra.Command, newClient ClientFactory) (err error) { } // Write + // A function deployed from a git repository has no working tree of its own. + // A local function at path, if there is one, records the request and the + // outcome so that later commands (describe, delete, another deploy) find + // them; its own metadata is left alone. + if f.Root == "" { + if !local.Initialized() { + return nil + } + if local, err = cfg.Configure(local); err != nil { + return + } + local.Registry = f.Registry + local.Deploy.Image = f.Deploy.Image + local.Deploy.Namespace = f.Deploy.Namespace + local.Deploy.Deployer = f.Deploy.Deployer + local.Deploy.Expose = f.Deploy.Expose + f = local + } if err = f.Write(); err != nil { return } @@ -450,6 +468,40 @@ func runDeploy(cmd *cobra.Command, newClient ClientFactory) (err error) { return f.Stamp() } +// newFunctionFromGit loads a function from a git repository. A variable so +// tests can stand in for the network. +var newFunctionFromGit = fn.NewFunctionFromGit + +// function returns the function to deploy. When a git repository is the +// source of a remote deployment it is the function committed there: the +// pipeline must describe what the cluster builds, and a local checkout may +// be absent, on another branch or in another directory. Otherwise it is the +// given local function, which must be initialized. +func (c deployConfig) function(ctx context.Context, local fn.Function) (fn.Function, error) { + if c.Remote && c.GitURL != "" { + return newFunctionFromGit(ctx, c.gitSource()) + } + if !local.Initialized() { + return local, NewErrNotInitializedFromPath(local.Root, "deploy") + } + return local, nil +} + +// gitSource is the git repository to build from, as configured. The URL may +// carry the revision as a fragment (#), which takes precedence +// over --git-branch. +// +// TODO: the system should support specifying revision (refSpec) as a URL +// fragment throughout, which, when implemented, removes the need for the +// separate members. +func (c deployConfig) gitSource() fn.Git { + g := fn.Git{URL: c.GitURL, Revision: c.GitBranch, ContextDir: c.GitDir} + if parts := strings.SplitN(c.GitURL, "#", 2); len(parts) == 2 { + g.URL, g.Revision = parts[0], parts[1] + } + return g +} + // build determines if the function should be built based on given flag func build(cmd *cobra.Command, flag string, f fn.Function) (bool, error) { if flag == "auto" { @@ -469,7 +521,7 @@ func build(cmd *cobra.Command, flag string, f fn.Function) (bool, error) { return false, nil } -func NewRegistryValidator(path string) survey.Validator { +func NewRegistryValidator(f fn.Function) survey.Validator { return func(val interface{}) error { // if the value passed in is the zero value of the appropriate type @@ -477,15 +529,10 @@ func NewRegistryValidator(path string) survey.Validator { return fn.ErrRegistryRequired } - f, err := fn.NewFunction(path) - if err != nil { - return err - } - // Set the function's registry to that provided f.Registry = val.(string) - _, err = f.ImageName() //image can be derived without any error + _, err := f.ImageName() //image can be derived without any error if err != nil { return fmt.Errorf("invalid registry [%q]: %w", val.(string), err) } @@ -664,9 +711,7 @@ func (c deployConfig) Configure(f fn.Function) (fn.Function, error) { // Configure basic members f.Domain = c.Domain f.Namespace = c.Namespace - f.Build.Git.URL = c.GitURL - f.Build.Git.ContextDir = c.GitDir - f.Build.Git.Revision = c.GitBranch // TODO: should match; perhaps "refSpec" + f.Build.Git = c.gitSource() f.Build.RemoteStorageClass = c.RemoteStorageClass f.Deploy.ServiceAccountName = c.ServiceAccountName f.Deploy.ImagePullSecret = c.ImagePullSecret @@ -691,15 +736,6 @@ func (c deployConfig) Configure(f fn.Function) (fn.Function, error) { if err != nil { return f, err } - - // .Revision - // TODO: the system should support specifying revision (refSpec) as a URL - // fragment ([#]) throughout, which, when implemented, removes - // the need for the below split into separate members: - if parts := strings.SplitN(c.GitURL, "#", 2); len(parts) == 2 { - f.Build.Git.URL = parts[0] - f.Build.Git.Revision = parts[1] - } return f, nil } @@ -720,9 +756,9 @@ func applyEnvs(current []fn.Env, args []string) (final []fn.Env, err error) { // Prompt the user with value of config members, allowing for interactive changes. // Skipped if not in an interactive terminal (non-TTY), or if --yes (agree to // all prompts) was explicitly set. -func (c deployConfig) Prompt() (deployConfig, error) { +func (c deployConfig) Prompt(f fn.Function) (deployConfig, error) { var err error - if c.buildConfig, err = c.buildConfig.Prompt(); err != nil { + if c.buildConfig, err = c.buildConfig.Prompt(f); err != nil { return c, err } @@ -932,21 +968,6 @@ func printDeployMessages(out io.Writer, f fn.Function) { if !f.Local.Remote && (f.Build.Git.URL != "" || f.Build.Git.Revision != "" || f.Build.Git.ContextDir != "") { fmt.Fprintf(out, "Warning: git settings are only applicable when running with --remote. Local source code will be used.") } - - // Git Branch Mismatch - // ------------------- - // When doing a remote build with --git-branch, warn if the local branch - // doesn't match, as this can lead to confusion about which func.yaml is used. - if f.Local.Remote && f.Build.Git.URL != "" && f.Build.Git.Revision != "" { - // Doing a remote build, specified a git repository to pull from, and - // specified a reference within that remote. - currentBranch, err := common.DefaultCurrentBranch(f.Root) - if err != nil { - fmt.Fprintf(out, "Warning: unable to verify local and remote references match. %v\n", err) - } else if currentBranch != f.Build.Git.Revision { - fmt.Fprintf(out, "Warning: Local git branch '%s' does not match --git-branch '%s'. The local func.yaml will be used for function metadata (name, runtime, etc). Ensure your local branch matches the remote branch to avoid deployment issues.\n", currentBranch, f.Build.Git.Revision) - } - } } // isDigested checks that the given image reference has a digest. Invalid diff --git a/cmd/deploy_test.go b/cmd/deploy_test.go index 906993f54e..9b7f0391f7 100644 --- a/cmd/deploy_test.go +++ b/cmd/deploy_test.go @@ -535,6 +535,156 @@ func testFunctionContext(cmdFn commandConstructor, t *testing.T) { // TestDeploy_GitArgsPersist ensures that the git flags, if provided, are // persisted to the Function for subsequent deployments. +// gitFunction is a function as committed in a git repository: initialized, +// with the given name, and with no Root. +func gitFunction(name string) fn.Function { + return fn.NewFunctionWith(fn.Function{Name: name, Runtime: "go", Created: time.Now()}) +} + +// fromGit stands in for the git loader for the duration of the test, +// returning f for any source. The source requested is recorded in the +// returned value. +func fromGit(t *testing.T, f fn.Function) *fn.Git { + t.Helper() + old := newFunctionFromGit + t.Cleanup(func() { newFunctionFromGit = old }) + requested := &fn.Git{} + newFunctionFromGit = func(_ context.Context, g fn.Git) (fn.Function, error) { + *requested = g + return f, nil + } + return requested +} + +// TestDeploy_RemoteGitNoLocalFunction ensures a remote deployment of a git +// repository needs no local copy of the function: the function is read from +// the repository, and nothing is written to the (empty) current directory. +// +// func deploy --remote --git-url={url} +// +// https://github.com/knative/func/issues/3203 +func TestDeploy_RemoteGitNoLocalFunction(t *testing.T) { + root := FromTempDirectory(t) + requested := fromGit(t, gitFunction("remote-fn")) + + pipeliner := mock.NewPipelinesProvider() + var deployed fn.Function + base := pipeliner.RunFn + pipeliner.RunFn = func(f fn.Function) (string, fn.Function, error) { + deployed = f + return base(f) + } + + cmd := NewDeployCmd(NewTestClient( + fn.WithPipelinesProvider(pipeliner), + fn.WithRegistry(TestRegistry), + )) + cmd.SetArgs([]string{"--remote", + "--git-url=https://example.com/alice/remote-fn.git#feature", + "--git-dir=functions/remote-fn", + "--namespace=fnns"}) + if err := cmd.Execute(); err != nil { + t.Fatal(err) + } + + // The requested source is exactly what the flags described + want := fn.Git{URL: "https://example.com/alice/remote-fn.git", Revision: "feature", ContextDir: "functions/remote-fn"} + if *requested != want { + t.Errorf("expected git source %+v, got %+v", want, *requested) + } + // The pipeline received the function from the repository, configured + // by the flags + if !pipeliner.RunInvoked { + t.Fatal("pipeline was not invoked") + } + if deployed.Name != "remote-fn" { + t.Errorf("expected the repository's function to be deployed, got %q", deployed.Name) + } + if deployed.Root != "" { + t.Errorf("expected no root, got %q", deployed.Root) + } + if deployed.Namespace != "fnns" || deployed.Build.Git != want { + t.Errorf("expected flags to configure the deployed function, got %+v", deployed) + } + // Nothing was written locally + if _, err := os.Stat(filepath.Join(root, fn.FunctionFile)); !os.IsNotExist(err) { + t.Errorf("expected no %s to be written, got err %v", fn.FunctionFile, err) + } +} + +// TestDeploy_RemoteGitUsesRepositoryFunction ensures that, when a local +// function exists alongside a remote deployment of a git repository, the +// pipeline is created for the function as committed in the repository, +// while the local function records the request and the outcome. +func TestDeploy_RemoteGitUsesRepositoryFunction(t *testing.T) { + root := FromTempDirectory(t) + fromGit(t, gitFunction("remote-fn")) + + if _, err := fn.New().Init(fn.Function{Name: "local-fn", Runtime: "node", Root: root}); err != nil { + t.Fatal(err) + } + + pipeliner := mock.NewPipelinesProvider() + var deployed fn.Function // as returned by the pipeline: the outcome + base := pipeliner.RunFn + pipeliner.RunFn = func(f fn.Function) (string, fn.Function, error) { + url, f, err := base(f) + deployed = f + return url, f, err + } + cmd := NewDeployCmd(NewTestClient( + fn.WithPipelinesProvider(pipeliner), + fn.WithRegistry(TestRegistry), + )) + cmd.SetArgs([]string{"--remote", "--git-url=https://example.com/alice/remote-fn.git", "--namespace=fnns"}) + if err := cmd.Execute(); err != nil { + t.Fatal(err) + } + + if deployed.Name != "remote-fn" || deployed.Runtime != "go" { + t.Errorf("expected the repository's function to be deployed, got %q (%v)", deployed.Name, deployed.Runtime) + } + + local, err := fn.NewFunction(root) + if err != nil { + t.Fatal(err) + } + if local.Name != "local-fn" || local.Runtime != "node" { + t.Errorf("expected the local function's metadata to be untouched, got %q (%v)", local.Name, local.Runtime) + } + if local.Build.Git.URL != "https://example.com/alice/remote-fn.git" { + t.Errorf("expected the git source to be recorded locally, got %q", local.Build.Git.URL) + } + if local.Deploy.Namespace != "fnns" { + t.Errorf("expected the deployed namespace to be recorded locally, got %q", local.Deploy.Namespace) + } + if local.Deploy.Image != deployed.Deploy.Image || local.Deploy.Image == "" { + t.Errorf("expected the deployed image %q to be recorded locally, got %q", deployed.Deploy.Image, local.Deploy.Image) + } +} + +// TestDeploy_RemoteGitLoadError ensures a repository the function cannot be +// read from fails the deployment before any pipeline is run. +func TestDeploy_RemoteGitLoadError(t *testing.T) { + _ = FromTempDirectory(t) + old := newFunctionFromGit + t.Cleanup(func() { newFunctionFromGit = old }) + newFunctionFromGit = func(context.Context, fn.Git) (fn.Function, error) { + return fn.Function{}, errors.New("revision \"nope\" not found") + } + + pipeliner := mock.NewPipelinesProvider() + cmd := NewDeployCmd(NewTestClient(fn.WithPipelinesProvider(pipeliner), fn.WithRegistry(TestRegistry))) + cmd.SetArgs([]string{"--remote", "--git-url=https://example.com/alice/remote-fn.git", "--git-branch=nope"}) + err := cmd.Execute() + if err == nil || !strings.Contains(err.Error(), "not found") { + t.Fatalf("expected the loader's error, got %v", err) + } + if pipeliner.RunInvoked { + t.Error("pipeline should not run when the function cannot be read") + } +} + func TestDeploy_GitArgsPersist(t *testing.T) { root := FromTempDirectory(t) @@ -549,6 +699,7 @@ func TestDeploy_GitArgsPersist(t *testing.T) { if err != nil { t.Fatal(err) } + fromGit(t, gitFunction("repo-fn")) // Deploy the Function specifying all of the git-related flags cmd := NewDeployCmd(NewTestClient( @@ -577,7 +728,8 @@ func TestDeploy_GitArgsPersist(t *testing.T) { } // TestDeploy_GitArgsUsed ensures that any git values provided as flags are used -// when invoking a remote deployment. +// when invoking a remote deployment: to read the function from the repository +// and as the pipeline's source. func TestDeploy_GitArgsUsed(t *testing.T) { root := FromTempDirectory(t) @@ -591,6 +743,7 @@ func TestDeploy_GitArgsUsed(t *testing.T) { if err != nil { t.Fatal(err) } + requested := fromGit(t, gitFunction("repo-fn")) // A Pipelines Provider which will validate the expected values were received pipeliner := mock.NewPipelinesProvider() @@ -618,6 +771,9 @@ func TestDeploy_GitArgsUsed(t *testing.T) { if err := cmd.Execute(); err != nil { t.Fatal(err) } + if want := (fn.Git{URL: url, Revision: branch, ContextDir: dir}); *requested != want { + t.Errorf("expected the function to be read from %+v, got %+v", want, *requested) + } } // TestDeploy_GitURLBranch ensures that a --git-url which specifies the branch @@ -635,6 +791,7 @@ func TestDeploy_GitURLBranch(t *testing.T) { expectedUrl = "https://example.com/user/repo" expectedBranch = "branch" ) + requested := fromGit(t, gitFunction("repo-fn")) cmd := NewDeployCmd(NewTestClient( fn.WithDeployer(mock.NewDeployer()), fn.WithBuilder(mock.NewBuilder()), @@ -646,6 +803,9 @@ func TestDeploy_GitURLBranch(t *testing.T) { if err := cmd.Execute(); err != nil { t.Fatal(err) } + if requested.URL != expectedUrl || requested.Revision != expectedBranch { + t.Errorf("expected the function to be read from %q at %q, got %+v", expectedUrl, expectedBranch, *requested) + } f, err = fn.NewFunction(root) if err != nil { @@ -1619,6 +1779,7 @@ func TestDeploy_RemoteBuildURLPermutations(t *testing.T) { newTestFn := func(remote, build, url string) func(t *testing.T) { return func(t *testing.T) { root := FromTempDirectory(t) + fromGit(t, gitFunction("repo-fn")) // Create a new Function in the temp directory if _, err := fn.New().Init(fn.Function{Runtime: "go", Root: root}); err != nil { @@ -1811,6 +1972,7 @@ func TestDeploy_UnsetFlag(t *testing.T) { } // Deploy it, specifying a Git URL + fromGit(t, gitFunction("f")) cmd := NewDeployCmd(NewTestClient()) cmd.SetArgs([]string{"--remote", "--git-url=https://git.example.com/alice/f"}) if err := cmd.Execute(); err != nil { @@ -3002,6 +3164,7 @@ func TestDeploy_RemoteExposeRecordsObservation(t *testing.T) { root := FromTempDirectory(t) cleanup := k8s.SetOpenShiftForTest(true, nil) defer cleanup() + fromGit(t, gitFunction("repo-fn")) if _, err := fn.New().Init(fn.Function{Runtime: "go", Root: root}); err != nil { t.Fatal(err) diff --git a/cmd/run.go b/cmd/run.go index f60ef3e9aa..5059d46795 100644 --- a/cmd/run.go +++ b/cmd/run.go @@ -154,11 +154,14 @@ func runRun(cmd *cobra.Command, newClient ClientFactory) (err error) { cfg runConfig f fn.Function ) - if cfg, err = newRunConfig(cmd).Prompt(); err != nil { + cfg = newRunConfig(cmd) + if f, err = fn.NewFunction(cfg.Path); err != nil { + return + } + if cfg, err = cfg.Prompt(f); err != nil { return wrapPromptError(err, "run") } - - if f, err = fn.NewFunction(cfg.Path); err != nil { + if f, err = fn.NewFunction(cfg.Path); err != nil { // The prompt may change the path return } if !f.Initialized() { @@ -371,10 +374,10 @@ func (c runConfig) Configure(f fn.Function) (fn.Function, error) { return f, err } -func (c runConfig) Prompt() (runConfig, error) { +func (c runConfig) Prompt(f fn.Function) (runConfig, error) { var err error - if c.buildConfig, err = c.buildConfig.Prompt(); err != nil { + if c.buildConfig, err = c.buildConfig.Prompt(f); err != nil { return c, err } diff --git a/docs/reference/func_deploy.md b/docs/reference/func_deploy.md index 6fdb998af7..e4b9b44297 100644 --- a/docs/reference/func_deploy.md +++ b/docs/reference/func_deploy.md @@ -57,6 +57,9 @@ DESCRIPTION eliminating the need for a local container engine. To trigger deployment of a git repository instead of local source, combine with '--git-url': 'func deploy --remote --git-url=git.example.com/alice/f.git' + The function is then read from the repository, so no local copy is + needed. Choose the revision with '--git-branch' and the directory within + the repository with '--git-dir'. Domain When deploying, a function's route is automatically generated using the diff --git a/e2e/e2e_remote_test.go b/e2e/e2e_remote_test.go index 653a32a765..600a110ade 100644 --- a/e2e/e2e_remote_test.go +++ b/e2e/e2e_remote_test.go @@ -5,7 +5,6 @@ package e2e import ( "fmt" "os" - "os/exec" "path/filepath" "testing" "time" @@ -42,21 +41,15 @@ func TestRemote_Deploy(t *testing.T) { } // TestRemote_Source ensures a remote build can be triggered which pulls -// source from a remote repository. +// source from a remote repository, with no local copy of the function. // // func deploy --remote --git-url={url} --registry={} --builder=pack func TestRemote_Source(t *testing.T) { name := "func-e2e-test-remote-source" _ = fromCleanEnv(t, name) - // This command currently requires the function source also be available - // locally in order to use its name. - cmd := exec.Command("git", "clone", "https://github.com/functions-dev/func-e2e-tests", ".") - if err := cmd.Run(); err != nil { - t.Fatal(err) - } - - // Trigger the deploy + // Trigger the deploy from an empty directory: the function is read from + // the repository. if err := newCmd(t, "deploy", "--remote", "--git-url", "https://github.com/functions-dev/func-e2e-tests", "--registry", Registry, @@ -77,28 +70,12 @@ func TestRemote_Source(t *testing.T) { // TestRemote_Ref ensures a remote build can be triggered which pulls // source from a specific reference (branch/tag) of a remote repository. +// The function's metadata (name, runtime, etc) is read from that reference, +// so no local checkout is involved. func TestRemote_Ref(t *testing.T) { name := "func-e2e-test-remote-ref" _ = fromCleanEnv(t, name) - // This command currently requires the function source also be available - // locally in order to use its name. - cmd := exec.Command("git", "clone", "https://github.com/functions-dev/func-e2e-tests", ".") - if err := cmd.Run(); err != nil { - t.Fatal(err) - } - - // IMPORTANT: The local func.yaml must match the one in the target branch. - // This is a current limitation where remote builds still require local - // source to determine function metadata (name, runtime, etc). - // TODO: Remove this checkout once the implementation supports fetching - // function metadata from the remote repository. - // https://github.com/knative/func/issues/3203 - cmd = exec.Command("git", "checkout", name) - if err := cmd.Run(); err != nil { - t.Fatal(err) - } - // Trigger the deploy if err := newCmd(t, "deploy", "--remote", "--git-url", "https://github.com/functions-dev/func-e2e-tests", @@ -120,32 +97,14 @@ func TestRemote_Ref(t *testing.T) { } // TestRemote_Dir ensures that remote builds can be instructed to build and -// deploy a function located in a subdirectory. +// deploy a function located in a subdirectory of the repository. The +// function's metadata is read from that subdirectory. // -// func deploy --remote --git-dir={subdir} // func deploy --remote --git-dir={subdir} --git-url={url} func TestRemote_Dir(t *testing.T) { name := "func-e2e-test-remote-dir" _ = fromCleanEnv(t, name) - // This command currently requires the function source also be available - // locally in order to use its name. - cmd := exec.Command("git", "clone", "https://github.com/functions-dev/func-e2e-tests", ".") - if err := cmd.Run(); err != nil { - t.Fatal(err) - } - - // IMPORTANT: When using --git-dir, we need to change to that directory locally - // to ensure the local func.yaml matches the one that will be used in the remote build. - // This is a current limitation where remote builds still require local source to - // determine function metadata (name, runtime, etc). - // TODO: Remove this cd once the implementation supports fetching function metadata - // from the remote repository subdirectory. - // https://github.com/knative/func/issues/3203 - if err := os.Chdir(name); err != nil { - t.Fatalf("failed to change to subdirectory %s: %v", name, err) - } - // Trigger the deploy if err := newCmd(t, "deploy", "--remote", "--git-url", "https://github.com/functions-dev/func-e2e-tests", diff --git a/pkg/cluster/dns.go b/pkg/cluster/dns.go index 7a0781df36..25cf1fdaa6 100644 --- a/pkg/cluster/dns.go +++ b/pkg/cluster/dns.go @@ -27,17 +27,14 @@ func configureMagicDNS(ctx context.Context, cfg ClusterConfig, out io.Writer) er return err } - // Deployment patch triggers a rolling restart. Sleep so the new pods - // enter NotReady before we wait for Ready — otherwise the old pods - // still satisfy the condition and we return before the restart lands. - if err := wait(ctx, 5*time.Second); err != nil { - return err - } + // The deployment patch triggers a rolling restart. Wait for the rollout + // itself: the new pods are available and the old ones are gone. Waiting + // for every kube-system pod to be Ready instead raced with the old + // coredns pods, which are terminating and never become Ready again. if err := run(ctx, out, "", - cfg.kubectl(), "wait", "pod", - "--for=condition=Ready", "-l", "!job-name", - "-n", "kube-system", "--timeout=60s"); err != nil { - return fmt.Errorf("waiting for coredns: %w", err) + cfg.kubectl(), "rollout", "status", "deployment/coredns", + "-n", "kube-system", "--timeout=120s"); err != nil { + return fmt.Errorf("waiting for coredns rollout: %w", err) } success(out, "Magic DNS", time.Since(start)) diff --git a/pkg/functions/client.go b/pkg/functions/client.go index c926956fae..18fef0f40b 100644 --- a/pkg/functions/client.go +++ b/pkg/functions/client.go @@ -18,7 +18,6 @@ import ( "time" "golang.org/x/sync/errgroup" - "gopkg.in/yaml.v2" "knative.dev/func/pkg/deployers" "knative.dev/func/pkg/utils" ) @@ -1609,11 +1608,8 @@ func hasInitializedFunction(path string) (bool, error) { if err != nil { return false, err } - f := Function{} - if err = yaml.Unmarshal(bb, &f); err != nil { - return false, err - } - if f, err = f.Migrate(); err != nil { + f, err := parseFunction(bb) + if err != nil { return false, err } return f.Initialized(), nil diff --git a/pkg/functions/function.go b/pkg/functions/function.go index df1762350d..9f6192046b 100644 --- a/pkg/functions/function.go +++ b/pkg/functions/function.go @@ -374,24 +374,10 @@ func NewFunction(root string) (f Function, err error) { if err != nil { return } - var functionMarshallingError error - var functionMigrationError error - if marshallingErr := yaml.Unmarshal(bb, &f); marshallingErr != nil { - functionMarshallingError = formatUnmarshalError(marshallingErr) // human-friendly unmarshalling errors - } - if f, err = f.Migrate(); err != nil { - functionMigrationError = err - } - // Only if migration fail return errors to the user. include marshalling error if present - if functionMigrationError != nil { - //returning both migrations and marshalling errors to the user - errorText := "Error: \n" - if functionMarshallingError != nil { - errorText += "Marshalling: " + functionMarshallingError.Error() - } - errorText += "\n" + "Migration: " + functionMigrationError.Error() - return Function{}, errors.New(errorText) + if f, err = parseFunction(bb); err != nil { + return } + f.Root = root f.Local, err = f.newLocal() if err != nil { @@ -404,13 +390,38 @@ func NewFunction(root string) (f Function, err error) { return } -// Validate function is logically correct, returning a bundled, and quite -// verbose, formatted error detailing any issues. -func (f Function) Validate() error { - if f.Root == "" { - return errors.New("function root path is required") +// parseFunction unmarshals a serialized function (the content of a func.yaml) +// and migrates it to the current spec version. The result has no Root: where +// the bytes came from is the caller's concern. +// +// Unmarshalling errors are reported only when the migration also fails. A +// function whose migration succeeds is accepted even if some of its fields +// did not unmarshal cleanly. +func parseFunction(bb []byte) (f Function, err error) { + f.Build.BuilderImages = make(map[string]string) + f.Deploy.Annotations = make(map[string]string) + + var marshallingErr error + if err = yaml.Unmarshal(bb, &f); err != nil { + marshallingErr = formatUnmarshalError(err) // human-friendly unmarshalling errors + } + if f, err = f.migrate(bb); err != nil { + // Return both the migration and any marshalling error to the user + errorText := "Error: \n" + if marshallingErr != nil { + errorText += "Marshalling: " + marshallingErr.Error() + } + errorText += "\n" + "Migration: " + err.Error() + return Function{}, errors.New(errorText) } + return f, nil +} +// Validate function is logically correct, returning a bundled, and quite +// verbose, formatted error detailing any issues. Where the function lives is +// not part of its correctness: a function read from a git repository has no +// Root, which Write requires. +func (f Function) Validate() error { var ctr int errs := [][]string{ validateVolumes(f.Run.Volumes), @@ -533,6 +544,9 @@ func (f Function) MarshalFuncYaml() ([]byte, error) { // Write Function struct (metadata) to Disk at f.Root func (f Function) Write() (err error) { + if f.Root == "" { + return ErrRootRequired + } // Skip writing (and dirtying the work tree) if there were no modifications. f1, _ := NewFunction(f.Root) if reflect.DeepEqual(f, f1) { @@ -680,7 +694,8 @@ func (f Function) HasScaffolding() bool { // https://github.com/knative/func/pull/3436) and can interfere with other // builders (mostly just pack). func WarnIfLegacyS2IScaffolding(f Function, w io.Writer) { - if !f.HasScaffolding() { + // A function without a Root has no working tree to inspect. + if !f.HasScaffolding() || f.Root == "" { return } legacyAssemble := filepath.Join(f.Root, ".s2i", "bin", "assemble") diff --git a/pkg/functions/function_git.go b/pkg/functions/function_git.go index b3122ac791..47f02b80d1 100644 --- a/pkg/functions/function_git.go +++ b/pkg/functions/function_git.go @@ -1,10 +1,22 @@ package functions import ( + "context" + "errors" "fmt" + "os" + "path" "strings" giturls "github.com/chainguard-dev/git-urls" + "github.com/go-git/go-billy/v5" + "github.com/go-git/go-billy/v5/memfs" + "github.com/go-git/go-billy/v5/util" + "github.com/go-git/go-git/v5" + "github.com/go-git/go-git/v5/config" + "github.com/go-git/go-git/v5/plumbing" + "github.com/go-git/go-git/v5/plumbing/transport" + "github.com/go-git/go-git/v5/storage/memory" ) type Git struct { @@ -32,3 +44,191 @@ func validateGit(git Git) (errors []string) { } return } + +// NewFunctionFromGit loads the function committed in the repository g +// describes: the func.yaml in g.ContextDir at g.Revision, which is the +// remote's default branch when empty. A revision may be a branch, a tag or a +// full commit hash. +// +// No working copy is involved. The returned function therefore has no Root +// and none of the state NewFunction reads from one (local settings, last +// built image). +func NewFunctionFromGit(ctx context.Context, g Git) (Function, error) { + src, err := resolveGitSource(ctx, g) + if err != nil { + return Function{}, err + } + tree, err := src.checkout(ctx) + if err != nil { + return Function{}, err + } + bb, err := util.ReadFile(tree, path.Join(g.ContextDir, FunctionFile)) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + return Function{}, fmt.Errorf("no %s in %q of %s at %s", FunctionFile, g.ContextDir, g.URL, src.describe()) + } + return Function{}, fmt.Errorf("cannot read %s from %s: %w", FunctionFile, g.URL, err) + } + return parseFunction(bb) +} + +// gitSource is a revision of a remote repository, resolved against the refs +// the remote advertises. +type gitSource struct { + url string + auth transport.AuthMethod + // ref is the branch or tag to fetch. It is empty when the revision was + // given as a bare commit hash. + ref plumbing.ReferenceName + // hash is the commit the revision resolves to. + hash plumbing.Hash +} + +// resolveGitSource lists the refs of g.URL and matches g.Revision against +// them. An empty revision means the remote's default branch. Otherwise a +// branch, a tag, a full ref name and a full commit hash are tried, in that +// order. +func resolveGitSource(ctx context.Context, g Git) (gitSource, error) { + src := gitSource{url: g.URL} + if g.URL == "" { + return src, errors.New("git URL required") + } + + remote := git.NewRemote(memory.NewStorage(), &config.RemoteConfig{ + Name: git.DefaultRemoteName, + URLs: []string{g.URL}, + }) + opts := &git.ListOptions{PeelingOption: git.AppendPeeled} + refs, err := remote.ListContext(ctx, opts) + if isAuthError(err) { + if src.auth = credentialsForURL(g.URL); src.auth != nil { + opts.Auth = src.auth + refs, err = remote.ListContext(ctx, opts) + } + } + if err != nil { + return src, fmt.Errorf("cannot list refs of %s: %w", g.URL, err) + } + + byName := make(map[plumbing.ReferenceName]*plumbing.Reference, len(refs)) + for _, r := range refs { + byName[r.Name()] = r + } + // commitOf returns the commit a ref points to: the peeled object for an + // annotated tag, the ref's own hash otherwise. + commitOf := func(name plumbing.ReferenceName) plumbing.Hash { + if peeled, ok := byName[name+"^{}"]; ok { + return peeled.Hash() + } + return byName[name].Hash() + } + + if g.Revision == "" { + head, ok := byName[plumbing.HEAD] + if !ok { + return src, fmt.Errorf("%s advertises no HEAD; specify a revision", g.URL) + } + if head.Type() != plumbing.SymbolicReference { + src.hash = head.Hash() + return src, nil + } + if _, ok := byName[head.Target()]; !ok { + return src, fmt.Errorf("%s: HEAD points to %s, which the remote does not advertise", g.URL, head.Target()) + } + src.ref = head.Target() + src.hash = commitOf(src.ref) + return src, nil + } + + for _, name := range []plumbing.ReferenceName{ + plumbing.NewBranchReferenceName(g.Revision), + plumbing.NewTagReferenceName(g.Revision), + plumbing.ReferenceName(g.Revision), + } { + if _, ok := byName[name]; ok { + src.ref = name + src.hash = commitOf(name) + return src, nil + } + } + if plumbing.IsHash(g.Revision) { + src.hash = plumbing.NewHash(g.Revision) + return src, nil + } + return src, fmt.Errorf("revision %q not found in %s", g.Revision, g.URL) +} + +// describe returns the revision for messages: the ref's short name when the +// revision named one, the commit hash otherwise. +func (s gitSource) describe() string { + if s.ref != "" { + return s.ref.Short() + } + return s.hash.String() +} + +// checkout fetches the resolved revision, depth one, into memory and returns +// its tree. +func (s gitSource) checkout(ctx context.Context) (billy.Filesystem, error) { + var ( + repo *git.Repository + err error + ) + if s.ref != "" { + repo, err = git.CloneContext(ctx, memory.NewStorage(), memfs.New(), &git.CloneOptions{ + URL: s.url, + Auth: s.auth, + ReferenceName: s.ref, + SingleBranch: true, + Depth: 1, + Tags: git.NoTags, + RecurseSubmodules: git.NoRecurseSubmodules, + }) + } else { + // A bare commit cannot be cloned: fetch it by hash and check it out. + repo, err = fetchGitCommit(ctx, s) + } + if err != nil { + return nil, fmt.Errorf("cannot fetch %s at %s: %w", s.url, s.describe(), err) + } + wt, err := repo.Worktree() + if err != nil { + return nil, err + } + return wt.Filesystem, nil +} + +func fetchGitCommit(ctx context.Context, s gitSource) (*git.Repository, error) { + repo, err := git.Init(memory.NewStorage(), memfs.New()) + if err != nil { + return nil, err + } + remote, err := repo.CreateRemote(&config.RemoteConfig{ + Name: git.DefaultRemoteName, + URLs: []string{s.url}, + }) + if err != nil { + return nil, err + } + // Fetching a hash directly requires the server to allow it + // (uploadpack.allowReachableSHA1InWant), as the common hosts do. + err = remote.FetchContext(ctx, &git.FetchOptions{ + Auth: s.auth, + Depth: 1, + Tags: git.NoTags, + RefSpecs: []config.RefSpec{ + config.RefSpec(s.hash.String() + ":" + plumbing.NewRemoteReferenceName(git.DefaultRemoteName, "source").String()), + }, + }) + if err != nil { + return nil, err + } + wt, err := repo.Worktree() + if err != nil { + return nil, err + } + if err = wt.Checkout(&git.CheckoutOptions{Hash: s.hash}); err != nil { + return nil, err + } + return repo, nil +} diff --git a/pkg/functions/function_git_test.go b/pkg/functions/function_git_test.go new file mode 100644 index 0000000000..5dbe4f99e8 --- /dev/null +++ b/pkg/functions/function_git_test.go @@ -0,0 +1,202 @@ +package functions_test + +import ( + "context" + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" + "testing" + + fn "knative.dev/func/pkg/functions" +) + +// gitFixture is a local repository served over file:// with: +// - main: func.yaml naming "root-fn" and sub/func.yaml naming "sub-fn" +// - tag v1 (annotated) on the first commit of main +// - branch feature: func.yaml naming "feature-fn" +// - branch legacy: a func.yaml from spec version 0.25.0, needing migration +type gitFixture struct { + url string + main string // full hash of main's head + feature string // full hash of feature's head +} + +func newGitFixture(t *testing.T) gitFixture { + t.Helper() + if runtime.GOOS == "windows" { + // The fixture is served over file://, which go-git only supports + // through the git binary and not from a Windows path. + t.Skip("file:// repositories are not supported on Windows") + } + if _, err := exec.LookPath("git"); err != nil { + t.Skip("No 'git' found in path. Skipping test.") + } + dir := t.TempDir() + run := func(args ...string) string { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + cmd.Env = append(os.Environ(), + "GIT_AUTHOR_NAME=test", "GIT_AUTHOR_EMAIL=test@example.com", + "GIT_COMMITTER_NAME=test", "GIT_COMMITTER_EMAIL=test@example.com") + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("git %s: %v\n%s", strings.Join(args, " "), err, out) + } + return strings.TrimSpace(string(out)) + } + write := func(rel, name string) { + t.Helper() + p := filepath.Join(dir, rel) + if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil { + t.Fatal(err) + } + content := "specVersion: " + fn.LastSpecVersion() + "\nname: " + name + "\nruntime: go\ncreated: 2024-01-01T00:00:00Z\n" + if err := os.WriteFile(p, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + } + + run("init", "-q", "-b", "main") + // Let the test fetch a bare commit hash, as the common git hosts allow. + run("config", "uploadpack.allowAnySHA1InWant", "true") + write("func.yaml", "root-fn") + write("sub/func.yaml", "sub-fn") + run("add", ".") + run("commit", "-q", "-m", "initial") + run("tag", "-a", "v1", "-m", "v1") + main := run("rev-parse", "HEAD") + + run("checkout", "-q", "-b", "feature") + write("func.yaml", "feature-fn") + run("commit", "-q", "-am", "feature") + feature := run("rev-parse", "HEAD") + + run("checkout", "-q", "-b", "legacy", "main") + legacy, err := os.ReadFile(filepath.Join("testdata", "migrations", "v0.34.0", "func.yaml")) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "func.yaml"), legacy, 0o644); err != nil { + t.Fatal(err) + } + run("commit", "-q", "-am", "legacy") + run("checkout", "-q", "main") + + return gitFixture{url: "file://" + dir, main: main, feature: feature} +} + +// TestNewFunctionFromGit ensures a function is loaded from the func.yaml of +// the requested revision and context directory without a local checkout. +func TestNewFunctionFromGit(t *testing.T) { + fx := newGitFixture(t) + tests := []struct { + name string + git fn.Git + wantName string + }{ + {"default branch", fn.Git{URL: fx.url}, "root-fn"}, + {"context dir", fn.Git{URL: fx.url, ContextDir: "sub"}, "sub-fn"}, + {"branch", fn.Git{URL: fx.url, Revision: "feature"}, "feature-fn"}, + {"annotated tag", fn.Git{URL: fx.url, Revision: "v1"}, "root-fn"}, + {"full ref", fn.Git{URL: fx.url, Revision: "refs/heads/feature"}, "feature-fn"}, + {"commit hash", fn.Git{URL: fx.url, Revision: fx.feature}, "feature-fn"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f, err := fn.NewFunctionFromGit(context.Background(), tt.git) + if err != nil { + t.Fatal(err) + } + if f.Name != tt.wantName { + t.Errorf("expected name %q, got %q", tt.wantName, f.Name) + } + if f.Runtime != "go" { + t.Errorf("expected runtime go, got %q", f.Runtime) + } + if f.Root != "" { + t.Errorf("expected no root, got %q", f.Root) + } + if !f.Initialized() { + t.Error("expected the function to be initialized") + } + }) + } +} + +// TestNewFunctionFromGit_Migrates ensures a func.yaml of an earlier spec +// version is migrated on load, as NewFunction does for a local checkout: +// migrations read the previous structure from the fetched bytes. +func TestNewFunctionFromGit_Migrates(t *testing.T) { + fx := newGitFixture(t) + f, err := fn.NewFunctionFromGit(context.Background(), fn.Git{URL: fx.url, Revision: "legacy"}) + if err != nil { + t.Fatal(err) + } + if f.SpecVersion != fn.LastSpecVersion() { + t.Errorf("expected spec version %q, got %q", fn.LastSpecVersion(), f.SpecVersion) + } + if f.Name != "testfunc" { + t.Errorf("expected name testfunc, got %q", f.Name) + } + // Moved from the top level into build.git by migrateToSpecsStructure + if f.Build.Git.URL != "http://test-url" { + t.Errorf("expected migrated git url, got %q", f.Build.Git.URL) + } +} + +// TestNewFunctionFromGit_Errors ensures unknown revisions and directories +// without a func.yaml are reported, not silently returned as empty functions. +func TestNewFunctionFromGit_Errors(t *testing.T) { + fx := newGitFixture(t) + tests := []struct { + name string + git fn.Git + wantErr string + }{ + {"unknown revision", fn.Git{URL: fx.url, Revision: "nope"}, "not found"}, + {"missing func.yaml", fn.Git{URL: fx.url, ContextDir: "nope"}, "no func.yaml"}, + {"no url", fn.Git{}, "required"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := fn.NewFunctionFromGit(context.Background(), tt.git) + if err == nil { + t.Fatal("expected an error") + } + if !strings.Contains(err.Error(), tt.wantErr) { + t.Errorf("expected error containing %q, got %q", tt.wantErr, err) + } + }) + } +} + +// TestGitRemoteCommit ensures the commit reported for a remote revision is +// the commit that revision resolves to: an annotated tag yields the tagged +// commit, not the tag object. +func TestGitRemoteCommit(t *testing.T) { + fx := newGitFixture(t) + tests := []struct { + name string + git fn.Git + want string + }{ + {"default branch", fn.Git{URL: fx.url}, fx.main[:7]}, + {"branch", fn.Git{URL: fx.url, Revision: "feature"}, fx.feature[:7]}, + {"annotated tag", fn.Git{URL: fx.url, Revision: "v1"}, fx.main[:7]}, + {"commit hash", fn.Git{URL: fx.url, Revision: fx.feature}, fx.feature[:7]}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := fn.GitRemoteCommit(context.Background(), tt.git) + if err != nil { + t.Fatal(err) + } + if got != tt.want { + t.Errorf("expected commit %q, got %q", tt.want, got) + } + }) + } +} diff --git a/pkg/functions/function_migrations.go b/pkg/functions/function_migrations.go index 42995cc59c..d9758f9d9d 100644 --- a/pkg/functions/function_migrations.go +++ b/pkg/functions/function_migrations.go @@ -1,7 +1,6 @@ package functions import ( - "errors" "fmt" "os" "path/filepath" @@ -18,20 +17,34 @@ var unknownFieldsOnce sync.Once // version of the function. It is the caller's responsibility to // .Write() the function to persist to disk. Additionally it will warn on // up-to-date spec but wrong func.yaml (eg. extraneous fields) +// +// Migrations need the function as it was serialized, which Migrate reads +// from the func.yaml at f.Root. See migrate for functions without a Root. func (f Function) Migrate() (migrated Function, err error) { + var raw []byte + if f.Root != "" { + if raw, err = os.ReadFile(filepath.Join(f.Root, FunctionFile)); err != nil && !os.IsNotExist(err) { + return f, err + } + } + return f.migrate(raw) +} + +// migrate applies any necessary migrations to f, whose serialized form (the +// content of its func.yaml) is raw. Migrations read the previous structure +// from raw, so a function needs no working tree to be migrated. +func (f Function) migrate(raw []byte) (migrated Function, err error) { // Return immediately if the function indicates it has already been // migrated. if f.Migrated() { - // Already at the latest spec — check for unknown fields - if f.Root != "" { - if bb, readErr := os.ReadFile(filepath.Join(f.Root, FunctionFile)); readErr == nil { - unknownFieldsOnce.Do(func() { - var strict Function - if strictErr := yaml.UnmarshalStrict(bb, &strict); strictErr != nil { - fmt.Fprintf(os.Stderr, "Warning (unknown fields will be ignored):\n %v\n.\n", formatUnmarshalError(strictErr)) - } - }) - } + // Already at the latest spec: check for unknown fields + if raw != nil { + unknownFieldsOnce.Do(func() { + var strict Function + if strictErr := yaml.UnmarshalStrict(raw, &strict); strictErr != nil { + fmt.Fprintf(os.Stderr, "Warning (unknown fields will be ignored):\n %v\n.\n", formatUnmarshalError(strictErr)) + } + }) } return f, nil } @@ -45,7 +58,7 @@ func (f Function) Migrate() (migrated Function, err error) { } // Apply this migration when the function's specVersion is less than that which // the migration will impart. - migrated, err = m.migrate(migrated, m) + migrated, err = m.migrate(migrated, raw, m) if err != nil { return // fail fast on any migration errors } @@ -61,7 +74,20 @@ type migration struct { } // migrator is a function which returns a migrated copy of an inbound function. -type migrator func(Function, migration) (Function, error) +// It receives the function's serialized form to read the previous structure. +type migrator func(f Function, raw []byte, m migration) (Function, error) + +// unmarshalPrevious loads the pertinent parts of a previous schema version +// from the serialized function raw, on behalf of the named migration. +func unmarshalPrevious(raw []byte, migration string, previous interface{}) error { + if raw == nil { + return fmt.Errorf("migration '%s' error: the serialized function is required", migration) + } + if err := yaml.Unmarshal(raw, previous); err != nil { + return fmt.Errorf("migration '%s' error: %w", migration, err) + } + return nil +} // Migrated returns whether the function has been migrated to the highest // level the currently executing system is aware of (or beyond). @@ -125,7 +151,7 @@ var migrations = []migration{ // created stamp. Otherwise, this is an in-memory (new) function that is // currently in the process of being created and as such need not be mutated // to consider this migration having been evaluated. -func migrateToCreationStamp(f Function, m migration) (Function, error) { +func migrateToCreationStamp(f Function, _ []byte, m migration) (Function, error) { // For functions with no creation timestamp, but appear to have been pre- // existing, populate their created stamp and version. // Yes, it's a little gnarly, but bootstrapping into the loveliness of a @@ -172,16 +198,11 @@ func migrateToCreationStamp(f Function, m migration) (Function, error) { // a customized builder image, that value is preserved as the builder image // for the 'pack' builder in the new version (s2i did not exist prior). // See associated unit tests. -func migrateToBuilderImages(f1 Function, m migration) (Function, error) { +func migrateToBuilderImages(f1 Function, raw []byte, m migration) (Function, error) { // Load the function using pertinent parts of the previous version's schema: - f0Filename := filepath.Join(f1.Root, FunctionFile) - bb, err := os.ReadFile(f0Filename) - if err != nil { - return f1, errors.New("migration 'migrateToBuilderImages' error: " + err.Error()) - } f0 := migrateToBuilderImages_previousFunction{} - if err = yaml.Unmarshal(bb, &f0); err != nil { - return f1, errors.New("migration 'migrateToBuilderImages' error: " + err.Error()) + if err := unmarshalPrevious(raw, "migrateToBuilderImages", &f0); err != nil { + return f1, err } // At time of this migration, the default pack builder image for all language @@ -205,18 +226,11 @@ func migrateToBuilderImages(f1 Function, m migration) (Function, error) { // migrateToSpecVersion updates a func.yaml file to use SpecVersion // instead of Version to track the migration numbers -func migrateToSpecVersion(f Function, m migration) (Function, error) { +func migrateToSpecVersion(f Function, raw []byte, m migration) (Function, error) { // Load the function func.yaml file - f0Filename := filepath.Join(f.Root, FunctionFile) - bb, err := os.ReadFile(f0Filename) - if err != nil { - return f, errors.New("migration 'migrateToSpecVersion' error: " + err.Error()) - } - - // Only handle the Version field if it exists f0 := migrateToSpecVersion_previousFunction{} - if err = yaml.Unmarshal(bb, &f0); err != nil { - return f, errors.New("migration 'migrateToSpecVersion' error: " + err.Error()) + if err := unmarshalPrevious(raw, "migrateToSpecVersion", &f0); err != nil { + return f, err } f.SpecVersion = m.version @@ -226,16 +240,11 @@ func migrateToSpecVersion(f Function, m migration) (Function, error) { // migrateToSpecsStructure migration makes sure use the sub-specs structs for build, run and deploy phases. // To avoid unmarshalling issues with the old format this migration needs to be executed first. // Further migrations will operate on this new struct with sub-specs -func migrateToSpecsStructure(f1 Function, m migration) (Function, error) { +func migrateToSpecsStructure(f1 Function, raw []byte, m migration) (Function, error) { // Load the Function using pertinent parts of the previous version's schema: - f0Filename := filepath.Join(f1.Root, FunctionFile) - bb, err := os.ReadFile(f0Filename) - if err != nil { - return f1, errors.New("migration 'migrateToSpecsStructure' error: " + err.Error()) - } f0 := migrateToSpecs_previousFunction{} - if err = yaml.Unmarshal(bb, &f0); err != nil { - return f1, errors.New("migration 'migrateToSpecsStructure' error: " + err.Error()) + if err := unmarshalPrevious(raw, "migrateToSpecsStructure", &f0); err != nil { + return f1, err } if f0.Git.URL != "" { @@ -305,16 +314,11 @@ func migrateToSpecsStructure(f1 Function, m migration) (Function, error) { // file. When Invoke now holds default value (http) it will not show up in // func.yaml as the default value is implicitly expected. Otherwise if Invoke // is non-default value, it will be written in func.yaml. -func migrateFromInvokeStructure(f1 Function, m migration) (Function, error) { +func migrateFromInvokeStructure(f1 Function, raw []byte, m migration) (Function, error) { // Load the Function using pertinent parts of the previous version's schema: - f0Filename := filepath.Join(f1.Root, FunctionFile) - bb, err := os.ReadFile(f0Filename) - if err != nil { - return f1, errors.New("migration 'migrateFromInvokeStructure' error: " + err.Error()) - } f0 := migrateFromInvokeStructure_previousFunction{} - if err = yaml.Unmarshal(bb, &f0); err != nil { - return f1, errors.New("migration 'migrateFromInvokeStructure' error: " + err.Error()) + if err := unmarshalPrevious(raw, "migrateFromInvokeStructure", &f0); err != nil { + return f1, err } if f0.Invocation.Format != "" && f0.Invocation.Format != "http" { @@ -326,13 +330,7 @@ func migrateFromInvokeStructure(f1 Function, m migration) (Function, error) { return f1, nil } -func migratePersistentVolumeTypoFixup(fn Function, m migration) (Function, error) { - f, err := os.Open(filepath.Join(fn.Root, FunctionFile)) - if err != nil { - return Function{}, fmt.Errorf("cannot open func.yaml: %w", err) - } - defer f.Close() - +func migratePersistentVolumeTypoFixup(fn Function, raw []byte, m migration) (Function, error) { data := struct { Run struct { Volumes []struct { @@ -340,11 +338,8 @@ func migratePersistentVolumeTypoFixup(fn Function, m migration) (Function, error } `yaml:"volumes,omitempty"` } }{} - - dec := yaml.NewDecoder(f) - err = dec.Decode(&data) - if err != nil { - return Function{}, fmt.Errorf("cannot deserialize old sub-structure: %w", err) + if err := unmarshalPrevious(raw, "migratePersistentVolumeTypoFixup", &data); err != nil { + return fn, err } for idx, volume := range data.Run.Volumes { diff --git a/pkg/functions/git_commit.go b/pkg/functions/git_commit.go index d4ecdb351d..e89cd0f655 100644 --- a/pkg/functions/git_commit.go +++ b/pkg/functions/git_commit.go @@ -1,6 +1,8 @@ package functions import ( + "context" + "github.com/go-git/go-git/v5" ) @@ -39,3 +41,18 @@ func GitCommit(dir string) (string, error) { return sha, nil } + +// GitRemoteCommit returns the short commit SHA that g.Revision of the +// repository at g.URL currently resolves to, without a local checkout. It is +// the remote counterpart of GitCommit, so a build from a git repository can +// be labelled with the commit that is actually built. +func GitRemoteCommit(ctx context.Context, g Git) (string, error) { + src, err := resolveGitSource(ctx, g) + if err != nil { + return "", err + } + if src.hash.IsZero() { + return "", nil + } + return src.hash.String()[:7], nil +} diff --git a/pkg/pipelines/tekton/pipelines_provider.go b/pkg/pipelines/tekton/pipelines_provider.go index bba07e4da2..5abad58a0a 100644 --- a/pkg/pipelines/tekton/pipelines_provider.go +++ b/pkg/pipelines/tekton/pipelines_provider.go @@ -123,6 +123,19 @@ func (pp *PipelinesProvider) Run(ctx context.Context, f fn.Function) (string, fn return "", f, err } + // The source is either a git repository or the local working tree, which + // is uploaded. A function loaded from git has no Root. + if f.Build.Git.URL == "" && f.Root == "" { + return "", f, errors.New("a local function directory is required to upload sources; set a git URL to build from a repository") + } + + // Resolve the commit to label the image with before creating any cluster + // resources: an unreachable repository or unknown revision fails here. + commit, err := sourceCommit(ctx, f) + if err != nil { + 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. // Remote deploy doesn't go through Client.Build, so we re-check here. @@ -239,7 +252,7 @@ func (pp *PipelinesProvider) Run(ctx context.Context, f fn.Function) (string, fn return "", f, fmt.Errorf("problem in creating secret: %v", err) } - err = createAndApplyPipelineRunTemplate(f, namespace, labels) + err = createAndApplyPipelineRunTemplate(f, namespace, labels, commit) if err != nil { return "", f, fmt.Errorf("problem in creating pipeline run: %v", err) } @@ -305,6 +318,21 @@ func (pp *PipelinesProvider) Run(ctx context.Context, f fn.Function) (string, fn return obj.Route, f, nil } +// sourceCommit returns the short commit SHA of the source the pipeline +// builds, used to label the image (org.opencontainers.image.revision): the +// resolved revision of the git repository when one is set, the local +// checkout otherwise. +func sourceCommit(ctx context.Context, f fn.Function) (string, error) { + if f.Build.Git.URL != "" { + commit, err := fn.GitRemoteCommit(ctx, f.Build.Git) + if err != nil { + return "", fmt.Errorf("cannot resolve the git source: %w", err) + } + return commit, nil + } + return fn.GitCommit(f.Root) +} + // Creates tar stream with the function sources as they were in "./source" directory. func sourcesAsTarStream(f fn.Function) *io.PipeReader { // Apply the same ignore policy as local builds: fn.IsIgnored always excludes diff --git a/pkg/pipelines/tekton/source_commit_test.go b/pkg/pipelines/tekton/source_commit_test.go new file mode 100644 index 0000000000..a4d7aad25b --- /dev/null +++ b/pkg/pipelines/tekton/source_commit_test.go @@ -0,0 +1,95 @@ +package tekton + +import ( + "context" + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" + "testing" + + fn "knative.dev/func/pkg/functions" +) + +// newGitRepo creates a repository with a single commit of a func.yaml and +// returns its path and the full hash of that commit. +func newGitRepo(t *testing.T) (dir, commit string) { + t.Helper() + if runtime.GOOS == "windows" { + // The fixture is served over file://, which go-git only supports + // through the git binary and not from a Windows path. + t.Skip("file:// repositories are not supported on Windows") + } + if _, err := exec.LookPath("git"); err != nil { + t.Skip("No 'git' found in path. Skipping test.") + } + dir = t.TempDir() + run := func(args ...string) string { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + cmd.Env = append(os.Environ(), + "GIT_AUTHOR_NAME=test", "GIT_AUTHOR_EMAIL=test@example.com", + "GIT_COMMITTER_NAME=test", "GIT_COMMITTER_EMAIL=test@example.com") + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("git %s: %v\n%s", strings.Join(args, " "), err, out) + } + return strings.TrimSpace(string(out)) + } + content := "specVersion: " + fn.LastSpecVersion() + "\nname: f\nruntime: go\ncreated: 2024-01-01T00:00:00Z\n" + if err := os.WriteFile(filepath.Join(dir, "func.yaml"), []byte(content), 0o644); err != nil { + t.Fatal(err) + } + run("init", "-q", "-b", "main") + run("add", ".") + run("commit", "-q", "-m", "initial") + return dir, run("rev-parse", "HEAD") +} + +// Test_sourceCommit ensures the image is labelled with the commit of the +// source the pipeline builds: the git revision when a repository is set, +// even from within an unrelated local checkout, and the local checkout +// otherwise. +func Test_sourceCommit(t *testing.T) { + remoteDir, remoteCommit := newGitRepo(t) + localDir, localCommit := newGitRepo(t) + + tests := []struct { + name string + f fn.Function + want string + }{ + {"git source without a local checkout", + fn.Function{Build: fn.BuildSpec{Git: fn.Git{URL: "file://" + remoteDir}}}, + remoteCommit[:7]}, + {"git source wins over a local checkout", + fn.Function{Root: localDir, Build: fn.BuildSpec{Git: fn.Git{URL: "file://" + remoteDir}}}, + remoteCommit[:7]}, + {"local checkout", + fn.Function{Root: localDir}, + localCommit[:7]}, + {"no source information", + fn.Function{Root: t.TempDir()}, + ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := sourceCommit(context.Background(), tt.f) + if err != nil { + t.Fatal(err) + } + if got != tt.want { + t.Errorf("expected commit %q, got %q", tt.want, got) + } + }) + } + + t.Run("unknown revision is an error", func(t *testing.T) { + f := fn.Function{Build: fn.BuildSpec{Git: fn.Git{URL: "file://" + remoteDir, Revision: "nope"}}} + if _, err := sourceCommit(context.Background(), f); err == nil { + t.Fatal("expected an error") + } + }) +} diff --git a/pkg/pipelines/tekton/task-buildpack.yaml.tmpl b/pkg/pipelines/tekton/task-buildpack.yaml.tmpl index c60e2c06df..be025e73db 100644 --- a/pkg/pipelines/tekton/task-buildpack.yaml.tmpl +++ b/pkg/pipelines/tekton/task-buildpack.yaml.tmpl @@ -72,6 +72,23 @@ spec: - name: CNB_PLATFORM_API value: "0.10" steps: + - name: clean-src + # The git-clone StepAction runs as user 65532 and deletes what is in the + # workspace before cloning. A previous run of this pipeline leaves the + # sources there owned by the build user, which 65532 cannot remove, so a + # second build of the same function from git failed. Empty the workspace + # as root first and hand the directory to the clone user. + image: docker.io/library/bash:5.1 + when: + - input: "$(params.GIT_REPOSITORY)" + operator: notin + values: [""] + script: | + #!/usr/bin/env bash + set -e + src="$(workspaces.source.path)" + find "$src" -mindepth 1 -delete + chown 65532:65532 "$src" - name: fetch-src ref: resolver: bundles diff --git a/pkg/pipelines/tekton/task-s2i.yaml.tmpl b/pkg/pipelines/tekton/task-s2i.yaml.tmpl index c739a45a1f..5ea3335222 100644 --- a/pkg/pipelines/tekton/task-s2i.yaml.tmpl +++ b/pkg/pipelines/tekton/task-s2i.yaml.tmpl @@ -57,6 +57,23 @@ spec: An optional workspace that allows providing a .docker/config.json file for Buildah to access the container registry. The file should be placed at the root of the Workspace with name config.json. optional: true steps: + - name: clean-src + # The git-clone StepAction runs as user 65532 and deletes what is in the + # workspace before cloning. A previous run of this pipeline leaves the + # sources there owned by the build user, which 65532 cannot remove, so a + # second build of the same function from git failed. Empty the workspace + # as root first and hand the directory to the clone user. + image: docker.io/library/bash:5.1 + when: + - input: "$(params.GIT_REPOSITORY)" + operator: notin + values: [""] + script: | + #!/usr/bin/env bash + set -e + src="$(workspaces.source.path)" + find "$src" -mindepth 1 -delete + chown 65532:65532 "$src" - name: fetch-src ref: resolver: bundles diff --git a/pkg/pipelines/tekton/tasks_test.go b/pkg/pipelines/tekton/tasks_test.go index f4fca3284e..95d7c74fe9 100644 --- a/pkg/pipelines/tekton/tasks_test.go +++ b/pkg/pipelines/tekton/tasks_test.go @@ -61,6 +61,25 @@ func TestGetTasks(t *testing.T) { if apiErr != nil { t.Fatalf("%+v\n", apiErr) } + + // The workspace is emptied as root right before the clone, and + // only when there is a repository to clone; the upload path must + // keep the sources it received. + steps := task.Spec.Steps + if len(steps) < 2 || steps[0].Name != "clean-src" || steps[1].Name != "fetch-src" { + t.Fatalf("expected clean-src to precede fetch-src, got %v", stepNames(steps)) + } + if len(steps[0].When) != 1 || steps[0].When[0].Input != "$(params.GIT_REPOSITORY)" { + t.Errorf("expected clean-src to be gated on GIT_REPOSITORY, got %+v", steps[0].When) + } }) } } + +func stepNames(steps []tektonv1.Step) []string { + names := make([]string, 0, len(steps)) + for _, s := range steps { + names = append(names, s.Name) + } + return names +} diff --git a/pkg/pipelines/tekton/templates.go b/pkg/pipelines/tekton/templates.go index b853194bcf..eea8e20bc8 100644 --- a/pkg/pipelines/tekton/templates.go +++ b/pkg/pipelines/tekton/templates.go @@ -347,9 +347,11 @@ func createAndApplyPipelineTemplate(f fn.Function, namespace string, labels map[ return createAndApplyResource(f.Root, pipelineFileName, template, "pipeline", getPipelineName(f), namespace, data) } -// createAndApplyPipelineRunTemplate creates and applies PipelineRun template for a standard on-cluster build -// all resources are created on the fly, if there's a PipelineRun defined in the project directory, it is used instead -func createAndApplyPipelineRunTemplate(f fn.Function, namespace string, labels map[string]string) error { +// createAndApplyPipelineRunTemplate creates and applies PipelineRun template +// for a standard on-cluster build all resources are created on the fly, if +// there's a PipelineRun defined in the project directory, it is used instead. +// commit is the source commit the image is labelled with (see sourceCommit). +func createAndApplyPipelineRunTemplate(f fn.Function, namespace string, labels map[string]string, commit string) error { contextDir := f.Build.Git.ContextDir if contextDir == "" && f.Build.Builder == builders.S2I { // TODO(lkingland): could instead update S2I to interpret empty string @@ -389,8 +391,6 @@ func createAndApplyPipelineRunTemplate(f fn.Function, namespace string, labels m tlsVerify = "false" } - commit, _ := fn.GitCommit(f.Root) - data := templateData{ FunctionName: f.Name, Annotations: f.Deploy.Annotations, @@ -431,12 +431,13 @@ func createAndApplyPipelineRunTemplate(f fn.Function, namespace string, labels m var manifestivalClient = k8s.GetManifestivalClient // createAndApplyResource tries to create and apply a resource to the k8s cluster from the input template and data, -// if there's the same resource already created in the project directory, it is used instead +// if there's the same resource already created in the project directory, it is used instead. +// An empty projectRoot (a function loaded from git) has no such directory. func createAndApplyResource(projectRoot, fileName, fileTemplate, kind, resourceName, namespace string, data interface{}) error { var source manifestival.Source filePath := path.Join(projectRoot, resourcesDirectory, fileName) - if _, err := os.Stat(filePath); !os.IsNotExist(err) { + if _, err := os.Stat(filePath); projectRoot != "" && !os.IsNotExist(err) { source = manifestival.Path(filePath) } else { tmpl, err := template.New("template").Parse(fileTemplate) diff --git a/pkg/pipelines/tekton/templates_test.go b/pkg/pipelines/tekton/templates_test.go index 7523ce931c..6e906482b8 100644 --- a/pkg/pipelines/tekton/templates_test.go +++ b/pkg/pipelines/tekton/templates_test.go @@ -297,6 +297,35 @@ var testData = []struct { }, } +// Test_createAndApplyPipelineRunTemplate_NoRoot ensures a function loaded +// from a git repository, which has no Root, yields a PipelineRun: nothing +// is read from a project directory in that case. +func Test_createAndApplyPipelineRunTemplate_NoRoot(t *testing.T) { + old := manifestivalClient + defer func() { manifestivalClient = old }() + manifestivalClient = func() (manifestival.Client, error) { + return fake.New(), nil + } + + f := fn.Function{ + Name: "remote-fn", + Runtime: "go", + Registry: TestRegistry, + Build: fn.BuildSpec{ + Builder: builders.Pack, + Git: fn.Git{URL: "https://example.com/alice/remote-fn.git", Revision: "main"}, + }, + } + f.Deploy.Image = "docker.io/alice/remote-fn" + + if err := createAndApplyPipelineTemplate(f, "test-ns", nil); err != nil { + t.Fatal(err) + } + if err := createAndApplyPipelineRunTemplate(f, "test-ns", nil, "abc1234"); err != nil { + t.Fatal(err) + } +} + func Test_createAndApplyPipelineRunTemplate(t *testing.T) { for _, tt := range testData { t.Run(tt.name, func(t *testing.T) { @@ -321,7 +350,7 @@ func Test_createAndApplyPipelineRunTemplate(t *testing.T) { f.Image = "docker.io/alice/" + f.Name f.Registry = TestRegistry - if err := createAndApplyPipelineRunTemplate(f, tt.namespace, tt.labels); (err != nil) != tt.wantErr { + if err := createAndApplyPipelineRunTemplate(f, tt.namespace, tt.labels, "abc1234"); (err != nil) != tt.wantErr { t.Errorf("createAndApplyPipelineRunTemplate() error = %v, wantErr %v", err, tt.wantErr) } })