yandex-cloud / terraform-provider-yandex

Terraform Yandex provider
https://www.terraform.io/docs/providers/yandex/
Mozilla Public License 2.0
213 stars 116 forks source link

Incorrect error handling when processing storage_bucket with insufficient rights #437

Closed MikailBag closed 4 months ago

MikailBag commented 6 months ago

If storage_bucket is present in configuration on behalf SA with only storage.editor permission (which is suggested in the provider documentation), than subsequent plan / apply operations become extremely slow.

This happens because provider reads bucket policy and retries, among others, authorization errors. But request fails (this is fragment of output I got when running with TF_LOG=debug):

-----------------------------------------------------: @caller=github.com/yandex-cloud/terraform-provider-yandex/yandex/storage_client.go:104 tf_provider_addr=yandex-cloud/yandex tf_req_id=baa2d08d-bb19-d167-2c52-6956daefe703 @module=yandex tf_mux_provider=tf5to6server.v5tov6Server tf_resource_type=yandex_storage_bucket tf_rpc=ReadResource timestamp="2024-05-16T00:02:27.135+0300"
2024-05-16T00:02:27.146+0300 [DEBUG] provider.terraform-provider-yandex_v0.118.0: DEBUG: Response s3/GetBucketPolicy Details:
---[ RESPONSE ]--------------------------------------
HTTP/2.0 403 Forbidden
Content-Type: application/xml; charset=UTF-8
Date: Wed, 15 May 2024 21:02:27 GMT
Server: nginx
X-Amz-Request-Id: 5db324b31f9572e4

And, of course, it fails consistently, because storage.editor does not have permission to call GetBucketPolicy (adding storage.configViewer makes this issue go away).

So request is retried for, AFAICT, one minute. After this minute passes, provider silently proceeds as if it successfully fetched policy and it is empty.

Also note that interrupting plan or apply with Ctrl-C also works slowly, likely because ctx is not propagated by the retryOnAwsCodes(). (UPD: it seems that resource.Retry is deprecated anyway with RetryContext as suggested alternative)

In my opinion, current behavior is very confusing.

As an alternative, a new flag can be added to storage_bucket resource, e.g. skip_policy. When this flag is set to true, provider does not manage bucket policy (e.g. neither reads nor writes it). And when flag is not set, provider does not ignore errors when reading policy. This way both storage.admin and storage.editor roles can be used for managing bucket, and users can't get into unexpected slowdowns.

Also it seems that documentation should be updated to mention both storage.editor and storage.configViewer roles.

opportunity356 commented 4 months ago

Fixed is merged. Will be released in version 0.124.0