Adding Alpn for TCP metadata exchange support by mandarjog · Pull Request #20126 · istio/istio

mandarjog · 2020-01-13T17:57:32Z

Please specifically review DownstreamTlsContext we add istio-peer-exchange as the first one in order along with http and for UpstreamTlsContext, add istio-peer-exchange with istio for TCP.

Doc for ref: https://docs.google.com/document/d/1s6ou__qRL4UiWY1amyk5p8YzOMQvBhfR15o1U_ZLIow/edit#heading=h.7zgnj8bwqfld

howardjohn

cc @yxue

howardjohn · 2020-01-13T18:00:45Z

go.mod

is this part of the WIP?

mandarjog · 2020-01-13T18:02:56Z

@yxue @diemtvu please review per @gargnupur s email. Thanks.

yxue · 2020-01-13T18:24:42Z

pilot/pkg/networking/core/v1alpha3/gateway.go

Why changing the TLS context for gateway?

Thanks for pointing.. removing

pilot/pkg/networking/core/v1alpha3/listener.go

rshriram · 2020-01-13T19:06:48Z

Pay attention to upgrades please.

What happens if a 1.4 proxy gets this config and tries to talk to another 1.4 proxy that hasn't gotten this config?
What happens if a 1.5 proxy tries to talk to a 1.4 proxy that got this config (with the new alpn) ?

yxue · 2020-01-13T19:41:21Z

My initial thoughts is that we should only use this ALPN for proxy 1.5. If one of the client or server is using proxies with other versions, the metadata exchange filters will keep silent since one of them doesn't provide ALPN "istio-peer-exchange".

gargnupur · 2020-01-14T01:39:07Z

@rshriram : @yxue pointed it right, in terms of upgrades if one of the proxy is a different version, ALPN "istio-peer-exchange" will not get negotiated and metadata exchange filter will not do anything as it won't find "istio-peer-exchange" as the negotiated alpn.

mandarjog · 2020-01-14T01:49:16Z

@rshriram Should we put this behind a feature flag, in this case on by default?

mandarjog · 2020-01-14T01:51:25Z

@gargnupur we also need integration/e2e tests with this pr.

yxue · 2020-01-14T06:50:44Z

@rshriram : @yxue pointed it right, in terms of upgrades if one of the proxy is a different version, ALPN "istio-peer-exchange" will not get negotiated and metadata exchange filter will not do anything as it won't find "istio-peer-exchange" as the negotiated alpn.

I think we should put the code behind the feature flag as @mandarjog pointed out which is controlled by both the proxy version and some feature flag to prevent the scenario that Pilot sends the configuration with metadata exchange filter to 1.4 proxy.

Examples:

istio/pilot/pkg/networking/util/util.go

Line 272 in f6d396f

func IsProtocolSniffingEnabledForOutbound(node *model.Proxy) bool {

bianpengyuan · 2020-01-17T06:24:29Z

tests/integration/telemetry/stats/prometheus/TCP/stats_tcp_filter_test.go

istio_requests_total is a http metric. I think a TCP metric is supposed to be tested here?

istio_requests_total is for both http and tcp now. It has protocol field that differentiates between the two.

Why aren't we following what we have right now? https://istio.io/docs/reference/config/policy-and-telemetry/metrics/ Our dashboard built on the assumption about the categorization of the metrics. And I think there would be user's customized dashboard based on this assumption too. I looked into proxy's code and looks like we are actually not reporting those four tcp metrics?

We added TCP as protocol in existing metrics.. I can make a change to proxy first and then here to match what we had for 1.4..

Thanks.. Added a TODO for the same.

bianpengyuan · 2020-01-17T22:04:05Z

tests/integration/telemetry/stats/prometheus/TCP/stats_tcp_filter_test.go

Can you lower case the package name as well as 'TCP' in path?

Thanks..done

bianpengyuan · 2020-01-17T22:06:40Z

tests/integration/telemetry/stats/prometheus/TCP/stats_tcp_filter_test.go

Why not verify source side telemetry?

Because we don't get client complete destination info in client side telemetry yet because of envoyproxy/envoy-wasm#291. Will add once this bug is fixed

bianpengyuan · 2020-01-17T22:07:37Z

tests/integration/telemetry/stats/prometheus/TCP/stats_tcp_filter_test.go

Please add a todo here as well to install these filters via istioctl.

bianpengyuan · 2020-01-17T22:32:49Z

tests/integration/telemetry/stats/prometheus/tcp/stats_tcp_filter_test.go

Unrelated change? If you want to disable mixer, you should set up operator values:

istio/tests/integration/telemetry/stats/prometheus/stats_filter_test.go

Line 146 in f2ee9ef

cfg.Values["telemetry.enabled"] = "true"

bianpengyuan · 2020-01-17T22:33:58Z

tests/integration/telemetry/stats/prometheus/tcp/stats_tcp_filter_test.go

Remove this? I don't think you need to instantiate a mixer instance.

Thanks..Fixed

gargnupur · 2020-01-18T00:32:15Z

/test gencheck_istio
/test unit-tests_istio
/test integ-istioio-k8s-tests_istio

incfly · 2020-01-18T15:07:51Z

pilot/pkg/security/authn/v1alpha1/policy_applier.go

there seems quite some duplicate in this if/else, at least for comments.

pilot/pkg/security/authn/v1alpha1/policy_applier.go

incfly · 2020-01-18T15:14:18Z

tests/integration/telemetry/stats/prometheus/tcp/stats_tcp_filter_test.go

this flag is default off and the effect only are showed in this particular test, right? might worth to try a default on (WIP pr, no need for merge) to see if other features like permissive or sniffing still works from CI tests running results

Have a PR for turning this default on.. #20258 (will work to fix the conflicts from it)

Regarding permissive or sniffing related tests, they should get ALPN on by default because that is dependent on value of PILOT_ENABLE_TCP_METADATA_EXCHANGE which is default on. This test, tests the mixer v2 part of it.

diemtvu · 2020-01-19T05:17:31Z

pilot/pkg/networking/core/v1alpha3/listener.go

Could this be a problem if the clients of this server has not upgraded?

It should not be because, then ALPN won't match and metadata exchange won't get enabled.

diemtvu · 2020-01-19T05:21:52Z

pilot/pkg/features/pilot.go

So it's true by default in 1.5? Could it cause troubles for upgrade path? (some servers have switched to the new ALPN but others might haven't)

