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

EDGECLOUD-5233 create appinst fails fqdn prefix invalid DNS name #1403

Merged
merged 2 commits into from Jul 9, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion cloudcommon/compatibility.go
Expand Up @@ -2,9 +2,10 @@ package cloudcommon

const (
CRMCompatibilityAutoReservableCluster uint32 = 1
CRMCompatibilitySharedRootLBFQDN uint32 = 2
)

// This should always return the highest compatibility version
func GetCRMCompatibilityVersion() uint32 {
return CRMCompatibilityAutoReservableCluster
return CRMCompatibilitySharedRootLBFQDN
}
56 changes: 44 additions & 12 deletions cloudcommon/names.go
Expand Up @@ -151,21 +151,34 @@ func IsPlatformApp(devname string, appname string) bool {

var AllocatedIpDynamic = "dynamic"

// GetRootLBFQDN gets the global Load Balancer's Fully Qualified Domain Name
// for apps using "shared" IP access.
func GetRootLBFQDN(key *edgeproto.CloudletKey, domain string) string {
var RootLBHostname = "shared"

// GetCloudletBaseFQDN gets the base 3-label FQDN for the cloudlet.
// For TLS termination, we should only require a single cert that
// wildcard matches *.<cloudlet-base-fqdn>, as all other DNS names
// should only add one more label on top of the base fqdn.
func GetCloudletBaseFQDN(key *edgeproto.CloudletKey, domain string) string {
loc := util.DNSSanitize(key.Name)
oper := util.DNSSanitize(key.Organization)
return fmt.Sprintf("%s.%s.%s", loc, oper, domain)
}

// GetRootLBFQDN gets the global Load Balancer's Fully Qualified Domain Name
// for apps using "shared" IP access.
func GetRootLBFQDN(key *edgeproto.CloudletKey, domain string) string {
return fmt.Sprintf("%s.%s", RootLBHostname, GetCloudletBaseFQDN(key, domain))
}

// Old version of getting the shared root lb, does not match wildcard cert.
func GetRootLBFQDNOld(key *edgeproto.CloudletKey, domain string) string {
return GetCloudletBaseFQDN(key, domain)
}

// GetDedicatedLBFQDN gets the cluster-specific Load Balancer's Fully Qualified Domain Name
// for clusters using "dedicated" IP access.
func GetDedicatedLBFQDN(cloudletKey *edgeproto.CloudletKey, clusterKey *edgeproto.ClusterKey, domain string) string {
clust := util.DNSSanitize(clusterKey.Name)
loc := util.DNSSanitize(cloudletKey.Name)
oper := util.DNSSanitize(cloudletKey.Organization)
return fmt.Sprintf("%s.%s.%s.%s", clust, loc, oper, domain)
return fmt.Sprintf("%s.%s", clust, GetCloudletBaseFQDN(cloudletKey, domain))
}

// Get Fully Qualified Name for the App i.e. with developer & version info
Expand All @@ -177,29 +190,48 @@ func GetAppFQN(key *edgeproto.AppKey) string {
}

// GetAppFQDN gets the app-specific Load Balancer's Fully Qualified Domain Name
// for apps using "dedicated" IP access.
// for apps using "dedicated" IP access. This will not allow TLS, but will
// ensure uniqueness when an IP is assigned per k8s-service per AppInst per cluster.
func GetAppFQDN(key *edgeproto.AppInstKey, cloudletKey *edgeproto.CloudletKey, clusterKey *edgeproto.ClusterKey, domain string) string {
lb := GetDedicatedLBFQDN(cloudletKey, clusterKey, domain)
clusterBase := GetDedicatedLBFQDN(cloudletKey, clusterKey, domain)
appFQN := GetAppFQN(&key.AppKey)
return fmt.Sprintf("%s.%s", appFQN, lb)
return fmt.Sprintf("%s.%s", appFQN, clusterBase)
}

// GetVMAppFQDN gets the app-specific Fully Qualified Domain Name
// for VM based apps
func GetVMAppFQDN(key *edgeproto.AppInstKey, cloudletKey *edgeproto.CloudletKey, domain string) string {
lb := GetRootLBFQDN(cloudletKey, domain)
appFQN := GetAppFQN(&key.AppKey)
return fmt.Sprintf("%s.%s", appFQN, lb)
return fmt.Sprintf("%s.%s", appFQN, GetCloudletBaseFQDN(cloudletKey, domain))
}

// FqdnPrefix is used only for IP-per-service platforms that allocate
// an IP for each kubernetes service. Because it adds an extra level of
// DNS label hierarchy and cannot match the wildcard cert, we do not
// support TLS for it.
func FqdnPrefix(svcName string) string {
return svcName + "-"
return svcName + "."
}

func ServiceFQDN(svcName, baseFQDN string) string {
return fmt.Sprintf("%s%s", FqdnPrefix(svcName), baseFQDN)
}

// 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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

if len(fqdn) > 253 {
return fmt.Errorf("DNS name %q exceeds 253 chars, please shorten some names", fqdn)
}
for _, label := range strings.Split(fqdn, ".") {
if len(label) > 63 {
return fmt.Errorf("Label %q of DNS name %q exceeds 63 chars, please shorten it", label, fqdn)
}
}
return nil
}

// For the DME and CRM that require a cloudlet key to be specified
// at startup, this function parses the string argument.
func ParseMyCloudletKey(standalone bool, keystr *string, mykey *edgeproto.CloudletKey) {
Expand Down
31 changes: 27 additions & 4 deletions controller/appinst_api.go
Expand Up @@ -445,6 +445,7 @@ func (s *AppInstApi) createAppInstInternal(cctx *CallContext, in *edgeproto.AppI
var reservedClusterInstKey *edgeproto.ClusterInstKey
realClusterName := in.RealClusterName
var cloudletFeatures *platform.Features
cloudletCompatibilityVersion := uint32(0)

defer func() {
if reterr != nil {
Expand All @@ -464,6 +465,7 @@ func (s *AppInstApi) createAppInstInternal(cctx *CallContext, in *edgeproto.AppI
reservedAutoClusterId = -1
reservedClusterInstKey = nil
in.RealClusterName = realClusterName
cloudletCompatibilityVersion = 0

// lookup App so we can get flavor for reservable ClusterInst
var app edgeproto.App
Expand All @@ -489,6 +491,7 @@ func (s *AppInstApi) createAppInstInternal(cctx *CallContext, in *edgeproto.AppI
if !cloudletInfoApi.store.STMGet(stm, &in.Key.ClusterInstKey.CloudletKey, &info) {
return fmt.Errorf("No resource information found for Cloudlet %s", in.Key.ClusterInstKey.CloudletKey)
}
cloudletCompatibilityVersion = info.CompatibilityVersion
cloudletFeatures, err = GetCloudletFeatures(ctx, cloudlet.PlatformType)
if err != nil {
return fmt.Errorf("Failed to get features for platform: %s", err)
Expand Down Expand Up @@ -524,7 +527,7 @@ func (s *AppInstApi) createAppInstInternal(cctx *CallContext, in *edgeproto.AppI
if !cloudcommon.IsClusterInstReqd(&app) {
return fmt.Errorf("No cluster required for App deployment type %s, cannot use cluster name %s which attempts to use or create a ClusterInst", app.Deployment, cloudcommon.AutoClusterPrefix)
}
if info.CompatibilityVersion < cloudcommon.CRMCompatibilityAutoReservableCluster {
if cloudletCompatibilityVersion < cloudcommon.CRMCompatibilityAutoReservableCluster {
autoClusterType = DeprecatedAutoCluster
}
if autoClusterType != DeprecatedAutoCluster {
Expand Down Expand Up @@ -951,7 +954,15 @@ func (s *AppInstApi) createAppInstInternal(cctx *CallContext, in *edgeproto.AppI
ports[ii].PublicPort = ports[ii].InternalPort
}
} else if ipaccess == edgeproto.IpAccess_IP_ACCESS_SHARED && !app.InternalPorts {
in.Uri = cloudcommon.GetRootLBFQDN(&in.Key.ClusterInstKey.CloudletKey, *appDNSRoot)
if cloudletCompatibilityVersion < cloudcommon.CRMCompatibilitySharedRootLBFQDN {
// CRM has issued DNS entry only for old style FQDN.
// This case can be removed once all CRMs have been
// updated to current version.
in.Uri = cloudcommon.GetRootLBFQDNOld(&in.Key.ClusterInstKey.CloudletKey, *appDNSRoot)
} else {
// current style FQDN
in.Uri = cloudcommon.GetRootLBFQDN(&in.Key.ClusterInstKey.CloudletKey, *appDNSRoot)
}
if cloudletRefs.RootLbPorts == nil {
cloudletRefs.RootLbPorts = make(map[int32]int32)
}
Expand Down Expand Up @@ -999,7 +1010,7 @@ func (s *AppInstApi) createAppInstInternal(cctx *CallContext, in *edgeproto.AppI
cloudletRefsChanged = true
}
} else {
if isIPAllocatedPerService(ctx, cloudlet.PlatformType, cloudletFeatures, clusterInst.Key.CloudletKey.Organization) {
if isIPAllocatedPerService(ctx, cloudlet.PlatformType, cloudletFeatures, in.Key.ClusterInstKey.CloudletKey.Organization) {
//dedicated access in which each service gets a different ip
in.Uri = cloudcommon.GetAppFQDN(&in.Key, &in.Key.ClusterInstKey.CloudletKey, clusterKey, *appDNSRoot)
for ii, _ := range ports {
Expand All @@ -1013,9 +1024,18 @@ func (s *AppInstApi) createAppInstInternal(cctx *CallContext, in *edgeproto.AppI
}
}
}
if app.InternalPorts || len(ports) == 0 {
// no external access to AppInst, no need for URI
in.Uri = ""
}
if err := cloudcommon.CheckFQDNLengths("", in.Uri); err != nil {
return err
}
if len(ports) > 0 {
in.MappedPorts = ports
setPortFQDNPrefixes(in, &app)
if isIPAllocatedPerService(ctx, cloudlet.PlatformType, cloudletFeatures, in.Key.ClusterInstKey.CloudletKey.Organization) {
setPortFQDNPrefixes(in, &app)
}
}

// TODO: Make sure resources are available
Expand Down Expand Up @@ -1859,6 +1879,9 @@ func setPortFQDNPrefixes(in *edgeproto.AppInst, app *edgeproto.App) error {
}
for ii, _ := range in.MappedPorts {
setPortFQDNPrefix(&in.MappedPorts[ii], objs)
if err := cloudcommon.CheckFQDNLengths(in.MappedPorts[ii].FqdnPrefix, in.Uri); err != nil {
return err
}
}
}
return nil
Expand Down
12 changes: 11 additions & 1 deletion controller/appinst_api_test.go
Expand Up @@ -8,6 +8,7 @@ import (
"time"

"github.com/coreos/etcd/clientv3/concurrency"
"github.com/mobiledgex/edge-cloud/cloud-resource-manager/platform"
"github.com/mobiledgex/edge-cloud/cloudcommon"
"github.com/mobiledgex/edge-cloud/edgeproto"
"github.com/mobiledgex/edge-cloud/log"
Expand Down Expand Up @@ -275,6 +276,12 @@ func TestAppInstApi(t *testing.T) {
if obj.Key.AppKey.Name == "helmApp" || obj.Key.AppKey.Name == "vm lb" {
continue
}
cloudlet := edgeproto.Cloudlet{}
found := cloudletApi.cache.Get(&obj.Key.ClusterInstKey.CloudletKey, &cloudlet)
require.True(t, found)
features := platform.Features{}
operator := obj.Key.ClusterInstKey.CloudletKey.Organization

for _, port := range obj.MappedPorts {
lproto, err := edgeproto.LProtoStr(port.Proto)
if err != nil {
Expand All @@ -283,7 +290,10 @@ func TestAppInstApi(t *testing.T) {
if lproto == "http" {
continue
}
test_prefix := fmt.Sprintf("%s-%s-", util.DNSSanitize(app_name), lproto)
test_prefix := ""
if isIPAllocatedPerService(ctx, cloudlet.PlatformType, &features, operator) {
test_prefix = fmt.Sprintf("%s-%s-", util.DNSSanitize(app_name), lproto)
}
require.Equal(t, test_prefix, port.FqdnPrefix, "check port fqdn prefix")
}
}
Expand Down
3 changes: 0 additions & 3 deletions edgeproto/alldata.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.