[trafficcontrol] branch 5.0.x updated: Validate ORG server for topology based DS (#5307)

[oc...@apache.org]

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


The following commit(s) were added to refs/heads/5.0.x by this push:
     new 38c86ba  Validate ORG server for topology based DS (#5307)
38c86ba is described below

commit 38c86ba87d45ca09cafd648384023e046a2efbfb
Author: rimashah25 <22...@users.noreply.github.com>
AuthorDate: Wed Dec 2 15:20:10 2020 -0700

    Validate ORG server for topology based DS (#5307)
    
    * Edited updateV30() to check if ORG server exists and is part of topology cachegroup
    
    * Added helper function call to db_helpers.go
    
    * Updated code based on review feedback
    
    * Removed fmt statements
    
    * Added test case. Formatted code.
    
    * Updated CHANGELOG.md
    
    * Updated based on feedback.
    
    * Updated testcase
    
    (cherry picked from commit 38fc19e5af2cb4a2dca168dd2ccbbc81654dbe47)
---
 CHANGELOG.md                                       |  1 +
 .../testing/api/v3/deliveryservices_test.go        | 48 ++++++++++++++++++++
 .../traffic_ops_golang/dbhelpers/db_helpers.go     | 53 +++++++++++++++++++++-
 .../deliveryservice/deliveryservices.go            |  5 ++
 .../topology_validation/topology_validation.go     |  5 +-
 5 files changed, 109 insertions(+), 3 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 5245584..d3fd9f1 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -73,6 +73,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 - Traffic Portal: upgraded change log UI table to use more powerful/performant ag-grid component
 - Traffic Portal: change log days are now configurable in traffic_portal_properties.json (default is 7 days) and can be overridden by the user in TP
 - [#5319](https://github.com/apache/trafficcontrol/issues/5319) - Added support for building RPMs that target CentOS 8
+- Traffic Ops: Added validation to ensure assigned ORG server cachegroups are in the topology when updating a delivery service
 
 ### 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/deliveryservices_test.go b/traffic_ops/testing/api/v3/deliveryservices_test.go
index 5842990..c51ccf8 100644
--- a/traffic_ops/testing/api/v3/deliveryservices_test.go
+++ b/traffic_ops/testing/api/v3/deliveryservices_test.go
@@ -48,6 +48,7 @@ func TestDeliveryServices(t *testing.T) {
 		GetTestDeliveryServicesIMS(t)
 		GetAccessibleToTest(t)
 		UpdateTestDeliveryServices(t)
+		UpdateValidateORGServerCacheGroup(t)
 		UpdateTestDeliveryServicesWithHeaders(t, header)
 		UpdateNullableTestDeliveryServices(t)
 		UpdateDeliveryServiceWithInvalidRemapText(t)
@@ -880,6 +881,53 @@ func UpdateDeliveryServiceWithInvalidSliceRangeRequest(t *testing.T) {
 
 }
 
+// UpdateValidateORGServerCacheGroup validates ORG server's cachegroup are part of topology's cachegroup
+func UpdateValidateORGServerCacheGroup(t *testing.T) {
+	params := url.Values{}
+	params.Set("xmlId", "ds-top")
+
+	//Get the correct DS
+	remoteDS, _, err := TOSession.GetDeliveryServicesV30WithHdr(nil, params)
+	if err != nil {
+		t.Errorf("cannot GET Delivery Services: %v", err)
+	}
+
+	//Assign ORG server to DS
+	assignServer := []string{"denver-mso-org-01"}
+	_, _, err = TOSession.AssignServersToDeliveryService(assignServer, *remoteDS[0].XMLID)
+	if err != nil {
+		t.Errorf("cannot assign server to Delivery Services: %v", err)
+	}
+
+	//Update DS's Topology to a non-ORG server's cachegroup
+	origTopo := *remoteDS[0].Topology
+	*remoteDS[0].Topology = "4-tiers"
+	ds, reqInf, err := TOSession.UpdateDeliveryServiceV30WithHdr(*remoteDS[0].ID, remoteDS[0], nil)
+	if err == nil {
+		t.Errorf("shouldnot UPDATE DeliveryService by ID: %v, but update was successful", ds.XMLID)
+	}
+	if reqInf.StatusCode != http.StatusBadRequest {
+		t.Fatalf("expected to fail since ORG server's topology not part of DS. Expected:%v, Got: :%v", http.StatusBadRequest, reqInf.StatusCode)
+	}
+
+	// Retrieve the DS to check if topology was updated with missing ORG server
+	params.Set("id", strconv.Itoa(*remoteDS[0].ID))
+	apiResp, _, err := TOSession.GetDeliveryServicesV30WithHdr(nil, params)
+	if err != nil {
+		t.Fatalf("cannot GET Delivery Service by ID: %v - %v", *remoteDS[0].XMLID, err)
+	}
+	if len(apiResp) < 1 {
+		t.Fatalf("cannot GET Delivery Service by ID: %v - nil", *remoteDS[0].XMLID)
+	}
+
+	//Set topology back to as it was for further testing
+	remoteDS[0].Topology = &origTopo
+	_, _, err = TOSession.UpdateDeliveryServiceV30WithHdr(*remoteDS[0].ID, remoteDS[0], nil)
+	if err != nil {
+		t.Fatalf("couldn't update topology:%v, %v", *remoteDS[0].Topology, err)
+	}
+}
+
 func GetAccessibleToTest(t *testing.T) {
 	//Every delivery service is associated with the root tenant
 	err := getByTenants(1, len(testData.DeliveryServices))
diff --git a/traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go b/traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
index 53edd6d..09a9e06 100644
--- a/traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
+++ b/traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
@@ -23,7 +23,6 @@ import (
 	"database/sql"
 	"errors"
 	"fmt"
-	"github.com/jmoiron/sqlx"
 	"net/http"
 	"strconv"
 	"strings"
@@ -32,6 +31,7 @@ import (
 	"github.com/apache/trafficcontrol/lib/go-tc"
 	"github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/topology/topology_validation"
 
+	"github.com/jmoiron/sqlx"
 	"github.com/lib/pq"
 )
 
@@ -1090,3 +1090,54 @@ GROUP BY t.name, ds.topology
 	}
 	return dsType, reqCap, topology, true, nil
 }
+
+// CheckOriginServerInCacheGroupTopology checks if a DS has ORG server and if it does, to make sure the cachegroup is part of DS
+func CheckOriginServerInCacheGroupTopology(tx *sql.Tx, dsID int, dsTopology string) (error, error, int) {
+	// get servers and respective cachegroup name that have ORG type in a delivery service
+	q := `
+		SELECT s.host_name, c.name 
+		FROM server s
+			INNER JOIN deliveryservice_server ds ON ds.server = s.id
+			INNER JOIN type t ON t.id = s.type
+			INNER JOIN cachegroup c ON c.id = s.cachegroup
+		WHERE ds.deliveryservice=$1 AND t.name=$2
+	`
+
+	serverName := ""
+	cacheGroupName := ""
+	servers := make(map[string]string)
+	var offendingSCG []string
+	rows, err := tx.Query(q, dsID, tc.OriginTypeName)
+	if err != nil {
+		return nil, fmt.Errorf("querying deliveryservice origin server: %s", err), http.StatusInternalServerError
+	}
+	defer log.Close(rows, "error closing rows")
+	for rows.Next() {
+		if err := rows.Scan(&serverName, &cacheGroupName); err != nil {
+			return nil, fmt.Errorf("querying deliveryservice origin server: %s", err), http.StatusInternalServerError
+		}
+		servers[cacheGroupName] = serverName
+	}
+
+	if len(servers) > 0 {
+		_, cachegroups, sysErr := GetTopologyCachegroups(tx, dsTopology)
+		if sysErr != nil {
+			return nil, fmt.Errorf("validating %s servers in topology: %v", tc.OriginTypeName, sysErr), http.StatusInternalServerError
+		}
+		// put slice values into map
+		topoCachegroups := make(map[string]string)
+		for _, cg := range cachegroups {
+			topoCachegroups[cg] = ""
+		}
+		for cg, s := range servers {
+			_, ok := topoCachegroups[cg]
+			if !ok {
+				offendingSCG = append(offendingSCG, fmt.Sprintf("%s (%s)", cg, s))
+			}
+		}
+	}
+	if len(offendingSCG) > 0 {
+		return errors.New("the following ORG server cachegroups are not in the delivery service's topology (" + dsTopology + "): " + strings.Join(offendingSCG, ", ")), nil, http.StatusBadRequest
+	}
+	return nil, nil, http.StatusOK
+}
diff --git a/traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go b/traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
index 199058a..eb1bad3 100644
--- a/traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
+++ b/traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
@@ -796,6 +796,11 @@ func updateV30(w http.ResponseWriter, r *http.Request, inf *api.APIInfo, ds *tc.
 				return nil, status, userErr, sysErr
 			}
 		}
+
+		userErr, sysErr, status := dbhelpers.CheckOriginServerInCacheGroupTopology(tx, *ds.ID, *ds.Topology)
+		if userErr != nil || sysErr != nil {
+			return nil, status, userErr, sysErr
+		}
 	}
 
 	resultRows, err := tx.Query(updateDSQuery(),
diff --git a/traffic_ops/traffic_ops_golang/topology/topology_validation/topology_validation.go b/traffic_ops/traffic_ops_golang/topology/topology_validation/topology_validation.go
index e91f9e7..de19198 100644
--- a/traffic_ops/traffic_ops_golang/topology/topology_validation/topology_validation.go
+++ b/traffic_ops/traffic_ops_golang/topology/topology_validation/topology_validation.go
@@ -25,11 +25,12 @@ package topology_validation
 import (
 	"errors"
 	"fmt"
-	"github.com/jmoiron/sqlx"
-	"github.com/lib/pq"
 	"strings"
 
 	"github.com/apache/trafficcontrol/lib/go-log"
+
+	"github.com/jmoiron/sqlx"
+	"github.com/lib/pq"
 )
 
 // CheckForEmptyCacheGroups checks if the cachegroups are empty (altogether) or empty in any of the given CDN IDs.