Skip to content

add upstream info #2434

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

Closed
kyessenov wants to merge 1 commit into from
Closed

add upstream info #2434

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
6 changes: 6 additions & 0 deletions include/istio/control/http/report_data.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,12 @@ class ReportData {
// Useful for logging info generated by custom codecs.
virtual const ::google::protobuf::Map<std::string, ::google::protobuf::Struct>
&GetDynamicFilterState() const = 0;

// Get upstream mTLS status.
virtual bool IsUpstreamMutualTLS() const = 0;

// Get upstream failure reason.
virtual const std::string &GetUpstreamFailureReason() const = 0;
};

} // namespace http
Expand Down
5 changes: 5 additions & 0 deletions include/istio/control/tcp/report_data.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,11 @@ class ReportData {
virtual const ::google::protobuf::Map<::std::string,
::google::protobuf::Struct>
&GetDynamicFilterState() const = 0;

// Get upstream mTLS status.
virtual bool IsUpstreamMutualTLS() const = 0;

virtual const std::string &GetUpstreamFailureReason() const = 0;
};

} // namespace tcp
Expand Down
2 changes: 2 additions & 0 deletions include/istio/utils/attribute_names.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,8 @@ struct AttributeName {
static const char kConnectionId[];
// Record TCP connection status: open, continue, close
static const char kConnectionEvent[];
static const char kUpstreamMtls[];
Copy link
Copy Markdown
Member

@nrjpoddar nrjpoddar Sep 30, 2019

Choose a reason for hiding this comment

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

how does work with this attribute kConnectionMtls

Copy link
Copy Markdown
Contributor Author

@kyessenov kyessenov Sep 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

connection.mtls means downstream connection mTLS status.
upstream.mtls means upstream connection mTLS status.
Depending on whether the proxy acts like an outbound or an inbound sidecar, you use one or the other.

Copy link
Copy Markdown
Member

@nrjpoddar nrjpoddar Sep 30, 2019

Choose a reason for hiding this comment

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

i see, may be this attribute kConnectionMtls needs to explicitly say that it's related to downstream. It's can be confusing specially after adding this new attribute.

Copy link
Copy Markdown
Contributor

@lizan lizan Oct 2, 2019

Choose a reason for hiding this comment

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

+1

Copy link
Copy Markdown
Contributor Author

@kyessenov kyessenov Oct 2, 2019

Choose a reason for hiding this comment

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

We can't change the meaning of existing attributes. There are UIs that have expectations. We can change the metric, however, in istio config.

static const char kUpstreamFailureReason[];

// Context attributes
static const char kContextProtocol[];
Expand Down
8 changes: 8 additions & 0 deletions src/envoy/http/mixer/report_data.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,14 @@ class ReportData : public ::istio::control::http::ReportData,
&GetDynamicFilterState() const override {
return info_.dynamicMetadata().filter_metadata();
}

bool IsUpstreamMutualTLS() const override {
return Utils::IsUpstreamMutualTLS(info_);
}

const std::string &GetUpstreamFailureReason() const override {
return info_.upstreamTransportFailureReason();
}
};

} // namespace Mixer
Expand Down
11 changes: 11 additions & 0 deletions src/envoy/tcp/mixer/filter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,17 @@ std::string Filter::GetConnectionId() const {
return uuid_connection_id;
}

bool Filter::IsUpstreamMutualTLS() const {
return Utils::IsUpstreamMutualTLS(
filter_callbacks_->connection().streamInfo());
}

const std::string &Filter::GetUpstreamFailureReason() const {
return filter_callbacks_->connection()
.streamInfo()
.upstreamTransportFailureReason();
}

