Skip to content
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

Add AWS direct connect virtual interface resources #5212

Closed
wants to merge 11 commits into from

Conversation

mhlias
Copy link

@mhlias mhlias commented Feb 19, 2016

Hello,

This PR is adding support for DirectConnect virtual interfaces as seen in the documentation here: http://docs.aws.amazon.com/directconnect/latest/UserGuide/createhostedvirtualinterface.html

I decided to not create separate resources for public and private virtual interfaces as they only have minor differences at creation time and from there they are described as the same resource by the API. So I am handling them with parameters instead. I am not sure if that is a good way of doing it or not, so any feedback on that would be welcome.

I also have two types of resources. One for interfaces that will be created on the same AWS account where the direct connect connection has been created and one for when the virtual interface will be created for usage on a separate AWS account to the one that has the direct connect connection.
I provide an example in the documentation on how to use the latter case.

I wasn't sure how to handle some metaparameters that are used for internal logic and not directly for the resource itself. Also how to have required parameters or not based on another parameter's value. e.g. interface_type private or public require 2 different things, the former requires a virtual_gateway_id and the latter requires a route_filter_prefixes. I would appreciate some feedback on that.

I added some testing but it is not easy to run as it requires a pre-existing direct connect connection which requires an offline procedure to get it set up. Please provide some feedback on how testing should be for this kind of resource.

Best,

Ilias.

@pixitha
Copy link

pixitha commented Feb 23, 2016

I should be able to help test this as I have an existing direct connect already up and in service.

@mhlias
Copy link
Author

mhlias commented Mar 1, 2016

@pixitha Did you get a chance to test it ?
Please let me know if you have any questions about it.

@williamrhancock
Copy link

I'd be available to assist in testing as well, we have a direct connect in several environment that we could work with.

@mhlias
Copy link
Author

mhlias commented May 18, 2016

@williamrhancock Please check out the documentation included in the PR as it contains examples for usage.
Let me know if you have any questions.

@mcraig88
Copy link

Any update on this? I have a need to connect an existing VGW to our DirectConnect after destroy/apply of our VPC.

@mhlias
Copy link
Author

mhlias commented Jun 15, 2016

@mcraig88 I personally use it without any issues in a custom version. I haven't got back any feedback and it doesn't seem to be a priority at the moment to merge. Please vote for the PR and/or open a feature request mentioning the PR. It should help move things faster.

@mcraig88
Copy link

I actually found another way to accomplish what I needed.
Just needed to set this on the route table.

propagating_vgws = ["${var.direct_connect_vgw}"]

Thank you.

On Wed, Jun 15, 2016 at 3:31 AM, mhlias notifications@github.com wrote:

@mcraig88 https://github.com/mcraig88 I personally use it without any
issues in a custom version. I haven't got back any feedback and it doesn't
seem to be a priority at the moment to merge. Please vote for the PR and/or
open a feature request mentioning the PR. It should help move things faster.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#5212 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AIFybcXTz9SivikV3W4cv2xzAacr24-wks5qL9STgaJpZM4Hd1Gl
.

@spkane
Copy link

spkane commented Sep 2, 2016

+1
I'd love to see this support get added into Terraform. Is there anything specific holding this up?

@mhlias
Copy link
Author

mhlias commented Oct 25, 2016

Rebased but it seems something fails in the tests. Need to get up to date with what has changed.

@mhlias
Copy link
Author

mhlias commented Nov 3, 2016

@stack72 Any comments on the PR please ?

@stack72
Copy link
Contributor

stack72 commented Nov 3, 2016

Hi @mhlias

Will get this on my list to review this week - sorry for the delay!

Paul

@stack72 stack72 self-assigned this Nov 3, 2016
@mhlias
Copy link
Author

mhlias commented Nov 3, 2016

Great thanks! No worries, just found some time myself to rebase and fix some issues.

Copy link
Contributor

@stack72 stack72 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @mhlias

Thanks for the work here - I belive there are some changes needed. I have gone through and left some general styling comments WRT to the Read funcs and deref - please have a look at rolling those across how all resources in the PR are

Apart from that, the only other file that needs updated is:

website/source/layouts/aws.erb

As this will add the links to the DC pages. I suggest a section of it's own

Please can you include an output of the acceptance tests in the fix up?

Thanks

Paul

})

if err != nil {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a chance that someone can delete the VirtualInterface from the AWS Console? If so, this will error out. We should handle the case of 404 and then remove from state if necessary

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, a VirtualInterface can be deleted from the AWS Console.


}

