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

Add ClusterInfo as a property in StreamInfo #10432

Merged
merged 11 commits into from
Mar 20, 2020
Merged

Add ClusterInfo as a property in StreamInfo #10432

merged 11 commits into from
Mar 20, 2020

Conversation

gargnupur
Copy link
Contributor

Signed-off-by: gargnupur gargnupur@google.com

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description:Add ClusterName as a property in StreamInfo. This is because for TCP, there is no RouteEntry and thus we are not able to get clustername for TCP in scenarios where upstream cluster is not healthy
Risk Level: Low
Testing: Unit Tests
Docs Changes:
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]

@gargnupur
Add ClusterName as a property in StreamInfo
06996b8 
Signed-off-by: gargnupur <gargnupur@google.com>
@gargnupur gargnupur mentioned this pull request Mar 18, 2020
Closed
@gargnupur
fix lint
388a740 
Signed-off-by: gargnupur <gargnupur@google.com>
@gargnupur
fix lint
4e44e9f 
Signed-off-by: gargnupur <gargnupur@google.com>
kyessenov
kyessenov reviewed Mar 18, 2020
View reviewed changes
test/common/tcp_proxy/tcp_proxy_test.cc Outdated
@@ -1927,6 +1927,23 @@ TEST_F(TcpProxyRoutingTest, DEPRECATED_FEATURE_TEST(ApplicationProtocols)) {
filter_->onNewConnection();
}

TEST_F(TcpProxyRoutingTest, DEPRECATED_FEATURE_TEST(ClusterNameSet)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this a deprecated feature test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure why all other tests for TcpProxyRoutingTest are marked DEPRECATED_FEATURE_TEST, hence I marked my test as the same...
will remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed, because TcpProxyRoutingTest is using deprecated_v1 config.
@mattklein123 : what's the plan for such classes? should we add new tests somewhere else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Other then that, PR is ready for your review.. PTAL

Copy link
Member

Choose a reason for hiding this comment

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

Please make a new test to test this feature which doesn't use deprecated config, then you can remove the DEPRECATED_FEATURE_TEST

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.. PTAL

mattklein123
mattklein123 reviewed Mar 18, 2020
View reviewed changes
include/envoy/stream_info/stream_info.h Outdated
/**
* @param Upstream connection's ClusterName.
*/
virtual void setUpstreamClusterName(const absl::string_view upstream_cluster_name) PURE;
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 this would be a lot more useful if we snap the ClusterInfo shared_ptr. Is there any reason not to do that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks changed to ClusterInfoConstSharedPtr...

@gargnupur
Change to use ClusterInfo instead of ClusterName
0802de2 
Signed-off-by: gargnupur <gargnupur@google.com>
@gargnupur gargnupur changed the title Add ClusterName as a property in StreamInfo Add ClusterInfo as a property in StreamInfo Mar 18, 2020
@gargnupur
Remove Deprecated from test
57dd8d6 
Signed-off-by: gargnupur <gargnupur@google.com>
@gargnupur
Remove left over comments
db6a87b 
Signed-off-by: gargnupur <gargnupur@google.com>
@gargnupur
Add Deprecated Option in Test
815e7f4 
Signed-off-by: gargnupur <gargnupur@google.com>
@gargnupur
trying to kick circle ci test again
bd32542 
Signed-off-by: gargnupur <gargnupur@google.com>
@gargnupur
Copy link
Contributor Author

@mattklein123 , @kyessenov : all the tests are passing, can you please review it again?

@mattklein123 mattklein123 self-assigned this Mar 19, 2020
mattklein123
mattklein123 requested changes Mar 19, 2020
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

LGTM modulo some small comments. Thank you!

/wait

include/envoy/stream_info/stream_info.h Show resolved Hide resolved
test/common/tcp_proxy/tcp_proxy_test.cc Outdated
@@ -1927,6 +1927,23 @@ TEST_F(TcpProxyRoutingTest, DEPRECATED_FEATURE_TEST(ApplicationProtocols)) {
filter_->onNewConnection();
}

TEST_F(TcpProxyRoutingTest, DEPRECATED_FEATURE_TEST(ClusterNameSet)) {
Copy link
Member

Choose a reason for hiding this comment

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

Please make a new test to test this feature which doesn't use deprecated config, then you can remove the DEPRECATED_FEATURE_TEST

@gargnupur
Removed deprecated test
b939fb0 
Signed-off-by: gargnupur <gargnupur@google.com>
@gargnupur
Add Api notes
bd2cfae 
Signed-off-by: gargnupur <gargnupur@google.com>
kyessenov
kyessenov previously approved these changes Mar 19, 2020
View reviewed changes
Copy link
Contributor

@kyessenov kyessenov left a comment

Choose a reason for hiding this comment

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

Thanks! This is what I had in mind to generalize cluster name to a cluster info.

mattklein123
mattklein123 requested changes Mar 19, 2020
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks LGTM modulo comment comment.

/wait

include/envoy/stream_info/stream_info.h Outdated Show resolved Hide resolved
@gargnupur @mattklein123
Update include/envoy/stream_info/stream_info.h
8ef276d 
Co-Authored-By: Matt Klein <mattklein123@gmail.com>
Signed-off-by: gargnupur <gargnupur@google.com>
@gargnupur gargnupur force-pushed the nup_add_cluster_name branch from 6d85104 to 8ef276d Compare March 19, 2020 21:22
@mattklein123
Copy link
Member

@gargnupur please never force push. It makes reviews much more difficult. Thank you!

mattklein123
mattklein123 approved these changes Mar 19, 2020
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks!

@gargnupur
Copy link
Contributor Author

@gargnupur please never force push. It makes reviews much more difficult. Thank you!

@mattklein123 : Thanks for the feedback.. didn't know that.

@gargnupur
Copy link
Contributor Author

@mattklein123 , @lizan : Can you guys please help merge this?

@lizan lizan merged commit 9186ebf into envoyproxy:master Mar 20, 2020
@gargnupur
Copy link
Contributor Author

Thanks @mattklein123, @lizan and @kyessenov for the review and helping it merge it.

gargnupur added a commit to gargnupur/envoy that referenced this pull request Mar 20, 2020
@gargnupur @mattklein123
Add ClusterInfo as a property in StreamInfo (envoyproxy#10432)
42b94b8 
Add ClusterName as a property in StreamInfo. This is because for TCP, there is no RouteEntry and thus we are not able to get clustername for TCP in scenarios where upstream cluster is not healthy

Risk Level: Low
Testing: Unit Tests
Docs Changes:
Release Notes:

Signed-off-by: gargnupur <gargnupur@google.com>
Co-authored-by: Matt Klein <mattklein123@gmail.com>
istio-testing pushed a commit to istio/envoy that referenced this pull request Mar 20, 2020
@gargnupur @mattklein123
Add ClusterInfo as a property in StreamInfo (envoyproxy#10432) (#188)
98e5945 
Add ClusterName as a property in StreamInfo. This is because for TCP, there is no RouteEntry and thus we are not able to get clustername for TCP in scenarios where upstream cluster is not healthy

Risk Level: Low
Testing: Unit Tests
Docs Changes:
Release Notes:

Signed-off-by: gargnupur <gargnupur@google.com>
Co-authored-by: Matt Klein <mattklein123@gmail.com>

Co-authored-by: Matt Klein <mattklein123@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattklein123 mattklein123 mattklein123 approved these changes

@kyessenov kyessenov kyessenov left review comments

Assignees

@mattklein123 mattklein123

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gargnupur @mattklein123 @kyessenov @lizan