Add the auto_accept option for VpcPeeringConnection

[julienba]

Following the description of http://docs.aws.amazon.com/AWSEC2/latest/CommandLineReference/ApiReference-cmd-AcceptVpcPeeringConnection.html

[catsby]

Can we try re-writing this so that resourceAwsVpcPeeringRead is the only place we're calling d.Set("accept_status") (at least in this context).

As is, a cursory glance shows setting accept_status if auto_accept is false, simply b/c the other set is in another function.

Additionally, we ignore the error returned from resourceVpcPeeringConnectionAccept here.

I'm thinking something like this:

[...]
// in resourceAwsVpcPeeringRead
code := pc.Status.Code
if v, ok := d.GetOk("auto_accept"); ok {
    updatedCode, err := resourceVpcPeeringConnectionAccept(ec2conn, pc, d.Id()); 
    if err!=nil {
        // error handling
    }

  //    set to new Status.code,
    //  or other handling
    code = updatedCode
}

d.Set("accept_status", code)
// remainder of func
[...]

func resourceVpcPeeringConnectionAccept(conn *ec2.EC2, oldPc *ec2.VpcPeeringConnection, id string) (string, error) {
// [...]
// accepts string ID instead of resource 
// returns string code, error
}

Or something to that effect..

[catsby]

Looks good so far, pending some nit-pick and questions

[catsby]

Hey @julienba – thanks for the cleanups!
I recently converted the AWS VPC Peering resource over to aws-sdk-go, so master has been updated (see #1174).

Could you take a shot at rebasing this on master? If it doesn't go cleanly I can try and make a new PR based off of yours here and get it merged.

Thanks again!

[julienba]

I merged it. But I'm not sure of what I'm doing (go noobie here) with aws.StringValue, is that some kind of pointer ?

[catsby]

Yeah, the aws.StringValue is a bit crazy to me too. It may go away, awslabs is refactoring the library.

The merge looks good, thanks!

After further review though, I have to ask why the block of code that calls resourceVpcPeeringConnectionAccept is done in resourceAwsVpcPeeringRead and not resourceAwsVpcPeeringUpdate. If I understand correctly, that func will be called on every Read operation. I think the Read operation potentially changing the remote state would not be good. Can you move that block to the Update func?

Assuming I understand correctly, the flow would then be:

create->update (auto connect if needed) -> read.

[julienba]

You're right, it makes more sense that way.

I doing it for avoiding to read ec2.VPCPeeringConnection one more time as I need it for knowing the pending-acceptance status before eventually calling AcceptVPCPeeringConnection.

Should I duplicate the call of resourceAwsVpcPeeringConnectionStateRefreshFunc inside the Update or inside resourceVpcPeeringConnectionAccept ?

[catsby]

I would make the change and move this idea to the *Update func.

In that function, I would use a resource.StateChangeConf struct and do polling for the target state, like we do in Create. The target for your case would be pending-acceptance.

As is, it looks like if your VPC peering connection isn't in that state on the first run, you never re-attempt resourceVpcPeeringConnectionAccept until the next time we Read, unless I'm missing something ...

[catsby]

Looking into the code more, I noticed that, if without hitting an error, the refresh func will always return a state of ready (the string) instead of the actual state it found, which I think is a bug.

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

The AWS docs aren't terribly clear (from what I saw) as the the actual valid states, so I'm assuming the target state should really be our pending-acceptance, right?

[julienba]

Yes it's pending-acceptance

[julienba]

It's just me or the master is currently broken ?
I've this error:
Error creating VPC: Authorization header or parameters are not formatted correctly.

aws-sdk-go don't use credential in .aws folder anymore

[radeksimko]

aws-sdk-go don't use credential in .aws folder anymore

Keep watching #1049

[julienba]

great !

[catsby]

Hello – sorry for the late catch-up here. Terraform and it's current aws-sdk-go library should pick up an credentials in ~/.aws now. I hope that helps 😄

[mitchellh]

LGTM just needs to be squashed

[mitchellh]

or not squashed, but rebased

[catsby]

Did the rebasing #1809, going with that one. Thanks @julienba !

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