-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add https to core system #11
Conversation
Thanks a lot for this contribution @aragilar. It looks like very good work! I will go thoroughly through the code but that will probably have to wait until tomorrow or early next week, as my plate is currently a bit full. I gave the patch a quick overview though, and I wanted to have a clear idea of the intent behind this.
Please let me know your thoughts on this, specially about your use case as I might have misunderstood it from the beginning. I would definitely want to accept the client-side code changes as part of this PR as they are definitely useful! The server-side changes are the only thing that I need some clarifications on, basically to understand how necessary they are to your use case. |
Regarding failures: the coverage always fluctuates a bit (it's not 100% deterministic), so it can be somewhat safely ignored. The MacOS build failed because it took too long -- and in general all jobs took longer than previously, do you have an idea of why this might be? For example: $> git co master
$> pytest test/commands/test_containers.py::ngamsContainerTest::test_AppendRemove -v |& grep passed
=========================== 1 passed in 1.92 seconds ===========================
$> git co add-https
$> pytest test/commands/test_containers.py::ngamsContainerTest::test_AppendRemove -v |& grep passed
========================== 1 passed in 11.43 seconds =========================== |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aragilar I just finished reviewing more in depth your changes. They look awesome! There are a few minor formatting things, and a few observations on the import machinery, but otherwise looks fine.
I think my reading yesterday was correct, in that you are adding https support on the server just to support testing. If that is the case, I'd rather include only the client-side changes in this PR, and replace the test with a dummy server. The server-side changes are still quite good, but I'd save them until we really start picking up https support on the server.
I think the slowdown might be some of the TLS stuff, I'll see if I can speed it up. |
I think it was the |
I think I've made all the changes. Major things:
|
It looks like the mac build failed to install berkeleydb for some reason? |
test/test_subscription.py
Outdated
@@ -321,3 +333,61 @@ def test_continuous_subscription(self): | |||
self.retrieve(8889, fname, fileVersion=2, targetFile=tmp_path()) | |||
finally: | |||
subscription_listener.close() | |||
|
|||
if ssl is not None: | |||
@unittest.skipIf('NGAS_TESTDB' in os.environ, "Need two cluster for this test to run") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is still preventing the tests from running in Travis because NGAS_TESTDB is always set, but sometimes to an empty value; hence the coverage still doesn't show the code paths being executed. This will need to be changed to os.environ.get('NGAS_TESTDB', None)
or similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also the if ssl is not None
could be part of the condition within skipIf
so the whole method won't require an extra level of indentation
test/test_subscription.py
Outdated
tmpCfgFile = genTmpFilename(suffix='xml') | ||
cfg.save(tmpCfgFile, 0) | ||
self.prepExtSrv(port=8778, cert_file=cert_file) | ||
self.prepExtSrv(port=8779, cert_file=cert_file, cfgFile=tmpCfgFile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of creating a configuration file, saving it, and passing it via cfgFile
you can pass down a list of 2-element tuples directly to prepExtSrv via cfgProps
and it will do that for you. Shouldn't change the behavior in any way, just make it easier to read the test code.
test/ngamsTestLib.py
Outdated
def __init__(self, add_to_certifi=True): | ||
self._create_ca() | ||
if add_to_certifi: | ||
self.add_ca_to_certifi() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is causing problems when running the tests multiple times. Each time this get executed, a new certificate is appended into the vault for the same CA; then I get SSLError(1, '[SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed (_ssl.c:852)')
probably due to the different certificates for the same CA? If I go to $VIRTUAL_ENV/lib/python3.6/site-packages/certifi/cacert.pem
and remove the last certificates generated by this code I can run the test again.
I think a better approach would be not to save the CA perm into the central vault, and instead capture it and give it to the ngamsPClient for usage with requests
. It's a bit more of code, but you'd avoid touching the user's vault, and add the flexibility to the ngamsPClient to use arbitrary CA certs.
if self.ngasServer.getCfg().getVal('NgamsCfg.Server[1].UseSendfile'): | ||
pysendfile.sendfile(self.connection, fin, start_byte) | ||
else: | ||
pysendfile.sendfile_send(self.connection, fin, start_byte) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see my comment above, I think the new UseSendfile
setting is unnecessary given we have already ngasServer.get_server_access_proto
or ngasServer._cert
to distinguish this.
Thanks for the updates! It looks neater now. The MacOS build seems to be a homebrew problem totally unrelated to us; I wouldn't worry about it for now, but I'm still giving it a couple of restarts to see if I it's intermittent enough so it goes away after a couple of tries. I think at the moment the main points to solve are two: first, the new test is still not being executed in Travis, because of the condition used for skipping, please see my comments on that. That explains why the coverage still doesn't pick up the new code paths. And secondly, the notification listener check on the https subscription test fails because there's effectively no file transmission going through! This is because you are starting two server independently, and this is currently not well supported by the testing code (the second will overwrite the root directory created by the first). I'd suggest you do exactly what is done in the |
b098c09
to
78f6b82
Compare
Hi @aragilar any news on this? I'm really looking forward to include these changes, which seem to be almost ready for inclusion. A few comments on the last push (there were are a couple of tests failing in Travis):
- first_server = [[8778, []],]
- second_server = [
- [8779, [
- ['NgamsCfg.ArchiveHandling[1].EventHandlerPlugIn[1].Name',
- 'test.ngamsTestLib.SenderHandler'],
- ]],
- ]
- self.prepCluster(first_server, cert_file=cert_file)
- self.prepCluster(second_server, cert_file=cert_file)
+ self._prep_subscription_cluster(8778, (8779, [], True), cert_file=cert_file)
- if ssl is not None:
- @unittest.skipIf(os.environ.get('NGAS_TESTDB'), "Need two cluster for this test to run")
- def test_https_subscription(self):
+ @unittest.skipIf(ssl is None, "Need SSL for this test to run")
+ def test_https_subscription(self):
I think that's the gist of it. Let me know if I can be of any assistance! :) |
Sorry, was working on other tasks today, I should have something either today or (more likely, given the time here) tomorrow. I know I tried running them as a single cluster previously, but had issues with the http vs https protocol things, I'll see if I modify the database whether that will be fixed. |
Apparently I didn't need to modify the database. I thought the cluster needed to communicate over http, but apparently not, I suspect the errors I was seeing previously when using a single cluster were down to the sendfile usage. |
Great! This looks even neater than before, and all checks are now happily passing. I also tested locally a couple of times and it all seems in working order, so I merged just now. Thanks again @aragilar for the great effort! :) |
This adds some HTTPS support to NGAS. The main changes are whether sendfile is used (it's possible to use sendfile with https, but you need a recent linux kernel, very new openssl etc., so it makes sense to wait until that has stabilised before trying to add that), and using urls where possible to keep around the protocols. Let me know if there are any issues with this (I don't think there are any breaking changes.
Closes #8.