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

Remove api v1 #5539

Merged
merged 21 commits into from
Jul 14, 2021
Merged

Remove api v1 #5539

merged 21 commits into from
Jul 14, 2021

Conversation

ocket8888
Copy link
Contributor

What does this PR (Pull Request) do?

  • This PR is not related to any Issue

This PR removes API version 1 entirely, including documentation and tests.

Which Traffic Control components are affected by this PR?

  • Documentation
  • Traffic Ops Client (Go)
  • Traffic Ops
  • CI tests

What is the best way to verify this PR?

verify existing API tests pass for all versions greater than 1, make a request to APIv1 and verify it doesn't succeed.

The following criteria are ALL met by this PR

  • Tests are unnecessary
  • This PR includes documentation
  • This PR includes an update to CHANGELOG.md
  • This PR includes any and all required license headers
  • This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY

@ocket8888 ocket8888 added Traffic Ops related to Traffic Ops documentation related to documentation tech debt rework due to choosing easy/limited solution TO Client (Go) related to the Go implementation of a TC client labels Feb 18, 2021
@ocket8888
Copy link
Contributor Author

This can't be done until #5541 is closed

@ocket8888 ocket8888 marked this pull request as draft February 19, 2021 02:36
@ocket8888 ocket8888 marked this pull request as ready for review March 25, 2021 23:05
@ocket8888 ocket8888 force-pushed the remove-api-v1 branch 2 times, most recently from 59e5fc4 to 83966b3 Compare March 30, 2021 20:19
@zrhoffman zrhoffman added this to the TO API v1 removal milestone Mar 31, 2021
@zrhoffman
Copy link
Member

zrhoffman commented Mar 31, 2021

The tests all pass now, but we should not merge it until #5377 is done.

@ocket8888
Copy link
Contributor Author

The tests all pass now, but we should not merge it until #5377 is done.

I made this not a draft because someone had told me that was done... I'll investigate to see if #5377 can be closed

Copy link
Member

@zrhoffman zrhoffman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When grepping the repo for api/1, there are still matches in several files:

  • cache-config/t3c-request/getdata/getdata.go
  • cache-config/t3cutil/getdata.go
  • docs/source/development/debugging.rst
  • experimental/ort-python/traffic_ops_ort/to_api.py
  • grove/grovetccfg/grovetccfg.go
  • infrastructure/docker/README.md
  • infrastructure/docker/traffic_monitor/run.sh
  • infrastructure/docker/traffic_ops/run.sh
  • infrastructure/docker/traffic_router/run.sh
  • infrastructure/docker/traffic_server_edge/run.sh
  • infrastructure/docker/traffic_server_mid/run.sh
  • infrastructure/docker/traffic_stats/run.sh
  • infrastructure/docker/traffic_vault/run.sh
  • lib/go-tc/federation.go
  • test/router/js/load-test.jsx
  • test/router/server/server.go
  • test/traffic_ops_cfg/cfg_test.pl
  • traffic_control/clients/python/trafficops/restapi.py
  • traffic_control/clients/python/trafficops/tosession.py
  • traffic_ops/install/bin/copy-pgdata.sh
  • traffic_ops/testing/api/v2/cdnfederations_test.go
  • traffic_ops/testing/api/v3/cdnfederations_test.go
  • traffic_ops/testing/api/v4/cdnfederations_test.go
  • traffic_ops/traffic_ops_golang/deliveryservice/consistenthash/consistenthash.go
  • traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go
  • traffic_ops/traffic_ops_golang/invalidationjobs/invalidationjobs.go
  • traffic_ops/traffic_ops_golang/login/logout_test.go
  • traffic_ops/traffic_ops_golang/routing/routing_test.go
  • traffic_ops/traffic_ops_golang/swaggerdocs/v13/docs.go
  • traffic_ops/traffic_ops_golang/trafficstats/util_test.go
  • traffic_portal/app/src/scripts/config.js

@ocket8888
Copy link
Contributor Author

Usages in cache-config/ are in GoDoc comments - it looks safe to change them since the information they're conveying isn't actually specific to that API version. I can do that if you want, but it shouldn't affect anything.

