Skip LFS fragment on redirect
What does this MR do?
Fixes issue #335495 (closed) : LFS download service not skipping body fragment on redirect:
When mirroring an external repository using object storage (Gitlab + S3), I get a 'size mismatch' error.
This is due to the body of the redirection not being skipped in this code: https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/services/projects/lfs_pointers/lfs_download_service.rb#L63
Does this MR meet the acceptance criteria?
Minor change only impacting LFS download service
Conformity
-
I have included changelog trailers, or none are needed. (Does this MR need a changelog?) -
I have added/updated documentation, or it's not needed. (Is documentation required?) -
I have properly separated EE content from FOSS, or this MR is FOSS only. (Where should EE code go?) -
I have added information for database reviewers in the MR description, or it's not needed. (Does this MR have database related changes?) -
I have self-reviewed this MR per code review guidelines. -
This MR does not harm performance, or I have asked a reviewer to help assess the performance impact. (Merge request performance guidelines) -
I have followed the style guides. -
This change is backwards compatible across updates, or this does not apply.
Availability and Testing
-
Tested to resolve the issue on our Gitlab 13.12.8-ee instance. -
I have added/updated tests following the Testing Guide, or it's not needed. (Consider all test levels. See the Test Planning Process.) -
I have tested this MR in all supported browsers, or it's not needed. -
I have informed the Infrastructure department of a default or new setting change per definition of done, or it's not needed.
Security
Does this MR contain changes to processing or storing of credentials or tokens, authorization and authentication methods or other items described in the security review guidelines? If not, then delete this Security section.
-
Label as security and @ mention @gitlab-com/gl-security/appsec -
The MR includes necessary changes to maintain consistency between UI, API, email, or other methods -
Security reports checked/validated by a reviewer from the AppSec team
Merge request reports
Activity
👋 @ScapalThank you for your contributions to GitLab. We believe that everyone can contribute and contributions like yours are what make GitLab great!
Our Merge Request coaches will ensure your contribution is reviewed in a timely manner. To learn more about the merge request triage process you can refer to the Wider Community Merge Request Triage page.
To bring your merge request to the attention of the relevant team within GitLab, you can ask our bot to label it with a group label. For example, if your merge request changes a project management feature, it can be labelled by commenting
@gitlab-bot label ~"group::project management". To find the most relevant group for your change, you can look up the group based on the most relevant product category in the product categories table. Once you have found the group name, type@gitlab-bot label ~group::, then start to type the group name and select the applicable group label, then submit the comment and the bot will apply the label for you.If after a few days, there's no message from a GitLab team member, feel free to ping
@gitlab-org/coachesor ask for help by commenting@gitlab-bot help.These resources may help you to move your Merge Request to the next steps:
This message was generated automatically. You're welcome to improve it.
added Community contribution label
added 1st contribution label
added devopscreate groupsource code sectiondev typebug + 1 deleted label
@Scapal Thanks for investigating this and contributing this MR!
🙇 @vyaklushin Would you be able to review this community contribution as domain expert?
requested review from @vyaklushin
assigned to @vyaklushin
-
Setting label(s) Category:Source Code Management based on groupsource code.
added Category:Source Code Management label
marked the checklist item I have added information for database reviewers in the MR description, or it's not needed. (Does this MR have database related changes?) as completed
marked the checklist item I have added information for database reviewers in the MR description, or it's not needed. (Does this MR have database related changes?) as incomplete
Thank you for the contribution @Scapal!
👍 Your change makes sense for me! I have only one suggestion. Can you please add a test for this case to cover this fix?
Let me know if you have any questions or need help with that.
- Resolved by Pascal Fautré
Hi @vyaklushin, I never coded in Ruby. I wouldn’t know where to start to simulate a redirection to an object storage in a test.
- Last reply by Vasilii Iakliushin
marked the checklist item I have added information for database reviewers in the MR description, or it's not needed. (Does this MR have database related changes?) as completed
marked the checklist item I have properly separated EE content from FOSS, or this MR is FOSS only. (Where should EE code go?) as completed
marked the checklist item I have added/updated documentation, or it's not needed. (Is documentation required?) as completed
marked the checklist item This change is backwards compatible across updates, or this does not apply. as completed
- Resolved by Pascal Fautré
1/ Do you know what triggers this redirect? Is it a part of S3 configuration setup?
The redirect is part of this kind of configuration: https://docs.gitlab.com/ee/administration/object_storage.html
When
proxy_downloadis false the behaviour is the same as for storing static objects.Here is the schema taken from the Gitlab Doc that matches the LFS on S3 behaviour:
With the bug, the downloaded content starts with the redirection body. Ex.
<html><body>You are being <a href="https://os-front.some-url.com:9024/ce-lfs/eb/70/b302b50fc3203c6810192e10da7f3abcd2403282ec9dbeae977d89445490?X-Amz-Expires=600&X-Amz-Date=20210708T091927Z&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=gitlab%2F20210708%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-SignedHeaders=host&X-Amz-Signature=169deadbeefdeadbeefdeadbeeff9e">redirected</a>.</body></html>2/ Have you had a chance to manually test your fix? Did it work for you?
Yes, our production server is now patched with this merge request and LFS mirroring is now functioning as expected.
3/ When I setup a S3 object storage, will the problem with redirects be visible immediately? What steps should I follow to reproduce it?
Yes, just be sure that
proxy_downloadis false in the common object storage configuration, so that the content is using a redirection instead of being proxied through Gitlab.In the
application.logof the Gitlab trying to pull mirror the repository from the remote server containing LFS objects stored externally, you will see errors about mismatched object size. - Last reply by Pascal Fautré
- Resolved by Pascal Fautré
Thank you for your fix @Scapal!
👍 I approve it.Hi @robotmay_gitlab!
👋 Can you please review it as a maintainer? - Last reply by Robert May
requested review from @robotmay_gitlab and removed review request for @vyaklushin
- A deleted user
added backend label
2 Warnings ⚠ 3e9e0365: The commit subject must start with a capital letter. For more information, take a look at our Commit message guidelines. ⚠ 260b5261: Use full URLs instead of short references ( gitlab-org/gitlab#123or!123), as short references are displayed as plain text outside of GitLab. For more information, take a look at our Commit message guidelines.1 Message 📖 CHANGELOG missing: If you want to create a changelog entry for GitLab FOSS, add the
Changelogtrailer to the commit message you want to add to the changelog.If you want to create a changelog entry for GitLab EE, also add the
EE: truetrailer to your commit message.If this merge request doesn't need a CHANGELOG entry, feel free to ignore this message.
Reviewer roulette
Changes that require review have been detected!
Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:
Category Reviewer Maintainer backend George Koltsov ( @georgekoltsov) (UTC+1)Tetiana Chupryna ( @brytannia) (UTC+3)To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.
To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.
Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.
If needed, you can retry the
danger-reviewjob that generated this comment.Generated by
🚫 Dangerchanged milestone to %14.2
enabled an automatic merge when the pipeline for dc41d2bb succeeds
marked the checklist item I have included changelog trailers, or none are needed. (Does this MR need a changelog?) as completed
mentioned in commit d112e364
mentioned in commit 559e7a5c
added workflowstaging label
added workflowcanary label and removed workflowstaging label
added workflowproduction label and removed workflowcanary label
added releasedcandidate label
mentioned in commit eac4aa46
added releasedpublished label and removed releasedcandidate label
mentioned in merge request kubitus-project/kubitus-installer!39 (merged)
Hi there, and congratulations on having your first MR merged!
I want to offer you a small token of our appreciation as you’re getting started with your code contributions to GitLab. Please fill out this form, and I will be reaching out with more information on how to receive your special #my1stMRmerged coffee mug.
