Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove HTTP fallback #2355

Merged
merged 15 commits into from
Jul 25, 2017
Merged

Remove HTTP fallback #2355

merged 15 commits into from
Jul 25, 2017

Conversation

dvalters
Copy link
Contributor

@dvalters dvalters commented Jul 10, 2017 •
edited
Loading

To close #2204:

  • Fails if HTTPS configured but only HTTP available (no SSL)
  • Communication protocol is stored in suite contact file, can also be set in the users global config file.
  • Default setting is https (same as current behaviour), http must be explicitly configured.

e.g. in global.rc

# Optional, defaults to attempting HTTPS if not set. 
[communication]
    method=http

@dvalters dvalters self-assigned this Jul 10, 2017
@matthewrmshin matthewrmshin added this to the next release milestone Jul 10, 2017
dvalters
dvalters commented Jul 12, 2017
View reviewed changes
tests/communication-method/test_header Outdated
fi
set +x

set +e
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will change this to a symlink instead of a copy of test header.

matthewrmshin
matthewrmshin reviewed Jul 13, 2017
View reviewed changes
lib/cylc/network/daemon.py Outdated
@@ -19,5 +19,5 @@

from cylc.network.method import METHOD

if METHOD == "https":
if METHOD == "https" or "http":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be if METHOD == "https" or METHOD == "http": or if METHOD in ["https", "http"]:

@matthewrmshin
Copy link
Contributor

(As discussed offline, please be aware that cylc.task_message loads the contact file before calling the HTTPS client, so it may be worth making a small modification so that the contact file only gets loaded once.)

@matthewrmshin
Copy link
Contributor

(Site test battery OK in my environment.)

@matthewrmshin
Copy link
Contributor

(Profile battery 😐 neutral in my environment, as expected.)

matthewrmshin
matthewrmshin approved these changes Jul 14, 2017
View reviewed changes
Copy link
Contributor

@matthewrmshin matthewrmshin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now looking good.

@dvalters
Remove HTTP fallback automatically
46cc08e
@dvalters
Dumnmy methods set up to read from suite contact file
8130bb0
@dvalters
Modifications to suite srv mangager
f993c75
@dvalters
Committing relatively stable version now
bc5ad35
@dvalters
Removed debugging print statements
a56e013
@dvalters
Extra unit tests
8ec36b7
@dvalters
PEP8 compliance
da232d0
@dvalters
Codacy suggestions fixed
8d3a48d
@dvalters
New test-battery tests for http-https
a6f716b
@dvalters
Updated cylc check-software for OpenSSL
721c6f8
@dvalters
Changes to the CUG to reflect http fallback removal
806d7d0
@dvalters
Fix if import logic, change to loading of contact file.
5c2e0c2 
Passes the comms_protocol/schema to the BaseCommsClient from task
message,py and also added a check to see if we have it already stored in
the contact file before going to the global config file.
@dvalters
Logic for dealing with no SSL in task_job_mgr
4df408e
@dvalters
PEP8
c7bc6eb
hjoliver
hjoliver reviewed Jul 20, 2017
View reviewed changes
lib/cylc/network/https/base_client.py Outdated
@@ -38,6 +39,11 @@
" falling back to HTTP: {0}\n"
)

ERROR_NO_HTTPS_SUPPORT = (
"ERROR: server has no HTTPS support," +
" configure user's global.rc file to use HTTPS : {0}\n"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't that be "to use HTTP" (no S!).

Better wording might be "configure your global.rc file to use HTTP" (as the the person receiving this warning will be the user in question).

lib/cylc/network/https/base_client.py
return comms_prtcl
except (AttributeError, KeyError, TypeError, ValueError):
raise KeyError("No suite contact info for comms protocol found")

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to be worried about back-compatibility here, for already-running suites?

Copy link
Contributor Author

@dvalters dvalters Jul 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will fall back to finding it from the global.rc file if not in the contact file here, which already has a default setting of https in previous versions. But yes, what if the old running suite is using http...I will have to think about how to handle this...

(Already-running suites using https should be ok with this)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, roger that.

lib/cylc/network/https/base_client.py Outdated
http_request_items)
sys.stderr.write(ERROR_NO_HTTPS_SUPPORT.format(exc))
raise CylcError("Cannot issue a https command"
" over unsecured http.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not really an "https command". Maybe just "Cannot issue commands over unsecured http"?

lib/cylc/network/https/base_client.py Outdated
http_request_items)
sys.stderr.write(ERROR_NO_HTTPS_SUPPORT.format(exc))
raise CylcError("Cannot issue a https command"
" over unsecured http.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

hjoliver
hjoliver approved these changes Jul 25, 2017
View reviewed changes
Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good now.

@hjoliver hjoliver merged commit 9b7d344 into cylc:master Jul 25, 2017
@hjoliver hjoliver mentioned this pull request Jul 26, 2017
Closed
This was referenced Aug 7, 2017
Merged
Merged
@matthewrmshin matthewrmshin mentioned this pull request Oct 18, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hjoliver hjoliver hjoliver approved these changes

@matthewrmshin matthewrmshin matthewrmshin approved these changes

Assignees

@dvalters dvalters

Labels
None yet
Projects
None yet
Milestone
cylc-7.5.0
Development

Successfully merging this pull request may close these issues.

Remove auto-fallback to HTTP comms.
3 participants
@dvalters @matthewrmshin @hjoliver