Provider Porkbun: Delete Default Parked DNS Entry for *.domain.tld

[bentemple]

Description:

Porkbun creates default ALIAS and CNAME domain records pointing to pixie.porkbun.com (Porkbun's parked domain website)

The current logic flow prior to this PR would look for an A or AAAA domain record, and if none exists, attempt to delete the ALIAS record for any subdomain and then proceed with the record creation.
This updates the logic flow to only look for a conflicting ALIAS record for the top level domain.tld, and a conflicting CNAME record for the *.domain.tld. Additionally, we verify that the content of this record matches pixie.porkbun.com and we only delete for the expected default value.
If the value does not match the expected pixie.porkbun.com we produce more helpful error messages.

Note: Any other domain name we simply attempt to create a record if no record exists, which will return a 400 error with no helpful error reason if there is a conflicting record of another type. We may want to consider checking for any conflicting records to produce a more helpful error for the user, but that change is well outside of the scope of this PR, and should likely be done in a more general way and apply to all DNS providers, rather than porkbun exclusively.

Test-Plan:

Created a new domain.tld on Porkbun
Verified the default records were created:
ALIAS domain.tld -> pixie.porkbun.com
CNAME *.domain.tld -> pixie.porkbun.com
Started DDNS-Updater
Verified that both domain records were successfully deleted and updated

Reset the ALIAS domain record to point to not-pixie.porkbun.com
Reset the CNAME domain record to point to not-pixie.porkbun.com
Started DDNS-Updater
Verified that both domain records failed with the expected conflicting record error message.

[bentemple]

As commented elsewhere, probably should scrap this and just move the logic higher in the program. i.e. on a failed check, after 5 minutes re-check if the domain is in-fact wrong and needs updating still.

Though, this would get into a scenario where ostensibly you might be failing to update 1 provider, but another provider has the correct DNS, and so it obfuscates the error.

[Bananas-Are-Yellow]

Ultimately simply restarting the DDNS-Updater service after the domain has successfully propogated will resolve the issue

I'm having the 400 error problem, but the problem is not resolved by restarting the container with:

docker compose stop && docker compose up -d

The updates.json file shows the correct IP address recorded a few times, but the 400 error still occurs once the A records have been set. It's as if the updates.json file is not being read to see if the IP address has changed.

Here is my config.json file.

{
    "settings": [
        {
            "provider": "porkbun",
            "domain": "example.com",
            "host": "@,*",
            "api_key": "pk1_...",
            "secret_api_key": "sk1_...",
            "ip_version": "ipv4",
            "ipv6_suffix": ""
        }
    ]
}

[qdm12]

As commented elsewhere, probably should scrap this and just move the logic higher in the program. i.e. on a failed check, after 5 minutes re-check if the domain is in-fact wrong and needs updating still.

I think #485 would solve it, and we can log a warning if that period is less than the ttl of the record. I will mark it as 🚨 urgent, but will hold off a few days to start working on it (got other issues to attend to!)

this would get into a scenario where ostensibly you might be failing to update 1 provider, but another provider has the correct DNS, and so it obfuscates the error.

One more reason we need a period per setting to clarify/simplify all this 😄

@Bananas-Are-Yellow First of all bananas can be brown and black too 🍌 !

I'm having the 400 error problem

That's odd, @bentemple any idea why that could be? @Bananas-Are-Yellow can you share your logs, running the container with LOG_LEVEL=debug (⚠️ and hide your credentials)

It's as if the updates.json file is not being read to see if the IP address has changed.

It's not, the program resolves the domain name (abc.domain.com) every period and compares it to your public ip address. updates.json is only used to store changes for the web ui AND is used by cloudflare when proxied is enabled, since ddns-updater cannot resolve the domain (since it's proxied).

[Bananas-Are-Yellow]

It looks like I misunderstood how updates.json is used. I thought it was used as a record of what had already been set, to avoid setting the same value again. My confusion came from this:

Ultimately simply restarting the DDNS-Updater service after the domain has successfully propogated will resolve the issue, but this can be quite confusing for someone who is dealing with setup issues, removes the updates.json and then gets into a bad state where the domain is constantly returning a 400 error and it never resolves itself without a service restart.

For me, "restarting the DDNS-Updater service after the domain has successfully propogated", makes no difference. But the main issue is certainly true:

Porkbun will return a 400 error if you attempt to update the IP for a DNS record that is already set to the same value.

That's what I see too.

[qdm12]

We may end up merging this, since there is also another person with the same problem on Porkbun. But I would ideally like to understand the root cause of this since it may apply to other registrars. I'm also wondering if this is specific to porkbun being slow at propagating updates 🤔 since I have not seen similar issues for other registrars.

[bentemple]

I wonder if it actually makes sense to update the logic to:

if (noRecords)
  if ( top level domain ) 
      look for domain.tld ALIAS
      if ALIAS == pixie.porkbun.com
        delete
      else 
        logical error denoting the conflicting entry and user action needed
  if ( *.domain.tld ) 
    look for *.domain.tld CNAME
    if CNAME == pixie.porkbun.com
      delete
    else
      logical error denoting the conflicting CNAME entry and user action needed

Ultimately, we want to create the A / AAAA entries if none exist, but we really only want to be messing with CNAME and ALIAS conflicts if they're expected values. Just need to queue the user in better to the reason for the failure, and sadly the porkbun API generates worthless error messages. "400, something went wrong"

[qdm12]

I wonder if it actually makes sense to update the logic to

TLDR: let's do your version!

Longer story:
I've commented my own version in #776 (comment) which is quite similar. I just don't delete unless there is a 400 code, so the API calls count is 2 (get records+create) to 4 (get records+create+deletion+create).
In your case, API calls count is 3 (get records+get alias|cname+create) to 4 (get records+get alias|cname+delete+create) but it adds the safety to check the alias|cname are the default porkbun ones. Plus creation of records are rare so API calls count doesnt' matter much really.

[bentemple]

I wonder if it actually makes sense to update the logic to

TLDR: let's do your version!

Longer story: I've commented my own version in #776 (comment) which is quite similar. I just don't delete unless there is a 400 code, so the API calls count is 2 (get records+create) to 4 (get records+create+deletion+create). In your case, API calls count is 3 (get records+get alias|cname+create) to 4 (get records+get alias|cname+delete+create) but it adds the safety to check the alias|cname are the default porkbun ones. Plus creation of records are rare so API calls count doesnt' matter much really.

sounds good. I'll update it accordingly. Sorry went to Defcon this last week so just busy life.

[qdm12]

sounds good. I'll update it accordingly. Sorry went to Defcon this last week so just busy life.

No pressure at all, the existing version already works more or less fine 😉

Ps: I saved that picture 😄

[qdm12]

Thanks!

A few comments, many nits (apologies) and a few important changes to make (without the nit prefix 😄). Feel free to skip nits if you want.

[bentemple]

I took a pass at deleting the @ ALIAS opportunistically when setting the * domain. From what I could tell, this required updating the API to pass in the owner, but maybe there's a better way I just wasn't seeing.

I kept that separate as it's own commit, so it's both easy to see the change, and we can trivially revert it if it's not the direction you were thinking.

Side-note: I really appreciate all the nits. Code homogeneity is really important, and nits are a really great way to level up code quality (imo) so thank you for taking the time!

[qdm12]

Thanks for the re-work! And sorry for the 3 weeks delay, but I'm more or less back on the keyboard now. 😉

[qdm12]

I allowed myself to push that generic http post function refactor commit to the master branch at 8c1b3e5 in order to only have relevant deltas in this PR, and then rebased your branch on the master branch resolving conflicts. I also pushed 2 small-ish commits on top of your commits.

Please review the code and perhaps test the program to double check it works fine (even basic update should do the test trick), and then I'm happy to merge it 😉

[bentemple]

I allowed myself to push that generic http post function refactor commit to the master branch at 8c1b3e5 in order to only have relevant deltas in this PR, and then rebased your branch on the master branch resolving conflicts. I also pushed 2 small-ish commits on top of your commits.

Please review the code and perhaps test the program to double check it works fine (even basic update should do the test trick), and then I'm happy to merge it 😉

no problem. I'll re-run my testplan tonight and push it up

[qdm12]

All good then? Thanks! 💯

[bentemple]

So sorry for the long delay. life has been crazy.

[qdm12]

Hey no problem 🎉 !

One tiny nitpicky thing for next PR: merge with a 'conventional commit message', for example it would had been fix(porkbun): delete default parked DNS entries if needed (since it's not only for the wildcard as well), and resume final changes as bullet points instead of using the PR description. For example:

• delete default @ ALIAS entry when creating a record for the root domain or the wildcard domain
• delete default * CNAME entry when creating a record for the wildcard domain

It's fine though, I've already copied/pasted this for the next draft of the next release 😉

[bentemple]

Hey no problem 🎉 !

One tiny nitpicky thing for next PR: merge with a 'conventional commit message', for example it would had been fix(porkbun): delete default parked DNS entries if needed (since it's not only for the wildcard as well), and resume final changes as bullet points instead of using the PR description. For example:

* delete default `@` ALIAS entry when creating a record for the root domain or the wildcard domain

* delete default `*` CNAME entry when creating a record for the wildcard domain

It's fine though, I've already copied/pasted this for the next draft of the next release 😉

Yeah... :( I looked at all of the other commit messages after merging and realized that I hadn't followed the convention. I'll def follow the convention in the future. Thanks for pointing out :)