Assert that TLS connection is refused if identity is not certified yet by zaharidichev · Pull Request #243 · linkerd/linkerd2-proxy

zaharidichev · 2019-05-03T14:13:12Z

This branch adds tls capability to the support cient used in tests. In addition to that it adds two tests verifying that a TLS connection is refused in case the identity is not certified yet. This attempts to fix #linkerd/linkerd2#2598 and provide facility to write tests for linkerd/linkerd2#2676.

As these are still some of my first lines of Rust code, it is advised to approach everything with a healthy dose of doubt :)

Signed-off-by: Zahari Dichev [email protected]

kleimkuhler

This is a really good start. I left some preliminary comments, but I would like to see new functions introduced to support/client.rs that support a tls_config parameter instead of adding the Option to the current functions.

After addressing that, the diff will be a lot smaller since we won't need to change all the signatures in the rest of the tests.

tests/identity.rs

tests/support/client.rs

hawkw

This looks like a good start. I left a number of comments, primarily on coding style.

I think my biggest concern is that I'd prefer to minimize the change to unrelated test code as much as possible --- i.e., having to pass None to all plaintext http client constructors causes a lot of unnecessary Git churn.

tests/support/mod.rs

tests/support/client.rs

hawkw · 2019-05-03T20:16:57Z

tests/support/client.rs

