txn: don't try to decode request bodies > raft.SuggestedMaxDataSize

[s-mang]

This PR is a small, incremental step towards "fixing" #6147.

This change was originally suggested by @mkeeler in the issue description.

The basic premise of this change is that we are checking the request body size at the end of this fn (convertOps), after a huge request body may have already been decoded by mapstructure. The idea here is that we should try to check the request body size at the beginning of the fn as well, before sending it to mapstructure.

I'm submitting a PR here to suggest this change, but I am fully ready to scrap this code if it is not desirable. I will leave that decision to my reviewers.

[s-mang]

open question: should this functionality only be for convertOps, and not universally applied to decodeBody? (cc @i0rek )

[mkeeler]

Its unfortunate but I don't think we can do this. Vault will send Txn or KV requests that can exceed that suggested size. Prior to 1.5.3 they couldn't but then we added the ability to enable raft chunking specifically so Vault could send larger requests.

[banks]

@mkeeler I think you were right because the original commit here used raft. SuggestedMaxDataSize as the limit and we allow users to increase beyond that (undocumented) now. That said, I think the correct fix is to move to using the config. KVMaxValueSize which happened in the latest commit, but I'm not sure why the check needed to move to a more specific location? If we use config. KVMaxValueSize I think that should be appropriate for any endpoint unless I'm missing something?

[mkeeler]

@banks I think you are right in that we could use config.KVMaxValueSize instead of the raft suggested data size and that should allow this to work with our target use case for raft chunking. The odd part (that I missed during those original PRs) is that its not config.MaxTxnSize or something less tied to the KV because for non-Vault use cases whats coming in might have nothing to do with the KV but instead be a batch of node, service and check updates. Another interesting bit is that the Txn we are parsing might not do writes at all and thus might avoid raft all together. We should still place some upper bound on what we think the max content length prior to deserialization should be but I am not certain it should definitely be tied to the KVMaxValueSize.

[banks]

Yeah the name is kinda jank and has history - we added it when we made this configurable but it was just based on the old constant name it replaced. That constant pre-dates transaction API but was effectively "the biggest raft log message we want to handle" and so re-used for transaction API. If we ever actually document this properly, we could consider renaming it to be more appropriate but that said, it's arguable easier for users to grok that there is a max KV size and that transactions whether they touch KV or not also have to fit in that same size. Maybe easier to document than try to come up with a more correct name like "max_processable_entity_size" or something. IMO, I'd leave that bikeshed to paint later as this is nothing new being added :)

…

On Fri, Aug 30, 2019 at 5:05 PM Matt Keeler ***@***.***> wrote: @banks <https://github.com/banks> I think you are right in that we could use config.KVMaxValueSize instead of the raft suggested data size and that should allow this to work with our target use case for raft chunking. The odd part (that I missed during those original PRs) is that its not config.MaxTxnSize or something less tied to the KV because for non-Vault use cases whats coming in might have nothing to do with the KV but instead be a batch of node, service and check updates. Another interesting bit is that the Txn we are parsing might not do writes at all and thus might avoid raft all together. We should still place some upper bound on what we think the max content length prior to deserialization should be but I am not certain it should definitely be tied to the KVMaxValueSize. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6422?email_source=notifications&email_token=AAA5QU634LGKESOMT3BRF7LQHFAMVA5CNFSM4ISFMNY2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5SCRKY#issuecomment-526657707>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAA5QU3AIY7NT6ZUR2VQBH3QHFAMVANCNFSM4ISFMNYQ> .

[mkeeler]

LGTM