-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
docs: clarify the use of "/" as a prefix_rewrite #3160
Changes from 3 commits
65d01cc
ae407fd
64bf6ad
a88cebb
e5bf03b
9541fb6
dc34878
ddf2a2e
d46c26e
b47c282
a4c60d5
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 |
---|---|---|
|
@@ -228,5 +228,11 @@ Grpc::AsyncClientFactoryPtr Utility::factoryForGrpcApiConfigSource( | |
return async_client_manager.factoryForGrpcService(grpc_service, scope, false); | ||
} | ||
|
||
std::string Utility::normalizePath(absl::string_view path) { | ||
std::vector<absl::string_view> segments = StringUtil::splitToken(path, "/", false); | ||
bool has_trailing_slash = path.back() == '/'; | ||
return absl::StrCat("/", absl::StrJoin(segments, "/"), has_trailing_slash ? "/" : ""); | ||
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. Q: I feel like this is a pretty heavy hammer from a perf perspective for solving a specific issue (duplicate '/' after rewrite). Could we detect the duplicate '/' and copy strings in a way in which this would perform better? cc @jmarantz who likes these types of things. :) 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. Interesting. Probably most of the time the use-case is to have duplicates '/' at the beginning of the path. 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. actually I have functional concerns about this PR first. |
||
} | ||
|
||
} // namespace Config | ||
} // namespace Envoy |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -263,6 +263,13 @@ class Utility { | |
factoryForGrpcApiConfigSource(Grpc::AsyncClientManager& async_client_manager, | ||
const envoy::api::v2::core::ApiConfigSource& api_config_source, | ||
Stats::Scope& scope); | ||
|
||
/** | ||
* Normalize URI path. Currently it only handles to remove repeated slashes. | ||
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. Nit: "Currently it only handles removing repeated slashes." |
||
* @param path supplies the path to be normalized. | ||
* @return std::string the normalized path. | ||
*/ | ||
static std::string normalizePath(absl::string_view path); | ||
}; | ||
|
||
} // namespace Config | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -188,6 +188,18 @@ TEST(UtilityTest, CheckFilesystemSubscriptionBackingPath) { | |
Utility::checkFilesystemSubscriptionBackingPath(test_path); | ||
} | ||
|
||
TEST(UtilityTest, NormalizePath) { | ||
EXPECT_EQ("/ok/a/b/c", Utility::normalizePath("/ok/a/b/c")); | ||
EXPECT_EQ("/ok/a/b/c", Utility::normalizePath("/ok//a//b//c")); | ||
EXPECT_EQ("/ok/a/b/c", Utility::normalizePath("/ok///a///b///c")); | ||
EXPECT_EQ("/ok/a/b/c", Utility::normalizePath("////ok/a/b/c")); | ||
|
||
// Maintain the trailing slash. | ||
EXPECT_EQ("/ok/a/b/c/", Utility::normalizePath("/ok/a/b/c/")); | ||
EXPECT_EQ("/ok/a/b/c/", Utility::normalizePath("///ok/a/b/c///")); | ||
EXPECT_EQ("/ok/a/b/c/", Utility::normalizePath("///ok////a////b////c///")); | ||
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. I don't think normalization should be this aggressive, messing with the slashes in the middle of the path. I think the beginning of the path is a special case, and "scheme://origin/" should be treated the same as "scheme://origin". But I don't think that URL normalizers usually mess with anything after the "/". The motivation force in the bug the fixes was really the behavior of replacing prefix "/foo" with "/". WDYT of handling this at the parsing of that command, where a trailing slash is appended to src or dest if they don't have them? Then the swap can be done fairly cleanly. 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. +1, that's basically my thinking as well. 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. Sorry for overthinking it. What do you think of handling this “/“ specifically: treating that as replacing with empty string? https://github.com/dio/envoy/blob/fix-2956/source/common/router/config_impl.cc#L375 |
||
} | ||
|
||
// TEST(UtilityTest, FactoryForGrpcApiConfigSource) should catch misconfigured | ||
// API configs along the dimension of ApiConfigSource type. | ||
TEST(UtilityTest, FactoryForGrpcApiConfigSource) { | ||
|
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.
nit: const here and next line