It should not be because, then ALPN won't match and metadata exchange won't get enabled.

I mean if the server side was enabled, but some clients haven't, then does traffic will get into the (catch-all) filter chain? It my understanding, just want to confirm.

So this is for TCP only.. so it should get into TCP specific filter chain only...
@yxue : that sound right, right?

pilot/pkg/security/authn/v1alpha1/policy_applier.go

… when using mTLS Update based on config Fix tests Trying to fix tests Just do for TCP Enable Metadata Exchange for TCP when mTLS is on cleanup cleanup Fixing lint errors and running make gen Fix make gen Running make gen after rebase Fix license file Add option to enable metadata exchange Fix licensing file Add e2e test Fix e2e test Fix e2e test Fix tests Fix cleanup Fix based on feedback Add meta exc config to filter_types.go Run make gen Using INSERT_BEFORE to adding metadata exchange filter in the top of the list Fix spacing nit Fixed based on feedback Fixed based on feedback Fixed based on feedback Fixed dup in comments

fejta · 2020-01-21T19:22:59Z

Can you rebase this PR please? (and then /hold cancel)
/hold

fejta · 2020-01-21T19:23:40Z

/hold cancel

NM, already happened

howardjohn · 2020-01-22T00:22:05Z

go.mod


 replace github.com/Azure/go-autorest => github.com/Azure/go-autorest v13.2.0+incompatible
+
+replace github.com/istio.io/proxy/src/envoy/tcp/metadata_exchange/config v0.0.0 => ./pilot/pkg/metadata_exchange


Why we are doing this? Either import from pilot directly or import from istio.io/proxy?

If this is actually intended, and not just a WIP like previously indicated, can we at least comment why since its not intuitive at all

#20395

@howardjohn

Turn on v2 telemetry on by default for http and tcp THIS IS PENDING TOC APPROVAL For TCP this will use Alpn based prefixed tunelling metadata exchange -> pending istio#20126 Followed @howardjohn's PR istio#20225 for this. Fixed based on feedback Fix bad rebase Trying to fix bas rebase again Trying to fix bas rebase again Trying to fix bas rebase again

@howardjohn

Turn on v2 telemetry on by default for http and tcp THIS IS PENDING TOC APPROVAL For TCP this will use Alpn based prefixed tunelling metadata exchange -> pending istio#20126 Followed @howardjohn's PR istio#20225 for this. Fixed based on feedback Fix bad rebase Trying to fix bas rebase again Trying to fix bas rebase again Trying to fix bas rebase again fix assets file Running Make Gen Running Make Gen after changes to values.yaml Set telemetry component to false Add EnvoyFilter for TCP Run Make gen again Enable mixer in mixer tests Fix Unit test

@howardjohn

