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

Fix HTTPS client host no domain issue #2432

Merged
merged 8 commits into from
Oct 19, 2017
Merged

Fix HTTPS client host no domain issue #2432

merged 8 commits into from
Oct 19, 2017

Conversation

matthewrmshin
Copy link
Contributor

@matthewrmshin matthewrmshin commented Sep 20, 2017 •
edited
Loading

A CLI specified host name with no domain part was causing the HTTPS
client to fail because it is unable to match the host name in the SSL
certificate (in which the host name has the domain name).

Rename cylc.suite_host to cylc.hostuserutil (as the module is now a generic utility for working with hosts and users).

Should fix #2422.

@matthewrmshin matthewrmshin added this to the next release milestone Sep 20, 2017
@matthewrmshin matthewrmshin self-assigned this Sep 20, 2017
matthewrmshin added a commit to matthewrmshin/cylc-flow that referenced this pull request Sep 20, 2017
@matthewrmshin
Remove unnecessary fix
6f56179 
Proper fix in cylc#2432.
dvalters
dvalters reviewed Sep 28, 2017
View reviewed changes
Copy link
Contributor

@dvalters dvalters left a comment

Choose a reason for hiding this comment

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

Looks good - I am not seeing the symptoms in #2422 any more

lib/cylc/hostuserutil.py
self._host = self._get_host_info()[0]
return self._host

def get_fqdn_by_host(self, target):
Copy link
Contributor

Choose a reason for hiding this comment

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

Could fqdn be replaced by a more verbose name (or at least an expansion of the abbreviation in the docstring)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fully qualified domain name is probably too long? And fqdn matches socket.getfqdn, which is hopefully what this method is returning.

Copy link
Contributor

Choose a reason for hiding this comment

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

It could just go in the docstring then

@oliver-sanders
Copy link
Member

The hostname as it appears in the title of the gcylc window is different when gcylc is launched from the command line to when it is launched from gscan.

This fixes #2422 for me but I have found a new similar bug which can be repeated with these steps:

  1. Run a suite on another host.
  2. Open gcylc <suite> from the command line.
  3. Open another gcylc window from gscan.
  4. Stop the suite.
  5. Restart the suite from the first gcylc window.

@dvalters
Copy link
Contributor

dvalters commented Sep 28, 2017 •
edited
Loading

The hostname as it appears in the title of the gcylc window is different when gcylc is launched from the command line to when it is launched from gscan.

Yes I get this too - it is missing the domain name qualifier when gcylc is launched via gscan - it only prints the base hostname.

@matthewrmshin
Copy link
Contributor Author

(Should have gone for my original implementation, which would have eliminated all of these issues.)

@matthewrmshin
Copy link
Contributor Author

(Should now work?)

@dvalters
Copy link
Contributor

Yep, the gcylc title outputs are matching now. @oliver-sanders bug looks to be fixed too.

oliver-sanders
oliver-sanders approved these changes Oct 2, 2017
View reviewed changes
Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

Seems to all be working now.

matthewrmshin added a commit to matthewrmshin/cylc-flow that referenced this pull request Oct 5, 2017
@matthewrmshin
Remove unnecessary fix
c0862ff 
Proper fix in cylc#2432.
@matthewrmshin matthewrmshin force-pushed the fix-https-client-host-no-domain-issue branch from 0c95d43 to 5a8bb97 Compare October 10, 2017 10:12
@matthewrmshin
Copy link
Contributor Author

(Conflicts resolved.)

matthewrmshin added a commit to matthewrmshin/cylc-flow that referenced this pull request Oct 19, 2017
@matthewrmshin
Remove unnecessary fix
6f11c67 
Proper fix in cylc#2432.
@matthewrmshin
Copy link
Contributor Author

@dvalters please do a final sanity check.

dvalters
dvalters reviewed Oct 19, 2017
View reviewed changes
Copy link
Contributor

@dvalters dvalters left a comment

Choose a reason for hiding this comment

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

No comment.

@matthewrmshin
Fix HTTPS client host no domain issue
7dd4b7d 
A CLI specified host name with no domain part was causing the HTTPS
client to fail because it is unable to match the host name in the SSL
certificate (in which the host name has the domain name).
@matthewrmshin
Reinstate hardwire address idenitification
68703e4
@matthewrmshin
httpclient: get host to behave earlier
66de8d7
@matthewrmshin
host issue feedback
38be9c7
@matthewrmshin
Fix GUI network client reset issue
92127c4 
Reset GUI network client when suite is stopped. Create new client on
reconnect.
@matthewrmshin
gui network client: fix port reset
3b9960c
@matthewrmshin
Reinstate lost logic on merge
189639a
@matthewrmshin matthewrmshin force-pushed the fix-https-client-host-no-domain-issue branch from 5a8bb97 to 189639a Compare October 19, 2017 10:29
@matthewrmshin
Copy link
Contributor Author

(All looking healthy again.)

@matthewrmshin
Copy link
Contributor Author

(1 final tweak. Waiting for test.)

@matthewrmshin
Copy link
Contributor Author

(Merge as all green with double review approval.)

@matthewrmshin matthewrmshin merged commit 31b1398 into cylc:master Oct 19, 2017
@matthewrmshin matthewrmshin deleted the fix-https-client-host-no-domain-issue branch October 19, 2017 11:48
hjoliver pushed a commit to hjoliver/cylc-flow that referenced this pull request Oct 4, 2018
@matthewrmshin @hjoliver
Remove unnecessary fix
83b726a 
Proper fix in cylc#2432.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dvalters dvalters dvalters approved these changes

@oliver-sanders oliver-sanders oliver-sanders approved these changes

Assignees

@matthewrmshin matthewrmshin

Labels
None yet
Projects
None yet
Milestone
cylc-7.6.0
Development

Successfully merging this pull request may close these issues.

gscan/gcylc: launching gcylc via gscan incorrectly shows suite as stopped
3 participants
@matthewrmshin @oliver-sanders @dvalters