From f8441988c046ec67387e7d1e4515c9e692a0c277 Mon Sep 17 00:00:00 2001 From: David Fridrich Date: Thu, 16 Jul 2026 15:09:55 +0200 Subject: [PATCH] fix: persist effective deployer in func.yaml; block deployer switch except raw->keda --- cmd/client_test.go | 2 +- cmd/delete.go | 13 +- cmd/delete_test.go | 178 +++++++++++++++++ cmd/deploy.go | 24 +-- cmd/deploy_test.go | 182 ++++++++++++++++++ cmd/func-util/main.go | 14 +- docs/reference/func_delete.md | 3 +- docs/reference/func_deploy.md | 2 +- pkg/config/config.go | 19 +- pkg/config/config_test.go | 1 + .../testing/integration_test_helper.go | 15 +- pkg/deployers/deployers.go | 32 +++ pkg/deployers/deployers_test.go | 39 ++++ .../testing/integration_test_helper.go | 2 +- pkg/functions/client.go | 44 ++++- pkg/functions/client_int_test.go | 5 +- pkg/functions/client_test.go | 176 ++++++++++++++++- pkg/functions/function.go | 11 +- pkg/k8s/deployer.go | 4 +- pkg/keda/deployer.go | 4 +- pkg/knative/deployer.go | 5 +- pkg/lister/testing/integration_test_helper.go | 2 +- pkg/mock/deployer.go | 8 + pkg/pipelines/tekton/pipelines_int_test.go | 2 +- pkg/pipelines/tekton/pipelines_provider.go | 9 + .../testing/integration_test_helper.go | 3 +- schema/func_yaml-schema.json | 11 +- 27 files changed, 754 insertions(+), 56 deletions(-) create mode 100644 pkg/deployers/deployers.go create mode 100644 pkg/deployers/deployers_test.go diff --git a/cmd/client_test.go b/cmd/client_test.go index c767f184fa..4c6519cb8c 100644 --- a/cmd/client_test.go +++ b/cmd/client_test.go @@ -27,7 +27,7 @@ func Test_NewTestClient(t *testing.T) { // Trigger an invocation of the mocks by running the associated client // methods which depend on them - err := client.Remove(t.Context(), "myfunc", "myns", fn.Function{}, true) + _, err := client.Remove(t.Context(), "myfunc", "myns", fn.Function{}, true) if err != nil { t.Fatal(err) } diff --git a/cmd/delete.go b/cmd/delete.go index 103a6270c4..7cf2a24627 100644 --- a/cmd/delete.go +++ b/cmd/delete.go @@ -21,7 +21,8 @@ This command undeploys a function from the cluster. By default the function from the project in the current directory is undeployed. Alternatively either the name of the function can be given as argument or the project path provided with --path. -No local files are deleted. +No local files are deleted. When undeploying by project (not by name), on success +the local func.yaml is updated to reflect that the function is no longer deployed. `, Example: ` # Undeploy the function defined in the local directory @@ -82,13 +83,19 @@ func runDelete(cmd *cobra.Command, args []string, newClient ClientFactory) (err defer done() if cfg.Name != "" { // Delete by name if provided - return client.Remove(cmd.Context(), cfg.Name, cfg.Namespace, fn.Function{}, cfg.All) + _, err = client.Remove(cmd.Context(), cfg.Name, cfg.Namespace, fn.Function{}, cfg.All) + return err } else { // Otherwise; delete the function at path (cwd by default) f, err := fn.NewFunction(cfg.Path) if err != nil { return err } - return client.Remove(cmd.Context(), "", "", f, cfg.All) + // updates f.Deploy. (clears them on success) + f, err = client.Remove(cmd.Context(), "", "", f, cfg.All) + if err != nil { + return err + } + return f.Write() } } diff --git a/cmd/delete_test.go b/cmd/delete_test.go index 9ef36ce06d..1c6efebb6b 100644 --- a/cmd/delete_test.go +++ b/cmd/delete_test.go @@ -7,7 +7,9 @@ import ( "strings" "testing" + "knative.dev/func/pkg/deployers" fn "knative.dev/func/pkg/functions" + "knative.dev/func/pkg/keda" "knative.dev/func/pkg/mock" . "knative.dev/func/pkg/testing" ) @@ -340,3 +342,179 @@ func TestDelete_NoFunctionAtPath(t *testing.T) { t.Fatalf("unexpected error message: %v", err) } } + +// TestDelete_ByProjectClearsDeployedMarker ensures that a successful +// path-based delete persists a cleared function on disk so we have the +// function locally and on-cluster synced. +func TestDelete_ByProjectClearsDeployedMarker(t *testing.T) { + root := FromTempDirectory(t) + f := fn.Function{ + Root: root, + Runtime: "go", + Registry: TestRegistry, + Deployer: keda.KedaDeployerName, // intent - how to deploy + Deploy: fn.DeploySpec{Namespace: "myns", Deployer: keda.KedaDeployerName}, + } + f, err := fn.New().Init(f) + if err != nil { + t.Fatal(err) + } + if err = f.Write(); err != nil { + t.Fatal(err) + } + + remover := mock.NewRemover() + remover.RemoveFn = func(_, _ string) error { return nil } + + cmd := NewDeleteCmd(NewTestClient(fn.WithRemovers(remover))) + cmd.SetArgs([]string{}) + if err := cmd.Execute(); err != nil { + t.Fatal(err) + } + if !remover.RemoveInvoked { + t.Fatal("fn.Remover not invoked") + } + + loaded, err := fn.NewFunction(root) + if err != nil { + t.Fatal(err) + } + if loaded.Deploy.Namespace != "" { + t.Fatalf("expected Deploy.Namespace cleared after a successful undeploy, got %q", loaded.Deploy.Namespace) + } + if loaded.Deploy.Deployer != "" { + t.Fatalf("expected Deploy.Deployer cleared after a successful undeploy, got %q", loaded.Deploy.Deployer) + } + if loaded.Deployer != keda.KedaDeployerName { + t.Fatalf("expected the intended Deployer preserved as a remembered choice, got %q", loaded.Deployer) + } +} + +// TestDelete_ByNameLeavesLocalFunctionUntouched ensures delete-by-name does NOT +// modify the local function's deployed state, even when a local function +// exists in the working directory. This is the distinction we make, its the +// caller's responsibility (of client.Remove) to deal with this. +func TestDelete_ByNameLeavesLocalFunctionUntouched(t *testing.T) { + root := FromTempDirectory(t) + f := fn.Function{ + Root: root, + Runtime: "go", + Registry: TestRegistry, + Name: "localfn", + Deploy: fn.DeploySpec{Namespace: "myns", Deployer: keda.KedaDeployerName}, + } + f, err := fn.New().Init(f) + if err != nil { + t.Fatal(err) + } + if err = f.Write(); err != nil { + t.Fatal(err) + } + + remover := mock.NewRemover() + remover.RemoveFn = func(n, _ string) error { + if n != "otherfn" { + t.Fatalf("expected delete-by-name target %q, got %q", "otherfn", n) + } + return nil + } + + cmd := NewDeleteCmd(NewTestClient(fn.WithRemovers(remover))) + cmd.SetArgs([]string{"otherfn"}) // explicit name, distinct from the local function + if err := cmd.Execute(); err != nil { + t.Fatal(err) + } + if !remover.RemoveInvoked { + t.Fatal("fn.Remover not invoked") + } + + loaded, err := fn.NewFunction(root) + if err != nil { + t.Fatal(err) + } + if loaded.Deploy.Namespace != "myns" { + t.Fatalf("expected delete-by-name to leave the local function untouched, Deploy.Namespace changed to %q", loaded.Deploy.Namespace) + } +} + +// TestDelete_ByProjectPreservesDeployerForRedeploy ensures that redeploying +// after removal keeps the INTENT deployer intact and functional. +func TestDelete_ByProjectPreservesDeployerForRedeploy(t *testing.T) { + root := FromTempDirectory(t) + if _, err := fn.New().Init(fn.Function{Runtime: "go", Root: root, Registry: TestRegistry}); err != nil { + t.Fatal(err) + } + + // Deploy with keda + deployCmd := NewDeployCmd(NewTestClient(fn.WithDeployer(mock.NewDeployer()))) + deployCmd.SetArgs([]string{"--deployer", keda.KedaDeployerName, "--namespace", "myns"}) + if err := deployCmd.Execute(); err != nil { + t.Fatal(err) + } + + // Undeploy by project + remover := mock.NewRemover() + remover.RemoveFn = func(_, _ string) error { return nil } + deleteCmd := NewDeleteCmd(NewTestClient(fn.WithRemovers(remover))) + deleteCmd.SetArgs([]string{}) + if err := deleteCmd.Execute(); err != nil { + t.Fatal(err) + } + + // Flag-less redeploy: must reuse the persisted keda deployer. + deployer := mock.NewDeployer() + redeployCmd := NewDeployCmd(NewTestClient(fn.WithDeployer(deployer))) + redeployCmd.SetArgs([]string{}) + if err := redeployCmd.Execute(); err != nil { + t.Fatalf("expected a flag-less redeploy after delete to succeed, got: %v", err) + } + if !deployer.DeployInvoked { + t.Fatal("expected the deployer to be invoked on the redeploy") + } + + loaded, err := fn.NewFunction(root) + if err != nil { + t.Fatal(err) + } + if loaded.Deploy.Deployer != keda.KedaDeployerName { + t.Fatalf("expected the flag-less redeploy to reuse the persisted %q deployer, got %q", keda.KedaDeployerName, loaded.Deploy.Deployer) + } +} + +// TestDelete_ByProjectThenRedeployWithDifferentDeployerNotBlocked ensures that +// undeploy removes "deployed status" effectively unblocking the subsequent +// re-deployment with different deployer +func TestDelete_ByProjectThenRedeployWithDifferentDeployerNotBlocked(t *testing.T) { + root := FromTempDirectory(t) + f := fn.Function{ + Root: root, + Runtime: "go", + Registry: TestRegistry, + Deploy: fn.DeploySpec{Namespace: "myns", Deployer: keda.KedaDeployerName}, + } + f, err := fn.New().Init(f) + if err != nil { + t.Fatal(err) + } + if err := f.Write(); err != nil { + t.Fatal(err) + } + + remover := mock.NewRemover() + remover.RemoveFn = func(_, _ string) error { return nil } + deleteCmd := NewDeleteCmd(NewTestClient(fn.WithRemovers(remover))) + deleteCmd.SetArgs([]string{}) + if err := deleteCmd.Execute(); err != nil { + t.Fatal(err) + } + + deployer := mock.NewDeployer() + deployCmd := NewDeployCmd(NewTestClient(fn.WithDeployer(deployer))) + deployCmd.SetArgs([]string{"--deployer", deployers.Kubernetes}) + if err := deployCmd.Execute(); err != nil { + t.Fatalf("expected the redeploy with a different deployer to succeed after undeploy, got: %v", err) + } + if !deployer.DeployInvoked { + t.Fatal("expected the deployer to be invoked - the guard must not fire on an undeployed function") + } +} diff --git a/cmd/deploy.go b/cmd/deploy.go index 540c482639..50ec7647c6 100644 --- a/cmd/deploy.go +++ b/cmd/deploy.go @@ -16,6 +16,7 @@ import ( "knative.dev/func/cmd/common" "knative.dev/func/pkg/builders" "knative.dev/func/pkg/config" + "knative.dev/func/pkg/deployers" fn "knative.dev/func/pkg/functions" "knative.dev/func/pkg/k8s" "knative.dev/func/pkg/keda" @@ -159,6 +160,8 @@ EXAMPLES // contextually relevant function; but sets are flattened via cfg.Apply(f) cmd.Flags().StringP("builder", "b", cfg.Builder, fmt.Sprintf("Builder to use when creating the function's container. Currently supported builders are %s.", KnownBuilders())) + cmd.Flags().String("deployer", cfg.Deployer, + fmt.Sprintf("Type of deployment to use: '%s' for Knative Service, '%s' for Kubernetes Deployment, or '%s' for Deployment with a KEDA HTTP scaler ($FUNC_DEPLOYER)", deployers.Knative, deployers.Kubernetes, deployers.Keda)) cmd.Flags().StringP("registry", "r", cfg.Registry, "Container registry + registry namespace. (ex 'ghcr.io/myuser'). The full image name is automatically determined using this along with function name. ($FUNC_REGISTRY)") cmd.Flags().Bool("registry-insecure", cfg.RegistryInsecure, "Skip TLS certificate verification when communicating in HTTPS with the registry. The value is persisted over consecutive runs ($FUNC_REGISTRY_INSECURE)") @@ -197,8 +200,6 @@ EXAMPLES "Service account to be used in the deployed function ($FUNC_SERVICE_ACCOUNT)") cmd.Flags().String("image-pull-secret", f.Deploy.ImagePullSecret, "Image pull secret to use when the function's image is in a private registry ($FUNC_IMAGE_PULL_SECRET)") - cmd.Flags().String("deployer", f.Deploy.Deployer, - fmt.Sprintf("Type of deployment to use: '%s' for Knative Service (default), '%s' for Kubernetes Deployment or '%s' for Deployment with a Keda HTTP scaler ($FUNC_DEPLOY_TYPE)", knative.KnativeDeployerName, k8s.KubernetesDeployerName, keda.KedaDeployerName)) // Static Flags: // Options which have static defaults only (not globally configurable nor // persisted with the function) @@ -284,6 +285,12 @@ func runDeploy(cmd *cobra.Command, newClient ClientFactory) (err error) { // Warn if registry changed but registryInsecure is still true warnRegistryInsecureChange(cmd.OutOrStderr(), cfg.Registry, f) + // Back-compat: a function deployed before the deployer was recorded has a + // namespace but no deployer, which historically could only mean knative. + if f.Deploy.Namespace != "" && f.Deploy.Deployer == "" { + f.Deploy.Deployer = deployers.Knative + } + if f, err = cfg.Configure(f); err != nil { // Updates f with deploy cfg return } @@ -620,7 +627,7 @@ func (c deployConfig) Configure(f fn.Function) (fn.Function, error) { f.Build.RemoteStorageClass = c.RemoteStorageClass f.Deploy.ServiceAccountName = c.ServiceAccountName f.Deploy.ImagePullSecret = c.ImagePullSecret - f.Deploy.Deployer = c.Deployer + f.Deployer = c.Deployer f.Deploy.ManagementDisabled = c.ManagementDisabled f.Local.Remote = c.Remote @@ -807,13 +814,8 @@ func (c deployConfig) clientOptions() ([]fn.Option, error) { // This is needed for remote builds (deploy --remote) o = append(o, fn.WithPipelinesProvider(newTektonPipelinesProvider(creds, c.Verbose, t))) - // Add the appropriate deployer based on deploy type - deployer := c.Deployer - if deployer == "" { - deployer = knative.KnativeDeployerName // default to knative for backwards compatibility - } - - switch deployer { + // Add the appropriate deployer based on deploy type. + switch c.Deployer { case knative.KnativeDeployerName: o = append(o, fn.WithDeployer(newKnativeDeployer(c.Verbose))) case k8s.KubernetesDeployerName: @@ -821,7 +823,7 @@ func (c deployConfig) clientOptions() ([]fn.Option, error) { case keda.KedaDeployerName: o = append(o, fn.WithDeployer(newKedaDeployer(c.Verbose))) default: - return o, fmt.Errorf("unsupported deploy type: %s (supported: %s, %s, %s)", deployer, knative.KnativeDeployerName, k8s.KubernetesDeployerName, keda.KedaDeployerName) + return o, fmt.Errorf("unsupported deploy type: %s (supported: %s, %s, %s)", c.Deployer, knative.KnativeDeployerName, k8s.KubernetesDeployerName, keda.KedaDeployerName) } return o, nil diff --git a/cmd/deploy_test.go b/cmd/deploy_test.go index 540cb4e15f..0b2446b1fe 100644 --- a/cmd/deploy_test.go +++ b/cmd/deploy_test.go @@ -14,8 +14,11 @@ import ( "github.com/spf13/cobra" "knative.dev/func/pkg/builders" + "knative.dev/func/pkg/config" + "knative.dev/func/pkg/deployers" fn "knative.dev/func/pkg/functions" "knative.dev/func/pkg/k8s" + "knative.dev/func/pkg/keda" "knative.dev/func/pkg/mock" . "knative.dev/func/pkg/testing" ) @@ -2547,3 +2550,182 @@ func TestDeploy_ValidDomain(t *testing.T) { func TestDeploy_RegistryInsecurePersists(t *testing.T) { testRegistryInsecurePersists(NewDeployCmd, t) } + +// TestDeploy_DeployerPersists ensures the effective deployer is always +// persisted as a concrete value, static-default aware: read deployers.Default +func TestDeploy_DeployerPersists(t *testing.T) { + t.Run("no --deployer on a fresh function persists the concrete default", func(t *testing.T) { + root := FromTempDirectory(t) + f := fn.Function{Runtime: "go", Root: root, Registry: TestRegistry} + if _, err := fn.New().Init(f); err != nil { + t.Fatal(err) + } + + cmd := NewDeployCmd(NewTestClient(fn.WithDeployer(mock.NewDeployer()))) + dflt := cmd.Flags().Lookup("deployer").DefValue + if dflt != deployers.Default { + t.Fatalf("expected flag default value %q, got %q", deployers.Default, dflt) + } + + cmd.SetArgs([]string{"--namespace", "myns"}) + if err := cmd.Execute(); err != nil { + t.Fatal(err) + } + + loaded, err := fn.NewFunction(root) + if err != nil { + t.Fatal(err) + } + if loaded.Deploy.Deployer != deployers.Default { + t.Fatalf("expected persisted deployer %q, got %q", deployers.Default, loaded.Deploy.Deployer) + } + }) + + t.Run("explicit non-default deployer persists and survives a flag-less redeploy unblocked", func(t *testing.T) { + root := FromTempDirectory(t) + f := fn.Function{Runtime: "go", Root: root, Registry: TestRegistry} + if _, err := fn.New().Init(f); err != nil { + t.Fatal(err) + } + + // Choose the deployer that is not the default + var other string + if deployers.Default == deployers.Knative { + other = deployers.Kubernetes + } else { + other = deployers.Knative + } + + cmd := NewDeployCmd(NewTestClient(fn.WithDeployer(mock.NewDeployer()))) + cmd.SetArgs([]string{"--deployer", other, "--namespace", "myns"}) + if err := cmd.Execute(); err != nil { + t.Fatal(err) + } + + loaded, err := fn.NewFunction(root) + if err != nil { + t.Fatal(err) + } + if loaded.Deploy.Deployer != other { + t.Fatalf("expected persisted deployer %q, got %q", other, loaded.Deploy.Deployer) + } + + // no --deployer flag: the flag defaults to the persisted value + deployer := mock.NewDeployer() + cmd = NewDeployCmd(NewTestClient(fn.WithDeployer(deployer))) + cmd.SetArgs([]string{}) + if err := cmd.Execute(); err != nil { + t.Fatalf("expected a flag-less redeploy to retain the persisted deployer unblocked, got: %v", err) + } + if !deployer.DeployInvoked { + t.Fatal("expected the deployer to be invoked for the flag-less redeploy") + } + + loaded, err = fn.NewFunction(root) + if err != nil { + t.Fatal(err) + } + if loaded.Deploy.Deployer != other { + t.Fatalf("expected deployer to remain %q after a flag-less redeploy, got %q", other, loaded.Deploy.Deployer) + } + }) +} + +// TestDeploy_DeployerGlobalConfig ensures the deployer flag's +// default is sourced from the global config chain, same as builder. +func TestDeploy_DeployerGlobalConfig(t *testing.T) { + root := FromTempDirectory(t) + + if err := config.CreatePaths(); err != nil { + t.Fatal(err) + } + globalCfg := config.Global{Deployer: k8s.KubernetesDeployerName} + if err := globalCfg.Write(config.File()); err != nil { + t.Fatal(err) + } + + f := fn.Function{Runtime: "go", Root: root, Registry: TestRegistry} + if _, err := fn.New().Init(f); err != nil { + t.Fatal(err) + } + + cmd := NewDeployCmd(NewTestClient(fn.WithDeployer(mock.NewDeployer()))) + cmd.SetArgs([]string{"--namespace", "myns"}) + if err := cmd.Execute(); err != nil { + t.Fatal(err) + } + + loaded, err := fn.NewFunction(root) + if err != nil { + t.Fatal(err) + } + if loaded.Deploy.Deployer != k8s.KubernetesDeployerName { + t.Fatalf("expected the global config's deployer %q to seed the flagless deploy, got %q", k8s.KubernetesDeployerName, loaded.Deploy.Deployer) + } +} + +// TestDeploy_DeployerSwitch pins the CLI-only part of the switch guard. +func TestDeploy_DeployerSwitch(t *testing.T) { + for _, tt := range []struct { + name string + deployedDep string // Deploy.Deployer already deployed ("" = legacy) + requested string // --deployer flag value + wantBlocked bool + }{ + // A blocked and an allowed switch, proving the guard is reached and its + // error surfaced through the full CLI path (flag -> config -> client). + {"keda2raw blocked", keda.KedaDeployerName, k8s.KubernetesDeployerName, true}, + {"raw2keda safe switch", k8s.KubernetesDeployerName, keda.KedaDeployerName, false}, + // Legacy (deployed before the deployer was persisted, so empty): the CLI + // treats it as knative, so an explicit knative is allowed but any other + // deployer is blocked. This normalization is the CLI-only bit here. + {"legacy empty is treated as knative", "", deployers.Knative, false}, + {"legacy empty to raw is blocked", "", k8s.KubernetesDeployerName, true}, + // A function deployed by an older binary has its deployer only in state, + // not intent. A flag-less redeploy must reuse it. + {"legacy keda reused on flag-less redeploy", keda.KedaDeployerName, "", false}, + } { + t.Run(tt.name, func(t *testing.T) { + root := FromTempDirectory(t) + f := fn.Function{ + Runtime: "go", + Root: root, + Registry: TestRegistry, + // Namespace set == already deployed, which is what the guard gates on. + Deploy: fn.DeploySpec{Namespace: "myns", Deployer: tt.deployedDep}, + } + if _, err := fn.New().Init(f); err != nil { + t.Fatal(err) + } + + deployer := mock.NewDeployer() + cmd := NewDeployCmd(NewTestClient(fn.WithDeployer(deployer))) + args := []string{} + if tt.requested != "" { + args = append(args, "--deployer", tt.requested) + } + cmd.SetArgs(args) + + err := cmd.Execute() + + if tt.wantBlocked { + if err == nil { + t.Fatalf("expected switch %q -> %q to be blocked", tt.deployedDep, tt.requested) + } + if !strings.Contains(err.Error(), "func delete") { + t.Fatalf("expected the error to point at 'func delete', got: %v", err) + } + if deployer.DeployInvoked { + t.Fatal("expected the deployer NOT to run on a blocked switch") + } + return + } + if err != nil { + t.Fatalf("expected switch %q -> %q to be allowed, got: %v", tt.deployedDep, tt.requested, err) + } + if !deployer.DeployInvoked { + t.Fatal("expected the deployer to run when the switch is allowed") + } + }) + } +} diff --git a/cmd/func-util/main.go b/cmd/func-util/main.go index 360fcf38a7..7a9dcaf48c 100644 --- a/cmd/func-util/main.go +++ b/cmd/func-util/main.go @@ -153,11 +153,17 @@ func deploy(ctx context.Context) error { if f.Deploy.Image == "" { f.Deploy.Image = f.Image } - if f.Deploy.Deployer == "" { - f.Deploy.Deployer = knative.KnativeDeployerName + // Resolve the deployer. Mirrors config.Apply on the CLI so a remote deploy + // honors --deployer, which travels in func.yaml as intent. + deployer := f.Deployer + if deployer == "" { + deployer = f.Deploy.Deployer + } + if deployer == "" { + deployer = knative.KnativeDeployerName } var d fn.Deployer - switch f.Deploy.Deployer { + switch deployer { case knative.KnativeDeployerName: d = knative.NewDeployer( knative.WithDeployerDecorator(deployDecorator{}), @@ -174,7 +180,7 @@ func deploy(ctx context.Context) error { keda.WithDeployerVerbose(true), ) default: - return fmt.Errorf("unknown deployer: %s", f.Deploy.Deployer) + return fmt.Errorf("unknown deployer: %s", deployer) } client := fn.New(fn.WithDeployer(d)) diff --git a/docs/reference/func_delete.md b/docs/reference/func_delete.md index c22a87a692..de3587951c 100644 --- a/docs/reference/func_delete.md +++ b/docs/reference/func_delete.md @@ -10,7 +10,8 @@ This command undeploys a function from the cluster. By default the function from the project in the current directory is undeployed. Alternatively either the name of the function can be given as argument or the project path provided with --path. -No local files are deleted. +No local files are deleted. When undeploying by project (not by name), on success +the local func.yaml is updated to reflect that the function is no longer deployed. ``` diff --git a/docs/reference/func_deploy.md b/docs/reference/func_deploy.md index 81880ce1c4..b8257073ad 100644 --- a/docs/reference/func_deploy.md +++ b/docs/reference/func_deploy.md @@ -119,7 +119,7 @@ func deploy -b, --builder string Builder to use when creating the function's container. Currently supported builders are "host", "pack" and "s2i". (default "pack") --builder-image string Specify a custom builder image for use by the builder other than its default. ($FUNC_BUILDER_IMAGE) -c, --confirm Prompt to confirm options interactively ($FUNC_CONFIRM) - --deployer string Type of deployment to use: 'knative' for Knative Service (default), 'raw' for Kubernetes Deployment or 'keda' for Deployment with a Keda HTTP scaler ($FUNC_DEPLOY_TYPE) + --deployer string Type of deployment to use: 'knative' for Knative Service, 'raw' for Kubernetes Deployment, or 'keda' for Deployment with a KEDA HTTP scaler ($FUNC_DEPLOYER) (default "knative") --domain string Domain to use for the function's route. Cluster must be configured with domain matching for the given domain (ignored if unrecognized) ($FUNC_DOMAIN) -e, --env stringArray Environment variable to set in the form NAME=VALUE. You may provide this flag multiple times for setting multiple environment variables. To unset, specify the environment variable name followed by a "-" (e.g., NAME-). -t, --git-branch string Git revision (branch) to be used when deploying via the Git repository ($FUNC_GIT_BRANCH) diff --git a/pkg/config/config.go b/pkg/config/config.go index 0b8dddd9e7..00d7e180a2 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -11,6 +11,7 @@ import ( "gopkg.in/yaml.v2" "knative.dev/func/pkg/builders" + "knative.dev/func/pkg/deployers" fn "knative.dev/func/pkg/functions" "knative.dev/func/pkg/k8s" ) @@ -27,12 +28,16 @@ const ( // DefaultBuilder is statically defined by the builders package. DefaultBuilder = builders.Default + + // DefaultDeployer is statically defined by the deployers package. + DefaultDeployer = deployers.Default ) // Global configuration settings. type Global struct { Builder string `yaml:"builder,omitempty"` Confirm bool `yaml:"confirm,omitempty"` + Deployer string `yaml:"deployer,omitempty"` Language string `yaml:"language,omitempty"` Namespace string `yaml:"namespace,omitempty"` Registry string `yaml:"registry,omitempty"` @@ -49,12 +54,13 @@ type Global struct { func New() Global { return Global{ Builder: DefaultBuilder, + Deployer: DefaultDeployer, Language: DefaultLanguage, // ... } } -// RegistyDefault is a convenience method for deferred calculation of a +// RegistryDefault is a convenience method for deferred calculation of a // default registry taking into account both the global config file and cluster // detection. func (c Global) RegistryDefault() string { @@ -126,6 +132,14 @@ func (c Global) Apply(f fn.Function) Global { if f.Build.Builder != "" { c.Builder = f.Build.Builder } + // opt 1: last deployed deployer + if f.Deploy.Deployer != "" { + c.Deployer = f.Deploy.Deployer + } + // opt 2: intent to deploy deployer + if f.Deployer != "" { + c.Deployer = f.Deployer + } if f.Runtime != "" { c.Language = f.Runtime } @@ -151,6 +165,9 @@ func (c Global) Configure(f fn.Function) fn.Function { if c.Builder != "" { f.Build.Builder = c.Builder } + if c.Deployer != "" { + f.Deployer = c.Deployer + } if c.Language != "" { f.Runtime = c.Language } diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 1e3dd287b8..81f9c1962a 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -373,6 +373,7 @@ func TestList(t *testing.T) { expected := []string{ "builder", "confirm", + "deployer", "language", "namespace", "registry", diff --git a/pkg/deployer/testing/integration_test_helper.go b/pkg/deployer/testing/integration_test_helper.go index 379cad6a73..1c5b9158c3 100644 --- a/pkg/deployer/testing/integration_test_helper.go +++ b/pkg/deployer/testing/integration_test_helper.go @@ -96,7 +96,7 @@ func TestInt_Deploy(t *testing.T, deployer fn.Deployer, remover fn.Remover, desc t.Fatal(err) } t.Cleanup(func() { - err := client.Remove(ctx, "", "", f, true) + _, err := client.Remove(ctx, "", "", f, true) if err != nil { t.Logf("error removing Function: %v", err) } @@ -236,7 +236,7 @@ func TestInt_Metadata(t *testing.T, deployer fn.Deployer, remover fn.Remover, de t.Fatal(err) } t.Cleanup(func() { - err := client.Remove(ctx, "", "", f, true) + _, err := client.Remove(ctx, "", "", f, true) if err != nil { t.Logf("error removing Function: %v", err) } @@ -357,7 +357,7 @@ func TestInt_Events(t *testing.T, deployer fn.Deployer, remover fn.Remover, desc t.Fatal(err) } t.Cleanup(func() { - err := client.Remove(ctx, "", "", f, true) + _, err := client.Remove(ctx, "", "", f, true) if err != nil { t.Logf("error removing Function: %v", err) } @@ -441,7 +441,7 @@ func TestInt_Scale(t *testing.T, deployer fn.Deployer, remover fn.Remover, descr t.Fatal(err) } t.Cleanup(func() { - err := client.Remove(ctx, "", "", f, true) + _, err := client.Remove(ctx, "", "", f, true) if err != nil { t.Logf("error removing Function: %v", err) } @@ -560,7 +560,7 @@ func TestInt_EnvsUpdate(t *testing.T, deployer fn.Deployer, remover fn.Remover, t.Fatal(err) } t.Cleanup(func() { - err := client.Remove(ctx, "", "", f, true) + _, err := client.Remove(ctx, "", "", f, true) if err != nil { t.Logf("error removing Function: %v", err) } @@ -976,7 +976,8 @@ func TestInt_ResourceValidationOnFirstDeploy(t *testing.T, deployer fn.Deployer, t.Fatalf("expected deploy to succeed after Secret was created, got: %v", err) } t.Cleanup(func() { - if err := client.Remove(ctx, "", "", f, true); err != nil { + _, err = client.Remove(ctx, "", "", f, true) + if err != nil { t.Logf("error removing Function: %v", err) } }) @@ -1263,7 +1264,7 @@ func TestInt_OperatorSync(t *testing.T, deployer fn.Deployer, remover fn.Remover t.Fatal(err) } t.Cleanup(func() { - err := client.Remove(ctx, "", "", f, true) + _, err := client.Remove(ctx, "", "", f, true) if err != nil { t.Logf("error removing Function: %v", err) } diff --git a/pkg/deployers/deployers.go b/pkg/deployers/deployers.go new file mode 100644 index 0000000000..a24af9302d --- /dev/null +++ b/pkg/deployers/deployers.go @@ -0,0 +1,32 @@ +/* +Package deployers provides canonical short names for the function +deployer implementations. +*/ +package deployers + +import "fmt" + +const ( + Knative = "knative" + Kubernetes = "raw" + Keda = "keda" + + // Default deployer absent any other configuration. + Default = Knative +) + +// ValidateSwitch reports an error if redeploying an already-deployed function +// with deployer 'to' would strand the previous deployer's resources on the +// cluster. The only safe cross-deployer change is raw -> keda, because the keda +// deployer embeds the raw one; same-deployer redeploys are always allowed. +// 'from' is the deployer the function is currently deployed with. 'to' is the +// one to deploy to. An empty value on either side means "not known" -> returns nil. +func ValidateSwitch(from, to string) error { + if from == "" || to == "" { + return nil + } + if from == to || (from == Kubernetes && to == Keda) { + return nil + } + return fmt.Errorf("function was deployed with the %q deployer; redeploying with %q would orphan the old deployer's resources on the cluster - run func delete first to remove them, then redeploy", from, to) +} diff --git a/pkg/deployers/deployers_test.go b/pkg/deployers/deployers_test.go new file mode 100644 index 0000000000..053e5ece83 --- /dev/null +++ b/pkg/deployers/deployers_test.go @@ -0,0 +1,39 @@ +package deployers + +import "testing" + +// TestValidateSwitch covers the deployer-switch policy: the same deployer is +// always allowed, raw -> keda is the one safe cross-switch (keda embeds raw), +// and every other change is blocked. The undeployed case (any deployer allowed) +// is the caller's responsibility and is covered by the cmd-level deploy tests. +func TestValidateSwitch(t *testing.T) { + for _, tt := range []struct { + name string + from string + to string + wantErr bool + }{ + {"same deployer is a no-op", Keda, Keda, false}, + {"raw to keda is the one safe switch", Kubernetes, Keda, false}, + {"keda to raw is blocked", Keda, Kubernetes, true}, + {"knative to raw is blocked", Knative, Kubernetes, true}, + {"knative to keda is blocked", Knative, Keda, true}, + {"keda to knative is blocked", Keda, Knative, true}, + // An empty deployer means "not known", not "a deployer named empty": + // no switch can be established, so none is reported. Guards library + // callers, which have no CLI to resolve either side for them. + {"unknown deployed-with is not a switch", "", Keda, false}, + {"unknown requested is not a switch", Keda, "", false}, + {"both unknown is not a switch", "", "", false}, + } { + t.Run(tt.name, func(t *testing.T) { + err := ValidateSwitch(tt.from, tt.to) + if tt.wantErr && err == nil { + t.Fatalf("expected %q->%q to be blocked, got nil", tt.from, tt.to) + } + if !tt.wantErr && err != nil { + t.Fatalf("expected %q->%q to be allowed, got: %v", tt.from, tt.to, err) + } + }) + } +} diff --git a/pkg/describer/testing/integration_test_helper.go b/pkg/describer/testing/integration_test_helper.go index 7e5db43d37..45638f86fb 100644 --- a/pkg/describer/testing/integration_test_helper.go +++ b/pkg/describer/testing/integration_test_helper.go @@ -67,7 +67,7 @@ func TestInt_Describe(t *testing.T, describer fn.Describer, deployer fn.Deployer t.Fatal(err) } t.Cleanup(func() { - err := client.Remove(ctx, "", "", f, true) + _, err := client.Remove(ctx, "", "", f, true) if err != nil { t.Logf("error removing Function: %v", err) } diff --git a/pkg/functions/client.go b/pkg/functions/client.go index a7f45c4254..a4b2902ff9 100644 --- a/pkg/functions/client.go +++ b/pkg/functions/client.go @@ -19,6 +19,7 @@ import ( "golang.org/x/sync/errgroup" "gopkg.in/yaml.v2" + "knative.dev/func/pkg/deployers" "knative.dev/func/pkg/utils" ) @@ -121,6 +122,7 @@ type DeploymentResult struct { Status Status URL string Namespace string + Deployer string } // Status of the function from the DeploymentResult @@ -861,6 +863,18 @@ func (c *Client) Deploy(ctx context.Context, f Function, oo ...DeployOption) (Fu return f, ErrNameRequired } + // Deployer switch gate - changing deployers when function is currently + // deployed would leave stranded resources on cluster. We error clearly + // and expect the user to undeploy first, which removes the resources + // correctly. + // One special case is raw -> keda switch which works because keda embeds + // 'raw' deployer, working with the same resources. + if f.Deploy.Namespace != "" { + if err := deployers.ValidateSwitch(f.Deploy.Deployer, f.Deployer); err != nil { + return f, fmt.Errorf("function %q: %w", f.Name, err) + } + } + // Warn if moving changingNamespace := func(f Function) bool { // We're changing namespace if: @@ -878,7 +892,7 @@ func (c *Client) Deploy(ctx context.Context, f Function, oo ...DeployOption) (Fu // 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) + _, err := c.Remove(ctx, "", "", f, true) if err != nil { // Warn when service is not found and set err to nil to continue. Function's // service might have been manually deleted prior to the subsequent deploy or the @@ -901,6 +915,7 @@ func (c *Client) Deploy(ctx context.Context, f Function, oo ...DeployOption) (Fu } // Update the function to reflect the new deployed state of the Function f.Deploy.Namespace = result.Namespace + f.Deploy.Deployer = result.Deployer switch result.Status { case Deployed: @@ -1150,8 +1165,12 @@ func (c *Client) List(ctx context.Context, namespace string) ([]ListItem, error) // 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 pipelines.Remove -func (c *Client) Remove(ctx context.Context, name, namespace string, f Function, all bool) error { +// in the sub functions remover.Remove and pipelines.Remove. +// +// Returns structure 'f' with f.Deploy.Namespace & f.Deploy.Deployer cleared if +// the removal was successful and error returned is nil. If error was +// encountered, returns 'f' unmodified. +func (c *Client) Remove(ctx context.Context, name, namespace string, f Function, all bool) (Function, error) { // Default to name/namespace, fallback to passed Function if name == "" { name = f.Name @@ -1160,10 +1179,10 @@ func (c *Client) Remove(ctx context.Context, name, namespace string, f Function, // Preconditions if name == "" { - return ErrNameRequired + return f, ErrNameRequired } if namespace == "" { - return ErrNamespaceRequired + return f, ErrNamespaceRequired } // Logging @@ -1183,8 +1202,6 @@ func (c *Client) Remove(ctx context.Context, name, namespace string, f Function, serviceRemovalErrGroup := &errgroup.Group{} var removeHandled atomic.Bool for _, remover := range c.removers { - remover := remover - serviceRemovalErrGroup.Go(func() error { err := remover.Remove(ctx, name, namespace) if err != nil { @@ -1209,11 +1226,11 @@ func (c *Client) Remove(ctx context.Context, name, namespace string, f Function, serviceRemovalError := serviceRemovalErrGroup.Wait() if serviceRemovalError == nil && resourceRemovalError == nil && !removeHandled.Load() { // no error, but resource was not handled by any of the removers - return fmt.Errorf("no remover handled %s in %s", name, namespace) + return f, fmt.Errorf("no remover handled %s in %s", name, namespace) } // Return a combined error - return func(e1, e2 error) error { + combinedErr := func(e1, e2 error) error { if e1 == nil && e2 == nil { return nil } @@ -1225,6 +1242,15 @@ func (c *Client) Remove(ctx context.Context, name, namespace string, f Function, } return e2 }(serviceRemovalError, resourceRemovalError) + + if combinedErr == nil { + // Function was undeployed successfully. The user's INTENT (the top-level + // Function.Deployer and Function.Namespace) is untouched and is what a + // subsequent deploy reuses. + f.Deploy.Namespace = "" + f.Deploy.Deployer = "" + } + return f, combinedErr } // Invoke is a convenience method for triggering the execution of a function diff --git a/pkg/functions/client_int_test.go b/pkg/functions/client_int_test.go index b474e57621..258dc7a70c 100644 --- a/pkg/functions/client_int_test.go +++ b/pkg/functions/client_int_test.go @@ -559,7 +559,7 @@ func (f *Function) Handle(w http.ResponseWriter, req *http.Request) { if route, f, err = client2.Apply(ctx, f); err != nil { t.Fatal(err) } - defer func() { _ = client2.Remove(ctx, "", "", f, true) }() + defer func() { _, _ = client2.Remove(ctx, "", "", f, true) }() resp, err := http.Get(route) if err != nil { @@ -710,7 +710,8 @@ func del(t *testing.T, c *fn.Client, name, namespace string) { t.Helper() waitFor(t, c, name, namespace) f := fn.Function{Name: name, Deploy: fn.DeploySpec{Namespace: DefaultIntTestNamespace}} - if err := c.Remove(t.Context(), "", "", f, false); err != nil { + _, err := c.Remove(t.Context(), "", "", f, false) + if err != nil { t.Fatal(err) } diff --git a/pkg/functions/client_test.go b/pkg/functions/client_test.go index d229c2e0fd..4cc49242ec 100644 --- a/pkg/functions/client_test.go +++ b/pkg/functions/client_test.go @@ -20,6 +20,7 @@ import ( cloudevents "github.com/cloudevents/sdk-go/v2" "knative.dev/func/pkg/builders" + "knative.dev/func/pkg/deployers" fn "knative.dev/func/pkg/functions" "knative.dev/func/pkg/mock" "knative.dev/func/pkg/oci" @@ -1091,7 +1092,7 @@ func TestClient_Remove_ByPath(t *testing.T) { return nil } - if err := client.Remove(t.Context(), "", "", f, false); err != nil { + if _, err := client.Remove(t.Context(), "", "", f, false); err != nil { t.Fatal(err) } @@ -1134,7 +1135,7 @@ func TestClient_Remove_DeleteAll(t *testing.T) { return nil } - if err := client.Remove(t.Context(), "", "", f, deleteAll); err != nil { + if _, err := client.Remove(t.Context(), "", "", f, deleteAll); err != nil { t.Fatal(err) } @@ -1181,7 +1182,7 @@ func TestClient_Remove_Dont_DeleteAll(t *testing.T) { return nil } - if err := client.Remove(t.Context(), "", "", f, deleteAll); err != nil { + if _, err := client.Remove(t.Context(), "", "", f, deleteAll); err != nil { t.Fatal(err) } @@ -1223,12 +1224,12 @@ func TestClient_Remove_ByName(t *testing.T) { } // Run remove with name (and namespace in .Deploy to simulate deployed function) - if err := client.Remove(t.Context(), "", "", fn.Function{Name: expectedName, Deploy: fn.DeploySpec{Namespace: namespace}}, false); err != nil { + if _, err := client.Remove(t.Context(), "", "", 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(t.Context(), "", "", fn.Function{Name: expectedName, Root: root, Deploy: fn.DeploySpec{Namespace: namespace}}, false); err != nil { + if _, err := client.Remove(t.Context(), "", "", fn.Function{Name: expectedName, Root: root, Deploy: fn.DeploySpec{Namespace: namespace}}, false); err != nil { t.Fatal(err) } @@ -1259,11 +1260,73 @@ func TestClient_Remove_UninitializedFails(t *testing.T) { fn.WithRemovers(remover)) // Attempt to remove by path (uninitialized), expecting an error. - if err := client.Remove(t.Context(), "", "", fn.Function{Root: root}, false); err == nil { + if _, err := client.Remove(t.Context(), "", "", fn.Function{Root: root}, false); err == nil { t.Fatalf("did not received expected error removing an uninitialized func") } } +// TestClient_Remove_ReturnsReconciledFunction asserts both halves of Remove's +// (Function, error) contract: on success the returned function has its observed +// STATE cleared (Deploy.Namespace and Deploy.Deployer) while the user's INTENT +// (top-level Deployer/Namespace) survives; on a remover error nothing is touched +// and the error propagates. The clear is guarded to happen only on success. +func TestClient_Remove_ReturnsReconciledFunction(t *testing.T) { + const deployer = "keda" // arbitrary; never interpreted + + newFn := func() fn.Function { + return fn.Function{ + Name: "fn", + Deployer: deployer, // intent + Deploy: fn.DeploySpec{Namespace: "ns", Deployer: deployer}, // state + } + } + + t.Run("success clears deployed state and preserves intent", func(t *testing.T) { + remover := mock.NewRemover() + remover.RemoveFn = func(_, _ string) error { return nil } + client := fn.New(fn.WithRemovers(remover)) + + //remove by-project + got, err := client.Remove(t.Context(), "", "", newFn(), false) + if err != nil { + t.Fatal(err) + } + // removes the deployed markers + if got.Deploy.Namespace != "" { + t.Fatalf("expected Deploy.Namespace cleared on success, got %q", got.Deploy.Namespace) + } + if got.Deploy.Deployer != "" { + t.Fatalf("expected Deploy.Deployer cleared on success, got %q", got.Deploy.Deployer) + } + // keeps the intent + if got.Deployer != deployer { + t.Fatalf("expected the intended Deployer preserved as %q, got %q", deployer, got.Deployer) + } + }) + + t.Run("remover error leaves the function untouched and propagates", func(t *testing.T) { + remover := mock.NewRemover() + // simulate an error + remover.RemoveFn = func(_, _ string) error { return fmt.Errorf("remover boom") } + client := fn.New(fn.WithRemovers(remover)) + + got, err := client.Remove(t.Context(), "", "", newFn(), false) + if err == nil { + t.Fatal("expected the remover error to propagate, got nil") + } + // expect unchanged deployed markers + if got.Deploy.Namespace != "ns" { + t.Fatalf("expected Deploy.Namespace preserved on failure, got %q", got.Deploy.Namespace) + } + if got.Deploy.Deployer != deployer { + t.Fatalf("expected Deploy.Deployer untouched on failure, got %q", got.Deploy.Deployer) + } + if got.Deployer != deployer { + t.Fatalf("expected the intended Deployer untouched on failure, got %q", got.Deployer) + } + }) +} + // TestClient_List merely ensures that the client invokes the configured lister. func TestClient_List(t *testing.T) { lister := mock.NewLister() @@ -2513,3 +2576,104 @@ func absPath(p string) string { } return abs } + +// TestClient_Deploy_BlocksDeployerSwitch ensures the deployer-switch guard is +// enforced by the client itself, so every API consumer is protected, not only +// the CLI. Redeploying an already-deployed function with a different deployer +// would strand the previous deployer's resources; raw -> keda is the one safe +// change because the keda deployer embeds the raw one. +// +// A blocked switch must also fail BEFORE the deployer runs: the point of the +// guard is that nothing on the cluster is touched. +func TestClient_Deploy_BlocksDeployerSwitch(t *testing.T) { + for _, tt := range []struct { + name string + deployedWith string // observed state: Deploy.Deployer + requested string // intent: Deployer + deployedNS string // observed state: Deploy.Namespace ("" = not deployed) + wantBlocked bool + }{ + {"keda2raw blocked", deployers.Keda, deployers.Kubernetes, "ns", true}, + {"knative2keda blocked", deployers.Knative, deployers.Keda, "ns", true}, + {"raw2keda is safe switch", deployers.Kubernetes, deployers.Keda, "ns", false}, + {"same deployer is not a switch", deployers.Keda, deployers.Keda, "ns", false}, + + {"undeployed is never blocked", "", deployers.Keda, "", false}, + // It is the client's caller's responsibility to make sure the deployer + // is set (stated specifically for the legacy unpersisted deployer key) + {"unknown deployed-with is not blocked", "", deployers.Kubernetes, "ns", false}, + {"unknown requested is not blocked", deployers.Keda, "", "ns", false}, + } { + t.Run(tt.name, func(t *testing.T) { + deployer := mock.NewDeployer() + client := fn.New(fn.WithDeployer(deployer)) + + f := fn.Function{ + Name: "f", + // Target ns (intent) and it is Deploy.Namespace (state) below + // that decides whether the guard considers f already deployed. + Namespace: "ns", + Deployer: tt.requested, + Deploy: fn.DeploySpec{ + Namespace: tt.deployedNS, + Deployer: tt.deployedWith, + }, + } + + _, err := client.Deploy(t.Context(), f, fn.WithDeploySkipBuildCheck(true)) + + if tt.wantBlocked { + if err == nil { + t.Fatalf("expected %q -> %q to be blocked, got nil", tt.deployedWith, tt.requested) + } + if deployer.DeployInvoked { + t.Fatal("expected the deployer NOT to run on a blocked switch") + } + return + } + if err != nil { + t.Fatalf("expected %q -> %q to be allowed, got: %v", tt.deployedWith, tt.requested, err) + } + if !deployer.DeployInvoked { + t.Fatal("expected the deployer to run when the switch is allowed") + } + }) + } +} + +// TestClient_Deploy_PersistsSelfReportedDeployer ensures the deployer recorded +// as state is the one the implementation reports having used, not the one that +// was requested. Deploy.Deployer must describe what actually happened, exactly +// as Deploy.Namespace is taken from the result rather than from the request - +// otherwise the switch guard would compare against a value the cluster never +// confirmed. +func TestClient_Deploy_PersistsSelfReportedDeployer(t *testing.T) { + const reported = "reported-by-implementation" + + deployer := mock.NewDeployer() + deployer.DeployFn = func(_ context.Context, _ fn.Function) (fn.DeploymentResult, error) { + return fn.DeploymentResult{ + Status: fn.Deployed, + Namespace: "reported-ns", + Deployer: reported, + }, nil + } + + client := fn.New(fn.WithDeployer(deployer)) + f := fn.Function{Name: "f", Deployer: deployers.Knative} + + f, err := client.Deploy(t.Context(), f, fn.WithDeploySkipBuildCheck(true)) + if err != nil { + t.Fatal(err) + } + + if f.Deploy.Deployer != reported { + t.Fatalf("expected the self-reported deployer %q persisted as state, got %q", reported, f.Deploy.Deployer) + } + if f.Deployer != deployers.Knative { + t.Fatalf("expected the requested deployer %q preserved as intent, got %q", deployers.Knative, f.Deployer) + } + if f.Deploy.Namespace != "reported-ns" { + t.Fatalf("expected the self-reported namespace persisted as state, got %q", f.Deploy.Namespace) + } +} diff --git a/pkg/functions/function.go b/pkg/functions/function.go index f9470fb3a9..02bd45d2e4 100644 --- a/pkg/functions/function.go +++ b/pkg/functions/function.go @@ -100,6 +100,12 @@ type Function struct { // Namespace in which to deploy the Function Namespace string `yaml:"namespace,omitempty"` + // Deployer with which to deploy the Function: the requested (intended) + // deployer. This is the user's choice and persists across undeploy. + // The deployer a Function is CURRENTLY deployed with is recorded separately + // in .Deploy.Deployer, which is cleared on undeploy. + Deployer string `yaml:"deployer,omitempty" jsonschema:"enum=knative,enum=raw,enum=keda"` + // Created time is the moment that creation was successfully completed // according to the client which is in charge of what constitutes being // fully "Created" (aka initialized) @@ -260,8 +266,9 @@ type DeploySpec struct { // More info: https://kubernetes.io/docs/tasks/configure-pod-container/pull-image-private-registry/ ImagePullSecret string `yaml:"imagePullSecret,omitempty"` - // Deployer specifies the type of deployment to use: "knative", "raw" or "keda" - // Defaults to "knative" for backwards compatibility + // Deployer records the deployer the Function is CURRENTLY DEPLOYED: + // observed state, written after successful deployment, and cleared on + // undeploy alongside Namespace. Deployer string `yaml:"deployer,omitempty" jsonschema:"enum=knative,enum=raw,enum=keda"` Subscriptions []KnativeSubscription `yaml:"subscriptions,omitempty"` diff --git a/pkg/k8s/deployer.go b/pkg/k8s/deployer.go index 47d6fc5209..b7102557a7 100644 --- a/pkg/k8s/deployer.go +++ b/pkg/k8s/deployer.go @@ -25,13 +25,14 @@ import ( eventingv1 "knative.dev/eventing/pkg/apis/eventing/v1" eventingv1client "knative.dev/eventing/pkg/client/clientset/versioned/typed/eventing/v1" "knative.dev/func/pkg/deployer" + "knative.dev/func/pkg/deployers" fn "knative.dev/func/pkg/functions" "knative.dev/pkg/apis" duckv1 "knative.dev/pkg/apis/duck/v1" ) const ( - KubernetesDeployerName = "raw" + KubernetesDeployerName = deployers.Kubernetes DefaultLivenessEndpoint = "/health/liveness" DefaultReadinessEndpoint = "/health/readiness" @@ -248,6 +249,7 @@ func (d *Deployer) Deploy(ctx context.Context, f fn.Function) (fn.DeploymentResu Status: status, URL: url, Namespace: namespace, + Deployer: KubernetesDeployerName, }, nil } diff --git a/pkg/keda/deployer.go b/pkg/keda/deployer.go index 72ac31cd01..bb03c35176 100644 --- a/pkg/keda/deployer.go +++ b/pkg/keda/deployer.go @@ -14,12 +14,13 @@ import ( "k8s.io/client-go/kubernetes" "k8s.io/utils/ptr" "knative.dev/func/pkg/deployer" + "knative.dev/func/pkg/deployers" fn "knative.dev/func/pkg/functions" "knative.dev/func/pkg/k8s" ) const ( - KedaDeployerName = "keda" + KedaDeployerName = deployers.Keda ) type DeployerOpt func(*Deployer) @@ -132,6 +133,7 @@ func (d *Deployer) Deploy(ctx context.Context, f fn.Function) (fn.DeploymentResu Status: deployResult.Status, URL: fmt.Sprintf("http://%s:8080", hosts[0]), // TODO: check on HTTPS too Namespace: deployResult.Namespace, + Deployer: KedaDeployerName, }, nil } diff --git a/pkg/knative/deployer.go b/pkg/knative/deployer.go index 1c131bba8f..7d47065873 100644 --- a/pkg/knative/deployer.go +++ b/pkg/knative/deployer.go @@ -37,12 +37,13 @@ import ( servingv1 "knative.dev/serving/pkg/apis/serving/v1" "knative.dev/func/pkg/deployer" + "knative.dev/func/pkg/deployers" fn "knative.dev/func/pkg/functions" "knative.dev/func/pkg/k8s" ) const ( - KnativeDeployerName = "knative" + KnativeDeployerName = deployers.Knative ) type DeployerOpt func(*Deployer) @@ -297,6 +298,7 @@ consider using the --image-pull-secret flag, or setting up pull secrets manually Status: fn.Deployed, URL: route.Status.URL.String(), Namespace: namespace, + Deployer: KnativeDeployerName, }, nil } else { @@ -359,6 +361,7 @@ consider using the --image-pull-secret flag, or setting up pull secrets manually Status: fn.Updated, URL: route.Status.URL.String(), Namespace: namespace, + Deployer: KnativeDeployerName, }, nil } } diff --git a/pkg/lister/testing/integration_test_helper.go b/pkg/lister/testing/integration_test_helper.go index 3e329bd46a..d063fb175f 100644 --- a/pkg/lister/testing/integration_test_helper.go +++ b/pkg/lister/testing/integration_test_helper.go @@ -68,7 +68,7 @@ func TestInt_List(t *testing.T, lister fn.Lister, deployer fn.Deployer, describe t.Fatal(err) } t.Cleanup(func() { - err := client.Remove(ctx, "", "", f, true) + _, err := client.Remove(ctx, "", "", f, true) if err != nil { t.Logf("error removing Function: %v", err) } diff --git a/pkg/mock/deployer.go b/pkg/mock/deployer.go index 456b8494e8..4398faeeec 100644 --- a/pkg/mock/deployer.go +++ b/pkg/mock/deployer.go @@ -22,6 +22,9 @@ type Deployer struct { func NewDeployer() *Deployer { return &Deployer{ DeployFn: func(_ context.Context, f fn.Function) (result fn.DeploymentResult, err error) { + // Both namespace and deployer self-report their identity in this + // mock deployer. + // the minimum necessary logic for a deployer, which should be // confirmed by tests in the respective implementations. if f.Namespace != "" { @@ -31,6 +34,11 @@ func NewDeployer() *Deployer { } else { err = errors.New("namespace required for initial deployment") } + if f.Deployer != "" { + result.Deployer = f.Deployer // deployed with that requested + } else { + result.Deployer = f.Deploy.Deployer // redeploy with current + } return }, } diff --git a/pkg/pipelines/tekton/pipelines_int_test.go b/pkg/pipelines/tekton/pipelines_int_test.go index c0ecf5e87b..428045007b 100644 --- a/pkg/pipelines/tekton/pipelines_int_test.go +++ b/pkg/pipelines/tekton/pipelines_int_test.go @@ -180,7 +180,7 @@ func TestInt_Remote_Default(t *testing.T) { t.Fatal(err) } defer func() { - _ = client.Remove(ctx, "", "", f, true) + _, _ = client.Remove(ctx, "", "", f, true) }() httpClient := httpClientForDeployer(t, ctx, d) diff --git a/pkg/pipelines/tekton/pipelines_provider.go b/pkg/pipelines/tekton/pipelines_provider.go index 6d13178c30..186a2dd2cf 100644 --- a/pkg/pipelines/tekton/pipelines_provider.go +++ b/pkg/pipelines/tekton/pipelines_provider.go @@ -150,6 +150,15 @@ func (pp *PipelinesProvider) Run(ctx context.Context, f fn.Function) (string, fn } f.Deploy.Image = image + // Deployer is either the intended deployer (f.Deployer) or the one it was + // last deployed with (f.Deploy.Deployer). Recorded so a remote deploy is + // remembered in func.yaml, mirroring Namespace and Image above. + deployer := f.Deployer + if deployer == "" { + deployer = f.Deploy.Deployer + } + f.Deploy.Deployer = deployer + // Client for the given namespace client, err := NewTektonClient(namespace) if err != nil { diff --git a/pkg/remover/testing/integration_test_helper.go b/pkg/remover/testing/integration_test_helper.go index fbe9de2667..07d9274825 100644 --- a/pkg/remover/testing/integration_test_helper.go +++ b/pkg/remover/testing/integration_test_helper.go @@ -91,7 +91,8 @@ func TestInt_Remove(t *testing.T, remover fn.Remover, deployer fn.Deployer, desc } // Remove it - if err := client.Remove(ctx, "", "", f, true); err != nil { + _, err = client.Remove(ctx, "", "", f, true) + if err != nil { t.Logf("error removing Function: %v", err) } diff --git a/schema/func_yaml-schema.json b/schema/func_yaml-schema.json index a5a46cb3a8..044a19b699 100644 --- a/schema/func_yaml-schema.json +++ b/schema/func_yaml-schema.json @@ -118,7 +118,7 @@ "keda" ], "type": "string", - "description": "Deployer specifies the type of deployment to use: \"knative\", \"raw\" or \"keda\"\nDefaults to \"knative\" for backwards compatibility" + "description": "Deployer records the deployer the Function is CURRENTLY DEPLOYED:\nobserved state, written after successful deployment, and cleared on\nundeploy alongside Namespace." }, "subscriptions": { "items": { @@ -202,6 +202,15 @@ "type": "string", "description": "Namespace in which to deploy the Function" }, + "deployer": { + "enum": [ + "knative", + "raw", + "keda" + ], + "type": "string", + "description": "Deployer with which to deploy the Function: the requested (intended)\ndeployer. This is the user's choice and persists across undeploy.\nThe deployer a Function is CURRENTLY deployed with is recorded separately\nin .Deploy.Deployer, which is cleared on undeploy." + }, "created": { "type": "string", "description": "Created time is the moment that creation was successfully completed\naccording to the client which is in charge of what constitutes being\nfully \"Created\" (aka initialized)",