Preserve unknown transport configurations#6610
Conversation
|
There is an alternative solution to make everything dynamic: #6522 |
mythical-fred
left a comment
There was a problem hiding this comment.
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:
-
Please verify malformed known configs still produce a good error.
TransportConfigis adjacently-tagged (#[serde(tag = "name", content = "config", …)]) withUnknownmarked#[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 akafka_input-specific parse error, not a silent promotion toUnknown { name: "kafka_input", … }. If it does fall through, the fix is to makeUnknownonly match whennameis not one of the known snake-case tags (customDeserializethat checks the tag against a fixed set before falling back). -
Gate the acceptance path on
runtime_versionexplicitly. The test script runs the manager withFELDERA_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 rejectUnknownvariants whenruntime_versionis unset (or when the effective runtime version equals the manager's own build). Otherwise the "invalid transport" ergonomic in point 1 gets worse. -
OpenAPI
HideUnknownTransportis position-dependent.schema.items.pop()works only ifUnknownis the lastTransportConfigvariant. The test enforces that indirectly by checkinglen == 24and!contains("unknown"), but a future variant added belowUnknownsilently corrupts the schema. Consider popping by matching the schema for theUnknownshape rather than by index, or at least add an inlinedebug_assert!inmodifythat the popped schema is theUnknownone. -
name()forUnknownreturns 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). -
Minor: manual test-plan shell script is nice, but consider whether it should also assert that a malformed known config (e.g.
kafka_inputwith a bogus field) still produces a specific parse error rather than being silently promoted toUnknown— that's a good regression fence for point 1.
I'll do a proper line-by-line pass once you mark it ready.
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_transportthat:Datagenalias in a local cloneChecklist
Breaking Changes?
Mark if you think the answer is yes for any of these components:
Describe Incompatible Changes