feat: Add routing protocols resource to terraform provider #436

thogarty · 2023-11-02T06:41:05Z

Add routing protocols resource to provider
Include acceptance tests and docs
Tested manually with all permutations of direct and bgp routing protocol data
Data Source tested manually with both direct and bgp

codecov-commenter · 2023-11-02T07:25:18Z

Codecov Report

Attention: 466 lines in your changes are missing coverage. Please review.

Comparison is base (26b2f3d) 60.11% compared to head (e2d7c1f) 59.72%.
Report is 45 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #436      +/-   ##
==========================================
- Coverage   60.11%   59.72%   -0.40%     
==========================================
  Files          99      102       +3     
  Lines       20045    20768     +723     
==========================================
+ Hits        12051    12404     +353     
- Misses       7691     8063     +372     
+ Partials      303      301       -2

Files	Coverage Δ
equinix/errors.go	`88.88% <ø> (+2.52%)`	⬆️
equinix/fabric_routing_protocols_schema.go	`100.00% <100.00%> (ø)`
equinix/provider.go	`82.24% <100.00%> (+0.10%)`	⬆️
equinix/resource_fabric_routing_protocol.go	`4.34% <0.00%> (+0.07%)`	⬆️
equinix/data_source_fabric_routing_protocols.go	`81.63% <81.63%> (ø)`
equinix/fabric_mapping_helper.go	`0.77% <0.00%> (ø)`
equinix/resource_fabric_routing_protocols.go	`3.55% <3.55%> (ø)`

... and 8 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

.gitignore

docs/data-sources/equinix_fabric_routing_protocols.md

equinix/errors.go

displague · 2023-11-03T19:58:22Z

equinix/resource_fabric_routing_protocols_acc_test.go

+						"equinix_iface_ip": fmt.Sprintf("190.1.1.1/29"),
+					}),
+				),
+				ExpectNonEmptyPlan: true,


It would be helpful to test imports to ensure we aren't drifting in state by executing a refresh using an import of the identifier.

https://developer.hashicorp.com/terraform/plugin/sdkv2/resources/import#resource-acceptance-testing-implementation

Suggested change

ExpectNonEmptyPlan: true,

ExpectNonEmptyPlan: true,

ImportState: true,

ImportStateVerify: true,

Assuming we include Import in the resource schema, which we should be doing for every resource.

Agree, change made.

…cols resource

.gitignore

displague · 2023-11-16T13:32:03Z

docs/resources/equinix_fabric_routing_protocols.md

