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/10/01 15:36:38 UTC
[GitHub] [trafficcontrol] rawlinp opened a new pull request #5091: Validate DS Topologies when deleting a server capability
rawlinp opened a new pull request #5091:
URL: https://github.com/apache/trafficcontrol/pull/5091
## What does this PR (Pull Request) do?
There must be at least one server in a cachegroup that can satisfy the
required capabilities of the delivery services the cachegroup is
assigned to via topologies. Otherwise, the topologies would no longer be
valid w.r.t. the required capabilities of the delivery services.
P.S. don't be alarmed at the size of the PR, most of it is from the additions to `tc-fixtures.json` and associated whitespace changes.
- [x] This PR is not related to any issue
## Which Traffic Control components are affected by this PR?
- Traffic Ops
## What is the best way to verify this PR?
Review the added TO API tests, verify the check passes.
## The following criteria are ALL met by this PR
- [x] This PR includes tests OR I have explained why tests are unnecessary
- [x] added validation, no docs necessary
- [x] This PR includes an update to CHANGELOG.md OR such an update is not necessary
- [x] This PR includes any and all required license headers
- [x] This PR ensures that database migration sequence is correct OR this PR does not include a database migration
- [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] zrhoffman commented on a change in pull request #5091: Validate DS Topologies when deleting a server capability
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5091:
URL: https://github.com/apache/trafficcontrol/pull/5091#discussion_r498384871
##########
File path: traffic_ops/traffic_ops_golang/server/servers_server_capability.go
##########
@@ -137,46 +138,145 @@ JOIN server s ON sc.server = s.id ` + where + orderBy + pagination +
}
func (ssc *TOServerServerCapability) Delete() (error, error, int) {
+ tenantIDs, err := tenant.GetUserTenantIDListTx(ssc.APIInfo().Tx.Tx, ssc.APIInfo().User.TenantID)
+ if err != nil {
+ return nil, fmt.Errorf("deleting servers_server_capability: %v", err), http.StatusInternalServerError
+ }
+ accessibleTenants := make(map[int]struct{}, len(tenantIDs))
+ for _, id := range tenantIDs {
+ accessibleTenants[id] = struct{}{}
+ }
+ userErr, sysErr, status := checkTopologyBasedDSRequiredCapabilities(ssc, accessibleTenants)
+ if userErr != nil || sysErr != nil {
+ return userErr, sysErr, status
+ }
+
+ userErr, sysErr, status = checkDSRequiredCapabilities(ssc, accessibleTenants)
+ if userErr != nil || sysErr != nil {
+ return userErr, sysErr, status
+ }
+
+ return api.GenericDelete(ssc)
+}
+
+func checkTopologyBasedDSRequiredCapabilities(ssc *TOServerServerCapability, accessibleTenants map[int]struct{}) (error, error, int) {
+ dsRows, err := ssc.APIInfo().Tx.Tx.Query(getTopologyBasedDSesReqCapQuery(), ssc.ServerID, ssc.ServerCapability)
+ if err != nil {
+ return nil, fmt.Errorf("querying topology-based DSes with the required capability %s: %v", *ssc.ServerCapability, err), http.StatusInternalServerError
+ }
+ defer log.Close(dsRows, "closing dsRows in checkTopologyBasedDSRequiredCapabilities")
+
+ xmlidToTopology := make(map[string]string)
+ xmlidToTenantID := make(map[string]int)
+ xmlidToReqCaps := make(map[string][]string)
Review comment:
I suppose keeping `xmlid` all-lowercase makes the structure of these variables more apparent
##########
File path: traffic_ops/traffic_ops_golang/server/servers_server_capability.go
##########
@@ -275,6 +375,43 @@ SELECT ARRAY(
AND dsrc.required_capability = $2)`
}
+// get the topology-based DSes (with all their required capabilities) that a given
+// server is assigned to, filtered by the given capability
+func getTopologyBasedDSesReqCapQuery() string {
+ return `
+SELECT
+ ds.xml_id,
+ ds.topology,
+ ds.tenant_id,
+ array_agg(dsrc.required_capability) as req_caps
+FROM server s
+JOIN cachegroup c ON s.cachegroup = c.id
+JOIN topology_cachegroup tc ON c.name = tc.cachegroup
+JOIN deliveryservice ds ON ds.topology = tc.topology
+JOIN deliveryservices_required_capability dsrc on dsrc.deliveryservice_id = ds.id
+WHERE s.id = $1
+GROUP BY ds.xml_id, ds.tenant_id, ds.topology
+HAVING $2 = ANY(array_agg(dsrc.required_capability))
+`
+}
+
+// get all the capabilities of the servers in a given server's cachegroup
+// that have a given capability
+func getServerCapabilitiesOfCachegoupQuery() string {
+ return `
+SELECT s.id, array_agg(ssc.server_capability) as capabilities
Review comment:
Another case where ARRAY_AGG should be capitalized for readability
##########
File path: traffic_ops/traffic_ops_golang/server/servers_server_capability.go
##########
@@ -275,6 +375,43 @@ SELECT ARRAY(
AND dsrc.required_capability = $2)`
}
+// get the topology-based DSes (with all their required capabilities) that a given
+// server is assigned to, filtered by the given capability
+func getTopologyBasedDSesReqCapQuery() string {
+ return `
+SELECT
+ ds.xml_id,
+ ds.topology,
+ ds.tenant_id,
+ array_agg(dsrc.required_capability) as req_caps
+FROM server s
+JOIN cachegroup c ON s.cachegroup = c.id
+JOIN topology_cachegroup tc ON c.name = tc.cachegroup
+JOIN deliveryservice ds ON ds.topology = tc.topology
+JOIN deliveryservices_required_capability dsrc on dsrc.deliveryservice_id = ds.id
+WHERE s.id = $1
+GROUP BY ds.xml_id, ds.tenant_id, ds.topology
+HAVING $2 = ANY(array_agg(dsrc.required_capability))
+`
+}
+
+// get all the capabilities of the servers in a given server's cachegroup
+// that have a given capability
+func getServerCapabilitiesOfCachegoupQuery() string {
+ return `
+SELECT s.id, array_agg(ssc.server_capability) as capabilities
+FROM server s
+JOIN cachegroup c ON c.id = s.cachegroup
+JOIN server_server_capability ssc ON ssc.server = s.id
+WHERE
+ c.id = (SELECT cachegroup FROM server WHERE server.id = $1)
Review comment:
This condition can instead be a second condition for the JOIN on cachegroup
##########
File path: traffic_ops/traffic_ops_golang/server/servers_server_capability.go
##########
@@ -137,46 +138,145 @@ JOIN server s ON sc.server = s.id ` + where + orderBy + pagination +
}
func (ssc *TOServerServerCapability) Delete() (error, error, int) {
+ tenantIDs, err := tenant.GetUserTenantIDListTx(ssc.APIInfo().Tx.Tx, ssc.APIInfo().User.TenantID)
+ if err != nil {
+ return nil, fmt.Errorf("deleting servers_server_capability: %v", err), http.StatusInternalServerError
+ }
+ accessibleTenants := make(map[int]struct{}, len(tenantIDs))
+ for _, id := range tenantIDs {
+ accessibleTenants[id] = struct{}{}
+ }
+ userErr, sysErr, status := checkTopologyBasedDSRequiredCapabilities(ssc, accessibleTenants)
+ if userErr != nil || sysErr != nil {
+ return userErr, sysErr, status
+ }
+
+ userErr, sysErr, status = checkDSRequiredCapabilities(ssc, accessibleTenants)
+ if userErr != nil || sysErr != nil {
+ return userErr, sysErr, status
+ }
+
+ return api.GenericDelete(ssc)
+}
+
+func checkTopologyBasedDSRequiredCapabilities(ssc *TOServerServerCapability, accessibleTenants map[int]struct{}) (error, error, int) {
+ dsRows, err := ssc.APIInfo().Tx.Tx.Query(getTopologyBasedDSesReqCapQuery(), ssc.ServerID, ssc.ServerCapability)
+ if err != nil {
+ return nil, fmt.Errorf("querying topology-based DSes with the required capability %s: %v", *ssc.ServerCapability, err), http.StatusInternalServerError
+ }
+ defer log.Close(dsRows, "closing dsRows in checkTopologyBasedDSRequiredCapabilities")
+
+ xmlidToTopology := make(map[string]string)
+ xmlidToTenantID := make(map[string]int)
+ xmlidToReqCaps := make(map[string][]string)
+ for dsRows.Next() {
+ xmlID := ""
+ topology := ""
+ tenantID := 0
+ reqCaps := []string{}
Review comment:
These can all be `var` declarations without assignment
##########
File path: traffic_ops/traffic_ops_golang/server/servers_server_capability.go
##########
@@ -275,6 +375,43 @@ SELECT ARRAY(
AND dsrc.required_capability = $2)`
}
+// get the topology-based DSes (with all their required capabilities) that a given
+// server is assigned to, filtered by the given capability
+func getTopologyBasedDSesReqCapQuery() string {
+ return `
+SELECT
+ ds.xml_id,
+ ds.topology,
+ ds.tenant_id,
+ array_agg(dsrc.required_capability) as req_caps
Review comment:
ARRAY_AGG should be capitalized for readability
##########
File path: traffic_ops/traffic_ops_golang/server/servers_server_capability.go
##########
@@ -275,6 +375,43 @@ SELECT ARRAY(
AND dsrc.required_capability = $2)`
}
+// get the topology-based DSes (with all their required capabilities) that a given
+// server is assigned to, filtered by the given capability
+func getTopologyBasedDSesReqCapQuery() string {
+ return `
+SELECT
+ ds.xml_id,
+ ds.topology,
+ ds.tenant_id,
+ array_agg(dsrc.required_capability) as req_caps
+FROM server s
+JOIN cachegroup c ON s.cachegroup = c.id
+JOIN topology_cachegroup tc ON c.name = tc.cachegroup
+JOIN deliveryservice ds ON ds.topology = tc.topology
+JOIN deliveryservices_required_capability dsrc on dsrc.deliveryservice_id = ds.id
+WHERE s.id = $1
+GROUP BY ds.xml_id, ds.tenant_id, ds.topology
+HAVING $2 = ANY(array_agg(dsrc.required_capability))
+`
+}
+
+// get all the capabilities of the servers in a given server's cachegroup
+// that have a given capability
+func getServerCapabilitiesOfCachegoupQuery() string {
+ return `
+SELECT s.id, array_agg(ssc.server_capability) as capabilities
+FROM server s
+JOIN cachegroup c ON c.id = s.cachegroup
+JOIN server_server_capability ssc ON ssc.server = s.id
+WHERE
+ c.id = (SELECT cachegroup FROM server WHERE server.id = $1)
+ AND s.cdn_id = (SELECT cdn_id FROM server WHERE server.id = $1)
+ AND s.id != $1
+GROUP BY s.id
+HAVING $2 = ANY(array_agg(ssc.server_capability));
Review comment:
Here too ARRAY_AGG should be capitalized for readability
##########
File path: traffic_ops/traffic_ops_golang/server/servers_server_capability.go
##########
@@ -137,46 +138,145 @@ JOIN server s ON sc.server = s.id ` + where + orderBy + pagination +
}
func (ssc *TOServerServerCapability) Delete() (error, error, int) {
+ tenantIDs, err := tenant.GetUserTenantIDListTx(ssc.APIInfo().Tx.Tx, ssc.APIInfo().User.TenantID)
+ if err != nil {
+ return nil, fmt.Errorf("deleting servers_server_capability: %v", err), http.StatusInternalServerError
+ }
+ accessibleTenants := make(map[int]struct{}, len(tenantIDs))
+ for _, id := range tenantIDs {
+ accessibleTenants[id] = struct{}{}
+ }
+ userErr, sysErr, status := checkTopologyBasedDSRequiredCapabilities(ssc, accessibleTenants)
+ if userErr != nil || sysErr != nil {
+ return userErr, sysErr, status
+ }
+
+ userErr, sysErr, status = checkDSRequiredCapabilities(ssc, accessibleTenants)
+ if userErr != nil || sysErr != nil {
+ return userErr, sysErr, status
+ }
+
+ return api.GenericDelete(ssc)
+}
+
+func checkTopologyBasedDSRequiredCapabilities(ssc *TOServerServerCapability, accessibleTenants map[int]struct{}) (error, error, int) {
+ dsRows, err := ssc.APIInfo().Tx.Tx.Query(getTopologyBasedDSesReqCapQuery(), ssc.ServerID, ssc.ServerCapability)
+ if err != nil {
+ return nil, fmt.Errorf("querying topology-based DSes with the required capability %s: %v", *ssc.ServerCapability, err), http.StatusInternalServerError
+ }
+ defer log.Close(dsRows, "closing dsRows in checkTopologyBasedDSRequiredCapabilities")
+
+ xmlidToTopology := make(map[string]string)
+ xmlidToTenantID := make(map[string]int)
+ xmlidToReqCaps := make(map[string][]string)
+ for dsRows.Next() {
+ xmlID := ""
+ topology := ""
+ tenantID := 0
+ reqCaps := []string{}
+ if err := dsRows.Scan(&xmlID, &topology, &tenantID, pq.Array(&reqCaps)); err != nil {
+ return nil, fmt.Errorf("scanning dsRows in checkTopologyBasedDSRequiredCapabilities: %v", err), http.StatusInternalServerError
+ }
+ xmlidToTenantID[xmlID] = tenantID
+ xmlidToTopology[xmlID] = topology
+ xmlidToReqCaps[xmlID] = reqCaps
+ }
+ if len(xmlidToTopology) == 0 {
+ return nil, nil, http.StatusOK
+ }
+
+ serverRows, err := ssc.APIInfo().Tx.Tx.Query(getServerCapabilitiesOfCachegoupQuery(), ssc.ServerID, ssc.ServerCapability)
+ if err != nil {
+ return nil, fmt.Errorf("querying server capabilitites of server %d's cachegroup: %v", *ssc.ServerID, err), http.StatusInternalServerError
+ }
+ defer log.Close(serverRows, "closing serverRows in checkTopologyBasedDSRequiredCapabilities")
+
+ serverIDToCapabilities := make(map[int]map[string]struct{})
+ for serverRows.Next() {
+ serverID := 0
+ capabilities := []string{}
Review comment:
These can also be `var` declarations without assignment
##########
File path: traffic_ops/traffic_ops_golang/server/servers_server_capability.go
##########
@@ -275,6 +375,43 @@ SELECT ARRAY(
AND dsrc.required_capability = $2)`
}
+// get the topology-based DSes (with all their required capabilities) that a given
+// server is assigned to, filtered by the given capability
+func getTopologyBasedDSesReqCapQuery() string {
+ return `
+SELECT
+ ds.xml_id,
+ ds.topology,
+ ds.tenant_id,
+ array_agg(dsrc.required_capability) as req_caps
+FROM server s
+JOIN cachegroup c ON s.cachegroup = c.id
+JOIN topology_cachegroup tc ON c.name = tc.cachegroup
+JOIN deliveryservice ds ON ds.topology = tc.topology
+JOIN deliveryservices_required_capability dsrc on dsrc.deliveryservice_id = ds.id
+WHERE s.id = $1
+GROUP BY ds.xml_id, ds.tenant_id, ds.topology
+HAVING $2 = ANY(array_agg(dsrc.required_capability))
Review comment:
ARRAY_AGG should also be capitalized here for readability
##########
File path: traffic_ops/testing/api/v3/withobjs_test.go
##########
@@ -78,38 +80,40 @@ type TCObjFuncs struct {
}
var withFuncs = map[TCObj]TCObjFuncs{
- CacheGroups: {CreateTestCacheGroups, DeleteTestCacheGroups},
- CacheGroupsDeliveryServices: {CreateTestCachegroupsDeliveryServices, DeleteTestCachegroupsDeliveryServices},
- CacheGroupParameters: {CreateTestCacheGroupParameters, DeleteTestCacheGroupParameters},
- CDNs: {CreateTestCDNs, DeleteTestCDNs},
- CDNFederations: {CreateTestCDNFederations, DeleteTestCDNFederations},
- Coordinates: {CreateTestCoordinates, DeleteTestCoordinates},
- DeliveryServices: {CreateTestDeliveryServices, DeleteTestDeliveryServices},
- DeliveryServicesRegexes: {CreateTestDeliveryServicesRegexes, DeleteTestDeliveryServicesRegexes},
- DeliveryServiceRequests: {CreateTestDeliveryServiceRequests, DeleteTestDeliveryServiceRequests},
- DeliveryServiceRequestComments: {CreateTestDeliveryServiceRequestComments, DeleteTestDeliveryServiceRequestComments},
- DeliveryServicesRequiredCapabilities: {CreateTestDeliveryServicesRequiredCapabilities, DeleteTestDeliveryServicesRequiredCapabilities},
- Divisions: {CreateTestDivisions, DeleteTestDivisions},
- FederationUsers: {CreateTestFederationUsers, DeleteTestFederationUsers},
- FederationResolvers: {CreateTestFederationResolvers, DeleteTestFederationResolvers},
- Origins: {CreateTestOrigins, DeleteTestOrigins},
- Parameters: {CreateTestParameters, DeleteTestParameters},
- PhysLocations: {CreateTestPhysLocations, DeleteTestPhysLocations},
- Profiles: {CreateTestProfiles, DeleteTestProfiles},
- ProfileParameters: {CreateTestProfileParameters, DeleteTestProfileParameters},
- Regions: {CreateTestRegions, DeleteTestRegions},
- Roles: {CreateTestRoles, DeleteTestRoles},
- ServerCapabilities: {CreateTestServerCapabilities, DeleteTestServerCapabilities},
- ServerChecks: {CreateTestServerChecks, DeleteTestServerChecks},
- ServerServerCapabilities: {CreateTestServerServerCapabilities, DeleteTestServerServerCapabilities},
- Servers: {CreateTestServers, DeleteTestServers},
- ServiceCategories: {CreateTestServiceCategories, DeleteTestServiceCategories},
- Statuses: {CreateTestStatuses, DeleteTestStatuses},
- StaticDNSEntries: {CreateTestStaticDNSEntries, DeleteTestStaticDNSEntries},
- SteeringTargets: {SetupSteeringTargets, DeleteTestSteeringTargets},
- Tenants: {CreateTestTenants, DeleteTestTenants},
- ServerCheckExtensions: {CreateTestServerCheckExtensions, DeleteTestServerCheckExtensions},
- Topologies: {CreateTestTopologies, DeleteTestTopologies},
- Types: {CreateTestTypes, DeleteTestTypes},
- Users: {CreateTestUsers, ForceDeleteTestUsers},
+ CacheGroups: {CreateTestCacheGroups, DeleteTestCacheGroups},
+ CacheGroupsDeliveryServices: {CreateTestCachegroupsDeliveryServices, DeleteTestCachegroupsDeliveryServices},
+ CacheGroupParameters: {CreateTestCacheGroupParameters, DeleteTestCacheGroupParameters},
+ CDNs: {CreateTestCDNs, DeleteTestCDNs},
+ CDNFederations: {CreateTestCDNFederations, DeleteTestCDNFederations},
+ Coordinates: {CreateTestCoordinates, DeleteTestCoordinates},
+ DeliveryServices: {CreateTestDeliveryServices, DeleteTestDeliveryServices},
+ DeliveryServicesRegexes: {CreateTestDeliveryServicesRegexes, DeleteTestDeliveryServicesRegexes},
+ DeliveryServiceRequests: {CreateTestDeliveryServiceRequests, DeleteTestDeliveryServiceRequests},
+ DeliveryServiceRequestComments: {CreateTestDeliveryServiceRequestComments, DeleteTestDeliveryServiceRequestComments},
+ DeliveryServicesRequiredCapabilities: {CreateTestDeliveryServicesRequiredCapabilities, DeleteTestDeliveryServicesRequiredCapabilities},
+ Divisions: {CreateTestDivisions, DeleteTestDivisions},
+ FederationUsers: {CreateTestFederationUsers, DeleteTestFederationUsers},
+ FederationResolvers: {CreateTestFederationResolvers, DeleteTestFederationResolvers},
+ Origins: {CreateTestOrigins, DeleteTestOrigins},
+ Parameters: {CreateTestParameters, DeleteTestParameters},
+ PhysLocations: {CreateTestPhysLocations, DeleteTestPhysLocations},
+ Profiles: {CreateTestProfiles, DeleteTestProfiles},
+ ProfileParameters: {CreateTestProfileParameters, DeleteTestProfileParameters},
+ Regions: {CreateTestRegions, DeleteTestRegions},
+ Roles: {CreateTestRoles, DeleteTestRoles},
+ ServerCapabilities: {CreateTestServerCapabilities, DeleteTestServerCapabilities},
+ ServerChecks: {CreateTestServerChecks, DeleteTestServerChecks},
+ ServerServerCapabilities: {CreateTestServerServerCapabilities, DeleteTestServerServerCapabilities},
+ ServerServerCapabilitiesForTopologies: {CreateTestServerServerCapabilities, DeleteTestServerServerCapabilitiesForTopologies},
+ Servers: {CreateTestServers, DeleteTestServers},
+ ServiceCategories: {CreateTestServiceCategories, DeleteTestServiceCategories},
+ Statuses: {CreateTestStatuses, DeleteTestStatuses},
+ StaticDNSEntries: {CreateTestStaticDNSEntries, DeleteTestStaticDNSEntries},
+ SteeringTargets: {SetupSteeringTargets, DeleteTestSteeringTargets},
+ Tenants: {CreateTestTenants, DeleteTestTenants},
+ ServerCheckExtensions: {CreateTestServerCheckExtensions, DeleteTestServerCheckExtensions},
+ Topologies: {CreateTestTopologies, DeleteTestTopologies},
+ TopologyBasedDeliveryServiceRequiredCapabilities: {CreateTestTopologyBasedDeliveryServicesRequiredCapabilities, DeleteTestDeliveryServicesRequiredCapabilities},
+ Types: {CreateTestTypes, DeleteTestTypes},
+ Users: {CreateTestUsers, ForceDeleteTestUsers},
Review comment:
Weird what `gofmt` did with this
##########
File path: traffic_ops/traffic_ops_golang/server/servers_server_capability.go
##########
@@ -137,46 +138,145 @@ JOIN server s ON sc.server = s.id ` + where + orderBy + pagination +
}
func (ssc *TOServerServerCapability) Delete() (error, error, int) {
+ tenantIDs, err := tenant.GetUserTenantIDListTx(ssc.APIInfo().Tx.Tx, ssc.APIInfo().User.TenantID)
+ if err != nil {
+ return nil, fmt.Errorf("deleting servers_server_capability: %v", err), http.StatusInternalServerError
+ }
+ accessibleTenants := make(map[int]struct{}, len(tenantIDs))
+ for _, id := range tenantIDs {
+ accessibleTenants[id] = struct{}{}
+ }
+ userErr, sysErr, status := checkTopologyBasedDSRequiredCapabilities(ssc, accessibleTenants)
+ if userErr != nil || sysErr != nil {
+ return userErr, sysErr, status
+ }
+
+ userErr, sysErr, status = checkDSRequiredCapabilities(ssc, accessibleTenants)
+ if userErr != nil || sysErr != nil {
+ return userErr, sysErr, status
+ }
+
+ return api.GenericDelete(ssc)
+}
+
+func checkTopologyBasedDSRequiredCapabilities(ssc *TOServerServerCapability, accessibleTenants map[int]struct{}) (error, error, int) {
+ dsRows, err := ssc.APIInfo().Tx.Tx.Query(getTopologyBasedDSesReqCapQuery(), ssc.ServerID, ssc.ServerCapability)
+ if err != nil {
+ return nil, fmt.Errorf("querying topology-based DSes with the required capability %s: %v", *ssc.ServerCapability, err), http.StatusInternalServerError
+ }
+ defer log.Close(dsRows, "closing dsRows in checkTopologyBasedDSRequiredCapabilities")
+
+ xmlidToTopology := make(map[string]string)
+ xmlidToTenantID := make(map[string]int)
+ xmlidToReqCaps := make(map[string][]string)
+ for dsRows.Next() {
+ xmlID := ""
+ topology := ""
+ tenantID := 0
+ reqCaps := []string{}
+ if err := dsRows.Scan(&xmlID, &topology, &tenantID, pq.Array(&reqCaps)); err != nil {
+ return nil, fmt.Errorf("scanning dsRows in checkTopologyBasedDSRequiredCapabilities: %v", err), http.StatusInternalServerError
+ }
+ xmlidToTenantID[xmlID] = tenantID
+ xmlidToTopology[xmlID] = topology
+ xmlidToReqCaps[xmlID] = reqCaps
+ }
+ if len(xmlidToTopology) == 0 {
+ return nil, nil, http.StatusOK
+ }
+
+ serverRows, err := ssc.APIInfo().Tx.Tx.Query(getServerCapabilitiesOfCachegoupQuery(), ssc.ServerID, ssc.ServerCapability)
+ if err != nil {
+ return nil, fmt.Errorf("querying server capabilitites of server %d's cachegroup: %v", *ssc.ServerID, err), http.StatusInternalServerError
+ }
+ defer log.Close(serverRows, "closing serverRows in checkTopologyBasedDSRequiredCapabilities")
+
+ serverIDToCapabilities := make(map[int]map[string]struct{})
+ for serverRows.Next() {
+ serverID := 0
+ capabilities := []string{}
+ if err := serverRows.Scan(&serverID, pq.Array(&capabilities)); err != nil {
+ return nil, fmt.Errorf("scanning serverRows in checkTopologyBasedDSRequiredCapabilities: %v", err), http.StatusInternalServerError
+ }
+ serverIDToCapabilities[serverID] = make(map[string]struct{})
+ for _, c := range capabilities {
+ serverIDToCapabilities[serverID][c] = struct{}{}
+ }
+ }
+ var dsStrings []string
+ dsStrings = make([]string, 0, len(xmlidToTopology))
+ if len(serverIDToCapabilities) == 0 {
+ for ds, top := range xmlidToTopology {
+ if _, ok := accessibleTenants[xmlidToTenantID[ds]]; ok {
+ dsStrings = append(dsStrings, "(xml_id = "+ds+", topology = "+top+")")
+ }
+ }
+ return fmt.Errorf("this capability is required by delivery services, but there are no other servers in this server's cachegroup to satisfy them %s", strings.Join(dsStrings, ", ")), nil, http.StatusBadRequest
+ }
Review comment:
This case is unnecessary IMO because it saves O(number of delivery services) which is already spent earlier.
----------------------------------------------------------------
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] rob05c commented on a change in pull request #5091: Validate DS Topologies when deleting a server capability
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #5091:
URL: https://github.com/apache/trafficcontrol/pull/5091#discussion_r498450839
##########
File path: traffic_ops/traffic_ops_golang/server/servers_server_capability.go
##########
@@ -137,46 +138,145 @@ JOIN server s ON sc.server = s.id ` + where + orderBy + pagination +
}
func (ssc *TOServerServerCapability) Delete() (error, error, int) {
+ tenantIDs, err := tenant.GetUserTenantIDListTx(ssc.APIInfo().Tx.Tx, ssc.APIInfo().User.TenantID)
+ if err != nil {
+ return nil, fmt.Errorf("deleting servers_server_capability: %v", err), http.StatusInternalServerError
+ }
+ accessibleTenants := make(map[int]struct{}, len(tenantIDs))
+ for _, id := range tenantIDs {
+ accessibleTenants[id] = struct{}{}
+ }
+ userErr, sysErr, status := checkTopologyBasedDSRequiredCapabilities(ssc, accessibleTenants)
+ if userErr != nil || sysErr != nil {
+ return userErr, sysErr, status
+ }
+
+ userErr, sysErr, status = checkDSRequiredCapabilities(ssc, accessibleTenants)
+ if userErr != nil || sysErr != nil {
+ return userErr, sysErr, status
+ }
+
+ return api.GenericDelete(ssc)
+}
+
+func checkTopologyBasedDSRequiredCapabilities(ssc *TOServerServerCapability, accessibleTenants map[int]struct{}) (error, error, int) {
+ dsRows, err := ssc.APIInfo().Tx.Tx.Query(getTopologyBasedDSesReqCapQuery(), ssc.ServerID, ssc.ServerCapability)
+ if err != nil {
+ return nil, fmt.Errorf("querying topology-based DSes with the required capability %s: %v", *ssc.ServerCapability, err), http.StatusInternalServerError
+ }
+ defer log.Close(dsRows, "closing dsRows in checkTopologyBasedDSRequiredCapabilities")
+
+ xmlidToTopology := make(map[string]string)
+ xmlidToTenantID := make(map[string]int)
+ xmlidToReqCaps := make(map[string][]string)
+ for dsRows.Next() {
+ xmlID := ""
+ topology := ""
+ tenantID := 0
+ reqCaps := []string{}
Review comment:
I also like `:=` for that reason, and also to use a unified declaration everywhere, instead of having 2 different ways to declare variables
----------------------------------------------------------------
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 #5091: Validate DS Topologies when deleting a server capability
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5091:
URL: https://github.com/apache/trafficcontrol/pull/5091#discussion_r498452007
##########
File path: traffic_ops/traffic_ops_golang/server/servers_server_capability.go
##########
@@ -137,46 +138,145 @@ JOIN server s ON sc.server = s.id ` + where + orderBy + pagination +
}
func (ssc *TOServerServerCapability) Delete() (error, error, int) {
+ tenantIDs, err := tenant.GetUserTenantIDListTx(ssc.APIInfo().Tx.Tx, ssc.APIInfo().User.TenantID)
+ if err != nil {
+ return nil, fmt.Errorf("deleting servers_server_capability: %v", err), http.StatusInternalServerError
+ }
+ accessibleTenants := make(map[int]struct{}, len(tenantIDs))
+ for _, id := range tenantIDs {
+ accessibleTenants[id] = struct{}{}
+ }
+ userErr, sysErr, status := checkTopologyBasedDSRequiredCapabilities(ssc, accessibleTenants)
+ if userErr != nil || sysErr != nil {
+ return userErr, sysErr, status
+ }
+
+ userErr, sysErr, status = checkDSRequiredCapabilities(ssc, accessibleTenants)
+ if userErr != nil || sysErr != nil {
+ return userErr, sysErr, status
+ }
+
+ return api.GenericDelete(ssc)
+}
+
+func checkTopologyBasedDSRequiredCapabilities(ssc *TOServerServerCapability, accessibleTenants map[int]struct{}) (error, error, int) {
+ dsRows, err := ssc.APIInfo().Tx.Tx.Query(getTopologyBasedDSesReqCapQuery(), ssc.ServerID, ssc.ServerCapability)
+ if err != nil {
+ return nil, fmt.Errorf("querying topology-based DSes with the required capability %s: %v", *ssc.ServerCapability, err), http.StatusInternalServerError
+ }
+ defer log.Close(dsRows, "closing dsRows in checkTopologyBasedDSRequiredCapabilities")
+
+ xmlidToTopology := make(map[string]string)
+ xmlidToTenantID := make(map[string]int)
+ xmlidToReqCaps := make(map[string][]string)
+ for dsRows.Next() {
+ xmlID := ""
+ topology := ""
+ tenantID := 0
+ reqCaps := []string{}
Review comment:
I can see the value in that
----------------------------------------------------------------
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 #5091: Validate DS Topologies when deleting a server capability
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #5091:
URL: https://github.com/apache/trafficcontrol/pull/5091#discussion_r498448116
##########
File path: traffic_ops/traffic_ops_golang/server/servers_server_capability.go
##########
@@ -137,46 +138,145 @@ JOIN server s ON sc.server = s.id ` + where + orderBy + pagination +
}
func (ssc *TOServerServerCapability) Delete() (error, error, int) {
+ tenantIDs, err := tenant.GetUserTenantIDListTx(ssc.APIInfo().Tx.Tx, ssc.APIInfo().User.TenantID)
+ if err != nil {
+ return nil, fmt.Errorf("deleting servers_server_capability: %v", err), http.StatusInternalServerError
+ }
+ accessibleTenants := make(map[int]struct{}, len(tenantIDs))
+ for _, id := range tenantIDs {
+ accessibleTenants[id] = struct{}{}
+ }
+ userErr, sysErr, status := checkTopologyBasedDSRequiredCapabilities(ssc, accessibleTenants)
+ if userErr != nil || sysErr != nil {
+ return userErr, sysErr, status
+ }
+
+ userErr, sysErr, status = checkDSRequiredCapabilities(ssc, accessibleTenants)
+ if userErr != nil || sysErr != nil {
+ return userErr, sysErr, status
+ }
+
+ return api.GenericDelete(ssc)
+}
+
+func checkTopologyBasedDSRequiredCapabilities(ssc *TOServerServerCapability, accessibleTenants map[int]struct{}) (error, error, int) {
+ dsRows, err := ssc.APIInfo().Tx.Tx.Query(getTopologyBasedDSesReqCapQuery(), ssc.ServerID, ssc.ServerCapability)
+ if err != nil {
+ return nil, fmt.Errorf("querying topology-based DSes with the required capability %s: %v", *ssc.ServerCapability, err), http.StatusInternalServerError
+ }
+ defer log.Close(dsRows, "closing dsRows in checkTopologyBasedDSRequiredCapabilities")
+
+ xmlidToTopology := make(map[string]string)
+ xmlidToTenantID := make(map[string]int)
+ xmlidToReqCaps := make(map[string][]string)
+ for dsRows.Next() {
+ xmlID := ""
+ topology := ""
+ tenantID := 0
+ reqCaps := []string{}
Review comment:
I kind of prefer the type inference because I can see the zero-values.
----------------------------------------------------------------
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 #5091: Validate DS Topologies when deleting a server capability
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5091:
URL: https://github.com/apache/trafficcontrol/pull/5091#discussion_r498478136
##########
File path: traffic_ops/testing/api/v3/withobjs_test.go
##########
@@ -78,38 +80,40 @@ type TCObjFuncs struct {
}
var withFuncs = map[TCObj]TCObjFuncs{
- CacheGroups: {CreateTestCacheGroups, DeleteTestCacheGroups},
- CacheGroupsDeliveryServices: {CreateTestCachegroupsDeliveryServices, DeleteTestCachegroupsDeliveryServices},
- CacheGroupParameters: {CreateTestCacheGroupParameters, DeleteTestCacheGroupParameters},
- CDNs: {CreateTestCDNs, DeleteTestCDNs},
- CDNFederations: {CreateTestCDNFederations, DeleteTestCDNFederations},
- Coordinates: {CreateTestCoordinates, DeleteTestCoordinates},
- DeliveryServices: {CreateTestDeliveryServices, DeleteTestDeliveryServices},
- DeliveryServicesRegexes: {CreateTestDeliveryServicesRegexes, DeleteTestDeliveryServicesRegexes},
- DeliveryServiceRequests: {CreateTestDeliveryServiceRequests, DeleteTestDeliveryServiceRequests},
- DeliveryServiceRequestComments: {CreateTestDeliveryServiceRequestComments, DeleteTestDeliveryServiceRequestComments},
- DeliveryServicesRequiredCapabilities: {CreateTestDeliveryServicesRequiredCapabilities, DeleteTestDeliveryServicesRequiredCapabilities},
- Divisions: {CreateTestDivisions, DeleteTestDivisions},
- FederationUsers: {CreateTestFederationUsers, DeleteTestFederationUsers},
- FederationResolvers: {CreateTestFederationResolvers, DeleteTestFederationResolvers},
- Origins: {CreateTestOrigins, DeleteTestOrigins},
- Parameters: {CreateTestParameters, DeleteTestParameters},
- PhysLocations: {CreateTestPhysLocations, DeleteTestPhysLocations},
- Profiles: {CreateTestProfiles, DeleteTestProfiles},
- ProfileParameters: {CreateTestProfileParameters, DeleteTestProfileParameters},
- Regions: {CreateTestRegions, DeleteTestRegions},
- Roles: {CreateTestRoles, DeleteTestRoles},
- ServerCapabilities: {CreateTestServerCapabilities, DeleteTestServerCapabilities},
- ServerChecks: {CreateTestServerChecks, DeleteTestServerChecks},
- ServerServerCapabilities: {CreateTestServerServerCapabilities, DeleteTestServerServerCapabilities},
- Servers: {CreateTestServers, DeleteTestServers},
- ServiceCategories: {CreateTestServiceCategories, DeleteTestServiceCategories},
- Statuses: {CreateTestStatuses, DeleteTestStatuses},
- StaticDNSEntries: {CreateTestStaticDNSEntries, DeleteTestStaticDNSEntries},
- SteeringTargets: {SetupSteeringTargets, DeleteTestSteeringTargets},
- Tenants: {CreateTestTenants, DeleteTestTenants},
- ServerCheckExtensions: {CreateTestServerCheckExtensions, DeleteTestServerCheckExtensions},
- Topologies: {CreateTestTopologies, DeleteTestTopologies},
- Types: {CreateTestTypes, DeleteTestTypes},
- Users: {CreateTestUsers, ForceDeleteTestUsers},
+ CacheGroups: {CreateTestCacheGroups, DeleteTestCacheGroups},
+ CacheGroupsDeliveryServices: {CreateTestCachegroupsDeliveryServices, DeleteTestCachegroupsDeliveryServices},
+ CacheGroupParameters: {CreateTestCacheGroupParameters, DeleteTestCacheGroupParameters},
+ CDNs: {CreateTestCDNs, DeleteTestCDNs},
+ CDNFederations: {CreateTestCDNFederations, DeleteTestCDNFederations},
+ Coordinates: {CreateTestCoordinates, DeleteTestCoordinates},
+ DeliveryServices: {CreateTestDeliveryServices, DeleteTestDeliveryServices},
+ DeliveryServicesRegexes: {CreateTestDeliveryServicesRegexes, DeleteTestDeliveryServicesRegexes},
+ DeliveryServiceRequests: {CreateTestDeliveryServiceRequests, DeleteTestDeliveryServiceRequests},
+ DeliveryServiceRequestComments: {CreateTestDeliveryServiceRequestComments, DeleteTestDeliveryServiceRequestComments},
+ DeliveryServicesRequiredCapabilities: {CreateTestDeliveryServicesRequiredCapabilities, DeleteTestDeliveryServicesRequiredCapabilities},
+ Divisions: {CreateTestDivisions, DeleteTestDivisions},
+ FederationUsers: {CreateTestFederationUsers, DeleteTestFederationUsers},
+ FederationResolvers: {CreateTestFederationResolvers, DeleteTestFederationResolvers},
+ Origins: {CreateTestOrigins, DeleteTestOrigins},
+ Parameters: {CreateTestParameters, DeleteTestParameters},
+ PhysLocations: {CreateTestPhysLocations, DeleteTestPhysLocations},
+ Profiles: {CreateTestProfiles, DeleteTestProfiles},
+ ProfileParameters: {CreateTestProfileParameters, DeleteTestProfileParameters},
+ Regions: {CreateTestRegions, DeleteTestRegions},
+ Roles: {CreateTestRoles, DeleteTestRoles},
+ ServerCapabilities: {CreateTestServerCapabilities, DeleteTestServerCapabilities},
+ ServerChecks: {CreateTestServerChecks, DeleteTestServerChecks},
+ ServerServerCapabilities: {CreateTestServerServerCapabilities, DeleteTestServerServerCapabilities},
+ ServerServerCapabilitiesForTopologies: {CreateTestServerServerCapabilities, DeleteTestServerServerCapabilitiesForTopologies},
+ Servers: {CreateTestServers, DeleteTestServers},
+ ServiceCategories: {CreateTestServiceCategories, DeleteTestServiceCategories},
+ Statuses: {CreateTestStatuses, DeleteTestStatuses},
+ StaticDNSEntries: {CreateTestStaticDNSEntries, DeleteTestStaticDNSEntries},
+ SteeringTargets: {SetupSteeringTargets, DeleteTestSteeringTargets},
+ Tenants: {CreateTestTenants, DeleteTestTenants},
+ ServerCheckExtensions: {CreateTestServerCheckExtensions, DeleteTestServerCheckExtensions},
+ Topologies: {CreateTestTopologies, DeleteTestTopologies},
+ TopologyBasedDeliveryServiceRequiredCapabilities: {CreateTestTopologyBasedDeliveryServicesRequiredCapabilities, DeleteTestDeliveryServicesRequiredCapabilities},
Review comment:
Works for me
----------------------------------------------------------------
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 #5091: Validate DS Topologies when deleting a server capability
Posted by GitBox <gi...@apache.org>.
zrhoffman merged pull request #5091:
URL: https://github.com/apache/trafficcontrol/pull/5091
----------------------------------------------------------------
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 #5091: Validate DS Topologies when deleting a server capability
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #5091:
URL: https://github.com/apache/trafficcontrol/pull/5091#discussion_r498475254
##########
File path: traffic_ops/testing/api/v3/withobjs_test.go
##########
@@ -78,38 +80,40 @@ type TCObjFuncs struct {
}
var withFuncs = map[TCObj]TCObjFuncs{
- CacheGroups: {CreateTestCacheGroups, DeleteTestCacheGroups},
- CacheGroupsDeliveryServices: {CreateTestCachegroupsDeliveryServices, DeleteTestCachegroupsDeliveryServices},
- CacheGroupParameters: {CreateTestCacheGroupParameters, DeleteTestCacheGroupParameters},
- CDNs: {CreateTestCDNs, DeleteTestCDNs},
- CDNFederations: {CreateTestCDNFederations, DeleteTestCDNFederations},
- Coordinates: {CreateTestCoordinates, DeleteTestCoordinates},
- DeliveryServices: {CreateTestDeliveryServices, DeleteTestDeliveryServices},
- DeliveryServicesRegexes: {CreateTestDeliveryServicesRegexes, DeleteTestDeliveryServicesRegexes},
- DeliveryServiceRequests: {CreateTestDeliveryServiceRequests, DeleteTestDeliveryServiceRequests},
- DeliveryServiceRequestComments: {CreateTestDeliveryServiceRequestComments, DeleteTestDeliveryServiceRequestComments},
- DeliveryServicesRequiredCapabilities: {CreateTestDeliveryServicesRequiredCapabilities, DeleteTestDeliveryServicesRequiredCapabilities},
- Divisions: {CreateTestDivisions, DeleteTestDivisions},
- FederationUsers: {CreateTestFederationUsers, DeleteTestFederationUsers},
- FederationResolvers: {CreateTestFederationResolvers, DeleteTestFederationResolvers},
- Origins: {CreateTestOrigins, DeleteTestOrigins},
- Parameters: {CreateTestParameters, DeleteTestParameters},
- PhysLocations: {CreateTestPhysLocations, DeleteTestPhysLocations},
- Profiles: {CreateTestProfiles, DeleteTestProfiles},
- ProfileParameters: {CreateTestProfileParameters, DeleteTestProfileParameters},
- Regions: {CreateTestRegions, DeleteTestRegions},
- Roles: {CreateTestRoles, DeleteTestRoles},
- ServerCapabilities: {CreateTestServerCapabilities, DeleteTestServerCapabilities},
- ServerChecks: {CreateTestServerChecks, DeleteTestServerChecks},
- ServerServerCapabilities: {CreateTestServerServerCapabilities, DeleteTestServerServerCapabilities},
- Servers: {CreateTestServers, DeleteTestServers},
- ServiceCategories: {CreateTestServiceCategories, DeleteTestServiceCategories},
- Statuses: {CreateTestStatuses, DeleteTestStatuses},
- StaticDNSEntries: {CreateTestStaticDNSEntries, DeleteTestStaticDNSEntries},
- SteeringTargets: {SetupSteeringTargets, DeleteTestSteeringTargets},
- Tenants: {CreateTestTenants, DeleteTestTenants},
- ServerCheckExtensions: {CreateTestServerCheckExtensions, DeleteTestServerCheckExtensions},
- Topologies: {CreateTestTopologies, DeleteTestTopologies},
- Types: {CreateTestTypes, DeleteTestTypes},
- Users: {CreateTestUsers, ForceDeleteTestUsers},
+ CacheGroups: {CreateTestCacheGroups, DeleteTestCacheGroups},
+ CacheGroupsDeliveryServices: {CreateTestCachegroupsDeliveryServices, DeleteTestCachegroupsDeliveryServices},
+ CacheGroupParameters: {CreateTestCacheGroupParameters, DeleteTestCacheGroupParameters},
+ CDNs: {CreateTestCDNs, DeleteTestCDNs},
+ CDNFederations: {CreateTestCDNFederations, DeleteTestCDNFederations},
+ Coordinates: {CreateTestCoordinates, DeleteTestCoordinates},
+ DeliveryServices: {CreateTestDeliveryServices, DeleteTestDeliveryServices},
+ DeliveryServicesRegexes: {CreateTestDeliveryServicesRegexes, DeleteTestDeliveryServicesRegexes},
+ DeliveryServiceRequests: {CreateTestDeliveryServiceRequests, DeleteTestDeliveryServiceRequests},
+ DeliveryServiceRequestComments: {CreateTestDeliveryServiceRequestComments, DeleteTestDeliveryServiceRequestComments},
+ DeliveryServicesRequiredCapabilities: {CreateTestDeliveryServicesRequiredCapabilities, DeleteTestDeliveryServicesRequiredCapabilities},
+ Divisions: {CreateTestDivisions, DeleteTestDivisions},
+ FederationUsers: {CreateTestFederationUsers, DeleteTestFederationUsers},
+ FederationResolvers: {CreateTestFederationResolvers, DeleteTestFederationResolvers},
+ Origins: {CreateTestOrigins, DeleteTestOrigins},
+ Parameters: {CreateTestParameters, DeleteTestParameters},
+ PhysLocations: {CreateTestPhysLocations, DeleteTestPhysLocations},
+ Profiles: {CreateTestProfiles, DeleteTestProfiles},
+ ProfileParameters: {CreateTestProfileParameters, DeleteTestProfileParameters},
+ Regions: {CreateTestRegions, DeleteTestRegions},
+ Roles: {CreateTestRoles, DeleteTestRoles},
+ ServerCapabilities: {CreateTestServerCapabilities, DeleteTestServerCapabilities},
+ ServerChecks: {CreateTestServerChecks, DeleteTestServerChecks},
+ ServerServerCapabilities: {CreateTestServerServerCapabilities, DeleteTestServerServerCapabilities},
+ ServerServerCapabilitiesForTopologies: {CreateTestServerServerCapabilities, DeleteTestServerServerCapabilitiesForTopologies},
+ Servers: {CreateTestServers, DeleteTestServers},
+ ServiceCategories: {CreateTestServiceCategories, DeleteTestServiceCategories},
+ Statuses: {CreateTestStatuses, DeleteTestStatuses},
+ StaticDNSEntries: {CreateTestStaticDNSEntries, DeleteTestStaticDNSEntries},
+ SteeringTargets: {SetupSteeringTargets, DeleteTestSteeringTargets},
+ Tenants: {CreateTestTenants, DeleteTestTenants},
+ ServerCheckExtensions: {CreateTestServerCheckExtensions, DeleteTestServerCheckExtensions},
+ Topologies: {CreateTestTopologies, DeleteTestTopologies},
+ TopologyBasedDeliveryServiceRequiredCapabilities: {CreateTestTopologyBasedDeliveryServicesRequiredCapabilities, DeleteTestDeliveryServicesRequiredCapabilities},
Review comment:
While it is deeply regrettable that the columns are no longer fully lined up, I think I prefer reading it unabbreviated (at least when looking at the test file that uses it) since its counterpart (`DeliveryServicesRequiredCapabilities`) is also unabbreviated. When we go full topology, we will be able to remove the `TopologyBased` prefix anyways, and keeping it sticking out like a sore thumb like this is a good reminder to follow through on that.
----------------------------------------------------------------
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 #5091: Validate DS Topologies when deleting a server capability
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #5091:
URL: https://github.com/apache/trafficcontrol/pull/5091#discussion_r498450705
##########
File path: traffic_ops/traffic_ops_golang/server/servers_server_capability.go
##########
@@ -137,46 +138,145 @@ JOIN server s ON sc.server = s.id ` + where + orderBy + pagination +
}
func (ssc *TOServerServerCapability) Delete() (error, error, int) {
+ tenantIDs, err := tenant.GetUserTenantIDListTx(ssc.APIInfo().Tx.Tx, ssc.APIInfo().User.TenantID)
+ if err != nil {
+ return nil, fmt.Errorf("deleting servers_server_capability: %v", err), http.StatusInternalServerError
+ }
+ accessibleTenants := make(map[int]struct{}, len(tenantIDs))
+ for _, id := range tenantIDs {
+ accessibleTenants[id] = struct{}{}
+ }
+ userErr, sysErr, status := checkTopologyBasedDSRequiredCapabilities(ssc, accessibleTenants)
+ if userErr != nil || sysErr != nil {
+ return userErr, sysErr, status
+ }
+
+ userErr, sysErr, status = checkDSRequiredCapabilities(ssc, accessibleTenants)
+ if userErr != nil || sysErr != nil {
+ return userErr, sysErr, status
+ }
+
+ return api.GenericDelete(ssc)
+}
+
+func checkTopologyBasedDSRequiredCapabilities(ssc *TOServerServerCapability, accessibleTenants map[int]struct{}) (error, error, int) {
+ dsRows, err := ssc.APIInfo().Tx.Tx.Query(getTopologyBasedDSesReqCapQuery(), ssc.ServerID, ssc.ServerCapability)
+ if err != nil {
+ return nil, fmt.Errorf("querying topology-based DSes with the required capability %s: %v", *ssc.ServerCapability, err), http.StatusInternalServerError
+ }
+ defer log.Close(dsRows, "closing dsRows in checkTopologyBasedDSRequiredCapabilities")
+
+ xmlidToTopology := make(map[string]string)
+ xmlidToTenantID := make(map[string]int)
+ xmlidToReqCaps := make(map[string][]string)
+ for dsRows.Next() {
+ xmlID := ""
+ topology := ""
+ tenantID := 0
+ reqCaps := []string{}
+ if err := dsRows.Scan(&xmlID, &topology, &tenantID, pq.Array(&reqCaps)); err != nil {
+ return nil, fmt.Errorf("scanning dsRows in checkTopologyBasedDSRequiredCapabilities: %v", err), http.StatusInternalServerError
+ }
+ xmlidToTenantID[xmlID] = tenantID
+ xmlidToTopology[xmlID] = topology
+ xmlidToReqCaps[xmlID] = reqCaps
+ }
+ if len(xmlidToTopology) == 0 {
+ return nil, nil, http.StatusOK
+ }
+
+ serverRows, err := ssc.APIInfo().Tx.Tx.Query(getServerCapabilitiesOfCachegoupQuery(), ssc.ServerID, ssc.ServerCapability)
+ if err != nil {
+ return nil, fmt.Errorf("querying server capabilitites of server %d's cachegroup: %v", *ssc.ServerID, err), http.StatusInternalServerError
+ }
+ defer log.Close(serverRows, "closing serverRows in checkTopologyBasedDSRequiredCapabilities")
+
+ serverIDToCapabilities := make(map[int]map[string]struct{})
+ for serverRows.Next() {
+ serverID := 0
+ capabilities := []string{}
+ if err := serverRows.Scan(&serverID, pq.Array(&capabilities)); err != nil {
+ return nil, fmt.Errorf("scanning serverRows in checkTopologyBasedDSRequiredCapabilities: %v", err), http.StatusInternalServerError
+ }
+ serverIDToCapabilities[serverID] = make(map[string]struct{})
+ for _, c := range capabilities {
+ serverIDToCapabilities[serverID][c] = struct{}{}
+ }
+ }
+ var dsStrings []string
+ dsStrings = make([]string, 0, len(xmlidToTopology))
+ if len(serverIDToCapabilities) == 0 {
+ for ds, top := range xmlidToTopology {
+ if _, ok := accessibleTenants[xmlidToTenantID[ds]]; ok {
+ dsStrings = append(dsStrings, "(xml_id = "+ds+", topology = "+top+")")
+ }
+ }
+ return fmt.Errorf("this capability is required by delivery services, but there are no other servers in this server's cachegroup to satisfy them %s", strings.Join(dsStrings, ", ")), nil, http.StatusBadRequest
+ }
Review comment:
I think this was necessary in an earlier version, but re-reading my code here I believe I agree with you. I'll remove it in the next revision.
----------------------------------------------------------------
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 #5091: Validate DS Topologies when deleting a server capability
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5091:
URL: https://github.com/apache/trafficcontrol/pull/5091#discussion_r498469061
##########
File path: traffic_ops/testing/api/v3/withobjs_test.go
##########
@@ -78,38 +80,40 @@ type TCObjFuncs struct {
}
var withFuncs = map[TCObj]TCObjFuncs{
- CacheGroups: {CreateTestCacheGroups, DeleteTestCacheGroups},
- CacheGroupsDeliveryServices: {CreateTestCachegroupsDeliveryServices, DeleteTestCachegroupsDeliveryServices},
- CacheGroupParameters: {CreateTestCacheGroupParameters, DeleteTestCacheGroupParameters},
- CDNs: {CreateTestCDNs, DeleteTestCDNs},
- CDNFederations: {CreateTestCDNFederations, DeleteTestCDNFederations},
- Coordinates: {CreateTestCoordinates, DeleteTestCoordinates},
- DeliveryServices: {CreateTestDeliveryServices, DeleteTestDeliveryServices},
- DeliveryServicesRegexes: {CreateTestDeliveryServicesRegexes, DeleteTestDeliveryServicesRegexes},
- DeliveryServiceRequests: {CreateTestDeliveryServiceRequests, DeleteTestDeliveryServiceRequests},
- DeliveryServiceRequestComments: {CreateTestDeliveryServiceRequestComments, DeleteTestDeliveryServiceRequestComments},
- DeliveryServicesRequiredCapabilities: {CreateTestDeliveryServicesRequiredCapabilities, DeleteTestDeliveryServicesRequiredCapabilities},
- Divisions: {CreateTestDivisions, DeleteTestDivisions},
- FederationUsers: {CreateTestFederationUsers, DeleteTestFederationUsers},
- FederationResolvers: {CreateTestFederationResolvers, DeleteTestFederationResolvers},
- Origins: {CreateTestOrigins, DeleteTestOrigins},
- Parameters: {CreateTestParameters, DeleteTestParameters},
- PhysLocations: {CreateTestPhysLocations, DeleteTestPhysLocations},
- Profiles: {CreateTestProfiles, DeleteTestProfiles},
- ProfileParameters: {CreateTestProfileParameters, DeleteTestProfileParameters},
- Regions: {CreateTestRegions, DeleteTestRegions},
- Roles: {CreateTestRoles, DeleteTestRoles},
- ServerCapabilities: {CreateTestServerCapabilities, DeleteTestServerCapabilities},
- ServerChecks: {CreateTestServerChecks, DeleteTestServerChecks},
- ServerServerCapabilities: {CreateTestServerServerCapabilities, DeleteTestServerServerCapabilities},
- Servers: {CreateTestServers, DeleteTestServers},
- ServiceCategories: {CreateTestServiceCategories, DeleteTestServiceCategories},
- Statuses: {CreateTestStatuses, DeleteTestStatuses},
- StaticDNSEntries: {CreateTestStaticDNSEntries, DeleteTestStaticDNSEntries},
- SteeringTargets: {SetupSteeringTargets, DeleteTestSteeringTargets},
- Tenants: {CreateTestTenants, DeleteTestTenants},
- ServerCheckExtensions: {CreateTestServerCheckExtensions, DeleteTestServerCheckExtensions},
- Topologies: {CreateTestTopologies, DeleteTestTopologies},
- Types: {CreateTestTypes, DeleteTestTypes},
- Users: {CreateTestUsers, ForceDeleteTestUsers},
+ CacheGroups: {CreateTestCacheGroups, DeleteTestCacheGroups},
+ CacheGroupsDeliveryServices: {CreateTestCachegroupsDeliveryServices, DeleteTestCachegroupsDeliveryServices},
+ CacheGroupParameters: {CreateTestCacheGroupParameters, DeleteTestCacheGroupParameters},
+ CDNs: {CreateTestCDNs, DeleteTestCDNs},
+ CDNFederations: {CreateTestCDNFederations, DeleteTestCDNFederations},
+ Coordinates: {CreateTestCoordinates, DeleteTestCoordinates},
+ DeliveryServices: {CreateTestDeliveryServices, DeleteTestDeliveryServices},
+ DeliveryServicesRegexes: {CreateTestDeliveryServicesRegexes, DeleteTestDeliveryServicesRegexes},
+ DeliveryServiceRequests: {CreateTestDeliveryServiceRequests, DeleteTestDeliveryServiceRequests},
+ DeliveryServiceRequestComments: {CreateTestDeliveryServiceRequestComments, DeleteTestDeliveryServiceRequestComments},
+ DeliveryServicesRequiredCapabilities: {CreateTestDeliveryServicesRequiredCapabilities, DeleteTestDeliveryServicesRequiredCapabilities},
+ Divisions: {CreateTestDivisions, DeleteTestDivisions},
+ FederationUsers: {CreateTestFederationUsers, DeleteTestFederationUsers},
+ FederationResolvers: {CreateTestFederationResolvers, DeleteTestFederationResolvers},
+ Origins: {CreateTestOrigins, DeleteTestOrigins},
+ Parameters: {CreateTestParameters, DeleteTestParameters},
+ PhysLocations: {CreateTestPhysLocations, DeleteTestPhysLocations},
+ Profiles: {CreateTestProfiles, DeleteTestProfiles},
+ ProfileParameters: {CreateTestProfileParameters, DeleteTestProfileParameters},
+ Regions: {CreateTestRegions, DeleteTestRegions},
+ Roles: {CreateTestRoles, DeleteTestRoles},
+ ServerCapabilities: {CreateTestServerCapabilities, DeleteTestServerCapabilities},
+ ServerChecks: {CreateTestServerChecks, DeleteTestServerChecks},
+ ServerServerCapabilities: {CreateTestServerServerCapabilities, DeleteTestServerServerCapabilities},
+ ServerServerCapabilitiesForTopologies: {CreateTestServerServerCapabilities, DeleteTestServerServerCapabilitiesForTopologies},
+ Servers: {CreateTestServers, DeleteTestServers},
+ ServiceCategories: {CreateTestServiceCategories, DeleteTestServiceCategories},
+ Statuses: {CreateTestStatuses, DeleteTestStatuses},
+ StaticDNSEntries: {CreateTestStaticDNSEntries, DeleteTestStaticDNSEntries},
+ SteeringTargets: {SetupSteeringTargets, DeleteTestSteeringTargets},
+ Tenants: {CreateTestTenants, DeleteTestTenants},
+ ServerCheckExtensions: {CreateTestServerCheckExtensions, DeleteTestServerCheckExtensions},
+ Topologies: {CreateTestTopologies, DeleteTestTopologies},
+ TopologyBasedDeliveryServiceRequiredCapabilities: {CreateTestTopologyBasedDeliveryServicesRequiredCapabilities, DeleteTestDeliveryServicesRequiredCapabilities},
Review comment:
Nit: If this `TCObj` identifier is 40 characters or less (like `TopologyBasedDeliveryServiceRequiredCaps` or `TopologyBasedDeliverySvcReqdCapabilities`), `go fmt` will align the `TCObjFuncs`. Hard to say if that makes it more or less readable overall, I'll leave that call up to you.
----------------------------------------------------------------
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