add connection.upstream_secure to indicate upstream cluster secure transport by kyessenov · Pull Request #2133 · istio/proxy

kyessenov · 2019-02-26T21:44:59Z

connection.mtls reports downstream mTLS statuc. For client-side telemetry, it's the connection from the app to the proxy. This PR adds connection.upstream_secure attribute to indicate whether the upstream connection is secure (e.g. proxy->proxy for client-side).

/assign @JimmyCYJ
/assign @douglas-reid
/assign @mandarjog

Signed-off-by: Kuat Yessenov <kuat@google.com>

istio-testing · 2019-02-26T21:45:01Z

@kyessenov: GitHub didn't allow me to assign the following users: douglas-reid.

Note that only istio members and repo collaborators can be assigned.
For more information please see the contributor guide

Details

In response to this:

connection.mtls reports downstream mTLS statuc. For client-side telemetry, it's the connection from the app to the proxy. This PR adds connection.upstream_secure attribute to indicate whether the upstream connection is secure (e.g. proxy->proxy for client-side).

/assign @JimmyCYJ
/assign @douglas-reid
/assign @mandarjog

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

JimmyCYJ

/lgtm

istio-testing · 2019-02-26T22:25:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JimmyCYJ, kyessenov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [JimmyCYJ]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

kyessenov · 2019-02-26T23:50:45Z

Reviewers: please indicate whether you are OK with the choice of attribute name connection.upstream_secure.

@duderino : this is meant to improve client-side telemetry dashboards. Right now we don't have a signal whether upstream is secure or not.

kyessenov · 2019-03-01T19:07:47Z

Ping @douglas-reid @mandarjog
Are you OK with the naming of the attribute? Josh merged this one into 1.1, so I can make a corresponding update in istio to pick it up.

douglas-reid · 2019-03-01T19:22:02Z

I'm sorry I missed this. I'm not sure what the distinction here is, and what the intent fully is.

connection.mtls doesn't reflect mutual TLS ? what is the meaning of connection.mtls: true, connection.upstream_secure: false (and vice versa)?

I feel like we need a different attribute, maybe connection.security_policy or something that has multiple values of tls, mutual_tls, none. Is that not possible?

kyessenov · 2019-03-01T19:27:33Z

Here is a diagram to help:

client app ---->  client envoy ---------------mTLS----------------->  server envoy ----> server app
                  mtls: false                                         mtls: true
                  upstream_secure:true                                upstream_secure: false

kyessenov · 2019-03-01T19:28:22Z

@douglas-reid The problem is that envoy has two connections with two security bits. One attribute is not sufficient to describe the security policy.

douglas-reid · 2019-03-01T19:29:29Z

Can we type up a small doc on this and discuss in the WG? I feel like this will add some confusion (it did in my case).

We may need to deprecate / rename some of the attributes for clarity.

kyessenov · 2019-03-01T19:32:14Z

@duderino Can you please revert this? I didn't mean to merge it until we discuss this in WG.

This reverts commit d857bdd.

@PiotrSikora

