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.

Sign up for GitHub

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

filter/firewall-rules fix for double API calls #1016

Merged
merged 3 commits into from
Aug 10, 2022
Merged

filter/firewall-rules fix for double API calls #1016

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a3b00d7
filter/firewall-rules Update method signartures & pagination
tamas-jozsa Aug 3, 2022
ff622a4
Add changelog
tamas-jozsa Aug 10, 2022
4f1d602
fix changelog entries
jacobbednarz Aug 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changelog/1016.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
```release-note:enhancement
firewall_rule: fix double endpoint calls & moving towards common method signature
```

```release-note:enhancement
filter: fix double endpoint calls & moving towards common method signature
```
95 changes: 55 additions & 40 deletions filter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,26 @@ type FilterValidationExpressionMessage struct {
Message string `json:"message"`
}

// FilterCreateParams contains required and optional params
// for creating a filter.
type FilterCreateParams struct {
ID string `json:"id,omitempty"`
Expression string `json:"expression"`
Paused bool `json:"paused"`
Description string `json:"description"`
Ref string `json:"ref,omitempty"`
}

// FilterUpdateParams contains required and optional params
// for updating a filter.
type FilterUpdateParams struct {
ID string `json:"id"`
Expression string `json:"expression"`
Paused bool `json:"paused"`
Description string `json:"description"`
Ref string `json:"ref,omitempty"`
}

type FilterListParams struct {
ResultInfo
}
Expand All @@ -84,62 +104,57 @@ func (api *API) Filter(ctx context.Context, rc *ResourceContainer, filterID stri
return filterResponse.Result, nil
}

