net.FQDN does not work as expected

[sahroshan]

What version of CUE are you using (cue version)?

$ cue version

cue version v0.5.0

go version go1.19.8
       -compiler gc
       -trimpath true
     CGO_ENABLED 0
          GOARCH amd64
            GOOS darwin
         GOAMD64 v1

Does this issue reproduce with the latest stable release?

yes

What did you do?

1. create a cue file

   
   import "net"

loki: { Endpoint: net.FQDN & string }

2. create yaml file

loki: Endpoint: "https://abcd_ab.com"

3. export cue

$cue export abcd.cue abcd.yaml

{ "loki": { "Endpoint": "https://abcd_ab.com" } }

### What did you expect to see?

Cue should throw error as there is a _ in the domain name.

### What did you see instead?
No error was observed.

[slewiskelly]

I don't know if this is more of a bug in the golang.org/x/net/idna package, but it seems like the StrictDomainName option can't be used without also using either of theMapForLookup or ValidateForRegistration options.

I would propose replacing both ValidateLabels and StrictDomainName with MapForLookup as this sets both of these options also.

[slewiskelly]

https://review.gerrithub.io/c/cue-lang/cue/+/554858

[mvdan]

I don't know if this is more of a bug in the golang.org/x/net/idna package, but it seems like the StrictDomainName option can't be used without also using either of theMapForLookup or ValidateForRegistration options.

Given what the documentation says:

StrictDomainName limits the set of permissible ASCII characters to those allowed in domain names as defined in RFC 1034 (A-Z, a-z, 0-9 and the hyphen). This is set by default for MapForLookup and ValidateForRegistration, but is only useful if ValidateLabels is set.

then I would expect our current code to work correctly - we do set ValidateLabels. I think this is a bug in the upstream package that we should raise there first. Either the upstream docs aren't very clear and we need to update our code, or upstream is buggy and we shouldn't change our code at all.

[mvdan]

Briefly spoke about this with Marcel - switching to MapForLookup per https://review.gerrithub.io/c/cue-lang/cue/+/554858 seems fine, it's a question of what the use cases for FQDN are. The current set of options we use are presumably less strict than "require the domain to be valid for lookups", even if they appear to be buggy.

I think we should aim to get StrictDomainName fixed upstream before we switch to MapForLookup. Upstream should get it fixed per the docs no matter what we end up doing here, and I also prefer to avoid breaking changes on our standard library unless we absolutely must do them.

[mvdan]

To reiterate - I'm happy with the change as proposed with two minor changes:

1) We should file a bug report upstream to flag that StrictDomainName does not behave as documented. 2) Our workaround to instead use MapForLookup should have a // TODO comment to be reverted once the upstream fix has been implemented and we can rely on its Go version.

@slewiskelly let me know if that sounds good and if you can file the upstream issue and update the CL. If you're not able to do that, I can always do it myself and pick up your change from its current state.