Take topology-based DSes into account when checking DSS assignments

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

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!

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

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
+			}
 		}
 	}
 

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)
 		}
 	}

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)
 		}
 	}