From 51cb15b78ae0908334c06b0a71a03b80f6cb258e Mon Sep 17 00:00:00 2001 From: Luke Kingland Date: Wed, 21 Jun 2023 02:33:34 +0900 Subject: [PATCH] feat: enable host builder via cli (#1748) --- cmd/build.go | 104 ++++++++++++------ cmd/deploy.go | 86 ++++++++------- cmd/deploy_test.go | 4 +- cmd/invoke_test.go | 4 +- cmd/run.go | 42 +++---- pkg/builders/builders.go | 3 +- pkg/scaffolding/detectors_test.go | 3 + .../go/scaffolding/instanced-http/main.go | 7 ++ 8 files changed, 146 insertions(+), 107 deletions(-) create mode 100644 pkg/scaffolding/testdata/go/scaffolding/instanced-http/main.go diff --git a/cmd/build.go b/cmd/build.go index 00eba209..374c70f7 100644 --- a/cmd/build.go +++ b/cmd/build.go @@ -14,6 +14,7 @@ import ( "knative.dev/func/pkg/builders/s2i" "knative.dev/func/pkg/config" fn "knative.dev/func/pkg/functions" + "knative.dev/func/pkg/oci" ) func NewBuildCmd(newClient ClientFactory) *cobra.Command { @@ -156,8 +157,9 @@ func runBuild(cmd *cobra.Command, _ []string, newClient ClientFactory) (err erro // TODO: this logic is duplicated with runDeploy. Shouild be in buildConfig // constructor. - // Checks if there is a difference between defined registry and its value used as a prefix in the image tag - // In case of a mismatch a new image tag is created and used for build + // Checks if there is a difference between defined registry and its value + // used as a prefix in the image tag In case of a mismatch a new image tag is + // created and used for build. // Do not react if image tag has been changed outside configuration if f.Registry != "" && !cmd.Flags().Changed("image") && strings.Index(f.Image, "/") > 0 && !strings.HasPrefix(f.Image, f.Registry) { prfx := f.Registry @@ -171,23 +173,14 @@ func runBuild(cmd *cobra.Command, _ []string, newClient ClientFactory) (err erro } // Client - // Concrete implementations (ex builder) vary based on final effective config - o := []fn.Option{fn.WithRegistry(cfg.Registry)} - if f.Build.Builder == builders.Pack { - o = append(o, fn.WithBuilder(pack.NewBuilder( - pack.WithName(builders.Pack), - pack.WithTimestamp(cfg.WithTimestamp), - pack.WithVerbose(cfg.Verbose)))) - } else if f.Build.Builder == builders.S2I { - o = append(o, fn.WithBuilder(s2i.NewBuilder( - s2i.WithName(builders.S2I), - s2i.WithVerbose(cfg.Verbose)))) + clientOptions, err := cfg.clientOptions() + if err != nil { + return } - - client, done := newClient(ClientConfig{Verbose: cfg.Verbose}, o...) + client, done := newClient(ClientConfig{Verbose: cfg.Verbose}, clientOptions...) defer done() - // Build and (optionally) push + // Build buildOptions, err := cfg.buildOptions() if err != nil { return @@ -236,26 +229,6 @@ type buildConfig struct { WithTimestamp bool } -func (c buildConfig) buildOptions() (oo []fn.BuildOption, err error) { - oo = []fn.BuildOption{} - - // Platforms - // - // TODO: upgrade --platform to a multi-value field. The individual builder - // implementations are responsible for bubbling an error if they do - // not support this. Pack supports none, S2I supports one, host builder - // supports multi. - if c.Platform != "" { - parts := strings.Split(c.Platform, "/") - if len(parts) != 2 { - return oo, fmt.Errorf("the value for --patform must be in the form [OS]/[Architecture]. eg \"linux/amd64\"") - } - oo = append(oo, fn.BuildWithPlatforms([]fn.Platform{{OS: parts[0], Architecture: parts[1]}})) - } - - return -} - // newBuildConfig gathers options into a single build request. func newBuildConfig() buildConfig { return buildConfig{ @@ -369,3 +342,62 @@ func (c buildConfig) Validate() (err error) { return } + +// clientOptions returns options suitable for instantiating a client based on +// the current state of the build config object. +// This will be unnecessary and refactored away when the host-based OCI +// builder and pusher are the default implementations and the Pack and S2I +// constructors simplified. +// +// TODO: Platform is currently only used by the S2I builder. This should be +// a multi-valued argument which passes through to the "host" builder (which +// supports multi-arch/platform images), and throw an error if either trying +// to specify a platform for buildpacks, or trying to specify more than one +// for S2I. +// +// TODO: As a further optimization, it might be ideal to only build the +// image necessary for the target cluster, since the end product of a function +// deployment is not the contiainer, but rather the running service. +func (c buildConfig) clientOptions() ([]fn.Option, error) { + o := []fn.Option{fn.WithRegistry(c.Registry)} + if c.Builder == builders.Host { + o = append(o, + fn.WithBuilder(oci.NewBuilder(builders.Host, c.Verbose)), + fn.WithPusher(oci.NewPusher(false, c.Verbose))) + } else if c.Builder == builders.Pack { + o = append(o, + fn.WithBuilder(pack.NewBuilder( + pack.WithName(builders.Pack), + pack.WithTimestamp(c.WithTimestamp), + pack.WithVerbose(c.Verbose)))) + } else if c.Builder == builders.S2I { + o = append(o, + fn.WithBuilder(s2i.NewBuilder( + s2i.WithName(builders.S2I), + s2i.WithVerbose(c.Verbose)))) + } else { + return o, builders.ErrUnknownBuilder{Name: c.Builder, Known: KnownBuilders()} + } + return o, nil +} + +// buildOptions returns options for use with the client.Build request +func (c buildConfig) buildOptions() (oo []fn.BuildOption, err error) { + oo = []fn.BuildOption{} + + // Platforms + // + // TODO: upgrade --platform to a multi-value field. The individual builder + // implementations are responsible for bubbling an error if they do + // not support this. Pack supports none, S2I supports one, host builder + // supports multi. + if c.Platform != "" { + parts := strings.Split(c.Platform, "/") + if len(parts) != 2 { + return oo, fmt.Errorf("the value for --patform must be in the form [OS]/[Architecture]. eg \"linux/amd64\"") + } + oo = append(oo, fn.BuildWithPlatforms([]fn.Platform{{OS: parts[0], Architecture: parts[1]}})) + } + + return +} diff --git a/cmd/deploy.go b/cmd/deploy.go index b8734078..53b330f5 100644 --- a/cmd/deploy.go +++ b/cmd/deploy.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "io" + "os" "strconv" "strings" @@ -13,8 +14,6 @@ import ( "k8s.io/apimachinery/pkg/api/resource" "knative.dev/client-pkg/pkg/util" "knative.dev/func/pkg/builders" - "knative.dev/func/pkg/builders/buildpacks" - "knative.dev/func/pkg/builders/s2i" "knative.dev/func/pkg/config" fn "knative.dev/func/pkg/functions" "knative.dev/func/pkg/k8s" @@ -251,26 +250,11 @@ func runDeploy(cmd *cobra.Command, newClient ClientFactory) (err error) { // Informative non-error messages regarding the final deployment request printDeployMessages(cmd.OutOrStdout(), cfg) - // Client - // Concrete implementations (ex builder) vary based on final effective cfg. - var builder fn.Builder - if f.Build.Builder == builders.Pack { - builder = buildpacks.NewBuilder( - buildpacks.WithName(builders.Pack), - buildpacks.WithVerbose(cfg.Verbose), - buildpacks.WithTimestamp(cfg.Timestamp), - ) - } else if f.Build.Builder == builders.S2I { - builder = s2i.NewBuilder( - s2i.WithName(builders.S2I), - s2i.WithVerbose(cfg.Verbose)) - } else { - return builders.ErrUnknownBuilder{Name: f.Build.Builder, Known: KnownBuilders()} + clientOptions, err := cfg.clientOptions() + if err != nil { + return } - - client, done := newClient(ClientConfig{Namespace: f.Deploy.Namespace, Verbose: cfg.Verbose}, - fn.WithRegistry(cfg.Registry), - fn.WithBuilder(builder)) + client, done := newClient(ClientConfig{Namespace: f.Deploy.Namespace, Verbose: cfg.Verbose}, clientOptions...) defer done() // Deploy @@ -281,15 +265,12 @@ func runDeploy(cmd *cobra.Command, newClient ClientFactory) (err error) { return } } else { - if shouldBuild(cfg.Build, f, client) { // --build or "auto" with FS changes - buildOptions, err := cfg.buildOptions() - if err != nil { - return err - } - - if f, err = client.Build(cmd.Context(), f, buildOptions...); err != nil { - return err - } + var buildOptions []fn.BuildOption + if buildOptions, err = cfg.buildOptions(); err != nil { + return + } + if f, err = build(cmd, cfg.Build, f, client, buildOptions); err != nil { + return } if cfg.Push { if f, err = client.Push(cmd.Context(), f); err != nil { @@ -313,16 +294,30 @@ func runDeploy(cmd *cobra.Command, newClient ClientFactory) (err error) { return f.Stamp() } -// shouldBuild returns true if the value of the build option is a truthy value, -// or if it is the literal "auto" and the function reports as being currently -// unbuilt. Invalid errors are not reported as this is the purview of -// deployConfig.Validate -func shouldBuild(buildCfg string, f fn.Function, client *fn.Client) bool { - if buildCfg == "auto" { - return !f.Built() // first build or modified filesystem +// build when flag == 'auto' and the function is out-of-date, or when the +// flag value is explicitly truthy such as 'true' or '1'. Error if flag +// is neither 'auto' nor parseable as a boolean. Return CLI-specific error +// message verbeage suitable for both Deploy and Run commands which feature an +// optional build step. +func build(cmd *cobra.Command, flag string, f fn.Function, client *fn.Client, buildOptions []fn.BuildOption) (fn.Function, error) { + var err error + if flag == "auto" { + if f.Built() { + fmt.Fprintln(cmd.OutOrStdout(), "function up-to-date. Force rebuild with --build") + } else { + if f, err = client.Build(cmd.Context(), f, buildOptions...); err != nil { + return f, err + } + } + } else if build, _ := strconv.ParseBool(flag); build { + if f, err = client.Build(cmd.Context(), f, buildOptions...); err != nil { + return f, err + } + } else if _, err = strconv.ParseBool(flag); err != nil { + return f, fmt.Errorf("--build ($FUNC_BUILD) %q not recognized. Should be 'auto' or a truthy value such as 'true', 'false', '0', or '1'.", flag) + } - build, _ := strconv.ParseBool(buildCfg) - return build + return f, nil } func NewRegistryValidator(path string) survey.Validator { @@ -368,6 +363,19 @@ func KnownBuilders() builders.Known { // the set of builders enumerated in the builders pacakage. // However, future third-party integrations may support less than, or more // builders, and certain environmental considerations may alter this list. + + // Also a good place to stick feature-flags; to wit: + enable_host, _ := strconv.ParseBool(os.Getenv("FUNC_ENABLE_HOST_BUILDER")) + if !enable_host { + bb := []string{} + for _, b := range builders.All() { + if b != builders.Host { + bb = append(bb, b) + } + } + return bb + } + return builders.All() } diff --git a/cmd/deploy_test.go b/cmd/deploy_test.go index 8afc7c5f..1a27fd82 100644 --- a/cmd/deploy_test.go +++ b/cmd/deploy_test.go @@ -1516,9 +1516,9 @@ func TestDeploy_UnsetFlag(t *testing.T) { } // Test_ValidateBuilder tests that the bulder validation accepts the -// accepts === the set of known builders. +// the set of known builders, and spot-checks an error is thrown for unknown. func Test_ValidateBuilder(t *testing.T) { - for _, name := range builders.All() { + for _, name := range KnownBuilders() { if err := ValidateBuilder(name); err != nil { t.Fatalf("expected builder '%v' to be valid, but got error: %v", name, err) } diff --git a/cmd/invoke_test.go b/cmd/invoke_test.go index c7e69b96..8995041f 100644 --- a/cmd/invoke_test.go +++ b/cmd/invoke_test.go @@ -47,10 +47,10 @@ func TestInvoke(t *testing.T) { fmt.Fprintf(os.Stderr, "error serving: %v", err) } }() - _, port, _ := net.SplitHostPort(l.Addr().String()) + host, port, _ := net.SplitHostPort(l.Addr().String()) errs := make(chan error, 10) stop := func() error { _ = s.Close(); return nil } - return fn.NewJob(f, "127.0.0.1", port, errs, stop, false) + return fn.NewJob(f, host, port, errs, stop, false) } // Run the mock http service function interloper diff --git a/cmd/run.go b/cmd/run.go index e21f5e2b..3315b707 100644 --- a/cmd/run.go +++ b/cmd/run.go @@ -12,9 +12,6 @@ import ( "github.com/ory/viper" "github.com/spf13/cobra" - "knative.dev/func/pkg/builders" - pack "knative.dev/func/pkg/builders/buildpacks" - "knative.dev/func/pkg/builders/s2i" "knative.dev/func/pkg/config" "knative.dev/func/pkg/docker" fn "knative.dev/func/pkg/functions" @@ -148,10 +145,10 @@ func runRun(cmd *cobra.Command, args []string, newClient ClientFactory) (err err if cfg, err = newRunConfig(cmd).Prompt(); err != nil { return } - if err = cfg.Validate(cmd); err != nil { + if f, err = fn.NewFunction(cfg.Path); err != nil { return } - if f, err = fn.NewFunction(cfg.Path); err != nil { + if err = cfg.Validate(cmd, f); err != nil { return } if !f.Initialized() { @@ -176,39 +173,30 @@ func runRun(cmd *cobra.Command, args []string, newClient ClientFactory) (err err } // Client - // - // Builder and runner implementations are based on the value of f.Build.Builder, and - // - o := []fn.Option{} - if f.Build.Builder == builders.Pack { - o = append(o, fn.WithBuilder(pack.NewBuilder( - pack.WithName(builders.Pack), - pack.WithVerbose(cfg.Verbose)))) - } else if f.Build.Builder == builders.S2I { - o = append(o, fn.WithBuilder(s2i.NewBuilder( - s2i.WithName(builders.S2I), - s2i.WithVerbose(cfg.Verbose)))) + clientOptions, err := cfg.clientOptions() + if err != nil { + return } if cfg.Container { - o = append(o, fn.WithRunner(docker.NewRunner(cfg.Verbose, os.Stdout, os.Stderr))) + clientOptions = append(clientOptions, fn.WithRunner(docker.NewRunner(cfg.Verbose, os.Stdout, os.Stderr))) } if cfg.StartTimeout != 0 { - o = append(o, fn.WithStartTimeout(cfg.StartTimeout)) + clientOptions = append(clientOptions, fn.WithStartTimeout(cfg.StartTimeout)) } - client, done := newClient(ClientConfig{Verbose: cfg.Verbose}, o...) + client, done := newClient(ClientConfig{Verbose: cfg.Verbose}, clientOptions...) defer done() // Build // // If requesting to run via the container, build the container if it is // either out-of-date or a build was explicitly requested. - if cfg.Container && shouldBuild(cfg.Build, f, client) { + if cfg.Container { buildOptions, err := cfg.buildOptions() if err != nil { return err } - if f, err = client.Build(cmd.Context(), f, buildOptions...); err != nil { + if f, err = build(cmd, cfg.Build, f, client, buildOptions); err != nil { return err } } @@ -321,7 +309,7 @@ func (c runConfig) Prompt() (runConfig, error) { return c, nil } -func (c runConfig) Validate(cmd *cobra.Command) (err error) { +func (c runConfig) Validate(cmd *cobra.Command, f fn.Function) (err error) { // Bubble if err = c.buildConfig.Validate(); err != nil { return @@ -335,13 +323,13 @@ func (c runConfig) Validate(cmd *cobra.Command) (err error) { } // There is currently no local host runner implemented, so specifying - // --container=false should always return an informative error to the user - // such that they do not receive the rather cryptic "no runner defined" - // error from a Client instance which was instantiated with no runner. + // --container=false should return an informative error for runtimes other + // than Go that is more helpful than the cryptic, though correct, error + // from the Client that it was instantated without a runner. // TODO: modify this check when the local host runner is available to // only generate this error when --container==false && the --language is // not yet implemented. - if !c.Container { + if !c.Container && f.Runtime != "go" { return errors.New("the ability to run functions outside of a container via 'func run' is coming soon.") } diff --git a/pkg/builders/builders.go b/pkg/builders/builders.go index 3575a8d3..e172834c 100644 --- a/pkg/builders/builders.go +++ b/pkg/builders/builders.go @@ -13,6 +13,7 @@ import ( ) const ( + Host = "host" Pack = "pack" S2I = "s2i" Default = Pack @@ -22,7 +23,7 @@ const ( type Known []string func All() Known { - return Known([]string{Pack, S2I}) + return Known([]string{Host, Pack, S2I}) } func (k Known) String() string { diff --git a/pkg/scaffolding/detectors_test.go b/pkg/scaffolding/detectors_test.go index 0ba05942..519b5d17 100644 --- a/pkg/scaffolding/detectors_test.go +++ b/pkg/scaffolding/detectors_test.go @@ -1,3 +1,6 @@ +//go:build !integration +// +build !integration + package scaffolding import ( diff --git a/pkg/scaffolding/testdata/go/scaffolding/instanced-http/main.go b/pkg/scaffolding/testdata/go/scaffolding/instanced-http/main.go new file mode 100644 index 00000000..a3dd973f --- /dev/null +++ b/pkg/scaffolding/testdata/go/scaffolding/instanced-http/main.go @@ -0,0 +1,7 @@ +package main + +import "fmt" + +func main() { + fmt.Println("Hello, World!") +}