You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2020/11/06 22:17:43 UTC
[GitHub] [trafficcontrol] rawlinp opened a new pull request #5260: Validate the assignment of ORG servers to topology-based DSes
rawlinp opened a new pull request #5260:
URL: https://github.com/apache/trafficcontrol/pull/5260
## What does this PR (Pull Request) do?
Add validation to TO: when assigning ORG servers to topology-based delivery services, the ORG server cachegroup ORG must belong to the DS's topology. Otherwise, MSO won't work properly.
## Which Traffic Control components are affected by this PR?
- Traffic Ops
## What is the best way to verify this PR?
Review the tests, verify they pass. Attempt to assign ORG servers to topology-based delivery services where the ORG server cachegroup belongs to the DS's topology (should succeed) and where the ORG server cachegroup does not belong to the DS's topology (should fail).
## The following criteria are ALL met by this PR
- [x] This PR includes tests
- [x] new validation, no docs necessary
- [x] This PR includes an update to CHANGELOG.md
- [x] This PR includes any and all required license headers
- [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://www.apache.org/security/) for details)
----------------------------------------------------------------
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
[GitHub] [trafficcontrol] rawlinp commented on pull request #5260: Validate the assignment of ORG servers to topology-based DSes
Posted by GitBox <gi...@apache.org>.
rawlinp commented on pull request #5260:
URL: https://github.com/apache/trafficcontrol/pull/5260#issuecomment-723333825
Rebased to resolve merge conflicts.
----------------------------------------------------------------
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
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #5260: Validate the assignment of ORG servers to topology-based DSes
Posted by 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
[GitHub] [trafficcontrol] zrhoffman merged pull request #5260: Validate the assignment of ORG servers to topology-based DSes
Posted by GitBox <gi...@apache.org>.
zrhoffman merged pull request #5260:
URL: https://github.com/apache/trafficcontrol/pull/5260
----------------------------------------------------------------
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
[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #5260: Validate the assignment of ORG servers to topology-based DSes
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #5260:
URL: https://github.com/apache/trafficcontrol/pull/5260#discussion_r520102413
##########
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:
I think because if the scan failed, we probably don't have the ID or the xmlID. I will add the ID/XMLID to the callers.
##########
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:
That's confusing to me because an `ORG`-type server is not a cache.
----------------------------------------------------------------
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
[GitHub] [trafficcontrol] zrhoffman commented on pull request #5260: Validate the assignment of ORG servers to topology-based DSes
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #5260:
URL: https://github.com/apache/trafficcontrol/pull/5260#issuecomment-723328811
There's merge conflicts.
----------------------------------------------------------------
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
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #5260: Validate the assignment of ORG servers to topology-based DSes
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5260:
URL: https://github.com/apache/trafficcontrol/pull/5260#discussion_r520110014
##########
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:
Good point
----------------------------------------------------------------
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