[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #5430: Add Traffic Ops Client Minor Fallback

[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