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.

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

src: implement OriginProvider and S3Provider #119

Merged
merged 1 commit into from
Jun 3, 2024

Conversation

flakey5
Copy link
Member

@flakey5 flakey5 commented Apr 24, 2024

Implements the OriginProvider class as described in #111 and allows R2Provider to take in a fallback provider that will be used if R2 fails.

@flakey5 flakey5 requested a review from a team as a code owner April 24, 2024 05:26
@flakey5 flakey5 force-pushed the flakey5/20240414/s3-provider branch from 96a6790 to e1a15d2 Compare April 24, 2024 05:29
this.ctx.sentry
);
} catch (err) {
if (this.fallbackProvider !== undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

shoult this fallback for all errors or 404 only?

Copy link
Member Author

Choose a reason for hiding this comment

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

Any error that reaches here would be regarding the request to R2 failing, if a file doesn't exist in the bucket the binding returns null which is handled a few lines further down.

So only if the initial request to R2 + all of the retries fail with a 5xx, then fallback. Notable that an error can be thrown for 4xx response codes (re #106), but I'll be opening a pr soonish to validate any client-provided variables to prevent a 4xx from ever being the cause here

Copy link
Member

Choose a reason for hiding this comment

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

I'd say this logic should not reside within the R2Provider, but on whatever middleware that manages the providers.

Like, since the responses of these methods are always a Promise with either an object or undefined, we could say, well if we got undefined, go to the next middleware, until we get an object.

We could also have booleans like if a next provider should be attempted by the middleware of just skipped.

So the method types could be Promise<HeadFileResult | boolean> so when false it means, simply did not handle due to X reasons, and if true provider got skipped try next one. So if false was returned it won't do anything anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right now there isn't any middleware that manages the providers. The middleware that is planned however would be something like

const provider = getPrimaryProvider();
const result  = await provider.headFile(...)
//...

(impl ref here)

A response of undefined means that the file/directory doesn't exist, if an error occurs it's thrown. But we might be able to do something like this, where it's a Provider that will loop through all of the providers that

class ParentProvider implements Provider {
  providers: Provider[]

  headFile(path) {
    let response
    for (const provider of this.providers) {
      try {
        response = await provider.headFile(path)
      } catch (err) {
        // continue, do nothing
      }
    }
    return response
  }
  // ...getFile, headDirectory, ...
}

// using it would be:
const provider = new ParentProvider()
const result = await provider.headFile('...')
//...

Imo the former way would be a bit nicer to implement and use however

Copy link
Member

Choose a reason for hiding this comment

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

I feel that having something like a routing middleware makes more sense, and then you have these "s3, origin, etc" middlewares.

I'm really fond of the idea, as it allows us to conditionally handle things on different middlewares on-demand, rather than everything goes in one middleware and if it fails it tries another one.

Copy link
Member

Choose a reason for hiding this comment

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

but even then I think it'd lead to a lot of code duplication with parsing the request since each middleware needs to do it.

Not sure what you're talking about, could you give an example?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure what you're talking about, could you give an example?

From my understanding, an api like get('/dist/*', [r2Middleware, originMiddleware]) means each middleware is a request handler, so we'd need to have the same validation logic in each of the middlewares. Although I think we might be able to have a pass-through middleware specifically for validating the request, get('/dist/*', [validationMiddleware, r2Middleware, originMiddleware])

but there will be cases in which you don't want to use R2 and go straight to the origin

Could you give an example?

Copy link
Member

Choose a reason for hiding this comment

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

means each middleware is a request handler, so we'd need to have the same validation logic in each of the middlewares. Although I think we might be able to have a pass-through

Im pretty sure we can have a base class that handles such things or having a router middleware that handles the basic common validations for each route and respective http method and after it all got validated it calls next() or simply aborts.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll remove the retry stuff from this pr in a bit and will open another pr to explore the middleware approach

Copy link
Member Author

Choose a reason for hiding this comment

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

The retry logic is removed now in the provider impls

src/providers/r2Provider.ts Outdated Show resolved Hide resolved
@flakey5 flakey5 force-pushed the flakey5/20240414/s3-provider branch from e1a15d2 to 1f709ed Compare April 24, 2024 19:14
): Promise<GetFileResult | undefined> {
const res = await fetch(this.ctx.env.ORIGIN_HOST + path, {
headers: {
'user-agent': 'release-cloudflare-worker',
Copy link
Member

Choose a reason for hiding this comment

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

Interesting, love the approach on the cache headers by using content matches, so that the browser can "proactively" fetch and replace on successive calls to this endpoint if an update was done 🆒

@flakey5 flakey5 force-pushed the flakey5/20240414/s3-provider branch 3 times, most recently from 37215f4 to b2c3e11 Compare May 31, 2024 01:33
Implements the `OriginProvider` and `S3Provider` classes as described in #111. Allows `R2Provider` to take in a fallback provider that will be used if R2 fails.
@flakey5 flakey5 force-pushed the flakey5/20240414/s3-provider branch from b2c3e11 to 6bf5d2a Compare June 2, 2024 18:48
options?: GetFileOptions | undefined
): Promise<GetFileResult | undefined> {
const res = await fetch(this.ctx.env.ORIGIN_HOST + path, {
headers: {
Copy link
Member

Choose a reason for hiding this comment

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

TODO: For future, having a function to build these headers extracted from this function would be nice.

Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

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

LGTM

@flakey5 flakey5 merged commit d0a9746 into main Jun 3, 2024
3 checks passed
@flakey5 flakey5 deleted the flakey5/20240414/s3-provider branch June 3, 2024 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants