Skip to content

Commit

Permalink
resource/cloudflare_page_rule: Swap cache_key_fields to use TypeSet (…
Browse files Browse the repository at this point in the history 
…#914)

* swap schema to use Set

* resource/cloudflare_page_rule: Swap `cache_key_fields` to use TypeSet

When this resource was introduced, it was done so using `TypeList` for
the inner structure. In Terraform, `TypeList` is used for ordered lists
whereas `TypeSet` is for unordered.

This was working for the most part however the `cache_key_fields` fields
don't place any importance on the order so they are returned from the
API with inconsistent ordering.

To address this issue, we need to swap the representation to be
`TypeSet` and ignore any changes in the ordering when comparing to the
state.

Fixes #890

* resource/cloudflare_page_rule: default to `include = "*"`

Should the resource not have any `include`, `exclude` or `ignore`
attributes, we default to including all query strings in the cache key.

* protect against nil values on the query_string

* update TestCacheKeyFieldsNilValue assertion

* fix cache_key_fields header assertion
  • Loading branch information
@jacobbednarz
jacobbednarz authored Jan 20, 2021
1 parent a0719c9 commit 69ccf5c
Show file tree
Hide file tree
Showing 3 changed files with 221 additions and 29 deletions.
63 changes: 43 additions & 20 deletions cloudflare/resource_cloudflare_page_rule.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -308,15 +308,15 @@ func resourceCloudflarePageRule() *schema.Resource {
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"check_presence": {
Type: schema.TypeList,
Type: schema.TypeSet,
Optional: true,
Computed: true,
Elem: &schema.Schema{
Type: schema.TypeString,
},
},
"include": {
Type: schema.TypeList,
Type: schema.TypeSet,
Optional: true,
Computed: true,
Elem: &schema.Schema{
Expand All @@ -335,23 +335,23 @@ func resourceCloudflarePageRule() *schema.Resource {
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"check_presence": {
Type: schema.TypeList,
Type: schema.TypeSet,
Optional: true,
Computed: true,
Elem: &schema.Schema{
Type: schema.TypeString,
},
},
"exclude": {
Type: schema.TypeList,
Type: schema.TypeSet,
Optional: true,
Computed: true,
Elem: &schema.Schema{
Type: schema.TypeString,
},
},
"include": {
Type: schema.TypeList,
Type: schema.TypeSet,
Optional: true,
Computed: true,
Elem: &schema.Schema{
Expand Down Expand Up @@ -386,15 +386,15 @@ func resourceCloudflarePageRule() *schema.Resource {
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"exclude": {
Type: schema.TypeList,
Type: schema.TypeSet,
Optional: true,
Computed: true,
Elem: &schema.Schema{
Type: schema.TypeString,
},
},
"include": {
Type: schema.TypeList,
Type: schema.TypeSet,
Optional: true,
Computed: true,
Elem: &schema.Schema{
Expand Down Expand Up @@ -745,12 +745,12 @@ func transformFromCloudflarePageRuleAction(pageRuleAction *cloudflare.PageRuleAc
fieldOutput[fieldID] = fieldValue
}

if fieldOutput["exclude"] == "*" {
fieldOutput["exclude"] = []interface{}{}
if itemExistsInSlice(fieldOutput["exclude"], "*") {
fieldOutput["exclude"] = []interface{}{"*"}
fieldOutput["ignore"] = true
}

if fieldOutput["include"] == "*" {
if itemExistsInSlice(fieldOutput["include"], "*") {
fieldOutput["include"] = []interface{}{}
fieldOutput["ignore"] = false
}
Expand Down Expand Up @@ -866,24 +866,47 @@ func transformToCloudflarePageRuleAction(id string, value interface{}, d *schema
for sectionID, sectionValue := range cacheKeyActionSchema[0].(map[string]interface{}) {
sectionOutput := map[string]interface{}{}

if sectionValue.([]interface{})[0] != nil {
switch sectionID {
case "cookie", "header":
for fieldID, fieldValue := range sectionValue.([]interface{})[0].(map[string]interface{}) {
sectionOutput[fieldID] = fieldValue
sectionOutput[fieldID] = fieldValue.(*schema.Set).List()
}
}

if sectionID == "query_string" {
queryKey := "include"
if ignore, ok := sectionOutput["ignore"]; ok && ignore.(bool) {
queryKey = "exclude"
case "query_string":
if sectionValue.([]interface{})[0] != nil {
for fieldID, fieldValue := range sectionValue.([]interface{})[0].(map[string]interface{}) {
switch fieldID {
case "exclude", "include":
if fieldValue.(*schema.Set).Len() > 0 {
sectionOutput[fieldID] = fieldValue.(*schema.Set).List()
}
case "ignore":
sectionOutput[fieldID] = fieldValue
default:
sectionOutput[fieldID] = fieldValue.(*schema.Set).List()
}
}

if sectionOutput["ignore"].(bool) {
sectionOutput["exclude"] = []interface{}{"*"}
}

delete(sectionOutput, "ignore")
}
delete(sectionOutput, "ignore")

exclude, ok1 := sectionOutput["exclude"]
include, ok2 := sectionOutput["include"]

// Ensure that if no `include`, `exclude` or `ignore` attributes are
// set, we default to including all query string parameters in the
// cache key.
if (!ok1 || len(exclude.([]interface{})) == 0) && (!ok2 || len(include.([]interface{})) == 0) {
sectionOutput[queryKey] = "*"
sectionOutput["include"] = []interface{}{"*"}
}

output[sectionID] = sectionOutput
default:
for fieldID, fieldValue := range sectionValue.([]interface{})[0].(map[string]interface{}) {
sectionOutput[fieldID] = fieldValue
}
}

Expand Down
170 changes: 161 additions & 9 deletions cloudflare/resource_cloudflare_page_rule_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (

cloudflare "github.com/cloudflare/cloudflare-go"
"github.com/hashicorp/terraform-plugin-sdk/helper/resource"
"github.com/hashicorp/terraform-plugin-sdk/helper/schema"
"github.com/hashicorp/terraform-plugin-sdk/terraform"
)

Expand Down Expand Up @@ -284,16 +285,47 @@ func TestTranformForwardingURL(t *testing.T) {
func TestCacheKeyFieldsNilValue(t *testing.T) {
pageRuleAction, err := transformToCloudflarePageRuleAction(
"cache_key_fields",
[]interface{}{map[string]interface{}{"cookie": []interface{}{map[string]interface{}{"check_presence": []interface{}{}, "include": []interface{}{"next-i18next"}}}, "header": []interface{}{map[string]interface{}{"check_presence": []interface{}{}, "exclude": []interface{}{}, "include": []interface{}{"x-forwarded-host"}}}, "host": []interface{}{map[string]interface{}{"resolved": false}}, "query_string": []interface{}{interface{}(nil)}, "user": []interface{}{map[string]interface{}{"device_type": true, "geo": true, "lang": true}}}},
[]interface{}{
map[string]interface{}{
"cookie": []interface{}{
map[string]interface{}{
"include": schema.NewSet(schema.HashString, []interface{}{}),
"check_presence": schema.NewSet(schema.HashString, []interface{}{"next-i18next"}),
},
},
"header": []interface{}{
map[string]interface{}{
"check_presence": schema.NewSet(schema.HashString, []interface{}{}),
"exclude": schema.NewSet(schema.HashString, []interface{}{}),
"include": schema.NewSet(schema.HashString, []interface{}{"x-forwarded-host"}),
},
},
"host": []interface{}{
map[string]interface{}{
"resolved": false,
},
},
"query_string": []interface{}{
interface{}(nil),
},
"user": []interface{}{
map[string]interface{}{
"device_type": true,
"geo": true,
"lang": true,
},
},
},
},
nil,
)

if err != nil {
t.Fatalf("Unexpected error transforming page rule action: %s", err)
}

if !reflect.DeepEqual(pageRuleAction.Value.(map[string]interface{})["query_string"], map[string]interface{}{"include": "*"}) {
t.Fatalf("Unexpected transformToCloudflarePageRuleAction result, expected %#v, got %#v", map[string]interface{}{"include": "*"}, pageRuleAction.Value.(map[string]interface{})["query_string"])
if !reflect.DeepEqual(pageRuleAction.Value.(map[string]interface{})["query_string"], map[string]interface{}{"include": []interface{}{"*"}}) {
t.Fatalf("Unexpected transformToCloudflarePageRuleAction result, expected %#v, got %#v", map[string]interface{}{"include": []interface{}{"*"}}, pageRuleAction.Value.(map[string]interface{})["query_string"])
}
}

Expand Down Expand Up @@ -420,12 +452,70 @@ func TestAccCloudflarePageRuleCacheKeyFieldsBasic(t *testing.T) {
Config: testAccCheckCloudflarePageRuleConfigCacheKeyFields(zoneID, target),
Check: resource.ComposeTestCheckFunc(
testAccCheckCloudflarePageRuleExists("cloudflare_page_rule.test", &pageRule),
resource.TestCheckResourceAttr("cloudflare_page_rule.test", "actions.0.cache_key_fields.0.cookie.0.check_presence.0", "cookie_presence"),
resource.TestCheckResourceAttr("cloudflare_page_rule.test", "actions.0.cache_key_fields.0.cookie.0.include.0", "cookie_include"),
resource.TestCheckResourceAttr("cloudflare_page_rule.test", "actions.0.cache_key_fields.0.header.0.check_presence.0", "header_presence"),
resource.TestCheckResourceAttr("cloudflare_page_rule.test", "actions.0.cache_key_fields.0.header.0.include.0", "header_include"),
resource.TestCheckResourceAttr("cloudflare_page_rule.test", "actions.0.cache_key_fields.0.cookie.0.check_presence.#", "1"),
resource.TestCheckResourceAttr("cloudflare_page_rule.test", "actions.0.cache_key_fields.0.cookie.0.include.#", "1"),
resource.TestCheckResourceAttr("cloudflare_page_rule.test", "actions.0.cache_key_fields.0.header.0.check_presence.#", "1"),
resource.TestCheckResourceAttr("cloudflare_page_rule.test", "actions.0.cache_key_fields.0.header.0.include.#", "1"),
resource.TestCheckResourceAttr("cloudflare_page_rule.test", "actions.0.cache_key_fields.0.host.0.resolved", "true"),
resource.TestCheckResourceAttr("cloudflare_page_rule.test", "actions.0.cache_key_fields.0.query_string.0.exclude.0", "qs_exclude"),
resource.TestCheckResourceAttr("cloudflare_page_rule.test", "actions.0.cache_key_fields.0.query_string.0.exclude.#", "1"),
),
},
},
})
}

func TestAccCloudflarePageRuleCacheKeyFieldsIgnoreQueryStringOrdering(t *testing.T) {
var pageRule cloudflare.PageRule
domain := os.Getenv("CLOUDFLARE_DOMAIN")
zoneID := os.Getenv("CLOUDFLARE_ZONE_ID")
rnd := generateRandomResourceName()
pageRuleTarget := fmt.Sprintf("%s.%s", rnd, domain)
resourceName := fmt.Sprintf("cloudflare_page_rule.%s", rnd)

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckCloudflarePageRuleDestroy,
Steps: []resource.TestStep{
{
Config: testAccCheckCloudflarePageRuleConfigCacheKeyFieldsWithUnorderedEntries(zoneID, rnd, pageRuleTarget),
Check: resource.ComposeTestCheckFunc(
testAccCheckCloudflarePageRuleExists(resourceName, &pageRule),
resource.TestCheckResourceAttr(resourceName, "actions.0.cache_key_fields.0.cookie.0.check_presence.#", "1"),
resource.TestCheckResourceAttr(resourceName, "actions.0.cache_key_fields.0.cookie.0.include.#", "1"),
resource.TestCheckResourceAttr(resourceName, "actions.0.cache_key_fields.0.header.0.check_presence.#", "1"),
resource.TestCheckResourceAttr(resourceName, "actions.0.cache_key_fields.0.header.0.include.#", "1"),
resource.TestCheckResourceAttr(resourceName, "actions.0.cache_key_fields.0.host.0.resolved", "true"),
resource.TestCheckResourceAttr(resourceName, "actions.0.cache_key_fields.0.query_string.0.include.#", "7"),
),
},
},
})
}

func TestAccCloudflarePageRuleCacheKeyFieldsExcludeAllQueryString(t *testing.T) {
var pageRule cloudflare.PageRule
domain := os.Getenv("CLOUDFLARE_DOMAIN")
zoneID := os.Getenv("CLOUDFLARE_ZONE_ID")
rnd := generateRandomResourceName()
pageRuleTarget := fmt.Sprintf("%s.%s", rnd, domain)
resourceName := fmt.Sprintf("cloudflare_page_rule.%s", rnd)

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckCloudflarePageRuleDestroy,
Steps: []resource.TestStep{
{
Config: testAccCheckCloudflarePageRuleConfigCacheKeyFieldsIgnoreAllQueryString(zoneID, rnd, pageRuleTarget),
Check: resource.ComposeTestCheckFunc(
testAccCheckCloudflarePageRuleExists(resourceName, &pageRule),
resource.TestCheckResourceAttr(resourceName, "actions.0.cache_key_fields.0.cookie.0.check_presence.#", "1"),
resource.TestCheckResourceAttr(resourceName, "actions.0.cache_key_fields.0.cookie.0.include.#", "1"),
resource.TestCheckResourceAttr(resourceName, "actions.0.cache_key_fields.0.header.0.check_presence.#", "1"),
resource.TestCheckResourceAttr(resourceName, "actions.0.cache_key_fields.0.header.0.include.#", "1"),
resource.TestCheckResourceAttr(resourceName, "actions.0.cache_key_fields.0.host.0.resolved", "true"),
resource.TestCheckResourceAttr(resourceName, "actions.0.cache_key_fields.0.query_string.0.exclude.#", "1"),
),
},
},
Expand All @@ -447,7 +537,7 @@ func TestAccCloudflarePageRuleCacheKeyFields2(t *testing.T) {
Config: testAccCheckCloudflarePageRuleConfigCacheKeyFields2(zoneID, target),
Check: resource.ComposeTestCheckFunc(
testAccCheckCloudflarePageRuleExists("cloudflare_page_rule.test", &pageRule),
resource.TestCheckResourceAttr("cloudflare_page_rule.test", "actions.0.cache_key_fields.0.header.0.exclude.0", "origin"),
resource.TestCheckResourceAttr("cloudflare_page_rule.test", "actions.0.cache_key_fields.0.header.0.exclude.#", "1"),
resource.TestCheckResourceAttr("cloudflare_page_rule.test", "actions.0.cache_key_fields.0.host.0.resolved", "false"),
resource.TestCheckResourceAttr("cloudflare_page_rule.test", "actions.0.cache_key_fields.0.user.0.device_type", "true"),
resource.TestCheckResourceAttr("cloudflare_page_rule.test", "actions.0.cache_key_fields.0.user.0.geo", "true"),
Expand Down Expand Up @@ -788,6 +878,68 @@ resource "cloudflare_page_rule" "test" {
}`, zoneID, target)
}

func testAccCheckCloudflarePageRuleConfigCacheKeyFieldsWithUnorderedEntries(zoneID, rnd, target string) string {
return fmt.Sprintf(`
resource "cloudflare_page_rule" "%[3]s" {
zone_id = "%[1]s"
target = "%[3]s"
actions {
cache_key_fields {
cookie {
check_presence = ["cookie_presence"]
include = ["cookie_include"]
}
header {
check_presence = ["header_presence"]
include = ["header_include"]
}
host {
resolved = true
}
query_string {
include = [
"test.anothertest",
"test.regiontest",
"test.devicetest",
"test.testthis",
"test.hello",
"test.segmenttest",
"test.usertype"
]
}
user {}
}
}
}`, zoneID, target, rnd)
}

func testAccCheckCloudflarePageRuleConfigCacheKeyFieldsIgnoreAllQueryString(zoneID, rnd, target string) string {
return fmt.Sprintf(`
resource "cloudflare_page_rule" "%[3]s" {
zone_id = "%[1]s"
target = "%[3]s"
actions {
cache_key_fields {
cookie {
check_presence = ["cookie_presence"]
include = ["cookie_include"]
}
header {
check_presence = ["header_presence"]
include = ["header_include"]
}
host {
resolved = true
}
query_string {
ignore = true
}
user {}
}
}
}`, zoneID, target, rnd)
}

func testAccCheckCloudflarePageRuleConfigCacheKeyFields2(zoneID, target string) string {
return fmt.Sprintf(`
resource "cloudflare_page_rule" "test" {
Expand Down
17 changes: 17 additions & 0 deletions cloudflare/utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"encoding/json"
"fmt"
"log"
"reflect"
"regexp"
"sort"
"strings"
Expand Down Expand Up @@ -83,6 +84,22 @@ func contains(slice []string, item string) bool {
return ok
}

func itemExistsInSlice(slice interface{}, item interface{}) bool {
s := reflect.ValueOf(slice)

if s.Kind() != reflect.Slice {
panic("Invalid data-type")
}

for i := 0; i < s.Len(); i++ {
if s.Index(i).Interface() == item {
return true
}
}

return false
}

// findIndex returns the smallest index i at which x == a[i],
// or (0, false) if there is no such index.
func findIndex(a []interface{}, x interface{}) (int, bool) {
Expand Down

0 comments on commit 69ccf5c

Please sign in to comment.