From 4428fdbeafac3971216cc7bbd0431197ad645fc8 Mon Sep 17 00:00:00 2001 From: Rawlin Peters Date: Mon, 6 Dec 2021 13:29:51 -0700 Subject: [PATCH 1/3] Take topology-based DSes into account when checking DSS assignments In order to prevent accidental ouages, TO currently has validation which prevents things like deleting the last server that is assigned to an active delivery service. However, this validation ignored the fact that delivery services can now be assigned to a topology instead of using DSS assignments. This prevents a number of actually valid things, such as safely setting caches to ADMIN_DOWN, assigning MSO servers to topology-based DSes, and removing legacy DSS EDGE assignments from topology-based DSes. This fixes that validation in order to take topology-based DSes into account. Closes: #6392 --- CHANGELOG.md | 1 + .../deliveryservice/servers/delete.go | 7 +++-- .../deliveryservice/servers/servers.go | 30 ++++++++++--------- .../traffic_ops_golang/server/servers.go | 9 +++--- .../server/servers_assignment.go | 9 +++--- 5 files changed, 31 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 846f16bb28..4abcb1d875 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/). - [#6255](https://github.com/apache/trafficcontrol/issues/6255) - Unreadable Prod Mode CDN Notifications in Traffic Portal - Fixed broken `GET /cdns/routing` Traffic Ops API - [#6259](https://github.com/apache/trafficcontrol/issues/6259) - Traffic Portal No Longer Allows Spaces in Server Object "Router Port Name" +- [#6392](https://github.com/apache/trafficcontrol/issues/6392) - Traffic Ops prevents assigning ORG servers to topology-based delivery services (as well as a number of other valid operations being prohibited by "last server assigned to DS" validations which don't apply to topology-based delivery services) - [#6175](https://github.com/apache/trafficcontrol/issues/6175) - POST request to /api/4.0/phys_locations accepts mismatch values for regionName. - [#6285](https://github.com/apache/trafficcontrol/issues/6285) - The Traffic Ops Postinstall script will work in CentOS 7, even if Python 3 is installed - [#5373](https://github.com/apache/trafficcontrol/issues/5373) - Traffic Monitor logs not consistent diff --git a/traffic_ops/traffic_ops_golang/deliveryservice/servers/delete.go b/traffic_ops/traffic_ops_golang/deliveryservice/servers/delete.go index c9857f745b..b4fa6a58b9 100644 --- a/traffic_ops/traffic_ops_golang/deliveryservice/servers/delete.go +++ b/traffic_ops/traffic_ops_golang/deliveryservice/servers/delete.go @@ -77,7 +77,7 @@ SELECT ( // back to the user, and an error that must not be shown to the user. If the server is, in fact, // the last available edge or origin assigned to the Delivery Service, the "user error" will be // set to an appropriate, non-nil value. -func checkLastAvailableEdgeOrOrigin(dsID, serverID int, usesMSO bool, tx *sql.Tx) (int, error, error) { +func checkLastAvailableEdgeOrOrigin(dsID, serverID int, usesMSO, hasTopology bool, tx *sql.Tx) (int, error, error) { var isLastEdge bool var isLastOrigin bool if tx == nil { @@ -87,7 +87,7 @@ func checkLastAvailableEdgeOrOrigin(dsID, serverID int, usesMSO bool, tx *sql.Tx if err != nil { return http.StatusInternalServerError, nil, fmt.Errorf("checking if server #%d is the last one assigned to DS #%d: %v", serverID, dsID, err) } - if isLastEdge { + if isLastEdge && !hasTopology { return http.StatusConflict, fmt.Errorf("removing server #%d from active Delivery Service #%d would leave it with no REPORTED/ONLINE EDGE servers", serverID, dsID), nil } if usesMSO && isLastOrigin { @@ -151,7 +151,8 @@ func deleteDSS(w http.ResponseWriter, r *http.Request) { if *ds.Active { usesMSO := ds.MultiSiteOrigin == nil || *ds.MultiSiteOrigin - errCode, userErr, sysErr = checkLastAvailableEdgeOrOrigin(dsID, serverID, usesMSO, tx) + hasTopology := ds.Topology != nil + errCode, userErr, sysErr = checkLastAvailableEdgeOrOrigin(dsID, serverID, usesMSO, hasTopology, tx) if userErr != nil || sysErr != nil { api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr) return diff --git a/traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go b/traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go index e5bdc88ea1..4be3c762c2 100644 --- a/traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go +++ b/traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go @@ -564,26 +564,28 @@ func validateDSSAssignments(tx *sql.Tx, ds DSInfo, serverInfos []tc.ServerInfo, } } } - // Prevent the user from deleting all the servers in an active DS - if len(ids) == 0 { + // Prevent the user from deleting all the servers in an active, non-topology-based DS + if len(ids) == 0 && ds.Topology == nil { return fmt.Errorf("this server assignment leaves Active Delivery Service #%d without any '%s' or '%s' servers", ds.ID, tc.CacheStatusOnline, tc.CacheStatusReported), nil, http.StatusConflict } // prevent the user from deleting all ORG servers from an active, MSO-enabled DS if ds.UseMultiSiteOrigin && newOrgCount < 1 { return fmt.Errorf("this server assignment leaves Active, MSO-enabled Delivery Service #%d without any '%s' or '%s' %s servers", ds.ID, tc.CacheStatusOnline, tc.CacheStatusReported, tc.OriginTypeName), nil, http.StatusConflict } - // The following check is necessary because of the following: - // Consider a brand new active DS that has no server assignments. - // Now, you wish to assign an online/ reported ORG server to it. - // Since this is a new DS and it didnt have any "pre existing" online/ reported EDGEs, this should be possible. - // However, if that DS had a couple of online/ reported EDGEs assigned to it, and now if you wanted to "replace" - // that assignment with the new assignment of an online/ reported ORG, this should be prohibited by TO. - currentlyHasAvailableEdgesAssigned, err := hasAvailableEdgesCurrentlyAssigned(tx, ds.ID) - if err != nil { - return nil, fmt.Errorf("checking for pre existing ONLINE/ REPORTED EDGES: %v", err), http.StatusInternalServerError - } - if (currentlyHasAvailableEdgesAssigned && newAvailableEdgeCount < 1) || !anyAvailableServers { - return fmt.Errorf("this server assignment leaves Active Delivery Service #%d without any '%s' or '%s' servers", ds.ID, tc.CacheStatusOnline, tc.CacheStatusReported), nil, http.StatusConflict + if ds.Topology == nil { + // The following check is necessary because of the following: + // Consider a brand new active DS that has no server assignments. + // Now, you wish to assign an online/ reported ORG server to it. + // Since this is a new DS and it didnt have any "pre existing" online/ reported EDGEs, this should be possible. + // However, if that DS had a couple of online/ reported EDGEs assigned to it, and now if you wanted to "replace" + // that assignment with the new assignment of an online/ reported ORG, this should be prohibited by TO. + currentlyHasAvailableEdgesAssigned, err := hasAvailableEdgesCurrentlyAssigned(tx, ds.ID) + if err != nil { + return nil, fmt.Errorf("checking for pre existing ONLINE/ REPORTED EDGES: %v", err), http.StatusInternalServerError + } + if (currentlyHasAvailableEdgesAssigned && newAvailableEdgeCount < 1) || !anyAvailableServers { + return fmt.Errorf("this server assignment leaves Active Delivery Service #%d without any '%s' or '%s' servers", ds.ID, tc.CacheStatusOnline, tc.CacheStatusReported), nil, http.StatusConflict + } } } diff --git a/traffic_ops/traffic_ops_golang/server/servers.go b/traffic_ops/traffic_ops_golang/server/servers.go index e87c0fe7e8..c43a9cc80e 100644 --- a/traffic_ops/traffic_ops_golang/server/servers.go +++ b/traffic_ops/traffic_ops_golang/server/servers.go @@ -1908,7 +1908,7 @@ func Create(w http.ResponseWriter, r *http.Request) { } const lastServerTypeOfDSesQuery = ` -SELECT ds.id, ds.multi_site_origin +SELECT ds.id, ds.multi_site_origin, ds.topology FROM deliveryservice_server dss JOIN server s ON dss.server = s.id JOIN type t ON s.type = t.id @@ -1941,16 +1941,17 @@ func getActiveDeliveryServicesThatOnlyHaveThisServerAssigned(id int, serverType if err != nil { return ids, fmt.Errorf("querying: %v", err) } - defer rows.Close() + defer log.Close(rows, "closing rows from getActiveDeliveryServicesThatOnlyHaveThisServerAssigned") for rows.Next() { var dsID int var mso bool - err = rows.Scan(&dsID, &mso) + var topology *string + err = rows.Scan(&dsID, &mso, &topology) if err != nil { return ids, fmt.Errorf("scanning: %v", err) } - if isEdge || (isOrigin && mso) { + if (isEdge && topology == nil) || (isOrigin && mso) { ids = append(ids, dsID) } } diff --git a/traffic_ops/traffic_ops_golang/server/servers_assignment.go b/traffic_ops/traffic_ops_golang/server/servers_assignment.go index 33e53f2258..e2e734488b 100644 --- a/traffic_ops/traffic_ops_golang/server/servers_assignment.go +++ b/traffic_ops/traffic_ops_golang/server/servers_assignment.go @@ -63,7 +63,7 @@ func getConfigFile(prefix string, xmlId string) string { } const lastServerInActiveDeliveryServicesQuery = ` -SELECT d.id, d.multi_site_origin +SELECT d.id, d.multi_site_origin, d.topology FROM deliveryservice d INNER JOIN deliveryservice_server dss ON dss.deliveryservice = d.id INNER JOIN server s ON s.id = dss.server @@ -100,15 +100,16 @@ func checkForLastServerInActiveDeliveryServices(serverID int, serverType string, if err != nil { return violations, fmt.Errorf("querying: %v", err) } - defer rows.Close() + defer log.Close(rows, "closing rows in checkForLastServerInActiveDeliveryServices") for rows.Next() { var violation int var mso bool - if err = rows.Scan(&violation, &mso); err != nil { + var topology *string + if err = rows.Scan(&violation, &mso, &topology); err != nil { return violations, fmt.Errorf("scanning: %v", err) } - if isEdge || (isOrigin && mso) { + if (isEdge && topology == nil) || (isOrigin && mso) { violations = append(violations, violation) } } From 541e8daca9fe65a97fe0af392b63754e1c581587 Mon Sep 17 00:00:00 2001 From: Rawlin Peters Date: Mon, 6 Dec 2021 14:32:01 -0700 Subject: [PATCH 2/3] Fix GROUP BY --- traffic_ops/traffic_ops_golang/server/servers.go | 2 +- traffic_ops/traffic_ops_golang/server/servers_assignment.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/traffic_ops/traffic_ops_golang/server/servers.go b/traffic_ops/traffic_ops_golang/server/servers.go index c43a9cc80e..8ceb3b5c30 100644 --- a/traffic_ops/traffic_ops_golang/server/servers.go +++ b/traffic_ops/traffic_ops_golang/server/servers.go @@ -1914,7 +1914,7 @@ JOIN server s ON dss.server = s.id JOIN type t ON s.type = t.id JOIN deliveryservice ds ON dss.deliveryservice = ds.id WHERE t.name LIKE $1 AND ds.active -GROUP BY ds.id, ds.multi_site_origin +GROUP BY ds.id, ds.multi_site_origin, ds.topology HAVING COUNT(dss.server) = 1 AND $2 = ANY(ARRAY_AGG(dss.server)); ` diff --git a/traffic_ops/traffic_ops_golang/server/servers_assignment.go b/traffic_ops/traffic_ops_golang/server/servers_assignment.go index e2e734488b..d9f0d87ac3 100644 --- a/traffic_ops/traffic_ops_golang/server/servers_assignment.go +++ b/traffic_ops/traffic_ops_golang/server/servers_assignment.go @@ -79,7 +79,7 @@ WHERE d.id IN ( AND NOT (dss.deliveryservice = ANY($2::BIGINT[])) AND (st.name = '` + string(tc.CacheStatusOnline) + `' OR st.name = '` + string(tc.CacheStatusReported) + `') AND t.name LIKE $3 -GROUP BY d.id, d.multi_site_origin +GROUP BY d.id, d.multi_site_origin, d.topology HAVING COUNT(dss.server) = 1 ` From 8c4c0997fd2e0eb8583b992c17474c1f70e67e49 Mon Sep 17 00:00:00 2001 From: Rawlin Peters Date: Mon, 6 Dec 2021 15:55:35 -0700 Subject: [PATCH 3/3] Add TO API test --- .../testing/api/v4/serverupdatestatus_test.go | 57 +++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/traffic_ops/testing/api/v4/serverupdatestatus_test.go b/traffic_ops/testing/api/v4/serverupdatestatus_test.go index 98f1d2680d..eac7ebb4ce 100644 --- a/traffic_ops/testing/api/v4/serverupdatestatus_test.go +++ b/traffic_ops/testing/api/v4/serverupdatestatus_test.go @@ -28,6 +28,63 @@ import ( client "github.com/apache/trafficcontrol/traffic_ops/v4-client" ) +func TestServerUpdateStatusLastAssigned(t *testing.T) { + WithObjs(t, []TCObj{CDNs, Types, Tenants, Parameters, Profiles, Statuses, Divisions, Regions, PhysLocations, CacheGroups, Servers, ServiceCategories, Topologies, DeliveryServices}, func() { + opts := client.NewRequestOptions() + opts.QueryParameters.Set("hostName", "atlanta-edge-01") + resp, _, err := TOSession.GetServers(opts) + if err != nil { + t.Fatalf("cannot get server by hostname: %v", err) + } + if len(resp.Response) != 1 { + t.Fatalf("Expected a server named 'atlanta-edge-01' to exist") + } + edge := resp.Response[0] + opts = client.NewRequestOptions() + opts.QueryParameters.Set("xmlId", "ds-top") + dsResp, _, err := TOSession.GetDeliveryServices(opts) + if err != nil { + t.Fatalf("cannot get delivery service by xmlId: %v", err) + } + if len(resp.Response) != 1 { + t.Fatalf("Expected one delivery service with xmlId 'ds-top' to exist") + } + // temporarily unassign the topology in order to assign an EDGE + ds := dsResp.Response[0] + tmpTop := *ds.Topology + ds.Topology = nil + ds.FirstHeaderRewrite = nil + ds.LastHeaderRewrite = nil + ds.InnerHeaderRewrite = nil + _, _, err = TOSession.UpdateDeliveryService(*ds.ID, ds, client.RequestOptions{}) + if err != nil { + t.Fatalf("cannot update delivery service 'ds-top': %v", err) + } + _, _, err = TOSession.CreateDeliveryServiceServers(*ds.ID, []int{*edge.ID}, true, client.RequestOptions{}) + if err != nil { + t.Fatalf("cannot create delivery service server: %v", err) + } + // reassign the topology + ds.Topology = &tmpTop + _, _, err = TOSession.UpdateDeliveryService(*ds.ID, ds, client.RequestOptions{}) + if err != nil { + t.Fatalf("cannot update delivery service 'ds-top': %v", err) + } + // attempt to set the edge to OFFLINE + _, _, err = TOSession.UpdateServerStatus(*edge.ID, tc.ServerPutStatus{ + Status: util.JSONNameOrIDStr{Name: util.StrPtr("OFFLINE")}, + OfflineReason: util.StrPtr("testing")}, client.RequestOptions{}) + if err != nil { + t.Errorf("setting edge to OFFLINE when it's the only edge assigned to a topology-based delivery service - expected: no error, actual: %v", err) + } + // remove EDGE assignment + _, _, err = TOSession.CreateDeliveryServiceServers(*ds.ID, []int{}, true, client.RequestOptions{}) + if err != nil { + t.Errorf("removing delivery service servers from topology-based delivery service - expected: no error, actual: %v", err) + } + }) +} + func TestServerUpdateStatus(t *testing.T) { WithObjs(t, []TCObj{CDNs, Types, Parameters, Profiles, Statuses, Divisions, Regions, PhysLocations, CacheGroups, Servers}, func() { //TODO: DON'T hard-code server hostnames!