Skip to content

Commit

Permalink
Additional validation and filter config cleanup for Backend Routing (#…
Browse files Browse the repository at this point in the history
…175)

Config Manager changes:
- For empty prefix in APPEND: Don't generate rules, they are no-op.
- For empty prefix in CONST: Generate rules with path prefix '/'.
- For URL parser: Don't parse query params. None of the control plane config should allow query params anyways.

Filter changes:
- More validation of proto config (as described above) and tests for this.
- Tests to show duplicated query param behavior.

Signed-off-by: Teju Nareddy <[email protected]>
  • Loading branch information
nareddyt authored Jun 2, 2020
1 parent a5c9e3b commit fa11613
Show file tree
Hide file tree
Showing 13 changed files with 312 additions and 74 deletions.
2 changes: 1 addition & 1 deletion api/VERSION
Original file line number Diff line number Diff line change
@@ -1 +1 @@
v2
v3
6 changes: 4 additions & 2 deletions api/envoy/http/backend_routing/config.proto
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,14 @@ message BackendRoutingRule {
APPEND_PATH_TO_ADDRESS = 2;
}

PathTranslation path_translation = 4;
PathTranslation path_translation = 4
[(validate.rules).enum = { not_in: [ 0 ] }];

// Prefix used when re-writing the path for the backend.
string path_prefix = 3 [(validate.rules).string = {
well_known_regex: HTTP_HEADER_VALUE,
strict: false
strict: false,
min_len: 1,
}];
}

Expand Down
15 changes: 15 additions & 0 deletions src/envoy/http/backend_routing/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,21 @@ envoy_cc_test(
],
)

envoy_cc_test(
name = "filter_config_test",
size = "small",
srcs = [
"filter_config_test.cc",
],
repository = "@envoy",
deps = [
":filter_lib",
"//api/envoy/http/backend_routing:config_proto_cc_proto",
"@envoy//test/mocks/server:server_mocks",
"@envoy//test/test_common:utility_lib",
],
)