void Filter::OnReportTimer() {
handler_->Report(this, ConnectionEvent::CONTINUE);
clearCachedFilterMetadata();
Expand Down
2 changes: 2 additions & 0 deletions src/envoy/tcp/mixer/filter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ class Filter : public Network::Filter,
void GetReportInfo(
::istio::control::tcp::ReportData::ReportInfo *data) const override;
std::string GetConnectionId() const override;
bool IsUpstreamMutualTLS() const override;
const std::string &GetUpstreamFailureReason() const override;

void cacheFilterMetadata(
const ::google::protobuf::Map<std::string, ::google::protobuf::Struct>
Expand Down
5 changes: 5 additions & 0 deletions src/envoy/utils/utils.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,11 @@ bool IsMutualTLS(const Network::Connection* connection) {
connection->ssl()->peerCertificatePresented();
}

bool IsUpstreamMutualTLS(const StreamInfo::StreamInfo& stream_info) {
const auto ssl = stream_info.upstreamSslConnection();
return ssl != nullptr && ssl->peerCertificatePresented();
Copy link
Copy Markdown
Member

@nrjpoddar nrjpoddar Oct 2, 2019

Choose a reason for hiding this comment

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

I'm a little confused on the usage of ssl->peerCertificatePresented(). If this flag is set to true, does this imply client was using TLS for talking to upstream and got a successful certificate from upstream or it should mean that client is using TLS irrespective of how the negotiation went and what upstream server presented?

Copy link
Copy Markdown
Contributor Author

@kyessenov kyessenov Oct 2, 2019

Choose a reason for hiding this comment

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

Ouch, I think this might be an issue. We should be checking whether a local certificate was presented, not peer.

Copy link
Copy Markdown
Member

@nrjpoddar nrjpoddar Oct 2, 2019

Choose a reason for hiding this comment

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

that's what I thought. I don't see an Envoy API for that. @lizan may be you know better?

Copy link
Copy Markdown
Contributor Author

@kyessenov kyessenov Oct 2, 2019

Choose a reason for hiding this comment

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

We can extend envoy to report whether a local certificate is presented. That might not mean that the server validated it. Not sure how to get that info.

Copy link
Copy Markdown
Contributor

@lizan lizan Oct 2, 2019

Choose a reason for hiding this comment

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

we didn't have this in Envoy API because this has been simply reflecting what in the TLS context config, though we're adding more dynamic nature to the config for incremental adoption etc, so it makes sense to add, thanks for posting envoyproxy/envoy#8464.

}

bool GetRequestedServerName(const Network::Connection* connection,
std::string* name) {
if (connection && !connection->requestedServerName().empty()) {
Expand Down
5 changes: 4 additions & 1 deletion src/envoy/utils/utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,12 @@ bool GetPrincipal(const Network::Connection* connection, bool peer,
bool GetTrustDomain(const Network::Connection* connection, bool peer,
std::string* trust_domain);

// Returns true if connection is mutual TLS enabled.
// Returns true if downstream connection is mutual TLS enabled.
Copy link
Copy Markdown
Contributor

@lizan lizan Oct 2, 2019

Choose a reason for hiding this comment

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

rename this to IsDownstreamMutualTLS?

bool IsMutualTLS(const Network::Connection* connection);

// Returns true if upstream connection is mutual TLS enabled.
bool IsUpstreamMutualTLS(const StreamInfo::StreamInfo& stream_info);

// Get requested server name, SNI in case of TLS
bool GetRequestedServerName(const Network::Connection* connection,
std::string* name);
Expand Down
8 changes: 8 additions & 0 deletions src/istio/control/http/attributes_builder.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,14 @@ void AttributesBuilder::ExtractReportAttributes(
}

builder.FlattenMapOfStringToStruct(report_data->GetDynamicFilterState());

builder.AddBool(utils::AttributeName::kUpstreamMtls,
report_data->IsUpstreamMutualTLS());
Copy link
Copy Markdown
Member

@rshriram rshriram Oct 2, 2019

Choose a reason for hiding this comment

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

just a nit. instead of having a bool for mutual TLs, why not have an enum of sorts? upstream: plaintext, tls, mutualTLS ? We are coming across a lot of setups where the sidecar has to terminate one way TLS on VMs.

Copy link
Copy Markdown
Contributor Author

@kyessenov kyessenov Oct 2, 2019

Choose a reason for hiding this comment

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

Enums are generally painful to deal with in proto. mTLS is special since that's the end state, so it's needed to track the progress on getting to that state. We can populate other TLS properties later (e.g. version, cipher suite ID, etc).

const std::string &failure_reason = report_data->GetUpstreamFailureReason();
if (!failure_reason.empty()) {
builder.AddString(utils::AttributeName::kUpstreamFailureReason,
failure_reason);
}
}

} // namespace http
Expand Down
22 changes: 22 additions & 0 deletions src/istio/control/http/attributes_builder_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -438,6 +438,18 @@ attributes {
}
}
}
attributes {
key: "upstream.mtls"
value {
bool_value: false
}
}
attributes {
key: "upstream.failure_reason"
value {
string_value: "wrong cert"
}
}
)";

