Update proxy to preserve external payload details #231
Conversation
|
|
| return status.ErrorProto(p) | ||
| } | ||
| func preserveExternalPayloadsVisitor(visitor PayloadsVisitor) PayloadsVisitor { |
There was a problem hiding this comment.
Hrmm. We usually allow users that implement encryption to encrypt the entire payload proto structure without leaking anything about the contents of the structure, but I understand we want to start allowing leaking of this field. Are we also adjusting all SDKs to also leak this data when a codec is applied? I think in both situations, we should at least provide an opt-out.
I also think an SDK implementation first can give us good guidance on how we want to approach propagation of this data across codecs instead of this proxy lib first.
| } | ||
| if len(payloads) != len(newPayloads) { | ||
| return newPayloads, fmt.Errorf("expected payload count %d but received %d", len(payloads), len(newPayloads)) |
There was a problem hiding this comment.
Our system does not require this, it is plenty common for a user to turn many payloads into a single one in one direction (e.g. encryption) and one to many in the other (e.g. decryption). There is no way to get the translation exactly right (on purpose, arity is a user choice and none of our business).
However, for many cases (probably like 95% or something) the payload counts will be the same. So the algorithm can probably just be to put extra external payload details on final payload post-visitor if they were on an index past the end of the new set.
This will of course cause the situation where 5 payloads w/ 1 external detail each -> encryption -> 1 payload with 5 external details -> decryption -> 5 payloads, but first w/ 5 external details, others with 0.
I think this may deserve more thinking about where to put payload telemetry and whether it needs to be part of the initial phase.
There was a problem hiding this comment.
Ok, per off-PR discussion, the logic for transferring these stats will actually be on Go SDK side where gRPC interceptors are created.
| if reqMsg, ok := req.(proto.Message); ok && options.Outbound != nil { | ||
| err := VisitPayloads(ctx, reqMsg, *options.Outbound) | ||
| outboundVisitorOptions := VisitPayloadsOptions{ | ||
| Visitor: preserveExternalPayloadsVisitor(options.Outbound.Visitor), |
There was a problem hiding this comment.
IMO making our own visitor isn't the best way to do this. There are only three spots where a user's visitor is called, it's at invocation time there we should add this logic IMO (we can reduce those spots down to one via a new function if needed).
3 participants
What changed?
Updated the go proxy to preserve the external payload details information on payloads.
Why?
Consumers of the proxy may create encrypted payloads from the original payloads with preserving the external payload details, which would prevent it from being usable by CLI/Server/UI.
How did you test it?
Add extra tests to validate the preservation of the external payload details.
Potential risks
The internal preservation visitor validates that the number of payloads that were generated by the provided visitor is the same length that was provided to the visitor originally. This is necessary for keeping track on which external payload details copies belong to which payload returned by the visitor. It is technically possible that existing visitor implementations change the number of payloads as part of their visitation routine, but this should be unlikely and probably would have caused downstream problems when the transformed payloads were consumed.