Skip to content

Removed exception in getResponseStatus() #13314

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
yanavlasov merged 13 commits into from
Oct 28, 2020
Merged

Removed exception in getResponseStatus() #13314

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
6c1b18e
Removed exception in getResponseStatus()
zasweq Sep 29, 2020
5d54f1e
Responded to Alex's comments
zasweq Sep 30, 2020
b4564b2
Added crashing integration test and responded to Asra's comments
zasweq Sep 30, 2020
1d723cf
Changed back to exception. Added integration test for overflowing int…
zasweq Oct 1, 2020
0900801
Save progress
zasweq Oct 6, 2020
582a966
Dump of work
zasweq Oct 19, 2020
dcfddd3
Removed protocol integration test
zasweq Oct 19, 2020
5f345b3
Added integration test for overflowing status in h2
zasweq Oct 20, 2020
f34ac08
Save progress
zasweq Oct 23, 2020
5d49ae6
Merge branch 'master' into http-util-remove-exceptions
zasweq Oct 23, 2020
0e260cd
Overflowing and missing status added to protocol integration
zasweq Oct 23, 2020
ebd0f1a
Merge branch 'master' into http-util-remove-exceptions
zasweq Oct 26, 2020
8f2e640
Switched from exception to ASSERT
zasweq Oct 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions source/common/http/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,7 @@ envoy_cc_library(
"//source/common/common:assert_lib",
"//source/common/common:empty_string",
"//source/common/common:enum_to_int",
"//source/common/common:statusor_lib",
"//source/common/common:utility_lib",
"//source/common/grpc:status_lib",
"//source/common/json:json_loader_lib",
Expand Down
2 changes: 2 additions & 0 deletions source/common/http/http1/codec_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,8 @@ void ResponseEncoderImpl::encodeHeaders(const ResponseHeaderMap& headers, bool e

// The contract is that client codecs must ensure that :status is present.
ASSERT(headers.Status() != nullptr);
// encodeHeaders() is not supposed to throw any exceptions. This will crash on a release assert if

@yanavlasov yanavlasov Sep 30, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is possible that it could throw exception and exception and not crash when processing upstream response. The exception would have been caught by the dispatch() method of the client connection. Do we have an integration test where upstream responds with a garbage status?

// response status is invalid.
Comment thread
zasweq marked this conversation as resolved.
Outdated
uint64_t numeric_status = Utility::getResponseStatus(headers);

if (connection_.protocol() == Protocol::Http10 && connection_.supportsHttp10()) {
Expand Down
17 changes: 12 additions & 5 deletions source/common/http/http2/codec_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,9 +296,14 @@ void ConnectionImpl::StreamImpl::pendingRecvBufferLowWatermark() {
readDisable(false);
}

void ConnectionImpl::ClientStreamImpl::decodeHeaders() {
Status ConnectionImpl::ClientStreamImpl::decodeHeaders() {
auto& headers = absl::get<ResponseHeaderMapPtr>(headers_or_trailers_);
const uint64_t status = Http::Utility::getResponseStatus(*headers);

auto response_status_or_absl_status = Http::Utility::getResponseStatusOr(*headers);
if (!response_status_or_absl_status.ok()) {
return codecClientError(response_status_or_absl_status.status().message());
}
const uint64_t status = response_status_or_absl_status.value();

if (!upgrade_type_.empty() && headers->Status()) {
Http::Utility::transformUpgradeResponseFromH2toH1(*headers, upgrade_type_);
Expand All @@ -315,19 +320,21 @@ void ConnectionImpl::ClientStreamImpl::decodeHeaders() {
} else {
response_decoder_.decodeHeaders(std::move(headers), remote_end_stream_);
}
return okStatus();
}

void ConnectionImpl::ClientStreamImpl::decodeTrailers() {
response_decoder_.decodeTrailers(
std::move(absl::get<ResponseTrailerMapPtr>(headers_or_trailers_)));
}

void ConnectionImpl::ServerStreamImpl::decodeHeaders() {
Status ConnectionImpl::ServerStreamImpl::decodeHeaders() {
auto& headers = absl::get<RequestHeaderMapPtr>(headers_or_trailers_);
if (Http::Utility::isH2UpgradeRequest(*headers)) {
Http::Utility::transformUpgradeRequestFromH2toH1(*headers);
}
request_decoder_->decodeHeaders(std::move(headers), remote_end_stream_);
return okStatus();
}

void ConnectionImpl::ServerStreamImpl::decodeTrailers() {
Expand Down Expand Up @@ -783,7 +790,7 @@ Status ConnectionImpl::onFrameReceived(const nghttp2_frame* frame) {
switch (frame->headers.cat) {
case NGHTTP2_HCAT_RESPONSE:
case NGHTTP2_HCAT_REQUEST: {
stream->decodeHeaders();
RETURN_IF_ERROR(stream->decodeHeaders());
break;
}

Expand All @@ -797,7 +804,7 @@ Status ConnectionImpl::onFrameReceived(const nghttp2_frame* frame) {
stream->decodeTrailers();
} else {
// We're a client session and still waiting for non-informational headers.
stream->decodeHeaders();
RETURN_IF_ERROR(stream->decodeHeaders());
}
}
break;
Expand Down
6 changes: 3 additions & 3 deletions source/common/http/http2/codec_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log

// Does any necessary WebSocket/Upgrade conversion, then passes the headers
// to the decoder_.
virtual void decodeHeaders() PURE;
virtual Status decodeHeaders() PURE;
virtual void decodeTrailers() PURE;

// Get MetadataEncoder for this stream.
Expand Down Expand Up @@ -318,7 +318,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
void submitHeaders(const std::vector<nghttp2_nv>& final_headers,
nghttp2_data_provider* provider) override;
StreamDecoder& decoder() override { return response_decoder_; }
void decodeHeaders() override;
Status decodeHeaders() override;
void decodeTrailers() override;
HeaderMap& headers() override {
if (absl::holds_alternative<ResponseHeaderMapPtr>(headers_or_trailers_)) {
Expand Down Expand Up @@ -368,7 +368,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
void submitHeaders(const std::vector<nghttp2_nv>& final_headers,
nghttp2_data_provider* provider) override;
StreamDecoder& decoder() override { return *request_decoder_; }
void decodeHeaders() override;
Status decodeHeaders() override;
void decodeTrailers() override;
HeaderMap& headers() override {
if (absl::holds_alternative<RequestHeaderMapPtr>(headers_or_trailers_)) {
Expand Down
10 changes: 9 additions & 1 deletion source/common/http/utility.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
#include "absl/strings/str_cat.h"
#include "absl/strings/str_split.h"
#include "absl/strings/string_view.h"
#include "absl/types/optional.h"
#include "nghttp2/nghttp2.h"

namespace Envoy {
Expand Down Expand Up @@ -369,10 +370,17 @@ std::string Utility::makeSetCookieValue(const std::string& key, const std::strin
}

uint64_t Utility::getResponseStatus(const ResponseHeaderMap& headers) {
absl::StatusOr<uint64_t> response_status_or_absl_status = getResponseStatusOr(headers);

@yanavlasov yanavlasov Oct 21, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a case where this function can fail parsing the Status header now that there are tests that verify that invalid Status header is rejected by both H/1 and H/2 codecs?

@zasweq zasweq Oct 22, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, but you don't think it's worth removing exceptions (removing meaning converting to error earlier instead of in try catch block) for any future iterations, considering if we keep the exception then it'll still be there in the long run, even though currently not triggerable?

@yanavlasov yanavlasov Oct 23, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is totally worth removing this exception. We have proven with the test that the code does the right thing and does not accept responses with invalid Status header. Given this I think we should just assert that the parsing of the status number was successful. I think we could use ENVOY_BUG or event RELEASE_ASSERT here, given that we have solid test.

@zasweq zasweq Oct 24, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Yan, I've been thinking about this some, and I think that the problem is that getResponseStatus() is called all around the codebase, and the problem is that if we make it an assert rather than an exception for other parts of the codebase, it would introduce a point of failure out of nothing? Let me know what you're thinking :)

RELEASE_ASSERT(response_status_or_absl_status.ok(),
":status must be specified and a valid unsigned long");
Comment thread
zasweq marked this conversation as resolved.
Outdated
return response_status_or_absl_status.value();
}

absl::StatusOr<uint64_t> Utility::getResponseStatusOr(const ResponseHeaderMap& headers) {

@yanavlasov yanavlasov Oct 21, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When this method is only called from the ConnectionImpl::ClientStreamImpl::decodeHeaders(). Given that the H/2 codec verifies validity of the Status header, the only case where this can fail is if the Status header is missing altogether.
I think we should just move the check for Status header presence into the ConnectionImpl::ClientStreamImpl::decodeHeaders() and here just ASSERT success of the absl::SimpleAtoi with the comment that invalid Status values should not get past response parsing in codec.

@zasweq zasweq Oct 22, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. I liked how this got rid of exceptions, but your suggestion is a much cleaner way to remove the exceptions. Take the two things in the if and move one thing (simpleAtoi() call) to an assert and another (status presence) to codebase. However, I do believe the codecs already account for missing status headers similarly to overflowing status, so why even add a check there? @asraa what are your thoughts

@zasweq zasweq Oct 22, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quickly glanced around codebase, couldn't seem to find a missing status integration test? If not there (perhaps ctrl f in GitHub missed it :)) I can add one!

@zasweq zasweq Oct 22, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of my own interest I wrote an http2 frame method for header frames without statuses breaking the HTTP/2 requirement and wrote an integration test for it. I got "[2020-10-22 05:28:34.375][31][trace][http2] [source/common/http/http2/codec_impl.cc:715] [C2] about to recv frame type=1, flags=5
[2020-10-22 05:28:34.375][31][debug][http2] [source/common/http/http2/codec_impl.cc:890] [C2] invalid frame: Violation in HTTP messaging rule on stream 1". Thus, this is similar to overflowing statuses. Let me know if you want me to check that test in too, because it would potentially hit this line and if the codec didn't validate status presence which is what the test checked for.

@zasweq zasweq Oct 23, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Yan, I discussed this with Asra in our standup. Asra wants to keep it consistent, and is indifferent to whether we ASSERT or have a check, however wants to keep it consistent, and not an assert on atoi and a check for headers, because that would be unneeded complexity by splitting them.

@yanavlasov yanavlasov Oct 23, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I understand. I think given that we also verify that missing status from the response headers is rejected, I think we could either ENVOY_BUG or RELEASE_ASSERT that status header is present.

@zasweq zasweq Oct 23, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So release assert on both conditions in the utility function?

@zasweq zasweq Oct 23, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Both" meaning missing status and overflowed status

@zasweq zasweq Oct 23, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - so just to make sure - what you want is to keep the function as is, but instead of throwing up an exception with the if clause, wrap those two booleans (headers present, atoi call okay) in a RELASE_ASSERT. Basically, since we have validated that across both codecs the codecs handle it, we can move it.

@zasweq zasweq Oct 23, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Keep the function as is" meaning what is currently checked into master for getResponseStatus? Both utility function and not having my error handling in codecs.

const HeaderEntry* header = headers.Status();
uint64_t response_code;
if (!header || !absl::SimpleAtoi(headers.getStatusValue(), &response_code)) {
throw CodecClientException(":status must be specified and a valid unsigned long");
return absl::InvalidArgumentError(":status must be specified and a valid unsigned long");
}
return response_code;
}
Expand Down
6 changes: 5 additions & 1 deletion source/common/http/utility.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include "envoy/http/metadata_interface.h"
#include "envoy/http/query_params.h"

#include "common/common/statusor.h"
#include "common/http/exception.h"
#include "common/http/status.h"
#include "common/json/json_loader.h"
Expand Down Expand Up @@ -238,10 +239,13 @@ std::string makeSetCookieValue(const std::string& key, const std::string& value,
/**
* Get the response status from the response headers.
* @param headers supplies the headers to get the status from.
* @return uint64_t the response code or throws an exception if the headers are invalid.
* @return uint64_t the response code and release asserts if the
* headers are invalid.
*/
Comment thread
zasweq marked this conversation as resolved.
uint64_t getResponseStatus(const ResponseHeaderMap& headers);

absl::StatusOr<uint64_t> getResponseStatusOr(const ResponseHeaderMap& headers);

/**
* Determine whether these headers are a valid Upgrade request or response.
* This function returns true if the following HTTP headers and values are present:
Expand Down