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

Verify that EIP was actually associated. #25

Closed
wants to merge 2 commits into from

Conversation

brodygov
Copy link

Add functionality to check that the EIP was actually associated with the
desired instance. This is important because the EC2 API is eventually
consistent and receiving an HTTP 200 response from the association API call is
not a guarantee that the association will actually happen.

This verification is enabled by default but can be disabled with
--skip-verify.

By default, it will retry up to 10 times, sleeping 0.1 seconds after
each attempt, in order to give the EC2 API plenty of time to achieve
a result.

This allows aws-ec2-assign-elastic-ip to exit nonzero when there is a
race condition that causes EIP association to fail, for example because another
instance ran the script at the same time and won the race to assign the EIP.

Fixes: #24

Add functionality to check that an EIP was actually associated with the
desired instance. This is important because the EC2 API is eventually
consistent and receiving an HTTP 200 response from the association API
call is not a guarantee that the association will actually happen.

This verification is enabled by default but can be disabled with
--skip-verify.

By default, it will retry up to 10 times, sleeping 0.1 seconds after
each attempt, in order to give the EC2 API plenty of time to achieve
a result.

This allows aws-ec2-assign-elastic-ip to exit nonzero when there is a
race condition that causes EIP association to fail.

Fixes: sebdah#24
Copy link
Owner

@sebdah sebdah left a comment

Choose a reason for hiding this comment

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

Well written PR @brodygov, thank you so much. I just have a little nitpick comment. But I'd be happy to merge this if you could just update that line.

Cheers

PARSER.add_argument(
'--skip-verify',
help='Skip verification that the EIP was successfully associated',
action='store_false', default=True, dest='verify')
Copy link
Owner

Choose a reason for hiding this comment

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

default=True is redundant here, that would be the default since action='store_false', right?

Copy link
Author

Choose a reason for hiding this comment

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

Yep, updated the PR

@brodygov
Copy link
Author

brodygov commented Dec 2, 2017

Actually I am still encountering a race condition here, even with this patch.

My current hypothesis is that this is a boto bug. It appears that boto doesn't pass an AllowReassociation param even when it is set to false. This is problematic because in a VPC only account, AWS will automatically allow reassociation by default unless that param is provided and set to false.

https://github.com/boto/boto/blob/6c5b98861d726fdd5e05702972b14692e73e84f4/boto/ec2/connection.py#L1909-L1910

http://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_AssociateAddress.html

AllowReassociation
[EC2-VPC] For a VPC in an EC2-Classic account, specify true to allow an Elastic IP address that is already associated with an instance or network interface to be reassociated with the specified instance or network interface. Otherwise, the operation fails. In a VPC in an EC2-VPC-only account, reassociation is automatic, therefore you can specify false to ensure the operation fails if the Elastic IP address is already associated with another resource.

Type: Boolean

Required: No

@sebdah
Copy link
Owner

sebdah commented Dec 4, 2017

Hmm, yeah, that's annoying. Have you raised / found a bug / question to boto about this? I'm thinking we hold off mergning this until the boto issue is resolved. What do you think?

@kylemacfarlane
Copy link

I don't use this script anymore but was having my own race and other verification problems so checked in to see how this script handles them.

Personally I believe that this PR doesn't actually solve anything.

AllowReassociation=False does work

The Boto source is the way it is because the AWS API defaults to false. If two instances race for the same Elastic IP and reassociation isn't allowed then a ClientError with a code of Resource.AlreadyAssociated will be raised.

Thus because this script doesn't allow reassociations (#13) there is no benefit to verifying with get_all_addresses / describe_addresses as done in this PR.

An association is not a remap

This is the real problem (for me at least): just because you've successfully associated with an Elastic IP doesn't mean it has been remapped and is your public IP yet.

You can verify this with the following code:

import boto3
import requests

md = requests.get('http://169.254.169.254/latest/dynamic/instance-identity/document').json()
ec2 = boto3.client('ec2', region_name=md['region'])
ec2.associate_address(InstanceId=md['instanceId'], AllocationId='YOUR-ALLOCATION-ID', AllowReassociation=True)
print(requests.get('https://api.ipify.org').text)

This will output your current IP and not the Elastic IP you just associated with.

This is a problem if you need a whitelisted IP to access a third party. There's no way to truly verify your public IP with the AWS API so you have to use a third party like ipify.org, whatismyip.com, etc. None of them seem to be backed by large companies so I don't know who to choose for the best privacy and reliability. Does anyone have any recommendations?

@kylemacfarlane
Copy link

kylemacfarlane commented Jul 5, 2018

Sorry, I just realised that there probably is a bug in regards to AllowReassociation in Boto 2 but since I use Boto 3 I didn't run into it.

Also it would be prudent to explicitly set AllowReassociation to False for any EC2-VPC-only account holders. This is because while the EC2 pricing page says you're billed per remap a warning in the Boto 3 docs point out you're actually billed per successful association even if no remap takes place: https://boto3.readthedocs.io/en/latest/reference/services/ec2.html#EC2.Client.associate_address

So if there happened to be a bug in this script (there's not currently one that I am aware of) and someone was using this script in a 1 minute cronjob they could be charged up to $6 per hour per instance.

@brodygov
Copy link
Author

brodygov commented Jul 5, 2018

The Boto source is the way it is because the AWS API defaults to false.

The AWS API defaults to AllowReassociation=True if it's not provided in the API call.

Sorry, I just realised that there probably is a bug in regards to AllowReassociation in Boto 2 but since I use Boto 3 I didn't run into it.

Right, this is a bug in Boto 2. Boto 3 does not have this problem since it explicitly sets AllowReassociation=True by default.

I made a fix for boto2 but never got around to writing tests and submitting it. ab/boto@239013b

It might be easier to just upgrade this script to use boto3 instead.

In any case, the post hoc verification done in this PR isn't a good way to handle it, so I'm closing this.

For the problem of the actual mapping of the EIP being eventually consistent and taking a while, I personally just add a sleep ~20s to hopefully give EC2 enough time to map the EIP before making network requests.

@brodygov brodygov closed this Jul 5, 2018
@brodygov brodygov deleted the verify-association branch July 5, 2018 21:41
@kylemacfarlane
Copy link

The AWS API defaults to AllowReassociation=True if it's not provided in the API call.

It seems to be true for new AWS accounts and false for pre-2014 accounts even if you're actually using a VPC.

So this script will behave differently depending on the age of your AWS account. Upgrading to boto3 and explicitly setting AllowReassociation to false is probably the only fix.

@kylemacfarlane
Copy link

@brodygov I discovered that there actually is https://checkip.amazonaws.com which you can poll instead of blindly sleeping for 20s.

@brodygov
Copy link
Author

Ah thanks for that, I didn't know about checkip.amazonaws.com.

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.

3 participants