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

fix: Ruleset override action #1249

Merged
merged 10 commits into from
Oct 14, 2021

Conversation

vences
Copy link
Contributor

@vences vences commented Oct 13, 2021

This add action and enabled for override in action_parameters for Ruleset -> #1239

When I did test, I still have an issue with the enabled, it is not take into consideration if it set-up to false.

resource "cloudflare_ruleset" "zone_level_managed_waf_2" {
  zone_id     = var.cloudflare_zone_id
  name        = "managed WAF"
  description = "managed WAF ruleset description"
  kind        = "zone"
  phase       = "http_request_firewall_managed"

  rules {
    action = "execute"
    action_parameters {
        id = "efb7b8c949ac4650a09736fc376e9aee"
        version = "latest"
        overrides {
          action = "challenge"
          enabled = false
        }
    }
    expression = "true"
    description = "Execute Cloudflare Managed Ruleset on my zone-level phase entry point ruleset"
    enabled = true
  }
}

The API call return

"rules": [
      {
          "id": "add866a850eb47668c0642ae42dedbf6",
          "version": "1",
          "action": "execute",
          "action_parameters": {
              "id": "efb7b8c949ac4650a09736fc376e9aee",
              "version": "latest",
              "overrides": {
                  "action": "challenge"
              }
          },
          "expression": "true",
          "description": "Execute Cloudflare Managed Ruleset on my zone-level phase entry point ruleset",
          "last_updated": "2021-10-13T10:32:36.345925Z",
          "ref": "add866a850eb47668c0642ae42dedbf6",
          "enabled": true
      }
  ]

As you may see the enabled is not pass to the API.

@jacobbednarz if you have any clue where that issue may come from, would like to get your help :)

As it is my first PR any comments/improvement welcome ;)

@vences vences changed the title fix: Ruleset override action #1239 fix: Ruleset override action Oct 13, 2021
Fixing parameters string issue
@vences
Copy link
Contributor Author

vences commented Oct 13, 2021

Acceptance test result

➜  terraform-provider-cloudflare git:(ruleset-override-action) ✗ TF_ACC=1 go test $(go list ./...) -v -run TestAccCloudflareRuleset_ActionParametersOverridesAction
?       github.com/cloudflare/terraform-provider-cloudflare     [no test files]
=== RUN   TestAccCloudflareRuleset_ActionParametersOverridesAction
=== PAUSE TestAccCloudflareRuleset_ActionParametersOverridesAction
=== CONT  TestAccCloudflareRuleset_ActionParametersOverridesAction
--- PASS: TestAccCloudflareRuleset_ActionParametersOverridesAction (7.48s)
PASS
ok      github.com/cloudflare/terraform-provider-cloudflare/cloudflare  7.832s
?       github.com/cloudflare/terraform-provider-cloudflare/version     [no test files]

@jacobbednarz
Copy link
Member

I noted in your cloudflare-go PR that you probably want enabled to a boolean pointer and when it's explicitly set, it will be passed (whether it is true or false). example https://github.com/cloudflare/terraform-provider-cloudflare/pull/1249/files#diff-c3d3a73e9b05d7cea55e6fac7f280de1aa750869b5e2c73e7bb495de70246be7R694

Before I added the action challenge within the test testAccCheckCloudflareRulesetManagedWAFWithIDBasedOverrides and forget to remove it.
@vences
Copy link
Contributor Author

vences commented Oct 13, 2021

Thanks @jacobbednarz it is in fact better with the boolean pointer. I changed the acceptance test accordingly and it passed

➜  terraform-provider-cloudflare git:(ruleset-override-action) ✗ TF_ACC=1 go test $(go list ./...) -v -run TestAccCloudflareRuleset_ActionParametersOverridesAction
?       github.com/cloudflare/terraform-provider-cloudflare     [no test files]
=== RUN   TestAccCloudflareRuleset_ActionParametersOverridesAction
=== PAUSE TestAccCloudflareRuleset_ActionParametersOverridesAction
=== CONT  TestAccCloudflareRuleset_ActionParametersOverridesAction
--- PASS: TestAccCloudflareRuleset_ActionParametersOverridesAction (8.53s)
PASS
ok      github.com/cloudflare/terraform-provider-cloudflare/cloudflare  8.809s
?       github.com/cloudflare/terraform-provider-cloudflare/version     [no test files]