if len(resp.VirtualInterfaces) != 1 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we error on > 1 VirtualInterfaces?

Copy link
Author

@mhlias mhlias Nov 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stack72 Because the DescribeVirtualInterfaces request is done with filter on VirtualInterfaceId and according to SDK documentation: "If a virtual interface ID is included then only a single virtual interface will be returned." which should be unique and return at most 1 VIF. so if we get 0 we didn't find it and if we get more than 1 something is terribly wrong.

}

if len(resp.VirtualInterfaces) != 1 {
return fmt.Errorf("[ERROR] Error finding DirectConnect PrivateVirtualInterface: %s", d.Id())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Misleading error message - we have found a list of VirtualInterfaces

return fmt.Errorf("[ERROR] Error finding DirectConnect PrivateVirtualInterface: %s", d.Id())
}

virtualInterface := resp.VirtualInterfaces[0]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we guarantee that [0] VirtualInterface is the one we want? Shouldn't we range the list here?

virtualInterface := resp.VirtualInterfaces[0]

// Set attributes under the user's control.
d.Set("connection_id", *virtualInterface.ConnectionId)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

d.Set takes care of pointer dereference in a safe way - you shouldn't need to deref before passing in - it's safer without it

return fmt.Errorf("[ERROR] Error finding DirectConnect PrivateVirtualInterface: %s", d.Id())
}

virtualInterface := resp.VirtualInterfaces[0]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we guarantee that the first found is the correct one?

d.Set("vlan", *virtualInterface.Vlan)
d.Set("amazon_address", *virtualInterface.AmazonAddress)
d.Set("customer_address", *virtualInterface.CustomerAddress)
// d.Set("auth_key", *virtualInterface.AuthKey)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we not want to set an auth_key?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't remember why I made that choice back then... it has been a while, but I suspect it was because I was trying to avoid printing the AuthKey. Since any change to a VIF forces a recreate as there is no update method it should not matter, unless I am mistaken and causes an issue with detecting a change in the AuthKey.

"github.com/hashicorp/terraform/terraform"
)

func TestAccAWSDCINTRAVIRTUALINTERFACECONFIRM_basic(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Casing is not consistent - we use CamelCase rather than UPPERCASE :)

var err error
var resp *directconnect.VirtualInterface

if v, ok := d.GetOk("interface_type"); ok && v == "public" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

v.(string)

Asn: aws.Int64(int64(d.Get("asn").(int))),
VirtualInterfaceName: aws.String(d.Get("virtual_interface_name").(string)),
Vlan: aws.Int64(int64(d.Get("vlan").(int))),
RouteFilterPrefixes: []*directconnect.RouteFilterPrefix{},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we pass an empty slice here?

@mhlias
Copy link
Author

mhlias commented Nov 17, 2016

@stack72 I made some changes based on your comments. In some cases I will need some more feedback. Please refer to the review comments replies above.

@stack72
Copy link
Contributor

stack72 commented Nov 18, 2016

Hi @mhlias

ok, 1 last request, please can you standardise the names of your tests?

Right now I have tests like this:

TestAccAWSDCINTRAVIRTUALINTERFACE_

and this

TestAccAwsDcIntraVirtualInterfaceConfirm_basic

Thanks

Paul

@stack72
Copy link
Contributor

stack72 commented Nov 18, 2016

also, FYI:

% make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSDC'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/11/18 13:50:12 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSDC -timeout 120m
=== RUN   TestAccAWSDCINTRAVIRTUALINTERFACE_basic
--- FAIL: TestAccAWSDCINTRAVIRTUALINTERFACE_basic (44.85s)
    testing.go:265: Step 0 error: Error applying: 1 error(s) occurred:

        * aws_dc_virtual_interface.vif: Error creating DirectConnect connection: DirectConnectClientException: ConnectionID dxcon-xyz has an invalid format.
            status code: 400, request id: 46f75aa1-ad85-11e6-8f57-21f8b033b1fe
=== RUN   TestAccAWSDCVIRTUALINTERFACE_basic
--- FAIL: TestAccAWSDCVIRTUALINTERFACE_basic (41.85s)
    testing.go:265: Step 0 error: Error applying: 1 error(s) occurred:

        * aws_dc_virtual_interface.vif: Error creating DirectConnect connection: DirectConnectClientException: ConnectionID dxcon-xyz has an invalid format.
            status code: 400, request id: 5feafd67-ad85-11e6-bb9f-2f120043637d
FAIL
exit status 1
FAIL    github.com/hashicorp/terraform/builtin/providers/aws    86.712s
make: *** [testacc] Error 1

