bucket: Use auto lookup type

[zhiyu0729]

Resolves #1199
change default lookup type from path to auto, it may cause the previous butcket configuration to be unavailable, but I think auto can cover most cases.

[makkes]

Please update the documentation at docs/spec/.

[zhiyu0729]

Please update the documentation at docs/spec/.

Thank you for your advice

[darkowlzz]

Thanks for submitting this fix. I had a short discussion about this with Hidde, who carried the change that introduced the lookup path value in ed6c6eb, a few weeks ago and he wasn't sure why this field was set to BucketLookupPath. I checked the git history to find out when this was introduced and if it worked without setting anything before. The BucketLookupPath lookup value was introduced in v0.22.0, as can be seen from the aforementioned commit. In v0.21.0, it looks like minio.Options didn't had any lookup value, refer https://github.com/fluxcd/source-controller/blob/v0.21.0/controllers/bucket_controller.go#L482 . Also, I couldn't find any mention of "lookup" in the initial PR that the final commit is based on #455.
I also checked if it ever changed on the minio side since source-controller v0.21.0, the default BucketLookupAuto was introduced in 2018 minio/minio-go@ff08912 .

Based on the above, I believe this may have been an unintended change and just using the default would be enough, unless we have good reason to expose this value in Bucket API. As mentioned in #1199, the default auto lookup would fix it for Alibaba Cloud's OSS buckets.

We can also verify with AWS that it continues to work with auto lookup.

[zhiyu0729]

I also approve use auto lookup, Amazon S3 is planning to deprecation path type

https://docs.aws.amazon.com/AmazonS3/latest/userguide/VirtualHosting.html#path-style-access

[darkowlzz]

I did some manual testing by changing the BucketLookup to auto in the code and running source-controller against both S3 and a self-hosted minio server with proper DNS in a subdomain with reverse proxy and it worked for both of them. My minio server was not configured with MINIO_DOMAIN, as mentioned in https://min.io/docs/minio/linux/reference/minio-server/minio-server.html#envvar.MINIO_DOMAIN which would enable DNS style requests. I enabled it and tried using the same source-controller Bucket object and it was able to fetch the bucket content again. I guess it's safe to switch to auto. This may only affect the usage of the bucket name as a subdomain but we don't use it that way.
While setting up the minio bucket, I also noticed that my mc - minio CLI, configuration file had all the targets listed in them with path auto (for non-minio but other S3 compatible servers too), similar to the configurations in https://github.com/minio/mc/blob/master/docs/minio-client-configuration-files.md. Which makes me believe that most of the use cases will be covered with auto.

If we find need to configure it in the future, we can add a new field in the Bucket API to make it configurable.
@zhiyu0729 let's go ahead with auto for now. It'd help to provide some context in the code. Can you update

source-controller/pkg/minio/minio.go

Line 43 in 4deb8cf

BucketLookup: minio.BucketLookupPath,

to use auto and also add a comment stating that path lookup is not supported by all the servers, so we let minio client automatically resolve what to use?

[stefanprodan]

@zhiyu0729 please rebase with upstream main and force push.

stale

[zhiyu0729]

I have removed BucketLookup: minio.BucketLookupPath, which will use the default initialization value BucketLookupAuto. I believe that when there are no fields in the API that can be configured, using minio's default initialization value should be enough.

[stefanprodan]

LGTM

Thanks @zhiyu0729

[darkowlzz]

LGTM!
Thanks.

[fluxcdbot]

Successfully created backport PR for release/v1.0.x:

• [release/v1.0.x] bucket: Use auto lookup type #1221

[fluxcdbot]

Successfully created backport PR for release/v1.1.x:

• [release/v1.1.x] bucket: Use auto lookup type #1222