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

SDK-80: Zone to zone_id refactoring #472

Merged
merged 16 commits into from
Sep 24, 2019
Merged

SDK-80: Zone to zone_id refactoring #472

merged 16 commits into from
Sep 24, 2019

Conversation

patryk
Copy link
Contributor

@patryk patryk commented Sep 20, 2019

Closes #470

@ghost ghost added size/XXL kind/documentation Categorizes issue or PR as related to documentation. labels Sep 20, 2019
@patryk patryk requested a review from jacobbednarz September 20, 2019 16:58
@jacobbednarz
Copy link
Member

I’m about to fly back home so I’ll take a look at this first thing Monday and do some testing with our internal setup.

Gets the build green by updating all tests to match the expected
behaviour of using zone ID instead of relying on zone from the
schema.
@jacobbednarz
Copy link
Member

The integration suite is now pretty green. Just waiting on CLOUDFLARE_ALT_ZONE_ID to be added to clean up the last of the issues.

@patryk
Copy link
Contributor Author

patryk commented Sep 23, 2019

Great, after we merge this one, I'll start working on Workers (sic!), need to branch from v2.0 to avoid merge conflicts. Also, we need to write migration guide.

@jacobbednarz jacobbednarz force-pushed the zone-to-zone_id branch 2 times, most recently from e169ee8 to b0c4476 Compare September 23, 2019 23:14
@patryk patryk requested a review from zackproser September 23, 2019 23:28
The Workers service does not currently support the use of API tokens
(which is used by default when the `CLOUDFLARE_API_TOKEN` environment
variable is set[1]) and throws a very misleading exception.

```
=== RUN   TestAccCloudflareWorkerScript_SingleScriptNonEnt
--- FAIL: TestAccCloudflareWorkerScript_SingleScriptNonEnt (0.59s)
    testing.go:569: Step 0 error: errors during apply:

        Error: script already exists.

          on /var/folders/d4/5sgps61s2jg8f0_71663vw800000gn/T/tf-test617106443/main.tf line 2:
          (source code not available)
```

The reason we hit this exception is due to the `WorkerScript.Script`
being populated with the API error instead of being empty which falls
into this `if` block[2].

```
(cloudflare.WorkerScript) {
 WorkerMetaData: (cloudflare.WorkerMetaData) {
  ID: (string) "",
  ETAG: (string) "",
  Size: (int) 0,
  CreatedOn: (time.Time) 0001-01-01 00:00:00 +0000 UTC,
  ModifiedOn: (time.Time) 0001-01-01 00:00:00 +0000 UTC
 },
 Script: (string) (len=105) "{\"success\":false,\"errors\":[{\"code\":10000,\"message\":\"API Tokens are not supported by this API for now\"}]}\n"
}
```

This was initially puzzling as `err` is handled in `cloudflare-go` where
this field is defined[3] but looking deeper, the HTTP response is coming
back as a 200 OK response, hence the `Script` value being populated
and not falling into the `err` handling.

To fix this, it is two fold. The first is unsetting the
`CLOUDFLARE_API_TOKEN` environment variable in CI. This gets the tests
passing again however to address the underlying issue, we've contacted
Cloudflare to fix the HTTP response code which will then made everything
in the library work as intended. Depending on the length of time before
the fix is deployed, we might also hotfix the library to instead check
`r.Success = true` as well to fail in the correct spot instead of
allowing this to bubble up in unexpected ways.

[1]: https://git.io/Jes9l
[2]: https://git.io/Jes9A
[3]: https://git.io/Jes9h
@patryk patryk merged commit 831e840 into v2.0 Sep 24, 2019
@patryk
Copy link
Contributor Author

patryk commented Sep 24, 2019

Thanks all!

@patryk patryk deleted the zone-to-zone_id branch September 24, 2019 14:29
patryk pushed a commit that referenced this pull request Sep 30, 2019
* Update to cloudflare-go 0.10.0

Pulls in the deprecation of `Organization` in favour of `Account`.

Closes #227

* add go-version file

* newer go version

* Last org reference

* Rename "key" to "token"

