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

Unlock unicode domain support in 'cloudflare_zone' #399

Closed
wants to merge 2 commits into from
Closed

Unlock unicode domain support in 'cloudflare_zone' #399

wants to merge 2 commits into from

Conversation

ad-m
Copy link

@ad-m ad-m commented Jun 30, 2019

Since RFC 3492 due punycode the range of allowable values for the field has significantly expanded. The correct zone is, for example, "zajęzyk.pl" (xn--zajzyk-y4a.pl).

The CloudFlare API returns domain addresses without Punycode encoding, i.e. with a huge number of characters, much larger than allowed in this regular expression.

@ghost ghost added the size/XS label Jun 30, 2019
Copy link
Member

@jacobbednarz jacobbednarz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ad-m Appreciate the time you've taken to submit the PR! I have a couple of questions:

  • Why are we completely removing the regular expression here? Wouldn't you just want to expand upon it? The only change I think is actually needed is the length of string that is accepted.
  • Have you confirmed any length restrictions of domains with Cloudflare Support? If not, I'd suggest we double check this before we put any additional restrictions into the validation.

@ad-m
Copy link
Author

ad-m commented Jun 30, 2019

The only change I think is actually needed is the length of string that is accepted.

No, this is not true. The expression does not accept characters such as ą, ę, ś,ć, which are valid values - accepted and returned by API CloudFlare. This is the basic motivations for which I have drawn attention to this regular expression.

Have you confirmed any length restrictions of domains with Cloudflare Support?

I'm not a CloudFlare employee, I did not contact CloudFlare support. I just want to be able to manage a domain that contains characters like ą in its name.

@jacobbednarz
Copy link
Member

jacobbednarz commented Jun 30, 2019

Right, I misunderstood what you initially meant here. I thought you meant that you had to input xn--zajzyk-y4a.pl to have it work (not zajęzyk.pl). If that's not the case, we have another issue here. Terraform compares what you put into the value via the configuration with what it has in it's state. Here, the value would be zajęzyk.pl but the state would be xn--zajzyk-y4a.pl. This is going to result in a continuous loop of trying to update the resource to the expected value. You should be able to confirm this by building the provider with the validation rule removed and terraform applying the following:

resource "cloudflare_zone" "foo" {
    zone = "zajęzyk.pl"
}

You will be able to keep terraform applying for more than once despite the value not changing in the configuration.

Going forward there are a couple of options:

  • Use alphanumeric-ish only characters in your value. This will keep the regex we already have but just allow longer values. We will want to confirm with Cloudflare Support regarding the upper limits.
  • Update the DiffSuppressFunc to convert to alphanumeric-ish values before comparing whether the value has changed.. This will allow use of punycode style domains in the configuration but store them how Cloudflare returns with the comparison done to take it into account.

@ad-m
Copy link
Author

ad-m commented Jul 1, 2019

CloudFlare API already return unicode-encoded (no punycode) values.

Why state would be xn--zajzyk-y4a.pl?

@jacobbednarz
Copy link
Member

Are you able to provide your actual configuration and HTTP responses you're seeing? It's not completely clear which parts are not working for you and I'd like to make sure we're addressing the right thing here.

@codeflows
Copy link

Hi! Having this issue myself. Unicode domain names not accepted by the regex validator, Punycoded ones do work, here with the fictitious domain äää.fi:

provider "cloudflare" {
  version = "~> 1.16"
}

resource "cloudflare_zone" "test_domain" {
  # Punycoded version of äää.fi
  zone = "xn--4caaa.fi"
}

Initial terraform apply works and the domain äää.fi is created. terraform.tfstate contains the unicode version of the domain name (äää.fi) which was returned by the Cloudflare API:

{
  "version": 4,
  "terraform_version": "0.12.3",

  "resources": [
    {
      "mode": "managed",
      "type": "cloudflare_zone",
      "name": "test_domain",
      "provider": "provider.cloudflare",
      "instances": [
        {

          "attributes": {
            "id": "123",

            "zone": "äää.fi"
          }
        }
      ]
    }
  ]
}

Subsequent terraform apply wants to recreate the resource since the definition and the state differ:

$ terraform apply

Terraform will perform the following actions:

  # cloudflare_zone.test_domain must be replaced
-/+ resource "cloudflare_zone" "test_domain" {
     ... values omitted
      ~ zone                = "äää.fi" -> "xn--4caaa.fi" # forces replacement
    }

Plan: 1 to add, 0 to change, 1 to destroy.

Regarding the regex validation: since Cloudflare API validates the domain name anyways (e.g. rejecting non-registered domain names), I think dropping the regex wouldn't be that big of a deal? Writing a regex that covers all the cases seems non-trivial

Thanks!

@jacobbednarz
Copy link
Member

Thanks for the additional information here @codeflows, the issue is becoming clearer now. My understanding now is that we probably need two things to happen:

  • Remove the regex. Allow anything to be put in there and let Cloudflare sort it out. This will sort the use case where you want to put punycode in as your zone name.
    resource "cloudflare_zone" "test_domain" {
      zone = "äää.fi"
    }
  • Add a DiffSuppressFunc that converts everything to unicode before evaluating the change. This should address the use case where you put in xn--4caaa.fi as your zone name and Cloudflare returns the non-unicoded version of your domain (and ends up in the state file). This will ensure that regardless of which value you put in, the comparison will only make the change once the value has actually changed.
    resource "cloudflare_zone" "test_domain" {
      zone = "xn--4caaa.fi"
    }

Does this seem reasonable?

@ad-m
Copy link
Author

ad-m commented Jul 12, 2019

Why we need to handle puny-encoded domain? Can we solve that through documentation?

@jacobbednarz
Copy link
Member

We can definitely add documentation however for something like this that we can tuck away in the Terraform schema itself and have it Just Work™ for people, I'm all for. Regardless of what people put in there, we will support it and not have Terraform perform unexpectedly.

@codeflows
Copy link

Sounds good to me :)

@jacobbednarz
Copy link
Member

@ad-m Would you like to update this PR to implement the suggestions above or would you prefer someone else take a pass at them?

@codeflows
Copy link

I can also try implementing the suggestions if need be

@ad-m
Copy link
Author

ad-m commented Jul 15, 2019

I would you prefer someone else take a pass at them. I don't have any Go experience.

@jacobbednarz
Copy link
Member

Addressing both concerns in #412.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants