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

Fix S3 ObjectStore proxy option #26463

Merged
merged 3 commits into from
Jul 14, 2021
Merged

Conversation

maxbes
Copy link
Contributor

@maxbes maxbes commented Apr 8, 2021

The proxy option in S3 object store does not current work

  'objectstore' => 
  array (
    'class' => '\\OC\\Files\\ObjectStore\\S3',
    'arguments' => 
    array (
    # other_arguments
      'proxy' => 'localhost:3128',
    ),

tries to set an unknown/deprecated "request.options" key when constructing a S3Client. The correct one is "http"

Additionally, S3ObjectTrait::readObject uses fopen() directly, so object store proxy options must also be passed to it

@solracsf solracsf added the 3. to review Waiting for reviews label Apr 10, 2021
Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

I can't test but it looks sane

@rullzer rullzer added this to the Nextcloud 22 milestone Apr 11, 2021
This was referenced May 20, 2021
@blizzz blizzz mentioned this pull request Jun 2, 2021
57 tasks
@MorrisJobke MorrisJobke mentioned this pull request Jun 10, 2021
59 tasks
@blizzz blizzz mentioned this pull request Jun 16, 2021
45 tasks
@blizzz blizzz requested a review from juliushaertl June 16, 2021 13:22
@blizzz blizzz mentioned this pull request Jun 23, 2021
39 tasks
@blizzz blizzz added the bug label Jun 23, 2021
@blizzz blizzz modified the milestones: Nextcloud 22, Nextcloud 23 Jun 24, 2021
Signed-off-by: Maxime Besson <[email protected]>
@maxbes
Copy link
Contributor Author

maxbes commented Jun 30, 2021

Sorry for the delay in fixing this, I did not realize the comment on getProxy() was a blocker. Any chance this PR could get in NC22?

@juliushaertl
Copy link
Member

Failures unrelated

@juliushaertl juliushaertl merged commit 3e67637 into nextcloud:master Jul 14, 2021
@welcome
Copy link

welcome bot commented Jul 14, 2021

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

@juliushaertl
Copy link
Member

/backport to stable22

@orandev
Copy link

orandev commented Sep 1, 2021

Hello @juliushaertl
I see no PR for the backport to NC 22 and I don't see the code of the PR in NC 22.1.1, is it normal?

@juliushaertl
Copy link
Member

Thanks for the hint, seems the backport bot was not picking that up. Let me retry.

@juliushaertl
Copy link
Member

/backport to stable22

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants