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

page_rules: Swap to completely replacing rules #338

Merged

Conversation

jacobbednarz
Copy link
Member

@jacobbednarz jacobbednarz commented May 6, 2019

For a long time, this provider relied on sending page rules using
cloudflare-go's ChangePageRule which was documentated to allow inline
updates using a PATCH request. Until recently, this wasn't the case and
it actually mimicked the PUT alternative (UpdatePageRule) where it
attempted to replace the entire rule. This was fixed and while you
could now send inline updates to actions, there wasn't a way to remove
them once they were there. To address this, I've swapped back to
UpdatePageRule (the PUT request) which will resolve the aforementioned
issue with removing rules but it does re-introduce the caveat of not
being able to manage actions that require a Solutions Engineer. This
means, you can't manage them using this endpoint as you won't have
permission to apply them. This extends to page rule changes like
re-ordering or updating other actions which are in the same rule. If you
hit this use case, you should move the cache key into Cloudflare
Workers.

This commit backs out most of the changes from c1799f4 as they are no
longer required for differentiating the actions to send but it does
introduce some changes to the way we determine whether or not we send
browser_cache_ttl and edge_cache_ttl based on some issues
encountered with their default values. It now performs a HasChange on
their values to help determine whether or not they should be a part of
the payload.

Closes #198
Closes #265

For a long time, this provider relied on sending page rules using
cloudflare-go's `ChangePageRule` which was documentated to allow inline
updates using a PATCH request. Until recently, this wasn't the case and
it actually mimicked the PUT alternative (`UpdatePageRule`) where it
attempted to replace the entire rule. This was fixed[1] and while you
could now send inline updates to actions, there wasn't a way to remove
them once they were there. To address this, I've swapped back to
`UpdatePageRule` (the PUT request) which will resolve the aforementioned
issue with removing rules but it does re-introduce the caveat of not
being able to manage actions that require a Solutions Engineer. This
means, you can't manage them using this endpoint as you won't have
permission to apply them. This extends to page rule changes like
re-ordering or updating other actions which are in the same rule. If you
hit this use case, you should can move the cache key into Cloudflare
Workers.

This commit backs out most of the changes from c1799f4 as they are no
longer required for differentiating the actions to send.

[1]: https://github.com/terraform-providers/terraform-provider-cloudflare/pull/176#issuecomment-452906541
@ghost ghost added the size/S label May 6, 2019
@jacobbednarz jacobbednarz force-pushed the swap-page-rules-to-put-endpoint branch 3 times, most recently from 21f1008 to 44ac57c Compare May 9, 2019 04:50
@jacobbednarz jacobbednarz requested a review from patryk May 9, 2019 06:06
Updates the `transformToCloudflarePageRuleAction` functionality to
perform checks using `HasChange` to determine whether or not the value
should be sent or `nil`.
@jacobbednarz jacobbednarz force-pushed the swap-page-rules-to-put-endpoint branch from 44ac57c to 230976a Compare May 9, 2019 23:22
@jacobbednarz
Copy link
Member Author

We're also green on integration tests now.

=== RUN   TestAccCloudflarePageRule_Import
=== PAUSE TestAccCloudflarePageRule_Import
=== RUN   TestAccCloudflarePageRule_Basic
--- PASS: TestAccCloudflarePageRule_Basic (4.13s)
=== RUN   TestAccCloudflarePageRule_FullySpecified
--- PASS: TestAccCloudflarePageRule_FullySpecified (4.25s)
=== RUN   TestAccCloudflarePageRule_ForwardingOnly
--- PASS: TestAccCloudflarePageRule_ForwardingOnly (3.66s)
=== RUN   TestAccCloudflarePageRule_ForwardingAndOthers
--- PASS: TestAccCloudflarePageRule_ForwardingAndOthers (0.02s)
=== RUN   TestAccCloudflarePageRule_Updated
--- PASS: TestAccCloudflarePageRule_Updated (6.15s)
=== RUN   TestAccCloudflarePageRule_CreateAfterManualDestroy
--- PASS: TestAccCloudflarePageRule_CreateAfterManualDestroy (6.35s)
=== RUN   TestAccCloudflarePageRule_UpdatingZoneForcesNewResource
=== RUN   TestAccCloudflarePageRuleMinifyAction
--- PASS: TestAccCloudflarePageRuleMinifyAction (4.71s)
=== CONT  TestAccCloudflarePageRule_Import
--- PASS: TestAccCloudflarePageRule_Import (6.14s)
PASS
PASS	github.com/terraform-providers/terraform-provider-cloudflare/cloudflare	35.441s

@patryk patryk merged commit 14128d0 into cloudflare:master May 10, 2019
@patryk
Copy link
Contributor

patryk commented May 10, 2019

Thank you @jacobbednarz, merged!

@grogancolin
Copy link
Contributor

grogancolin commented May 30, 2019

Hi @jacobbednarz @patryk - I understand your reasoning on this, however I do feel it's a mistake that this provider explicitly backs out of managing specific page rules.

Not all organisations use workers, and assuming we can and should isn't something that the provider should be forcing on end users.

For our Page Rule configuration, this PR breaks completely our ability to manage them via code.

@jacobbednarz
Copy link
Member Author

@grogancolin I totally sympathise with you. This wasn't just a decision based on what made my maintainer life easier as internally, we are facing the issues both you and I have outlined in these PRs.

  • We have custom cache keys in page rules
  • We don't use Workers
  • We manage everything in code

One thing that helped make this decision was that up until about three months ago, this behaviour was consistent regardless of whether we used the PATCH or PUT endpoint (due to a regression in the API). We've been lucky enough that the number of page rules we're changing on the zones has really slowed down and importing from what changes are made in the UI is sufficient. This doesn't mean by any means that I'm settling for this; it just means I've opted for the masses and not the minority to allow bandwidth for focusing on the issue.

I will open a new issue with the current state of affairs and see if we can brainstorm some potential work arounds for the Cloudflare API (trying to handle it in the provider isn't really viable) and drag in the Cloudflare folks for their 2c. WDYT?

@grogancolin
Copy link
Contributor

@jacobbednarz - apologies I didn't get back to you on this.

I will open a new issue with the current state of affairs and see if we can brainstorm some potential work arounds for the Cloudflare API (trying to handle it in the provider isn't really viable) and drag in the Cloudflare folks for their 2c. WDYT?

I think that's a fair position - however it would be good to give it more urgency as this is a regular irritant for us.

Any updates on the current state of affairs?

Anything I can do to help with the task? Obviously I can't modify Cloudflare's APIs but I can and am more than willing to work on the provider to keep this issue moving in the right direction.

boekkooi-lengoo pushed a commit to boekkooi-lengoo/terraform-provider-cloudflare that referenced this pull request Feb 28, 2022
Creates support for configuring the Access Organization details. This is
essentially the login page you receive when a service is sitting behind
Cloudflare Access.

API documentation: https://api.cloudflare.com/#access-organizations-properties
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants