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

DNS Records: Unused/irrelevant data fields are sent to the API #3348

Open
3 tasks done
janik-cloudflare opened this issue Jun 7, 2024 · 13 comments
Open
3 tasks done
Labels
kind/bug Categorizes issue or PR as related to a bug. triage/needs-information Indicates an issue needs more information in order to work on it.

Comments

@janik-cloudflare
Copy link
Member

Confirmation

  • This is a bug with an existing resource and is not a feature request or enhancement. Feature requests should be submitted with Cloudflare Support or your account team.
  • I have searched the issue tracker and my issue isn't already found.
  • I have replicated my issue using the latest version of the provider and it is still present.

Terraform and Cloudflare provider version

Terraform v1.8.5
on linux_amd64

  • provider registry.terraform.io/cloudflare/cloudflare v4.34.0

Affected resource(s)

  • cloudflare_record

Terraform configuration files

resource "cloudflare_record" "example" {
  zone_id = var.zone_id
  name    = "example"
  type    = "SVCB"

  data {
    priority = 10
    target   = "example.com"
    value    = "alpn=h2"
  }
}

Link to debug output

https://gist.github.com/janik-cloudflare/310eb92bb92fbd47db79fde4168a199d

Panic output

No response

Expected output

Terraform sends only the data fields that are actually present in the resource

Actual output

Terraform sends all data fields, including any that are not present in the resource and even data fields for completely unrelated record types, such as lat_degrees (which only makes sense for LOC records).

Steps to reproduce

  1. Add resource for URI record as in example above
  2. Inspect request payload in output of TF_LOG=TRACE terraform apply --auto-approve

Additional factoids

Sending unrelated fields or fields not present in the resource is unexpected and bad practice.

This has caused problems with a rollout related to SRV record data fields: https://community.cloudflare.com/t/eol-of-name-related-data-fields-on-srv-dns-records-is-not-applying/668360

References

No response

@janik-cloudflare janik-cloudflare added kind/bug Categorizes issue or PR as related to a bug. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jun 7, 2024
Copy link
Contributor

github-actions bot commented Jun 7, 2024

Community Note

Voting for Prioritization

  • Please vote on this issue by adding a 👍 reaction to the original post to help the community and maintainers prioritize this request.
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request.

Volunteering to Work on This Issue

  • If you are interested in working on this issue, please leave a comment.
  • If this would be your first contribution, please review the contribution guide.

Copy link
Contributor

github-actions bot commented Jun 7, 2024

Thank you for reporting this issue! For maintainers to dig into issues it is required that all issues include the entirety of TF_LOG=DEBUG output to be provided. The only parts that should be redacted are your user credentials in the X-Auth-Key, X-Auth-Email and Authorization HTTP headers. Details such as zone or account identifiers are not considered sensitive but can be redacted if you are very cautious. This log file provides additional context from Terraform, the provider and the Cloudflare API that helps in debugging issues. Without it, maintainers are very limited in what they can do and may hamper diagnosis efforts.

This issue has been marked with triage/needs-information and is unlikely to receive maintainer attention until the log file is provided making this a complete bug report.

@github-actions github-actions bot added triage/needs-information Indicates an issue needs more information in order to work on it. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jun 7, 2024
@janik-cloudflare
Copy link
Member Author

I didn't have time to review the entire log file for anything that shouldn't be published, but I believe the lines I included are sufficient :)

@sureshgurugubilli4
Copy link

Cloudflare API deprecates the service, proto and name fields within the data map in favor of the name field outside the data map, which is the same name field that’s used by all other record types. However, the terraform provider still maintains these fields in the data map.

https://developers.cloudflare.com/fundamentals/api/reference/deprecations/#name-related-data-fields-on-srv-dns-records

@frankpengau
Copy link

Hi All,

Is there any chance we can get this issue revisited, as it seems to have officially kicked in and been removed from the Cloudflare API from today onwards?

I believe that @janik-cloudflare has raised a draft PR for the potential schema changes. Any chance we can get this prioritised?

Related to:

Ref:

@janik-cloudflare
Copy link
Member Author

Hi @frankpengau!

