gardener / etcd-backup-restore

Collection of components to backup and restore the etcd of a Kubernetes cluster.
Apache License 2.0
287 stars 100 forks source link

[Enhancement] Why ECS provider does not support regions and always uses the default region US-Standard ? #726

Closed KubeKyrie closed 6 months ago

KubeKyrie commented 6 months ago

Enhancement (What you would like to be added): Region can be selected by config.

Motivation (Why is this needed?): Region of my minio server is us-east-1, but ECS does not support it. And the error log:

time="2024-04-09T06:57:37Z" level=fatal msg="Failed to create snapshotter: AuthorizationHeaderMalformed: The authorization header is malformed; the region is wrong; expecting 'us-east-1'.\n\tstatus code: 400, request id: 17C48A5558166D26, host id: "

Approach/Hint to the implement solution (optional):

renormalize commented 6 months ago

@KubeKyrie

That certainly is a limitation at the moment with ECS, as can be seen in: https://github.com/gardener/etcd-backup-restore/blob/eba697be8b11f9fa7a82cbc57307cadc82bd29cb/pkg/snapstore/ecs_s3_snapstore.go#L9-L12

The fix is quite easy, as can be seen in: https://github.com/gardener/etcd-backup-restore/blob/eba697be8b11f9fa7a82cbc57307cadc82bd29cb/pkg/snapstore/ecs_s3_snapstore.go#L32-L64 Provider configuration is picked up from environment variables and added to the s3AuthOptions struct. Just as it is done for credentials like accessKeyID, secretAccessKey, the same could be done for region. Read the environment variable for region, and if none is present, a default value could be assigned to region (the value hardcoded till now could be used as the fall back value).

The maintainers currently don't have plans to implement the feature, so PRs are welcome if you wish to see this feature in etcd-backup-restore. It's quite an easy fix.

KubeKyrie commented 6 months ago

Yeah, thanks for your detailed reply. I'd be very happy to fix it when I am free.

ishan16696 commented 6 months ago

Hi @KubeKyrie , The support for Dell ECS was contributed by the community, as seen in this pull request. I'm uncertain why the region was set to US-Standard by the author of the pull request. Given that the pull request was introduced in 2020, it's possible that Dell ECS did not support other regions at that time 🤔 . However, as @renormalize suggested, please feel free to open a new pull request to rectify this. Thank you.

KubeKyrie commented 6 months ago

Hi @KubeKyrie , The support for Dell ECS was contributed by the community, as seen in this pull request. I'm uncertain why the region was set to US-Standard by the author of the pull request. Given that the pull request was introduced in 2020, it's possible that Dell ECS did not support other regions at that time 🤔 . However, as @renormalize suggested, please feel free to open a new pull request to rectify this. Thank you.

@ishan16696 yeah, I also think so. By the way, I'd like to ask whether --storage-provider=S3 is better than ECS for us, for our storage mainly is S3-compatible, such as MinIO. And actually I'm not particularly understand the difference between the storage provider. Both of them can work. Thanks.

renormalize commented 6 months ago

@KubeKyrie If you're using S3 compatible storage providers, like MinIO, --storage-provider=S3 is indeed the best way to do this, and you will also find the same recommendation in the getting started docs. https://github.com/gardener/etcd-backup-restore/blob/eba697be8b11f9fa7a82cbc57307cadc82bd29cb/docs/deployment/getting_started.md?plain=1#L14-L18 Underneath the hood, providers like ECS just use the program flow that is defined for S3 in etcd-backup-restore.

renormalize commented 6 months ago

@ishan16696 would it be a good idea to remove ECS as a storage provider option and delete those files altogether, since ECS is S3 compatible anyway? We could just recommend usage of --storage-provider=S3 for anyone who is currently using ECS.

Dell ECS supports a subset of the S3 API as can be seen in its documentation regarding S3 compatibility, and looking at our code I can see that there's nothing Dell ECS specific that is happening there, all of it is S3 compatible.

KubeKyrie commented 6 months ago

@ishan16696 would it be a good idea to remove ECS as a storage provider option and delete those files altogether, since ECS is S3 compatible anyway? We could just recommend usage of --storage-provider=S3 for anyone who is currently using ECS.

Dell ECS supports a subset of the S3 API as can be seen in its documentation regarding S3 compatibility, and looking at our code I can see that there's nothing Dell ECS specific that is happening there, all of it is S3 compatible.

Good idea!

renormalize commented 6 months ago

@KubeKyrie I'll close the issue if you think your issue has been resolved.

KubeKyrie commented 6 months ago

Sure.

ishan16696 commented 6 months ago

@ishan16696 would it be a good idea to remove ECS as a storage provider option and delete those files altogether, since ECS is S3 compatible anyway? We could just recommend usage of --storage-provider=S3 for anyone who is currently using ECS.

I'm not sure that will be a good idea or not as OCS S3 and ECS S3 are already maintained by community as you can check this PR https://github.com/gardener/etcd-backup-restore/pull/465, so I'm not leaning towards to drop support for them as it's not harming us in anyway.

renormalize commented 6 months ago

Thanks for the clarification @ishan16696.