-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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.
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
http2: replaces nghttp2 functions with equivalents from QUICHE. #24943
Changes from all commits
f17c493
69036d4
651d775
21a51b8
e5641f6
443c519
ddec137
c0ab09d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,7 @@ | |
|
||
#include "absl/strings/match.h" | ||
#include "nghttp2/nghttp2.h" | ||
#include "quiche/http2/adapter/header_validator.h" | ||
|
||
namespace Envoy { | ||
namespace Http { | ||
|
@@ -190,17 +191,22 @@ bool HeaderUtility::schemeIsValid(const absl::string_view scheme) { | |
} | ||
|
||
bool HeaderUtility::headerValueIsValid(const absl::string_view header_value) { | ||
return nghttp2_check_header_value(reinterpret_cast<const uint8_t*>(header_value.data()), | ||
header_value.size()) != 0; | ||
return http2::adapter::HeaderValidator::IsValidHeaderValue(header_value, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, do we need a runtime guard for this? I would imagine it's possible that this is a behavior change in the event that QUICHE and nghttp2 do something different? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think about a runtime guard for this change? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let me see if I can write a unit test that demonstrates whether there is/is not a behavior change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With ObsTextOption::kAllow, the set of characters allowed by the oghttp2 method is exactly the same as the nghttp2 function. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm. Can we write a unit test then where we loop through all the possible characters and verify we get the same output? I just want to make 100% sure that this change doesn't result in some crazy corner case behavior change which isn't protected by a runtime guard? Or alternatively, if it obvious from inspection can you point me at the two methods? (Sorry to be pedantic here!) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the offline discussion and the pointer to the test. |
||
http2::adapter::ObsTextOption::kAllow); | ||
} | ||
|
||
bool HeaderUtility::headerNameContainsUnderscore(const absl::string_view header_name) { | ||
return header_name.find('_') != absl::string_view::npos; | ||
} | ||
|
||
bool HeaderUtility::authorityIsValid(const absl::string_view header_value) { | ||
return nghttp2_check_authority(reinterpret_cast<const uint8_t*>(header_value.data()), | ||
header_value.size()) != 0; | ||
if (Runtime::runtimeFeatureEnabled( | ||
"envoy.reloadable_features.http2_validate_authority_with_quiche")) { | ||
return http2::adapter::HeaderValidator::IsValidAuthority(header_value); | ||
} else { | ||
return nghttp2_check_authority(reinterpret_cast<const uint8_t*>(header_value.data()), | ||
header_value.size()) != 0; | ||
} | ||
} | ||
|
||
bool HeaderUtility::isSpecial1xx(const ResponseHeaderMap& response_headers) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm by no means a bazel expert, but should this continue to reference nghttp2 until the runtime flag is removed?