Skip to content
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

xds: deprecate envoy.reloadable_features.xdstp_path_avoid_colon_encoding #38121

Merged
merged 1 commit into from
Jan 23, 2025
+6 −57
Merged

xds: deprecate envoy.reloadable_features.xdstp_path_avoid_colon_encoding #38121

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 3 additions & 0 deletions changelogs/current.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ removed_config_or_runtime:
- area: access_log
change: |
Removed runtime guard ``envoy.reloadable_features.upstream_remote_address_use_connection`` and legacy code paths.
- area: xds
change: |
Removed runtime guard ``envoy.reloadable_features.xdstp_path_avoid_colon_encoding`` and legacy code paths.
- area: thread_local
change: |
Removed runtime guard ``envoy.reloadable_features.allow_slot_destroy_on_worker_threads`` and legacy code paths.
Expand Down
5 changes: 1 addition & 4 deletions source/common/config/xds_resource.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,7 @@ std::string encodeAuthority(const std::string& authority) {
}

std::string encodeIdPath(const std::string& id) {
const std::string path =
Runtime::runtimeFeatureEnabled("envoy.reloadable_features.xdstp_path_avoid_colon_encoding")
? PercentEncoding::encode(id, "%?#[]")
: PercentEncoding::encode(id, "%:?#[]");
const std::string path = PercentEncoding::encode(id, "%?#[]");
return path.empty() ? "" : absl::StrCat("/", path);
}

Expand Down
1 change: 0 additions & 1 deletion source/common/runtime/runtime_features.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,6 @@ RUNTIME_GUARD(envoy_reloadable_features_validate_upstream_headers);
RUNTIME_GUARD(envoy_reloadable_features_wait_for_first_byte_before_balsa_msg_done);
RUNTIME_GUARD(envoy_reloadable_features_xds_failover_to_primary_enabled);
RUNTIME_GUARD(envoy_reloadable_features_xds_prevent_resource_copy);
RUNTIME_GUARD(envoy_reloadable_features_xdstp_path_avoid_colon_encoding);
RUNTIME_GUARD(envoy_restart_features_fix_dispatcher_approximate_now);
RUNTIME_GUARD(envoy_restart_features_skip_backing_cluster_check_for_sds);
RUNTIME_GUARD(envoy_restart_features_use_eds_cache_for_ads);
Expand Down
54 changes: 2 additions & 52 deletions test/common/config/xds_resource_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,6 @@ namespace Envoy {
namespace Config {
namespace {

// Both EscapedUrnOldColon and EscapedUrnWithManyQueryParamsOldColon should be
// removed once envoy.reloadable_features.xdstp_path_avoid_colon_encoding is
// removed.
const std::string EscapedUrnOldColon =
"xdstp://f123%25%2F%3F%23o/envoy.config.listener.v3.Listener/b%25%3A%3F%23%5B%5Dar//"
"baz?%25%23%5B%5D%26%3Dab=cde%25%23%5B%5D%26%3Df";
const std::string EscapedUrnWithManyQueryParamsOldColon =
"xdstp://f123%25%2F%3F%23o/envoy.config.listener.v3.Listener/b%25%3A%3F%23%5B%5Dar//"
"baz?%25%23%5B%5D%26%3D=bar&%25%23%5B%5D%26%3Dab=cde%25%23%5B%5D%26%3Df&foo=%25%23%5B%5D%26%3D";

const std::string EscapedUrn =
"xdstp://f123%25%2F%3F%23o/envoy.config.listener.v3.Listener/b%25:%3F%23%5B%5Dar//"
"baz?%25%23%5B%5D%26%3Dab=cde%25%23%5B%5D%26%3Df";
Expand Down Expand Up @@ -49,15 +39,8 @@ TEST(XdsResourceIdentifierTest, DecodeEncode) {
"xdstp://foo/envoy.config.listener.v3.Listener/bar/baz?=cd",
"xdstp://foo/envoy.config.listener.v3.Listener/bar/baz?ab=cde&ba=edc&z=f",
// Sets the escaped string contents depending on whether
// envoy.reloadable_features.xdstp_path_avoid_colon_encoding is set.
// This should be replaced by using the plain EscapedUrn and EscapedUrnWithManyQueryParams
// once envoy.reloadable_features.xdstp_path_avoid_colon_encoding is removed.
Runtime::runtimeFeatureEnabled("envoy.reloadable_features.xdstp_path_avoid_colon_encoding")
? EscapedUrn
: EscapedUrnOldColon,
Runtime::runtimeFeatureEnabled("envoy.reloadable_features.xdstp_path_avoid_colon_encoding")
? EscapedUrnWithManyQueryParams
: EscapedUrnWithManyQueryParamsOldColon,
EscapedUrn,
EscapedUrnWithManyQueryParams,
};

XdsResourceIdentifier::EncodeOptions encode_options;
Expand Down Expand Up @@ -98,39 +81,6 @@ TEST(XdsResourceIdentifierTest, ColonNotEncodedInPath) {
}
}

// Encoding of ":" in path if envoy.reloadable_features.xdstp_path_avoid_colon_encoding
// is set. This test should be removed once
// envoy.reloadable_features.xdstp_path_avoid_colon_encoding is removed.
TEST(XdsResourceIdentifierTest, ColonEncodedInPath) {
TestScopedRuntime scoped_runtime;
scoped_runtime.mergeValues(
{{"envoy.reloadable_features.xdstp_path_avoid_colon_encoding", "false"}});
// Validate decoding of %3A in path is converted to colon.
{
const auto resource_name =
XdsResourceIdentifier::decodeUrn("xdstp:///type/foo%3Abar/baz").value();
EXPECT_EQ("foo:bar/baz", resource_name.id());
}
{
const auto resource_locator =
XdsResourceIdentifier::decodeUrl("xdstp:///type/foo%3Abar/baz").value();
EXPECT_EQ("foo:bar/baz", resource_locator.id());
}
// Validate decoding and encoding of ":" is converted to %3A and back.
{
const auto resource_name =
XdsResourceIdentifier::decodeUrn("xdstp:///type/foo:bar/baz").value();
EXPECT_EQ("foo:bar/baz", resource_name.id());
EXPECT_EQ("xdstp:///type/foo%3Abar/baz", XdsResourceIdentifier::encodeUrn(resource_name));
}
{
const auto resource_locator =
XdsResourceIdentifier::decodeUrl("xdstp:///type/foo:bar/baz").value();
EXPECT_EQ("foo:bar/baz", resource_locator.id());
EXPECT_EQ("xdstp:///type/foo%3Abar/baz", XdsResourceIdentifier::encodeUrl(resource_locator));
}
}

// Corner cases around path-identifier encoding/decoding.
TEST(XdsResourceIdentifierTest, PathDividerEscape) {
{
Expand Down
Loading