-    pub fn request(&self, builder: &mut http::request::Builder) -> Response {
-        self.request_async(builder).wait().expect("response")
+    pub fn request(&self, builder: &mut http::request::Builder) -> Result<Response, ClientError> {
+        self.request_async(builder).wait()


I'm not sure if I understand why the expect was removed here. Removing it requires changing a bunch of other code (e.g., a lot of the transparency tests) that otherwise would not need to be removed.

If the intention is to assert that some requests will fail (such as when the proxy cannot yet accept TLS connections), can't we just use

client.request_async(...).wait()

in those specific cases?

tests/support/client.rs

hawkw · 2019-05-03T20:24:00Z

tests/support/client.rs

-            });
-        Box::new(c)
+impl CustomConnector {
+    fn initialize_connection(


Not sure why this function was renamed?

hawkw · 2019-05-03T20:24:43Z

tests/support/client.rs

-    /// When this drops, the related Receiver is notified that the connection
-    /// is closed.
-    running: oneshot::Sender<()>,
+pub enum ConnectorFuture<F: Future> {


I don't think this needs to be pub --- AFAICT it's not exposed outside this module?

hawkw · 2019-05-03T20:25:20Z

tests/support/client.rs

+pub enum ConnectorFuture<F: Future> {
+    Init {
+        future: F,
+        tls: Option<(Arc<ClientConfig>, DNSName)>,


Again, would prefer to see the type alias used consistently if we're going to continue using one:

Suggested change

tls: Option<(Arc<ClientConfig>, DNSName)>,

tls: Option<TlsConfig>,

hawkw · 2019-05-03T20:26:40Z

tests/support/client.rs

+    type Error = F::Error;
+
+    fn poll(&mut self) -> Poll<Self::Item, Self::Error> {
+        fn take_running(r: &Mutex<Option<oneshot::Sender<()>>>) -> oneshot::Sender<()> {


Take it or leave it: if this was a closure rather than a function, it could just capture running...

hawkw · 2019-05-03T20:27:54Z

tests/support/client.rs

 }

-impl io::Read for RunningIo {
+pub enum RunningIo<T> {


Again, not sure if this needs to be pub.

Also, I think the underlying IO will always be a TcpStream, so I don't think it really needs to be generic over a T...

hawkw · 2019-05-03T20:32:39Z

It looks like the CI build is failing because the client constructor is being called without the new TLS config parameter in tests/discovery.rs.

IMO that's a further argument for trying to make this change without modifying the existing API that the rest of the tests rely on, if possible.

Signed-off-by: Zahari Dichev <[email protected]>

kleimkuhler

@zaharidichev I have a few comments that are questioning the addition of new functionality that does not seem to be needed right now, but the rest are mostly small nits. I think we're in the right place with the structure of this change, so I think we'll be good to go after you address those comments.

I've noticed you're force-pushing the same commit and want to let you know multiple commits are okay (and helpful)! As separate commits are added, we can review the changes incrementally instead of looking at everything again :)

tests/support/identity.rs

tests/support/client.rs

Signed-off-by: Zahari Dichev <[email protected]>

kleimkuhler

Great! Thanks for addressing all the comments so far. When @hawkw is able to loop back to this, we should be close to getting this merged.

hawkw

Looks like all my feedback has been addressed, great!

I commented on a couple things I wanted to point out, just in case you weren't already aware of them. I'm fine with merging this as-is regardless. Thanks again for your contribution!

tests/identity.rs

tests/support/identity.rs

tests/support/tap.rs

Signed-off-by: Zahari Dichev <[email protected]>

hawkw · 2019-05-08T22:08:54Z

aaand...merged! thanks again for the contribution @zaharidichev!

commit 5f89351 Author: Oliver Gould <[email protected]> Date: Thu May 9 16:39:24 2019 -0700 Upgrade tower dependencies (linkerd#249) Tower must be updated in order to pickup tower-rs/tower#281 to address linkerd/linkerd2#2804. This adopts released crates where possible. commit 5d5eed6 Author: Zahari Dichev <[email protected]> Date: Thu May 9 01:08:34 2019 +0300 Assert that TLS connection is refused if identity is not certified yet (linkerd#243) This branch adds tls capability to the support cient used in tests. In addition to that it adds two tests verifying that a TLS connection is refused in case the identity is not certified yet. This attempts to fix #linkerd/linkerd2#2598 and provide facility to write tests for linkerd/linkerd2#2676. As these are still some of my first lines of Rust code, it is advised to approach everything with a healthy dose of doubt :) Signed-off-by: Zahari Dichev <[email protected]> commit 1b9bb37 Author: Oliver Gould <[email protected]> Date: Wed May 8 14:28:37 2019 -0700 Introduce dispatch timeouts around buffers (linkerd#246) The proxy has several buffers, especially where it routes requests over shared stacks. If any of these routes is unavailable, then a request may remain buffered indefinitely. Previously, before service profiles were introduced, there was a default _response_ timeout that would cause these requests to fail; but since this response timeout is now optional (and is only applied once the request has been routed within a proxy), then we need a new mechanism to prevent requests from getting "stuck". This change does the following: - all proxied requests are annotated with a dispatch deadline; - each time a request is bufered, a timeout is registered. - if the timeout fires, the response exception fails, a 503 is returned, and the request is dropped. - if the request is processed into the inner stack, the timeout is ignored. The dispatch timeout limits the _time a request is buffered in a proxy_. This is distinct from the response timeout, as the server's response may naturally be delayed for any number of (non-proxy-related) reasons. The `insert_target` module has been generalized to `insert` to support setting the DispatchDeadline extension. The `buffer` module has been augmented with generic deadline-extraction logic. The `svc` module now exposes its own builder type that notably adds a `buffer_pending` helper. It's helpful to pull a builder type into the proxy to assist debugging type errors when modifying stacks. Fixes linkerd/linkerd2#2779 linkerd/linkerd2#2795 commit caf8995 Author: Oliver Gould <[email protected]> Date: Fri May 3 15:55:32 2019 -0700 router: Fail requests when the route is not ready (linkerd#241) In linkerd/linkerd2#2779, we plan to expire requests while they are buffered. However, the router _implicitly_ buffers requests in the executor when the inner service is not ready. This change alters the route to wrap all inner layers in a `LoadShed` so it can expect all services to `poll_ready()` immediately. commit 587bad1 Author: Eliza Weisman <[email protected]> Date: Fri May 3 14:18:08 2019 -0700 Remove Destination service query concurrency limit (linkerd#244) Currently, the proxy enforces a limit on the number of concurrent queries (i.e., the number of gRPC streams) to the Destination service. This limit was added based on information about the behaviour of the Destination service that is now known to be incorrect. This branch removes the limit on concurrent queries from the proxy's `control::destination` module. Although it should now be possible to simplify this code as a result of this change, I've refrained from doing any major refactoring in this branch --- my intention is to do this after the DNS fallback behaviour has also been removed, as together with this change, that will result in a _significant_ simplification of the module. Additionally, I've removed the tests for the concurrency limit, as they are no longer relevant. The `LINKERD2_PROXY_DESTINATION_CLIENT_CONCURRENCY_LIMIT` environment variable was also removed; this is not a breaking change as neither the CLI nor the proxy injector will currently set this env var. Signed-off-by: Eliza Weisman <[email protected]> commit cbdf45b Author: Sean McArthur <[email protected]> Date: Thu May 2 11:47:48 2019 -0700 Remove h2::Error requirement from metrics Signed-off-by: Sean McArthur <[email protected]> commit 3276949 Author: Sean McArthur <[email protected]> Date: Thu May 2 09:44:00 2019 -0700 delete unused proxy::http::metrics::class module Signed-off-by: Sean McArthur <[email protected]> Signed-off-by: Eliza Weisman <[email protected]>

kleimkuhler self-requested a review May 3, 2019 15:21

zaharidichev force-pushed the zd/support-client-tls branch 2 times, most recently from 2aab689 to d748765 Compare May 3, 2019 17:13

kleimkuhler reviewed May 3, 2019

View reviewed changes

tests/identity.rs Outdated Show resolved Hide resolved

tests/support/client.rs Outdated Show resolved Hide resolved

tests/support/client.rs Outdated Show resolved Hide resolved

hawkw reviewed May 3, 2019

View reviewed changes

zaharidichev force-pushed the zd/support-client-tls branch from d748765 to 6f5fb95 Compare May 4, 2019 14:46

Assert that TLS connection is refused if identity is not certified yet

ec19959

Signed-off-by: Zahari Dichev <[email protected]>

zaharidichev force-pushed the zd/support-client-tls branch from 6f5fb95 to ec19959 Compare May 4, 2019 15:34

kleimkuhler suggested changes May 7, 2019

View reviewed changes

Address more comments and add http2 tests

9287351

Signed-off-by: Zahari Dichev <[email protected]>

kleimkuhler approved these changes May 7, 2019

View reviewed changes

hawkw approved these changes May 7, 2019

View reviewed changes

tests/identity.rs Show resolved Hide resolved

tests/support/identity.rs Outdated Show resolved Hide resolved

tests/support/tap.rs Show resolved Hide resolved

Addressing final comments

e167b67

Signed-off-by: Zahari Dichev <[email protected]>

olix0r requested a review from seanmonstar May 8, 2019 21:25

seanmonstar approved these changes May 8, 2019

View reviewed changes

hawkw merged commit 5d5eed6 into linkerd:master May 8, 2019

zaharidichev mentioned this pull request May 13, 2019

proxy: Test that inbound listener refuses connections until its identity is certified linkerd/linkerd2#2598

Closed

	tls: Option<(Arc<ClientConfig>, DNSName)>,
	tls: Option<TlsConfig>,

Conversation

zaharidichev commented May 3, 2019

Uh oh!

kleimkuhler left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hawkw May 3, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hawkw May 3, 2019

Choose a reason for hiding this comment

Uh oh!

hawkw May 3, 2019

Choose a reason for hiding this comment

Uh oh!

hawkw May 3, 2019

Choose a reason for hiding this comment

Uh oh!

hawkw May 3, 2019

Choose a reason for hiding this comment

Uh oh!

hawkw May 3, 2019

Choose a reason for hiding this comment

Uh oh!

hawkw commented May 3, 2019

Uh oh!

kleimkuhler left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kleimkuhler left a comment

Choose a reason for hiding this comment

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hawkw commented May 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants