💥 Payload limit enforcement#1363
Conversation
c42f207 to
aebcc45
Compare
aebcc45 to
a9f694c
Compare
jmaeagle99
left a comment
There was a problem hiding this comment.
Some self-review comments. If you have ideas/opinions, would love to hear them.
| pub struct PayloadLimitsOptions { | ||
| /// Warning threshold (bytes) for the size of an outbound payload-bearing field; over-threshold | ||
| /// fields are logged but still sent to server. Defaults to 512 KiB. Set to `0` to disable. | ||
| pub payloads_size_warn: u64, |
There was a problem hiding this comment.
I might flip the naming of these fields to:
payloads_warn_sizememo_warn_size
I think it would allow better grouping of related settings together if we ever need to add more for each severity level e.g.payload_warn_fancy_featureand I think it reads better when saying them because there aren't two S's next to each other like there is inpayloads_size_warn.
This suggestion is contrary to what we did for Python and Go. Also note that the payloads field is plural here where it is singular in Python and Go; I intentionally did that since the sizing is applied to payload-bearing fields, which include single payload fields, repeated payload fields (a sequence), and the Payloads proto message.
So options are:
- Leave as-is.
- Match Python and Go naming for consistency sake.
- Rename to
payloads_warn_sizeandmemo_warn_sizefor (IMO) maringally better naming and conceptualization. - Some other better suggestion.
Naming is hard.
| F: Send + Sync + Unpin + 'static, | ||
| { | ||
| // Validate payload sizes after any request mutation but before encoding/metrics. | ||
| validate_request_payload_limits( |
There was a problem hiding this comment.
This is situated right before the gRPC message is sent so that it is the last thing in the gRPC client chain that could modify payloads and memos. Things I need to check here are:
- This needs to be checked before gRPC request metrics are emitted so that we aren't double counting attempts e.g. a WFT completion that is rejected due to payload sizes should not emit a gRPC request metric for that message.
- I recall there is a grpc override callback. This probably should occur before that callback is invoked.
| /// Per-call payload/memo size error limits, attached to a request's extensions by a caller that | ||
| /// wants error-level enforcement on this call. | ||
| #[derive(Debug, Clone, Copy)] | ||
| pub struct PayloadErrorLimits { | ||
| /// Blob (payload) size error threshold, in bytes. | ||
| pub blob: usize, | ||
| /// Memo size error threshold, in bytes. | ||
| pub memo: usize, | ||
| } |
There was a problem hiding this comment.
Nit: There is request_extensions.rs that this could go in, but, I'm fine with it here too.
| pub struct PayloadLimitsClient<C> { | ||
| inner: C, | ||
| error_limits: Arc<RwLock<Option<PayloadErrorLimits>>>, | ||
| } |
There was a problem hiding this comment.
I think this should probably be an interceptor, rather than a client wrapper?
There was a problem hiding this comment.
There are a couple of issues that I think prevent this from being an interceptor:
- tonic interceptors receive type-erased request bodies. We need the typed body to decide if it should be validated and how. I do not think there is a technical workaround as an interceptor. It could be done as a tower layer, but that would require decoding the already encoded body, which seems wasteful.
- The connection is namespace unaware. I don't think we'd be able to construct the interceptor at channel creation time and be able to plumb through namespace limit information. We'd have to mutate the interceptor later but not for the client because that'll lead to inconsistent behavior depending on when a worker is attached to the channel. We could copy of the channel for each worker, but then we lose pooling/sharing.
We could eliminate this wrapper and just have the call sites attach the error limit information per caller; that would be ripe for forgetting to do that for new callers, but its workable and could be made with a simple convenience method.
If opposed to the client wrapper, I could do the per-call site attachment.
There was a problem hiding this comment.
Ah, yeah, good points. I forgot that there isn't really a kind of interceptor that would allow us to have all that in one place.
I suppose my only other comment is I don't think this needs to be fully pub?
| /// The violation, if `status` is the client proactively rejecting an outbound request for exceeding a | ||
| /// payload/memo error size limit. |
There was a problem hiding this comment.
| /// The violation, if `status` is the client proactively rejecting an outbound request for exceeding a | |
| /// payload/memo error size limit. | |
| /// Returns the violation, if `status` is the client proactively rejecting an outbound request for exceeding a | |
| /// payload/memo error size limit. |
| pub struct PayloadLimitsOptions { | ||
| /// Warning threshold (bytes) for the size of an outbound payload-bearing field; over-threshold | ||
| /// fields are logged but still sent to server. Defaults to 512 KiB. Set to `0` to disable. | ||
| pub payloads_size_warn: u64, |
| /// Warn/error thresholds for both limit classes. A `None` threshold disables that check for that | ||
| /// class: `None` warn = no warnings, `None` error = no error enforcement (warnings only). | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] | ||
| pub struct PayloadLimits { |
There was a problem hiding this comment.
If the resolver thing earlier resolves 0 to None, I'd kinda vote for these just being 0 also means disabled? 0-but-respected seems like a bit of a nonsense state
| Err(e) if payload_limit_violation_from(&e).is_some() => { | ||
| let violation = payload_limit_violation_from(&e) | ||
| .expect("violation present per guard"); |
There was a problem hiding this comment.
I want to say this can be combined with an if-let (forget if those work in match arms)
| Err(e) if payload_limit_violation_from(&e).is_some() => { | ||
| let violation = payload_limit_violation_from(&e) |
| Err(e) => { | ||
| if let Some(violation) = payload_limit_violation_from(&e) { | ||
| act_metrics.act_execution_failed(); | ||
| client |
There was a problem hiding this comment.
Does this make sense? Should we instead just truncate the cancel details or something?
| /// Issues outbound gRPC calls, automatically attaching this worker's payload/memo error limits | ||
| /// (set via `set_payload_error_limits`) so the gRPC layer can enforce them. Wraps a clone of | ||
| /// `connection`, so a client replacement on `connection` is reflected here too. | ||
| client: PayloadLimitsClient<SharedReplaceableClient<Connection>>, |
There was a problem hiding this comment.
Per my bit about interceptors, I think ideally we wouldn't need to store a separate client here..
| task_queue=%self.config.task_queue, worker_id=%self.client.identity(), | ||
| workflow_id, run_id, workflow_type, activity_type, attempt))] |
There was a problem hiding this comment.
Adding these additional attributes could be nice, could also be noisy, was this to match other SDKs?
What was changed
Builds on the existing payload-field classification codegen to add end-to-end payload/memo
size-limit enforcement in the client and worker.
PayloadLimitViolationas its source.ConnectionOptionsaspayload_limits PayloadLimitsOptionsfield. Default is to warn payloads at 512 KiB and memos at 2 KiB, same defaults as server.PayloadLimitsClientdecorator that wraps the existing connection decorator. Each request has the error limits (if set) attached as a request extension.RespondWorkflowTaskFailedRespondActivityTaskFailedRespondActivityTaskFailedand report activity as cancelledRespondNexusTaskFailedFailureReason::PayloadsTooLargemetric reason.Why?
Prevent server from hard-failing workflows, activities, and operations due to payloads/memo size limit violations. Allows customers to potentially update their workflows, activities, and operations to reduce the oversized payloads/memos.
Checklist