Adds new resource cloudflare_byo_ip_prefix #671

cehrig · 2020-04-28T20:00:39Z

This PR introduces a new resource cloudflare_ip_prefix for managing

IP Prefix descriptions via the description attribute
IP Prefix Dynamic Advertisement Status via the advertisement attribute

The resource needs account_id to be set in the provider configuration.

It uses GetPrefix and GetAdvertisementStatus for reading IP prefix information + UpdatePrefixDescription and UpdateAdvertisementStatus for writing updates from cloudflare-go.

@jacobbednarz I'd appreciate if you could take a look if that's going into the right direction before I proceed. Some remarks:

I'm not using a boolean for the advertisement status, as there's no reliable alternative for GetOK to check if an optional boolean has been set and GetOKExists is marked as deprecated.
Writing a test for updating the advertisement status is going to be complicated since there's a conservative rate-limit on the API

TODO

Update cloudflare-go to the latest version
Write acceptance test for updating the prefix description
Write doc

jacobbednarz

Thanks for raising this PR!

The overall feel of this PR seems OK but there are more notes inline regarding things like naming and using the lifecycle methods appropriately to ensure we're not introducing more issues.

I'm not using a boolean for the advertisement status, as there's no reliable alternative for GetOK to check if an optional boolean has been set and GetOKExists is marked as deprecated.

Have you seen cloudflare_record proxied status? Looks like a similar use case to what you're after here that works.

Writing a test for updating the advertisement status is going to be complicated since there's a conservative rate-limit on the API

Let's give it a go and see what we need to do. I don't really want functionality without tests to make it into the provider as we've been there and it makes maintainer life especially hard. If we hit rate limits, we can chat to Cloudflare about some next steps.

cloudflare/provider.go

cloudflare/resource_cloudflare_ip_prefix.go

cehrig · 2020-04-28T21:17:28Z

I'm not using a boolean for the advertisement status, as there's no reliable alternative for GetOK to check if an optional boolean has been set and GetOKExists is marked as deprecated.

Have you seen cloudflare_record proxied status? Looks like a similar use case to what you're after here that works.

Looking at cloudflare_record, that seems to work fine when a default value is set in the schema. I'm afraid we can't set a default value for the advertisement status. The use-case here is if someone creates the resource locally and just wants to "terraform" the description, not specifying the advertisement attribute.

Writing a test for updating the advertisement status is going to be complicated since there's a conservative rate-limit on the API

Let's give it a go and see what we need to do. I don't really want functionality without tests to make it into the provider as we've been there and it makes maintainer life especially hard. If we hit rate limits, we can chat to Cloudflare about some next steps.

Fully agree. Acceptance-Testing the prefix description is completely fine. The only concern here is testing the advertisement status. I'll check internally if there might be a way.

tarnfeld · 2020-04-30T01:07:06Z

Fully agree. Acceptance-Testing the prefix description is completely fine. The only concern here is testing the advertisement status. I'll check internally if there might be a way.

@cehrig unfortunately there isn't a way we can relax that rate limit without essentially performing a noop if the call is within a certain time frame (15 minutes, for example). Route changes take time to converge and are disruptive to the internet, so we have to be quite conservative about how frequently we allow external parties to change bgp status.

(Note: Yes, we certainly could batch the changes and debounce on our end, but then the user feedback loop and delays may cause confusion. We decided it better to 429)

One recommendation would be to check the latest advertised modified timestamp, and only perform the PATCH request if it is beyond the 15 minute time window. I'm sorry I can't make this any easier for your testing. If it makes you feel better, we have the same issue internally. It's important to test frequently, but we can't disrupt the internet for the sake of our tests... and if you're testing the API but not actually changing BGP -- then it's not really a good test.

Can you mark the test case as Skipped somehow when you reach this condition? Perhaps you can at least highlight this event with t.Skip(msg)

jacobbednarz · 2020-04-30T02:50:42Z

Your testing comments make total sense, thanks @tarnfeld. Given the difficulty in testing this, I think we may just need to rely on manual use for the Terraform side of things and unit tests within cloudflare-go to ensure we're doing everything at that level correct. It's a shame there isn't a staging internet we could try these things on 😂

cehrig · 2020-05-01T10:35:05Z

I have at least added a test for the prefix description. It requires CLOUDFLARE_BYO_IP_PREFIX_ID set to the prefix ID.

cehrig@che-box ~/go/src/github.com/terraform-providers/terraform-provider-cloudflare $ go test -count=1 -v -timeout 120s github.com/terraform-providers/terraform-provider-cloudflare/cloudflare -run TestAccCloudflareBYOIPPrefix
=== RUN   TestAccCloudflareBYOIPPrefix
=== PAUSE TestAccCloudflareBYOIPPrefix
=== CONT  TestAccCloudflareBYOIPPrefix
--- PASS: TestAccCloudflareBYOIPPrefix (10.86s)
PASS
ok      github.com/terraform-providers/terraform-provider-cloudflare/cloudflare 10.874s

…anagement

cloudflare/provider_test.go

cloudflare/resource_cloudflare_byo_ip_prefix_test.go

jacobbednarz · 2020-05-03T23:36:25Z

cloudflare/resource_cloudflare_byo_ip_prefix_test.go

+	return fmt.Sprintf(`
+				resource "cloudflare_byo_ip_prefix" "%[3]s" {
+					prefix_id = "%[1]s"    
+					description = "%[2]s"
+				}`, prefixID, description, name)


Can we format this JSON to not have really big indents?

