Remove cacheurl from Delivery Services by shamrickus · Pull Request #4644 · apache/trafficcontrol

shamrickus · 2020-04-17T19:48:18Z

What does this PR (Pull Request) do?

This PR begins Remove CacheURL Delivery Service Field #3225 and fixes cache url expression configs do not get added to mid profiles #1125.

This does not finish #3225 as CacheURL exists in ATC5 and TO API v3, so until those are no longer supported CacheURL cannot be completely removed.

Which Traffic Control components are affected by this PR?

CDN in a Box
Documentation
Traffic Control Client
Traffic Ops
Traffic Portal
Traffic Router

What is the best way to verify this PR?

Ensure db migration works
Verify CDN still works (specificially TO, TR, TP)
Verify modify/delete/create of DS in TP/TO

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)

lib/go-atscfg/remapdotconfig.go

lib/go-atscfg/cacheurldotconfig.go

lib/go-tc/deliveryservices.go

ocket8888

So to be clear, this PR removes support for the cacheurl, but does not remove it from the API? Like it'll always show it as null in responses because if you do a POST with some cacheurl value that doesn't get unmarshaled and doesn't get stored in the database, but from what I can see the structures marshaled for responses still have it.

I'm also not sure if imparting that behavior on API versions 1.x and 2.x constitutes a breaking change.

lib/go-atscfg/remapdotconfig_test.go

shamrickus · 2020-06-15T15:12:16Z

So to be clear, this PR removes support for the cacheurl, but does not remove it from the API? Like it'll always show it as null in responses because if you do a POST with some cacheurl value that doesn't get unmarshaled and doesn't get stored in the database, but from what I can see the structures marshaled for responses still have it.

Only when querying old API versions on an update server. V3 should not have these fields while v1/v2 should.

ocket8888

Compile error at traffic_ops/traffic_ops_golang/routing/routes.go:1232:119:

undefined: atscdn.GetCacheURLDotConfig

traffic_ops/traffic_ops_golang/ats/atscdn/cacheurldotconfig.go

ocket8888 · 2020-06-23T18:09:17Z

So like I was saying before, this removes support for cacheurl, but does not actually remove it from the API? I can still see it in v3:

