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

Zappa never finds domain name in route 53 and will always delete api gateway domain when calling certify again #458

Merged
merged 2 commits into from
Dec 1, 2016

Conversation

kwellman
Copy link
Contributor

@kwellman kwellman commented Nov 7, 2016

Description

Zappa is not properly checking if there is an existing route 53 record for the domain. Record names end with a period so that has to be stripped when comparing to the domain name.

I've also removed the deletion of the api gateway domain when no route 53 record is found because deletion takes several minutes to take effect and you'll get an error trying to create another one right away (I've run into this problem multiple times). A better way would be to use the update_domain_name method in the api gateway client.

@kwellman kwellman changed the title Zappa never finds domain name in route 53 and will always delete api gateway domain Zappa never finds domain name in route 53 and will always delete api gateway domain when calling certify again Nov 7, 2016
@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 78.762% when pulling 3bb68a6 on kwellman:get_domain_name into b5b22fe on Miserlou:master.

@Miserlou
Copy link
Owner

Can you elaborate a bit more about why you took out the delete_domain_name call? That was added for a reason. I'd certainly like to see that code replaced by a faster/smarter update call, but I don't just want to remove functional code for a performance increase.

I also think this section of code needs to be tested.

@kwellman
Copy link
Contributor Author

I removed the delete_domain_name because I would get this error when that code is executed:

An error occurred (BadRequestException) when calling the CreateDomainName operation: The domain name you provided is already associated with an existing CloudFront distribution.  

It seems like the domain was being deleted, but immediately trying to recreate it (as Zappa currently does) would cause this exception. After waiting a few minutes it would work when running Zappa again. Based on posts like this https://forums.aws.amazon.com/thread.jspa?threadID=222468, it seems like it's expected behaviour and therefore the logic of deleting the api gateway domain to recreate it doesn't work unless a delay is added (but I wouldn't know what amount to use).

With the changes in the pull request, you would have to delete the api gateway manually if you ever get into a situation where the api gateway wasn't created correctly. I prefer this for now because:

  1. Currently the exception above occurs
  2. The method get_domain_name is potentially very dangerous if called when your site is live. As it is now, my api gateway domain gets deleted when certify is called again and this brings down my site. This is because the method also fails to find the route53 records (because of not removing the trailing '.'). Of course that is fixed easily, but I don't think that situation should ever be close to possible. Maybe I'm being too neurotic that bugs could be introduced again.
  3. A method named get_domain_name shouldn't delete anything, but that's just personal style.

Of course I could be wrong and misunderstand something, but the changes work for me. I agree that tests need to be made. I could work on that when I have the time.

@Miserlou Miserlou merged commit 3bb68a6 into Miserlou:master Dec 1, 2016
@Miserlou
Copy link
Owner

Miserlou commented Dec 1, 2016

Okay, I've finally merged this but I'm still finding the behavior to be funky. One particularly annoying piece is that it seems like there is no way to check which server certificates are active in the IAM panel.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants