Skip to content

Preserve unknown transport configurations#6610

Draft
xiasongh wants to merge 1 commit into
feldera:mainfrom
xiasongh:shuang/custom-connector-runtime-compat
Draft

Preserve unknown transport configurations#6610
xiasongh wants to merge 1 commit into
feldera:mainfrom
xiasongh:shuang/custom-connector-runtime-compat

Conversation

@xiasongh

@xiasongh xiasongh commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

If we create a new custom runtime that adds a new transport config, the pipeline manager will not recognize and reject the new config. This makes custom runtimes that add new connectors difficult to use.

With this implementation, we would have to rely on the compiler for validation, and the pipeline manager can't differentiate between unknown transports and known transports with invalid configs.

Describe Manual Test Plan

Wrote scripts test_custom_runtime_transport that:

  1. Create a runtime-only Datagen alias in a local clone
  2. Start an unmodified pipeline manager
  3. Compile a pipeline against a cloned runtime revision
  4. Verify that the pipeline processes data successfully

Checklist

  • Unit tests added/updated
  • Integration tests added/updated
  • Documentation updated
  • Changelog updated

Breaking Changes?

Mark if you think the answer is yes for any of these components:

Describe Incompatible Changes

@mihaibudiu

Copy link
Copy Markdown
Contributor

There is an alternative solution to make everything dynamic: #6522
We should be moving in that direction, eventually allowing dynamic linking of connectors.
This may work as a temporary solution, though.

@mythical-fred mythical-fred left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

High-level, since this is still draft.

Nice motivation — an older manager needs to forward transports it does not yet know about. The mechanism (a fallback Unknown { name, config } variant + a Modify hook to strip it from OpenAPI) is clean, and the shell/pytest cross-version harness is a nice touch. A few things worth resolving before this leaves draft:

  1. Please verify malformed known configs still produce a good error. TransportConfig is adjacently-tagged (#[serde(tag = "name", content = "config", …)]) with Unknown marked #[serde(untagged)]. That mixture is unusual, and serde's untagged fallback can eat the specific error from a matched tag when a sibling variant is untagged. Please add a test that submits e.g. {"name":"kafka_input","config":{ "bootstrap_servers": 42 }} and asserts the user gets a kafka_input-specific parse error, not a silent promotion to Unknown { name: "kafka_input", … }. If it does fall through, the fix is to make Unknown only match when name is not one of the known snake-case tags (custom Deserialize that checks the tag against a fixed set before falling back).

  2. Gate the acceptance path on runtime_version explicitly. The test script runs the manager with FELDERA_UNSTABLE_FEATURES=runtime_version, but the config change itself is unconditional — any client can now submit an arbitrary {"name":"whatever","config":…} transport and the manager will store it. If custom-runtime forwarding is meant to require an explicit runtime version, the API layer should reject Unknown variants when runtime_version is unset (or when the effective runtime version equals the manager's own build). Otherwise the "invalid transport" ergonomic in point 1 gets worse.

  3. OpenAPI HideUnknownTransport is position-dependent. schema.items.pop() works only if Unknown is the last TransportConfig variant. The test enforces that indirectly by checking len == 24 and !contains("unknown"), but a future variant added below Unknown silently corrupts the schema. Consider popping by matching the schema for the Unknown shape rather than by index, or at least add an inline debug_assert! in modify that the popped schema is the Unknown one.

  4. name() for Unknown returns the raw name. Fine, but downstream code paths that log/format transport names may now surface arbitrary user-controlled strings; make sure log-injection isn't a concern (probably fine given they go into structured tracing fields, but worth a look).

  5. Minor: manual test-plan shell script is nice, but consider whether it should also assert that a malformed known config (e.g. kafka_input with a bogus field) still produces a specific parse error rather than being silently promoted to Unknown — that's a good regression fence for point 1.

I'll do a proper line-by-line pass once you mark it ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants