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

bentemple · 2024-07-27T05:00:14Z

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 · 2024-07-27T05:18:28Z

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 · 2024-08-02T08:55:05Z

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 · 2024-08-07T16:16:52Z

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 · 2024-08-07T17:10:57Z

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 · 2024-08-07T18:43:35Z

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 · 2024-08-07T22:21:51Z

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 · 2024-08-08T08:12:08Z

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 · 2024-08-15T00:49:37Z

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 · 2024-08-15T08:17:05Z

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 😄

internal/provider/providers/porkbun/provider.go

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.

docs/porkbun.md

internal/provider/constants/ip.go

internal/provider/providers/porkbun/provider.go

internal/provider/providers/porkbun/api.go

internal/provider/providers/porkbun/provider.go

internal/provider/providers/porkbun/api.go

internal/provider/providers/porkbun/provider.go

bentemple · 2024-08-28T05:56:32Z

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!

bentemple · 2024-08-28T06:06:49Z

internal/provider/providers/porkbun/api.go

 	}
 	defer response.Body.Close()

 	if response.StatusCode != http.StatusOK {
-		return nil, fmt.Errorf("%w: %d: %s", errors.ErrHTTPStatusNotValid,
+		return nil, fmt.Errorf("%s - %w: %d: %s", recordType, errors.ErrHTTPStatusNotValid,


Wasn't entirely sure the best way to format these error messages to include the recordType. I welcome any input (always)

I thought about just leaving them alone, but given how abysmal the error messages are that Porkbun returns, thought it would be better to keep it everywhere possible to provide the most context.

Agreed! And, although repetitive, it feels logical to wrap the recordType (and owner) within the function instead of by the caller I would say with the code as is.

However, I managed to refactor the code to use a generic function

func httpPost[T any](ctx context.Context, client *http.Client, url string, requestData any, decodeBody bool) (responseData T, err error)

to remove a lot of the repetition (in terms of raw http client code and error wrappings), and now the error wrapping gets done at the caller level, for example:

responseData, err := httpPost[jsonResponseData](ctx, client, url, postRecordsParams, decodeBody) if err != nil { return nil, fmt.Errorf("for record type %s and record owner %s: %w", recordType, owner, err) }

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 · 2024-09-16T06:07:12Z

internal/provider/providers/porkbun/api.go

 	}
 	defer response.Body.Close()

 	if response.StatusCode != http.StatusOK {
-		return nil, fmt.Errorf("%w: %d: %s", errors.ErrHTTPStatusNotValid,
+		return nil, fmt.Errorf("%s - %w: %d: %s", recordType, errors.ErrHTTPStatusNotValid,


Agreed! And, although repetitive, it feels logical to wrap the recordType (and owner) within the function instead of by the caller I would say with the code as is.

However, I managed to refactor the code to use a generic function

func httpPost[T any](ctx context.Context, client *http.Client, url string, requestData any, decodeBody bool) (responseData T, err error)

to remove a lot of the repetition (in terms of raw http client code and error wrappings), and now the error wrapping gets done at the caller level, for example:

responseData, err := httpPost[jsonResponseData](ctx, client, url, postRecordsParams, decodeBody) if err != nil { return nil, fmt.Errorf("for record type %s and record owner %s: %w", recordType, owner, err) }

qdm12 · 2024-09-16T07:34:04Z

internal/provider/providers/porkbun/provider.go

-		err = p.createRecord(ctx, client, recordType, ipStr)
-		if err != nil {
-			return netip.Addr{}, fmt.Errorf("creating record: %w", err)
+	if len(records) > 0 {


Reverted to len(recordIDs) == 0 given the new function deleteDefaultConflictingRecordsIfNeeded not taking as many lines in the if block 😉 (to have less deltas)

qdm12 · 2024-09-16T07:36:07Z

internal/provider/providers/porkbun/provider.go

+	// For new domains, Porkbun Creates 2 Default DNS Records which point to their "parked" domain page:
+	// ALIAS domain.tld -> pixie.porkbun.com
+	// CNAME *.domain.tld -> pixie.porkbun.com
+	// ALIAS and CNAME records conflict with A and AAAA records, and attempting to create an A or AAAA record
+	// will return a 400 error if they aren't first deleted.


Resumed the comment with

// deleteDefaultConflictingRecordsIfNeeded deletes any default records that would conflict with a new record,
// see https://github.com/qdm12/ddns-updater/blob/master/docs/porkbun.md#record-creation

qdm12 · 2024-09-16T07:36:35Z

internal/provider/providers/porkbun/provider.go

+	// CNAME *.domain.tld -> pixie.porkbun.com
+	// ALIAS and CNAME records conflict with A and AAAA records, and attempting to create an A or AAAA record
+	// will return a 400 error if they aren't first deleted.
+	porkbunParkedDomain := "pixie.porkbun.com"


changed this to

const porkbunParkedDomain = "pixie.porkbun.com"

because constants are always nicer if possible 😉

qdm12 · 2024-09-16T07:37:08Z

internal/provider/providers/porkbun/provider.go

+	// ALIAS and CNAME records conflict with A and AAAA records, and attempting to create an A or AAAA record
+	// will return a 400 error if they aren't first deleted.
+	porkbunParkedDomain := "pixie.porkbun.com"
+	switch {


changed to switch p.owner { directly

qdm12 · 2024-09-16T07:39:01Z

internal/provider/providers/porkbun/provider.go

+		// Delete ALIAS domain.tld -> pixie.porkbun.com record
+		err = p.deleteSingleMatchingRecord(ctx, client, constants.ALIAS, "@", porkbunParkedDomain)
+		if err != nil {
+			return netip.Addr{}, fmt.Errorf("deleting default parked domain record: %w", err)


changed error wrapping to

fmt.Errorf("deleting default ALIAS @ parked domain record: %w", err)

qdm12 · 2024-09-16T07:42:04Z

internal/provider/providers/porkbun/provider.go

+		// Error is ignored as the ALIAS could be set to something besides the parked domain. Failure here is non-fatal.
+		_ = p.deleteSingleMatchingRecord(ctx, client, constants.ALIAS, "@", porkbunParkedDomain)


Eh it should be a fatal error if we get an API error for example, so I changed it to

err = p.deleteSingleMatchingRecord(ctx, client, constants.ALIAS, "@", porkbunParkedDomain) if err != nil && !stderrors.Is(err, errors.ErrConflictingRecord) { // allow conflict ALIAS records to be set to something besides the parked domain return fmt.Errorf("deleting default ALIAS @ parked domain record: %w", err) }

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 😉

Description: By default, 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. 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 values. 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. ![screenshot_2024-08-17-0210 20](https://github.com/user-attachments/assets/eb567401-ad4b-454d-a7aa-70ab1db1e3e9)

Imorting suggested changes. More fixes to come to address remaining comments. Co-authored-by: Quentin McGaw <[email protected]>

Fix lint, again. :rip: Remove href to https://pixie.porkbun.com/ since that url doesn't actually load and caused the tests to fail

- add `deleteDefaultConflictingRecordsIfNeeded` method - handle non conflicting errors from `deleteSingleMatchingRecord` - simplify comments by linking to documentation - improve error wrappings

bentemple force-pushed the ben.temple/porkbun-skip-duplicate-updates branch from a76b6a3 to e30c143 Compare July 27, 2024 05:03

bentemple mentioned this pull request Jul 27, 2024

Provider Porkbun: Fix wildcard domain modifications #773

Merged

bentemple force-pushed the ben.temple/porkbun-skip-duplicate-updates branch from e30c143 to f678492 Compare July 29, 2024 16:14

qdm12 added the Priority: 🚨 Urgent label Aug 7, 2024

This comment was marked as resolved.

Sign in to view

This comment was marked as off-topic.

Sign in to view

bentemple marked this pull request as draft August 16, 2024 06:30

bentemple force-pushed the ben.temple/porkbun-skip-duplicate-updates branch from 8e8c50e to bf424f7 Compare August 17, 2024 07:57

bentemple changed the title ~~Provider Porkbun: Don't attempt to update a record if it already matches the expected ipStr~~ Provider Porkbun: Delete Default Porkbun Parked DNS Entry for *.domain.tld Aug 17, 2024

bentemple changed the title ~~Provider Porkbun: Delete Default Porkbun Parked DNS Entry for *.domain.tld~~ Provider Porkbun: Delete Default Parked DNS Entry for *.domain.tld Aug 17, 2024

bentemple force-pushed the ben.temple/porkbun-skip-duplicate-updates branch 3 times, most recently from 5a4fbdf to 81875b6 Compare August 17, 2024 09:19

bentemple commented Aug 17, 2024

View reviewed changes

internal/provider/providers/porkbun/provider.go Outdated Show resolved Hide resolved

bentemple force-pushed the ben.temple/porkbun-skip-duplicate-updates branch 2 times, most recently from 59ff9c0 to 8a11f19 Compare August 17, 2024 09:30

bentemple marked this pull request as ready for review August 17, 2024 09:40

bentemple requested a review from qdm12 August 17, 2024 09:40

qdm12 requested changes Aug 17, 2024

View reviewed changes

bentemple force-pushed the ben.temple/porkbun-skip-duplicate-updates branch 2 times, most recently from b91e9c1 to afeb6bb Compare August 28, 2024 06:04

bentemple commented Aug 28, 2024

View reviewed changes

bentemple force-pushed the ben.temple/porkbun-skip-duplicate-updates branch 2 times, most recently from e617e37 to 8e9e23e Compare August 28, 2024 06:22

bentemple requested a review from qdm12 August 28, 2024 06:26

qdm12 reviewed Sep 16, 2024

View reviewed changes

qdm12 force-pushed the ben.temple/porkbun-skip-duplicate-updates branch 2 times, most recently from 55223ba to e94838b Compare September 16, 2024 13:29

qdm12 approved these changes Sep 16, 2024

View reviewed changes

bentemple and others added 8 commits September 17, 2024 07:39

Apply suggestions from code review

112ae1e

Imorting suggested changes. More fixes to come to address remaining comments. Co-authored-by: Quentin McGaw <[email protected]>

Modifications to match PR Comments

bf4a3a9

Fix lint

a7efa83

Update API to accept owner parameter instead of using p.owner by default

fb420ec

Fix missed comment

0863c0e

Fix lint, again. :rip: Remove href to https://pixie.porkbun.com/ since that url doesn't actually load and caused the tests to fail

chore: add owner to API error contexts

a739c33

chore: refactor Update function

2827fd4

- add `deleteDefaultConflictingRecordsIfNeeded` method - handle non conflicting errors from `deleteSingleMatchingRecord` - simplify comments by linking to documentation - improve error wrappings

qdm12 force-pushed the ben.temple/porkbun-skip-duplicate-updates branch from e94838b to 2827fd4 Compare September 17, 2024 07:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bentemple commented Jul 27, 2024 •

edited

Loading

bentemple commented Jul 27, 2024 •

edited

Loading

Bananas-Are-Yellow commented Aug 2, 2024

qdm12 commented Aug 7, 2024

Bananas-Are-Yellow commented Aug 7, 2024

qdm12 commented Aug 7, 2024

This comment was marked as resolved.

This comment was marked as off-topic.

bentemple commented Aug 7, 2024 •

edited

Loading

qdm12 commented Aug 8, 2024

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

bentemple commented Aug 15, 2024

This comment was marked as off-topic.

qdm12 commented Aug 15, 2024

qdm12 left a comment

bentemple commented Aug 28, 2024 •

edited

Loading

bentemple Aug 28, 2024 •

edited

Loading

qdm12 Sep 16, 2024

qdm12 left a comment

qdm12 Sep 16, 2024

qdm12 Sep 16, 2024

qdm12 Sep 16, 2024

qdm12 Sep 16, 2024

qdm12 Sep 16, 2024

qdm12 Sep 16, 2024

qdm12 Sep 16, 2024

qdm12 left a comment

		// Error is ignored as the ALIAS could be set to something besides the parked domain. Failure here is non-fatal.
		_ = p.deleteSingleMatchingRecord(ctx, client, constants.ALIAS, "@", porkbunParkedDomain)

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

Are you sure you want to change the base?

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

Conversation

bentemple commented Jul 27, 2024 • edited Loading

Description:

Test-Plan:

bentemple commented Jul 27, 2024 • edited Loading

Bananas-Are-Yellow commented Aug 2, 2024

qdm12 commented Aug 7, 2024

Bananas-Are-Yellow commented Aug 7, 2024

qdm12 commented Aug 7, 2024

This comment was marked as resolved.

This comment was marked as off-topic.

bentemple commented Aug 7, 2024 • edited Loading

qdm12 commented Aug 8, 2024

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

bentemple commented Aug 15, 2024

This comment was marked as off-topic.

qdm12 commented Aug 15, 2024

qdm12 left a comment

Choose a reason for hiding this comment

bentemple commented Aug 28, 2024 • edited Loading

bentemple Aug 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qdm12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qdm12 left a comment

Choose a reason for hiding this comment

bentemple commented Jul 27, 2024 •

edited

Loading

bentemple commented Jul 27, 2024 •

edited

Loading

bentemple commented Aug 7, 2024 •

edited

Loading

bentemple commented Aug 28, 2024 •

edited

Loading

bentemple Aug 28, 2024 •

edited

Loading