Skip to content

Add support for MiniO storage backend#667

Open
alfeg wants to merge 3 commits into
loic-sharma:mainfrom
alfeg:main
Open

Add support for MiniO storage backend#667
alfeg wants to merge 3 commits into
loic-sharma:mainfrom
alfeg:main

Conversation

@alfeg
Copy link
Copy Markdown

@alfeg alfeg commented Jul 14, 2021

This PR is yet another attempt to address #621, #551, and #271

Added additional settings to S3StorageOptions:

  • ForcePathStyle = false setting to true will make AWSSDK.net compatible with MiniO installations
  • ServiceUrl - ability to override S3 service location
  • UseHttp - ability to use HTTP protocol for ServiceUrl

All those settings are real settings from actual AmazonS3Config from official AWSSDK library

There is already another PR's to address those issues: #644 and #272

My PR is much less intrusive, only adding ability to pass settings that are already exists in AwsSdk library

@viceice
Copy link
Copy Markdown
Contributor

viceice commented Sep 20, 2021

@loic-sharma Can you please add this? I like BaGet but would like to use MinIO as storage backend in my kubernetes cluster instead of nfs.

Comment thread docs/installation/aws.md
Comment thread src/BaGet.Aws/S3StorageOptions.cs Outdated
Comment thread docs/installation/aws.md Outdated
Comment thread docs/installation/aws.md
Comment on lines +26 to +28
"ServiceUrl": "",
"UseHttp": false,
"ForcePathStyle": false
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

It'd be less confusing to folks if we separate the AWS S3 and MinIO samples:

Suggested change
"ServiceUrl": "",
"UseHttp": false,
"ForcePathStyle": false

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.

Sound good

@loic-sharma
Copy link
Copy Markdown
Owner

loic-sharma commented Sep 25, 2021

Hello @alfeg, thank you for the contribution! Your changes look good. I suggested some minor tweaks but I've never used MinIO. Could you let me know what you think?

@viceice and @DTeuchert Feel free to jump in too if you have comments!


public bool ForcePathStyle { get; set; } = false;

public string ServiceUrl { get; set; }
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Could we add the data annotation attributes that @DTeuchert had used in #272?

-       [Required]
+       [RequiredIf(nameof(ServiceUrl), null)]
        public string Region { get; set; }

+       [RequiredIf(nameof(Region), null)]
        public string ServiceUrl { get; set; }

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yes in my understanding we need a service url for a private / on premise solutions or a region.

var config = new AmazonS3Config
{
RegionEndpoint = RegionEndpoint.GetBySystemName(options.Region)
RegionEndpoint = RegionEndpoint.GetBySystemName(options.Region),
Copy link
Copy Markdown
Owner

@loic-sharma loic-sharma Sep 25, 2021

Choose a reason for hiding this comment

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

From my understanding the region can be set using the service URL. If so should we follow @DTeuchert's pattern in #272:

Suggested change
RegionEndpoint = RegionEndpoint.GetBySystemName(options.Region),
RegionEndpoint = (options.Region != null)
? RegionEndpoint.GetBySystemName(options.Region)
: null,
ServiceURL = options.ServiceUrl

This will let us delete lines 47-50 below:

-               if (!string.IsNullOrWhiteSpace(options.ServiceUrl))
-               {
-                   config.ServiceURL = options.ServiceUrl;
-               }

Copy link
Copy Markdown
Contributor

@viceice viceice Sep 25, 2021

Choose a reason for hiding this comment

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

I think the AWS SDK requires a region independent from service url, but im not fully sure.

@alfeg
Copy link
Copy Markdown
Author

alfeg commented Sep 25, 2021

Hello @alfeg, thank you for the contribution! Your changes look good. I suggested some minor tweaks but I've never used MinIO. Could you let me know what you think?

Hello,

I'm OK with all proposed changes, but cannot test them in any way for next two weeks. I'm on vacation without laptop and with poor internet :)

Copy link
Copy Markdown
Contributor

@viceice viceice left a comment

Choose a reason for hiding this comment

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

Sounds good

@DTeuchert
Copy link
Copy Markdown

On Monday I can test this request with a ceph cluster as s3 backend.

@krmxd
Copy link
Copy Markdown

krmxd commented May 3, 2024

are there any plans on following up on this ?

@viceice
Copy link
Copy Markdown
Contributor

viceice commented May 4, 2024

you should check

@pishangujeniya
Copy link
Copy Markdown

pishangujeniya commented Feb 19, 2026

@loic-sharma Any plans on this?

@loic-sharma
Copy link
Copy Markdown
Owner

No. I'd consider checking out BaGetter instead, it is an active fork of BaGet.

@pishangujeniya
Copy link
Copy Markdown

No. I'd consider checking out BaGetter instead, it is an active fork of BaGet.

that is also not having support of MinIO.

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.

6 participants