constexpr char kAuthenticationResultStruct[] = R"(
Expand Down Expand Up @@ -809,6 +821,11 @@ TEST(AttributesBuilderTest, TestReportAttributes) {
}));
EXPECT_CALL(mock_data, GetDynamicFilterState())
.WillOnce(ReturnRef(filter_metadata));
EXPECT_CALL(mock_data, IsUpstreamMutualTLS())
.WillOnce(testing::Return(false));
std::string failure_reason = "wrong cert";
EXPECT_CALL(mock_data, GetUpstreamFailureReason())
.WillOnce(ReturnRef(failure_reason));

istio::mixer::v1::Attributes attributes;
AttributesBuilder builder(&attributes);
Expand Down Expand Up @@ -889,6 +906,11 @@ TEST(AttributesBuilderTest, TestReportAttributesWithDestIP) {
}));
EXPECT_CALL(mock_data, GetDynamicFilterState())
.WillOnce(ReturnRef(filter_metadata));
EXPECT_CALL(mock_data, IsUpstreamMutualTLS())
.WillOnce(testing::Return(false));
std::string failure_reason = "wrong cert";
EXPECT_CALL(mock_data, GetUpstreamFailureReason())
.WillOnce(ReturnRef(failure_reason));

istio::mixer::v1::Attributes attributes;
SetDestinationIp(&attributes, "1.2.3.4");
Expand Down
2 changes: 2 additions & 0 deletions src/istio/control/http/mock_report_data.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ class MockReportData : public ReportData {
GetDynamicFilterState,
const ::google::protobuf::Map<std::string, ::google::protobuf::Struct>
&());
MOCK_CONST_METHOD0(IsUpstreamMutualTLS, bool());
MOCK_CONST_METHOD0(GetUpstreamFailureReason, const std::string &());
};

} // namespace http
Expand Down
11 changes: 11 additions & 0 deletions src/istio/control/http/request_handler_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,6 +395,13 @@ TEST_F(RequestHandlerImplTest, TestHandlerReport) {
EXPECT_CALL(mock_report, GetDynamicFilterState())
.Times(1)
.WillOnce(ReturnRef(filter_metadata));
EXPECT_CALL(mock_report, IsUpstreamMutualTLS())
.Times(1)
.WillOnce(testing::Return(true));
const std::string empty;
EXPECT_CALL(mock_report, GetUpstreamFailureReason())
.Times(1)
.WillOnce(ReturnRef(empty));

// Report should be called.
EXPECT_CALL(*mock_client_, Report(_)).Times(1);
Expand All @@ -414,6 +421,8 @@ TEST_F(RequestHandlerImplTest, TestHandlerDisabledReport) {
EXPECT_CALL(mock_report, GetResponseHeaders()).Times(0);
EXPECT_CALL(mock_report, GetReportInfo(_)).Times(0);
EXPECT_CALL(mock_report, GetDynamicFilterState()).Times(0);
EXPECT_CALL(mock_report, IsUpstreamMutualTLS()).Times(0);
EXPECT_CALL(mock_report, GetUpstreamFailureReason()).Times(0);

// Report should NOT be called.
EXPECT_CALL(*mock_client_, Report(_)).Times(0);
Expand Down Expand Up @@ -452,6 +461,8 @@ TEST_F(RequestHandlerImplTest, TestEmptyConfig) {
EXPECT_CALL(mock_report, GetResponseHeaders()).Times(0);
EXPECT_CALL(mock_report, GetReportInfo(_)).Times(0);
EXPECT_CALL(mock_report, GetDynamicFilterState()).Times(0);
EXPECT_CALL(mock_report, IsUpstreamMutualTLS()).Times(0);
EXPECT_CALL(mock_report, GetUpstreamFailureReason()).Times(0);

// Report should NOT be called.
EXPECT_CALL(*mock_client_, Report(_)).Times(0);
Expand Down
9 changes: 9 additions & 0 deletions src/istio/control/tcp/attributes_builder.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,15 @@ void AttributesBuilder::ExtractReportAttributes(

builder.AddTimestamp(utils::AttributeName::kContextTime,
std::chrono::system_clock::now());

builder.AddBool(utils::AttributeName::kUpstreamMtls,
report_data->IsUpstreamMutualTLS());

const std::string &failure_reason = report_data->GetUpstreamFailureReason();
if (!failure_reason.empty()) {
builder.AddString(utils::AttributeName::kUpstreamFailureReason,
failure_reason);
}
}

} // namespace tcp
Expand Down
28 changes: 28 additions & 0 deletions src/istio/control/tcp/attributes_builder_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,12 @@ attributes {
}
}
}
attributes {
key: "upstream.mtls"
value {
bool_value: true
}
}
)";

