-
Notifications
You must be signed in to change notification settings - Fork 9.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
provider/aws: aws_s3_bucket
acceleration_status not available in china or us-gov
#7999
Conversation
88be3ef
to
5b361ae
Compare
if err != nil { | ||
return err | ||
s3HostedRegion := meta.(*AWSClient).region | ||
if s3HostedRegion != "cn-north-1" && s3HostedRegion != "us-gov-west-1" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard-coding regions means that once AWS actually starts supporting this feature in cn-north-1
& us-gov-west-1
, we'll have to patch it again and users will have to wait for the next release :/
How about silently ignoring UnsupportedArgument
errors instead? i.e. we'd still error out if any other error comes from AWS API, but this error code seems to be unique enough that it shouldn't have any unintended consequences (like hiding actual errors).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@radeksimko something like this?
https://gist.github.com/kwilczynski/287639d36ce7bde585c351b7ce9f30f7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kwilczynski Pretty much, except I would still log full error message. I think we even have a helper function somewhere for matching these AWS errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@radeksimko yes, we have, and with it it would be not necessarily better, as per:
isAWSErr(err, "UnsupportedArgument", "The request contained an unsupported argument")
I mean, this is fine too, although very verbose. In this particular case accessing the "ArgumentName" would be a plus, but sadly this is not exposed via the error interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what do you mean, the error message doesn't contain the name of unsupported argument - that's sad, true 😞
It just depends on the level of reusability of that error code - the isAWSErr
may potentially help us avoiding false positives/negatives when matching the error message. The question is do we want to ignore error messages we haven't seen yet in a context we didn't think about? IMO we don't because we don't know the exact effect of that. That's why more precise match may be helpful.
The official API docs aren't very helpful in this area either. 😞 http://docs.aws.amazon.com/AmazonS3/latest/API/ErrorResponses.html#ErrorCodeList doesn't contain UnsupportedArgument
at all. Instead it contains bunch of 400 Bad Request errors with the exact same code related to transfer acceleration, most of which would probably come from POST
/Create operations.
I don't have a strong opinion about this specific case (UnsupportedArgument
/ aws_s3_bucket
) though, either way (error code match or isAWSErr
) is fine with me.
or us-gov Fixes #7969 `acceleration_status` is not available in China or US-Gov data centers. Even querying for this will give the following: ``` Error refreshing state: 1 error(s) occurred: 2016/08/04 13:58:52 [DEBUG] plugin: waiting for all plugin processes to complete... * aws_s3_bucket.registry_cn: UnsupportedArgument: The request contained * an unsupported argument. status code: 400, request id: F74BA6AA0985B103 ``` We are going to stop any Read calls for acceleration status from these data centers ``` % make testacc TEST=./builtin/providers/aws % TESTARGS='-run=TestAccAWSS3Bucket_' ✹ ==> Checking that code complies with gofmt requirements... go generate $(go list ./... | grep -v /terraform/vendor/) TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSS3Bucket_ -timeout 120m === RUN TestAccAWSS3Bucket_Notification --- PASS: TestAccAWSS3Bucket_Notification (409.46s) === RUN TestAccAWSS3Bucket_NotificationWithoutFilter --- PASS: TestAccAWSS3Bucket_NotificationWithoutFilter (166.84s) === RUN TestAccAWSS3Bucket_basic --- PASS: TestAccAWSS3Bucket_basic (133.48s) === RUN TestAccAWSS3Bucket_acceleration --- PASS: TestAccAWSS3Bucket_acceleration (282.06s) === RUN TestAccAWSS3Bucket_Policy --- PASS: TestAccAWSS3Bucket_Policy (332.14s) === RUN TestAccAWSS3Bucket_UpdateAcl --- PASS: TestAccAWSS3Bucket_UpdateAcl (225.96s) === RUN TestAccAWSS3Bucket_Website_Simple --- PASS: TestAccAWSS3Bucket_Website_Simple (358.15s) === RUN TestAccAWSS3Bucket_WebsiteRedirect --- PASS: TestAccAWSS3Bucket_WebsiteRedirect (380.38s) === RUN TestAccAWSS3Bucket_WebsiteRoutingRules --- PASS: TestAccAWSS3Bucket_WebsiteRoutingRules (258.29s) === RUN TestAccAWSS3Bucket_shouldFailNotFound --- PASS: TestAccAWSS3Bucket_shouldFailNotFound (92.24s) === RUN TestAccAWSS3Bucket_Versioning --- PASS: TestAccAWSS3Bucket_Versioning (654.19s) === RUN TestAccAWSS3Bucket_Cors --- PASS: TestAccAWSS3Bucket_Cors (143.58s) === RUN TestAccAWSS3Bucket_Logging --- PASS: TestAccAWSS3Bucket_Logging (249.79s) === RUN TestAccAWSS3Bucket_Lifecycle --- PASS: TestAccAWSS3Bucket_Lifecycle (259.87s) PASS ok github.com/hashicorp/terraform/builtin/providers/aws 3946.464s ``` thanks to @kwilczynski and @radeksimko for the research on how to handle the generic errors here Running these over a 4G tethering connection has been painful :)
5b361ae
to
a9f24c9
Compare
the changes have been resolved thanks to @radeksimko and @kwilczynski :) |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
Fixes #7969
acceleration_status
is not available in China or US-Gov data centers.Even querying for this will give the following:
We are going to stop any Read calls for acceleration status from these
data centers
Running these over a 4G tethering connection has been painful :)