GET /api/3.0/deliveryservices?limit=1 HTTP/1.1
User-Agent: python-requests/2.22.0
Accept-Encoding: gzip, deflate
Accept: */*
Connection: keep-alive
Cookie: mojolicious=eyJhdXRoX2RhdGEiOiJhZG1pbiIsImV4cGlyZXMiOjE1OTI5NTcyNjgsImJ5IjoidHJhZmZpY2NvbnRyb2wtZ28tdG9jb29raWUifQ--625c11141971227d3862de47c92810086ebd7389

HTTP/1.1 200 OK
Access-Control-Allow-Credentials: true
Access-Control-Allow-Headers: Origin, X-Requested-With, Content-Type, Accept, Set-Cookie, Cookie
Access-Control-Allow-Methods: POST,GET,OPTIONS,PUT,DELETE
Access-Control-Allow-Origin: *
Content-Encoding: gzip
Content-Type: application/json
Set-Cookie: mojolicious=eyJhdXRoX2RhdGEiOiJhZG1pbiIsImV4cGlyZXMiOjE1OTI5MzkyNjgsImJ5IjoidHJhZmZpY2NvbnRyb2wtZ28tdG9jb29raWUifQ--784bea340410815bda72048f7fa1c6cc6520e590; Path=/; Expires=Tue, 23 Jun 2020 19:07:48 GMT; Max-Age=3600; HttpOnly
Vary: Accept-Encoding
Whole-Content-Sha512: 94qFFF5yps/vfOFe/E3M285fBWJzgmpylXwwdGf4xa8w/J32nPboY2jgd2Ehi52jt1pK0zYtOc+Vtud3f/rVSA==
X-Server-Name: traffic_ops_golang/
Date: Tue, 23 Jun 2020 18:07:48 GMT
Content-Length: 806

{
	"response": [
		{
			"active": true,
			"anonymousBlockingEnabled": false,
			"cacheurl": null,
			"ccrDnsTtl": null,
			"cdnId": 2,
			"cdnName": "CDN-in-a-Box",
			"checkPath": null,
			"displayName": "Demo 1",
			"dnsBypassCname": null,
			"dnsBypassIp": null,
			"dnsBypassIp6": null,
			"dnsBypassTtl": null,
			"dscp": 0,
			"edgeHeaderRewrite": null,
			"geoLimit": 0,
			"geoLimitCountries": null,
			"geoLimitRedirectURL": null,
			"geoProvider": 0,
			"globalMaxMbps": null,
			"globalMaxTps": null,
			"httpBypassFqdn": null,
			"id": 1,
			"infoUrl": null,
			"initialDispersion": 1,
			"ipv6RoutingEnabled": true,
			"lastUpdated": "2020-06-23 18:04:18+00",
			"logsEnabled": true,
			"longDesc": "Apachecon North America 2018",
			"longDesc1": null,
			"longDesc2": null,
			"matchList": [
				{
					"type": "HOST_REGEXP",
					"setNumber": 0,
					"pattern": ".*\\.demo1\\..*"
				}
			],
			"maxDnsAnswers": null,
			"midHeaderRewrite": null,
			"missLat": 42,
			"missLong": -88,
			"multiSiteOrigin": false,
			"originShield": null,
			"orgServerFqdn": "http://origin.infra.ciab.test",
			"profileDescription": null,
			"profileId": null,
			"profileName": null,
			"protocol": 2,
			"qstringIgnore": 0,
			"rangeRequestHandling": 0,
			"regexRemap": null,
			"regionalGeoBlocking": false,
			"remapText": null,
			"routingName": "video",
			"signed": false,
			"sslKeyVersion": null,
			"tenantId": 1,
			"type": "HTTP",
			"typeId": 1,
			"xmlId": "demo1",
			"exampleURLs": [
				"http://video.demo1.mycdn.ciab.test",
				"https://video.demo1.mycdn.ciab.test"
			],
			"deepCachingType": "NEVER",
			"fqPacingRate": null,
			"signingAlgorithm": null,
			"tenant": "root",
			"trResponseHeaders": null,
			"trRequestHeaders": null,
			"consistentHashRegex": null,
			"consistentHashQueryParams": [
				"abc",
				"pdq",
				"xxx",
				"zyx"
			],
			"maxOriginConnections": 0,
			"ecsEnabled": false,
			"rangeSliceBlockSize": null,
			"topology": null
		}
	]
}

shamrickus · 2020-06-23T20:19:55Z

With my original changes that didn't remove the unused methods this was not possible. Since those have now been removed I can remove it from the v3 api.

ocket8888

API/client integration tests (v1) broken:

# github.com/apache/trafficcontrol/traffic_ops/testing/api/v1 [github.com/apache/trafficcontrol/traffic_ops/testing/api/v1.test]
./deliveryservices_test.go:362:35: ds.DeliveryServiceNullableV11 undefined (type tc.DeliveryServiceNullable has no field or method DeliveryServiceNullableV11)
FAIL	github.com/apache/trafficcontrol/traffic_ops/testing/api/v1 [build failed]

v2 tests pass

v3 tests failing on TestDeliveryServices with:

    topologies_test.go:57: Response:  &{{a topology my-topology [{0 parentCachegroup [] <nil>} {0 secondaryCachegroup [] <nil>} {0 cachegroup1 [0 1] <nil>}] <nil>} {[{topology was created. success}]}}
    topologies_test.go:57: 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:57: 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:57: 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}]}}
    deliveryservices_test.go:248: expected: no error updating topology-based header rewrite fields for topology-based DS, actual: 404 Not Found[404] - Error requesting Traffic Ops https://localhost:6443/api/3.0/deliveryservices/215 {"alerts":[{"text":"topology not found","level":"error"}]}

shamrickus · 2020-07-08T15:47:05Z

Test failures should be fixed now

ocket8888 · 2020-07-08T16:22:44Z

Seeing this in the v1 tests:

 deliveryservices_test.go:121: cannot UPDATE DeliveryService by ID: 500 Internal Server Error[500] - Error requesting Traffic Ops https://localhost:6443/api/1.5/deliveryservices/65.json {"alerts":[{"text":"Internal Server Error","level":"error"}]}
         - <nil>
    deliveryservices_test.go:134: results do not match actual: d s 1, expected: something different
    deliveryservices_test.go:135: results do not match actual: 0, expected: 164598
    deliveryservices_test.go:136: results do not match actual: 0, expected: 100
    deliveryservices_test.go:170: cannot UPDATE DeliveryService by ID: 500 Internal Server Error[500] - Error requesting Traffic Ops https://localhost:6443/api/1.5/deliveryservices/65.json {"alerts":[{"text":"Internal Server Error","level":"error"}]}
         - <nil>

shamrickus · 2020-07-08T18:04:58Z

v1 issues should be fixed

mitchell852 · 2021-01-20T21:33:24Z

just following up on status of this. is this waiting for after 5.0 release?

shamrickus · 2021-01-20T21:50:26Z

At the least, probably best for whatever release perl gets removed in.

shamrickus · 2021-02-03T15:40:43Z

Should be ready for re-review. Not mentioned in the original PR but it is good to also test compatibility with old versions.

ocket8888

I think this should only edit the tc-fixtures.json in v4/ - also, why'd you delete the userdeliveryservices_test.go file in v1/?

Also, I think you already know this, but the docs have a bunch of references to the now-undefined ds-cacheurl label.

lib/go-tc/deliveryservices.go

ocket8888

Would you mind adding GoDoc comments to the new exported symbols?

traffic_ops/client

lib/go-tc/deliveryservices.go

rawlinp

I added a bunch of comments as I went along, but it might be easier to understand if you go through all the comments first before making changes. Basically it boils down to reducing duplication and making it easier to make future changes to the DS API.

lib/go-tc/deliveryservices.go

traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go

traffic_ops/traffic_ops_golang/server/servers_assignment.go

…ol into cacheurl" This reverts commit 2a92ba59e59bc01b0b8b16c63c2d59b500fcaea8, reversing changes made to 1f4ec08.

traffic_ops/client

ocket8888

LGTM

This changes DSRs to use APIv4 Delivery Services - across all API versions - which is technically a breaking change. But the only fixes are to either quadruple the DSR handling code or rewrite it to not use the CRUDer. Since the latter is already in progress, I'm inclined to let that be tomorrow's problem.

lib/go-tc/deliveryservices.go

rawlinp · 2021-02-18T22:34:02Z

traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go

+	dsNullable := tc.DeliveryServiceNullableV30(dsV31)
+	ds := dsNullable.UpgradeToV4()
+	res, status, userErr, sysErr := createV40(w, r, inf, tc.DeliveryServiceV40(ds))
+	if res == nil {


Should we be checking userErr != nil || sysErr != nil instead?

I'm just doing what the previous versions of this function did (but flipped). It looks like if there is an error in createV40, res will always be nil, otherwise it will have a value. I can still change it, but if I think it would make sense to also change all the create/updateV{old} functions too since they do the same thing.

traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go

alficles suggested changes Apr 17, 2020

View reviewed changes

lib/go-atscfg/remapdotconfig.go Outdated Show resolved Hide resolved

lib/go-atscfg/cacheurldotconfig.go Outdated Show resolved Hide resolved

lib/go-tc/deliveryservices.go Outdated Show resolved Hide resolved

ocket8888 reviewed May 5, 2020

View reviewed changes

lib/go-atscfg/remapdotconfig_test.go Outdated Show resolved Hide resolved

lib/go-atscfg/remapdotconfig_test.go Outdated Show resolved Hide resolved

ocket8888 suggested changes Jun 18, 2020

View reviewed changes

ocket8888 reviewed Jun 19, 2020

View reviewed changes

traffic_ops/traffic_ops_golang/ats/atscdn/cacheurldotconfig.go Outdated Show resolved Hide resolved

ocket8888 suggested changes Jun 23, 2020

View reviewed changes

mitchell852 mentioned this pull request Dec 8, 2020

cache url expression configs do not get added to mid profiles #1125

Closed

shamrickus mentioned this pull request Jan 25, 2021

Remove CacheURL Delivery Service Field #3225

Open

ocket8888 reviewed Feb 8, 2021

View reviewed changes

lib/go-tc/deliveryservices.go Outdated Show resolved Hide resolved

ocket8888 self-assigned this Feb 8, 2021

ocket8888 reviewed Feb 9, 2021

View reviewed changes

traffic_ops/client Outdated Show resolved Hide resolved

lib/go-tc/deliveryservices.go Outdated Show resolved Hide resolved

rawlinp suggested changes Feb 10, 2021

View reviewed changes

shamrickus added 4 commits February 11, 2021 11:19

Remove from types and fix compile errors

08f113b

Remove remaining traces

f5afee0

Missed some files

e471996

Remove from TP

2785376

shamrickus added 19 commits February 11, 2021 12:48

Handle old versions without issue.

52ab594

Remove from docs

b4bbf8c

Readd userdeliveryservice test

f123cba

Unremove Cacheurl, add DS V40

e32ba70

Readd docs

f165419

Add V40 DS

7f66773

Dont need this link

062d79f

Readd symlink for now

eb43a06

Upgrade atscfg server version

b97e903

v4 tests use v4

df8780a

Update headers and dont use new v4 route

d9eb666

Revert "Merge branch 'cacheurl' of github.com:shamrickus/trafficcontr…

4d067b5

…ol into cacheurl" This reverts commit 2a92ba59e59bc01b0b8b16c63c2d59b500fcaea8, reversing changes made to 1f4ec08.

Fix merge issues, use v4 methods in v4 tests, rename some v4 methods

5bf8fd8

More merge fixes

4e334d1

Missed some conversions.

3020088

Fix tests

9cdeb8b

Move ds functions

3191885

Add comments & clean up

4a292e0

Handle old versions of cachegroup post

27e27b5

ocket8888 reviewed Feb 17, 2021

View reviewed changes

traffic_ops/client Outdated Show resolved Hide resolved

Remove symlink

36a00b9

ocket8888 approved these changes Feb 17, 2021

View reviewed changes

ocket8888 requested a review from rawlinp February 17, 2021 21:17

shamrickus and others added 2 commits February 18, 2021 09:27

Merge branch 'master' into cacheurl

95b5e79

Fix Merge

abd5ad6

rawlinp suggested changes Feb 18, 2021

View reviewed changes

shamrickus added 2 commits February 19, 2021 07:45

Rename ds

2db9b54

Correctly cast ds based on version

206189d

rawlinp approved these changes Feb 19, 2021

View reviewed changes

rawlinp merged commit 56ef357 into apache:master Feb 19, 2021

Comments

Conversation

shamrickus commented Apr 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR (Pull Request) do?

Which Traffic Control components are affected by this PR?

What is the best way to verify this PR?

The following criteria are ALL met by this PR

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ocket8888 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

shamrickus commented Jun 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ocket8888 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ocket8888 commented Jun 23, 2020

Uh oh!

shamrickus commented Jun 23, 2020

Uh oh!

ocket8888 left a comment

Choose a reason for hiding this comment

Uh oh!

shamrickus commented Jul 8, 2020

Uh oh!

ocket8888 commented Jul 8, 2020

Uh oh!

shamrickus commented Jul 8, 2020

Uh oh!

mitchell852 commented Jan 20, 2021

Uh oh!

shamrickus commented Jan 20, 2021

Uh oh!

shamrickus commented Feb 3, 2021

Uh oh!

ocket8888 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ocket8888 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rawlinp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ocket8888 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rawlinp Feb 18, 2021

Choose a reason for hiding this comment

Uh oh!

shamrickus Feb 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shamrickus commented Apr 17, 2020 •

edited

Loading

shamrickus commented Jun 15, 2020 •

edited

Loading

shamrickus Feb 19, 2021 •

edited

Loading