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

Adds implementation for (IP) Rules Lists #507

Merged
merged 12 commits into from
Jul 30, 2020
Merged

Conversation

cehrig
Copy link

@cehrig cehrig commented Jul 26, 2020

This PR adds some new methods for working with IP Lists (https://api.cloudflare.com/#rules-lists-properties).

  • List IP Lists
  • Create IP List
  • Get IP List
  • Update IP List
  • Delete IP List
  • List IP List Items
  • Create IP List Items (single and multi) (is asynchronous)
  • Replace IP List Items (is asynchronous)
  • Delete IP List Items (is asynchronous)
  • Get IP List Item
  • Get Bulk Operation Status

This feature has been introduced as IP Lists (https://blog.cloudflare.com/introducing-ip-lists/) although there will be Country lists or ASN lists in the future. The API calls it Rules Lists. I kept it more specific and re-used the term IP Lists here.

Todo:

  • Write Tests
  • I'm on the fence if we shall keep the asynchronous behavior in the methods mentioned above and handle polling in Terraform. We could also handle polling in cloudflare-go directly.

List IP List Items:
This endpoint works based on cursors, where each cursor returns 25 Items and there's no option to increase 'page size'. This will potentially cause some wait times when working with big IP Lists.

#505

@cehrig
Copy link
Author

cehrig commented Jul 26, 2020

Acceptance Tests have been added

=== RUN   TestListIPLists
--- PASS: TestListIPLists (0.00s)
=== RUN   TestCreateIPList
--- PASS: TestCreateIPList (0.00s)
=== RUN   TestGetIPList
--- PASS: TestGetIPList (0.00s)
=== RUN   TestUpdateIPList
--- PASS: TestUpdateIPList (0.00s)
=== RUN   TestDeleteIPList
--- PASS: TestDeleteIPList (0.00s)
=== RUN   TestListIPListsItems
--- PASS: TestListIPListsItems (0.00s)
=== RUN   TestCreateIPListItems
--- PASS: TestCreateIPListItems (0.00s)
=== RUN   TestReplaceIPListItems
--- PASS: TestReplaceIPListItems (0.00s)
=== RUN   TestDeleteIPListItems
--- PASS: TestDeleteIPListItems (0.00s)
=== RUN   TestGetIPListItem
--- PASS: TestGetIPListItem (0.00s)

Comment on lines +379 to +380
Cursor string `json:"cursor"`
Cursors ResultInfoCursors `json:"cursors"`
Copy link
Member

Choose a reason for hiding this comment

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

Nice! This would be great to see on other API endpoints 😄

@jacobbednarz
Copy link
Member

jacobbednarz commented Jul 26, 2020

I see what you mean about the method names and blanketing the functionality as "IP Lists". For the future planned improvements, will they still be IPs when managing? Or will it be managed as say, the country code or ASN?

I don't like varying from the Cloudflare terminology for features as it creates confusion however I'm also wary of the potential issue we could be introducing by calling this an "IP List" if in fact, it is not a list of IPs. Does something like Firewall Lists or Firewall Rule Lists make more sense as to what this does?

I'm on the fence if we shall keep the asynchronous behaviour in the methods mentioned above and handle polling in Terraform. We could also handle polling in cloudflare-go directly.

In these scenarios, I've lent on putting as much in cloudflare-go as makes sense because Terraform should be a thin wrapper around the APIs focused only on lifecycle management of resources. I'm not saying that's always 100% the case as I can think of a couple of Terraform-isms we need but ideally, cloudflare-go should be able to do most of the heavy lifting and Terraform does as little as possible outside of that realm. I also suspect, if polling is required by the client, it should be possible without Terraform.

@cehrig
Copy link
Author

cehrig commented Jul 27, 2020

Why actually decide between one or the other if we can have both. Create, Replace and Delete IP List Items now have

  • an asynchronous method, returning just an operation_id and
  • a synchronous method, that polls the status and upon completion return the current IP List.
    The algorithm for polling is rather naive and looks like this:
Wait for 1 second
Wait for 1 second
Wait for 2 seconds
Wait for 2 seconds
Wait for 4 seconds
Wait for 4 seconds 
...

Up to a total of 510 seconds.

var i uint8
for i = 0; i < 16; i++ {
	time.Sleep(0x1 << uint8(math.Ceil(float64(i/2))) * time.Second)
	... do stuff ...
}

Regarding the naming, I think continuing with IP Lists is a good approach. We will stick to the announcement from the Blog post. If there will be support for Countries or ASNs in the future, this is data that will be derived from IP Addresses in the end. @prdonahue might have other proposals but I think it's a good compromise and much clearer than something like Firewall Rules Lists.

@jacobbednarz
Copy link
Member

Having both is a great middle ground option; leave it up to the implementation to work out which suits the use case.

I'm 👍 to leaving the feature naming as is for now and we can manage the upcoming changes when they land.

@cehrig cehrig marked this pull request as ready for review July 28, 2020 08:04
@cehrig
Copy link
Author

cehrig commented Jul 28, 2020

Sounds good. I'm opening this for reviews.

Copy link
Member

@jacobbednarz jacobbednarz left a comment

Choose a reason for hiding this comment

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

This is looking pretty great @cehrig! Despite the number of comments (most of them are duplicate suggestions to save you some work if you choose to make the change) I'm really liking this and none of my comments are blockers; just nitpicks to tighten up a few things with existing conventions or readability.

ip_list.go Outdated Show resolved Hide resolved
ip_list.go Outdated Show resolved Hide resolved
ip_list.go Outdated Show resolved Hide resolved
ip_list.go Outdated Show resolved Hide resolved
ip_list.go Outdated Show resolved Hide resolved
ip_list_test.go Outdated Show resolved Hide resolved
ip_list.go Outdated Show resolved Hide resolved
ip_list.go Outdated Show resolved Hide resolved
ip_list.go Show resolved Hide resolved
ip_list.go Outdated Show resolved Hide resolved
cehrig and others added 2 commits July 29, 2020 10:50
@cehrig
Copy link
Author

cehrig commented Jul 29, 2020

I've applied your suggestions. Thanks a bunch @jacobbednarz for taking the time reviewing!

Copy link
Member

@jacobbednarz jacobbednarz left a comment

Choose a reason for hiding this comment

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

Nice work! This is awesome. Looking forward to getting this into Terraform and using it :shipit:

Copy link

@patryk patryk left a comment

Choose a reason for hiding this comment

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

Looks awesome, thanks!

@patryk patryk merged commit 4f8e993 into cloudflare:master Jul 30, 2020
Justin-Holmes pushed a commit to Justin-Holmes/cloudflare-go that referenced this pull request Sep 17, 2020
* Adds implementation for Rules Lists

* Apply suggestions from code review

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

* Returns an error if BulkOperation returns an unexpected status

Co-authored-by: Christian Ehrig <cehrig@cloudflare.com>
Co-authored-by: Jacob Bednarz <jacob.bednarz@gmail.com>
Justin-Holmes pushed a commit to Justin-Holmes/cloudflare-go that referenced this pull request Sep 18, 2020
* Adds implementation for Rules Lists

* Apply suggestions from code review

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

* Returns an error if BulkOperation returns an unexpected status

Co-authored-by: Christian Ehrig <cehrig@cloudflare.com>
Co-authored-by: Jacob Bednarz <jacob.bednarz@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants