Add TLS/SASL authentication support for Kafka functions - #3975
Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends the function-level Kafka configuration to support TLS and SASL authentication, and propagates the resulting settings into the various deploy/run paths (Kubernetes/Knative deployers and local runners).
Changes:
- Extend
run.kafkaschema withsecurityProtocol,tls, andsasl(including validation). - Emit additional
KAFKA_SECURITY_PROTOCOL,KAFKA_TLS_*, andKAFKA_SASL_*environment variables during deployment/run. - Support
{{ secret:name:key }}-style value references for Kafka SASL user/password in the k8s deployer env var generation.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/knative/deployer.go | Updates Knative deploy path to use the new error-returning Kafka env injection (and track referenced resources). |
| pkg/k8s/deployer.go | Extends Kafka env var generation to include TLS/SASL fields and secret/configMap key refs for SASL values. |
| pkg/k8s/deployer_test.go | Adapts existing tests to new signature and adds coverage for TLS/SASL and secret-ref cases. |
| pkg/functions/runner.go | Propagates Kafka TLS/SASL env vars for the host runner (func run). |
| pkg/functions/function.go | Adds new Kafka config types/fields and validation rules for protocol/TLS/SASL combinations. |
| pkg/functions/function_test.go | Adds validation test cases for the new Kafka TLS/SASL config combinations. |
| pkg/docker/runner.go | Propagates Kafka TLS/SASL env vars for the Docker runner. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (4)
schema/func_yaml-schema.json:313
- schema/func_yaml-schema.json appears to be a generated artifact (see Makefile:393-403 and schema/generator/main.go:23-55). Editing it manually can easily drift from the Go struct tags in pkg/functions/function.go; it should be regenerated via
make schema-generateand the regenerated output committed instead of hand-maintaining this section.
"securityProtocol": {
"enum": [
"PLAINTEXT",
"SSL",
"SASL_PLAINTEXT",
"SASL_SSL"
],
"type": "string",
"description": "Security protocol: PLAINTEXT SSL SASL_PLAINTEXT or SASL_SSL"
},
"tls": {
"$schema": "http://json-schema.org/draft-04/schema#",
"$ref": "#/definitions/KafkaTLS",
"description": "TLS configuration for SSL or SASL_SSL"
},
"sasl": {
"$schema": "http://json-schema.org/draft-04/schema#",
"$ref": "#/definitions/KafkaSASL",
"description": "SASL authentication for SASL_PLAINTEXT or SASL_SSL"
}
pkg/functions/function.go:265
- Kafka SASL template refs are validated against templateRefPattern, but the current regex doesn’t allow trailing whitespace after the closing
}}while the deploy-time parser (pkg/k8s/deployer.go) effectively tolerates it via TrimSpace/Trim. This can cause valid-looking values (e.g."{{ secret:n:k }} ") to fail validation even though they’d parse during deploy. Consider trimming before validation and allowing optional trailing whitespace so validation and parsing accept the same inputs.
var templateRefPattern = regexp.MustCompile(`^\{\{\s*(secret|configMap):[^:]+:[^:]+\s*\}\}$`)
func validateTemplateRef(field, value string) (errors []string) {
if strings.HasPrefix(value, "{{") && !templateRefPattern.MatchString(value) {
errors = append(errors, fmt.Sprintf("%s has invalid reference format %q, expected {{ secret:name:key }} or {{ configMap:name:key }}", field, value))
}
pkg/k8s/deployer.go:808
- appendKafkaEnvValue only treats values as template refs when the raw string starts with
"{{", so leading whitespace (e.g." {{ secret:n:k }}") will silently be treated as a literal and won’t produce a ValueFrom SecretKeyRef/ConfigMapKeyRef. Since validation/parsing already tolerates whitespace elsewhere, it’s safer to TrimSpace before checking for{{and to parse the innersecret|configMap:name:keyby stripping{{/}}explicitly.
func appendKafkaEnvValue(envVars []corev1.EnvVar, name, value string, referencedSecrets, referencedConfigMaps *sets.Set[string]) ([]corev1.EnvVar, error) {
if strings.HasPrefix(value, "{{") {
if !strings.HasSuffix(strings.TrimSpace(value), "}}") {
return nil, fmt.Errorf("invalid reference format %q, expected {{ secret:name:key }} or {{ configMap:name:key }}", value)
}
slices := strings.Split(strings.Trim(value, "{} "), ":")
if len(slices) == 3 {
valueFrom, err := createEnvVarSource(slices, referencedSecrets, referencedConfigMaps)
if err != nil {
return nil, err
}
return append(envVars, corev1.EnvVar{Name: name, ValueFrom: valueFrom}), nil
}
return nil, fmt.Errorf("invalid reference format %q, expected {{ secret:name:key }} or {{ configMap:name:key }}", value)
}
return append(envVars, corev1.EnvVar{Name: name, Value: value}), nil
pkg/functions/runner.go:344
- This adds new Kafka TLS/SASL environment variable propagation for the host runner, but there are already unit tests covering Kafka env construction (pkg/functions/runner_test.go:42-78) and they don’t assert the new variables. Adding a test case for SecurityProtocol/TLS/SASL here would prevent regressions and keep runner behavior aligned with the k8s deployer tests.
if k.SecurityProtocol != "" && k.SecurityProtocol != "PLAINTEXT" {
env = append(env, "KAFKA_SECURITY_PROTOCOL="+k.SecurityProtocol)
}
if k.TLS != nil {
if k.TLS.CACert != "" {
env = append(env, "KAFKA_TLS_CA_CERT="+k.TLS.CACert)
}
if k.TLS.ClientCert != "" {
env = append(env, "KAFKA_TLS_CLIENT_CERT="+k.TLS.ClientCert)
}
if k.TLS.ClientKey != "" {
env = append(env, "KAFKA_TLS_CLIENT_KEY="+k.TLS.ClientKey)
}
if k.TLS.SkipVerify {
env = append(env, "KAFKA_TLS_SKIP_VERIFY=true")
}
}
if k.SASL != nil {
if k.SASL.Mechanism != "" {
env = append(env, "KAFKA_SASL_MECHANISM="+k.SASL.Mechanism)
}
if k.SASL.User != "" {
env = append(env, "KAFKA_SASL_USER="+k.SASL.User)
}
if k.SASL.Password != "" {
env = append(env, "KAFKA_SASL_PASSWORD="+k.SASL.Password)
}
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (7)
pkg/functions/runner.go:324
- The host runner's new TLS/SASL environment branches are untested even though
buildRunnerEnvalready has focused Kafka tests. Add cases covering protocol, TLS fields/skip verification, SASL credentials, and explicitPLAINTEXTprecedence over an inherited protocol.
if k.TLS != nil {
if k.TLS.CACert != "" {
env = append(env, "KAFKA_TLS_CA_CERT="+k.TLS.CACert)
}
if k.TLS.ClientCert != "" {
pkg/functions/function.go:247
- SASL protocols can currently pass validation with no
saslblock, or with an empty user/password, even though the dependent runtime requires both credentials and returns an error at startup otherwise. Require the SASL block and both credential fields whenever a SASL protocol is selected.
if kafka.SASL != nil {
if kafka.SecurityProtocol != "SASL_PLAINTEXT" && kafka.SecurityProtocol != "SASL_SSL" {
errors = append(errors, "run.kafka.sasl requires securityProtocol SASL_PLAINTEXT or SASL_SSL")
}
validMechanisms := map[string]bool{"": true, "PLAIN": true, "SCRAM-SHA-256": true, "SCRAM-SHA-512": true}
pkg/functions/runner.go:319
- An explicit
securityProtocol: PLAINTEXTis not added to the host process environment. BecausebuildRunnerEnvstarts withos.Environ(), an inheritedKAFKA_SECURITY_PROTOCOLsuch asSASL_SSLremains effective and changes the configured function's protocol. Emit every explicitly configured protocol; only omit the empty default.
This issue also appears on line 320 of the same file.
if k.SecurityProtocol != "" && k.SecurityProtocol != "PLAINTEXT" {
env = append(env, "KAFKA_SECURITY_PROTOCOL="+k.SecurityProtocol)
}
pkg/functions/function.go:241
- A configuration with only one of
clientCertorclientKeypassesFunction.Validate, but the Kafka runtime rejects that pair before connecting. Validate that these mutual-TLS fields are either both set or both empty so deployment fails early with a configuration error.
This issue also appears on line 243 of the same file.
if kafka.TLS != nil {
if kafka.SecurityProtocol != "SSL" && kafka.SecurityProtocol != "SASL_SSL" {
errors = append(errors, "run.kafka.tls requires securityProtocol SSL or SASL_SSL")
}
}
pkg/k8s/deployer.go:753
- An explicit
securityProtocol: PLAINTEXTis omitted here, so a conflictingKAFKA_SECURITY_PROTOCOLfromrun.envsor the image remains effective. This also makes the dedicated Kafka field overriderun.envsfor secure protocols but not forPLAINTEXT; emit every non-empty configured protocol.
if kafka.SecurityProtocol != "" && kafka.SecurityProtocol != "PLAINTEXT" {
envVars = append(envVars, corev1.EnvVar{Name: "KAFKA_SECURITY_PROTOCOL", Value: kafka.SecurityProtocol})
}
pkg/docker/runner.go:311
- An explicit
securityProtocol: PLAINTEXTis omitted here, so a conflictingKAFKA_SECURITY_PROTOCOLfromrun.envsor the image remains effective. This also makes dedicated Kafka configuration overriderun.envsfor secure protocols but not forPLAINTEXT; emit every non-empty configured protocol.
if k.SecurityProtocol != "" && k.SecurityProtocol != "PLAINTEXT" {
c.Env = append(c.Env, "KAFKA_SECURITY_PROTOCOL="+k.SecurityProtocol)
}
pkg/k8s/deployer_test.go:598
- This loop only checks an env var when it happens to be present, so the test still passes if either
KAFKA_SASL_USERorKAFKA_SASL_PASSWORDis omitted. Look up both names explicitly and fail when either is missing before checking itsSecretKeyRef.
for _, ev := range got {
if ev.Name == "KAFKA_SASL_USER" {
if ev.ValueFrom == nil || ev.ValueFrom.SecretKeyRef == nil {
t.Fatal("KAFKA_SASL_USER should have ValueFrom with SecretKeyRef")
}
|
I tested the instructions manually. All works. |
781ffef to
1248c19
Compare
|
knative-extensions/func-go#186 is merged and this one is updated to use the newly released func-go. |
653c46a to
93c649c
Compare
gauron99
left a comment
There was a problem hiding this comment.
Me and my bot friends found only some minor items
There was a problem hiding this comment.
🟡 Changes recommended
Validation currently accepts missing SASL credentials and incomplete mutual-TLS certificate pairs that fail during runtime startup.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 14/19 changed files
- Comments generated: 2
- Review effort level: Balanced
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aliok, gauron99 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
just to note here we are requiring some values like kafka sasl user and password to be passed via the func.yaml. if theyre not the validator block wont pass. therefore using the envs so Currently its kept as is and required to use the This is a TP feature and I suspect we will iterate on this a bit more, any cleanups or more clarity might come from that |
Summary
KafkaConfigwithsecurityProtocol,tls, andsaslfields in func.yamlKAFKA_SECURITY_PROTOCOL,KAFKA_TLS_*,KAFKA_SASL_*env vars{{ secret:name:key }}syntax supported forsasl.userandsasl.passwordDepends on knative-extensions/func-go#186
func.yaml example (SASL_SSL)
Verification instructions (Kind + Strimzi)
Prerequisites
1. Build the func CLI
Both repos have un-merged branches. Build the CLI from the
kafka-tls-saslbranch:2. Patch the scaffolding to use the func-go fork
The func-go dependency lives in the scaffolding's
go.mod(embedded in the CLI), not the function'sgo.mod. Add areplacedirective, re-tidy, regenerate the embedded filesystem, and rebuild:3. Create a Kind cluster with Knative
4. Install Strimzi with a TLS+SASL listener
5. Create a KafkaUser and topic
6. Create the function
7. Configure func.yaml
Copy secrets to the function namespace:
Edit
func.yaml:8. Deploy and verify
FUNC_REGISTRY=ttl.sh/my-kafka-tls-test /tmp/func-local deploy --build --verbose kubectl wait pods -l serving.knative.dev/service=my-kafka-tls-func \ --for=condition=Ready --timeout=120sCheck env vars on the pod:
9. Tail logs and send a test message
Expected log output:
Cleanup
kubectl delete ksvc my-kafka-tls-func kubectl delete secret my-cluster-cluster-ca-cert my-kafka-user -n default kubectl delete kafkauser my-kafka-user -n kafka kubectl delete kafkatopic test-topic -n kafka kubectl delete kafka my-cluster -n kafka kubectl delete -f 'https://strimzi.io/install/latest?namespace=kafka' -n kafka kubectl delete namespace kafka kind delete cluster --name kafka-tls-test rm -rf /tmp/my-kafka-tls-func /tmp/func-local