diff --git a/cmd/delete_test.go b/cmd/delete_test.go index 1c6efebb6b..07dfb63da1 100644 --- a/cmd/delete_test.go +++ b/cmd/delete_test.go @@ -353,7 +353,13 @@ func TestDelete_ByProjectClearsDeployedMarker(t *testing.T) { Runtime: "go", Registry: TestRegistry, Deployer: keda.KedaDeployerName, // intent - how to deploy - Deploy: fn.DeploySpec{Namespace: "myns", Deployer: keda.KedaDeployerName}, + Deploy: fn.DeploySpec{ + Namespace: "myns", + ActiveDeployer: keda.KedaDeployerName, + }, + Scale: &fn.ScaleOptions{ + KEDA: &fn.KEDAScaleOptions{Triggers: []fn.KEDATrigger{{Type: "http"}}}, + }, } f, err := fn.New().Init(f) if err != nil { @@ -382,8 +388,8 @@ func TestDelete_ByProjectClearsDeployedMarker(t *testing.T) { if loaded.Deploy.Namespace != "" { t.Fatalf("expected Deploy.Namespace cleared after a successful undeploy, got %q", loaded.Deploy.Namespace) } - if loaded.Deploy.Deployer != "" { - t.Fatalf("expected Deploy.Deployer cleared after a successful undeploy, got %q", loaded.Deploy.Deployer) + if loaded.Deploy.ActiveDeployer != "" { + t.Fatalf("expected Deploy.Deployer cleared after a successful undeploy, got %q", loaded.Deploy.ActiveDeployer) } if loaded.Deployer != keda.KedaDeployerName { t.Fatalf("expected the intended Deployer preserved as a remembered choice, got %q", loaded.Deployer) @@ -401,7 +407,7 @@ func TestDelete_ByNameLeavesLocalFunctionUntouched(t *testing.T) { Runtime: "go", Registry: TestRegistry, Name: "localfn", - Deploy: fn.DeploySpec{Namespace: "myns", Deployer: keda.KedaDeployerName}, + Deploy: fn.DeploySpec{Namespace: "myns", ActiveDeployer: keda.KedaDeployerName}, } f, err := fn.New().Init(f) if err != nil { @@ -441,7 +447,16 @@ func TestDelete_ByNameLeavesLocalFunctionUntouched(t *testing.T) { // after removal keeps the INTENT deployer intact and functional. func TestDelete_ByProjectPreservesDeployerForRedeploy(t *testing.T) { root := FromTempDirectory(t) - if _, err := fn.New().Init(fn.Function{Runtime: "go", Root: root, Registry: TestRegistry}); err != nil { + f, err := fn.New().Init(fn.Function{Runtime: "go", Root: root, Registry: TestRegistry}) + if err != nil { + t.Fatal(err) + } + // keda requires at least one trigger to be declared explicitly. + f.Deployer = keda.KedaDeployerName + f.Scale = &fn.ScaleOptions{ + KEDA: &fn.KEDAScaleOptions{Triggers: []fn.KEDATrigger{{Type: "http"}}}, + } + if err := f.Write(); err != nil { t.Fatal(err) } @@ -476,8 +491,8 @@ func TestDelete_ByProjectPreservesDeployerForRedeploy(t *testing.T) { if err != nil { t.Fatal(err) } - if loaded.Deploy.Deployer != keda.KedaDeployerName { - t.Fatalf("expected the flag-less redeploy to reuse the persisted %q deployer, got %q", keda.KedaDeployerName, loaded.Deploy.Deployer) + if loaded.Deploy.ActiveDeployer != keda.KedaDeployerName { + t.Fatalf("expected the flag-less redeploy to reuse the persisted %q deployer, got %q", keda.KedaDeployerName, loaded.Deploy.ActiveDeployer) } } @@ -490,7 +505,7 @@ func TestDelete_ByProjectThenRedeployWithDifferentDeployerNotBlocked(t *testing. Root: root, Runtime: "go", Registry: TestRegistry, - Deploy: fn.DeploySpec{Namespace: "myns", Deployer: keda.KedaDeployerName}, + Deploy: fn.DeploySpec{Namespace: "myns", ActiveDeployer: keda.KedaDeployerName}, } f, err := fn.New().Init(f) if err != nil { diff --git a/cmd/deploy.go b/cmd/deploy.go index b8b8273efd..aa5f1b31b8 100644 --- a/cmd/deploy.go +++ b/cmd/deploy.go @@ -168,7 +168,7 @@ EXAMPLES cmd.Flags().StringP("builder", "b", cfg.Builder, fmt.Sprintf("Builder to use when creating the function's container. Currently supported builders are %s.", KnownBuilders())) cmd.Flags().String("deployer", cfg.Deployer, - fmt.Sprintf("Type of deployment to use: '%s' for Knative Service, '%s' for Kubernetes Deployment, or '%s' for Deployment with a KEDA HTTP scaler ($FUNC_DEPLOYER)", deployers.Knative, deployers.Kubernetes, deployers.Keda)) + fmt.Sprintf("Type of deployment to use: '%s' for Knative Service, '%s' for Kubernetes Deployment, or '%s' for Deployment scaled by KEDA (HTTP or Kafka triggers) ($FUNC_DEPLOYER)", deployers.Knative, deployers.Kubernetes, deployers.Keda)) cmd.Flags().StringP("registry", "r", cfg.Registry, "Container registry + registry namespace. (ex 'ghcr.io/myuser'). The full image name is automatically determined using this along with function name. ($FUNC_REGISTRY)") cmd.Flags().Bool("registry-insecure", cfg.RegistryInsecure, "Skip TLS certificate verification when communicating in HTTPS with the registry. The value is persisted over consecutive runs ($FUNC_REGISTRY_INSECURE)") @@ -306,8 +306,8 @@ func runDeploy(cmd *cobra.Command, newClient ClientFactory) (err error) { // Back-compat: a function deployed before the deployer was recorded has a // namespace but no deployer, which historically could only mean knative. - if f.Deploy.Namespace != "" && f.Deploy.Deployer == "" { - f.Deploy.Deployer = deployers.Knative + if f.Deploy.Namespace != "" && f.Deploy.ActiveDeployer == "" { + f.Deploy.ActiveDeployer = deployers.Knative } if f, err = cfg.Configure(f); err != nil { // Updates f with deploy cfg @@ -368,7 +368,7 @@ func runDeploy(cmd *cobra.Command, newClient ClientFactory) (err error) { var url string // Invoke a remote build/push/deploy pipeline // Returned is the function with fields like Registry, f.Deploy.Image & - // f.Deploy.Namespace, f.Deploy.Expose populated. + // f.Deploy.Namespace, f.Deploy.ActiveExpose populated. if url, f, err = client.RunPipeline(cmd.Context(), f); err != nil { return wrapDeploymentError(err) } @@ -377,7 +377,7 @@ func runDeploy(cmd *cobra.Command, newClient ClientFactory) (err error) { // After a remote pipeline: intent was active, describer saw no // applied expose. Typical cause is a func-util image that predates // this field. Knative is excluded; it never applies expose. - if fn.ExposureRecordMissing(f.Expose, f.Deploy.Expose, f.Deploy.Deployer) { + if fn.ExposureRecordMissing(f.Expose, f.Deploy.ActiveExpose, f.Deploy.ActiveDeployer) { fmt.Fprintf(cmd.OutOrStderr(), "Warning: expose %q was requested but the cluster's "+ "func-util image applied no external exposure; the function is running cluster-local\n", f.Expose) } diff --git a/cmd/deploy_test.go b/cmd/deploy_test.go index 906993f54e..2d627cca02 100644 --- a/cmd/deploy_test.go +++ b/cmd/deploy_test.go @@ -8,6 +8,7 @@ import ( "os" "path/filepath" "reflect" + "slices" "strings" "testing" "time" @@ -386,7 +387,7 @@ func TestDeploy_Envs(t *testing.T) { t.Fatal(err) } - // Mock deployer records f.Deploy.Deployer so later redeploys are not + // Mock deployer records f.Deploy.ActiveDeployer so later redeploys are not // treated as the pre-field legacy (knative) case. clientFn := NewTestClient(fn.WithDeployer(mock.NewDeployer())) @@ -1161,7 +1162,7 @@ func TestDeploy_NamespaceRedeployWarning(t *testing.T) { f := fn.Function{ Runtime: "go", Root: root, - Deploy: fn.DeploySpec{Namespace: "funcns", Deployer: deployers.Default}, + Deploy: fn.DeploySpec{Namespace: "funcns", ActiveDeployer: deployers.Default}, } f, err := fn.New().Init(f) if err != nil { @@ -1211,8 +1212,8 @@ func TestDeploy_NamespaceUpdateWarning(t *testing.T) { Runtime: "go", Root: root, Deploy: fn.DeploySpec{ - Namespace: "myns", - Deployer: deployers.Default, + Namespace: "myns", + ActiveDeployer: deployers.Default, }, } f, err := fn.New().Init(f) @@ -1363,7 +1364,7 @@ func TestDeploy_NamespaceChangePreservesExternalRegistry(t *testing.T) { Runtime: "go", Root: root, Registry: "docker.io/user", - Deploy: fn.DeploySpec{Namespace: "ns1", Deployer: deployers.Default}, + Deploy: fn.DeploySpec{Namespace: "ns1", ActiveDeployer: deployers.Default}, } f, err := fn.New().Init(f) if err != nil { @@ -1398,7 +1399,7 @@ func TestDeploy_NamespaceChangeUpdatesInternalRegistry(t *testing.T) { Runtime: "go", Root: root, Registry: "image-registry.openshift-image-registry.svc:5000/ns1", - Deploy: fn.DeploySpec{Namespace: "ns1", Deployer: deployers.Default}, + Deploy: fn.DeploySpec{Namespace: "ns1", ActiveDeployer: deployers.Default}, } f, err := fn.New().Init(f) if err != nil { @@ -2599,8 +2600,8 @@ func TestDeploy_DeployerPersists(t *testing.T) { if err != nil { t.Fatal(err) } - if loaded.Deploy.Deployer != deployers.Default { - t.Fatalf("expected persisted deployer %q, got %q", deployers.Default, loaded.Deploy.Deployer) + if loaded.Deploy.ActiveDeployer != deployers.Default { + t.Fatalf("expected persisted deployer %q, got %q", deployers.Default, loaded.Deploy.ActiveDeployer) } }) @@ -2629,8 +2630,8 @@ func TestDeploy_DeployerPersists(t *testing.T) { if err != nil { t.Fatal(err) } - if loaded.Deploy.Deployer != other { - t.Fatalf("expected persisted deployer %q, got %q", other, loaded.Deploy.Deployer) + if loaded.Deploy.ActiveDeployer != other { + t.Fatalf("expected persisted deployer %q, got %q", other, loaded.Deploy.ActiveDeployer) } // no --deployer flag: the flag defaults to the persisted value @@ -2648,8 +2649,8 @@ func TestDeploy_DeployerPersists(t *testing.T) { if err != nil { t.Fatal(err) } - if loaded.Deploy.Deployer != other { - t.Fatalf("expected deployer to remain %q after a flag-less redeploy, got %q", other, loaded.Deploy.Deployer) + if loaded.Deploy.ActiveDeployer != other { + t.Fatalf("expected deployer to remain %q after a flag-less redeploy, got %q", other, loaded.Deploy.ActiveDeployer) } }) } @@ -2682,8 +2683,8 @@ func TestDeploy_DeployerGlobalConfig(t *testing.T) { if err != nil { t.Fatal(err) } - if loaded.Deploy.Deployer != k8s.KubernetesDeployerName { - t.Fatalf("expected the global config's deployer %q to seed the flagless deploy, got %q", k8s.KubernetesDeployerName, loaded.Deploy.Deployer) + if loaded.Deploy.ActiveDeployer != k8s.KubernetesDeployerName { + t.Fatalf("expected the global config's deployer %q to seed the flagless deploy, got %q", k8s.KubernetesDeployerName, loaded.Deploy.ActiveDeployer) } } @@ -2716,7 +2717,20 @@ func TestDeploy_DeployerSwitch(t *testing.T) { Root: root, Registry: TestRegistry, // Namespace set == already deployed, which is what the guard gates on. - Deploy: fn.DeploySpec{Namespace: "myns", Deployer: tt.deployedDep}, + Deploy: fn.DeploySpec{Namespace: "myns", ActiveDeployer: tt.deployedDep}, + } + // keda requires at least one trigger to be declared explicitly, + // but only matters when keda ends up the effective deployer for + // this attempt (an explicit switch away from it does not). + effectiveDeployer := tt.requested + if effectiveDeployer == "" { + effectiveDeployer = tt.deployedDep + } + if effectiveDeployer == keda.KedaDeployerName { + f.Deployer = keda.KedaDeployerName + f.Scale = &fn.ScaleOptions{ + KEDA: &fn.KEDAScaleOptions{Triggers: []fn.KEDATrigger{{Type: "http"}}}, + } } if _, err := fn.New().Init(f); err != nil { t.Fatal(err) @@ -2824,8 +2838,8 @@ func TestDeploy_ExposeEmptyVsUnset(t *testing.T) { if f.Expose != "" { t.Errorf("expected intent expose empty, got %q", f.Expose) } - if f.Deploy.Expose != "" { - t.Errorf("expected status expose empty, got %q", f.Deploy.Expose) + if f.Deploy.ActiveExpose != "" { + t.Errorf("expected status expose empty, got %q", f.Deploy.ActiveExpose) } }) @@ -2837,8 +2851,8 @@ func TestDeploy_ExposeEmptyVsUnset(t *testing.T) { t.Fatalf("expected intent expose 'none', got %q", f.Expose) } // status is observed applied mode; "none"/empty both mean cluster-local - if f := loadFn(t, root); f.Deploy.Expose != "" { - t.Fatalf("expected status expose empty for cluster-local, got %q", f.Deploy.Expose) + if f := loadFn(t, root); f.Deploy.ActiveExpose != "" { + t.Fatalf("expected status expose empty for cluster-local, got %q", f.Deploy.ActiveExpose) } // redeploy without the flag should keep intent via flag default @@ -2897,8 +2911,8 @@ func TestDeploy_ExposeRoutePersists(t *testing.T) { if f.Expose != "route" { t.Fatalf("expected intent expose 'route', got %q", f.Expose) } - if f.Deploy.Expose != "route" { - t.Fatalf("expected status expose 'route', got %q", f.Deploy.Expose) + if f.Deploy.ActiveExpose != "route" { + t.Fatalf("expected status expose 'route', got %q", f.Deploy.ActiveExpose) } } @@ -2944,9 +2958,20 @@ func TestDeploy_ExposeIgnoredByDeployerNote(t *testing.T) { // route cases need OpenShift gate open; none/empty do not care. cleanup := k8s.SetOpenShiftForTest(true, nil) defer cleanup() - if _, err := fn.New().Init(fn.Function{Runtime: "go", Root: root}); err != nil { + f, err := fn.New().Init(fn.Function{Runtime: "go", Root: root}) + if err != nil { t.Fatal(err) } + if slices.Contains(tt.args, "keda") { + // keda requires at least one trigger to be declared explicitly. + f.Deployer = keda.KedaDeployerName + f.Scale = &fn.ScaleOptions{ + KEDA: &fn.KEDAScaleOptions{Triggers: []fn.KEDATrigger{{Type: "http"}}}, + } + if err := f.Write(); err != nil { + t.Fatal(err) + } + } builder := mock.NewBuilder() cmd := NewDeployCmd(NewTestClient( @@ -2958,7 +2983,7 @@ func TestDeploy_ExposeIgnoredByDeployerNote(t *testing.T) { var stderr strings.Builder cmd.SetOut(&stderr) cmd.SetErr(&stderr) - err := cmd.Execute() + err = cmd.Execute() if err != nil { t.Fatalf("unexpected error: %v", err) @@ -3016,7 +3041,7 @@ func TestDeploy_RemoteExposeRecordsObservation(t *testing.T) { pipeliner.RunFn = func(f fn.Function) (string, fn.Function, error) { // add exposure tracking to the base RunFn url, f, err := base(f) - f.Deploy.Expose = tt.observed + f.Deploy.ActiveExpose = tt.observed return url, f, err } @@ -3038,8 +3063,8 @@ func TestDeploy_RemoteExposeRecordsObservation(t *testing.T) { if err != nil { t.Fatal(err) } - if f.Deploy.Expose != tt.wantRecord { - t.Errorf("Deploy.Expose = %q, want %q", f.Deploy.Expose, tt.wantRecord) + if f.Deploy.ActiveExpose != tt.wantRecord { + t.Errorf("Deploy.Expose = %q, want %q", f.Deploy.ActiveExpose, tt.wantRecord) } warned := strings.Contains(out.String(), "applied no external exposure") if warned != tt.wantWarning { diff --git a/cmd/func-util/main.go b/cmd/func-util/main.go index 41c01e4124..514f1e9bd2 100644 --- a/cmd/func-util/main.go +++ b/cmd/func-util/main.go @@ -159,7 +159,7 @@ func deploy(ctx context.Context) error { // honors --deployer, which travels in func.yaml as intent. deployer := f.Deployer if deployer == "" { - deployer = f.Deploy.Deployer + deployer = f.Deploy.ActiveDeployer } if deployer == "" { deployer = knative.KnativeDeployerName diff --git a/docs/reference/func_deploy.md b/docs/reference/func_deploy.md index 6fdb998af7..3ac76edf7a 100644 --- a/docs/reference/func_deploy.md +++ b/docs/reference/func_deploy.md @@ -126,7 +126,7 @@ func deploy -b, --builder string Builder to use when creating the function's container. Currently supported builders are "host", "pack" and "s2i". (default "pack") --builder-image string Specify a custom builder image for use by the builder other than its default. ($FUNC_BUILDER_IMAGE) -c, --confirm Prompt to confirm options interactively ($FUNC_CONFIRM) - --deployer string Type of deployment to use: 'knative' for Knative Service, 'raw' for Kubernetes Deployment, or 'keda' for Deployment with a KEDA HTTP scaler ($FUNC_DEPLOYER) (default "knative") + --deployer string Type of deployment to use: 'knative' for Knative Service, 'raw' for Kubernetes Deployment, or 'keda' for Deployment scaled by KEDA (HTTP or Kafka triggers) ($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' for an OpenShift Route (OpenShift clusters only), 'none' for cluster-local. Default: no exposure. Raw and keda deployers only. ($FUNC_EXPOSE) diff --git a/docs/reference/func_yaml.md b/docs/reference/func_yaml.md index 6c0eabba13..8d544a4db0 100644 --- a/docs/reference/func_yaml.md +++ b/docs/reference/func_yaml.md @@ -35,6 +35,17 @@ build: s2i: example.com/user/my-s2i-node-builder ``` +### `deployer` + +The type of deployment to use when deploying the function. Possible values are: +- `knative` (default): deploys a Knative Service, scaled by Knative's KPA (Knative Pod Autoscaler). +- `raw`: deploys a plain Kubernetes Deployment with a static replica count. +- `keda`: deploys a plain Kubernetes Deployment scaled by [KEDA](https://keda.sh), based on triggers such as incoming HTTP traffic or Kafka consumer lag. See [`scale.keda`](#scale) below. + +```yaml +deployer: keda +``` + ### `git` If using a `git` build strategy, this field is used to specify the git URL as well @@ -131,14 +142,42 @@ must exist in the namespace to succeed. More info: https://k8s.io/docs/tasks/configure-pod-container/configure-service-account +### `scale` + +Top-level autoscaling configuration. Settings are deployer-aware: `kpa` is used with `deployer: knative`, `keda` with `deployer: keda`. `min`/`max` are shared across all deployers, but the default when left unset differs per deployer: `deployer: raw` deploys a fixed-size Deployment with no autoscaler (`min` unset or 0 effectively means 1 replica; `max` isn't enforced), `deployer: knative` defaults to `min=0`/`max=0` (scale-to-zero, no limit, per Knative Serving's own defaults), and `deployer: keda` defaults to `min=1`/`max=10`. + +- `min`: Minimum number of replicas. Non-negative integer. Default is 0 for `deployer: knative`, but 1 for `deployer: raw` and `deployer: keda`. See related [Knative docs](https://knative.dev/docs/serving/autoscaling/scale-bounds/#lower-bound). +- `max`: Maximum number of replicas. Non-negative integer. Default is 0 (no limit) for `deployer: knative`, not enforced for `deployer: raw`, and 10 for `deployer: keda`. For `deployer: keda` specifically, `max: 0` is rejected (unlike `knative`, where it means no limit): KEDA maps it to an HPA `maxReplicas`, which must be `>= 1`. See related [Knative docs](https://knative.dev/docs/serving/autoscaling/scale-bounds/#upper-bound). +- `kpa`: Knative Pod Autoscaler config, used only with `deployer: knative`. + - `metric`: metric type watched by the autoscaler: `concurrency` (default) or `rps`. See related [Knative docs](https://knative.dev/docs/serving/autoscaling/autoscaling-metrics/). + - `target`: target value for the metric. Defaults to `options.resources.limits.concurrency` when given. Float >= 0.01, default is 100. See related [Knative docs](https://knative.dev/docs/serving/autoscaling/concurrency/#soft-limit). + - `utilization`: target utilization percentage before scaling up. Float 1-100, default is 70. See related [Knative docs](https://knative.dev/docs/serving/autoscaling/concurrency/#target-utilization). +- `keda`: KEDA-specific scaling config, required when `deployer: keda`. + - `pollingInterval`: how often KEDA checks triggers, in seconds. Default is 30. Only applies to `kafka` triggers (a `ScaledObject`, which polls); the `http` trigger's `HTTPScaledObject` has no polling concept — it scales from interceptor-reported metrics instead — so this setting has no effect when only an `http` trigger is configured. + - `cooldownPeriod`: seconds to wait after the last trigger fires before scaling to min. Default is 300. + - `triggers`: a list of KEDA triggers. At least one is required. Each trigger has a `type` of `http`, `kafka`, or `cron`: + - `http`: scales based on incoming HTTP request rate. + - `targetValue`: requests per second per replica before scaling up. Default is 100. + - `kafka`: scales based on consumer group lag. Requires [`run.kafka`](#runkafka) to be configured. + - `lagThreshold`: average consumer lag per partition that triggers scaling up. Default is 10. + - `activationLagThreshold`: lag below which KEDA keeps replicas at 0 when `scale.min` is 0. Default is 0. + - `cron`: scales based on a time window. + - `timezone`: e.g. `Europe/Istanbul`. + - `start`, `end`: cron expressions defining the active window, e.g. `0 8 * * *`. + - `desiredReplicas`: number of replicas to scale to during the active window. + +```yaml +scale: + min: 0 + max: 10 + kpa: + metric: concurrency + target: 75 + utilization: 75 +``` + ### `options` -Options allows you to set specific configuration for the deployed function, allowing you to tweak Knative Service options related to autoscaling and other properties. If these options are not set, the Knative defaults will be used. -- `scale` - - `min`: Minimum number of replicas. Must me non-negative integer, default is 0. See related [Knative docs](https://knative.dev/docs/serving/autoscaling/scale-bounds/#lower-bound). - - `max`: Maximum number of replicas. Must me non-negative integer, default is 0 - meaning no limit. See related [Knative docs](https://knative.dev/docs/serving/autoscaling/scale-bounds/#upper-bound). - - `metric`: Defines which metric type is watched by the Autoscaler. Could be `concurrency` (default) or `rps`. See related [Knative docs](https://knative.dev/docs/serving/autoscaling/autoscaling-metrics/). - - `target`: Recommendation for when to scale up based on the concurrent number of incoming request. Defaults to `options.resources.limits.concurrency` when given. Can be float value greater than 0.01, default is 100. See related [Knative docs](https://knative.dev/docs/serving/autoscaling/concurrency/#soft-limit). - - `utilization`: Percentage of concurrent requests utilization before scaling up. Can be float value between 1 and 100, default is 70. See related [Knative docs](https://knative.dev/docs/serving/autoscaling/concurrency/#target-utilization). +Options allows you to set resource limits and requests for the deployed function container. - `resources` - `requests` - `cpu`: A CPU resource request for the container with deployed function. See related [Kubernetes docs](https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/#requests-and-limits). @@ -149,21 +188,103 @@ Options allows you to set specific configuration for the deployed function, allo - `concurrency`: Hard Limit of concurrent requests to be processed by a single replica. Can be integer value greater than or equal to 0, default is 0 - meaning no limit. See related [Knative docs](https://knative.dev/docs/serving/autoscaling/concurrency/#hard-limit). ```yaml -options: - scale: - min: 0 - max: 10 +deploy: + options: + resources: + requests: + cpu: 100m + memory: 128Mi + limits: + cpu: 1000m + memory: 256Mi + concurrency: 100 +``` + +Example using `deployer: keda` with an HTTP trigger: + +```yaml +deployer: keda +scale: + min: 0 + max: 10 + keda: + pollingInterval: 30 + cooldownPeriod: 300 + triggers: + - type: http + targetValue: 200 +``` + +Example using `deployer: keda` with a Kafka consumer-lag trigger: + +```yaml +deployer: keda +scale: + min: 0 + max: 10 + keda: + pollingInterval: 30 + cooldownPeriod: 300 + triggers: + - type: kafka + lagThreshold: 5 + activationLagThreshold: 0 +``` + +Note: `http` and `kafka` triggers cannot currently be combined in the same +`scale.keda.triggers` list. The keda deployer creates a separate +`HTTPScaledObject` for `http` and a separate `ScaledObject` for `kafka`, +both targeting the same Deployment, and KEDA only allows one scaler per +workload. `func` rejects this combination at validation time. The `cron` +trigger type is accepted by the schema but not yet implemented by any +deployer; using it also fails validation. + +Example using `deployer: knative` with explicit KPA settings: + +```yaml +deployer: knative +scale: + min: 1 + max: 10 + kpa: metric: concurrency - target: 75 - utilization: 75 - resources: - requests: - cpu: 100m - memory: 128Mi - limits: - cpu: 1000m - memory: 256Mi - concurrency: 100 + target: 50 +``` + +### `run.kafka` + +When set, the function is deployed as a Kafka consumer: it reads CloudEvents from a Kafka +topic instead of serving HTTP requests. Requires `invoke: cloudevent` and the Go runtime. + +- `brokers`: comma-separated list of Kafka broker addresses. +- `topic`: the topic to consume. +- `consumerGroup`: the Kafka consumer group ID. +- `securityProtocol`: one of `PLAINTEXT`, `SSL`, `SASL_PLAINTEXT`, `SASL_SSL`. +- `tls`: TLS configuration, only valid for `SSL` and `SASL_SSL`. Optional for both: if unset, the broker certificate is verified against the system's CA trust store. Set it to use a custom CA certificate or mutual TLS. + - `caCert`: path to the CA certificate PEM file used to verify the broker certificate. Typically mounted via [`volumes`](#volumes). + - `clientCert`, `clientKey`: paths to the client certificate/key PEM files, for mutual TLS. + - `skipVerify`: skip broker certificate verification (development only). +- `sasl`: SASL configuration, required for `SASL_PLAINTEXT` and `SASL_SSL`. + - `mechanism`: one of `PLAIN`, `SCRAM-SHA-256`, `SCRAM-SHA-512`. + - `user`: SASL username. Supports `{{ secret:name:key }}` and `{{ configMap:name:key }}` syntax, or a plain value. + - `password`: SASL password. Supports `{{ secret:name:key }}` and `{{ configMap:name:key }}` syntax, or a plain value (at least for debugging purposes). + +```yaml +run: + kafka: + brokers: "my-cluster-kafka-bootstrap.kafka.svc.cluster.local:9093" + topic: "my-topic" + consumerGroup: "my-function-group" + securityProtocol: "SASL_SSL" + tls: + caCert: "/etc/kafka/ca/ca.crt" + sasl: + mechanism: "SCRAM-SHA-512" + user: "my-kafka-user" + password: "{{ secret:my-kafka-user:password }}" + volumes: + - secret: my-cluster-cluster-ca-cert + path: /etc/kafka/ca ``` ### `runtime` diff --git a/e2e/e2e_expose_test.go b/e2e/e2e_expose_test.go index aad8c367a7..41117b7e4a 100644 --- a/e2e/e2e_expose_test.go +++ b/e2e/e2e_expose_test.go @@ -60,6 +60,30 @@ import ( // and s2i. Remote deploys keep pack: host cannot build in-cluster. // --------------------------------------------------------------------------- +// setKedaHTTPTrigger declares an explicit http KEDA trigger in the func.yaml +// at root. The keda deployer requires at least one trigger to be declared +// explicitly (it no longer infers one), so tests that only care about HTTP +// exposure/routing behavior need this to reach a deployable state. +func setKedaHTTPTrigger(t *testing.T, root string) { + t.Helper() + f, err := fn.NewFunction(root) + if err != nil { + t.Fatal(err) + } + // scale.keda requires deployer: keda, and this is written before the + // deploy command has a chance to set it from the --deployer flag. + f.Deployer = "keda" + if f.Scale == nil { + f.Scale = &fn.ScaleOptions{} + } + f.Scale.KEDA = &fn.KEDAScaleOptions{ + Triggers: []fn.KEDATrigger{{Type: "http"}}, + } + if err := f.Write(); err != nil { + t.Fatal(err) + } +} + // requiresOpenShift skips a test whose assertions need a real Route. func requiresOpenShift(t *testing.T) { t.Helper() @@ -177,8 +201,8 @@ func TestExpose_ClusterLocalByDefault(t *testing.T) { if f.Expose != "" { t.Errorf("expected no exposure intent recorded, got %q", f.Expose) } - if f.Deploy.Expose != "" { - t.Errorf("expected no exposure applied, got %q", f.Deploy.Expose) + if f.Deploy.ActiveExpose != "" { + t.Errorf("expected no exposure applied, got %q", f.Deploy.ActiveExpose) } } @@ -194,11 +218,12 @@ func TestExpose_KedaRejectsLongName(t *testing.T) { if len(name) != 45 { t.Fatalf("test setup: expected a 45 character name, got %d", len(name)) } - fromCleanEnv(t, name) + root := fromCleanEnv(t, name) if err := newCmd(t, "init", "-l=go").Run(); err != nil { t.Fatal(err) } + setKedaHTTPTrigger(t, root) out, err := newCmdOutput(t, "deploy", "--builder=host", "--deployer=keda").CombinedOutput() if err == nil { @@ -238,8 +263,8 @@ func TestExpose_Route(t *testing.T) { t.Fatal(err) } ns := f.Deploy.Namespace - if f.Deploy.Expose != "" { - t.Errorf("expected no exposure applied on a flagless deploy, got %q", f.Deploy.Expose) + if f.Deploy.ActiveExpose != "" { + t.Errorf("expected no exposure applied on a flagless deploy, got %q", f.Deploy.ActiveExpose) } if n := routeCount(t, ns, name, ns); n != 0 { t.Fatalf("expected no Route for a cluster-local function, found %d in %q", n, ns) @@ -257,8 +282,8 @@ func TestExpose_Route(t *testing.T) { if f.Expose != fn.ExposeRoute { t.Errorf("expected intent %q, got %q", fn.ExposeRoute, f.Expose) } - if f.Deploy.Expose != fn.ExposeRoute { - t.Errorf("expected applied exposure %q, got %q", fn.ExposeRoute, f.Deploy.Expose) + if f.Deploy.ActiveExpose != fn.ExposeRoute { + t.Errorf("expected applied exposure %q, got %q", fn.ExposeRoute, f.Deploy.ActiveExpose) } ann := serviceAnnotations(t, ns, name) if ann[k8s.RouteHostnameAnnotation] == "" { @@ -279,8 +304,8 @@ func TestExpose_Route(t *testing.T) { if f, err = fn.NewFunction(root); err != nil { t.Fatal(err) } - if f.Deploy.Expose != "" { - t.Errorf("expected applied exposure cleared after opting out, got %q", f.Deploy.Expose) + if f.Deploy.ActiveExpose != "" { + t.Errorf("expected applied exposure cleared after opting out, got %q", f.Deploy.ActiveExpose) } if n := routeCount(t, ns, name, ns); n != 0 { t.Errorf("expected the Route removed on opt-out, found %d in %q", n, ns) @@ -347,8 +372,8 @@ func TestExpose_RouteAllBuilders(t *testing.T) { if err != nil { t.Fatal(err) } - if f.Deploy.Expose != fn.ExposeRoute { - t.Errorf("expected applied exposure %q, got %q", fn.ExposeRoute, f.Deploy.Expose) + if f.Deploy.ActiveExpose != fn.ExposeRoute { + t.Errorf("expected applied exposure %q, got %q", fn.ExposeRoute, f.Deploy.ActiveExpose) } ns := f.Deploy.Namespace ann := serviceAnnotations(t, ns, name) @@ -379,6 +404,7 @@ func TestExpose_KedaRoute(t *testing.T) { if err := newCmd(t, "init", "-l=go").Run(); err != nil { t.Fatal(err) } + setKedaHTTPTrigger(t, root) if err := newCmd(t, "deploy", "--builder=host", "--deployer=keda", "--expose=route").Run(); err != nil { t.Fatal(err) } @@ -388,11 +414,11 @@ func TestExpose_KedaRoute(t *testing.T) { if err != nil { t.Fatal(err) } - if f.Deploy.Deployer != "keda" { - t.Fatalf("expected the keda deployer to be recorded, got %q", f.Deploy.Deployer) + if f.Deploy.ActiveDeployer != "keda" { + t.Fatalf("expected the keda deployer to be recorded, got %q", f.Deploy.ActiveDeployer) } - if f.Deploy.Expose != fn.ExposeRoute { - t.Errorf("expected applied exposure %q, got %q", fn.ExposeRoute, f.Deploy.Expose) + if f.Deploy.ActiveExpose != fn.ExposeRoute { + t.Errorf("expected applied exposure %q, got %q", fn.ExposeRoute, f.Deploy.ActiveExpose) } // An exposed keda function must lead with its external URL. The bridge @@ -413,8 +439,8 @@ func TestExpose_KedaRoute(t *testing.T) { if f, err = fn.NewFunction(root); err != nil { t.Fatal(err) } - if f.Deploy.Expose != "" { - t.Errorf("expected applied exposure cleared after opting out, got %q", f.Deploy.Expose) + if f.Deploy.ActiveExpose != "" { + t.Errorf("expected applied exposure cleared after opting out, got %q", f.Deploy.ActiveExpose) } } @@ -470,6 +496,7 @@ func TestExpose_KedaToggle(t *testing.T) { if err := newCmd(t, "init", "-l=go").Run(); err != nil { t.Fatal(err) } + setKedaHTTPTrigger(t, root) if err := newCmd(t, "deploy", "--builder=host", "--deployer=keda", "--expose=route").Run(); err != nil { t.Fatal(err) } @@ -535,6 +562,7 @@ func TestExpose_KedaDeleteCleansRoute(t *testing.T) { if err := newCmd(t, "init", "-l=go").Run(); err != nil { t.Fatal(err) } + setKedaHTTPTrigger(t, root) if err := newCmd(t, "deploy", "--builder=host", "--deployer=keda", "--expose=route").Run(); err != nil { t.Fatal(err) } @@ -587,6 +615,7 @@ func TestExpose_KedaRouteDomain(t *testing.T) { if err := newCmd(t, "init", "-l=go").Run(); err != nil { t.Fatal(err) } + setKedaHTTPTrigger(t, root) if err := newCmd(t, "deploy", "--builder=host", "--deployer=keda", "--expose=route", "--domain="+domain).Run(); err != nil { t.Fatal(err) } @@ -816,8 +845,8 @@ func TestExpose_RemoteRoute(t *testing.T) { } // Recorded from the cluster by the pipeline describer; empty here means // the pipeline ran a func-util that ignored the intent. - if f.Deploy.Expose != fn.ExposeRoute { - t.Fatalf("expected applied exposure %q read back from the cluster, got %q", fn.ExposeRoute, f.Deploy.Expose) + if f.Deploy.ActiveExpose != fn.ExposeRoute { + t.Fatalf("expected applied exposure %q read back from the cluster, got %q", fn.ExposeRoute, f.Deploy.ActiveExpose) } ns := f.Deploy.Namespace ann := serviceAnnotations(t, ns, name) @@ -852,6 +881,7 @@ func TestExpose_RemoteKedaRoute(t *testing.T) { if err := newCmd(t, "init", "-l=go").Run(); err != nil { t.Fatal(err) } + setKedaHTTPTrigger(t, root) if err := newCmd(t, "deploy", "--remote", "--builder=pack", "--registry="+Registry, "--deployer=keda", "--expose=route").Run(); err != nil { t.Fatal(err) @@ -862,11 +892,11 @@ func TestExpose_RemoteKedaRoute(t *testing.T) { if err != nil { t.Fatal(err) } - if f.Deploy.Deployer != "keda" { - t.Fatalf("expected the keda deployer to be recorded, got %q", f.Deploy.Deployer) + if f.Deploy.ActiveDeployer != "keda" { + t.Fatalf("expected the keda deployer to be recorded, got %q", f.Deploy.ActiveDeployer) } - if f.Deploy.Expose != fn.ExposeRoute { - t.Fatalf("expected applied exposure %q read back from the cluster, got %q", fn.ExposeRoute, f.Deploy.Expose) + if f.Deploy.ActiveExpose != fn.ExposeRoute { + t.Fatalf("expected applied exposure %q read back from the cluster, got %q", fn.ExposeRoute, f.Deploy.ActiveExpose) } ns := f.Deploy.Namespace diff --git a/e2e/e2e_recorder_test.go b/e2e/e2e_recorder_test.go index 8ebb63ab90..99b5e12a8f 100644 --- a/e2e/e2e_recorder_test.go +++ b/e2e/e2e_recorder_test.go @@ -136,7 +136,7 @@ func deployRecorder(t *testing.T, name string) *recorder { t.Fatal(err) } minScale := int64(1) - f.Deploy.Options.Scale = &fn.ScaleOptions{Min: &minScale} + f.Scale = &fn.ScaleOptions{Min: &minScale} if err := f.Write(); err != nil { t.Fatal(err) } diff --git a/pkg/config/config.go b/pkg/config/config.go index 00d7e180a2..c9f8bc96ec 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -133,8 +133,8 @@ func (c Global) Apply(f fn.Function) Global { c.Builder = f.Build.Builder } // opt 1: last deployed deployer - if f.Deploy.Deployer != "" { - c.Deployer = f.Deploy.Deployer + if f.Deploy.ActiveDeployer != "" { + c.Deployer = f.Deploy.ActiveDeployer } // opt 2: intent to deploy deployer if f.Deployer != "" { diff --git a/pkg/deployer/testing/integration_test_helper.go b/pkg/deployer/testing/integration_test_helper.go index baaad2ea77..dcd748e49d 100644 --- a/pkg/deployer/testing/integration_test_helper.go +++ b/pkg/deployer/testing/integration_test_helper.go @@ -417,11 +417,9 @@ func TestInt_Scale(t *testing.T, deployer fn.Deployer, remover fn.Remover, descr // Note: There is no reason for all these being pointers: minScale := int64(2) maxScale := int64(100) - f.Deploy.Options = fn.Options{ - Scale: &fn.ScaleOptions{ - Min: &minScale, - Max: &maxScale, - }, + f.Scale = &fn.ScaleOptions{ + Min: &minScale, + Max: &maxScale, } // Scaffold @@ -738,19 +736,17 @@ func TestInt_FullPath(t *testing.T, deployer fn.Deployer, remover fn.Remover, li // * files under /etc/cm and /etc/sc. // * application also prints the same info to stderr on startup Created: now, + Scale: &fn.ScaleOptions{ + Min: &minScale, + Max: &maxScale, + }, Deploy: fn.DeploySpec{ // 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{ - Min: &minScale, - Max: &maxScale, - }, - }, + Image: "quay.io/mvasek/func-test-service@sha256:2eca4de00d7569c8791634bdbb0c4d5ec8fb061b001549314591e839dabd5269", + Namespace: namespace, + ActiveExpose: "none", + Labels: []fn.Label{{Key: ptr("my-label"), Value: ptr("my-label-value")}}, }, Run: fn.RunSpec{ Envs: []fn.Env{ diff --git a/pkg/functions/client.go b/pkg/functions/client.go index c926956fae..cf0ef39bf9 100644 --- a/pkg/functions/client.go +++ b/pkg/functions/client.go @@ -879,7 +879,7 @@ func (c *Client) Deploy(ctx context.Context, f Function, oo ...DeployOption) (Fu // and expect the user to undeploy first, which removes the resources // correctly. if f.Deploy.Namespace != "" { - if err := deployers.ValidateSwitch(f.Deploy.Deployer, f.Deployer); err != nil { + if err := deployers.ValidateSwitch(f.Deploy.ActiveDeployer, f.Deployer); err != nil { return f, fmt.Errorf("function %q: %w", f.Name, err) } } @@ -924,8 +924,8 @@ func (c *Client) Deploy(ctx context.Context, f Function, oo ...DeployOption) (Fu } // Update the function to reflect the new deployed state of the Function f.Deploy.Namespace = result.Namespace - f.Deploy.Deployer = result.Deployer - f.Deploy.Expose = result.Expose + f.Deploy.ActiveDeployer = result.Deployer + f.Deploy.ActiveExpose = result.Expose // Raw/keda with a nil exposer (library) applied nothing. Knative ignores // expose by design; the CLI already warned. @@ -1184,7 +1184,7 @@ func (c *Client) List(ctx context.Context, namespace string) ([]ListItem, error) // in which case empty namespace is accepted because its existence is checked // in the sub functions remover.Remove and pipelines.Remove. // -// Returns structure 'f' with f.Deploy.Namespace & f.Deploy.Deployer cleared if +// Returns structure 'f' with f.Deploy.Namespace & f.Deploy.ActiveDeployer cleared if // the removal was successful and error returned is nil. If error was // encountered, returns 'f' unmodified. func (c *Client) Remove(ctx context.Context, name, namespace string, f Function, all bool) (Function, error) { @@ -1265,8 +1265,8 @@ func (c *Client) Remove(ctx context.Context, name, namespace string, f Function, // Function.Deployer, Function.Expose, Function.Namespace) is untouched // and is what a subsequent deploy reuses. f.Deploy.Namespace = "" - f.Deploy.Deployer = "" - f.Deploy.Expose = "" + f.Deploy.ActiveDeployer = "" + f.Deploy.ActiveExpose = "" } return f, combinedErr } diff --git a/pkg/functions/client_int_test.go b/pkg/functions/client_int_test.go index 9f47dee8d5..114356cb38 100644 --- a/pkg/functions/client_int_test.go +++ b/pkg/functions/client_int_test.go @@ -149,15 +149,17 @@ func TestInt_Deploy_WithOptions(t *testing.T) { verbose := false f := fn.Function{Runtime: "go", Name: "test-deploy-with-options", Root: root, Namespace: DefaultIntTestNamespace} + f.Scale = &fn.ScaleOptions{ + Min: ptr.Int64(1), + Max: ptr.Int64(10), + KPA: &fn.KPAScaleOptions{ + Metric: ptr.String("concurrency"), + Target: ptr.Float64(5), + Utilization: ptr.Float64(5), + }, + } f.Deploy = fn.DeploySpec{ Options: fn.Options{ - Scale: &fn.ScaleOptions{ - Min: ptr.Int64(1), - Max: ptr.Int64(10), - Metric: ptr.String("concurrency"), - Target: ptr.Float64(5), - Utilization: ptr.Float64(5), - }, Resources: &fn.ResourcesOptions{ Requests: &fn.ResourcesRequestsOptions{ CPU: ptr.String("10m"), diff --git a/pkg/functions/client_test.go b/pkg/functions/client_test.go index 028a27c7c3..4273dca83c 100644 --- a/pkg/functions/client_test.go +++ b/pkg/functions/client_test.go @@ -1276,8 +1276,8 @@ func TestClient_Remove_ReturnsReconciledFunction(t *testing.T) { newFn := func() fn.Function { return fn.Function{ Name: "fn", - Deployer: deployer, // intent - Deploy: fn.DeploySpec{Namespace: "ns", Deployer: deployer}, // state + Deployer: deployer, // intent + Deploy: fn.DeploySpec{Namespace: "ns", ActiveDeployer: deployer}, // state } } @@ -1295,8 +1295,8 @@ func TestClient_Remove_ReturnsReconciledFunction(t *testing.T) { if got.Deploy.Namespace != "" { t.Fatalf("expected Deploy.Namespace cleared on success, got %q", got.Deploy.Namespace) } - if got.Deploy.Deployer != "" { - t.Fatalf("expected Deploy.Deployer cleared on success, got %q", got.Deploy.Deployer) + if got.Deploy.ActiveDeployer != "" { + t.Fatalf("expected Deploy.Deployer cleared on success, got %q", got.Deploy.ActiveDeployer) } // keeps the intent if got.Deployer != deployer { @@ -1318,8 +1318,8 @@ func TestClient_Remove_ReturnsReconciledFunction(t *testing.T) { if got.Deploy.Namespace != "ns" { t.Fatalf("expected Deploy.Namespace preserved on failure, got %q", got.Deploy.Namespace) } - if got.Deploy.Deployer != deployer { - t.Fatalf("expected Deploy.Deployer untouched on failure, got %q", got.Deploy.Deployer) + if got.Deploy.ActiveDeployer != deployer { + t.Fatalf("expected Deploy.Deployer untouched on failure, got %q", got.Deploy.ActiveDeployer) } if got.Deployer != deployer { t.Fatalf("expected the intended Deployer untouched on failure, got %q", got.Deployer) @@ -2677,8 +2677,8 @@ func TestClient_Deploy_BlocksDeployerSwitch(t *testing.T) { Namespace: "ns", Deployer: tt.requested, Deploy: fn.DeploySpec{ - Namespace: tt.deployedNS, - Deployer: tt.deployedWith, + Namespace: tt.deployedNS, + ActiveDeployer: tt.deployedWith, }, } @@ -2729,8 +2729,8 @@ func TestClient_Deploy_PersistsSelfReportedDeployer(t *testing.T) { t.Fatal(err) } - if f.Deploy.Deployer != reported { - t.Fatalf("expected the self-reported deployer %q persisted as state, got %q", reported, f.Deploy.Deployer) + if f.Deploy.ActiveDeployer != reported { + t.Fatalf("expected the self-reported deployer %q persisted as state, got %q", reported, f.Deploy.ActiveDeployer) } if f.Deployer != deployers.Knative { t.Fatalf("expected the requested deployer %q preserved as intent, got %q", deployers.Knative, f.Deployer) diff --git a/pkg/functions/function.go b/pkg/functions/function.go index e40cae3750..88f2c5952c 100644 --- a/pkg/functions/function.go +++ b/pkg/functions/function.go @@ -103,7 +103,7 @@ type Function struct { // Deployer with which to deploy the Function: the requested (intended) // deployer. This is the user's choice and persists across undeploy. // The deployer a Function is CURRENTLY deployed with is recorded separately - // in .Deploy.Deployer, which is cleared on undeploy. + // in .Deploy.ActiveDeployer, which is cleared on undeploy. Deployer string `yaml:"deployer,omitempty" jsonschema:"enum=knative,enum=raw,enum=keda"` // Expose is the requested (intended) external exposure mode for the raw @@ -111,7 +111,7 @@ type Function struct { // Values: "route" (OpenShift Route; OpenShift only), "none" (cluster-local). // Empty means cluster-local. Persists across undeploy like Deployer. // The mode CURRENTLY applied on the cluster is recorded separately in - // .Deploy.Expose, which is cleared on undeploy. + // .Deploy.ActiveExpose, which is cleared on undeploy. Expose string `yaml:"expose,omitempty" jsonschema:"enum=route,enum=none,enum="` // Created time is the moment that creation was successfully completed @@ -132,6 +132,9 @@ type Function struct { // Deploy defines the deployment properties for a function Deploy DeploySpec `yaml:"deploy,omitempty"` + // Scale defines autoscaling configuration for the function. + Scale *ScaleOptions `yaml:"scale,omitempty"` + Local Local `yaml:"-"` } @@ -336,10 +339,10 @@ type DeploySpec struct { // More info: https://kubernetes.io/docs/tasks/configure-pod-container/pull-image-private-registry/ ImagePullSecret string `yaml:"imagePullSecret,omitempty"` - // Deployer records the deployer the Function is CURRENTLY DEPLOYED: - // observed state, written after successful deployment, and cleared on - // undeploy alongside Namespace. - Deployer string `yaml:"deployer,omitempty" jsonschema:"enum=knative,enum=raw,enum=keda"` + // ActiveDeployer records the deployer the Function is CURRENTLY DEPLOYED + // with: observed state, written after successful deployment, and cleared + // on undeploy alongside Namespace. User intent lives on Function.Deployer. + ActiveDeployer string `yaml:"activeDeployer,omitempty" jsonschema:"enum=knative,enum=raw,enum=keda"` Subscriptions []KnativeSubscription `yaml:"subscriptions,omitempty"` @@ -348,11 +351,12 @@ type DeploySpec struct { // the function is managed by default when the func-operator is installed. ManagementDisabled bool `yaml:"managementDisabled,omitempty"` - // Expose records the external exposure mode CURRENTLY applied on the - // cluster for raw/keda (observed state). Written after successful deploy, - // cleared on undeploy alongside Namespace and Deployer. Empty means - // cluster-local (or never exposed). User intent lives on Function.Expose. - Expose string `yaml:"expose,omitempty" jsonschema:"enum=route,enum=none,enum="` + // ActiveExpose records the external exposure mode CURRENTLY applied on + // the cluster for raw/keda (observed state). Written after successful + // deploy, cleared on undeploy alongside Namespace and ActiveDeployer. + // Empty means cluster-local (or never exposed). User intent lives on + // Function.Expose. + ActiveExpose string `yaml:"activeExpose,omitempty" jsonschema:"enum=route,enum=none,enum="` } // HealthEndpoints specify the liveness and readiness endpoints for a Runtime @@ -479,10 +483,11 @@ func (f Function) Validate() error { ValidateBuildEnvs(f.Build.BuildEnvs), ValidateEnvs(f.Run.Envs), validateOptions(f.Deploy.Options), + ValidateScale(f.Scale, f.Deployer, f.Run.Kafka), ValidateLabels(f.Deploy.Labels), validateGit(f.Build.Git), validateKafka(f.Run.Kafka, f.Invoke, f.Runtime), - validateExpose(f.Deploy.Expose, f.Expose), + validateExpose(f.Deploy.ActiveExpose, f.Expose), } var b strings.Builder diff --git a/pkg/functions/function_migrations.go b/pkg/functions/function_migrations.go index 42995cc59c..540cf9c6ed 100644 --- a/pkg/functions/function_migrations.go +++ b/pkg/functions/function_migrations.go @@ -99,6 +99,7 @@ var migrations = []migration{ {"0.34.0", migrateToSpecsStructure}, {"0.35.0", migrateFromInvokeStructure}, {"0.36.0", migratePersistentVolumeTypoFixup}, + {"0.37.0", migrateScaleToTopLevel}, // New Migrations Here. } @@ -356,6 +357,129 @@ func migratePersistentVolumeTypoFixup(fn Function, m migration) (Function, error return fn, nil } +// migrateScaleToTopLevel moves scale config from deploy.options.scale to the +// top-level scale field, moves the flat metric/target/utilization fields +// (from pre-0.37.0 func.yaml files) into the kpa sub-key, and renames +// DeploySpec's observed-state deploy.deployer/deploy.expose YAML keys to +// deploy.activeDeployer/deploy.activeExpose (they collided in name with the +// top-level deployer/expose fields -- user intent -- despite meaning the +// opposite thing: observed, currently-deployed state). +func migrateScaleToTopLevel(f Function, m migration) (Function, error) { + // Read the on-disk func.yaml to capture pre-migration fields that no + // longer deserialize under their current shape: the flat KPA fields + // (Metric, Target, Utilization), which no longer exist on ScaleOptions, + // and deploy.deployer/deploy.expose, since DeploySpec.ActiveDeployer/ + // ActiveExpose (the Go fields the primary unmarshal reads into) now use + // different YAML tags (activeDeployer/activeExpose). + type oldScale struct { + Min *int64 `yaml:"min,omitempty"` + Max *int64 `yaml:"max,omitempty"` + Metric *string `yaml:"metric,omitempty"` + Target *float64 `yaml:"target,omitempty"` + Utilization *float64 `yaml:"utilization,omitempty"` + KEDA *KEDAScaleOptions `yaml:"keda,omitempty"` + KPA *KPAScaleOptions `yaml:"kpa,omitempty"` + } + type oldOptions struct { + Scale *oldScale `yaml:"scale,omitempty"` + } + type oldDeploy struct { + Options oldOptions `yaml:"options,omitempty"` + Deployer string `yaml:"deployer,omitempty"` + Expose string `yaml:"expose,omitempty"` + } + var disk struct { + Deploy oldDeploy `yaml:"deploy,omitempty"` + Scale *ScaleOptions `yaml:"scale,omitempty"` + } + + if f.Root != "" { + bb, err := os.ReadFile(filepath.Join(f.Root, FunctionFile)) + if err == nil { + _ = yaml.Unmarshal(bb, &disk) + } + } + + old := disk.Deploy.Options.Scale + if old == nil && f.Deploy.Options.Scale != nil { + // f.Root is empty (library callers construct a Function without a + // backing file) or the on-disk read found nothing: fall back to the + // already-deserialized in-memory value instead of treating it as + // absent. It can't carry the old flat metric/target/utilization + // fields -- those no longer exist on the current ScaleOptions type, + // so there's nothing on this path to recover them from -- but its + // Min/Max/KEDA/KPA must not be silently dropped. + mem := f.Deploy.Options.Scale + old = &oldScale{Min: mem.Min, Max: mem.Max, KEDA: mem.KEDA, KPA: mem.KPA} + } + + if old != nil { + newScale := &ScaleOptions{ + Min: old.Min, + Max: old.Max, + KEDA: old.KEDA, + KPA: old.KPA, + } + + // scale.kpa is only valid for deployer: knative (or the unset/ + // default deployer, which behaves as knative) -- see ValidateScale. + // Building it here regardless of deployer let a pre-0.37 + // deployer: keda function with legacy flat fields end up with both + // scale.kpa (from this block) and scale.keda (from the block below, + // which always populates it for deployer: keda), which ValidateScale + // then rejects as a mutually-exclusive combination -- so the + // migration itself produced a function that immediately failed + // validation. + hasFlat := old.Metric != nil || old.Target != nil || old.Utilization != nil + validKPADeployer := f.Deployer == "" || f.Deployer == "knative" + if hasFlat && newScale.KPA == nil && validKPADeployer { + newScale.KPA = &KPAScaleOptions{ + Metric: old.Metric, + Target: old.Target, + Utilization: old.Utilization, + } + } + + f.Scale = newScale + } + + // If there was already a top-level scale in the file (shouldn't happen + // in practice, but be defensive), the on-disk value wins. + if disk.Scale != nil { + f.Scale = disk.Scale + } + + // Clear the old location so it doesn't get serialized. + f.Deploy.Options.Scale = nil + + // keda deployer without triggers: default to http + if f.Deployer == "keda" { + if f.Scale == nil { + f.Scale = &ScaleOptions{} + } + if f.Scale.KEDA == nil || len(f.Scale.KEDA.Triggers) == 0 { + f.Scale.KEDA = &KEDAScaleOptions{ + Triggers: []KEDATrigger{{Type: "http"}}, + } + } + } + + // deploy.deployer/deploy.expose moved to deploy.activeDeployer/ + // deploy.activeExpose. f.Root == "" (library callers) needs no handling + // here: the in-memory value already reflects whatever was set via the + // Go field name, unaffected by the YAML tag change, so there's nothing + // on disk to migrate from and nothing to fall back to. + if disk.Deploy.Deployer != "" { + f.Deploy.ActiveDeployer = disk.Deploy.Deployer + } + if disk.Deploy.Expose != "" { + f.Deploy.ActiveExpose = disk.Deploy.Expose + } + + f.SpecVersion = m.version + return f, nil +} + // The pertinent aspects of the Function's schema prior the 1.0.0 version migrations type migrateToSpecs_previousFunction struct { diff --git a/pkg/functions/function_migrations_unit_test.go b/pkg/functions/function_migrations_unit_test.go index b56abffaf0..237362bad0 100644 --- a/pkg/functions/function_migrations_unit_test.go +++ b/pkg/functions/function_migrations_unit_test.go @@ -2,6 +2,7 @@ package functions import ( "os" + "path/filepath" "strings" "sync" "testing" @@ -316,3 +317,333 @@ func writeFunc(f Function, root string) error { } return os.WriteFile(root+"/func.yaml", bb, 0644) } + +func TestMigrateScaleToTopLevel(t *testing.T) { + t.Run("flat fields move to top-level scale.kpa", func(t *testing.T) { + root := t.TempDir() + // Write an old-format func.yaml with flat KPA fields under deploy.options.scale + funcYaml := `specVersion: "0.36.0" +name: testfn +runtime: go +deploy: + options: + scale: + min: 1 + max: 10 + metric: concurrency + target: 100.0 + utilization: 70.0 +` + if err := os.WriteFile(filepath.Join(root, FunctionFile), []byte(funcYaml), 0644); err != nil { + t.Fatal(err) + } + + f := Function{ + SpecVersion: "0.36.0", + Root: root, + } + migrated, err := migrateScaleToTopLevel(f, migration{version: "0.37.0"}) + if err != nil { + t.Fatal(err) + } + + if migrated.SpecVersion != "0.37.0" { + t.Errorf("specVersion = %q, want 0.37.0", migrated.SpecVersion) + } + if migrated.Scale == nil { + t.Fatal("expected top-level scale to be populated") + } + if migrated.Scale.Min == nil || *migrated.Scale.Min != 1 { + t.Errorf("scale.min = %v, want 1", migrated.Scale.Min) + } + if migrated.Scale.Max == nil || *migrated.Scale.Max != 10 { + t.Errorf("scale.max = %v, want 10", migrated.Scale.Max) + } + if migrated.Scale.KPA == nil { + t.Fatal("expected scale.kpa to be populated from flat fields") + } + if *migrated.Scale.KPA.Metric != "concurrency" { + t.Errorf("scale.kpa.metric = %q, want concurrency", *migrated.Scale.KPA.Metric) + } + if *migrated.Scale.KPA.Target != 100.0 { + t.Errorf("scale.kpa.target = %f, want 100", *migrated.Scale.KPA.Target) + } + if *migrated.Scale.KPA.Utilization != 70.0 { + t.Errorf("scale.kpa.utilization = %f, want 70", *migrated.Scale.KPA.Utilization) + } + if migrated.Deploy.Options.Scale != nil { + t.Error("expected deploy.options.scale to be cleared") + } + }) + + t.Run("no-op when no scale fields", func(t *testing.T) { + root := t.TempDir() + funcYaml := `specVersion: "0.36.0" +name: testfn +runtime: go +` + if err := os.WriteFile(filepath.Join(root, FunctionFile), []byte(funcYaml), 0644); err != nil { + t.Fatal(err) + } + + f := Function{SpecVersion: "0.36.0", Root: root} + migrated, err := migrateScaleToTopLevel(f, migration{version: "0.37.0"}) + if err != nil { + t.Fatal(err) + } + if migrated.SpecVersion != "0.37.0" { + t.Errorf("specVersion = %q, want 0.37.0", migrated.SpecVersion) + } + if migrated.Scale != nil { + t.Errorf("expected nil scale, got %+v", migrated.Scale) + } + }) + + t.Run("preserves kpa sub-key when already set", func(t *testing.T) { + root := t.TempDir() + funcYaml := `specVersion: "0.36.0" +name: testfn +runtime: go +deploy: + options: + scale: + kpa: + metric: rps +` + if err := os.WriteFile(filepath.Join(root, FunctionFile), []byte(funcYaml), 0644); err != nil { + t.Fatal(err) + } + + f := Function{SpecVersion: "0.36.0", Root: root} + migrated, err := migrateScaleToTopLevel(f, migration{version: "0.37.0"}) + if err != nil { + t.Fatal(err) + } + if migrated.Scale == nil || migrated.Scale.KPA == nil { + t.Fatal("expected scale.kpa to be preserved") + } + if *migrated.Scale.KPA.Metric != "rps" { + t.Errorf("scale.kpa.metric = %q, want rps", *migrated.Scale.KPA.Metric) + } + }) + + t.Run("keda deployer gets http trigger", func(t *testing.T) { + root := t.TempDir() + funcYaml := `specVersion: "0.36.0" +name: testfn +runtime: go +deployer: keda +` + if err := os.WriteFile(filepath.Join(root, FunctionFile), []byte(funcYaml), 0644); err != nil { + t.Fatal(err) + } + + f := Function{SpecVersion: "0.36.0", Deployer: "keda", Root: root} + migrated, err := migrateScaleToTopLevel(f, migration{version: "0.37.0"}) + if err != nil { + t.Fatal(err) + } + if migrated.Scale == nil || migrated.Scale.KEDA == nil { + t.Fatal("expected scale.keda to be populated") + } + triggers := migrated.Scale.KEDA.Triggers + if len(triggers) != 1 || triggers[0].Type != "http" { + t.Errorf("expected [{http}], got %v", triggers) + } + }) + + t.Run("keda deployer with existing triggers unchanged", func(t *testing.T) { + root := t.TempDir() + funcYaml := `specVersion: "0.36.0" +name: testfn +runtime: go +deployer: keda +deploy: + options: + scale: + keda: + triggers: + - type: kafka +` + if err := os.WriteFile(filepath.Join(root, FunctionFile), []byte(funcYaml), 0644); err != nil { + t.Fatal(err) + } + + f := Function{ + SpecVersion: "0.36.0", + Deployer: "keda", + Root: root, + } + migrated, err := migrateScaleToTopLevel(f, migration{version: "0.37.0"}) + if err != nil { + t.Fatal(err) + } + triggers := migrated.Scale.KEDA.Triggers + if len(triggers) != 1 || triggers[0].Type != "kafka" { + t.Errorf("expected [{kafka}], got %v", triggers) + } + }) + + t.Run("non-keda deployer no triggers added", func(t *testing.T) { + root := t.TempDir() + funcYaml := `specVersion: "0.36.0" +name: testfn +runtime: go +deployer: raw +` + if err := os.WriteFile(filepath.Join(root, FunctionFile), []byte(funcYaml), 0644); err != nil { + t.Fatal(err) + } + + f := Function{SpecVersion: "0.36.0", Deployer: "raw", Root: root} + migrated, err := migrateScaleToTopLevel(f, migration{version: "0.37.0"}) + if err != nil { + t.Fatal(err) + } + if migrated.Scale != nil { + t.Errorf("expected no scale for raw deployer, got %+v", migrated.Scale) + } + }) + + t.Run("empty Root falls back to the in-memory scale instead of dropping it", func(t *testing.T) { + // Library callers can construct a Function with no backing file + // (Root == ""). The migration must not silently clear + // Deploy.Options.Scale without moving it to the top-level field. + min := int64(2) + max := int64(20) + f := Function{ + SpecVersion: "0.36.0", + Deploy: DeploySpec{ + Options: Options{ + Scale: &ScaleOptions{Min: &min, Max: &max}, + }, + }, + } + migrated, err := migrateScaleToTopLevel(f, migration{version: "0.37.0"}) + if err != nil { + t.Fatal(err) + } + if migrated.Scale == nil { + t.Fatal("expected the in-memory scale to be moved to the top level, got nil") + } + if migrated.Scale.Min == nil || *migrated.Scale.Min != 2 { + t.Errorf("scale.min = %v, want 2", migrated.Scale.Min) + } + if migrated.Scale.Max == nil || *migrated.Scale.Max != 20 { + t.Errorf("scale.max = %v, want 20", migrated.Scale.Max) + } + if migrated.Deploy.Options.Scale != nil { + t.Error("expected deploy.options.scale to be cleared") + } + }) + + t.Run("keda deployer with legacy flat fields does not produce scale.kpa", func(t *testing.T) { + // scale.kpa is only valid for deployer: knative. Building it from + // legacy flat fields regardless of deployer, combined with the + // keda-defaults-to-http-trigger block always setting scale.keda for + // deployer: keda, previously produced a migrated function with both + // scale.kpa and scale.keda set -- which ValidateScale rejects as + // mutually exclusive. + root := t.TempDir() + funcYaml := `specVersion: "0.36.0" +name: testfn +runtime: go +deployer: keda +deploy: + options: + scale: + metric: concurrency + target: 100.0 + utilization: 70.0 +` + if err := os.WriteFile(filepath.Join(root, FunctionFile), []byte(funcYaml), 0644); err != nil { + t.Fatal(err) + } + + f := Function{SpecVersion: "0.36.0", Deployer: "keda", Root: root} + migrated, err := migrateScaleToTopLevel(f, migration{version: "0.37.0"}) + if err != nil { + t.Fatal(err) + } + if migrated.Scale == nil { + t.Fatal("expected scale to be populated") + } + if migrated.Scale.KPA != nil { + t.Errorf("expected scale.kpa to stay nil for deployer: keda, got %+v", migrated.Scale.KPA) + } + if migrated.Scale.KEDA == nil || len(migrated.Scale.KEDA.Triggers) == 0 { + t.Fatal("expected scale.keda to be populated with a default http trigger") + } + if errs := ValidateScale(migrated.Scale, "keda", nil); len(errs) != 0 { + t.Errorf("expected the migrated scale to pass validation, got: %v", errs) + } + }) + + t.Run("old deploy.deployer/deploy.expose keys move to the renamed fields", func(t *testing.T) { + root := t.TempDir() + funcYaml := `specVersion: "0.36.0" +name: testfn +runtime: go +deploy: + deployer: keda + expose: route +` + if err := os.WriteFile(filepath.Join(root, FunctionFile), []byte(funcYaml), 0644); err != nil { + t.Fatal(err) + } + + f := Function{SpecVersion: "0.36.0", Root: root} + migrated, err := migrateScaleToTopLevel(f, migration{version: "0.37.0"}) + if err != nil { + t.Fatal(err) + } + if migrated.Deploy.ActiveDeployer != "keda" { + t.Errorf("Deploy.ActiveDeployer = %q, want keda", migrated.Deploy.ActiveDeployer) + } + if migrated.Deploy.ActiveExpose != "route" { + t.Errorf("Deploy.ActiveExpose = %q, want route", migrated.Deploy.ActiveExpose) + } + }) + + t.Run("no-op when neither old deployer/expose key is present", func(t *testing.T) { + root := t.TempDir() + funcYaml := `specVersion: "0.36.0" +name: testfn +runtime: go +` + if err := os.WriteFile(filepath.Join(root, FunctionFile), []byte(funcYaml), 0644); err != nil { + t.Fatal(err) + } + + f := Function{SpecVersion: "0.36.0", Root: root} + migrated, err := migrateScaleToTopLevel(f, migration{version: "0.37.0"}) + if err != nil { + t.Fatal(err) + } + if migrated.Deploy.ActiveDeployer != "" || migrated.Deploy.ActiveExpose != "" { + t.Errorf("expected both fields to stay empty, got ActiveDeployer=%q ActiveExpose=%q", + migrated.Deploy.ActiveDeployer, migrated.Deploy.ActiveExpose) + } + }) + + t.Run("empty Root does not touch the in-memory deployer/expose value", func(t *testing.T) { + // Library callers can construct a Function with no backing file. + // The in-memory Deploy.ActiveDeployer/ActiveExpose already reflect + // whatever the caller set via the current Go field names -- this + // migration must leave them alone, not clear them. + f := Function{ + SpecVersion: "0.36.0", + Deploy: DeploySpec{ActiveDeployer: "raw", ActiveExpose: "none"}, + } + migrated, err := migrateScaleToTopLevel(f, migration{version: "0.37.0"}) + if err != nil { + t.Fatal(err) + } + if migrated.Deploy.ActiveDeployer != "raw" { + t.Errorf("Deploy.ActiveDeployer = %q, want raw", migrated.Deploy.ActiveDeployer) + } + if migrated.Deploy.ActiveExpose != "none" { + t.Errorf("Deploy.ActiveExpose = %q, want none", migrated.Deploy.ActiveExpose) + } + }) +} diff --git a/pkg/functions/function_options.go b/pkg/functions/function_options.go index 1af6a32b7e..32b71487c0 100644 --- a/pkg/functions/function_options.go +++ b/pkg/functions/function_options.go @@ -7,15 +7,41 @@ import ( ) type Options struct { - Scale *ScaleOptions `yaml:"scale,omitempty"` + // Scale is kept for YAML deserialization of old func.yaml files (pre-0.37.0 + // stored scale under deploy.options.scale). The 0.34.0 migration writes to + // it and the 0.37.0 migration moves it to Function.Scale. Hidden from the + // JSON schema so new files never use this path. + Scale *ScaleOptions `yaml:"scale,omitempty" jsonschema:"-"` Resources *ResourcesOptions `yaml:"resources,omitempty"` } type ScaleOptions struct { - Min *int64 `yaml:"min,omitempty" jsonschema_extras:"minimum=0"` - Max *int64 `yaml:"max,omitempty" jsonschema_extras:"minimum=0"` + Min *int64 `yaml:"min,omitempty" jsonschema_extras:"minimum=0"` + Max *int64 `yaml:"max,omitempty" jsonschema_extras:"minimum=0"` + KEDA *KEDAScaleOptions `yaml:"keda,omitempty"` + KPA *KPAScaleOptions `yaml:"kpa,omitempty"` +} + +type KEDAScaleOptions struct { + PollingInterval *int32 `yaml:"pollingInterval,omitempty" jsonschema_extras:"minimum=1"` + CooldownPeriod *int32 `yaml:"cooldownPeriod,omitempty" jsonschema_extras:"minimum=1"` + Triggers []KEDATrigger `yaml:"triggers,omitempty"` +} + +type KEDATrigger struct { + Type string `yaml:"type" jsonschema:"enum=http,enum=kafka,enum=cron"` + TargetValue *int64 `yaml:"targetValue,omitempty" jsonschema_extras:"minimum=1"` + LagThreshold *int64 `yaml:"lagThreshold,omitempty" jsonschema_extras:"minimum=1"` + ActivationLagThreshold *int64 `yaml:"activationLagThreshold,omitempty" jsonschema_extras:"minimum=0"` + Timezone string `yaml:"timezone,omitempty"` + Start string `yaml:"start,omitempty"` + End string `yaml:"end,omitempty"` + DesiredReplicas *int64 `yaml:"desiredReplicas,omitempty" jsonschema_extras:"minimum=1"` +} + +type KPAScaleOptions struct { Metric *string `yaml:"metric,omitempty" jsonschema:"enum=concurrency,enum=rps"` - Target *float64 `yaml:"target,omitempty" jsonschema_extras:"minimum=0.01"` + Target *float64 `yaml:"target,omitempty" jsonschema:"exclusiveMinimum=true" jsonschema_extras:"minimum=0.01"` // exclusiveMinimum=true: jsonschema_extras' "minimum" truncates "0.01" to 0 via strconv.Atoi, so this at least excludes the concrete invalid value (0) ValidateScale rejects Utilization *float64 `yaml:"utilization,omitempty" jsonschema:"minimum=1,maximum=100"` } @@ -36,54 +62,10 @@ type ResourcesRequestsOptions struct { } // validateOptions checks that input Options are correctly set. +// Scale validation is handled separately by ValidateScale. // Returns array of error messages, empty if no errors are found func validateOptions(options Options) (errors []string) { - // options.scale - if options.Scale != nil { - if options.Scale.Min != nil { - if *options.Scale.Min < 0 { - errors = append(errors, fmt.Sprintf("options field \"scale.min\" has invalid value set: %d, the value must be greater than \"0\"", - *options.Scale.Min)) - } - } - - if options.Scale.Max != nil { - if *options.Scale.Max < 0 { - errors = append(errors, fmt.Sprintf("options field \"scale.max\" has invalid value set: %d, the value must be greater than \"0\"", - *options.Scale.Max)) - } - } - - if options.Scale.Min != nil && options.Scale.Max != nil { - if *options.Scale.Max < *options.Scale.Min { - errors = append(errors, "options field \"scale.max\" value must be greater or equal to \"scale.min\"") - } - } - - if options.Scale.Metric != nil { - if *options.Scale.Metric != "concurrency" && *options.Scale.Metric != "rps" { - errors = append(errors, fmt.Sprintf("options field \"scale.metric\" has invalid value set: %s, allowed is only \"concurrency\" or \"rps\"", - *options.Scale.Metric)) - } - } - - if options.Scale.Target != nil { - if *options.Scale.Target < 0.01 { - errors = append(errors, fmt.Sprintf("options field \"scale.target\" has value set to \"%f\", but it must not be less than 0.01", - *options.Scale.Target)) - } - } - - if options.Scale.Utilization != nil { - if *options.Scale.Utilization < 1 || *options.Scale.Utilization > 100 { - errors = append(errors, - fmt.Sprintf("options field \"scale.utilization\" has value set to \"%f\", but it must not be less than 1 or greater than 100", - *options.Scale.Utilization)) - } - } - } - // options.resource if options.Resources != nil { diff --git a/pkg/functions/function_options_unit_test.go b/pkg/functions/function_options_unit_test.go index 9b798b0362..9709f2af1f 100644 --- a/pkg/functions/function_options_unit_test.go +++ b/pkg/functions/function_options_unit_test.go @@ -13,33 +13,6 @@ func Test_validateOptions(t *testing.T) { options Options errs int }{ - { - "correct 'scale.metric' - concurrency", - Options{ - Scale: &ScaleOptions{ - Metric: ptr.String("concurrency"), - }, - }, - 0, - }, - { - "correct 'scale.metric' - rps", - Options{ - Scale: &ScaleOptions{ - Metric: ptr.String("rps"), - }, - }, - 0, - }, - { - "incorrect 'scale.metric'", - Options{ - Scale: &ScaleOptions{ - Metric: ptr.String("foo"), - }, - }, - 1, - }, { "correct 'scale.min'", Options{ @@ -58,89 +31,6 @@ func Test_validateOptions(t *testing.T) { }, 0, }, - { - "correct 'scale.min' & 'scale.max'", - Options{ - Scale: &ScaleOptions{ - Min: ptr.Int64(0), - Max: ptr.Int64(10), - }, - }, - 0, - }, - { - "incorrect 'scale.min' & 'scale.max'", - Options{ - Scale: &ScaleOptions{ - Min: ptr.Int64(100), - Max: ptr.Int64(10), - }, - }, - 1, - }, - { - "incorrect 'scale.min' - negative value", - Options{ - Scale: &ScaleOptions{ - Min: ptr.Int64(-10), - }, - }, - 1, - }, - { - "incorrect 'scale.max' - negative value", - Options{ - Scale: &ScaleOptions{ - Max: ptr.Int64(-10), - }, - }, - 1, - }, - { - "correct 'scale.target'", - Options{ - Scale: &ScaleOptions{ - Target: ptr.Float64(50), - }, - }, - 0, - }, - { - "incorrect 'scale.target'", - Options{ - Scale: &ScaleOptions{ - Target: ptr.Float64(0), - }, - }, - 1, - }, - { - "correct 'scale.utilization'", - Options{ - Scale: &ScaleOptions{ - Utilization: ptr.Float64(50), - }, - }, - 0, - }, - { - "incorrect 'scale.utilization' - < 1", - Options{ - Scale: &ScaleOptions{ - Utilization: ptr.Float64(0), - }, - }, - 1, - }, - { - "incorrect 'scale.utilization' - > 100", - Options{ - Scale: &ScaleOptions{ - Utilization: ptr.Float64(110), - }, - }, - 1, - }, { "correct 'resources.requests.cpu'", Options{ @@ -263,7 +153,7 @@ func Test_validateOptions(t *testing.T) { 1, }, { - "correct all options", + "correct all resource options", Options{ Resources: &ResourcesOptions{ Requests: &ResourcesRequestsOptions{ @@ -276,18 +166,11 @@ func Test_validateOptions(t *testing.T) { Concurrency: ptr.Int64(10), }, }, - Scale: &ScaleOptions{ - Min: ptr.Int64(0), - Max: ptr.Int64(10), - Metric: ptr.String("concurrency"), - Target: ptr.Float64(40.5), - Utilization: ptr.Float64(35.5), - }, }, 0, }, { - "incorrect all options", + "incorrect all resource options", Options{ Resources: &ResourcesOptions{ Requests: &ResourcesRequestsOptions{ @@ -300,15 +183,8 @@ func Test_validateOptions(t *testing.T) { Concurrency: ptr.Int64(-1), }, }, - Scale: &ScaleOptions{ - Min: ptr.Int64(-1), - Max: ptr.Int64(-1), - Metric: ptr.String("foo"), - Target: ptr.Float64(-1), - Utilization: ptr.Float64(110), - }, }, - 10, + 5, }, } @@ -319,5 +195,263 @@ func Test_validateOptions(t *testing.T) { } }) } +} +func Test_ValidateScale(t *testing.T) { + tests := []struct { + name string + scale *ScaleOptions + deployer string + kafka *KafkaConfig + errs int + }{ + { + "nil scale is valid", + nil, "", nil, 0, + }, + { + "correct min", + &ScaleOptions{Min: ptr.Int64(1)}, + "", nil, 0, + }, + { + "correct max", + &ScaleOptions{Max: ptr.Int64(10)}, + "", nil, 0, + }, + { + "correct min & max", + &ScaleOptions{Min: ptr.Int64(0), Max: ptr.Int64(10)}, + "", nil, 0, + }, + { + "incorrect min & max", + &ScaleOptions{Min: ptr.Int64(100), Max: ptr.Int64(10)}, + "", nil, 1, + }, + { + "negative min", + &ScaleOptions{Min: ptr.Int64(-10)}, + "", nil, 1, + }, + { + "negative max", + &ScaleOptions{Max: ptr.Int64(-10)}, + "", nil, 1, + }, + { + "keda and kpa mutually exclusive", + &ScaleOptions{ + KEDA: &KEDAScaleOptions{Triggers: []KEDATrigger{{Type: "http"}}}, + KPA: &KPAScaleOptions{Metric: ptr.String("concurrency")}, + }, + "keda", nil, 1, + }, + { + "valid kpa options", + &ScaleOptions{ + KPA: &KPAScaleOptions{ + Metric: ptr.String("rps"), + Target: ptr.Float64(50), + Utilization: ptr.Float64(80), + }, + }, + "knative", nil, 0, + }, + { + "invalid kpa metric", + &ScaleOptions{ + KPA: &KPAScaleOptions{Metric: ptr.String("bad")}, + }, + "knative", nil, 1, + }, + { + "kpa target too low", + &ScaleOptions{ + KPA: &KPAScaleOptions{Target: ptr.Float64(0)}, + }, + "knative", nil, 1, + }, + { + "kpa utilization out of range", + &ScaleOptions{ + KPA: &KPAScaleOptions{Utilization: ptr.Float64(110)}, + }, + "knative", nil, 1, + }, + { + "kpa requires knative deployer", + &ScaleOptions{ + KPA: &KPAScaleOptions{Metric: ptr.String("concurrency")}, + }, + "raw", nil, 1, + }, + { + "valid keda http trigger", + &ScaleOptions{ + KEDA: &KEDAScaleOptions{ + Triggers: []KEDATrigger{ + {Type: "http"}, + }, + }, + }, + "keda", nil, 0, + }, + { + "valid keda kafka trigger", + &ScaleOptions{ + KEDA: &KEDAScaleOptions{ + Triggers: []KEDATrigger{ + {Type: "kafka", LagThreshold: ptr.Int64(10)}, + }, + }, + }, + "keda", &KafkaConfig{Brokers: "b", Topic: "t", ConsumerGroup: "g"}, 0, + }, + { + "keda http and kafka triggers combined is not yet supported", + &ScaleOptions{ + KEDA: &KEDAScaleOptions{ + Triggers: []KEDATrigger{ + {Type: "http"}, + {Type: "kafka", LagThreshold: ptr.Int64(10)}, + }, + }, + }, + "keda", &KafkaConfig{Brokers: "b", Topic: "t", ConsumerGroup: "g"}, 1, + }, + { + "empty keda triggers", + &ScaleOptions{ + KEDA: &KEDAScaleOptions{}, + }, + "keda", nil, 2, + }, + { + "invalid keda trigger type", + &ScaleOptions{ + KEDA: &KEDAScaleOptions{ + Triggers: []KEDATrigger{{Type: "invalid"}}, + }, + }, + "keda", nil, 1, + }, + { + "keda cron trigger is not yet supported", + &ScaleOptions{ + KEDA: &KEDAScaleOptions{ + Triggers: []KEDATrigger{{Type: "cron"}}, + }, + }, + "keda", nil, 1, + }, + { + "fully specified keda cron trigger is still not yet supported", + &ScaleOptions{ + KEDA: &KEDAScaleOptions{ + Triggers: []KEDATrigger{ + {Type: "cron", Timezone: "UTC", Start: "0 8 * * *", End: "0 20 * * *", DesiredReplicas: ptr.Int64(3)}, + }, + }, + }, + "keda", nil, 1, + }, + { + "keda requires deployer keda", + &ScaleOptions{ + KEDA: &KEDAScaleOptions{Triggers: []KEDATrigger{{Type: "http"}}}, + }, + "knative", nil, 1, + }, + { + "keda deployer requires triggers", + nil, "keda", nil, 1, + }, + { + "kafka trigger without kafka config", + &ScaleOptions{ + KEDA: &KEDAScaleOptions{ + Triggers: []KEDATrigger{{Type: "kafka"}}, + }, + }, + "keda", nil, 1, + }, + { + "keda pollingInterval too low", + &ScaleOptions{ + KEDA: &KEDAScaleOptions{ + PollingInterval: ptr.Int32(0), + Triggers: []KEDATrigger{{Type: "http"}}, + }, + }, + "keda", nil, 1, + }, + { + "keda cooldownPeriod too low", + &ScaleOptions{ + KEDA: &KEDAScaleOptions{ + CooldownPeriod: ptr.Int32(0), + Triggers: []KEDATrigger{{Type: "http"}}, + }, + }, + "keda", nil, 1, + }, + { + "http targetValue too low", + &ScaleOptions{ + KEDA: &KEDAScaleOptions{ + Triggers: []KEDATrigger{{Type: "http", TargetValue: ptr.Int64(0)}}, + }, + }, + "keda", nil, 1, + }, + { + "keda max 0 is invalid: not a valid HPA maxReplicas", + &ScaleOptions{ + Max: ptr.Int64(0), + KEDA: &KEDAScaleOptions{Triggers: []KEDATrigger{{Type: "http"}}}, + }, + "keda", nil, 1, + }, + { + "knative max 0 means no limit, still valid", + &ScaleOptions{ + Max: ptr.Int64(0), + KPA: &KPAScaleOptions{Metric: ptr.String("concurrency")}, + }, + "knative", nil, 0, + }, + { + "duplicate http triggers rejected", + &ScaleOptions{ + KEDA: &KEDAScaleOptions{ + Triggers: []KEDATrigger{ + {Type: "http", TargetValue: ptr.Int64(100)}, + {Type: "http", TargetValue: ptr.Int64(200)}, + }, + }, + }, + "keda", nil, 1, + }, + { + "duplicate kafka triggers rejected", + &ScaleOptions{ + KEDA: &KEDAScaleOptions{ + Triggers: []KEDATrigger{ + {Type: "kafka", LagThreshold: ptr.Int64(5)}, + {Type: "kafka", LagThreshold: ptr.Int64(10)}, + }, + }, + }, + "keda", &KafkaConfig{Brokers: "b", Topic: "t", ConsumerGroup: "g"}, 1, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := ValidateScale(tt.scale, tt.deployer, tt.kafka); len(got) != tt.errs { + t.Errorf("ValidateScale() = %v\n got %d errors but want %d", got, len(got), tt.errs) + } + }) + } } diff --git a/pkg/functions/function_scale.go b/pkg/functions/function_scale.go new file mode 100644 index 0000000000..50ca7c0a77 --- /dev/null +++ b/pkg/functions/function_scale.go @@ -0,0 +1,131 @@ +package functions + +import "fmt" + +// ValidateScale validates the top-level scale configuration against the chosen +// deployer and Kafka config. It replaces the previous validateScaleDeployer, +// validateKEDAScale, and validateKPAScale functions with a single entry point. +func ValidateScale(scale *ScaleOptions, deployer string, kafka *KafkaConfig) (errors []string) { + if deployer == "keda" && (scale == nil || scale.KEDA == nil || len(scale.KEDA.Triggers) == 0) { + errors = append(errors, "deployer keda requires at least one trigger in scale.keda.triggers") + } + if scale == nil { + return + } + + if scale.Min != nil && *scale.Min < 0 { + errors = append(errors, fmt.Sprintf("scale.min has invalid value: %d, must be >= 0", *scale.Min)) + } + if scale.Max != nil && *scale.Max < 0 { + errors = append(errors, fmt.Sprintf("scale.max has invalid value: %d, must be >= 0", *scale.Max)) + } + if scale.Min != nil && scale.Max != nil && *scale.Max < *scale.Min { + errors = append(errors, "scale.max must be >= scale.min") + } + if deployer == "keda" && scale.Max != nil && *scale.Max == 0 { + // 0 means "no limit" for the knative/kpa deployer, but keda's + // HTTPScaledObject/ScaledObject map it straight to the HPA's + // maxReplicas, which must be >= 1. Leave scale.max unset to get + // keda's own default instead. + errors = append(errors, "scale.max must be >= 1 when deployer is keda: 0 (\"no limit\") is not a valid value, leave scale.max unset to use keda's default") + } + + if scale.KEDA != nil && scale.KPA != nil { + errors = append(errors, "scale.keda and scale.kpa are mutually exclusive") + return + } + if scale.KEDA != nil && deployer != "keda" { + errors = append(errors, "scale.keda requires deployer: keda") + } + if scale.KPA != nil && deployer != "knative" && deployer != "" { + errors = append(errors, "scale.kpa requires deployer: knative") + } + + if scale.KEDA != nil { + errors = append(errors, validateKEDAScale(scale.KEDA, kafka)...) + } + if scale.KPA != nil { + errors = append(errors, validateKPAScale(scale.KPA)...) + } + + return +} + +func validateKEDAScale(keda *KEDAScaleOptions, kafka *KafkaConfig) (errors []string) { + if len(keda.Triggers) == 0 { + errors = append(errors, "scale.keda.triggers must not be empty when scale.keda is set") + return + } + + if keda.PollingInterval != nil && *keda.PollingInterval < 1 { + errors = append(errors, "scale.keda.pollingInterval must be >= 1") + } + if keda.CooldownPeriod != nil && *keda.CooldownPeriod < 1 { + errors = append(errors, "scale.keda.cooldownPeriod must be >= 1") + } + + var sawHTTP, sawKafka bool + seenTypes := map[string]bool{} + for i, t := range keda.Triggers { + // The deployer only ever materializes one HTTPScaledObject and one + // Kafka ScaledObject regardless of how many triggers of that type + // are configured -- kafkaTrigger() and the HTTP targetValue lookup + // both take just the first match. A second trigger of the same type + // would silently have its settings ignored, so reject it here + // instead. + if seenTypes[t.Type] && (t.Type == "http" || t.Type == "kafka") { + errors = append(errors, fmt.Sprintf("scale.keda.triggers[%d].type %q is repeated: only one trigger of each type is supported", i, t.Type)) + } + seenTypes[t.Type] = true + + switch t.Type { + case "http": + sawHTTP = true + if t.TargetValue != nil && *t.TargetValue < 1 { + errors = append(errors, fmt.Sprintf("scale.keda.triggers[%d].targetValue must be >= 1", i)) + } + case "kafka": + sawKafka = true + if kafka == nil { + errors = append(errors, fmt.Sprintf("scale.keda.triggers[%d] has type kafka but run.kafka is not configured", i)) + } + if t.LagThreshold != nil && *t.LagThreshold < 1 { + errors = append(errors, fmt.Sprintf("scale.keda.triggers[%d].lagThreshold must be >= 1", i)) + } + if t.ActivationLagThreshold != nil && *t.ActivationLagThreshold < 0 { + errors = append(errors, fmt.Sprintf("scale.keda.triggers[%d].activationLagThreshold must not be negative", i)) + } + case "cron": + // "cron" is a valid value in func.yaml's schema, reserved for a + // future deployer implementation, but the keda deployer does not + // implement it yet: accepting it here would deploy successfully + // and silently create no scaler at all. + errors = append(errors, fmt.Sprintf("scale.keda.triggers[%d].type cron is not yet supported", i)) + default: + errors = append(errors, fmt.Sprintf("scale.keda.triggers[%d].type has invalid value %q, allowed: http, kafka", i, t.Type)) + } + } + + // The keda deployer creates a separate HTTPScaledObject for "http" and a + // separate ScaledObject for "kafka", both targeting the same Deployment. + // KEDA only allows one scaler per workload, so combining them is + // rejected up front instead of failing later, mid-deploy, against the + // Kubernetes API. + if sawHTTP && sawKafka { + errors = append(errors, "scale.keda.triggers must not combine type http with type kafka: they cannot scale the same Deployment together, not yet supported") + } + return +} + +func validateKPAScale(kpa *KPAScaleOptions) (errors []string) { + if kpa.Metric != nil && *kpa.Metric != "concurrency" && *kpa.Metric != "rps" { + errors = append(errors, fmt.Sprintf("scale.kpa.metric has invalid value: %s, allowed: concurrency, rps", *kpa.Metric)) + } + if kpa.Target != nil && *kpa.Target < 0.01 { + errors = append(errors, fmt.Sprintf("scale.kpa.target must be >= 0.01, got %f", *kpa.Target)) + } + if kpa.Utilization != nil && (*kpa.Utilization < 1 || *kpa.Utilization > 100) { + errors = append(errors, fmt.Sprintf("scale.kpa.utilization must be 1-100, got %f", *kpa.Utilization)) + } + return +} diff --git a/pkg/k8s/deployer.go b/pkg/k8s/deployer.go index d29bcc3d8d..3da9b68b63 100644 --- a/pkg/k8s/deployer.go +++ b/pkg/k8s/deployer.go @@ -673,8 +673,8 @@ func (d *Deployer) generateDeployment(f fn.Function, namespace string, labels, a SetSecurityContext(&container) replicas := int32(1) - if f.Deploy.Options.Scale != nil && f.Deploy.Options.Scale.Min != nil && *f.Deploy.Options.Scale.Min > 0 { - replicas = int32(*f.Deploy.Options.Scale.Min) + if f.Scale != nil && f.Scale.Min != nil && *f.Scale.Min > 0 { + replicas = int32(*f.Scale.Min) } deployment := &appsv1.Deployment{ diff --git a/pkg/k8s/wait.go b/pkg/k8s/wait.go index 4d46736331..80af28781d 100644 --- a/pkg/k8s/wait.go +++ b/pkg/k8s/wait.go @@ -69,6 +69,10 @@ func checkIfDeploymentIsAvailable(ctx context.Context, clientset *kubernetes.Cli desiredReplicas := *deployment.Spec.Replicas + if desiredReplicas == 0 { + return true, nil + } + // Check if deployment is available for _, condition := range deployment.Status.Conditions { if condition.Type == appsv1.DeploymentAvailable && condition.Status == corev1.ConditionTrue { diff --git a/pkg/keda/deployer.go b/pkg/keda/deployer.go index 86d268d2eb..d519a3a8bf 100644 --- a/pkg/keda/deployer.go +++ b/pkg/keda/deployer.go @@ -3,6 +3,7 @@ package keda import ( "context" "fmt" + "os" "time" httpv1alpha1 "github.com/kedacore/http-add-on/operator/apis/http/v1alpha1" @@ -99,8 +100,72 @@ func (k *kedaDeployerDecorator) UpdateLabels(function fn.Function, labels map[st } func (d *Deployer) Deploy(ctx context.Context, f fn.Function) (fn.DeploymentResult, error) { - if err := validateBridgeName(f.Name); err != nil { - return fn.DeploymentResult{}, err + triggers := triggers(f) + if len(triggers) == 0 { + // triggers(f) only returns empty when scale.keda is present with an + // explicitly empty triggers list (the nil-Scale/nil-KEDA case falls + // back to a plain http trigger). ValidateScale already rejects this, + // but Deploy is reachable without Function.Validate first (library + // callers, tests): without this check, Deploy would silently skip + // both the HTTPScaledObject and Kafka ScaledObject paths and deploy + // with no scaler at all. + return fn.DeploymentResult{}, fmt.Errorf("function %q: deployer keda requires at least one trigger in scale.keda.triggers", f.Name) + } + wantHTTP := hasHTTPTrigger(triggers) + wantKafka := hasKafkaTrigger(triggers) + + for i, t := range triggers { + if t.Type != "http" && t.Type != "kafka" { + // ValidateScale already rejects any type other than http/kafka + // (cron is explicitly unsupported; anything else is invalid), + // but Deploy is reachable without it first (library callers, + // tests): an unrecognized type makes both wantHTTP and + // wantKafka false, so without this check Deploy would + // silently skip every scaler path and deploy the raw + // workload with no scaling at all, instead of failing. + return fn.DeploymentResult{}, fmt.Errorf( + "function %q: scale.keda.triggers[%d].type has invalid value %q, allowed: http, kafka", f.Name, i, t.Type) + } + } + if wantHTTP && wantKafka { + // ValidateScale already rejects this combination, but Deploy is + // reachable without it first: the deployer creates a separate + // HTTPScaledObject for "http" and a separate ScaledObject for + // "kafka", both targeting the same Deployment, and KEDA only + // allows one scaler per workload. + return fn.DeploymentResult{}, fmt.Errorf( + "function %q: scale.keda.triggers must not combine type http with type kafka: they cannot scale the same Deployment together, not yet supported", f.Name) + } + + if wantHTTP { + if err := validateBridgeName(f.Name); err != nil { + return fn.DeploymentResult{}, err + } + } + + // The following are all pure functions of f -- no cluster state needed -- + // so they run before d.Deployer.Deploy creates anything. ValidateScale + // already rejects each of these, but Deploy is reachable without going + // through Function.Validate first (library callers, tests): failing + // before the raw Deployment/Service exist, rather than after, avoids + // leaving a partial workload behind with no Kafka scaler and no error + // pointing at why. + minScale, maxScale := replicaBounds(f) + if maxScale < 1 { + // deployer: keda's HTTPScaledObject/ScaledObject map scale.max + // straight into an HPA's maxReplicas, which must be >= 1. + return fn.DeploymentResult{}, fmt.Errorf("function %q: scale.max must be >= 1 for deployer: keda, got %d", f.Name, maxScale) + } + if wantKafka && f.Run.Kafka == nil { + return fn.DeploymentResult{}, fmt.Errorf("function %q: scale.keda.triggers has a kafka trigger but run.kafka is not configured", f.Name) + } + if wantKafka && f.Run.Kafka != nil && f.Run.Kafka.SASL != nil && f.Run.Kafka.SASL.Mechanism != "" && kedaSASLType(f.Run.Kafka.SASL.Mechanism) == "" { + // kedaSASLType returns "" for anything it doesn't recognize, and + // buildScaledObject would set the trigger's "sasl" metadata to that + // empty string, producing an invalid KEDA trigger. + return fn.DeploymentResult{}, fmt.Errorf( + "function %q: run.kafka.sasl.mechanism %q is not supported, must be one of PLAIN, SCRAM-SHA-256, SCRAM-SHA-512", + f.Name, f.Run.Kafka.SASL.Mechanism) } k8sClientset, err := k8s.NewKubernetesClientset() @@ -112,12 +177,13 @@ func (d *Deployer) Deploy(ctx context.Context, f fn.Function) (fn.DeploymentResu return fn.DeploymentResult{}, fmt.Errorf("failed to create dynamic client: %w", err) } - // Resolved once per deploy and threaded down - interceptorNS, exposeRefusal := interceptorNamespace(ctx, k8sClientset) - - // DNS label checks before we create anything on cluster - if err := d.validateExposure(f, exposeRefusal); err != nil { - return fn.DeploymentResult{}, err + var interceptorNS string + var exposeRefusal error + if wantHTTP { + interceptorNS, exposeRefusal = interceptorNamespace(ctx, k8sClientset) + if err := d.validateExposure(f, exposeRefusal); err != nil { + return fn.DeploymentResult{}, err + } } // execute raw deployment deployer @@ -126,7 +192,6 @@ func (d *Deployer) Deploy(ctx context.Context, f fn.Function) (fn.DeploymentResu return fn.DeploymentResult{}, fmt.Errorf("failed to deploy function via raw deployer: %w", err) } - // create additional required keda resources namespace := deployResult.Namespace deployment, err := k8sClientset.AppsV1().Deployments(namespace).Get(ctx, f.Name, metav1.GetOptions{}) @@ -139,42 +204,139 @@ func (d *Deployer) Deploy(ctx context.Context, f fn.Function) (fn.DeploymentResu return fn.DeploymentResult{}, fmt.Errorf("failed to get service %s/%s: %v", namespace, f.Name, err) } - ref := deployer.NewExposureRef(f.Name, namespace, interceptorNS) - if err := ensureInterceptorBridgeService(ctx, k8sClientset, ref, deployment); err != nil { - return fn.DeploymentResult{}, fmt.Errorf("failed to ensure proxy service exists: %w", err) + // Delete stale scaler resources for whichever trigger type is NOT + // currently configured, before provisioning the type that is: creating + // a new scaler while an old one of the other kind still targets the + // same Deployment can trip KEDA's one-scaler-per-workload rule and fail + // the new scaler's readiness wait. Not fatal to Deploy, same as + // Remover.Remove's treatment of these: they're owned by the Deployment + // and get garbage-collected regardless. + if !wantHTTP { + // No HTTP trigger: a prior deploy's HTTPScaledObject and + // interceptor bridge Service, if any, are now orphaned. + if err := deleteHTTPScaledObject(ctx, namespace, f.Name); err != nil { + fmt.Fprintf(os.Stderr, "warning: %v\n", err) + } + if err := deleteInterceptorBridgeService(ctx, k8sClientset, namespace, f.Name); err != nil { + fmt.Fprintf(os.Stderr, "warning: %v\n", err) + } } - - labels, err := deployer.GenerateCommonLabels(f, d.decorator) - if err != nil { - return fn.DeploymentResult{}, fmt.Errorf("failed to generate common labels: %w", err) + if !wantKafka { + // The Kafka trigger was dropped (or never configured): remove any + // scaler resources a prior deploy left behind, so switching back to + // http-only doesn't leave a ScaledObject/TriggerAuthentication + // still acting on stale Kafka lag config. + if err := deleteScaledObject(ctx, dynClient, namespace, scaledObjectName(f.Name)); err != nil { + fmt.Fprintf(os.Stderr, "warning: %v\n", err) + } + if err := deleteTriggerAuth(ctx, dynClient, namespace, triggerAuthName(f.Name)); err != nil { + fmt.Fprintf(os.Stderr, "warning: %v\n", err) + } } - annotations := deployer.GenerateCommonAnnotations(f, d.decorator, false, KedaDeployerName) - minScale, maxScale := replicaBounds(f) - target := deployTarget{ - clientset: k8sClientset, - dynClient: dynClient, - ref: ref, - deployment: deployment, - appService: appService, - labels: labels, - annotations: annotations, - minScale: minScale, - maxScale: maxScale, - } + // HTTP trigger path: bridge Service + HTTPScaledObject var url string appliedExpose := "" - if d.exposer != nil && fn.ActiveExpose(f.Expose) { - if url, err = d.deployExposed(ctx, target); err != nil { - return fn.DeploymentResult{}, err + if wantHTTP { + ref := deployer.NewExposureRef(f.Name, namespace, interceptorNS) + if err := ensureInterceptorBridgeService(ctx, k8sClientset, ref, deployment); err != nil { + return fn.DeploymentResult{}, fmt.Errorf("failed to ensure proxy service exists: %w", err) + } + + labels, err := deployer.GenerateCommonLabels(f, d.decorator) + if err != nil { + return fn.DeploymentResult{}, fmt.Errorf("failed to generate common labels: %w", err) + } + annotations := deployer.GenerateCommonAnnotations(f, d.decorator, false, KedaDeployerName) + + target := deployTarget{ + clientset: k8sClientset, + dynClient: dynClient, + ref: ref, + deployment: deployment, + appService: appService, + labels: labels, + annotations: annotations, + minScale: minScale, + maxScale: maxScale, + scale: f.Scale, + } + + if d.exposer != nil && fn.ActiveExpose(f.Expose) { + if url, err = d.deployExposed(ctx, target); err != nil { + return fn.DeploymentResult{}, err + } + appliedExpose = f.Expose + } else { + if url, err = d.deployClusterLocal(ctx, target); err != nil { + return fn.DeploymentResult{}, err + } } - appliedExpose = f.Expose } else { - if url, err = d.deployClusterLocal(ctx, target); err != nil { + // No HTTP trigger — URL is the app service. The Service listens on + // port 80 (routing to the container's DefaultHTTPPort via + // targetPort), so the URL, like elsewhere in the codebase (e.g. + // pkg/k8s/describer.go), has no explicit port. + url = fmt.Sprintf("http://%s.%s.svc", f.Name, namespace) + + // A prior deploy may have exposed this function over HTTP. Nothing + // reconciles that exposure once the HTTP trigger is gone, so clear + // it the same way deployClusterLocal does -- otherwise the old + // Route and the Service's exposure annotations stay active, + // pointing at a function that no longer has anything serving HTTP. + target := deployTarget{ + clientset: k8sClientset, + dynClient: dynClient, + ref: deployer.NewExposureRef(f.Name, namespace, ""), + } + if err := d.clearExposure(ctx, target, appService.Annotations[k8s.RouteNamespaceAnnotation]); err != nil { return fn.DeploymentResult{}, err } } + // Kafka trigger path: TriggerAuthentication + ScaledObject + if wantKafka && f.Run.Kafka != nil { + if needsTriggerAuth(f.Run.Kafka) { + ta, err := buildTriggerAuth(f, deployment, namespace) + if err != nil { + // A TLS path was explicitly configured but doesn't resolve + // to any configured volume. Failing here avoids a + // ScaledObject whose authenticationRef points at a + // TriggerAuthentication missing the credential it needs. + return fn.DeploymentResult{}, fmt.Errorf("function %q: %w", f.Name, err) + } + if ta == nil { + // needsTriggerAuth said SASL/TLS credentials need a + // TriggerAuthentication, but buildTriggerAuth found nothing + // to resolve at all. Failing here avoids a ScaledObject + // whose authenticationRef points at a TriggerAuthentication + // that was never created. + return fn.DeploymentResult{}, fmt.Errorf( + "function %q: run.kafka SASL/TLS credentials are configured but could not be resolved to a Secret or environment variable; "+ + "check that run.kafka.sasl/tls paths match a configured volume", f.Name) + } + if err := ensureTriggerAuth(ctx, dynClient, ta); err != nil { + return fn.DeploymentResult{}, fmt.Errorf("failed to ensure TriggerAuthentication: %w", err) + } + } else { + // SASL/TLS credentials were removed from run.kafka while the + // kafka trigger stayed: a prior deploy may have left a + // TriggerAuthentication behind that nothing references anymore. + // Not fatal, same treatment as the no-kafka-at-all cleanup above. + if err := deleteTriggerAuth(ctx, dynClient, namespace, triggerAuthName(f.Name)); err != nil { + fmt.Fprintf(os.Stderr, "warning: %v\n", err) + } + } + + kt := kafkaTrigger(triggers) + so := buildScaledObject(f, kt, deployment, namespace, minScale, maxScale) + if so != nil { + if err := ensureScaledObject(ctx, dynClient, so); err != nil { + return fn.DeploymentResult{}, fmt.Errorf("failed to ensure ScaledObject: %w", err) + } + } + } + return fn.DeploymentResult{ Status: deployResult.Status, URL: url, @@ -225,6 +387,7 @@ type deployTarget struct { annotations map[string]string minScale int32 maxScale int32 + scale *fn.ScaleOptions } // bridgeHosts are the cluster-local names the HSO registers for f: requests @@ -252,7 +415,7 @@ func (d *Deployer) deployExposed(ctx context.Context, t deployTarget) (string, e } hosts := append(bridgeHosts(t.ref), exposedHost) - if err := ensureHTTPScaledObject(ctx, t, hosts); err != nil { + if err := ensureHTTPScaledObject(ctx, t, hosts, t.scale); err != nil { return "", fmt.Errorf("failed to ensure http scaled object exists: %w", err) } @@ -276,7 +439,7 @@ func (d *Deployer) deployExposed(ctx context.Context, t deployTarget) (string, e // effectively unexposed. func (d *Deployer) deployClusterLocal(ctx context.Context, t deployTarget) (string, error) { hosts := bridgeHosts(t.ref) - if err := ensureHTTPScaledObject(ctx, t, hosts); err != nil { + if err := ensureHTTPScaledObject(ctx, t, hosts, t.scale); err != nil { return "", fmt.Errorf("failed to ensure http scaled object exists: %w", err) } @@ -323,24 +486,39 @@ const ( // deployers. func replicaBounds(f fn.Function) (min, max int32) { min, max = defaultMinReplicas, defaultMaxReplicas - if scale := f.Deploy.Options.Scale; scale != nil { - if scale.Min != nil { - min = int32(*scale.Min) + if f.Scale != nil { + if f.Scale.Min != nil { + min = int32(*f.Scale.Min) } - if scale.Max != nil { - max = int32(*scale.Max) + if f.Scale.Max != nil { + max = int32(*f.Scale.Max) } } return } -func httpScaledObject(t deployTarget, hosts []string) (*httpv1alpha1.HTTPScaledObject, error) { +func httpScaledObject(t deployTarget, hosts []string, scale *fn.ScaleOptions) (*httpv1alpha1.HTTPScaledObject, error) { deployment := t.deployment service := t.appService if len(service.Spec.Ports) == 0 { return nil, fmt.Errorf("service %s has no ports defined", service.Name) } + cooldown := int32(300) + targetValue := int64(100) + if scale != nil && scale.KEDA != nil { + if scale.KEDA.CooldownPeriod != nil { + cooldown = *scale.KEDA.CooldownPeriod + } + for _, trig := range scale.KEDA.Triggers { + if trig.Type == "http" && trig.TargetValue != nil { + targetValue = *trig.TargetValue + break + } + } + } + + controllerTrue := true return &httpv1alpha1.HTTPScaledObject{ ObjectMeta: metav1.ObjectMeta{ Name: t.ref.FunctionName, @@ -353,7 +531,7 @@ func httpScaledObject(t deployTarget, hosts []string) (*httpv1alpha1.HTTPScaledO Kind: "Deployment", Name: deployment.Name, UID: deployment.UID, - Controller: new(true), + Controller: &controllerTrue, }, }, }, @@ -367,13 +545,13 @@ func httpScaledObject(t deployTarget, hosts []string) (*httpv1alpha1.HTTPScaledO Port: service.Spec.Ports[0].Port, }, Replicas: &httpv1alpha1.ReplicaStruct{ - Min: new(t.minScale), - Max: new(t.maxScale), + Min: &t.minScale, + Max: &t.maxScale, }, - CooldownPeriod: new(int32(300)), + CooldownPeriod: &cooldown, ScalingMetric: &httpv1alpha1.ScalingMetricSpec{ Rate: &httpv1alpha1.RateMetricSpec{ - TargetValue: 100, + TargetValue: int(targetValue), Window: metav1.Duration{ Duration: time.Minute, }, @@ -391,6 +569,7 @@ func interceptorBridgeServiceName(name string) string { } func interceptorBridgeService(ref deployer.ExposureRef, deployment *v1.Deployment) *corev1.Service { + controllerTrue := true return &corev1.Service{ ObjectMeta: metav1.ObjectMeta{ Name: interceptorBridgeServiceName(ref.FunctionName), @@ -401,7 +580,7 @@ func interceptorBridgeService(ref deployer.ExposureRef, deployment *v1.Deploymen Kind: "Deployment", Name: deployment.Name, UID: deployment.UID, - Controller: new(true), + Controller: &controllerTrue, }, }, }, @@ -450,8 +629,8 @@ func ensureInterceptorBridgeService(ctx context.Context, return nil } -func ensureHTTPScaledObject(ctx context.Context, t deployTarget, hosts []string) error { - expected, err := httpScaledObject(t, hosts) +func ensureHTTPScaledObject(ctx context.Context, t deployTarget, hosts []string, scale *fn.ScaleOptions) error { + expected, err := httpScaledObject(t, hosts, scale) if err != nil { return fmt.Errorf("failed to generate http scaled object: %w", err) } @@ -497,6 +676,30 @@ func ensureHTTPScaledObject(ctx context.Context, t deployTarget, hosts []string) return nil } +// deleteHTTPScaledObject removes an HTTPScaledObject if it exists. +func deleteHTTPScaledObject(ctx context.Context, ns, name string) error { + httpScaledObjectClientset, err := NewHTTPScaledObjectClientset() + if err != nil { + return fmt.Errorf("failed to create HTTPScaledObject clientset: %w", err) + } + err = httpScaledObjectClientset.HttpV1alpha1().HTTPScaledObjects(ns).Delete(ctx, name, metav1.DeleteOptions{}) + if err != nil && !k8serrors.IsNotFound(err) { + return fmt.Errorf("failed to delete HTTPScaledObject %s/%s: %w", ns, name, err) + } + return nil +} + +// deleteInterceptorBridgeService removes the interceptor bridge Service for +// functionName if it exists. +func deleteInterceptorBridgeService(ctx context.Context, clientset *kubernetes.Clientset, ns, functionName string) error { + name := interceptorBridgeServiceName(functionName) + err := clientset.CoreV1().Services(ns).Delete(ctx, name, metav1.DeleteOptions{}) + if err != nil && !k8serrors.IsNotFound(err) { + return fmt.Errorf("failed to delete interceptor bridge Service %s/%s: %w", ns, name, err) + } + return nil +} + func UsesKedaDeployer(annotations map[string]string) bool { deployer, ok := annotations[deployer.DeployerNameAnnotation] diff --git a/pkg/keda/deployer_unit_test.go b/pkg/keda/deployer_unit_test.go index 54e6df6398..485e8e3f10 100644 --- a/pkg/keda/deployer_unit_test.go +++ b/pkg/keda/deployer_unit_test.go @@ -251,3 +251,18 @@ func Test_validateExposure(t *testing.T) { t.Errorf("expected a nil exposer to skip exposure validation, got: %v", err) } } + +// TestReplicaBounds_MaxBelowOne documents the precondition Deploy's +// maxScale < 1 guard depends on: ValidateScale rejects scale.max: 0 for +// deployer: keda, but Deploy is reachable without going through +// Function.Validate first (library callers, tests), so replicaBounds can +// still hand back a maxScale that would produce an invalid (< 1) HPA +// maxReplicas if Deploy didn't check it itself. +func TestReplicaBounds_MaxBelowOne(t *testing.T) { + zero := int64(0) + f := fn.Function{Scale: &fn.ScaleOptions{Max: &zero}} + _, max := replicaBounds(f) + if max >= 1 { + t.Fatalf("expected replicaBounds to pass scale.max: 0 through unchecked, got max=%d", max) + } +} diff --git a/pkg/keda/describer.go b/pkg/keda/describer.go index b1fb36e132..f526b80103 100644 --- a/pkg/keda/describer.go +++ b/pkg/keda/describer.go @@ -7,6 +7,7 @@ import ( "strings" "github.com/kedacore/http-add-on/operator/apis/http/v1alpha1" + appsv1 "k8s.io/api/apps/v1" v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" @@ -72,29 +73,14 @@ func (d *Describer) Describe(ctx context.Context, name, namespace string) (fn.In } httpScaledObject, err := httpScaledObjectClientset.HttpV1alpha1().HTTPScaledObjects(namespace).Get(ctx, name, metav1.GetOptions{}) + hasHTTPTrigger := true if err != nil { - return fn.Instance{}, fmt.Errorf("unable to get HTTPScaledObject: %w", err) - } - - ready := v1.ConditionUnknown - if meta.IsStatusConditionTrue(httpScaledObject.Status.Conditions, v1alpha1.ConditionTypeReady) { - ready = v1.ConditionTrue - } else if meta.IsStatusConditionFalse(httpScaledObject.Status.Conditions, v1alpha1.ConditionTypeReady) { - ready = v1.ConditionFalse - } - - if len(httpScaledObject.Spec.Hosts) == 0 { - return fn.Instance{}, fmt.Errorf("HTTPScaledObject %q does not have any hosts", name) - } - - // Deploy recorded the externally exposed hostname on the function's own - // Service, so no second lookup is needed to tell the exposed host apart - // from the bridge hosts it sits beside in Spec.Hosts. - hostname := service.Annotations[k8s.RouteHostnameAnnotation] - primaryRouteURL, routes := functionURLs(httpScaledObject.Spec.Hosts, hostname) - expose := "" - if hostname != "" { - expose = fn.ExposeRoute + if !errors.IsNotFound(err) { + return fn.Instance{}, fmt.Errorf("unable to get HTTPScaledObject: %w", err) + } + // A Kafka-only (or otherwise no-http-trigger) function never gets an + // HTTPScaledObject at all -- that's expected, not a failure. + hasHTTPTrigger = false } deploymentClient := clientset.AppsV1().Deployments(namespace) @@ -103,6 +89,47 @@ func (d *Describer) Describe(ctx context.Context, name, namespace string) (fn.In return fn.Instance{}, fmt.Errorf("unable to get deployment %q: %v", name, err) } + var ready v1.ConditionStatus + var primaryRouteURL string + var routes []string + expose := "" + + if hasHTTPTrigger { + ready = v1.ConditionUnknown + if meta.IsStatusConditionTrue(httpScaledObject.Status.Conditions, v1alpha1.ConditionTypeReady) { + ready = v1.ConditionTrue + } else if meta.IsStatusConditionFalse(httpScaledObject.Status.Conditions, v1alpha1.ConditionTypeReady) { + ready = v1.ConditionFalse + } + + if len(httpScaledObject.Spec.Hosts) == 0 { + return fn.Instance{}, fmt.Errorf("HTTPScaledObject %q does not have any hosts", name) + } + + // Deploy recorded the externally exposed hostname on the function's + // own Service, so no second lookup is needed to tell the exposed + // host apart from the bridge hosts it sits beside in Spec.Hosts. + hostname := service.Annotations[k8s.RouteHostnameAnnotation] + primaryRouteURL, routes = functionURLs(httpScaledObject.Spec.Hosts, hostname) + if hostname != "" { + expose = fn.ExposeRoute + } + } else { + // No HTTP trigger: there's no interceptor/bridge host list to + // report, so fall back to the Deployment's own readiness condition + // and the cluster-local Service URL -- the same baseline the raw + // k8s describer reports for an unexposed function. + ready = v1.ConditionUnknown + for _, cond := range deployment.Status.Conditions { + if cond.Type == appsv1.DeploymentAvailable { + ready = cond.Status + break + } + } + primaryRouteURL = fmt.Sprintf("http://%s.%s.svc", name, namespace) + routes = []string{primaryRouteURL} + } + // get image image := "" for _, container := range deployment.Spec.Template.Spec.Containers { diff --git a/pkg/keda/kafka_scaling.go b/pkg/keda/kafka_scaling.go new file mode 100644 index 0000000000..1aad3bea97 --- /dev/null +++ b/pkg/keda/kafka_scaling.go @@ -0,0 +1,481 @@ +package keda + +import ( + "context" + "fmt" + "path/filepath" + "strings" + + v1 "k8s.io/api/apps/v1" + k8serrors "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/client-go/dynamic" + fn "knative.dev/func/pkg/functions" +) + +var ( + scaledObjectGVR = schema.GroupVersionResource{ + Group: "keda.sh", + Version: "v1alpha1", + Resource: "scaledobjects", + } + triggerAuthGVR = schema.GroupVersionResource{ + Group: "keda.sh", + Version: "v1alpha1", + Resource: "triggerauthentications", + } +) + +func scaledObjectName(funcName string) string { + return funcName + "-kafka" +} + +func triggerAuthName(funcName string) string { + return funcName + "-kafka-auth" +} + +// triggers returns the function's explicitly configured KEDA triggers, or a +// plain http trigger when none are configured at all. This only matters for +// callers of Deploy that bypass fn.Function.Validate (which requires +// deployer: keda to declare triggers explicitly) -- e.g. tests and other +// direct API consumers. It never infers a kafka trigger: that decision is +// never made silently, on any path. +func triggers(f fn.Function) []fn.KEDATrigger { + if f.Scale != nil && f.Scale.KEDA != nil { + return f.Scale.KEDA.Triggers + } + return []fn.KEDATrigger{{Type: "http"}} +} + +func hasHTTPTrigger(triggers []fn.KEDATrigger) bool { + for _, t := range triggers { + if t.Type == "http" { + return true + } + } + return false +} + +func hasKafkaTrigger(triggers []fn.KEDATrigger) bool { + for _, t := range triggers { + if t.Type == "kafka" { + return true + } + } + return false +} + +func kafkaTrigger(triggers []fn.KEDATrigger) fn.KEDATrigger { + for _, t := range triggers { + if t.Type == "kafka" { + return t + } + } + return fn.KEDATrigger{} +} + +// needsTriggerAuth returns true when the Kafka config uses SASL or TLS with +// secrets that must be referenced via a TriggerAuthentication. +func needsTriggerAuth(kafka *fn.KafkaConfig) bool { + if kafka == nil { + return false + } + if kafka.SASL != nil && kafka.SASL.Password != "" { + return true + } + if kafka.TLS != nil && (kafka.TLS.CACert != "" || kafka.TLS.ClientCert != "" || kafka.TLS.ClientKey != "") { + return true + } + return false +} + +// parseSecretRef extracts the secret name and key from a {{ secret:name:key }} +// reference. Returns empty strings if the value is not a secret reference. +// Uses fn.TemplateRefPattern -- the same pattern fn.Function.Validate and +// pkg/k8s/deployer.go's env wiring match against -- so a malformed +// {{ ... }} value is treated the same way everywhere instead of being +// accepted here via ad-hoc trim/split and looked up as a mismatched secret. +func parseSecretRef(value string) (secretName, secretKey string) { + matches := fn.TemplateRefPattern.FindStringSubmatch(value) + if matches == nil || matches[1] != "secret" { + return + } + return matches[2], matches[3] +} + +// findSecretForPath finds the volume secret name that backs a given file path. +// It matches by checking which volume's mount path is a parent of the cert path. +func findSecretForPath(certPath string, volumes []fn.Volume) (secretName, key string) { + for _, v := range volumes { + if v.Secret == nil || v.Path == nil { + continue + } + mountPath := filepath.Clean(*v.Path) + cp := filepath.Clean(certPath) + if cp == mountPath || strings.HasPrefix(cp, mountPath+string(filepath.Separator)) { + rel, err := filepath.Rel(mountPath, cp) + if err != nil { + continue + } + // func mounts a Secret volume at a single directory level, so its + // data keys are plain filenames: certPath == mountPath (rel == ".") + // names the directory, not a file in it, and a rel containing a + // separator names a file nested more than one level deep. Neither + // is a valid Secret data key. + if rel == "." || strings.ContainsRune(rel, filepath.Separator) { + continue + } + return *v.Secret, rel + } + } + return +} + +// buildTriggerAuth creates the unstructured TriggerAuthentication for Kafka +// SASL/TLS. Returns a non-nil error when a TLS path is explicitly +// configured but doesn't resolve to any configured volume -- distinct from +// a field that was never set at all, which is silently skipped. +func buildTriggerAuth(f fn.Function, deployment *v1.Deployment, namespace string) (*unstructured.Unstructured, error) { + kafka := f.Run.Kafka + if kafka == nil { + return nil, nil + } + + var secretRefs []interface{} + var envRefs []interface{} + + if kafka.SASL != nil && kafka.SASL.Password != "" { + secretName, secretKey := parseSecretRef(kafka.SASL.Password) + if secretName != "" { + secretRefs = append(secretRefs, map[string]interface{}{ + "parameter": "password", + "name": secretName, + "key": secretKey, + }) + } else { + // Plaintext value: ends up as a literal KAFKA_SASL_PASSWORD env var + // on the function's container. A {{ configMap:... }} reference + // ends up as a KAFKA_SASL_PASSWORD env var too, but backed by a + // ConfigMapKeyRef instead of a literal value (see + // appendKafkaEnvValue in pkg/k8s/deployer.go). Either way the env + // var name is the same, so point KEDA at that name instead of a + // secretTargetRef. Plaintext is allowed here, at least for + // debugging purposes -- func doesn't force SASL credentials + // through Secrets. + envRefs = append(envRefs, map[string]interface{}{ + "parameter": "password", + "name": "KAFKA_SASL_PASSWORD", + "containerName": deployment.Spec.Template.Spec.Containers[0].Name, + }) + } + + if kafka.SASL.User != "" { + userName, userKey := parseSecretRef(kafka.SASL.User) + if userName != "" { + secretRefs = append(secretRefs, map[string]interface{}{ + "parameter": "username", + "name": userName, + "key": userKey, + }) + } else { + envRefs = append(envRefs, map[string]interface{}{ + "parameter": "username", + "name": "KAFKA_SASL_USER", + "containerName": deployment.Spec.Template.Spec.Containers[0].Name, + }) + } + } + } + + if kafka.TLS != nil && kafka.TLS.CACert != "" { + caSecretName, caKey := findSecretForPath(kafka.TLS.CACert, f.Run.Volumes) + if caSecretName == "" { + return nil, fmt.Errorf("run.kafka.tls.caCert %q does not match any configured volume", kafka.TLS.CACert) + } + secretRefs = append(secretRefs, map[string]interface{}{ + "parameter": "ca", + "name": caSecretName, + "key": caKey, + }) + } + + if kafka.TLS != nil && kafka.TLS.ClientCert != "" { + certSecretName, certKey := findSecretForPath(kafka.TLS.ClientCert, f.Run.Volumes) + if certSecretName == "" { + return nil, fmt.Errorf("run.kafka.tls.clientCert %q does not match any configured volume", kafka.TLS.ClientCert) + } + secretRefs = append(secretRefs, map[string]interface{}{ + "parameter": "cert", + "name": certSecretName, + "key": certKey, + }) + } + + if kafka.TLS != nil && kafka.TLS.ClientKey != "" { + keySecretName, keyKey := findSecretForPath(kafka.TLS.ClientKey, f.Run.Volumes) + if keySecretName == "" { + return nil, fmt.Errorf("run.kafka.tls.clientKey %q does not match any configured volume", kafka.TLS.ClientKey) + } + secretRefs = append(secretRefs, map[string]interface{}{ + "parameter": "key", + "name": keySecretName, + "key": keyKey, + }) + } + + if len(secretRefs) == 0 && len(envRefs) == 0 { + return nil, nil + } + + spec := map[string]interface{}{} + if len(secretRefs) > 0 { + spec["secretTargetRef"] = secretRefs + } + if len(envRefs) > 0 { + spec["env"] = envRefs + } + + ta := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "keda.sh/v1alpha1", + "kind": "TriggerAuthentication", + "metadata": map[string]interface{}{ + "name": triggerAuthName(f.Name), + "namespace": namespace, + "ownerReferences": []interface{}{ + map[string]interface{}{ + // blockOwnerDeletion deliberately omitted: it only + // takes effect for foreground cascading deletion + // (unused here), but makes the + // OwnerReferencesPermissionEnforcement admission + // plugin require update on the owner's finalizers + // subresource -- a grant the deploying/pipeline + // service account doesn't hold by default, so the + // create is rejected outright on OpenShift, which + // enables that plugin (KinD doesn't). Same reasoning + // as the Service owner reference in + // pkg/k8s/deployer.go. + "apiVersion": "apps/v1", + "kind": "Deployment", + "name": deployment.Name, + "uid": string(deployment.UID), + "controller": true, + }, + }, + }, + "spec": spec, + }, + } + + return ta, nil +} + +// kedaSASLType maps func.yaml SASL mechanism names to KEDA trigger metadata values. +func kedaSASLType(mechanism string) string { + switch mechanism { + case "SCRAM-SHA-256": + return "scram_sha256" + case "SCRAM-SHA-512": + return "scram_sha512" + case "PLAIN": + return "plain" + default: + return "" + } +} + +// buildScaledObject creates the unstructured ScaledObject for Kafka consumer-lag scaling. +func buildScaledObject(f fn.Function, trigger fn.KEDATrigger, deployment *v1.Deployment, namespace string, minScale, maxScale int32) *unstructured.Unstructured { + kafka := f.Run.Kafka + if kafka == nil { + return nil + } + + lagThreshold := int64(10) + if trigger.LagThreshold != nil { + lagThreshold = *trigger.LagThreshold + } + + triggerMeta := map[string]interface{}{ + "bootstrapServers": kafka.Brokers, + "consumerGroup": kafka.ConsumerGroup, + "topic": kafka.Topic, + "lagThreshold": fmt.Sprintf("%d", lagThreshold), + } + + if trigger.ActivationLagThreshold != nil { + triggerMeta["activationLagThreshold"] = fmt.Sprintf("%d", *trigger.ActivationLagThreshold) + } + + if kafka.SecurityProtocol == "SSL" || kafka.SecurityProtocol == "SASL_SSL" { + // Enable KEDA's TLS handshake based on securityProtocol, not the + // presence of run.kafka.tls: a valid config can set SSL/SASL_SSL and + // rely on the system's CA trust store, with no explicit tls block at + // all. Gating on kafka.TLS != nil would leave KEDA attempting a + // plaintext connection for that config. + triggerMeta["tls"] = "enable" + + if kafka.TLS != nil && kafka.TLS.SkipVerify { + // SkipVerify is propagated to the function's own container (see + // pkg/functions/runner.go), but KEDA's scaler connects to the + // broker independently -- without this, a self-signed broker + // lets the app's consumer connect while the scaler's own TLS + // handshake fails and the ScaledObject never scales, silently. + triggerMeta["unsafeSsl"] = "true" + } + } + + if kafka.SASL != nil && kafka.SASL.Mechanism != "" { + triggerMeta["sasl"] = kedaSASLType(kafka.SASL.Mechanism) + } + + triggerSpec := map[string]interface{}{ + "type": "kafka", + "metadata": triggerMeta, + } + + if needsTriggerAuth(kafka) { + triggerSpec["authenticationRef"] = map[string]interface{}{ + "name": triggerAuthName(f.Name), + } + } + + so := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "keda.sh/v1alpha1", + "kind": "ScaledObject", + "metadata": map[string]interface{}{ + "name": scaledObjectName(f.Name), + "namespace": namespace, + "ownerReferences": []interface{}{ + map[string]interface{}{ + // blockOwnerDeletion deliberately omitted: it only + // takes effect for foreground cascading deletion + // (unused here), but makes the + // OwnerReferencesPermissionEnforcement admission + // plugin require update on the owner's finalizers + // subresource -- a grant the deploying/pipeline + // service account doesn't hold by default, so the + // create is rejected outright on OpenShift, which + // enables that plugin (KinD doesn't). Same reasoning + // as the Service owner reference in + // pkg/k8s/deployer.go. + "apiVersion": "apps/v1", + "kind": "Deployment", + "name": deployment.Name, + "uid": string(deployment.UID), + "controller": true, + }, + }, + }, + "spec": buildScaledObjectSpec(f, deployment.Name, minScale, maxScale, triggerSpec), + }, + } + + return so +} + +func buildScaledObjectSpec(f fn.Function, deploymentName string, minScale, maxScale int32, triggerSpec map[string]interface{}) map[string]interface{} { + cooldown := int64(300) + polling := int64(30) + if f.Scale != nil && f.Scale.KEDA != nil { + if f.Scale.KEDA.CooldownPeriod != nil { + cooldown = int64(*f.Scale.KEDA.CooldownPeriod) + } + if f.Scale.KEDA.PollingInterval != nil { + polling = int64(*f.Scale.KEDA.PollingInterval) + } + } + + return map[string]interface{}{ + "scaleTargetRef": map[string]interface{}{ + "kind": "Deployment", + "name": deploymentName, + }, + "minReplicaCount": int64(minScale), + "maxReplicaCount": int64(maxScale), + "cooldownPeriod": cooldown, + "pollingInterval": polling, + "triggers": []interface{}{ + triggerSpec, + }, + } +} + +// ensureScaledObject creates or updates a KEDA ScaledObject for Kafka scaling. +func ensureScaledObject(ctx context.Context, dynClient dynamic.Interface, so *unstructured.Unstructured) error { + ns := so.GetNamespace() + name := so.GetName() + client := dynClient.Resource(scaledObjectGVR).Namespace(ns) + + existing, err := client.Get(ctx, name, metav1.GetOptions{}) + if err != nil { + if k8serrors.IsNotFound(err) { + if _, err := client.Create(ctx, so, metav1.CreateOptions{}); err != nil { + return fmt.Errorf("failed to create ScaledObject %s/%s: %w", ns, name, err) + } + return nil + } + return fmt.Errorf("failed to get ScaledObject %s/%s: %w", ns, name, err) + } + + so.SetResourceVersion(existing.GetResourceVersion()) + // KEDA attaches its own finalizer to a ScaledObject it's managing; this + // Update is a full replace, so without carrying it over, a redeploy + // would silently strip it and let a later delete bypass KEDA's cleanup + // ordering (see the note on this in kafka_scaling_int_test.go). + so.SetFinalizers(existing.GetFinalizers()) + if _, err := client.Update(ctx, so, metav1.UpdateOptions{}); err != nil { + return fmt.Errorf("failed to update ScaledObject %s/%s: %w", ns, name, err) + } + return nil +} + +// ensureTriggerAuth creates or updates a KEDA TriggerAuthentication. +func ensureTriggerAuth(ctx context.Context, dynClient dynamic.Interface, ta *unstructured.Unstructured) error { + ns := ta.GetNamespace() + name := ta.GetName() + client := dynClient.Resource(triggerAuthGVR).Namespace(ns) + + existing, err := client.Get(ctx, name, metav1.GetOptions{}) + if err != nil { + if k8serrors.IsNotFound(err) { + if _, err := client.Create(ctx, ta, metav1.CreateOptions{}); err != nil { + return fmt.Errorf("failed to create TriggerAuthentication %s/%s: %w", ns, name, err) + } + return nil + } + return fmt.Errorf("failed to get TriggerAuthentication %s/%s: %w", ns, name, err) + } + + ta.SetResourceVersion(existing.GetResourceVersion()) + // Same reasoning as ensureScaledObject: preserve KEDA's own finalizer + // across this full-replace Update instead of silently dropping it. + ta.SetFinalizers(existing.GetFinalizers()) + if _, err := client.Update(ctx, ta, metav1.UpdateOptions{}); err != nil { + return fmt.Errorf("failed to update TriggerAuthentication %s/%s: %w", ns, name, err) + } + return nil +} + +// deleteScaledObject removes a ScaledObject if it exists. +func deleteScaledObject(ctx context.Context, dynClient dynamic.Interface, ns, name string) error { + client := dynClient.Resource(scaledObjectGVR).Namespace(ns) + err := client.Delete(ctx, name, metav1.DeleteOptions{}) + if err != nil && !k8serrors.IsNotFound(err) { + return fmt.Errorf("failed to delete ScaledObject %s/%s: %w", ns, name, err) + } + return nil +} + +// deleteTriggerAuth removes a TriggerAuthentication if it exists. +func deleteTriggerAuth(ctx context.Context, dynClient dynamic.Interface, ns, name string) error { + client := dynClient.Resource(triggerAuthGVR).Namespace(ns) + err := client.Delete(ctx, name, metav1.DeleteOptions{}) + if err != nil && !k8serrors.IsNotFound(err) { + return fmt.Errorf("failed to delete TriggerAuthentication %s/%s: %w", ns, name, err) + } + return nil +} diff --git a/pkg/keda/kafka_scaling_int_test.go b/pkg/keda/kafka_scaling_int_test.go new file mode 100644 index 0000000000..de91f14f17 --- /dev/null +++ b/pkg/keda/kafka_scaling_int_test.go @@ -0,0 +1,226 @@ +//go:build integration + +package keda_test + +import ( + "context" + "testing" + "time" + + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/rand" + "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/client-go/kubernetes" + + fn "knative.dev/func/pkg/functions" + "knative.dev/func/pkg/k8s" + "knative.dev/func/pkg/keda" + testingk8s "knative.dev/func/pkg/testing/k8s" +) + +var ( + scaledObjectGVR = schema.GroupVersionResource{ + Group: "keda.sh", + Version: "v1alpha1", + Resource: "scaledobjects", + } + triggerAuthGVR = schema.GroupVersionResource{ + Group: "keda.sh", + Version: "v1alpha1", + Resource: "triggerauthentications", + } +) + +// TestInt_KafkaScaling deploys a function with a Kafka-only KEDA trigger +// (no HTTP trigger, since a Deployment can only be owned by one ScaledObject +// and the http trigger's HTTPScaledObject creates its own -- see #4043) and +// verifies that the deployer creates a ScaledObject and TriggerAuthentication +// with the expected spec, and that both are cleaned up on removal. +// +// This does not require a reachable Kafka broker: KEDA admits and reconciles +// the ScaledObject as long as the trigger metadata is well-formed, regardless +// of whether it can actually reach the broker to read lag. +func TestInt_KafkaScaling(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), time.Minute*10) + t.Cleanup(cancel) + + name := "func-int-keda-kafka-" + rand.String(5) + ns := testingk8s.Namespace(t, ctx) + + cliSet, err := k8s.NewKubernetesClientset() + if err != nil { + t.Fatal(err) + } + + caSecretName := name + "-ca" + createSecretForTest(t, ctx, cliSet, ns, caSecretName, map[string][]byte{"ca.crt": []byte("placeholder-ca-cert")}) + + userSecretName := name + "-user" + createSecretForTest(t, ctx, cliSet, ns, userSecretName, map[string][]byte{"password": []byte("placeholder-password")}) + + minScale := int64(0) + maxScale := int64(10) + lagThreshold := int64(7) + + function := fn.Function{ + SpecVersion: "SNAPSHOT", + Root: "/non/existent", + Name: name, + Runtime: "blub", + Template: "cloudevents", + Created: time.Now(), + Expose: "none", + Deploy: fn.DeploySpec{ + // pinned prebuilt image: this test exercises the deployer's + // Kafka-scaling object creation, not the build/image flow + Image: "quay.io/mvasek/func-test-service@sha256:2eca4de00d7569c8791634bdbb0c4d5ec8fb061b001549314591e839dabd5269", + Namespace: ns, + }, + Scale: &fn.ScaleOptions{ + Min: &minScale, + Max: &maxScale, + KEDA: &fn.KEDAScaleOptions{ + Triggers: []fn.KEDATrigger{ + {Type: "kafka", LagThreshold: &lagThreshold}, + }, + }, + }, + Run: fn.RunSpec{ + Kafka: &fn.KafkaConfig{ + Brokers: "placeholder-kafka-bootstrap." + ns + ".svc:9093", + Topic: "test-topic", + ConsumerGroup: name + "-group", + SecurityProtocol: "SASL_SSL", + TLS: &fn.KafkaTLS{ + CACert: "/etc/kafka/ca/ca.crt", + }, + SASL: &fn.KafkaSASL{ + Mechanism: "SCRAM-SHA-512", + User: "test-kafka-user", + Password: "{{ secret:" + userSecretName + ":password }}", + }, + }, + Volumes: []fn.Volume{ + {Secret: ptrTo(caSecretName), Path: ptrTo("/etc/kafka/ca")}, + }, + }, + } + + deployer := keda.NewDeployer(keda.WithDeployerVerbose(false)) + remover := keda.NewRemover(false) + + _, err = deployer.Deploy(ctx, function) + if err != nil { + t.Fatalf("deploy failed: %v", err) + } + t.Cleanup(func() { + if err := remover.Remove(context.Background(), name, ns); err != nil { + t.Logf("error removing function: %v", err) + } + }) + + dynClient, err := k8s.NewDynamicClient() + if err != nil { + t.Fatal(err) + } + + so, err := dynClient.Resource(scaledObjectGVR).Namespace(ns).Get(ctx, name+"-kafka", metav1.GetOptions{}) + if err != nil { + t.Fatalf("expected ScaledObject to exist: %v", err) + } + + spec, _ := so.Object["spec"].(map[string]interface{}) + if spec["minReplicaCount"] != int64(0) { + t.Errorf("minReplicaCount = %v, want 0", spec["minReplicaCount"]) + } + if spec["maxReplicaCount"] != int64(10) { + t.Errorf("maxReplicaCount = %v, want 10", spec["maxReplicaCount"]) + } + triggersList, _ := spec["triggers"].([]interface{}) + if len(triggersList) != 1 { + t.Fatalf("expected 1 trigger, got %d", len(triggersList)) + } + trigger, _ := triggersList[0].(map[string]interface{}) + if trigger["type"] != "kafka" { + t.Errorf("trigger type = %v, want kafka", trigger["type"]) + } + meta, _ := trigger["metadata"].(map[string]interface{}) + if meta["lagThreshold"] != "7" { + t.Errorf("lagThreshold = %v, want 7", meta["lagThreshold"]) + } + if meta["consumerGroup"] != name+"-group" { + t.Errorf("consumerGroup = %v, want %s", meta["consumerGroup"], name+"-group") + } + if meta["sasl"] != "scram_sha512" { + t.Errorf("sasl = %v, want scram_sha512", meta["sasl"]) + } + if meta["tls"] != "enable" { + t.Errorf("tls = %v, want enable", meta["tls"]) + } + authRef, _ := trigger["authenticationRef"].(map[string]interface{}) + if authRef["name"] != name+"-kafka-auth" { + t.Errorf("authenticationRef.name = %v, want %s", authRef["name"], name+"-kafka-auth") + } + + ownerRefs, _ := so.Object["metadata"].(map[string]interface{})["ownerReferences"].([]interface{}) + if len(ownerRefs) != 1 { + t.Fatalf("expected 1 owner reference, got %d", len(ownerRefs)) + } + owner, _ := ownerRefs[0].(map[string]interface{}) + if owner["name"] != name || owner["kind"] != "Deployment" { + t.Errorf("unexpected owner reference: %v", owner) + } + + ta, err := dynClient.Resource(triggerAuthGVR).Namespace(ns).Get(ctx, name+"-kafka-auth", metav1.GetOptions{}) + if err != nil { + t.Fatalf("expected TriggerAuthentication to exist: %v", err) + } + + taSpec, _ := ta.Object["spec"].(map[string]interface{}) + secretRefs, _ := taSpec["secretTargetRef"].([]interface{}) + if len(secretRefs) != 2 { + t.Fatalf("expected 2 secretTargetRef entries (password, ca), got %d: %v", len(secretRefs), secretRefs) + } + envRefs, _ := taSpec["env"].([]interface{}) + if len(envRefs) != 1 { + t.Fatalf("expected 1 env entry (username), got %d: %v", len(envRefs), envRefs) + } + envRef, _ := envRefs[0].(map[string]interface{}) + if envRef["parameter"] != "username" || envRef["name"] != "KAFKA_SASL_USER" { + t.Errorf("unexpected env ref: %v", envRef) + } + + // Removal: TriggerAuthentication/ScaledObject carry KEDA's own finalizer, + // so deletion is asynchronous even after remover.Remove returns. + if err := remover.Remove(ctx, name, ns); err != nil { + t.Fatalf("remove failed: %v", err) + } + + err = wait.PollUntilContextTimeout(ctx, time.Second, time.Minute, true, func(ctx context.Context) (bool, error) { + _, soErr := dynClient.Resource(scaledObjectGVR).Namespace(ns).Get(ctx, name+"-kafka", metav1.GetOptions{}) + _, taErr := dynClient.Resource(triggerAuthGVR).Namespace(ns).Get(ctx, name+"-kafka-auth", metav1.GetOptions{}) + return apierrors.IsNotFound(soErr) && apierrors.IsNotFound(taErr), nil + }) + if err != nil { + t.Fatalf("expected ScaledObject and TriggerAuthentication to be removed: %v", err) + } +} + +func createSecretForTest(t *testing.T, ctx context.Context, cliSet *kubernetes.Clientset, ns, name string, data map[string][]byte) { + t.Helper() + _, err := cliSet.CoreV1().Secrets(ns).Create(ctx, &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: name}, + Data: data, + Type: corev1.SecretTypeOpaque, + }, metav1.CreateOptions{}) + if err != nil { + t.Fatal(err) + } +} + +func ptrTo[T any](v T) *T { + return &v +} diff --git a/pkg/keda/kafka_scaling_test.go b/pkg/keda/kafka_scaling_test.go new file mode 100644 index 0000000000..b55fc248fc --- /dev/null +++ b/pkg/keda/kafka_scaling_test.go @@ -0,0 +1,864 @@ +package keda + +import ( + "testing" + + v1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + k8serrors "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" + "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 newScalingDynClient(objects ...runtime.Object) *dynamicfake.FakeDynamicClient { + return dynamicfake.NewSimpleDynamicClientWithCustomListKinds( + runtime.NewScheme(), + map[schema.GroupVersionResource]string{ + scaledObjectGVR: "ScaledObjectList", + triggerAuthGVR: "TriggerAuthenticationList", + }, + objects...) +} + +func unstructuredScaledObject(name, ns string) *unstructured.Unstructured { + return &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "keda.sh/v1alpha1", + "kind": "ScaledObject", + "metadata": map[string]interface{}{"name": name, "namespace": ns}, + }} +} + +func unstructuredTriggerAuth(name, ns string) *unstructured.Unstructured { + return &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "keda.sh/v1alpha1", + "kind": "TriggerAuthentication", + "metadata": map[string]interface{}{"name": name, "namespace": ns}, + }} +} + +func TestEnsureScaledObject_PreservesFinalizers(t *testing.T) { + ns := "fn-ns" + name := "f-kafka" + + existing := unstructuredScaledObject(name, ns) + existing.SetFinalizers([]string{"scaledobject.keda.sh/finalizer"}) + dynClient := newScalingDynClient(existing) + + // A freshly-built ScaledObject, as buildScaledObject would produce: no + // finalizers set at all. + updated := unstructuredScaledObject(name, ns) + if err := ensureScaledObject(t.Context(), dynClient, updated); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + got, err := dynClient.Resource(scaledObjectGVR).Namespace(ns).Get(t.Context(), name, metav1.GetOptions{}) + if err != nil { + t.Fatal(err) + } + if finalizers := got.GetFinalizers(); len(finalizers) != 1 || finalizers[0] != "scaledobject.keda.sh/finalizer" { + t.Errorf("expected KEDA's finalizer to survive the update, got %v", finalizers) + } +} + +func TestEnsureTriggerAuth_PreservesFinalizers(t *testing.T) { + ns := "fn-ns" + name := "f-kafka-auth" + + existing := unstructuredTriggerAuth(name, ns) + existing.SetFinalizers([]string{"triggerauthentication.keda.sh/finalizer"}) + dynClient := newScalingDynClient(existing) + + updated := unstructuredTriggerAuth(name, ns) + if err := ensureTriggerAuth(t.Context(), dynClient, updated); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + got, err := dynClient.Resource(triggerAuthGVR).Namespace(ns).Get(t.Context(), name, metav1.GetOptions{}) + if err != nil { + t.Fatal(err) + } + if finalizers := got.GetFinalizers(); len(finalizers) != 1 || finalizers[0] != "triggerauthentication.keda.sh/finalizer" { + t.Errorf("expected KEDA's finalizer to survive the update, got %v", finalizers) + } +} + +func TestDeleteScaledObject(t *testing.T) { + ns := "fn-ns" + name := "f-kafka" + + t.Run("removes an existing ScaledObject", func(t *testing.T) { + dynClient := newScalingDynClient(unstructuredScaledObject(name, ns)) + if err := deleteScaledObject(t.Context(), dynClient, ns, name); err != nil { + t.Fatalf("unexpected error: %v", err) + } + _, err := dynClient.Resource(scaledObjectGVR).Namespace(ns).Get(t.Context(), name, metav1.GetOptions{}) + if !k8serrors.IsNotFound(err) { + t.Errorf("expected ScaledObject to be gone, got err: %v", err) + } + }) + + t.Run("not-found is not an error", func(t *testing.T) { + dynClient := newScalingDynClient() + if err := deleteScaledObject(t.Context(), dynClient, ns, name); err != nil { + t.Fatalf("expected nil error for a non-existent ScaledObject, got: %v", err) + } + }) +} + +func TestDeleteTriggerAuth(t *testing.T) { + ns := "fn-ns" + name := "f-kafka-auth" + + t.Run("removes an existing TriggerAuthentication", func(t *testing.T) { + dynClient := newScalingDynClient(unstructuredTriggerAuth(name, ns)) + if err := deleteTriggerAuth(t.Context(), dynClient, ns, name); err != nil { + t.Fatalf("unexpected error: %v", err) + } + _, err := dynClient.Resource(triggerAuthGVR).Namespace(ns).Get(t.Context(), name, metav1.GetOptions{}) + if !k8serrors.IsNotFound(err) { + t.Errorf("expected TriggerAuthentication to be gone, got err: %v", err) + } + }) + + t.Run("not-found is not an error", func(t *testing.T) { + dynClient := newScalingDynClient() + if err := deleteTriggerAuth(t.Context(), dynClient, ns, name); err != nil { + t.Fatalf("expected nil error for a non-existent TriggerAuthentication, got: %v", err) + } + }) +} + +func TestTriggers_NoScale(t *testing.T) { + f := fn.Function{Name: "test"} + got := triggers(f) + if len(got) != 1 || got[0].Type != "http" { + t.Errorf("expected [http] fallback, got %v", got) + } +} + +func TestTriggers_Explicit(t *testing.T) { + lag := int64(5) + f := fn.Function{ + Name: "test", + Scale: &fn.ScaleOptions{ + KEDA: &fn.KEDAScaleOptions{ + Triggers: []fn.KEDATrigger{ + {Type: "kafka", LagThreshold: &lag}, + }, + }, + }, + } + got := triggers(f) + if len(got) != 1 { + t.Fatalf("expected 1 trigger, got %d", len(got)) + } + if got[0].Type != "kafka" { + t.Errorf("expected kafka, got %s", got[0].Type) + } + if *got[0].LagThreshold != 5 { + t.Errorf("expected lagThreshold 5, got %d", *got[0].LagThreshold) + } +} + +// TestTriggers_ExplicitlyEmpty documents the precondition Deploy's +// len(triggers) == 0 guard depends on: scale.keda present with an +// explicitly empty triggers list returns an empty slice here (unlike a nil +// Scale/KEDA, which falls back to a plain http trigger), which would +// otherwise make Deploy skip both the HTTPScaledObject and Kafka +// ScaledObject paths and deploy with no scaler at all. +func TestTriggers_ExplicitlyEmpty(t *testing.T) { + f := fn.Function{ + Name: "test", + Scale: &fn.ScaleOptions{KEDA: &fn.KEDAScaleOptions{Triggers: []fn.KEDATrigger{}}}, + } + if got := triggers(f); len(got) != 0 { + t.Fatalf("expected an explicitly empty triggers list to stay empty, got %v", got) + } +} + +// TestHasKafkaTrigger_WithoutRunKafka documents the precondition +// pkg/keda/deployer.go's Deploy guards against: a kafka trigger declared in +// scale.keda.triggers with no run.kafka configured is exactly the case +// ValidateScale already rejects, but Deploy must reject it too for callers +// that reach Deploy without going through Function.Validate first. +func TestHasKafkaTrigger_WithoutRunKafka(t *testing.T) { + f := fn.Function{ + Name: "test", + Scale: &fn.ScaleOptions{ + KEDA: &fn.KEDAScaleOptions{ + Triggers: []fn.KEDATrigger{{Type: "kafka"}}, + }, + }, + // Run.Kafka intentionally left nil. + } + if !hasKafkaTrigger(triggers(f)) { + t.Fatal("expected hasKafkaTrigger to be true") + } + if f.Run.Kafka != nil { + t.Fatal("expected Run.Kafka to be nil for this test") + } +} + +// TestHasHTTPKafkaTrigger_UnsupportedType documents the precondition +// Deploy's trigger-type guard depends on: a trigger of an unrecognized +// type makes both hasHTTPTrigger and hasKafkaTrigger false, which would +// otherwise make Deploy silently skip every scaler path instead of +// failing. +func TestHasHTTPKafkaTrigger_UnsupportedType(t *testing.T) { + f := fn.Function{ + Name: "test", + Scale: &fn.ScaleOptions{ + KEDA: &fn.KEDAScaleOptions{ + Triggers: []fn.KEDATrigger{{Type: "cron"}}, + }, + }, + } + got := triggers(f) + if hasHTTPTrigger(got) { + t.Error("expected hasHTTPTrigger to be false for an unsupported type") + } + if hasKafkaTrigger(got) { + t.Error("expected hasKafkaTrigger to be false for an unsupported type") + } +} + +func TestParseSecretRef(t *testing.T) { + tests := []struct { + input string + wantName string + wantKey string + }{ + {"{{ secret:my-secret:my-key }}", "my-secret", "my-key"}, + {"{{ secret:foo:bar }}", "foo", "bar"}, + {"plaintext-value", "", ""}, + {"{{ configMap:cm:key }}", "", ""}, + {"{{ invalid }}", "", ""}, + // Trailing garbage after "}}": the old ad-hoc Trim/Split parsing only + // stripped matching cutset characters from the string's own ends, so + // this left "key }} extra" as the parsed key instead of rejecting the + // whole value. fn.TemplateRefPattern anchors on "$" and rejects it. + {"{{ secret:name:key }} extra", "", ""}, + } + for _, tt := range tests { + name, key := parseSecretRef(tt.input) + if name != tt.wantName || key != tt.wantKey { + t.Errorf("parseSecretRef(%q) = (%q, %q), want (%q, %q)", tt.input, name, key, tt.wantName, tt.wantKey) + } + } +} + +func TestFindSecretForPath(t *testing.T) { + secret := "my-cluster-ca" + path := "/etc/kafka/ca" + volumes := []fn.Volume{ + {Secret: &secret, Path: &path}, + } + + name, key := findSecretForPath("/etc/kafka/ca/ca.crt", volumes) + if name != "my-cluster-ca" || key != "ca.crt" { + t.Errorf("got (%q, %q), want (my-cluster-ca, ca.crt)", name, key) + } + + name, _ = findSecretForPath("/other/path", volumes) + if name != "" { + t.Errorf("expected empty for non-matching path, got %q", name) + } + + // Sibling directory sharing a prefix must not match (e.g. "/etc/kafka/ca" + // is not a parent of "/etc/kafka/cab/ca.crt"). + name, _ = findSecretForPath("/etc/kafka/cab/ca.crt", volumes) + if name != "" { + t.Errorf("expected empty for sibling-directory path, got %q", name) + } + + // certPath equal to the mount path names the directory, not a file in + // it: no valid Secret data key, so this must not match. + name, _ = findSecretForPath("/etc/kafka/ca", volumes) + if name != "" { + t.Errorf("expected empty when certPath is the mount path itself, got %q", name) + } + + // A cert nested more than one level deep resolves to a rel containing a + // separator, which isn't a valid Secret data key either. + name, _ = findSecretForPath("/etc/kafka/ca/sub/ca.crt", volumes) + if name != "" { + t.Errorf("expected empty for a cert nested more than one level deep, got %q", name) + } +} + +func testDeployment() *v1.Deployment { + return &v1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-func", + Namespace: "default", + UID: types.UID("test-uid-123"), + }, + Spec: v1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "user-container"}}, + }, + }, + }, + } +} + +// TestOwnerReferences_OmitBlockOwnerDeletion guards against reintroducing +// blockOwnerDeletion: true on the TriggerAuthentication/ScaledObject owner +// references. That flag makes OpenShift's OwnerReferencesPermissionEnforcement +// admission plugin require a finalizers-update grant on the owning +// Deployment that the deploying/pipeline service account doesn't hold by +// default, rejecting the create outright -- the same problem already fixed +// once for the Service owner reference in pkg/k8s/deployer.go. +func TestOwnerReferences_OmitBlockOwnerDeletion(t *testing.T) { + f := fn.Function{ + Name: "test-func", + Run: fn.RunSpec{ + Kafka: &fn.KafkaConfig{ + Brokers: "broker:9093", Topic: "t", ConsumerGroup: "g", + SASL: &fn.KafkaSASL{Mechanism: "PLAIN", Password: "{{ secret:s:k }}"}, + }, + }, + } + deployment := testDeployment() + + ta, err := buildTriggerAuth(f, deployment, "default") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ta == nil { + t.Fatal("expected TriggerAuthentication, got nil") + } + for _, ref := range ta.GetOwnerReferences() { + if ref.BlockOwnerDeletion != nil && *ref.BlockOwnerDeletion { + t.Errorf("TriggerAuthentication owner reference must not set blockOwnerDeletion: true, got %+v", ref) + } + } + + so := buildScaledObject(f, fn.KEDATrigger{Type: "kafka"}, deployment, "default", 0, 10) + if so == nil { + t.Fatal("expected ScaledObject, got nil") + } + for _, ref := range so.GetOwnerReferences() { + if ref.BlockOwnerDeletion != nil && *ref.BlockOwnerDeletion { + t.Errorf("ScaledObject owner reference must not set blockOwnerDeletion: true, got %+v", ref) + } + } +} + +func TestBuildTriggerAuth(t *testing.T) { + f := fn.Function{ + Name: "test-func", + Run: fn.RunSpec{ + Kafka: &fn.KafkaConfig{ + Brokers: "broker:9093", + Topic: "topic", + ConsumerGroup: "group", + SecurityProtocol: "SASL_SSL", + TLS: &fn.KafkaTLS{ + CACert: "/etc/kafka/ca/ca.crt", + }, + SASL: &fn.KafkaSASL{ + Mechanism: "SCRAM-SHA-512", + User: "admin", + Password: "{{ secret:my-user:password }}", + }, + }, + Volumes: []fn.Volume{ + {Secret: strPtr("my-cluster-ca"), Path: strPtr("/etc/kafka/ca")}, + }, + }, + } + + ta, err := buildTriggerAuth(f, testDeployment(), "default") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ta == nil { + t.Fatal("expected TriggerAuthentication, got nil") + } + + if ta.GetName() != "test-func-kafka-auth" { + t.Errorf("name = %q, want test-func-kafka-auth", ta.GetName()) + } + + spec, ok := ta.Object["spec"].(map[string]interface{}) + if !ok { + t.Fatal("missing spec") + } + refs, ok := spec["secretTargetRef"].([]interface{}) + if !ok { + t.Fatal("missing secretTargetRef") + } + if len(refs) != 2 { + t.Fatalf("expected 2 secretTargetRef entries, got %d", len(refs)) + } + + ref0 := refs[0].(map[string]interface{}) + if ref0["parameter"] != "password" || ref0["name"] != "my-user" || ref0["key"] != "password" { + t.Errorf("unexpected password ref: %v", ref0) + } + + ref1 := refs[1].(map[string]interface{}) + if ref1["parameter"] != "ca" || ref1["name"] != "my-cluster-ca" || ref1["key"] != "ca.crt" { + t.Errorf("unexpected ca ref: %v", ref1) + } + + envs, ok := spec["env"].([]interface{}) + if !ok { + t.Fatal("missing env") + } + if len(envs) != 1 { + t.Fatalf("expected 1 env entry, got %d", len(envs)) + } + env0 := envs[0].(map[string]interface{}) + if env0["parameter"] != "username" || env0["name"] != "KAFKA_SASL_USER" { + t.Errorf("unexpected env ref: %v", env0) + } +} + +func TestNeedsTriggerAuth_MutualTLSOnly(t *testing.T) { + // mTLS with no CA cert and no SASL: needsTriggerAuth must still be true, + // or buildTriggerAuth is never even consulted and the client cert/key + // never make it into a TriggerAuthentication. + kafka := &fn.KafkaConfig{ + TLS: &fn.KafkaTLS{ + ClientCert: "/etc/kafka/tls/tls.crt", + ClientKey: "/etc/kafka/tls/tls.key", + }, + } + if !needsTriggerAuth(kafka) { + t.Error("expected needsTriggerAuth to be true for mTLS-only config") + } +} + +// TestBuildTriggerAuth_UnresolvedCACertReturnsError documents that an +// explicitly-configured TLS path that doesn't match any configured volume +// is an error, not a silent skip: pkg/keda/deployer.go's Deploy relies on +// this to fail fast instead of proceeding to create a ScaledObject whose +// authenticationRef points at a TriggerAuthentication that was never +// created (or, before this fix, one missing the credential it needs). +func TestBuildTriggerAuth_UnresolvedCACertReturnsError(t *testing.T) { + f := fn.Function{ + Name: "test-func", + Run: fn.RunSpec{ + Kafka: &fn.KafkaConfig{ + Brokers: "broker:9093", + Topic: "topic", + ConsumerGroup: "group", + TLS: &fn.KafkaTLS{ + CACert: "/etc/kafka/ca/ca.crt", + }, + }, + // No volume backs /etc/kafka/ca: findSecretForPath finds nothing. + }, + } + + if !needsTriggerAuth(f.Run.Kafka) { + t.Fatal("expected needsTriggerAuth to be true") + } + ta, err := buildTriggerAuth(f, testDeployment(), "default") + if err == nil { + t.Fatal("expected an error when the CA cert path matches no volume") + } + if ta != nil { + t.Errorf("expected nil TriggerAuthentication alongside the error, got %v", ta) + } +} + +// TestBuildTriggerAuth_PartiallyResolvedMutualTLSReturnsError covers the +// case Copilot flagged: the CA cert resolves to a Secret, but the client +// cert doesn't match any configured volume. Each TLS field used to be +// optionalized independently, so this returned a TriggerAuthentication +// containing only the "ca" entry -- needsTriggerAuth was satisfied by the +// CA alone, so the ScaledObject would reference a TriggerAuthentication +// missing the client cert/key mTLS needs, and the scaler would silently +// fail to authenticate. An explicitly-configured path that fails to +// resolve must error regardless of whether other fields resolved fine. +func TestBuildTriggerAuth_PartiallyResolvedMutualTLSReturnsError(t *testing.T) { + f := fn.Function{ + Name: "test-func", + Run: fn.RunSpec{ + Kafka: &fn.KafkaConfig{ + Brokers: "broker:9093", + Topic: "topic", + ConsumerGroup: "group", + TLS: &fn.KafkaTLS{ + CACert: "/etc/kafka/ca/ca.crt", + ClientCert: "/etc/kafka/tls/tls.crt", + ClientKey: "/etc/kafka/tls/tls.key", + }, + }, + Volumes: []fn.Volume{ + // Only the CA volume is configured; client cert/key are not. + {Secret: strPtr("my-cluster-ca"), Path: strPtr("/etc/kafka/ca")}, + }, + }, + } + + ta, err := buildTriggerAuth(f, testDeployment(), "default") + if err == nil { + t.Fatal("expected an error when the client cert path matches no volume, even though the CA resolved") + } + if ta != nil { + t.Errorf("expected nil TriggerAuthentication alongside the error, got %v", ta) + } +} + +func TestBuildTriggerAuth_MutualTLS(t *testing.T) { + f := fn.Function{ + Name: "test-func", + Run: fn.RunSpec{ + Kafka: &fn.KafkaConfig{ + Brokers: "broker:9093", + Topic: "topic", + ConsumerGroup: "group", + SecurityProtocol: "SSL", + TLS: &fn.KafkaTLS{ + CACert: "/etc/kafka/ca/ca.crt", + ClientCert: "/etc/kafka/tls/tls.crt", + ClientKey: "/etc/kafka/tls/tls.key", + }, + }, + Volumes: []fn.Volume{ + {Secret: strPtr("my-cluster-ca"), Path: strPtr("/etc/kafka/ca")}, + {Secret: strPtr("my-client-tls"), Path: strPtr("/etc/kafka/tls")}, + }, + }, + } + + ta, err := buildTriggerAuth(f, testDeployment(), "default") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ta == nil { + t.Fatal("expected TriggerAuthentication, got nil") + } + + spec, ok := ta.Object["spec"].(map[string]interface{}) + if !ok { + t.Fatal("missing spec") + } + refs, ok := spec["secretTargetRef"].([]interface{}) + if !ok { + t.Fatal("missing secretTargetRef") + } + if len(refs) != 3 { + t.Fatalf("expected 3 secretTargetRef entries (ca, cert, key), got %d: %v", len(refs), refs) + } + + want := map[string][2]string{ + "ca": {"my-cluster-ca", "ca.crt"}, + "cert": {"my-client-tls", "tls.crt"}, + "key": {"my-client-tls", "tls.key"}, + } + for _, r := range refs { + ref := r.(map[string]interface{}) + param := ref["parameter"].(string) + exp, ok := want[param] + if !ok { + t.Errorf("unexpected parameter %q in secretTargetRef", param) + continue + } + if ref["name"] != exp[0] || ref["key"] != exp[1] { + t.Errorf("parameter %q: got (name=%v, key=%v), want (%q, %q)", param, ref["name"], ref["key"], exp[0], exp[1]) + } + delete(want, param) + } + if len(want) != 0 { + t.Errorf("missing secretTargetRef parameters: %v", want) + } +} + +func TestBuildTriggerAuth_PlaintextPassword(t *testing.T) { + f := fn.Function{ + Name: "test-func", + Run: fn.RunSpec{ + Kafka: &fn.KafkaConfig{ + Brokers: "broker:9093", + Topic: "topic", + ConsumerGroup: "group", + SecurityProtocol: "SASL_SSL", + SASL: &fn.KafkaSASL{ + Mechanism: "SCRAM-SHA-512", + User: "admin", + // plaintext, not a {{ secret:... }} reference -- allowed, at + // least for debugging purposes. + Password: "plaintext-password", + }, + }, + }, + } + + ta, err := buildTriggerAuth(f, testDeployment(), "default") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ta == nil { + t.Fatal("expected TriggerAuthentication, got nil") + } + + spec, ok := ta.Object["spec"].(map[string]interface{}) + if !ok { + t.Fatal("missing spec") + } + + // A plaintext password must not be silently dropped: it should be wired + // up as an env-based auth reference pointing at the KAFKA_SASL_PASSWORD + // env var that pkg/k8s/deployer.go sets on the function's container. + if _, ok := spec["secretTargetRef"]; ok { + t.Error("did not expect secretTargetRef for a plaintext password") + } + + envs, ok := spec["env"].([]interface{}) + if !ok { + t.Fatal("missing env") + } + if len(envs) != 2 { + t.Fatalf("expected 2 env entries (username + password), got %d", len(envs)) + } + + var sawUser, sawPassword bool + for _, e := range envs { + entry := e.(map[string]interface{}) + switch entry["parameter"] { + case "username": + sawUser = true + if entry["name"] != "KAFKA_SASL_USER" { + t.Errorf("username env name = %v, want KAFKA_SASL_USER", entry["name"]) + } + case "password": + sawPassword = true + if entry["name"] != "KAFKA_SASL_PASSWORD" { + t.Errorf("password env name = %v, want KAFKA_SASL_PASSWORD", entry["name"]) + } + } + } + if !sawUser { + t.Error("expected a username env ref") + } + if !sawPassword { + t.Error("expected a password env ref") + } +} + +func TestBuildScaledObject(t *testing.T) { + lag := int64(20) + f := fn.Function{ + Name: "test-func", + Run: fn.RunSpec{ + Kafka: &fn.KafkaConfig{ + Brokers: "broker:9093", + Topic: "my-topic", + ConsumerGroup: "my-group", + SecurityProtocol: "SASL_SSL", + TLS: &fn.KafkaTLS{CACert: "/etc/kafka/ca/ca.crt"}, + SASL: &fn.KafkaSASL{Mechanism: "SCRAM-SHA-512", Password: "{{ secret:s:k }}"}, + }, + }, + } + trigger := fn.KEDATrigger{Type: "kafka", LagThreshold: &lag} + + so := buildScaledObject(f, trigger, testDeployment(), "default", 0, 10) + if so == nil { + t.Fatal("expected ScaledObject, got nil") + } + + if so.GetName() != "test-func-kafka" { + t.Errorf("name = %q, want test-func-kafka", so.GetName()) + } + + spec := so.Object["spec"].(map[string]interface{}) + if spec["minReplicaCount"] != int64(0) { + t.Errorf("minReplicaCount = %v, want 0", spec["minReplicaCount"]) + } + if spec["maxReplicaCount"] != int64(10) { + t.Errorf("maxReplicaCount = %v, want 10", spec["maxReplicaCount"]) + } + + triggers := spec["triggers"].([]interface{}) + if len(triggers) != 1 { + t.Fatalf("expected 1 trigger, got %d", len(triggers)) + } + trigger0 := triggers[0].(map[string]interface{}) + meta := trigger0["metadata"].(map[string]interface{}) + if meta["bootstrapServers"] != "broker:9093" { + t.Errorf("bootstrapServers = %v", meta["bootstrapServers"]) + } + if meta["lagThreshold"] != "20" { + t.Errorf("lagThreshold = %v, want 20", meta["lagThreshold"]) + } + if meta["tls"] != "enable" { + t.Errorf("tls = %v, want enable", meta["tls"]) + } + if meta["sasl"] != "scram_sha512" { + t.Errorf("sasl = %v, want scram_sha512", meta["sasl"]) + } + + authRef := trigger0["authenticationRef"].(map[string]interface{}) + if authRef["name"] != "test-func-kafka-auth" { + t.Errorf("authenticationRef name = %v", authRef["name"]) + } +} + +func TestBuildScaledObject_TLSFromSecurityProtocol(t *testing.T) { + // SecurityProtocol: SSL with no explicit run.kafka.tls block (relying on + // the system's CA trust store) must still enable KEDA's tls handshake -- + // gating on kafka.TLS != nil would leave it plaintext for this config. + f := fn.Function{ + Name: "test-func", + Run: fn.RunSpec{ + Kafka: &fn.KafkaConfig{ + Brokers: "broker:9093", + Topic: "t", + ConsumerGroup: "g", + SecurityProtocol: "SSL", + }, + }, + } + trigger := fn.KEDATrigger{Type: "kafka"} + + so := buildScaledObject(f, trigger, testDeployment(), "default", 0, 10) + if so == nil { + t.Fatal("expected ScaledObject, got nil") + } + spec := so.Object["spec"].(map[string]interface{}) + trigger0 := spec["triggers"].([]interface{})[0].(map[string]interface{}) + meta := trigger0["metadata"].(map[string]interface{}) + if meta["tls"] != "enable" { + t.Errorf("tls = %v, want enable for securityProtocol SSL with no explicit tls block", meta["tls"]) + } +} + +func TestBuildScaledObject_UnsafeSslFromSkipVerify(t *testing.T) { + f := fn.Function{ + Name: "test-func", + Run: fn.RunSpec{ + Kafka: &fn.KafkaConfig{ + Brokers: "broker:9093", + Topic: "t", + ConsumerGroup: "g", + SecurityProtocol: "SSL", + TLS: &fn.KafkaTLS{SkipVerify: true}, + }, + }, + } + trigger := fn.KEDATrigger{Type: "kafka"} + + so := buildScaledObject(f, trigger, testDeployment(), "default", 0, 10) + if so == nil { + t.Fatal("expected ScaledObject, got nil") + } + spec := so.Object["spec"].(map[string]interface{}) + trigger0 := spec["triggers"].([]interface{})[0].(map[string]interface{}) + meta := trigger0["metadata"].(map[string]interface{}) + if meta["unsafeSsl"] != "true" { + t.Errorf("unsafeSsl = %v, want \"true\" when run.kafka.tls.skipVerify is set", meta["unsafeSsl"]) + } +} + +func TestBuildScaledObject_NoUnsafeSslWithoutSkipVerify(t *testing.T) { + f := fn.Function{ + Name: "test-func", + Run: fn.RunSpec{ + Kafka: &fn.KafkaConfig{ + Brokers: "broker:9093", + Topic: "t", + ConsumerGroup: "g", + SecurityProtocol: "SSL", + TLS: &fn.KafkaTLS{CACert: "/etc/kafka/ca/ca.crt"}, + }, + }, + } + trigger := fn.KEDATrigger{Type: "kafka"} + + so := buildScaledObject(f, trigger, testDeployment(), "default", 0, 10) + if so == nil { + t.Fatal("expected ScaledObject, got nil") + } + spec := so.Object["spec"].(map[string]interface{}) + trigger0 := spec["triggers"].([]interface{})[0].(map[string]interface{}) + meta := trigger0["metadata"].(map[string]interface{}) + if _, ok := meta["unsafeSsl"]; ok { + t.Errorf("expected no unsafeSsl key when skipVerify is false, got %v", meta["unsafeSsl"]) + } +} + +func TestBuildScaledObject_NoTLSForPlaintext(t *testing.T) { + f := fn.Function{ + Name: "test-func", + Run: fn.RunSpec{ + Kafka: &fn.KafkaConfig{ + Brokers: "broker:9092", + Topic: "t", + ConsumerGroup: "g", + SecurityProtocol: "PLAINTEXT", + }, + }, + } + trigger := fn.KEDATrigger{Type: "kafka"} + + so := buildScaledObject(f, trigger, testDeployment(), "default", 0, 10) + if so == nil { + t.Fatal("expected ScaledObject, got nil") + } + spec := so.Object["spec"].(map[string]interface{}) + trigger0 := spec["triggers"].([]interface{})[0].(map[string]interface{}) + meta := trigger0["metadata"].(map[string]interface{}) + if _, ok := meta["tls"]; ok { + t.Errorf("expected no tls key for securityProtocol PLAINTEXT, got %v", meta["tls"]) + } +} + +func TestBuildScaledObject_DefaultLag(t *testing.T) { + f := fn.Function{ + Name: "test-func", + Run: fn.RunSpec{ + Kafka: &fn.KafkaConfig{ + Brokers: "broker:9092", + Topic: "t", + ConsumerGroup: "g", + }, + }, + } + trigger := fn.KEDATrigger{Type: "kafka"} + + so := buildScaledObject(f, trigger, testDeployment(), "default", 1, 5) + if so == nil { + t.Fatal("expected ScaledObject, got nil") + } + + spec := so.Object["spec"].(map[string]interface{}) + triggers := spec["triggers"].([]interface{}) + trigger0 := triggers[0].(map[string]interface{}) + meta := trigger0["metadata"].(map[string]interface{}) + if meta["lagThreshold"] != "10" { + t.Errorf("default lagThreshold = %v, want 10", meta["lagThreshold"]) + } + + // No TLS/SASL, so no authenticationRef + if _, ok := trigger0["authenticationRef"]; ok { + t.Error("expected no authenticationRef for plaintext Kafka") + } +} + +func TestKedaSASLType(t *testing.T) { + tests := map[string]string{ + "SCRAM-SHA-256": "scram_sha256", + "SCRAM-SHA-512": "scram_sha512", + "PLAIN": "plain", + "UNKNOWN": "", + } + for in, want := range tests { + if got := kedaSASLType(in); got != want { + t.Errorf("kedaSASLType(%q) = %q, want %q", in, got, want) + } + } +} + +func strPtr(s string) *string { return &s } diff --git a/pkg/keda/lister.go b/pkg/keda/lister.go index 7e0edd0c9e..c3f7c02965 100644 --- a/pkg/keda/lister.go +++ b/pkg/keda/lister.go @@ -6,7 +6,9 @@ import ( "github.com/kedacore/http-add-on/operator/apis/http/v1alpha1" "github.com/kedacore/http-add-on/operator/generated/clientset/versioned" + appsv1 "k8s.io/api/apps/v1" v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" @@ -80,8 +82,14 @@ func (l *Lister) List(ctx context.Context, namespace string) ([]fn.ListItem, err // object up again. func (l *Lister) get(ctx context.Context, clientset *kubernetes.Clientset, httpScaledObjectClientset *versioned.Clientset, name, namespace, runtime, exposedHost string) (fn.ListItem, error) { httpScaledObject, err := httpScaledObjectClientset.HttpV1alpha1().HTTPScaledObjects(namespace).Get(ctx, name, metav1.GetOptions{}) + hasHTTPTrigger := true if err != nil { - return fn.ListItem{}, fmt.Errorf("unable to get HTTPScaledObject: %v", err) + if !errors.IsNotFound(err) { + return fn.ListItem{}, fmt.Errorf("unable to get HTTPScaledObject: %v", err) + } + // A Kafka-only (or otherwise no-http-trigger) function never gets an + // HTTPScaledObject at all -- that's expected, not a failure. + hasHTTPTrigger = false } deployment, err := clientset.AppsV1().Deployments(namespace).Get(ctx, name, metav1.GetOptions{}) @@ -90,15 +98,29 @@ func (l *Lister) get(ctx context.Context, clientset *kubernetes.Clientset, httpS } replicas := int(deployment.Status.ReadyReplicas) - ready := v1.ConditionUnknown - if meta.IsStatusConditionTrue(httpScaledObject.Status.Conditions, v1alpha1.ConditionTypeReady) { - ready = v1.ConditionTrue - } else if meta.IsStatusConditionFalse(httpScaledObject.Status.Conditions, v1alpha1.ConditionTypeReady) { - ready = v1.ConditionFalse + var ready v1.ConditionStatus + var url string + if hasHTTPTrigger { + ready = v1.ConditionUnknown + if meta.IsStatusConditionTrue(httpScaledObject.Status.Conditions, v1alpha1.ConditionTypeReady) { + ready = v1.ConditionTrue + } else if meta.IsStatusConditionFalse(httpScaledObject.Status.Conditions, v1alpha1.ConditionTypeReady) { + ready = v1.ConditionFalse + } + url, _ = functionURLs(httpScaledObject.Spec.Hosts, exposedHost) + } else { + // No HTTP trigger: fall back to the Deployment's own readiness + // condition and the cluster-local Service URL, same as Describe. + ready = v1.ConditionUnknown + for _, cond := range deployment.Status.Conditions { + if cond.Type == appsv1.DeploymentAvailable { + ready = cond.Status + break + } + } + url = fmt.Sprintf("http://%s.%s.svc", name, namespace) } - url, _ := functionURLs(httpScaledObject.Spec.Hosts, exposedHost) - listItem := fn.ListItem{ Name: name, Namespace: namespace, diff --git a/pkg/keda/remover.go b/pkg/keda/remover.go index 21e5955a84..c5d2eb2bfe 100644 --- a/pkg/keda/remover.go +++ b/pkg/keda/remover.go @@ -50,6 +50,11 @@ func (remover *Remover) Remove(ctx context.Context, name, ns string) error { return fn.ErrNotHandled } + dynClient, err := k8s.NewDynamicClient() + if err != nil { + return fmt.Errorf("could not setup dynamic client: %w", err) + } + // Remove the recorded Route before deleting anything: keda's Route has no // owner reference (it would have to cross namespaces), so nothing collects // it, and its record - these Service annotations - is deleted with the @@ -57,10 +62,6 @@ func (remover *Remover) Remove(ctx context.Context, name, ns string) error { // A Route left unrecorded by a crash is not searched for; the next // exposed redeploy finds it by its function labels. if recordedNS := svc.Annotations[k8s.RouteNamespaceAnnotation]; recordedNS != "" { - dynClient, err := k8s.NewDynamicClient() - if err != nil { - return fmt.Errorf("could not setup dynamic client: %w", err) - } if err := ocproute.New(KedaDeployerName).Unexpose(ctx, dynClient, deployer.NewExposureRef(name, ns, recordedNS)); err != nil { return fmt.Errorf("could not remove the Route exposing function %q in namespace %q; "+ "nothing was deleted and the function is still running, if you fix this you can run delete again: %w", @@ -68,6 +69,20 @@ func (remover *Remover) Remove(ctx context.Context, name, ns string) error { } } + // Clean up Kafka scaling resources before deleting the Deployment. + // These have ownerReferences so they'd be garbage-collected, but + // explicit deletion avoids races with a slow GC. Errors here (both + // functions already ignore not-found) are not fatal to Remove: the + // owner reference still cleans these up eventually, but the user is + // warned so a persistent failure (e.g. missing RBAC) doesn't go + // unnoticed. + if err := deleteScaledObject(ctx, dynClient, ns, scaledObjectName(name)); err != nil { + fmt.Fprintf(os.Stderr, "warning: %v\n", err) + } + if err := deleteTriggerAuth(ctx, dynClient, ns, triggerAuthName(name)); err != nil { + fmt.Fprintf(os.Stderr, "warning: %v\n", err) + } + deploymentClient := clientset.AppsV1().Deployments(ns) // Delete only the Deployment; owner references take the rest with it. diff --git a/pkg/knative/deployer.go b/pkg/knative/deployer.go index 4e719d6b05..ddad2830d3 100644 --- a/pkg/knative/deployer.go +++ b/pkg/knative/deployer.go @@ -492,7 +492,7 @@ func generateNewService(f fn.Function, decorator deployer.DeployDecorator, daprI }, } - err = setServiceOptions(&service.Spec.Template, f.Deploy.Options) + err = setServiceOptions(&service.Spec.Template, f.Scale, f.Deploy.Options) if err != nil { return service, err } @@ -551,7 +551,7 @@ func updateService(f fn.Function, previousService *servingv1.Service, newEnv []c cp := &service.Spec.Template.Spec.Containers[0] k8s.SetHealthEndpoints(f, cp) - err := setServiceOptions(&service.Spec.Template, f.Deploy.Options) + err := setServiceOptions(&service.Spec.Template, f.Scale, f.Deploy.Options) if err != nil { return service, err } @@ -580,38 +580,47 @@ func updateService(f fn.Function, previousService *servingv1.Service, newEnv []c } // setServiceOptions sets annotations on Service Revision Template or in the Service Spec -// from values specified in function configuration options -func setServiceOptions(template *servingv1.RevisionTemplateSpec, options fn.Options) error { +// from values specified in function configuration options and scale config. +func setServiceOptions(template *servingv1.RevisionTemplateSpec, scale *fn.ScaleOptions, options fn.Options) error { toRemove := []string{} toUpdate := map[string]string{} - if options.Scale != nil { - if options.Scale.Min != nil { - toUpdate[autoscaling.MinScaleAnnotationKey] = fmt.Sprintf("%d", *options.Scale.Min) + if scale != nil { + if scale.Min != nil { + toUpdate[autoscaling.MinScaleAnnotationKey] = fmt.Sprintf("%d", *scale.Min) } else { toRemove = append(toRemove, autoscaling.MinScaleAnnotationKey) } - if options.Scale.Max != nil { - toUpdate[autoscaling.MaxScaleAnnotationKey] = fmt.Sprintf("%d", *options.Scale.Max) + if scale.Max != nil { + toUpdate[autoscaling.MaxScaleAnnotationKey] = fmt.Sprintf("%d", *scale.Max) } else { toRemove = append(toRemove, autoscaling.MaxScaleAnnotationKey) } - if options.Scale.Metric != nil { - toUpdate[autoscaling.MetricAnnotationKey] = *options.Scale.Metric + var metric *string + var target *float64 + var utilization *float64 + if scale.KPA != nil { + metric = scale.KPA.Metric + target = scale.KPA.Target + utilization = scale.KPA.Utilization + } + + if metric != nil { + toUpdate[autoscaling.MetricAnnotationKey] = *metric } else { toRemove = append(toRemove, autoscaling.MetricAnnotationKey) } - if options.Scale.Target != nil { - toUpdate[autoscaling.TargetAnnotationKey] = fmt.Sprintf("%f", *options.Scale.Target) + if target != nil { + toUpdate[autoscaling.TargetAnnotationKey] = fmt.Sprintf("%f", *target) } else { toRemove = append(toRemove, autoscaling.TargetAnnotationKey) } - if options.Scale.Utilization != nil { - toUpdate[autoscaling.TargetUtilizationPercentageKey] = fmt.Sprintf("%f", *options.Scale.Utilization) + if utilization != nil { + toUpdate[autoscaling.TargetUtilizationPercentageKey] = fmt.Sprintf("%f", *utilization) } else { toRemove = append(toRemove, autoscaling.TargetUtilizationPercentageKey) } diff --git a/pkg/mock/deployer.go b/pkg/mock/deployer.go index 874e48bc93..4ad571b688 100644 --- a/pkg/mock/deployer.go +++ b/pkg/mock/deployer.go @@ -37,7 +37,7 @@ func NewDeployer() *Deployer { if f.Deployer != "" { result.Deployer = f.Deployer // deployed with that requested } else { - result.Deployer = f.Deploy.Deployer // redeploy with current + result.Deployer = f.Deploy.ActiveDeployer // redeploy with current } // Observed exposure mirrors intent when active (same as real deployers). if fn.ActiveExpose(f.Expose) { diff --git a/pkg/pipelines/tekton/pipelines_int_test.go b/pkg/pipelines/tekton/pipelines_int_test.go index 52bb3dac5a..cafeeb7983 100644 --- a/pkg/pipelines/tekton/pipelines_int_test.go +++ b/pkg/pipelines/tekton/pipelines_int_test.go @@ -165,12 +165,10 @@ func TestInt_Remote_Default(t *testing.T) { Template: "echo", Registry: TestRegistry, Namespace: TestNamespace, + Deployer: d, Build: fn.BuildSpec{ Builder: "pack", // TODO: test "s2i". Currently it causes a 'no space left on device' error in GH actions. }, - Deploy: fn.DeploySpec{ - Deployer: d, - }, } if f, err = client.Init(f); err != nil { diff --git a/pkg/pipelines/tekton/pipelines_provider.go b/pkg/pipelines/tekton/pipelines_provider.go index bba07e4da2..d1ea3155d7 100644 --- a/pkg/pipelines/tekton/pipelines_provider.go +++ b/pkg/pipelines/tekton/pipelines_provider.go @@ -113,7 +113,7 @@ func NewPipelinesProvider(opts ...Opt) *PipelinesProvider { // definition, sending it to the cluster to be run via Tekton. // Progress is by default piped to stdtout. // Returned is the final url, and the input Function with the final results of the run populated -// (f.Deploy.Image, f.Deploy.Namespace, f.Deploy.Deployer and f.Deploy.Expose) +// (f.Deploy.Image, f.Deploy.Namespace, f.Deploy.ActiveDeployer and f.Deploy.ActiveExpose) // or an error. func (pp *PipelinesProvider) Run(ctx context.Context, f fn.Function) (string, fn.Function, error) { var err error @@ -152,15 +152,15 @@ func (pp *PipelinesProvider) Run(ctx context.Context, f fn.Function) (string, fn f.Deploy.Image = image // Deployer is either the intended deployer (f.Deployer) or the one it was - // last deployed with (f.Deploy.Deployer). Recorded so a remote deploy is + // last deployed with (f.Deploy.ActiveDeployer). Recorded so a remote deploy is // remembered in func.yaml, mirroring Namespace and Image above. deployer := f.Deployer if deployer == "" { - deployer = f.Deploy.Deployer + deployer = f.Deploy.ActiveDeployer } - f.Deploy.Deployer = deployer + f.Deploy.ActiveDeployer = deployer - // Applied exposure (f.Deploy.Expose) is deliberately NOT derived from intent + // Applied exposure (f.Deploy.ActiveExpose) is deliberately NOT derived from intent // here: the pipeline runs a published func-util image this build does not // compile, so what it did with expose is established by looking. Recorded // after the run from the describer, which reads the annotation the on-cluster @@ -273,7 +273,7 @@ func (pp *PipelinesProvider) Run(ctx context.Context, f fn.Function) (string, fn } var describer fn.Describer - switch f.Deploy.Deployer { + switch f.Deploy.ActiveDeployer { case k8s.KubernetesDeployerName: describer = k8s.NewDescriber(false, k8s.WithDescriberTransport(pp.transport)) case keda.KedaDeployerName: @@ -287,7 +287,7 @@ func (pp *PipelinesProvider) Run(ctx context.Context, f fn.Function) (string, fn if err != nil { return "", f, fmt.Errorf("problem in retrieving status of deployed function: %v", err) } - f.Deploy.Expose = obj.Expose + f.Deploy.ActiveExpose = obj.Expose verb := "deployed" if obj.Generation != 1 { diff --git a/schema/func_yaml-schema.json b/schema/func_yaml-schema.json index ee142243d3..7da34726bf 100644 --- a/schema/func_yaml-schema.json +++ b/schema/func_yaml-schema.json @@ -111,14 +111,14 @@ "type": "string", "description": "ImagePullSecret is the name of a Secret in the same namespace used\nfor pulling the function's container image from a private registry.\nMore info: https://kubernetes.io/docs/tasks/configure-pod-container/pull-image-private-registry/" }, - "deployer": { + "activeDeployer": { "enum": [ "knative", "raw", "keda" ], "type": "string", - "description": "Deployer records the deployer the Function is CURRENTLY DEPLOYED:\nobserved state, written after successful deployment, and cleared on\nundeploy alongside Namespace." + "description": "ActiveDeployer records the deployer the Function is CURRENTLY DEPLOYED\nwith: observed state, written after successful deployment, and cleared\non undeploy alongside Namespace. User intent lives on Function.Deployer." }, "subscriptions": { "items": { @@ -131,14 +131,14 @@ "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": { + "activeExpose": { "enum": [ "route", "none", "" ], "type": "string", - "description": "Expose records the external exposure mode CURRENTLY applied on the\ncluster for raw/keda (observed state). Written after successful deploy,\ncleared on undeploy alongside Namespace and Deployer. Empty means\ncluster-local (or never exposed). User intent lives on Function.Expose." + "description": "ActiveExpose records the external exposure mode CURRENTLY applied on\nthe cluster for raw/keda (observed state). Written after successful\ndeploy, cleared on undeploy alongside Namespace and ActiveDeployer.\nEmpty means cluster-local (or never exposed). User intent lives on\nFunction.Expose." } }, "additionalProperties": false, @@ -218,7 +218,7 @@ "keda" ], "type": "string", - "description": "Deployer with which to deploy the Function: the requested (intended)\ndeployer. This is the user's choice and persists across undeploy.\nThe deployer a Function is CURRENTLY deployed with is recorded separately\nin .Deploy.Deployer, which is cleared on undeploy." + "description": "Deployer with which to deploy the Function: the requested (intended)\ndeployer. This is the user's choice and persists across undeploy.\nThe deployer a Function is CURRENTLY deployed with is recorded separately\nin .Deploy.ActiveDeployer, which is cleared on undeploy." }, "expose": { "enum": [ @@ -227,7 +227,7 @@ "" ], "type": "string", - "description": "Expose is the requested (intended) external exposure mode for the raw\nand keda deployers (knative manages its own networking and ignores it).\nValues: \"route\" (OpenShift Route; OpenShift only), \"none\" (cluster-local).\nEmpty means cluster-local. Persists across undeploy like Deployer.\nThe mode CURRENTLY applied on the cluster is recorded separately in\n.Deploy.Expose, which is cleared on undeploy." + "description": "Expose is the requested (intended) external exposure mode for the raw\nand keda deployers (knative manages its own networking and ignores it).\nValues: \"route\" (OpenShift Route; OpenShift only), \"none\" (cluster-local).\nEmpty means cluster-local. Persists across undeploy like Deployer.\nThe mode CURRENTLY applied on the cluster is recorded separately in\n.Deploy.ActiveExpose, which is cleared on undeploy." }, "created": { "type": "string", @@ -256,6 +256,11 @@ "$schema": "http://json-schema.org/draft-04/schema#", "$ref": "#/definitions/DeploySpec", "description": "Deploy defines the deployment properties for a function" + }, + "scale": { + "$schema": "http://json-schema.org/draft-04/schema#", + "$ref": "#/definitions/ScaleOptions", + "description": "Scale defines autoscaling configuration for the function." } }, "additionalProperties": false, @@ -290,6 +295,92 @@ "type": "object", "description": "HealthEndpoints specify the liveness and readiness endpoints for a Runtime" }, + "KEDAScaleOptions": { + "properties": { + "pollingInterval": { + "type": "integer", + "minimum": 1 + }, + "cooldownPeriod": { + "type": "integer", + "minimum": 1 + }, + "triggers": { + "items": { + "$schema": "http://json-schema.org/draft-04/schema#", + "$ref": "#/definitions/KEDATrigger" + }, + "type": "array" + } + }, + "additionalProperties": false, + "type": "object" + }, + "KEDATrigger": { + "required": [ + "type" + ], + "properties": { + "type": { + "enum": [ + "http", + "kafka", + "cron" + ], + "type": "string" + }, + "targetValue": { + "type": "integer", + "minimum": 1 + }, + "lagThreshold": { + "type": "integer", + "minimum": 1 + }, + "activationLagThreshold": { + "type": "integer", + "minimum": 0 + }, + "timezone": { + "type": "string" + }, + "start": { + "type": "string" + }, + "end": { + "type": "string" + }, + "desiredReplicas": { + "type": "integer", + "minimum": 1 + } + }, + "additionalProperties": false, + "type": "object" + }, + "KPAScaleOptions": { + "properties": { + "metric": { + "enum": [ + "concurrency", + "rps" + ], + "type": "string" + }, + "target": { + "exclusiveMinimum": true, + "type": "number", + "minimum": 0 + }, + "utilization": { + "maximum": 100, + "minimum": 1, + "type": "number" + } + }, + "additionalProperties": false, + "type": "object" + }, "KafkaConfig": { "required": [ "brokers", @@ -438,10 +529,6 @@ }, "Options": { "properties": { - "scale": { - "$schema": "http://json-schema.org/draft-04/schema#", - "$ref": "#/definitions/ScaleOptions" - }, "resources": { "$schema": "http://json-schema.org/draft-04/schema#", "$ref": "#/definitions/ResourcesOptions" @@ -551,21 +638,13 @@ "type": "integer", "minimum": 0 }, - "metric": { - "enum": [ - "concurrency", - "rps" - ], - "type": "string" - }, - "target": { - "type": "number", - "minimum": 0 + "keda": { + "$schema": "http://json-schema.org/draft-04/schema#", + "$ref": "#/definitions/KEDAScaleOptions" }, - "utilization": { - "maximum": 100, - "minimum": 1, - "type": "number" + "kpa": { + "$schema": "http://json-schema.org/draft-04/schema#", + "$ref": "#/definitions/KPAScaleOptions" } }, "additionalProperties": false,