From 16edf163f7ce2a96bb71803a2d0daed2e9e3588a Mon Sep 17 00:00:00 2001 From: David Fridrich Date: Sun, 5 Jul 2026 20:13:47 +0200 Subject: [PATCH 1/2] feat: expose raw k8s deployer via OpenShift Route by default, gated to OCP --- cmd/completion_util.go | 20 ++ cmd/deploy.go | 44 ++- cmd/deploy_test.go | 210 +++++++++++++ cmd/errors.go | 29 ++ docs/reference/func_deploy.md | 1 + e2e/e2e_metadata_test.go | 2 +- e2e/e2e_trigger_sync_test.go | 6 +- .../testing/integration_test_helper.go | 15 +- .../testing/integration_test_helper.go | 1 + pkg/functions/client.go | 8 +- pkg/functions/client_test.go | 36 +++ pkg/functions/errors.go | 1 + pkg/functions/function.go | 11 + pkg/functions/function_expose.go | 21 ++ pkg/functions/function_expose_unit_test.go | 30 ++ pkg/k8s/deployer.go | 223 +++++++++++++- pkg/k8s/deployer_test.go | 84 +++++ pkg/k8s/describer.go | 16 +- pkg/k8s/lister.go | 7 +- pkg/k8s/route.go | 268 ++++++++++++++++ pkg/k8s/route_test.go | 290 ++++++++++++++++++ pkg/keda/deployer.go | 5 + pkg/knative/deployer.go | 2 +- pkg/lister/testing/integration_test_helper.go | 4 + pkg/mock/deployer.go | 3 + pkg/pipelines/tekton/pipelines_provider.go | 13 +- .../testing/integration_test_helper.go | 1 + schema/func_yaml-schema.json | 4 + 28 files changed, 1321 insertions(+), 34 deletions(-) create mode 100644 pkg/functions/function_expose.go create mode 100644 pkg/functions/function_expose_unit_test.go create mode 100644 pkg/k8s/route.go create mode 100644 pkg/k8s/route_test.go diff --git a/cmd/completion_util.go b/cmd/completion_util.go index cffb092534..014f4136b1 100644 --- a/cmd/completion_util.go +++ b/cmd/completion_util.go @@ -190,3 +190,23 @@ func CompleteDeployerList(cmd *cobra.Command, args []string, complete string) (m return } + +func CompleteExposeList(cmd *cobra.Command, args []string, complete string) (matches []string, d cobra.ShellCompDirective) { + values := []string{"none", "route"} + + d = cobra.ShellCompDirectiveNoFileComp + matches = []string{} + + if len(complete) == 0 { + matches = values + return + } + + for _, v := range values { + if strings.HasPrefix(v, complete) { + matches = append(matches, v) + } + } + + return +} diff --git a/cmd/deploy.go b/cmd/deploy.go index 50ec7647c6..5d91b3a755 100644 --- a/cmd/deploy.go +++ b/cmd/deploy.go @@ -132,10 +132,10 @@ EXAMPLES SuggestFor: []string{"delpoy", "deplyo"}, PreRunE: bindEnv("build", "build-timestamp", "builder", "builder-image", "base-image", "confirm", "domain", "env", "git-branch", "git-dir", - "git-url", "image", "image-pull-secret", "management-disabled", "namespace", "path", "platform", "push", "pvc-size", - "service-account", "deployer", "registry", "registry-insecure", - "registry-authfile", "remote", "username", "password", "token", "verbose", - "remote-storage-class"), + "git-url", "image", "image-pull-secret", "management-disabled", + "namespace", "path", "platform", "push", "pvc-size", "service-account", + "deployer", "expose", "registry", "registry-insecure", "registry-authfile", + "remote", "username", "password", "token", "verbose", "remote-storage-class"), RunE: func(cmd *cobra.Command, args []string) error { return runDeploy(cmd, newClient) }, @@ -200,6 +200,12 @@ 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("expose", f.Deploy.Expose, + "External exposure mode: 'route' (create a Route; OpenShift clusters only), "+ + "'none' (cluster-local opt-out). Raw and keda deployers only. "+ + "Defaults to exposed on OpenShift, cluster-local elsewhere. "+ + "An explicitly empty value (--expose=\"\") clears the persisted deploy.expose key and "+ + "returns to the default. ($FUNC_EXPOSE)") // Static Flags: // Options which have static defaults only (not globally configurable nor // persisted with the function) @@ -240,6 +246,10 @@ EXAMPLES fmt.Println("internal: error while calling RegisterFlagCompletionFunc: ", err) } + if err := cmd.RegisterFlagCompletionFunc("expose", CompleteExposeList); err != nil { + fmt.Println("internal: error while calling RegisterFlagCompletionFunc: ", err) + } + return cmd } @@ -285,6 +295,9 @@ func runDeploy(cmd *cobra.Command, newClient ClientFactory) (err error) { // Warn if registry changed but registryInsecure is still true warnRegistryInsecureChange(cmd.OutOrStderr(), cfg.Registry, f) + // Warn if expose flag is used with deployer where it has no effect + warnExposeIgnore(cmd.OutOrStderr(), cfg.Expose, cfg.Deployer) + // 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 == "" { @@ -570,6 +583,11 @@ type deployConfig struct { // ManagementDisabled disables automatic Function CR sync after deploy. ManagementDisabled bool + + // Expose controls external access - how/if the function should be + // exposed externally. Defaults to exposed on OpenShift, cluster-local + // elsewhere; "none" opts out explicitly. + Expose string } // newDeployConfig creates a buildConfig populated from command flags and @@ -592,6 +610,7 @@ func newDeployConfig(cmd *cobra.Command) deployConfig { ImagePullSecret: viper.GetString("image-pull-secret"), Deployer: viper.GetString("deployer"), ManagementDisabled: viper.GetBool("management-disabled"), + Expose: viper.GetString("expose"), } // NOTE: .Env should be viper.GetStringSlice, but this returns unparsed // results and appears to be an open issue since 2017: @@ -629,6 +648,7 @@ func (c deployConfig) Configure(f fn.Function) (fn.Function, error) { f.Deploy.ImagePullSecret = c.ImagePullSecret f.Deployer = c.Deployer f.Deploy.ManagementDisabled = c.ManagementDisabled + f.Deploy.Expose = c.Expose f.Local.Remote = c.Remote // PVCSize @@ -748,6 +768,11 @@ func (c deployConfig) Validate(cmd *cobra.Command) (err error) { } } + // Validate expose flag if provided + if err = fn.ValidateExpose(c.Expose); err != nil { + return err + } + // Check Image Digest was included var digest bool if c.Image != "" { @@ -909,3 +934,14 @@ func isDigested(v string) (validDigest bool, err error) { _, ok := ref.(name.Digest) return ok, nil } + +// warnExposeIgnore warns when non raw|keda deployer is used with Expose flag +// where it is simply ignored and has no effect. An empty deployer means the +// default (knative), which also ignores expose. +func warnExposeIgnore(w io.Writer, expose, deployer string) { + if expose != "" && deployer != k8s.KubernetesDeployerName && + deployer != keda.KedaDeployerName { + fmt.Fprintf(w, "warning: deploy.expose %q is ignored - only the raw and keda deployers "+ + "support external exposure via this field.\n", expose) + } +} diff --git a/cmd/deploy_test.go b/cmd/deploy_test.go index 0b2446b1fe..9f4eee0a42 100644 --- a/cmd/deploy_test.go +++ b/cmd/deploy_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "os" "path/filepath" "reflect" "strings" @@ -2729,3 +2730,212 @@ func TestDeploy_DeployerSwitch(t *testing.T) { }) } } + +// TestDeploy_ExposeEmptyVsUnset: an explicitly empty --expose="" +// clears the persisted deploy.expose key reverting to the default at deploy +// time, while a deploy without the flag leaves the persisted value untouched. +func TestDeploy_ExposeEmptyVsUnset(t *testing.T) { + // newFn initializes a Go function in a temp directory and returns its root. + newFn := func(t *testing.T) string { + t.Helper() + root := FromTempDirectory(t) + if _, err := fn.New().Init(fn.Function{Runtime: "go", Root: root}); err != nil { + t.Fatal(err) + } + return root + } + + // deploy runs `func deploy` with args against mock builder/deployer, + // failing the test on error and returning the command's combined output. + deploy := func(t *testing.T, args ...string) string { + t.Helper() + cmd := NewDeployCmd(NewTestClient( + fn.WithBuilder(mock.NewBuilder()), + fn.WithDeployer(mock.NewDeployer()), + fn.WithRegistry(TestRegistry), + )) + cmd.SetArgs(args) + var out strings.Builder + cmd.SetOut(&out) + cmd.SetErr(&out) + if err := cmd.Execute(); err != nil { + t.Fatal(err) + } + return out.String() + } + + // loadFn re-reads the function from disk. + loadFn := func(t *testing.T, root string) fn.Function { + t.Helper() + f, err := fn.NewFunction(root) + if err != nil { + t.Fatal(err) + } + return f + } + + t.Run(`--expose="" clears a previously-persisted "none"`, func(t *testing.T) { + root := newFn(t) + + deploy(t, "--deployer", "raw", "--expose", "none") + if f := loadFn(t, root); f.Deploy.Expose != "none" { + t.Fatalf("setup: expected expose 'none' to be persisted, got %q", f.Deploy.Expose) + } + + deploy(t, "--deployer", "raw", "--expose=") + // unmarshalled yaml would not be able to distinguish between the value + // being empty and gone (not in the file) + raw, err := os.ReadFile(filepath.Join(root, "func.yaml")) + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(raw), "expose") { + t.Errorf("expected NO expose key in func.yaml, got:\n%s", raw) + } + }) + + t.Run("plain deploy without the flag still works and leaves expose unpersisted", func(t *testing.T) { + root := newFn(t) + deploy(t, "--deployer", "raw") + if f := loadFn(t, root); f.Deploy.Expose != "" { + t.Errorf("expected expose to remain unpersisted (empty), got %q", f.Deploy.Expose) + } + }) + + t.Run("persisted none + no flag round-trips untouched", func(t *testing.T) { + root := newFn(t) + + deploy(t, "--deployer", "raw", "--expose", "none") + if f := loadFn(t, root); f.Deploy.Expose != "none" { + t.Fatalf("expected expose 'none' to be persisted, got %q", f.Deploy.Expose) + } + + // redeploy without changing the flag should keep it as is + deploy(t, "--deployer", "raw") + if f := loadFn(t, root); f.Deploy.Expose != "none" { + t.Errorf("expected persisted 'none' to round-trip untouched, got %q", f.Deploy.Expose) + } + }) +} + +// TestDeploy_ExposeInvalidValueError: a malformed --expose value fails the +// deploy (any deployer) with the CLI's typed ErrInvalidExpose. +func TestDeploy_ExposeInvalidValueError(t *testing.T) { + root := FromTempDirectory(t) + if _, err := fn.New().Init(fn.Function{Runtime: "go", Root: root}); err != nil { + t.Fatal(err) + } + cmd := NewDeployCmd(NewTestClient( + fn.WithBuilder(mock.NewBuilder()), + fn.WithDeployer(mock.NewDeployer()), + fn.WithRegistry(TestRegistry), + )) + cmd.SetArgs([]string{"--expose", "bogus"}) + var want *ErrInvalidExpose + if err := cmd.Execute(); !errors.As(err, &want) { + t.Errorf("expected ErrInvalidExpose, got %v", err) + } +} + +// TestDeploy_ExposeRoutePersists ensures "route" round-trips through +// --expose into f.Deploy.Expose end-to-end. +func TestDeploy_ExposeRoutePersists(t *testing.T) { + root := FromTempDirectory(t) + + if _, err := fn.New().Init(fn.Function{Runtime: "go", Root: root}); err != nil { + t.Fatal(err) + } + + cmd := NewDeployCmd(NewTestClient( + fn.WithBuilder(mock.NewBuilder()), + fn.WithDeployer(mock.NewDeployer()), + fn.WithRegistry(TestRegistry), + )) + cmd.SetArgs([]string{"--deployer", "raw", "--expose=route"}) + if err := cmd.Execute(); err != nil { + t.Fatal(err) + } + + f, err := fn.NewFunction(root) + if err != nil { + t.Fatal(err) + } + if f.Deploy.Expose != "route" { + t.Fatalf("expected expose 'route' to be persisted, got %q", f.Deploy.Expose) + } +} + +// TestDeploy_ExposeIgnoredByDeployerNote: a deployer that ignores a set +// deploy.expose warns and proceeds +func TestDeploy_ExposeIgnoredByDeployerNote(t *testing.T) { + tests := []struct { + name string + args []string + wantWarning string // distinguishing substring of the warning; "" means silent + }{ + { + name: "raw+route: silent", + args: []string{"--deployer", "raw", "--expose", "route"}, + }, + { + name: "knative+empty: silent", + args: []string{"--deployer", "knative"}, + }, + { + name: "knative+route: warns, proceeds", + args: []string{"--deployer", "knative", "--expose", "route"}, + wantWarning: `deploy.expose "route" is ignored - only the raw and keda deployers support external exposure via this field.`, + }, + { + name: "knative+none: warns, proceeds", + args: []string{"--deployer", "knative", "--expose", "none"}, + wantWarning: `deploy.expose "none" is ignored - only the raw and keda deployers support external exposure via this field.`, + }, + { + name: "keda+route: silent, keda supports expose too", + args: []string{"--deployer", "keda", "--expose", "route"}, + }, + { + name: "keda+none: silent, keda supports expose too", + args: []string{"--deployer", "keda", "--expose", "none"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + root := FromTempDirectory(t) + if _, err := fn.New().Init(fn.Function{Runtime: "go", Root: root}); err != nil { + t.Fatal(err) + } + + builder := mock.NewBuilder() + cmd := NewDeployCmd(NewTestClient( + fn.WithBuilder(builder), + fn.WithDeployer(mock.NewDeployer()), + fn.WithRegistry(TestRegistry), + )) + cmd.SetArgs(tt.args) + var stderr strings.Builder + cmd.SetOut(&stderr) + cmd.SetErr(&stderr) + err := cmd.Execute() + + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !builder.BuildInvoked { + t.Error("expected the deploy to proceed to build") + } + + if tt.wantWarning == "" { + if strings.Contains(stderr.String(), "deploy.expose") { + t.Errorf("expected no warning on stderr, got:\n%s", stderr.String()) + } + return + } + if !strings.Contains(stderr.String(), tt.wantWarning) { + t.Errorf("expected stderr to contain:\n%s\ngot:\n%s", tt.wantWarning, stderr.String()) + } + }) + } +} diff --git a/cmd/errors.go b/cmd/errors.go index e820df1d96..f99ed9db3b 100644 --- a/cmd/errors.go +++ b/cmd/errors.go @@ -31,6 +31,9 @@ Internal error during error-wrapping: specified cmd '%s' not supported`, cmd) if errors.Is(err, fn.ErrPlatformNotSupported) { return NewErrPlatformNotSupported(err, cmd) } + if errors.Is(err, fn.ErrInvalidExpose) { + return NewErrInvalidExpose(err) + } return err } @@ -217,6 +220,32 @@ func (e *ErrInvalidDomain) Unwrap() error { // -------------------------------------------------------------------------- // +type ErrInvalidExpose struct { + Err error +} + +func NewErrInvalidExpose(err error) error { + return &ErrInvalidExpose{Err: err} +} + +func (e *ErrInvalidExpose) Error() string { + return fmt.Sprintf(`%v + +Try this: + func deploy --expose=route Create an OpenShift Route (OpenShift clusters only) + func deploy --expose=none Cluster-local opt-out, no external exposure + +deploy.expose takes effect with the raw and keda deployers only (--deployer=raw or --deployer=keda), +which expose by default when the platform and deployer support it. +For more options, run 'func deploy --help'`, e.Err) +} + +func (e *ErrInvalidExpose) Unwrap() error { + return e.Err +} + +// -------------------------------------------------------------------------- // + type ErrInvalidKubeconfig struct { Err error } diff --git a/docs/reference/func_deploy.md b/docs/reference/func_deploy.md index b8257073ad..9ecd104022 100644 --- a/docs/reference/func_deploy.md +++ b/docs/reference/func_deploy.md @@ -122,6 +122,7 @@ func deploy --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-). + --expose string External exposure mode: 'route' (create a Route; OpenShift clusters only), 'none' (cluster-local opt-out). Raw and keda deployers only. Defaults to exposed on OpenShift, cluster-local elsewhere. An explicitly empty value (--expose="") clears the persisted deploy.expose key and returns to the default. ($FUNC_EXPOSE) -t, --git-branch string Git revision (branch) to be used when deploying via the Git repository ($FUNC_GIT_BRANCH) -d, --git-dir string Directory in the Git repository containing the function (default is the root) ($FUNC_GIT_DIR) -g, --git-url string Repository url containing the function to build ($FUNC_GIT_URL) diff --git a/e2e/e2e_metadata_test.go b/e2e/e2e_metadata_test.go index 63c376558b..98ad6da5b0 100644 --- a/e2e/e2e_metadata_test.go +++ b/e2e/e2e_metadata_test.go @@ -662,7 +662,7 @@ func TestMetadata_Subscriptions_Raw(t *testing.T) { } // Deploy with raw deployer to test trigger creation - if err := newCmd(t, "deploy", "--deployer", "raw").Run(); err != nil { + if err := newCmd(t, "deploy", "--deployer", "raw", "--expose", "none").Run(); err != nil { t.Fatal(err) } defer clean(t, subscriberName, Namespace) diff --git a/e2e/e2e_trigger_sync_test.go b/e2e/e2e_trigger_sync_test.go index 72953b0d1c..019eef09db 100644 --- a/e2e/e2e_trigger_sync_test.go +++ b/e2e/e2e_trigger_sync_test.go @@ -50,7 +50,7 @@ func TestMetadata_TriggerSync(t *testing.T) { if err := f.Write(); err != nil { t.Fatal(err) } - if err := newCmd(t, "deploy", "--deployer", "raw").Run(); err != nil { + if err := newCmd(t, "deploy", "--deployer", "raw", "--expose", "none").Run(); err != nil { t.Fatal(err) } } @@ -109,7 +109,7 @@ func TestMetadata_TriggerSync(t *testing.T) { t.Logf("Created manual trigger: %s", manualTriggerName) // Redeploy (no changes to subscriptions) - if err := newCmd(t, "deploy", "--deployer", "raw").Run(); err != nil { + if err := newCmd(t, "deploy", "--deployer", "raw", "--expose", "none").Run(); err != nil { t.Fatal(err) } time.Sleep(5 * time.Second) @@ -171,7 +171,7 @@ func TestMetadata_TriggerSync(t *testing.T) { // (AlreadyExists-tolerated) cluster path; trigger-name determinism is // already exhaustively unit-tested (pkg/k8s/deployer_test.go:157-333), // so the previous ×3 loop is reduced to ×1. - if err := newCmd(t, "deploy", "--deployer", "raw").Run(); err != nil { + if err := newCmd(t, "deploy", "--deployer", "raw", "--expose", "none").Run(); err != nil { t.Fatal(err) } time.Sleep(3 * time.Second) diff --git a/pkg/deployer/testing/integration_test_helper.go b/pkg/deployer/testing/integration_test_helper.go index 1c5b9158c3..9a4aaff3c3 100644 --- a/pkg/deployer/testing/integration_test_helper.go +++ b/pkg/deployer/testing/integration_test_helper.go @@ -62,6 +62,10 @@ func TestInt_Deploy(t *testing.T, deployer fn.Deployer, remover fn.Remover, desc Runtime: "go", Namespace: ns, Registry: Registry(), + // Explicit opt-out: keeps this integration deploy cluster-local and + // platform-deterministic under exposed-by-default; ignored entirely + // by the knative deployer. + Deploy: fn.DeploySpec{Expose: "none"}, }) if err != nil { t.Fatal(err) @@ -165,6 +169,7 @@ func TestInt_Metadata(t *testing.T, deployer fn.Deployer, remover fn.Remover, de Runtime: "go", Namespace: ns, Registry: Registry(), + Deploy: fn.DeploySpec{Expose: "none"}, }) if err != nil { t.Fatal(err) @@ -320,6 +325,7 @@ func TestInt_Events(t *testing.T, deployer fn.Deployer, remover fn.Remover, desc Runtime: "go", Namespace: ns, Registry: Registry(), + Deploy: fn.DeploySpec{Expose: "none"}, }) if err != nil { t.Fatal(err) @@ -403,6 +409,7 @@ func TestInt_Scale(t *testing.T, deployer fn.Deployer, remover fn.Remover, descr Runtime: "go", Namespace: ns, Registry: Registry(), + Deploy: fn.DeploySpec{Expose: "none"}, }) if err != nil { t.Fatal(err) @@ -518,6 +525,7 @@ func TestInt_EnvsUpdate(t *testing.T, deployer fn.Deployer, remover fn.Remover, Runtime: "go", Namespace: ns, Registry: Registry(), + Deploy: fn.DeploySpec{Expose: "none"}, }) if err != nil { t.Fatal(err) @@ -731,10 +739,11 @@ func TestInt_FullPath(t *testing.T, deployer fn.Deployer, remover fn.Remover, li // * application also prints the same info to stderr on startup 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 + // pinned prebuilt image: these tests exercise deployment, not the + // build/image-resolution flow Image: "quay.io/mvasek/func-test-service@sha256:2eca4de00d7569c8791634bdbb0c4d5ec8fb061b001549314591e839dabd5269", Namespace: namespace, + Expose: "none", Labels: []fn.Label{{Key: ptr("my-label"), Value: ptr("my-label-value")}}, Options: fn.Options{ Scale: &fn.ScaleOptions{ @@ -927,6 +936,7 @@ func TestInt_ResourceValidationOnFirstDeploy(t *testing.T, deployer fn.Deployer, Runtime: "go", Namespace: ns, Registry: Registry(), + Deploy: fn.DeploySpec{Expose: "none"}, }) if err != nil { t.Fatal(err) @@ -1234,6 +1244,7 @@ func TestInt_OperatorSync(t *testing.T, deployer fn.Deployer, remover fn.Remover Runtime: "go", Namespace: ns, Registry: Registry(), + Deploy: fn.DeploySpec{Expose: "none"}, }) if err != nil { t.Fatal(err) diff --git a/pkg/describer/testing/integration_test_helper.go b/pkg/describer/testing/integration_test_helper.go index 45638f86fb..5d57cbdc0b 100644 --- a/pkg/describer/testing/integration_test_helper.go +++ b/pkg/describer/testing/integration_test_helper.go @@ -38,6 +38,7 @@ func TestInt_Describe(t *testing.T, describer fn.Describer, deployer fn.Deployer Runtime: "go", Namespace: ns, Registry: Registry(), + Deploy: fn.DeploySpec{Expose: "none"}, }) if err != nil { t.Fatal(err) diff --git a/pkg/functions/client.go b/pkg/functions/client.go index a4b2902ff9..d79a2f049a 100644 --- a/pkg/functions/client.go +++ b/pkg/functions/client.go @@ -187,8 +187,8 @@ type Describer interface { type Instance struct { // Route is the primary route of a function instance. Route string - // Routes is the primary route plus any other route at which the function - // can be contacted. + // Routes is the primary route first (external when exposed), plus any + // other route at which the function can be contacted. Routes []string `json:"routes" yaml:"routes"` Name string `json:"name" yaml:"name"` Image string `json:"image" yaml:"image"` @@ -919,9 +919,9 @@ func (c *Client) Deploy(ctx context.Context, f Function, oo ...DeployOption) (Fu switch result.Status { case Deployed: - fmt.Fprintf(os.Stderr, "✅ Function deployed in namespace %q and exposed at URL: \n %v\n", result.Namespace, result.URL) + fmt.Fprintf(os.Stderr, "✅ Function deployed in namespace %q at URL: \n %v\n", result.Namespace, result.URL) case Updated: - fmt.Fprintf(os.Stderr, "✅ Function updated in namespace %q and exposed at URL: \n %v\n", result.Namespace, result.URL) + fmt.Fprintf(os.Stderr, "✅ Function updated in namespace %q at URL: \n %v\n", result.Namespace, result.URL) default: } diff --git a/pkg/functions/client_test.go b/pkg/functions/client_test.go index 4cc49242ec..eb6f061a55 100644 --- a/pkg/functions/client_test.go +++ b/pkg/functions/client_test.go @@ -1603,6 +1603,42 @@ func TestClient_Deploy_UnbuiltErrors(t *testing.T) { } } +// TestClient_Deploy_PrintsResultMessage asserts Deploy prints the deploy +// status message (namespace + URL) to stderr on success. +func TestClient_Deploy_PrintsResultMessage(t *testing.T) { + root, rm := Mktemp(t) + defer rm() + f, err := fn.New().Init(fn.Function{Runtime: TestRuntime, Name: "f", Root: root}) + if err != nil { + t.Fatal(err) + } + + deployer := mock.NewDeployerWithResult(fn.DeploymentResult{ + Status: fn.Deployed, + Namespace: TestNamespace, + URL: "http://f.example.com", + }) + client := fn.New(fn.WithDeployer(deployer)) + + old := os.Stderr + r, w, _ := os.Pipe() + os.Stderr = w + + _, err = client.Deploy(t.Context(), f, fn.WithDeploySkipBuildCheck(true)) + + w.Close() + os.Stderr = old + if err != nil { + t.Fatal(err) + } + + var buf [4096]byte + n, _ := r.Read(buf[:]) + if output := string(buf[:n]); !strings.Contains(output, "deployed in namespace") || !strings.Contains(output, "http://f.example.com") { + t.Errorf("expected stderr to contain the deploy message and URL, got: %q", output) + } +} + // TestClient_New_BuilderImagesPersisted Asserts that the client preserves user- // provided Builder Images func TestClient_New_BuildersPersisted(t *testing.T) { diff --git a/pkg/functions/errors.go b/pkg/functions/errors.go index 1829bddb03..b52cc3d137 100644 --- a/pkg/functions/errors.go +++ b/pkg/functions/errors.go @@ -10,6 +10,7 @@ import ( var ( ErrEnvironmentNotFound = errors.New("environment not found") ErrFunctionNotFound = errors.New("function not found") + ErrInvalidExpose = errors.New("invalid deploy.expose value") ErrMismatchedName = errors.New("name passed the function source") ErrNameRequired = errors.New("name required") ErrNamespaceRequired = errors.New("namespace required") diff --git a/pkg/functions/function.go b/pkg/functions/function.go index 02bd45d2e4..f9273760b2 100644 --- a/pkg/functions/function.go +++ b/pkg/functions/function.go @@ -277,6 +277,17 @@ type DeploySpec struct { // for operator management after deploy. The zero value (false) means // the function is managed by default when the func-operator is installed. ManagementDisabled bool `yaml:"managementDisabled,omitempty"` + + // Expose controls external access for the raw and keda deployers (the + // knative deployer manages its own exposure and ignores it). Optional. + // Values: "route" (create an OpenShift Route; OpenShift clusters only - + // a hard error elsewhere), "none" (cluster-local only, explicit + // opt-out). Defaults to "route" behavior on OpenShift - a deployed + // function being externally reachable is the expected outcome - and to + // cluster-local on any other cluster, since a Route is an + // OpenShift-only mechanism and the unset default must not impose a + // platform requirement. + Expose string `yaml:"expose,omitempty"` } // HealthEndpoints specify the liveness and readiness endpoints for a Runtime diff --git a/pkg/functions/function_expose.go b/pkg/functions/function_expose.go new file mode 100644 index 0000000000..104bddcb74 --- /dev/null +++ b/pkg/functions/function_expose.go @@ -0,0 +1,21 @@ +package functions + +import ( + "fmt" +) + +// ValidateExpose reports whether expose is a valid deploy.expose value: "" +// (default - exposed via an OpenShift Route, since a deployed function +// being reachable is the expected outcome; cluster-local on non-OpenShift +// clusters, since a Route is an OpenShift-only mechanism), "none" +// (cluster-local, explicit opt-out), or "route" (explicit request for an +// OpenShift Route). There is no ref suffix: an OpenShift Route has no +// concept of "which ingress controller to attach to" - the cluster's +// IngressController picks the router, and the Route object doesn't +// reference one. Any other value is rejected. +func ValidateExpose(expose string) error { + if expose == "" || expose == "none" || expose == "route" { + return nil + } + return fmt.Errorf("%w: %q", ErrInvalidExpose, expose) +} diff --git a/pkg/functions/function_expose_unit_test.go b/pkg/functions/function_expose_unit_test.go new file mode 100644 index 0000000000..0af532c640 --- /dev/null +++ b/pkg/functions/function_expose_unit_test.go @@ -0,0 +1,30 @@ +package functions + +import ( + "errors" + "fmt" + "strings" + "testing" +) + +func Test_ValidateExpose(t *testing.T) { + for _, v := range []string{"", "route", "none"} { + t.Run(v, func(t *testing.T) { + if err := ValidateExpose(v); err != nil { + t.Fatalf("ValidateExpose(%q): unexpected error: %v", v, err) + } + }) + } + + for _, v := range []string{"auto", "bogus", "ingress"} { + t.Run(v, func(t *testing.T) { + err := ValidateExpose(v) + if !errors.Is(err, ErrInvalidExpose) { + t.Fatalf("ValidateExpose(%q): expected errors.Is(err, ErrInvalidExpose), got %v", v, err) + } + if !strings.Contains(err.Error(), fmt.Sprintf("%q", v)) { + t.Errorf("ValidateExpose(%q): expected error to quote the bad value, got %v", v, err) + } + }) + } +} diff --git a/pkg/k8s/deployer.go b/pkg/k8s/deployer.go index b7102557a7..231cd44a68 100644 --- a/pkg/k8s/deployer.go +++ b/pkg/k8s/deployer.go @@ -19,8 +19,10 @@ import ( "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/apimachinery/pkg/util/rand" "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/client-go/dynamic" "k8s.io/client-go/kubernetes" "k8s.io/client-go/rest" + "k8s.io/client-go/util/retry" clienteventingv1 "knative.dev/client/pkg/eventing/v1" eventingv1 "knative.dev/eventing/pkg/apis/eventing/v1" eventingv1client "knative.dev/eventing/pkg/client/clientset/versioned/typed/eventing/v1" @@ -38,6 +40,11 @@ const ( DefaultReadinessEndpoint = "/health/readiness" DefaultHTTPPort = 8080 + // RouteHostnameAnnotation records the externally-exposed hostname (if + // any) on the function's Service, so lister/describer can read it back + // without re-deriving or re-querying the Route. + RouteHostnameAnnotation = "function.knative.dev/route-hostname" + // managedByAnnotation identifies triggers managed by this deployer managedByAnnotation = "func.knative.dev/managed-by" managedByValue = "func-raw-deployer" @@ -48,6 +55,11 @@ type DeployerOpt func(*Deployer) type Deployer struct { verbose bool decorator deployer.DeployDecorator + + // exposureDisabled marks a Deployer embedded by another deployer (keda) + // whose functions must stay cluster-local: a Route pointed at the + // raw ClusterIP Service would bypass keda's scale-to-zero interceptor. + exposureDisabled bool } func NewDeployer(opts ...DeployerOpt) *Deployer { @@ -64,6 +76,16 @@ func WithDeployerVerbose(verbose bool) DeployerOpt { } } +// WithDeployerExposureDisabled turns off this Deployer's own OpenShift +// Route exposure; for deployers that embed this Deployer but manage +// exposure themselves (eg. keda, whose functions stay behind its own +// interceptor and mint their own Route separately). +func WithDeployerExposureDisabled() DeployerOpt { + return func(d *Deployer) { + d.exposureDisabled = true + } +} + func WithDeployerDecorator(decorator deployer.DeployDecorator) DeployerOpt { return func(d *Deployer) { d.decorator = decorator @@ -133,6 +155,11 @@ func (d *Deployer) Deploy(ctx context.Context, f fn.Function) (fn.DeploymentResu return fn.DeploymentResult{}, err } + dynClient, err := dynamic.NewForConfig(config) + if err != nil { + return fn.DeploymentResult{}, fmt.Errorf("failed to create dynamic client: %w", err) + } + // Check if Dapr is installed daprInstalled := false _, err = clientset.CoreV1().Namespaces().Get(ctx, "dapr-system", metav1.GetOptions{}) @@ -161,7 +188,15 @@ func (d *Deployer) Deploy(ctx context.Context, f fn.Function) (fn.DeploymentResu return fn.DeploymentResult{}, fmt.Errorf("failed to validate referenced resources: %w", err) } - svc, err := d.generateService(f, namespace, daprInstalled, existingDeployment) + existingService, svcGetErr := serviceClient.Get(ctx, f.Name, metav1.GetOptions{}) + if svcGetErr != nil { + if !errors.IsNotFound(svcGetErr) { + return fn.DeploymentResult{}, fmt.Errorf("failed to get existing service: %w", svcGetErr) + } + existingService = nil + } + + svc, err := d.generateService(f, namespace, daprInstalled, existingDeployment, existingService) if err != nil { return fn.DeploymentResult{}, fmt.Errorf("failed to generate service resources: %w", err) } @@ -173,19 +208,17 @@ func (d *Deployer) Deploy(ctx context.Context, f fn.Function) (fn.DeploymentResu return fn.DeploymentResult{}, fmt.Errorf("failed to update deployment: %w", err) } - existingService, err := serviceClient.Get(ctx, f.Name, metav1.GetOptions{}) - if err == nil { + // update/create service + if svcGetErr == nil { svc.ResourceVersion = existingService.ResourceVersion if _, err = serviceClient.Update(ctx, svc, metav1.UpdateOptions{}); err != nil { return fn.DeploymentResult{}, fmt.Errorf("failed to update service: %w", err) } - } else if errors.IsNotFound(err) { - // Service doesn't exist, create it + } else { + // Confirmed IsNotFound above the generateService() if _, err = serviceClient.Create(ctx, svc, metav1.CreateOptions{}); err != nil { return fn.DeploymentResult{}, fmt.Errorf("failed to create service: %w", err) } - } else { - return fn.DeploymentResult{}, fmt.Errorf("failed to get existing service: %w", err) } status = fn.Updated @@ -215,7 +248,7 @@ func (d *Deployer) Deploy(ctx context.Context, f fn.Function) (fn.DeploymentResu return fn.DeploymentResult{}, fmt.Errorf("failed to create deployment: %w", err) } - svc, err := d.generateService(f, namespace, daprInstalled, deployment) + svc, err := d.generateService(f, namespace, daprInstalled, deployment, nil) if err != nil { return fn.DeploymentResult{}, fmt.Errorf("failed to generate service resources: %w", err) } @@ -234,6 +267,12 @@ func (d *Deployer) Deploy(ctx context.Context, f fn.Function) (fn.DeploymentResu return fn.DeploymentResult{}, fmt.Errorf("deployment did not become ready: %w", err) } + // External exposure via an OpenShift Route; see resolveExposure(). + url, _, err := d.resolveExposure(ctx, f, namespace, clientset, dynClient) + if err != nil { + return fn.DeploymentResult{}, err + } + // Sync triggers eventingClient, err := newEventingClient(config, namespace) if err != nil { @@ -243,8 +282,6 @@ func (d *Deployer) Deploy(ctx context.Context, f fn.Function) (fn.DeploymentResu return fn.DeploymentResult{}, err } - url := fmt.Sprintf("http://%s.%s.svc", f.Name, namespace) - return fn.DeploymentResult{ Status: status, URL: url, @@ -253,6 +290,160 @@ func (d *Deployer) Deploy(ctx context.Context, f fn.Function) (fn.DeploymentResu }, nil } +// resolveExposure keeps a raw-deployer function's external exposure (an +// OpenShift Route) in sync with what's currently wanted. This is symmetric +// for both directions: create/update the Route when exposure is wanted, +// remove it when it isn't - so toggling expose:route on and off across +// redeploys just works. +// Removal is unconditional whenever exposure isn't currently wanted, so a +// stale Route from a prior raw deploy never survives a raw -> keda deployer +// switch (the only cross-deployer path that still runs this code, since +// keda embeds this deployer with exposure disabled). +// Functions are exposed BY DEFAULT: a deployed function being reachable is +// the expected outcome, matching what a plain "func deploy" already implies +// for every other deployer, so the unset value behaves the same as +// explicit expose:route, not like expose:none. This is only meaningful on +// OpenShift, since a Route is an OpenShift-only mechanism: IsOpenShift() +// keeps plain-Kubernetes deploys safe without requiring any flag - an +// explicit expose:route request off OpenShift is still a hard error (the +// user asked for something impossible), but the unset default just quietly +// degrades to cluster-local there rather than failing an ordinary deploy. +func (d *Deployer) resolveExposure(ctx context.Context, f fn.Function, namespace string, clientset kubernetes.Interface, dynClient dynamic.Interface) (string, bool, error) { + defaultURL := fmt.Sprintf("http://%s.%s.svc", f.Name, namespace) + + if err := fn.ValidateExpose(f.Deploy.Expose); err != nil { + return "", false, err + } + tech := f.Deploy.Expose + + if tech == "route" && !IsOpenShift() { + return "", false, fmt.Errorf( + "expose:route requires an OpenShift cluster: route.openshift.io Routes are an " + + "OpenShift-specific resource, and this does not appear to be an OpenShift cluster") + } + + if d.exposureDisabled { + if err := d.removeExposure(ctx, clientset, dynClient, namespace, f.Name, false); err != nil { + return "", false, err + } + return defaultURL, false, nil + } + + wantsRoute := tech == "route" || (tech == "" && IsOpenShift()) + if !wantsRoute { + // expose:none (explicit opt-out): enforce=true, a hard error if + // removal fails to verify/clear. Unset on a non-OpenShift cluster + // (default gracefully declined, not requested): enforce=false, + // since nothing was actually asked for here. + if err := d.removeExposure(ctx, clientset, dynClient, namespace, f.Name, tech == "none"); err != nil { + return "", false, err + } + return defaultURL, false, nil + } + + url, err := d.ensureExposure(ctx, f, namespace, clientset, dynClient) + if err != nil { + return "", false, fmt.Errorf("external exposure failed: %w", err) + } + return url, true, nil +} + +// removeExposure deletes the managed Route (never a user-authored route +// sharing the function's name) and clears the recorded exposure state. +// Missing Route API support needs no special-casing: the GET reports +// NotFound either way, meaning nothing to remove. +// +// enforce selects the failure posture: +// - true (unset or expose:none): failing to verify/remove is a hard error; +// - false (deployer switched away from raw): an RBAC 403 on the route +// GET/DELETE prints a warning and the deploy continues, since keda +// users without Route permissions must stay green. +func (d *Deployer) removeExposure(ctx context.Context, clientset kubernetes.Interface, dynClient dynamic.Interface, namespace, name string, enforce bool) error { + if _, err := RemoveManagedRoute(ctx, dynClient, namespace, name); err != nil { + if !enforce && errors.IsForbidden(err) { + fmt.Fprintf(os.Stderr, "⚠️ cannot remove Route %q (forbidden) - leaving it in place\n", name) + } else { + return fmt.Errorf("failed to remove Route: %w", err) + } + } + + if err := writeRouteHostnameAnnotation(ctx, clientset, namespace, name, ""); err != nil { + if !enforce { + fmt.Fprintf(os.Stderr, "⚠️ failed to clear exposure state: %v\n", err) + return nil + } + return fmt.Errorf("failed to clear exposure state: %w", err) + } + return nil +} + +// ensureExposure creates or updates the Route exposing f, waits for it to +// be admitted by a router, and records the minted hostname. +func (d *Deployer) ensureExposure(ctx context.Context, f fn.Function, namespace string, clientset kubernetes.Interface, dynClient dynamic.Interface) (string, error) { + deployment, err := clientset.AppsV1().Deployments(namespace).Get(ctx, f.Name, metav1.GetOptions{}) + if err != nil { + return "", fmt.Errorf("failed to get deployment for owner reference: %w", err) + } + + route, err := GenerateRoute(f, f.Name, deployment, d.decorator, KubernetesDeployerName) + if err != nil { + return "", fmt.Errorf("failed to generate Route: %w", err) + } + + fmt.Fprintf(os.Stderr, "🌐 Exposing function externally -> %s\n", f.Name) + + if err := EnsureRoute(ctx, dynClient, namespace, route); err != nil { + return "", err + } + + // Wait for a router to accept the route - enforced, never downgraded to a warning. + host, err := WaitForRouteAdmitted(ctx, dynClient, namespace, f.Name, 30*time.Second) + if err != nil { + return "", fmt.Errorf("route was not admitted: %w", err) + } + + if err := writeRouteHostnameAnnotation(ctx, clientset, namespace, f.Name, host); err != nil { + return "", err + } + + // The Route redirects http to https (see GenerateRoute's tls stanza). + return fmt.Sprintf("https://%s", host), nil +} + +// writeRouteHostnameAnnotation records (hostname != "") or clears +// (hostname == "") the exposed hostname on the function's Service: no-op +// when already current, retried on write conflicts. A missing Service is +// tolerated only when clearing; recording against one that doesn't exist +// is a real error. +func writeRouteHostnameAnnotation(ctx context.Context, clientset kubernetes.Interface, namespace, name, hostname string) error { + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + svc, err := clientset.CoreV1().Services(namespace).Get(ctx, name, metav1.GetOptions{}) + if err != nil { + return err + } + if svc.Annotations[RouteHostnameAnnotation] == hostname { + return nil + } + if hostname == "" { + delete(svc.Annotations, RouteHostnameAnnotation) + } else { + if svc.Annotations == nil { + svc.Annotations = map[string]string{} + } + svc.Annotations[RouteHostnameAnnotation] = hostname + } + _, err = clientset.CoreV1().Services(namespace).Update(ctx, svc, metav1.UpdateOptions{}) + return err + }) + if err != nil { + if hostname == "" && errors.IsNotFound(err) { + return nil + } + return fmt.Errorf("failed to update exposure state on service %q: %w", name, err) + } + return nil +} + // generateTriggerName creates a deterministic trigger name based on subscription content func generateTriggerName(functionName, broker string, filters map[string]string) string { filterKeys := make([]string, 0, len(filters)) @@ -489,13 +680,23 @@ func (d *Deployer) generateDeployment(f fn.Function, namespace string, daprInsta return deployment, nil } -func (d *Deployer) generateService(f fn.Function, namespace string, daprInstalled bool, deployment *appsv1.Deployment) (*corev1.Service, error) { +// generateService builds the function's Service; existingService is the +// currently-deployed Service on update, nil on create. +func (d *Deployer) generateService(f fn.Function, namespace string, daprInstalled bool, deployment *appsv1.Deployment, existingService *corev1.Service) (*corev1.Service, error) { labels, err := deployer.GenerateCommonLabels(f, d.decorator) if err != nil { return nil, err } annotations := deployer.GenerateCommonAnnotations(f, d.decorator, daprInstalled, KubernetesDeployerName) + // re-apply the hostname annotation, contrary to the rest of annotations + // which are "always regenerate" -- the hostname is cluster-derived, not + // in func.yaml: the router mints it, and only the exposure step (after + // the Route is admitted) can write it, which happens after this + // Service write. + if existingService != nil && existingService.Annotations[RouteHostnameAnnotation] != "" { + annotations[RouteHostnameAnnotation] = existingService.Annotations[RouteHostnameAnnotation] + } service := &corev1.Service{ ObjectMeta: metav1.ObjectMeta{ diff --git a/pkg/k8s/deployer_test.go b/pkg/k8s/deployer_test.go index 020abc2d9d..16b6096559 100644 --- a/pkg/k8s/deployer_test.go +++ b/pkg/k8s/deployer_test.go @@ -6,7 +6,10 @@ import ( "testing" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/sets" + dynamicfakeclient "k8s.io/client-go/dynamic/fake" + "k8s.io/client-go/kubernetes/fake" fn "knative.dev/func/pkg/functions" ) @@ -563,3 +566,84 @@ func Test_ProcessVolumes_ValidPath(t *testing.T) { t.Errorf("expected mount path /etc/secret, got %s", mounts[0].MountPath) } } + +// Test_WithDeployerExposureDisabled: exposure is on by default (raw) and off +// with others +func Test_WithDeployerExposureDisabled(t *testing.T) { + if NewDeployer().exposureDisabled { + t.Error("expected exposure enabled on a default Deployer") + } + if !NewDeployer(WithDeployerExposureDisabled()).exposureDisabled { + t.Error("expected exposure disabled with WithDeployerExposureDisabled") + } +} + +// Test_ResolveExposure_RouteGatedOnOpenShift: functions are exposed by +// default, so an explicit expose:route request hard-errors off OpenShift +// (the user asked for something impossible), and expose:none (explicit +// opt-out) never requires OpenShift or touches the Route API at all, on +// either platform - removeExposure's Get against an empty fake dynamic +// client returns NotFound immediately. The unset/empty value off OpenShift +// also stays cluster-local, but silently (no error): the default degrading +// gracefully rather than failing an ordinary deploy is exactly the point. +// +// The "route on OpenShift" and "empty on OpenShift" cases are NOT exercised +// here: both fall through to ensureExposure, which waits up to 30s +// (hardcoded) for a router to admit the Route - a real wait against a fake +// client with no controller to populate status would either hang the test +// for 30s or require simulating async status writes, disproportionate for +// this table. That deeper path (EnsureRoute, WaitForRouteAdmitted, +// GenerateRoute) is covered directly and fast in route_test.go instead, +// each with its own short timeout. +// +// Note: SetOpenShiftForTest mutates a package-level bool without a mutex - +// this test must not run with t.Parallel() (see openshift.go). +func Test_ResolveExposure_RouteGatedOnOpenShift(t *testing.T) { + d := NewDeployer() + f := fn.Function{Name: "f", Deploy: fn.DeploySpec{Namespace: "ns"}} + ctx := t.Context() + clientset := fake.NewClientset() + dynClient := dynamicfakeclient.NewSimpleDynamicClient(runtime.NewScheme()) + + tests := []struct { + name string + expose string + openShift bool + wantErr bool + wantExpose bool + }{ + {name: "route off OpenShift: hard error", expose: "route", openShift: false, wantErr: true}, + {name: "none off OpenShift: fine", expose: "none", openShift: false}, + {name: "none on OpenShift: fine", expose: "none", openShift: true}, + {name: "empty off OpenShift: fine, cluster-local, no error", expose: "", openShift: false}, + // "empty on OpenShift" is NOT in this table: functions are exposed + // by default now, so unset+OpenShift takes the same real + // Route-creation path as explicit expose:route does - excluded + // here for the same reason "route on OpenShift" already is (see + // the comment above this test). + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cleanup := SetOpenShiftForTest(tt.openShift) + defer cleanup() + + f.Deploy.Expose = tt.expose + url, exposed, err := d.resolveExposure(ctx, f, "ns", clientset, dynClient) + if tt.wantErr { + if err == nil { + t.Fatalf("resolveExposure(%q) on OpenShift=%v: expected an error, got nil", tt.expose, tt.openShift) + } + return + } + if err != nil { + t.Fatalf("resolveExposure(%q) on OpenShift=%v: unexpected error: %v", tt.expose, tt.openShift, err) + } + if exposed != tt.wantExpose { + t.Errorf("resolveExposure(%q) on OpenShift=%v: exposed = %v, want %v", tt.expose, tt.openShift, exposed, tt.wantExpose) + } + if url == "" { + t.Errorf("resolveExposure(%q) on OpenShift=%v: expected a non-empty URL", tt.expose, tt.openShift) + } + }) + } +} diff --git a/pkg/k8s/describer.go b/pkg/k8s/describer.go index 14f8468fdf..4054d135ac 100644 --- a/pkg/k8s/describer.go +++ b/pkg/k8s/describer.go @@ -78,7 +78,19 @@ func (d *Describer) Describe(ctx context.Context, name, namespace string) (fn.In } } - primaryRouteURL := fmt.Sprintf("http://%s.%s.svc", name, namespace) // TODO: get correct scheme? + internalURL := fmt.Sprintf("http://%s.%s.svc", name, namespace) + primaryRouteURL := internalURL + + // External hostname (if exposed) was recorded on the Service by Deploy() + // at exposure time - no extra API call or client needed here. + if hostname, ok := service.Annotations[RouteHostnameAnnotation]; ok && hostname != "" { + primaryRouteURL = fmt.Sprintf("https://%s", hostname) + } + // an exposed function stays reachable in-cluster too + routes := []string{primaryRouteURL} + if primaryRouteURL != internalURL { + routes = append(routes, internalURL) + } // get image image := "" @@ -104,7 +116,7 @@ func (d *Describer) Describe(ctx context.Context, name, namespace string) (fn.In Deployer: KubernetesDeployerName, Labels: deployment.Labels, Route: primaryRouteURL, - Routes: []string{primaryRouteURL}, + Routes: routes, Image: image, Middleware: fn.Middleware{ Version: middlewareVersion, diff --git a/pkg/k8s/lister.go b/pkg/k8s/lister.go index 82e0566f88..9175d029c4 100644 --- a/pkg/k8s/lister.go +++ b/pkg/k8s/lister.go @@ -77,12 +77,17 @@ func (l *Lister) get(ctx context.Context, clientset *kubernetes.Clientset, name, return fn.ListItem{}, fmt.Errorf("could not get service: %w", err) } + url := fmt.Sprintf("http://%s.%s.svc", service.Name, service.Namespace) // TODO: use correct scheme + if hostname, ok := service.Annotations[RouteHostnameAnnotation]; ok && hostname != "" { + url = fmt.Sprintf("https://%s", hostname) + } + runtimeLabel := "" listItem := fn.ListItem{ Name: service.Name, Namespace: service.Namespace, Runtime: runtimeLabel, - URL: fmt.Sprintf("http://%s.%s.svc", service.Name, service.Namespace), // TODO: use correct scheme + URL: url, Ready: string(ready), Deployer: KubernetesDeployerName, } diff --git a/pkg/k8s/route.go b/pkg/k8s/route.go new file mode 100644 index 0000000000..e492dbfd37 --- /dev/null +++ b/pkg/k8s/route.go @@ -0,0 +1,268 @@ +package k8s + +import ( + "context" + "fmt" + "os" + "time" + + appsv1 "k8s.io/api/apps/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/client-go/dynamic" + "k8s.io/client-go/util/retry" + + "knative.dev/func/pkg/deployer" + fn "knative.dev/func/pkg/functions" +) + +// routeGVR identifies the OpenShift Route resource. No typed client is used +// here: adding github.com/openshift/api as a direct dependency for a +// handful of fields is disproportionate, this project already has a +// precedent for reading Routes through the dynamic client (see +// pkg/pipelines/tekton/pac/pac.go DetectPACOpenShiftRoute), and there is no +// existing github.com/openshift/api requirement anywhere in go.mod to build +// on. Route's structure is also small and stable (a v1, GA API since +// OpenShift 3.x), so hand-built unstructured content carries little +// maintenance risk. +var routeGVR = schema.GroupVersionResource{ + Group: "route.openshift.io", + Version: "v1", + Resource: "routes", +} + +// GenerateRoute builds (but does not create) the OpenShift Route that +// exposes svcName's "http" port. spec.host is left empty so the cluster's +// router mints one (see docs/research citations in the openshift-route-fork +// records) - custom domains are out of scope for this commit. +func GenerateRoute(f fn.Function, svcName string, deployment *appsv1.Deployment, decorator deployer.DeployDecorator, deployerName string) (*unstructured.Unstructured, error) { + labels, err := deployer.GenerateCommonLabels(f, decorator) + if err != nil { + return nil, err + } + annotations := deployer.GenerateCommonAnnotations(f, decorator, false /* dapr n/a for routing */, deployerName) + + route := &unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": routeGVR.GroupVersion().String(), + "kind": "Route", + "metadata": map[string]any{ + "name": f.Name, + "namespace": deployment.Namespace, + "labels": stringMapToAny(labels), + "annotations": stringMapToAny(annotations), + "ownerReferences": []any{ + map[string]any{ + "apiVersion": appsv1.SchemeGroupVersion.WithKind("Deployment").GroupVersion().String(), + "kind": "Deployment", + "name": deployment.Name, + "uid": string(deployment.UID), + "controller": true, + }, + }, + }, + "spec": map[string]any{ + "to": map[string]any{ + "kind": "Service", + "name": svcName, + }, + "port": map[string]any{ + "targetPort": "http", + }, + // Edge TLS via the router's wildcard cert - zero cert + // management; Redirect upgrades http requests to https. + "tls": map[string]any{ + "termination": "edge", + "insecureEdgeTerminationPolicy": "Redirect", + }, + }, + }, + } + + return route, nil +} + +// stringMapToAny converts a map[string]string to the map[string]any +// unstructured.Unstructured needs its nested fields to be. +func stringMapToAny(m map[string]string) map[string]any { + out := make(map[string]any, len(m)) + for k, v := range m { + out[k] = v + } + return out +} + +// EnsureRoute creates or updates a Route, retrying the whole +// get-mutate-update cycle on a 409 conflict (a controller status write can +// race an update from here). +func EnsureRoute(ctx context.Context, dynClient dynamic.Interface, ns string, route *unstructured.Unstructured) error { + client := dynClient.Resource(routeGVR).Namespace(ns) + name := route.GetName() + + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + existing, getErr := client.Get(ctx, name, metav1.GetOptions{}) + if getErr != nil { + if apierrors.IsNotFound(getErr) { + route.SetResourceVersion("") + _, createErr := client.Create(ctx, route, metav1.CreateOptions{}) + return createErr + } + return getErr + } + route.SetResourceVersion(existing.GetResourceVersion()) + _, updateErr := client.Update(ctx, route, metav1.UpdateOptions{}) + return updateErr + }) + if err != nil { + return fmt.Errorf("failed to ensure Route %q: %w", name, err) + } + return nil +} + +// isManagedRoute reports whether route was created by GenerateRoute() - as +// opposed to a user-authored or third-party Route that happens to share the +// function's name, which must never be deleted out from under the user. +// Both signals are required: a bare boson.dev/function label, or a +// deployer annotation written by some other component, alone does not +// prove func's raw deployer owns the route. +func isManagedRoute(route *unstructured.Unstructured) bool { + return route.GetLabels()["boson.dev/function"] == "true" && + route.GetAnnotations()[deployer.DeployerNameAnnotation] == KubernetesDeployerName +} + +// RemoveManagedRoute deletes the Route named 'name' in 'ns' only if func +// owns it (isManagedRoute()). Returns (removed, error): +// - not found (route absent, or the Route API isn't installed) -> (false, nil) +// - found but not managed -> (false, nil), warning printed, route kept +// - found and managed, deleted -> (true, nil) +func RemoveManagedRoute(ctx context.Context, dynClient dynamic.Interface, ns, name string) (bool, error) { + client := dynClient.Resource(routeGVR).Namespace(ns) + + route, err := client.Get(ctx, name, metav1.GetOptions{}) + if err != nil { + if apierrors.IsNotFound(err) { + return false, nil + } + return false, fmt.Errorf("failed to check for existing Route %q: %w", name, err) + } + + if !isManagedRoute(route) { + fmt.Fprintf(os.Stderr, + "⚠️ a Route named %q exists in namespace %q but is not managed by func - leaving it in place\n", + name, ns) + return false, nil + } + + if err := client.Delete(ctx, name, metav1.DeleteOptions{}); err != nil && !apierrors.IsNotFound(err) { + return false, fmt.Errorf("failed to delete Route %q: %w", name, err) + } + return true, nil +} + +// WaitForRouteAdmitted polls the Route status until any ingress entry (one +// per router/IngressController shard - a cluster can run more than one) +// reports Admitted=True, returning that entry's host. It fails immediately +// (not waiting out the full timeout) only when an ingress entry explicitly +// reports Admitted=False - e.g. a host already claimed by another Route - +// surfacing the condition's reason and message. An entry with no Admitted +// condition yet is polled through to the timeout, fail-open on unknown. +func WaitForRouteAdmitted(ctx context.Context, dynClient dynamic.Interface, ns, name string, timeout time.Duration) (string, error) { + client := dynClient.Resource(routeGVR).Namespace(ns) + + var host string + var lastErr error + pollErr := wait.PollUntilContextTimeout(ctx, time.Second, timeout, true, func(ctx context.Context) (bool, error) { + route, err := client.Get(ctx, name, metav1.GetOptions{}) + if err != nil { + lastErr = fmt.Errorf("failed to get Route %q: %w", name, err) + return false, nil + } + + ingresses, found, err := unstructured.NestedSlice(route.Object, "status", "ingress") + if err != nil || !found { + return false, nil + } + + for _, raw := range ingresses { + ingress, ok := raw.(map[string]any) + if !ok { + continue + } + conditions, found, err := unstructured.NestedSlice(ingress, "conditions") + if err != nil || !found { + continue + } + for _, rawCond := range conditions { + cond, ok := rawCond.(map[string]any) + if !ok || cond["type"] != "Admitted" { + continue + } + status, _ := cond["status"].(string) + switch status { + case "True": + host, _, _ = unstructured.NestedString(ingress, "host") + return true, nil + case "False": + reason, _ := cond["reason"].(string) + message, _ := cond["message"].(string) + lastErr = fmt.Errorf("route %q was rejected by the router: %s: %s", name, reason, message) + return false, lastErr + } + // Unknown or missing status: keep polling. + } + } + + return false, nil + }) + if pollErr != nil { + if lastErr != nil { + return "", lastErr + } + return "", fmt.Errorf("route %q was not admitted by any router within %s: %w", name, timeout, pollErr) + } + return host, nil +} + +// GetAdmittedRouteHost is a single, non-blocking read of a Route's currently +// admitted host, for display paths (describe/list) that must return +// immediately rather than poll like WaitForRouteAdmitted does. Returns +// ("", false, nil) if the Route doesn't exist or has no Admitted=True +// ingress entry yet - both are "no external URL to show", not errors. +func GetAdmittedRouteHost(ctx context.Context, dynClient dynamic.Interface, ns, name string) (string, bool, error) { + route, err := dynClient.Resource(routeGVR).Namespace(ns).Get(ctx, name, metav1.GetOptions{}) + if err != nil { + if apierrors.IsNotFound(err) { + return "", false, nil + } + return "", false, fmt.Errorf("failed to get Route %q: %w", name, err) + } + + ingresses, found, err := unstructured.NestedSlice(route.Object, "status", "ingress") + if err != nil || !found { + return "", false, nil + } + for _, raw := range ingresses { + ingress, ok := raw.(map[string]any) + if !ok { + continue + } + conditions, found, err := unstructured.NestedSlice(ingress, "conditions") + if err != nil || !found { + continue + } + for _, rawCond := range conditions { + cond, ok := rawCond.(map[string]any) + if !ok || cond["type"] != "Admitted" { + continue + } + if status, _ := cond["status"].(string); status == "True" { + host, _, _ := unstructured.NestedString(ingress, "host") + return host, host != "", nil + } + } + } + return "", false, nil +} diff --git a/pkg/k8s/route_test.go b/pkg/k8s/route_test.go new file mode 100644 index 0000000000..85bb44bda6 --- /dev/null +++ b/pkg/k8s/route_test.go @@ -0,0 +1,290 @@ +package k8s + +import ( + "testing" + "time" + + appsv1 "k8s.io/api/apps/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" + dynamicfake "k8s.io/client-go/dynamic/fake" + + fn "knative.dev/func/pkg/functions" +) + +func newFakeDynamicClient(objects ...runtime.Object) *dynamicfake.FakeDynamicClient { + return dynamicfake.NewSimpleDynamicClientWithCustomListKinds( + runtime.NewScheme(), + map[schema.GroupVersionResource]string{routeGVR: "RouteList"}, + objects..., + ) +} + +func testDeployment() *appsv1.Deployment { + return &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "f", + Namespace: "ns", + UID: types.UID("abc-123"), + }, + } +} + +func Test_GenerateRoute(t *testing.T) { + f := fn.Function{Name: "f", Runtime: "go"} + deployment := testDeployment() + + route, err := GenerateRoute(f, "f", deployment, nil, KubernetesDeployerName) + if err != nil { + t.Fatal(err) + } + + if route.GetName() != "f" || route.GetNamespace() != "ns" { + t.Errorf("expected name/namespace f/ns, got %s/%s", route.GetName(), route.GetNamespace()) + } + toName, _, _ := unstructured.NestedString(route.Object, "spec", "to", "name") + if toName != "f" { + t.Errorf("expected spec.to.name %q, got %q", "f", toName) + } + toKind, _, _ := unstructured.NestedString(route.Object, "spec", "to", "kind") + if toKind != "Service" { + t.Errorf("expected spec.to.kind Service, got %q", toKind) + } + targetPort, _, _ := unstructured.NestedString(route.Object, "spec", "port", "targetPort") + if targetPort != "http" { + t.Errorf("expected spec.port.targetPort http, got %q", targetPort) + } + if host, found, _ := unstructured.NestedString(route.Object, "spec", "host"); found && host != "" { + t.Errorf("expected spec.host to be unset (router-minted), got %q", host) + } + termination, _, _ := unstructured.NestedString(route.Object, "spec", "tls", "termination") + if termination != "edge" { + t.Errorf("expected spec.tls.termination edge, got %q", termination) + } + insecurePolicy, _, _ := unstructured.NestedString(route.Object, "spec", "tls", "insecureEdgeTerminationPolicy") + if insecurePolicy != "Redirect" { + t.Errorf("expected spec.tls.insecureEdgeTerminationPolicy Redirect, got %q", insecurePolicy) + } + if !isManagedRoute(route) { + t.Error("expected a freshly generated Route to be self-managed") + } + owners := route.GetOwnerReferences() + if len(owners) != 1 || owners[0].Name != "f" || owners[0].Kind != "Deployment" { + t.Errorf("expected a single Deployment ownerRef named f, got %+v", owners) + } +} + +func Test_EnsureRoute_CreateThenUpdate(t *testing.T) { + ctx := t.Context() + f := fn.Function{Name: "f", Runtime: "go"} + deployment := testDeployment() + client := newFakeDynamicClient() + + route, err := GenerateRoute(f, "f", deployment, nil, KubernetesDeployerName) + if err != nil { + t.Fatal(err) + } + if err := EnsureRoute(ctx, client, "ns", route); err != nil { + t.Fatalf("create: %v", err) + } + got, err := client.Resource(routeGVR).Namespace("ns").Get(ctx, "f", metav1.GetOptions{}) + if err != nil { + t.Fatalf("expected Route to exist after create: %v", err) + } + toName, _, _ := unstructured.NestedString(got.Object, "spec", "to", "name") + if toName != "f" { + t.Errorf("expected spec.to.name f, got %q", toName) + } + + // Update path: regenerate (idempotent) and ensure again, no error. + route2, err := GenerateRoute(f, "f", deployment, nil, KubernetesDeployerName) + if err != nil { + t.Fatal(err) + } + if err := EnsureRoute(ctx, client, "ns", route2); err != nil { + t.Fatalf("update: %v", err) + } +} + +func Test_RemoveManagedRoute(t *testing.T) { + ctx := t.Context() + + t.Run("not found: no-op", func(t *testing.T) { + client := newFakeDynamicClient() + removed, err := RemoveManagedRoute(ctx, client, "ns", "f") + if err != nil || removed { + t.Errorf("expected (false, nil), got (%v, %v)", removed, err) + } + }) + + t.Run("managed: deleted", func(t *testing.T) { + f := fn.Function{Name: "f", Runtime: "go"} + deployment := testDeployment() + route, err := GenerateRoute(f, "f", deployment, nil, KubernetesDeployerName) + if err != nil { + t.Fatal(err) + } + route.SetNamespace("ns") + client := newFakeDynamicClient(route) + + removed, err := RemoveManagedRoute(ctx, client, "ns", "f") + if err != nil || !removed { + t.Fatalf("expected (true, nil), got (%v, %v)", removed, err) + } + if _, err := client.Resource(routeGVR).Namespace("ns").Get(ctx, "f", metav1.GetOptions{}); err == nil { + t.Error("expected Route to be gone after removal") + } + }) + + t.Run("not managed: kept", func(t *testing.T) { + foreign := &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "route.openshift.io/v1", + "kind": "Route", + "metadata": map[string]any{ + "name": "f", + "namespace": "ns", + }, + }} + client := newFakeDynamicClient(foreign) + + removed, err := RemoveManagedRoute(ctx, client, "ns", "f") + if err != nil || removed { + t.Fatalf("expected (false, nil) for a foreign Route, got (%v, %v)", removed, err) + } + if _, err := client.Resource(routeGVR).Namespace("ns").Get(ctx, "f", metav1.GetOptions{}); err != nil { + t.Error("expected the foreign Route to be left in place") + } + }) +} + +func Test_WaitForRouteAdmitted(t *testing.T) { + ctx := t.Context() + + admittedRoute := func(host string) *unstructured.Unstructured { + return &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "route.openshift.io/v1", + "kind": "Route", + "metadata": map[string]any{"name": "f", "namespace": "ns"}, + "status": map[string]any{ + "ingress": []any{ + map[string]any{ + "host": host, + "conditions": []any{ + map[string]any{"type": "Admitted", "status": "True"}, + }, + }, + }, + }, + }} + } + + t.Run("admitted: returns host", func(t *testing.T) { + client := newFakeDynamicClient(admittedRoute("f-ns.apps.example.com")) + host, err := WaitForRouteAdmitted(ctx, client, "ns", "f", time.Second) + if err != nil { + t.Fatal(err) + } + if host != "f-ns.apps.example.com" { + t.Errorf("expected host f-ns.apps.example.com, got %q", host) + } + }) + + t.Run("rejected: fails fast with reason", func(t *testing.T) { + rejected := &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "route.openshift.io/v1", + "kind": "Route", + "metadata": map[string]any{"name": "f", "namespace": "ns"}, + "status": map[string]any{ + "ingress": []any{ + map[string]any{ + "host": "", + "conditions": []any{ + map[string]any{"type": "Admitted", "status": "False", "reason": "HostAlreadyClaimed", "message": "taken"}, + }, + }, + }, + }, + }} + client := newFakeDynamicClient(rejected) + _, err := WaitForRouteAdmitted(ctx, client, "ns", "f", 5*time.Second) + if err == nil { + t.Fatal("expected an error for a rejected Route") + } + }) + + t.Run("never admitted: times out cleanly", func(t *testing.T) { + f := fn.Function{Name: "f", Runtime: "go"} + deployment := testDeployment() + route, err := GenerateRoute(f, "f", deployment, nil, KubernetesDeployerName) + if err != nil { + t.Fatal(err) + } + route.SetNamespace("ns") + client := newFakeDynamicClient(route) + + _, err = WaitForRouteAdmitted(ctx, client, "ns", "f", 100*time.Millisecond) + if err == nil { + t.Fatal("expected a timeout error when no router ever admits the route") + } + }) +} + +func Test_GetAdmittedRouteHost(t *testing.T) { + ctx := t.Context() + + admittedRoute := func(host string) *unstructured.Unstructured { + return &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "route.openshift.io/v1", + "kind": "Route", + "metadata": map[string]any{"name": "f", "namespace": "ns"}, + "status": map[string]any{ + "ingress": []any{ + map[string]any{ + "host": host, + "conditions": []any{ + map[string]any{"type": "Admitted", "status": "True"}, + }, + }, + }, + }, + }} + } + + t.Run("admitted: returns host", func(t *testing.T) { + client := newFakeDynamicClient(admittedRoute("f-ns.apps.example.com")) + host, ok, err := GetAdmittedRouteHost(ctx, client, "ns", "f") + if err != nil { + t.Fatal(err) + } + if !ok || host != "f-ns.apps.example.com" { + t.Errorf("expected (f-ns.apps.example.com, true), got (%q, %v)", host, ok) + } + }) + + t.Run("not found: no error, not found", func(t *testing.T) { + client := newFakeDynamicClient() + host, ok, err := GetAdmittedRouteHost(ctx, client, "ns", "f") + if err != nil || ok || host != "" { + t.Errorf("expected (\"\", false, nil), got (%q, %v, %v)", host, ok, err) + } + }) + + t.Run("not yet admitted: no error, not found", func(t *testing.T) { + f := fn.Function{Name: "f", Runtime: "go"} + deployment := testDeployment() + route, err := GenerateRoute(f, "f", deployment, nil, KubernetesDeployerName) + if err != nil { + t.Fatal(err) + } + route.SetNamespace("ns") + client := newFakeDynamicClient(route) + + host, ok, err := GetAdmittedRouteHost(ctx, client, "ns", "f") + if err != nil || ok || host != "" { + t.Errorf("expected (\"\", false, nil) for an unadmitted route, got (%q, %v, %v)", host, ok, err) + } + }) +} diff --git a/pkg/keda/deployer.go b/pkg/keda/deployer.go index bb03c35176..7fff29a7a6 100644 --- a/pkg/keda/deployer.go +++ b/pkg/keda/deployer.go @@ -37,6 +37,11 @@ func NewDeployer(opts ...DeployerOpt) *Deployer { Deployer: *k8s.NewDeployer( // init with the kedaDeployerDecorator to have the correct deployer labels&annotations k8s.WithDeployerDecorator(&kedaDeployerDecorator{}), + // keda functions stay behind the interceptor; this deployer + // mints its own Route separately (see route.go) rather than + // letting the embedded raw deployer expose the function's own + // Service directly, which would bypass the interceptor entirely + k8s.WithDeployerExposureDisabled(), ), } diff --git a/pkg/knative/deployer.go b/pkg/knative/deployer.go index 7d47065873..8082864f48 100644 --- a/pkg/knative/deployer.go +++ b/pkg/knative/deployer.go @@ -292,7 +292,7 @@ consider using the --image-pull-secret flag, or setting up pull secrets manually } if d.verbose { - fmt.Printf("Function deployed in namespace %q and exposed at URL:\n%s\n", namespace, route.Status.URL.String()) + fmt.Printf("Function deployed in namespace %q at URL:\n%s\n", namespace, route.Status.URL.String()) } return fn.DeploymentResult{ Status: fn.Deployed, diff --git a/pkg/lister/testing/integration_test_helper.go b/pkg/lister/testing/integration_test_helper.go index d063fb175f..e794d0f452 100644 --- a/pkg/lister/testing/integration_test_helper.go +++ b/pkg/lister/testing/integration_test_helper.go @@ -39,6 +39,10 @@ func TestInt_List(t *testing.T, lister fn.Lister, deployer fn.Deployer, describe Runtime: "go", Namespace: ns, Registry: Registry(), + // Explicit opt-out: keeps this integration deploy cluster-local and + // platform-deterministic under exposed-by-default; ignored entirely + // by the knative deployer. + Deploy: fn.DeploySpec{Expose: "none"}, }) if err != nil { t.Fatal(err) diff --git a/pkg/mock/deployer.go b/pkg/mock/deployer.go index 4398faeeec..74ccd5badc 100644 --- a/pkg/mock/deployer.go +++ b/pkg/mock/deployer.go @@ -39,6 +39,9 @@ func NewDeployer() *Deployer { } else { result.Deployer = f.Deploy.Deployer // redeploy with current } + if err == nil { + result.Status = fn.Deployed + } return }, } diff --git a/pkg/pipelines/tekton/pipelines_provider.go b/pkg/pipelines/tekton/pipelines_provider.go index 186a2dd2cf..4758380e03 100644 --- a/pkg/pipelines/tekton/pipelines_provider.go +++ b/pkg/pipelines/tekton/pipelines_provider.go @@ -275,11 +275,14 @@ func (pp *PipelinesProvider) Run(ctx context.Context, f fn.Function) (string, fn return "", f, fmt.Errorf("problem in retrieving status of deployed function: %v", err) } - if obj.Generation == 1 { - fmt.Fprintf(os.Stderr, "✅ Function deployed in namespace %q and exposed at URL: \n %s\n", obj.Namespace, obj.Route) - } else { - fmt.Fprintf(os.Stderr, "✅ Function updated in namespace %q and exposed at URL: \n %s\n", obj.Namespace, obj.Route) - } + verb := "deployed" + if obj.Generation != 1 { + verb = "updated" + } + // Mirrors the deploy status message in pkg/functions/client.go's Deploy - + // same neutral wording, no exposure claim (duplicated rather than + // exported+imported across packages for one format string). + fmt.Fprintf(os.Stderr, "✅ Function %s in namespace %q at URL: \n %s\n", verb, obj.Namespace, obj.Route) if obj.Namespace != namespace { fmt.Fprintf(os.Stderr, "Warning: Final function namespace %q does not match expected %q", obj.Namespace, namespace) diff --git a/pkg/remover/testing/integration_test_helper.go b/pkg/remover/testing/integration_test_helper.go index 07d9274825..ce0634048c 100644 --- a/pkg/remover/testing/integration_test_helper.go +++ b/pkg/remover/testing/integration_test_helper.go @@ -39,6 +39,7 @@ func TestInt_Remove(t *testing.T, remover fn.Remover, deployer fn.Deployer, desc Runtime: "go", Namespace: ns, Registry: Registry(), + Deploy: fn.DeploySpec{Expose: "none"}, }) if err != nil { t.Fatal(err) diff --git a/schema/func_yaml-schema.json b/schema/func_yaml-schema.json index 044a19b699..79e7af6b3d 100644 --- a/schema/func_yaml-schema.json +++ b/schema/func_yaml-schema.json @@ -130,6 +130,10 @@ "managementDisabled": { "type": "boolean", "description": "ManagementDisabled disables automatic creation/update of a Function CR\nfor operator management after deploy. The zero value (false) means\nthe function is managed by default when the func-operator is installed." + }, + "expose": { + "type": "string", + "description": "Expose controls external access for the raw and keda deployers (the\nknative deployer manages its own exposure and ignores it). Optional.\nValues: \"route\" (create an OpenShift Route; OpenShift clusters only -\na hard error elsewhere), \"none\" (cluster-local only, explicit\nopt-out). Defaults to \"route\" behavior on OpenShift - a deployed\nfunction being externally reachable is the expected outcome - and to\ncluster-local on any other cluster, since a Route is an\nOpenShift-only mechanism and the unset default must not impose a\nplatform requirement." } }, "additionalProperties": false, From d2880a995a304d6d3438fd43a7eeb3aa45b08351 Mon Sep 17 00:00:00 2001 From: David Fridrich Date: Mon, 13 Jul 2026 20:31:31 +0200 Subject: [PATCH 2/2] feat: keda deployer OpenShift Route via shared interceptor namespace --- cmd/deploy.go | 11 +- pkg/functions/function.go | 3 +- pkg/functions/function_expose.go | 24 +-- pkg/functions/function_expose_unit_test.go | 48 ++++++ pkg/k8s/deployer.go | 49 +++--- pkg/k8s/deployer_test.go | 60 +++++++ pkg/k8s/route.go | 39 +++-- pkg/k8s/route_test.go | 8 +- pkg/k8s/wait.go | 2 +- pkg/keda/deployer.go | 64 +++++++- pkg/keda/deployer_test.go | 95 +++++++++++ pkg/keda/describer.go | 15 +- pkg/keda/lister.go | 13 +- pkg/keda/remover.go | 35 ++++- pkg/keda/remover_test.go | 108 +++++++++++++ pkg/keda/route.go | 172 ++++++++++++++++++++ pkg/keda/route_test.go | 175 +++++++++++++++++++++ schema/func_yaml-schema.json | 4 + 18 files changed, 854 insertions(+), 71 deletions(-) create mode 100644 pkg/keda/deployer_test.go create mode 100644 pkg/keda/remover_test.go create mode 100644 pkg/keda/route.go create mode 100644 pkg/keda/route_test.go diff --git a/cmd/deploy.go b/cmd/deploy.go index 5d91b3a755..538dcc128b 100644 --- a/cmd/deploy.go +++ b/cmd/deploy.go @@ -295,7 +295,8 @@ func runDeploy(cmd *cobra.Command, newClient ClientFactory) (err error) { // Warn if registry changed but registryInsecure is still true warnRegistryInsecureChange(cmd.OutOrStderr(), cfg.Registry, f) - // Warn if expose flag is used with deployer where it has no effect + // Warn if deploy.expose is set (by flag or persisted in func.yaml) for a + // deployer that ignores it warnExposeIgnore(cmd.OutOrStderr(), cfg.Expose, cfg.Deployer) // Back-compat: a function deployed before the deployer was recorded has a @@ -935,9 +936,11 @@ func isDigested(v string) (validDigest bool, err error) { return ok, nil } -// warnExposeIgnore warns when non raw|keda deployer is used with Expose flag -// where it is simply ignored and has no effect. An empty deployer means the -// default (knative), which also ignores expose. +// warnExposeIgnore warns when a non-empty deploy.expose is paired with a +// deployer that ignores it. The value is the RESOLVED one, not just what the +// user typed: the --expose flag registers f.Deploy.Expose as its own default, +// so a value persisted in func.yaml warns on its own with no flag present. +// An empty deployer means the default (knative), which also ignores expose. func warnExposeIgnore(w io.Writer, expose, deployer string) { if expose != "" && deployer != k8s.KubernetesDeployerName && deployer != keda.KedaDeployerName { diff --git a/pkg/functions/function.go b/pkg/functions/function.go index f9273760b2..996410272b 100644 --- a/pkg/functions/function.go +++ b/pkg/functions/function.go @@ -287,7 +287,7 @@ type DeploySpec struct { // cluster-local on any other cluster, since a Route is an // OpenShift-only mechanism and the unset default must not impose a // platform requirement. - Expose string `yaml:"expose,omitempty"` + Expose string `yaml:"expose,omitempty" jsonschema:"enum=route,enum=none"` } // HealthEndpoints specify the liveness and readiness endpoints for a Runtime @@ -414,6 +414,7 @@ func (f Function) Validate() error { ValidateBuildEnvs(f.Build.BuildEnvs), ValidateEnvs(f.Run.Envs), validateOptions(f.Deploy.Options), + validateExpose(f.Deploy.Expose), ValidateLabels(f.Deploy.Labels), validateGit(f.Build.Git), validateKafka(f.Run.Kafka, f.Invoke, f.Runtime), diff --git a/pkg/functions/function_expose.go b/pkg/functions/function_expose.go index 104bddcb74..f22a561cf7 100644 --- a/pkg/functions/function_expose.go +++ b/pkg/functions/function_expose.go @@ -4,18 +4,24 @@ import ( "fmt" ) -// ValidateExpose reports whether expose is a valid deploy.expose value: "" -// (default - exposed via an OpenShift Route, since a deployed function -// being reachable is the expected outcome; cluster-local on non-OpenShift -// clusters, since a Route is an OpenShift-only mechanism), "none" -// (cluster-local, explicit opt-out), or "route" (explicit request for an -// OpenShift Route). There is no ref suffix: an OpenShift Route has no -// concept of "which ingress controller to attach to" - the cluster's -// IngressController picks the router, and the Route object doesn't -// reference one. Any other value is rejected. +// ValidateExpose checks a deploy.expose value. Valid are "route" for an +// OpenShift Route, "none" for cluster-local only, and "" which means route on +// OpenShift and none anywhere else. Anything else is an error. func ValidateExpose(expose string) error { if expose == "" || expose == "none" || expose == "route" { return nil } return fmt.Errorf("%w: %q", ErrInvalidExpose, expose) } + +// validateExpose is the Function.Validate() form: messages rather than an +// error. It is the only check callers get when they never run the CLI's own +// flag validation - the --remote path, and library callers building a +// Function directly. +func validateExpose(expose string) (errors []string) { + if err := ValidateExpose(expose); err != nil { + errors = append(errors, fmt.Sprintf( + "specified option \"deploy.expose=%s\" is not valid, allowed values are \"route\", \"none\" or empty", expose)) + } + return +} diff --git a/pkg/functions/function_expose_unit_test.go b/pkg/functions/function_expose_unit_test.go index 0af532c640..0b1cddea29 100644 --- a/pkg/functions/function_expose_unit_test.go +++ b/pkg/functions/function_expose_unit_test.go @@ -28,3 +28,51 @@ func Test_ValidateExpose(t *testing.T) { }) } } + +// Test_validateExpose asserts valid input yields no errors, and invalid input +// exactly one, containing the offending value. +func Test_validateExpose(t *testing.T) { + for _, v := range []string{"", "route", "none"} { + t.Run(v, func(t *testing.T) { + if errs := validateExpose(v); len(errs) != 0 { + t.Fatalf("validateExpose(%q): expected no errors, got %v", v, errs) + } + }) + } + + for _, v := range []string{"auto", "bogus", "ingress"} { + t.Run(v, func(t *testing.T) { + errs := validateExpose(v) + if len(errs) != 1 { + t.Fatalf("validateExpose(%q): expected exactly one error, got %v", v, errs) + } + if !strings.Contains(errs[0], v) { + t.Errorf("validateExpose(%q): expected the message to name the bad value, got %q", v, errs[0]) + } + }) + } +} + +// Test_Validate_Expose asserts Function.Validate() accepts valid deploy.expose +// values and rejects invalid ones, covering use of Function as a library. +func Test_Validate_Expose(t *testing.T) { + for _, v := range []string{"", "route", "none"} { + t.Run("valid/"+v, func(t *testing.T) { + f := Function{Root: "/tmp/fn", Deploy: DeploySpec{Expose: v}} + if err := f.Validate(); err != nil { + t.Fatalf("expected deploy.expose=%q to validate, got %v", v, err) + } + }) + } + + t.Run("invalid surfaces through Function.Validate", func(t *testing.T) { + f := Function{Root: "/tmp/fn", Deploy: DeploySpec{Expose: "bogus"}} + err := f.Validate() + if err == nil { + t.Fatal("expected an invalid deploy.expose to fail Function.Validate()") + } + if !strings.Contains(err.Error(), "deploy.expose=bogus") { + t.Errorf("expected the bundled error to name the offending field and value, got %v", err) + } + }) +} diff --git a/pkg/k8s/deployer.go b/pkg/k8s/deployer.go index 231cd44a68..0f54ada846 100644 --- a/pkg/k8s/deployer.go +++ b/pkg/k8s/deployer.go @@ -359,7 +359,7 @@ func (d *Deployer) resolveExposure(ctx context.Context, f fn.Function, namespace // GET/DELETE prints a warning and the deploy continues, since keda // users without Route permissions must stay green. func (d *Deployer) removeExposure(ctx context.Context, clientset kubernetes.Interface, dynClient dynamic.Interface, namespace, name string, enforce bool) error { - if _, err := RemoveManagedRoute(ctx, dynClient, namespace, name); err != nil { + if _, err := RemoveManagedRoute(ctx, dynClient, namespace, name, KubernetesDeployerName); err != nil { if !enforce && errors.IsForbidden(err) { fmt.Fprintf(os.Stderr, "⚠️ cannot remove Route %q (forbidden) - leaving it in place\n", name) } else { @@ -725,52 +725,55 @@ func (d *Deployer) generateService(f fn.Function, namespace string, daprInstalle return service, nil } +// referenceCheckMessage returns the message for a failed reference check: a +// Forbidden error means the resource could not be checked, any other error +// that it is not there. Both still fail the deploy, so this picks the wording +// only. +func referenceCheckMessage(kind, name, namespace string, err error) string { + if errors.IsForbidden(err) { + return fmt.Sprintf(" referenced %s %q in namespace %q could not be checked: access is denied. "+ + "Ensure the service account has permission to get it.\n", kind, name, namespace) + } + return fmt.Sprintf(" referenced %s %q is not present in namespace %q\n", kind, name, namespace) +} + // CheckResourcesArePresent returns error if Secrets or ConfigMaps // referenced in input sets are not deployed on the cluster in the specified namespace func CheckResourcesArePresent(ctx context.Context, namespace string, referencedSecrets, referencedConfigMaps, referencedPVCs *sets.Set[string], referencedServiceAccount, imagePullSecret string) error { - errMsg := "" + var msgs strings.Builder for s := range *referencedSecrets { - _, err := GetSecret(ctx, s, namespace) - if err != nil { - if errors.IsForbidden(err) { - errMsg += " Ensure that the service account has the necessary permissions to access the secret.\n" - } else { - errMsg += fmt.Sprintf(" referenced Secret \"%s\" is not present in namespace \"%s\"\n", s, namespace) - } + if _, err := GetSecret(ctx, s, namespace); err != nil { + msgs.WriteString(referenceCheckMessage("Secret", s, namespace, err)) } } for cm := range *referencedConfigMaps { - _, err := GetConfigMap(ctx, cm, namespace) - if err != nil { - errMsg += fmt.Sprintf(" referenced ConfigMap \"%s\" is not present in namespace \"%s\"\n", cm, namespace) + if _, err := GetConfigMap(ctx, cm, namespace); err != nil { + msgs.WriteString(referenceCheckMessage("ConfigMap", cm, namespace, err)) } } for pvc := range *referencedPVCs { - _, err := GetPersistentVolumeClaim(ctx, pvc, namespace) - if err != nil { - errMsg += fmt.Sprintf(" referenced PersistentVolumeClaim \"%s\" is not present in namespace \"%s\"\n", pvc, namespace) + if _, err := GetPersistentVolumeClaim(ctx, pvc, namespace); err != nil { + msgs.WriteString(referenceCheckMessage("PersistentVolumeClaim", pvc, namespace, err)) } } // check if referenced ServiceAccount is present in the namespace if it is not default if referencedServiceAccount != "" && referencedServiceAccount != "default" { - err := GetServiceAccount(ctx, referencedServiceAccount, namespace) - if err != nil { - errMsg += fmt.Sprintf(" referenced ServiceAccount \"%s\" is not present in namespace \"%s\"\n", referencedServiceAccount, namespace) + if err := GetServiceAccount(ctx, referencedServiceAccount, namespace); err != nil { + msgs.WriteString(referenceCheckMessage("ServiceAccount", referencedServiceAccount, namespace, err)) } } if imagePullSecret != "" { - _, err := GetSecret(ctx, imagePullSecret, namespace) - if err != nil { - errMsg += fmt.Sprintf(" referenced image pull Secret \"%s\" is not present in namespace \"%s\"\n", imagePullSecret, namespace) + if _, err := GetSecret(ctx, imagePullSecret, namespace); err != nil { + msgs.WriteString(referenceCheckMessage("image pull Secret", imagePullSecret, namespace, err)) } } - if errMsg != "" { - return fmt.Errorf("error(s) while validating resources:\n%s", errMsg) + if msgs.Len() > 0 { + return fmt.Errorf("error(s) while validating resources:\n%s", msgs.String()) } return nil diff --git a/pkg/k8s/deployer_test.go b/pkg/k8s/deployer_test.go index 16b6096559..5d1e87a9c8 100644 --- a/pkg/k8s/deployer_test.go +++ b/pkg/k8s/deployer_test.go @@ -6,7 +6,9 @@ import ( "testing" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/sets" dynamicfakeclient "k8s.io/client-go/dynamic/fake" "k8s.io/client-go/kubernetes/fake" @@ -647,3 +649,61 @@ func Test_ResolveExposure_RouteGatedOnOpenShift(t *testing.T) { }) } } + +// Test_referenceCheckMessage asserts that, for every resource kind, a Forbidden +// error yields the access-denied wording and any other error the not-present +// wording, and that both name the kind, the resource and the namespace. +func Test_referenceCheckMessage(t *testing.T) { + kinds := []struct { + kind string + resource string + }{ + {"Secret", "secrets"}, + {"ConfigMap", "configmaps"}, + {"PersistentVolumeClaim", "persistentvolumeclaims"}, + {"ServiceAccount", "serviceaccounts"}, + {"image pull Secret", "secrets"}, + } + + for _, k := range kinds { + gr := schema.GroupResource{Resource: k.resource} + + t.Run(k.kind+"/forbidden", func(t *testing.T) { + msg := referenceCheckMessage(k.kind, "my-res", "my-ns", apierrors.NewForbidden(gr, "my-res", nil)) + + if strings.Contains(msg, "is not present") { + t.Errorf("a forbidden GET must not claim the resource is absent, got %q", msg) + } + if !strings.Contains(msg, "denied") { + t.Errorf("expected the message to say access was denied, got %q", msg) + } + if !strings.Contains(msg, k.kind) || !strings.Contains(msg, "my-res") || !strings.Contains(msg, "my-ns") { + t.Errorf("expected the message to name kind, resource and namespace, got %q", msg) + } + }) + + t.Run(k.kind+"/absent", func(t *testing.T) { + msg := referenceCheckMessage(k.kind, "my-res", "my-ns", apierrors.NewNotFound(gr, "my-res")) + + if !strings.Contains(msg, "is not present") { + t.Errorf("a genuinely absent resource must be reported as not present, got %q", msg) + } + if strings.Contains(msg, "denied") { + t.Errorf("an absent resource must not be reported as a permissions problem, got %q", msg) + } + if !strings.Contains(msg, k.kind) || !strings.Contains(msg, "my-res") || !strings.Contains(msg, "my-ns") { + t.Errorf("expected the message to name kind, resource and namespace, got %q", msg) + } + }) + } + + // A timeout or a conflict must not be reported as a permissions problem. + t.Run("other errors read as absent", func(t *testing.T) { + msg := referenceCheckMessage("Secret", "my-res", "my-ns", + apierrors.NewTimeoutError("too slow", 1)) + if !strings.Contains(msg, "is not present") || strings.Contains(msg, "denied") { + t.Errorf("expected the not-present wording for a non-forbidden error, got %q", msg) + } + }) + +} diff --git a/pkg/k8s/route.go b/pkg/k8s/route.go index e492dbfd37..0f1afdb887 100644 --- a/pkg/k8s/route.go +++ b/pkg/k8s/route.go @@ -122,23 +122,29 @@ func EnsureRoute(ctx context.Context, dynClient dynamic.Interface, ns string, ro return nil } -// isManagedRoute reports whether route was created by GenerateRoute() - as +// isManagedRoute reports whether route was created by func's own +// deployerName (GenerateRoute(), or the keda-specific equivalent that +// targets the interceptor rather than the function's own Service) - as // opposed to a user-authored or third-party Route that happens to share the -// function's name, which must never be deleted out from under the user. -// Both signals are required: a bare boson.dev/function label, or a -// deployer annotation written by some other component, alone does not -// prove func's raw deployer owns the route. -func isManagedRoute(route *unstructured.Unstructured) bool { +// same name, which must never be deleted out from under the user. Both +// signals are required: a bare boson.dev/function label, or a deployer +// annotation written by some other component, alone does not prove func +// owns the route. deployerName is checked exactly (not "any func +// deployer"): a raw deploy must never delete a Route keda's own deployer +// manages, or vice versa, since the two live under different lifecycle +// rules (ownerRef GC vs explicit label-based cleanup - see +// RemoveManagedRoute). +func isManagedRoute(route *unstructured.Unstructured, deployerName string) bool { return route.GetLabels()["boson.dev/function"] == "true" && - route.GetAnnotations()[deployer.DeployerNameAnnotation] == KubernetesDeployerName + route.GetAnnotations()[deployer.DeployerNameAnnotation] == deployerName } -// RemoveManagedRoute deletes the Route named 'name' in 'ns' only if func -// owns it (isManagedRoute()). Returns (removed, error): +// RemoveManagedRoute deletes the Route named 'name' in 'ns' only if +// deployerName owns it (isManagedRoute()). Returns (removed, error): // - not found (route absent, or the Route API isn't installed) -> (false, nil) -// - found but not managed -> (false, nil), warning printed, route kept +// - found but not managed by deployerName -> (false, nil), warning printed, route kept // - found and managed, deleted -> (true, nil) -func RemoveManagedRoute(ctx context.Context, dynClient dynamic.Interface, ns, name string) (bool, error) { +func RemoveManagedRoute(ctx context.Context, dynClient dynamic.Interface, ns, name, deployerName string) (bool, error) { client := dynClient.Resource(routeGVR).Namespace(ns) route, err := client.Get(ctx, name, metav1.GetOptions{}) @@ -149,10 +155,15 @@ func RemoveManagedRoute(ctx context.Context, dynClient dynamic.Interface, ns, na return false, fmt.Errorf("failed to check for existing Route %q: %w", name, err) } - if !isManagedRoute(route) { + if !isManagedRoute(route, deployerName) { + // Not "not managed by func": isManagedRoute checks ownership by THIS + // deployerName, so a Route func created under a different deployer + // (raw vs keda) lands here too, and saying otherwise would be wrong in + // exactly the raw/keda switch this check exists to protect. fmt.Fprintf(os.Stderr, - "⚠️ a Route named %q exists in namespace %q but is not managed by func - leaving it in place\n", - name, ns) + "⚠️ a Route named %q exists in namespace %q but func's %q deployer does not own it "+ + "(user-authored, or created by a different deployer) - leaving it in place\n", + name, ns, deployerName) return false, nil } diff --git a/pkg/k8s/route_test.go b/pkg/k8s/route_test.go index 85bb44bda6..c2a8302ea7 100644 --- a/pkg/k8s/route_test.go +++ b/pkg/k8s/route_test.go @@ -68,7 +68,7 @@ func Test_GenerateRoute(t *testing.T) { if insecurePolicy != "Redirect" { t.Errorf("expected spec.tls.insecureEdgeTerminationPolicy Redirect, got %q", insecurePolicy) } - if !isManagedRoute(route) { + if !isManagedRoute(route, KubernetesDeployerName) { t.Error("expected a freshly generated Route to be self-managed") } owners := route.GetOwnerReferences() @@ -114,7 +114,7 @@ func Test_RemoveManagedRoute(t *testing.T) { t.Run("not found: no-op", func(t *testing.T) { client := newFakeDynamicClient() - removed, err := RemoveManagedRoute(ctx, client, "ns", "f") + removed, err := RemoveManagedRoute(ctx, client, "ns", "f", KubernetesDeployerName) if err != nil || removed { t.Errorf("expected (false, nil), got (%v, %v)", removed, err) } @@ -130,7 +130,7 @@ func Test_RemoveManagedRoute(t *testing.T) { route.SetNamespace("ns") client := newFakeDynamicClient(route) - removed, err := RemoveManagedRoute(ctx, client, "ns", "f") + removed, err := RemoveManagedRoute(ctx, client, "ns", "f", KubernetesDeployerName) if err != nil || !removed { t.Fatalf("expected (true, nil), got (%v, %v)", removed, err) } @@ -150,7 +150,7 @@ func Test_RemoveManagedRoute(t *testing.T) { }} client := newFakeDynamicClient(foreign) - removed, err := RemoveManagedRoute(ctx, client, "ns", "f") + removed, err := RemoveManagedRoute(ctx, client, "ns", "f", KubernetesDeployerName) if err != nil || removed { t.Fatalf("expected (false, nil) for a foreign Route, got (%v, %v)", removed, err) } diff --git a/pkg/k8s/wait.go b/pkg/k8s/wait.go index 4d46736331..ca0f7ec295 100644 --- a/pkg/k8s/wait.go +++ b/pkg/k8s/wait.go @@ -50,7 +50,7 @@ func WaitForDeploymentAvailableBySelector(ctx context.Context, clientset *kubern }) } -func WaitForServiceRemoved(ctx context.Context, clientset *kubernetes.Clientset, namespace, name string, timeout time.Duration) error { +func WaitForServiceRemoved(ctx context.Context, clientset kubernetes.Interface, namespace, name string, timeout time.Duration) error { return wait.PollUntilContextTimeout(ctx, 1*time.Second, timeout, true, func(ctx context.Context) (bool, error) { _, err := clientset.CoreV1().Services(namespace).Get(ctx, name, metav1.GetOptions{}) if errors.IsNotFound(err) { diff --git a/pkg/keda/deployer.go b/pkg/keda/deployer.go index 7fff29a7a6..cab7befd5e 100644 --- a/pkg/keda/deployer.go +++ b/pkg/keda/deployer.go @@ -3,6 +3,7 @@ package keda import ( "context" "fmt" + "slices" "time" httpv1alpha1 "github.com/kedacore/http-add-on/operator/apis/http/v1alpha1" @@ -37,10 +38,10 @@ func NewDeployer(opts ...DeployerOpt) *Deployer { Deployer: *k8s.NewDeployer( // init with the kedaDeployerDecorator to have the correct deployer labels&annotations k8s.WithDeployerDecorator(&kedaDeployerDecorator{}), - // keda functions stay behind the interceptor; this deployer - // mints its own Route separately (see route.go) rather than - // letting the embedded raw deployer expose the function's own - // Service directly, which would bypass the interceptor entirely + // Traffic has to reach the function through the interceptor, so + // the embedded raw deployer must not expose the function's own + // Service. This deployer creates its own Route to the + // interceptor instead, in route.go. k8s.WithDeployerExposureDisabled(), ), } @@ -130,13 +131,64 @@ func (d *Deployer) Deploy(ctx context.Context, f fn.Function) (fn.DeploymentResu d.interceptorBridgeServiceName(f), } + tech := f.Deploy.Expose + + dynClient, err := k8s.NewDynamicClient() + if err != nil { + return fn.DeploymentResult{}, fmt.Errorf("failed to create dynamic client: %w", err) + } + + exposedURL := fmt.Sprintf("http://%s:8080", hosts[0]) + removeStaleRoute := false + + // "route" is an explicit request; unset means yes on OpenShift, no anywhere + // else. Otherwise remove any Route an earlier deploy created. Both branches are + // OpenShift-only because Routes exist nowhere else. + // An explicit expose:route on non-OpenShift cluster never reaches here, it + // would have been rejected in the embedded raw deployer - k8s.Deployer.resolveExposure() + if tech == "route" || (tech == "" && k8s.IsOpenShift()) { + host, err := ensureInterceptorRoute(ctx, dynClient, f, namespace, d.decorator) + if err != nil { + return fn.DeploymentResult{}, fmt.Errorf("failed to ensure interceptor Route: %w", err) + } + // The interceptor matches on the literal Host header, so the Route's + // hostname has to be in hosts as well. + if !slices.Contains(hosts, host) { + hosts = append(hosts, host) + } + // The Route redirects http to https (see generateInterceptorRoute). + exposedURL = fmt.Sprintf("https://%s", host) + } else if k8s.IsOpenShift() { + // Not exposed, so a Route from an earlier deploy must go. Deferred until + // after the scaler exists: see the removal below. + removeStaleRoute = true + } + if err := d.ensureHTTPScaledObject(ctx, f, namespace, deployment, appService, hosts); err != nil { return fn.DeploymentResult{}, fmt.Errorf("failed to ensure http scaled object exists: %w", err) } + // Removing the stale Route runs LAST, after the HTTPScaledObject exists, for the + // same reason remove() deletes it last: the call needs Route permissions in the + // interceptor namespace, and a Forbidden there must not leave the function + // deployed but unscaled. Ordered this way, the error is still fatal - an + // explicit expose:none that cannot be honoured has to be reported - but the + // function it leaves behind is complete rather than half-configured. + // + // A Route that outlives this call is inert, not a leak of exposure: the + // interceptor matches the literal Host header against the hosts registered by + // an HTTPScaledObject, and expose:none registers only the cluster-local bridge + // hosts. Verified on OpenShift with the interceptor's own Route recreated by + // hand: that hostname answers 404 while the bridge answers 200. + if removeStaleRoute { + if err := removeInterceptorRoute(ctx, dynClient, f.Name, namespace); err != nil { + return fn.DeploymentResult{}, err + } + } + return fn.DeploymentResult{ Status: deployResult.Status, - URL: fmt.Sprintf("http://%s:8080", hosts[0]), // TODO: check on HTTPS too + URL: exposedURL, Namespace: deployResult.Namespace, Deployer: KedaDeployerName, }, nil @@ -231,7 +283,7 @@ func (d *Deployer) interceptorBridgeService(f fn.Function, namespace string, dep }, Spec: corev1.ServiceSpec{ Type: corev1.ServiceTypeExternalName, - ExternalName: "keda-add-ons-http-interceptor-proxy.keda.svc.cluster.local", + ExternalName: fmt.Sprintf("%s.%s.svc.cluster.local", interceptorServiceName, interceptorNamespace()), }, } } diff --git a/pkg/keda/deployer_test.go b/pkg/keda/deployer_test.go new file mode 100644 index 0000000000..02ea59f125 --- /dev/null +++ b/pkg/keda/deployer_test.go @@ -0,0 +1,95 @@ +package keda + +import ( + "testing" + + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + fn "knative.dev/func/pkg/functions" + "knative.dev/func/pkg/k8s" +) + +// Test_interceptorNamespace asserts openshift-keda and keda stay distinct and +// that interceptorNamespace returns the one matching the cluster type. Both +// branches are forced rather than read from the ambient kubeconfig, so +// openshift-keda is proven on CI too, where detection always reports +// not-OpenShift and that branch would otherwise never run. +func Test_interceptorNamespace(t *testing.T) { + if interceptorNamespaceOpenShift == interceptorNamespaceUpstream { + t.Fatal("the two install namespaces must differ, or resolving between them is pointless") + } + if interceptorNamespaceOpenShift != "openshift-keda" { + t.Errorf("expected the Custom Metrics Autoscaler namespace, got %q", interceptorNamespaceOpenShift) + } + if interceptorNamespaceUpstream != "keda" { + t.Errorf("expected the upstream helm chart namespace, got %q", interceptorNamespaceUpstream) + } + + // Not parallel: SetOpenShiftForTest mutates a package-level bool without a + // mutex. See openshift.go:SetOpenShiftForTest. + for _, tt := range []struct { + name string + openShift bool + want string + }{ + {"OpenShift runs the Custom Metrics Autoscaler", true, interceptorNamespaceOpenShift}, + {"elsewhere runs the upstream helm chart", false, interceptorNamespaceUpstream}, + } { + t.Run(tt.name, func(t *testing.T) { + defer k8s.SetOpenShiftForTest(tt.openShift)() + if got := interceptorNamespace(); got != tt.want { + t.Errorf("expected %q, got %q", tt.want, got) + } + }) + } +} + +// Test_interceptorBridgeService asserts the bridge Service is an ExternalName +// addressing the interceptor in interceptorNamespace, sits in the function's +// own namespace, and is owned by the function's Deployment. +func Test_interceptorBridgeService(t *testing.T) { + d := NewDeployer() + f := fn.Function{Name: "f", Runtime: "go"} + deployment := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "f", Namespace: "fn-ns", UID: "deployment-uid"}, + } + + // Full literals, never interceptorNamespace(): deriving the expectation from + // the code under test would pass just as happily if the resolver returned + // the wrong namespace. The non-OpenShift value must stay byte-identical to + // the constant this rung replaced, or the KinD keda path regresses. + // Not parallel: SetOpenShiftForTest mutates a package-level bool without a + // mutex. See openshift.go:SetOpenShiftForTest. + for _, tt := range []struct { + name string + openShift bool + want string + }{ + {"OpenShift", true, "keda-add-ons-http-interceptor-proxy.openshift-keda.svc.cluster.local"}, + {"elsewhere", false, "keda-add-ons-http-interceptor-proxy.keda.svc.cluster.local"}, + } { + t.Run(tt.name, func(t *testing.T) { + defer k8s.SetOpenShiftForTest(tt.openShift)() + if got := d.interceptorBridgeService(f, "fn-ns", deployment).Spec.ExternalName; got != tt.want { + t.Errorf("expected the bridge to address %q, got %q", tt.want, got) + } + }) + } + + svc := d.interceptorBridgeService(f, "fn-ns", deployment) + + if svc.Spec.Type != corev1.ServiceTypeExternalName { + t.Errorf("expected an ExternalName Service, got %q", svc.Spec.Type) + } + // The bridge lives in the FUNCTION's namespace, not the interceptor's - it + // is the function's own entrypoint, and it is ownerRef'd to the function's + // Deployment for GC, which only works same-namespace. + if svc.Namespace != "fn-ns" { + t.Errorf("expected the bridge in the function's own namespace, got %q", svc.Namespace) + } + if len(svc.OwnerReferences) != 1 || svc.OwnerReferences[0].UID != "deployment-uid" { + t.Errorf("expected an ownerRef to the function's Deployment, got %+v", svc.OwnerReferences) + } +} diff --git a/pkg/keda/describer.go b/pkg/keda/describer.go index 946f6e2a44..ac17efc5a2 100644 --- a/pkg/keda/describer.go +++ b/pkg/keda/describer.go @@ -87,11 +87,18 @@ func (d *Describer) Describe(ctx context.Context, name, namespace string) (fn.In return fn.Instance{}, fmt.Errorf("HTTPScaledObject %q does not have any hosts", name) } - routes := make([]string, 0, len(httpScaledObject.Spec.Hosts)) - for _, host := range httpScaledObject.Spec.Hosts { - routes = append(routes, fmt.Sprintf("http://%s:8080", host)) + // Identify the external OpenShift Route by name (interceptorRouteName is + // deterministic), not by guessing at Spec.Hosts array position - the + // Route host is appended there alongside the internal bridge hosts, + // which are indistinguishable from it by shape alone. + var routeHost string + var routeFound bool + if k8s.IsOpenShift() { + if dynClient, err := k8s.NewDynamicClient(); err == nil { + routeHost, routeFound, _ = k8s.GetAdmittedRouteHost(ctx, dynClient, interceptorNamespace(), interceptorRouteName(name, namespace)) + } } - primaryRouteURL := routes[0] + primaryRouteURL, routes := selectRouteURLs(httpScaledObject.Spec.Hosts, routeHost, routeFound) deploymentClient := clientset.AppsV1().Deployments(namespace) deployment, err := deploymentClient.Get(ctx, name, metav1.GetOptions{}) diff --git a/pkg/keda/lister.go b/pkg/keda/lister.go index 4d100430ae..f9fa4a435a 100644 --- a/pkg/keda/lister.go +++ b/pkg/keda/lister.go @@ -74,10 +74,17 @@ func (l *Lister) get(ctx context.Context, httpScaledObjectClientset *versioned.C ready = v1.ConditionFalse } - url := "" - if len(httpScaledObject.Spec.Hosts) > 0 { - url = fmt.Sprintf("http://%s:8080", httpScaledObject.Spec.Hosts[0]) + // Prefer the external OpenShift Route's URL when one exists - identified + // by name (interceptorRouteName is deterministic), not by guessing at + // Spec.Hosts array position (see describer.go for the same reasoning). + var routeHost string + var routeFound bool + if k8s.IsOpenShift() { + if dynClient, err := k8s.NewDynamicClient(); err == nil { + routeHost, routeFound, _ = k8s.GetAdmittedRouteHost(ctx, dynClient, interceptorNamespace(), interceptorRouteName(name, namespace)) + } } + url, _ := selectRouteURLs(httpScaledObject.Spec.Hosts, routeHost, routeFound) runtimeLabel := "" listItem := fn.ListItem{ diff --git a/pkg/keda/remover.go b/pkg/keda/remover.go index 097d020856..8e37caed7f 100644 --- a/pkg/keda/remover.go +++ b/pkg/keda/remover.go @@ -7,6 +7,8 @@ import ( apiErrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/dynamic" + "k8s.io/client-go/kubernetes" fn "knative.dev/func/pkg/functions" "knative.dev/func/pkg/k8s" ) @@ -48,11 +50,34 @@ func (remover *Remover) Remove(ctx context.Context, name, ns string) error { // We're responsible, for this function --> proceed... + // Routes exist only on OpenShift. Elsewhere dynClient stays nil and remove + // skips the Route entirely. + var dynClient dynamic.Interface + if k8s.IsOpenShift() { + if dynClient, err = k8s.NewDynamicClient(); err != nil { + return fmt.Errorf("could not setup dynamic client: %w", err) + } + } + + return remove(ctx, clientset, dynClient, name, ns) +} + +// remove deletes the function: Deployment first, interceptor Route last. A nil +// dynClient means the cluster has no Route API, so there is no Route to remove. +// +// Route is the only resource not Garbage-Collected through Deployment's owner +// reference. Route lives in different namespace. Route delete runs last just so +// we don't gate function removal on insufficient Route RBAC grants or similar. +// +// Removal does not depend on the current expose setting, so a Route left by an +// earlier deploy goes too. It is found by exact name, derived from the function +// name and namespace, in the interceptor namespace as resolved now, and deleted +// only if it carries func's managed label. Anything outside that is left alone. +func remove(ctx context.Context, clientset kubernetes.Interface, dynClient dynamic.Interface, name, ns string) error { deploymentClient := clientset.AppsV1().Deployments(ns) // delete only the deployment and let the api server handle the others via the owner reference - err = deploymentClient.Delete(ctx, name, metav1.DeleteOptions{}) - if err != nil { + if err := deploymentClient.Delete(ctx, name, metav1.DeleteOptions{}); err != nil { if apiErrors.IsNotFound(err) { return fn.ErrFunctionNotFound } @@ -63,5 +88,11 @@ func (remover *Remover) Remove(ctx context.Context, name, ns string) error { return fmt.Errorf("k8s remover failed to propagate service deletion: %v", err) } + if dynClient != nil { + if err := removeInterceptorRoute(ctx, dynClient, name, ns); err != nil { + return err + } + } + return nil } diff --git a/pkg/keda/remover_test.go b/pkg/keda/remover_test.go new file mode 100644 index 0000000000..0b3ff56419 --- /dev/null +++ b/pkg/keda/remover_test.go @@ -0,0 +1,108 @@ +package keda + +import ( + "errors" + "strings" + "testing" + + appsv1 "k8s.io/api/apps/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/client-go/kubernetes/fake" + k8stesting "k8s.io/client-go/testing" + + fn "knative.dev/func/pkg/functions" +) + +func deployment(name, ns string) *appsv1.Deployment { + return &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: ns}} +} + +// deploymentExists reports whether the named Deployment is still present. +func deploymentExists(t *testing.T, client *fake.Clientset, name, ns string) bool { + t.Helper() + _, err := client.AppsV1().Deployments(ns).Get(t.Context(), name, metav1.GetOptions{}) + if err != nil && !apierrors.IsNotFound(err) { + t.Fatalf("unexpected error checking the Deployment: %v", err) + } + return err == nil +} + +// Test_remove_RouteFailureStillDeletesDeployment asserts that when the Route +// removal fails, the Deployment is deleted anyway and the error still names the +// Route and its namespace. +func Test_remove_RouteFailureStillDeletesDeployment(t *testing.T) { + clientset := fake.NewSimpleClientset(deployment("f", "fn-ns")) + + // Refuses every Route operation, as a user with no permissions in the + // interceptor namespace would be refused. + dynClient := newFakeDynamicClient() + dynClient.PrependReactor("*", "routes", func(k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, apierrors.NewForbidden( + schema.GroupResource{Group: "route.openshift.io", Resource: "routes"}, "f-fn-ns", errors.New("nope")) + }) + + err := remove(t.Context(), clientset, dynClient, "f", "fn-ns") + + if err == nil { + t.Fatal("expected the Route failure to surface, so the user knows cleanup was incomplete") + } + if deploymentExists(t, clientset, "f", "fn-ns") { + t.Error("the Deployment must be deleted even when the Route removal fails - it is the whole point of the ordering") + } + if !strings.Contains(err.Error(), "f-fn-ns") || !strings.Contains(err.Error(), interceptorNamespace()) { + t.Errorf("expected the error to name the Route and its namespace, got %v", err) + } +} + +// Test_remove_Succeeds asserts the Deployment is deleted when removing the +// Route succeeds too, and that remove() really does remove the Route. The Route +// has to be seeded: RemoveManagedRoute treats a missing Route as success, so +// against an empty client this test would pass even if remove() dropped the +// Route call outright - which is the one path this rung reorders. +func Test_remove_Succeeds(t *testing.T) { + clientset := fake.NewSimpleClientset(deployment("f", "fn-ns")) + route, err := generateInterceptorRoute(fn.Function{Name: "f", Runtime: "go"}, "fn-ns", nil) + if err != nil { + t.Fatal(err) + } + dynClient := newFakeDynamicClient(route) + + if err := remove(t.Context(), clientset, dynClient, "f", "fn-ns"); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if deploymentExists(t, clientset, "f", "fn-ns") { + t.Error("expected the Deployment to be deleted") + } + if _, err := dynClient.Resource(routeGVR).Namespace(interceptorNamespace()). + Get(t.Context(), interceptorRouteName("f", "fn-ns"), metav1.GetOptions{}); err == nil { + t.Error("expected remove() to delete the interceptor Route, not only the Deployment") + } +} + +// Test_remove_NoRouteAPI covers a non-OpenShift cluster, where Remove passes a +// nil dynClient because no Route can exist. +func Test_remove_NoRouteAPI(t *testing.T) { + clientset := fake.NewSimpleClientset(deployment("f", "fn-ns")) + + if err := remove(t.Context(), clientset, nil, "f", "fn-ns"); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if deploymentExists(t, clientset, "f", "fn-ns") { + t.Error("expected the Deployment to be deleted") + } +} + +// Test_remove_FunctionNotFound asserts a missing Deployment yields +// fn.ErrFunctionNotFound rather than any other error. +func Test_remove_FunctionNotFound(t *testing.T) { + clientset := fake.NewSimpleClientset() // no Deployment + dynClient := newFakeDynamicClient() + + err := remove(t.Context(), clientset, dynClient, "f", "fn-ns") + if !errors.Is(err, fn.ErrFunctionNotFound) { + t.Fatalf("expected fn.ErrFunctionNotFound, got %v", err) + } +} diff --git a/pkg/keda/route.go b/pkg/keda/route.go new file mode 100644 index 0000000000..2331160f4a --- /dev/null +++ b/pkg/keda/route.go @@ -0,0 +1,172 @@ +package keda + +import ( + "context" + "fmt" + "time" + + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/client-go/dynamic" + + "knative.dev/func/pkg/deployer" + fn "knative.dev/func/pkg/functions" + "knative.dev/func/pkg/k8s" +) + +const ( + // Where the keda http-add-on installs its interceptor. The OpenShift + // Custom Metrics Autoscaler operator uses openshift-keda; the upstream + // helm chart, which is what pkg/cluster/keda.go and hack/cluster.sh set up + // on KinD, uses keda. Call interceptorNamespace() rather than picking one. + interceptorNamespaceOpenShift = "openshift-keda" + interceptorNamespaceUpstream = "keda" + + interceptorServiceName = "keda-add-ons-http-interceptor-proxy" + // interceptorServicePortName is the interceptor Service's own port name. + // It is "proxy", not "http" like the function's own Service uses. + interceptorServicePortName = "proxy" +) + +// interceptorNamespace returns the namespace the interceptor runs in on this +// cluster. A cluster has exactly one - the same Service backs both the +// cluster-local bridge and the OpenShift Route - so every caller resolves it +// here rather than assuming an install method. +func interceptorNamespace() string { + if k8s.IsOpenShift() { + return interceptorNamespaceOpenShift + } + return interceptorNamespaceUpstream +} + +// interceptorRouteName builds the Route name for a function. Every keda +// function's Route shares one interceptor namespace, so the name carries the +// function's namespace too, or two functions of the same name would collide. +func interceptorRouteName(name, namespace string) string { + return fmt.Sprintf("%s-%s", name, namespace) +} + +// generateInterceptorRoute returns the Route object exposing the shared keda +// interceptor. ensureInterceptorRoute is what sends it to the cluster. +// It targets the interceptor's own Service - not the function's Service, and +// not the per-function ExternalName bridge. Route's spec.to has no namespace +// field (openshift/api route/v1 RouteTargetReference is Kind/Name/Weight only), +// so a Route can only target a Service in its own namespace. +// +// No ownerRef: Kubernetes rejects cross-namespace owner references, so this +// Route cannot be owned by the function's Deployment and is never garbage +// collected. removeInterceptorRoute deletes it explicitly instead. +func generateInterceptorRoute(f fn.Function, namespace string, decorator deployer.DeployDecorator) (*unstructured.Unstructured, error) { + labels, err := deployer.GenerateCommonLabels(f, decorator) + if err != nil { + return nil, err + } + // The name already encodes the function's namespace; the label makes it + // selectable without parsing names apart. + labels["function.knative.dev/namespace"] = namespace + + annotations := deployer.GenerateCommonAnnotations(f, decorator, false /* dapr n/a for routing */, KedaDeployerName) + + route := &unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "route.openshift.io/v1", + "kind": "Route", + "metadata": map[string]any{ + "name": interceptorRouteName(f.Name, namespace), + "namespace": interceptorNamespace(), + "labels": stringMapToAny(labels), + "annotations": stringMapToAny(annotations), + }, + "spec": map[string]any{ + "to": map[string]any{ + "kind": "Service", + "name": interceptorServiceName, + }, + "port": map[string]any{ + "targetPort": interceptorServicePortName, + }, + // Edge TLS via the router's wildcard cert - zero cert + // management; Redirect upgrades http requests to https. + "tls": map[string]any{ + "termination": "edge", + "insecureEdgeTerminationPolicy": "Redirect", + }, + }, + }, + } + + return route, nil +} + +// stringMapToAny converts a map[string]string to the map[string]any that +// unstructured.Unstructured requires for nested fields. +func stringMapToAny(m map[string]string) map[string]any { + out := make(map[string]any, len(m)) + for k, v := range m { + out[k] = v + } + return out +} + +// selectRouteURLs returns the URLs to report for a function: one per bridge +// host, cluster-internal on :8080. When routeFound, the Route's https URL is +// appended and returned as primary, since it is the reachable one. +func selectRouteURLs(hosts []string, routeHost string, routeFound bool) (primary string, all []string) { + all = make([]string, 0, len(hosts)+1) + for _, host := range hosts { + // hosts carries the Route hostname as well, so the interceptor can + // match it, but it is not a bridge address and must not get :8080. + if routeFound && host == routeHost { + continue + } + all = append(all, fmt.Sprintf("http://%s:8080", host)) + } + if len(all) > 0 { + primary = all[0] + } + if routeFound { + routeURL := fmt.Sprintf("https://%s", routeHost) + all = append(all, routeURL) + primary = routeURL + } + return primary, all +} + +// ensureInterceptorRoute creates or updates the shared-namespace Route +// exposing the interceptor for f, waits for it to be admitted, and returns +// the minted host. Reuses k8s.EnsureRoute/WaitForRouteAdmitted directly - +// both are already Route-shape-agnostic (they only need a namespace, name, +// and a pre-built unstructured object), so nothing keda-specific is needed +// there. +func ensureInterceptorRoute(ctx context.Context, dynClient dynamic.Interface, f fn.Function, namespace string, decorator deployer.DeployDecorator) (string, error) { + route, err := generateInterceptorRoute(f, namespace, decorator) + if err != nil { + return "", fmt.Errorf("failed to generate interceptor Route: %w", err) + } + + if err := k8s.EnsureRoute(ctx, dynClient, interceptorNamespace(), route); err != nil { + return "", err + } + + host, err := k8s.WaitForRouteAdmitted(ctx, dynClient, interceptorNamespace(), route.GetName(), 30*time.Second) + if err != nil { + return "", fmt.Errorf("interceptor Route was not admitted: %w", err) + } + return host, nil +} + +// removeInterceptorRoute deletes the interceptor Route for the function 'name' +// in 'namespace', but only if keda's deployer owns it: k8s.RemoveManagedRoute +// leaves a Route without the managed label in place. +// +// Every error is fatal, Forbidden included. Both callers run it LAST for that +// reason - remove() after the Deployment is already gone, Deploy() after the +// HTTPScaledObject exists - so a failure, typically no Route permissions in the +// interceptor namespace, orphans only the Route and never leaves the function +// half-removed or deployed without a scaler. +func removeInterceptorRoute(ctx context.Context, dynClient dynamic.Interface, name, namespace string) error { + routeName := interceptorRouteName(name, namespace) + if _, err := k8s.RemoveManagedRoute(ctx, dynClient, interceptorNamespace(), routeName, KedaDeployerName); err != nil { + return fmt.Errorf("failed to remove interceptor Route %q in namespace %q: %w", routeName, interceptorNamespace(), err) + } + return nil +} diff --git a/pkg/keda/route_test.go b/pkg/keda/route_test.go new file mode 100644 index 0000000000..1485eab361 --- /dev/null +++ b/pkg/keda/route_test.go @@ -0,0 +1,175 @@ +package keda + +import ( + "context" + "testing" + "time" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + dynamicfake "k8s.io/client-go/dynamic/fake" + + fn "knative.dev/func/pkg/functions" +) + +var routeGVR = schema.GroupVersionResource{ + Group: "route.openshift.io", Version: "v1", Resource: "routes", +} + +func newFakeDynamicClient(objects ...runtime.Object) *dynamicfake.FakeDynamicClient { + return dynamicfake.NewSimpleDynamicClientWithCustomListKinds( + runtime.NewScheme(), + map[schema.GroupVersionResource]string{routeGVR: "RouteList"}, + objects..., + ) +} + +func Test_interceptorRouteName(t *testing.T) { + // Same function name, different namespaces must not collide - the + // whole reason the name includes the namespace, since every keda + // function's Route lives together in the one shared interceptor namespace. + a := interceptorRouteName("f", "ns1") + b := interceptorRouteName("f", "ns2") + if a == b { + t.Fatalf("expected distinct names for the same function name in different namespaces, got %q for both", a) + } + if a != "f-ns1" || b != "f-ns2" { + t.Errorf("expected f-ns1/f-ns2, got %q/%q", a, b) + } +} + +func Test_generateInterceptorRoute(t *testing.T) { + f := fn.Function{Name: "f", Runtime: "go"} + + route, err := generateInterceptorRoute(f, "ns", nil) + if err != nil { + t.Fatal(err) + } + + if route.GetName() != "f-ns" { + t.Errorf("expected name f-ns, got %q", route.GetName()) + } + if route.GetNamespace() != interceptorNamespace() { + t.Errorf("expected namespace %q, got %q", interceptorNamespace(), route.GetNamespace()) + } + toName, _, _ := unstructured.NestedString(route.Object, "spec", "to", "name") + if toName != interceptorServiceName { + t.Errorf("expected spec.to.name %q (the interceptor, not the function's own Service), got %q", interceptorServiceName, toName) + } + targetPort, _, _ := unstructured.NestedString(route.Object, "spec", "port", "targetPort") + if targetPort != interceptorServicePortName { + t.Errorf("expected spec.port.targetPort %q, got %q", interceptorServicePortName, targetPort) + } + if len(route.GetOwnerReferences()) != 0 { + t.Errorf("expected no ownerReferences (cross-namespace GC is impossible), got %+v", route.GetOwnerReferences()) + } + if route.GetLabels()["function.knative.dev/namespace"] != "ns" { + t.Errorf("expected the function.knative.dev/namespace label to disambiguate the shared route namespace, got %q", + route.GetLabels()["function.knative.dev/namespace"]) + } +} + +func Test_ensureAndRemoveInterceptorRoute(t *testing.T) { + ctx := t.Context() + f := fn.Function{Name: "f", Runtime: "go"} + + // ensureInterceptorRoute's admitted-returns-host path is NOT + // independently exercised here: EnsureRoute's update branch replaces + // the whole object with the freshly-generated one (no status field), + // so a fake dynamic client can't simulate "already admitted, then + // ensured again" without wiping the very status being asserted on - + // the same fake-client status-subresource limitation noted in + // pkg/k8s/route_test.go's commit. WaitForRouteAdmitted's own + // admitted/rejected/timeout behavior is already covered directly and + // fast there; this package only re-proves the timeout path below, + // since ensureInterceptorRoute wires that call with its own hardcoded + // duration worth confirming is reachable. + + t.Run("remove deletes a managed route", func(t *testing.T) { + route, err := generateInterceptorRoute(f, "ns", nil) + if err != nil { + t.Fatal(err) + } + client := newFakeDynamicClient(route) + + if err := removeInterceptorRoute(ctx, client, "f", "ns"); err != nil { + t.Fatal(err) + } + if _, err := client.Resource(routeGVR).Namespace(interceptorNamespace()).Get(ctx, "f-ns", metav1.GetOptions{}); err == nil { + t.Error("expected the interceptor Route to be gone after removal") + } + }) + + t.Run("remove is a no-op when nothing exists", func(t *testing.T) { + client := newFakeDynamicClient() + if err := removeInterceptorRoute(ctx, client, "f", "ns"); err != nil { + t.Fatal(err) + } + }) + + t.Run("ensure never admitted times out cleanly", func(t *testing.T) { + route, err := generateInterceptorRoute(f, "ns", nil) + if err != nil { + t.Fatal(err) + } + client := newFakeDynamicClient(route) + + // A parent context with its own short deadline cancels the poll + // well before ensureInterceptorRoute's hardcoded 30s internal + // timeout is reached - proves the timeout path is reachable + // without actually waiting 30 real seconds. + shortCtx, cancel := context.WithTimeout(ctx, 100*time.Millisecond) + defer cancel() + + _, err = ensureInterceptorRoute(shortCtx, client, f, "ns", nil) + if err == nil { + t.Fatal("expected an error when no router ever admits the route") + } + }) +} + +func Test_selectRouteURLs(t *testing.T) { + bridgeHosts := []string{"f-interceptor-bridge.ns.svc", "f-interceptor-bridge"} + + t.Run("no Route: bridge host is primary, both bridge hosts listed", func(t *testing.T) { + primary, all := selectRouteURLs(bridgeHosts, "", false) + if primary != "http://f-interceptor-bridge.ns.svc:8080" { + t.Errorf("expected the first bridge host as primary, got %q", primary) + } + if len(all) != 2 || all[0] != "http://f-interceptor-bridge.ns.svc:8080" || all[1] != "http://f-interceptor-bridge:8080" { + t.Errorf("expected both bridge hosts with :8080, got %v", all) + } + }) + + t.Run("Route found: its https URL is primary and appended, bridge hosts unchanged", func(t *testing.T) { + // hosts here matches the real call site's shape (deployer.go appends + // the Route host onto the same slice HTTPScaledObject.Spec.Hosts + // ends up with) - the Route host itself must appear exactly once, + // portless https, never also as a :8080 bridge entry. + hostsWithRoute := append(append([]string{}, bridgeHosts...), "f-ns.apps.example.com") + primary, all := selectRouteURLs(hostsWithRoute, "f-ns.apps.example.com", true) + if primary != "https://f-ns.apps.example.com" { + t.Errorf("expected the Route's https URL as primary, got %q", primary) + } + if len(all) != 3 || all[2] != "https://f-ns.apps.example.com" { + t.Errorf("expected the Route URL appended after the two bridge hosts, got %v", all) + } + if all[0] != "http://f-interceptor-bridge.ns.svc:8080" || all[1] != "http://f-interceptor-bridge:8080" { + t.Errorf("expected the bridge hosts unaffected by the Route being found, got %v", all) + } + for _, u := range all { + if u == "http://f-ns.apps.example.com:8080" { + t.Errorf("expected the Route host NOT to also appear as a :8080 bridge entry, got %v", all) + } + } + }) + + t.Run("no hosts, no Route: empty primary, empty list", func(t *testing.T) { + primary, all := selectRouteURLs(nil, "", false) + if primary != "" || len(all) != 0 { + t.Errorf("expected (\"\", empty), got (%q, %v)", primary, all) + } + }) +} diff --git a/schema/func_yaml-schema.json b/schema/func_yaml-schema.json index 79e7af6b3d..e09240ea30 100644 --- a/schema/func_yaml-schema.json +++ b/schema/func_yaml-schema.json @@ -132,6 +132,10 @@ "description": "ManagementDisabled disables automatic creation/update of a Function CR\nfor operator management after deploy. The zero value (false) means\nthe function is managed by default when the func-operator is installed." }, "expose": { + "enum": [ + "route", + "none" + ], "type": "string", "description": "Expose controls external access for the raw and keda deployers (the\nknative deployer manages its own exposure and ignores it). Optional.\nValues: \"route\" (create an OpenShift Route; OpenShift clusters only -\na hard error elsewhere), \"none\" (cluster-local only, explicit\nopt-out). Defaults to \"route\" behavior on OpenShift - a deployed\nfunction being externally reachable is the expected outcome - and to\ncluster-local on any other cluster, since a Route is an\nOpenShift-only mechanism and the unset default must not impose a\nplatform requirement." }