[WIP] provider/aws elb Add access logs setting (fixes #2315)

[dlsniper]

This PR adds the ability to set the ELB access logs facility for AWS.
AWS documentation: http://docs.aws.amazon.com/ElasticLoadBalancing/latest/DeveloperGuide/enable-access-logs.html

[dlsniper]

Hi,

I'd like to work on this issue but there's too little documentation to be helpful on how to make this work.
Right now I get the following error:

There are warnings and/or errors related to your configuration. Please
fix these before continuing.

Errors:

  * provider.aws: Internal validation of the provider failed! This is always a bug
with the provider itself, and not a user issue. Please report
this bug:

aws_elb: access_logs: Set must be set

I've ran terraform with TF_LOG=1 but I can't see anything in the output nor the error message is clear enough to trace the error back to where this happens in the code so that I have a chance to catch it.

Please advize.

Thank you.

[catsby]

Hey @dlsniper – sorry for the cryptic message there, what it's saying is that the method Set must be set.. for example:

• https://github.com/hashicorp/terraform/blob/master/builtin/providers/aws/resource_aws_network_acl.go#L49
• https://github.com/hashicorp/terraform/blob/master/builtin/providers/aws/resource_aws_elb.go#L124

Schema elements of type TypeSet need a Set method, so Terraform knows how to generate an id for it.

Here's the definition:

    // The following fields are only valid for a TypeSet type.
    //
    // Set defines a function to determine the unique ID of an item so that
    // a proper set can be built.
    Set SchemaSetFunc

From https://github.com/hashicorp/terraform/blob/master/helper/schema/schema.go#L104-L108

[catsby]

This should fix #2315

[catsby]

Reading the documentation, it seems the prefix is optional, in which case a Default of "" may not be the best. Instead, we should make it optional and likely Computed.

[dlsniper]

AWS handles this situation is like this:

• if you only have the bucket name (with or without the creation option) then the logs will be put here in <bucket-name>/AWSLogs/<account-id>/elasticloadbalancing/<region>/<year>/<month>/<day>/
• if you have the bucket name / prefix then the logs will be stored under <bucket-name>/<prefix>/AWSLogs/<account-id>/elasticloadbalancing/<region>/<year>/<month>/<day>/

Now, the issue would be how should Terraform handle this (and how does the AWS sdk does it as well)? If we use the Computed option then the default value should still be "" but if we use an existing prefix, we should make sure that the default parts are removed from the output.

[catsby]

I wasn't very clear in my first post, sorry 😦

If we have a default of "", then Terraform will send "" as the prefix in the struct. Assuming AWS doesn't just flat out reject that, you'll end up with this:

mybucket//AWSLogs/123455

But I suspect it will plain reject it.

The aws-sdk expects nil for "non existent" attributes, which is why the attributes are all pointers:

// Information about the AccessLog attribute.
type AccessLog struct {
        [...]
    // The name of the Amazon S3 bucket where the access logs are stored.
    S3BucketName *string `type:"string"`

    // The logical hierarchy you created for your Amazon S3 bucket, for example
    // my-bucket-prefix/prod. If the prefix is not provided, the log is placed at
    // the root level of the bucket.
    S3BucketPrefix *string `type:"string"`
        [...]
}

I was poorly articulating that for prefix, we need to do the same as I mentioned below, and only add it if it's not ""

[dlsniper]

Thanks for the help, I'll look more into this during the weekend as I have more time then (and I need the feature as well so unless someone fixes it before me, I'm definitely keen into having this done asap).

[catsby]

Thanks for doing the heavy lifting here @dlsniper ! Let us know if you think you'll get to the last mile, or if you need someone else to chip in

[dwradcliffe]

Looking forward to using this! :)

[dlsniper]

@catsby sorry, this one fell a bit from my radar, I'll fix it this upcoming weekend.

[danabr]

I don't know, but maybe it would make sense to implement this as a separate resource, in the same way as the aws_lb_cookie_stickiness_policy. Thoughts?

[dlsniper]

@dadgar this would help reusing the same bucket / prefix but I don't know if this is desirable. What advantages do you see over the current solution?

---

Small update: I've looked more into this and changed things as suggested in the feedback but now I have a different error to handle: panic: interface conversion: interface {} is *elb.AccessLog, not *schema.Set
From what I can tell it's caused by this line: https://github.com/hashicorp/terraform/pull/3062/files#diff-5bd4dc8acacea4f1ba989ac829a90894R470 but I don't know yet how to fix the issue and the stack trace is fairly useless.
@catsby how should one go around doing debugging in the providers? Log statements to syslog? It seems things like panic(fmt.Sprintf("%#v", d.Get("access_logs")) do not work either does simply logging withlog.Printf()orfmt.Printf()`.

Thank you.

[catsby]

Hey @dlsniper sorry for the silence.
log.Print and friends will output logs, but are only visible if you execute things like plan and apply with verbose logging. You can do so by prefixing your commands with TF_LOG=1.

Regarding your panic and *elb.AccessLog, you'll need to convert that slice into a set. You can see an example of this with Security Group Rules:

• https://github.com/hashicorp/terraform/blob/master/builtin/providers/aws/resource_aws_security_group.go#L63

Let me know if you need anything else to continue debugging and working on this.
Thanks!

[tpounds]

I'm pretty sure it's an error for this to be specified more than once.

see: http://docs.aws.amazon.com/ElasticLoadBalancing/latest/APIReference/API_ModifyLoadBalancerAttributes.html

[catsby]

I believe the check here is just to see if we've found access_logs in the configuration. access_logs is a set and technically it's valid to have more than one in the config (from a HCL standpoint), but on AWS side yes it's likely an error. You can see later in the code only logs[0] is used.

[tpounds]

Right. Unfortunately, this isn't great because it will silently discard anything but the first occurrence. IMHO, we should return an error back to the user if there is > 1 block so there isn't any ambiguity.

[tpounds]

I've built upon this pull request to fix some issues I had when I tried to apply the changes as-is to the v0.6.6 release. Please see pr #3708 for more info.

[dlsniper]

I'll close this in favor of #3708. Sorry for being silent so far, there's a lot of things going and this fell of my radar. @tpounds thanks for picking it up.

[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 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.