Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/16658 )
Change subject: KUDU-3210 Increase key size in tests and EMC ...................................................................... Patch Set 10: (3 comments) http://gerrit.cloudera.org:8080/#/c/16658/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16658/3//COMMIT_MSG@11 PS3, Line 11: This commit removes the test-only key size The master test, for example, has an assertion that would fail depending on what the key size is set to: https://gerrit.cloudera.org/c/16658/3/src/kudu/master/master-test.cc#1970 There are other examples in this patch that are more straight-forward, but developers adding new tests in the future would still need to make sure they use two different key sizes, both chosen correctly. > I'm cautious about bumping up key sizes because I don't know how much impact > that would be for our tests. If taking about speed and what part of overall > costs of benchmarks crypto-related stuff is taking, I'm curious how much is > that if running the whole suite of the tests on the same machine with short > and FIPS-required key length? Or at least, maybe the the following tests: Of course, there is a significant difference in the runtime of some of the tests you mention: Performance counter stats for './bin/token-test': Performance counter stats for './bin/token-test': 12.309491423 seconds time elapsed | 15.027583910 seconds time elapsed Performance counter stats for './bin/registration-test': Performance counter stats for './bin/registration-test': 3.844352371 seconds time elapsed | 10.652710483 seconds time elapsed Performance counter stats for './bin/tls_socket-test': Performance counter stats for './bin/tls_socket-test': 4.630019880 seconds time elapsed | 4.626133437 seconds time elapsed Performance counter stats for './bin/token_signer-itest': Performance counter stats for './bin/token_signer-itest': 7.316916991 seconds time elapsed | 10.807419288 seconds time elapsed Performance counter stats for './bin/master_cert_authority-i Performance counter stats for './bin/master_cert_authority-i 13.531314727 seconds time elapsed | 18.915764233 seconds time elapsed I'm just saying this makes up for only a small part of the overall runtime of the test suite. I mentioned above in this thread that on Jenkins, the runtime was 46 minutes which is well within the normal range. I compared perf stats on these test > I guess we are interested to see not only time taken, but also CPU consumed > (so, running some of the tests above under perf-stat might be a good idea as > well). I'd think that if the overall increase is less than 3%, we probably > should switch to bigger keys. I understand we care about even small performance impacts, and I agree that we should, in production. This is, however, a test-only change, that in addition to fulfilling its primary goal to make the tests pass in FIPS-approved mode, it also changes the tests' behavior to mirror real-life behavior more closely. http://gerrit.cloudera.org:8080/#/c/16658/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16658/4//COMMIT_MSG@10 PS4, Line 10: these tests > I see; I thought those tweaks were necessary to pass (I missed the importan Done http://gerrit.cloudera.org:8080/#/c/16658/3/src/kudu/mini-cluster/external_mini_cluster.cc File src/kudu/mini-cluster/external_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/16658/3/src/kudu/mini-cluster/external_mini_cluster.cc@a990 PS3, Line 990: > I'd suggest you run the tests on RHEL/CentOS 8.1 if you are about to remove FIPS 140-2 does too, and that is our default RSA key size too. This serves to underline my point though - if we use the default key sizes in security levels in our tests, it's easier to catch real problems. -- To view, visit http://gerrit.cloudera.org:8080/16658 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I45b96e0b4499ea1d80db5871a529c732ad41220e Gerrit-Change-Number: 16658 Gerrit-PatchSet: 10 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Fri, 30 Oct 2020 16:18:20 +0000 Gerrit-HasComments: Yes