* add option Turn on v2 telemetry on by default for http and tcp THIS IS PENDING TOC APPROVAL For TCP this will use Alpn based prefixed tunelling metadata exchange -> pending #20126 Followed @howardjohn's PR #20225 for this. Fixed based on feedback Fix bad rebase Trying to fix bas rebase again Trying to fix bas rebase again Trying to fix bas rebase again fix assets file Running Make Gen Running Make Gen after changes to values.yaml Set telemetry component to false Add EnvoyFilter for TCP Run Make gen again Enable mixer in mixer tests Fix Unit test * Run make gen

@howardjohn

* add option Turn on v2 telemetry on by default for http and tcp THIS IS PENDING TOC APPROVAL For TCP this will use Alpn based prefixed tunelling metadata exchange -> pending istio#20126 Followed @howardjohn's PR istio#20225 for this. Fixed based on feedback Fix bad rebase Trying to fix bas rebase again Trying to fix bas rebase again Trying to fix bas rebase again fix assets file Running Make Gen Running Make Gen after changes to values.yaml Set telemetry component to false Add EnvoyFilter for TCP Run Make gen again Enable mixer in mixer tests Fix Unit test * Run make gen (cherry picked from commit a24f595)

@howardjohn

* add option Turn on v2 telemetry on by default for http and tcp THIS IS PENDING TOC APPROVAL For TCP this will use Alpn based prefixed tunelling metadata exchange -> pending #20126 Followed @howardjohn's PR #20225 for this. Fixed based on feedback Fix bad rebase Trying to fix bas rebase again Trying to fix bas rebase again Trying to fix bas rebase again fix assets file Running Make Gen Running Make Gen after changes to values.yaml Set telemetry component to false Add EnvoyFilter for TCP Run Make gen again Enable mixer in mixer tests Fix Unit test * Run make gen (cherry picked from commit a24f595) Co-authored-by: Nupur Garg <37600866+gargnupur@users.noreply.github.com>

mandarjog requested review from a team, costinm, howardjohn, linsun and rshriram as code owners January 13, 2020 17:57

istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Jan 13, 2020

googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Jan 13, 2020

istio-testing added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 13, 2020

mandarjog requested review from diemtvu and yxue January 13, 2020 17:59

howardjohn reviewed Jan 13, 2020

View reviewed changes

yxue reviewed Jan 13, 2020

View reviewed changes

pilot/pkg/networking/core/v1alpha3/listener.go Outdated Show resolved Hide resolved

yxue self-assigned this Jan 13, 2020

mandarjog requested review from a team January 14, 2020 01:45

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jan 14, 2020

gargnupur force-pushed the nup_tcp_meta_alpn branch from 9baecbe to 9c90327 Compare January 14, 2020 19:57

istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR needs to be rebased before being merged size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 14, 2020

bianpengyuan reviewed Jan 17, 2020

View reviewed changes

bianpengyuan approved these changes Jan 17, 2020

View reviewed changes

incfly reviewed Jan 18, 2020

View reviewed changes

pilot/pkg/security/authn/v1alpha1/policy_applier.go Outdated Show resolved Hide resolved

incfly reviewed Jan 18, 2020

View reviewed changes

diemtvu reviewed Jan 19, 2020

View reviewed changes

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jan 21, 2020

gargnupur force-pushed the nup_tcp_meta_alpn branch from d46b6d6 to a19b474 Compare January 21, 2020 18:45

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Jan 21, 2020

diemtvu approved these changes Jan 22, 2020

View reviewed changes

gargnupur requested a review from richardwxn January 22, 2020 00:08

richardwxn approved these changes Jan 22, 2020

View reviewed changes

howardjohn previously requested changes Jan 22, 2020

View reviewed changes

istio-testing merged commit 92ed864 into istio:master Jan 22, 2020

howardjohn mentioned this pull request Jan 27, 2020

[1.5] Cherrypick telemetry v2 #20258 #20572

Merged


		replace github.com/Azure/go-autorest => github.com/Azure/go-autorest v13.2.0+incompatible

		replace github.com/istio.io/proxy/src/envoy/tcp/metadata_exchange/config v0.0.0 => ./pilot/pkg/metadata_exchange

Conversation

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

Uh oh!

Uh oh!

howardjohn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mandarjog commented Jan 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rshriram commented Jan 13, 2020

Uh oh!

yxue commented Jan 13, 2020

Uh oh!

gargnupur commented Jan 14, 2020

Uh oh!

mandarjog commented Jan 14, 2020

Uh oh!

mandarjog commented Jan 14, 2020

Uh oh!

yxue commented Jan 14, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gargnupur commented Jan 18, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mandarjog commented Jan 13, 2020 •

edited

Loading

gargnupur Jan 17, 2020 •

edited

Loading