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.

Sign up for GitHub

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

Jump to bottom

AV-97465: Handle 419 response properly for go sdk. #1279

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+4 −4
Open

AV-97465: Handle 419 response properly for go sdk. #1279

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d6d4bea
Handle 419 response properly for go sdk.
mayank-avinetworks Nov 24, 2020
3939490
AV-97465: Handle 419 response properly for go sdk.
mayank-avinetworks Nov 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions go/session/avisession.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ func (avisess *AviSession) restMultipartFileObjectUploadRequest(verb string, fil
return err
}
retryReq = true
} else if resp.StatusCode == 419 || (resp.StatusCode >= 500 && resp.StatusCode < 599) {
} else if resp.StatusCode >= 500 && resp.StatusCode < 599 {
resp.Body.Close()
retryReq = true
glog.Infof("Retrying %d due to Status Code %d", retryNum, resp.StatusCode)
Expand Down Expand Up @@ -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 {
Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, I'll try to check the corner cases , if any, and will handle this for 401 by tomorrow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link

Choose a reason for hiding this comment

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

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

resp.Body.Close()
retryReq = true
glog.Infof("Retrying url: %s; retry: %d due to Status Code %d", url, retry, resp.StatusCode)
Expand Down Expand Up @@ -849,7 +849,7 @@ func (avisess *AviSession) restMultipartUploadRequest(verb string, uri string, f
return err
}
retryReq = true
} else if resp.StatusCode == 419 || (resp.StatusCode >= 500 && resp.StatusCode < 599) {
} else if resp.StatusCode >= 500 && resp.StatusCode < 599 {
resp.Body.Close()
retryReq = true
glog.Infof("Retrying %d due to Status Code %d", retry, resp.StatusCode)
Expand Down Expand Up @@ -928,7 +928,7 @@ func (avisess *AviSession) restMultipartDownloadRequest(verb string, uri string,
return err
}
retryReq = true
} else if resp.StatusCode == 419 || (resp.StatusCode >= 500 && resp.StatusCode < 599) {
} else if resp.StatusCode >= 500 && resp.StatusCode < 599 {
resp.Body.Close()
retryReq = true
glog.Infof("Retrying %d due to Status Code %d", retry, resp.StatusCode)
Expand Down