From dev-return-76228-archive-asf-public=cust-asf.ponee.io@zookeeper.apache.org Fri Nov 23 12:39:01 2018 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by mx-eu-01.ponee.io (Postfix) with SMTP id EEFB9180660 for ; Fri, 23 Nov 2018 12:39:00 +0100 (CET) Received: (qmail 25908 invoked by uid 500); 23 Nov 2018 11:39:00 -0000 Mailing-List: contact dev-help@zookeeper.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@zookeeper.apache.org Delivered-To: mailing list dev@zookeeper.apache.org Received: (qmail 25897 invoked by uid 99); 23 Nov 2018 11:38:59 -0000 Received: from git1-us-west.apache.org (HELO git1-us-west.apache.org) (140.211.11.23) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 23 Nov 2018 11:38:59 +0000 Received: by git1-us-west.apache.org (ASF Mail Server at git1-us-west.apache.org, from userid 33) id 1ACA2E11D7; Fri, 23 Nov 2018 11:38:59 +0000 (UTC) From: anmolnar To: dev@zookeeper.apache.org Reply-To: dev@zookeeper.apache.org References: In-Reply-To: Subject: [GitHub] zookeeper issue #679: ZOOKEEPER-3172: Quorum TLS - fix port unification to a... Content-Type: text/plain Message-Id: <20181123113859.1ACA2E11D7@git1-us-west.apache.org> Date: Fri, 23 Nov 2018 11:38:59 +0000 (UTC) Github user anmolnar commented on the issue: https://github.com/apache/zookeeper/pull/679 @ivmaykov I did another cycle of review the unit tests, sorry I still not see value in denial-of-service tests, but maybe I don't see something important. Let's put it this way: Use case of `UnifiedServerSocket` is the following: > Extend standard Socket class to catch all read events in order to trigger TLS/Plaintext mode detection in a lazy fashion (catching the last chance of doing so). In order to do that it captures calls to the underlying input/output streams and initiates detection of channel type on the first read/write operation. So far so good. The important bit here is that `UnifiedServerSocket` **doesn't contain anything related to threading**. As a consequence if we write purely unit tests for this class we won't have to verify anything which is related to threading. DOS test comment says: > This test makes sure that UnifiedServerSocket used properly (a single thread accept()-ing connections and handing the resulting sockets to other threads for processing) is not vulnerable to a simple denial-of-service attack in which a client connects and never writes any bytes. **This should not block the accepting thread, since the read to determine if the client is sending a TLS handshake or not happens in the processing thread.** And here's the thing: this test is testing the proper implementation of the server (e.g. dealing with threads in the right way), which is implemented in the test itself: `UnifiedServerThread`. My 2 cents here is that if you think you haven't covered a user scenario or an edge with the existing tests, you need to rewrite these tests to be more strict and test that-and-only-that particular case which is missing. I think the coverage is acceptable (what you mentioned in your latest comment has already been covered), but there's no harm in adding more. I just don't want tests which don't add value **or** adds value in an unreasonably cumbersome way. Please correct me if I'm wrong. ---