-
Notifications
You must be signed in to change notification settings - Fork 19
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.
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
feat: access-api proxy.js has configurable options.catchInvocationError, by default catches HTTPError -> error result w/ status=502 #366
Conversation
…ich defaults to something that catches HTTPErrors and rewrites to a status=502 error result
issuer: proxyInvocationIssuer, | ||
capability: capabilities[0], | ||
audience, | ||
proofs: [invocationIn], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should add the other props from invocationIn like expiration, etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
github is forcing me to add another comments because i approved instead of requesting changes lol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Why? I don't think that will add anything. All that info will already be in the resulting
proxyInvocation
proof chain because the wholeinvocationIn
(including itsexpiration
is inproofs
). wdyt @Gozala - That's not related to the point of this PR, so if you're proposing changing the proxy logic, let's do it as a separate issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? I don't think that will add anything. All that info will already be in the resulting proxyInvocation proof chain because the whole invocationIn (including its expiration is in proofs). wdyt @Gozala
You are correct that expiration in the invocationIn
will restrict time bounds, however if you do not specify expiration here it will automatically default to 30secs (if I'm not mistaken), which is to say it may be shorter than what's in the invocationIn
.
In other words I think it is a good idea to include expiration
and notBefore
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also prompted me to look at take another look at the spec and it seems to state
All proofs MUST contain time bounds equal to or broader than the UCAN being delegated. If the proof expires before the outer UCAN — or starts after it — the reader MUST treat the UCAN as invalid.
Which is to say that UCANs that not copying those time bounds would render a UCAN invalid, that said I'm not sure if this is how ucanto does it, but I'll check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah I didn't realize it would default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created storacha-network/ucanto#198
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which is to say that UCANs that not copying those time bounds would render a UCAN invalid, that said I'm not sure if this is how ucanto does it, but I'll check
I read it differently... in this case of proxying, the incoming invocationIn
becomes the proof. Let's say it expires in 24hrs. Let's say without copying over that expiry, the proxyInvocation
gets created with a 30sec expiry.
All proofs MUST contain time bounds equal to or broader than the UCAN being delegated.
The time bounds of the proof invocationIn
is [t0, 0 + 24hrs]. The time bounds of 'the UCAN being delegated' I think is the proxyInvocation
whose time bounds is [t1, t1 + 30sec]. Assuming t1 - t0 ~= 0
, then I think it's fair to say the time bounds of the proof are broader than the invocation/delegation.
If the proof expires before the outer UCAN — or starts after it — the reader MUST treat the UCAN as invalid.
The proof would not expire before the 'outer UCAN'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it would be bad if invocationIn
expiry was like 15sec, and the proxyInvocation
expiry defaulted to 30sec. So that makes it worth copying the expiration for that scenario
issuer: proxyInvocationIssuer, | ||
capability: capabilities[0], | ||
audience, | ||
proofs: [invocationIn], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
github is forcing me to add another comments because i approved instead of requesting changes lol
…In in proxyInvocation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make sure i understand this catches upload api 500s and instead of throwing returns a generic Bad Gateway with the error in x-proxy-error
?
*/ | ||
function defaultCatchInvocationError(error) { | ||
const badGatewayResult = BadGatewayHTTPErrorResult.catch(error) | ||
if (badGatewayResult) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should probably send error
to sentry here
return result | ||
} catch (error) { | ||
if (catchInvocationError) { | ||
const caughtResult = await catchInvocationError(error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need this to be a promise ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, but resolving a returned promise is intentional so the process of building a result from the error can be async.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since @hugomrdias is reviewing this I'll let him handle this.
@hugomrdias yes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, only thing missing is sending to sentry the error but that is not blocking
🤖 I have created a release *beep* *boop* --- ## [4.6.0](access-api-v4.5.0...access-api-v4.6.0) (2023-01-19) ### Features * access-api proxy.js has configurable options.catchInvocationError, by default catches HTTPError -> error result w/ status=502 ([#366](#366)) ([c8ca473](c8ca473)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
…or, by default catches HTTPError -> error result w/ status=502 (#366) , which defaults to something that catches HTTPErrors and rewrites to a status=502 error result Motivation: * relates to: #363 * instead of that behavior of getting a `HandlerExecutionError` (due to internally having an uncaught `HTTPError`), after this PR we should * not have the uncaught `HTTPError`, so no uncaught `HandlerExecutionError` * the result will have `status=502` and `x-proxy-error` with information about the proxy invocation request that errored (which will help debug #363) Limitations * This inlcudes in `result['x-proxy-error']` any info from the underlying `@ucanto/transport/http` `HTTPError`, but unless/until we put more info on that error, we still won't have the raw response object and e.g. won't have response headers. * but if those properties are ever added to `HTTPError`, they should show up in `x-proxy-error`
🤖 I have created a release *beep* *boop* --- ## [4.6.0](access-api-v4.5.0...access-api-v4.6.0) (2023-01-19) ### Features * access-api proxy.js has configurable options.catchInvocationError, by default catches HTTPError -> error result w/ status=502 ([#366](#366)) ([57d0fa4](57d0fa4)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
, which defaults to something that catches HTTPErrors and rewrites to a status=502 error result
Motivation:
HandlerExecutionError
+HTTPError
#363HandlerExecutionError
(due to internally having an uncaughtHTTPError
), after this PR we shouldHTTPError
, so no uncaughtHandlerExecutionError
status=502
andx-proxy-error
with information about the proxy invocation request that errored (which will help debug access-api: invoking store/list in production errors withHandlerExecutionError
+HTTPError
#363)Limitations
result['x-proxy-error']
any info from the underlying@ucanto/transport/http
HTTPError
, but unless/until we put more info on that error, we still won't have the raw response object and e.g. won't have response headers.HTTPError
, they should show up inx-proxy-error