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

resource_cloudflare_record: Add DiffSuppressFunc to value param #1713

Merged
merged 1 commit into from Jun 27, 2022

Conversation

stack72
Copy link
Contributor

@stack72 stack72 commented Jun 20, 2022

Fixes: #154

When a trailing period is passed to the cloudflare_record resource,
we get a perpetual diff that looks as follows:

~ value: "_dbdxxx05384ca.hnyhpvdqhv.acm-validations.aws" => "_dbdxxxf205384ca.hnyhpvdqhv.acm-validations.aws."

We should ensure that we are adhering to the following RFCs where it
states that an FQDN can have a trailing .

Therefore, we can now use a DiffSuppressFunc to check that the old
value with the final . removed and the new value with the .
removed are the same and if they are then we don't need to cause a
diff

@github-actions
Copy link
Contributor

github-actions bot commented Jun 20, 2022

changelog detected ✅

@stack72 stack72 force-pushed the fix-record-diffSuppression branch from 091b0c9 to 426735e Compare June 20, 2022 15:43
Fixes: cloudflare#154

When a trailing period is passed to the cloudflare_record resource,
we get a perpetual diff that looks as follows:

```
~ value: "_dbdxxx05384ca.hnyhpvdqhv.acm-validations.aws" => "_dbdxxxf205384ca.hnyhpvdqhv.acm-validations.aws."
```

We should ensure that we are adhering to the following RFCs where it
states that an FQDN can have a trailing `.`

Therefore, we can now use a DiffSuppressFunc to check that the old
value with the final `.` removed and the new value with the `.`
removed are the same and if they are then we don't need to cause a
diff
@stack72 stack72 force-pushed the fix-record-diffSuppression branch from 426735e to f739b46 Compare June 20, 2022 16:20
@jacobbednarz
Copy link
Member

thanks for this @stack72, appreciate it! as I mentioned in #154 (and a couple of follow up issues), I'm still not sure if we want to be covering this up as they are potentially different things depending on that character. as this tool is used by people migrating from services that rely on this to Cloudflare that makes it optional, I'm worried we may be covering up a config issue in doing this. let me check with the DNS folks whether we have better safeguards in place now to support this.

you mentioned adhering to RFCs; could you provide which part we are missing as to the best of my knowledge, the mention of trailing dots is optional based on the implementation.

@stack72
Copy link
Contributor Author

stack72 commented Jun 20, 2022

Hi @jacobbednarz

Thanks for the response, the following RFC - section 3.1 - states this:

When a user needs to type a domain name, the length of each label is
omitted and the labels are separated by dots (".").  Since a complete
domain name ends with the root label, this leads to a printed form which
ends in a dot.  We use this property to distinguish between:

   - a character string which represents a complete domain name
     (often called "absolute").  For example, "poneria.ISI.EDU."

   - a character string that represents the starting labels of a
     domain name which is incomplete, and should be completed by
     local software using knowledge of the local domain (often
     called "relative").  For example, "poneria" used in the
     ISI.EDU domain.

Right now the provider only handles a user based domain name and not the printed form (including the trailing dot)

@jacobbednarz
Copy link
Member

thanks 🙏 the API definitely allows both however, (this is where my memory gets fuzzy) the response can be normalised with the trailing period as not everyone had the entitlements to use it in that. iirc, it was being phased out but I will need to confirm.

@stack72
Copy link
Contributor Author

stack72 commented Jun 20, 2022

I understand - thanks for confirmation on this. Just to add that AWS ACM does standardise on the printed form now so it would be really good to be able to support this easily and without having to make users cut their strings :)

I'm happy to pull this PR into a Pulumi fork of the provider if you'd rather I did that so that we can take advantage in pulumi-cloudflare

Paul

@jacobbednarz
Copy link
Member

the other thing we have to consider here as well is that given a zone of example.com, a value of foo results in the API response being foo.example.com. in it's current state, i think that would result in a permadiff but something to keep in mind.

@jacobbednarz
Copy link
Member

i've been chatting internally with the DNS team and i now have a list of the available inputs, their expected outputs and some normalisation that we modified recently.

within this API, we do take into account the trailing comma to denote whether the value is canonical or not however, that is no longer considered in the response itself since the input value is restricted and must now contain the zone name itself for that to take effect.

input zone name output
foo example.com foo.example.com
foo. example.com foo.example.com
foo.bar example.com foo.bar.example.com
@ example.com example.com
/"" example.com example.com
example.com. example.com example.com
example.com example.com example.com

the other scenarios (such as . and foo.) won't be an issue here without resulting in a permadiff since the API expands those completely.

i think we're safe with the normalisation as is now to merge this.

@jacobbednarz jacobbednarz merged commit bbc5e99 into cloudflare:master Jun 27, 2022
@github-actions github-actions bot added this to the v3.18.0 milestone Jun 27, 2022
@github-actions
Copy link
Contributor

This functionality has been released in v3.18.0 of the Terraform Cloudflare Provider.

Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

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

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.

cloudflare_record used with aws_acm_certificate always thinks it has to update
2 participants