Updated. I blame it on my formatter's settings :-)

website/docs/r/byo_ip_prefix.html.markdown

Co-authored-by: Jacob Bednarz <jacob.bednarz@gmail.com>

Should the `CLOUDFLARE_BYO_IP_PREFIX_ID` not be set (such as the case with the integration suite) don't attempt to run the BYO IP prefix tests. Instead, let the runner know it is being skipped.

jacobbednarz · 2020-05-04T21:46:27Z

Thanks a bunch for persisting to get this one over the line @cehrig! You've done a great job here ⭐

cehrig · 2020-05-04T21:48:04Z

Thanks a bunch for persisting to get this one over the line @cehrig! You've done a great job here

@jacobbednarz I have to Thank You. I'll keep in mind to run the full test suite in the future ... You rock, Thanks a lot! :-)

Respect context cancellation and deadlines.

Adds new resource cloudflare_ip_prefix

bb39b20

ghost added the size/M label Apr 28, 2020

don't exit

8775186

jacobbednarz reviewed Apr 28, 2020

View reviewed changes

cloudflare/provider.go Outdated Show resolved Hide resolved

cloudflare/resource_cloudflare_ip_prefix.go Outdated Show resolved Hide resolved

cloudflare/resource_cloudflare_ip_prefix.go Outdated Show resolved Hide resolved

Christian Ehrig added 3 commits April 29, 2020 15:08

update on create, then sync with read

2f3f6bc

set resource ID on create, not resetting ID on update

78579ca

remove setId from read

206dbc7

update resource name

62d2deb

cehrig changed the title ~~Adds new resource cloudflare_ip_prefix~~ Adds new resource cloudflare_byo_ip_prefix Apr 30, 2020

ghost added size/L and removed size/M labels May 1, 2020

Christian Ehrig added 2 commits May 1, 2020 12:14

renamed resource file

341fb02

adds test

21adc92

cehrig marked this pull request as ready for review May 1, 2020 11:59

Merge remote-tracking branch 'github/master' into cehrig/ip-address-m…

871f964

…anagement

ghost added the kind/documentation Categorizes issue or PR as related to documentation. label May 1, 2020

adds website documentation

aa9fbf8

jacobbednarz reviewed May 3, 2020

View reviewed changes

cloudflare/provider_test.go Outdated Show resolved Hide resolved

jacobbednarz reviewed May 3, 2020

View reviewed changes

cloudflare/resource_cloudflare_byo_ip_prefix_test.go Outdated Show resolved Hide resolved

jacobbednarz reviewed May 3, 2020

View reviewed changes

website/docs/r/byo_ip_prefix.html.markdown Outdated Show resolved Hide resolved

jacobbednarz reviewed May 3, 2020

View reviewed changes

website/docs/r/byo_ip_prefix.html.markdown Outdated Show resolved Hide resolved

cehrig and others added 4 commits May 4, 2020 10:26

Apply suggestions from code review

163d529

Co-authored-by: Jacob Bednarz <jacob.bednarz@gmail.com>

make fmt

70673d5

remove indentation

639292b

lowercase exceptions messages

bedcfc2

Skip BYO IP prefix tests if they don't have a prefix ID

14b1179

Should the `CLOUDFLARE_BYO_IP_PREFIX_ID` not be set (such as the case with the integration suite) don't attempt to run the BYO IP prefix tests. Instead, let the runner know it is being skipped.

jacobbednarz merged commit 4c10939 into cloudflare:master May 4, 2020

jacobbednarz mentioned this pull request Jan 5, 2021

Add new resource cloudflare_magic_firewall_ruleset #884

Merged

jacobbednarz mentioned this pull request Aug 2, 2021

bump cloudflare-go to v0.20.0 #1146

Merged

boekkooi-lengoo pushed a commit to boekkooi-lengoo/terraform-provider-cloudflare that referenced this pull request Feb 28, 2022

Merge pull request cloudflare#671 from favonia/respect-context-timeout

a6793ca

Respect context cancellation and deadlines.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds new resource cloudflare_byo_ip_prefix #671

Adds new resource cloudflare_byo_ip_prefix #671

cehrig commented Apr 28, 2020 •

edited

Loading

jacobbednarz left a comment

cehrig commented Apr 28, 2020

tarnfeld commented Apr 30, 2020 •

edited

Loading

jacobbednarz commented Apr 30, 2020

cehrig commented May 1, 2020 •

edited

Loading

jacobbednarz May 3, 2020

cehrig May 4, 2020 •

edited

Loading

jacobbednarz commented May 4, 2020

cehrig commented May 4, 2020

Adds new resource cloudflare_byo_ip_prefix #671

Adds new resource cloudflare_byo_ip_prefix #671

Conversation

cehrig commented Apr 28, 2020 • edited Loading

jacobbednarz left a comment

Choose a reason for hiding this comment

cehrig commented Apr 28, 2020

tarnfeld commented Apr 30, 2020 • edited Loading

jacobbednarz commented Apr 30, 2020

cehrig commented May 1, 2020 • edited Loading

jacobbednarz May 3, 2020

Choose a reason for hiding this comment

cehrig May 4, 2020 • edited Loading

Choose a reason for hiding this comment

jacobbednarz commented May 4, 2020

cehrig commented May 4, 2020

cehrig commented Apr 28, 2020 •

edited

Loading

tarnfeld commented Apr 30, 2020 •

edited

Loading

cehrig commented May 1, 2020 •

edited

Loading

cehrig May 4, 2020 •

edited

Loading