// Filters returns all filters for a zone.
// Filters returns filters for a zone.
//
// Automatically paginates all results unless `params.PerPage` and `params.Page`
// is set.
//
// API reference: https://developers.cloudflare.com/firewall/api/cf-filters/get/#get-all-filters
func (api *API) Filters(ctx context.Context, rc *ResourceContainer, params FilterListParams) ([]Filter, *ResultInfo, error) {
uri := buildURI(fmt.Sprintf("/zones/%s/filters", rc.Identifier), params)
Copy link
Member

Choose a reason for hiding this comment

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

this won't work for the multiple pages condition as it requires the ResultInfo to have the pagination populated to move the pager.

what is the issue you're trying to solve here? right now, I'm not particularly fussed with the additional HTTP call providing it's not duplicating results in the array. I have an idea to remove it however, it requires making all endpoints use pagination by default which will need some digging internally to ensure we don't break things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've opened a bug describing my findings: #1024

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And I also updated my PR and verified that the changes work as expected.

Copy link
Member

Choose a reason for hiding this comment

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

the List* style methods need to support automatic pagination and if someone passes in their own pagination options. i've pulled this locally and confirmed it's still not fixed for the scenario where we allow manual pagination. below is an example of it not working:

package main

import (
	"context"
	"log"

	"github.com/cloudflare/cloudflare-go"
	"github.com/davecgh/go-spew/spew"
)

func main() {

	api, err := cloudflare.New(os.Getenv("CLOUDFLARE_API_KEY"), os.Getenv("CLOUDFLARE_API_EMAIL"))
	if err != nil {
		log.Fatal(err)
	}
	ctx := context.Background()

	filters, res, _ := api.Filters(ctx, cloudflare.ZoneIdentifier("0da42c8d2132a9ddaf714f9e7c920711"), cloudflare.FilterListParams{ResultInfo: cloudflare.ResultInfo{Page: 2}})

	spew.Dump(len(filters), res) 
    // => filters should be > 0 however it is empty and `res` shows an empty ResultInfo
    // 
	// (*cloudflare.ResultInfo)(0xc0000e82d8)({
	// 	Page: (int) 0,
	// 	PerPage: (int) 0,
	// 	TotalPages: (int) 0,
	// 	Count: (int) 0,
	// 	Total: (int) 0,
	// 	Cursor: (string) "",
	// 	Cursors: (cloudflare.ResultInfoCursors) {
	// 		Before: (string) "",
	// 		After: (string) ""
	// 	}
	// })

Copy link
Contributor Author

@tamas-jozsa tamas-jozsa Aug 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Gotcha, I missed that requirement. I've then introduced an autoPaginate var that I'll set to false in case either Page or PerPage is provided. It looks better know I think.

However, I noticed, that not all PerPage values are supported. For example, if you provide 3, you get

filters.api.unknown_error

Wonder if we could handle this better.

Also, I further updated method signatures as discussed.

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha, I missed that requirement. I've then introduced an autoPaginate var that I'll set to false in case either Page or PerPage is provided. It looks better know I think.

definite improvement however, i'm really not happy with how we are handling pagination as a whole because this is quite verbose to add in all endpoints that need to support it. i'd really prefer if we had a way in the ListParams to explicitly say we only intended to fetch a single result. something we can work on with the experimental client down the track i guess.

Wonder if we could handle this better.

something to raise with the service team; i suspect there are minimums in the per page counts as < 5 (or even 10) don't really make sense for pagination.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with you. We can do better and abstract away pagination logic in a more efficient and reusable manner.

FWIW, you can retrieve a single results with the current setup from any page.

For example, if you want one result from page 2:

fwRules, resultInfo, err := api.Filters(context.Background(), 
  cloudflare.ZoneIdentifier("b1fbb152bbde3bd28919a7f4bdca841f"), 
  cloudflare.FilterListParams{ResultInfo: cloudflare.ResultInfo{PerPage: 1, Page: 2}})

Or 1 result from page 1

fwRules, resultInfo, err := api.Filters(context.Background(), 
  cloudflare.ZoneIdentifier("b1fbb152bbde3bd28919a7f4bdca841f"), 
  cloudflare.FilterListParams{ResultInfo: cloudflare.ResultInfo{PerPage: 1}})

Or everything from page 2

fwRules, resultInfo, err := api.Filters(context.Background(), 
  cloudflare.ZoneIdentifier("b1fbb152bbde3bd28919a7f4bdca841f"), 
  cloudflare.FilterListParams{ResultInfo: cloudflare.ResultInfo{Page: 2}})


res, err := api.makeRequestContext(ctx, http.MethodGet, uri, nil)
if err != nil {
return []Filter{}, &ResultInfo{}, err
}

var filtersResponse FiltersDetailResponse
err = json.Unmarshal(res, &filtersResponse)

if err != nil {
return []Filter{}, &ResultInfo{}, fmt.Errorf("%s: %w", errUnmarshalError, err)
autoPaginate := true
if params.PerPage >= 1 || params.Page >= 1 {
autoPaginate = false
}

if params.PerPage < 1 && params.Page < 1 {
var filters []Filter
if params.PerPage < 1 {
params.PerPage = 50
}
if params.Page < 1 {
params.Page = 1
}

for !params.ResultInfo.Done() {
uri := buildURI(fmt.Sprintf("/zones/%s/filters", rc.Identifier), params)
var filters []Filter
var fResponse FiltersDetailResponse
for {
uri := buildURI(fmt.Sprintf("/zones/%s/filters", rc.Identifier), params)

res, err := api.makeRequestContext(ctx, http.MethodGet, uri, nil)
if err != nil {
return []Filter{}, &ResultInfo{}, err
}
res, err := api.makeRequestContext(ctx, http.MethodGet, uri, nil)
if err != nil {
return []Filter{}, &ResultInfo{}, err
}

err = json.Unmarshal(res, &fResponse)
if err != nil {
return []Filter{}, &ResultInfo{}, fmt.Errorf("failed to unmarshal filters JSON data: %w", err)
}

var fResponse FiltersDetailResponse
err = json.Unmarshal(res, &fResponse)
if err != nil {
return []Filter{}, &ResultInfo{}, fmt.Errorf("failed to unmarshal filters JSON data: %w", err)
}
filters = append(filters, fResponse.Result...)
params.ResultInfo = fResponse.ResultInfo.Next()

filters = append(filters, fResponse.Result...)
params.ResultInfo = fResponse.ResultInfo.Next()
if params.ResultInfo.Done() || !autoPaginate {
break
}
filtersResponse.Result = filters
}

return filtersResponse.Result, &filtersResponse.ResultInfo, nil
return filters, &fResponse.ResultInfo, nil
}

// CreateFilters creates new filters.
//
// API reference: https://developers.cloudflare.com/firewall/api/cf-filters/post/
func (api *API) CreateFilters(ctx context.Context, rc *ResourceContainer, filters []Filter) ([]Filter, error) {
func (api *API) CreateFilters(ctx context.Context, rc *ResourceContainer, params []FilterCreateParams) ([]Filter, error) {
uri := fmt.Sprintf("/zones/%s/filters", rc.Identifier)

res, err := api.makeRequestContext(ctx, http.MethodPost, uri, filters)
res, err := api.makeRequestContext(ctx, http.MethodPost, uri, params)
if err != nil {
return []Filter{}, err
}
Expand All @@ -156,14 +171,14 @@ func (api *API) CreateFilters(ctx context.Context, rc *ResourceContainer, filter
// UpdateFilter updates a single filter.
//
// API reference: https://developers.cloudflare.com/firewall/api/cf-filters/put/#update-a-single-filter
func (api *API) UpdateFilter(ctx context.Context, rc *ResourceContainer, filter Filter) (Filter, error) {
if filter.ID == "" {
func (api *API) UpdateFilter(ctx context.Context, rc *ResourceContainer, params FilterUpdateParams) (Filter, error) {
if params.ID == "" {
return Filter{}, fmt.Errorf("filter ID cannot be empty")
}

uri := fmt.Sprintf("/zones/%s/filters/%s", rc.Identifier, filter.ID)
uri := fmt.Sprintf("/zones/%s/filters/%s", rc.Identifier, params.ID)

res, err := api.makeRequestContext(ctx, http.MethodPut, uri, filter)
res, err := api.makeRequestContext(ctx, http.MethodPut, uri, params)
if err != nil {
return Filter{}, err
}
Expand All @@ -180,16 +195,16 @@ func (api *API) UpdateFilter(ctx context.Context, rc *ResourceContainer, filter
// UpdateFilters updates many filters at once.
//
// API reference: https://developers.cloudflare.com/firewall/api/cf-filters/put/#update-multiple-filters
func (api *API) UpdateFilters(ctx context.Context, rc *ResourceContainer, filters []Filter) ([]Filter, error) {
for _, filter := range filters {
func (api *API) UpdateFilters(ctx context.Context, rc *ResourceContainer, params []FilterUpdateParams) ([]Filter, error) {
for _, filter := range params {
if filter.ID == "" {
return []Filter{}, fmt.Errorf("filter ID cannot be empty")
}
}

uri := fmt.Sprintf("/zones/%s/filters", rc.Identifier)

res, err := api.makeRequestContext(ctx, http.MethodPut, uri, filters)
res, err := api.makeRequestContext(ctx, http.MethodPut, uri, params)
if err != nil {
return []Filter{}, err
}
Expand Down
54 changes: 50 additions & 4 deletions filter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,15 @@ func TestCreateSingleFilter(t *testing.T) {
}

mux.HandleFunc("/zones/d56084adb405e0b7e32c52321bf07be6/filters", handler)
params := []FilterCreateParams{
{
ID: "b7ff25282d394be7b945e23c7106ce8a",
Paused: false,
Description: "Login from office",
Expression: "ip.src eq 127.0.0.1",
},
}

want := []Filter{
{
ID: "b7ff25282d394be7b945e23c7106ce8a",
Expand All @@ -166,7 +175,7 @@ func TestCreateSingleFilter(t *testing.T) {
},
}

actual, err := client.CreateFilters(context.Background(), ZoneIdentifier("d56084adb405e0b7e32c52321bf07be6"), want)
actual, err := client.CreateFilters(context.Background(), ZoneIdentifier("d56084adb405e0b7e32c52321bf07be6"), params)

if assert.NoError(t, err) {
assert.Equal(t, want, actual)
Expand Down Expand Up @@ -203,6 +212,21 @@ func TestCreateMultipleFilters(t *testing.T) {
}

mux.HandleFunc("/zones/d56084adb405e0b7e32c52321bf07be6/filters", handler)
params := []FilterCreateParams{
{
ID: "b7ff25282d394be7b945e23c7106ce8a",
Paused: false,
Description: "Login from office",
Expression: "ip.src eq 127.0.0.1",
},
{
ID: "b7ff25282d394be7b945e23c7106ce8a",
Paused: false,
Description: "Login from second office",
Expression: "ip.src eq 10.0.0.1",
},
}

want := []Filter{
{
ID: "b7ff25282d394be7b945e23c7106ce8a",
Expand All @@ -218,7 +242,7 @@ func TestCreateMultipleFilters(t *testing.T) {
},
}

actual, err := client.CreateFilters(context.Background(), ZoneIdentifier("d56084adb405e0b7e32c52321bf07be6"), want)
actual, err := client.CreateFilters(context.Background(), ZoneIdentifier("d56084adb405e0b7e32c52321bf07be6"), params)

if assert.NoError(t, err) {
assert.Equal(t, want, actual)
Expand Down Expand Up @@ -247,14 +271,21 @@ func TestUpdateSingleFilter(t *testing.T) {
}

mux.HandleFunc("/zones/d56084adb405e0b7e32c52321bf07be6/filters/60ee852f9cbb4802978d15600c7f3110", handler)
params := FilterUpdateParams{
ID: "60ee852f9cbb4802978d15600c7f3110",
Paused: false,
Description: "IP of example.org",
Expression: "ip.src eq 93.184.216.0",
}

want := Filter{
ID: "60ee852f9cbb4802978d15600c7f3110",
Paused: false,
Description: "IP of example.org",
Expression: "ip.src eq 93.184.216.0",
}

actual, err := client.UpdateFilter(context.Background(), ZoneIdentifier("d56084adb405e0b7e32c52321bf07be6"), want)
actual, err := client.UpdateFilter(context.Background(), ZoneIdentifier("d56084adb405e0b7e32c52321bf07be6"), params)

if assert.NoError(t, err) {
assert.Equal(t, want, actual)
Expand Down Expand Up @@ -291,6 +322,21 @@ func TestUpdateMultipleFilters(t *testing.T) {
}

mux.HandleFunc("/zones/d56084adb405e0b7e32c52321bf07be6/filters", handler)
params := []FilterUpdateParams{
{
ID: "60ee852f9cbb4802978d15600c7f3110",
Paused: false,
Description: "IP of example.org",
Expression: "ip.src eq 93.184.216.0",
},
{
ID: "c218c536b2bd406f958f278cf0fa8c0f",
Paused: false,
Description: "IP of example.com",
Expression: "ip.src ne 127.0.0.1",
},
}

want := []Filter{
{
ID: "60ee852f9cbb4802978d15600c7f3110",
Expand All @@ -306,7 +352,7 @@ func TestUpdateMultipleFilters(t *testing.T) {
},
}

actual, err := client.UpdateFilters(context.Background(), ZoneIdentifier("d56084adb405e0b7e32c52321bf07be6"), want)
actual, err := client.UpdateFilters(context.Background(), ZoneIdentifier("d56084adb405e0b7e32c52321bf07be6"), params)

if assert.NoError(t, err) {
assert.Equal(t, want, actual)
Expand Down
Loading