@jacobbednarz
Copy link
Member

jacobbednarz commented Oct 14, 2021

I did push a small change to handle when some phases don't allow empty overrides in e8737e0 however otherwise looks for the acceptance tests (with cloudflare-go shimmed in)!

TF_ACC=1 go test $(go list ./...) -v -run "^TestAccCloudflareRuleset_" -count 1 -parallel 1 -timeout 120m -parallel 1
?   	github.com/cloudflare/terraform-provider-cloudflare	[no test files]
=== RUN   TestAccCloudflareRuleset_WAFBasic
=== PAUSE TestAccCloudflareRuleset_WAFBasic
=== RUN   TestAccCloudflareRuleset_WAFManagedRuleset
=== PAUSE TestAccCloudflareRuleset_WAFManagedRuleset
=== RUN   TestAccCloudflareRuleset_WAFManagedRulesetOWASP
=== PAUSE TestAccCloudflareRuleset_WAFManagedRulesetOWASP
=== RUN   TestAccCloudflareRuleset_WAFManagedRulesetOWASPBlockXSSWithAnomalyOver60
=== PAUSE TestAccCloudflareRuleset_WAFManagedRulesetOWASPBlockXSSWithAnomalyOver60
=== RUN   TestAccCloudflareRuleset_WAFManagedRulesetOWASPOnlyPL1
=== PAUSE TestAccCloudflareRuleset_WAFManagedRulesetOWASPOnlyPL1
=== RUN   TestAccCloudflareRuleset_WAFManagedRulesetDeployMultiple
=== PAUSE TestAccCloudflareRuleset_WAFManagedRulesetDeployMultiple
=== RUN   TestAccCloudflareRuleset_WAFManagedRulesetDeployMultipleWithSkip
=== PAUSE TestAccCloudflareRuleset_WAFManagedRulesetDeployMultipleWithSkip
=== RUN   TestAccCloudflareRuleset_WAFManagedRulesetDeployMultipleWithTopSkipAndLastSkip
=== PAUSE TestAccCloudflareRuleset_WAFManagedRulesetDeployMultipleWithTopSkipAndLastSkip
=== RUN   TestAccCloudflareRuleset_WAFManagedRulesetWithCategoryBasedOverrides
=== PAUSE TestAccCloudflareRuleset_WAFManagedRulesetWithCategoryBasedOverrides
=== RUN   TestAccCloudflareRuleset_WAFManagedRulesetWithIDBasedOverrides
=== PAUSE TestAccCloudflareRuleset_WAFManagedRulesetWithIDBasedOverrides
=== RUN   TestAccCloudflareRuleset_MagicTransitUpdateWithHigherPriority
    provider_test.go:142: Skipping acceptance test as 0da42c8d2132a9ddaf714f9e7c920711 is not configured for Magic Transit