const char kReportAttributes[] = R"(
Expand Down Expand Up @@ -271,6 +277,12 @@ attributes {
}
}
}
attributes {
key: "upstream.mtls"
value {
bool_value: true
}
}
)";

const char kDeltaOneReportAttributes[] = R"(
Expand Down Expand Up @@ -344,6 +356,12 @@ attributes {
}
}
}
attributes {
key: "upstream.mtls"
value {
bool_value: true
}
}
)";

const char kDeltaTwoReportAttributes[] = R"(
Expand Down Expand Up @@ -417,6 +435,12 @@ attributes {
}
}
}
attributes {
key: "upstream.mtls"
value {
bool_value: true
}
}
)";

void ClearContextTime(istio::mixer::v1::Attributes *attributes) {
Expand Down Expand Up @@ -526,6 +550,10 @@ TEST(AttributesBuilderTest, TestReportAttributes) {
info->send_bytes = 678;
info->duration = std::chrono::nanoseconds(4);
}));
EXPECT_CALL(mock_data, IsUpstreamMutualTLS()).WillRepeatedly(Return(true));
const std::string failure_reason;
EXPECT_CALL(mock_data, GetUpstreamFailureReason())
.WillRepeatedly(ReturnRef(failure_reason));

istio::mixer::v1::Attributes attributes;
AttributesBuilder builder(&attributes);
Expand Down
2 changes: 2 additions & 0 deletions src/istio/control/tcp/mock_report_data.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ class MockReportData : public ReportData {
const ::google::protobuf::Map<std::string, ::google::protobuf::Struct>
&());
MOCK_CONST_METHOD1(GetReportInfo, void(ReportInfo *info));
MOCK_CONST_METHOD0(IsUpstreamMutualTLS, bool());
MOCK_CONST_METHOD0(GetUpstreamFailureReason, const std::string &());
};

} // namespace tcp
Expand Down
7 changes: 7 additions & 0 deletions src/istio/control/tcp/request_handler_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,13 @@ TEST_F(RequestHandlerImplTest, TestHandlerReport) {
EXPECT_CALL(mock_data, GetDynamicFilterState())
.Times(1)
.WillOnce(ReturnRef(filter_metadata));
EXPECT_CALL(mock_data, IsUpstreamMutualTLS())
.Times(1)
.WillOnce(testing::Return(true));
const std::string empty;
EXPECT_CALL(mock_data, GetUpstreamFailureReason())
.Times(1)
.WillOnce(ReturnRef(empty));

// Report should be called.
EXPECT_CALL(*mock_client_, Report(_)).Times(1);
Expand Down
2 changes: 2 additions & 0 deletions src/istio/utils/attribute_names.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,8 @@ const char AttributeName::kConnectionDuration[] = "connection.duration";
const char AttributeName::kConnectionMtls[] = "connection.mtls";
const char AttributeName::kConnectionRequestedServerName[] =
"connection.requested_server_name";
const char AttributeName::kUpstreamMtls[] = "upstream.mtls";
const char AttributeName::kUpstreamFailureReason[] = "upstream.failure_reason";

// Downstream TCP connection id.
const char AttributeName::kConnectionId[] = "connection.id";
Expand Down