[GitHub] [trafficcontrol] alficles commented on a change in pull request #5247: Change ORT/atstccfg to use standard TC objects

[GitBox <gi...@apache.org>]

alficles commented on a change in pull request #5247:
URL: https://github.com/apache/trafficcontrol/pull/5247#discussion_r523973339



##########
File path: lib/go-atscfg/cachedotconfig.go
##########
@@ -23,33 +23,95 @@ import (
 	"sort"
 	"strings"
 
-	"github.com/apache/trafficcontrol/lib/go-log"
 	"github.com/apache/trafficcontrol/lib/go-tc"
 )
 
 const ContentTypeCacheDotConfig = ContentTypeTextASCII
 const LineCommentCacheDotConfig = LineCommentHash
 
-type ProfileDS struct {
-	Type       tc.DSType
-	OriginFQDN *string
+func MakeCacheDotConfig(
+	server *tc.ServerNullable,
+	servers []tc.ServerNullable,
+	deliveryServices []tc.DeliveryServiceNullableV30,
+	deliveryServiceServers []tc.DeliveryServiceServer,
+	hdrComment string,
+) (Cfg, error) {
+	if tc.CacheTypeFromString(server.Type) == tc.CacheTypeMid {
+		return makeCacheDotConfigMid(server, deliveryServices, hdrComment)
+	} else {
+		return makeCacheDotConfigEdge(server, servers, deliveryServices, deliveryServiceServers, hdrComment)
+	}
 }
 
 // MakeCacheDotConfig makes the ATS cache.config config file.
 // profileDSes must be the list of delivery services, which are assigned to severs, for which this profile is assigned. It MUST NOT contain any other delivery services. Note DSesToProfileDSes may be helpful if you have a []tc.DeliveryServiceNullable, for example from traffic_ops/client.
-func MakeCacheDotConfig(
-	profileName string,
-	profileDSes []ProfileDS,
-	toToolName string, // tm.toolname global parameter (TODO: cache itself?)
-	toURL string, // tm.url global parameter (TODO: cache itself?)
-) string {
+func makeCacheDotConfigEdge(
+	server *tc.ServerNullable,
+	servers []tc.ServerNullable,
+	deliveryServices []tc.DeliveryServiceNullableV30,
+	deliveryServiceServers []tc.DeliveryServiceServer,
+	hdrComment string,
+) (Cfg, error) {
+	warnings := []string{}
+
+	if server.Profile == nil {
+		return Cfg{}, makeErr(warnings, "server missing profile")
+	}
+
+	profileServerIDsMap := map[int]struct{}{}
+	for _, sv := range servers {
+		if sv.Profile == nil {
+			warnings = append(warnings, "servers had server with nil profile, skipping!")
+			continue
+		}
+		if sv.ID == nil {
+			warnings = append(warnings, "servers had server with nil id, skipping!")
+			continue
+		}
+		if *sv.Profile != *server.Profile {
+			continue
+		}
+		profileServerIDsMap[*sv.ID] = struct{}{}
+	}
+
+	dsServers := filterDSS(deliveryServiceServers, nil, profileServerIDsMap)
+
+	dsIDs := map[int]struct{}{}
+	for _, dss := range dsServers {
+		if dss.Server == nil || dss.DeliveryService == nil {
+			continue // TODO warn? err?
+		}
+		if _, ok := profileServerIDsMap[*dss.Server]; !ok {
+			continue
+		}
+		dsIDs[*dss.DeliveryService] = struct{}{}
+	}
+
+	profileDSes := []profileDS{}
+	for _, ds := range deliveryServices {
+		if ds.ID == nil || ds.Type == nil || ds.OrgServerFQDN == nil {
+			continue // TODO warn? err?
+		}
+		if *ds.Type == tc.DSTypeInvalid {
+			continue // TODO warn? err?
+		}
+		if *ds.OrgServerFQDN == "" {
+			continue // TODO warn? err?
+		}

Review comment:
       If I understand this correctly, these only occur if TC passes ORT data it thinks is bad. This suggests either a defect in TC or a misunderstanding on ort's part. This warrants a warning, I think. Not necessarily an error, though one hopes that operators have alarms on warnings here.
   
   I think this is a result of moved code, though, instead of new code. So this isn't a major issue.

##########
File path: lib/go-atscfg/cacheurldotconfig.go
##########
@@ -86,7 +110,8 @@ func MakeCacheURLDotConfig(
 			}
 
 			if !strings.HasPrefix(org, scheme) {
-				log.Errorln("MakeCacheURLDotConfig got ds '" + string(dsName) + "' origin '" + org + "' with no scheme! cacheurl.config will likely be malformed!")
+				// TODO determine if we should return an empty config here. A bad DS should not break config gen, and MUST NOT for self-service
+				warnings = append(warnings, "ds '"+string(dsName)+"' origin '"+org+"' with no scheme! cacheurl.config will likely be malformed!")

Review comment:
       This should probably be opened as a separate ticket for resolution. That TODO is spot on and we shouldn't lose track of it. This change is fine, though.

##########
File path: lib/go-atscfg/headerrewritedotconfig.go
##########
@@ -39,7 +39,127 @@ const ServiceCategoryHeader = "CDN-SVC"
 
 const MaxOriginConnectionsNoMax = 0 // 0 indicates no limit on origin connections
 
-type HeaderRewriteDS struct {
+func MakeHeaderRewriteDotConfig(
+	fileName string,
+	deliveryServices []tc.DeliveryServiceNullableV30,
+	deliveryServiceServers []tc.DeliveryServiceServer,
+	server *tc.ServerNullable,
+	servers []tc.ServerNullable,
+	hdrComment string,
+) (Cfg, error) {
+	warnings := []string{}
+
+	dsName := strings.TrimSuffix(strings.TrimPrefix(fileName, HeaderRewritePrefix), ConfigSuffix) // TODO verify prefix and suffix? Perl doesn't
+
+	tcDS := tc.DeliveryServiceNullableV30{}
+	for _, ds := range deliveryServices {
+		if ds.XMLID == nil {
+			warnings = append(warnings, "deliveryServices had DS with nil xmlId (name)")
+			continue
+		}
+		if *ds.XMLID != dsName {
+			continue
+		}
+		tcDS = ds
+		break
+	}
+	if tcDS.ID == nil {
+		return Cfg{}, makeErr(warnings, "ds '"+dsName+"' not found")
+	}
+
+	if tcDS.CDNName == nil {
+		return Cfg{}, makeErr(warnings, "ds '"+dsName+"' missing cdn")
+	}
+
+	ds, err := headerRewriteDSFromDS(&tcDS)
+	if err != nil {
+		return Cfg{}, makeErr(warnings, "converting ds to config ds: "+err.Error())
+	}
+
+	dsServers := filterDSS(deliveryServiceServers, map[int]struct{}{ds.ID: {}}, nil)
+
+	dsServerIDs := map[int]struct{}{}
+	for _, dss := range dsServers {
+		if dss.Server == nil || dss.DeliveryService == nil {
+			continue // TODO warn?
+		}
+		if *dss.DeliveryService != *tcDS.ID {
+			continue
+		}
+		dsServerIDs[*dss.Server] = struct{}{}
+	}
+
+	assignedEdges := []headerRewriteServer{}
+	for _, server := range servers {
+		if server.CDNName == nil {
+			warnings = append(warnings, "servers had server with missing cdnName, skipping!")
+			continue
+		}
+		if server.ID == nil {
+			warnings = append(warnings, "servers had server with missing kid, skipping!")

Review comment:
       Typo in name here? s/kid/id/?

##########
File path: lib/go-atscfg/headerrewritedotconfig.go
##########
@@ -39,7 +39,127 @@ const ServiceCategoryHeader = "CDN-SVC"
 
 const MaxOriginConnectionsNoMax = 0 // 0 indicates no limit on origin connections
 
-type HeaderRewriteDS struct {
+func MakeHeaderRewriteDotConfig(
+	fileName string,
+	deliveryServices []tc.DeliveryServiceNullableV30,
+	deliveryServiceServers []tc.DeliveryServiceServer,
+	server *tc.ServerNullable,
+	servers []tc.ServerNullable,
+	hdrComment string,
+) (Cfg, error) {
+	warnings := []string{}
+
+	dsName := strings.TrimSuffix(strings.TrimPrefix(fileName, HeaderRewritePrefix), ConfigSuffix) // TODO verify prefix and suffix? Perl doesn't
+
+	tcDS := tc.DeliveryServiceNullableV30{}
+	for _, ds := range deliveryServices {
+		if ds.XMLID == nil {
+			warnings = append(warnings, "deliveryServices had DS with nil xmlId (name)")
+			continue
+		}
+		if *ds.XMLID != dsName {
+			continue
+		}
+		tcDS = ds
+		break
+	}
+	if tcDS.ID == nil {
+		return Cfg{}, makeErr(warnings, "ds '"+dsName+"' not found")
+	}
+
+	if tcDS.CDNName == nil {
+		return Cfg{}, makeErr(warnings, "ds '"+dsName+"' missing cdn")
+	}
+
+	ds, err := headerRewriteDSFromDS(&tcDS)
+	if err != nil {
+		return Cfg{}, makeErr(warnings, "converting ds to config ds: "+err.Error())
+	}
+
+	dsServers := filterDSS(deliveryServiceServers, map[int]struct{}{ds.ID: {}}, nil)
+
+	dsServerIDs := map[int]struct{}{}
+	for _, dss := range dsServers {
+		if dss.Server == nil || dss.DeliveryService == nil {
+			continue // TODO warn?

Review comment:
       Same story as above. This should be a warning.

##########
File path: lib/go-atscfg/headerrewritemiddotconfig.go
##########
@@ -20,24 +20,134 @@ package atscfg
  */
 
 import (
+	"fmt"
 	"math"
 	"regexp"
 	"strconv"
+	"strings"
 
 	"github.com/apache/trafficcontrol/lib/go-tc"
 )
 
 const HeaderRewriteMidPrefix = "hdr_rw_mid_"
 
 func MakeHeaderRewriteMidDotConfig(
-	cdnName tc.CDNName,
-	toToolName string, // tm.toolname global parameter (TODO: cache itself?)
-	toURL string, // tm.url global parameter (TODO: cache itself?)
-	ds HeaderRewriteDS,
-	assignedMids []HeaderRewriteServer, // the mids assigned to ds (mids whose cachegroup is the parent of the cachegroup of any edge assigned to this ds)
-) string {
-	text := GenericHeaderComment(string(cdnName), toToolName, toURL)
+	fileName string,
+	deliveryServices []tc.DeliveryServiceNullableV30,
+	deliveryServiceServers []tc.DeliveryServiceServer,
+	server *tc.ServerNullable,
+	servers []tc.ServerNullable,
+	cacheGroups []tc.CacheGroupNullable,
+	hdrComment string,
+) (Cfg, error) {
+	warnings := []string{}
+	if server.CDNName == nil {
+		return Cfg{}, makeErr(warnings, "this server missing CDNName")
+	}
+
+	dsName := strings.TrimSuffix(strings.TrimPrefix(fileName, HeaderRewriteMidPrefix), ConfigSuffix) // TODO verify prefix and suffix? Perl doesn't
+
+	tcDS := tc.DeliveryServiceNullableV30{}
+	for _, ds := range deliveryServices {
+		if ds.XMLID == nil || *ds.XMLID != dsName {
+			continue
+		}
+		tcDS = ds
+		break
+	}
+	if tcDS.ID == nil {
+		return Cfg{}, makeErr(warnings, "ds '"+dsName+"' not found")
+	}
+
+	if tcDS.CDNName == nil {
+		return Cfg{}, makeErr(warnings, "ds '"+dsName+"' missing cdn")
+	}
+
+	ds, err := headerRewriteDSFromDS(&tcDS)
+	if err != nil {
+		return Cfg{}, makeErr(warnings, "converting ds to config ds: "+err.Error())
+	}
+
+	assignedServers := map[int]struct{}{}
+	for _, dss := range deliveryServiceServers {
+		if dss.Server == nil || dss.DeliveryService == nil {
+			continue
+		}
+		if *dss.DeliveryService != *tcDS.ID {
+			continue
+		}
+		assignedServers[*dss.Server] = struct{}{}
+	}
 
+	serverCGs := map[tc.CacheGroupName]struct{}{}
+	for _, sv := range servers {
+		if sv.CDNName == nil {
+			warnings = append(warnings, "TO returned Servers server with missing CDNName, skipping!")
+			continue
+		} else if sv.ID == nil {
+			warnings = append(warnings, "TO returned Servers server with missing ID, skipping!")
+			continue
+		} else if sv.Status == nil {
+			warnings = append(warnings, "TO returned Servers server with missing Status, skipping!")
+			continue
+		} else if sv.Cachegroup == nil {
+			warnings = append(warnings, "TO returned Servers server with missing Cachegroup, skipping!")
+			continue
+		}
+
+		if sv.CDNName != server.CDNName {
+			continue
+		}
+		if _, ok := assignedServers[*sv.ID]; !ok && (tcDS.Topology == nil || *tcDS.Topology == "") {
+			continue
+		}
+		if tc.CacheStatus(*sv.Status) != tc.CacheStatusReported && tc.CacheStatus(*sv.Status) != tc.CacheStatusOnline {
+			continue
+		}
+		serverCGs[tc.CacheGroupName(*sv.Cachegroup)] = struct{}{}
+	}
+
+	parentCGs := map[string]struct{}{} // names of cachegroups which are parent cachegroups of the cachegroup of any edge assigned to the given DS
+	for _, cg := range cacheGroups {
+		if cg.Name == nil {
+			continue // TODO warn?
+		}
+		if cg.ParentName == nil {
+			continue // TODO warn?

Review comment:
       Same as above. Worth a warning, imo.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org