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

update file node configuration to provide S3 credentials and force path style #22

Merged
merged 2 commits into from
Aug 7, 2023

Conversation

clems4ever
Copy link
Contributor

@clems4ever clems4ever commented Aug 5, 2023

Description

for self-hosted use cases, users might use an S3-compatible service like MinIO. This service requires authentication using an access key and a secret key however I think it does not really make sense for users to create an AWS configuration on the machine since there is no AWS account involved.

Moreover, for using MinIO, path style is mandatory unless users setup some kind of DNS resolution for their bucket. I think it is best to avoid this complication and simply force the path style with the related option in the AWS SDK. Without that option set in the case of minio, errors are thrown because the minio service cannot be resolved by the DNS resolver.

All this has been tested with MinIO.

What type of PR is this? (check all applicable)

  • 🍕 Feature
  • 🐛 Bug Fix
  • 📝 Documentation Update
  • 🎨 Style
  • 🧑‍💻 Code Refactor
  • 🔥 Performance Improvements
  • ✅ Test
  • 🤖 Build
  • 🔁 CI

Related Tickets & Documents

None

Mobile & Desktop Screenshots/Recordings

NA

Added tests?

  • 👍 yes
  • 🙅 no, because they aren't needed
  • 🙋 no, because I need help
  • 🙋 no, because it's hard to test without adding the minio dependency but I'm open to discuss it

Added to documentation?

  • 📜 README.md
  • 📓 tech-docs
  • 🙅 no documentation needed

@clems4ever
Copy link
Contributor Author

If we merge this PR, I will add some documentation to tech-docs in the self-hosted document.

Also, I would advise to avoid duplicating the configuration datastructures like in https://github.com/anyproto/any-sync-tools/blob/b1ec0d33bb6452ac8582dd34a3a243bae9d4a2c1/any-sync-network/cmd/create.go#L70C9-L70C9 because it would require to keep both in sync. This might lead to issues if it is forgotten, I think it's better to avoid the issue altogether by maintaining the structure in one place and importing it in the other. Just a suggestion.

@clems4ever
Copy link
Contributor Author

Good job on this wonderful project btw. FYI, I have made various PRs already in some of your repos and this one allowed me complete building a fully working self-hosted backup node with docker. I will wrap everything up and share my repo as soon as I have something satisfactory to get published.

for self-hosted use cases, users might use an S3-compatible service like
MinIO. This service requires authentication using access keys however it
does not really make sense for users to create an AWS configuration on
the machine since there is no AWS account involved.

Moreover, for using MinIO, path style is mandatory unless users setup
some kind of DNS resolution for their buckets. I think it is best to
avoid this complication and simply force the path style with the related
option in the AWS SDK.
@cheggaaa
Copy link
Member

cheggaaa commented Aug 7, 2023

Also, I would advise to avoid duplicating the configuration datastructures like in https://github.com/anyproto/any-sync-tools/blob/b1ec0d33bb6452ac8582dd34a3a243bae9d4a2c1/any-sync-network/cmd/create.go#L70C9-L70C9 because it would require to keep both in sync. This might lead to issues if it is forgotten, I think it's better to avoid the issue altogether by maintaining the structure in one place and importing it in the other. Just a suggestion.

I agree, it's not good.
Maybe it will be good if any-sync-tools repo will stores configs and other repos will import it. But seems it's a big change and maybe not very usable for development.
So anyway thanks for the advice, we will think about it!

@cheggaaa cheggaaa merged commit 23d44d8 into anyproto:main Aug 7, 2023
2 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 7, 2023
@cheggaaa
Copy link
Member

cheggaaa commented Aug 7, 2023

Thank you! Merged and v0.3.5 created.

@fuksman
Copy link
Member

fuksman commented Aug 7, 2023

@any contributor @clems4ever code

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants