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

EC2 SSH key use user data #68

Merged
merged 7 commits into from
Mar 27, 2018
Merged

EC2 SSH key use user data #68

merged 7 commits into from
Mar 27, 2018

Conversation

smarlowucf
Copy link
Collaborator

@smarlowucf smarlowucf commented Mar 8, 2018

Use user data to add SSH public key to instance on EC2 launch when ssh key name is not provided.

  • SSH public key is generated based on private key file.
  • ssh_key_name arg is optional but takes precedence if provided.

@rjschwei
Copy link
Collaborator

This is snot a good idea. If the code that handles user data is broken, and we probably want a test for that in IPA ;) then debugging is basically impossible as one cannot login.

@rjschwei rjschwei closed this Mar 20, 2018
@smarlowucf
Copy link
Collaborator Author

If the code that handles user data is broken

If the code is in EC2 what precludes key-pair code from also being broken?

@smarlowucf
Copy link
Collaborator Author

then debugging is basically impossible as one cannot login.

We also decided that from MASH the instances are not debuggable. So why can't this be an option? Provide optional ssh-key name which takes precedence over user-data.

@smarlowucf
Copy link
Collaborator Author

smarlowucf commented Mar 20, 2018

@rjschwei There's a plethora of ways an instance could end up un-reachable which cannot be distinguished by IPA.

  • Might be launched in a subnet which the calling system does not have access to.
  • Instance might have failed initialization.
  • Possible failure on reboot.
  • Mismatching key-pair.
  • Incorrect user.
  • Local network might be blocking connection.
  • Etc. etc.

From an automated testing standpoint all we can do is log a generic failure that instance is not reachable. And fail the test.

Then if ssh-key and user-data were interchangeable options one could manually test both with IPA. So IMHO this would be an advantage. Now the user can use IPA to test cloud-init issues. Otherwise the user would have to use a separate tool/method if debugging cloud-init problem.

If provided instance is launched using the ssh key name. Otherwise
the instance the ssh public key is added via user-data and cloud
init.
@smarlowucf
Copy link
Collaborator Author

Reopening with amended commit. I think allowing for optional ssh key name solves the concern of debugging instances with cloud init issues.

And default use of cloud-init user-data gives an added implicit test for user-data handling in images.

@smarlowucf smarlowucf reopened this Mar 20, 2018
@rjschwei
Copy link
Collaborator

1.) The proposed implementation was not optional, it was for removing the the ssh-key argument having an optional implementation is a different discussion
2.) The ssh-key as "built in" vs. "provided as user data" is a much simpler implementation and a data path that has historically never been broken. However user data execution/injection has been broken.
3.) mash is another level of abstraction where ipa runs in an automated fashion, thus not supporting "debugging" in that environment is a completely different cup of tea.
4.) Most of the other issues you mentioned could be considered "operator errors"

@smarlowucf
Copy link
Collaborator Author

smarlowucf commented Mar 20, 2018

The proposed implementation was not optional

Right, but the PR was closed prematurely without providing an opportunity to discuss possible resolutions for your concerns.

Which I think making it optional is certainly worth consideration. Since that means from an API standpoint nothing that was available in regards to ssh key names has been removed. And if provided the key name is still used by default.

Most of the other issues you mentioned could be considered "operator errors"

Right, but whether or not they are user created doesn't matter. In any of the scenarios IPA cannot provide a more useful error aside from, "The instance is unreachable". But this should be moved to a different discussion thread.

Instead of using cloud-init a bash script removes more variables
from the equation.
ipa/ipa_ec2.py Outdated
"""
key = ipa_utils.generate_public_ssh_key(self.ssh_private_key).decode()
data = BASH_SSH_SCRIPT.format(user=self.ssh_user, key=key)
return data
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not data, the value returned is the actual script, i.e. name it "script"

Update docstring to reflect return value type.
@rjschwei rjschwei merged commit 3b2ea73 into master Mar 27, 2018
@smarlowucf smarlowucf deleted the ec2-user-data branch March 28, 2018 14:55
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.

2 participants