feat: access-api proxy.js has configurable options.catchInvocationError, by default catches HTTPError -> error result w/ status=502 #366

gobengo · 2023-01-19T01:08:42Z

, which defaults to something that catches HTTPErrors and rewrites to a status=502 error result

Motivation:

relates to: access-api: invoking store/list in production errors with HandlerExecutionError + HTTPError #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 access-api: invoking store/list in production errors with HandlerExecutionError + HTTPError #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

…ich defaults to something that catches HTTPErrors and rewrites to a status=502 error result

hugomrdias · 2023-01-19T17:45:05Z

packages/access-api/src/ucanto/proxy.js

+      issuer: proxyInvocationIssuer,
+      capability: capabilities[0],
+      audience,
+      proofs: [invocationIn],


we should add the other props from invocationIn like expiration, etc

github is forcing me to add another comments because i approved instead of requesting changes lol

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

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.

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.

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

ah I didn't realize it would default.

44a054b

Created storacha/ucanto#198

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'.

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

hugomrdias · 2023-01-19T17:47:34Z

packages/access-api/src/ucanto/proxy.js

+      issuer: proxyInvocationIssuer,
+      capability: capabilities[0],
+      audience,
+      proofs: [invocationIn],


github is forcing me to add another comments because i approved instead of requesting changes lol

…In in proxyInvocation

…tps://github.com/web3-storage/w3protocol/actions/runs/3961179127/jobs/6786401949#step:6:30

hugomrdias

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 ?

hugomrdias · 2023-01-19T18:28:07Z

packages/access-api/src/ucanto/proxy.js

+ */
+function defaultCatchInvocationError(error) {
+  const badGatewayResult = BadGatewayHTTPErrorResult.catch(error)
+  if (badGatewayResult) {


we should probably send error to sentry here

hugomrdias · 2023-01-19T18:28:36Z

packages/access-api/src/ucanto/proxy.js

+      return result
+    } catch (error) {
+      if (catchInvocationError) {
+        const caughtResult = await catchInvocationError(error)


do we need this to be a promise ?

no, but resolving a returned promise is intentional so the process of building a result from the error can be async.

Gozala

Since @hugomrdias is reviewing this I'll let him handle this.

gobengo · 2023-01-19T18:54:29Z

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 ?

@hugomrdias yes

hugomrdias

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).

access-api proxy.js has configurable options.catchInvocationError, wh…

ba64da4

…ich defaults to something that catches HTTPErrors and rewrites to a status=502 error result

gobengo changed the title ~~access-api proxy.js has configurable options.catchInvocationError~~ access-api proxy.js has configurable options.catchInvocationError, by default catches HTTPError -> error result w/ status=502 Jan 19, 2023

gobengo requested a review from Gozala January 19, 2023 01:09

gobengo changed the title ~~access-api proxy.js has configurable options.catchInvocationError, by default catches HTTPError -> error result w/ status=502~~ feat: access-api proxy.js has configurable options.catchInvocationError, by default catches HTTPError -> error result w/ status=502 Jan 19, 2023

gobengo temporarily deployed to dev January 19, 2023 01:10 — with GitHub Actions Inactive

gobengo mentioned this pull request Jan 19, 2023

access-api: invoking store/list in production errors with HandlerExecutionError + HTTPError #363

Closed

hugomrdias approved these changes Jan 19, 2023

View reviewed changes

hugomrdias suggested changes Jan 19, 2023

View reviewed changes

access-api ucanto proxy includes expiration,notBefore from invocation…

44a054b

…In in proxyInvocation

gobengo requested a review from hugomrdias January 19, 2023 18:11

adjust how ucanto/proxy.js checks for undefined to appease ci lint ht…

ed1cbf7

…tps://github.com/web3-storage/w3protocol/actions/runs/3961179127/jobs/6786401949#step:6:30

gobengo temporarily deployed to dev January 19, 2023 18:21 — with GitHub Actions Inactive

hugomrdias suggested changes Jan 19, 2023

View reviewed changes

Gozala reviewed Jan 19, 2023

View reviewed changes

hugomrdias approved these changes Jan 19, 2023

View reviewed changes

gobengo merged commit c8ca473 into main Jan 19, 2023

gobengo deleted the 1674089929-proxy-bad-gateway branch January 19, 2023 19:08

hugomrdias mentioned this pull request Jan 19, 2023

chore(main): release access-api 4.6.0 #372

Merged

it-dag-house mentioned this pull request Mar 23, 2023

chore(main): release access-api 11.0.0-rc.0 #620

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: access-api proxy.js has configurable options.catchInvocationError, by default catches HTTPError -> error result w/ status=502 #366

feat: access-api proxy.js has configurable options.catchInvocationError, by default catches HTTPError -> error result w/ status=502 #366

gobengo commented Jan 19, 2023

hugomrdias Jan 19, 2023 •

edited

Loading

hugomrdias Jan 19, 2023

gobengo Jan 19, 2023

Gozala Jan 19, 2023

Gozala Jan 19, 2023

gobengo Jan 19, 2023

Gozala Jan 19, 2023

gobengo Jan 19, 2023

gobengo Jan 19, 2023

hugomrdias Jan 19, 2023

hugomrdias left a comment

hugomrdias Jan 19, 2023

hugomrdias Jan 19, 2023

gobengo Jan 19, 2023

Gozala left a comment

gobengo commented Jan 19, 2023

hugomrdias left a comment

feat: access-api proxy.js has configurable options.catchInvocationError, by default catches HTTPError -> error result w/ status=502 #366

feat: access-api proxy.js has configurable options.catchInvocationError, by default catches HTTPError -> error result w/ status=502 #366

Conversation

gobengo commented Jan 19, 2023

hugomrdias Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hugomrdias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gozala left a comment

Choose a reason for hiding this comment

gobengo commented Jan 19, 2023

hugomrdias left a comment

Choose a reason for hiding this comment

hugomrdias Jan 19, 2023 •

edited

Loading