strips out mids in the wrong cdn when requesting servers for a ds #5128

mitchell852 · 2020-10-08T21:35:50Z

This PR fixes GET /servers?dsId={id} returning mid servers outside of the ds's cdn #5147

Which Traffic Control components are affected by this PR?

Traffic Ops
CI tests

What is the best way to verify this PR?

Run the API tests

If this is a bug fix, what versions of Traffic Control are affected?

master (eecf076)

The following criteria are ALL met by this PR

This PR includes tests OR I have explained why tests are unnecessary
This PR includes documentation OR I have explained why documentation is unnecessary
This PR includes an update to CHANGELOG.md OR such an update is not necessary
This PR includes any and all required license headers
This PR ensures that database migration sequence is correct OR this PR does not include a database migration
This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY (see the Apache Software Foundation's security guidelines for details)

Additional Information

zrhoffman · 2020-10-14T15:08:08Z

traffic_ops/traffic_ops_golang/server/servers.go

+		_, dsCDNName, _, err = dbhelpers.GetDSNameAndCDNFromID(tx.Tx, dsID)
+		if err != nil {
+			return nil, 0, errors.New("ds not found"), nil, http.StatusNotFound, nil
+		}


We should not need to check which CDN the delivery service is in. The query just needs to be adjusted to make sure the servers match the delivery service's CDN.

well, it doesn't always check the cdn. for example:

GET /servers <-- doesn't check cdn
GET /servers?type=ORG <-- doesn't check cdn
GET /servers?dsId=X <-- does check cdn

Right, but it always should check the CDN when the dsId query parameter exists, and all of the SQL for that part is in deliveryServiceServersJoin and dssTopologiesJoinSubquery.

zrhoffman · 2020-10-14T15:08:30Z

traffic_ops/traffic_ops_golang/server/servers.go

+		// if requesting servers for a given ds, need to only include those in the same cdn
+		if len(dsCDNName) > 0 && string(dsCDNName) != *server.CDNName {
+			continue
+		}


Is there a way to perform this check in the query itself?

yeah, i think you're right. best if done in the query but i had no luck figuring that out. i'll look again.

@zrhoffman - updated. how's that look?

traffic_ops/traffic_ops_golang/server/servers.go

zrhoffman · 2020-10-14T23:02:42Z

Servers API tests fail

[user@computer api]$ time go test ./v3 -test.v -cfg=../conf/traffic-ops-test.conf -fixtures=tc-fixtures.json -test.run '^TestServers$'
INFO: traffic_ops_test.go:71: 2020-10-14T17:00:48.15509479-06:00: Using Config values:
			   TO Config File:       ../conf/traffic-ops-test.conf
			   TO Fixtures:          tc-fixtures.json
			   TO URL:               https://localhost:6443
			   TO Session Timeout In Secs:  60
			   DB Server:            localhost
			   DB User:              traffic_ops
			   DB Name:              traffic_ops
			   DB Ssl:               false
               UseIMS:               true
=== RUN   TestServers
    cdns_test.go:89: Response:  {[{cdn was created. success}]}
    cdns_test.go:89: Response:  {[{cdn was created. success}]}
    cdns_test.go:89: Response:  {[{cdn was created. success}]}
    cdns_test.go:89: Response:  {[{cdn was created. success}]}
    cdns_test.go:89: Response:  {[{cdn was created. success}]}
    types_test.go:89: ---- CreateTestTypes ----
    user_test.go:103: Response:  {[{user was created. success}]}
    user_test.go:103: Response:  {[{user was created. success}]}
    user_test.go:103: Response:  {[{user was created. success}]}
    user_test.go:103: Response:  {[{user was created. success}]}
    user_test.go:103: Response:  {[{user was created. success}]}
    user_test.go:103: Response:  {[{user was created. success}]}
    user_test.go:103: Response:  {[{user was created. success}]}
    parameters_test.go:75: Response:  {[{param was created. success}]}
    parameters_test.go:75: Response:  {[{param was created. success}]}
    parameters_test.go:75: Response:  {[{param was created. success}]}
    parameters_test.go:75: Response:  {[{param was created. success}]}
    parameters_test.go:75: Response:  {[{param was created. success}]}
    parameters_test.go:75: Response:  {[{param was created. success}]}
    parameters_test.go:75: Response:  {[{param was created. success}]}
    parameters_test.go:75: Response:  {[{param was created. success}]}
    parameters_test.go:75: Response:  {[{param was created. success}]}
    parameters_test.go:75: Response:  {[{param was created. success}]}
    parameters_test.go:75: Response:  {[{param was created. success}]}
    parameters_test.go:75: Response:  {[{param was created. success}]}
    parameters_test.go:75: Response:  {[{param was created. success}]}
    parameters_test.go:75: Response:  {[{param was created. success}]}
    parameters_test.go:75: Response:  {[{param was created. success}]}
    parameters_test.go:75: Response:  {[{param was created. success}]}
    parameters_test.go:75: Response:  {[{param was created. success}]}
    parameters_test.go:75: Response:  {[{param was created. success}]}
    parameters_test.go:75: Response:  {[{param was created. success}]}
    parameters_test.go:75: Response:  {[{param was created. success}]}
    profiles_test.go:162: Response:  {[{profile was created. success}]}
    profiles_test.go:162: Response:  {[{profile was created. success}]}
    profiles_test.go:162: Response:  {[{profile was created. success}]}
    profiles_test.go:162: Response:  {[{profile was created. success}]}
    profiles_test.go:162: Response:  {[{profile was created. success}]}
    profiles_test.go:162: Response:  {[{profile was created. success}]}
    profiles_test.go:162: Response:  {[{profile was created. success}]}
    profiles_test.go:162: Response:  {[{profile was created. success}]}
    profiles_test.go:162: Response:  {[{profile was created. success}]}
    profiles_test.go:162: Response:  {[{profile was created. success}]}
    profiles_test.go:162: Response:  {[{profile was created. success}]}
    profiles_test.go:162: Response:  {[{profile was created. success}]}
    profiles_test.go:162: Response:  {[{profile was created. success}]}
    statuses_test.go:98: Response:  {[{status was created. success}]}
    statuses_test.go:98: Response:  {[{status was created. success}]}
    statuses_test.go:98: Response:  {[{status was created. success}]}
    statuses_test.go:98: Response:  {[{status was created. success}]}
    statuses_test.go:98: Response:  {[{status was created. success}]}
    statuses_test.go:98: Response:  {[{status was created. success}]}
    statuses_test.go:98: Response:  {[{status was created. success}]}
    divisions_test.go:117: Response:  {[{division was created. success}]}
    divisions_test.go:117: Response:  {[{division was created. success}]}
    regions_test.go:91: Response:  {[{region was created. success}]}
    regions_test.go:91: Response:  {[{region was created. success}]}
    phys_locations_test.go:91: Response:  {[{physLocation was created. success}]}
    phys_locations_test.go:91: Response:  {[{physLocation was created. success}]}
    phys_locations_test.go:91: Response:  {[{physLocation was created. success}]}
    topologies_test.go:56: Response:  &{{a topology my-topology [{0 parentCachegroup [] <nil>} {0 secondaryCachegroup [] <nil>} {0 cachegroup3 [0 1] <nil>}] <nil>} {[{topology was created. success}]}}
    topologies_test.go:56: Response:  &{{another topology another-topology [{0 parentCachegroup [] <nil>} {0 cachegroup1 [0] <nil>} {0 secondaryCachegroup [] <nil>} {0 cachegroup2 [2] <nil>}] <nil>} {[{topology was created. success}]}}
    topologies_test.go:56: Response:  &{{A topology with secondary parents secondary-parents [{0 parentCachegroup [] <nil>} {0 cachegroup1 [0 2] <nil>} {0 secondaryCachegroup [] <nil>} {0 fallback1 [2 0] <nil>} {0 fallback2 [2 0] <nil>}] <nil>} {[{topology was created. success}]}}
    topologies_test.go:56: Response:  &{{A 4-tier topology 4-tiers [{0 parentCachegroup [] <nil>} {0 parentCachegroup2 [0] <nil>} {0 cachegroup1 [1] <nil>} {0 secondaryCachegroup [1 0] <nil>} {0 fallback1 [3] <nil>}] <nil>} {[{topology was created. success}]}}
    topologies_test.go:56: Response:  &{{This topology stems from 2 ancestors forked-topology [{0 topology-edge-cg-01 [2] <nil>} {0 topology-edge-cg-02 [6] <nil>} {0 topology-mid-cg-01 [3] <nil>} {0 topology-mid-cg-02 [4] <nil>} {0 topology-mid-cg-03 [5] <nil>} {0 topology-mid-cg-04 [] <nil>} {0 topology-mid-cg-05 [7] <nil>} {0 topology-mid-cg-06 [8] <nil>} {0 topology-mid-cg-07 [] <nil>}] <nil>} {[{topology was created. success}]}}
    topologies_test.go:56: Response:  &{{a topology top-for-ds-req [{0 dtrc1 [] <nil>} {0 dtrc2 [0] <nil>} {0 dtrc3 [0] <nil>}] <nil>} {[{topology was created. success}]}}
    topologies_test.go:56: Response:  &{{a topology top-for-ds-req2 [{0 dtrc1 [] <nil>} {0 dtrc2 [0] <nil>}] <nil>} {[{topology was created. success}]}}
    servers_test.go:246: Response:  atlanta-edge-01   {[{Server created success}]}
    servers_test.go:246: Response:  cdn2-test-edge   {[{Server created success}]}
    servers_test.go:246: Response:  influxdb02   {[{Server created success}]}
    servers_test.go:246: Response:  atlanta-router-01   {[{Server created success}]}
    servers_test.go:246: Response:  atlanta-edge-03   {[{Server created success}]}
    servers_test.go:246: Response:  atlanta-edge-14   {[{Server created success}]}
    servers_test.go:246: Response:  atlanta-edge-15   {[{Server created success}]}
    servers_test.go:246: Response:  atlanta-mid-16   {[{Server created success}]}
    servers_test.go:246: Response:  atlanta-mid-17   {[{Server created success}]}
    servers_test.go:246: Response:  atlanta-org-1   {[{Server created success}]}
    servers_test.go:246: Response:  atlanta-org-2   {[{Server created success}]}
    servers_test.go:246: Response:  atlanta-mid-01   {[{Server created success}]}
    servers_test.go:246: Response:  rascal01   {[{Server created success}]}
    servers_test.go:246: Response:  edge1-cdn2   {[{Server created success}]}
    servers_test.go:246: Response:  riak   {[{Server created success}]}
    servers_test.go:246: Response:  denver-mso-org-01   {[{Server created success}]}
    servers_test.go:246: Response:  edge1-cdn1-cg3   {[{Server created success}]}
    servers_test.go:246: Response:  edge2-cdn1-cg3   {[{Server created success}]}
    servers_test.go:246: Response:  topology-edge-01   {[{Server created success}]}
    servers_test.go:246: Response:  topology-edge-02   {[{Server created success}]}
    servers_test.go:246: Response:  topology-mid-04   {[{Server created success}]}
    servers_test.go:246: Response:  dtrc-mid-01   {[{Server created success}]}
    servers_test.go:246: Response:  dtrc-mid-02   {[{Server created success}]}
    servers_test.go:246: Response:  dtrc-mid-03   {[{Server created success}]}
    servers_test.go:246: Response:  dtrc-edge-01   {[{Server created success}]}
    servers_test.go:246: Response:  dtrc-edge-02   {[{Server created success}]}
    servers_test.go:246: Response:  dtrc-edge-03   {[{Server created success}]}
    servers_test.go:246: Response:  dtrc-edge-04   {[{Server created success}]}
    servers_test.go:246: Response:  dtrc-edge-05   {[{Server created success}]}
    servers_test.go:246: Response:  dtrc-edge-06   {[{Server created success}]}
    servers_test.go:246: Response:  dtrc-edge-07   {[{Server created success}]}
    servers_test.go:246: Response:  dtrc-edge-08   {[{Server created success}]}
    servers_test.go:246: Response:  dtrc-mid-04   {[{Server created success}]}
    servers_test.go:753: type change update alerts: {Alerts:[]}
    servers_to_deliveryservice_assignment_test.go:74: alerts: {Alerts:[{Text:successfully assigned dses to server Level:success}]}
    servers_to_deliveryservice_assignment_test.go:77: response: [{DeliveryServiceNullableV14:{DeliveryServiceNullableV13:{DeliveryServiceNullableV12:{DeliveryServiceNullableV11:{Active:0xc002038478 AnonymousBlockingEnabled:0xc002038479 CacheURL:0xc00224cd00 CCRDNSTTL:0xc0020384a0 CDNID:0xc0020384b0 CDNName:0xc00224cd10 CheckPath:0xc00224cd20 DisplayName:0xc00224cd30 DNSBypassCNAME:<nil> DNSBypassIP:0xc00224cd40 DNSBypassIP6:0xc00224cd50 DNSBypassTTL:0xc0020384d0 DSCP:0xc0020384e0 EdgeHeaderRewrite:0xc00224cd70 GeoLimit:0xc0020384f0 GeoLimitCountries:0xc00224cd90 GeoLimitRedirectURL:<nil> GeoProvider:0xc0020384f8 GlobalMaxMBPS:0xc002038500 GlobalMaxTPS:0xc002038508 HTTPBypassFQDN:0xc00224cda0 ID:0xc002038510 InfoURL:0xc00224cdb0 InitialDispersion:0xc002038520 IPV6RoutingEnabled:0xc002038528 LastUpdated:0001-01-01 00:00:00 +0000 UTC LogsEnabled:0xc002038529 LongDesc:0xc00224cdc0 LongDesc1:0xc00224cde0 LongDesc2:0xc00224cdf0 MatchList:0xc002234e00 MaxDNSAnswers:0xc002038538 MidHeaderRewrite:0xc00224ce00 MissLat:0xc002038560 MissLong:0xc002038568 MultiSiteOrigin:0xc00203858a OriginShield:<nil> OrgServerFQDN:0xc00224ce10 ProfileDesc:<nil> ProfileID:<nil> ProfileName:<nil> Protocol:0xc002038590 QStringIgnore:0xc002038598 RangeRequestHandling:0xc0020385a0 RegexRemap:0xc00224ce20 RegionalGeoBlocking:0xc0020385af RemapText:0xc00224ce30 RoutingName:0xc00224ce40 Signed:true SSLKeyVersion:0xc0020385c8 TenantID:0xc0020385d0 Type:HTTP TypeID:0xc002038690 XMLID:0xc00224ce60 ExampleURLs:[http://ccr-ds1.ds1.test.cdn1.net https://ccr-ds1.ds1.test.cdn1.net]}} DeepCachingType:NEVER FQPacingRate:0xc0020386e8 SigningAlgorithm:0xc00224ce80 Tenant:0xc00224ce90 TRResponseHeaders:<nil> TRRequestHeaders:<nil>} ConsistentHashRegex:<nil> ConsistentHashQueryParams:[] MaxOriginConnections:0xc002038700} EcsEnabled:false RangeSliceBlockSize:<nil>}]
    servers_test.go:365: expected to get 1 server for Delivery Service: 322, actual: 2
    types_test.go:208: ---- DeleteTestTypes ----
--- FAIL: TestServers (11.69s)
FAIL
FAIL	github.com/apache/trafficcontrol/traffic_ops/testing/api/v3	12.020s
FAIL

zrhoffman · 2020-10-14T23:15:19Z

traffic_ops/testing/api/v3/deliveryserviceservers_test.go

+	serversResp, _, err := TOSession.GetServersWithHdr(nil, nil)
+	serversIds := []int{}
+	for _, s := range serversResp.Response {


Should check err before trying to use serversResp

zrhoffman · 2020-10-14T23:15:51Z

traffic_ops/testing/api/v3/deliveryserviceservers_test.go

+	dsServersResp, _, err := TOSession.GetServersWithHdr(&params, nil)
+	dsServerIds := []int{}
+	for _, dss := range dsServersResp.Response {


Another case that should check err before trying to use serversResp

zrhoffman · 2020-10-14T23:18:07Z

traffic_ops/testing/api/v3/deliveryserviceservers_test.go

+
+	_, _, err = TOSession.CreateDeliveryServiceServers(*dsWithMid[0].ID, serversIds, true)
+	if err != nil {
+		t.Errorf("POST delivery service servers: %v", err)


The rest of the function uses t.Fatalf(), why use t.Errorf() here?

good question

zrhoffman · 2020-10-14T23:30:04Z

traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go

+func GetDSCDNIdFromID(tx *sql.Tx, id int) (int, bool, error) {
+	var cdnID int


cdnID is a good name because we know that it refers to the ID of a CDN. id is less clear, the variable name should mention what kind of ID it is

mitchell852 · 2020-10-15T01:31:37Z

servers_test.go:365: expected to get 1 server for Delivery Service: 322, actual: 2
types_test.go:208: ---- DeleteTestTypes ----
--- FAIL: TestServers (11.69s)
FAIL
FAIL github.com/apache/trafficcontrol/traffic_ops/testing/api/v3 12.020s
FAIL

so strange. all passed for me before. checking.

zrhoffman · 2020-10-15T12:54:19Z

TestServers API test gets a little further now but still fails. Last 10 lines:

    servers_test.go:246: Response:  dtrc-mid-04   {[{Server created success}]}
    servers_test.go:753: type change update alerts: {Alerts:[]}
    servers_to_deliveryservice_assignment_test.go:74: alerts: {Alerts:[{Text:successfully assigned dses to server Level:success}]}
    servers_to_deliveryservice_assignment_test.go:77: response: [{DeliveryServiceNullableV14:{DeliveryServiceNullableV13:{DeliveryServiceNullableV12:{DeliveryServiceNullableV11:{Active:0xc000249ad8 AnonymousBlockingEnabled:0xc000249ad9 CacheURL:0xc0020fa9a0 CCRDNSTTL:0xc000249b00 CDNID:0xc000249b10 CDNName:0xc0020fa9c0 CheckPath:0xc0020fa9e0 DisplayName:0xc0020fa9f0 DNSBypassCNAME:<nil> DNSBypassIP:0xc0020faa00 DNSBypassIP6:0xc0020faa10 DNSBypassTTL:0xc000249b30 DSCP:0xc000249b40 EdgeHeaderRewrite:0xc0020faa30 GeoLimit:0xc000249b50 GeoLimitCountries:0xc0020faa40 GeoLimitRedirectURL:<nil> GeoProvider:0xc000249b58 GlobalMaxMBPS:0xc000249b60 GlobalMaxTPS:0xc000249b68 HTTPBypassFQDN:0xc0020faa50 ID:0xc000249b70 InfoURL:0xc0020faa60 InitialDispersion:0xc000249b80 IPV6RoutingEnabled:0xc000249b88 LastUpdated:0001-01-01 00:00:00 +0000 UTC LogsEnabled:0xc000249b89 LongDesc:0xc0020faa70 LongDesc1:0xc0020faa80 LongDesc2:0xc0020faa90 MatchList:0xc00160a640 MaxDNSAnswers:0xc000249b98 MidHeaderRewrite:0xc0020faaa0 MissLat:0xc000249bc0 MissLong:0xc000249bc8 MultiSiteOrigin:0xc000249bea OriginShield:<nil> OrgServerFQDN:0xc0020faab0 ProfileDesc:<nil> ProfileID:<nil> ProfileName:<nil> Protocol:0xc000249bf0 QStringIgnore:0xc000249bf8 RangeRequestHandling:0xc000249c00 RegexRemap:0xc0020faac0 RegionalGeoBlocking:0xc000249c0f RemapText:0xc0020faad0 RoutingName:0xc0020faae0 Signed:true SSLKeyVersion:0xc000249c28 TenantID:0xc000249c30 Type:HTTP TypeID:0xc000249c40 XMLID:0xc0020fab10 ExampleURLs:[http://ccr-ds1.ds1.test.cdn1.net https://ccr-ds1.ds1.test.cdn1.net]}} DeepCachingType:NEVER FQPacingRate:0xc000249c58 SigningAlgorithm:0xc0020fab40 Tenant:0xc0020fab50 TRResponseHeaders:<nil> TRRequestHeaders:<nil>} ConsistentHashRegex:<nil> ConsistentHashQueryParams:[] MaxOriginConnections:0xc000249c70} EcsEnabled:false RangeSliceBlockSize:<nil>}]
    servers_test.go:458: expected hostnames map[atlanta-mid-16:true edge1-cdn1-cg3:true edge2-cdn1-cg3:true], actual atlanta-mid-17 actual: 
    types_test.go:208: ---- DeleteTestTypes ----
--- FAIL: TestServers (12.39s)
FAIL
FAIL	github.com/apache/trafficcontrol/traffic_ops/testing/api/v3	15.930s
FAIL

mitchell852 · 2020-10-15T14:05:11Z

@zrhoffman - do you understand why the api tests are failing?

zrhoffman · 2020-10-15T14:27:36Z

Look at the expectedHostnames array. If it encounters a host that is not in that array, it fails on line 458. In the excerpt of the API tests I posted:

servers_test.go:458: expected hostnames map[atlanta-mid-16:true edge1-cdn1-cg3:true edge2-cdn1-cg3:true], actual atlanta-mid-17 actual:

It means that the only servers it expected in the response were

atlanta-mid-16
edge1-cdn1-cg3
edge2-cdn1-cg3

but it found atlanta-mid-17 as well, so it failed.

That actual: part at the end of the message is extra and should be removed (my bad).

So, look at whether atlanta-mid-17 should be included in the result. If it should, add it to the array. If it should not, then there's a bug that should be fixed. I would recommend against simply moving it to a different cachegroup like 1026a81bb6 does.

zrhoffman · 2020-10-15T14:28:34Z

traffic_ops/testing/api/v3/tc-fixtures.json

@@ -2317,7 +2317,7 @@
            "xmppPasswd": "X"
        },
        {
-            "cachegroup": "parentCachegroup",
+            "cachegroup": "parentCachegroup2",


Why does the test fail when atlanta-mid-17 is in parentCachegroup?

parentCachegroup was used as part of a topology so i just put it in another cg so not to mess w/ that test. but all passes now.

It shouldn't matter that it is used as part of a topology. atlanta-mid-17 is in CDN cdn2. The delivery service used in the test has xmlId ds-top and is in CDN cdn1. So, atlanta-mid-17 being returned in the response is a bug because it's in a different CDN, right?

looking. its hard imo to weed thru all these fixtures and map them to the test cases they are fulfilling. but maybe you're right. maybe it uncovered a different bug...

reverting that.... parentCachegroup is fine for my test...now to figure out why it's breaking the topology test..

mitchell852 · 2020-10-15T14:38:12Z

Look at the expectedHostnames array. If it encounters a host that is not in that array, it fails on line 458. In the excerpt of the API tests I posted:
servers_test.go:458: expected hostnames map[atlanta-mid-16:true edge1-cdn1-cg3:true edge2-cdn1-cg3:true], actual atlanta-mid-17 actual:
It means that the only servers it expected in the response were

atlanta-mid-16

edge1-cdn1-cg3

edge2-cdn1-cg3

but it found atlanta-mid-17 as well, so it failed.

That actual: part at the end of the message is extra and should be removed (my bad).

So, look at whether atlanta-mid-17 should be included in the result. If it should, add it to the array. If it should not, then there's a bug that should be fixed. I would recommend against simply moving it to a different cachegroup like 1026a81 does.

honestly, i could not find that in the GHA output....

zrhoffman · 2020-10-15T14:42:13Z

honestly, i could not find that in the GHA output....

Neither could I. Your best bet is running the tests locally in CDN-in-a-Box.

mitchell852 · 2020-10-15T17:40:51Z

So, look at whether atlanta-mid-17 should be included in the result. If it should, add it to the array. If it should not, then there's a bug that should be fixed. I would recommend against simply moving it to a different cachegroup like 1026a81 does.

yeah, looks like a bug. atlanta-mid-17 should not be in there as atlanta-mid-17 is in a different cdn than the ds...

traffic_ops/traffic_ops_golang/server/servers_test.go

traffic_ops/traffic_ops_golang/server/servers.go

zrhoffman · 2020-10-15T19:23:36Z

yeah, looks like a bug. atlanta-mid-17 should not be in there as atlanta-mid-17 is in a different cdn than the ds...

Looking at

trafficcontrol/traffic_ops/testing/api/v3/servers_test.go

Lines 493 to 508 in ecb3806

    
           params.Del("dsId") 
        
           params.Add("topology", topology) 
        
           response, _, err = TOSession.GetServersWithHdr(&params, nil) 
        
           if err != nil { 
        
           	t.Fatalf("Failed to get servers belonging to cachegroups in topology %s: %s", topology, err) 
        
           } 
        
           if len(response.Response) == 0 { 
        
           	t.Fatalf("Did not find any servers belonging to cachegroups in topology %s: %s", topology, err) 
        
           } 
        
           for _, server := range response.Response { 
        
           	if _, exists := expectedHostnames[*server.HostName]; !exists { 
        
           		t.Fatalf("expected hostnames %v, actual %s actual: ", expectedHostnames, *server.HostName) 
        
           	} 
        
           } 
        
           params.Del("topology")

the query parameter that is failing in the test is topology. In the database, atlanta-mid-17 is there, so I think it can be safely added to the list.

… same cdn as the ds

…al bug

zrhoffman

Tests pass. Manual testing confirms only servers within a Delivery Service's CDN are returned when querying servers by Delivery Service.

mitchell852 force-pushed the fixes-non-cdn-servers branch from d010be2 to 451e8d6 Compare October 14, 2020 01:28

mitchell852 added Traffic Ops related to Traffic Ops bug something isn't working as intended labels Oct 14, 2020

mitchell852 marked this pull request as ready for review October 14, 2020 01:53

mitchell852 force-pushed the fixes-non-cdn-servers branch from a8a1f67 to f4e9cfb Compare October 14, 2020 01:58

zrhoffman requested changes Oct 14, 2020

View reviewed changes

mitchell852 changed the title ~~strips out servers in the wrong cdn if requesting servers for a ds~~ strips out mids in the wrong cdn if requesting servers for a ds Oct 14, 2020

mitchell852 changed the title ~~strips out mids in the wrong cdn if requesting servers for a ds~~ strips out mids in the wrong cdn when requesting servers for a ds Oct 14, 2020

zrhoffman reviewed Oct 14, 2020

View reviewed changes

traffic_ops/traffic_ops_golang/server/servers.go Show resolved Hide resolved

zrhoffman reviewed Oct 14, 2020

View reviewed changes

zrhoffman reviewed Oct 15, 2020

View reviewed changes

ocket8888 added this to the 5.0.0 milestone Oct 15, 2020

zrhoffman requested changes Oct 15, 2020

View reviewed changes

traffic_ops/traffic_ops_golang/server/servers_test.go Show resolved Hide resolved

traffic_ops/traffic_ops_golang/server/servers.go Show resolved Hide resolved

mitchell852 added 8 commits October 15, 2020 14:20

strips out servers in the wrong cdn if requesting servers for a ds

4ee96bb

adds an api test to check for the presence of mid servers only in the…

35891ac

… same cdn as the ds

formatting

e2ccf93

adds changelog.md entry

9bfb7ab

pushes optional cdn filter into the query that fetches the mids

a32f802

updates changelog entry

aa8c109

removing extra code that slipped in

f4342be

comment change

6719975

mitchell852 added 10 commits October 15, 2020 14:20

formatting

02b160a

fixes unit test

f776562

better function argument name

6d8c048

Fatalf instead of Errorf

c6eb1cb

error checking

a7a3dbb

fixes broken api test

c26c156

uses a cg not in a topology

b5b8b5b

changing back to parentcachegroup as this was masking another potenti…

f84e64b

…al bug

fixes topology query param test

e3a223a

adds more expected hosts in the topology

92edce6

mitchell852 force-pushed the fixes-non-cdn-servers branch from 4198d55 to 92edce6 Compare October 15, 2020 20:28

zrhoffman approved these changes Oct 15, 2020

View reviewed changes

zrhoffman merged commit 00c12c4 into apache:master Oct 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

strips out mids in the wrong cdn when requesting servers for a ds #5128

strips out mids in the wrong cdn when requesting servers for a ds #5128

mitchell852 commented Oct 8, 2020 •

edited

Loading

zrhoffman Oct 14, 2020

mitchell852 Oct 14, 2020

zrhoffman Oct 14, 2020

zrhoffman Oct 14, 2020

mitchell852 Oct 14, 2020 •

edited

Loading

mitchell852 Oct 14, 2020

zrhoffman commented Oct 14, 2020

zrhoffman Oct 14, 2020

zrhoffman Oct 14, 2020

zrhoffman Oct 14, 2020

mitchell852 Oct 15, 2020

zrhoffman Oct 14, 2020

mitchell852 commented Oct 15, 2020 •

edited

Loading

zrhoffman commented Oct 15, 2020

mitchell852 commented Oct 15, 2020

zrhoffman commented Oct 15, 2020

zrhoffman Oct 15, 2020

mitchell852 Oct 15, 2020

zrhoffman Oct 15, 2020

mitchell852 Oct 15, 2020

mitchell852 Oct 15, 2020

mitchell852 commented Oct 15, 2020

zrhoffman commented Oct 15, 2020 •

edited

Loading

mitchell852 commented Oct 15, 2020

zrhoffman commented Oct 15, 2020

zrhoffman left a comment

		func GetDSCDNIdFromID(tx *sql.Tx, id int) (int, bool, error) {
		var cdnID int

strips out mids in the wrong cdn when requesting servers for a ds #5128

strips out mids in the wrong cdn when requesting servers for a ds #5128

Conversation

mitchell852 commented Oct 8, 2020 • edited Loading

Which Traffic Control components are affected by this PR?

What is the best way to verify this PR?

If this is a bug fix, what versions of Traffic Control are affected?

The following criteria are ALL met by this PR

Additional Information

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchell852 Oct 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zrhoffman commented Oct 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchell852 commented Oct 15, 2020 • edited Loading

zrhoffman commented Oct 15, 2020

mitchell852 commented Oct 15, 2020

zrhoffman commented Oct 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchell852 commented Oct 15, 2020

zrhoffman commented Oct 15, 2020 • edited Loading

mitchell852 commented Oct 15, 2020

zrhoffman commented Oct 15, 2020

zrhoffman left a comment

Choose a reason for hiding this comment

mitchell852 commented Oct 8, 2020 •

edited

Loading

mitchell852 Oct 14, 2020 •

edited

Loading

mitchell852 commented Oct 15, 2020 •

edited

Loading

zrhoffman commented Oct 15, 2020 •

edited

Loading