Add upstream and downstream info in parent read callbacks in tcp too by gargnupur · Pull Request #9949 · envoyproxy/envoy

gargnupur · 2020-02-06T20:15:00Z

Add upstream and downstream info in parent read callbacks in tcp too
Ref: istio/istio#20802
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 upstream and downstream info in parent read callbacks in tcp too
Http is already doing this because of which in decoder and encoder callbacks can get information like upstream host which is not available in TCP
Risk Level: Low
Testing:
Docs Changes:
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]

mandarjog · 2020-02-07T22:24:06Z

cc @lizan

gargnupur · 2020-02-10T18:15:27Z

@lizan , @asraa, @mattklein123 : Can you please review this PR? This is a blocker for TCP telemetry v2 in Istio...

kyessenov

Thanks!

lizan

What's the difference with read_callbacks_->connection().streamInfo() and getStreamInfo() (stream_info_)? From a quick glance getStreamInfo() is used for access logging purpose, this seems due to a historical reason: to support TCP access logging before we have StreamInfo in Connection interface. Now it seems we can consolidate them. cc @mattklein123

gargnupur · 2020-02-10T22:37:40Z

@lizan , @mattklein123 : AFAIU, yeah looks like getStreamInfo is maintained just for access logging purposes and we can just maintain one stream info in connection call.. I can volunteer to refactor streaminfo to do that after this PR is in...

mattklein123 · 2020-02-10T22:41:41Z

Yes please consolidate. We can do it in this PR, it should not be difficult, thanks!

/wait

gargnupur · 2020-02-11T00:32:18Z

@mattklein123 , @lizan : just want to make sure, that the connection callback(read_callback_) has a longer life span than the TCP filter object(https://sourcegraph.com/github.com/envoyproxy/envoy@master/-/blob/source/common/tcp_proxy/tcp_proxy.cc#L214:1&tab=def) . We access streamInfo on destruction of filter object and after changing to use connection's streaminfo, tcp_proxy access log tests are failing. It could be because I need to implement some stuff in mock streaminfo but just wanted to check before I do that...

mattklein123 · 2020-02-11T17:43:01Z

I'm sure of the specific issue you are facing but AFAIK the lifetime should be fine as we use this in other filters as well, so I'm guessing it's a test issue.

gargnupur · 2020-02-11T17:59:54Z

Thanks for confirming! I am fixing the test right now.

On Tue, Feb 11, 2020 at 9:43 AM Matt Klein ***@***.***> wrote: I'm sure of the specific issue you are facing but AFAIK the lifetime should be fine as we use this in other filters as well, so I'm guessing it's a test issue. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#9949?email_source=notifications&email_token=AI634YRXZXGN6JDK7ORY4Y3RCLPSPA5CNFSM4KRDJJD2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELNMCUQ#issuecomment-584761682>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AI634YTANUB4WQG2FOYJF2DRCLPSPANCNFSM4KRDJJDQ> .

-- Thanks, Nupur

Ref: istio/istio#20802 Signed-off-by: gargnupur <gargnupur@google.com> Add test Signed-off-by: gargnupur <gargnupur@google.com> Use streaminfo from connection callback as compared to maintaining an extra one just for access logging Signed-off-by: gargnupur <gargnupur@google.com>

gargnupur · 2020-02-11T23:30:50Z

@mattklein123 , @lizan , @kyessenov : PTAL again.. running tests locally too to see if any more tests need to be changed...

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

mattklein123 · 2020-02-12T19:51:36Z

Please check format.

/wait

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

mattklein123

LGTM at a high level, thanks! I will defer to @lizan for final review/merge.

mattklein123 · 2020-02-12T20:48:48Z

source/common/tcp_proxy/tcp_proxy.cc

 }

