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/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! 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..8ceb3b5c30 100644 --- a/traffic_ops/traffic_ops_golang/server/servers.go +++ b/traffic_ops/traffic_ops_golang/server/servers.go @@ -1908,13 +1908,13 @@ 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 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)); ` @@ -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..d9f0d87ac3 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 @@ -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 ` @@ -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) } }