Adding append_hash parameter to ec2_lc module for #1321

[shawnsi]

Before now I've used select parameters (instance size and AMI) in my launch configuration names. At some point AWS stopped allowing deletion of a launch configuration which is attached to an autoscaling group. That makes it very difficult to deal with changes that didn't explicitly update the launch configuration name. This hash based approach should guarantee a consistent and distinct launch configuration name for every possible set of parameters.

This implements the issue described in #1321 as well.

[gregdek]

Thanks @shawnsi for this PR. This module is maintained by the Ansible core team, so it can take a while for patches to be reviewed. Thanks for your patience.

Core team: please review according to guidelines (http://docs.ansible.com/ansible/developing_modules.html#module-checklist) and comment with 'needs_revision' or merge as appropriate.

[This message brought to you by your friendly Ansibull-bot.]

[akvadrako]

This seems quite important to have, since otherwise launch configurations are not idempotent. I would however prefer another approach because this doesn't behave how one would expect ansible modules to behave. Instead of an append_hash I would use name_prefix, which replaces name (since it's no longer accurate) and appends a hash.

Or, even better, allow a general magic variable in tasks called task_hash then we can say name=mylc-{{ task_hash }}.

[shawnsi]

@akvadrako Are you proposing that I then set name from the concatenated name_prefix variable and computed hash? I currently register the result of the task calling the module and use the name variable in any downstream launch configuration references.

[akvadrako]

I think that would preserve the semantics of name better.

[ryansb]

This is mostly an aesthetic request, but the AWS UI doesn't handle super-long launchconfig names that well. Could you maybe truncate the hash to 8 characters? For the hash-collision-concerned: 8 hex characters encodes 32 bits of the hash, and to reach a probability of 50% collision, you'd need to have (on average) 77164 hashes. Of course, it's unlikely someone will have 77,000 launchconfigs, let alone 77,000 at the same time.

hash_size = 2**32 # 32 bits
probUnique = 1.0
for k in xrange(1, 200000):
    probUnique = probUnique * (hash_size - (k - 1)) / hash_size
    if probUnique <= 0.5: 
        print "greater than 50% probability of collision at {} hashes, probability {}".format(k, probUnique * 100)
        break

[shawnsi]

@ryansb thanks for the feedback. I think aligning with API capabilities and lower probability of collisions makes sense for automation. Is there a specific operation in the console this complicates? I've been using it in production for some time and haven't hit any issues.

My vote is for more bits. According to https://en.wikipedia.org/wiki/Birthday_attack 32 bit collisions would creep into the realm of possibility without exceeding launch configuration limits. It probably won't happen to either of us but why make it more likely?

[shawnsi]

@ryansb do you have any opinion on @akvadrako's comments regarding the handling of the name variable?

[ansibot]

This repository has been locked. All new issues and pull requests should be filed in https://github.com/ansible/ansible

Please read through the repomerge page in the dev guide. The guide contains links to tools which automatically move your issue or pull request to the ansible/ansible repo.

[ansibot]

This repository has been locked. All new issues and pull requests should be filed in https://github.com/ansible/ansible

Please read through the repomerge page in the dev guide. The guide contains links to tools which automatically move your issue or pull request to the ansible/ansible repo.