8158 bug UI not using presign for uploading objects #8365

ItamarYuran · 2024-11-13T12:06:20Z

Change Description

With the added changes, it is now possible to upload a file from the UI, with a minimal IAM permissions installation.

Background

Bug Fix

Root cause - the GET request for the presigned url did not include a checksum of the desired file.
Solution - Adding the checksum parameter to the api for getPhysicalAddress operation

Additional info

In order to upload a file to a presigned url, there is a need to configure the bucket's CORS.

github-actions · 2024-11-13T12:07:49Z

🎊 PR Preview 4f7f211 has been successfully built and deployed to https://treeverse-lakeFS-preview-pr-8365.surge.sh

🕐 Build time: 0.013s

_{🤖 By surge-preview}

github-actions · 2024-11-13T12:14:19Z

E2E Test Results - DynamoDB Local - Local Block Adapter

github-actions · 2024-11-13T12:15:35Z

E2E Test Results - Quickstart

Jonathan-Rosenberg

Really great catch, thanks!
A few change requests and a few questions and we're done 😃

Jonathan-Rosenberg · 2024-11-24T10:15:23Z

webui/src/pages/repositories/repository/objects.jsx

+    const parsedHeaders = {};
+
+    headerLines.forEach((line) => {
+    const [key, value] = line.split(':', 2).map((part) => part.trim());
+    if (key && value) {
+      parsedHeaders[key.toLowerCase()] = value; 
    }


More of a styling suggestion to initialize the parsedHeaders variable with data in a more functional way:

Suggested change

const parsedHeaders = {};

headerLines.forEach((line) => {

const [key, value] = line.split(':', 2).map((part) => part.trim());

if (key && value) {

parsedHeaders[key.toLowerCase()] = value;

}

const parsedHeaders = headerLines.reduce((acc, line) => {

const [key, value] = line.split(':', 2).map((part) => part.trim());

if (key && value) {

acc[key.toLowerCase()] = value;

}

return acc;

}, {});

Jonathan-Rosenberg · 2024-11-24T11:34:49Z

webui/src/pages/repositories/repository/objects.jsx

+        const checksum = extractChecksumFromResponse(uploadResponse.rawHeaders);
+        if (uploadResponse.status >= 400) {
+            throw new Error(`Error uploading file: HTTP ${uploadResponse.status}`);
+        }


Suggested change

const checksum = extractChecksumFromResponse(uploadResponse.rawHeaders);

if (uploadResponse.status >= 400) {

throw new Error(`Error uploading file: HTTP ${uploadResponse.status}`);

}

if (uploadResponse.status >= 400) {

throw new Error(`Error uploading file: HTTP ${uploadResponse.status}`);

}

const checksum = extractChecksumFromResponse(uploadResponse.rawHeaders);

webui/src/pages/repositories/repository/objects.jsx

Jonathan-Rosenberg · 2024-11-24T11:51:34Z

webui/src/pages/repositories/repository/objects.jsx

+    const parsedHeaders = {};
+
+    headerLines.forEach((line) => {
+    const [key, value] = line.split(':', 2).map((part) => part.trim());


Splitting this way means that if there's a : in the value of the line (the value of the header) it won't appear in the array. It's possible that it will never happen but I don't really know. If you want to make sure that the array will be split into a header name and a value (which might include a :) you can use something like:

let [key, ...value] = line.split(':') value = value.join(':')

In addition, it would be great if you could extract the headers map creation out (it could be used on its own), and call it from this function, or even better, call it from the uploadFile function, and pass it to this function.

Jonathan-Rosenberg · 2024-11-24T12:02:11Z

webui/src/lib/api/index.js

@@ -671,9 +671,8 @@ export const uploadWithProgress = (url, file, method = 'POST', onProgress = null
                status: xhr.status,
                body: xhr.responseText,
                contentType: xhr.getResponseHeader('Content-Type'),


Why is this left?
Doesn't rawHeaders include this one as well?

webui/src/lib/api/index.js

ItamarYuran · 2024-11-27T14:14:21Z

Thank you for your review @Jonathan-Rosenberg, I corrected everything you mentioned.

Jonathan-Rosenberg

There are a few more comments (and a one that you didn't address), but the biggest question of them all is:

How was this tested?

When I tried to run it it failed with:

Jonathan-Rosenberg · 2024-11-28T12:32:19Z

webui/src/pages/repositories/repository/objects.jsx

+        if (uploadResponse.status >= 400) {
+            throw new Error(`Error uploading file: HTTP ${uploadResponse.status}`);
+        }
+		const checksum = extractChecksumFromResponse(uploadResponse.rawHeaders);


indentation looks off

webui/src/pages/repositories/repository/objects.jsx

Jonathan-Rosenberg · 2024-11-28T12:33:25Z

webui/src/lib/api/index.js

@@ -1125,7 +1121,7 @@ class Statistics {

 class Staging {
    async get(repoId, branchId, path, presign = false) {
-        const query = qs({path, presign});
+		const query = qs({path, presign});


indentation looks off

Jonathan-Rosenberg · 2024-11-28T13:49:45Z

webui/src/lib/api/index.js

-                status: xhr.status,
-                body: xhr.responseText,


These should not get deleted

Jonathan-Rosenberg · 2024-11-28T13:57:18Z

Defining CORS on the S3 bucket is crucial for the upload to succeed. If the user didn't configure the exposedHeaders field in the CORS policy it won't work.
Please return a proper error message if the checksum couldn't get extracted from the response.

Jonathan-Rosenberg

A few more comments but we're almost there!

Jonathan-Rosenberg · 2024-12-08T09:46:45Z

webui/src/pages/repositories/repository/objects.jsx

+  }
+  // fallback to ETag
+  if (parsedHeaders['etag']) {
+	// drop any quote and space


Indentation again 😆

Jonathan-Rosenberg · 2024-12-08T09:47:38Z

webui/src/lib/api/index.js

        });
-        xhr.addEventListener('error', () => reject(new Error('Upload Failed')));
+        xhr.addEventListener('error', () => resolve({


Could you explain why is the error resolved instead of getting rejected?
If this is because the calling function doesn't catch it, then it should...

The reason for this is that I wanted to throw the correct error. This way if there is an error due to missing CORS setting the calling func will catch it and throw suitable error. In case other piece of code uses this func and upload fails for some reason - the error will just state 'upload failed'.

The proper way of handling errors in Promises is using the try/catch mechanism at the calling function.
This should still reject, and the error (the object returned to the reject function) can be analyzed further by the calling function.
I would like to mention in this context that this is javascript and it's s**t.

webui/src/lib/api/index.js

Jonathan-Rosenberg · 2024-12-08T10:18:47Z

webui/src/pages/repositories/repository/objects.jsx

+    const headersString = typeof rawHeaders === 'string' ? rawHeaders : rawHeaders.toString();
+    const cleanedHeadersString = headersString.trim();
+    const headerLines = cleanedHeadersString.split('\n');
+    const parsedHeaders = headerLines.reduce((acc, line) => {
+        let [key, ...value] = line.split(':'); // split into key and the rest of the value
+        key = key.trim(); 
+        value = value.join(':').trim(); 
+        if (key && value) {
+            acc[key.toLowerCase()] = value;
+        }
+        return acc;
+    }, {});


Please extract these lines to a function called parseRawHeaders (or something like that), and instead of those line run:

const parsedHeaders = parseRawHeaders(rawHeaders); if (parsedHeaders['content-md5']) { .....

I would actually pass the extractChecksumFromResponse function the parsed headers as an argument and let it fetch the checksum from it.

ItamarYuran · 2024-12-09T14:04:48Z

webui/src/lib/api/index.js

+export const parseRawHeaders = (rawHeaders) => {
+    const headersString = typeof rawHeaders === 'string' ? rawHeaders : rawHeaders.toString();
+    const cleanedHeadersString = headersString.trim();
+    const headerLines = cleanedHeadersString.split('\n');
+    const parsedHeaders = headerLines.reduce((acc, line) => {
+        let [key, ...value] = line.split(':'); // split into key and the rest of the value
+        key = key.trim(); 
+        value = value.join(':').trim(); 
+        if (key && value) {
+            acc[key.toLowerCase()] = value;
+        }
+        return acc;
+    }, {});
+    return parsedHeaders;
+};
+


moved this func here, as ive seen that no func was exported from the other file to this, and it is now also being used in this file.

ItamarYuran · 2024-12-09T14:09:21Z

webui/src/lib/api/index.js

    async upload(repoId, branchId, path, fileObject, onProgressFn = null) {
        const query = qs({path});
        const uploadUrl = `${API_ENDPOINT}/repositories/${encodeURIComponent(repoId)}/branches/${encodeURIComponent(branchId)}/objects?` + query;
-        const {status, body, contentType} = await uploadWithProgress(uploadUrl, fileObject, 'POST', onProgressFn)
+        const {status, body, rawHeaders} = await uploadWithProgress(uploadUrl, fileObject, 'POST', onProgressFn)
        if (status !== 201) {
+			const contentType = rawHeaders ? parseRawHeaders(rawHeaders)['content-type'] : undefined;


This func is being used when lakefs is configured not to use presigned url. The change I introduced is due to the change in uploadWithProgress that only returns raw headers. Tested this manually, and in case of an error the func does extract the header and the error message.

Great.
Only the indentation again 😅

Jonathan-Rosenberg · 2024-12-10T09:12:45Z

webui/src/pages/repositories/repository/objects.jsx

-    return response.etag.replace(/[" ]+/g, "");
+    return parsedHeaders['etag'].replace(/"/g, ''); 


Why is the regex different?

weird, can't tell, switching back

Jonathan-Rosenberg

The regex explanation (which I think will not suffice 😅 ) and the indentation, and we're off!

Jonathan-Rosenberg · 2024-12-10T09:15:23Z

webui/src/pages/repositories/repository/objects.jsx

+		try {
+			const uploadResponse = await uploadWithProgress(getResp.presigned_url, file, 'PUT', onProgress, additionalHeaders);
+			const parsedHeaders = parseRawHeaders(uploadResponse.rawHeaders);
+			const checksum = extractChecksumFromResponse(parsedHeaders);
+			await staging.link(repo.id, reference.id, fpath, getResp, checksum, file.size, file.type);
+		} catch(error) {
+			if (error.status >= 400) {
+				throw new Error(`Error uploading file: HTTP ${error.status}`);
+			}
+			if (error.status === 0) {
+				throw new Error(`CORS settings error. Check documentation for more info.`);
+			}
+			throw error;
+		}


I-N-D-E-N-T-A-T-I-O-N

webui/src/pages/repositories/repository/objects.jsx

Jonathan-Rosenberg

Thank you for this and answering all of my comments!

ItamarYuran added 5 commits November 11, 2024 11:01

doesnt work

c9cd2f9

was workin now it aint

5a10afc

works!'

1c7a5f3

lets start

635d4df

better

4f7f211

ItamarYuran added the exclude-changelog PR description should not be included in next release changelog label Nov 13, 2024

ItamarYuran linked an issue Nov 13, 2024 that may be closed by this pull request

[Bug]: UI not using presign for uploading objects #8158

Closed

ItamarYuran added 3 commits November 20, 2024 15:12

works, somehow

825d817

looks gooood

4bc6cdb

classy

5934a42

ItamarYuran requested review from Isan-Rivkin and Jonathan-Rosenberg November 21, 2024 10:31

Jonathan-Rosenberg requested changes Nov 24, 2024

View reviewed changes

with changes

c2d9149

Jonathan-Rosenberg requested changes Nov 28, 2024

View reviewed changes

Jonathan-Rosenberg reviewed Nov 28, 2024

View reviewed changes

ItamarYuran added 2 commits December 1, 2024 10:14

check identetion

1429bb8

this time baby

1bb78a7

Jonathan-Rosenberg requested changes Dec 8, 2024

View reviewed changes

ItamarYuran added 2 commits December 8, 2024 15:08

changes

f5a2762

maybe this time

2e739ac

ItamarYuran commented Dec 9, 2024

View reviewed changes

Jonathan-Rosenberg reviewed Dec 10, 2024

View reviewed changes

Jonathan-Rosenberg requested changes Dec 10, 2024

View reviewed changes

ItamarYuran added 2 commits December 10, 2024 12:21

minor changes

2b3ec1f

ohh come on

2917355

Jonathan-Rosenberg reviewed Dec 10, 2024

View reviewed changes

webui/src/pages/repositories/repository/objects.jsx Outdated Show resolved Hide resolved

HTTP

e767f86

ItamarYuran force-pushed the 8158-bug-ui-not-using-presign-for-uploading-objects branch from 31bff69 to e767f86 Compare December 10, 2024 12:21

Jonathan-Rosenberg approved these changes Dec 10, 2024

View reviewed changes

ItamarYuran merged commit deb40d4 into master Dec 10, 2024
39 checks passed

ItamarYuran deleted the 8158-bug-ui-not-using-presign-for-uploading-objects branch December 10, 2024 12:53

Jonathan-Rosenberg added include-changelog PR description should be included in next release changelog and removed exclude-changelog PR description should not be included in next release changelog labels Dec 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8158 bug UI not using presign for uploading objects #8365

8158 bug UI not using presign for uploading objects #8365

ItamarYuran commented Nov 13, 2024

github-actions bot commented Nov 13, 2024 •

edited

Loading

github-actions bot commented Nov 13, 2024

github-actions bot commented Nov 13, 2024

Jonathan-Rosenberg left a comment

Jonathan-Rosenberg Nov 24, 2024

Jonathan-Rosenberg Nov 24, 2024

Jonathan-Rosenberg Nov 24, 2024

Jonathan-Rosenberg Nov 24, 2024

Jonathan-Rosenberg Nov 24, 2024

ItamarYuran commented Nov 27, 2024

Jonathan-Rosenberg left a comment

Jonathan-Rosenberg Nov 28, 2024

Jonathan-Rosenberg Nov 28, 2024

Jonathan-Rosenberg Nov 28, 2024

Jonathan-Rosenberg commented Nov 28, 2024

Jonathan-Rosenberg left a comment

Jonathan-Rosenberg Dec 8, 2024

Jonathan-Rosenberg Dec 8, 2024

ItamarYuran Dec 8, 2024

Jonathan-Rosenberg Dec 8, 2024

Jonathan-Rosenberg Dec 8, 2024

ItamarYuran Dec 9, 2024

ItamarYuran Dec 9, 2024

Jonathan-Rosenberg Dec 10, 2024

Jonathan-Rosenberg Dec 10, 2024

ItamarYuran Dec 10, 2024

Jonathan-Rosenberg left a comment

Jonathan-Rosenberg Dec 10, 2024

Jonathan-Rosenberg left a comment

		return response.etag.replace(/[" ]+/g, "");
		return parsedHeaders['etag'].replace(/"/g, '');

8158 bug UI not using presign for uploading objects #8365

8158 bug UI not using presign for uploading objects #8365

Conversation

ItamarYuran commented Nov 13, 2024

Change Description

Background

Bug Fix

Additional info

github-actions bot commented Nov 13, 2024 • edited Loading

github-actions bot commented Nov 13, 2024

E2E Test Results - DynamoDB Local - Local Block Adapter

github-actions bot commented Nov 13, 2024

E2E Test Results - Quickstart

Jonathan-Rosenberg 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ItamarYuran commented Nov 27, 2024

Jonathan-Rosenberg left a comment

Choose a reason for hiding this comment

How was this tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jonathan-Rosenberg commented Nov 28, 2024

Jonathan-Rosenberg 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

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

Jonathan-Rosenberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jonathan-Rosenberg left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 13, 2024 •

edited

Loading