Skip to content

Commit

Permalink
xds: refactor decodeUrn to not throw (envoyproxy#31128)
Browse files Browse the repository at this point in the history 
Risk Level: low
Testing: n/a
Docs Changes: n/a
Release Notes: n/a
envoyproxy/envoy-mobile#176
Signed-off-by: Alyssa Wilk <[email protected]>
  • Loading branch information
@alyssawilk
alyssawilk authored Dec 5, 2023
1 parent a9e51fb commit 69481d2
Show file tree
Hide file tree
Showing 8 changed files with 52 additions and 31 deletions.
38 changes: 23 additions & 15 deletions source/common/config/xds_resource.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,19 +104,20 @@ std::string XdsResourceIdentifier::encodeUrl(const xds::core::v3::ResourceLocato

namespace {

void decodePath(absl::string_view path, std::string* resource_type, std::string& id) {
absl::Status decodePath(absl::string_view path, std::string* resource_type, std::string& id) {
// This is guaranteed by Http::Utility::extractHostPathFromUrn.
ASSERT(absl::StartsWith(path, "/"));
const std::vector<absl::string_view> path_components = absl::StrSplit(path.substr(1), '/');
auto id_it = path_components.cbegin();
if (resource_type != nullptr) {
*resource_type = std::string(path_components[0]);
if (resource_type->empty()) {
throwEnvoyExceptionOrPanic(fmt::format("Resource type missing from {}", path));
return absl::InvalidArgumentError(fmt::format("Resource type missing from {}", path));
}
id_it = std::next(id_it);
}
id = PercentEncoding::decode(absl::StrJoin(id_it, path_components.cend(), "/"));
return absl::OkStatus();
}

void decodeQueryParams(absl::string_view query_params,
Expand All @@ -128,9 +129,9 @@ void decodeQueryParams(absl::string_view query_params,
}
}

void decodeFragment(
absl::string_view fragment,
Protobuf::RepeatedPtrField<xds::core::v3::ResourceLocator::Directive>& directives) {
absl::Status
decodeFragment(absl::string_view fragment,
Protobuf::RepeatedPtrField<xds::core::v3::ResourceLocator::Directive>& directives) {
const std::vector<absl::string_view> fragment_components = absl::StrSplit(fragment, ',');
for (const absl::string_view& fragment_component : fragment_components) {
if (absl::StartsWith(fragment_component, "alt=")) {
Expand All @@ -139,17 +140,20 @@ void decodeFragment(
} else if (absl::StartsWith(fragment_component, "entry=")) {
directives.Add()->set_entry(PercentEncoding::decode(fragment_component.substr(6)));
} else {
throwEnvoyExceptionOrPanic(fmt::format("Unknown fragment component {}", fragment_component));
;
return absl::InvalidArgumentError(
fmt::format("Unknown fragment component {}", fragment_component));
}
}
return absl::OkStatus();
}

} // namespace

xds::core::v3::ResourceName XdsResourceIdentifier::decodeUrn(absl::string_view resource_urn) {
absl::StatusOr<xds::core::v3::ResourceName>
XdsResourceIdentifier::decodeUrn(absl::string_view resource_urn) {
if (!hasXdsTpScheme(resource_urn)) {
throwEnvoyExceptionOrPanic(fmt::format("{} does not have an xdstp: scheme", resource_urn));
return absl::InvalidArgumentError(
fmt::format("{} does not have an xdstp: scheme", resource_urn));
}
absl::string_view host, path;
Http::Utility::extractHostPathFromUri(resource_urn, host, path);
Expand All @@ -160,8 +164,11 @@ xds::core::v3::ResourceName XdsResourceIdentifier::decodeUrn(absl::string_view r
decodeQueryParams(path.substr(query_params_start), *decoded_resource_name.mutable_context());
path = path.substr(0, query_params_start);
}
decodePath(path, decoded_resource_name.mutable_resource_type(),
*decoded_resource_name.mutable_id());
auto status = decodePath(path, decoded_resource_name.mutable_resource_type(),
*decoded_resource_name.mutable_id());
if (!status.ok()) {
return status;
}
return decoded_resource_name;
}

Expand All @@ -171,7 +178,8 @@ xds::core::v3::ResourceLocator XdsResourceIdentifier::decodeUrl(absl::string_vie
xds::core::v3::ResourceLocator decoded_resource_locator;
const size_t fragment_start = path.find('#');
if (fragment_start != absl::string_view::npos) {
decodeFragment(path.substr(fragment_start + 1), *decoded_resource_locator.mutable_directives());
THROW_IF_NOT_OK(decodeFragment(path.substr(fragment_start + 1),
*decoded_resource_locator.mutable_directives()));
path = path.substr(0, fragment_start);
}
if (hasXdsTpScheme(resource_url)) {
Expand All @@ -181,7 +189,7 @@ xds::core::v3::ResourceLocator XdsResourceIdentifier::decodeUrl(absl::string_vie
} else if (absl::StartsWith(resource_url, "file:")) {
decoded_resource_locator.set_scheme(xds::core::v3::ResourceLocator::FILE);
// File URLs only have a path and fragment.
decodePath(path, nullptr, *decoded_resource_locator.mutable_id());
THROW_IF_NOT_OK(decodePath(path, nullptr, *decoded_resource_locator.mutable_id()));
return decoded_resource_locator;
} else {
throwEnvoyExceptionOrPanic(
Expand All @@ -194,8 +202,8 @@ xds::core::v3::ResourceLocator XdsResourceIdentifier::decodeUrl(absl::string_vie
*decoded_resource_locator.mutable_exact_context());
path = path.substr(0, query_params_start);
}
decodePath(path, decoded_resource_locator.mutable_resource_type(),
*decoded_resource_locator.mutable_id());
THROW_IF_NOT_OK(decodePath(path, decoded_resource_locator.mutable_resource_type(),
*decoded_resource_locator.mutable_id()));
return decoded_resource_locator;
}

Expand Down
2 changes: 1 addition & 1 deletion source/common/config/xds_resource.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ class XdsResourceIdentifier {
* @return xds::core::v3::ResourceName resource name message for resource_urn.
* @throws EnvoyException when parsing fails.
*/
static xds::core::v3::ResourceName decodeUrn(absl::string_view resource_urn);
static absl::StatusOr<xds::core::v3::ResourceName> decodeUrn(absl::string_view resource_urn);

/**
* Decode a xdstp:// URL string to a xds::core::v3::ResourceLocator.
Expand Down
9 changes: 6 additions & 3 deletions source/extensions/config_subscription/grpc/grpc_mux_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,9 @@ bool isXdsTpWildcard(const std::string& resource_name) {
// Must only be called on XdsTp resource names.
std::string convertToWildcard(const std::string& resource_name) {
ASSERT(XdsResourceIdentifier::hasXdsTpScheme(resource_name));
xds::core::v3::ResourceName xdstp_resource = XdsResourceIdentifier::decodeUrn(resource_name);
auto resource_or_error = XdsResourceIdentifier::decodeUrn(resource_name);
THROW_IF_STATUS_NOT_OK(resource_or_error, throw);
xds::core::v3::ResourceName xdstp_resource = resource_or_error.value();
const auto pos = xdstp_resource.id().find_last_of('/');
xdstp_resource.set_id(
pos == std::string::npos ? "*" : absl::StrCat(xdstp_resource.id().substr(0, pos), "/*"));
Expand Down Expand Up @@ -384,8 +386,9 @@ void GrpcMuxImpl::processDiscoveryResources(const std::vector<DecodedResourcePtr
all_resource_refs.emplace_back(*resource);
if (XdsResourceIdentifier::hasXdsTpScheme(resource->name())) {
// Sort the context params of an xdstp resource, so we can compare them easily.
xds::core::v3::ResourceName xdstp_resource =
XdsResourceIdentifier::decodeUrn(resource->name());
auto resource_or_error = XdsResourceIdentifier::decodeUrn(resource->name());
THROW_IF_STATUS_NOT_OK(resource_or_error, throw);
xds::core::v3::ResourceName xdstp_resource = resource_or_error.value();
XdsResourceIdentifier::EncodeOptions options;
options.sort_context_params_ = true;
resource_ref_map.emplace(XdsResourceIdentifier::encodeUrn(xdstp_resource, options),
Expand Down
4 changes: 3 additions & 1 deletion source/extensions/config_subscription/grpc/grpc_mux_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,9 @@ class GrpcMuxImpl : public GrpcMux,
resources.begin(), resources.end(), std::inserter(resources_, resources_.begin()),
[this](const std::string& resource_name) -> std::string {
if (XdsResourceIdentifier::hasXdsTpScheme(resource_name)) {
auto xdstp_resource = XdsResourceIdentifier::decodeUrn(resource_name);
auto xdstp_resource_or_error = XdsResourceIdentifier::decodeUrn(resource_name);
THROW_IF_STATUS_NOT_OK(xdstp_resource_or_error, throw);
auto xdstp_resource = xdstp_resource_or_error.value();
if (subscription_options_.add_xdstp_node_context_params_) {
const auto context = XdsContextParams::encodeResource(
local_info_.contextProvider().nodeContext(), xdstp_resource.context(), {}, {});
Expand Down
4 changes: 3 additions & 1 deletion source/extensions/config_subscription/grpc/new_grpc_mux_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,9 @@ void NewGrpcMuxImpl::updateWatch(const std::string& type_url, Watch* watch,
absl::flat_hash_set<std::string> effective_resources;
for (const auto& resource : resources) {
if (XdsResourceIdentifier::hasXdsTpScheme(resource)) {
auto xdstp_resource = XdsResourceIdentifier::decodeUrn(resource);
auto xdstp_resource_or_error = XdsResourceIdentifier::decodeUrn(resource);
THROW_IF_STATUS_NOT_OK(xdstp_resource_or_error, throw);
auto xdstp_resource = xdstp_resource_or_error.value();
if (options.add_xdstp_node_context_params_) {
const auto context = XdsContextParams::encodeResource(
local_info_.contextProvider().nodeContext(), xdstp_resource.context(), {}, {});
Expand Down
4 changes: 3 additions & 1 deletion source/extensions/config_subscription/grpc/watch_map.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,9 @@ absl::flat_hash_set<Watch*> WatchMap::watchesInterestedIn(const std::string& res
// This is not very efficient; it is possible to canonicalize etc. much faster with raw string
// operations, but this implementation provides a reference for later optimization while we
// adopt xdstp://.
xdstp_resource = XdsResourceIdentifier::decodeUrn(resource_name);
auto resource_or_error = XdsResourceIdentifier::decodeUrn(resource_name);
THROW_IF_STATUS_NOT_OK(resource_or_error, throw);
xdstp_resource = resource_or_error.value();
}
auto watches_interested = watch_interest_.find(
is_xdstp ? XdsResourceIdentifier::encodeUrn(xdstp_resource, encode_options) : resource_name);
Expand Down
4 changes: 3 additions & 1 deletion source/extensions/config_subscription/grpc/xds_mux/grpc_mux_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,9 @@ void GrpcMuxImpl<S, F, RQ, RS>::updateWatch(const std::string& type_url, Watch*
absl::flat_hash_set<std::string> effective_resources;
for (const auto& resource : resources) {
if (XdsResourceIdentifier::hasXdsTpScheme(resource)) {
auto xdstp_resource = XdsResourceIdentifier::decodeUrn(resource);
auto xdstp_resource_or_error = XdsResourceIdentifier::decodeUrn(resource);
THROW_IF_STATUS_NOT_OK(xdstp_resource_or_error, throw);
auto xdstp_resource = xdstp_resource_or_error.value();
if (options.add_xdstp_node_context_params_) {
const auto context = XdsContextParams::encodeResource(
local_info_.contextProvider().nodeContext(), xdstp_resource.context(), {}, {});
Expand Down
18 changes: 10 additions & 8 deletions test/common/config/xds_resource_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ TEST(XdsResourceIdentifierTest, DecodeEncode) {
XdsResourceIdentifier::EncodeOptions encode_options;
encode_options.sort_context_params_ = true;
for (const std::string& uri : uris) {
EXPECT_EQ(uri, XdsResourceIdentifier::encodeUrn(XdsResourceIdentifier::decodeUrn(uri),
EXPECT_EQ(uri, XdsResourceIdentifier::encodeUrn(XdsResourceIdentifier::decodeUrn(uri).value(),
encode_options));
EXPECT_EQ(uri, XdsResourceIdentifier::encodeUrl(XdsResourceIdentifier::decodeUrl(uri),
encode_options));
Expand All @@ -53,7 +53,8 @@ TEST(XdsResourceIdentifierTest, DecodeEncode) {
// Corner cases around path-identifier encoding/decoding.
TEST(XdsResourceIdentifierTest, PathDividerEscape) {
{
const auto resource_name = XdsResourceIdentifier::decodeUrn("xdstp:///type/foo%2Fbar/baz");
const auto resource_name =
XdsResourceIdentifier::decodeUrn("xdstp:///type/foo%2Fbar/baz").value();
EXPECT_EQ("foo/bar/baz", resource_name.id());
EXPECT_EQ("xdstp:///type/foo/bar/baz", XdsResourceIdentifier::encodeUrn(resource_name));
}
Expand All @@ -66,7 +67,8 @@ TEST(XdsResourceIdentifierTest, PathDividerEscape) {

// Validate that URN decoding behaves as expected component-wise.
TEST(XdsResourceNameTest, DecodeSuccess) {
const auto resource_name = XdsResourceIdentifier::decodeUrn(EscapedUrnWithManyQueryParams);
const auto resource_name =
XdsResourceIdentifier::decodeUrn(EscapedUrnWithManyQueryParams).value();
EXPECT_EQ("f123%/?#o", resource_name.authority());
EXPECT_EQ("envoy.config.listener.v3.Listener", resource_name.resource_type());
EXPECT_EQ(resource_name.id(), "b%:?#[]ar//baz");
Expand Down Expand Up @@ -98,7 +100,7 @@ TEST(XdsResourceLocatorTest, DecodeSuccess) {
// Validate that the URN decoding behaves with a near-empty xDS resource name.
TEST(XdsResourceLocatorTest, DecodeEmpty) {
const auto resource_name =
XdsResourceIdentifier::decodeUrn("xdstp:///envoy.config.listener.v3.Listener");
XdsResourceIdentifier::decodeUrn("xdstp:///envoy.config.listener.v3.Listener").value();
EXPECT_TRUE(resource_name.authority().empty());
EXPECT_EQ("envoy.config.listener.v3.Listener", resource_name.resource_type());
EXPECT_TRUE(resource_name.id().empty());
Expand All @@ -119,12 +121,12 @@ TEST(XdsResourceNameTest, DecodeEmpty) {
// Negative tests for URN decoding.
TEST(XdsResourceNameTest, DecodeFail) {
{
EXPECT_THROW_WITH_MESSAGE(XdsResourceIdentifier::decodeUrn("foo://"), EnvoyException,
"foo:// does not have an xdstp: scheme");
EXPECT_EQ(XdsResourceIdentifier::decodeUrn("foo://").status().message(),
"foo:// does not have an xdstp: scheme");
}
{
EXPECT_THROW_WITH_MESSAGE(XdsResourceIdentifier::decodeUrn("xdstp://foo"), EnvoyException,
"Resource type missing from /");
EXPECT_EQ(XdsResourceIdentifier::decodeUrn("xdstp://foo").status().message(),
"Resource type missing from /");
}
}

Expand Down

0 comments on commit 69481d2

Please sign in to comment.