MX priority not read/set correctly

[hmoffatt]

Confirmation

• My issue isn't already found on the issue tracker.
• I have replicated my issue using the latest version of the library and it is still present.

cloudflare-go version

0.67 (same on HEAD at the time of writing)

Go environment

GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/hamish/.cache/go-build"
GOENV="/home/hamish/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/hamish/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/hamish/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go-1.19"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go-1.19/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.19.8"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/hamish/src/dnscontrol/cloudflare-go/go.mod"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build3473117222=/tmp/go-build -gno-record-gcc-switches"

Expected output

MX record priority matches the web dashboard when running flarectl dns list.

I'm also unable to modify the priority of existing records from go - the value does not change (set via library, checked on dashboard).

Actual output

MX priority is garbage, and changes each time flarectl is run. I think it's printing the pointer value.

$ ./flarectl dns l --zone the.test.zone
                 ID                | TYPE |         NAME         |       CONTENT        | PROXIED | TTL  
-----------------------------------+------+----------------------+----------------------+---------+------
  09e960014d7e2f94b6c2f09de70a2ab5 | MX   | testmx.the.test.zone | 824636592504 bar.com | false   |   1  

$ ./flarectl dns l --zone the.test.zone
                 ID                | TYPE |         NAME         |       CONTENT        | PROXIED | TTL  
-----------------------------------+------+----------------------+----------------------+---------+------
  09e960014d7e2f94b6c2f09de70a2ab5 | MX   | testmx.the.test.zone | 824636646456 bar.com | false   |   1  

$ ./flarectl dns l --zone the.test.zone
                 ID                | TYPE |         NAME         |       CONTENT        | PROXIED | TTL  
-----------------------------------+------+----------------------+----------------------+---------+------
  09e960014d7e2f94b6c2f09de70a2ab5 | MX   | testmx.the.test.zone | 824636610968 bar.com | false   |   1  

Code demonstrating the issue

flarectl dns l --zone the.test.zone

Steps to reproduce

flarectl dns l --zone the.test.zone

References

No response

[hmoffatt]

The list issue seems to be present even back in v0.58.1.

The modify issue appears in v0.59.0, but is not present in v0.58.1. I can reproduce this using flarectl and checking in the web dashboard.

[jacobbednarz]

thanks for the raising the issue here. flarectl is in maintenance only mode at the moment so we're only doing critical fixes to it. are you only able to reproduce this issue with flarectl? if so, i suspect the remainder of the library is OK if you're using it directly?

[hmoffatt]

Getting the priority seems to be a flarectl problem.
I saw the priority setting problem when using the library (from https://github.com/StackExchange/dnscontrol) though, I was only using flarectl to test.

[jacobbednarz]

gotcha. flarectl is in maintenance only mode at the moment so we're not actively improving things too mcuh however, since this was a side effect of a recent change to support a change in Terraform and broke it pretty dramatically, i've pushed up a quick fix for it.

from what i can see, this is only a flarectl issue and the issue isn't present anywhere else in the library. for future checking, i'd be wary of the flarectl output until it is revamped with the SDK to be automatically generated and not have issues like this one.

[hmoffatt]

This doesn't seem to address the problem with modifying the MX priority though. This isn't working for me from either flarectl or directly with the library (from dnscontrol).

[jacobbednarz]

is your setting issue similar to #1015? that issue has been open for a while but lost the tration due to lack of op feedback and no repro. if so, can you drop your repro into that issue and we can continue the discussion there keeping these two issues separate?

[hmoffatt]

No, I'm just trying to modify an existing record. The MX priority does not change. This started in v0.59.

[jacobbednarz]

do you have a repro i can look at for the library? i have some scripts internally and terraform configs that were definitely working recently so will need to dig into your specifics to see where it is falling down for you.

i suspect something is silently failing validation in the API itself.

[hmoffatt]

I see it in the dnscontrol unit tests, but I don't have a standalone test case. I think I also saw it in flarectl - let me see if I can reproduce it with that. Is there a way to log all the HTTP requests made to the API?

[jacobbednarz]

if you export HTTPS_PROXY with a MITM proxy, you will get the HTTP interactions. if you already have the library, you can pass in cloudflare.Debug(true) into your client instantiation and it will spew out the HTTP logs to stdout/stderr.

[hmoffatt]

Here's what I see using flarectl:

$ ./flarectl dns l --zone the.test.zone
                 ID                | TYPE |        NAME        |    CONTENT    | PROXIED | TTL
-----------------------------------+------+--------------------+---------------+---------+------
  21307984418275ee4c0cdfa697e74caf | MX   | test.the.test.zone | 10 google.com | false   |   1

$ ./flarectl dns u --zone the.test.zone --priority 100 --name test --id 21307984418275ee4c0cdfa697e74caf

$ ./flarectl dns l --zone the.test.zone
                 ID                | TYPE |        NAME        |    CONTENT    | PROXIED | TTL
-----------------------------------+------+--------------------+---------------+---------+------
  21307984418275ee4c0cdfa697e74caf | MX   | test.the.test.zone | 10 google.com | false   |   1

The web dashboard also shows that the priority doesn't change.

[hmoffatt]

and here's what I see in the log from flarectl with Debug enabled; the new priority isn't in the PATCH:

$ ./flarectl dns u --zone the.test.zone --priority 100 --name test --id 21307984418275ee4c0cdfa697e74caf

2023/05/22 23:38:15
PATCH /client/v4/zones/d7c71d641dea7ce273b7a5ec80d1109a/dns_records/21307984418275ee4c0cdfa697e74caf HTTP/1.1
Host: api.cloudflare.com
User-Agent: cloudflare-go/v4
Content-Length: 64
Authorization: Bearer [redacted]
Content-Type: application/json
Accept-Encoding: gzip

{"name":"test","ttl":1,"proxied":false,"comment":"","tags":null}

adding --type MX changes the PATCH but still the priority is not sent.

I see the same in the dnscontrol test that fails - the PATCH to change the MX priority does not include the new priority value. This works in v0.58, updating the library to v0.59 (and not changing anything else) breaks it.

[jacobbednarz]

thanks for the flarectl example. as i mentioned, flarectl is in maintenace only mode so that is a less urgent issue for me right now however, if the library is having issues, that is something i'll need to jump onto. are you able to provide me a reproduction of just using the library where you are seeing issues?

[hmoffatt]

Just creating an MX record then immediately modifying the priority fails; the debug shows the PATCH does not include the priority.

package main

import (
	"context"
	"fmt"
	"log"
	"os"

	"github.com/cloudflare/cloudflare-go"
)

func main() {
	api, err := cloudflare.NewWithAPIToken(os.Getenv("CLOUDFLARE_API_TOKEN"))
	if err != nil {
		log.Fatal(err)
	}
	api.Debug = true

	// Most API calls require a Context
	ctx := context.Background()

	// Create record
	var prio uint16 = 10;
	rec, err := api.CreateDNSRecord(ctx, cloudflare.ZoneIdentifier("d7c71d641dea7ce273b7a5ec80d1109a"), cloudflare.CreateDNSRecordParams{
		Type: "MX",
		Name: "test",
		Priority: &prio,
		Content: "google.com",
	});
	if err != nil {
		log.Fatal(err)
	}
	//fmt.Println(rec)

	// Modify record
	prio = 100;
	rec, err = api.UpdateDNSRecord(ctx, cloudflare.ZoneIdentifier("d7c71d641dea7ce273b7a5ec80d1109a"), cloudflare.UpdateDNSRecordParams{
		ID: rec.ID,
		Type: "MX",
		Name: "test",
		Priority: &prio,
	});
	if err != nil {
		log.Fatal(err)
	}
	fmt.Println(rec)

}

[hmoffatt]

The problem is, I think, that the metadata on the UpdateDNSRecordParams struct says not to send the priority value. If I change it as follows, then I can modify the priority.

diff --git a/dns.go b/dns.go
index 7e2ddb7..78627b0 100644
--- a/dns.go
+++ b/dns.go
@@ -74,7 +74,7 @@ type UpdateDNSRecordParams struct {
        Content  string      `json:"content,omitempty"`
        Data     interface{} `json:"data,omitempty"` // data for: SRV, LOC
        ID       string      `json:"-"`
-       Priority *uint16     `json:"-"` // internal use only
+       Priority *uint16     `json:"priority,omitempty` // internal use only
        TTL      int         `json:"ttl,omitempty"`
        Proxied  *bool       `json:"proxied,omitempty"`
        Comment  string      `json:"comment"`

[jacobbednarz]

looks like #1170 removed this after it was determined that it was a read only field for updates. with your example where you patch it locally to send the value, does the API accept and retain that value?

[hmoffatt]

Yes it does, and the documentation says it is valid also: https://developers.cloudflare.com/api/operations/dns-records-for-a-zone-patch-dns-record

#1160 also added the "internal use only" comment which appears to be incorrect.

[jacobbednarz]

happy if you want to send over a PR for what you have locally and we'll get it reviewed and merged. otherwise, I can take a look later this week.

thanks again!

[github-actions]

This functionality has been released in v0.68.0.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!