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

Jump to bottom

api: use traffic_direction over operation_name if specified #7999

Merged
merged 8 commits into from
Sep 4, 2019
Merged

api: use traffic_direction over operation_name if specified #7999

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a190c44
use traffic_direction over operation_name
kyessenov Aug 21, 2019
2b146cd
doc fix
kyessenov Aug 22, 2019
ea6f8dd
Merge remote-tracking branch 'upstream/master' into traffic_direction
kyessenov Aug 23, 2019
7190173
deprecate old field
kyessenov Aug 23, 2019
a7cd6e0
docs lint
kyessenov Aug 23, 2019
1a83763
Merge remote-tracking branch 'upstream/master' into traffic_direction
kyessenov Sep 4, 2019
a8aaed2
add deprecation note
kyessenov Sep 4, 2019
b868f6e
Merge remote-tracking branch 'upstream/master' into traffic_direction
kyessenov Sep 4, 2019
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
10 changes: 7 additions & 3 deletions api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,6 @@ message HttpConnectionManager {
google.protobuf.BoolValue add_user_agent = 6;

message Tracing {
// [#comment:TODO(kyessenov): Align this field with listener traffic direction field.]
enum OperationName {
option (gogoproto.goproto_enum_prefix) = false;

Expand All @@ -91,8 +90,13 @@ message HttpConnectionManager {
EGRESS = 1;
}

// The span name will be derived from this field.
OperationName operation_name = 1 [(validate.rules).enum.defined_only = true];
// The span name will be derived from this field. If
// :ref:`traffic_direction <envoy_api_field_Listener.traffic_direction>` is
// specified on the parent listener, then it is used instead of this field.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be marked as deprecated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should deprecate this rather than having this override thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't reliably tell if the field is present or not (due to a bad choice of the default).
That means we have to respect it until we can remove it from the API (which will happen according to the regular deprecation policy).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But you don't want to add [deprecated = true] to the field's options yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK adding deprecated = true, just waiting for the input from API shepherds.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, let's deprecate, thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

//
// .. attention::
// This field has been deprecated in favor of `traffic_direction`.
OperationName operation_name = 1 [(validate.rules).enum.defined_only = true, deprecated = true];

// A list of header names used to create tags for the active span. The header name is used to
// populate the tag name, and the header value is used to populate the tag value. The tag is
Expand Down
24 changes: 19 additions & 5 deletions source/extensions/filters/network/http_connection_manager/config.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,13 +244,27 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig(
Tracing::OperationName tracing_operation_name;
std::vector<Http::LowerCaseString> request_headers_for_tags;

switch (tracing_config.operation_name()) {
case envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager::
Tracing::INGRESS:
// Listener level traffic direction overrides the operation name
switch (context.direction()) {
case envoy::api::v2::core::TrafficDirection::UNSPECIFIED: {
switch (tracing_config.operation_name()) {
case envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager::
Tracing::INGRESS:
tracing_operation_name = Tracing::OperationName::Ingress;
break;
case envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager::
Tracing::EGRESS:
tracing_operation_name = Tracing::OperationName::Egress;
break;
default:
NOT_REACHED_GCOVR_EXCL_LINE;
}
break;
}
case envoy::api::v2::core::TrafficDirection::INBOUND:
tracing_operation_name = Tracing::OperationName::Ingress;
break;
case envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager::
Tracing::EGRESS:
case envoy::api::v2::core::TrafficDirection::OUTBOUND:
tracing_operation_name = Tracing::OperationName::Egress;
break;
default:
Expand Down
56 changes: 56 additions & 0 deletions test/extensions/filters/network/http_connection_manager/config_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,62 @@ stat_prefix: router
EXPECT_EQ(5 * 60 * 1000, config.streamIdleTimeout().count());
}

TEST_F(HttpConnectionManagerConfigTest, ListenerDirectionOutboundOverride) {
const std::string yaml_string = R"EOF(
stat_prefix: router
route_config:
virtual_hosts:
- name: service
domains:
- "*"
routes:
- match:
prefix: "/"
route:
cluster: cluster
tracing:
operation_name: ingress
http_filters:
- name: envoy.router
config: {}
)EOF";

ON_CALL(context_, direction())
.WillByDefault(Return(envoy::api::v2::core::TrafficDirection::OUTBOUND));
HttpConnectionManagerConfig config(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_,
date_provider_, route_config_provider_manager_,
scoped_routes_config_provider_manager_);
EXPECT_EQ(Tracing::OperationName::Egress, config.tracingConfig()->operation_name_);
}

TEST_F(HttpConnectionManagerConfigTest, ListenerDirectionInboundOverride) {
const std::string yaml_string = R"EOF(
stat_prefix: router
route_config:
virtual_hosts:
- name: service
domains:
- "*"
routes:
- match:
prefix: "/"
route:
cluster: cluster
tracing:
operation_name: egress
http_filters:
- name: envoy.router
config: {}
)EOF";

ON_CALL(context_, direction())
.WillByDefault(Return(envoy::api::v2::core::TrafficDirection::INBOUND));
HttpConnectionManagerConfig config(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_,
date_provider_, route_config_provider_manager_,
scoped_routes_config_provider_manager_);
EXPECT_EQ(Tracing::OperationName::Ingress, config.tracingConfig()->operation_name_);
}

TEST_F(HttpConnectionManagerConfigTest, SamplingDefault) {
const std::string yaml_string = R"EOF(
stat_prefix: ingress_http
Expand Down