envoy_cc_fuzz_test(
name = "backend_routing_filter_fuzz_test",
srcs = ["filter_fuzz_test.cc"],
Expand Down
7 changes: 1 addition & 6 deletions src/envoy/http/backend_routing/filter_config.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#pragma once

#include "api/envoy/http/backend_routing/config.pb.h"
#include "common/common/empty_string.h"
#include "common/common/logger.h"
#include "envoy/server/filter_config.h"

Expand Down Expand Up @@ -51,12 +52,6 @@ class FilterConfig : public Envoy::Logger::Loggable<Envoy::Logger::Id::filter> {
: proto_config_(proto_config),
stats_(generateStats(stats_prefix, context.scope())) {
for (const auto& rule : proto_config_.rules()) {
if (rule.path_translation() ==
::google::api::envoy::http::backend_routing::BackendRoutingRule::
PATH_TRANSLATION_UNSPECIFIED) {
throw Envoy::ProtoValidationException(
"Path translation for BackendRouting rule must be specified", rule);
}
backend_routing_map_[rule.operation()] = &rule;
}
}
Expand Down
105 changes: 105 additions & 0 deletions src/envoy/http/backend_routing/filter_config_test.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
// Copyright 2019 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

#include "src/envoy/http/backend_routing/filter_config.h"

#include "api/envoy/http/backend_routing/config.pb.validate.h"
#include "common/common/empty_string.h"
#include "gmock/gmock.h"
#include "google/protobuf/text_format.h"
#include "gtest/gtest.h"
#include "test/mocks/server/mocks.h"
#include "test/test_common/utility.h"

using ::testing::_;
using ::testing::Invoke;
using ::testing::Return;

namespace espv2 {
namespace envoy {
namespace http_filters {
namespace backend_routing {
namespace {

class BackendRoutingConfigTest : public ::testing::Test {
protected:
void validateConfig(absl::string_view filter_config) {
google::api::envoy::http::backend_routing::FilterConfig proto_config;
ASSERT_TRUE(google::protobuf::TextFormat::ParseFromString(
std::string(filter_config), &proto_config));
ASSERT_GT(proto_config.rules_size(), 0);

Envoy::TestUtility::validate(proto_config);
}
};

TEST_F(BackendRoutingConfigTest, AppendAddressNoPrefixThrows) {
ASSERT_THROW(validateConfig(R"(
rules {
operation: "append-with-noop-prefix-operation"
path_translation: APPEND_PATH_TO_ADDRESS
path_prefix: ""
}
)"),
Envoy::ProtoValidationException);
}

TEST_F(BackendRoutingConfigTest, ConstAddressNoPrefixThrows) {
ASSERT_THROW(validateConfig(R"(
rules {
operation: "const-with-invalid-prefix-operation"
path_translation: CONSTANT_ADDRESS
path_prefix: ""
}
)"),
Envoy::ProtoValidationException);
}

TEST_F(BackendRoutingConfigTest, ConstAddressRootPrefixWorks) {
ASSERT_NO_THROW(validateConfig(R"(
rules {
operation: "const-with-root-prefix-operation"
path_translation: CONSTANT_ADDRESS
path_prefix: "/"
}
)"));
}

TEST_F(BackendRoutingConfigTest, PathTranslationUnspecifiedThrows) {
ASSERT_THROW(validateConfig(R"(
rules {
operation: "invalid-path-translation-operation"
path_prefix: "/test"
}
)"),
Envoy::ProtoValidationException);
}

TEST_F(BackendRoutingConfigTest, InvalidPathCharactersThrows) {
ASSERT_THROW(validateConfig(R"(
rules {
operation: "invalid-path-prefix-operation"
path_translation: APPEND_PATH_TO_ADDRESS
path_prefix: "/test\r\n/invalid"
}
)"),
Envoy::ProtoValidationException);
}

} // namespace

} // namespace backend_routing
} // namespace http_filters
} // namespace envoy
} // namespace espv2
75 changes: 21 additions & 54 deletions src/envoy/http/backend_routing/filter_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,6 @@ namespace backend_routing {
namespace {

const char kFilterConfig[] = R"(
rules {
operation: "append-operation"
path_translation: APPEND_PATH_TO_ADDRESS
path_prefix: ""
}
rules {
operation: "append-with-prefix-operation"
path_translation: APPEND_PATH_TO_ADDRESS
Expand All @@ -55,11 +50,6 @@ rules {
path_translation: CONSTANT_ADDRESS
path_prefix: "/test-prefix"
}
rules {
operation: "const-with-bad-prefix-operation"
path_translation: CONSTANT_ADDRESS
path_prefix: ""
}
)";

/**
Expand Down Expand Up @@ -181,33 +171,6 @@ TEST_F(BackendRoutingFilterTest, ConstantAddress) {
EXPECT_EQ(counter->value(), 1);
}

TEST_F(BackendRoutingFilterTest, ConstantAddressWithBadPrefix) {
Envoy::Http::TestRequestHeaderMapImpl headers{{":method", "GET"},
{":path", "/books/1"}};
utils::setStringFilterState(
*mock_decoder_callbacks_.stream_info_.filter_state_, utils::kOperation,
"const-with-bad-prefix-operation");

// Call function under test
Envoy::Http::FilterHeadersStatus status =
filter_->decodeHeaders(headers, false);

// Expect the path to be modified. Note that it is expected to be an empty
// URL here. This is problematic in practice, since it doesn't have a '/'.
// Config manager will ensure that this configuration is never passed to
// Envoy.
EXPECT_EQ(headers.Path()->value().getStringView(), Envoy::EMPTY_STRING);
EXPECT_EQ(status, Envoy::Http::FilterHeadersStatus::Continue);

// Stats.
const Envoy::Stats::CounterSharedPtr counter =
Envoy::TestUtility::findCounter(
mock_factory_context_.scope_,
"backend_routing.constant_address_request");
ASSERT_NE(counter, nullptr);
EXPECT_EQ(counter->value(), 1);
}

TEST_F(BackendRoutingFilterTest, ConstantAddressWithPathMatcherQueryParams) {
Envoy::Http::TestRequestHeaderMapImpl headers{{":method", "GET"},
{":path", "/books/1"}};
Expand Down Expand Up @@ -238,23 +201,24 @@ TEST_F(BackendRoutingFilterTest, ConstantAddressWithPathMatcherQueryParams) {
/**
* The request URL contains multiple query parameters.
*/
class BackendRoutingFilterWithQueryParamsTest
class BackendRoutingFilterQueryParamInRequestTest
: public BackendRoutingFilterTest {};

TEST_F(BackendRoutingFilterWithQueryParamsTest, AppendPathToAddress) {
TEST_F(BackendRoutingFilterQueryParamInRequestTest,
AppendPathToAddressWithPrefix) {
Envoy::Http::TestRequestHeaderMapImpl headers{
{":method", "GET"}, {":path", "/books/1?view=summary&filter=deleted"}};
utils::setStringFilterState(
*mock_decoder_callbacks_.stream_info_.filter_state_, utils::kOperation,
"append-operation");
"append-with-prefix-operation");

// Call function under test
Envoy::Http::FilterHeadersStatus status =
filter_->decodeHeaders(headers, false);

// Expect the path to be modified.
EXPECT_EQ(headers.Path()->value().getStringView(),
"/books/1?view=summary&filter=deleted");
"/test-prefix/books/1?view=summary&filter=deleted");
EXPECT_EQ(status, Envoy::Http::FilterHeadersStatus::Continue);

// Stats.
Expand All @@ -266,45 +230,49 @@ TEST_F(BackendRoutingFilterWithQueryParamsTest, AppendPathToAddress) {
EXPECT_EQ(counter->value(), 1);
}

TEST_F(BackendRoutingFilterWithQueryParamsTest, AppendPathToAddressWithPrefix) {
TEST_F(BackendRoutingFilterQueryParamInRequestTest, ConstantAddress) {
Envoy::Http::TestRequestHeaderMapImpl headers{
{":method", "GET"}, {":path", "/books/1?view=summary&filter=deleted"}};
utils::setStringFilterState(
*mock_decoder_callbacks_.stream_info_.filter_state_, utils::kOperation,
"append-with-prefix-operation");
"const-operation");

// Call function under test
Envoy::Http::FilterHeadersStatus status =
filter_->decodeHeaders(headers, false);

// Expect the path to be modified.
EXPECT_EQ(headers.Path()->value().getStringView(),
"/test-prefix/books/1?view=summary&filter=deleted");
"/?view=summary&filter=deleted");
EXPECT_EQ(status, Envoy::Http::FilterHeadersStatus::Continue);

// Stats.
const Envoy::Stats::CounterSharedPtr counter =
Envoy::TestUtility::findCounter(
mock_factory_context_.scope_,
"backend_routing.append_path_to_address_request");
"backend_routing.constant_address_request");
ASSERT_NE(counter, nullptr);
EXPECT_EQ(counter->value(), 1);
}

TEST_F(BackendRoutingFilterWithQueryParamsTest, ConstantAddress) {
TEST_F(BackendRoutingFilterQueryParamInRequestTest,
ConstantAddressWithPathMatcherQueryParams) {
Envoy::Http::TestRequestHeaderMapImpl headers{
{":method", "GET"}, {":path", "/books/1?view=summary&filter=deleted"}};
utils::setStringFilterState(
*mock_decoder_callbacks_.stream_info_.filter_state_, utils::kOperation,
"const-operation");
utils::setStringFilterState(
*mock_decoder_callbacks_.stream_info_.filter_state_, utils::kQueryParams,
"id=1");

// Call function under test
Envoy::Http::FilterHeadersStatus status =
filter_->decodeHeaders(headers, false);

// Expect the path to be modified.
EXPECT_EQ(headers.Path()->value().getStringView(),
"/?view=summary&filter=deleted");
"/?view=summary&filter=deleted&id=1");
EXPECT_EQ(status, Envoy::Http::FilterHeadersStatus::Continue);

// Stats.
Expand All @@ -316,10 +284,10 @@ TEST_F(BackendRoutingFilterWithQueryParamsTest, ConstantAddress) {
EXPECT_EQ(counter->value(), 1);
}

TEST_F(BackendRoutingFilterWithQueryParamsTest,
ConstantAddressWithPathMatcherQueryParams) {
Envoy::Http::TestRequestHeaderMapImpl headers{
{":method", "GET"}, {":path", "/books/1?view=summary&filter=deleted"}};
TEST_F(BackendRoutingFilterQueryParamInRequestTest,
ConstantAddressWithDuplicatedPathMatcherQueryParams) {
Envoy::Http::TestRequestHeaderMapImpl headers{{":method", "GET"},
{":path", "/books/1?id=2"}};
utils::setStringFilterState(
*mock_decoder_callbacks_.stream_info_.filter_state_, utils::kOperation,
"const-operation");
Expand All @@ -332,8 +300,7 @@ TEST_F(BackendRoutingFilterWithQueryParamsTest,
filter_->decodeHeaders(headers, false);

// Expect the path to be modified.
EXPECT_EQ(headers.Path()->value().getStringView(),
"/?view=summary&filter=deleted&id=1");
EXPECT_EQ(headers.Path()->value().getStringView(), "/?id=2&id=1");
EXPECT_EQ(status, Envoy::Http::FilterHeadersStatus::Continue);

// Stats.
Expand All @@ -345,7 +312,7 @@ TEST_F(BackendRoutingFilterWithQueryParamsTest,
EXPECT_EQ(counter->value(), 1);
}

TEST_F(BackendRoutingFilterWithQueryParamsTest, ConstantAddressWithPrefix) {
TEST_F(BackendRoutingFilterQueryParamInRequestTest, ConstantAddressWithPrefix) {
Envoy::Http::TestRequestHeaderMapImpl headers{
{":method", "GET"}, {":path", "/books/1?view=summary&filter=deleted"}};
utils::setStringFilterState(
Expand Down
18 changes: 15 additions & 3 deletions src/go/configgenerator/listener_generator.go
Original file line number Diff line number Diff line change
Expand Up @@ -788,8 +788,13 @@ func makeBackendRoutingFilter(serviceInfo *sc.ServiceInfo) (*hcmpb.HttpFilter, e
for _, operation := range serviceInfo.Operations {
method := serviceInfo.Methods[operation]

// For gRPC service config that has no path translation, no need for backend routing filter (only host rewrite via route config).
if method.BackendInfo != nil && method.BackendInfo.TranslationType != confpb.BackendRule_PATH_TRANSLATION_UNSPECIFIED {
if method.BackendInfo != nil {

// For gRPC service config that has no path translation, no need for backend routing filter (only host rewrite via route config).
if method.BackendInfo.TranslationType == confpb.BackendRule_PATH_TRANSLATION_UNSPECIFIED {
continue
}

newRule := &brpb.BackendRoutingRule{
Operation: operation,
}
Expand All @@ -804,8 +809,15 @@ func makeBackendRoutingFilter(serviceInfo *sc.ServiceInfo) (*hcmpb.HttpFilter, e
}

if method.BackendInfo != nil {
newRule.PathPrefix = method.BackendInfo.Uri
newRule.PathPrefix = method.BackendInfo.Path
}

// If the path prefix is empty, then the filter is a no-op for this operation.
// It doesn't need to be in the filter config, skip it.
if newRule.PathPrefix == "" {
continue
}

rules = append(rules, newRule)
}
}
Expand Down
Loading

0 comments on commit fa11613

Please sign in to comment.