capture+materialize: Support Nonsensitive Field Overlays#3119
capture+materialize: Support Nonsensitive Field Overlays#3119willdonnelly wants to merge 14 commits into
Conversation
6f50475 to
3fd8931
Compare
|
I believe this is ready for review now. I've tested locally that with these changes a source-hello-world->materialize-postgres pipeline can be set up as normal when not using an overlay, and that an overlay can be added without re-encrypting a task config and (with a connector modified to mark those fields as nonsensitive) will be applied to the effective task config. I have not specifically tested this with both the v1 and v2 runtimes because I don't know how to toggle between those, but the actual code changes are basically identical between the two so I think that's probably sufficient testing just to make sure I didn't break something dumb but subtle. |
|
Hmm, the "Dekaf Test" CI failure appears to be because the |
This is necessary so that builds of this connector pick up the "tolerate unknown JSON fields" change from Flow commit 184db0e, which in turn is necessary so that Flow CI tests of estuary/flow#3119 can pass since the Dekaf test uses `source-http-ingest:dev` and currently it errors out because that PR adds a new field which it doesn't recognize.
3fd8931 to
9dc610a
Compare
williamhbaker
left a comment
There was a problem hiding this comment.
LGTM, no blockers, a couple of things to consider as comments, and also concurring deployment:
As is, this PR will start to populate the new sealed config field right away. We've been bitten numerous times by new field additions where consumers use the strict pbjson structs that disallow unknown fields. As you noted with source-http-ingest, it hadn't yet been updated with the recent move to more lenient pbjson structs that do allow unknown fields. Anything else that hasn't yet been updated will similarly crash if it sees these populated fields.
Off the top of my head, that would be the other rust connectors: source/materialize-kafka, and all the ATF connectors (at least, I don't remember seeing us having updated those recently, but I might have missed it).
Since the new field only shows up on Open messages it is just limited to consumers of Open messages, which is only connectors.
I'm betting this would also break flowctl preview and flowctl raw materialize-fixture unless an up-to-date flowctl is used, built from at least this commit or later. This is a practical concern for connectors repo integration tests, unless they are always pulling/building the latest flowctl.
| Ok(()) | ||
| } | ||
|
|
||
| fn display_ptr(ptr: &json::Pointer) -> String { |
There was a problem hiding this comment.
Any reason not use the Display already implemented by json::Pointer?
There was a problem hiding this comment.
The only functional difference I can see is that using Display an overlay which attempts to modify the root object would produce an error message like overlay modifies location , which is not marked nonsensitive (since the RFC 6901 representation of the root is the empty string) whereas this helper function special-cased the root as / for readability in the error message.
That probably isn't sufficient reason to reimplement a hackier version of JSON Pointer stringification so I've removed this helper.
There was a problem hiding this comment.
Actually that's a dumb objection anyway, I've added single-quotes so that error would be overlay modifies location '', which is not marked nonsensitive which makes it pretty unambiguous to me.
| fn empty_overlay_is_a_noop() { | ||
| check(json!({})).unwrap(); | ||
| // An empty object writes nothing even at an otherwise-sensitive location. | ||
| check(json!({"credentials": {}})).unwrap(); |
There was a problem hiding this comment.
I can't think of a reason why this would need fixing, but if I'm understanding this correctly an empty object would kind of write something to a sensitive location if it were not of type object (like a string), replacing it instead with an object. In any sane world the resulting config would fail to parse in the connector, or otherwise not have any kind of exposure risk.
There was a problem hiding this comment.
The worst I can think of is a connector that had something like tls: true as a sensitive field and then somehow it thought tls: {} meant false 🤷
There was a problem hiding this comment.
Good catch, yeah, an empty object (or even a nested chain of objects as long as the final one is empty) passes the current nonsensitive-property validation but it probably shouldn't since merge(scalar, object) = merge({}, object).
I think to close that gap the validation rule needs to be something like "we only recurse into an object if the target schema permits an object at this location" and an object in a scalar location needs to be rejected.
|
Am I right in my understanding that this allows connectors to emit an update to a non-encrypted field (e.g. |
Thanks, I believe those should be addressed now.
Is your recommendation to split this up into separate pieces, adding the field and making sure the connectors pick that up and then populating it in a separate PR once that's done, or are you just calling out that we should make sure everything's picked up the recent allow-unknown-fields change before merging this? If it's the latter, I think the current state of things is:
TL;DR it looks like only ATF connectors still need any work, we would need to bump their Flow version pin and make sure they're all released before merging this.
The integration tests do actually pull down the latest flowctl ( |
Yeah I think making sure everything's picked up the allow-unknown-fields change would be the practical way to go about it. Doesn't seem like there's much additional needed for that.
I was probably wrong about this actually. I was thinking that since |
Okay, thanks. I've uploaded estuary/airbyte#360 for the ATF connectors and will make sure that's merged and pushed to all 40-something of them before this gets merged. |
Add the field to the capture Open RPC and thread it through all construction and destructure sites, which default or ignore it. It's actually populated in a later commit. The regenerated protobuf bindings follow in the next commit.
Checked-in output of the Rust and Go protobuf generators for the preceding capture Open change. No hand-written edits.
Defer endpoint config decryption until after the connector's spec response is available, capturing the sealed config from the matched endpoint. This prepares for overlay-aware decryption, which needs the connector's config schema to validate overlay property non-sensitivity.
Decrypt endpoint configuration with unseal::overlay::decrypt_with_overlay, which applies sops.overlay properties subject to config schema validation.
Thread the sealed endpoint configuration into the Open request's sealed_config_json field, so running capture connectors can emit a configUpdate which adjusts their nonsensitive sops.overlay without re-encrypting the whole configuration.
Add the field to the materialize Open RPC and thread it through all construction and destructure sites, which default or ignore it. It's actually populated in a later commit. The regenerated protobuf bindings follow in the next commit.
Checked-in output of the Rust and Go protobuf generators for the preceding materialize Open change. No hand-written edits.
Defer endpoint config decryption until after the connector's spec response is available, capturing the sealed config from the matched endpoint. This prepares for overlay-aware decryption, which needs the connector's config schema to validate overlay property non-sensitivity. Dekaf decrypts its own nested config outside this path, so its sealed config is None and nothing is decrypted here.
Decrypt endpoint configuration with unseal::overlay::decrypt_with_overlay, which applies sops.overlay properties subject to config schema validation.
Thread the sealed endpoint configuration into the Open request's sealed_config_json field, so running materialization connectors can emit a configUpdate which adjusts their nonsensitive sops.overlay without re-encrypting the whole configuration. Dekaf has no sealed config on this path, so its Open is left unpopulated.
Removes an unnecessary `display_ptr` helper function and tweaks the nonsensitivity validation logic to handle an edge case around empty objects in the overlay correctly.
Part of estuary/flow#2985, we need all connectors to have picked up the "tolerate unknown fields" change from Flow commit 184db0e before merging estuary/flow#3119 which introduces a new sealed endpoint config property to the Open RPCs. As far as I'm aware everything else either has this change or didn't need it, but airbyte-to-flow connectors still need it.
This could probably be folded into a prior commit but I'm just keeping up with codebase churn so the PR can land cleanly.
fb74e33 to
3ed5f64
Compare
Description:
This PR:
nonsensitive: trueJSON schema annotation for endpoint configs.unseal::decrypt_with_overlayhelper function which validates overlay non-sensitivity and does the extract-decrypt-merge dance around overlay properties and the main config.sealed_config_jsonfields to theOpenRPCs in the capture and materialization protocols, which will be used to provide the encrypted endpoint config to connectors. Currently they only receive the decrypted config, but in order to produceconfigUpdates which modify their own configuration via the overlays they'll need access to the raw encrypted one too.unseal::decrypt_sopsforunseal::decrypt_with_overlayin those places. This is basically the followup to the previous refactor, I just found it cleaner to make them separate commits.sealed_config_jsonproperty of Open RPCs.Since I'm not super familiar with the runtime codebase I've broken this down into fairly small, atomic commits so that I could review them individually and be pretty confident they're correct.
Part of #2985
Workflow steps:
Nothing really changes. In theory someone could manually produce a task with an endpoint overlay but nothing will be generating them automatically yet, because we need the runtime to fully support this before we start using the feature.