+StreamInfo::StreamInfo& Filter::getStreamInfo() {
+  ASSERT(read_callbacks_ != nullptr);


nit: del as this will crash in an obvious way on the next line.

Thanks.. Will remove

lizan · 2020-02-12T00:28:08Z

source/common/tcp_proxy/tcp_proxy.cc


-Filter::Filter(ConfigSharedPtr config, Upstream::ClusterManager& cluster_manager,
-               TimeSource& time_source)
+Filter::Filter(ConfigSharedPtr config, Upstream::ClusterManager& cluster_manager, TimeSource&)


TimeSource& is no longer needed in the signature then?

gargnupur · 2020-02-12T21:49:58Z

@mattklein123 : Thanks for the review!
@lizan: There are some more tests failing and will update the PR soon with the fixes for them.
Adding wait label till then.
/wait

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

gargnupur · 2020-02-13T00:38:38Z

@kyessenov , @mattklein123 , @lizan : the only test that's failing now is the //test/common/upstream:health_checker_impl_test and it's failing at the verification of start time in HttpHealthCheckerImplTest.SuccessServiceCheckWithAdditionalHeaders test case.
I am not sure about implementation so much but test expects that whenever encodeHeaders is called, this value changes.. why is that the case..
With my changes, it's not changing now..

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

gargnupur · 2020-02-13T07:59:25Z

@kyessenov , @mattklein123 , @lizan : Fixed health check test too (for some reason, I was trying same thing before and it was not working :( ) Now, test is more close to what should happen in code too.. PTAL

gargnupur · 2020-02-14T04:41:26Z

@lizan : can you please take a look at this? Hoping to get this in before Istio Community Testing on 18th...

gargnupur · 2020-02-14T18:02:54Z

@mattklein123 , @lizan : Looks like it didn't get merged by itself, can you please help merge this?

…nvoyproxy#9949) Signed-off-by: gargnupur <gargnupur@google.com>

Add upstream and downstream info in parent read callbacks in tcp too(envoyproxy#9949)

gargnupur requested review from PiotrSikora, jplevyak, kyessenov and mattklein123 February 6, 2020 20:15

gargnupur mentioned this pull request Feb 6, 2020

Add access to upstreamfilterstate in WasmStateWrapper envoyproxy/envoy-wasm#394

Merged

junr03 assigned asraa Feb 7, 2020

gargnupur requested a review from lizan February 10, 2020 18:15

kyessenov previously approved these changes Feb 10, 2020

View reviewed changes

lizan reviewed Feb 10, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Feb 10, 2020

mattklein123 self-assigned this Feb 10, 2020

gargnupur dismissed kyessenov’s stale review via 6e7654e February 11, 2020 23:25

repokitteh-read-only bot removed the waiting label Feb 11, 2020

gargnupur force-pushed the nup_fix_tcp branch from 6e7654e to 276def2 Compare February 11, 2020 23:29

howardjohn mentioned this pull request Feb 11, 2020

1.5 regression: tcp metrics missing destination_service istio/istio#21044

Closed

Fix tests

0fab79c

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

repokitteh-read-only bot added the waiting label Feb 12, 2020

Fix fmt

a7b0412

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

repokitteh-read-only bot removed the waiting label Feb 12, 2020

mattklein123 previously approved these changes Feb 12, 2020

View reviewed changes

mattklein123 assigned lizan and unassigned mattklein123 Feb 12, 2020

lizan reviewed Feb 12, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Feb 12, 2020

Fixed based on feedback and fix tests

de05ee0

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

gargnupur dismissed mattklein123’s stale review via de05ee0 February 12, 2020 23:56

gargnupur requested review from alyssawilk and zuercher as code owners February 12, 2020 23:56

repokitteh-read-only bot removed the waiting label Feb 12, 2020

Fix health check test

870db7e

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

mattklein123 mentioned this pull request Feb 13, 2020

Proxy proto header generation #1031

Closed

lizan approved these changes Feb 14, 2020

View reviewed changes

mattklein123 merged commit fade668 into envoyproxy:master Feb 14, 2020

gargnupur added a commit to gargnupur/envoy that referenced this pull request Feb 15, 2020

Add upstream and downstream info in parent read callbacks in tcp too (e…

01ad97f

…nvoyproxy#9949) Signed-off-by: gargnupur <gargnupur@google.com>

gargnupur added a commit to gargnupur/envoy that referenced this pull request Feb 15, 2020

Add upstream and downstream info in parent read callbacks in tcp too (e…

ca5bef6

…nvoyproxy#9949) Signed-off-by: gargnupur <gargnupur@google.com>

gargnupur added a commit to gargnupur/envoy that referenced this pull request Feb 15, 2020

Add upstream and downstream info in parent read callbacks in tcp too (e…

22b9eb3

…nvoyproxy#9949) Signed-off-by: gargnupur <gargnupur@google.com>

gargnupur mentioned this pull request Feb 16, 2020

Add upstream and downstream info in parent read callbacks in tcp too(#9949) istio/envoy#150

Merged

kyessenov added a commit to istio/envoy that referenced this pull request Feb 18, 2020

Merge pull request #150 from gargnupur/nup_ch4

1485c93

Add upstream and downstream info in parent read callbacks in tcp too(envoyproxy#9949)

gargnupur mentioned this pull request Feb 25, 2020

Add myself to istio/wg-networking-maintainers-data-plane team member istio/community#282

Merged

Conversation

gargnupur commented Feb 6, 2020

Uh oh!

mandarjog commented Feb 7, 2020

Uh oh!

gargnupur commented Feb 10, 2020

Uh oh!

kyessenov left a comment

Choose a reason for hiding this comment

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

gargnupur commented Feb 10, 2020

Uh oh!

mattklein123 commented Feb 10, 2020

Uh oh!

gargnupur commented Feb 11, 2020

Uh oh!

mattklein123 commented Feb 11, 2020

Uh oh!

gargnupur commented Feb 11, 2020 via email

Uh oh!

gargnupur commented Feb 11, 2020

Uh oh!

mattklein123 commented Feb 12, 2020

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 Feb 12, 2020

Choose a reason for hiding this comment

Uh oh!

gargnupur Feb 12, 2020

Choose a reason for hiding this comment

Uh oh!

lizan Feb 12, 2020

Choose a reason for hiding this comment

Uh oh!

gargnupur Feb 12, 2020

Choose a reason for hiding this comment

Uh oh!

gargnupur commented Feb 12, 2020

Uh oh!

gargnupur commented Feb 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gargnupur commented Feb 13, 2020

Uh oh!

gargnupur commented Feb 14, 2020

Uh oh!

gargnupur commented Feb 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

gargnupur commented Feb 13, 2020 •

edited

Loading