https://dev.arvados.org/https://dev.arvados.org/favicon.ico?15576888422015-10-08T19:20:15ZArvadosArvados - Bug #7492: [keepproxy] Report upstream server errors as 502 Bad Gateway instead of 404https://dev.arvados.org/issues/7492?journal_id=311412015-10-08T19:20:15ZPeter Amstutzpeter.amstutz@curii.com
<ul><li><strong>Description</strong> updated (<a title="View differences" href="/journals/31141/diff?detail_id=30577">diff</a>)</li></ul> Arvados - Bug #7492: [keepproxy] Report upstream server errors as 502 Bad Gateway instead of 404https://dev.arvados.org/issues/7492?journal_id=311482015-10-08T20:12:28ZNico César
<ul></ul><p>same as in <a class="external" href="https://dev.arvados.org/issues/7491#note-4">https://dev.arvados.org/issues/7491#note-4</a></p> Arvados - Bug #7492: [keepproxy] Report upstream server errors as 502 Bad Gateway instead of 404https://dev.arvados.org/issues/7492?journal_id=313172015-10-14T19:38:07ZBrett Smithbrett.smith@curii.com
<ul><li><strong>Target version</strong> set to <i>2015-10-28 sprint</i></li></ul> Arvados - Bug #7492: [keepproxy] Report upstream server errors as 502 Bad Gateway instead of 404https://dev.arvados.org/issues/7492?journal_id=313242015-10-14T19:45:12ZRadhika Chippadaradhika@curoverse.com
<ul><li><strong>Assigned To</strong> set to <i>Radhika Chippada</i></li></ul> Arvados - Bug #7492: [keepproxy] Report upstream server errors as 502 Bad Gateway instead of 404https://dev.arvados.org/issues/7492?journal_id=314872015-10-19T13:43:43ZPeter Amstutzpeter.amstutz@curii.com
<ul></ul><p>keepclient.go:144 func getOrHead()</p>
<p>If the client was unable to get a final answer from one or more servers (either due to network failure or the server returned a HTTP 408, 429, or 500+ error) it should not return BlockNotFound, it should return some other error. Keepproxy probably does not need to be changed, because it already responds 502 Gateway error as the default case for errors other than BlockNotFound.</p>
<p>Comments from Tom on another <a class="issue tracker-1 status-3 priority-4 priority-default closed parent" title="Bug: [SDK] Go keepclient should retry on network errors (Resolved)" href="https://dev.arvados.org/issues/7491">#7491</a>:</p>
<ul>
<li>Return BlockNotFound only if all servers succeeded and replied exactly 404.</li>
<li>If not returning BlockNotFound, return something like fmt.Errorf("%+v", errs) so the caller can see/report what went wrong.</li>
</ul>
<p>Unresolved questions (ask Tom):</p>
<ul>
<li>What about 401 (Unauthorized) and 403 (Permission denied)?
<ul>
<li>If the same error is returned from every server, that's probably the one that should be returned, but need to define more error types and handle them in keepproxy.</li>
</ul>
</li>
<li>What is the correct response if the query gets a mix of 401, 403 and/or 404 errors?
<ul>
<li>Could happen if the site has a mix of servers with permissions enabled or disabled.</li>
</ul></li>
</ul> Arvados - Bug #7492: [keepproxy] Report upstream server errors as 502 Bad Gateway instead of 404https://dev.arvados.org/issues/7492?journal_id=314932015-10-19T14:53:29ZPeter Amstutzpeter.amstutz@curii.com
<ul></ul><ul>
<li>In keepclient.go, define a new struct <code>KeepRequestError</code>
<ul>
<li>This should be equivalent to the arvados.errors.KeepRequestError in the Python SDK</li>
<li>Should contain an map of keep servers with the last error received resulting from contacting each keep server</li>
</ul>
</li>
<li>getOrHead() should return BlockNotFound if all servers replied http 404.</li>
<li>getOrHead() should return KeepRequestError for any other error</li>
<li>In keepproxy.go, <code>GetBlockHandler.ServeHTTP</code>, add a check for KeepRequestError. This should return a HTTP 422 status code and return a formatted text string of KeepRequestError in the body.</li>
</ul> Arvados - Bug #7492: [keepproxy] Report upstream server errors as 502 Bad Gateway instead of 404https://dev.arvados.org/issues/7492?journal_id=315002015-10-19T16:17:58ZTom Cleggtom@curii.com
<ul><li><strong>Description</strong> updated (<a title="View differences" href="/journals/31500/diff?detail_id=30942">diff</a>)</li></ul> Arvados - Bug #7492: [keepproxy] Report upstream server errors as 502 Bad Gateway instead of 404https://dev.arvados.org/issues/7492?journal_id=315012015-10-19T16:38:27ZTom Cleggtom@curii.com
<ul><li><strong>Description</strong> updated (<a title="View differences" href="/journals/31501/diff?detail_id=30943">diff</a>)</li></ul> Arvados - Bug #7492: [keepproxy] Report upstream server errors as 502 Bad Gateway instead of 404https://dev.arvados.org/issues/7492?journal_id=315072015-10-19T18:16:53ZRadhika Chippadaradhika@curoverse.com
<ul><li><strong>Status</strong> changed from <i>New</i> to <i>In Progress</i></li></ul> Arvados - Bug #7492: [keepproxy] Report upstream server errors as 502 Bad Gateway instead of 404https://dev.arvados.org/issues/7492?journal_id=316082015-10-21T13:57:24ZRadhika Chippadaradhika@curoverse.com
<ul></ul><p>All the details from the description are implemented as suggested. One exception:</p>
<blockquote>
<p>"In all cases, return err.Error() in the response body"</p>
</blockquote>
<p>Setting the body with error message is interfering with "http.Error(resp, err.Error(), status)" in the defer func (Panic: runtime error: invalid memory address or nil pointer dereference). This is resulting in errors in KeepClient which is expecting error and nil reader etc. So, I have not implemented this. Please clarify requirement around this.</p> Arvados - Bug #7492: [keepproxy] Report upstream server errors as 502 Bad Gateway instead of 404https://dev.arvados.org/issues/7492?journal_id=316832015-10-22T20:36:48ZPeter Amstutzpeter.amstutz@curii.com
<ul></ul><p>Reviewing 7492-keepproxy-upstream-errors:</p>
<ul>
<li>Is there a reason why you are using <pre>respErr.Error() == "Block not found'</pre> instead of <pre>respErr == keepclient.BlockNotFound</pre> or <pre>c.Check(err, Equals, keepclient.BlockNotFound)</pre>?</li>
</ul>
<ul>
<li>Instead of calling kc.getSortedRoots(locator) twice, suggest doing this in <code>getOrHead()</code>:</li>
</ul>
<pre>
count404 := len(serversToTry)
...
if resp.StatusCode == 404 {
count404--
}
...
if count404 == 0 {
}
</pre>
<ul>
<li>Can we add a test to keepproxy that checks for permanent and temporary errors from the perspective of the client talking to the proxy?</li>
</ul> Arvados - Bug #7492: [keepproxy] Report upstream server errors as 502 Bad Gateway instead of 404https://dev.arvados.org/issues/7492?journal_id=317002015-10-23T15:20:33ZRadhika Chippadaradhika@curoverse.com
<ul></ul><blockquote>
<p>Is there a reason why you are using respErr.Error() "Block not found'</p>
</blockquote>
<p>Updated to use respErr keepclient.BlockNotFound and c.Check(err, Equals, keepclient.BlockNotFound). Much better now.</p>
<blockquote>
<p>Instead of calling kc.getSortedRoots(locator) twice, suggest doing this in getOrHead() ...</p>
</blockquote>
<p>Removed the unnecessary getSortedRoots call and used numServers variable instead. I did not want to make the change to count404-- as you suggested. Doing so results in code such as if count404 == 0 { set status code to 404 }, which can be quite counter-intuitive even if I use a better variable name :)</p>
<blockquote>
<p>Can we add a test to keepproxy that checks for permanent and temporary errors from the perspective of the client talking to the proxy?</p>
</blockquote>
<p>Added one more test set to do Get with a bad token and expired token. If you have any other specific tests in mind, please let me know.</p>
<p>Thanks.</p> Arvados - Bug #7492: [keepproxy] Report upstream server errors as 502 Bad Gateway instead of 404https://dev.arvados.org/issues/7492?journal_id=317042015-10-23T15:45:31ZPeter Amstutzpeter.amstutz@curii.com
<ul></ul><p>Radhika Chippada wrote:</p>
<blockquote><blockquote>
<p>Is there a reason why you are using respErr.Error() "Block not found'</p>
</blockquote>
<p>Updated to use respErr keepclient.BlockNotFound and c.Check(err, Equals, keepclient.BlockNotFound). Much better now.</p>
<blockquote>
<p>Instead of calling kc.getSortedRoots(locator) twice, suggest doing this in getOrHead() ...</p>
</blockquote>
<p>Removed the unnecessary getSortedRoots call and used numServers variable instead. I did not want to make the change to count404-- as you suggested. Doing so results in code such as if count404 == 0 { set status code to 404 }, which can be quite counter-intuitive even if I use a better variable name :)</p>
</blockquote>
<p>Thanks.</p>
<blockquote><blockquote>
<p>Can we add a test to keepproxy that checks for permanent and temporary errors from the perspective of the client talking to the proxy?</p>
</blockquote>
<p>Added one more test set to do Get with a bad token and expired token. If you have any other specific tests in mind, please let me know.</p>
</blockquote>
<p>Yes, can we have a test where keepproxy generates a Temporary() error? The original goal of this story was for keepproxy to start returning 502 Bad Gateway when one of the keep servers behind the proxy failed in a "temporary" way so as to trigger retry behavior on the external client.</p> Arvados - Bug #7492: [keepproxy] Report upstream server errors as 502 Bad Gateway instead of 404https://dev.arvados.org/issues/7492?journal_id=317222015-10-24T20:02:46ZRadhika Chippadaradhika@curoverse.com
<ul></ul><blockquote>
<p>Yes, can we have a test where keepproxy generates a Temporary() error ...</p>
</blockquote>
<p>Added another test to keepproxy_test with "connection refused" temporary error. Also, improved test assertions in keepclient_test. Thanks.</p> Arvados - Bug #7492: [keepproxy] Report upstream server errors as 502 Bad Gateway instead of 404https://dev.arvados.org/issues/7492?journal_id=317922015-10-27T17:20:47ZPeter Amstutzpeter.amstutz@curii.com
<ul></ul><p>Radhika Chippada wrote:</p>
<blockquote><blockquote>
<p>Yes, can we have a test where keepproxy generates a Temporary() error ...</p>
</blockquote>
<p>Added another test to keepproxy_test with "connection refused" temporary error. Also, improved test assertions in keepclient_test. Thanks.</p>
</blockquote>
<p>Unfortunately, that is not testing what I was looking to be tested. This is testing whether a failed connection to the proxy results in a Temporary error. I'm asking for a test where the client can connect to the proxy, but the proxy can't connect to the keep node.</p> Arvados - Bug #7492: [keepproxy] Report upstream server errors as 502 Bad Gateway instead of 404https://dev.arvados.org/issues/7492?journal_id=319002015-10-28T16:17:42ZPeter Amstutzpeter.amstutz@curii.com
<ul></ul><p><a class="changeset" title="7492: add a test that simulates keep server unavailable error." href="https://dev.arvados.org/projects/arvados/repository/arvados/revisions/0bb6625febf6838867e105e3e7f66a485b639c76">0bb6625</a> looks good to me.</p> Arvados - Bug #7492: [keepproxy] Report upstream server errors as 502 Bad Gateway instead of 404https://dev.arvados.org/issues/7492?journal_id=319022015-10-28T16:23:39ZRadhika Chippadaradhika@curoverse.com
<ul><li><strong>Story points</strong> set to <i>1.0</i></li></ul> Arvados - Bug #7492: [keepproxy] Report upstream server errors as 502 Bad Gateway instead of 404https://dev.arvados.org/issues/7492?journal_id=319042015-10-28T16:25:10ZRadhika Chippadaradhika@curoverse.com
<ul><li><strong>Status</strong> changed from <i>In Progress</i> to <i>Resolved</i></li><li><strong>% Done</strong> changed from <i>0</i> to <i>100</i></li></ul><p>Applied in changeset arvados|commit:b90d3ea4986a1af59395f223e0320f07cc91c272.</p>