rawlinp commented on a change in pull request #5350:
URL: https://github.com/apache/trafficcontrol/pull/5350#discussion_r544385312
##########
File path: traffic_ops/testing/api/v3/deliveryserviceservers_test.go
##########
@@ -111,17 +111,17 @@ func TryToRemoveLastServerInDeliveryService(t *testing.T)
{
_, _, err = TOSession.DeleteDeliveryServiceServer(*ds.ID, *server.ID)
if err == nil {
- t.Error("Didn't get expected error trying to remove the only
server assigned to a Delivery Service")
+ t.Logf("Didn't get expected error trying to remove the only
server assigned to a Delivery Service")
Review comment:
Why did you swap these errors to logs and vice versa? I think the logic
was correct before.
##########
File path: traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
##########
@@ -1150,3 +1151,52 @@ func CheckOriginServerInCacheGroupTopology(tx *sql.Tx,
dsID int, dsTopology stri
}
return nil, nil, http.StatusOK
}
+
+// CheckTopologyOrgServerCGInDSCG checks if ORG server are part of DS. IF they
are then the user is not allowed to remove the ORG servers from the associated
DS's topology
+func CheckTopologyOrgServerCGInDSCG(tx *sql.Tx, cdnIds []int, dsTopology
string, topologyCGNames []string) (error, error, int) {
+ // get servers and respective cachegroup name that have ORG type for
evert delivery service
+ q := `
+ SELECT ARRAY_AGG(d.xml_id), s.id, c.name
+ FROM server s
+ INNER JOIN deliveryservice_server ds ON ds.server = s.id
+ INNER JOIN deliveryservice d ON d.id =
ds.deliveryservice
+ INNER JOIN type t ON t.id = s.type
+ INNER JOIN cachegroup c ON c.id = s.cachegroup
+ WHERE d.cdn_id =ANY($1) AND t.name=$2 AND topology=$3
Review comment:
instead of `topology` by itself we should include the table alias
`d.topology`
##########
File path: traffic_ops/testing/api/v3/topologies_test.go
##########
@@ -240,6 +241,49 @@ func UpdateTestTopologies(t *testing.T) {
}
}
+func UpdateValidateTopologyORGServerCacheGroup(t *testing.T) {
+ params := url.Values{}
+ params.Set("xmlId", "ds-top")
+
+ //Get the correct DS
+ remoteDS, _, err := TOSession.GetDeliveryServicesV30WithHdr(nil, params)
+ if err != nil {
+ t.Errorf("cannot GET Delivery Services: %v", err)
+ }
+
+ //Assign ORG server to DS
+ assignServer := []string{"denver-mso-org-01"}
+ _, _, err = TOSession.AssignServersToDeliveryService(assignServer,
*remoteDS[0].XMLID)
+ if err != nil {
+ t.Errorf("cannot assign server to Delivery Services: %v", err)
+ }
+
+ //Get Topology node to update and remove ORG server nodes
+ origTopo := *remoteDS[0].Topology
+ resp, _, err := TOSession.GetTopologyWithHdr(origTopo, nil)
+ if err != nil {
+ t.Fatalf("couldn't find any topologies: %v", err)
+ }
+
+ // remove org server cachegroup
+ var p []int
+ newNodes := []tc.TopologyNode{{Id: 0, Cachegroup:
"topology-edge-cg-01", Parents: p, LastUpdated: nil}}
+ if *remoteDS[0].Topology == resp.Name {
+ resp.Nodes = newNodes
+ }
+ _, _, err = TOSession.UpdateTopology(*remoteDS[0].Topology, *resp)
+ if err == nil {
+ t.Fatalf("shouldnot UPDATE topology:%v to %v, but update was a
success", *remoteDS[0].Topology, newNodes[0].Cachegroup)
+ }
+
+ // Remove org server assignment and reset DS back to as it was for
further testing
+ serverResp, _, err := TOSession.GetDeliveryServiceServersWithHdr(nil)
Review comment:
I think we need to use `GetServersWithHdr` here, including `host_name =
denver-mso-org-01` in the `url.Values`, then use that ID in
`DeleteDeliveryServiceServer` below instead. When you use
`GetDeliveryServiceServersWithHdr`, it returns _all_ deliveryservice-server
assignments, and it's not guaranteed that the assignment you're trying to
delete is the first in the response.
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/servers/delete.go
##########
@@ -45,10 +45,11 @@ func DeleteDeprecated(w http.ResponseWriter, r
*http.Request) {
}
const lastServerQuery = `
-SELECT COUNT(*) = 0
-FROM deliveryservice_server
-WHERE deliveryservice = $1
- AND server <> $2
+ SELECT COUNT(*)
Review comment:
Why remove the `= 0`? The query is expected to return a boolean value
not an actual count. As-is, does this actually scan successfully into a bool?
##########
File path: traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
##########
@@ -1150,3 +1151,52 @@ func CheckOriginServerInCacheGroupTopology(tx *sql.Tx,
dsID int, dsTopology stri
}
return nil, nil, http.StatusOK
}
+
+// CheckTopologyOrgServerCGInDSCG checks if ORG server are part of DS. IF they
are then the user is not allowed to remove the ORG servers from the associated
DS's topology
+func CheckTopologyOrgServerCGInDSCG(tx *sql.Tx, cdnIds []int, dsTopology
string, topologyCGNames []string) (error, error, int) {
+ // get servers and respective cachegroup name that have ORG type for
evert delivery service
+ q := `
+ SELECT ARRAY_AGG(d.xml_id), s.id, c.name
+ FROM server s
+ INNER JOIN deliveryservice_server ds ON ds.server = s.id
+ INNER JOIN deliveryservice d ON d.id =
ds.deliveryservice
+ INNER JOIN type t ON t.id = s.type
+ INNER JOIN cachegroup c ON c.id = s.cachegroup
+ WHERE d.cdn_id =ANY($1) AND t.name=$2 AND topology=$3
+ GROUP BY s.id, c.name
+ `
+ serverId := ""
+ cacheGroupName := ""
+ dsNames := []string{}
+ serversCG := make(map[string]string)
+ serversDS := make(map[string]interface{})
+ rows, err := tx.Query(q, pq.Array(cdnIds), tc.OriginTypeName,
dsTopology)
+ if err != nil {
+ return nil, fmt.Errorf("querying deliveryservice origin server:
%s", err), http.StatusInternalServerError
+ }
+ defer log.Close(rows, "error closing rows")
+ for rows.Next() {
+ if err := rows.Scan(pq.Array(&dsNames), &serverId,
&cacheGroupName); err != nil {
+ return nil, fmt.Errorf("querying deliveryservice origin
server: %s", err), http.StatusInternalServerError
+ }
+ serversCG[cacheGroupName] = serverId
+ serversDS[serverId] = dsNames
+ }
+
+ var offendingDSSerCG []string
+ // put slice values into map for Topology's validation
+ topoCacheGroupNames := make(map[string]string)
+ for _, currentCG := range topologyCGNames {
+ topoCacheGroupNames[currentCG] = ""
+ }
+ for cg, s := range serversCG {
+ _, currentTopoCGOk := topoCacheGroupNames[cg]
+ if !currentTopoCGOk {
+ offendingDSSerCG = append(offendingDSSerCG,
fmt.Sprintf("cachegroup=%s (serverID=%s, delivery_service=%s)", cg, s,
serversDS[s]))
Review comment:
since there could be multiple DSes, `delivery_service` should be plural
##########
File path: traffic_ops/traffic_ops_golang/topology/topologies.go
##########
@@ -150,6 +150,17 @@ func (topology *TOTopology) Validate() error {
rules["empty cachegroups"] =
topology_validation.CheckForEmptyCacheGroups(topology.ReqInfo.Tx,
cacheGroupIds, dsCDNs, false, nil)
rules["required capabilities"] =
topology.validateDSRequiredCapabilities()
+ //Get current Topology-CG for the requested change.
+ topoCachegroupNames := topology.getCachegroupNames()
+ userErr, sysErr, _ =
dbhelpers.CheckTopologyOrgServerCGInDSCG(topology.ReqInfo.Tx.Tx, dsCDNs,
topology.Name, topoCachegroupNames)
+ if userErr != nil {
+ return fmt.Errorf("%s", userErr.Error())
Review comment:
this can just be `return userErr`
##########
File path: traffic_ops/traffic_ops_golang/topology/topologies.go
##########
@@ -150,6 +150,17 @@ func (topology *TOTopology) Validate() error {
rules["empty cachegroups"] =
topology_validation.CheckForEmptyCacheGroups(topology.ReqInfo.Tx,
cacheGroupIds, dsCDNs, false, nil)
rules["required capabilities"] =
topology.validateDSRequiredCapabilities()
+ //Get current Topology-CG for the requested change.
+ topoCachegroupNames := topology.getCachegroupNames()
+ userErr, sysErr, _ =
dbhelpers.CheckTopologyOrgServerCGInDSCG(topology.ReqInfo.Tx.Tx, dsCDNs,
topology.Name, topoCachegroupNames)
+ if userErr != nil {
+ return fmt.Errorf("%s", userErr.Error())
+ }
+ if sysErr != nil {
+ fmt.Println("System Error", sysErr)
Review comment:
This should use `log.Errorf` instead and should include a more precise
context (e.g. `log.Errorf("validating topology: %s", sysErr.Error())`
##########
File path: traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
##########
@@ -1150,3 +1151,52 @@ func CheckOriginServerInCacheGroupTopology(tx *sql.Tx,
dsID int, dsTopology stri
}
return nil, nil, http.StatusOK
}
+
+// CheckTopologyOrgServerCGInDSCG checks if ORG server are part of DS. IF they
are then the user is not allowed to remove the ORG servers from the associated
DS's topology
+func CheckTopologyOrgServerCGInDSCG(tx *sql.Tx, cdnIds []int, dsTopology
string, topologyCGNames []string) (error, error, int) {
+ // get servers and respective cachegroup name that have ORG type for
evert delivery service
+ q := `
+ SELECT ARRAY_AGG(d.xml_id), s.id, c.name
+ FROM server s
+ INNER JOIN deliveryservice_server ds ON ds.server = s.id
+ INNER JOIN deliveryservice d ON d.id =
ds.deliveryservice
+ INNER JOIN type t ON t.id = s.type
+ INNER JOIN cachegroup c ON c.id = s.cachegroup
+ WHERE d.cdn_id =ANY($1) AND t.name=$2 AND topology=$3
+ GROUP BY s.id, c.name
+ `
+ serverId := ""
+ cacheGroupName := ""
+ dsNames := []string{}
+ serversCG := make(map[string]string)
+ serversDS := make(map[string]interface{})
Review comment:
why `interface{}` instead of `[]string`?
----------------------------------------------------------------
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:
[email protected]
