adding support for publishing bundle to Minio Object Storage (#5395) #5757
Conversation
Dimss
requested review from
evan2645,
amartinezfayo,
azdagron,
MarcosDY and
rturner3
as code owners
|
Thanks for the contribution @Dimss! I'll try to take a look at it this week. |
|
@Dimss Minio is API compatible with AWS S3 API, so it should be possible to use the existing |
|
@sorindumitru I think the main question here, should it be a generic S3 publisher or publisher for each provider with it's backend implementation. |
|
Thank you so much for working on this! :) Its using the minio client, but I think it could work against any s3 server as coded? There are a lot of s3 compatible implementations out there (many storage vendors support it) and they all work with the aws s3 client I believe, or one of the implementations like minio's client. Having one generic s3 driver I think would be preferable to one per storage implementation, as they all essentially work the same. |
|
Having separate implementations for different platforms may make sense eventually, but I feel that for now it only adds maintenance burden. If someone does end up needing something from this plugin where it makes sense to have it use platform specific APIs, such as platform specific authentication methods, we should look into splitting off that part into a separate plugin. @Dimss, do you need anything from an S3 publisher that is Minio specific? For now, it's probably easiest to just add support for specifying an optional endpoint to the awss3 plugin. |
|
@sorindumitru ok by me. |
|
Just endpoint for now I think? if you need to load in a custom ca for talking to minio, it can be done via the system trust bundle. If its actually insecure, not sure there is any advantage to funneling it through the s3 service rather then just using the agents |
|
@kfox1111 agree. then I'll just add one optional |
…other compatible object storage)
|
@kfox1111, @sorindumitru I applied required changes, pls review. |
There was a problem hiding this comment.
Thank you @Dimss for this contribution!
We need to update the documentation of the aws_s3 bundle publisher plugin with the new Endpoint setting, indicating that's an optional setting (not required).
We should also add test coverage for this, making sure to check that the configuration has been properly wired up to the aws config.
| @@ -2230,4 +2230,4 @@ sigs.k8s.io/structured-merge-diff/v4 v4.4.2/go.mod h1:N8f93tFZh9U6vpxwRArLiikrE5 | |||
| sigs.k8s.io/yaml v1.4.0 h1:Mk1wCc2gy/F0THH0TAp1QYyJNzRm2KCLy3o5ASXVI5E= | |||
| sigs.k8s.io/yaml v1.4.0/go.mod h1:Ejl7/uTz7PSA4eKMyQCUTnhZYNmLIl+5c2lQPGR2BPY= | |||
| software.sslmate.com/src/go-pkcs12 v0.4.0 h1:H2g08FrTvSFKUj+D309j1DPfk5APnIdAQAB8aEykJ5k= | |||
| software.sslmate.com/src/go-pkcs12 v0.4.0/go.mod h1:Qiz0EyvDRJjjxGyUQa2cCNZn/wMyzrRJ/qcDXOQazLI= | |||
| software.sslmate.com/src/go-pkcs12 v0.4.0/go.mod h1:Qiz0EyvDRJjjxGyUQa2cCNZn/wMyzrRJ/qcDXOQazLI= | |||
| @@ -2,7 +2,6 @@ package awss3 | |||
|
|
|||
| import ( | |||
| "context" | |||
|
|
|||
There was a problem hiding this comment.
It seems that this is not goimports-ed
| return cfg, nil | ||
| } | ||
|
|
||
| func newS3Client(c aws.Config) (simpleStorageService, error) { | ||
| return s3.NewFromConfig(c), nil | ||
| options := func(options *s3.Options) {} |
There was a problem hiding this comment.
Prefer the use of var for zero value.
| options := func(options *s3.Options) {} | |
| var options func(options *s3.Options) |
As a part of #5395 this PR add support to Minio S3 storage.
The PR does not meant to be merge to main yet.
I do need someone look on the PR and provide initial feedback.
I wanna make sure this functionality is still needed and overall logic does make sense.
if all good, I'll continue to work on it, add tests, etc...