-
Notifications
You must be signed in to change notification settings - Fork 1
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
Adds Backblaze B2 proxy function for Cloudflare workers #11
base: master
Are you sure you want to change the base?
Conversation
a0c43bf
to
dd8c0b5
Compare
I have tested this on a Cloudflare worker in my name, and need to test it on a more central Kendraio organisation worker. |
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.
Just one small point - otherwise very nice work!
functions/cloudflare_b2_proxy.js
Outdated
console.log('Download authorisation successful'); | ||
|
||
const signedUrlData = await downloadAuthorisation.json(); | ||
const downloadUrl = `${authData.downloadUrl}/file/${B2_BUCKET_NAME}/${fileName}?Authorization=${signedUrlData.authorizationToken}`; |
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 looks a bit untidy - can you split it onto multiple lines more gracefully?
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.
Mh, I don't fully agree @CodeKrakken
I think that is good to have it in one line as it is 1 URL.
To me, the naming is clear enough to make it readable, and I would probably feel a bit more lost if they started to be in separate lines or even more split.
Anyway, just because is clear to me doesn't mean it is for everybody.
Can you make a example of how it would be written if it were on multiple lines @CodeKrakken ?
I am thinking maybe also something like that can be helpful for you:
const b2AuthUrl = authData.downloadUrl;
const authorizationParam = `Authorization=${signedUrlData.authorizationToken}`;
const downloadUrl = `${b2AuthUrl}/file/${B2_BUCKET_NAME}/${fileName}? authorizationParam`;
What do you all think?
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.
Great job @lukestanley !
Just some minor styling comments and a bit more clarity in regard of Cloudflare.
Thank you 🙏
functions/cloudflare_b2_proxy.js
Outdated
|
||
const url = new URL(request.url); | ||
|
||
// Return hello world if no path specified |
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 doesn't seems very helpful to me.
Can we not return random text which can result in confusion?
I would suggest to either return an error or a message like "please specify path" (if we want people to have hints)
functions/cloudflare_b2_proxy.js
Outdated
console.log(`Using bucket: ${B2_BUCKET_NAME} (${B2_BUCKET_ID})`); | ||
|
||
// Step 1: Authorise with Backblaze B2 | ||
const authResponse = await fetch('https://api.backblazeb2.com/b2api/v2/b2_authorize_account', { |
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.
Can you please wrap the fetch()
in a try/catch block?
functions/cloudflare_b2_proxy.js
Outdated
const apiUrl = authData.apiUrl; | ||
|
||
// Step 2: Get Download Authorisation | ||
const downloadAuthorisation = await fetch(`${apiUrl}/b2api/v2/b2_get_download_authorization`, { |
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.
same here. A try/catch block pls
functions/cloudflare_b2_proxy.js
Outdated
console.log('Download authorisation successful'); | ||
|
||
const signedUrlData = await downloadAuthorisation.json(); | ||
const downloadUrl = `${authData.downloadUrl}/file/${B2_BUCKET_NAME}/${fileName}?Authorization=${signedUrlData.authorizationToken}`; |
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.
Mh, I don't fully agree @CodeKrakken
I think that is good to have it in one line as it is 1 URL.
To me, the naming is clear enough to make it readable, and I would probably feel a bit more lost if they started to be in separate lines or even more split.
Anyway, just because is clear to me doesn't mean it is for everybody.
Can you make a example of how it would be written if it were on multiple lines @CodeKrakken ?
I am thinking maybe also something like that can be helpful for you:
const b2AuthUrl = authData.downloadUrl;
const authorizationParam = `Authorization=${signedUrlData.authorizationToken}`;
const downloadUrl = `${b2AuthUrl}/file/${B2_BUCKET_NAME}/${fileName}? authorizationParam`;
What do you all think?
functions/cloudflare_b2_proxy.js
Outdated
|
||
// Step 3: Fetch the content from signed URL | ||
console.log(`Fetching content from signed URL for: ${fileName}`); | ||
const fileResponse = await fetch(downloadUrl); |
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.
and try/catch again :)
functions/cloudflare_b2_proxy.js
Outdated
@@ -0,0 +1,113 @@ | |||
/** | |||
* Cloudflare Workers script to proxy requests to Backblaze B2 with |
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.
I am a bit confuse, why is this a Cloudflare workers script? I don't see anywhere in the code that this script talks to Cloudflare. Or am I missing something?
Or maybe you means that this script is written and meant to be hosted in Cloudflare?
If so, I would encourage to change the comment as one day we could decided to host it somewhere else and i believe it will still work.
|
||
// Return response if no path is specified | ||
if (url.pathname === "/" || !url.pathname) { | ||
return new Response("The proxy server is active. This is the root path. Note: To access a file resource, append the filename to the path.", { |
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.
🤩
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.
Looks good to me! Thank you for your changes @lukestanley 🙏
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.
Lovely stuff!
It made sense to have a service specific proxy for the S3 like API, Backblaze B2 to share all (audio files) from a file bucket in a way that is transparent to the end user.
The most simple way to deploy seemed to be a Cloudflare worker.