tls: Refactor the ssl socket test by vadimeisenbergibm · Pull Request #5402 · envoyproxy/envoy

vadimeisenbergibm · 2018-12-22T07:24:50Z

Description: Refactor the ssl socket test - use options objects instead of long parameter lists.
Risk Level: Zero risk
Testing: the test itself
Docs Changes: None
Release Notes:

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

to match the order in the original call to testUtilV2() Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

This reverts commit 53bbe1e. Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

This reverts commit 05f85c5. Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

This reverts commit c43b76f. Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

This reverts commit cc9a9c5. Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

This reverts commit 9ccec68. Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

….exit"" This reverts commit 2090ae3. Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

This reverts commit c43b76f. Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

This reverts commit f02bdb8. Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

This reverts commit bcbbe9d. Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

This reverts commit 334c2c0. Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

This reverts commit 53bbe1e. Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

This reverts commit 05f85c5. Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

…ilOptionsV2) Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

…lOptionsV2) remove the other version of testUtilV2, leave the version with options only Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

vadimeisenbergibm · 2018-12-23T20:03:53Z

@htuch Following your comment #4973 (comment)

htuch

Thanks @vadimeisenbergibm! This is a really nice cleanup!

htuch · 2018-12-24T12:31:32Z

test/common/ssl/ssl_socket_test.cc

+      : expected_server_stats_(expected_server_stats), expect_success_(expect_success),
+        version_(version) {}
+
+  void expectedClientCertURI(const std::string& expected_client_cert_uri) {


Nit: can you prefix all these mutators with set, e.g. setExpectedClientCertUri? I think this will improve readability a bit.

htuch · 2018-12-24T12:31:53Z

test/common/ssl/ssl_socket_test.cc

+      : TestUtilOptionsBase(expected_server_stats, expect_success, version),
+        client_ctx_yaml_(client_ctx_yaml), server_ctx_yaml_(server_ctx_yaml) {}
+
+  const std::string& clientCtxYAML() const { return client_ctx_yaml_; }


Nit: Envoy style is clientCtxtYaml.

htuch · 2018-12-24T12:32:29Z

test/common/ssl/ssl_socket_test.cc

+  }
+
+  const std::string& expectedClientCertURI() const {
+    return TestUtilOptionsBase::expectedClientCertURI();


You need to implement this method if it's just the same as in the parent class.

@htuch I guess you mean "You need not to implement". Currently, it is needed because the setter version of expectedClientCertURI() overrides the version of the parent class, so all the versions of expectedClientCertURI() are not inherited and have to be implemented, as far as I understand it.

Anyway, I will rename the setter to be setExpectedClientCertUri(), so the getter will be inherited from the parent class.

htuch · 2018-12-24T12:32:42Z

test/common/ssl/ssl_socket_test.cc

+  }
+
+  TestUtilOptions& expectedDigest(const std::string& expected_digest) {
+    expected_digest_ = expected_digest;


Nit: as above, setExpectedDigest() etc.

htuch · 2018-12-24T12:34:11Z

test/common/ssl/ssl_socket_test.cc

  // The SAN field only has DNS, expect "" for uriSanPeerCertificate().
-  testUtil(client_ctx_yaml, server_ctx_yaml, "", "", "", TEST_SAN_DNS_CERT_SERIAL, "", "", "",
-           "ssl.handshake", true, GetParam());
+  TestUtilOptions test_options(client_ctx_yaml, server_ctx_yaml, "ssl.handshake", true, GetParam());


There's still 5 params here, which is getting a little long, could some of these be switched to the builder pattern, or are they all compulsory?

@htuch I can switch all the parameters to be set by setters, however it will produce longer code at the call sites. How about leaving four parameters in the constructor, for TestUtilOptions and TestUtilOptionsV2? It will be client ctx, server ctx, expected_success and version. "ssl.handshake" can be set by default for client and server stats.

SGTM, thanks.

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

it is inherited from the base class Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

…ificate Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

vadimeisenbergibm · 2018-12-26T18:37:50Z

@htuch I have applied your comments. asan seems to fail on an unrelated test.

lizan

Nice cleanup.

/retest

repokitteh-read-only · 2018-12-27T00:29:01Z

🔨 rebuilding ci/circleci: asan (failed build)

🐱

Caused by: a #5402 (review) was submitted by @lizan.

see: more, trace.

htuch

Thanks, this is a really nice cleanup!

PiotrSikora · 2018-12-27T08:02:17Z

test/common/ssl/ssl_socket_test.cc

+    if (expect_success) {
+      setExpectedServerStats("ssl.handshake");
+    } else {
+      setExpectedServerStats("ssl.fail_verify_error");


I know this is merged already, but... why is this the default in case of failure instead of ssl.connection_error?

Because for testUtil this is the server stats in all the cases when expect_success is false, and for testUtilV2 it has the same number of cases as ssl.connection_error. If you think the default value is important, I can change it in a new PR.

PiotrSikora · 2018-12-27T08:02:33Z

test/common/ssl/ssl_socket_test.cc

+    if (expect_success) {
+      setExpectedServerStats("ssl.handshake").setExpectedClientStats("ssl.handshake");
+    } else {
+      setExpectedServerStats("ssl.fail_verify_error")


PiotrSikora · 2018-12-27T08:03:34Z

test/common/ssl/ssl_socket_test.cc


  // Connection using defaults (client & server) succeeds.
-  testUtilV2(listener, client, "", true, "", "", "spiffe://lyft.com/test-team", "", "",
-             "ssl.sigalgs.rsa_pss_rsae_sha256", "ssl.sigalgs.ecdsa_secp256r1_sha256", GetParam(),


I know this is merged already, but... you removed the only thing that's tested here (ssl.sigalgs.* stats).

@PiotrSikora Oops, let me put it back in a separate PR, sorry.

@PiotrSikora Submitted an additional PR to fix it #5429. Good catch!
I will go over all the tests and recheck the stats. More comments are welcome.

Refactor the ssl socket test - use options objects instead of long parameter lists. Risk Level: Zero risk Testing: the test itself Docs Changes: None Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com> Signed-off-by: Fred Douglas <fredlas@google.com>

vadimeisenbergibm added 2 commits December 22, 2018 09:17

refactor testUtil to use TestUtilOptions as the parameter

f9b88fd

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

fix a comment

9e5f58d

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

vadimeisenbergibm changed the title ~~ssl: Refactor the ssl socket test~~ tls: Refactor the ssl socket test Dec 22, 2018

vadimeisenbergibm added 4 commits December 22, 2018 09:56

fix a parameter: local uri instead of just uri

5096509

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

expectedURI -> expectedClientCertURI

66864b5

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

expectedStats() -> expectedServerStats()

4138679

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

refactor: extract TestUtilOptionsBase

9efab49

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

vadimeisenbergibm changed the title ~~tls: Refactor the ssl socket test~~ [WIP] tls: Refactor the ssl socket test Dec 22, 2018

vadimeisenbergibm added 22 commits December 22, 2018 12:16

add TestUtilOptionsV2 and use them in testUtilV2()

6868676

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

switch client and server stats in the constructor of TestUtilOptionsV2

fde7ccc

to match the order in the original call to testUtilV2() Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

use TestUtilOptionsV2 to call testUtilV2() in two calls to testUtilV2()

1507362

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

call setter methods before call to testUtilV2()

05f85c5

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

remove redundant parenthesis

53bbe1e

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

Revert "remove redundant parenthesis"

334c2c0

This reverts commit 53bbe1e. Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

Revert "call setter methods before call to testUtilV2()"

bcbbe9d

This reverts commit 05f85c5. Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

check statistics before calling dispatcher.exit()

f02bdb8

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

move checking statistics immediately after dispatcher.exit

c43b76f

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

Revert "move checking statistics immediately after dispatcher.exit"

2090ae3

This reverts commit c43b76f. Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

capture client and server stats by copy

9ccec68

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

use copy instead of reference for client/server stats

cc9a9c5

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

Revert "use copy instead of reference for client/server stats"

55a3218

This reverts commit cc9a9c5. Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

Revert "capture client and server stats by copy"

0561555

This reverts commit 9ccec68. Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

Revert "Revert "move checking statistics immediately after dispatcher…

dd2739c

….exit"" This reverts commit 2090ae3. Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

Revert "move checking statistics immediately after dispatcher.exit"

b28ed80

This reverts commit c43b76f. Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

Revert "check statistics before calling dispatcher.exit()"

e81833b

This reverts commit f02bdb8. Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

Revert "Revert "call setter methods before call to testUtilV2()""

01e924e

This reverts commit bcbbe9d. Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

Revert "Revert "remove redundant parenthesis""

fdf8e08

This reverts commit 334c2c0. Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

Revert "remove redundant parenthesis"

f4988ad

This reverts commit 53bbe1e. Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

Revert "call setter methods before call to testUtilV2()"

03e73f8

This reverts commit 05f85c5. Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

hold expected_client_stats by value and not by reference

3eb3d0f

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

vadimeisenbergibm added 2 commits December 23, 2018 18:59

replace more calls to testUtilV2(...) with calls to testUtilV2(TestUt…

e646c34

…ilOptionsV2) Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

replace all calls to testUtilV2(...) with calls to testUtilV2(TestUti…

77df00b

…lOptionsV2) remove the other version of testUtilV2, leave the version with options only Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

vadimeisenbergibm changed the title ~~[WIP] tls: Refactor the ssl socket test~~ tls: Refactor the ssl socket test Dec 23, 2018

htuch suggested changes Dec 24, 2018

View reviewed changes

htuch self-assigned this Dec 24, 2018

vadimeisenbergibm added 7 commits December 26, 2018 07:53

add 'set' prefix to setter methods

b385654

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

add setExpectedClientStats/ServerStats()

b112dfe

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

remove expectedClientCertURI() from TestUtilOptionsV2

79a601d

it is inherited from the base class Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

clientCtxYAML -> clientCtxYaml

904b224

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

URI -> Uri in function names

4c1b988

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

do not pass server and client stats as parameters in constructors

7360856

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

true -> false for FailedClientCertificateHashVerificationNoClientCert…

051e1ac

…ificate Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

lizan approved these changes Dec 27, 2018

View reviewed changes

htuch approved these changes Dec 27, 2018

View reviewed changes

htuch merged commit 2f77974 into envoyproxy:master Dec 27, 2018

PiotrSikora reviewed Dec 27, 2018

View reviewed changes

vadimeisenbergibm mentioned this pull request Dec 27, 2018

complete ssl_socket_test refactoring #5429

Merged

Conversation

vadimeisenbergibm commented Dec 22, 2018

Uh oh!

vadimeisenbergibm commented Dec 23, 2018

Uh oh!

htuch 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!

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!

vadimeisenbergibm commented Dec 26, 2018

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

repokitteh-read-only bot commented Dec 27, 2018

Uh oh!

htuch 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants