[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #5260: Validate the assignment of ORG servers to topology-based DSes

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

zrhoffman commented on a change in pull request #5260:
URL: https://github.com/apache/trafficcontrol/pull/5260#discussion_r519047562



##########
File path: traffic_ops/traffic_ops_golang/server/servers_assignment.go
##########
@@ -126,6 +132,45 @@ func AssignDeliveryServicesToServerHandler(w http.ResponseWriter, r *http.Reques
 	api.WriteRespAlertObj(w, r, tc.SuccessLevel, "successfully assigned dses to server", tc.AssignedDsResponse{server, assignedDSes, replace})
 }
 
+// checkOriginInTopologies checks to make sure the given ORG server's cachegroup belongs
+// to the topologies of the given delivery services.
+func checkOriginInTopologies(tx *sql.Tx, originCachegroup string, dsList []int) (error, error, int) {
+	// get the delivery services that don't have originCachegroup in their topology
+	q := `
+SELECT
+  ds.xml_id,
+  tc.topology,
+  ARRAY_AGG(tc.cachegroup)
+FROM
+  deliveryservice ds
+  JOIN topology_cachegroup tc ON tc.topology = ds.topology
+WHERE
+  ds.id = ANY($1::bigint[])

Review comment:
       `bigint` is a SQL key word and should be capitalized for visibility.

##########
File path: traffic_ops/traffic_ops_golang/server/servers_assignment.go
##########
@@ -112,6 +112,12 @@ func AssignDeliveryServicesToServerHandler(w http.ResponseWriter, r *http.Reques
 			api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
 			return
 		}
+		if strings.HasPrefix(serverInfo.Type, tc.OriginTypeName) {

Review comment:
       This might be a good time to add a `tc.CacheTypeOrg` constant and support `ORG`-type servers in `tc.CacheTypeFromString()`.

##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go
##########
@@ -503,26 +492,67 @@ func GetCreateHandler(w http.ResponseWriter, r *http.Request) {
 	api.WriteResp(w, r, tc.DeliveryServiceServers{payload.ServerNames, payload.XmlId})
 }
 
-// ValidateDSSAssignments returns an error if the given servers cannot be assigned to the given delivery service.
-func ValidateDSSAssignments(ds DSInfo, servers []dbhelpers.ServerHostNameCDNIDAndType) error {
+// validateDSSAssignments returns an error if the given servers cannot be assigned to the given delivery service.
+func validateDSSAssignments(tx *sql.Tx, ds DSInfo, serverInfos []tc.ServerInfo) (error, error, int) {
+	userErr, sysErr, status := validateDSS(tx, ds, serverInfos)
+	if userErr != nil || sysErr != nil {
+		return userErr, sysErr, status
+	}
+
+	userErr, sysErr, status = ValidateServerCapabilities(tx, ds.ID, serverInfos)
+	if userErr != nil || sysErr != nil {
+		return userErr, sysErr, status
+	}
+	return nil, nil, http.StatusOK
+}
+
+func validateDSS(tx *sql.Tx, ds DSInfo, servers []tc.ServerInfo) (error, error, int) {
 	if ds.Topology == nil {
 		for _, s := range servers {
 			if ds.CDNID != nil && s.CDNID != *ds.CDNID {
-				return errors.New("server and delivery service CDNs do not match")
+				return errors.New("server and delivery service CDNs do not match"), nil, http.StatusBadRequest
 			}
 		}
-		return nil
+		return nil, nil, http.StatusOK
 	}
 	for _, s := range servers {
 		if s.Type != tc.OriginTypeName {
-			return errors.New("only servers of type ORG may be assigned to topology-based delivery services")
+			return errors.New("only servers of type ORG may be assigned to topology-based delivery services"), nil, http.StatusBadRequest

Review comment:
       Nit: `"ORG"` here can be `tc.OriginTypeName`.

##########
File path: traffic_ops/traffic_ops_golang/server/servers_assignment.go
##########
@@ -126,6 +132,45 @@ func AssignDeliveryServicesToServerHandler(w http.ResponseWriter, r *http.Reques
 	api.WriteRespAlertObj(w, r, tc.SuccessLevel, "successfully assigned dses to server", tc.AssignedDsResponse{server, assignedDSes, replace})
 }
 
+// checkOriginInTopologies checks to make sure the given ORG server's cachegroup belongs
+// to the topologies of the given delivery services.
+func checkOriginInTopologies(tx *sql.Tx, originCachegroup string, dsList []int) (error, error, int) {
+	// get the delivery services that don't have originCachegroup in their topology
+	q := `
+SELECT
+  ds.xml_id,
+  tc.topology,
+  ARRAY_AGG(tc.cachegroup)
+FROM
+  deliveryservice ds
+  JOIN topology_cachegroup tc ON tc.topology = ds.topology
+WHERE
+  ds.id = ANY($1::bigint[])
+GROUP BY ds.xml_id, tc.topology
+HAVING NOT ($2 = ANY(ARRAY_AGG(tc.cachegroup)))
+`
+	rows, err := tx.Query(q, pq.Array(dsList), originCachegroup)
+	if err != nil {
+		return nil, errors.New("querying deliveryservice topologies: " + err.Error()), http.StatusInternalServerError
+	}
+	defer log.Close(rows, "error closing rows")
+
+	invalid := []string{}
+	for rows.Next() {
+		xmlID := ""
+		topology := ""
+		cachegroups := []string{}
+		if err := rows.Scan(&xmlID, &topology, pq.Array(&cachegroups)); err != nil {
+			return nil, errors.New("scanning deliveryservice topologies: " + err.Error()), http.StatusInternalServerError
+		}
+		invalid = append(invalid, fmt.Sprintf("%s (%s)", topology, xmlID))
+	}
+	if len(invalid) > 0 {
+		return fmt.Errorf("ORG server cachegroup (%s) not found in the following topologies: %s", originCachegroup, strings.Join(invalid, ", ")), nil, http.StatusBadRequest

Review comment:
       Nit: Literal `"ORG"` can be replaced with `tc.OriginTypeName`.

##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go
##########
@@ -460,21 +455,15 @@ func GetCreateHandler(w http.ResponseWriter, r *http.Request) {
 	payload.XmlId = dsName
 	serverNames := payload.ServerNames
 
-	serverNamesCdnIdAndTypes, err := dbhelpers.GetServerTypesCdnIdFromHostNames(inf.Tx.Tx, serverNames)
+	serverInfos, err := dbhelpers.GetServerInfosFromHostNames(inf.Tx.Tx, serverNames)
 	if err != nil {
 		api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, err, nil)

Review comment:
       Same thing here: Line is from a previous PR, but `err` is a system error, not a user error.

##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go
##########
@@ -379,20 +379,15 @@ func GetReplaceHandler(w http.ResponseWriter, r *http.Request) {
 		api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
 		return
 	}
-	serverNamesCdnIdAndTypes, err := dbhelpers.GetServerHostNamesAndTypesFromIDs(inf.Tx.Tx, servers)
+	serverInfos, err := dbhelpers.GetServerInfosFromIDs(inf.Tx.Tx, servers)
 	if err != nil {
 		api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, err, nil)

Review comment:
       This line was added in a previous PR, but the `err` should be passed as a system error, not a user error.

##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go
##########
@@ -503,26 +492,67 @@ func GetCreateHandler(w http.ResponseWriter, r *http.Request) {
 	api.WriteResp(w, r, tc.DeliveryServiceServers{payload.ServerNames, payload.XmlId})
 }
 
-// ValidateDSSAssignments returns an error if the given servers cannot be assigned to the given delivery service.
-func ValidateDSSAssignments(ds DSInfo, servers []dbhelpers.ServerHostNameCDNIDAndType) error {
+// validateDSSAssignments returns an error if the given servers cannot be assigned to the given delivery service.
+func validateDSSAssignments(tx *sql.Tx, ds DSInfo, serverInfos []tc.ServerInfo) (error, error, int) {
+	userErr, sysErr, status := validateDSS(tx, ds, serverInfos)
+	if userErr != nil || sysErr != nil {
+		return userErr, sysErr, status
+	}
+
+	userErr, sysErr, status = ValidateServerCapabilities(tx, ds.ID, serverInfos)
+	if userErr != nil || sysErr != nil {
+		return userErr, sysErr, status
+	}
+	return nil, nil, http.StatusOK
+}
+
+func validateDSS(tx *sql.Tx, ds DSInfo, servers []tc.ServerInfo) (error, error, int) {
 	if ds.Topology == nil {
 		for _, s := range servers {
 			if ds.CDNID != nil && s.CDNID != *ds.CDNID {
-				return errors.New("server and delivery service CDNs do not match")
+				return errors.New("server and delivery service CDNs do not match"), nil, http.StatusBadRequest
 			}
 		}
-		return nil
+		return nil, nil, http.StatusOK
 	}
 	for _, s := range servers {
 		if s.Type != tc.OriginTypeName {
-			return errors.New("only servers of type ORG may be assigned to topology-based delivery services")
+			return errors.New("only servers of type ORG may be assigned to topology-based delivery services"), nil, http.StatusBadRequest
+		}
+	}
+
+	cachegroups, sysErr := dbhelpers.GetTopologyCachegroups(tx, *ds.Topology)
+	if sysErr != nil {
+		return nil, fmt.Errorf("validating ORG servers in topology %s: %v", *ds.Topology, sysErr), http.StatusInternalServerError

Review comment:
       Nit: `tc.OriginTypeName` can be used here instead of literal `"ORG"`.

##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go
##########
@@ -503,26 +492,67 @@ func GetCreateHandler(w http.ResponseWriter, r *http.Request) {
 	api.WriteResp(w, r, tc.DeliveryServiceServers{payload.ServerNames, payload.XmlId})
 }
 
-// ValidateDSSAssignments returns an error if the given servers cannot be assigned to the given delivery service.
-func ValidateDSSAssignments(ds DSInfo, servers []dbhelpers.ServerHostNameCDNIDAndType) error {
+// validateDSSAssignments returns an error if the given servers cannot be assigned to the given delivery service.
+func validateDSSAssignments(tx *sql.Tx, ds DSInfo, serverInfos []tc.ServerInfo) (error, error, int) {
+	userErr, sysErr, status := validateDSS(tx, ds, serverInfos)
+	if userErr != nil || sysErr != nil {
+		return userErr, sysErr, status
+	}
+
+	userErr, sysErr, status = ValidateServerCapabilities(tx, ds.ID, serverInfos)
+	if userErr != nil || sysErr != nil {
+		return userErr, sysErr, status
+	}
+	return nil, nil, http.StatusOK
+}
+
+func validateDSS(tx *sql.Tx, ds DSInfo, servers []tc.ServerInfo) (error, error, int) {
 	if ds.Topology == nil {
 		for _, s := range servers {
 			if ds.CDNID != nil && s.CDNID != *ds.CDNID {
-				return errors.New("server and delivery service CDNs do not match")
+				return errors.New("server and delivery service CDNs do not match"), nil, http.StatusBadRequest
 			}
 		}
-		return nil
+		return nil, nil, http.StatusOK
 	}
 	for _, s := range servers {
 		if s.Type != tc.OriginTypeName {
-			return errors.New("only servers of type ORG may be assigned to topology-based delivery services")
+			return errors.New("only servers of type ORG may be assigned to topology-based delivery services"), nil, http.StatusBadRequest
+		}
+	}
+
+	cachegroups, sysErr := dbhelpers.GetTopologyCachegroups(tx, *ds.Topology)
+	if sysErr != nil {
+		return nil, fmt.Errorf("validating ORG servers in topology %s: %v", *ds.Topology, sysErr), http.StatusInternalServerError
+	}
+	userErr := CheckServersInCachegroups(servers, cachegroups)
+	if userErr != nil {
+		return fmt.Errorf("validating ORG servers in topology %s: %v", *ds.Topology, userErr), nil, http.StatusBadRequest

Review comment:
       Nit: Another place where `tc.OriginTypeName` can be used instead of literal `"ORG"`.

##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go
##########
@@ -857,47 +887,46 @@ SELECT
 FROM
   deliveryservice ds
   JOIN type tp ON ds.type = tp.id
-WHERE
-  ds.id = $1
 `
-	di := DSInfo{ID: id}
-	if err := tx.QueryRow(qry, id).Scan(&di.Name, &di.Type, &di.EdgeHeaderRewrite, &di.MidHeaderRewrite, &di.RegexRemap, &di.SigningAlgorithm, &di.CacheURL, &di.MaxOriginConnections, &di.Topology, &di.CDNID); err != nil {
+
+func scanDSInfoRow(row *sql.Row) (DSInfo, bool, error) {
+	di := DSInfo{}
+	if err := row.Scan(
+		&di.ID,
+		&di.Name,
+		&di.Type,
+		&di.EdgeHeaderRewrite,
+		&di.MidHeaderRewrite,
+		&di.RegexRemap,
+		&di.SigningAlgorithm,
+		&di.CacheURL,
+		&di.MaxOriginConnections,
+		&di.Topology,
+		&di.CDNID,
+	); err != nil {
 		if err == sql.ErrNoRows {
 			return DSInfo{}, false, nil
 		}
-		return DSInfo{}, false, fmt.Errorf("querying delivery service server ds info '%v': %v", id, err)
+		return DSInfo{}, false, fmt.Errorf("querying delivery service server ds info: %v", err)

Review comment:
       Why remove the Delivery Service ID from this line? There is nothing else to identify the Delivery Service if this error appears in the logs.




----------------------------------------------------------------
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