EDGECLOUD-5233 create appinst fails fqdn prefix invalid DNS name #1403
Conversation
| // DNS names must have labels <= 63 chars, and the total length | ||
| // <= 255 octets (which works out to 253 chars). | ||
| func CheckFQDNLengths(prefix, uri string) error { | ||
| fqdn := prefix + uri |
There was a problem hiding this comment.
you might consider using IsDNSName in the govalidator package. It checks for length as well as a regex match on each part being 63 chars
There was a problem hiding this comment.
That's a good suggestion to use an existing library function. I looked at the code for govalidator though, and I think it's off in the length check (should be 253 instead of 255, as noted here: https://devblogs.microsoft.com/oldnewthing/20120412-00/?p=7873). And that just uses a simple regexp to check. So I started to look for alternatives, and then spent an hour reading about valid DNS formats, and I couldn't really find a good definitive solution. It depends on if the name is in Ascii vs Unicode vs Punycode, and if it's an A-label or something else (https://datatracker.ietf.org/doc/html/rfc5890). There's still an open bug in the go net library on it from 2016 (golang/go#17659) because they couldn't agree on an answer. It's a mess because the standards and best practices and technologies have evolved over time.
So I think I'm going to leave it like it is, checking just the lengths. At least that part is pretty clear. At least our regexps for checking names (limits it to ASCII) and then DNSSanitize means the label formats are probably ok, so it's only the lengths that need to be checked.
cloudcommon/names.go
Outdated
| func CheckFQDNLengths(prefix, uri string) error { | ||
| fqdn := prefix + uri | ||
| if len(fqdn) > 253 { | ||
| return fmt.Errorf(`DNS name "%s" exceeds 253 chars, please shorten some names`, fqdn) |
There was a problem hiding this comment.
NIT: "%s" can be replaced with %q. Just a suggestion, you don't have to do this change.
There was a problem hiding this comment.
cool, I didn't know you could do that. Made that change.
Issues Fixed
Description
Fixes a regression caused by changing the fqdn prefix separate from dot to dash. The problem was that DNS labels (the section between dots) must be less than 63 chars, and the label was too long.
After discussion with Jim, I have made the following changes. The original intent was to allow using TLS, which requires that the dns match the wildcard cert, which is of the form *.cloudlet.oper.mobiledgex.net. However, we noted that the fqdn prefix was being used for all platforms, when it is really only need for platforms that issue an IP for each kubernetes service (i.e. GCP/Azure/etc). So, we will not generate the fqdn prefix for kubernetes on other platforms (like Openstack/VCD), and we will not support TLS on GCP/Azure/etc. I have also made one other change, which is to change the format of the shared root LB's DNS name to match the above wildcard cert, so that we don't need another wildcard cert like *.oper.mobiledgex.net anymore.
The code changes are pretty minimal but if affected a lot of the test data files, especially after enabling e2e-tests to check the URI (which it wasn't doing before for some reason).
Also I should mention I added in backwards compatibility for the shared root LB name change, for CRMs that cannot be upgraded at the same time as the Controllers.