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 initial Direct Connect resources #876

Closed
wants to merge 1 commit into from
Closed

Conversation

bodgit
Copy link
Contributor

@bodgit bodgit commented Jun 15, 2017

I've redone the PR started in hashicorp/terraform#12567, (which itself was based on hashicorp/terraform#5212), now that the providers seem to be split out into separate repositories. Pretty much the same except I've now renamed the resource to be:

  • aws_dx_virtual_interface_confirm

which is a lot shorter and so doesn't cause a massive diff in aws/provider.go, (AWS seem to often use "dx" in their literature). I've also made it potentially support both public and private interfaces although I personally only need public ones.

This still needs some way of testing, by design these interfaces are created in one account and then made available to the target account to confirm, and at least in the case of public interfaces have a required verification step by AWS staff so it's not something that can be easily tested directly (and quickly!).

I still believe this needs mocking in some way but I can't figure out how to do this. As I mentioned in my original PR the directconnectiface package has an ability to mock the API and I've seen there's getMockedAwsApiSession() which is used in aws/auth_helpers_test.go but I don't see how I could use either based on how regular AWS resources are tested.

I would really appreciate some help on this if possible.

@jescarri
Copy link

Any update on this? I have a DX and I can do some testing.

@grubernaut grubernaut added the enhancement Requests to existing resources that expand the functionality or scope. label Jul 26, 2017
@veetow
Copy link

veetow commented Sep 1, 2017

I would also like an update on this. I have a DX and can test as well.

@ungureanuvladvictor
Copy link
Contributor

Also interested in this.

@akumria
Copy link

akumria commented Oct 2, 2017

Looking over this, and working with DX Virtual Interfaces:

If I am within the target account, none of the properties of the interface (which I am about to confirm) are modifiable.

Whilst the AWS API wants the virtual_interface_id (unique but not constant), from a user point of view, I would require the virtual_interface_name (unique and constant).

If the virtual interface happens to be re-created (deleted, and then re-provisioned) the virtual_interface_id will change but, in most cases, the virtual_interface_name will not.

It'd probably require an additional look-up to map the virtual_interface_name to the virtual_interface_id the AWS API requires but does make for a better user experience.

@ketzacoatl
Copy link
Contributor

As this is a bit of a niche corner of AWS, I'm going to add my vote for this and acknowledge I can help with testing if that is actually helpful. Looking forward to this getting merged as DX is a key component of a project I'm working on, and I'd love to keep these resources in Terraform :) Thanks!

@radeksimko
Copy link
Member

Hi @bodgit
thank you for sending the PR and sorry to keep you waiting here.

It would be nice to support Direct Connect in Terraform, but I think there are other resources which we should add before jumping to this one.

Looking at the API I think this is the order we'll need to follow:

  1. aws_dx_lag
  2. aws_dx_connection (may require LAG)
  3. aws_dx_public_virtual_interface (requires Connection)
  4. aws_dx_private_virtual_interface (requires Connection)
  5. aws_dx_bgp_peer (requires virtual interface)

In general we always try to keep the schema and resource names aligned with API, where possible and sensible. More importantly we need to be able to test each resource properly in any AWS account (without relying on manually called resources with hard-coded static IDs).

I'm not suggesting you should work on any of the above, but I am saying that it's unlikely this PR will ever be merged without most of the above being done first.

Let me know what you think.

@radeksimko radeksimko added the waiting-response Maintainers are waiting on response from community or contributor. label Oct 16, 2017
@jonathan-deprizio
Copy link

I've also got a DX I can use to test this.

@bodgit
Copy link
Contributor Author

bodgit commented Nov 3, 2017

@radeksimko Thanks for the feedback.

The background for me only creating the one resource is that's all I use; we use a third party transit provider who manage most of the DX resources in their private AWS account, and just export a public interface into our AWS account so I just needed a resource to be able to confirm that association.

I did originally restrict it to just public interfaces (as again, that's all I use) however there does seem very little difference between public and private interfaces so it wasn't unreasonable to create a resource that handled both, however if the convention is to split them to match the API then that's fine, I have no strong opinion either way.

Automated testing is the tricky bit if you're not going to mock any of it; by it's nature I think you need at least two test accounts; one account as a target for another account to be able to expose resources into it. I can see you would probably need most of the additional resources you list (a connection and public interface at least) just to be able to export a public interface into another account. I'm just not sure if you can create DX resources in an account without having actual connectivity from somewhere, i.e. can I just create a DX connection that is effectively wired to /dev/null?

@radeksimko radeksimko added the size/XXL Managed by automation to categorize the size of a PR. label Nov 15, 2017
@johnkeates
Copy link

I'm not sure if someone has tested this, but automated testing of DX has its problems as lined out above. Besides those tests, is there anything else that prevents this merge?

@bflad
Copy link
Member

bflad commented Jun 25, 2018

After much waiting and anticipation, it looks like this PR has been superseded by the following split resources PRs (according to #876 (comment)) which were fully acceptance tested and merged into the provider.

In version 1.25.0 of the AWS provider, releasing later this week, the following new resources will be supported:

A new aws_dx_bgp_peer resource is also in the wings with: #4961

If there is still missing functionality with the new Direct Connect functionality, please do submit new issues and/or pull requests. Thanks!

@ghost
Copy link

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

@ghost ghost locked and limited conversation to collaborators Apr 5, 2020
@breathingdust breathingdust removed the waiting-response Maintainers are waiting on response from community or contributor. label Sep 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Requests to existing resources that expand the functionality or scope. new-resource Introduces a new resource. service/directconnect Issues and PRs that pertain to the directconnect service. size/XXL Managed by automation to categorize the size of a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet