Skip to content

feat: update cors headers for cross domain request - #33898

Merged
KristinAoki merged 1 commit into
masterfrom
KristinAoki/update-allowed-cors-headers
Dec 12, 2023
Merged

feat: update cors headers for cross domain request#33898
KristinAoki merged 1 commit into
masterfrom
KristinAoki/update-allowed-cors-headers

Conversation

@KristinAoki

Copy link
Copy Markdown
Contributor

Description

The import page chunks the uploaded file and uses the network headers content-range and content-disposition to send the chunk to the backend. These headers are not part of the default cors headers. This change impacts Authors.

Supporting information

JIRA Ticket: TNL-11163

I was able to import a 60MB course. When I tried to do a 250MB course it broke. I tried a bit smaller and it seemed to break around 100MB. It looked like the percent was going up. The percent got to 99%, then the percentage disappeared and the gear would just spin, see screenshots. The legacy page they can import a 500 MB course but with the new page cannot import 100 MB.

Testing instructions

Testing

  1. Navigate to the Import page
  2. If you wan to keep the content of your current course, export it before importing a new one
  3. Upload a course zip that is greater than 100 MB
  4. File should import
  5. Wait for the import to finish
  6. Click "View updated outline"
  7. Check that the course properly imported

Deadline

None

Other information

frontend-app-course-authoring PR #734 is dependent on this change.

@bszabo

bszabo commented Dec 8, 2023

Copy link
Copy Markdown
Contributor

Can you please clarify how these configuration changes fixed the problem? Is it the case that for files under the 100MB threshold no chunking is required, and therefore the offending headers (relative to CORS) are never used? But that chunking occurs with files bigger than this, that the headers are then introduced, and that CORS then blocks the operation?

@KristinAoki

KristinAoki commented Dec 8, 2023

Copy link
Copy Markdown
Contributor Author

Can you please clarify how these configuration changes fixed the problem? Is it the case that for files under the 100MB threshold no chunking is required, and therefore the offending headers (relative to CORS) are never used? But that for files bigger than this chunking occurs, the headers are introduced, and CORS blocks the operation?

Correct, files under 100MB do not require chunking because they are within the accepted file size range. However, when a file >= 100MB is submitted a 413 error is thrown. When no chunking is required, no additional headers are required on the request. However, the chunking requires the additional headers for the backend to know how to stitch the chunks back together. As a result, the additional CORS headers need to be added or the API will always fail with a CORS error. It should be noted, that regardless of file size all files will go through the chunking process when importing in the course authoring mfe.

@bszabo

bszabo commented Dec 11, 2023

Copy link
Copy Markdown
Contributor

Per our slack huddle, where we observed this PRs changes importing a 2GB file on hosted devstack, import looked to be in 1.2 kByte chunks, each taking a few seconds. A back-of-the-envelope calculation suggests that on the order of one million chunks would be required to get to a 2GByte file. A 10 minute delay would be 600 seconds, or maybe 200 chunks. This suggests that the solution we observed would not complete in anything near an acceptable timeframe.

@bszabo bszabo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Based on feedback that upload of 2 GB file took 2 minuts, lgtm

@KristinAoki
KristinAoki merged commit d08e93d into master Dec 12, 2023
@KristinAoki
KristinAoki deleted the KristinAoki/update-allowed-cors-headers branch December 12, 2023 14:09
@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

1 similar comment
@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

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.

3 participants