Could you help me understand the impact you're seeing as a result of this rollout? In theory it should be possible to simply omit the deprecated data fields without any issues!

@frankpengau
Copy link

Hi @janik-cloudflare ,

We've omitted the deprecated data fields from the SRV records and applied it to terraform state, but every time that we run our CI/CD, it is still picking up changes/drift to remove the data fields from the terraform state.

It looks like we might need to perform manual terraform state manipulation, in order to actually remove them, as it still expects them in the schema?

Otherwise, we were thinking we might need to look at moving to not using the data map entirely and just use value field/attribute to apply the port, priority, target and weight fields from the data map.

Any suggestions?

@janik-cloudflare
Copy link
Member Author

janik-cloudflare commented Jul 17, 2024

That's interesting, thanks @frankpengau. I have to admit I'm not too familiar with our Terraform provider but based on your message it sounds like the previous service/proto/name values are still cached in the tfstate file, and, because the API doesn't send them anymore, Terraform just assumes they still exist?

I'm using a cloudflare_record like this one and that works fine for me ("No changes. Your infrastructure matches the configuration."), even if I modify the tfstate file to add values to the old data fields (it just silently removes them again on the next apply).

Could you post an example of what your cloudflare_record looks like?

Perhaps setting the 3 deprecated fields in the data map to empty strings would work to clear out any caches that are still around?

@frankpengau
Copy link

frankpengau commented Jul 18, 2024

We have tried to plainly remove the name/proto/service fields like so:

resource "cloudflare_record" "example_com_sips" {
  name    = "_sips._tcp"
  proxied = false
  ttl     = 86400
  type    = "SRV"
  zone_id = "12345678123456781234567812345678"
  data {
    port     = 5060
    priority = 20
    target   = "target.another.com"
    weight   = 5
  }
}

But that didn't work, so we tried just empty values for name/proto/service fields:

resource "cloudflare_record" "example_com_sips" {
  name    = "_sips._tcp"
  proxied = false
  ttl     = 86400
  type    = "SRV"
  zone_id = "12345678123456781234567812345678"
  data {
    port     = 5060
    priority = 20
    target   = "target.another.com"
    weight   = 5
    name     = ""
    proto    = ""
    service  = ""
  }
}

However that didn't work either.

Is there a way to find out which zones it has rolled out to, in the cloudflare console/dashboard?

@janik-cloudflare
Copy link
Member Author

janik-cloudflare commented Jul 18, 2024

Hmm, that works fine for me. On the second run:

No changes. Your infrastructure matches the configuration.

Are you using the latest provider version?

% terraform --version | grep /cloudflare
+ provider registry.terraform.io/cloudflare/cloudflare v4.37.0

Could you share Terraform's output/diffs? Or, even better, TF_LOG=TRACE terraform apply --auto-approve &> tf.log?

Is there a way to find out which zones it has rolled out to

I can check if you have the zone ID, but at this point it's been rolled out to pretty much all zones. If you saw a change 2 days ago it must be because of that.

@arcnmx
Copy link

arcnmx commented Jul 18, 2024

Is the deprecation a slow rollout? I was very confused why the suggested changes worked in some places but resulted in an infinite apply loop in others. ignore_changes seems to be a valid workaround:

lifecycle {
  ignore_changes = [data[0].name, data[0].proto, data[0].service]
}

@frankpengau
Copy link

@janik-cloudflare,

I won't be able to share publicly on GitHub, but happy to share more details in our Cloudflare Enterprise Support Case, if that's okay?

Cloudflare Support Case: 3342269

@janik-cloudflare
Copy link
Member Author

@arcnmx it is indeed, but it's essentially done now (finishing 08:00 UTC on Monday). Sorry for the trouble; we had missed the impact on Terraform while planning the change. In the future we'd probably choose a different rollout plan and/or roll out similar changes significantly faster. Thanks for sharing that workaround!

@frankpengau that'd be great, thank you! In particular the TF_LOG=TRACE output should be very helpful, I think. (P.S.: I don't think #3286 would solve the behavior you're seeing, unfortunately.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. triage/needs-information Indicates an issue needs more information in order to work on it.
Projects
None yet
Development

No branches or pull requests

4 participants