rawlinp commented on a change in pull request #5350:
URL: https://github.com/apache/trafficcontrol/pull/5350#discussion_r542912301
##########
File path: traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
##########
@@ -1150,3 +1151,68 @@ func CheckOriginServerInCacheGroupTopology(tx *sql.Tx,
dsID int, dsTopology stri
}
return nil, nil, http.StatusOK
}
+
+//GetDSByCDNIdTopology return the id of delivery service based on cdn_id and
topology_name
+func GetDSByCDNIdTopology(tx *sql.Tx, cdnId int, topology string) (error,
[]int) {
+ dsId := []int64{}
+ q := `
+ SELECT COALESCE(ARRAY_AGG(id), '{}'::BIGINT[])
+ FROM deliveryservice
+ WHERE cdn_id=$1 AND topology=$2
+ `
+ err := tx.QueryRow(q, cdnId, topology).Scan(pq.Array(&dsId))
+ if err != nil {
+ return fmt.Errorf("querying delivery services: %s", err), nil
+ }
+ res := make([]int, len(dsId))
+ for i, id := range dsId {
+ res[i] = int(id)
+ }
+ return err, res
+}
+
+// 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, dsIDs []int, dsTopology
string, topologyCGNames []string) (error, error, int) {
+ // get servers and respective cachegroup name that have ORG type for
evert delivery service
+ q := `
+ SELECT d.xml_id, s.host_name, 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 ds.deliveryservice =ANY($1) AND t.name=$2
+ `
+ serverName := ""
+ cacheGroupName := ""
+ dsName := ""
+ servers := make(map[string]string)
+ rows, err := tx.Query(q, pq.Array(dsIDs), tc.OriginTypeName)
+ 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(&dsName, &serverName, &cacheGroupName); err
!= nil {
+ return nil, fmt.Errorf("querying deliveryservice origin
server: %s", err), http.StatusInternalServerError
+ }
+ servers[cacheGroupName] = serverName
Review comment:
We should also have a map of server ID to DS xml_id, so that we can
include the DS xml_id in the error message on L1211.
##########
File path: traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
##########
@@ -1150,3 +1151,68 @@ func CheckOriginServerInCacheGroupTopology(tx *sql.Tx,
dsID int, dsTopology stri
}
return nil, nil, http.StatusOK
}
+
+//GetDSByCDNIdTopology return the id of delivery service based on cdn_id and
topology_name
+func GetDSByCDNIdTopology(tx *sql.Tx, cdnId int, topology string) (error,
[]int) {
+ dsId := []int64{}
+ q := `
+ SELECT COALESCE(ARRAY_AGG(id), '{}'::BIGINT[])
+ FROM deliveryservice
+ WHERE cdn_id=$1 AND topology=$2
+ `
+ err := tx.QueryRow(q, cdnId, topology).Scan(pq.Array(&dsId))
+ if err != nil {
+ return fmt.Errorf("querying delivery services: %s", err), nil
+ }
+ res := make([]int, len(dsId))
+ for i, id := range dsId {
+ res[i] = int(id)
+ }
+ return err, res
+}
+
+// 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, dsIDs []int, dsTopology
string, topologyCGNames []string) (error, error, int) {
+ // get servers and respective cachegroup name that have ORG type for
evert delivery service
+ q := `
+ SELECT d.xml_id, s.host_name, 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 ds.deliveryservice =ANY($1) AND t.name=$2
+ `
+ serverName := ""
+ cacheGroupName := ""
+ dsName := ""
+ servers := make(map[string]string)
+ rows, err := tx.Query(q, pq.Array(dsIDs), tc.OriginTypeName)
+ 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(&dsName, &serverName, &cacheGroupName); err
!= nil {
+ return nil, fmt.Errorf("querying deliveryservice origin
server: %s", err), http.StatusInternalServerError
+ }
+ servers[cacheGroupName] = serverName
+ }
+
+ 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 servers {
+ _, currentTopoCGOk := topoCacheGroupNames[cg]
+ if !currentTopoCGOk {
+ offendingDSSerCG = append(offendingDSSerCG,
fmt.Sprintf("%s (%s)", cg, s))
Review comment:
With the server IDs change mentioned above and a map of server ID to DS
xml_id, we should format the message like this:
```
ORG servers are assigned to delivery services that use this topology, and
their cachegroups cannot be removed: my-cachegroup (server ID = x, delivery
service = y)
```
##########
File path: traffic_ops/testing/api/v3/topologies_test.go
##########
@@ -240,6 +241,39 @@ 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
+ resp, _, err := TOSession.GetTopologyWithHdr(*remoteDS[0].Topology, nil)
+ if err != nil {
+ t.Fatalf("couldn't find any topologies: %v", err)
+ }
+ 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, but update was a
success", *remoteDS[0].Topology)
+ }
Review comment:
This doesn't seem to be resolved yet
##########
File path: traffic_ops/traffic_ops_golang/topology/topologies.go
##########
@@ -150,6 +150,19 @@ func (topology *TOTopology) Validate() error {
rules["empty cachegroups"] =
topology_validation.CheckForEmptyCacheGroups(topology.ReqInfo.Tx,
cacheGroupIds, dsCDNs, false, nil)
rules["required capabilities"] =
topology.validateDSRequiredCapabilities()
+ for _, cdnId := range dsCDNs {
+ err, dsId :=
dbhelpers.GetDSByCDNIdTopology(topology.ReqInfo.Tx.Tx, cdnId, topology.Name)
Review comment:
Instead of looping over each CDN and running the check for each CDN, we
should just pass the `dsCDNs` slice into `CheckTopologyOrgServerCGInDSCG` and
update its query to replace `WHERE ds.deliveryservice =ANY($1)` with `WHERE
ds.cdn_id = ANY($1)`. We would also then have to add `INNER JOIN topology top
ON ds.topology = top.name` and add `AND top.name = $3` to the `WHERE` clause.
That would allow us to get all the data in a single query, and we could delete
the `GetDSByCDNIdTopology` function.
##########
File path: traffic_ops/traffic_ops_golang/topology/topologies.go
##########
@@ -150,6 +150,19 @@ func (topology *TOTopology) Validate() error {
rules["empty cachegroups"] =
topology_validation.CheckForEmptyCacheGroups(topology.ReqInfo.Tx,
cacheGroupIds, dsCDNs, false, nil)
rules["required capabilities"] =
topology.validateDSRequiredCapabilities()
+ for _, cdnId := range dsCDNs {
+ err, dsId :=
dbhelpers.GetDSByCDNIdTopology(topology.ReqInfo.Tx.Tx, cdnId, topology.Name)
+ if err != nil {
+ return fmt.Errorf("failed to get a response from DB:
%s", err)
+ }
+ //Get current Topology-CG for the requested change.
+ topoCachegroupNames := topology.getCachegroupNames()
+ userErr, sysErr, _ :=
dbhelpers.CheckTopologyOrgServerCGInDSCG(topology.ReqInfo.Tx.Tx, dsId,
topology.Name, topoCachegroupNames)
+ if userErr != nil || sysErr != nil {
+ return fmt.Errorf("%s", userErr.Error())
Review comment:
If `userErr != nil`, we should just `return userErr`. But if `sysErr !=
nil`, we should log the error then `return errors.New("unable to validate
topology")`
##########
File path: traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
##########
@@ -1150,3 +1151,68 @@ func CheckOriginServerInCacheGroupTopology(tx *sql.Tx,
dsID int, dsTopology stri
}
return nil, nil, http.StatusOK
}
+
+//GetDSByCDNIdTopology return the id of delivery service based on cdn_id and
topology_name
+func GetDSByCDNIdTopology(tx *sql.Tx, cdnId int, topology string) (error,
[]int) {
+ dsId := []int64{}
+ q := `
+ SELECT COALESCE(ARRAY_AGG(id), '{}'::BIGINT[])
+ FROM deliveryservice
+ WHERE cdn_id=$1 AND topology=$2
+ `
+ err := tx.QueryRow(q, cdnId, topology).Scan(pq.Array(&dsId))
+ if err != nil {
+ return fmt.Errorf("querying delivery services: %s", err), nil
+ }
+ res := make([]int, len(dsId))
+ for i, id := range dsId {
+ res[i] = int(id)
+ }
+ return err, res
+}
+
+// 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, dsIDs []int, dsTopology
string, topologyCGNames []string) (error, error, int) {
+ // get servers and respective cachegroup name that have ORG type for
evert delivery service
+ q := `
+ SELECT d.xml_id, s.host_name, c.name
Review comment:
I just remembered that `host_name` is not unique (sad, I know). So we
should basically replace the host_name with the server ID throughout this
function.
----------------------------------------------------------------
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]