API "token" is currently used for HTTP authentication requests where the
`X-Auth-Key` needs to be set. Until recently, it was fine to use the
wrong term for this authentication method (as there was only the one)
however there is now a feature from Cloudflare which is actually called
API tokens and it requires a different authentication procedure.

The functionality for API tokens has been released (v1.18.0) and this
cleans up the old references from "token" to "key" to better reflect
it's purpose.

* Remove use_account_from_zone for v2.0 refactor (#468)

* SDK-80: Zone to zone_id refactoring (#472)

* Refactor zone_id for resource_cloudflare_access_rule

* Refactor zone_id for cloudflare_filter

* Refactor zone_id for cloudflare_firewall_rule

* Refactor zone_id for cloudflare_load_balancer

* Refactor zone_id for cloudflare_page_rule

* Refactor zone_id for cloudflare_rate_limit

* Refactor zone_id for cloudflare_record

* Refactor zone_id for cloudflare_waf_rule

* Refactor zone_id for cloudflare_worker_route

* Refactor zone_id for cloudflare_worker_script

* Refactor zone_id for cloudflare_zone_lockdown

* Refactor zone_id for cloudflare_zone_settings_override

* Make tests `zone_id` compatible

Gets the build green by updating all tests to match the expected
behaviour of using zone ID instead of relying on zone from the
schema.

* Get rid of additional test output

* Add CLOUDFLARE_ALT_ZONE_ID to precheck

* Don't use API token authentication for Workers

The Workers service does not currently support the use of API tokens
(which is used by default when the `CLOUDFLARE_API_TOKEN` environment
variable is set[1]) and throws a very misleading exception.

```
=== RUN   TestAccCloudflareWorkerScript_SingleScriptNonEnt
--- FAIL: TestAccCloudflareWorkerScript_SingleScriptNonEnt (0.59s)
    testing.go:569: Step 0 error: errors during apply:

        Error: script already exists.

          on /var/folders/d4/5sgps61s2jg8f0_71663vw800000gn/T/tf-test617106443/main.tf line 2:
          (source code not available)
```

The reason we hit this exception is due to the `WorkerScript.Script`
being populated with the API error instead of being empty which falls
into this `if` block[2].

```
(cloudflare.WorkerScript) {
 WorkerMetaData: (cloudflare.WorkerMetaData) {
  ID: (string) "",
  ETAG: (string) "",
  Size: (int) 0,
  CreatedOn: (time.Time) 0001-01-01 00:00:00 +0000 UTC,
  ModifiedOn: (time.Time) 0001-01-01 00:00:00 +0000 UTC
 },
 Script: (string) (len=105) "{\"success\":false,\"errors\":[{\"code\":10000,\"message\":\"API Tokens are not supported by this API for now\"}]}\n"
}
```

This was initially puzzling as `err` is handled in `cloudflare-go` where
this field is defined[3] but looking deeper, the HTTP response is coming
back as a 200 OK response, hence the `Script` value being populated
and not falling into the `err` handling.

To fix this, it is two fold. The first is unsetting the
`CLOUDFLARE_API_TOKEN` environment variable in CI. This gets the tests
passing again however to address the underlying issue, we've contacted
Cloudflare to fix the HTTP response code which will then made everything
in the library work as intended. Depending on the length of time before
the fix is deployed, we might also hotfix the library to instead check
`r.Success = true` as well to fail in the correct spot instead of
allowing this to bubble up in unexpected ways.

[1]: https://git.io/Jes9l
[2]: https://git.io/Jes9A
[3]: https://git.io/Jes9h

* SDK-81: Refactor Workers resources to remove single-script code. (#478)

* SDK-81: Refactor Workers resources to remove single-script code.
Closes #470
Closes #387

* Version 2 Upgrade Guide (#480)

* Version 2 Upgrade Guide

* Update website/docs/guides/version-2-upgrade.html.markdown

Co-Authored-By: Jacob Bednarz <jacob.bednarz@gmail.com>

* Apply suggestions from code review

Co-Authored-By: Jacob Bednarz <jacob.bednarz@gmail.com>

* Drop go 1.11, add 1.13
boekkooi-lengoo pushed a commit to boekkooi-lengoo/terraform-provider-cloudflare that referenced this pull request Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/documentation Categorizes issue or PR as related to documentation. size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants