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

Safeguard against already existing _acme-challenge records #6

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Tea23
Copy link

@Tea23 Tea23 commented Dec 19, 2023

I noticed that sometimes my renewal would fail, either because of a timeout or something else. In these cases, the _acme-challenge record wouldn't be cleaned up and then any future runs of the renewal would then fail because the record already exists (ERROR: Response: {"result":null,"success":false,"errors":[{"code":81057,"message":"Record already exists."}],"messages":[]})

This adds a simple check to see if the record already exists, and deletes it if it does. I considered trying to clean up after failure instead, but this actually catches other potential sources of _acme-challenge, such as if multiple tools are used.

@GrantGryczan
Copy link

GrantGryczan commented Mar 20, 2024

I'm not a fan of cleaning up other potential sources of _acme-challenge. Cleaning up strictly the records this script created would be ideal, because otherwise you could be deleting _acme-challenge records that are still in use. The DNS challenge takes half a minute, so it's not unlikely that two of these on the same domain starting around the same time (which I have) would conflict with each other.

Also, cleaning up immediately after failure wouldn't be robust since a system reboot (or taking down a container, if it's in a container) would prevent a post-failure cleanup from running. I think a better way might be to save a temporary file remembering which _acme-challenge record will be created before creating it, and delete that file after a successful run. Then, if the file exists when the script starts, delete the record it refers to.

@socram8888
Copy link
Owner

I cannot accept the PR as is - code should use tabs as the rest, and it should cache the result of list_record_id into a variable rather than calling twice.

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