Usage in docs example has been updated

Usage in the ORT.py package is irrelevant, since that is due for removal. Pointless to change now, since it can't possibly work now that atstccfg has been removed, and whatever I do will be deleted anyway.

Usage in Grove is in a comment that wouldn't even appear in package documentation. I can get rid of it if you want, but it's not affecting anything.

I don't tend to touch anything in infrastructure/docker (except in the build subdirectory) because those Dockerfiles are archaic, they're parsing that API output in a way that may not be trivial (e.g. getting/setting server IP addresses), and their use is nowhere recommended or even documented. I don't even know if those work. I can change it, but it might break things in weird and unexpected ways, which doesn't seem worth it for something we don't even really support.

Usage in lib/go-tc is in GoDoc comments, which I can change if you want.

Usage in load-test.jsx is not referring to a Traffic Ops host - you can tell because it's passing a hostname for a Traffic Ops instance as a query parameter. test/router/server.go is the server serving that API, so the only real change necessary should be in there. But on the other hand - that test suite doesn't work. All browsers I've tried (Firefox, Chrome, Opera, all at latest versions) will refuse to load the .jsx file on the grounds of CORS request failure. If those tests are intended to work, they'll need to be reworked anyway, so the changes could just take place then.

cfg_test.pl is a script that, I believe, has not worked for longer than your involvement in the project. Possibly longer than mine. It was replaced by the compare tool, which has since been removed because it no longer serves any purpose. This script should not exist.

Usage in restapi.py is for a generic API, an API base URL containing an api/1 segment is perfectly valid in that context. In tosession.py it's in a comment on an example - which itself works afaik - and doesn't appear in any documentation. I can change it if you want, but it's not affecting anything.

Usage in copy-pgdata.sh has been updated. I thought that was for migrating from a mysql database to postgresql, but the comment header implies otherwise. Regardless, the usages are trivial enough to change easily.

Usage in traffic_ops/testing/ is in non-documenting comments, which I can change if you want but it's not affecting anything.

Usage in traffic_ops/traffic_ops_golang/ in GoDoc comments has been updated, as it was incorrect - except for one instance, which is still technically correct. One usage as a hard-coded value in a response header and one usage in a test have both been updated. The usages in the routing test are specifically checking for APIv1 not existing, and so should remain. The URL used in the DS Stats test is arbitrary, I can change it, but it won't be any more or less valid; only the headers on the request actually matter.

Swagger is not actually supported by Traffic Ops. The traffic_ops/traffic_ops_golang/swaggerdocs/ directory and any and all sub-packages thereof should be deleted, in my opinion. Unless someone is going to make them work, in which case they ought to update them to be correct themselves. But I don't think that's actually possible, in general, without significant code changes and some way to integrate the output with our existing documentation system (which doesn't exist afaik).

Usage in TP was previously removed, but must have been re-introduced during rebase with the commit that added/updated the stable API version.

@zrhoffman
Copy link
Member

zrhoffman commented Jul 8, 2021

Usages in cache-config/ are in GoDoc comments

Usage in lib/go-tc is in GoDoc comments

In tosession.py it's in a comment on an example - which itself works afaik - and doesn't appear in any documentation.

Usage in traffic_ops/testing/ is in non-documenting comments

The URL used in the DS Stats test is arbitrary

Updating all of these makes sense to me, IMO.

cfg_test.pl is a script that, I believe, has not worked for longer than your involvement in the project. Possibly longer than mine. It was replaced by the compare tool, which has since been removed because it no longer serves any purpose. This script should not exist.

Sounds like it could have been removed in the Perlectomy. Removing it with the rest of API v1 is the next best thing, right?


Usage in Grove is in a comment that wouldn't even appear in package documentation. I can get rid of it if you want, but it's not affecting anything.

// api := flag.String("api", "1.2", "API version. Determines whether to use /api/1.3/configs/ or older, less efficient 1.2 APIs")

I'm seeing more API v1 stuff lower down:

// TODO remove this once converted to 1.3 API
// using the 1.2 API
var hostServer tc.Server
var hostProfile tc.Profile
var ok bool
var profiles map[string]tc.Profile
var servers map[string]tc.Server
serversArr, _, err := toc.GetServers()
if err != nil {
fmt.Println(time.Now().Format(time.RFC3339Nano) + " Error getting Traffic Ops Servers: " + err.Error())
os.Exit(ExitError)
}
servers = makeServersHostnameMap(serversArr)
hostServer, ok = servers[*host]
if !ok {
fmt.Println(time.Now().Format(time.RFC3339Nano) + " Error: host '" + *host + "' not in Servers\n")
os.Exit(ExitError)
}
profilesArr, _, err := toc.GetProfiles()
if err != nil {
fmt.Println(time.Now().Format(time.RFC3339Nano) + " Error getting Traffic Ops Profiles: " + err.Error())
os.Exit(ExitError)
}
profiles = makeProfileNameMap(profilesArr)
hostProfile, ok = profiles[hostServer.Profile]
if !ok {
fmt.Println(time.Now().Format(time.RFC3339Nano) + " Error: profile '" + hostServer.Profile + "' not in Profiles\n")
os.Exit(ExitError)
}
// end of API 1.2 stuff

Removing the Grove API v1 stuff would make more sense in a separate PR.

Usage in the ORT.py package is irrelevant, since that is due for removal.

I don't tend to touch anything in infrastructure/docker (except in the build subdirectory) because those Dockerfiles are archaic, they're parsing that API output in a way that may not be trivial (e.g. getting/setting server IP addresses), and their use is nowhere recommended or even documented.

Usage in load-test.jsx is not referring to a Traffic Ops host - you can tell because it's passing a hostname for a Traffic Ops instance as a query parameter. test/router/server.go is the server serving that API, so the only real change necessary should be in there. But on the other hand - that test suite doesn't work. All browsers I've tried (Firefox, Chrome, Opera, all at latest versions) will refuse to load the .jsx file on the grounds of CORS request failure. If those tests are intended to work, they'll need to be reworked anyway, so the changes could just take place then.

Swagger is not actually supported by Traffic Ops. The traffic_ops/traffic_ops_golang/swaggerdocs/ directory and any and all sub-packages thereof should be deleted, in my opinion. Unless someone is going to make them work, in which case they ought to update them to be correct themselves.

Agreed that taking care of these outside of #5539 makes sense.

@zrhoffman
Copy link
Member

Oops, missed a response.

Usage in restapi.py is for a generic API, an API base URL containing an api/1 segment is perfectly valid in that context. In tosession.py it's in a comment on an example - which itself works afaik - and doesn't appear in any documentation. I can change it if you want, but it's not affecting anything.

https://github.com/apache/trafficcontrol/blob/b64fcd5bf675c73ea1d085ee40245fcbd92e0df5/traffic_control/clients/python/trafficops/restapi.py#L141
https://github.com/apache/trafficcontrol/blob/b64fcd5bf675c73ea1d085ee40245fcbd92e0df5/traffic_control/clients/python/trafficops/restapi.py#L147

Updating these makes sense to me, too.

@ocket8888 ocket8888 requested a review from zrhoffman July 14, 2021 17:54
Copy link
Member

@zrhoffman zrhoffman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm only seeing 1 unaddressed comment, the rest looks good.

Copy link
Member

@zrhoffman zrhoffman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Things to address outside of this PR:

  • Remaining API v1 references:

    • Grove API v1 references
    • Swagger API v1 references
    • test/router API v1 references
    • infrastructure/docker API v1 references

    API 1.x endpoint calls need to be updated to 2.0  #4461 captures most of these already.

  • Deprecating API v2 and API v3. Ideally, API v2 and API v3 responses would show deprecation alerts, but at a minimum, we need to deprecate the endpoints in the documentation.

@zrhoffman zrhoffman merged commit 20ba2bc into apache:master Jul 14, 2021
@ocket8888 ocket8888 deleted the remove-api-v1 branch July 14, 2021 22:33
@zrhoffman zrhoffman mentioned this pull request Jul 15, 2021
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation related to documentation tech debt rework due to choosing easy/limited solution TO Client (Go) related to the Go implementation of a TC client Traffic Ops related to Traffic Ops
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants