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

resource/cloudflare_ruleset: add 'ignore' to cache rules query_string #2074

Closed
wants to merge 12 commits into from

Conversation

jafowler
Copy link
Contributor

@jafowler jafowler commented Dec 5, 2022

Add the 'ignore' field to cache rules 'query_string'. This replaces 'exclude = "*"'.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 5, 2022

changelog detected ✅

@jafowler
Copy link
Contributor Author

jafowler commented Dec 5, 2022

depends on cloudflare/cloudflare-go#1140

@jafowler
Copy link
Contributor Author

jafowler commented Dec 5, 2022

I'll need to update the logic to transform this ignore = true/false into exclude = [*]/include = [*]

dependabot bot and others added 2 commits December 6, 2022 09:28
Bumps [github.com/cloudflare/cloudflare-go](https://github.com/cloudflare/cloudflare-go) from 0.55.0 to 0.56.0.
- [Release notes](https://github.com/cloudflare/cloudflare-go/releases)
- [Changelog](https://github.com/cloudflare/cloudflare-go/blob/master/CHANGELOG.md)
- [Commits](cloudflare/cloudflare-go@v0.55.0...v0.56.0)

---
updated-dependencies:
- dependency-name: github.com/cloudflare/cloudflare-go
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
@jafowler
Copy link
Contributor Author

jafowler commented Dec 6, 2022

i cherry picked dependabots bump of cloudflare-go into this PR so i can make the additional changes i needed. We can wait to merge this until after the dependency bump PR goes through.

@jacobbednarz
Copy link
Member

you shouldn't need to cherry pick in version bumps as we do them in a way that once it is merged, you can rebase in master and everything works without weird dependencies between branches.

@jacobbednarz
Copy link
Member

can you please add test coverage for this new schema field and run make docs to generate the updated documentation?

@jafowler
Copy link
Contributor Author

@jacobbednarz I believe this is good to go now.

@jacobbednarz
Copy link
Member

cool! will run the acceptance tests later today and get this merged.

@jacobbednarz
Copy link
Member

running the tests shows that we aren't setting the ignore key correctly

TF_ACC=1 go test $(go list ./...) -v -run "^TestAccCloudflareRuleset_CacheSettings" -count 1 -parallel 1 -timeout 120m -parallel 1
?   	github.com/cloudflare/terraform-provider-cloudflare	[no test files]
=== RUN   TestAccCloudflareRuleset_CacheSettingsFull
=== PAUSE TestAccCloudflareRuleset_CacheSettingsFull
=== RUN   TestAccCloudflareRuleset_CacheSettingsOptionalsEmpty
=== PAUSE TestAccCloudflareRuleset_CacheSettingsOptionalsEmpty
=== RUN   TestAccCloudflareRuleset_CacheSettingsOptionalCacheKeys
=== PAUSE TestAccCloudflareRuleset_CacheSettingsOptionalCacheKeys
=== RUN   TestAccCloudflareRuleset_CacheSettingsQueryStringIgnoreTrue
=== PAUSE TestAccCloudflareRuleset_CacheSettingsQueryStringIgnoreTrue
=== RUN   TestAccCloudflareRuleset_CacheSettingsIgnoreQueryStringFalse
=== PAUSE TestAccCloudflareRuleset_CacheSettingsIgnoreQueryStringFalse
=== CONT  TestAccCloudflareRuleset_CacheSettingsFull
    resource_cloudflare_ruleset_test.go:1551: Step 1/1 error: Check failed: Check 21/38 error: cloudflare_ruleset.jgdfglzysx: Attribute 'rules.0.action_parameters.0.cache_key.0.custom_key.0.query_string.0.ignore' not found
--- FAIL: TestAccCloudflareRuleset_CacheSettingsFull (9.28s)
=== CONT  TestAccCloudflareRuleset_CacheSettingsQueryStringIgnoreTrue
    resource_cloudflare_ruleset_test.go:1698: Step 1/1 error: Check failed: Check 9/9 error: cloudflare_ruleset.zjvajibhgf: Attribute 'rules.0.action_parameters.0.cache_key.0.custom_key.0.query_string.0.ignore' not found
--- FAIL: TestAccCloudflareRuleset_CacheSettingsQueryStringIgnoreTrue (7.64s)
=== CONT  TestAccCloudflareRuleset_CacheSettingsIgnoreQueryStringFalse
    resource_cloudflare_ruleset_test.go:1728: Step 1/1 error: Check failed: Check 9/9 error: cloudflare_ruleset.giltygcddz: Attribute 'rules.0.action_parameters.0.cache_key.0.custom_key.0.query_string.0.ignore' not found
--- FAIL: TestAccCloudflareRuleset_CacheSettingsIgnoreQueryStringFalse (7.03s)
=== CONT  TestAccCloudflareRuleset_CacheSettingsOptionalCacheKeys
--- PASS: TestAccCloudflareRuleset_CacheSettingsOptionalCacheKeys (10.20s)
=== CONT  TestAccCloudflareRuleset_CacheSettingsOptionalsEmpty
--- PASS: TestAccCloudflareRuleset_CacheSettingsOptionalsEmpty (10.55s)

the transformation is missing from https://github.com/cloudflare/terraform-provider-cloudflare/blob/master/internal/provider/resource_cloudflare_ruleset.go#L1078-L1103 but at the same time, we aren't actually setting a key anywhere for rules.0.action_parameters.0.cache_key.0.custom_key.0.query_string.0.ignore, should we be? what is the intended use of this? i'm not seeing this in the API response so a little confused as to what we're intending to happen here.

@jafowler
Copy link
Contributor Author

jafowler commented Dec 14, 2022

Using ignore was what we used to do for cache page rules. Basically anytime include = "*" or exclude = "*" we would return ignore = false or ignore = true.

Right now, the cache ruleset can return a couple of different responses.
ignore all query params

"rules": [
      {
        "id": "f809a6d15f65450ea004efe8d4ea295d",
        "version": "1",
        "action": "set_cache_settings",
        "expression": "(http.host eq \"example.com\")",
        "description": "test",
        "last_updated": "2022-12-14T11:49:22.4216Z",
        "ref": "f809a6d15f65450ea004efe8d4ea295d",
        "enabled": true,
        "action_parameters": {
          "cache": true,
          "cache_key": {
            "custom_key": {
              "query_string": {
                "exclude": "*"
              }
            }
          }
        }
      }
    ],

include all query params

    "rules": [
      {
        "id": "f809a6d15f65450ea004efe8d4ea295d",
        "version": "2",
        "action": "set_cache_settings",
        "expression": "(http.host eq \"example.com")",
        "description": "test",
        "last_updated": "2022-12-14T11:55:25.526402Z",
        "ref": "f809a6d15f65450ea004efe8d4ea295d",
        "enabled": true,
        "action_parameters": {
          "cache": true,
          "cache_key": {
            "custom_key": {
              "query_string": {
                "include": "*"
              }
            }
          }
        }
      }
    ],

The response can also have a list of query strings to include or exclude.
The overarching problem is that the type cannot be both a string and a list. So i had thought that by re-implementing the ignore field in the provider, and allowing cf-terraforming to transform the api response from include = * to ignore = false (and vise versa).

@jafowler
Copy link
Contributor Author

Taking another look at this, i think i'm making this more complicated than it needs to be. I think this could be completely handled in cf-terraforming by transform the response from include = "*" to include =["*"].

@jafowler jafowler closed this Dec 14, 2022
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.

None yet

3 participants