This is an automated email from the ASF dual-hosted git repository. ocket8888 pushed a commit to branch 5.0.x in repository https://gitbox.apache.org/repos/asf/trafficcontrol.git
commit 3d0a90f9918dfa1a0158a3e04da3a2440197555c Author: Rawlin Peters <[email protected]> AuthorDate: Wed Nov 18 06:47:14 2020 -0700 Add validation to topology updates and server updates/deletions (#5299) Topologies must contain at least 1 server per cachegroup in each of the CDNs of any assigned delivery services. Otherwise, a delivery service's topology is not truly representative of reality. (cherry picked from commit be43e76bef2e60b2a5df2049ab8633f035ecd995) --- CHANGELOG.md | 1 + traffic_ops/testing/api/v3/servers_test.go | 63 +++- traffic_ops/testing/api/v3/tc-fixtures.json | 335 +++++++++++++++++++++ traffic_ops/testing/api/v3/topologies_test.go | 66 +++- .../traffic_ops_golang/dbhelpers/db_helpers.go | 45 +++ traffic_ops/traffic_ops_golang/server/servers.go | 64 ++-- .../traffic_ops_golang/topology/topologies.go | 95 ++++-- 7 files changed, 602 insertions(+), 67 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 07861c4..1d6f591 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -67,6 +67,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/). - Added default sort logic to GET API calls using Read() - Traffic Ops: added validation for assigning ORG servers to topology-based delivery services - Added locationByDeepCoverageZone to the `crs/stats/ip/{ip}` endpoint in the Traffic Router API +- Traffic Ops: added validation for topology updates and server updates/deletions to ensure that topologies have at least one server per cachegroup in each CDN of any assigned delivery services ### Fixed - Fixed #5188 - DSR (delivery service request) incorrectly marked as complete and error message not displaying when DSR fulfilled and DS update fails in Traffic Portal. [Related Github issue](https://github.com/apache/trafficcontrol/issues/5188) diff --git a/traffic_ops/testing/api/v3/servers_test.go b/traffic_ops/testing/api/v3/servers_test.go index 1af5300..9d0d4d1 100644 --- a/traffic_ops/testing/api/v3/servers_test.go +++ b/traffic_ops/testing/api/v3/servers_test.go @@ -60,16 +60,23 @@ func LastServerInTopologyCacheGroup(t *testing.T) { const cacheGroupName = "topology-mid-cg-01" const moveToCacheGroup = "topology-mid-cg-02" const topologyName = "forked-topology" + const cdnName = "cdn2" const expectedLength = 1 + cdns, _, err := TOSession.GetCDNByNameWithHdr(cdnName, nil) + if err != nil { + t.Fatalf("unable to GET CDN: %v", err) + } + cdnID := cdns[0].ID params := url.Values{} params.Add("cachegroupName", cacheGroupName) params.Add("topology", topologyName) + params.Add("cdn", strconv.Itoa(cdnID)) servers, _, err := TOSession.GetServersWithHdr(¶ms, nil) if err != nil { - t.Fatalf("getting server from cachegroup %s in topology %s: %s", cacheGroupName, topologyName, err.Error()) + t.Fatalf("getting server from cdn %s from cachegroup %s in topology %s: %s", cdnName, cacheGroupName, topologyName, err.Error()) } if len(servers.Response) != expectedLength { - t.Fatalf("expected to get %d server from cachegroup %s in topology %s, got %d servers", expectedLength, cacheGroupName, topologyName, len(servers.Response)) + t.Fatalf("expected to get %d server from cdn %s from cachegroup %s in topology %s, got %d servers", expectedLength, cdnName, cacheGroupName, topologyName, len(servers.Response)) } server := servers.Response[0] _, reqInf, err := TOSession.DeleteServerByID(*server.ID) @@ -80,6 +87,28 @@ func LastServerInTopologyCacheGroup(t *testing.T) { t.Fatalf("expected a 400-level error deleting server with id %d, got status code %d: %s", *server.ID, reqInf.StatusCode, err.Error()) } + // attempt to move it to another CDN while it's the last server in the cachegroup in its CDN + cdns, _, err = TOSession.GetCDNByNameWithHdr("cdn1", nil) + if err != nil { + t.Fatalf("unable to GET CDN: %v", err) + } + newCDNID := cdns[0].ID + oldCDNID := *server.CDNID + server.CDNID = &newCDNID + profiles, _, err := TOSession.GetProfileByNameWithHdr("MID1", nil) + if err != nil { + t.Fatalf("unable to GET profile: %v", err) + } + newProfile := profiles[0].ID + oldProfile := *server.ProfileID + server.ProfileID = &newProfile + _, _, err = TOSession.UpdateServerByIDWithHdr(*server.ID, server, nil) + if err == nil { + t.Fatalf("changing the CDN of the last server (%s) in a CDN in a cachegroup used by a topology assigned to a delivery service(s) in that CDN - expected: error, actual: nil", *server.HostName) + } + server.CDNID = &oldCDNID + server.ProfileID = &oldProfile + params = url.Values{} params.Add("name", moveToCacheGroup) cgs, _, err := TOSession.GetCacheGroupsByQueryParamsWithHdr(params, nil) @@ -548,10 +577,11 @@ func GetTestServersQueryParameters(t *testing.T) { params.Set("dsId", strconv.Itoa(*ds.ID)) expectedHostnames := map[string]bool{ - "edge1-cdn1-cg3": false, - "edge2-cdn1-cg3": false, - "atlanta-mid-16": false, - "edgeInCachegroup3": false, + "edge1-cdn1-cg3": false, + "edge2-cdn1-cg3": false, + "atlanta-mid-16": false, + "edgeInCachegroup3": false, + "midInSecondaryCachegroupInCDN1": false, } response, _, err := TOSession.GetServersWithHdr(¶ms, nil) if err != nil { @@ -562,7 +592,7 @@ func GetTestServersQueryParameters(t *testing.T) { } for _, server := range response.Response { if _, exists := expectedHostnames[*server.HostName]; !exists { - t.Fatalf("expected hostnames %v, actual %s actual: ", expectedHostnames, *server.HostName) + t.Fatalf("expected hostnames %v, actual %s", expectedHostnames, *server.HostName) } expectedHostnames[*server.HostName] = true } @@ -600,14 +630,15 @@ func GetTestServersQueryParameters(t *testing.T) { params.Del("dsId") params.Add("topology", topology) expectedHostnames = map[string]bool{ - originHostname: false, - "edge1-cdn1-cg3": false, - "edge2-cdn1-cg3": false, - "atlanta-mid-16": false, - "atlanta-mid-17": false, - "edgeInCachegroup3": false, - "midInParentCachegroup": false, - "midInSecondaryCachegroup": false, + originHostname: false, + "edge1-cdn1-cg3": false, + "edge2-cdn1-cg3": false, + "atlanta-mid-16": false, + "atlanta-mid-17": false, + "edgeInCachegroup3": false, + "midInParentCachegroup": false, + "midInSecondaryCachegroup": false, + "midInSecondaryCachegroupInCDN1": false, } response, _, err = TOSession.GetServersWithHdr(¶ms, nil) if err != nil { @@ -618,7 +649,7 @@ func GetTestServersQueryParameters(t *testing.T) { } for _, server := range response.Response { if _, exists := expectedHostnames[*server.HostName]; !exists { - t.Fatalf("expected hostnames %v, actual %s actual: ", expectedHostnames, *server.HostName) + t.Fatalf("expected hostnames %v, actual %s", expectedHostnames, *server.HostName) } expectedHostnames[*server.HostName] = true } diff --git a/traffic_ops/testing/api/v3/tc-fixtures.json b/traffic_ops/testing/api/v3/tc-fixtures.json index 30c811f..796c630 100644 --- a/traffic_ops/testing/api/v3/tc-fixtures.json +++ b/traffic_ops/testing/api/v3/tc-fixtures.json @@ -222,6 +222,13 @@ "name": "dtrc3", "shortName": "dtrc3", "typeName": "EDGE_LOC" + }, + { + "latitude": 0, + "longitude": 0, + "name": "cdn1-only", + "shortName": "cdn1-only", + "typeName": "EDGE_LOC" } ], "cdns": [ @@ -1066,6 +1073,198 @@ "type": "CLIENT_STEERING", "xmlId": "ds-client-steering", "anonymousBlockingEnabled": false + }, + { + "active": true, + "cdnName": "cdn2", + "cacheurl": "", + "ccrDnsTtl": 3600, + "checkPath": "", + "consistentHashQueryParams": [], + "deepCachingType": "NEVER", + "displayName": "ds-forked-topology", + "dnsBypassCname": null, + "dnsBypassIp": "", + "dnsBypassIp6": "", + "dnsBypassTtl": 30, + "dscp": 40, + "edgeHeaderRewrite": null, + "fqPacingRate": 0, + "geoLimit": 0, + "geoLimitCountries": "", + "geoLimitRedirectURL": null, + "geoProvider": 0, + "globalMaxMbps": 0, + "globalMaxTps": 0, + "httpBypassFqdn": "", + "infoUrl": "TBD", + "initialDispersion": 1, + "ipv6RoutingEnabled": true, + "lastUpdated": "2018-04-06 16:48:51+00", + "logsEnabled": false, + "longDesc": "", + "longDesc1": "", + "longDesc2": "", + "matchList": [ + { + "pattern": ".*\\.ds-forked-topology\\..*", + "setNumber": 0, + "type": "HOST_REGEXP" + } + ], + "maxDnsAnswers": 0, + "midHeaderRewrite": null, + "missLat": 41.881944, + "missLong": -87.627778, + "multiSiteOrigin": false, + "orgServerFqdn": "http://example.org";, + "originShield": null, + "profileDescription": null, + "profileName": null, + "protocol": 0, + "qstringIgnore": 0, + "rangeRequestHandling": 0, + "regexRemap": null, + "regionalGeoBlocking": false, + "remapText": null, + "routingName": "cdn", + "signed": false, + "signingAlgorithm": null, + "sslKeyVersion": 0, + "tenant": "tenant1", + "tenantName": "tenant1", + "topology": "forked-topology", + "type": "HTTP", + "xmlId": "ds-forked-topology", + "anonymousBlockingEnabled": false + }, + { + "active": true, + "cdnName": "cdn1", + "cacheurl": "", + "ccrDnsTtl": 3600, + "checkPath": "", + "consistentHashQueryParams": [], + "deepCachingType": "NEVER", + "displayName": "top-ds-in-cdn1", + "dnsBypassCname": null, + "dnsBypassIp": "", + "dnsBypassIp6": "", + "dnsBypassTtl": 30, + "dscp": 40, + "edgeHeaderRewrite": null, + "fqPacingRate": 0, + "geoLimit": 0, + "geoLimitCountries": "", + "geoLimitRedirectURL": null, + "geoProvider": 0, + "globalMaxMbps": 0, + "globalMaxTps": 0, + "httpBypassFqdn": "", + "infoUrl": "TBD", + "initialDispersion": 1, + "ipv6RoutingEnabled": true, + "lastUpdated": "2018-04-06 16:48:51+00", + "logsEnabled": false, + "longDesc": "", + "longDesc1": "", + "longDesc2": "", + "matchList": [ + { + "pattern": ".*\\.top-ds-in-cdn1\\..*", + "setNumber": 0, + "type": "HOST_REGEXP" + } + ], + "maxDnsAnswers": 0, + "midHeaderRewrite": null, + "missLat": 41.881944, + "missLong": -87.627778, + "multiSiteOrigin": false, + "orgServerFqdn": "http://example.org";, + "originShield": null, + "profileDescription": null, + "profileName": null, + "protocol": 0, + "qstringIgnore": 0, + "rangeRequestHandling": 0, + "regexRemap": null, + "regionalGeoBlocking": false, + "remapText": null, + "routingName": "cdn", + "signed": false, + "signingAlgorithm": null, + "sslKeyVersion": 0, + "tenant": "tenant1", + "tenantName": "tenant1", + "topology": "top-used-by-cdn1-and-cdn2", + "type": "HTTP", + "xmlId": "top-ds-in-cdn1", + "anonymousBlockingEnabled": false + }, + { + "active": true, + "cdnName": "cdn2", + "cacheurl": "", + "ccrDnsTtl": 3600, + "checkPath": "", + "consistentHashQueryParams": [], + "deepCachingType": "NEVER", + "displayName": "top-ds-in-cdn2", + "dnsBypassCname": null, + "dnsBypassIp": "", + "dnsBypassIp6": "", + "dnsBypassTtl": 30, + "dscp": 40, + "edgeHeaderRewrite": null, + "fqPacingRate": 0, + "geoLimit": 0, + "geoLimitCountries": "", + "geoLimitRedirectURL": null, + "geoProvider": 0, + "globalMaxMbps": 0, + "globalMaxTps": 0, + "httpBypassFqdn": "", + "infoUrl": "TBD", + "initialDispersion": 1, + "ipv6RoutingEnabled": true, + "lastUpdated": "2018-04-06 16:48:51+00", + "logsEnabled": false, + "longDesc": "", + "longDesc1": "", + "longDesc2": "", + "matchList": [ + { + "pattern": ".*\\.top-ds-in-cdn2\\..*", + "setNumber": 0, + "type": "HOST_REGEXP" + } + ], + "maxDnsAnswers": 0, + "midHeaderRewrite": null, + "missLat": 41.881944, + "missLong": -87.627778, + "multiSiteOrigin": false, + "orgServerFqdn": "http://example.org";, + "originShield": null, + "profileDescription": null, + "profileName": null, + "protocol": 0, + "qstringIgnore": 0, + "rangeRequestHandling": 0, + "regexRemap": null, + "regionalGeoBlocking": false, + "remapText": null, + "routingName": "cdn", + "signed": false, + "signingAlgorithm": null, + "sslKeyVersion": 0, + "tenant": "tenant1", + "tenantName": "tenant1", + "topology": "top-used-by-cdn1-and-cdn2", + "type": "HTTP", + "xmlId": "top-ds-in-cdn2", + "anonymousBlockingEnabled": false } ], "deliveryServicesRegexes": [ @@ -3426,6 +3625,90 @@ "updPending": false }, { + "cachegroup": "secondaryCachegroup", + "cdnName": "cdn1", + "domainName": "kabletown.net", + "hostName": "midInSecondaryCachegroupInCDN1", + "httpsPort": 443, + "interfaces": [ + { + "ipAddresses": [ + { + "address": "2001:db8:deed:beef::12/64", + "gateway": "2001:db8:deed:beef::1", + "serviceAddress": false + }, + { + "address": "192.0.7.15/24", + "gateway": "192.0.7.1", + "serviceAddress": true + } + ], + "monitor": true, + "mtu": 9000, + "name": "bond0" + } + ], + "physLocation": "Denver", + "profile": "MID1", + "rack": "RR 119.02", + "revalPending": false, + "status": "REPORTED", + "tcpPort": 80, + "type": "MID", + "updPending": false + }, + { + "cachegroup": "cdn1-only", + "cdnName": "cdn1", + "domainName": "foo.kabletown.net", + "guid": null, + "hostName": "edge-in-cdn1-only", + "httpsPort": 443, + "iloIpAddress": "", + "iloIpGateway": "", + "iloIpNetmask": "", + "iloPassword": "", + "iloUsername": "", + "interfaces": [ + { + "ipAddresses": [ + { + "address": "192.0.2.88/24", + "gateway": "192.0.2.1", + "serviceAddress": true + }, + { + "address": "2001:db8:f33d:beef::2/64", + "gateway": "2001:db8:f33d:beef::1", + "serviceAddress": false + } + ], + "maxBandwidth": null, + "monitor": true, + "mtu": 9000, + "name": "bond0" + } + ], + "lastUpdated": "2018-03-28T17:30:00.220351+00:00", + "mgmtIpAddress": "", + "mgmtIpGateway": "", + "mgmtIpNetmask": "", + "offlineReason": null, + "physLocation": "Denver", + "profile": "EDGE1", + "rack": "RR 119.02", + "revalPending": false, + "routerHostName": "", + "routerPortName": "", + "status": "REPORTED", + "tcpPort": 80, + "type": "EDGE", + "updPending": false, + "xmppId": "", + "xmppPasswd": "" + }, + { "cachegroup": "fallback1", "cdnName": "cdn2", "domainName": "kabletown.net", @@ -3630,6 +3913,40 @@ "updPending": false }, { + "cachegroup": "topology-mid-cg-01", + "cdnName": "cdn1", + "domainName": "kabletown.net", + "hostName": "midInTopologyMidCg01InCDN1", + "httpsPort": 443, + "interfaces": [ + { + "ipAddresses": [ + { + "address": "2001:db8:de4d:beef::12/64", + "gateway": "2001:db8:de4d:beef::1", + "serviceAddress": false + }, + { + "address": "192.0.12.21/24", + "gateway": "192.0.12.1", + "serviceAddress": true + } + ], + "monitor": true, + "mtu": 9000, + "name": "bond0" + } + ], + "physLocation": "Denver", + "profile": "MID1", + "rack": "RR 119.02", + "revalPending": false, + "status": "REPORTED", + "tcpPort": 80, + "type": "MID", + "updPending": false + }, + { "cachegroup": "topology-mid-cg-02", "cdnName": "cdn2", "domainName": "kabletown.net", @@ -4230,6 +4547,24 @@ "parents": [1] } ] + }, + { + "name": "top-used-by-cdn1-and-cdn2", + "description": "a topology", + "nodes": [ + { + "cachegroup": "dtrc1", + "parents": [] + }, + { + "cachegroup": "dtrc2", + "parents": [0] + }, + { + "cachegroup": "dtrc3", + "parents": [0] + } + ] } ], "types": [ diff --git a/traffic_ops/testing/api/v3/topologies_test.go b/traffic_ops/testing/api/v3/topologies_test.go index 2b43a31..4350c8e 100644 --- a/traffic_ops/testing/api/v3/topologies_test.go +++ b/traffic_ops/testing/api/v3/topologies_test.go @@ -21,7 +21,9 @@ package v3 import ( "fmt" + "net/url" "reflect" + "strconv" "strings" "testing" @@ -164,14 +166,6 @@ func updateSingleTopology(topology tc.Topology) error { } func UpdateTestTopologies(t *testing.T) { - firstTopName := testData.Topologies[0].Name - for _, top := range testData.Topologies { - top.Name = firstTopName - if err := updateSingleTopology(top); err != nil { - t.Fatalf(err.Error()) - } - } - // Revert test topologies for _, topology := range testData.Topologies { if err := updateSingleTopology(topology); err != nil { t.Fatalf(err.Error()) @@ -188,6 +182,62 @@ func UpdateTestTopologies(t *testing.T) { if err == nil { t.Errorf("making invalid update to topology - expected: error, actual: nil") } + + // attempt to add a cachegroup that only has caches in one CDN while the topology is assigned to DSes from multiple CDNs + top, _, err = TOSession.GetTopologyWithHdr("top-used-by-cdn1-and-cdn2", nil) + if err != nil { + t.Fatalf("cannot GET topology: %v", err) + } + params := url.Values{} + params.Add("topology", "top-used-by-cdn1-and-cdn2") + dses, _, err := TOSession.GetDeliveryServicesV30WithHdr(nil, params) + if err != nil { + t.Fatalf("cannot GET delivery services: %v", err) + } + if len(dses) < 2 { + t.Fatalf("expected at least 2 delivery services assigned to topology top-used-by-cdn1-and-cdn2, actual: %d", len(dses)) + } + foundCDN1 := false + foundCDN2 := false + for _, ds := range dses { + if *ds.CDNName == "cdn1" { + foundCDN1 = true + } else if *ds.CDNName == "cdn2" { + foundCDN2 = true + } + } + if !foundCDN1 || !foundCDN2 { + t.Fatalf("expected delivery services assigned to topology top-used-by-cdn1-and-cdn2 to be assigned to cdn1 and cdn2") + } + cgs, _, err := TOSession.GetCacheGroupNullableByNameWithHdr("cdn1-only", nil) + if err != nil { + t.Fatalf("unable to GET cachegroup by name: %v", err) + } + if len(cgs) != 1 { + t.Fatalf("expected: to get 1 cachegroup named 'cdn1-only', actual: got %d", len(cgs)) + } + params = url.Values{} + params.Add("cachegroup", strconv.Itoa(*cgs[0].ID)) + servers, _, err := TOSession.GetServersWithHdr(¶ms, nil) + if err != nil { + t.Fatalf("unable to GET servers by cachegroup: %v", err) + } + for _, s := range servers.Response { + if *s.Cachegroup != "cdn1-only" { + t.Fatalf("GET servers by cachegroup 'cdn1-only' - expected: only servers in cachegroup 'cdn1-only', actual: got server in %s", *s.Cachegroup) + } + if *s.CDNName != "cdn1" { + t.Fatalf("expected: servers in cachegroup 'cdn1-only' to only be in cdn1, actual: servers in cdn %s", *s.CDNName) + } + } + top.Nodes = append(top.Nodes, tc.TopologyNode{ + Cachegroup: "cdn1-only", + Parents: []int{0}, + }) + _, _, err = TOSession.UpdateTopology(top.Name, *top) + if err == nil { + t.Errorf("making invalid update to topology (cachegroup contains only servers from cdn1 while the topology is assigned to delivery services in cdn1 and cdn2) - expected: error, actual: nil") + } } func DeleteTestTopologies(t *testing.T) { diff --git a/traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go b/traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go index 03ac0a8..53ce367 100644 --- a/traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go +++ b/traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go @@ -881,6 +881,51 @@ WHERE return dses, nil } +// GetDeliveryServiceCDNsByTopology returns a slice of CDN IDs for all delivery services +// assigned to the given topology. +func GetDeliveryServiceCDNsByTopology(tx *sql.Tx, topology string) ([]int, error) { + q := ` +SELECT + COALESCE(ARRAY_AGG(DISTINCT d.cdn_id), '{}'::BIGINT[]) +FROM + deliveryservice d +WHERE + d.topology = $1 +` + cdnIDs := []int64{} + if err := tx.QueryRow(q, topology).Scan(pq.Array(&cdnIDs)); err != nil { + return nil, fmt.Errorf("in GetDeliveryServiceCDNsByTopology: querying deliveryservices by topology '%s': %v", topology, err) + } + res := make([]int, len(cdnIDs)) + for i, id := range cdnIDs { + res[i] = int(id) + } + return res, nil +} + +// CheckCachegroupHasTopologyBasedDeliveryServicesOnCDN returns true if the given cachegroup is assigned to +// any topologies with delivery services assigned on the given CDN. +func CachegroupHasTopologyBasedDeliveryServicesOnCDN(tx *sql.Tx, cachegroupID int, CDNID int) (bool, error) { + q := ` +SELECT EXISTS( + SELECT + 1 + FROM cachegroup c + JOIN topology_cachegroup tc on c.name = tc.cachegroup + JOIN topology t ON tc.topology = t.name + JOIN deliveryservice d on t.name = d.topology + WHERE + c.id = $1 + AND d.cdn_id = $2 +) +` + res := false + if err := tx.QueryRow(q, cachegroupID, CDNID).Scan(&res); err != nil { + return false, fmt.Errorf("in CachegroupHasTopologyBasedDeliveryServicesOnCDN: %v", err) + } + return res, nil +} + // GetFederationIDForUserIDByXMLID retrieves the ID of the Federation assigned to the user defined by // userID on the Delivery Service identified by xmlid. If no such federation exists, the boolean // returned will be 'false', while the error indicates unexpected errors that occurred when querying. diff --git a/traffic_ops/traffic_ops_golang/server/servers.go b/traffic_ops/traffic_ops_golang/server/servers.go index 8a0a2b1..369dee9 100644 --- a/traffic_ops/traffic_ops_golang/server/servers.go +++ b/traffic_ops/traffic_ops_golang/server/servers.go @@ -26,7 +26,6 @@ import ( "encoding/json" "errors" "fmt" - "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/topology" "net" "net/http" "strconv" @@ -43,6 +42,7 @@ import ( "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/deliveryservice" "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/routing/middleware" "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/tenant" + "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/topology" "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/util/ims" validation "github.com/go-ozzo/ozzo-validation" @@ -1194,14 +1194,15 @@ func Update(w http.ResponseWriter, r *http.Request) { api.HandleErr(w, r, tx, http.StatusNotFound, errors.New("the server doesn't exist, cannot update"), nil) return } + origServer := origSer[0] originalXMPPID := "" originalStatusID := 0 changeXMPPID := false - if origSer[0].XMPPID != nil { - originalXMPPID = *origSer[0].XMPPID + if origServer.XMPPID != nil { + originalXMPPID = *origServer.XMPPID } - if origSer[0].Status != nil { - originalStatusID = *origSer[0].StatusID + if origServer.Status != nil { + originalStatusID = *origServer.StatusID } var server tc.ServerNullableV2 @@ -1220,7 +1221,7 @@ func Update(w http.ResponseWriter, r *http.Request) { if newServer.StatusID != nil && *newServer.StatusID != originalStatusID { newServer.StatusLastUpdated = ¤tTime } else { - newServer.StatusLastUpdated = origSer[0].StatusLastUpdated + newServer.StatusLastUpdated = origServer.StatusLastUpdated } serviceInterface, err := validateV3(&newServer, tx) if err != nil { @@ -1228,15 +1229,6 @@ func Update(w http.ResponseWriter, r *http.Request) { return } - cacheGroupIds := []int{*origSer[0].CachegroupID} - serverIds := []int{*origSer[0].ID} - if *origSer[0].CachegroupID != *newServer.CachegroupID { - if err = topology.CheckForEmptyCacheGroups(inf.Tx, cacheGroupIds, true, serverIds); err != nil { - api.HandleErr(w, r, tx, http.StatusBadRequest, errors.New("server is the last one in its cachegroup, which is used by a topology, so it cannot be moved to another cachegroup: "+err.Error()), nil) - return - } - } - server, err = newServer.ToServerV2() if err != nil { api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, fmt.Errorf("converting v3 server to v2 for update: %v", err)) @@ -1291,6 +1283,24 @@ func Update(w http.ResponseWriter, r *http.Request) { } } + if *origServer.CachegroupID != *server.CachegroupID || *origServer.CDNID != *server.CDNID { + hasDSOnCDN, err := dbhelpers.CachegroupHasTopologyBasedDeliveryServicesOnCDN(inf.Tx.Tx, *origServer.CachegroupID, *origServer.CDNID) + if err != nil { + api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, err) + return + } + CDNIDs := []int{} + if hasDSOnCDN { + CDNIDs = append(CDNIDs, *origServer.CDNID) + } + cacheGroupIds := []int{*origServer.CachegroupID} + serverIds := []int{*origServer.ID} + if err = topology.CheckForEmptyCacheGroups(inf.Tx, cacheGroupIds, CDNIDs, true, serverIds); err != nil { + api.HandleErr(w, r, tx, http.StatusBadRequest, errors.New("server is the last one in its cachegroup, which is used by a topology, so it cannot be moved to another cachegroup: "+err.Error()), nil) + return + } + } + server.ID = new(int) *server.ID = inf.IntParams["id"] @@ -1624,13 +1634,21 @@ func Delete(w http.ResponseWriter, r *http.Request) { api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, fmt.Errorf("there are somehow two servers with id %d - cannot delete", id)) return } - if version.Major >= 3 { - cacheGroupIds := []int{*servers[0].CachegroupID} - serverIds := []int{*servers[0].ID} - if err := topology.CheckForEmptyCacheGroups(inf.Tx, cacheGroupIds, true, serverIds); err != nil { - api.HandleErr(w, r, tx, http.StatusBadRequest, errors.New("server is the last one in its cachegroup, which is used by a topology: "+err.Error()), nil) - return - } + server := servers[0] + cacheGroupIds := []int{*server.CachegroupID} + serverIds := []int{*server.ID} + hasDSOnCDN, err := dbhelpers.CachegroupHasTopologyBasedDeliveryServicesOnCDN(inf.Tx.Tx, *server.CachegroupID, *server.CDNID) + if err != nil { + api.HandleErr(w, r, tx, http.StatusInternalServerError, nil, err) + return + } + CDNIDs := []int{} + if hasDSOnCDN { + CDNIDs = append(CDNIDs, *server.CDNID) + } + if err := topology.CheckForEmptyCacheGroups(inf.Tx, cacheGroupIds, CDNIDs, true, serverIds); err != nil { + api.HandleErr(w, r, tx, http.StatusBadRequest, errors.New("server is the last one in its cachegroup, which is used by a topology: "+err.Error()), nil) + return } userErr, sysErr, errCode = deleteInterfaces(id, tx) @@ -1652,8 +1670,6 @@ func Delete(w http.ResponseWriter, r *http.Request) { return } - server := servers[0] - if inf.Version.Major >= 3 { api.WriteRespAlertObj(w, r, tc.SuccessLevel, "Server deleted", server) } else { diff --git a/traffic_ops/traffic_ops_golang/topology/topologies.go b/traffic_ops/traffic_ops_golang/topology/topologies.go index bf77b01..57a9745 100644 --- a/traffic_ops/traffic_ops_golang/topology/topologies.go +++ b/traffic_ops/traffic_ops_golang/topology/topologies.go @@ -142,7 +142,12 @@ func (topology *TOTopology) Validate() error { for index, cacheGroup := range cacheGroups { cacheGroupIds[index] = *cacheGroup.ID } - rules["empty cachegroups"] = CheckForEmptyCacheGroups(topology.ReqInfo.Tx, cacheGroupIds, false, nil) + dsCDNs, err := dbhelpers.GetDeliveryServiceCDNsByTopology(topology.ReqInfo.Tx.Tx, topology.Name) + if err != nil { + log.Errorf("validating topology: %v", err) + return errors.New("unable to validate topology") + } + rules["empty cachegroups"] = CheckForEmptyCacheGroups(topology.ReqInfo.Tx, cacheGroupIds, dsCDNs, false, nil) rules["required capabilities"] = topology.validateDSRequiredCapabilities() /* Only perform further checks if everything so far is valid */ @@ -159,7 +164,10 @@ func (topology *TOTopology) Validate() error { return util.JoinErrs(tovalidate.ToErrors(rules)) } -func CheckForEmptyCacheGroups(tx *sqlx.Tx, cacheGroupIds []int, cachegroupsInTopology bool, excludeServerIds []int) error { +// CheckForEmptyCacheGroups checks if the cachegroups are empty (altogether) or empty in any of the given CDN IDs. +// If cachegroupsInTopology is true, it will only check cachegroups that are used in a topology. Any server IDs in +// excludeServerIds will not be counted. +func CheckForEmptyCacheGroups(tx *sqlx.Tx, cacheGroupIds []int, CDNIDs []int, cachegroupsInTopology bool, excludeServerIds []int) error { if excludeServerIds == nil { excludeServerIds = []int{} } @@ -180,14 +188,16 @@ func CheckForEmptyCacheGroups(tx *sqlx.Tx, cacheGroupIds []int, cachegroupsInTop } var ( - serverCount int - cacheGroup string - cacheGroups []string - topologies []string + serverCountByCDN int + cacheGroup string + cdnID *int ) + cgServerCountsByCDN := make(map[int]map[string]int) + cgServerCounts := make(map[string]int) + topologySetByCachegroup := make(map[string]map[string]struct{}) defer log.Close(rows, "unable to close DB connection when checking for cachegroups with no servers") for rows.Next() { - var scanTo = []interface{}{&cacheGroup, &serverCount} + var scanTo = []interface{}{&cacheGroup, &cdnID, &serverCountByCDN} var topologiesForRow []string if cachegroupsInTopology { scanTo = append(scanTo, pq.Array(&topologiesForRow)) @@ -196,23 +206,70 @@ func CheckForEmptyCacheGroups(tx *sqlx.Tx, cacheGroupIds []int, cachegroupsInTop log.Errorf(systemError, err.Error()) return baseError } - if serverCount != 0 { - break + if cdnID != nil { + if _, ok := cgServerCountsByCDN[*cdnID]; !ok { + cgServerCountsByCDN[*cdnID] = make(map[string]int) + } + cgServerCountsByCDN[*cdnID][cacheGroup] = serverCountByCDN } - cacheGroups = append(cacheGroups, cacheGroup) + cgServerCounts[cacheGroup] += serverCountByCDN + if cachegroupsInTopology { - topologies = append(topologies, topologiesForRow...) + if _, ok := topologySetByCachegroup[cacheGroup]; !ok { + topologySetByCachegroup[cacheGroup] = make(map[string]struct{}) + } + for _, topology := range topologiesForRow { + topologySetByCachegroup[cacheGroup][topology] = struct{}{} + } + } + } + topologiesByCachegroup := make(map[string][]string, len(topologySetByCachegroup)) + for cg, topologySet := range topologySetByCachegroup { + for topology := range topologySet { + topologiesByCachegroup[cg] = append(topologiesByCachegroup[cg], topology) + } + } + emptyCachegroups := []string{} + for cg, count := range cgServerCounts { + if count == 0 { + messageEntry := cg + if cachegroupsInTopology { + messageEntry += " (in topologies: " + strings.Join(topologiesByCachegroup[cg], ", ") + ")" + } + emptyCachegroups = append(emptyCachegroups, messageEntry) } } - if len(cacheGroups) > 0 { - errMessage := "cachegroups with no servers in them: " + strings.Join(cacheGroups, ", ") - if cachegroupsInTopology { - errMessage += " in topologies: " + strings.Join(topologies, ", ") + if len(emptyCachegroups) > 0 { + errMessage := "cachegroups with no servers in them: " + strings.Join(emptyCachegroups, ", ") + return errors.New(errMessage) + } + + errMessage := []string{} + for _, cdnID := range CDNIDs { + if _, ok := cgServerCountsByCDN[cdnID]; !ok { + return fmt.Errorf("topology is assigned to delivery service on CDN %d, but that CDN has no servers", cdnID) + } + emptyCachegroupsByCDN := []string{} + for cg, serverCount := range cgServerCountsByCDN[cdnID] { + if serverCount == 0 { + emptyCachegroupsByCDN = append(emptyCachegroupsByCDN, cg) + } + } + // check that this CDN has a count for all given cachegroups + for cg := range cgServerCounts { + if _, ok := cgServerCountsByCDN[cdnID][cg]; !ok { + emptyCachegroupsByCDN = append(emptyCachegroupsByCDN, cg) + } + } + if len(emptyCachegroupsByCDN) > 0 { + errMessage = append(errMessage, fmt.Sprintf("topology is assigned to delivery service(s) on CDN %d, but the following cachegroups have no servers in CDN %d: %s", cdnID, cdnID, strings.Join(emptyCachegroupsByCDN, ", "))) } - err = errors.New(errMessage) } - return err + if len(errMessage) > 0 { + return errors.New(strings.Join(errMessage, "; ")) + } + return nil } func (topology *TOTopology) nodesInOtherTopologies() ([]tc.TopologyNode, map[string][]string, error) { @@ -769,6 +826,7 @@ func selectEmptyCacheGroupsQuery(cachegroupsInTopology bool) string { query := fmt.Sprintf(` SELECT c."name", + s.cdn_id, COUNT(*) FILTER ( WHERE s.id IS NOT NULL AND NOT(s."id" = ANY(CAST(:exclude_server_ids AS INT[]))) @@ -777,8 +835,7 @@ func selectEmptyCacheGroupsQuery(cachegroupsInTopology bool) string { %s LEFT JOIN "server" s ON c.id = s.cachegroup WHERE c."id" = ANY(CAST(:cachegroup_ids AS BIGINT[])) - GROUP BY c."name" - ORDER BY server_count + GROUP BY c."name", s.cdn_id `, topologyNames, joinTopologyCachegroups) return query }
