admin: fix field mask parameter#17214
Conversation
Signed-off-by: Paul Gallagher <pgal@google.com>
|
@jmarantz could you take a look at this fix? |
| return absl::optional<std::pair<Http::Code, std::string>>{std::make_pair( | ||
| Http::Code::BadRequest, absl::StrCat("FieldMask ", field_mask.DebugString(), | ||
| " could not be successfully used."))}; | ||
| continue; |
There was a problem hiding this comment.
WDYT about just including in the output text the error message that we had before. IIUC you were taking a case that had an instructive error message before about what was wrong, and then making it just return an empty string.
Maybe collect the names of the fields that had invalid masks and return them in one error message?
We might also return an http error when none of the clusters had a useful mask. WDYT?
There was a problem hiding this comment.
I'm considering the case in the test, when someone just supplies mask=bootstrap. Intended behavior there is just that the bootstrap config is returned, with no error message. However, that mask is invalid for the majority of protos in the dump (which is why this was failing before).
I think that returning an HTTP error when no protos had a useful mask is a good idea.
Signed-off-by: Paul Gallagher <pgal@google.com>
Signed-off-by: Paul Gallagher <pgal@google.com>
jmarantz
left a comment
jmarantz
left a comment
There was a problem hiding this comment.
Looks great with one minor nit.
/assign-from @envoyproxy/senior-maintainers
|
@envoyproxy/senior-maintainers assignee is @lizan |
Signed-off-by: Paul Gallagher <pgal@google.com>
4 participants
Signed-off-by: Paul Gallagher pgal@google.com
Commit Message: admin: fix field mask parameter
Additional Description: PR #16979 caused a regression where providing field masks without providing a resource did not work. This change makes it so that providing a bad field mask fails silently when a resource is not provided.
Risk Level: low
Testing: unit test added to detect regression so that it will be caught in the future.