lightsail - Use AnsibleAWSModule #65275
Conversation
|
@prasadkatti, just so you are aware we have a dedicated Working Group for aws. |
ansibot
added
affects_2.10
- Use AnsibleAWSModule - Refactor the logic for wait into a separate function (Fixes ansible#63869) - Handle exceptions in find_instance_info and add a fail_if_not_found parameter - Add a new state `rebooted` as an alias for `restarted`. AWS calls the action Reboot. - Add required_if clause for when state is present
prasadkatti
force-pushed
the
lightsail_wait
branch
from
2cfe2ed
to
75c7dc0
Compare
|
The test The test The test The test The test |
ansibot
added
needs_revision
| if not HAS_BOTO3: | ||
| module.fail_json(msg='Python module "boto3" is missing, please install it') | ||
| module = AnsibleAWSModule(argument_spec=argument_spec, | ||
| required_if=[['state', 'present', ('zone', 'blueprint_id', 'bundle_id', 'key_pair_name')]]) |
There was a problem hiding this comment.
Personal niggle: boto3 doesn't list keyPairName as a 'required' argument.
I have some use cases where I bake the keys into the AMI and don't want to pass an additional key to the instances...
There was a problem hiding this comment.
I don't think Lightsail allows custom AMIs, at least from what I've seen in the docs and prodding at the console a bit. There is a question of if we should have a default to use the region default key when state=present instead of requiring a user specification, generally the way lightsail does keys is different from the way ec2 does keys though and I'm still familiarizing myself with the differences.
There was a problem hiding this comment.
I was going off my knowledge of EC2, I've not worked with Lightsail.
Fair enough if you can't use custom AMIs (today). At the same time, adding additional 'requirements' on top of boto3's only risks breaking use cases you might not have thought of.
There was a problem hiding this comment.
What concerns me is the implication (not directly stated in the docs, at least that I can find) that you can't add/modify a Lightsail instance's keypair after creation (short of ssh'ing to the instance with an existing key and editing auth keys by hand). If this is correct, I feel like we should anticipate users having a problem here and make keypair a required option for create actions. It may not be boto-y, but it's still something we can easily anticipate and make nicer for our users. If that's not correct and I'm just missing something about Lightsail key management, then maybe keypair management or a lightsail_key module would be good future improvements (but outside the scope of this PR).
There was a problem hiding this comment.
@tremble - The create_instances method takes in a blueprintId parameter that is analogous to an EC2 AMI. The method had a customImageName parameter but that is now deprecated.
It does not look like it is possible to change the keypair used by an existing instance. Boto3 does not require the keyPairName because when it is absent lightsail uses the default key for the region. Currently, the module does not support using that key AFAICT. Updating the module to add the capability of using the default keypair is quite easy. In fact, now that I think about it, we should absolutely do that.
@jillr - I didn't quite get the connection between being able to update an instance's keypair and making the key_pair_name a required option. (required option..lol). Are you saying we don't want users to unknowingly use the default key and then not be able to change it? The private key from the default keypair can always be downloaded at a later time unlike keypairs created explicitly.
I did a bare bones implementation of the lightsail_keypair module that is being used to create a keypair for every run of the lightsail integration testsuite. I would be happy to flesh it out into a fully featured module.
There was a problem hiding this comment.
You can set key_pair_name to "LightsailDefaultKeyPair" ("key_pair_name": "LightsailDefaultKeyPair") to use the default lightsail keypair for that region.
There was a problem hiding this comment.
@prasadkatti : @jillr is mostly worried about protecting folks from making stupid mistakes. If not setting the Key Pair meant it was never possible to get into a host, then there's a case to be made for making it 'required' on the Ansible side (Amazon for example gives you a nice big warning on the console when you do this). If there was a mechanism for injecting a new key somewhere down the line, then this is less of a worry.
Given that AWS will inject a default key when you don't supply one, I think it's reasonable to err towards not putting an additional 'requirement' in front of boto.
There was a problem hiding this comment.
Yes, I agree. We should not make it a requirement.
ansibot
removed
the
needs_triage
|
Added changes to allow usage of the default keypair from the region when creating instances. |
ansibot
added
core_review
| @@ -1,79 +0,0 @@ | |||
| #!/usr/bin/python | |||
There was a problem hiding this comment.
If Lightsail Key Pairs aren't the same as EC2 Key Pairs it would be nice to get this added as an additional module. At which point it would make sense to adjust these integration tests to test two creation modes:
- create a lightsail instance without specifying a key
- create a lightsail instance with a key
There was a problem hiding this comment.
Sure. I can work on a module to manage lightsail keypairs. Although, there are some other higher priority lightsail modules needed IMO. The current module only manages lightsail instances. Lightsail also has databases, networks, storage that will need separate modules. In fact, the current lightsail module should be renamed to lightsail_instance.
|
Fixes #36313 . |
* lightsail - Use AnsibleAWSModule - Use AnsibleAWSModule - Refactor the logic for wait into a separate function (Fixes ansible#63869) - Handle exceptions in find_instance_info and add a fail_if_not_found parameter - Add a new state `rebooted` as an alias for `restarted`. AWS calls the action Reboot. - Add required_if clause for when state is present * lightsail - Use the default keypair if one is not provided * lightsail - add a required_if for when state=present * Update short description for lightsail module
* lightsail - Use AnsibleAWSModule - Use AnsibleAWSModule - Refactor the logic for wait into a separate function (Fixes ansible#63869) - Handle exceptions in find_instance_info and add a fail_if_not_found parameter - Add a new state `rebooted` as an alias for `restarted`. AWS calls the action Reboot. - Add required_if clause for when state is present * lightsail - Use the default keypair if one is not provided * lightsail - add a required_if for when state=present * Update short description for lightsail module
SUMMARY
rebootedas an alias forrestarted. AWS calls the action Reboot.Fixes #63869 #36313
ISSUE TYPE
COMPONENT NAME
lightsail