You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by GitBox <gi...@apache.org> on 2021/07/02 21:02:51 UTC
[GitHub] [qpid-dispatch] kgiusti opened a new pull request #1282: DISPATCH-2190: add SSL tests for qdstat and qdmanage
kgiusti opened a new pull request #1282:
URL: https://github.com/apache/qpid-dispatch/pull/1282
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] asfgit closed pull request #1282: DISPATCH-2190: add SSL tests for qdstat and qdmanage
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #1282:
URL: https://github.com/apache/qpid-dispatch/pull/1282
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] jiridanek commented on a change in pull request #1282: DISPATCH-2190: add SSL tests for qdstat and qdmanage
Posted by GitBox <gi...@apache.org>.
jiridanek commented on a change in pull request #1282:
URL: https://github.com/apache/qpid-dispatch/pull/1282#discussion_r665459772
##########
File path: tests/system_tests_qdmanage.py
##########
@@ -86,7 +96,7 @@ def run_qdmanage(self, cmd, input=None, expect=Process.EXIT_OK, address=None):
try:
p.teardown()
except Exception as e:
- raise Exception(out if out else str(e))
+ raise sys.exc_info()[0](out if out else str(e))
Review comment:
Actually, the QdStat test cases suffer somewhat for this change of approach. When previously expecting AssertionError and no message text, there were only a few possibilities where that could've come from. Getting RuntimeError by accident is significantly easier. Many things in Python is runtime error.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] codecov-commenter commented on pull request #1282: DISPATCH-2190: add SSL tests for qdstat and qdmanage
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #1282:
URL: https://github.com/apache/qpid-dispatch/pull/1282#issuecomment-873304701
# [Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1282?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#1282](https://codecov.io/gh/apache/qpid-dispatch/pull/1282?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2b00a21) into [main](https://codecov.io/gh/apache/qpid-dispatch/commit/d7688f473459909288bc833be25d801ed1849596?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d7688f4) will **decrease** coverage by `0.04%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/qpid-dispatch/pull/1282?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## main #1282 +/- ##
==========================================
- Coverage 84.64% 84.59% -0.05%
==========================================
Files 113 113
Lines 28224 28224
==========================================
- Hits 23891 23877 -14
- Misses 4333 4347 +14
```
| [Impacted Files](https://codecov.io/gh/apache/qpid-dispatch/pull/1282?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [src/router\_core/connections.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1282/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL3JvdXRlcl9jb3JlL2Nvbm5lY3Rpb25zLmM=) | `89.45% <0.00%> (-1.29%)` | :arrow_down: |
| [src/router\_core/router\_core.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1282/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL3JvdXRlcl9jb3JlL3JvdXRlcl9jb3JlLmM=) | `86.08% <0.00%> (-0.97%)` | :arrow_down: |
| [src/router\_core/delivery.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1282/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL3JvdXRlcl9jb3JlL2RlbGl2ZXJ5LmM=) | `93.33% <0.00%> (-0.56%)` | :arrow_down: |
| [src/router\_core/transfer.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1282/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL3JvdXRlcl9jb3JlL3RyYW5zZmVyLmM=) | `93.75% <0.00%> (-0.44%)` | :arrow_down: |
| [src/router\_core/modules/edge\_router/edge\_mgmt.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1282/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL3JvdXRlcl9jb3JlL21vZHVsZXMvZWRnZV9yb3V0ZXIvZWRnZV9tZ210LmM=) | `85.14% <0.00%> (+0.99%)` | :arrow_up: |
| [src/router\_core/agent\_conn\_link\_route.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1282/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL3JvdXRlcl9jb3JlL2FnZW50X2Nvbm5fbGlua19yb3V0ZS5j) | `85.38% <0.00%> (+1.16%)` | :arrow_up: |
| [...router\_core/modules/edge\_router/link\_route\_proxy.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1282/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL3JvdXRlcl9jb3JlL21vZHVsZXMvZWRnZV9yb3V0ZXIvbGlua19yb3V0ZV9wcm94eS5j) | `82.84% <0.00%> (+4.14%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1282?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1282?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [d7688f4...2b00a21](https://codecov.io/gh/apache/qpid-dispatch/pull/1282?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] jiridanek commented on a change in pull request #1282: DISPATCH-2190: add SSL tests for qdstat and qdmanage
Posted by GitBox <gi...@apache.org>.
jiridanek commented on a change in pull request #1282:
URL: https://github.com/apache/qpid-dispatch/pull/1282#discussion_r663343758
##########
File path: tests/system_tests_qdmanage.py
##########
@@ -612,6 +622,64 @@ def test_check_memory_usage(self):
# is added
self.assertTrue(mem is None)
+ def test_ssl_connection(self):
+ """
+ Verify qdmanage can securely connect via SSL:
+ """
+ ssl_address = "amqps://localhost:%s" % self.secure_port
+ ssl_user_address = "amqps://localhost:%s" % self.secure_user_port
+ query = 'QUERY --type org.apache.qpid.dispatch.router'
+
+ # this should fail: no trustfile
+ try:
+ self.run_qdmanage(query, address=ssl_address)
+ self.assertTrue(False, "expected qdmanage to fail")
+ except Exception as exc:
Review comment:
There is a `with: block in unittest lib for constructing this style of tests, https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertRaises
I am not sure rewriting it to that form, but maybe for future tests...
I'd like to see catching a more specific exception class than just Exception; I'll try to see in the code if that's practical. (The assert regarding exception test is sufficient protection against catching unrelated exception here, I just want to see if it could be even more specific early on.)
##########
File path: tests/system_tests_qdmanage.py
##########
@@ -612,6 +622,64 @@ def test_check_memory_usage(self):
# is added
self.assertTrue(mem is None)
+ def test_ssl_connection(self):
+ """
+ Verify qdmanage can securely connect via SSL:
+ """
+ ssl_address = "amqps://localhost:%s" % self.secure_port
+ ssl_user_address = "amqps://localhost:%s" % self.secure_user_port
+ query = 'QUERY --type org.apache.qpid.dispatch.router'
+
+ # this should fail: no trustfile
+ try:
+ self.run_qdmanage(query, address=ssl_address)
+ self.assertTrue(False, "expected qdmanage to fail")
Review comment:
```suggestion
self.fail("expected qdmanage to fail")
```
tiny improvement regarding unittest API, https://docs.python.org/3/library/unittest.html#unittest.TestCase.fail
##########
File path: tests/system_tests_qdstat.py
##########
@@ -895,6 +921,29 @@ def test_ssl_trustfile_cert_to_unsecured(self):
def test_ssl_bad_trustfile_to_unsecured(self):
self.ssl_test_bad('unsecured_s', ['bad_trustfile'])
+ def test_ssl_peer_name_verify_disabled(self):
+ """
+ Verify the --ssl-disable-peer-name-verify option
+ """
+ params = [x for x in self.get_ssl_args()['trustfile']]
+ try:
+ self.run_qdstat(address="amqps://127.0.0.1:%s" % self.strict_port,
+ args=['--general'] + params)
+ self.assertTrue(False, "Expected connection attempt to fail!")
+ except AssertionError:
+ # Expect the connection to fail, since the certificate has
+ # 'localhost' as the peer name and we used '127.0.0.1' instead.
+ pass
Review comment:
Don't catch `AssertionError`? Catching AssertionError makes the test pass when the connection attempt does not fail.
##########
File path: tests/system_tests_qdstat.py
##########
@@ -760,7 +774,19 @@ def get_ssl_args(self):
return args
def ssl_test(self, url_name, arg_names):
- """Run simple SSL connection test with supplied parameters.
+ """
Review comment:
The summary sentence of the docstring should be right after `""""`, on the same line https://www.python.org/dev/peps/pep-0008/#documentation-strings and linked https://www.python.org/dev/peps/pep-0257/#multi-line-docstrings
There are some linters for this in flake8 but they are disabled for Dispatch.
##########
File path: tests/system_tests_qdmanage.py
##########
@@ -612,6 +622,64 @@ def test_check_memory_usage(self):
# is added
self.assertTrue(mem is None)
+ def test_ssl_connection(self):
+ """
+ Verify qdmanage can securely connect via SSL:
+ """
+ ssl_address = "amqps://localhost:%s" % self.secure_port
+ ssl_user_address = "amqps://localhost:%s" % self.secure_user_port
+ query = 'QUERY --type org.apache.qpid.dispatch.router'
+
+ # this should fail: no trustfile
+ try:
+ self.run_qdmanage(query, address=ssl_address)
+ self.assertTrue(False, "expected qdmanage to fail")
+ except Exception as exc:
Review comment:
So, the more specific exception won't work all that well for running a subprocess... never mind that.
##########
File path: tests/system_tests_qdmanage.py
##########
@@ -612,6 +622,64 @@ def test_check_memory_usage(self):
# is added
self.assertTrue(mem is None)
+ def test_ssl_connection(self):
+ """
+ Verify qdmanage can securely connect via SSL:
+ """
+ ssl_address = "amqps://localhost:%s" % self.secure_port
+ ssl_user_address = "amqps://localhost:%s" % self.secure_user_port
+ query = 'QUERY --type org.apache.qpid.dispatch.router'
+
+ # this should fail: no trustfile
+ try:
+ self.run_qdmanage(query, address=ssl_address)
+ self.assertTrue(False, "expected qdmanage to fail")
+ except Exception as exc:
+ self.assertIn("certificate verify failed", str(exc),
+ "unexpected exception: %s" % str(exc))
+
+ # this should pass:
+ self.run_qdmanage(query + " --ssl-trustfile " +
+ self.ssl_file('ca-certificate.pem'),
+ address=ssl_address)
Review comment:
```suggestion
with self.subTest("this should pass"):
self.run_qdmanage(query + " --ssl-trustfile " +
self.ssl_file('ca-certificate.pem'),
address=ssl_address)
```
It is possible to add a bit more structure in the test like this. Turning comments into subtest names, https://docs.python.org/3/library/unittest.html#unittest.TestCase.subTest
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] gemmellr commented on a change in pull request #1282: DISPATCH-2190: add SSL tests for qdstat and qdmanage
Posted by GitBox <gi...@apache.org>.
gemmellr commented on a change in pull request #1282:
URL: https://github.com/apache/qpid-dispatch/pull/1282#discussion_r664444156
##########
File path: tests/system_tests_qdmanage.py
##########
@@ -612,6 +622,64 @@ def test_check_memory_usage(self):
# is added
self.assertTrue(mem is None)
+ def test_ssl_connection(self):
+ """
+ Verify qdmanage can securely connect via SSL:
+ """
+ ssl_address = "amqps://localhost:%s" % self.secure_port
+ ssl_user_address = "amqps://localhost:%s" % self.secure_user_port
+ query = 'QUERY --type org.apache.qpid.dispatch.router'
+
+ # this should fail: no trustfile
+ try:
+ self.run_qdmanage(query, address=ssl_address)
+ self.assertTrue(False, "expected qdmanage to fail")
Review comment:
I'd say its more than tiny, makes a huge difference to readability, much easier to skim and understand a straight-up "fail" than considering the assertions.
(Aside, comment applies to the many instances of this)
##########
File path: tests/system_tests_qdstat.py
##########
@@ -895,6 +921,29 @@ def test_ssl_trustfile_cert_to_unsecured(self):
def test_ssl_bad_trustfile_to_unsecured(self):
self.ssl_test_bad('unsecured_s', ['bad_trustfile'])
+ def test_ssl_peer_name_verify_disabled(self):
+ """
+ Verify the --ssl-disable-peer-name-verify option
+ """
+ params = [x for x in self.get_ssl_args()['trustfile']]
+ try:
+ self.run_qdstat(address="amqps://127.0.0.1:%s" % self.strict_port,
+ args=['--general'] + params)
+ self.assertTrue(False, "Expected connection attempt to fail!")
+ except AssertionError:
+ # Expect the connection to fail, since the certificate has
+ # 'localhost' as the peer name and we used '127.0.0.1' instead.
+ pass
Review comment:
This was my main comment from looking it over. I expect its because run_qdstat performs asserts and throws on failure, per ssl_test_bad, but as you say catching that assertion will break the fallback failure when it succeeds connecting unexpectedly, since it obviously does too.
##########
File path: tests/system_tests_qdstat.py
##########
@@ -895,6 +921,29 @@ def test_ssl_trustfile_cert_to_unsecured(self):
def test_ssl_bad_trustfile_to_unsecured(self):
self.ssl_test_bad('unsecured_s', ['bad_trustfile'])
+ def test_ssl_peer_name_verify_disabled(self):
+ """
+ Verify the --ssl-disable-peer-name-verify option
+ """
+ params = [x for x in self.get_ssl_args()['trustfile']]
+ try:
+ self.run_qdstat(address="amqps://127.0.0.1:%s" % self.strict_port,
+ args=['--general'] + params)
+ self.assertTrue(False, "Expected connection attempt to fail!")
+ except AssertionError:
+ # Expect the connection to fail, since the certificate has
+ # 'localhost' as the peer name and we used '127.0.0.1' instead.
+ pass
Review comment:
This was my main comment from looking it over. I expect its because run_qdstat performs asserts and throws on failure, per ssl_test_bad, but as you say catching that assertion in this way will break the fallback failure when it succeeds connecting unexpectedly, since it obviously does too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] jiridanek commented on a change in pull request #1282: DISPATCH-2190: add SSL tests for qdstat and qdmanage
Posted by GitBox <gi...@apache.org>.
jiridanek commented on a change in pull request #1282:
URL: https://github.com/apache/qpid-dispatch/pull/1282#discussion_r665449504
##########
File path: tests/system_tests_qdmanage.py
##########
@@ -86,7 +96,7 @@ def run_qdmanage(self, cmd, input=None, expect=Process.EXIT_OK, address=None):
try:
p.teardown()
except Exception as e:
- raise Exception(out if out else str(e))
+ raise sys.exc_info()[0](out if out else str(e))
Review comment:
Does `sys.exc_info()[0]` mean `type(e)`?
##########
File path: tests/system_tests_qdmanage.py
##########
@@ -86,7 +96,7 @@ def run_qdmanage(self, cmd, input=None, expect=Process.EXIT_OK, address=None):
try:
p.teardown()
except Exception as e:
- raise Exception(out if out else str(e))
+ raise sys.exc_info()[0](out if out else str(e))
Review comment:
I'd go for a custom exception class here, probably. `class QdManageRunError(Exception):` or something like that. It would then allow expecting this in tests, instead of `RuntimeError`, or something unspecific like that. But this should serve ok and there is the check on exception message text, so it should be well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] jiridanek commented on a change in pull request #1282: DISPATCH-2190: add SSL tests for qdstat and qdmanage
Posted by GitBox <gi...@apache.org>.
jiridanek commented on a change in pull request #1282:
URL: https://github.com/apache/qpid-dispatch/pull/1282#discussion_r663343758
##########
File path: tests/system_tests_qdmanage.py
##########
@@ -612,6 +622,64 @@ def test_check_memory_usage(self):
# is added
self.assertTrue(mem is None)
+ def test_ssl_connection(self):
+ """
+ Verify qdmanage can securely connect via SSL:
+ """
+ ssl_address = "amqps://localhost:%s" % self.secure_port
+ ssl_user_address = "amqps://localhost:%s" % self.secure_user_port
+ query = 'QUERY --type org.apache.qpid.dispatch.router'
+
+ # this should fail: no trustfile
+ try:
+ self.run_qdmanage(query, address=ssl_address)
+ self.assertTrue(False, "expected qdmanage to fail")
+ except Exception as exc:
Review comment:
There is a `with: block in unittest lib for constructing this style of tests, https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertRaises
I am not sure rewriting it to that form, but maybe for future tests...
I'd like to see catching a more specific exception class than just Exception; I'll try to see in the code if that's practical. (The assert regarding exception test is sufficient protection against catching unrelated exception here, I just want to see if it could be even more specific early on.)
##########
File path: tests/system_tests_qdmanage.py
##########
@@ -612,6 +622,64 @@ def test_check_memory_usage(self):
# is added
self.assertTrue(mem is None)
+ def test_ssl_connection(self):
+ """
+ Verify qdmanage can securely connect via SSL:
+ """
+ ssl_address = "amqps://localhost:%s" % self.secure_port
+ ssl_user_address = "amqps://localhost:%s" % self.secure_user_port
+ query = 'QUERY --type org.apache.qpid.dispatch.router'
+
+ # this should fail: no trustfile
+ try:
+ self.run_qdmanage(query, address=ssl_address)
+ self.assertTrue(False, "expected qdmanage to fail")
Review comment:
```suggestion
self.fail("expected qdmanage to fail")
```
tiny improvement regarding unittest API, https://docs.python.org/3/library/unittest.html#unittest.TestCase.fail
##########
File path: tests/system_tests_qdstat.py
##########
@@ -895,6 +921,29 @@ def test_ssl_trustfile_cert_to_unsecured(self):
def test_ssl_bad_trustfile_to_unsecured(self):
self.ssl_test_bad('unsecured_s', ['bad_trustfile'])
+ def test_ssl_peer_name_verify_disabled(self):
+ """
+ Verify the --ssl-disable-peer-name-verify option
+ """
+ params = [x for x in self.get_ssl_args()['trustfile']]
+ try:
+ self.run_qdstat(address="amqps://127.0.0.1:%s" % self.strict_port,
+ args=['--general'] + params)
+ self.assertTrue(False, "Expected connection attempt to fail!")
+ except AssertionError:
+ # Expect the connection to fail, since the certificate has
+ # 'localhost' as the peer name and we used '127.0.0.1' instead.
+ pass
Review comment:
Don't catch `AssertionError`? Catching AssertionError makes the test pass when the connection attempt does not fail.
##########
File path: tests/system_tests_qdstat.py
##########
@@ -760,7 +774,19 @@ def get_ssl_args(self):
return args
def ssl_test(self, url_name, arg_names):
- """Run simple SSL connection test with supplied parameters.
+ """
Review comment:
The summary sentence of the docstring should be right after `""""`, on the same line https://www.python.org/dev/peps/pep-0008/#documentation-strings and linked https://www.python.org/dev/peps/pep-0257/#multi-line-docstrings
There are some linters for this in flake8 but they are disabled for Dispatch.
##########
File path: tests/system_tests_qdmanage.py
##########
@@ -612,6 +622,64 @@ def test_check_memory_usage(self):
# is added
self.assertTrue(mem is None)
+ def test_ssl_connection(self):
+ """
+ Verify qdmanage can securely connect via SSL:
+ """
+ ssl_address = "amqps://localhost:%s" % self.secure_port
+ ssl_user_address = "amqps://localhost:%s" % self.secure_user_port
+ query = 'QUERY --type org.apache.qpid.dispatch.router'
+
+ # this should fail: no trustfile
+ try:
+ self.run_qdmanage(query, address=ssl_address)
+ self.assertTrue(False, "expected qdmanage to fail")
+ except Exception as exc:
Review comment:
So, the more specific exception won't work all that well for running a subprocess... never mind that.
##########
File path: tests/system_tests_qdmanage.py
##########
@@ -612,6 +622,64 @@ def test_check_memory_usage(self):
# is added
self.assertTrue(mem is None)
+ def test_ssl_connection(self):
+ """
+ Verify qdmanage can securely connect via SSL:
+ """
+ ssl_address = "amqps://localhost:%s" % self.secure_port
+ ssl_user_address = "amqps://localhost:%s" % self.secure_user_port
+ query = 'QUERY --type org.apache.qpid.dispatch.router'
+
+ # this should fail: no trustfile
+ try:
+ self.run_qdmanage(query, address=ssl_address)
+ self.assertTrue(False, "expected qdmanage to fail")
+ except Exception as exc:
+ self.assertIn("certificate verify failed", str(exc),
+ "unexpected exception: %s" % str(exc))
+
+ # this should pass:
+ self.run_qdmanage(query + " --ssl-trustfile " +
+ self.ssl_file('ca-certificate.pem'),
+ address=ssl_address)
Review comment:
```suggestion
with self.subTest("this should pass"):
self.run_qdmanage(query + " --ssl-trustfile " +
self.ssl_file('ca-certificate.pem'),
address=ssl_address)
```
It is possible to add a bit more structure in the test like this. Turning comments into subtest names, https://docs.python.org/3/library/unittest.html#unittest.TestCase.subTest
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org