AV-97465: Handle 419 response properly for go sdk. #1279
Conversation
mayank-avinetworks
requested review from
grastogi23,
yograjshisode,
manojkumarjain and
monotosh-avi
| @@ -685,7 +685,7 @@ func (avisess *AviSession) restRequest(verb string, uri string, payload interfac | |||
| return nil, err | |||
| } | |||
| retryReq = true | |||
| } else if resp.StatusCode == 419 || (resp.StatusCode >= 500 && resp.StatusCode < 599) { | |||
| } else if resp.StatusCode >= 500 && resp.StatusCode < 599 { | |||
There was a problem hiding this comment.
Is it possible to return the response in case of 401 also ?
With the current change - the caller would get same the error - "Rest request error, returning to caller" for 401 and 5xx, if avisess.disableControllerStatusCheck is true.
There was a problem hiding this comment.
you had concerns about the behaviour after password change of controller user, for which status code returned will be 419. However, we can change this for 401 as well, but I'm not sure about the consequences of breaking existing yet.
There was a problem hiding this comment.
Yes Mayank. It would be great if we can handle 401 too. The Jira AV-97465 was for handling 401. Then we found some issues with 419, and I had raised that concern also.
There was a problem hiding this comment.
sure, I'll try to check the corner cases , if any, and will handle this for 401 by tomorrow.
There was a problem hiding this comment.
Hi @monotosh-avi : I tried handling the 401 response as well, but we have failures with existing functionality as this entails overhaul of go sdk. we'll handle 401 resp like 419 in coming time.
There was a problem hiding this comment.
In that case, we won't be able to use the updated sdk, instead we'll continue to use the sdk from locally modified vendor. We'll be able to use the latest sdk code when we can get response for 401.
cc @apalsule
There was a problem hiding this comment.
When we say overhaul of SDK, are we referring to existing test suites breaking with the change? @monotosh-avi can you pls share the SDK fork details here where we have modified this?
There was a problem hiding this comment.
I have made the change in vendor of AKO, not in an sdk branch. For reference we can check the avisession.go changes in this PR:
https://github.com/vmware/load-balancer-and-ingress-services-for-kubernetes/pull/147/files#diff-d6e1226035e649ad6ccc9e7826fc9d77
Go SDK delegates control to the controller status poller and also retries to init the session with existing creds in session object after password change which is not a proper way of handling 419 response.
Once the password is changed he older credentials saved in session object are obsolete and there is no benefit of re initialising the session from SDK with older creds.