From 382e0ac5f6db7fdb7c3ea44f6273a4ffab80d7c9 Mon Sep 17 00:00:00 2001 From: Zach Hoffman Date: Wed, 14 Oct 2020 03:49:03 -0600 Subject: [PATCH 1/3] Query servers by cachegroup name --- CHANGELOG.md | 3 +- docs/source/api/v3/cachegroups.rst | 2 + docs/source/api/v3/servers.rst | 68 ++++++++++--------- .../traffic_ops_golang/server/servers.go | 4 ++ 4 files changed, 43 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6005949ecf..d476df5a55 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,7 +43,8 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/). - Added the ability to view Hash ID field (aka xmppID) on Traffic Portals' server summary page - Added the ability to delete invalidation requests in Traffic Portal - Added the ability to set TLS config provided here: https://golang.org/pkg/crypto/tls/#Config in Traffic Ops -- Added an indiciator to the Traffic Monitor UI when using a disk backup of Traffic ops. +- Added support for the `cachegroupName` query parameter for `GET /api/3.0/servers` in Traffic Ops +- Added an indiciator to the Traffic Monitor UI when using a disk backup of Traffic Ops. - Added debugging functionality to CDN-in-a-Box for Traffic Stats. - Added debugging functionality to the Traffic Router unit tests runner at [`/traffic_router/tests`](https://github.com/apache/trafficcontrol/tree/master/traffic_router/tests) - Made the Traffic Router unit tests runner at [`/traffic_router/tests`](https://github.com/apache/trafficcontrol/tree/master/traffic_router/tests) run in Alpine Linux diff --git a/docs/source/api/v3/cachegroups.rst b/docs/source/api/v3/cachegroups.rst index 725ae96470..ae9d5f6dc1 100644 --- a/docs/source/api/v3/cachegroups.rst +++ b/docs/source/api/v3/cachegroups.rst @@ -36,6 +36,8 @@ Request Structure +===========+==========+==========================================================================================================================+ | id | no | Return the only :term:`Cache Group` that has this id | +-----------+----------+--------------------------------------------------------------------------------------------------------------------------+ + | name | no | Return only the :term:`Cache Group` identified by this :ref:`cache-group-name` | + +-----------+----------+--------------------------------------------------------------------------------------------------------------------------+ | type | no | Return only :term:`Cache Groups` that are of the :ref:`cache-group-type` identified by this integral, unique identifier | +-----------+----------+--------------------------------------------------------------------------------------------------------------------------+ | topology | no | Return only :term:`Cache Groups` that are used in the :term:`Topology` identified by this unique identifier | diff --git a/docs/source/api/v3/servers.rst b/docs/source/api/v3/servers.rst index 49060b541e..f52740b5d2 100644 --- a/docs/source/api/v3/servers.rst +++ b/docs/source/api/v3/servers.rst @@ -31,39 +31,41 @@ Request Structure ----------------- .. table:: Request Query Parameters - +------------+----------+-------------------------------------------------------------------------------------------------------------------+ - | Name | Required | Description | - +============+==========+===================================================================================================================+ - | cachegroup | no | Return only those servers within the :term:`Cache Group` that has this :ref:`cache-group-id` | - +------------+----------+-------------------------------------------------------------------------------------------------------------------+ - | dsId | no | Return only those servers assigned to the :term:`Delivery Service` identified by this integral, unique identifier.| - | | | If the Delivery Service has a :term:`Topology` assigned to it, the :ref:`to-api-servers` endpoint will return | - | | | each server whose :term:`Cache Group` is associated with a :term:`Topology Node` of that Topology and has the | - | | | :term:`Server Capabilities` that are | - | | | :term:`required by the Delivery Service `. | - +------------+----------+-------------------------------------------------------------------------------------------------------------------+ - | hostName | no | Return only those servers that have this (short) hostname | - +------------+----------+-------------------------------------------------------------------------------------------------------------------+ - | id | no | Return only the server with this integral, unique identifier | - +------------+----------+-------------------------------------------------------------------------------------------------------------------+ - | profileId | no | Return only those servers that are using the :term:`Profile` that has this :ref:`profile-id` | - +------------+----------+-------------------------------------------------------------------------------------------------------------------+ - | status | no | Return only those servers with this status - see :ref:`health-proto` | - +------------+----------+-------------------------------------------------------------------------------------------------------------------+ - | type | no | Return only servers of this :term:`Type` | - +------------+----------+-------------------------------------------------------------------------------------------------------------------+ - | topology | no | Return only servers who belong to cachegroups assigned to the :term:`Topology` identified by this name | - +------------+----------+-------------------------------------------------------------------------------------------------------------------+ - | sortOrder | no | Changes the order of sorting. Either ascending (default or "asc") or descending ("desc") | - +------------+----------+-------------------------------------------------------------------------------------------------------------------+ - | limit | no | Choose the maximum number of results to return | - +------------+----------+-------------------------------------------------------------------------------------------------------------------+ - | offset | no | The number of results to skip before beginning to return results. Must use in conjunction with limit | - +------------+----------+-------------------------------------------------------------------------------------------------------------------+ - | page | no | Return the n\ :sup:`th` page of results, where "n" is the value of this parameter, pages are ``limit`` long and | - | | | the first page is 1. If ``offset`` was defined, this query parameter has no effect. ``limit`` must be defined to | - | | | make use of ``page``. | - +------------+----------+-------------------------------------------------------------------------------------------------------------------+ + +----------------+----------+-------------------------------------------------------------------------------------------------------------------+ + | Name | Required | Description | + +================+==========+===================================================================================================================+ + | cachegroup | no | Return only those servers within the :term:`Cache Group` that has this :ref:`cache-group-id` | + +----------------+----------+-------------------------------------------------------------------------------------------------------------------+ + | cachegroupName | no | Return only those servers within the :term:`Cache Group` that has this :ref:`cache-group-name` | + +----------------+----------+-------------------------------------------------------------------------------------------------------------------+ + | dsId | no | Return only those servers assigned to the :term:`Delivery Service` identified by this integral, unique identifier.| + | | | If the Delivery Service has a :term:`Topology` assigned to it, the :ref:`to-api-servers` endpoint will return | + | | | each server whose :term:`Cache Group` is associated with a :term:`Topology Node` of that Topology and has the | + | | | :term:`Server Capabilities` that are | + | | | :term:`required by the Delivery Service `. | + +----------------+----------+-------------------------------------------------------------------------------------------------------------------+ + | hostName | no | Return only those servers that have this (short) hostname | + +----------------+----------+-------------------------------------------------------------------------------------------------------------------+ + | id | no | Return only the server with this integral, unique identifier | + +----------------+----------+-------------------------------------------------------------------------------------------------------------------+ + | profileId | no | Return only those servers that are using the :term:`Profile` that has this :ref:`profile-id` | + +----------------+----------+-------------------------------------------------------------------------------------------------------------------+ + | status | no | Return only those servers with this status - see :ref:`health-proto` | + +----------------+----------+-------------------------------------------------------------------------------------------------------------------+ + | type | no | Return only servers of this :term:`Type` | + +----------------+----------+-------------------------------------------------------------------------------------------------------------------+ + | topology | no | Return only servers who belong to cachegroups assigned to the :term:`Topology` identified by this name | + +----------------+----------+-------------------------------------------------------------------------------------------------------------------+ + | sortOrder | no | Changes the order of sorting. Either ascending (default or "asc") or descending ("desc") | + +----------------+----------+-------------------------------------------------------------------------------------------------------------------+ + | limit | no | Choose the maximum number of results to return | + +----------------+----------+-------------------------------------------------------------------------------------------------------------------+ + | offset | no | The number of results to skip before beginning to return results. Must use in conjunction with limit | + +----------------+----------+-------------------------------------------------------------------------------------------------------------------+ + | page | no | Return the n\ :sup:`th` page of results, where "n" is the value of this parameter, pages are ``limit`` long and | + | | | the first page is 1. If ``offset`` was defined, this query parameter has no effect. ``limit`` must be defined to | + | | | make use of ``page``. | + +----------------+----------+-------------------------------------------------------------------------------------------------------------------+ .. code-block:: http :caption: Request Example diff --git a/traffic_ops/traffic_ops_golang/server/servers.go b/traffic_ops/traffic_ops_golang/server/servers.go index 86aab8dfb7..45397575aa 100644 --- a/traffic_ops/traffic_ops_golang/server/servers.go +++ b/traffic_ops/traffic_ops_golang/server/servers.go @@ -779,6 +779,10 @@ func getServers(h http.Header, params map[string]string, tx *sqlx.Tx, user *auth "dsId": dbhelpers.WhereColumnInfo{"dss.deliveryservice", nil}, } + if version.Major >= 3 { + queryParamsToSQLCols["cachegroupName"] = dbhelpers.WhereColumnInfo{"cg.name", nil} + } + usesMids := false queryAddition := "" dsHasRequiredCapabilities := false From 8508dedf51438751ea97f7ee9230ce71b12cad81 Mon Sep 17 00:00:00 2001 From: Zach Hoffman Date: Wed, 14 Oct 2020 03:51:29 -0600 Subject: [PATCH 2/3] Topologies empty cachegroup validation: - Optionally require that the cachegroup is in an existing topology - Optionally exclude an array of server IDs --- .../traffic_ops_golang/topology/topologies.go | 82 +++++++++++++------ 1 file changed, 58 insertions(+), 24 deletions(-) diff --git a/traffic_ops/traffic_ops_golang/topology/topologies.go b/traffic_ops/traffic_ops_golang/topology/topologies.go index f1fcffc2db..f1233059ce 100644 --- a/traffic_ops/traffic_ops_golang/topology/topologies.go +++ b/traffic_ops/traffic_ops_golang/topology/topologies.go @@ -31,6 +31,7 @@ import ( "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/dbhelpers" "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/util/ims" validation "github.com/go-ozzo/ozzo-validation" + "github.com/jmoiron/sqlx" "github.com/lib/pq" "net/http" "strings" @@ -133,7 +134,11 @@ func (topology *TOTopology) Validate() error { rules[fmt.Sprintf("parent '%v' edge type", node.Cachegroup)] = topology.checkForEdgeParents(cacheGroups, index) } - rules["empty cachegroups"] = topology.checkForEmptyCacheGroups() + cacheGroupIds := make([]int, len(cacheGroupNames)) + for index, cacheGroup := range cacheGroups { + cacheGroupIds[index] = *cacheGroup.ID + } + rules["empty cachegroups"] = CheckForEmptyCacheGroups(topology.ReqInfo.Tx, cacheGroupIds, false, nil) /* Only perform further checks if everything so far is valid */ if err = util.JoinErrs(tovalidate.ToErrors(rules)); err != nil { @@ -149,20 +154,21 @@ func (topology *TOTopology) Validate() error { return util.JoinErrs(tovalidate.ToErrors(rules)) } -func (topology *TOTopology) checkForEmptyCacheGroups() error { +func CheckForEmptyCacheGroups(tx *sqlx.Tx, cacheGroupIds []int, cachegroupsInTopology bool, excludeServerIds []int) error { + if excludeServerIds == nil { + excludeServerIds = []int{} + } var ( - cacheGroupNames = make([]string, len(topology.Nodes)) - baseError = errors.New("unable to check for cachegroups with no servers") - systemError = "checking for cachegroups with no servers: %s" - parameters = map[string]interface{}{} - query = selectEmptyCacheGroupsQuery() + baseError = errors.New("unable to check for cachegroups with no servers") + systemError = "checking for cachegroups with no servers: %s" + query = selectEmptyCacheGroupsQuery(cachegroupsInTopology) + parameters = map[string]interface{}{ + "cachegroup_ids": pq.Array(cacheGroupIds), + "exclude_server_ids": pq.Array(excludeServerIds), + } ) - for index, node := range topology.Nodes { - cacheGroupNames[index] = node.Cachegroup - } - parameters["cachegroup_names"] = pq.Array(cacheGroupNames) - rows, err := topology.ReqInfo.Tx.NamedQuery(query, parameters) + rows, err := tx.NamedQuery(query, parameters) if err != nil { log.Errorf(systemError, err.Error()) return baseError @@ -172,10 +178,16 @@ func (topology *TOTopology) checkForEmptyCacheGroups() error { serverCount int cacheGroup string cacheGroups []string + topologies []string ) defer log.Close(rows, "unable to close DB connection when checking for cachegroups with no servers") for rows.Next() { - if err := rows.Scan(&cacheGroup, &serverCount); err != nil { + var scanTo = []interface{}{&cacheGroup, &serverCount} + var topologiesForRow []string + if cachegroupsInTopology { + scanTo = append(scanTo, pq.Array(&topologiesForRow)) + } + if err := rows.Scan(scanTo...); err != nil { log.Errorf(systemError, err.Error()) return baseError } @@ -183,10 +195,17 @@ func (topology *TOTopology) checkForEmptyCacheGroups() error { break } cacheGroups = append(cacheGroups, cacheGroup) + if cachegroupsInTopology { + topologies = append(topologies, topologiesForRow...) + } } if len(cacheGroups) > 0 { - err = fmt.Errorf("cachegroups with no servers in them: %s", strings.Join(cacheGroups, ", ")) + errMessage := "cachegroups with no servers in them: " + strings.Join(cacheGroups, ", ") + if cachegroupsInTopology { + errMessage += " in topologies: " + strings.Join(topologies, ", ") + } + err = errors.New(errMessage) } return err } @@ -614,19 +633,34 @@ JOIN topology_cachegroup tc on t.name = tc.topology return query } -func selectEmptyCacheGroupsQuery() string { +func selectEmptyCacheGroupsQuery(cachegroupsInTopology bool) string { + var joinTopologyCachegroups string + var topologyNames string + if cachegroupsInTopology { + // language=SQL + topologyNames = ` + , ARRAY_AGG(tc.topology) +` + // language=SQL + joinTopologyCachegroups = ` + JOIN topology_cachegroup tc ON c."name" = tc.cachegroup +` + } // language=SQL - query := ` + query := fmt.Sprintf(` SELECT c."name", - COUNT(*) FILTER (WHERE s.id IS NOT NULL) AS server_count - FROM - cachegroup c - LEFT JOIN "server" s ON c.id = s.cachegroup - WHERE c."name" = ANY(CAST(:cachegroup_names AS TEXT[])) - GROUP BY c."name" - ORDER BY server_count; - ` + COUNT(*) FILTER ( + WHERE s.id IS NOT NULL + AND NOT(s."id" = ANY(CAST(:exclude_server_ids AS INT[]))) + ) AS server_count %s + FROM cachegroup c + %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 + `, topologyNames, joinTopologyCachegroups) return query } From a8c3648e8461dbb8db45b2c2c3cb6229214ca58c Mon Sep 17 00:00:00 2001 From: Zach Hoffman Date: Wed, 14 Oct 2020 04:31:19 -0600 Subject: [PATCH 3/3] - Disallow deleting the last server in a cachegroup used by a topology - Disallow moving the last server in a cachegroup used by a topology to a different cachegroup --- CHANGELOG.md | 2 +- traffic_ops/testing/api/v3/servers_test.go | 44 +++++++++++++++++++ .../traffic_ops_golang/server/servers.go | 16 +++++++ traffic_ops/v3-client/server.go | 3 ++ 4 files changed, 64 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d476df5a55..334cebb19b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,7 +21,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/). - Traffic Ops: Added validation to prohibit assigning caches to topology-based delivery services - Traffic Ops: Added validation to prohibit removing a capability from a server if no other server in the same cachegroup can satisfy the required capabilities of the delivery services assigned to it via topologies. - Traffic Ops: Added validation to ensure that at least one server per cachegroup in a delivery service's topology has the delivery service's required capabilities. - - Traffic Ops: Added validation to ensure that at least one server exists in each cachegroup that that is used in a Topology. + - Traffic Ops: Added validation to ensure that at least one server exists in each cachegroup that is used in a Topology on the `/api/3.0/topologies` endpoint and the `/api/3.0/servers/{{ID}}` endpoint. - Traffic Ops: Consider Topologies parentage when queueing or checking server updates - ORT: Added Topologies to Config Generation. - Traffic Portal: Added the ability to create, read, update and delete flexible topologies. diff --git a/traffic_ops/testing/api/v3/servers_test.go b/traffic_ops/testing/api/v3/servers_test.go index aa238fc754..f52bc39b5b 100644 --- a/traffic_ops/testing/api/v3/servers_test.go +++ b/traffic_ops/testing/api/v3/servers_test.go @@ -45,9 +45,53 @@ func TestServers(t *testing.T) { CreateTestServerWithoutProfileId(t) UniqueIPProfileTestServers(t) UpdateTestServerStatus(t) + LastServerInTopologyCacheGroup(t) }) } +func LastServerInTopologyCacheGroup(t *testing.T) { + const cacheGroupName = "topology-mid-cg-01" + const moveToCacheGroup = "topology-mid-cg-02" + const topologyName = "forked-topology" + const expectedLength = 1 + params := url.Values{} + params.Add("cachegroupName", cacheGroupName) + params.Add("topology", topologyName) + servers, _, err := TOSession.GetServersWithHdr(¶ms, nil) + if err != nil { + t.Fatalf("getting server from cachegroup %s in topology %s: %s", 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)) + } + server := servers.Response[0] + _, reqInf, err := TOSession.DeleteServerByID(*server.ID) + if err == nil { + t.Fatalf("expected an error deleting server with id %d, received no error", *server.ID) + } + if reqInf.StatusCode < http.StatusBadRequest || reqInf.StatusCode >= http.StatusInternalServerError { + t.Fatalf("expected a 400-level error deleting server with id %d, got status code %d: %s", *server.ID, reqInf.StatusCode, err.Error()) + } + + params = url.Values{} + params.Add("name", moveToCacheGroup) + cgs, _, err := TOSession.GetCacheGroupsByQueryParamsWithHdr(params, nil) + if err != nil { + t.Fatalf("getting cachegroup with hostname %s: %s", moveToCacheGroup, err.Error()) + } + if len(cgs) != expectedLength { + t.Fatalf("expected %d cachegroup with hostname %s, received %d cachegroups", expectedLength, moveToCacheGroup, len(cgs)) + } + *server.CachegroupID = *cgs[0].ID + _, _, err = TOSession.UpdateServerByID(*server.ID, server) + if err == nil { + t.Fatalf("expected an error moving server with id %d to a different cachegroup, received no error", *server.ID) + } + if reqInf.StatusCode < http.StatusBadRequest || reqInf.StatusCode >= http.StatusInternalServerError { + t.Fatalf("expected a 400-level error moving server with id %d to a different cachegroup, got status code %d: %s", *server.ID, reqInf.StatusCode, err.Error()) + } +} + func UpdateTestServerStatus(t *testing.T) { if len(testData.Servers) < 1 { t.Fatal("Need at least one server to test updating") diff --git a/traffic_ops/traffic_ops_golang/server/servers.go b/traffic_ops/traffic_ops_golang/server/servers.go index 45397575aa..1c58690f43 100644 --- a/traffic_ops/traffic_ops_golang/server/servers.go +++ b/traffic_ops/traffic_ops_golang/server/servers.go @@ -26,6 +26,7 @@ import ( "encoding/json" "errors" "fmt" + "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/topology" "net" "net/http" "strconv" @@ -1214,6 +1215,13 @@ func Update(w http.ResponseWriter, r *http.Request) { return } + cacheGroupIds := []int{*origSer[0].CachegroupID} + serverIds := []int{*origSer[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, 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)) @@ -1595,6 +1603,14 @@ 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 + } + } userErr, sysErr, errCode = deleteInterfaces(id, tx) if userErr != nil || sysErr != nil { diff --git a/traffic_ops/v3-client/server.go b/traffic_ops/v3-client/server.go index 993642bd53..10729a311b 100644 --- a/traffic_ops/v3-client/server.go +++ b/traffic_ops/v3-client/server.go @@ -249,6 +249,9 @@ func (to *Session) DeleteServerByID(id int) (tc.Alerts, ReqInf, error) { route := fmt.Sprintf("%s/%d", API_SERVERS, id) resp, remoteAddr, err := to.request(http.MethodDelete, route, nil, nil) reqInf := ReqInf{CacheHitStatus: CacheHitStatusMiss, RemoteAddr: remoteAddr} + if resp != nil { + reqInf.StatusCode = resp.StatusCode + } if err != nil { return tc.Alerts{}, reqInf, err }