* Forwarded attributes override statically configured Local Attributes (#2097) * WIP * add local and override tests * revert attributes_builder * white list forward attributes * add tests with whitelist * fix builder test for white listed attributes * ignore istio.mixer in report (#2098) Signed-off-by: Lizan Zhou <lizan@tetrate.io> * whitelist kSourceNamespace attribute (#2100) * Update software in the build image used by CircleCI. (#2110) Signed-off-by: Piotr Sikora <piotrsikora@google.com> * Add flag indicating current semantics of report batch (#2111) * Add flag indicating current semantics of report batch * Fix Unit Test * Update Envoy SHA to latest with deterministic hash (master). (#2108) * Update Envoy SHA to latest with deterministic hash (master). Signed-off-by: Piotr Sikora <piotrsikora@google.com> * review: use lld linker for clang-asan and clang-tsan. Signed-off-by: Piotr Sikora <piotrsikora@google.com> * review: export PATH. Signed-off-by: Piotr Sikora <piotrsikora@google.com> * Update Envoy SHA to latest with deterministic hash (release-1.1). (#2109) * Update Envoy SHA to latest with deterministic hash (release-1.1). Signed-off-by: Piotr Sikora <piotrsikora@google.com> * review: use lld linker for clang-asan and clang-tsan. Signed-off-by: Piotr Sikora <piotrsikora@google.com> * review: export PATH. Signed-off-by: Piotr Sikora <piotrsikora@google.com> * remove unused bytestring include from sni_verifier for openssl (#2112) * Added client/server load test framework to find mixer faults. (#2105) This is a load generator client + origin server I created to test the Mixer filter under various fault conditions using Envoy's client and server stacks. This work falls under [istio/istio#8224](istio/istio#8224) @PiotrSikora @jplevyak would love your feedback because it could be used for the wasm work and especially because this is the first >=C++11 code I've written See test/integration/int_client_server_test.cc if you want to start with an example for context. Another example that uses this framework to sandwich Envoy+Mixer filter between the load generator and multiple origin servers simulating Mixer servers can be found in [istio/istio#8224](istio/istio#8224) * Warn user of using mTLS PERMISSIVE mode and suggest to upgrade to STRICT mode (#2114) * Warn user of using mTLS PERMISSIVE mode and suggest to upgrade to STRICT mode. Signed-off-by: Yangmin Zhu <ymzhu@google.com> * fix format * check in constructor * Update to latest istio/api on release-1.1 branch (#2115) * Update to latest istio/api on release-1.1 branch * Update istio/api to latest release-1.1 * Added simple logging abstraction so mixer client logs can be relayed to envoy logs. (#2116) * Added simple logging abstraction so mixer client logs can be relayed to envoy logs when running inside envoy, stderr when running standalone. * Log threshold guards that prevent needless serialization of logging arguments are now embedded in the log macros. * Format * Added do/while guards around logging statements. * Coalesce all memory for checks and reports into shared pointers (#2117) * Coalesce all memory for policy check requests and telemetry reports into shared pointers that live as long as a request's mixer filter instance. * A few small fixups for the code review. * Address some minor nits from code review. * Additional counters for mixer policy check (#2118) * Coalesce all memory for policy check requests and telemetry reports into shared pointers that live as long as a request's mixer filter instance. * A few small fixups for the code review. * Added finer-grained counters to mixer policy check * Add retries to policy checks on failed transport error (#2113) * Add configurable retry to policy/quota checks that failed due to transport error. * Added assertions on mixer filter stats to mixer fault test. * Reformat * Fix inaccurate comment. ` * Fix asan warning (thanks @silentdai!) and exclude mixer_fault_test from the asan and tsan sanitizers since it always times out. * Fix bad prefix check * Pull in latest istio/api from release-1.1 branch (#2120) * Add Joshua into proxy OWNER (#2121) * log authn permissive mode only when config is received (#2125) * log authn permissive mode only when config is received Signed-off-by: Yangmin Zhu <ymzhu@google.com> * fix format * fix build * clang-6/gcc: compiler barking fix (#2123) * compiler barking Signed-off-by: Kuat Yessenov <kuat@google.com> * piotrs fix Signed-off-by: Kuat Yessenov <kuat@google.com> * Add additional telemetry report counters (#2128) * Added counters to track telemetry report result. * reformat * replace tabs with spaces * Replace more tab with spaces. * New api sha for proxy (#2130) * API sha just changed, chanign it again for proxy (#2131) * Remove myself from owners add utka instead (#2129) * implement upstream secure bit (#2133) Signed-off-by: Kuat Yessenov <kuat@google.com> * Deflake macos MixerFaultTest by broadening assertion ranges. (#2126) * Deflake macos MixerFaultTest by broadening assertion ranges. Fix flake in macos tests that was introduced by #2113 * Cleanup a few readability issues and add an assertion. * More redability changes. * API sha for proxy (#2136) * Revert "implement upstream secure bit (#2133)" (#2135) This reverts commit d857bdd. * Add the support of bypassing JWT authn for CORS requests (#2139) * Add the support of bypassing JWT authn for CORS requests * Bail out earlier for CORS preflight requests * Use OPTIONS constant value from Envoy * Remove changing to lowercase * Add more checks for CORS preflight requests (#2140) * Rc3. new API sha for proxy. (#2146) * API sha for proxy * API sha for proxy * update envoy with latest build fixes (#2147) * update envoy with latest build fixes Signed-off-by: Lizan Zhou <lizan@tetrate.io> * update protobuf to match envoy Signed-off-by: Lizan Zhou <lizan@tetrate.io> * timeSystem -> timeSource Signed-off-by: Lizan Zhou <lizan@tetrate.io> * requesting to add myself as a reviewer/approver (#2148) I have 39 commits in this repo. * update envoy to pick up TLS logging for HTTP upstream (#2149) Signed-off-by: Lizan Zhou <lizan@tetrate.io> * Building 1.1rc4 (#2150) * fix build Signed-off-by: Lizan Zhou <lizan@tetrate.io> * fix format Signed-off-by: Lizan Zhou <lizan@tetrate.io> * fix status match Signed-off-by: Lizan Zhou <lizan@tetrate.io> * Fixes environment-dependent failures in MixerFaultTest (#2156) * Removed explicit log-level setting from tests, as it was interfering with cli '-l' option (#2155) * Update_Dependencies (#2178) * Update envoy sha and fix bulid break (#2179) * update envoy sha * fix build * Remove bazel shutdown from make deb * Ignore error code returned from bazel shutdown

yonatan-shorani · 2019-11-10T15:38:02Z

Any update on this issue?

implement upstream secure bit

9034ab0

Signed-off-by: Kuat Yessenov <kuat@google.com>

istio-testing assigned JimmyCYJ and mandarjog Feb 26, 2019

istio-testing requested review from JimmyCYJ and sebastienvas February 26, 2019 21:45

googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Feb 26, 2019

JimmyCYJ approved these changes Feb 26, 2019

View reviewed changes

istio-testing added the lgtm label Feb 26, 2019

istio-testing added the approved label Feb 26, 2019

duderino merged commit d857bdd into istio:release-1.1 Mar 1, 2019

duderino pushed a commit that referenced this pull request Mar 1, 2019

Revert "implement upstream secure bit (#2133)"

25c1d37

This reverts commit d857bdd.

duderino mentioned this pull request Mar 1, 2019

Revert "add connection.upstream_secure to indicate upstream cluster secure transport" #2135

Merged

duderino pushed a commit that referenced this pull request Mar 2, 2019

Revert "implement upstream secure bit (#2133)" (#2135)

f95f853

This reverts commit d857bdd.

kyessenov mentioned this pull request Mar 4, 2019

telemetry: add upstream secure bit #2138

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add connection.upstream_secure to indicate upstream cluster secure transport#2133

add connection.upstream_secure to indicate upstream cluster secure transport#2133
duderino merged 1 commit intoistio:release-1.1from
kyessenov:add_upstream_tls

kyessenov commented Feb 26, 2019

Uh oh!

istio-testing commented Feb 26, 2019

Uh oh!

JimmyCYJ left a comment

Uh oh!

istio-testing commented Feb 26, 2019

Uh oh!

kyessenov commented Feb 26, 2019

Uh oh!

kyessenov commented Mar 1, 2019

Uh oh!

douglas-reid commented Mar 1, 2019

Uh oh!

kyessenov commented Mar 1, 2019

Uh oh!

kyessenov commented Mar 1, 2019

Uh oh!

douglas-reid commented Mar 1, 2019

Uh oh!

kyessenov commented Mar 1, 2019

Uh oh!

yonatan-shorani commented Nov 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Conversation

kyessenov commented Feb 26, 2019

Uh oh!

istio-testing commented Feb 26, 2019

Uh oh!

JimmyCYJ left a comment

Choose a reason for hiding this comment

Uh oh!

istio-testing commented Feb 26, 2019

Uh oh!

kyessenov commented Feb 26, 2019

Uh oh!

kyessenov commented Mar 1, 2019

Uh oh!

douglas-reid commented Mar 1, 2019

Uh oh!

kyessenov commented Mar 1, 2019

Uh oh!

kyessenov commented Mar 1, 2019

Uh oh!

douglas-reid commented Mar 1, 2019

Uh oh!

kyessenov commented Mar 1, 2019

Uh oh!

yonatan-shorani commented Nov 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants