Support Kafka schema registry config#847
Conversation
|
@jiangpengcheng:Thanks for your contribution. For this PR, do we need to update docs? |
freeznet
left a comment
There was a problem hiding this comment.
Thanks for adding first-class Schema Registry support. The overall shape looks good: types + deepcopy + CRDs are consistent, the OAuth2 mount reuses the existing generateVolumeFromOAuth2Config helpers (volume name = secretName-secretKey, so a broker/SR shared secret collapses to one volume mounted at two paths — correct), and the webhook validation + unit tests are solid.
A couple of things worth confirming before merge (details inline):
- Basic-auth
${ENV}interpolation insideproducer_config— this is the first place the operator embeds${...}placeholders into the_kafka_config.producer_configJSON rather than into a shell command, so it depends on the generic runtime expanding env vars in producer config values. Worth confirming. - Minor: the OAuth2 sub-fields aren't validated in the webhook (relies on CRD
requiredmarkers) — confirming that's intentional.
Nits (non-blocking): schema.registry.oauth2.config always serializes "scope":"" when scope is unset (consistent with the existing broker OAuth2 mapping, so fine); the PR carries the doc-info-missing label — user-facing docs for the new schemaRegistry fields should follow; and the Go 1.25.11 -> 1.25.12 bump is unrelated housekeeping bundled in here.
|
@codex review this |
There was a problem hiding this comment.
Pull request overview
This PR adds first-class Schema Registry configuration to the Kafka messaging spec and wires it into the generic runtime’s generated Kafka producer config, including support for Basic Auth (via env vars from a Secret) and OAuth2 (via a dedicated Secret mount). It also updates generated CRDs/deepcopy code and bumps Go/tooling versions used in builds and CI.
Changes:
- Add
spec.kafka.schemaRegistryAPI fields + CRD schema, with webhook validation for required/compatible Schema Registry settings. - Map Schema Registry URL + auth settings into
_kafka_config.producer_configfor the generic runtime; mount OAuth2 credentials and expose Basic Auth via env vars. - Bump Go version in Dockerfiles/CI and refresh Go module indirect dependencies.
Reviewed changes
Copilot reviewed 30 out of 32 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| redhat.Dockerfile | Bumps Go builder image version. |
| operator.Dockerfile | Bumps Go builder image version. |
| Dockerfile | Bumps Go builder image version. |
| images/samples/go-function-samples/Dockerfile | Bumps Go builder image version for samples. |
| images/samples/go-function-samples/func/go.mod | Updates sample module’s go version. |
| go.mod | Updates indirect golang.org/x/* dependency versions. |
| go.sum | Updates dependency checksums to match module updates. |
| api/compute/v1alpha1/common.go | Introduces KafkaSchemaRegistryConfig (+ auth subtypes) and adds it to KafkaMessaging. |
| api/compute/v1alpha1/zz_generated.deepcopy.go | Regenerates deepcopy functions for the new Schema Registry API types. |
| pkg/webhook/validate.go | Adds Schema Registry validation to function Kafka messaging validation. |
| pkg/webhook/validate_test.go | Adds tests covering Schema Registry validation scenarios. |
| controllers/spec/common.go | Adds constants for Schema Registry OAuth2 mount path and Basic Auth env var names. |
| controllers/spec/utils.go | Maps Schema Registry config into _kafka_config.producer_config. |
| controllers/spec/utils_test.go | Adds tests asserting correct producer config mapping for Basic Auth and OAuth2. |
| controllers/spec/function.go | Adds Schema Registry OAuth2 volumes/mounts and Basic Auth env var injection for the function container. |
| controllers/spec/function_test.go | Adds tests for Schema Registry env vars (Basic) and OAuth2 mount presence. |
| config/crd/bases/compute.functionmesh.io_sources.yaml | Adds schemaRegistry section under Kafka messaging in the CRD schema. |
| config/crd/bases/compute.functionmesh.io_sinks.yaml | Adds schemaRegistry section under Kafka messaging in the CRD schema. |
| config/crd/bases/compute.functionmesh.io_functions.yaml | Adds schemaRegistry section under Kafka messaging in the CRD schema. |
| config/crd/bases/compute.functionmesh.io_functionmeshes.yaml | Adds schemaRegistry section under Kafka messaging in the CRD schema (all relevant schema copies). |
| charts/function-mesh-operator/charts/admission-webhook/templates/crd-compute.functionmesh.io-sources.yaml | Mirrors CRD schema update in Helm chart template. |
| charts/function-mesh-operator/charts/admission-webhook/templates/crd-compute.functionmesh.io-sinks.yaml | Mirrors CRD schema update in Helm chart template. |
| charts/function-mesh-operator/charts/admission-webhook/templates/crd-compute.functionmesh.io-functions.yaml | Mirrors CRD schema update in Helm chart template. |
| charts/function-mesh-operator/charts/admission-webhook/templates/crd-compute.functionmesh.io-functionmeshes.yaml | Mirrors CRD schema update in Helm chart template. |
| .github/workflows/trivy.yml | Bumps CI Go version. |
| .github/workflows/trivy_scheduled_master.yml | Bumps CI Go version. |
| .github/workflows/test-integration-skywalking-e2e.yml | Bumps CI Go version. |
| .github/workflows/test-helm-charts.yml | Bumps CI Go version. |
| .github/workflows/release.yml | Bumps CI Go version. |
| .github/workflows/project.yml | Bumps matrix Go version. |
| .github/workflows/olm-verify.yml | Bumps CI Go version. |
| .github/workflows/bundle-release.yml | Bumps CI Go version in bundle release jobs. |
Files not reviewed (1)
- api/compute/v1alpha1/zz_generated.deepcopy.go: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ed27bad33
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
freeznet
left a comment
There was a problem hiding this comment.
Review: Support Kafka schema registry config
Thanks for the change — the core wiring looks correct and is well covered by tests. A summary of my feedback; details are in the inline comments.
Correctness (looks good)
validateKafkaSchemaRegistryis placed so it always runs whenspec.Kafkais set, correctly rejects setting both Basic and OAuth2 auth, and enforces the required OAuth2 fields.- Reusing
generateVolumeFromOAuth2Config(volume name =secretName+key) with a dedicated mount path (/etc/oauth2-kafka-schema-registry) is safe even when the broker and Schema Registry share the same OAuth2 secret:appendVolumeMountIfNotExistsdedups by (name, path, subPath), so both mount paths are preserved.
Main question
- Schema Registry config is only merged into
producer_config. Input specs can carry Schema Registry coordinates (subject/version), so please confirm the consumer side doesn't also need it — see inline oncontrollers/spec/utils.go.
Scope / process (non-blocking)
- This PR also bumps Go
1.25.11 → 1.25.12across all workflows + Dockerfiles and severalgolang.org/x/*indirect deps. These are unrelated to the feature; consider splitting them into a separate maintenance PR to keep the history focused. - The
doc-info-missinglabel is set — the newschemaRegistryAPI fields don't appear to be documented for users yet.
Nits
- See inline on
api/compute/v1alpha1/common.go(undocumented default keys +omitemptyon a required field).
Overall looks good to me once the consumer-side question is resolved.
freeznet
left a comment
There was a problem hiding this comment.
Re-review (head e0e44eb0)
Thanks for the quick turnaround — I re-checked the delta since my last review and everything I raised is addressed correctly and well covered by tests. No outstanding change requests from my side.
Verified
- Consumer-side config —
addKafkaSchemaRegistryConfig(consumerConfig, kafka.SchemaRegistry)is now applied alongside the producer path, andutils_test.goasserts bothconsumer_configandproducer_configcarry the URL,USER_INFOsource, the env-backeduser.info, and the OAuth2 config. Given the runtime stripsschema.registry.*keys before building the Kafka clients, mirroring them on both sides is the right call. ✅ common.gonits —omitemptyremoved from the requiredsecretName;usernameKey/passwordKeynow carry godoc ++kubebuilder:default, and the defaults (username/password) are reflected consistently in bothconfig/crdand the Helmadmission-webhookcopies. ✅- Also confirms the earlier OAuth2 webhook validation and the
USER_INFOcredentials-source fix are present in the current head.
Non-blocking
- The Go
1.25.11 → 1.25.12bump +golang.org/x/*dependency updates (thefix vulnerabilitycommit) are still bundled. Unrelated to the feature; fine to keep if CI needs it, but a separate maintenance PR would keep this one focused. doc-info-missinglabel: there's no in-repo docs location for the Kafka messaging fields today (even the existing ones aren't documented underdocs/), so this looks like the template default — just worth making sure the external docs get the newschemaRegistrysection.
Code LGTM. Leaving the approve/merge call to the maintainers.
Summary
schemaRegistrysection under Kafka messaging config for generic functions_kafka_config.producer_configfor the generic runtimeDetails
The generic runtime expects Schema Registry settings in Kafka producer config using the
schema.registry.*keys. This change lets users configure those settings with first-class Function Mesh fields instead of embedding them manually in producer config. Basic auth credentials are sourced from a Kubernetes secret, and OAuth2 credentials are mounted separately from the Kafka broker OAuth2 mount path.Testing
go test -count=1 ./controllers/spec ./pkg/webhookgit diff --check