+}
+
+resource "equinix_fabric_routing_protocols" "routing_protocols" {
+  connection_uuid = "b0eb3892-6404-442b-85c0-4f8fcf53d123"


I have some questions about this resource that will be easier to answer in terms of this documentation example.

Will a single equinix_fabric_routing_protocols explicitly and comprehensively cover all of direct and BGP routing protocols for a specific connection?

If so:

Is it not sufficient to use the connection id as the SetId for this TF resource? Can't we fetch and sync all routes from connection id alone?

Will any routes for this connection not defined in this TF resource be deleted?

should we provide a warning that only a single routing_protocol resource per connection uuid should be active (count != 0) at a time

Multiple direct and BGP routes seem to be supported (can we show that in the example)

Is there a universal documented hard API upper bound on the number of routes? Should this be defined as MaxItems for BGP and direct?

If not:

I wonder if the plural resource name implies otherwise (that it should be a comprehensive resource)

I assume we are doing so to avoid touching routes not defined in TF?

we could surface those routes as smaller resources (one type or setting to differentiate BGP vs direct)

^ we wouldn't be able to use the bulk API endpoint, so perhaps that is the reason we took this path?

or if there is a 1:1 relationship between this resource and the connection, we could make these sub-resources of the connection itself.

there are pros and cons to laying out the resource in a different format. It would be nice to justify the design choice in the PR description.

In either case:

How do we use SetId and ImportId in a way that reflects that there may be multiple Direct or BGP sub-resources?

I think it would be helpful to include tests steps (on the existing tests) that:

add direct and bgp rules from a set

remove direct and bgp rules from a set

juggle (move) direct and BGP rules to another set in the same resource (this is what I'm most interested in)

^ include an import test on this step (I suspect the additional rules would be dropped today)

@displague, only one of Direct and one of BGP are allowed on a connection resource at a time:

https://github.com/equinix-product/interconnection_ic-routing-protocol-management/blob/db8f14a862f7cf9f9deb6283e0d9bc11f119e061/src/main/java/com/equinix/interconnection/fabric/routing/handler/RoutingProtocolHandler.java#L159-L168

This resource is to manage the Single Direct and BGP routing protocol resources under a connection. There will not be more than a single of each. I should add MaxItems = 1 to those set types to reflect that.

This resource is to manage the synchronization between Direct and BGP. Direct cannot be deleted before BGP if both are applied to the connection; an error will be returned if attempted. Additionally, you cannot send a POST for Direct or BGP after they have been created to overwrite; it will return an error saying only one of each type is allowed.

This restriction I believe simplifies your queries. Let me know if that's not the case though.

displague · 2024-02-22T20:40:35Z

Noting the relevance to #362 #571.

I thought this may have already been merged by other PRs before realizing this is for the plural resource and datasource. The singular variations are present today.

thogarty · 2024-06-03T19:49:54Z

Out of date. Closing. Open issue #484 refers to it and if that conversation ever gets going we can revisit this.

Add routing protocols resource to terraform provider

80b2691

thogarty temporarily deployed to internal November 2, 2023 06:41 — with GitHub Actions Inactive

Update id when creation fails

5a4feaa

thogarty temporarily deployed to internal November 2, 2023 07:10 — with GitHub Actions Inactive

displague reviewed Nov 3, 2023

View reviewed changes

.gitignore Show resolved Hide resolved

displague reviewed Nov 3, 2023

View reviewed changes

docs/data-sources/equinix_fabric_routing_protocols.md Outdated Show resolved Hide resolved

displague reviewed Nov 3, 2023

View reviewed changes

equinix/errors.go Outdated Show resolved Hide resolved

displague reviewed Nov 3, 2023

View reviewed changes

Update gateway reference, acceptance test, and docs for routing_proto…

b8d1189

…cols resource

thogarty temporarily deployed to internal November 11, 2023 07:56 — with GitHub Actions Inactive

Update docs to create correctly named github issue

e2d7c1f

thogarty temporarily deployed to internal November 11, 2023 08:15 — with GitHub Actions Inactive

displague reviewed Nov 16, 2023

View reviewed changes

thogarty marked this pull request as draft November 16, 2023 22:11

thogarty mentioned this pull request Dec 11, 2023

[Bug]: Fabric Routing Protocol Resource #484

Open

thogarty added the area/resources/fabric Issues related to Fabric and ECX APIs label Feb 13, 2024

thogarty closed this Jun 3, 2024

ctreatma deleted the CXF-77218-routing-protocol-updates branch August 6, 2024 21:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add routing protocols resource to terraform provider #436

feat: Add routing protocols resource to terraform provider #436

thogarty commented Nov 2, 2023 •

edited

Loading

codecov-commenter commented Nov 2, 2023 •

edited

Loading

displague Nov 3, 2023 •

edited

Loading

displague Nov 3, 2023

thogarty Nov 11, 2023

displague Nov 16, 2023 •

edited

Loading

thogarty Nov 16, 2023

displague commented Feb 22, 2024

thogarty commented Jun 3, 2024

feat: Add routing protocols resource to terraform provider #436

feat: Add routing protocols resource to terraform provider #436

Conversation

thogarty commented Nov 2, 2023 • edited Loading

codecov-commenter commented Nov 2, 2023 • edited Loading

Codecov Report

displague Nov 3, 2023 • edited Loading

Choose a reason for hiding this comment

displague Nov 3, 2023

Choose a reason for hiding this comment

thogarty Nov 11, 2023

Choose a reason for hiding this comment

displague Nov 16, 2023 • edited Loading

Choose a reason for hiding this comment

thogarty Nov 16, 2023

Choose a reason for hiding this comment

displague commented Feb 22, 2024

thogarty commented Jun 3, 2024

thogarty commented Nov 2, 2023 •

edited

Loading

codecov-commenter commented Nov 2, 2023 •

edited

Loading

displague Nov 3, 2023 •

edited

Loading

displague Nov 16, 2023 •

edited

Loading