[trafficcontrol] branch 5.0.x updated: Topology based DS's + reqd capabilities + ORG servers should show ORG servers, alongwith the other servers when queried for the linked servers (#5389)

[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 cd78596  Topology based DS's + reqd capabilities + ORG servers should show ORG servers, alongwith the other servers when queried for the linked servers (#5389)
cd78596 is described below

commit cd785960a6c172a612c6587c672113fe85ec7e3c
Author: Srijeet Chatterjee <30...@users.noreply.github.com>
AuthorDate: Tue Dec 22 10:57:21 2020 -0700

    Topology based DS's + reqd capabilities + ORG servers should show ORG servers, alongwith the other servers when queried for the linked servers (#5389)
    
    * wip
    
    * wip
    
    * modified query
    
    * adding test
    
    * Adding new test
    
    * adding test in the right file
    
    * refactoring
    
    * changing error msg
    
    * Adding changelog
    
    * code review fixes
    
    * code review fixes
    
    (cherry picked from commit bef00082a6b0d06ca1dc3d25996feff9acd3e362)
---
 CHANGELOG.md                                       |  1 +
 .../deliveryservices_required_capabilities_test.go | 36 ++++++++++++++++
 traffic_ops/traffic_ops_golang/server/servers.go   | 48 +++++++++++++++-------
 .../traffic_ops_golang/server/servers_test.go      |  4 ++
 4 files changed, 74 insertions(+), 15 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index b6eb2f4..7f33ac4 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -139,6 +139,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 - Fixed Astats csv issue where it could crash if caches dont return proc data
 - Fixed parent.config generation for topology-based delivery services (inline comments not supported)
 - Fixed parent.config generation for MSO delivery services with required capabilities
+- [#5380](https://github.com/apache/trafficcontrol/issues/5380) - Show the correct servers (including ORGs) when a topology based DS with required capabilities + ORG servers is queried for the assigned servers
 
 ### Changed
 - Changed some Traffic Ops Go Client methods to use `DeliveryServiceNullable` inputs and outputs.
diff --git a/traffic_ops/testing/api/v3/deliveryservices_required_capabilities_test.go b/traffic_ops/testing/api/v3/deliveryservices_required_capabilities_test.go
index 9a9862e..ea05fdd 100644
--- a/traffic_ops/testing/api/v3/deliveryservices_required_capabilities_test.go
+++ b/traffic_ops/testing/api/v3/deliveryservices_required_capabilities_test.go
@@ -19,6 +19,7 @@ import (
 	"fmt"
 	"net/http"
 	"net/url"
+	"strconv"
 	"strings"
 	"testing"
 	"time"
@@ -45,9 +46,44 @@ func TestDeliveryServicesRequiredCapabilities(t *testing.T) {
 func TestTopologyBasedDeliveryServicesRequiredCapabilities(t *testing.T) {
 	WithObjs(t, []TCObj{CDNs, Types, Tenants, Users, Parameters, Profiles, Statuses, Divisions, Regions, PhysLocations, CacheGroups, Servers, ServerCapabilities, ServerServerCapabilitiesForTopologies, Topologies, DeliveryServices, TopologyBasedDeliveryServiceRequiredCapabilities}, func() {
 		GetTestDeliveryServicesRequiredCapabilities(t)
+		OriginAssignTopologyBasedDeliveryServiceWithRequiredCapabilities(t)
 	})
 }
 
+func OriginAssignTopologyBasedDeliveryServiceWithRequiredCapabilities(t *testing.T) {
+	resp, _, err := TOSession.GetDeliveryServiceByXMLIDNullableWithHdr("ds-top-req-cap2", nil)
+	if err != nil {
+		t.Errorf("getting delivery service by xml ID: %v", err.Error())
+	}
+	if len(resp) != 1 {
+		t.Fatalf("expected to get only one delivery service in the response, but got %d", len(resp))
+	}
+	if resp[0].ID == nil {
+		t.Fatalf("no ID in the resulting delivery service")
+	}
+	dsID := *resp[0].ID
+	params := url.Values{}
+	_, _, err = TOSession.AssignServersToDeliveryService([]string{"denver-mso-org-01"}, "ds-top-req-cap2")
+	if err != nil {
+		t.Errorf("assigning ORG server to ds-top delivery service: %v", err.Error())
+	}
+	params.Add("dsId", strconv.Itoa(dsID))
+	params.Add("type", tc.OriginTypeName)
+	responseServers, _, err := TOSession.GetServersWithHdr(&params, nil)
+	if err != nil {
+		t.Fatalf("getting servers for ds-top-req-cap2 delivery service: %v", err.Error())
+	}
+	if len(responseServers.Response) != 1 {
+		t.Fatalf("expected just one ORG server in the response, but got %d", len(responseServers.Response))
+	}
+	if responseServers.Response[0].HostName == nil {
+		t.Fatal("expected a valid host name for the resulting ORG server, but got nothing")
+	}
+	if *responseServers.Response[0].HostName != "denver-mso-org-01" {
+		t.Errorf("expected host name of the resulting ORG server to be %v, but got %v", "denver-mso-org-01", *responseServers.Response[0].HostName)
+	}
+}
+
 func GetTestDeliveryServicesRequiredCapabilitiesIMSAfterChange(t *testing.T, header http.Header) {
 	data := testData.DeliveryServicesRequiredCapabilities
 	ds1 := helperGetDeliveryServiceID(t, data[0].XMLID)
diff --git a/traffic_ops/traffic_ops_golang/server/servers.go b/traffic_ops/traffic_ops_golang/server/servers.go
index 3765e86..0690250 100644
--- a/traffic_ops/traffic_ops_golang/server/servers.go
+++ b/traffic_ops/traffic_ops_golang/server/servers.go
@@ -399,6 +399,12 @@ RETURNING
 	upd_pending
 `
 
+const originServerQuery = `
+JOIN deliveryservice_server dsorg 
+ON dsorg.server = s.id 
+WHERE t.name = '` + tc.OriginTypeName + `' 
+AND dsorg.deliveryservice=:dsId
+`
 const deleteServerQuery = `DELETE FROM server WHERE id=$1`
 const deleteInterfacesQuery = `DELETE FROM interface WHERE server=$1`
 const deleteIPsQuery = `DELETE FROM ip_address WHERE server = $1`
@@ -765,6 +771,19 @@ JOIN type t ON s.type = t.id ` +
 	select max(last_updated) as t from last_deleted l where l.table_name='server') as res`
 }
 
+func getServerCount(tx *sqlx.Tx, query string, queryValues map[string]interface{}) (uint64, error) {
+	var serverCount uint64
+	ns, err := tx.PrepareNamed(query)
+	if err != nil {
+		return 0, fmt.Errorf("couldn't prepare the query to get server count : %v", err)
+	}
+	err = tx.NamedStmt(ns).QueryRow(queryValues).Scan(&serverCount)
+	if err != nil {
+		return 0, fmt.Errorf("failed to get servers count: %v", err)
+	}
+	return serverCount, nil
+}
+
 func getServers(h http.Header, params map[string]string, tx *sqlx.Tx, user *auth.CurrentUser, useIMS bool, version api.Version) ([]tc.ServerNullable, uint64, error, error, int, *time.Time) {
 	var maxTime time.Time
 	var runSecond bool
@@ -792,6 +811,9 @@ func getServers(h http.Header, params map[string]string, tx *sqlx.Tx, user *auth
 	queryAddition := ""
 	dsHasRequiredCapabilities := false
 	var cdnID int
+	var serverCount uint64
+	var err error
+
 	if dsIDStr, ok := params[`dsId`]; ok {
 		// don't allow query on ds outside user's tenant
 		dsID, err := strconv.Atoi(dsIDStr)
@@ -848,23 +870,15 @@ func getServers(h http.Header, params map[string]string, tx *sqlx.Tx, user *auth
 		return nil, 0, util.JoinErrs(errs), nil, http.StatusBadRequest, nil
 	}
 
-	// TODO there's probably a cleaner way to do this by preparing a NamedStmt first and using its QueryRow method
-	var serverCount uint64
-	countRows, err := tx.NamedQuery(serverCountQuery+queryAddition+where, queryValues)
+	countQuery := serverCountQuery + queryAddition + where
+	// If we are querying for a DS that has reqd capabilities, we need to make sure that we also include all the ORG servers directly assigned to this DS
+	if _, ok := params["dsId"]; ok && dsHasRequiredCapabilities {
+		countQuery = `SELECT (` + countQuery + `) + (` + serverCountQuery + originServerQuery + `) AS total`
+	}
+	serverCount, err = getServerCount(tx, countQuery, queryValues)
 	if err != nil {
 		return nil, 0, nil, fmt.Errorf("failed to get servers count: %v", err), http.StatusInternalServerError, nil
 	}
-	defer countRows.Close()
-	rowsAffected := 0
-	for countRows.Next() {
-		if err = countRows.Scan(&serverCount); err != nil {
-			return nil, 0, nil, fmt.Errorf("failed to read servers count: %v", err), http.StatusInternalServerError, nil
-		}
-		rowsAffected++
-	}
-	if rowsAffected != 1 {
-		return nil, 0, nil, fmt.Errorf("incorrect rows returned for server count, want: 1 got: %v", rowsAffected), http.StatusInternalServerError, nil
-	}
 
 	serversList := []tc.ServerNullable{}
 	if useIMS {
@@ -879,8 +893,12 @@ func getServers(h http.Header, params map[string]string, tx *sqlx.Tx, user *auth
 	}
 
 	query := selectQuery + queryAddition + where + orderBy + pagination
-	log.Debugln("Query is ", query)
+	// If you're looking to get the servers for a particular delivery service, make sure you're also querying the ORG servers from the deliveryservice_server table
+	if _, ok := params[`dsId`]; ok {
+		query = `(` + selectQuery + queryAddition + where + orderBy + pagination + `) UNION ` + selectQuery + originServerQuery
+	}
 
+	log.Debugln("Query is ", query)
 	rows, err := tx.NamedQuery(query, queryValues)
 	if err != nil {
 		return nil, serverCount, nil, errors.New("querying: " + err.Error()), http.StatusInternalServerError, nil
diff --git a/traffic_ops/traffic_ops_golang/server/servers_test.go b/traffic_ops/traffic_ops_golang/server/servers_test.go
index 6bc5651..7121cfd 100644
--- a/traffic_ops/traffic_ops_golang/server/servers_test.go
+++ b/traffic_ops/traffic_ops_golang/server/servers_test.go
@@ -248,6 +248,8 @@ func TestGetServersByCachegroup(t *testing.T) {
 	}
 
 	mock.ExpectBegin()
+	mock.ExpectPrepare("SELECT COUNT\\(s.id\\) FROM s")
+	mock.ExpectPrepare("SELECT COUNT\\(s.id\\) FROM s")
 	mock.ExpectQuery("SELECT COUNT\\(s.id\\) FROM s").WillReturnRows(unfilteredRows)
 	mock.ExpectQuery("SELECT").WillReturnRows(rows)
 	mock.ExpectQuery("SELECT").WillReturnRows(interfaceRows)
@@ -356,6 +358,8 @@ func TestGetMidServers(t *testing.T) {
 		}
 	}
 	mock.ExpectBegin()
+	mock.ExpectPrepare("SELECT COUNT\\(s.id\\) FROM s")
+	mock.ExpectPrepare("SELECT COUNT\\(s.id\\) FROM s")
 	mock.ExpectQuery("SELECT COUNT\\(s.id\\) FROM s").WillReturnRows(unfilteredRows)
 	mock.ExpectQuery("SELECT").WillReturnRows(rows)
 	mock.ExpectQuery("SELECT").WillReturnRows(interfaceRows)