diff --git a/cmd/build.go b/cmd/build.go index 06ce2cb101..98d5ad5a00 100644 --- a/cmd/build.go +++ b/cmd/build.go @@ -155,23 +155,6 @@ func runBuild(cmd *cobra.Command, _ []string, newClient ClientFactory) (err erro } f = cfg.Configure(f) // Updates f at path to include build request values - // 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. - // 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 - if prfx[len(prfx)-1:] != "/" { - prfx = prfx + "/" - } - sps := strings.Split(f.Image, "/") - updImg := prfx + sps[len(sps)-1] - fmt.Fprintf(cmd.ErrOrStderr(), "Warning: function has current image '%s' which has a different registry than the currently configured registry '%s'. The new image tag will be '%s'. To use an explicit image, use --image.\n", f.Image, f.Registry, updImg) - f.Image = updImg - } - // Client clientOptions, err := cfg.clientOptions() if err != nil { @@ -193,7 +176,6 @@ func runBuild(cmd *cobra.Command, _ []string, newClient ClientFactory) (err erro return } } - if err = f.Write(); err != nil { return } @@ -300,16 +282,12 @@ func (c buildConfig) Prompt() (buildConfig, error) { // Image Name Override // Calculate a better image name message which shows the value of the final // image name as it will be calculated if an explicit image name is not used. - var imagePromptMessageSuffix string - if name := deriveImage(c.Image, c.Registry, c.Path); name != "" { - imagePromptMessageSuffix = fmt.Sprintf(". if not specified, the default '%v' will be used')", name) - } qs := []*survey.Question{ { Name: "image", Prompt: &survey.Input{ - Message: fmt.Sprintf("Image name to use (e.g. quay.io/boson/node-sample)%v:", imagePromptMessageSuffix), + Message: "Optionally specify an exact image name to use (e.g. quay.io/boson/node-sample:latest)", }, }, { diff --git a/cmd/client.go b/cmd/client.go index c8c795c38e..6ff87bc232 100644 --- a/cmd/client.go +++ b/cmd/client.go @@ -69,7 +69,7 @@ func NewClient(cfg ClientConfig, options ...fn.Option) (*fn.Client, func()) { fn.WithTransport(t), fn.WithRepositoriesPath(config.RepositoriesPath()), fn.WithBuilder(buildpacks.NewBuilder(buildpacks.WithVerbose(cfg.Verbose))), - fn.WithRemover(knative.NewRemover(cfg.Namespace, cfg.Verbose)), + fn.WithRemover(knative.NewRemover(cfg.Verbose)), fn.WithDescriber(knative.NewDescriber(cfg.Namespace, cfg.Verbose)), fn.WithLister(knative.NewLister(cfg.Namespace, cfg.Verbose)), fn.WithDeployer(d), diff --git a/cmd/client_test.go b/cmd/client_test.go index aa9652be0c..b7a63f1b5a 100644 --- a/cmd/client_test.go +++ b/cmd/client_test.go @@ -8,6 +8,8 @@ import ( "knative.dev/func/pkg/mock" ) +const namespace = "func" + // Test_NewTestClient ensures that the convenience method for // constructing a mocked client for testing properly considers options: // options provided to the factory constructor are considered exaustive, @@ -26,7 +28,7 @@ func Test_NewTestClient(t *testing.T) { client, _ := clientFn(ClientConfig{}, fn.WithDescriber(describer)) // Trigger an invocation of the mocks - err := client.Remove(context.Background(), fn.Function{Name: "test"}, true) + err := client.Remove(context.Background(), fn.Function{Name: "test", Deploy: fn.DeploySpec{Namespace: namespace}}, true) if err != nil { t.Fatal(err) } diff --git a/cmd/create.go b/cmd/create.go index b123135113..c066eb20f9 100644 --- a/cmd/create.go +++ b/cmd/create.go @@ -141,7 +141,6 @@ func runCreate(cmd *cobra.Command, args []string, newClient ClientFactory) (err if err != nil { return err } - // Confirm fmt.Fprintf(cmd.OutOrStderr(), "Created %v function in %v\n", cfg.Runtime, cfg.Path) return nil diff --git a/cmd/delete.go b/cmd/delete.go index ec8573409d..2834fee839 100644 --- a/cmd/delete.go +++ b/cmd/delete.go @@ -62,7 +62,14 @@ func runDelete(cmd *cobra.Command, args []string, newClient ClientFactory) (err return } + // check that name is defined when deleting a Function in specific namespace + if cfg.Name == "" && cfg.Namespace != "" { + return fmt.Errorf("function name is required when namespace is specified") + } + var function fn.Function + // initialize namespace from the config + var namespace = cfg.Namespace // Initialize func with explicit name (when provided) if len(args) > 0 && args[0] != "" { @@ -84,17 +91,17 @@ func runDelete(cmd *cobra.Command, args []string, newClient ClientFactory) (err return fn.NewErrNotInitialized(function.Root) } - // If not provided, use the function's extant namespace - if !cmd.Flags().Changed("namespace") { - cfg.Namespace = function.Deploy.Namespace + // use the function's extant namespace -- already deployed function + if !cmd.Flags().Changed("namespace") && function.Deploy.Namespace != "" { + namespace = function.Deploy.Namespace } - } // Create a client instance from the now-final config - client, done := newClient(ClientConfig{Namespace: cfg.Namespace, Verbose: cfg.Verbose}) + client, done := newClient(ClientConfig{Namespace: namespace, Verbose: cfg.Verbose}) defer done() + function.Deploy.Namespace = namespace // Invoke remove using the concrete client impl return client.Remove(cmd.Context(), function, cfg.DeleteAll) } diff --git a/cmd/delete_test.go b/cmd/delete_test.go index 0f2f9e2e55..50f3ad8233 100644 --- a/cmd/delete_test.go +++ b/cmd/delete_test.go @@ -1,93 +1,103 @@ package cmd import ( + "context" "os" - "path/filepath" "testing" fn "knative.dev/func/pkg/functions" "knative.dev/func/pkg/mock" ) -// TestDelete_Namespace ensures that the namespace provided to the client -// for use when deleting a function is set -// 1. The flag /env variable if provided -// 2. The namespace of the function at path if provided -// 3. The user's current active namespace -func TestDelete_Namespace(t *testing.T) { - root := fromTempDirectory(t) - - // Ensure that the default is "default" when no context can be identified - t.Setenv("KUBECONFIG", filepath.Join(cwd(), "nonexistent")) - t.Setenv("KUBERNETES_SERVICE_HOST", "") - cmd := NewDeleteCmd(func(cc ClientConfig, options ...fn.Option) (*fn.Client, func()) { - if cc.Namespace != "" { - t.Fatalf("expected '', got '%v'", cc.Namespace) +// TestDelete_Default ensures that the deployed function is deleted correctly +// with default options +func TestDelete_Default(t *testing.T) { + var ( + root = fromTempDirectory(t) + namespace = "myns" + remover = mock.NewRemover() + err error + ) + + remover.RemoveFn = func(_, ns string) error { + if ns != namespace { + t.Fatalf("expected delete namespace '%v', got '%v'", namespace, ns) } - return fn.New(), func() {} - }) - cmd.SetArgs([]string{"somefunc"}) // delete by name such that no f need be created - if err := cmd.Execute(); err != nil { - t.Fatal(err) + return nil } // Ensure the extant function's namespace is used f := fn.Function{ - Root: root, - Runtime: "go", + Root: root, + Runtime: "go", + Registry: TestRegistry, + Name: "testname", Deploy: fn.DeploySpec{ - Namespace: "deployed", + Namespace: namespace, //simulate deployed Function }, } - if _, err := fn.New().Init(f); err != nil { + + if f, err = fn.New().Init(f); err != nil { t.Fatal(err) } - cmd = NewDeleteCmd(func(cc ClientConfig, options ...fn.Option) (*fn.Client, func()) { - if cc.Namespace != "deployed" { - t.Fatalf("expected 'deployed', got '%v'", cc.Namespace) - } - return fn.New(), func() {} - }) - cmd.SetArgs([]string{}) - if err := cmd.Execute(); err != nil { + + if err = f.Write(); err != nil { t.Fatal(err) } - // Ensure an explicit namespace is plumbed through - cmd = NewDeleteCmd(func(cc ClientConfig, options ...fn.Option) (*fn.Client, func()) { - if cc.Namespace != "ns" { - t.Fatalf("expected 'ns', got '%v'", cc.Namespace) - } - return fn.New(), func() {} - }) - cmd.SetArgs([]string{"--namespace", "ns"}) + cmd := NewDeleteCmd(NewTestClient(fn.WithRemover(remover))) + cmd.SetArgs([]string{}) //dont give any arguments to 'func delete' -- default if err := cmd.Execute(); err != nil { t.Fatal(err) } + // Fail if remover's .Remove not invoked at all + if !remover.RemoveInvoked { + t.Fatal("fn.Remover not invoked") + } } // TestDelete_ByName ensures that running delete specifying the name of the // function explicitly as an argument invokes the remover appropriately. func TestDelete_ByName(t *testing.T) { var ( - testname = "testname" // explicit name for the function - remover = mock.NewRemover() // with a mock remover + root = fromTempDirectory(t) + testname = "testname" // explicit name for the function + testnamespace = "testnamespace" // explicit namespace for the function + remover = mock.NewRemover() // with a mock remover + err error ) // Remover fails the test if it receives the incorrect name - // an incorrect name. - remover.RemoveFn = func(n string) error { + remover.RemoveFn = func(n, _ string) error { if n != testname { t.Fatalf("expected delete name %v, got %v", testname, n) } return nil } + f := fn.Function{ + Root: root, + Runtime: "go", + Registry: TestRegistry, + Name: "testname", + } + + if f, err = fn.New().Init(f); err != nil { + t.Fatal(err) + } + + // simulate deployed function in namespace for the client Remover + f.Deploy.Namespace = testnamespace + + if err = f.Write(); err != nil { + t.Fatal(err) + } + // Create a command with a client constructor fn that instantiates a client - // with a the mocked remover. + // with a mocked remover. cmd := NewDeleteCmd(NewTestClient(fn.WithRemover(remover))) - cmd.SetArgs([]string{testname}) + cmd.SetArgs([]string{testname}) // run: func delete if err := cmd.Execute(); err != nil { t.Fatal(err) @@ -99,6 +109,90 @@ func TestDelete_ByName(t *testing.T) { } } +// TestDelete_Namespace ensures that remover is envoked when --namespace flag is +// given --> func delete myfunc --namespace myns +func TestDelete_Namespace(t *testing.T) { + var ( + namespace = "myns" + remover = mock.NewRemover() + testname = "testname" + ) + + remover.RemoveFn = func(_, ns string) error { + if ns != namespace { + t.Fatalf("expected delete namespace '%v', got '%v'", namespace, ns) + } + return nil + } + + cmd := NewDeleteCmd(NewTestClient(fn.WithRemover(remover))) + cmd.SetArgs([]string{testname, "--namespace", namespace}) + if err := cmd.Execute(); err != nil { + t.Fatal(err) + } + + if !remover.RemoveInvoked { + t.Fatal("remover was not invoked") + } +} + +// TestDelete_NamespaceFlagPriority ensures that even thought there is +// a deployed function the namespace flag takes precedence and essentially +// ignores the the function on disk +func TestDelete_NamespaceFlagPriority(t *testing.T) { + var ( + root = fromTempDirectory(t) + namespace = "myns" + namespace2 = "myns2" + remover = mock.NewRemover() + testname = "testname" + err error + ) + + remover.RemoveFn = func(_, ns string) error { + if ns != namespace2 { + t.Fatalf("expected delete namespace '%v', got '%v'", namespace2, ns) + } + return nil + } + + // Ensure the extant function's namespace is used + f := fn.Function{ + Name: testname, + Root: root, + Runtime: "go", + Registry: TestRegistry, + Namespace: namespace, + } + client := fn.New() + _, _, err = client.New(context.Background(), f) + if err != nil { + t.Fatal(err) + } + + cmd := NewDeleteCmd(NewTestClient(fn.WithRemover(remover))) + cmd.SetArgs([]string{testname, "--namespace", namespace2}) + if err := cmd.Execute(); err != nil { + t.Fatal(err) + } + + if !remover.RemoveInvoked { + t.Fatal("remover was not invoked") + } +} + +// TestDelete_NamespaceWithoutNameFails ensures that providing wrong argument +// combination fails nice and fast (no name of the Function) +func TestDelete_NamespaceWithoutNameFails(t *testing.T) { + _ = fromTempDirectory(t) + + cmd := NewDeleteCmd(NewTestClient()) + cmd.SetArgs([]string{"--namespace=myns"}) + if err := cmd.Execute(); err == nil { + t.Fatal("invoking Delete with namespace BUT without name provided anywhere") + } +} + // TestDelete_ByProject ensures that running delete with a valid project as its // context invokes remove and with the correct name (reads name from func.yaml) func TestDelete_ByProject(t *testing.T) { @@ -106,10 +200,9 @@ func TestDelete_ByProject(t *testing.T) { // Write a func.yaml config which specifies a name funcYaml := `name: bar -namespace: "" +namespace: "func" runtime: go image: "" -imageDigest: "" builder: quay.io/boson/faas-go-builder builders: default: quay.io/boson/faas-go-builder @@ -124,7 +217,7 @@ created: 2021-01-01T00:00:00+00:00 // A mock remover which fails if the name from the func.yaml is not received. remover := mock.NewRemover() - remover.RemoveFn = func(n string) error { + remover.RemoveFn = func(n, _ string) error { if n != "bar" { t.Fatalf("expected name 'bar', got '%v'", n) } @@ -148,6 +241,50 @@ created: 2021-01-01T00:00:00+00:00 } } +// TestDelete_ByPath ensures that providing only path deletes the Function +// successfully +func TestDelete_ByPath(t *testing.T) { + var ( + + // A mock remover which will be sampled to ensure it is not invoked. + remover = mock.NewRemover() + root = fromTempDirectory(t) + err error + namespace = "func" + ) + + // Ensure the extant function's namespace is used + f := fn.Function{ + Root: root, + Runtime: "go", + Registry: TestRegistry, + Deploy: fn.DeploySpec{Namespace: namespace}, + } + + // Initialize a function in temp dir + if f, err = fn.New().Init(f); err != nil { + t.Fatal(err) + } + if err = f.Write(); err != nil { + t.Fatal(err) + } + + // Command with a Client constructor using the mock remover. + cmd := NewDeleteCmd(NewTestClient(fn.WithRemover(remover))) + + // Execute the command only with the path argument + cmd.SetArgs([]string{"-p", root}) + err = cmd.Execute() + if err != nil { + t.Fatalf("failed with: %v", err) + } + + // Also fail if remover's .Remove is not invoked. + if !remover.RemoveInvoked { + t.Fatal("fn.Remover not invoked despite valid argument") + } +} + // TestDelete_NameAndPathExclusivity ensures that providing both a name and a // path generates an error. // Providing the --path (-p) flag indicates the name of the function to delete diff --git a/cmd/deploy.go b/cmd/deploy.go index 85338e88f5..b7ddfebe46 100644 --- a/cmd/deploy.go +++ b/cmd/deploy.go @@ -234,34 +234,37 @@ func runDeploy(cmd *cobra.Command, newClient ClientFactory) (err error) { return } - // TODO: this is duplicate logic with runBuild and runRun. - // Refactor both to have this logic part of creating the buildConfig and thus - // shared because newDeployConfig uses newBuildConfig for its embedded struct. - if f.Registry != "" && !cmd.Flags().Changed("image") && strings.Index(f.Image, "/") > 0 && !strings.HasPrefix(f.Image, f.Registry) { - prfx := f.Registry - if prfx[len(prfx)-1:] != "/" { - prfx = prfx + "/" + // If using Openshift registry AND redeploying Function, update image registry + if f.Namespace != "" && f.Namespace != f.Deploy.Namespace && f.Deploy.Namespace != "" { + // when running openshift, namespace is tied to registry, override on --namespace change + // The most default part of registry (in buildConfig) checks 'k8s.IsOpenShift()' and if true, + // sets default registry by current namespace + + // If Function is being moved to different namespace in Openshift -- update registry + if k8s.IsOpenShift() { + // this name is based of k8s package + f.Registry = "image-registry.openshift-image-registry.svc:5000/" + f.Namespace + if cfg.Verbose { + fmt.Fprintf(cmd.OutOrStdout(), "Info: Overriding openshift registry to %s\n", f.Registry) + } } - sps := strings.Split(f.Image, "/") - updImg := prfx + sps[len(sps)-1] - fmt.Fprintf(cmd.ErrOrStderr(), "Warning: function has current image '%s' which has a different registry than the currently configured registry '%s'. The new image tag will be '%s'. To use an explicit image, use --image.\n", f.Image, f.Registry, updImg) - f.Image = updImg } // Informative non-error messages regarding the final deployment request - printDeployMessages(cmd.OutOrStdout(), cfg) + printDeployMessages(cmd.OutOrStdout(), f) clientOptions, err := cfg.clientOptions() if err != nil { return } - client, done := newClient(ClientConfig{Namespace: f.Deploy.Namespace, Verbose: cfg.Verbose}, clientOptions...) + client, done := newClient(ClientConfig{Namespace: f.Namespace, Verbose: cfg.Verbose}, clientOptions...) defer done() // Deploy if cfg.Remote { // Invoke a remote build/push/deploy pipeline - // Returned is the function with fields like Registry and Image populated. + // Returned is the function with fields like Registry, f.Deploy.Image & + // f.Deploy.Namespace populated. if f, err = client.RunPipeline(cmd.Context(), f); err != nil { return } @@ -270,14 +273,34 @@ func runDeploy(cmd *cobra.Command, newClient ClientFactory) (err error) { if buildOptions, err = cfg.buildOptions(); err != nil { return } - if f, err = build(cmd, cfg.Build, f, client, buildOptions); err != nil { + + // check if --image was provided with a digest. 'digested' bool indicates if + // image contains a digest or not (image is "digested"). + var digested bool + digested, err = isDigested(cfg.Image) + if err != nil { return } - if cfg.Push { - if f, err = client.Push(cmd.Context(), f); err != nil { + + // If user provided --image with digest, they are requesting that specific + // image to be used which means building phase should be skipped and image + // should be deployed as is + if digested { + f.Deploy.Image = cfg.Image + } else { + // if NOT digested, build and push the Function first + 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 { + return + } + } + // f.Build.Image is set in Push for now, just set it as a deployed image + f.Deploy.Image = f.Build.Image } + if f, err = client.Deploy(cmd.Context(), f, fn.WithDeploySkipBuildCheck(cfg.Build == "false")); err != nil { return } @@ -502,10 +525,10 @@ 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.Deploy.Namespace = c.Namespace f.Deploy.ServiceAccountName = c.ServiceAccountName f.Local.Remote = c.Remote @@ -518,17 +541,6 @@ func (c deployConfig) Configure(f fn.Function) (fn.Function, error) { f.Build.PVCSize = c.PVCSize } - // ImageDigest - // Parsed off f.Image if provided. Deploying adds the ability to specify a - // digest on the associated image (not available on build as nonsensical). - newDigest, err := imageDigest(f.Image) - if err != nil { - return f, err - } - if newDigest != "" { - f.ImageDigest = newDigest - } - // Envs // Preprocesses any Envs provided (which may include removals) into a final // set @@ -626,8 +638,8 @@ func (c deployConfig) Validate(cmd *cobra.Command) (err error) { // Check Image Digest was included // (will be set on the function during .Configure) - var digest string - if digest, err = imageDigest(c.Image); err != nil { + var digest bool + if digest, err = isDigested(c.Image); err != nil { return } @@ -643,12 +655,14 @@ func (c deployConfig) Validate(cmd *cobra.Command) (err error) { v, _ := strconv.ParseBool(s) return v } - if digest != "" && truthy(c.Build) { + + // Can not build when specifying an --image with digest + if digest && truthy(c.Build) { return errors.New("building can not be enabled when using an image with digest") } // Can not push when specifying an --image with digest - if digest != "" && c.Push { + if digest && c.Push { return errors.New("pushing is not valid when specifying an image with digest") } @@ -672,54 +686,25 @@ func (c deployConfig) Validate(cmd *cobra.Command) (err error) { return } -// imageDigest returns the image digest from a full image string if it exists, -// and includes basic validation that a provided digest is correctly formatted. -func imageDigest(v string) (digest string, err error) { - vv := strings.Split(v, "@") - if len(vv) < 2 { - return // has no digest - } else if len(vv) > 2 { - err = fmt.Errorf("image '%v' contains an invalid digest (extra '@')", v) - return - } - digest = vv[1] - - if !strings.HasPrefix(digest, "sha256:") { - err = fmt.Errorf("image digest '%s' requires 'sha256:' prefix", digest) - return - } - - if len(digest[7:]) != 64 { - err = fmt.Errorf("image digest '%v' has an invalid sha256 hash length of %v when it should be 64", digest, len(digest[7:])) - } - - return -} - // printDeployMessages to the output. Non-error deployment messages. -func printDeployMessages(out io.Writer, cfg deployConfig) { - // Digest - // ------ - // If providing an image digest, print this, and note that the values - // of push and build are ignored. - // TODO: perhaps just error if either --push or --build were actually - // provided (using the cobra .Changed accessor) - digest, err := imageDigest(cfg.Image) - if err != nil && digest != "" { - fmt.Fprintf(out, "Deploying image '%v' with digest '%s'. Build and push are disabled.\n", cfg.Image, digest) +func printDeployMessages(out io.Writer, f fn.Function) { + digest, err := isDigested(f.Image) + if err == nil && digest { + fmt.Fprintf(out, "Deploying image '%v', which has a digest. Build and push are disabled.\n", f.Image) } // Namespace // --------- - f, _ := fn.NewFunction(cfg.Path) currentNamespace := f.Deploy.Namespace // will be "" if no initialed f at path. - targetNamespace := cfg.Namespace + targetNamespace := f.Namespace + if targetNamespace == "" { + return + } - // If potentially creating a duplicate deployed function in a different - // namespace. TODO: perhaps add a --delete or --force flag which will - // automagically delete the deployment in the "old" namespace. + // If creating a duplicate deployed function in a different + // namespace. if targetNamespace != currentNamespace && currentNamespace != "" { - fmt.Fprintf(out, "Warning: function is in namespace '%s', but requested namespace is '%s'. Continuing with deployment to '%v'.\n", currentNamespace, targetNamespace, targetNamespace) + fmt.Fprintf(out, "Info: chosen namespace has changed from '%s' to '%s'. Undeploying function from '%s' and deploying new in '%s'.\n", currentNamespace, targetNamespace, currentNamespace, targetNamespace) } // Namespace Changing @@ -727,9 +712,9 @@ func printDeployMessages(out io.Writer, cfg deployConfig) { // If the target namespace is provided but differs from active, warn because // the function won't be visible to other commands such as kubectl unless // context namespace is switched. - activeNamespace, err := k8s.GetNamespace("") + activeNamespace, err := k8s.GetDefaultNamespace() if err == nil && targetNamespace != "" && targetNamespace != activeNamespace { - fmt.Fprintf(out, "Warning: namespace chosen is '%s', but currently active namespace is '%s'. Continuing with deployment to '%s'.\n", cfg.Namespace, activeNamespace, cfg.Namespace) + fmt.Fprintf(out, "Warning: namespace chosen is '%s', but currently active namespace is '%s'. Continuing with deployment to '%s'.\n", targetNamespace, activeNamespace, targetNamespace) } // Git Args @@ -749,8 +734,35 @@ func printDeployMessages(out io.Writer, cfg deployConfig) { // function source does include a reference to a git repository, but that it // will be ignored in favor of the local source code since --remote was not // specified. - if !cfg.Remote && (cfg.GitURL != "" || cfg.GitBranch != "" || cfg.GitDir != "") { + + // TODO update names of these to Source--Revision--Dir + 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.") } +} +// isDigested returns true if provided image string 'v' has digest and false if not. +// Includes basic validation that a provided digest is correctly formatted. +func isDigested(v string) (validDigest bool, err error) { + var digest string + vv := strings.Split(v, "@") + if len(vv) < 2 { + return // has no digest + } else if len(vv) > 2 { + err = fmt.Errorf("image '%v' contains an invalid digest (extra '@')", v) + return + } + digest = vv[1] + + if !strings.HasPrefix(digest, "sha256:") { + err = fmt.Errorf("image digest '%s' requires 'sha256:' prefix", digest) + return + } + + if len(digest[7:]) != 64 { + err = fmt.Errorf("image digest '%v' has an invalid sha256 hash length of %v when it should be 64", digest, len(digest[7:])) + } + + validDigest = true + return } diff --git a/cmd/deploy_test.go b/cmd/deploy_test.go index 1b6ae789ed..296a2b4cf0 100644 --- a/cmd/deploy_test.go +++ b/cmd/deploy_test.go @@ -13,9 +13,12 @@ import ( "github.com/ory/viper" "github.com/spf13/cobra" + apiErrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime/schema" "knative.dev/func/pkg/builders" "knative.dev/func/pkg/config" fn "knative.dev/func/pkg/functions" + "knative.dev/func/pkg/k8s" "knative.dev/func/pkg/mock" ) @@ -192,11 +195,12 @@ func testConfigApplied(cmdFn commandConstructor, t *testing.T) { if err := cmdFn(clientFn).Execute(); err != nil { t.Fatal(err) } + if f, err = fn.NewFunction(root); err != nil { t.Fatal(err) } - if f.Image != "registry.example.com/charlie/f:latest" { - t.Fatalf("expected image 'registry.example.com/charlie/f:latest' got '%v'", f.Image) + if f.Build.Image != "registry.example.com/charlie/f:latest" { + t.Fatalf("expected image 'registry.example.com/charlie/f:latest' got '%v'", f.Build.Image) } // Ensure environment variables loaded: Push @@ -569,7 +573,7 @@ func TestDeploy_GitArgsUsed(t *testing.T) { // A Pipelines Provider which will validate the expected values were received pipeliner := mock.NewPipelinesProvider() - pipeliner.RunFn = func(f fn.Function) (string, error) { + pipeliner.RunFn = func(f fn.Function) (string, string, error) { if f.Build.Git.URL != url { t.Errorf("Pipeline Provider expected git URL '%v' got '%v'", url, f.Build.Git.URL) } @@ -579,7 +583,7 @@ func TestDeploy_GitArgsUsed(t *testing.T) { if f.Build.Git.ContextDir != dir { t.Errorf("Pipeline Provider expected git dir '%v' got '%v'", url, f.Build.Git.ContextDir) } - return url, nil + return url, "", nil } // Deploy the Function specifying all of the git-related flags and --remote @@ -759,6 +763,7 @@ func TestDeploy_ImageWithDigestErrors(t *testing.T) { { name: "correctly formatted full image with digest yields no error (degen case)", image: "example.com/myNamespace/myFunction:latest@sha256:7d66645b0add6de7af77ef332ecd4728649a2f03b9a2716422a054805b595c4e", + build: "false", }, { name: "--build forced on yields error", @@ -832,6 +837,37 @@ func TestDeploy_ImageWithDigestErrors(t *testing.T) { } } +// TestDeploy_ImageWithDigestDoesntPopulateBuild ensures that when --image is +// given with digest f.Build.Image is not populated because no image building +// should happen; f.Deploy.Image should be populated because the image should +// just be deployed as is (since it already has digest) +func TestDeploy_ImageWithDigestDoesntPopulateBuild(t *testing.T) { + root := fromTempDirectory(t) + // image with digest (well almost, atleast in length and syntax) + const img = "docker.io/4141gauron3268@sha256:XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX" + // Create a new Function in the temp directory + _, err := fn.New().Init(fn.Function{Runtime: "go", Root: root}) + if err != nil { + t.Fatal(err) + } + + cmd := NewDeployCmd(NewTestClient( + fn.WithDeployer(mock.NewDeployer()), + fn.WithRegistry(TestRegistry))) + + cmd.SetArgs([]string{"--build=false", "--push=false", "--image", img}) + if err = cmd.Execute(); err != nil { + t.Fatal(err) + } + f, _ := fn.NewFunction(root) + if f.Build.Image != "" { + t.Fatal("build image should be empty when deploying with digested image") + } + if f.Deploy.Image != img { + t.Fatal("expected deployed image to match digested img given") + } +} + // TestDepoy_InvalidRegistry ensures that providing an invalid registry // fails with the expected error. func TestDeploy_InvalidRegistry(t *testing.T) { @@ -1072,7 +1108,7 @@ func TestDeploy_NamespaceUpdateWarning(t *testing.T) { )) cmd.SetArgs([]string{"--namespace=newns"}) out := strings.Builder{} - fmt.Fprintln(&out, "Test errpr") + fmt.Fprintln(&out, "Test error") cmd.SetOut(&out) cmd.SetErr(&out) @@ -1082,12 +1118,17 @@ func TestDeploy_NamespaceUpdateWarning(t *testing.T) { time.Sleep(1 * time.Second) - expected := "Warning: function is in namespace 'myns', but requested namespace is 'newns'. Continuing with deployment to 'newns'." + activeNamespace, err := k8s.GetDefaultNamespace() + if err != nil { + t.Fatalf("Couldnt get active namespace, got error: %v", err) + } - // Ensure output contained warning if changing namespace - if !strings.Contains(out.String(), expected) { + expected1 := "Info: chosen namespace has changed from 'myns' to 'newns'. Undeploying function from 'myns' and deploying new in 'newns'." + expected2 := fmt.Sprintf("Warning: namespace chosen is 'newns', but currently active namespace is '%s'. Continuing with deployment to 'newns'.", activeNamespace) + // Ensure output contained info and warning if changing namespace + if !strings.Contains(out.String(), expected1) || !strings.Contains(out.String(), expected2) { t.Log("STDERR:\n" + out.String()) - t.Fatalf("Expected warning not found:\n%v", expected) + t.Fatalf("Expected Info and/or Warning not found:\n%v|%v", expected1, expected2) } // Ensure the function was saved as having been deployed to @@ -1096,7 +1137,100 @@ func TestDeploy_NamespaceUpdateWarning(t *testing.T) { t.Fatal(err) } if f.Deploy.Namespace != "newns" { - t.Fatalf("expected function to be deoployed into namespace 'newns'. got '%v'", f.Deploy.Namespace) + t.Fatalf("expected function to be deployed into namespace 'newns'. got '%v'", f.Deploy.Namespace) + } +} + +// TestDeploy_BasicRedeploy simply ensures that redeploy works and doesnt brake +// using standard deploy method when desired namespace is deleted. +func TestDeploy_BasicRedeployInCorrectNamespace(t *testing.T) { + root := fromTempDirectory(t) + + // Create a Function which appears to have been deployed to 'myns' + f := fn.Function{ + Runtime: "go", + Root: root, + } + f, err := fn.New().Init(f) + if err != nil { + t.Fatal(err) + } + + // Redeploy the function, specifying 'newns' + cmd := NewDeployCmd(NewTestClient( + fn.WithDeployer(mock.NewDeployer()), + fn.WithRegistry(TestRegistry), + )) + + cmd.SetArgs([]string{"--namespace=mydns"}) + if err := cmd.Execute(); err != nil { + t.Fatal(err) + } + + f, _ = fn.NewFunction(root) + if f.Deploy.Namespace == "" { + t.Fatal("expected deployed namespace to be specified after deploy") + } + + // get rid of desired namespace -- should still deploy as usual, now taking + // the "already deployed" namespace + cmd.SetArgs([]string{"--namespace="}) + if err = cmd.Execute(); err != nil { + t.Fatal(err) + } + + f, _ = fn.NewFunction(root) + if f.Namespace != "" { + t.Fatalf("no desired namespace should be specified but is %s", f.Namespace) + } + + if f.Deploy.Namespace == "" { + t.Fatal("expected deployed namespace to be specified after second deploy") + } +} + +// TestDeploy_BasicRedeployPipelines simply ensures that deploy 2 times works +// and doesnt brake using pipelines +func TestDeploy_BasicRedeployPipelinesCorrectNamespace(t *testing.T) { + root := fromTempDirectory(t) + // Create a Function which appears to have been deployed to 'myns' + f := fn.Function{ + Runtime: "go", + Root: root, + } + f, err := fn.New().Init(f) + if err != nil { + t.Fatal(err) + } + + // Redeploy the function, specifying 'newns' + cmd := NewDeployCmd(NewTestClient( + fn.WithBuilder(mock.NewBuilder()), + fn.WithPipelinesProvider(mock.NewPipelinesProvider()), + fn.WithRegistry(TestRegistry), + )) + + cmd.SetArgs([]string{"--remote", "--namespace=myfuncns"}) + if err = cmd.Execute(); err != nil { + t.Fatal(err) + } + + f, _ = fn.NewFunction(root) + if f.Deploy.Namespace == "" || f.Deploy.Namespace != f.Namespace { + t.Fatalf("namespace should match desired ns when deployed - '%s' | '%s'", f.Deploy.Namespace, f.Namespace) + } + + cmd.SetArgs([]string{"--remote", "--namespace="}) + if err = cmd.Execute(); err != nil { + t.Fatal(err) + } + + f, _ = fn.NewFunction(root) + if f.Namespace != "" { + t.Fatal("desired ns should be empty") + } + if f.Deploy.Namespace != "myfuncns" { + t.Fatalf("deployed ns should NOT have changed but is '%s'\n", f.Deploy.Namespace) } } @@ -1124,7 +1258,9 @@ func testRegistry(cmdFn commandConstructor, t *testing.T) { name: "registry member mismatch", f: fn.Function{ Registry: "registry.example.com/alice", - Image: "registry.example.com/bob/f:latest", + Build: fn.BuildSpec{ + Image: "registry.example.com/bob/f:latest", + }, }, args: []string{}, expectedRegistry: "registry.example.com/alice", @@ -1137,7 +1273,9 @@ func testRegistry(cmdFn commandConstructor, t *testing.T) { name: "registry flag updates", f: fn.Function{ Registry: "registry.example.com/alice", - Image: "registry.example.com/bob/f:latest", + Build: fn.BuildSpec{ + Image: "registry.example.com/bob/f:latest", + }, }, args: []string{"--registry=registry.example.com/charlie"}, expectedRegistry: "registry.example.com/charlie", @@ -1150,7 +1288,9 @@ func testRegistry(cmdFn commandConstructor, t *testing.T) { name: "image flag overrides", f: fn.Function{ Registry: "registry.example.com/alice", - Image: "registry.example.com/bob/f:latest", + Build: fn.BuildSpec{ + Image: "registry.example.com/bob/f:latest", + }, }, args: []string{"--image=registry.example.com/charlie/f:latest"}, expectedRegistry: "registry.example.com/alice", // not updated @@ -1178,8 +1318,8 @@ func testRegistry(cmdFn commandConstructor, t *testing.T) { if f.Registry != test.expectedRegistry { t.Fatalf("expected registry '%v', got '%v'", test.expectedRegistry, f.Registry) } - if f.Image != test.expectedImage { - t.Fatalf("expected image '%v', got '%v'", test.expectedImage, f.Image) + if f.Build.Image != test.expectedImage { + t.Fatalf("expected image '%v', got '%v'", test.expectedImage, f.Build.Image) } }) } @@ -1220,8 +1360,8 @@ func testRegistryLoads(cmdFn commandConstructor, t *testing.T) { } expected := "example.com/alice/my-func:latest" - if f.Image != expected { - t.Fatalf("expected image name '%v'. got %v", expected, f.Image) + if f.Build.Image != expected { + t.Fatalf("expected image name '%v'. got %v", expected, f.Build.Image) } } @@ -1533,3 +1673,156 @@ func Test_ValidateBuilder(t *testing.T) { t.Fatalf("did not get expected error validating an invalid builder name") } } + +// TestReDeploy_OnRegistryChange tests that after deployed image with registry X, +// subsequent deploy with registry Y triggers build +func TestReDeploy_OnRegistryChange(t *testing.T) { + root := fromTempDirectory(t) + + // Create a basic go Function + f := fn.Function{ + Runtime: "go", + Root: root, + } + _, err := fn.New().Init(f) + if err != nil { + t.Fatal(err) + } + + // create build cmd + cmdBuild := NewBuildCmd(NewTestClient(fn.WithBuilder(mock.NewBuilder()))) + cmdBuild.SetArgs([]string{"--registry=" + TestRegistry}) + + // First: prebuild Function + if err := cmdBuild.Execute(); err != nil { + t.Fatal(err) + } + + // change registry and deploy again + newRegistry := "example.com/fred" + + cmd := NewDeployCmd(NewTestClient( + fn.WithDeployer(mock.NewDeployer()), + )) + + cmd.SetArgs([]string{"--registry=" + newRegistry}) + + // Second: Deploy with different registry and expect new build + if err := cmd.Execute(); err != nil { + t.Fatal(err) + } + + // ASSERT + expectF, err := fn.NewFunction(root) + if err != nil { + t.Fatal("couldnt load function from path") + } + + if !strings.Contains(expectF.Build.Image, newRegistry) { + t.Fatalf("expected built image '%s' to contain new registry '%s'\n", expectF.Build.Image, newRegistry) + } +} + +// TestReDeploy_OnRegistryChangeWithBuildFalse should fail with function not +// being built because the registry has changed +func TestReDeploy_OnRegistryChangeWithBuildFalse(t *testing.T) { + root := fromTempDirectory(t) + + // Create a basic go Function + f := fn.Function{ + Runtime: "go", + Root: root, + } + _, err := fn.New().Init(f) + if err != nil { + t.Fatal(err) + } + + // create build cmd + cmdBuild := NewBuildCmd(NewTestClient(fn.WithBuilder(mock.NewBuilder()))) + cmdBuild.SetArgs([]string{"--registry=" + TestRegistry}) + + // First: prebuild Function + if err := cmdBuild.Execute(); err != nil { + t.Fatal(err) + } + + // change registry and deploy again + newRegistry := "example.com/fred" + + cmd := NewDeployCmd(NewTestClient( + fn.WithDeployer(mock.NewDeployer()), + )) + + cmd.SetArgs([]string{"--registry=" + newRegistry, "--build=false"}) + + // Second: Deploy with different registry and expect 'not built' error because + // registry has changed but build is disabled + if err := cmd.Execute(); err == nil { + t.Fatal(err) + } + + // ASSERT + expectF, err := fn.NewFunction(root) + if err != nil { + t.Fatal("couldnt load function from path") + } + + if !strings.Contains(expectF.Build.Image, TestRegistry) { + t.Fatal("expected registry to NOT change since --build=false") + } +} + +// TestDeploy_NoErrorOnOldFunctionNotFound assures that no error is given when old Function's +// service is not available (is already deleted manually or the namespace doesnt exist etc.) +func TestDeploy_NoErrorOnOldFunctionNotFound(t *testing.T) { + var ( + root = fromTempDirectory(t) + nsOne = "nsone" + nsTwo = "nstwo" + remover = mock.NewRemover() + ) + + // Simulate remover error + remover.RemoveFn = func(n, ns string) error { + return apiErrors.NewNotFound(schema.GroupResource{Group: "", Resource: "Namespace"}, nsOne) + } + + // Create a basic go Function + f := fn.Function{ + Runtime: "go", + Root: root, + } + _, err := fn.New().Init(f) + if err != nil { + t.Fatal(err) + } + + // Deploy the function to ns "nsone" + cmd := NewDeployCmd(NewTestClient( + fn.WithDeployer(mock.NewDeployer()), + fn.WithRegistry(TestRegistry), + fn.WithRemover(remover))) + + cmd.SetArgs([]string{fmt.Sprintf("--namespace=%s", nsOne)}) + err = cmd.Execute() + if err != nil { + t.Fatal(err) + } + + // Second Deploy with different namespace + cmd.SetArgs([]string{fmt.Sprintf("--namespace=%s", nsTwo)}) + + err = cmd.Execute() + + // possible TODO: catch the os.Stderr output and check that this is printed out + // and if this is implemented, probably change the name to *_WarnOnFunction + // expectedWarning := fmt.Sprintf("Warning: Cant undeploy Function in namespace '%s' - service not found. Namespace/Service might be deleted already", nsOne) + + // ASSERT + + // Needs to pass since the error is set to nil for NotFound error + if err != nil { + t.Fatal(err) + } +} diff --git a/cmd/root.go b/cmd/root.go index dcfc3ded45..b9353a2ca4 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -214,46 +214,6 @@ func deriveNameAndAbsolutePathFromPath(path string) (string, string) { return pathParts[len(pathParts)-1], absPath } -// deriveImage returns the same image name which will be used. -// I.e. if the explicit name is empty, derive one from the configured registry -// (registry plus username) and the function's name. -// -// This is calculated preemptively here in the CLI (prior to invoking the -// client), only in order to provide information to the user via the prompt. -// The client will calculate this same value if the image override is not -// provided. -// -// Derivation logic: -// deriveImage attempts to arrive at a final, full image name: -// -// format: [registry]/[username]/[functionName]:[tag] -// example: quay.io/myname/my.function.name:tag. -// -// Registry can optionally be omitted, in which case DefaultRegistry -// will be prepended. -// -// If the image flag is provided, this value is used directly (the user supplied -// --image or $FUNC_IMAGE). Otherwise, the function at 'path' is loaded, and -// the Image name therein is used (i.e. it was previously calculated). -// Finally, the default registry is used, which is prepended to the function -// name, and appended with ':latest': -func deriveImage(explicitImage, defaultRegistry, path string) string { - if explicitImage != "" { - return explicitImage // use the explicit value provided. - } - f, err := fn.NewFunction(path) - if err != nil { - return "" // unable to derive due to load error (uninitialized?) - } - if f.Image != "" { - return f.Image // use value previously provided or derived. - } - // Use the func system's derivation logic. - // Errors deriving result in an empty return - derivedValue, _ := f.ImageName() - return derivedValue -} - func mergeEnvs(envs []fn.Env, envToUpdate *util.OrderedMap, envToRemove []string) ([]fn.Env, int, error) { updated := sets.NewString() diff --git a/cmd/root_test.go b/cmd/root_test.go index 5baf9b8ae0..138501711c 100644 --- a/cmd/root_test.go +++ b/cmd/root_test.go @@ -327,6 +327,8 @@ func fromTempDirectory(t *testing.T) string { // By default unit tests presum no config exists unless provided in testdata. t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + t.Setenv("KUBERNETES_SERVICE_HOST", "") + // creates and CDs to a temp directory d, done := Mktemp(t) diff --git a/cmd/run.go b/cmd/run.go index 19190d7129..f2147c4840 100644 --- a/cmd/run.go +++ b/cmd/run.go @@ -6,7 +6,6 @@ import ( "fmt" "os" "strconv" - "strings" "time" "github.com/ory/viper" @@ -158,20 +157,6 @@ func runRun(cmd *cobra.Command, args []string, newClient ClientFactory) (err err return } - // TODO: this is duplicate logic with runBuild and runRun. - // Refactor both to have this logic part of creating the buildConfig and thus - // shared because newRunConfig uses newBuildConfig for its embedded struct. - if f.Registry != "" && !cmd.Flags().Changed("image") && strings.Index(f.Image, "/") > 0 && !strings.HasPrefix(f.Image, f.Registry) { - prfx := f.Registry - if prfx[len(prfx)-1:] != "/" { - prfx = prfx + "/" - } - sps := strings.Split(f.Image, "/") - updImg := prfx + sps[len(sps)-1] - fmt.Fprintf(cmd.ErrOrStderr(), "Warning: function has current image '%s' which has a different registry than the currently configured registry '%s'. The new image tag will be '%s'. To use an explicit image, use --image.\n", f.Image, f.Registry, updImg) - f.Image = updImg - } - // Client clientOptions, err := cfg.clientOptions() if err != nil { diff --git a/pkg/builders/buildpacks/builder.go b/pkg/builders/buildpacks/builder.go index 73b1ae8991..6d3bc625a0 100644 --- a/pkg/builders/buildpacks/builder.go +++ b/pkg/builders/buildpacks/builder.go @@ -153,7 +153,7 @@ func (b *Builder) Build(ctx context.Context, f fn.Function, platforms []fn.Platf // Pack build options opts := pack.BuildOptions{ AppPath: f.Root, - Image: f.Image, + Image: f.Build.Image, LifecycleImage: DefaultLifecycleImage, Builder: image, Buildpacks: buildpacks, diff --git a/pkg/builders/s2i/builder.go b/pkg/builders/s2i/builder.go index cf2e7bcc88..cbc19f4b82 100644 --- a/pkg/builders/s2i/builder.go +++ b/pkg/builders/s2i/builder.go @@ -139,7 +139,7 @@ func (b *Builder) Build(ctx context.Context, f fn.Function, platforms []fn.Platf // Build Config cfg := &api.Config{} cfg.Quiet = !b.verbose - cfg.Tag = f.Image + cfg.Tag = f.Build.Image cfg.Source = &git.URL{URL: url.URL{Path: f.Root}, Type: git.URLTypeLocal} cfg.BuilderImage = builderImage cfg.BuilderPullPolicy = api.DefaultBuilderPullPolicy @@ -319,7 +319,7 @@ func (b *Builder) Build(ctx context.Context, f fn.Function, platforms []fn.Platf }() opts := types.ImageBuildOptions{ - Tags: []string{f.Image}, + Tags: []string{f.Build.Image}, PullParent: true, } diff --git a/pkg/config/config.go b/pkg/config/config.go index a140598ee7..4c8426b9c9 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -31,14 +31,12 @@ const ( // DefaultNamespace for remote operations is the currently active // context namespace (if available) or the fallback "default". -// Namespace, when left blank on the function itself, indicates this -// value should be used when deploying for the first time. Subsequelty -// the value will be populated, indicating the namespace in which the +// Subsequently the value will be populated, indicating the namespace in which the // function is currently deployed. Changes to this value will issue warnings // to the user. func DefaultNamespace() (namespace string) { var err error - if namespace, err = k8s.GetNamespace(""); err != nil { + if namespace, err = k8s.GetDefaultNamespace(); err != nil { return "default" } return @@ -148,7 +146,7 @@ func (c Global) Apply(f fn.Function) Global { return c } -// Configure a function with poipulated values of the config. +// Configure a function with populated values of the config. // The resulting function is the function overridden by values on config. func (c Global) Configure(f fn.Function) fn.Function { if c.Builder != "" { diff --git a/pkg/docker/pusher.go b/pkg/docker/pusher.go index ff1151ee93..67c0f20129 100644 --- a/pkg/docker/pusher.go +++ b/pkg/docker/pusher.go @@ -118,16 +118,16 @@ func (n *Pusher) Push(ctx context.Context, f fn.Function) (digest string, err er output = io.Discard } - if f.Image == "" { + if f.Build.Image == "" { return "", errors.New("Function has no associated image. Has it been built?") } - registry, err := GetRegistry(f.Image) + registry, err := GetRegistry(f.Build.Image) if err != nil { return "", err } - credentials, err := n.credentialsProvider(ctx, f.Image) + credentials, err := n.credentialsProvider(ctx, f.Build.Image) if err != nil { return "", fmt.Errorf("failed to get credentials: %w", err) } @@ -161,7 +161,7 @@ func (n *Pusher) daemonPush(ctx context.Context, f fn.Function, credentials Cred opts := types.ImagePushOptions{RegistryAuth: base64.StdEncoding.EncodeToString(b)} - r, err := cli.ImagePush(ctx, f.Image, opts) + r, err := cli.ImagePush(ctx, f.Build.Image, opts) if err != nil { return "", fmt.Errorf("failed to push the image: %w", err) } @@ -204,7 +204,7 @@ func (n *Pusher) push(ctx context.Context, f fn.Function, credentials Credential Password: credentials.Password, } - ref, err := name.ParseReference(f.Image) + ref, err := name.ParseReference(f.Build.Image) if err != nil { return "", err } diff --git a/pkg/docker/pusher_test.go b/pkg/docker/pusher_test.go index b785850af4..ac10af2578 100644 --- a/pkg/docker/pusher_test.go +++ b/pkg/docker/pusher_test.go @@ -113,7 +113,9 @@ func TestDaemonPush(t *testing.T) { ) f := fn.Function{ - Image: functionImageLocal, + Build: fn.BuildSpec{ + Image: functionImageLocal, + }, } digest, err := pusher.Push(ctx, f) @@ -196,7 +198,9 @@ func TestNonDaemonPush(t *testing.T) { ) f := fn.Function{ - Image: functionImageRemote, + Build: fn.BuildSpec{ + Image: functionImageRemote, + }, } actualDigest, err := pusher.Push(ctx, f) @@ -204,7 +208,7 @@ func TestNonDaemonPush(t *testing.T) { t.Fatal(err) } - if !reflect.DeepEqual(imagesPassedToMock, []string{f.Image}) { + if !reflect.DeepEqual(imagesPassedToMock, []string{f.Build.Image}) { t.Errorf("Bad image name passed to the Docker API Client: %q.", imagesPassedToMock) } diff --git a/pkg/docker/runner.go b/pkg/docker/runner.go index 8791b315b5..4b07464136 100644 --- a/pkg/docker/runner.go +++ b/pkg/docker/runner.go @@ -67,7 +67,7 @@ func (n *Runner) Run(ctx context.Context, f fn.Function, startTimeout time.Durat runtimeErrCh = make(chan error, 10) ) - if f.Image == "" { + if f.Build.Image == "" { return job, errors.New("Function has no associated image. Has it been built?") } if c, _, err = NewClient(client.DefaultDockerHost); err != nil { @@ -200,7 +200,7 @@ func newContainerConfig(f fn.Function, _ string, verbose bool) (c container.Conf // httpPort := nat.Port(fmt.Sprintf("%v/tcp", port)) httpPort := nat.Port("8080/tcp") c = container.Config{ - Image: f.Image, + Image: f.Build.Image, Tty: false, AttachStderr: true, AttachStdout: true, diff --git a/pkg/docker/runner_int_test.go b/pkg/docker/runner_int_test.go index e2ea69aed4..f159d21890 100644 --- a/pkg/docker/runner_int_test.go +++ b/pkg/docker/runner_int_test.go @@ -39,11 +39,15 @@ func TestRun(t *testing.T) { // Initialize a new function (creates all artifacts on disk necessary // to perform actions such as running) f := fn.Function{Runtime: "go", Root: root, Image: displayEventImg} - f, err := fn.New().Init(f) + + client := fn.New() + f, err := client.Init(f) if err != nil { t.Fatal(err) } + f, err = client.Build(ctx, f) + // Run the function using a docker runner var out, errOut bytes.Buffer runner := docker.NewRunner(true, &out, &errOut) diff --git a/pkg/functions/client.go b/pkg/functions/client.go index 090c125f40..b4fc18c34a 100644 --- a/pkg/functions/client.go +++ b/pkg/functions/client.go @@ -127,7 +127,7 @@ type Runner interface { // Remover of deployed services. type Remover interface { // Remove the function from remote. - Remove(ctx context.Context, name string) error + Remove(ctx context.Context, name string, namespace string) error } // Lister of deployed functions. @@ -186,7 +186,7 @@ type DNSProvider interface { // PipelinesProvider manages lifecyle of CI/CD pipelines used by a function type PipelinesProvider interface { - Run(context.Context, Function) (string, error) + Run(context.Context, Function) (string, string, error) Remove(context.Context, Function) error ConfigurePAC(context.Context, Function, any) error RemovePAC(context.Context, Function, any) error @@ -447,6 +447,13 @@ func (c *Client) Update(ctx context.Context, f Function) (string, Function, erro if f, err = c.Push(ctx, f); err != nil { return "", f, err } + + // TODO: change this later when push doesnt return built image. + // Assign this as c.Push is going to produce the built image (for now) to + // .Deploy.Image for the deployer -- figure out where to assign .Deploy.Image + // first, might be just moved above push + f.Deploy.Image = f.Build.Image + if f, err = c.Deploy(ctx, f); err != nil { return "", f, err } @@ -489,6 +496,12 @@ func (c *Client) New(ctx context.Context, cfg Function) (string, Function, error return route, f, err } + // TODO: change this later when push doesnt return built image. + // Assign this as c.Push is going to produce the built image (for now) to + // .Deploy.Image for the deployer -- figure out where to assign .Deploy.Image + // first, might be just moved above push + f.Deploy.Image = f.Build.Image + // Deploy the initialized function, returning its publicly // addressible name for possible registration. fmt.Fprintf(os.Stderr, "Deploying function to cluster\n") @@ -503,7 +516,7 @@ func (c *Client) New(ctx context.Context, cfg Function) (string, Function, error return route, f, err } - fmt.Fprint(os.Stderr, "Done") + fmt.Fprint(os.Stderr, "Done\n") return route, f, err } @@ -624,30 +637,35 @@ func (c *Client) Build(ctx context.Context, f Function, options ...BuildOption) f.Registry = c.registry } - // If no image name has been yet defined (not yet built/deployed), calculate. + // If no image name has been specified by user (--image), calculate. // Image name is stored on the function for later use by deploy, etc. - // TODO: write this to .func/build instead, and populate f.Image on deploy - // such that local builds do not dirty the work tree. var err error if f.Image == "" { - if f.Image, err = f.ImageName(); err != nil { + if f.Build.Image, err = f.ImageName(); err != nil { return f, err } + } else { + f.Build.Image = f.Image } if err = c.builder.Build(ctx, f, oo.Platforms); err != nil { return f, err } + // write .func/built-name as running metadata which is not persisted in yaml + if err = f.WriteRuntimeBuiltImage(c.verbose); err != nil { + return f, err + } + if err = f.Stamp(); err != nil { return f, err } // TODO: create a status structure and return it here for optional // use by the cli for user echo (rather than rely on verbose mode here) - message := fmt.Sprintf("🙌 Function built: %v", f.Image) + message := fmt.Sprintf("🙌 Function built: %v", f.Build.Image) if runtime.GOOS == "windows" { - message = fmt.Sprintf("Function built: %v", f.Image) + message = fmt.Sprintf("Function built: %v", f.Build.Image) } fmt.Fprintf(os.Stderr, "%s\n", message) @@ -729,6 +747,11 @@ func (c *Client) Deploy(ctx context.Context, f Function, opts ...DeployOption) ( return f, ErrNameRequired } + // TODO: gauron99 -- ideally namespace would be determined here to keep consistancy + // with the Remover but it either creates a cyclic dependency or deployer.namespace + // is not defined here for it to be complete. Maybe it would be worth to try to + // do it this way. + // Deploy a new or Update the previously-deployed function fmt.Fprintf(os.Stderr, "⬆️ Deploying function to the cluster\n") result, err := c.deployer.Deploy(ctx, f) @@ -737,6 +760,29 @@ func (c *Client) Deploy(ctx context.Context, f Function, opts ...DeployOption) ( return f, err } + // If Redeployment to NEW namespace was successful -- undeploy dangling Function in old namespace. + // On forced namespace change (using --namespace flag) + if f.Namespace != "" && f.Namespace != f.Deploy.Namespace && f.Deploy.Namespace != "" { + if c.verbose { + fmt.Fprintf(os.Stderr, "Info: Deleting old func in '%s' because the namespace has changed to '%s'\n", f.Deploy.Namespace, f.Namespace) + } + + // c.Remove removes a Function in f.Deploy.Namespace which removes the OLD Function + // because its not updated yet (see few lines below) + err = c.Remove(ctx, f, true) + + // Warn when service is not found and set err to nil to continue. Function's + // service mightve been manually deleted prior to the subsequent deploy or the + // namespace is already deleted therefore there is nothing to delete + if ErrFunctionNotFound != err { + fmt.Fprintf(os.Stderr, "Warning: Cant undeploy Function in namespace '%s' - service not found. Namespace/Service might be deleted already\n", f.Deploy.Namespace) + err = nil + } + if err != nil { + return f, err + } + } + // Update the function with the namespace into which the function was // deployed f.Deploy.Namespace = result.Namespace @@ -754,25 +800,28 @@ func (c *Client) Deploy(ctx context.Context, f Function, opts ...DeployOption) ( // Returned function contains applicable registry and deployed image name. func (c *Client) RunPipeline(ctx context.Context, f Function) (Function, error) { var err error - // Default function registry to the client's global registry if f.Registry == "" { f.Registry = c.registry } - // If no image name has been yet defined (not yet built/deployed), calculate. - // Image name is stored on the function for later use by deploy, etc. - if f.Image == "" { - if f.Image, err = f.ImageName(); err != nil { + // If no image name has been specified by user (--image), calculate. + // Image name is stored on the function for later use by deploy. + if f.Image != "" { + // if user specified an image, use it + f.Deploy.Image = f.Image + } else if f.Deploy.Image == "" { + f.Deploy.Image, err = f.ImageName() + if err != nil { return f, err } } // Build and deploy function using Pipeline - if _, err := c.pipelinesProvider.Run(ctx, f); err != nil { + _, f.Deploy.Namespace, err = c.pipelinesProvider.Run(ctx, f) + if err != nil { return f, fmt.Errorf("failed to run pipeline: %w", err) } - return f, nil } @@ -788,8 +837,8 @@ func (c *Client) ConfigurePAC(ctx context.Context, f Function, metadata any) err // If no image name has been yet defined (not yet built/deployed), calculate. // Image name is stored on the function for later use by deploy, etc. - if f.Image == "" { - if f.Image, err = f.ImageName(); err != nil { + if f.Deploy.Image == "" { + if f.Deploy.Image, err = f.ImageName(); err != nil { return err } } @@ -923,12 +972,17 @@ func (c *Client) List(ctx context.Context) ([]ListItem, error) { return c.lister.List(ctx) } -// Remove a function. Name takes precedence. If no name is provided, -// the function defined at root is used if it exists. +// Remove a function. Name takes precedence. If no name is provided, the +// function defined at root is used if it exists. If calling this directly +// namespace must be provided in .Deploy.Namespace field except when using mocks +// in which case empty namespace is accepted because its existence is checked +// in the sub functions remover.Remove and pipilines.Remove func (c *Client) Remove(ctx context.Context, cfg Function, deleteAll bool) error { + functionName := cfg.Name + functionNamespace := cfg.Deploy.Namespace + // If name is provided, it takes precedence. // Otherwise load the function defined at root. - functionName := cfg.Name if cfg.Name == "" { f, err := NewFunction(cfg.Root) if err != nil { @@ -937,23 +991,41 @@ func (c *Client) Remove(ctx context.Context, cfg Function, deleteAll bool) error if !f.Initialized() { return fmt.Errorf("function at %v can not be removed unless initialized. Try removing by name", f.Root) } + // take the functions name and namespace and load it as current function functionName = f.Name + functionNamespace = f.Deploy.Namespace cfg = f } + + // if still empty, get current function's yaml deployed namespace + if functionNamespace == "" { + var f Function + f, err := NewFunction(cfg.Root) + if err != nil { + return err + } + functionNamespace = f.Deploy.Namespace + } + if functionName == "" { return ErrNameRequired } + if functionNamespace == "" { + return ErrNamespaceRequired + } // Delete Knative Service and dependent resources in parallel - fmt.Fprintf(os.Stderr, "Removing Knative Service: %v\n", functionName) + fmt.Fprintf(os.Stderr, "Removing Knative Service: %v in namespace '%v'\n", functionName, functionNamespace) errChan := make(chan error) go func() { - errChan <- c.remover.Remove(ctx, functionName) + errChan <- c.remover.Remove(ctx, functionName, functionNamespace) }() var errResources error if deleteAll { fmt.Fprintf(os.Stderr, "Removing Knative Service '%v' and all dependent resources\n", functionName) + // TODO: might not be necessary + cfg.Deploy.Namespace = functionNamespace errResources = c.pipelinesProvider.Remove(ctx, cfg) } @@ -999,11 +1071,28 @@ func (c *Client) Push(ctx context.Context, f Function) (Function, error) { return f, ErrNotBuilt } var err error - if f.ImageDigest, err = c.pusher.Push(ctx, f); err != nil { + + if err != nil { + if os.IsNotExist(err) { + err = fmt.Errorf("error on push, function has not been built yet") + return f, err + } return f, err } - return f, nil + imageDigest, err := c.pusher.Push(ctx, f) + if err != nil { + return f, err + } + + // TODO: gauron99 - this is here because of a temporary workaround. + // f.Build.Image should contain full image name including the sha256 and + // should be populated earlier BUT because the sha256 is got only on push (here) + // its populated here. This will eventually be moved to build stage where we get + // the full image name and its digest right after building + f.Build.Image = f.ImageNameWithDigest(imageDigest) + + return f, err } // ensureRunDataDir creates a .func directory at the given path, and @@ -1241,14 +1330,14 @@ func (n *noopPusher) Push(ctx context.Context, f Function) (string, error) { ret // Deployer type noopDeployer struct{ output io.Writer } -func (n *noopDeployer) Deploy(ctx context.Context, _ Function) (DeploymentResult, error) { - return DeploymentResult{}, nil +func (n *noopDeployer) Deploy(ctx context.Context, f Function) (DeploymentResult, error) { + return DeploymentResult{Namespace: f.Namespace}, nil } // Remover type noopRemover struct{ output io.Writer } -func (n *noopRemover) Remove(context.Context, string) error { return nil } +func (n *noopRemover) Remove(context.Context, string, string) error { return nil } // Lister type noopLister struct{ output io.Writer } @@ -1265,8 +1354,8 @@ func (n *noopDescriber) Describe(context.Context, string) (Instance, error) { // PipelinesProvider type noopPipelinesProvider struct{} -func (n *noopPipelinesProvider) Run(ctx context.Context, _ Function) (string, error) { - return "", nil +func (n *noopPipelinesProvider) Run(ctx context.Context, _ Function) (string, string, error) { + return "", "", nil } func (n *noopPipelinesProvider) Remove(ctx context.Context, _ Function) error { return nil } func (n *noopPipelinesProvider) ConfigurePAC(ctx context.Context, _ Function, _ any) error { diff --git a/pkg/functions/client_int_test.go b/pkg/functions/client_int_test.go index 6c7eecb7eb..4ce5c2a4fa 100644 --- a/pkg/functions/client_int_test.go +++ b/pkg/functions/client_int_test.go @@ -108,7 +108,7 @@ func TestNew(t *testing.T) { } } -// TestDeploy updates +// TestDeploy deployes using client methods from New but manually func TestDeploy(t *testing.T) { defer Within(t, "testdata/example.com/deploy")() verbose := true @@ -116,10 +116,21 @@ func TestDeploy(t *testing.T) { client := newClient(verbose) f := fn.Function{Name: "deploy", Root: ".", Runtime: "go"} var err error - if _, f, err = client.New(context.Background(), f); err != nil { + + if f, err = client.Init(f); err != nil { + t.Fatal(err) + } + if f, err = client.Build(context.Background(), f); err != nil { t.Fatal(err) } + if f, err = client.Push(context.Background(), f); err != nil { + t.Fatal(err) + } + defer del(t, client, "deploy") + // TODO: gauron99 -- remove this when you set full image name after build instead + // of push -- this has to be here because of a workaround + f.Deploy.Image = f.Build.Image if f, err = client.Deploy(context.Background(), f); err != nil { t.Fatal(err) @@ -130,9 +141,9 @@ func TestDeploy(t *testing.T) { func TestDeployWithOptions(t *testing.T) { root, cleanup := Mktemp(t) defer cleanup() - verbose := true + verbose := false - f := fn.Function{Runtime: "go", Name: "test-deploy-with-options", Root: root} + f := fn.Function{Runtime: "go", Name: "test-deploy-with-options", Root: root, Namespace: DefaultNamespace} f.Deploy = fn.DeploySpec{ Options: fn.Options{ Scale: &fn.ScaleOptions{ @@ -191,7 +202,7 @@ func TestDeployWithTriggers(t *testing.T) { func TestUpdateWithAnnotationsAndLabels(t *testing.T) { functionName := "updateannlab" defer Within(t, "testdata/example.com/"+functionName)() - verbose := true + verbose := false servingClient, err := knative.NewServingClient(DefaultNamespace) @@ -204,10 +215,6 @@ func TestUpdateWithAnnotationsAndLabels(t *testing.T) { } defer del(t, client, functionName) - if f, err = client.Deploy(context.Background(), f); err != nil { - t.Fatal(err) - } - // Updated function with a new set of annotations and labels // deploy and check that deployed kcsv contains correct annotations and labels @@ -547,7 +554,7 @@ func newClient(verbose bool) *fn.Client { pusher := docker.NewPusher(docker.WithVerbose(verbose)) deployer := knative.NewDeployer(knative.WithDeployerNamespace(DefaultNamespace), knative.WithDeployerVerbose(verbose)) describer := knative.NewDescriber(DefaultNamespace, verbose) - remover := knative.NewRemover(DefaultNamespace, verbose) + remover := knative.NewRemover(verbose) lister := knative.NewLister(DefaultNamespace, verbose) return fn.New( @@ -575,7 +582,7 @@ func newClient(verbose bool) *fn.Client { func del(t *testing.T, c *fn.Client, name string) { t.Helper() waitFor(t, c, name) - if err := c.Remove(context.Background(), fn.Function{Name: name}, false); err != nil { + if err := c.Remove(context.Background(), fn.Function{Name: name, Deploy: fn.DeploySpec{Namespace: DefaultNamespace}}, false); err != nil { t.Fatal(err) } cli, _, err := docker.NewClient(client.DefaultDockerHost) diff --git a/pkg/functions/client_test.go b/pkg/functions/client_test.go index db8c0a65be..31554a4e50 100644 --- a/pkg/functions/client_test.go +++ b/pkg/functions/client_test.go @@ -12,6 +12,7 @@ import ( "net" "net/http" "os" + "path" "path/filepath" "reflect" "runtime" @@ -353,7 +354,7 @@ func TestClient_New_RepositoriesExtensible(t *testing.T) { } // TestRuntime_New_RuntimeNotFoundError generates an error when the provided -// runtime is not fo0und (embedded default repository). +// runtime is not found (embedded default repository). func TestClient_New_RuntimeNotFoundError(t *testing.T) { root := "testdata/example.com/testRuntimeNotFound" defer Using(t, root)() @@ -498,8 +499,8 @@ func TestClient_New_ImageNamePopulated(t *testing.T) { if err != nil { t.Fatal(err) } - if f.Image != imageTag { - t.Fatalf("expected image '%v' got '%v'", imageTag, f.Image) + if f.Build.Image != imageTag { + t.Fatalf("expected image '%v' got '%v'", imageTag, f.Build.Image) } } @@ -523,8 +524,8 @@ func TestClient_New_ImageRegistryDefaults(t *testing.T) { // Expected image is [DefaultRegistry]/[namespace]/[servicename]:latest expected := fn.DefaultRegistry + "/alice/" + f.Name + ":latest" - if f.Image != expected { - t.Fatalf("expected image '%v' got '%v'", expected, f.Image) + if f.Build.Image != expected { + t.Fatalf("expected image '%v' got '%v'", expected, f.Build.Image) } } @@ -565,8 +566,8 @@ func TestClient_New_Delegation(t *testing.T) { } pusher.PushFn = func(f fn.Function) (string, error) { - if f.Image != expectedImage { - t.Fatalf("pusher expected image '%v', got '%v'", expectedImage, f.Image) + if f.Build.Image != expectedImage { + t.Fatalf("pusher expected image '%v', got '%v'", expectedImage, f.Build.Image) } return "", nil } @@ -575,8 +576,8 @@ func TestClient_New_Delegation(t *testing.T) { if f.Name != expectedName { t.Fatalf("deployer expected name '%v', got '%v'", expectedName, f.Name) } - if f.Image != expectedImage { - t.Fatalf("deployer expected image '%v', got '%v'", expectedImage, f.Image) + if f.Build.Image != expectedImage { + t.Fatalf("deployer expected image '%v', got '%v'", expectedImage, f.Build.Image) } return } @@ -843,8 +844,8 @@ func TestClient_Update(t *testing.T) { // Pusher whose implementaiton verifies the expected image pusher.PushFn = func(f fn.Function) (string, error) { - if f.Image != expectedImage { - t.Fatalf("pusher expected image '%v', got '%v'", expectedImage, f.Image) + if f.Build.Image != expectedImage { + t.Fatalf("pusher expected image '%v', got '%v'", expectedImage, f.Build.Image) } // image of given name wouold be pushed to the configured registry. return "", nil @@ -855,8 +856,8 @@ func TestClient_Update(t *testing.T) { if f.Name != expectedName { t.Fatalf("updater expected name '%v', got '%v'", expectedName, f.Name) } - if f.Image != expectedImage { - t.Fatalf("updater expected image '%v', got '%v'", expectedImage, f.Image) + if f.Build.Image != expectedImage { + t.Fatalf("updater expected image '%v', got '%v'", expectedImage, f.Build.Image) } return } @@ -901,8 +902,8 @@ func TestClient_Update(t *testing.T) { } // TestClient_Deploy_RegistryUpdate ensures that deploying a Function updates -// its image member on initial deploy, and on subsequent deploys only -// if reset to it zero value. +// its image member on initial deploy, and on subsequent deploys where f.Image +// takes precedence func TestClient_Deploy_RegistryUpdate(t *testing.T) { root, rm := Mktemp(t) defer rm() @@ -915,8 +916,8 @@ func TestClient_Deploy_RegistryUpdate(t *testing.T) { if _, f, err = client.New(context.Background(), fn.Function{Runtime: "go", Name: "f", Root: root}); err != nil { t.Fatal(err) } - if f.Image != "example.com/alice/f:latest" { - t.Error("image name was not initially set") + if f.Build.Image != "example.com/alice/f:latest" { + t.Error("image was not built") } // Updating the registry and performing a subsequent update should not result @@ -926,27 +927,89 @@ func TestClient_Deploy_RegistryUpdate(t *testing.T) { if f, err = client.Build(context.Background(), f); err != nil { t.Fatal(err) } - if f, err = client.Deploy(context.Background(), f); err != nil { + // if f, err = client.Deploy(context.Background(), f); err != nil { + // t.Fatal(err) + // } + expected := "example.com/bob/f:latest" + if f.Build.Image != expected { // CHANGE to bob since its the first f.Registry + t.Errorf("expected image name to change to '%v', but got '%v'", expected, f.Build.Image) + } + + // Set the value of .Image which should override current image + f.Image = "example.com/fred/f:latest" + if f, err = client.Build(context.Background(), f); err != nil { t.Fatal(err) } - expected := "example.com/alice/f:latest" - if f.Image != expected { // NOT changed to bob - t.Errorf("expected image name to stay '%v' and not be updated, but got '%v'", expected, f.Image) + // if f, err = client.Deploy(context.Background(), f); err != nil { + // t.Fatal(err) + // } + expected = "example.com/fred/f:latest" + if f.Build.Image != expected { // DOES change to bob + t.Errorf("expected image name to change to '%v', but got '%v'", expected, f.Build.Image) } - // Reset the value of .Image to default "" and ensure this triggers recalc. + // Set the value of f.Image to "" to ensure the registry is used for new + // image calculation f.Image = "" - f.Registry = "example.com/bob" + // f.Registry is "example.com/bob" + if f, err = client.Build(context.Background(), f); err != nil { t.Fatal(err) } - if f, err = client.Deploy(context.Background(), f); err != nil { + + expected = "example.com/bob/f:latest" + if f.Build.Image != expected { + t.Errorf("expected image name to change to '%v', but got '%v'", expected, f.Build.Image) + } +} + +// TestClient_Deploy_NamespaceUpdate ensures that namespace deployment has +// the correct priorities, that means: +// 'default' gets overridden by 'already deployed' if aplicable and all gets +// overridden by 'specifically desired namespace'. +func TestClient_Deploy_NamespaceUpdate(t *testing.T) { + root, rm := Mktemp(t) + defer rm() + + var ( + ctx = context.Background() + deployer = mock.NewDeployer() + f fn.Function + err error + ) + + client := fn.New( + fn.WithRegistry("example.com/alice"), + fn.WithDeployer(deployer), + ) + + // New runs build and deploy, thus the initial instantiation should result in + // the namespace member being populated into the most default namespace + if _, f, err = client.New(ctx, fn.Function{Runtime: "go", Name: "f", Root: root}); err != nil { + t.Fatal(err) + } + if f.Deploy.Namespace == "" { + t.Fatal("namespace should be populated in deployer when initially undefined") + } + + // change deployed namespace to simulate already deployed function -- should + // take precedence + f.Deploy.Namespace = "alreadydeployed" + f, err = client.Deploy(ctx, f) + if err != nil { t.Fatal(err) } - expected = "example.com/bob/f:latest" - if f.Image != expected { // DOES change to bob - t.Errorf("expected image name to stay '%v' and not be updated, but got '%v'", expected, f.Image) + if f.Deploy.Namespace != "alreadydeployed" { + err = fmt.Errorf("namespace should match the already deployed function ns") + t.Fatal(err) + } + + // desired namespace takes precedence + f.Namespace = "desiredns" + f, err = client.Deploy(ctx, f) + if err != nil { + t.Fatal(err) } } @@ -957,6 +1020,7 @@ func TestClient_Remove_ByPath(t *testing.T) { root = "testdata/example.com/test-remove-by-path" expectedName = "test-remove-by-path" remover = mock.NewRemover() + namespace = "func" ) defer Using(t, root)() @@ -967,11 +1031,11 @@ func TestClient_Remove_ByPath(t *testing.T) { var f fn.Function var err error - if _, f, err = client.New(context.Background(), fn.Function{Runtime: TestRuntime, Root: root}); err != nil { + if _, f, err = client.New(context.Background(), fn.Function{Runtime: TestRuntime, Root: root, Namespace: namespace}); err != nil { t.Fatal(err) } - remover.RemoveFn = func(name string) error { + remover.RemoveFn = func(name, _ string) error { if name != expectedName { t.Fatalf("Expected to remove '%v', got '%v'", expectedName, name) } @@ -998,6 +1062,7 @@ func TestClient_Remove_DeleteAll(t *testing.T) { remover = mock.NewRemover() pipelinesProvider = mock.NewPipelinesProvider() deleteAll = true + namespace = "func" ) defer Using(t, root)() @@ -1009,11 +1074,11 @@ func TestClient_Remove_DeleteAll(t *testing.T) { var f fn.Function var err error - if _, f, err = client.New(context.Background(), fn.Function{Runtime: TestRuntime, Root: root}); err != nil { + if _, f, err = client.New(context.Background(), fn.Function{Runtime: TestRuntime, Root: root, Namespace: namespace}); err != nil { t.Fatal(err) } - remover.RemoveFn = func(name string) error { + remover.RemoveFn = func(name, _ string) error { if name != expectedName { t.Fatalf("Expected to remove '%v', got '%v'", expectedName, name) } @@ -1044,6 +1109,7 @@ func TestClient_Remove_Dont_DeleteAll(t *testing.T) { remover = mock.NewRemover() pipelinesProvider = mock.NewPipelinesProvider() deleteAll = false + namespace = "func" ) defer Using(t, root)() @@ -1055,11 +1121,11 @@ func TestClient_Remove_Dont_DeleteAll(t *testing.T) { var f fn.Function var err error - if _, f, err = client.New(context.Background(), fn.Function{Runtime: TestRuntime, Root: root}); err != nil { + if _, f, err = client.New(context.Background(), fn.Function{Runtime: TestRuntime, Root: root, Namespace: namespace}); err != nil { t.Fatal(err) } - remover.RemoveFn = func(name string) error { + remover.RemoveFn = func(name, _ string) error { if name != expectedName { t.Fatalf("Expected to remove '%v', got '%v'", expectedName, name) } @@ -1087,6 +1153,7 @@ func TestClient_Remove_ByName(t *testing.T) { root = "testdata/example.com/testRemoveByName" expectedName = "explicitName.example.com" remover = mock.NewRemover() + namespace = "func" ) defer Using(t, root)() @@ -1099,20 +1166,20 @@ func TestClient_Remove_ByName(t *testing.T) { t.Fatal(err) } - remover.RemoveFn = func(name string) error { + remover.RemoveFn = func(name, _ string) error { if name != expectedName { t.Fatalf("Expected to remove '%v', got '%v'", expectedName, name) } return nil } - // Run remove with only a name - if err := client.Remove(context.Background(), fn.Function{Name: expectedName}, false); err != nil { + // Run remove with name (and namespace in .Deploy to simulate deployed function) + if err := client.Remove(context.Background(), fn.Function{Name: expectedName, Deploy: fn.DeploySpec{Namespace: namespace}}, false); err != nil { t.Fatal(err) } // Run remove with a name and a root, which should be ignored in favor of the name. - if err := client.Remove(context.Background(), fn.Function{Name: expectedName, Root: root}, false); err != nil { + if err := client.Remove(context.Background(), fn.Function{Name: expectedName, Root: root, Deploy: fn.DeploySpec{Namespace: namespace}}, false); err != nil { t.Fatal(err) } @@ -1133,7 +1200,7 @@ func TestClient_Remove_UninitializedFails(t *testing.T) { defer Using(t, root)() // remover fails if invoked - remover.RemoveFn = func(name string) error { + remover.RemoveFn = func(name, _ string) error { return fmt.Errorf("remove invoked for unitialized function %v", name) } @@ -1202,8 +1269,8 @@ func TestClient_Deploy_Image(t *testing.T) { } // Upon initial creation, the value of .Image is empty - if f.Image != "" { - t.Fatalf("new function should have no image, got '%v'", f.Image) + if f.Build.Image != "" { + t.Fatalf("new function should have no image, got '%v'", f.Build.Image) } // Upon deployment, the function should be populated; @@ -1214,8 +1281,8 @@ func TestClient_Deploy_Image(t *testing.T) { t.Fatal(err) } expected := "example.com/alice/myfunc:latest" - if f.Image != expected { - t.Fatalf("expected image '%v', got '%v'", expected, f.Image) + if f.Build.Image != expected { + t.Fatalf("expected image '%v', got '%v'", expected, f.Build.Image) } expected = "example.com/alice" if f.Registry != "example.com/alice" { @@ -1234,8 +1301,8 @@ func TestClient_Deploy_Image(t *testing.T) { t.Fatal(err) } expected = "registry2.example.com/bob/myfunc:latest" - if f.Image != expected { - t.Fatalf("expected image '%v', got '%v'", expected, f.Image) + if f.Build.Image != expected { + t.Fatalf("expected image '%v', got '%v'", expected, f.Build.Image) } expected = "example.com/alice" if f.Registry != "example.com/alice" { @@ -1277,18 +1344,19 @@ func TestClient_Pipelines_Deploy_Image(t *testing.T) { t.Fatal(err) } - // Upon initial creation, the value of .Image is empty - if f.Image != "" { - t.Fatalf("new function should have no image, got '%v'", f.Image) + // Upon initial creation, the value of .Build.Image is empty and .Deploy.Image + // is empty because Function is not deployed yet. + if f.Build.Image != "" && f.Deploy.Image != "" { + t.Fatalf("new function should have no image, got '%v'", f.Build.Image) } - // Upon pipeline run, the function should be populated; + // Upon pipeline run, the .Deploy.Image should be populated if f, err = client.RunPipeline(context.Background(), f); err != nil { t.Fatal(err) } expected := "example.com/alice/myfunc:latest" - if f.Image != expected { - t.Fatalf("expected image '%v', got '%v'", expected, f.Image) + if f.Deploy.Image != expected { + t.Fatalf("expected image '%v', got '%v'", expected, f.Deploy.Image) } expected = "example.com/alice" if f.Registry != expected { @@ -1305,8 +1373,9 @@ func TestClient_Pipelines_Deploy_Image(t *testing.T) { t.Fatal(err) } expected = "registry2.example.com/bob/myfunc:latest" - if f.Image != expected { - t.Fatalf("expected image '%v', got '%v'", expected, f.Image) + + if f.Deploy.Image != expected { + t.Fatalf("expected image '%v', got '%v'", expected, f.Deploy.Image) } expected = "example.com/alice" if f.Registry != expected { @@ -1321,6 +1390,47 @@ func TestClient_Pipelines_Deploy_Image(t *testing.T) { } } +// TestClient_Pipelines_Deploy_Namespace ensures that correct namespace is returned +// when using remote deployment +func TestClient_Pipelines_Deploy_Namespace(t *testing.T) { + root, rm := Mktemp(t) + defer rm() + + pprovider := mock.NewPipelinesProvider() + pprovider.RunFn = func(f fn.Function) (string, string, error) { + // simulate function getting deployed here and return namespace + return "", f.Namespace, nil + } + + client := fn.New( + fn.WithPipelinesProvider(pprovider), + fn.WithRegistry("example.com/alice")) + + f := fn.Function{ + Name: "myfunc", + Runtime: "node", + Root: root, + Namespace: "myns", + Build: fn.BuildSpec{ + Git: fn.Git{URL: "http://example-git.com/alice/myfunc.git"}, + }, + } + + f, err := client.Init(f) + if err != nil { + t.Fatal(err) + } + + if f, err = client.RunPipeline(context.Background(), f); err != nil { + t.Fatal(err) + } + + // function is deployed in correct ns + if f.Deploy.Namespace != "myns" { + t.Fatalf("expected namespace to be '%s' but is '%s'", "myns", f.Deploy.Namespace) + } +} + // TestClient_Deploy_UnbuiltErrors ensures that a call to deploy a function // which was not fully created (ie. was only initialized, not actually built // or deployed) yields the expected error. @@ -1888,3 +1998,147 @@ func TestClient_RunRediness(t *testing.T) { t.Fatalf("err on job stop. %v", err) } } + +// TestClient_BuildCleanFingerprint ensures that when building a Function the +// source controlled state is not modified (git would show no unstaged changes). +// For example, the image name generated when building should not be stored +// in function metadata that is checked into source control (func.yaml). +func TestClient_BuildCleanFingerprint(t *testing.T) { + + // Create a temporary directory + root, cleanup := Mktemp(t) + defer cleanup() + + // create new client + client := fn.New() + + f := fn.Function{Root: root, Runtime: TestRuntime, Registry: TestRegistry} + ctx := context.Background() + + // init a new Function + f, err := client.Init(f) + if err != nil { + t.Fatal(err) + } + + // NOTE: Practically one would initialize a git repository, check the source code + // and compare that way. For now this only compares fingerprint before and after + // building Function + + // get fingerprint before building + hashA, _, err := fn.Fingerprint(root) + if err != nil { + t.Fatal(err) + } + + // Build a function + if f, err = client.Build(ctx, f); err != nil { + t.Fatal(err) + } + + // Write to disk because client.Build just stamps (writing is handled in its caller) + if err = f.Write(); err != nil { + t.Fatal(err) + } + + // compare fingerprints before and after + hashB, _, err := fn.Fingerprint(root) + if err != nil { + t.Fatal(err) + } + if hashA != hashB { + t.Fatal("just building a Function resulted in a dirty function state (fingerprint changed)") + } +} + +// TestClient_DeployRemoves ensures that the Remover is invoked when a +// function is moved to a new namespace. +// specifically: deploy to 'nsone' -> simulate change of namespace with change to +// f.Namespace -> redeploy to that namespace and expect the remover to be invoked +// for old Function in ns 'nsone'. +func TestClient_DeployRemoves(t *testing.T) { + // Create a temporary directory + root, cleanup := Mktemp(t) + defer cleanup() + + var ( + ctx = context.Background() + nsOne = "nsone" + nsTwo = "nstwo" + testname = "testfunc" + remover = mock.NewRemover() + ) + + remover.RemoveFn = func(n, ns string) error { + if ns != nsOne { + t.Fatalf("expected delete namespace %v, got %v", nsOne, ns) + } + if n != testname { + t.Fatalf("expected delete name %v, got %v", testname, n) + } + return nil + } + + client := fn.New(fn.WithRemover(remover)) + // initialize function with namespace defined as nsone + + f, err := client.Init(fn.Function{Runtime: "go", Root: root, + Namespace: nsOne, Name: testname, Registry: TestRegistry}) + if err != nil { + t.Fatal(err) + } + + f, err = client.Build(ctx, f) + if err != nil { + t.Fatal(err) + } + + // first deploy + f, err = client.Deploy(ctx, f) + if err != nil { + t.Fatal(err) + } + + // change namespace + f.Namespace = nsTwo + + // redeploy to different namespace + f, err = client.Deploy(ctx, f) + if err != nil { + t.Fatal(err) + } + + // check that a remove was invoked getting rid of the old Function + if !remover.RemoveInvoked { + t.Fatal(fmt.Errorf("remover was not invoked on an old function")) + } +} + +// TestClient_BuildPopulatesRuntimeImage ensures that building populates runtime +// metadata (.func/built-image) image. +func TestClient_BuildPopulatesRuntimeImage(t *testing.T) { + // Create a temporary directory + root, cleanup := Mktemp(t) + defer cleanup() + + client := fn.New() + f, err := client.Init(fn.Function{Runtime: "go", Root: root, Registry: TestRegistry}) + if err != nil { + t.Fatal(err) + } + + expect := f.Registry + "/" + f.Name + ":latest" + + f, err = client.Build(context.Background(), f) + if err != nil { + t.Fatal(err) + } + got, err := os.ReadFile(path.Join(root, ".func/built-image")) + if err != nil { + t.Fatal(err) + } + + if string(got) != expect { + t.Fatalf("written image in ./.func/built-image '%s' does not match expected '%s'", got, expect) + } +} diff --git a/pkg/functions/errors.go b/pkg/functions/errors.go index db2520e59d..f3c0ef0b96 100644 --- a/pkg/functions/errors.go +++ b/pkg/functions/errors.go @@ -8,8 +8,10 @@ import ( var ( ErrEnvironmentNotFound = errors.New("environment not found") + ErrFunctionNotFound = errors.New("function not found") ErrMismatchedName = errors.New("name passed the function source") ErrNameRequired = errors.New("name required") + ErrNamespaceRequired = errors.New("namespace required") ErrNotBuilt = errors.New("not built") ErrNotRunning = errors.New("function not running") ErrRepositoriesNotDefined = errors.New("custom template repositories location not specified") diff --git a/pkg/functions/function.go b/pkg/functions/function.go index 13ebe8bb44..66436edb93 100644 --- a/pkg/functions/function.go +++ b/pkg/functions/function.go @@ -24,6 +24,14 @@ const ( // By default it is excluded from source control. RunDataDir = ".func" RunDataLocalFile = "local.yaml" + + // BuiltHash is a name of a file that holds hash of built Function in runtime + // metadata dir (RunDataDir) + BuiltHash = "built-hash" + + // BuiltImage is a name of a file that holds name of built image in runtime + // metadata dir (RunDataDir) + BuiltImage = "built-image" ) // Local represents the transient runtime metadata which @@ -76,8 +84,8 @@ type Function struct { // "Registry+Name:latest" to derive the Image. Image string `yaml:"image,omitempty"` - // ImageDigest is the SHA256 hash of the latest image that has been built - ImageDigest string `yaml:"imageDigest,omitempty"` + // Namespace in which to deploy the Function + Namespace string `yaml:"namespace,omitempty"` // Created time is the moment that creation was successfully completed // according to the client which is in charge of what constitutes being @@ -132,6 +140,10 @@ type BuildSpec struct { // PVCSize specifies the size of persistent volume claim used to store function // when using deployment and remote build process (only relevant when Remote is true). PVCSize string `yaml:"pvcSize,omitempty"` + + // Image stores last built image name NOT in func.yaml, but instead + // in .func/built-image + Image string `yaml:"-"` } // RunSpec @@ -151,9 +163,12 @@ type RunSpec struct { // DeploySpec type DeploySpec struct { - // Namespace into which the function is deployed on supported platforms. + // Namespace into which the function was deployed on supported platforms. Namespace string `yaml:"namespace,omitempty"` + // Image is the deployed image including sha256 + Image string `yaml:"image,omitempty"` + // Map containing user-supplied annotations // Example: { "division": "finance" } Annotations map[string]string `yaml:"annotations,omitempty"` @@ -272,7 +287,15 @@ func NewFunction(root string) (f Function, err error) { errorText += "\n" + "Migration: " + functionMigrationError.Error() return Function{}, errors.New(errorText) } + f.Local, err = f.newLocal() + if err != nil { + return + } + // ---- LOCAL SETTINGS - STUFF NOT IN FUNC.YAML ---- // + + f.Build.Image, err = f.getLastBuiltImage() + return } @@ -416,7 +439,12 @@ func (f Function) Write() (err error) { } localConfigPath := filepath.Join(f.Root, RunDataDir, RunDataLocalFile) - err = os.WriteFile(localConfigPath, bb, 0644) + if err = os.WriteFile(localConfigPath, bb, 0644); err != nil { + return + } + + // Write built image to .func + err = f.WriteRuntimeBuiltImage(false) return } @@ -463,7 +491,7 @@ func (f Function) Stamp(oo ...stampOption) (err error) { } // Write out the hash - if err = os.WriteFile(filepath.Join(f.Root, RunDataDir, "built"), []byte(hash), os.ModePerm); err != nil { + if err = os.WriteFile(filepath.Join(f.Root, RunDataDir, BuiltHash), []byte(hash), os.ModePerm); err != nil { return } @@ -495,32 +523,6 @@ func (f Function) Initialized() bool { return !f.Created.IsZero() } -// ImageWithDigest returns the full reference to the image including SHA256 Digest. -// If Digest is empty, image:tag is returned. -// TODO: Populate this only on a successful deploy, as this results on a dirty -// git tree on every build. -func (f Function) ImageWithDigest() string { - // Return image, if Digest is empty - if f.ImageDigest == "" { - return f.Image - } - - // Return image with new Digest if image already contains SHA256 Digest - shaIndex := strings.Index(f.Image, "@sha256:") - if shaIndex > 0 { - return f.Image[:shaIndex] + "@" + f.ImageDigest - } - - lastSlashIdx := strings.LastIndexAny(f.Image, "/") - imageAsBytes := []byte(f.Image) - - part1 := string(imageAsBytes[:lastSlashIdx+1]) - part2 := string(imageAsBytes[lastSlashIdx+1:]) - - // Remove tag from the image name and append SHA256 hash instead - return part1 + strings.Split(part2, ":")[0] + "@" + f.ImageDigest -} - // 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 @@ -578,7 +580,7 @@ func (f Function) LabelsMap() (map[string]string, error) { // ImageName returns a full image name (OCI container tag) for the // Function based off of the Function's `Registry` member plus `Name`. -// Used to calculate the final value for .Image when none is provided +// Used to calculate the final value for .Deploy.Image when none is provided // explicitly. // // form: [registry]/[user]/[function]:latest @@ -654,21 +656,6 @@ func (f Function) Built() bool { return false } - // Missing an image name always means !Built (but does not satisfy staleness - // checks). - // NOTE: This will be updated in the future such that a build does not - // automatically update the function's serialized, source-controlled state, - // because merely building does not indicate the function has changed, but - // rather that field should be populated on deploy. I.e. the Image name - // and image stamp should reside as transient data in .func until such time - // as the given image has been deployed. - // An example of how this bug manifests is that every rebuild of a function - // registers the func.yaml as being dirty for source-control purposes, when - // this should only happen on deploy. - if f.Image == "" { - return false - } - // Calculate the current filesystem hash and see if it has changed. // // If this comparison returns true, the Function has a populated image, @@ -682,13 +669,29 @@ func (f Function) Built() bool { fmt.Fprintf(os.Stderr, "error calculating function's fingerprint: %v\n", err) return false } - return stamp == hash + + if stamp != hash { + return false + } + + // Special case of registry change on a subsequent deploy attempt should + // result in unbuilt image, forcing a rebuild if possible + // Example: Deploy with image using registry X. Then subsequently deploy with + // --registry=Y, changing registry resulting in unmatched Registry and Build.Image. + + // If f.Image is specified, registry is overridden -- meaning its not taken into + // consideration and can be different from actually built image. + if !strings.Contains(f.Build.Image, f.Registry) && f.Image == "" { + fmt.Fprintf(os.Stderr, "Warning: registry '%s' does not match currently built image '%s' and no direct image override was provided via --image\n", f.Registry, f.Build.Image) + return false + } + return true } // BuildStamp accesses the current (last) build stamp for the function. // Unbuilt functions return empty string. func (f Function) BuildStamp() string { - path := filepath.Join(f.Root, RunDataDir, "built") + path := filepath.Join(f.Root, RunDataDir, BuiltHash) if _, err := os.Stat(path); err != nil { return "" } @@ -718,3 +721,66 @@ func (f Function) newLocal() (localConfig Local, err error) { err = yaml.Unmarshal(b, &localConfig) return } + +// WriteRuntimeBuiltImage writes built image name into runtime metadata +// directory (.func/) from f.Build.Image +func (f Function) WriteRuntimeBuiltImage(verbose bool) error { + path := filepath.Join(f.Root, RunDataDir, BuiltImage) + + // dont write if empty (not built) + if f.Build.Image == "" { + return nil + } + + if verbose { + fmt.Printf("Writing built image: '%s' at path: '%s'\n", f.Build.Image, path) + } + + return os.WriteFile(path, []byte(f.Build.Image), os.ModePerm) +} + +// getLastBuiltImage reads .func/built-image and returns its value or empty string +// if the file doesnt exist (not built yet). Other errors are returned as usual. +func (f Function) getLastBuiltImage() (string, error) { + path := filepath.Join(f.Root, RunDataDir, BuiltImage) + if _, err := os.Stat(path); err != nil { + if os.IsNotExist(err) { + return "", nil + } + return "", err + } + + b, err := os.ReadFile(path) + if err != nil { + return "", err + } + return string(b), nil +} + +// ImageNameWithDigest works with f.Build.Image and image digest. if the func +// parameter newDigest is empty, just return the image name as is. +// TODO: This function is a temporary one for a workaround for a current +// solution on how the image digest is fetched (which is during/after Push). +// Image digest should be gotten from imageID right after building the Function. +// PS: I think that imageID item contains "sha256:[digest]" +func (f Function) ImageNameWithDigest(newDigest string) string { + if newDigest == "" { + return f.Build.Image + } + image := f.Build.Image + + // overwrite current digest + shaIndex := strings.Index(image, "@sha256:") + if shaIndex > 0 { + return image[:shaIndex] + "@" + newDigest + } + + // image doesnt have a digest yet == image not pushed yet + //parse f.Build.Image to separate its name and tag + lastSlashIdx := strings.LastIndexAny(image, "/") + imageAsBytes := []byte(image) + part1 := string(imageAsBytes[:lastSlashIdx+1]) + part2 := string(imageAsBytes[lastSlashIdx+1:]) + // Remove tag from the image name and append SHA256 hash instead + return part1 + strings.Split(part2, ":")[0] + "@" + newDigest +} diff --git a/pkg/functions/function_unit_test.go b/pkg/functions/function_unit_test.go index d890d73d8b..54e2e50cf9 100644 --- a/pkg/functions/function_unit_test.go +++ b/pkg/functions/function_unit_test.go @@ -116,14 +116,18 @@ func TestFunction_ImageWithDigest(t *testing.T) { want: "image-registry.openshift-image-registry.svc.cluster.local:50000/default/bar@sha256:42", }, } + //TODO: gauron99 - this is gonna need to be changed (probably) because: + // 1: imageDigest now doesnt have a dedicated structure member (resolved?) + // 2: is still fetched after pushing the Function (which is a temporary fix -- it really should be during build) for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { f := Function{ - Image: tt.fields.Image, - ImageDigest: tt.fields.ImageDigest, + Build: BuildSpec{ + Image: tt.fields.Image, + }, } - if got := f.ImageWithDigest(); got != tt.want { - t.Errorf("ImageWithDigest() = %v, want %v", got, tt.want) + if got := f.ImageNameWithDigest(tt.fields.ImageDigest); got != tt.want { + t.Errorf("ImageNameWithDigest(tt.fields.ImageDigest) = %v, want %v", got, tt.want) } }) } diff --git a/pkg/k8s/client.go b/pkg/k8s/client.go index 7fe86a5455..ff069f95bb 100644 --- a/pkg/k8s/client.go +++ b/pkg/k8s/client.go @@ -9,14 +9,17 @@ import ( "k8s.io/client-go/tools/clientcmd" ) -func NewClientAndResolvedNamespace(defaultNamespace string) (client *kubernetes.Clientset, namespace string, err error) { - namespace, err = GetNamespace(defaultNamespace) - if err != nil { - return +func NewClientAndResolvedNamespace(ns string) (*kubernetes.Clientset, string, error) { + var err error + if ns == "" { + ns, err = GetDefaultNamespace() + if err != nil { + return nil, ns, err + } } - client, err = NewKubernetesClientset() - return + client, err := NewKubernetesClientset() + return client, ns, err } func NewKubernetesClientset() (*kubernetes.Clientset, error) { @@ -37,15 +40,9 @@ func NewDynamicClient() (dynamic.Interface, error) { return dynamic.NewForConfig(restConfig) } -func GetNamespace(defaultNamespace string) (namespace string, err error) { - namespace = defaultNamespace - - if defaultNamespace == "" { - namespace, _, err = GetClientConfig().Namespace() - if err != nil { - return - } - } +// GetDefaultNamespace returns default namespace +func GetDefaultNamespace() (namespace string, err error) { + namespace, _, err = GetClientConfig().Namespace() return } diff --git a/pkg/k8s/openshift.go b/pkg/k8s/openshift.go index 6af64c5407..5797e5986f 100644 --- a/pkg/k8s/openshift.go +++ b/pkg/k8s/openshift.go @@ -86,7 +86,7 @@ func GetOpenShiftServiceCA(ctx context.Context) (*x509.Certificate, error) { } func GetDefaultOpenShiftRegistry() string { - ns, _ := GetNamespace("") + ns, _ := GetDefaultNamespace() if ns == "" { ns = "default" } diff --git a/pkg/k8s/persistent_volumes.go b/pkg/k8s/persistent_volumes.go index 19b1706870..6c60e1ad49 100644 --- a/pkg/k8s/persistent_volumes.go +++ b/pkg/k8s/persistent_volumes.go @@ -96,9 +96,11 @@ func runWithVolumeMounted(ctx context.Context, podImage string, podCommand []str return fmt.Errorf("cannot create k8s client: %w", err) } - namespace, err = GetNamespace(namespace) - if err != nil { - return fmt.Errorf("cannot get namespace: %w", err) + if namespace == "" { + namespace, err = GetDefaultNamespace() + if err != nil { + return fmt.Errorf("cannot get namespace: %w", err) + } } podName := "volume-uploader-" + rand.String(5) diff --git a/pkg/knative/deployer.go b/pkg/knative/deployer.go index 26cb7455ee..836f3cfded 100644 --- a/pkg/knative/deployer.go +++ b/pkg/knative/deployer.go @@ -33,6 +33,9 @@ import ( const LIVENESS_ENDPOINT = "/health/liveness" const READINESS_ENDPOINT = "/health/readiness" +// static default namespace for deployer +const StaticDefaultNamespace = "func" + type DeployDecorator interface { UpdateAnnotations(fn.Function, map[string]string) map[string]string UpdateLabels(fn.Function, map[string]string) map[string]string @@ -50,10 +53,10 @@ type Deployer struct { decorator DeployDecorator } -// DefaultNamespace attempts to read the kubernetes active namepsace. +// ActiveNamespace attempts to read the kubernetes active namepsace. // Missing configs or not having an active kuberentes configuration are // equivalent to having no default namespace (empty string). -func DefaultNamespace() string { +func ActiveNamespace() string { // Get client config, if it exists, and from that the namespace ns, _, err := k8s.GetClientConfig().Namespace() if err != nil { @@ -92,8 +95,8 @@ func WithDeployerDecorator(decorator DeployDecorator) DeployerOpt { // Checks the status of the "user-container" for the ImagePullBackOff reason meaning that // the container image is not reachable probably because a private registry is being used. -func (d *Deployer) isImageInPrivateRegistry(ctx context.Context, client clientservingv1.KnServingClient, funcName string) bool { - ksvc, err := client.GetService(ctx, funcName) +func (d *Deployer) isImageInPrivateRegistry(ctx context.Context, client clientservingv1.KnServingClient, f fn.Function) bool { + ksvc, err := client.GetService(ctx, f.Name) if err != nil { return false } @@ -101,8 +104,8 @@ func (d *Deployer) isImageInPrivateRegistry(ctx context.Context, client clientse if err != nil { return false } - list, err := k8sClient.CoreV1().Pods(d.Namespace).List(ctx, metav1.ListOptions{ - LabelSelector: "serving.knative.dev/revision=" + ksvc.Status.LatestCreatedRevisionName + ",serving.knative.dev/service=" + funcName, + list, err := k8sClient.CoreV1().Pods(namespace(d.Namespace, f)).List(ctx, metav1.ListOptions{ + LabelSelector: "serving.knative.dev/revision=" + ksvc.Status.LatestCreatedRevisionName + ",serving.knative.dev/service=" + f.Name, FieldSelector: "status.phase=Pending", }) if err != nil { @@ -120,20 +123,45 @@ func (d *Deployer) isImageInPrivateRegistry(ctx context.Context, client clientse return false } -func (d *Deployer) Deploy(ctx context.Context, f fn.Function) (fn.DeploymentResult, error) { - var err error - if d.Namespace == "" { - d.Namespace, err = k8s.GetNamespace(d.Namespace) +// returns correct namespace to deploy to, ordered in a descending order by +// priority: User specified via cli -> client WithDeployer -> already deployed -> +// -> k8s default; if fails, use static default +func namespace(dflt string, f fn.Function) string { + // namespace ordered by highest priority decending + namespace := f.Namespace + + // if deployed before: use already deployed namespace + if namespace == "" { + namespace = f.Deploy.Namespace + } + + // deployer WithDeployerNamespace provided + if namespace == "" { + namespace = dflt + } + + if namespace == "" { + var err error + // still not set, just use the defaultest default + namespace, err = k8s.GetDefaultNamespace() if err != nil { - return fn.DeploymentResult{}, err + fmt.Fprintf(os.Stderr, "trying to get default namespace returns an error: '%s'\nSetting static default namespace '%s'", err, StaticDefaultNamespace) + namespace = StaticDefaultNamespace } } + return namespace +} + +func (d *Deployer) Deploy(ctx context.Context, f fn.Function) (fn.DeploymentResult, error) { + + // returns correct namespace by priority + namespace := namespace(d.Namespace, f) - client, err := NewServingClient(d.Namespace) + client, err := NewServingClient(namespace) if err != nil { return fn.DeploymentResult{}, err } - eventingClient, err := NewEventingClient(d.Namespace) + eventingClient, err := NewEventingClient(namespace) if err != nil { return fn.DeploymentResult{}, err } @@ -146,7 +174,7 @@ func (d *Deployer) Deploy(ctx context.Context, f fn.Function) (fn.DeploymentResu } since := time.Now() go func() { - _ = GetKServiceLogs(ctx, d.Namespace, f.Name, f.ImageWithDigest(), &since, out) + _ = GetKServiceLogs(ctx, namespace, f.Name, f.Deploy.Image, &since, out) }() previousService, err := client.GetService(ctx, f.Name) @@ -163,7 +191,7 @@ func (d *Deployer) Deploy(ctx context.Context, f fn.Function) (fn.DeploymentResu return fn.DeploymentResult{}, err } - err = checkResourcesArePresent(ctx, d.Namespace, &referencedSecrets, &referencedConfigMaps, &referencedPVCs, f.Deploy.ServiceAccountName) + err = checkResourcesArePresent(ctx, namespace, &referencedSecrets, &referencedConfigMaps, &referencedPVCs, f.Deploy.ServiceAccountName) if err != nil { err = fmt.Errorf("knative deployer failed to generate the Knative Service: %v", err) return fn.DeploymentResult{}, err @@ -184,7 +212,7 @@ func (d *Deployer) Deploy(ctx context.Context, f fn.Function) (fn.DeploymentResu private := false for !private { time.Sleep(5 * time.Second) - private = d.isImageInPrivateRegistry(ctx, client, f.Name) + private = d.isImageInPrivateRegistry(ctx, client, f) chprivate <- private } close(chprivate) @@ -237,12 +265,12 @@ func (d *Deployer) Deploy(ctx context.Context, f fn.Function) (fn.DeploymentResu } if d.verbose { - fmt.Printf("Function deployed in namespace %q and exposed at URL:\n%s\n", d.Namespace, route.Status.URL.String()) + fmt.Printf("Function deployed in namespace %q and exposed at URL:\n%s\n", namespace, route.Status.URL.String()) } return fn.DeploymentResult{ Status: fn.Deployed, URL: route.Status.URL.String(), - Namespace: d.Namespace, + Namespace: namespace, }, nil } else { @@ -265,7 +293,7 @@ func (d *Deployer) Deploy(ctx context.Context, f fn.Function) (fn.DeploymentResu return fn.DeploymentResult{}, err } - err = checkResourcesArePresent(ctx, d.Namespace, &referencedSecrets, &referencedConfigMaps, &referencedPVCs, f.Deploy.ServiceAccountName) + err = checkResourcesArePresent(ctx, namespace, &referencedSecrets, &referencedConfigMaps, &referencedPVCs, f.Deploy.ServiceAccountName) if err != nil { err = fmt.Errorf("knative deployer failed to update the Knative Service: %v", err) return fn.DeploymentResult{}, err @@ -303,7 +331,7 @@ func (d *Deployer) Deploy(ctx context.Context, f fn.Function) (fn.DeploymentResu return fn.DeploymentResult{ Status: fn.Updated, URL: route.Status.URL.String(), - Namespace: d.Namespace, + Namespace: namespace, }, nil } } @@ -395,7 +423,7 @@ func generateNewService(f fn.Function, decorator DeployDecorator) (*v1.Service, Type: corev1.SeccompProfileType("RuntimeDefault"), } container := corev1.Container{ - Image: f.ImageWithDigest(), + Image: f.Deploy.Image, SecurityContext: &corev1.SecurityContext{ RunAsNonRoot: &runAsNonRoot, AllowPrivilegeEscalation: &allowPrivilegeEscalation, @@ -585,7 +613,7 @@ func updateService(f fn.Function, previousService *v1.Service, newEnv []corev1.E service.ObjectMeta.Labels = labels service.Spec.Template.ObjectMeta.Labels = labels - err = flags.UpdateImage(&service.Spec.Template.Spec.PodSpec, f.ImageWithDigest()) + err = flags.UpdateImage(&service.Spec.Template.Spec.PodSpec, f.Deploy.Image) if err != nil { return service, err } diff --git a/pkg/knative/deployer_test.go b/pkg/knative/deployer_test.go index f4b1c53bdd..88e6694625 100644 --- a/pkg/knative/deployer_test.go +++ b/pkg/knative/deployer_test.go @@ -14,14 +14,14 @@ import ( // Test_DefaultNamespace ensures that if there is an active kubeconfig, // that namespace will be returned as the default from the public -// DefaultNamespae accessor, empty string otherwise. +// DefaultNamespace accessor, empty string otherwise. func Test_DefaultNamespace(t *testing.T) { // Update Kubeconfig to indicate the currently active namespace is: // "test-ns-deploy" t.Setenv("KUBECONFIG", fmt.Sprintf("%s/testdata/test_default_namespace", cwd())) - if DefaultNamespace() != "test-ns-deploy" { - t.Fatalf("expected 'test-ns-deploy', got '%v'", DefaultNamespace()) + if ActiveNamespace() != "test-ns-deploy" { + t.Fatalf("expected 'test-ns-deploy', got '%v'", ActiveNamespace()) } } @@ -115,3 +115,46 @@ func Test_processValue(t *testing.T) { }) } } + +// Test_deployerNamespace tests that namespace function returns what it should +// via preferences. namespace() is used in knative deployer to determine what +// namespace to deploy the function to. +func Test_deployerNamespace(t *testing.T) { + // these are the namespaces being used descending in preference (top = highest pref) + var ( + desiredNs = "desiredNs" + deployedNs = "deployedNs" + deployerNs = "deployerNs" + defaultNs = "test-ns-deploy" + // StaticDefaultNamespace -- is exported + ) + f := fn.Function{Name: "myfunc"} + + //set static default + if ns := namespace("", f); ns != StaticDefaultNamespace { + t.Fatal("expected static default namespace") + } + t.Setenv("KUBECONFIG", fmt.Sprintf("%s/testdata/test_default_namespace", cwd())) + + // active kubernetes default + if ns := namespace("", f); ns != defaultNs { + t.Fatal("expected default k8s namespace") + } + + // knative deployer namespace + if ns := namespace(deployerNs, f); ns != deployerNs { + t.Fatal("expected knative deployer namespace") + } + + // already deployed namespace + f.Deploy.Namespace = deployedNs + if ns := namespace(deployerNs, f); ns != deployedNs { + t.Fatal("expected namespace where function is already deployed") + } + + // desired namespace + f.Namespace = desiredNs + if ns := namespace(deployerNs, f); ns != desiredNs { + t.Fatal("expected desired namespace defined via f.Namespace") + } +} diff --git a/pkg/knative/describer.go b/pkg/knative/describer.go index 22247f1607..0c8ca056bd 100644 --- a/pkg/knative/describer.go +++ b/pkg/knative/describer.go @@ -29,7 +29,7 @@ func NewDescriber(namespaceOverride string, verbose bool) *Describer { // www.example-site.com -> www-example--site-com func (d *Describer) Describe(ctx context.Context, name string) (description fn.Instance, err error) { if d.namespace == "" { - d.namespace, err = k8s.GetNamespace(d.namespace) + d.namespace, err = k8s.GetDefaultNamespace() if err != nil { return fn.Instance{}, err } diff --git a/pkg/knative/integration_test.go b/pkg/knative/integration_test.go index cf67e0024a..5df5a18ff1 100644 --- a/pkg/knative/integration_test.go +++ b/pkg/knative/integration_test.go @@ -127,10 +127,11 @@ func TestIntegration(t *testing.T) { // * environment variables starting which name starts with FUNC_TEST, // * files under /etc/cm and /etc/sc. // * application also prints the same info to stderr on startup - Image: "quay.io/mvasek/func-test-service", - ImageDigest: "sha256:2eca4de00d7569c8791634bdbb0c4d5ec8fb061b001549314591e839dabd5269", - Created: now, + Created: now, Deploy: fn.DeploySpec{ + // TODO: gauron99 - is it okay to have this explicitly set to deploy.image already? + // With this I skip the logic of setting the .Deploy.Image field but it should be fine for this test + Image: "quay.io/mvasek/func-test-service@sha256:2eca4de00d7569c8791634bdbb0c4d5ec8fb061b001549314591e839dabd5269", Namespace: namespace, Labels: []fn.Label{{Key: ptr("my-label"), Value: ptr("my-label-value")}}, Options: fn.Options{ @@ -155,7 +156,7 @@ func TestIntegration(t *testing.T) { var buff = &knative.SynchronizedBuffer{} go func() { - _ = knative.GetKServiceLogs(ctx, namespace, functionName, function.ImageWithDigest(), &now, buff) + _ = knative.GetKServiceLogs(ctx, namespace, functionName, function.Deploy.Image, &now, buff) }() deployer := knative.NewDeployer(knative.WithDeployerNamespace(namespace), knative.WithDeployerVerbose(false)) @@ -270,8 +271,8 @@ func TestIntegration(t *testing.T) { t.Error("environment variable was not set from config-map") } - remover := knative.NewRemover(namespace, false) - err = remover.Remove(ctx, functionName) + remover := knative.NewRemover(false) + err = remover.Remove(ctx, functionName, namespace) if err != nil { t.Fatal(err) } diff --git a/pkg/knative/lister.go b/pkg/knative/lister.go index d992928ea1..0328648a80 100644 --- a/pkg/knative/lister.go +++ b/pkg/knative/lister.go @@ -26,7 +26,7 @@ func NewLister(namespaceOverride string, verbose bool) *Lister { func (l *Lister) List(ctx context.Context) (items []fn.ListItem, err error) { if l.Namespace == "" { - l.Namespace, err = k8s.GetNamespace(l.Namespace) + l.Namespace, err = k8s.GetDefaultNamespace() if err != nil { return nil, err } diff --git a/pkg/knative/remover.go b/pkg/knative/remover.go index 1a3cac4921..e04c813769 100644 --- a/pkg/knative/remover.go +++ b/pkg/knative/remover.go @@ -3,40 +3,41 @@ package knative import ( "context" "fmt" + "os" "time" - "knative.dev/func/pkg/k8s" + apiErrors "k8s.io/apimachinery/pkg/api/errors" + fn "knative.dev/func/pkg/functions" ) const RemoveTimeout = 120 * time.Second -func NewRemover(namespaceOverride string, verbose bool) *Remover { +func NewRemover(verbose bool) *Remover { return &Remover{ - Namespace: namespaceOverride, - verbose: verbose, + verbose: verbose, } } type Remover struct { - Namespace string - verbose bool + verbose bool } -func (remover *Remover) Remove(ctx context.Context, name string) (err error) { - if remover.Namespace == "" { - remover.Namespace, err = k8s.GetNamespace(remover.Namespace) - if err != nil { - return err - } +func (remover *Remover) Remove(ctx context.Context, name, ns string) (err error) { + if ns == "" { + fmt.Fprintf(os.Stderr, "no namespace defined when trying to delete a function in knative remover\n") + return fn.ErrNamespaceRequired } - client, err := NewServingClient(remover.Namespace) + client, err := NewServingClient(ns) if err != nil { return } err = client.DeleteService(ctx, name, RemoveTimeout) if err != nil { + if apiErrors.IsNotFound(err) { + return fn.ErrFunctionNotFound + } err = fmt.Errorf("knative remover failed to delete the service: %v", err) } diff --git a/pkg/mock/deployer.go b/pkg/mock/deployer.go index b7618fa364..7a4ecb44e2 100644 --- a/pkg/mock/deployer.go +++ b/pkg/mock/deployer.go @@ -10,7 +10,7 @@ import ( // See deployer implementations for tests which ensure the currently // active kube namespace is chosen when no explicit namespace is provided. // This mock emulates a deployer which responds that the function was deployed -// to that defined on the function, or "default" if not defined. +// to desired or previously-deployed ns or "default" if not defined. const DefaultNamespace = "default" type Deployer struct { @@ -21,10 +21,14 @@ type Deployer struct { func NewDeployer() *Deployer { return &Deployer{ DeployFn: func(_ context.Context, f fn.Function) (fn.DeploymentResult, error) { - result := fn.DeploymentResult{Namespace: DefaultNamespace} - if f.Deploy.Namespace != "" { + result := fn.DeploymentResult{} + result.Namespace = f.Namespace + if result.Namespace == "" { result.Namespace = f.Deploy.Namespace } + if result.Namespace == "" { + result.Namespace = DefaultNamespace + } return result, nil }, } diff --git a/pkg/mock/pipelines_provider.go b/pkg/mock/pipelines_provider.go index 95fb3f4910..3abd438227 100644 --- a/pkg/mock/pipelines_provider.go +++ b/pkg/mock/pipelines_provider.go @@ -8,7 +8,7 @@ import ( type PipelinesProvider struct { RunInvoked bool - RunFn func(fn.Function) (string, error) + RunFn func(fn.Function) (string, string, error) RemoveInvoked bool RemoveFn func(fn.Function) error ConfigurePACInvoked bool @@ -19,14 +19,25 @@ type PipelinesProvider struct { func NewPipelinesProvider() *PipelinesProvider { return &PipelinesProvider{ - RunFn: func(fn.Function) (string, error) { return "", nil }, + RunFn: func(f fn.Function) (string, string, error) { + // simplified namespace resolution, doesnt take current k8s context into + // account and returns DefaultNamespace if nothing else instead + ns := f.Namespace + if ns == "" { + ns = f.Deploy.Namespace + } + if ns == "" { + ns = DefaultNamespace + } + return "", ns, nil + }, RemoveFn: func(fn.Function) error { return nil }, ConfigurePACFn: func(fn.Function) error { return nil }, RemovePACFn: func(fn.Function) error { return nil }, } } -func (p *PipelinesProvider) Run(ctx context.Context, f fn.Function) (string, error) { +func (p *PipelinesProvider) Run(ctx context.Context, f fn.Function) (string, string, error) { p.RunInvoked = true return p.RunFn(f) } diff --git a/pkg/mock/remover.go b/pkg/mock/remover.go index 88d4a9fba0..69572f4fef 100644 --- a/pkg/mock/remover.go +++ b/pkg/mock/remover.go @@ -4,14 +4,14 @@ import "context" type Remover struct { RemoveInvoked bool - RemoveFn func(string) error + RemoveFn func(string, string) error } func NewRemover() *Remover { - return &Remover{RemoveFn: func(string) error { return nil }} + return &Remover{RemoveFn: func(string, string) error { return nil }} } -func (r *Remover) Remove(ctx context.Context, name string) error { +func (r *Remover) Remove(ctx context.Context, name, ns string) error { r.RemoveInvoked = true - return r.RemoveFn(name) + return r.RemoveFn(name, ns) } diff --git a/pkg/oci/pusher.go b/pkg/oci/pusher.go index fde2830d78..bbcc26e9a7 100644 --- a/pkg/oci/pusher.go +++ b/pkg/oci/pusher.go @@ -47,7 +47,7 @@ func (p *Pusher) Push(ctx context.Context, f fn.Function) (digest string, err er // TODO: GitOps Tagging: tag :latest by default, :[branch] for pinned // environments and :[user]-[branch] for development/testing feature branches. // has been enabled, where branch is tag-encoded. - ref, err := name.ParseReference(f.Image) + ref, err := name.ParseReference(f.Build.Image) if err != nil { return } diff --git a/pkg/pipelines/tekton/client.go b/pkg/pipelines/tekton/client.go index 6fb10f4c62..7ffcedcbbe 100644 --- a/pkg/pipelines/tekton/client.go +++ b/pkg/pipelines/tekton/client.go @@ -13,10 +13,14 @@ const ( DefaultWaitingTimeout = 120 * time.Second ) -func NewTektonClientAndResolvedNamespace(defaultNamespace string) (*v1beta1.TektonV1beta1Client, string, error) { - namespace, err := k8s.GetNamespace(defaultNamespace) - if err != nil { - return nil, "", err +// NewTektonClientAndResolvedNamespace returns TektonV1beta1Client,namespace,error +func NewTektonClientAndResolvedNamespace(namespace string) (*v1beta1.TektonV1beta1Client, string, error) { + var err error + if namespace == "" { + namespace, err = k8s.GetDefaultNamespace() + if err != nil { + return nil, "", err + } } restConfig, err := k8s.GetClientConfig().ClientConfig() diff --git a/pkg/pipelines/tekton/pac/client.go b/pkg/pipelines/tekton/pac/client.go index aa66498f37..863cb14ad3 100644 --- a/pkg/pipelines/tekton/pac/client.go +++ b/pkg/pipelines/tekton/pac/client.go @@ -8,20 +8,24 @@ import ( "knative.dev/func/pkg/k8s" ) -func NewTektonPacClientAndResolvedNamespace(defaultNamespace string) (*pacv1alpha1.PipelinesascodeV1alpha1Client, string, error) { - namespace, err := k8s.GetNamespace(defaultNamespace) - if err != nil { - return nil, "", err +// NewTektonPacClientAndResolvedNamespace returns PipelinesascodeV1alpha1Client,namespace,error +func NewTektonPacClientAndResolvedNamespace(namespace string) (*pacv1alpha1.PipelinesascodeV1alpha1Client, string, error) { + var err error + if namespace != "" { + namespace, err = k8s.GetDefaultNamespace() + if err != nil { + return nil, "", err + } } restConfig, err := k8s.GetClientConfig().ClientConfig() if err != nil { - return nil, "", fmt.Errorf("failed to create new tekton pac client: %w", err) + return nil, namespace, fmt.Errorf("failed to create new tekton pac client: %w", err) } client, err := pacv1alpha1.NewForConfig(restConfig) if err != nil { - return nil, "", fmt.Errorf("failed to create new tekton pac client: %v", err) + return nil, namespace, fmt.Errorf("failed to create new tekton pac client: %v", err) } return client, namespace, nil diff --git a/pkg/pipelines/tekton/pipelines_integration_test.go b/pkg/pipelines/tekton/pipelines_integration_test.go index b10f66de53..a976d0cbf6 100644 --- a/pkg/pipelines/tekton/pipelines_integration_test.go +++ b/pkg/pipelines/tekton/pipelines_integration_test.go @@ -64,7 +64,10 @@ func TestOnClusterBuild(t *testing.T) { f := createSimpleGoProject(t, ns) f.Build.Builder = test.Builder - url, err := pp.Run(ctx, f) + // simulate deploying by passing the image + f.Deploy.Image = f.Image + + url, nsReturned, err := pp.Run(ctx, f) if err != nil { t.Error(err) cancel() @@ -74,6 +77,11 @@ func TestOnClusterBuild(t *testing.T) { cancel() } + if nsReturned == "" || nsReturned != ns { + t.Errorf("namespace returned is empty or does not match: '%s' should be '%s'", nsReturned, ns) + cancel() + } + resp, err := http.Get(url) if err != nil { t.Error(err) diff --git a/pkg/pipelines/tekton/pipelines_provider.go b/pkg/pipelines/tekton/pipelines_provider.go index 618cc9c979..7398037a7e 100644 --- a/pkg/pipelines/tekton/pipelines_provider.go +++ b/pkg/pipelines/tekton/pipelines_provider.go @@ -37,6 +37,9 @@ import ( "knative.dev/pkg/apis" ) +// static const namespace for deployement when everything else fails +const StaticDefaultNamespace = "func" + // DefaultPersistentVolumeClaimSize to allocate for the function. var DefaultPersistentVolumeClaimSize = resource.MustParse("256Mi") @@ -109,23 +112,26 @@ func NewPipelinesProvider(opts ...Opt) *PipelinesProvider { // Run creates a Tekton Pipeline and all necessary resources (PVCs, Secrets, SAs,...) for the input Function. // It ensures that all needed resources are present on the cluster so the PipelineRun can be initialized. // After the PipelineRun is being initialized, the progress of the PipelineRun is being watched and printed to the output. -func (pp *PipelinesProvider) Run(ctx context.Context, f fn.Function) (string, error) { - fmt.Fprintf(os.Stderr, "Creating Pipeline resources") +func (pp *PipelinesProvider) Run(ctx context.Context, f fn.Function) (string, string, error) { + fmt.Fprintf(os.Stderr, "Creating Pipeline resources\n") var err error if err = validatePipeline(f); err != nil { - return "", err + return "", "", err } + // namespace resolution + pp.namespace = namespace(pp.namespace, f) + client, namespace, err := NewTektonClientAndResolvedNamespace(pp.namespace) if err != nil { - return "", err + return "", "", err } pp.namespace = namespace // let's specify labels that will be applied to every resource that is created for a Pipeline labels, err := f.LabelsMap() if err != nil { - return "", err + return "", "", err } if pp.decorator != nil { labels = pp.decorator.UpdateLabels(f, labels) @@ -133,7 +139,7 @@ func (pp *PipelinesProvider) Run(ctx context.Context, f fn.Function) (string, er err = createPipelinePersistentVolumeClaim(ctx, f, pp.namespace, labels) if err != nil { - return "", err + return "", "", err } if f.Build.Git.URL == "" { @@ -142,7 +148,7 @@ func (pp *PipelinesProvider) Run(ctx context.Context, f fn.Function) (string, er defer content.Close() err = k8s.UploadToVolume(ctx, content, getPipelinePvcName(f), pp.namespace) if err != nil { - return "", fmt.Errorf("cannot upload sources to the PVC: %w", err) + return "", "", fmt.Errorf("cannot upload sources to the PVC: %w", err) } } @@ -150,20 +156,20 @@ func (pp *PipelinesProvider) Run(ctx context.Context, f fn.Function) (string, er if err != nil { if !k8serrors.IsAlreadyExists(err) { if k8serrors.IsNotFound(err) { - return "", fmt.Errorf("problem creating pipeline, missing tekton?: %v", err) + return "", "", fmt.Errorf("problem creating pipeline, missing tekton?: %v", err) } - return "", fmt.Errorf("problem creating pipeline: %v", err) + return "", "", fmt.Errorf("problem creating pipeline: %v", err) } } - registry, err := docker.GetRegistry(f.Image) + registry, err := docker.GetRegistry(f.Deploy.Image) if err != nil { - return "", fmt.Errorf("problem in resolving image registry name: %v", err) + return "", "", fmt.Errorf("problem in resolving image registry name: %v", err) } - creds, err := pp.credentialsProvider(ctx, f.Image) + creds, err := pp.credentialsProvider(ctx, f.Deploy.Image) if err != nil { - return "", err + return "", "", err } if registry == name.DefaultRegistry { @@ -172,7 +178,7 @@ func (pp *PipelinesProvider) Run(ctx context.Context, f fn.Function) (string, er err = k8s.EnsureDockerRegistrySecretExist(ctx, getPipelineSecretName(f), pp.namespace, labels, f.Deploy.Annotations, creds.Username, creds.Password, registry) if err != nil { - return "", fmt.Errorf("problem in creating secret: %v", err) + return "", "", fmt.Errorf("problem in creating secret: %v", err) } if f.Registry == "" { @@ -181,7 +187,7 @@ func (pp *PipelinesProvider) Run(ctx context.Context, f fn.Function) (string, er err = createAndApplyPipelineRunTemplate(f, pp.namespace, labels) if err != nil { - return "", fmt.Errorf("problem in creating pipeline run: %v", err) + return "", "", fmt.Errorf("problem in creating pipeline run: %v", err) } // we need to give k8s time to actually create the Pipeline Run @@ -189,37 +195,37 @@ func (pp *PipelinesProvider) Run(ctx context.Context, f fn.Function) (string, er newestPipelineRun, err := findNewestPipelineRunWithRetry(ctx, f, pp.namespace, client) if err != nil { - return "", fmt.Errorf("problem in listing pipeline runs: %v", err) + return "", "", fmt.Errorf("problem in listing pipeline runs: %v", err) } err = pp.watchPipelineRunProgress(ctx, newestPipelineRun) if err != nil { if !errors.Is(err, context.Canceled) { - return "", fmt.Errorf("problem in watching started pipeline run: %v", err) + return "", "", fmt.Errorf("problem in watching started pipeline run: %v", err) } // TODO replace deletion with pipeline-run cancellation _ = client.PipelineRuns(pp.namespace).Delete(context.TODO(), newestPipelineRun.Name, metav1.DeleteOptions{}) - return "", fmt.Errorf("pipeline run cancelled: %w", context.Canceled) + return "", "", fmt.Errorf("pipeline run cancelled: %w", context.Canceled) } newestPipelineRun, err = client.PipelineRuns(pp.namespace).Get(ctx, newestPipelineRun.Name, metav1.GetOptions{}) if err != nil { - return "", fmt.Errorf("problem in retriving pipeline run status: %v", err) + return "", "", fmt.Errorf("problem in retriving pipeline run status: %v", err) } if newestPipelineRun.Status.GetCondition(apis.ConditionSucceeded).Status == corev1.ConditionFalse { message := getFailedPipelineRunLog(ctx, client, newestPipelineRun, pp.namespace) - return "", fmt.Errorf("function pipeline run has failed with message: \n\n%s", message) + return "", "", fmt.Errorf("function pipeline run has failed with message: \n\n%s", message) } kClient, err := knative.NewServingClient(pp.namespace) if err != nil { - return "", fmt.Errorf("problem in retrieving status of deployed function: %v", err) + return "", "", fmt.Errorf("problem in retrieving status of deployed function: %v", err) } ksvc, err := kClient.GetService(ctx, f.Name) if err != nil { - return "", fmt.Errorf("problem in retrieving status of deployed function: %v", err) + return "", "", fmt.Errorf("problem in retrieving status of deployed function: %v", err) } if ksvc.Generation == 1 { @@ -228,7 +234,7 @@ func (pp *PipelinesProvider) Run(ctx context.Context, f fn.Function) (string, er fmt.Fprintf(os.Stderr, "✅ Function updated in namespace %q and exposed at URL: \n %s\n", ksvc.Namespace, ksvc.Status.URL.String()) } - return ksvc.Status.URL.String(), nil + return ksvc.Status.URL.String(), ksvc.Namespace, nil } // Creates tar stream with the function sources as they were in "./source" directory. @@ -345,8 +351,15 @@ func sourcesAsTarStream(f fn.Function) *io.PipeReader { // Remove tries to remove all resources that are present on the cluster and belongs to the input function and it's pipelines func (pp *PipelinesProvider) Remove(ctx context.Context, f fn.Function) error { - var err error + // expect deployed namespace to be defined since trying to delete + // a function (and its resources) + if f.Deploy.Namespace == "" { + fmt.Print("no namespace defined when trying to delete all resources on cluster regarding function and its pipelines\n") + return fn.ErrNamespaceRequired + } + pp.namespace = f.Deploy.Namespace + var err error errMsg := pp.removeClusterResources(ctx, f) if errMsg != "" { err = fmt.Errorf("%s", errMsg) @@ -542,3 +555,32 @@ func createPipelinePersistentVolumeClaim(ctx context.Context, f fn.Function, nam } return nil } + +// returns correct namespace to deploy to, ordered in a descending order by +// priority: User specified via cli -> client WithDeployer -> already deployed -> +// -> k8s default; if fails, use static default +func namespace(dflt string, f fn.Function) string { + // namespace ordered by highest priority decending + namespace := f.Namespace + + // if deployed before: use already deployed namespace + if namespace == "" { + namespace = f.Deploy.Namespace + } + + // client namespace provided + if namespace == "" { + namespace = dflt + } + + if namespace == "" { + var err error + // still not set, just use the defaultest default + namespace, err = k8s.GetDefaultNamespace() + if err != nil { + fmt.Fprintf(os.Stderr, "trying to get default namespace returns an error: '%s'\nSetting static default namespace '%s'", err, StaticDefaultNamespace) + namespace = StaticDefaultNamespace + } + } + return namespace +} diff --git a/pkg/pipelines/tekton/pipelines_provider_test.go b/pkg/pipelines/tekton/pipelines_provider_test.go index ee7894f443..99a1d6f3a5 100644 --- a/pkg/pipelines/tekton/pipelines_provider_test.go +++ b/pkg/pipelines/tekton/pipelines_provider_test.go @@ -117,6 +117,20 @@ func Test_createPipelinePersistentVolumeClaim(t *testing.T) { }, wantErr: false, }, + { + name: "returns err if namespace not defined and default returns an err", + args: args{ + ctx: context.Background(), + f: fn.Function{}, + namespace: "", + labels: nil, + size: DefaultPersistentVolumeClaimSize.String(), + }, + mock: func(ctx context.Context, name, namespaceOverride string, labels map[string]string, annotations map[string]string, accessMode corev1.PersistentVolumeAccessMode, resourceRequest resource.Quantity) (err error) { + return errors.New("no namespace defined") + }, + wantErr: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { // save current function and restore it at the end diff --git a/pkg/pipelines/tekton/templates.go b/pkg/pipelines/tekton/templates.go index b4f6f22eb6..6a92e62fcd 100644 --- a/pkg/pipelines/tekton/templates.go +++ b/pkg/pipelines/tekton/templates.go @@ -196,7 +196,7 @@ func createPipelineRunTemplatePAC(f fn.Function, labels map[string]string) error Annotations: f.Deploy.Annotations, Labels: labels, ContextDir: contextDir, - FunctionImage: f.Image, + FunctionImage: f.Deploy.Image, Registry: f.Registry, BuilderImage: getBuilderImage(f), BuildEnvs: buildEnvs, @@ -400,7 +400,7 @@ func createAndApplyPipelineRunTemplate(f fn.Function, namespace string, labels m Annotations: f.Deploy.Annotations, Labels: labels, ContextDir: contextDir, - FunctionImage: f.Image, + FunctionImage: f.Deploy.Image, Registry: f.Registry, BuilderImage: getBuilderImage(f), BuildEnvs: buildEnvs, diff --git a/schema/func_yaml-schema.json b/schema/func_yaml-schema.json index bad9f09de8..cda3ad9bef 100644 --- a/schema/func_yaml-schema.json +++ b/schema/func_yaml-schema.json @@ -54,7 +54,11 @@ "properties": { "namespace": { "type": "string", - "description": "Namespace into which the function is deployed on supported platforms." + "description": "Namespace into which the function was deployed on supported platforms." + }, + "image": { + "type": "string", + "description": "Image is the deployed image including sha256" }, "annotations": { "patternProperties": { @@ -157,9 +161,9 @@ "type": "string", "description": "Image is the full OCI image tag in form:\n [registry]/[namespace]/[name]:[tag]\nexample:\n quay.io/alice/my.function.name\nRegistry is optional and is defaulted to DefaultRegistry\nexample:\n alice/my.function.name\nIf Image is provided, it overrides the default of concatenating\n\"Registry+Name:latest\" to derive the Image." }, - "imageDigest": { + "namespace": { "type": "string", - "description": "ImageDigest is the SHA256 hash of the latest image that has been built" + "description": "Namespace in which to deploy the Function" }, "created": { "type": "string",