Deleting page rule action doesn't work with inline updates

[jacobbednarz]

This morning I was looking to apply a routine change to some page rules and noticed that actions which are removed cannot be performed using the current implementation of resourceCloudflarePageRuleUpdate due to it using ChangePageRule which performs a HTTP PATCH request (and was recently fixed to match the documented behaviour). The reason page rule actions cannot be removed using this API call is that the endpoint accepts partial updates to replace the existing action and to remove an action, it cannot be present or pass in an empty value (which the API doesn't allow). The Cloudflare Dashboard works around this issue by only using HTTP PUT requests and replace the rules each time.

This leaves us in a bit of a conundrum. If we swap to using the PUT endpoint, we can restore the previous functionality where deleting actions works but we can't perform partial updates to work around issues such as using actions like cache_key (due to requiring a Cloudflare Solutions Engineer to implement). If we leave the functionality as is, individual actions cannot be deleted but we can perform in place updates without recreating page rules each time.

I have considered a third option where we use a hybrid of the two but I'm unsure if the complexity is worth it as we might end up in a situation with lots of path branching to account for each scenario.

Thoughts?

cc @patryk as you might have some insight and thoughts here.

[garrettgalow]

@patryk is currently on leave. If I understand your point in the 2nd paragraph for the cache_key page rule PUT is not allowed since it won't let you create a PR with that key, but you can update a rule that already has the action?

For now, yes both is likely what would be necessary. I'll dig up some about the cache_key thing, and why it is the way it is.

[jacobbednarz]

@patryk is currently on leave.

Apologies on the extra ping 😛 I only found out he was off for a while the other day after I mentioned him here. This isn't a time sensitive discussion at the moment, so anytime async feedback is 👌 .

If I understand your point in the 2nd paragraph for the cache_key page rule PUT is not allowed since it won't let you create a PR with that key, but you can update a rule that already has the action?

Correct. The PATCH endpoint will only update the attributes you change whereas the PUT will replace the entire rule. Here is an example to solidify things:

Original rule

{
  "result": {
    "id": "eafa1a0d520e3d86d225f7f79c54d391",
    "targets": [
      {
        "target": "url",
        "constraint": {
          "operator": "matches",
          "value": "https://example.com"
        }
      }
    ],
    "actions": [
      {
        "id": "ssl",
        "value": "full"
      },
      {
        "id": "cache_key",
        "value": "${header:authorization}::${scheme}://${host_header}${uri}"
      }
    ],
    "priority": 3,
    "status": "active",
    "created_on": "2019-01-23T22:08:16.000000Z",
    "modified_on": "2019-01-23T22:55:06.000000Z"
  }
}

And to update ssl action to be "strict" we would perform the following PATCH request.

$ http PATCH https://api.cloudflare.com/client/v4/zones/$ZONE_ID/pagerules/$PAGE_RULE_ID \
  actions:='[{"id":"ssl","value":"strict"}]' 

The rule now looks like this.

{
  "result": {
    "id": "eafa1a0d520e3d86d225f7f79c54d391",
    "targets": [
      {
        "target": "url",
        "constraint": {
          "operator": "matches",
          "value": "https://example.com"
        }
      }
    ],
    "actions": [
      {
        "id": "ssl",
        "value": "strict"
      },
      {
        "id": "cache_key",
        "value": "${header:authorization}::${scheme}://${host_header}${uri}"
      }
    ],
    "priority": 3,
    "status": "active",
    "created_on": "2019-01-23T22:08:16.000000Z",
    "modified_on": "2019-01-23T22:55:06.000000Z"
  }
}

However, attempting a PUT request (with the full request structure) will fail due to it containing the cache_key attribute.

$ http PUT https://api.cloudflare.com/client/v4/zones/$ZONE_ID/pagerules/$PAGE_RULE_ID \
  targets:='[{"target":"url","constraint":{"operator":"matches","value":"https://example.com"}}]' \
  actions:='[{"id":"ssl","value":"strict"},{"id":"cache_key","value":"${header:authorization}::${scheme}://${host_header}${uri}"}]' \
  priority:= \
  status='active'

Results in an API error about not being able to processes the rule.

For now, yes both is likely what would be necessary.

I don't want to focus too much on the specific cache_key issue raised here as the sole issue but it's definitely a big consideration as it will introduce additional complexity.

The way I'm seeing this, can't really have a usable solution for when an action is deleted and and cache key is defined, as is with the API. In order to cater for that, I think we need a way to instruct the PATCH endpoint to remove a field (potentially by passing in an empty value or nil/null). If we can pass empty values for fields into the payload, the code could remain the same and we'd be able to continue using the PATCH endpoint.

Without the API changes, the code branching would look a little something like this:

if changesetIncludesDeletedAttribute && changesetIncludesCacheKey {
  // Fail out as we can't set the cache key value
} else if changesetIncludesDeleteAttribute {
  // Use PUT endpoint
} else {
  // Use PATCH endpoint
}

I'll dig up some about the cache_key thing, and why it is the way it is.

From what I remember about our previous conversations with our TAM and SE's on this, it was restricted to Cloudflare SE's to prevent setting cache keys that would overpopulate the cache systems due to having a cache key component with a high cardinality.

[garrettgalow]

There seems to be lots of issue with opening up the cache key action as customers can easily shoot themselves in the foot, so not likely to get opened up anytime soon.

I agree that the ability to nil out in PATCH would be the way to do it, but isn't very feasible given the schema used for page rules. Changing the schema is a much bigger discussion and not likely something we will undertake - we are more likely to replace page rules with a new system at some point.

I'll log an issue internally about your first if case, but I'm not sure what we are likely to do anytime soon.