Add support for WAF packages #475

xaf · 2019-09-20T21:15:50Z

Depends on #474

jacobbednarz · 2019-09-23T01:22:00Z

Please remove the dependency update from this PR, we'll do that separately.

cloudflare/resource_cloudflare_waf_package.go

xaf · 2019-09-23T01:23:23Z

Please remove the dependency update from this PR, we'll do that separately.

I can't as it depends on methods that are only available in the latest cloudflare-go

jacobbednarz · 2019-09-23T01:24:03Z

cloudflare/resource_cloudflare_waf_package.go

+			"sensitivity": {
+				Type:     schema.TypeString,
+				Optional: true,
+				Default:  "high",


Let's remove the defaults here and instead provide validation that the sensitivity is one of the accepted values.

Can't we simply do both? We need to know the default anyway to be able to act properly on the delete action, might as well use it as the default value if not provided.
I will add the Enum of valid values too, but thought it would be something that the API can directly check instead of forcing it here (doing that means any new value added for those in the API in the future will require an update to that object)

This isn't the provided default that a customer gets out of the box so we're making an assumption here to make the default high which comes with trade offs and not something we want to force on people.

If you want to leave this, it needs to be the actual default that comes for a customer.

We need to know the default anyway to be able to act properly on the delete action, might as well use it as the default value if not provided.

We don't have to have delete options if the resource doesn't support them. Once it's in Terraform, it can just be managed for Updates. If this complicates it, just noop the delete with an explanation in the method.

This isn't the provided default that a customer gets out of the box so we're making an assumption here to make the default high which comes with trade offs and not something we want to force on people.

I was just following the API defaults: https://api.cloudflare.com/#waf-rule-packages-edit-firewall-package
If the defaults are different, please tell me what they are so I can change that.

We don't have to have delete options if the resource doesn't support them. Once it's in Terraform, it can just be managed for Updates. If this complicates it, just noop the delete with an explanation in the method.

This would be weak and would be a different behavior than what is used for WAF Rules, which would be very confusing (unless we prefer disabling the delete from WAF Rules too ?). I think it makes more sense to have sort of a "reset to default", but I totally agree that the "reset to default" should reset to the actual default.

jacobbednarz · 2019-09-23T01:24:27Z

cloudflare/resource_cloudflare_waf_package.go

+			"action_mode": {
+				Type:     schema.TypeString,
+				Optional: true,
+				Default:  "challenge",


Similar to https://github.com/terraform-providers/terraform-provider-cloudflare/pull/475/files#r326930670, let's move towards better validation without a default.

cloudflare/resource_cloudflare_waf_package.go

xaf · 2019-09-23T15:21:10Z

Updated to only include those changes, but of course it won't pass the tests for now

xaf · 2019-09-30T22:43:24Z

Rebased on master

xaf · 2019-10-01T20:44:25Z

Fixed small mistakes I made when adding the ValidateFunc

xaf · 2019-10-01T22:55:45Z

Updated the ValidateFunc to use the helper as suggested by @jacobbednarz !

xaf · 2019-10-02T23:00:12Z

@patryk @jacobbednarz is there anything I could do so we can move forward with this PR and the other one for WAF groups ?

xaf · 2019-10-04T20:39:24Z

Added documentation for the change

jacobbednarz · 2019-10-08T03:05:44Z

I've just kicked off the integration tests for these and will report back shortly! Thanks for following through here @xaf!

jacobbednarz · 2019-10-08T03:13:19Z

Looks like there is a failure in the test suite.

------- Stdout: -------
=== RUN   TestAccCloudflareWAFPackage_CreateThenUpdate
=== PAUSE TestAccCloudflareWAFPackage_CreateThenUpdate
=== CONT  TestAccCloudflareWAFPackage_CreateThenUpdate
--- FAIL: TestAccCloudflareWAFPackage_CreateThenUpdate (0.00s)
    testing.go:569: Step 0 error: Missing newline after argument: On /opt/teamcity-agent/temp/buildTmp/tf-test483636842/main.tf line 3: An argument definition must end with a newline.
FAIL

I'm able to replicate this running locally too.

xaf · 2019-10-08T21:30:32Z

I would add it as a part of the PreCheck step for the test.

The problem with that approach is that I need to know the packageID to prepare the config, which is evaluated at the time we're creating the test, so if we grab the actual packageID only when running the tests, we cannot act properly.

I thus went and created a new function that creates a new client specifically to grab the information needed, as you can see in the updated code. Please tell me what you think about that @jacobbednarz and if you have any better way to implement this ?

For now, the acceptance tests are passing:

=== RUN   TestAccCloudflareWAFPackage_CreateThenUpdate
=== PAUSE TestAccCloudflareWAFPackage_CreateThenUpdate
=== CONT  TestAccCloudflareWAFPackage_CreateThenUpdate
2019/10/08 17:26:36 [INFO] Cloudflare Client configured for user:
--- PASS: TestAccCloudflareWAFPackage_CreateThenUpdate (3.65s)
PASS
ok  	github.com/terraform-providers/terraform-provider-cloudflare/cloudflare	3.676s

jacobbednarz · 2019-10-08T21:56:34Z

cloudflare/resource_cloudflare_waf_package_test.go

+
+func testAccGetWAFPackage(zoneID string) (string, error) {
+	config := Config{}
+	if apiToken, ok := os.LookupEnv("CLOUDFLARE_API_TOKEN"); ok {


I don't really like this in here as we're duplicating the logic we already have in the provider setup. If that ever changes, we now have somewhere that depends on it directly. Was there a reason you couldn't use the sharedClient? We're already using that in sweepers and it leverages the existing methods.

If we can't use the shareClient functionality, let's just copy the function and use it here. Don't worry about the logic as we can swap it all when/if we move to API tokens for the test suite.

Another drawback to having this logic here is that testAccPreCheck doesn't check for the API token, only the older method of authenticating which means use of API tokens isn't enforced at all.

Another drawback to having this logic here is that testAccPreCheck doesn't check for the API token, only the older method of authenticating which means use of API tokens isn't enforced at all.

Note that testAccPreCheck is still run just after, so any check it does will be done here, so that should be fine ? But true that we're listing the packages before having done that check...

I don't really like this in here as we're duplicating the logic we already have in the provider setup. If that ever changes, we now have somewhere that depends on it directly. Was there a reason you couldn't use the sharedClient? We're already using that in sweepers and it leverages the existing methods.

sharedClient currently seems to be forcing to use API Key & email instead of API token, is this expected ?

resource_cloudflare_waf_package_test.go:18: Error while listing WAF packages: error from makeRequest: HTTP status 400: content "{\"success\":false,\"errors\":[{\"code\":6003,\"message\":\"Invalid request headers\",\"error_chain\":[{\"code\":6103,\"message\":\"Invalid format for X-Auth-Key header\"}]}],\"messages\":[],\"result\":null}"

sharedClient currently seems to be forcing to use API Key & email instead of API token, is this expected ?

Yep, we'll eventually move this over to API tokens when it's supported fully.

WRT to your error, are you setting the CLOUDFLARE_API_KEY and CLOUDFLARE_EMAIL per the client initialisation when you're running the tests? As you mentioned, the PreCheck usually handles this but because we're running before it, it could be missed.

I did not want to go fetch an API key so I used CLOUDFLARE_API_KEY="blah" for my tests above, which is why I had that error (this is also why I corrected my message after the fact, realizing the error was related to that).

I think my last push (fixup this time, sorry again) should be good to go ? (using sharedClient)

Gotcha, that makes total sense then 😄

jacobbednarz · 2019-10-08T22:03:23Z

@xaf are you able to avoid rebasing until we're ready to merge? It's quite difficult to review the individual changes you're making.

We can squash it when it's ready to merge if you'd prefer that.

xaf · 2019-10-08T22:06:17Z

@xaf are you able to avoid rebasing until we're ready to merge? It's quite difficult to review the individual changes you're making.

Yes, sorry for that... I try to push fixup commits usually but forgot this time!

jacobbednarz · 2019-10-08T22:22:15Z

Great stuff. I've just kicked off the integration suite again and we'll see how we go this time.

xaf · 2019-10-08T22:23:18Z

🤞

jacobbednarz · 2019-10-09T21:01:35Z

Looks like the unit tests are failing to build - https://travis-ci.org/terraform-providers/terraform-provider-cloudflare/jobs/595347342#L234

Integration tests are also failing in a similar fashion.


[09:22:28]
cloudflare/resource_cloudflare_waf_package.go:8:2: cannot find package "github.com/hashicorp/terraform/helper/schema" in any of:
[09:22:28]
	/opt/teamcity-agent/work/962a4ee8807683e3/src/github.com/terraform-providers/terraform-provider-cloudflare/vendor/github.com/hashicorp/terraform/helper/schema (vendor tree)
[09:22:28]
	/opt/goenv/versions/1.12.9/src/github.com/hashicorp/terraform/helper/schema (from $GOROOT)
[09:22:28]
	/opt/teamcity-agent/work/962a4ee8807683e3/src/github.com/hashicorp/terraform/helper/schema (from $GOPATH)
[09:22:28]
cloudflare/resource_cloudflare_waf_package.go:9:2: cannot find package "github.com/hashicorp/terraform/helper/validation" in any of:
[09:22:28]
	/opt/teamcity-agent/work/962a4ee8807683e3/src/github.com/terraform-providers/terraform-provider-cloudflare/vendor/github.com/hashicorp/terraform/helper/validation (vendor tree)
[09:22:28]
	/opt/goenv/versions/1.12.9/src/github.com/hashicorp/terraform/helper/validation (from $GOROOT)
[09:22:28]
	/opt/teamcity-agent/work/962a4ee8807683e3/src/github.com/hashicorp/terraform/helper/validation (from $GOPATH)
[09:22:28]
Process exited with code 1

xaf · 2019-10-09T21:06:42Z

Probably because it's based on a previous version that did not use the same packages in the headers (the path seem to have changed).
Updated with the new paths, but I'm not able to test locally anymore unless I rebase (and thus push -f, so will let things run in here...

jacobbednarz · 2019-10-09T21:09:05Z

Updated with the new paths, but I'm not able to test locally anymore unless I rebase (and thus push -f, so will let things run in here...

I don't understand why this is? Are you able to let me know what issues you're hitting? You should always be able to run the tests locally when developing the provider.

xaf · 2019-10-09T21:11:00Z

Oh, failed again as I forgot to update all of them.

It's just that my local branch is not on top of the current master, and to get on top of the current master I need to rebase.
The errors I get are kind of the opposite really:

==> Checking that code complies with gofmt requirements...
go install -ldflags="-X github.com/terraform-providers/terraform-provider-cloudflare/version.ProviderVersion=v2.0.0-8-g43ed656"
cloudflare/resource_cloudflare_waf_package.go:8:2: cannot find package "github.com/hashicorp/terraform-plugin-sdk/helper/schema" in any of:
/Users/xaf/src/github.com/terraform-providers/terraform-provider-cloudflare/vendor/github.com/hashicorp/terraform-plugin-sdk/helper/schema (vendor tree)
/usr/local/Cellar/go/1.12.4/libexec/src/github.com/hashicorp/terraform-plugin-sdk/helper/schema (from $GOROOT)
/Users/xaf/src/github.com/hashicorp/terraform-plugin-sdk/helper/schema (from $GOPATH)
cloudflare/resource_cloudflare_waf_package.go:9:2: cannot find package "github.com/hashicorp/terraform-plugin-sdk/helper/validation" in any of:
/Users/xaf/src/github.com/terraform-providers/terraform-provider-cloudflare/vendor/github.com/hashicorp/terraform-plugin-sdk/helper/validation (vendor tree)
/usr/local/Cellar/go/1.12.4/libexec/src/github.com/hashicorp/terraform-plugin-sdk/helper/validation (from $GOROOT)
/Users/xaf/src/github.com/hashicorp/terraform-plugin-sdk/helper/validation (from $GOPATH)

jacobbednarz · 2019-10-09T21:16:15Z

It's just that my local branch is not on top of the current master, and to get on top of the current master I need to rebase.

You can still merge in the upstream master and it will have the same effect without needing to rebase or force push changes. We're not fussed if there is merge commits or not so that is up to you.

xaf · 2019-10-09T21:39:49Z

resource_cloudflare_waf_package_test.go:18: invalid credentials: key & email must not be empty
testing.go:462: Acceptance tests skipped unless env 'TF_ACC' set

Ok, might need to manually test for TF_ACC before trying to fetch the package!

xaf · 2019-10-09T21:50:49Z

All checks have passed

🎉

jacobbednarz · 2019-10-09T22:06:44Z

Looks like the integration tests are happy too! I'll take another look over this and we'll get it merged.

xaf · 2019-10-09T22:17:42Z

🎉

Let me know if you want me to rebase -i --autosquash all those fixups.
Once you confirm everything is fine, I'll also push the changes to the "Add support for WAF Groups" one!

jacobbednarz · 2019-10-09T22:23:12Z

website/docs/r/waf_package.html.markdown

+
+* `zone_id` - (Required) The DNS zone ID to apply to.
+* `package_id` - (Required) The WAF Package ID.
+* `sensitivity` - (Required) The sensitivity of the package, can be one of ["high", "medium", "low", "off"].


This documentation doesn't match the schema. Do you intend for this to be optional (to match the schema)? Same with action_mode.

You're right, should read Optional

jacobbednarz · 2019-10-09T22:42:16Z

Thanks for your patience with this! If you can apply the same fixes to #476, we'll get that merged too.

SetWorkersSecret creates or updates a secret DeleteWorkersSecret deletes a secret ListWorkersSecrets lists all secrets

ghost added size/XXL dependencies labels Sep 20, 2019

xaf mentioned this pull request Sep 20, 2019

Add support for WAF groups #476

Merged

jacobbednarz reviewed Sep 23, 2019

View reviewed changes