--- SKIP: TestAccCloudflareRuleset_MagicTransitUpdateWithHigherPriority (0.00s)
=== RUN   TestAccCloudflareRuleset_WAFManagedRulesetWithPayloadLogging
=== PAUSE TestAccCloudflareRuleset_WAFManagedRulesetWithPayloadLogging
=== RUN   TestAccCloudflareRuleset_RateLimit
=== PAUSE TestAccCloudflareRuleset_RateLimit
=== RUN   TestAccCloudflareRuleset_TransformationRuleURIPath
=== PAUSE TestAccCloudflareRuleset_TransformationRuleURIPath
=== RUN   TestAccCloudflareRuleset_TransformationRuleURIQuery
=== PAUSE TestAccCloudflareRuleset_TransformationRuleURIQuery
=== RUN   TestAccCloudflareRuleset_TransformationRuleHeaders
=== PAUSE TestAccCloudflareRuleset_TransformationRuleHeaders
=== RUN   TestAccCloudflareRuleset_ActionParametersMultipleSkips
=== PAUSE TestAccCloudflareRuleset_ActionParametersMultipleSkips
=== RUN   TestAccCloudflareRuleset_ActionParametersOverridesAction
=== PAUSE TestAccCloudflareRuleset_ActionParametersOverridesAction
=== RUN   TestAccCloudflareRuleset_ActionParametersHTTPDDoSOverride
=== PAUSE TestAccCloudflareRuleset_ActionParametersHTTPDDoSOverride
=== CONT  TestAccCloudflareRuleset_WAFBasic
--- PASS: TestAccCloudflareRuleset_WAFBasic (12.41s)
=== CONT  TestAccCloudflareRuleset_WAFManagedRulesetWithIDBasedOverrides
--- PASS: TestAccCloudflareRuleset_WAFManagedRulesetWithIDBasedOverrides (11.41s)
=== CONT  TestAccCloudflareRuleset_ActionParametersHTTPDDoSOverride
--- PASS: TestAccCloudflareRuleset_ActionParametersHTTPDDoSOverride (12.66s)
=== CONT  TestAccCloudflareRuleset_ActionParametersOverridesAction
--- PASS: TestAccCloudflareRuleset_ActionParametersOverridesAction (12.84s)
=== CONT  TestAccCloudflareRuleset_ActionParametersMultipleSkips
--- PASS: TestAccCloudflareRuleset_ActionParametersMultipleSkips (12.51s)
=== CONT  TestAccCloudflareRuleset_TransformationRuleHeaders
--- PASS: TestAccCloudflareRuleset_TransformationRuleHeaders (11.89s)
=== CONT  TestAccCloudflareRuleset_TransformationRuleURIQuery
--- PASS: TestAccCloudflareRuleset_TransformationRuleURIQuery (12.59s)
=== CONT  TestAccCloudflareRuleset_TransformationRuleURIPath
--- PASS: TestAccCloudflareRuleset_TransformationRuleURIPath (13.24s)
=== CONT  TestAccCloudflareRuleset_RateLimit
--- PASS: TestAccCloudflareRuleset_RateLimit (12.72s)
=== CONT  TestAccCloudflareRuleset_WAFManagedRulesetWithPayloadLogging
--- PASS: TestAccCloudflareRuleset_WAFManagedRulesetWithPayloadLogging (12.34s)
=== CONT  TestAccCloudflareRuleset_WAFManagedRulesetDeployMultiple
--- PASS: TestAccCloudflareRuleset_WAFManagedRulesetDeployMultiple (12.23s)
=== CONT  TestAccCloudflareRuleset_WAFManagedRulesetWithCategoryBasedOverrides
--- PASS: TestAccCloudflareRuleset_WAFManagedRulesetWithCategoryBasedOverrides (11.83s)
=== CONT  TestAccCloudflareRuleset_WAFManagedRulesetDeployMultipleWithTopSkipAndLastSkip
--- PASS: TestAccCloudflareRuleset_WAFManagedRulesetDeployMultipleWithTopSkipAndLastSkip (11.99s)
=== CONT  TestAccCloudflareRuleset_WAFManagedRulesetDeployMultipleWithSkip
--- PASS: TestAccCloudflareRuleset_WAFManagedRulesetDeployMultipleWithSkip (11.70s)
=== CONT  TestAccCloudflareRuleset_WAFManagedRulesetOWASPOnlyPL1
--- PASS: TestAccCloudflareRuleset_WAFManagedRulesetOWASPOnlyPL1 (16.62s)
=== CONT  TestAccCloudflareRuleset_WAFManagedRulesetOWASP
--- PASS: TestAccCloudflareRuleset_WAFManagedRulesetOWASP (15.24s)
=== CONT  TestAccCloudflareRuleset_WAFManagedRuleset
--- PASS: TestAccCloudflareRuleset_WAFManagedRuleset (13.53s)
=== CONT  TestAccCloudflareRuleset_WAFManagedRulesetOWASPBlockXSSWithAnomalyOver60
--- PASS: TestAccCloudflareRuleset_WAFManagedRulesetOWASPBlockXSSWithAnomalyOver60 (12.07s)
PASS
ok  	github.com/cloudflare/terraform-provider-cloudflare/cloudflare	230.106s
?   	github.com/cloudflare/terraform-provider-cloudflare/version	[no test files]

two last things:

then we're good to merge this once the upstream library changes are ready! unit tests will be 👌 once the cloudflare-go version is bumped.

@vences
Copy link
Contributor Author

vences commented Oct 14, 2021

Done!

@jacobbednarz
Copy link
Member

sorry, merged prematurely before we had a cloudflare-go release. the code is now at #1253 pending the upstream changes.

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.

2 participants