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

Allow setting path-style access for object storage #4510

Conversation

djmaze
Copy link

@djmaze djmaze commented Oct 31, 2021

Description

Not every S3 setup allows referencing buckets by subdomain. When hosting your own minio instance or another custom S3 service, you might need to use path-style bucket access instead.

The PR adds a force_path_style configuration option which sets the according option in the S3 SDK.

Related issues

See the original object storage implementation in #4290.

Has this been tested?

  • 👍 yes, I added tests to the test suite
  • 💭 no, because this PR is a draft and still needs work
  • 🙅 no, because this PR does not update server code
  • 🙋 no, because I need help

I had difficulties running the tests. It seems there is more prerequisites needed than what is documented.

@Chocobozzz
Copy link
Owner

I had difficulties running the tests. It seems there is more prerequisites needed than what is documented.

Does https://github.com/Chocobozzz/PeerTube/blob/develop/support/doc/development/tests.md#server-tests help?

@superboum
Copy link

superboum commented Nov 2, 2021

Hi,

I found this issue by doing some research on Peertube and Path Style support.

Indeed, we are developing our own S3 backend (here) and currently we only support the S3 path style. We chose to implement path style first over dns style because it is easier to deploy and maintain.

I would be very happy to see this PR merged as I think it could help many Peertube administrators by enabling them to have a bit less complicated infrastructure (and making Peertube compatible with our open source S3 backed :P).

Fixes #4455

@djmaze
Copy link
Author

djmaze commented Nov 7, 2021

I tried some more to get the tests working, but it currently fails locally with:

  1) Object storage for videos
       Test config
         Should succeed with credentials from env:

      AssertionError: http://localhost:9001/static/webseed/3fbb5d12-3111-4096-a381-8149d929dc44-720.webm does not start with http://videos.localhost:9444/: expected false to be true
      + expected - actual

      -false
      +true
      
      at expectStartWith (shared/extra-utils/miscs/checks.ts:20:77)
      at Context.<anonymous> (server/tests/api/object-storage/videos.ts:359:22)
      at Generator.next (<anonymous>)
      at fulfilled (node_modules/tslib/tslib.js:114:62)
      at processTicksAndRejections (internal/process/task_queues.js:95:5)

I don't know how to proceed on this. This failure occurs with the default config already (so without the new option set).

As the tests on Github are green, isn't that enough for now? Or do you want special tests just for this new configuration option?

@Chocobozzz
Copy link
Owner

Chocobozzz commented Nov 8, 2021

You can check tests server logs in using parse-log command: https://docs.joinpeertube.org/support/doc/development/tests?id=debug-server-logs

Or do you want special tests just for this new configuration option?

We need additional tests:

  • Simple tests
  • Tests with a prefix
  • Tests with a prefix and base URL

See https://github.com/Chocobozzz/PeerTube/blob/develop/server/tests/api/object-storage/videos.ts#L367

@Chocobozzz
Copy link
Owner

Closing due to inactivity, feel free to comment/push commits if you want to reopen :)

@Chocobozzz Chocobozzz closed this Dec 7, 2021
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