Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

/deliveryservices/{{ID}}/safe returns incorrect response for the requested API version #5891

Closed
ocket8888 opened this issue May 26, 2021 · 1 comment · Fixed by #5922
Closed
Assignees
Labels
regression bug a bug in existing functionality introduced by a new version Traffic Ops related to Traffic Ops

Comments

@ocket8888
Copy link
Contributor

ocket8888 commented May 26, 2021

I'm submitting a ...

  • bug report

Traffic Control components affected ...

  • Traffic Ops

Current behavior:

Making a PUT request to /deliveryservices/{{ID}}/safe will return a different structure from a request made to /deliveryservices for the same API version. As these are not the same endpoint, there's technically no reason they need to return the same structural representation for the same Delivery Service at the same API version, but because released API versions were retroactively changed to return extra fields, this is a regression bug. In fixing it, I suggest we define the response structure to be isomorphic to /deliveryservices.

The various API versions requested for /deliveryservices/{{ID}}/safe compared to the API version of /deliveryservices that would yield the same structure as /deliveryservices/{{ID}}/safe are given below:

/deliveryservices/{{ID}}/safe version /deliveryservices version
4.0 3.1
3.1 3.1
3.0 3.1
2.0 3.1
1.5 3.1
1.4 1.4
1.3 1.3
1.2 1.2
1.1 1.1

Expected behavior:

The structure of a response from /deliveryservices/{{ID}}/safe should be the same as that of a response from /deliveryservices for the same API version. In other words, instead of the behavior described by the above table, what I'd expect to see is something more like:

/deliveryservices/{{ID}}/safe version /deliveryservices version
4.0 4.0
3.1 3.1
3.0 3.0
2.0 2.0
1.5 1.5
1.4 1.4
1.3 1.3
1.2 1.2
1.1 1.1

Minimal reproduction of the problem with instructions:

Make requests to /deliveryservices/{{ID}}/safe, note the presence or absence of fields introduced or removed in various API versions of /deliveryservices and their presence or absence from /deliveryservices/{{ID}}/safe.

@ocket8888 ocket8888 added bug something isn't working as intended Traffic Ops related to Traffic Ops low impact affects only a small portion of a CDN, and cannot itself break one labels May 26, 2021
@ocket8888 ocket8888 self-assigned this May 26, 2021
@ocket8888 ocket8888 added regression bug a bug in existing functionality introduced by a new version and removed bug something isn't working as intended low impact affects only a small portion of a CDN, and cannot itself break one labels May 27, 2021
@ocket8888
Copy link
Contributor Author

Actually, this has to be a regression. The released 1.5, 2.0, and 3.0 APIs could not possibly have been returning data that didn't exist in the database until 3.1.

ocket8888 added a commit to ocket8888/trafficcontrol that referenced this issue Jul 8, 2021
Previously, it would return APIv3.1 structures for API version 1.5, 2.0,
3.0, 3.1, and 4.0 (as well as unrecognized versions). It now returns the
appropriate version structures for each requested version.

This fixes apache#5891
rawlinp pushed a commit that referenced this issue Jul 8, 2021
* Add migration for DS TLS versions

* Update DS model in APIv4

* Add TLSVersions handling and validation for invalid versions to /deliveryservices

* Fix incorrect responses from /deliveryservices/{ID}/safe

Previously, it would return APIv3.1 structures for API version 1.5, 2.0,
3.0, 3.1, and 4.0 (as well as unrecognized versions). It now returns the
appropriate version structures for each requested version.

This fixes #5891

* Fix DSRs storing/returning APIv4 DSes with empty tlsVersions instead of null

* Fix being unable to update DSes with no TLS versions

* Add ATC "known" TLS versions and a warning generation function for possibly insecure version sets

* Add tlsVersions warnings to /deliveryservices

* Add validation that prevents tlsVersions on STEERING/CLIENT_STEERING delivery services

* Fix a bug where adding TLS versions returns a 500 ISE response

* Fix indirection of non-pointer value

* Add TLS versions tests to the v4 API client

* Remove 'lastUpdated' timestamps from testing data

This breaks parsing when the API endpoints that use this data change
from our custom format to RFC3339.

* Update /deliveryservices documentation

* Update /deliveryservices/{{ID}} documentation

* Update /deliveryservices/{{ID}}/safe documentation

* Update /deliveryservice_requests documentation

* Update /deliveryservice_requests/{{ID}}/status documentation

* Update /deliveryservice_requests/{{ID}}/assign documentation

* Update /servers/{{ID}}/deliveryservices documentation

* Add section about TLS versions to the Delivery Service overview docs

* Updated CHANGELOG

* Revert non-nullable DS fields

* Change version checking to proper upgrade to preserve existing pattern

* Rename function to make its purpose clear

* Move comment into GoDoc where it's more useful

* Use Exec for queries that don't return rows

* Consolidate TLS Versions warnings into a single function

That function is now a method of a Delivery Service, which means the
call signatures of various functions in the
github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/deliveryservice
package no longer need to be changed.

* re-use query for TLS Versions

* Fix tests for new query structure

* Revert all breaking DS changes

* Fix not saving DS changes

* Fix unit test failure

* Fix integration test missing required DS fields
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression bug a bug in existing functionality introduced by a new version Traffic Ops related to Traffic Ops
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant