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.

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 SSH Provider #115

Merged
merged 3 commits into from Aug 15, 2018
Merged

Add SSH Provider #115

merged 3 commits into from Aug 15, 2018

Conversation

smarlowucf
Copy link
Collaborator

@smarlowucf smarlowucf commented Aug 9, 2018

  • Allows for testing of an instance based on SSH. No provider credentials are necessary.
  • Add ip address option for SSH testing.

@@ -133,6 +133,11 @@ For more information on updating an existing service account:
- Granting roles:
[granting-roles-to-service-accounts](https://cloud.google.com/iam/docs/granting-roles-to-service-accounts)

SSH
---
Requires no provider credentials to test instances. SSH user, SSH private key can
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lets also support a pointer to a private key file. Differentiating his in the code is a simple if condition.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a key file. It is poorly named:

--ssh-private-key PATH SSH private key file for accessing instance.

We can change the name for CLI but probably want to deprecate it then drop it in a major release:

--ssh-private-key-file PATH SSH private key file for accessing instance.

I don't think it should be expected that anyone would pass a private key in command line so just providing the key file option seems good.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please change according to the other Provider class changes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Please change according to the other Provider class changes

Rebased against master.

@@ -575,7 +593,10 @@ def test_image(self):
Returns:
A tuple with the exit code and results json.
"""
if self.running_instance_id:
if self.provider == 'ssh':
# SSH provider: instance must running
Copy link
Collaborator

Choose a reason for hiding this comment

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

missing "be"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed and rebased

ipa/ipa_ssh.py Outdated
self.ssh_private_key = (
ssh_private_key or
self._get_value(ssh_private_key, config_key='ssh_private_key')
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

if os.path.isfile(self.ssh_private_key):
self.ssh_private_key = self.ssh_private_key.read()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See previous comment. The key file path is passed directly into SSH commands, not the value.

ipa/ipa_ssh.py Outdated
else:
self.ssh_private_key = os.path.expanduser(
self.ssh_private_key
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm there is some inconsistency with this implementation vs. my assumption based on the doc. Pleas reconcile.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As per previous comments you cannot pass in a private key string. The CLI validates the string is a path and the file exists.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@click.option(
    '--ssh-private-key',
    type=click.Path(exists=True),
    help='SSH private key file for accessing instance.'
)

@smarlowucf
Copy link
Collaborator Author

@rjschwei This naming confusion has come up before and is really unrelated to this PR. I created an issue to fix it #118.

@smarlowucf smarlowucf force-pushed the ssh-provider branch 2 times, most recently from 618882f to 00c1f4b Compare August 15, 2018 14:27
Allows for testing of an instance based on SSH. No provider
credentials are necessary.
Add info on SSH provider.
@rjschwei rjschwei merged commit 6cd3026 into master Aug 15, 2018
@smarlowucf smarlowucf deleted the ssh-provider branch August 15, 2018 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants