You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2021/01/12 20:02:50 UTC
[GitHub] [trafficcontrol] rob05c opened a new pull request #5430: Add Traffic Ops Client Minor Fallback
rob05c opened a new pull request #5430:
URL: https://github.com/apache/trafficcontrol/pull/5430
Adds the Traffic Ops Client falling back to prior minor versions, if the Traffic Ops Server doesn't support the latest.
This is necessary for ORT/atstccfg to work if ORT is one version newer than the TO Server.
The feature is added via a new func, `v3-client.Login` and a new options flag. So the existing functions all behave as-before, and it's possible to disable the feature with the new Login func by passing an option.
It also adds some middleware abstraction to the client, which makes the client more flexible and easier to compose different "middleware" like features in the future. I added that because it was easier than not doing it.
Labelling "feature" even though this is necessary to fix a bug in ORT.
This is tested by the existing TO API Tests.
Includes GoDoc documentation for the new public symbols in `v3-client`.
Includes Changelog.
- [x] This PR is not related to any other Issue
## Which Traffic Control components are affected by this PR?
- Traffic Control Client <!-- Please specify which; e.g. 'Python', 'Go', 'Java' -->
## What is the best way to verify this PR?
Run apps that use the client, verify they're unchanged. Write a test app calling `v3-client/Login`, verify it works as expected. Verify the latest 3.1 client falls back to 3.0 with a 3.0 Traffic Ops.
## If this is a bug fix, what versions of Traffic Control are affected?
Not a bug fix.
## The following criteria are ALL met by this PR
- [x] This PR includes tests OR I have explained why tests are unnecessary
- [x] This PR includes documentation OR I have explained why documentation is unnecessary
- [x] This PR includes an update to CHANGELOG.md OR such an update is not necessary
- [x] This PR includes any and all required license headers
- [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://www.apache.org/security/) for details)
## Additional Information
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rob05c commented on pull request #5430: Add Traffic Ops Client Minor Fallback
Posted by GitBox <gi...@apache.org>.
rob05c commented on pull request #5430:
URL: https://github.com/apache/trafficcontrol/pull/5430#issuecomment-771959765
Fixed merge conflicts.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp commented on pull request #5430: Add Traffic Ops Client Minor Fallback
Posted by GitBox <gi...@apache.org>.
rawlinp commented on pull request #5430:
URL: https://github.com/apache/trafficcontrol/pull/5430#issuecomment-772773260
Oh, also, we now have API v4 (along w/ a v4 client), so we should duplicate these changes there as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rob05c commented on a change in pull request #5430: Add Traffic Ops Client Minor Fallback
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #5430:
URL: https://github.com/apache/trafficcontrol/pull/5430#discussion_r568905867
##########
File path: traffic_ops/v3-client/topology_queue_updates.go
##########
@@ -26,7 +26,8 @@ import (
)
func (to *Session) TopologiesQueueUpdate(topologyName tc.TopologyName, req tc.TopologiesQueueUpdateRequest) (tc.TopologiesQueueUpdateResponse, ReqInf, error) {
- path := fmt.Sprintf(ApiTopologies+"/%s/queue_update", topologyName)
+ path := fmt.Sprintf("%s/%s/queue_update", APITopologies, topologyName)
+ var reqInf ReqInf
Review comment:
Removed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp merged pull request #5430: Add Traffic Ops Client Minor Fallback
Posted by GitBox <gi...@apache.org>.
rawlinp merged pull request #5430:
URL: https://github.com/apache/trafficcontrol/pull/5430
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rob05c commented on a change in pull request #5430: Add Traffic Ops Client Minor Fallback
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #5430:
URL: https://github.com/apache/trafficcontrol/pull/5430#discussion_r568903517
##########
File path: traffic_ops/v3-client/topology.go
##########
@@ -23,12 +23,15 @@ import (
"github.com/apache/trafficcontrol/lib/go-tc"
)
+// ApiTopologies is Deprecated: will be removed in the next major version. Be aware this may not be the URI being requested, for clients created with Login and ClientOps.ForceLatestAPI false.
const ApiTopologies = apiBase + "/topologies"
+const APITopologies = "/topologies"
Review comment:
I had the same thought. I could imagine it being convenient and letting someone avoid duplicating, if they needed to get a route but not deserialize it with the standard mechanisms for some reason.
But since we're changing symbols anyway, it's not a deprecation issue, and we can always add them back later. I don't object to making them all private for now, if that's the consensus.
On that subject, I think there's a chance ORT may want or need to get objects without serialization at some point. As we move toward breaking up ORT and making it UNIX-style, we'll likely end up passing objects between apps. Parsing and serializing JSON is incredibly expensive, and it may end up making ORT take seconds or even minutes longer to run, if we don't have a way to request objects from TO without parsing them. Something to keep in mind.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rob05c commented on a change in pull request #5430: Add Traffic Ops Client Minor Fallback
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #5430:
URL: https://github.com/apache/trafficcontrol/pull/5430#discussion_r568949359
##########
File path: traffic_ops/v3-client/crconfig.go
##########
@@ -34,7 +37,7 @@ type OuterResponse struct {
// GetCRConfig returns the raw JSON bytes of the CRConfig from Traffic Ops, and whether the bytes were from the client's internal cache.
func (to *Session) GetCRConfig(cdn string) ([]byte, ReqInf, error) {
- uri := apiBase + `/cdns/` + cdn + `/snapshot`
+ uri := `/cdns/` + cdn + `/snapshot`
bts, reqInf, err := to.getBytesWithTTL(uri, tmPollingInterval)
Review comment:
Ok, I removed `getBytesWithTTL` and `getBytes`, and changed GetCRConfig()` and `GetCRConfigNew` to use the standard `to.req`, and changed `makeRequestWithHeader` to put the raw bytes in the `response` if it's a `*[]byte`.
That should also make it easier to add support for getting unparsed bytes in the future.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rob05c commented on a change in pull request #5430: Add Traffic Ops Client Minor Fallback
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #5430:
URL: https://github.com/apache/trafficcontrol/pull/5430#discussion_r568914090
##########
File path: traffic_ops/v3-client/endpoints.go
##########
@@ -15,4 +15,47 @@
package client
+// DEPRECATED: All new code should us Session.APIBase().
+// This isn't public, but only exists for deprecated public constants. It should be removed when they are.
const apiBase = "/api/3.1"
+
+const apiBaseStr = "/api/"
+
+// apiVersions is the list of minor API versions in this client's major version.
+// This should be all minor versions from 0 up to the latest minor in Traffic Control
+// as of this client code.
+//
+// Versions are ordered latest-first.
+func apiVersions() []string {
+ return []string{
+ "3.1",
+ "3.0",
+ }
+}
+
+// APIBase returns the base API string for HTTP requests, such as /api/3.1.
+// If UseLatestSupportedAPI
+func (sn *Session) APIBase() string {
+ if sn.latestSupportedAPI == "" {
Review comment:
Changed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rob05c commented on a change in pull request #5430: Add Traffic Ops Client Minor Fallback
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #5430:
URL: https://github.com/apache/trafficcontrol/pull/5430#discussion_r568898925
##########
File path: traffic_ops/v3-client/crconfig.go
##########
@@ -34,7 +37,7 @@ type OuterResponse struct {
// GetCRConfig returns the raw JSON bytes of the CRConfig from Traffic Ops, and whether the bytes were from the client's internal cache.
func (to *Session) GetCRConfig(cdn string) ([]byte, ReqInf, error) {
- uri := apiBase + `/cdns/` + cdn + `/snapshot`
+ uri := `/cdns/` + cdn + `/snapshot`
bts, reqInf, err := to.getBytesWithTTL(uri, tmPollingInterval)
Review comment:
I didn't change that. But I can look into changing it to `makeRequestWithHeader`
##########
File path: traffic_ops/v3-client/deliveryservice_regexes.go
##########
@@ -23,8 +23,11 @@ import (
)
const (
+ // API_DS_REGEXES is Deprecated: will be removed in the next major version. Be aware this may not be the URI being requested, for clients created with Login and ClientOps.ForceLatestAPI false.
// See: https://traffic-control-cdn.readthedocs.io/en/latest/api/v3/deliveryservices_id_regexes.html
API_DS_REGEXES = apiBase + "/deliveryservices/%d/regexes"
+
+ APIDSRegexes = "/deliveryservices/%v/regexes"
Review comment:
I don't have a strong opinion. Personally, it's a run-time error either way, so I'm not inclined to think it buys us anything. But I don't object to changing it
##########
File path: traffic_ops/v3-client/topology.go
##########
@@ -23,12 +23,15 @@ import (
"github.com/apache/trafficcontrol/lib/go-tc"
)
+// ApiTopologies is Deprecated: will be removed in the next major version. Be aware this may not be the URI being requested, for clients created with Login and ClientOps.ForceLatestAPI false.
const ApiTopologies = apiBase + "/topologies"
+const APITopologies = "/topologies"
Review comment:
I had the same thought. I could imagine it being convenient and letting someone avoid duplicating, if they needed to get a route but not deserialize it with the standard mechanisms for some reason.
But since we're changing symbols anyway, it's not a deprecation issue, and we can always add them back later. I don't object to making them all private for now, if that's the consensus.
On that subject, I think there's a chance ORT may want or need to get objects without serialization at some point. As we move toward breaking up ORT and making it UNIX-style, we'll likely end up passing objects between apps. Parsing and serializing JSON is incredibly expensive, and it may end up making ORT take seconds or even minutes longer to run, if we don't have a way to request objects from TO without parsing them. Something to keep in mind.
##########
File path: traffic_ops/v3-client/deliveryservice_regexes.go
##########
@@ -23,8 +23,11 @@ import (
)
const (
+ // API_DS_REGEXES is Deprecated: will be removed in the next major version. Be aware this may not be the URI being requested, for clients created with Login and ClientOps.ForceLatestAPI false.
// See: https://traffic-control-cdn.readthedocs.io/en/latest/api/v3/deliveryservices_id_regexes.html
API_DS_REGEXES = apiBase + "/deliveryservices/%d/regexes"
+
+ APIDSRegexes = "/deliveryservices/%v/regexes"
Review comment:
Changed.
##########
File path: traffic_ops/v3-client/topology_queue_updates.go
##########
@@ -26,7 +26,8 @@ import (
)
func (to *Session) TopologiesQueueUpdate(topologyName tc.TopologyName, req tc.TopologiesQueueUpdateRequest) (tc.TopologiesQueueUpdateResponse, ReqInf, error) {
- path := fmt.Sprintf(ApiTopologies+"/%s/queue_update", topologyName)
+ path := fmt.Sprintf("%s/%s/queue_update", APITopologies, topologyName)
+ var reqInf ReqInf
Review comment:
Removed
##########
File path: traffic_ops/v3-client/session.go
##########
@@ -64,6 +170,7 @@ func NewSession(user, password, url, userAgent string, client *http.Client, useC
}
const DefaultTimeout = time.Second * time.Duration(30)
+const DefaultAPIVersionCheckInterval = time.Second * time.Duration(60)
Review comment:
Changed
##########
File path: traffic_ops/v3-client/session.go
##########
@@ -119,22 +226,16 @@ func loginToken(token string) ([]byte, error) {
// login tries to log in to Traffic Ops, and set the auth cookie in the Session. Returns the IP address of the remote Traffic Ops.
func (to *Session) login() (net.Addr, error) {
- credentials, err := loginCreds(to.UserName, to.Password)
- if err != nil {
- return nil, errors.New("creating login credentials: " + err.Error())
- }
+ path := "/user/login"
+ body := tc.UserCredentials{Username: to.UserName, Password: to.Password}
+ alerts := tc.Alerts{}
- path := apiBase + "/user/login"
- resp, remoteAddr, err := to.RawRequestWithHdr("POST", path, credentials, nil)
- resp, remoteAddr, err = to.errUnlessOKOrNotModified(resp, remoteAddr, err, path)
- if err != nil {
- return remoteAddr, errors.New("requesting: " + err.Error())
- }
- defer resp.Body.Close()
+ // Can't use req() because it retries login failures, which would be an infinite loop.
+ reqF := composeReqFuncs(makeRequestWithHeader, []MidReqF{reqTryLatest, reqFallback, reqAPI})
- var alerts tc.Alerts
- if err := json.NewDecoder(resp.Body).Decode(&alerts); err != nil {
- return remoteAddr, errors.New("decoding response JSON: " + err.Error())
+ reqInf, err := reqF(to, http.MethodPost, path, body, nil, &alerts)
+ if err != nil {
+ return reqInf.RemoteAddr, fmt.Errorf("Login error %v, alertss string: %+v", err, alerts)
Review comment:
Fixed
##########
File path: traffic_ops/v3-client/endpoints.go
##########
@@ -15,4 +15,47 @@
package client
+// DEPRECATED: All new code should us Session.APIBase().
+// This isn't public, but only exists for deprecated public constants. It should be removed when they are.
const apiBase = "/api/3.1"
+
+const apiBaseStr = "/api/"
+
+// apiVersions is the list of minor API versions in this client's major version.
+// This should be all minor versions from 0 up to the latest minor in Traffic Control
+// as of this client code.
+//
+// Versions are ordered latest-first.
+func apiVersions() []string {
+ return []string{
+ "3.1",
+ "3.0",
+ }
+}
+
+// APIBase returns the base API string for HTTP requests, such as /api/3.1.
+// If UseLatestSupportedAPI
+func (sn *Session) APIBase() string {
+ if sn.latestSupportedAPI == "" {
Review comment:
Changed
##########
File path: traffic_ops/v3-client/crconfig.go
##########
@@ -34,7 +37,7 @@ type OuterResponse struct {
// GetCRConfig returns the raw JSON bytes of the CRConfig from Traffic Ops, and whether the bytes were from the client's internal cache.
func (to *Session) GetCRConfig(cdn string) ([]byte, ReqInf, error) {
- uri := apiBase + `/cdns/` + cdn + `/snapshot`
+ uri := `/cdns/` + cdn + `/snapshot`
bts, reqInf, err := to.getBytesWithTTL(uri, tmPollingInterval)
Review comment:
Ok, I removed `getBytesWithTTL` and `getBytes`, and changed GetCRConfig()` and `GetCRConfigNew` to use the standard `to.req`, and changed `makeRequestWithHeader` to put the raw bytes in the `response` if it's a `*[]byte`.
That should also make it easier to add support for getting unparsed bytes in the future.
##########
File path: traffic_ops/v3-client/crconfig.go
##########
@@ -34,7 +37,7 @@ type OuterResponse struct {
// GetCRConfig returns the raw JSON bytes of the CRConfig from Traffic Ops, and whether the bytes were from the client's internal cache.
func (to *Session) GetCRConfig(cdn string) ([]byte, ReqInf, error) {
- uri := apiBase + `/cdns/` + cdn + `/snapshot`
+ uri := `/cdns/` + cdn + `/snapshot`
bts, reqInf, err := to.getBytesWithTTL(uri, tmPollingInterval)
Review comment:
Ok, I removed `getBytesWithTTL` and `getBytes`, and changed `GetCRConfig()` and `GetCRConfigNew` to use the standard `to.req`, and changed `makeRequestWithHeader` to put the raw bytes in the `response` if it's a `*[]byte`.
That should also make it easier to add support for getting unparsed bytes in the future.
##########
File path: traffic_ops/v3-client/crconfig.go
##########
@@ -34,7 +37,7 @@ type OuterResponse struct {
// GetCRConfig returns the raw JSON bytes of the CRConfig from Traffic Ops, and whether the bytes were from the client's internal cache.
func (to *Session) GetCRConfig(cdn string) ([]byte, ReqInf, error) {
- uri := apiBase + `/cdns/` + cdn + `/snapshot`
+ uri := `/cdns/` + cdn + `/snapshot`
bts, reqInf, err := to.getBytesWithTTL(uri, tmPollingInterval)
Review comment:
>I left these methods out of the client refactoring due to getBytesWithTTL's bizarre caching implementation, so I'm not sure what we want to do here. I didn't really want to go down the TM rabbit hole to see if we could change this to not use getBytesWithTTL, but that seems like it would be ideal because caching in the client like this is kind of bizarre and probably unnecessary.
Agreed. I have a high degree of confidence that nothing in TM relies on this, and if anything was causing bugs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rob05c commented on a change in pull request #5430: Add Traffic Ops Client Minor Fallback
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #5430:
URL: https://github.com/apache/trafficcontrol/pull/5430#discussion_r568953002
##########
File path: traffic_ops/v3-client/crconfig.go
##########
@@ -34,7 +37,7 @@ type OuterResponse struct {
// GetCRConfig returns the raw JSON bytes of the CRConfig from Traffic Ops, and whether the bytes were from the client's internal cache.
func (to *Session) GetCRConfig(cdn string) ([]byte, ReqInf, error) {
- uri := apiBase + `/cdns/` + cdn + `/snapshot`
+ uri := `/cdns/` + cdn + `/snapshot`
bts, reqInf, err := to.getBytesWithTTL(uri, tmPollingInterval)
Review comment:
>I left these methods out of the client refactoring due to getBytesWithTTL's bizarre caching implementation, so I'm not sure what we want to do here. I didn't really want to go down the TM rabbit hole to see if we could change this to not use getBytesWithTTL, but that seems like it would be ideal because caching in the client like this is kind of bizarre and probably unnecessary.
Agreed. I have a high degree of confidence that nothing in TM relies on this, and if anything was causing bugs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rob05c commented on a change in pull request #5430: Add Traffic Ops Client Minor Fallback
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #5430:
URL: https://github.com/apache/trafficcontrol/pull/5430#discussion_r568949359
##########
File path: traffic_ops/v3-client/crconfig.go
##########
@@ -34,7 +37,7 @@ type OuterResponse struct {
// GetCRConfig returns the raw JSON bytes of the CRConfig from Traffic Ops, and whether the bytes were from the client's internal cache.
func (to *Session) GetCRConfig(cdn string) ([]byte, ReqInf, error) {
- uri := apiBase + `/cdns/` + cdn + `/snapshot`
+ uri := `/cdns/` + cdn + `/snapshot`
bts, reqInf, err := to.getBytesWithTTL(uri, tmPollingInterval)
Review comment:
Ok, I removed `getBytesWithTTL` and `getBytes`, and changed `GetCRConfig()` and `GetCRConfigNew` to use the standard `to.req`, and changed `makeRequestWithHeader` to put the raw bytes in the `response` if it's a `*[]byte`.
That should also make it easier to add support for getting unparsed bytes in the future.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rob05c commented on a change in pull request #5430: Add Traffic Ops Client Minor Fallback
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #5430:
URL: https://github.com/apache/trafficcontrol/pull/5430#discussion_r568900025
##########
File path: traffic_ops/v3-client/deliveryservice_regexes.go
##########
@@ -23,8 +23,11 @@ import (
)
const (
+ // API_DS_REGEXES is Deprecated: will be removed in the next major version. Be aware this may not be the URI being requested, for clients created with Login and ClientOps.ForceLatestAPI false.
// See: https://traffic-control-cdn.readthedocs.io/en/latest/api/v3/deliveryservices_id_regexes.html
API_DS_REGEXES = apiBase + "/deliveryservices/%d/regexes"
+
+ APIDSRegexes = "/deliveryservices/%v/regexes"
Review comment:
I don't have a strong opinion. Personally, it's a run-time error either way, so I'm not inclined to think it buys us anything. But I don't object to changing it
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #5430: Add Traffic Ops Client Minor Fallback
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #5430:
URL: https://github.com/apache/trafficcontrol/pull/5430#discussion_r560562014
##########
File path: traffic_ops/v3-client/crconfig.go
##########
@@ -48,15 +51,15 @@ func (to *Session) GetCRConfig(cdn string) ([]byte, ReqInf, error) {
}
func (to *Session) SnapshotCRConfigWithHdr(cdn string, header http.Header) (ReqInf, error) {
- uri := fmt.Sprintf("%s?cdn=%s", API_SNAPSHOT, url.QueryEscape(cdn))
+ uri := fmt.Sprintf("%s?cdn=%s", APISnapshot, url.QueryEscape(cdn))
resp := OuterResponse{}
reqInf, err := to.put(uri, nil, header, &resp)
return reqInf, err
}
// GetCRConfigNew returns the raw JSON bytes of the latest CRConfig from Traffic Ops, and whether the bytes were from the client's internal cache.
func (to *Session) GetCRConfigNew(cdn string) ([]byte, ReqInf, error) {
- uri := apiBase + `/cdns/` + cdn + `/snapshot/new`
+ uri := `/cdns/` + cdn + `/snapshot/new`
bts, reqInf, err := to.getBytesWithTTL(uri, tmPollingInterval)
Review comment:
see prior comment
##########
File path: traffic_ops/v3-client/topology_queue_updates.go
##########
@@ -26,7 +26,8 @@ import (
)
func (to *Session) TopologiesQueueUpdate(topologyName tc.TopologyName, req tc.TopologiesQueueUpdateRequest) (tc.TopologiesQueueUpdateResponse, ReqInf, error) {
- path := fmt.Sprintf(ApiTopologies+"/%s/queue_update", topologyName)
+ path := fmt.Sprintf("%s/%s/queue_update", APITopologies, topologyName)
+ var reqInf ReqInf
Review comment:
this declaration is unnecessary
##########
File path: traffic_ops/v3-client/deliveryservice_regexes.go
##########
@@ -23,8 +23,11 @@ import (
)
const (
+ // API_DS_REGEXES is Deprecated: will be removed in the next major version. Be aware this may not be the URI being requested, for clients created with Login and ClientOps.ForceLatestAPI false.
// See: https://traffic-control-cdn.readthedocs.io/en/latest/api/v3/deliveryservices_id_regexes.html
API_DS_REGEXES = apiBase + "/deliveryservices/%d/regexes"
+
+ APIDSRegexes = "/deliveryservices/%v/regexes"
Review comment:
Why the `%v` instead of `%d`? When refactoring the client I was trying to replace `%v`s with their more type-specific format for better type safety and clarity where possible.
##########
File path: traffic_ops/v3-client/crconfig.go
##########
@@ -34,7 +37,7 @@ type OuterResponse struct {
// GetCRConfig returns the raw JSON bytes of the CRConfig from Traffic Ops, and whether the bytes were from the client's internal cache.
func (to *Session) GetCRConfig(cdn string) ([]byte, ReqInf, error) {
- uri := apiBase + `/cdns/` + cdn + `/snapshot`
+ uri := `/cdns/` + cdn + `/snapshot`
bts, reqInf, err := to.getBytesWithTTL(uri, tmPollingInterval)
Review comment:
Ah, so `getBytesWithTTL` is kind of an outlier -- I think `GetCRConfig` and `GetCRConfigNew` are the only top-level methods that use this instead of the methods that funnel down to `makeRequestWithHeader`. I believe that is why the v3 API tests failed.
I left these methods out of the client refactoring due to `getBytesWithTTL`'s bizarre caching implementation, so I'm not sure what we want to do here. I didn't really want to go down the TM rabbit hole to see if we could change this to not use `getBytesWithTTL`, but that seems like it would be ideal because caching in the client like this is kind of bizarre and probably unnecessary.
##########
File path: traffic_ops/v3-client/topology.go
##########
@@ -23,12 +23,15 @@ import (
"github.com/apache/trafficcontrol/lib/go-tc"
)
+// ApiTopologies is Deprecated: will be removed in the next major version. Be aware this may not be the URI being requested, for clients created with Login and ClientOps.ForceLatestAPI false.
const ApiTopologies = apiBase + "/topologies"
+const APITopologies = "/topologies"
Review comment:
I wonder -- do these route constants really need to be public? The only reason I could see someone needing them would be to make "raw" requests, in which they would still need the API version prefix.
##########
File path: traffic_ops/v3-client/session.go
##########
@@ -119,22 +226,16 @@ func loginToken(token string) ([]byte, error) {
// login tries to log in to Traffic Ops, and set the auth cookie in the Session. Returns the IP address of the remote Traffic Ops.
func (to *Session) login() (net.Addr, error) {
- credentials, err := loginCreds(to.UserName, to.Password)
- if err != nil {
- return nil, errors.New("creating login credentials: " + err.Error())
- }
+ path := "/user/login"
+ body := tc.UserCredentials{Username: to.UserName, Password: to.Password}
+ alerts := tc.Alerts{}
- path := apiBase + "/user/login"
- resp, remoteAddr, err := to.RawRequestWithHdr("POST", path, credentials, nil)
- resp, remoteAddr, err = to.errUnlessOKOrNotModified(resp, remoteAddr, err, path)
- if err != nil {
- return remoteAddr, errors.New("requesting: " + err.Error())
- }
- defer resp.Body.Close()
+ // Can't use req() because it retries login failures, which would be an infinite loop.
+ reqF := composeReqFuncs(makeRequestWithHeader, []MidReqF{reqTryLatest, reqFallback, reqAPI})
- var alerts tc.Alerts
- if err := json.NewDecoder(resp.Body).Decode(&alerts); err != nil {
- return remoteAddr, errors.New("decoding response JSON: " + err.Error())
+ reqInf, err := reqF(to, http.MethodPost, path, body, nil, &alerts)
+ if err != nil {
+ return reqInf.RemoteAddr, fmt.Errorf("Login error %v, alertss string: %+v", err, alerts)
Review comment:
sp: alertss
##########
File path: traffic_ops/v3-client/endpoints.go
##########
@@ -15,4 +15,47 @@
package client
+// DEPRECATED: All new code should us Session.APIBase().
+// This isn't public, but only exists for deprecated public constants. It should be removed when they are.
const apiBase = "/api/3.1"
+
+const apiBaseStr = "/api/"
+
+// apiVersions is the list of minor API versions in this client's major version.
+// This should be all minor versions from 0 up to the latest minor in Traffic Control
+// as of this client code.
+//
+// Versions are ordered latest-first.
+func apiVersions() []string {
+ return []string{
+ "3.1",
+ "3.0",
+ }
+}
+
+// APIBase returns the base API string for HTTP requests, such as /api/3.1.
+// If UseLatestSupportedAPI
+func (sn *Session) APIBase() string {
+ if sn.latestSupportedAPI == "" {
Review comment:
Could this method just be `return apiBaseStr + APIVersion()`?
##########
File path: traffic_ops/v3-client/session.go
##########
@@ -64,6 +170,7 @@ func NewSession(user, password, url, userAgent string, client *http.Client, useC
}
const DefaultTimeout = time.Second * time.Duration(30)
+const DefaultAPIVersionCheckInterval = time.Second * time.Duration(60)
Review comment:
I know this is consistent w/ the default above, but shouldn't it just be `60*time.Second`? I think they evaluate to the same thing, but `time.Duration(60)` is really 60 ns by itself.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rob05c commented on a change in pull request #5430: Add Traffic Ops Client Minor Fallback
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #5430:
URL: https://github.com/apache/trafficcontrol/pull/5430#discussion_r568907531
##########
File path: traffic_ops/v3-client/session.go
##########
@@ -119,22 +226,16 @@ func loginToken(token string) ([]byte, error) {
// login tries to log in to Traffic Ops, and set the auth cookie in the Session. Returns the IP address of the remote Traffic Ops.
func (to *Session) login() (net.Addr, error) {
- credentials, err := loginCreds(to.UserName, to.Password)
- if err != nil {
- return nil, errors.New("creating login credentials: " + err.Error())
- }
+ path := "/user/login"
+ body := tc.UserCredentials{Username: to.UserName, Password: to.Password}
+ alerts := tc.Alerts{}
- path := apiBase + "/user/login"
- resp, remoteAddr, err := to.RawRequestWithHdr("POST", path, credentials, nil)
- resp, remoteAddr, err = to.errUnlessOKOrNotModified(resp, remoteAddr, err, path)
- if err != nil {
- return remoteAddr, errors.New("requesting: " + err.Error())
- }
- defer resp.Body.Close()
+ // Can't use req() because it retries login failures, which would be an infinite loop.
+ reqF := composeReqFuncs(makeRequestWithHeader, []MidReqF{reqTryLatest, reqFallback, reqAPI})
- var alerts tc.Alerts
- if err := json.NewDecoder(resp.Body).Decode(&alerts); err != nil {
- return remoteAddr, errors.New("decoding response JSON: " + err.Error())
+ reqInf, err := reqF(to, http.MethodPost, path, body, nil, &alerts)
+ if err != nil {
+ return reqInf.RemoteAddr, fmt.Errorf("Login error %v, alertss string: %+v", err, alerts)
Review comment:
Fixed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rob05c commented on a change in pull request #5430: Add Traffic Ops Client Minor Fallback
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #5430:
URL: https://github.com/apache/trafficcontrol/pull/5430#discussion_r568904780
##########
File path: traffic_ops/v3-client/deliveryservice_regexes.go
##########
@@ -23,8 +23,11 @@ import (
)
const (
+ // API_DS_REGEXES is Deprecated: will be removed in the next major version. Be aware this may not be the URI being requested, for clients created with Login and ClientOps.ForceLatestAPI false.
// See: https://traffic-control-cdn.readthedocs.io/en/latest/api/v3/deliveryservices_id_regexes.html
API_DS_REGEXES = apiBase + "/deliveryservices/%d/regexes"
+
+ APIDSRegexes = "/deliveryservices/%v/regexes"
Review comment:
Changed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rob05c commented on a change in pull request #5430: Add Traffic Ops Client Minor Fallback
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #5430:
URL: https://github.com/apache/trafficcontrol/pull/5430#discussion_r568898925
##########
File path: traffic_ops/v3-client/crconfig.go
##########
@@ -34,7 +37,7 @@ type OuterResponse struct {
// GetCRConfig returns the raw JSON bytes of the CRConfig from Traffic Ops, and whether the bytes were from the client's internal cache.
func (to *Session) GetCRConfig(cdn string) ([]byte, ReqInf, error) {
- uri := apiBase + `/cdns/` + cdn + `/snapshot`
+ uri := `/cdns/` + cdn + `/snapshot`
bts, reqInf, err := to.getBytesWithTTL(uri, tmPollingInterval)
Review comment:
I didn't change that. But I can look into changing it to `makeRequestWithHeader`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp commented on pull request #5430: Add Traffic Ops Client Minor Fallback
Posted by GitBox <gi...@apache.org>.
rawlinp commented on pull request #5430:
URL: https://github.com/apache/trafficcontrol/pull/5430#issuecomment-772772286
The TO API v3 tests are failing for some reason.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #5430: Add Traffic Ops Client Minor Fallback
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #5430:
URL: https://github.com/apache/trafficcontrol/pull/5430#discussion_r560562014
##########
File path: traffic_ops/v3-client/crconfig.go
##########
@@ -48,15 +51,15 @@ func (to *Session) GetCRConfig(cdn string) ([]byte, ReqInf, error) {
}
func (to *Session) SnapshotCRConfigWithHdr(cdn string, header http.Header) (ReqInf, error) {
- uri := fmt.Sprintf("%s?cdn=%s", API_SNAPSHOT, url.QueryEscape(cdn))
+ uri := fmt.Sprintf("%s?cdn=%s", APISnapshot, url.QueryEscape(cdn))
resp := OuterResponse{}
reqInf, err := to.put(uri, nil, header, &resp)
return reqInf, err
}
// GetCRConfigNew returns the raw JSON bytes of the latest CRConfig from Traffic Ops, and whether the bytes were from the client's internal cache.
func (to *Session) GetCRConfigNew(cdn string) ([]byte, ReqInf, error) {
- uri := apiBase + `/cdns/` + cdn + `/snapshot/new`
+ uri := `/cdns/` + cdn + `/snapshot/new`
bts, reqInf, err := to.getBytesWithTTL(uri, tmPollingInterval)
Review comment:
see prior comment
##########
File path: traffic_ops/v3-client/topology_queue_updates.go
##########
@@ -26,7 +26,8 @@ import (
)
func (to *Session) TopologiesQueueUpdate(topologyName tc.TopologyName, req tc.TopologiesQueueUpdateRequest) (tc.TopologiesQueueUpdateResponse, ReqInf, error) {
- path := fmt.Sprintf(ApiTopologies+"/%s/queue_update", topologyName)
+ path := fmt.Sprintf("%s/%s/queue_update", APITopologies, topologyName)
+ var reqInf ReqInf
Review comment:
this declaration is unnecessary
##########
File path: traffic_ops/v3-client/deliveryservice_regexes.go
##########
@@ -23,8 +23,11 @@ import (
)
const (
+ // API_DS_REGEXES is Deprecated: will be removed in the next major version. Be aware this may not be the URI being requested, for clients created with Login and ClientOps.ForceLatestAPI false.
// See: https://traffic-control-cdn.readthedocs.io/en/latest/api/v3/deliveryservices_id_regexes.html
API_DS_REGEXES = apiBase + "/deliveryservices/%d/regexes"
+
+ APIDSRegexes = "/deliveryservices/%v/regexes"
Review comment:
Why the `%v` instead of `%d`? When refactoring the client I was trying to replace `%v`s with their more type-specific format for better type safety and clarity where possible.
##########
File path: traffic_ops/v3-client/crconfig.go
##########
@@ -34,7 +37,7 @@ type OuterResponse struct {
// GetCRConfig returns the raw JSON bytes of the CRConfig from Traffic Ops, and whether the bytes were from the client's internal cache.
func (to *Session) GetCRConfig(cdn string) ([]byte, ReqInf, error) {
- uri := apiBase + `/cdns/` + cdn + `/snapshot`
+ uri := `/cdns/` + cdn + `/snapshot`
bts, reqInf, err := to.getBytesWithTTL(uri, tmPollingInterval)
Review comment:
Ah, so `getBytesWithTTL` is kind of an outlier -- I think `GetCRConfig` and `GetCRConfigNew` are the only top-level methods that use this instead of the methods that funnel down to `makeRequestWithHeader`. I believe that is why the v3 API tests failed.
I left these methods out of the client refactoring due to `getBytesWithTTL`'s bizarre caching implementation, so I'm not sure what we want to do here. I didn't really want to go down the TM rabbit hole to see if we could change this to not use `getBytesWithTTL`, but that seems like it would be ideal because caching in the client like this is kind of bizarre and probably unnecessary.
##########
File path: traffic_ops/v3-client/topology.go
##########
@@ -23,12 +23,15 @@ import (
"github.com/apache/trafficcontrol/lib/go-tc"
)
+// ApiTopologies is Deprecated: will be removed in the next major version. Be aware this may not be the URI being requested, for clients created with Login and ClientOps.ForceLatestAPI false.
const ApiTopologies = apiBase + "/topologies"
+const APITopologies = "/topologies"
Review comment:
I wonder -- do these route constants really need to be public? The only reason I could see someone needing them would be to make "raw" requests, in which they would still need the API version prefix.
##########
File path: traffic_ops/v3-client/session.go
##########
@@ -119,22 +226,16 @@ func loginToken(token string) ([]byte, error) {
// login tries to log in to Traffic Ops, and set the auth cookie in the Session. Returns the IP address of the remote Traffic Ops.
func (to *Session) login() (net.Addr, error) {
- credentials, err := loginCreds(to.UserName, to.Password)
- if err != nil {
- return nil, errors.New("creating login credentials: " + err.Error())
- }
+ path := "/user/login"
+ body := tc.UserCredentials{Username: to.UserName, Password: to.Password}
+ alerts := tc.Alerts{}
- path := apiBase + "/user/login"
- resp, remoteAddr, err := to.RawRequestWithHdr("POST", path, credentials, nil)
- resp, remoteAddr, err = to.errUnlessOKOrNotModified(resp, remoteAddr, err, path)
- if err != nil {
- return remoteAddr, errors.New("requesting: " + err.Error())
- }
- defer resp.Body.Close()
+ // Can't use req() because it retries login failures, which would be an infinite loop.
+ reqF := composeReqFuncs(makeRequestWithHeader, []MidReqF{reqTryLatest, reqFallback, reqAPI})
- var alerts tc.Alerts
- if err := json.NewDecoder(resp.Body).Decode(&alerts); err != nil {
- return remoteAddr, errors.New("decoding response JSON: " + err.Error())
+ reqInf, err := reqF(to, http.MethodPost, path, body, nil, &alerts)
+ if err != nil {
+ return reqInf.RemoteAddr, fmt.Errorf("Login error %v, alertss string: %+v", err, alerts)
Review comment:
sp: alertss
##########
File path: traffic_ops/v3-client/endpoints.go
##########
@@ -15,4 +15,47 @@
package client
+// DEPRECATED: All new code should us Session.APIBase().
+// This isn't public, but only exists for deprecated public constants. It should be removed when they are.
const apiBase = "/api/3.1"
+
+const apiBaseStr = "/api/"
+
+// apiVersions is the list of minor API versions in this client's major version.
+// This should be all minor versions from 0 up to the latest minor in Traffic Control
+// as of this client code.
+//
+// Versions are ordered latest-first.
+func apiVersions() []string {
+ return []string{
+ "3.1",
+ "3.0",
+ }
+}
+
+// APIBase returns the base API string for HTTP requests, such as /api/3.1.
+// If UseLatestSupportedAPI
+func (sn *Session) APIBase() string {
+ if sn.latestSupportedAPI == "" {
Review comment:
Could this method just be `return apiBaseStr + APIVersion()`?
##########
File path: traffic_ops/v3-client/session.go
##########
@@ -64,6 +170,7 @@ func NewSession(user, password, url, userAgent string, client *http.Client, useC
}
const DefaultTimeout = time.Second * time.Duration(30)
+const DefaultAPIVersionCheckInterval = time.Second * time.Duration(60)
Review comment:
I know this is consistent w/ the default above, but shouldn't it just be `60*time.Second`? I think they evaluate to the same thing, but `time.Duration(60)` is really 60 ns by itself.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rob05c commented on pull request #5430: Add Traffic Ops Client Minor Fallback
Posted by GitBox <gi...@apache.org>.
rob05c commented on pull request #5430:
URL: https://github.com/apache/trafficcontrol/pull/5430#issuecomment-771959765
Fixed merge conflicts.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rob05c commented on a change in pull request #5430: Add Traffic Ops Client Minor Fallback
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #5430:
URL: https://github.com/apache/trafficcontrol/pull/5430#discussion_r568906624
##########
File path: traffic_ops/v3-client/session.go
##########
@@ -64,6 +170,7 @@ func NewSession(user, password, url, userAgent string, client *http.Client, useC
}
const DefaultTimeout = time.Second * time.Duration(30)
+const DefaultAPIVersionCheckInterval = time.Second * time.Duration(60)
Review comment:
Changed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #5430: Add Traffic Ops Client Minor Fallback
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #5430:
URL: https://github.com/apache/trafficcontrol/pull/5430#discussion_r569697012
##########
File path: traffic_ops/v3-client/topology.go
##########
@@ -23,12 +23,15 @@ import (
"github.com/apache/trafficcontrol/lib/go-tc"
)
+// ApiTopologies is Deprecated: will be removed in the next major version. Be aware this may not be the URI being requested, for clients created with Login and ClientOps.ForceLatestAPI false.
const ApiTopologies = apiBase + "/topologies"
+const APITopologies = "/topologies"
Review comment:
Ok, I do like keeping the surface area of public symbols to a minimum, but I don't care too strongly about this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org