@mhlias
Copy link
Author

mhlias commented Nov 18, 2016

@stack72 I fixed the naming on the tests.

On the acceptance testing it is a bit of a tricky situation. You need a direct connect connection to create VIFs and those are not a resource that can be automated as it requires manual work on the Datacenter to get it up and running. I also no longer have access to a direct connect connection for testing.

I don't know if I could use https://docs.aws.amazon.com/sdk-for-go/api/service/directconnect/directconnectiface/ this to mock it instead.

@mubeta06
Copy link
Contributor

mubeta06 commented Jan 5, 2017

What is the current status on this one? I have an upcoming project which will involve AWS Direct Connect it would be great to use Terraform to manage this resource. What is required to get this one over the line?

@mhlias
Copy link
Author

mhlias commented Jan 6, 2017

@mubeta06 Testing remains as I don't currently have access to non-production-critical directconnect I can't test it and also the direct connect connection is a part that needs to be set up offline so not easy to create/expect one to be there for testing use. I found out the mocking interface provided in the AWS SDK but I didn't manage to find time to mock all those calls + missing sample data to return for success/failure scenarios.

@AndHei
Copy link
Contributor

AndHei commented Feb 10, 2017

@mhlias @mubeta06
I might be able to help with QA/QC/testing. What would be next steps?

@bodgit
Copy link
Contributor

bodgit commented Feb 16, 2017

I have a use case for this whereby a 3rd party we use for DirectConnect services handle most of the heavy lifting and just expose the VIF in our account so I just need to be able to confirm the VIF creation and then use the exposed attributes to create a CGW resource, etc.

@mhlias
Copy link
Author

mhlias commented Feb 16, 2017

@AndHei
Next steps would be another rebase as by now it has more conflicts. Will try to do it over the weekend.
The thing that got this stuck is the difficulty to run test cases as the resource relies on a direct connect connection which is set up offline by AWS and the partner network and client.

It would be great if someone could modify the tests with their direct connect info and run them to get outputs. Right now I don't have access to a direct connect connection for testing so it is impossible for me to do.

@bentterp
Copy link

bentterp commented Mar 8, 2017

We would also very much like to see this. As we will be creating DC's "real soon" and don't have anything in production yet, there will be a window for testing.

Our plans include LACP, VLANs and VIFs in multiple accounts.

@doprdele
Copy link

Is DC support available yet?

@@ -214,6 +214,9 @@ func Provider() terraform.ResourceProvider {
"aws_db_parameter_group": resourceAwsDbParameterGroup(),
"aws_db_security_group": resourceAwsDbSecurityGroup(),
"aws_db_subnet_group": resourceAwsDbSubnetGroup(),
"aws_dc_virtual_interface": resourceAwsDirectConnectVirtualInterface(),
"aws_dc_intra_virtual_interface": resourceAwsDirectConnectIntraVirtualInterface(),
"aws_dc_intra_virtual_interface_confirm": resourceAwsDirectConnectIntraVirtualInterfaceConfirm(),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AWS refer to direct connects as 'dx' is a fair amount of their documentation (see: https://aws.amazon.com/directconnect/faqs/)

Copy link

@akumria akumria left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be great if this used the same shorthand that AWS use themselves (i.e. 'dx' rather than 'dc').

Obviously needs rebase (and, potentially, extracting with the forthcoming 0.10.x changes) and further testing.

})

if err != nil {

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, a VirtualInterface can be deleted from the AWS Console.

@jescarri
Copy link

jescarri commented Jul 17, 2017

Is there any update on this?

I'm planing to create some vifs and I can help to test.

@mhlias
Copy link
Author

mhlias commented Jul 17, 2017 via email

@jescarri
Copy link

I might be able to test it, I have a DC and we are in the process of creating more vpcs /accounts.

@audynespinoza
Copy link

i work with Jesus Carrillo and part of the network team. We're in the process of working on a solution to automate direct connect deployments via terraform. I would love to commit to testing. let me know

@catsby
Copy link
Contributor

catsby commented Jul 26, 2017

Closing in favor of hashicorp/terraform-provider-aws#876 which is against the newly split out provider

@catsby catsby closed this Jul 26, 2017
@iliasbertsimas
Copy link

@catsby The one you mention only has support for a single resource to confirm a VIF and nothing else so it is not a replacement. If you want to move the PR to the new external structure just let me know.

@ghost
Copy link

ghost commented Apr 8, 2020

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.

@ghost ghost locked and limited conversation to collaborators Apr 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.