CreateBucket fails on non-aws endpoint due to non implemented feature

[tlawrence]

Terraform Version

Run terraform -v to show the version. If you are not running the latest version of Terraform, please upgrade because your issue may have already been fixed.

Terraform version: 0.11.3

Affected Resource(s)

Please list the resources as a list, for example:

• aws_s3_bucket

Terraform template:

https://gist.github.com/tlawrence/9ce8f48af687630ff44029369e733313

Debug Output

Please provider a link to a GitHub Gist containing the complete debug output: https://www.terraform.io/docs/internals/debugging.html. Please do NOT paste the debug output in the issue; just paste a link to the Gist.

Panic Output

If Terraform produced a panic, please provide a link to a GitHub Gist containing the output of the crash.log.

Expected Behavior

Bucket should create

Actual Behavior

GetBucketWebsite returns 501 "NotImplemented" error. AWS-SDK retries 10 times then fails.
501 should be handled gracefully

Steps to Reproduce

1. terraform apply

Important Factoids

Object storage platform is EMC ECS

tried to update the code in aws/resource_aws_s3_bucket.go to catch the error but think it may need catching in the SDK first?

[bflad]

You're likely correct that we'll want this handled upstream in the SDK. Have you opened a bug report with https://github.com/aws/aws-sdk-go/?

[bflad]

Sorry I should clarify. 😅

The retrying of the SDK on 501 NotImplemented, while we might be able to override some of the behavior, we would prefer that the retries were turned off upstream in the SDK.

Within the aws_s3_bucket resource, we might be able to catch that specific error and instead of error'ing out, produce a WARN log message and keep going.

[tlawrence]

Thanks Brian, Do you think you can override this now? I have tried catching the error in a local fork of this project but can't seem to prevent the override. Is there perhaps some request specific overrides I can pass through to the SDK? I haven't raised the issue there yet. Tim

…

On Fri, Mar 2, 2018 at 5:51 PM, Brian Flad ***@***.***> wrote: Sorry I should clarify. 😅 The retrying of the SDK on 501 NotImplemented, while we might be able to override some of the behavior, we would prefer that the retries were turned off upstream in the SDK. Within the aws_s3_bucket resource, we might be able to catch that specific error and instead of error'ing out, produce a WARN log message and keep going. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#3603 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFa6z6_Fnuc6hdaDPQ5uxOblCGwhTqw3ks5taYakgaJpZM4SaKd_> .

[bflad]

We might be able to do something like this in config.go after line 470 to override the SDK retries. Are you able to build and run a custom provider to see if this works in your environment to make it fail without retries?

	# Untested - not recommended for public usage
	client.s3conn.Handlers.Retry.PushBack(func(r *request.Request) {
		if isAWSErr(r.Error, "NotImplemented", "") {
			r.Retryable = aws.Bool(false)
		}
	})

[tlawrence]

SDK isse: aws/aws-sdk-go#1819

[tlawrence]

@bflad thanks, that did prevent the retries. However, I am still not successfuly catching the error.

I have put this into aws/resource_aws_s3_bucket.go

   // Read the website configuration
   wsResponse, err := retryOnAwsCode("NoSuchBucket", func() (interface{}, error) {
     return s3conn.GetBucketWebsite(&s3.GetBucketWebsiteInput{
       Bucket: aws.String(d.Id()),
     })
   })
   ws := wsResponse.(*s3.GetBucketWebsiteOutput)
 
   if err != nil {
     // An S3 Bucket might not have a website configuration set.
     if awsErr, ok := err.(awserr.Error); ok && awsErr.Code() != "NotImplemented" {
       return err
     }
     log.Printf("[WARN] S3 bucket: %s, no website configuration could be found.", d.Id())
   }

but it isn't picked up for some reason..

[bflad]

Out of curiosity, what happens with something like this?

// Untested - not recommended for public usage
gbwi := &s3.GetBucketWebsiteInput{
  Bucket: aws.String(d.Id()),
}
var gbwo *s3.GetBucketWebsiteOutput
err := resource.Retry(1*time.Minute, func() *resource.RetryError {    
  var err error
  gbwo, err = s3conn.GetBucketWebsite(gbwi)
  if err != nil {
    if isAWSErr(err, s3.ErrCodeNoSuchBucket, "") {
      return resource.RetryableError(err)
    }
    return resource.NonRetryableError(err)
  }
  return nil
})
 
if err != nil {
  if !isAWSErr(err, "NotImplemented", "") {
    return err
  }
  log.Printf("[WARN] S3 bucket: %s, no website configuration could be found.", d.Id())
}

[tlawrence]

this worked:

+   if err != nil {
+     // An S3 Bucket might not have a website configuration set. 
+     if awsErr, ok := err.(awserr.Error); ok && awsErr.Code() != "NotImplemented" {
+       return err
+     }
+     return nil
+   }

[tlawrence]

yours is tidier. I'll add that.

Sorry, I'm pretty new to Go code, its pretty obvious now I look at it.

Is the override in config.go still required do you think?
And in terms of a PR, is it ok to leave the override in there (maybe referencing this issue)?

[tlawrence]

slight change,

need to return nil otherwise original error is persisted:

+   if err != nil {
+     if !isAWSErr(err, "NotImplemented", "") {
+       return err
+     }
+     log.Printf("[WARN] S3 bucket: %s, website configuration not supported by storage server.", d.Id())
+     return nil
+   }

[bflad]

Is the override in config.go still required do you think?

If you don't want the SDK retries in Terraform for now, yes. Its just wasting everyone's time (for any S3 API implementation, its extremely unlikely an API will become "implemented" soon after an initial call that fails). I think its a trivial fix for the upstream SDK as-is, but maybe we should wait until they reply on your ticket as they may say the vendor should implement the error differently.

You might want to ensure the acceptance testing passes for all your use cases as I imagine there might be other "problem" code expecting a fully implemented S3 API. (Note this will take awhile unless you pick specific acceptance tests) make testacc TEST=./aws TESTARGS='-run=TestAccAWSS3Bucket_'

[tlawrence]

Thanks for your help @bflad. Do those acceptance tests run live tasks against the storage? What creds do they use?

[bflad]

Do those acceptance tests run live tasks against the storage?

Yes, the tests will create randomly named buckets, configure them, and destroy them to exercise the Terraform code fully against the API. The testing framework does not touch anything it did not create (unless you run a special make command).

What creds do they use?

It should be the same credentials as you're configuring the AWS provider normally if they are environment variables (e.g. I use export AWS_PROFILE=... in my case locally). The acceptance testing provider defaults to us-west-2 region for most testing so you will need to override via AWS_DEFAULT_REGION environment variable if the vendor connection requires something specific. Although now that I think about it, I'm sure your provider connection is probably very specific and might not work easily with the acceptance testing framework.

[tlawrence]

What does the AWS_PROFILE file look like?

we have the following in the Terraform "provider" block:

endpoints {
    s3 = "https://mystorageurl.com"
  }

The Creds and Region are read normally from the env vars.

[bflad]

Configuration profiles are documented here: https://docs.aws.amazon.com/cli/latest/userguide/cli-multiple-profiles.html

The AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY environment variables work fine too, I just use AWS_PROFILE instead of both of those.

I do not think there is a "standard" environment variable for overriding AWS endpoint URLs (or S3's) though. It may be supported in named profile configurations, but I have never tested that myself. AWS doesn't look like it supports that in the AWS CLI yet: aws/aws-cli#1270. If AWS does have endpoint override support via env var/profile, and its not supported in Terraform, we can add support for it.

[ghost]

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!