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(product_enablement): avoid accidentally disabling products on update #763

Merged
merged 1 commit into from
Oct 17, 2023

Conversation

Integralist
Copy link
Collaborator

@Integralist Integralist commented Sep 28, 2023

Fixes: #761

Problems

I realised there were some fundamental issues with the original implementation of the product_enablement block for the fastly_service_vcl and fastly_service_compute resources (docs).

  1. We had set the name attribute to be a Computed attribute. This would cause the block to be deleted, then recreated for any change the user made to the block, which consequently meant the 'Update' method for the block was never being called (I've now changed name to no longer be Computed).
  2. We were setting a default value of false for each product (I've removed that so now the attributes will be stored in the state file as null if they're not set in the user's TF config, which avoids accidental API calls to disable a product).
  3. We were setting all products into the state file when really we should have only have set products into the state file when the API indicated the product had been 'enabled'. If a product wasn't enabled, then we shouldn't have bothered setting it to false. We correct it in this PR and so products not enabled simply show as null in the state file.

Concerns

My main concern with this PR has been to avoid a breaking interface change, that would consequently require a new major version of the provider to be published.

I don't believe this PR introduces a breaking interface change.

There were two fundamental challenges...

  1. The change of name to be optional.
  2. No longer setting false as a default for each product.

The first challenge was the change to the name attribute, which is now Optional rather than Computed. To prevent any issues, we additionally set Default: "products" on the attribute, which helps to keep it in sync with the old Computed value, which we had previously hardcoded to "products" anyway. So there shouldn't be any problems for this point.

The second challenge was the move away from setting a default of false for all products, and this one is more of a 'grey area' when it comes to the "avoid breaking changes" concern and needs more context to explain...

A customer who already has product_enablement defined in their Terraform config (i.e. they're using it with an older version of the Fastly Terraform provider) is likely going to have all the products defined in their config (even those products they don't use!). This is something users had to do (i.e. set each product to false) to avoid seeing an unexpected diff after their terraform apply (this is because of point 3. above).

So although in this PR we've fixed things, such that the user no longer has to set any product other than the ones they are able to enable/disable, because we've also fixed an issue with the Update method never being called (see point 1. above), these older/existing customers might run into a separate scenario.

That separate scenario is triggering a 'disable product' API call in the Update method the next time they run an apply if using a version of the provider with this PR change set in it (remember the Update method isn't being called in the current/published implementation due to a bug that this PR fixes).

An example of this issue would be a customer who has Image Optimizer enabled on their account but who wasn't entitled to enable/disable Image Optimizer. The terraform apply would fail when the API returns an error to say (paraphrasing) "sorry, you're not entitled to enable/disable this product".

The solution to that problem is for the customer to just remove any unnecessary products from their TF (i.e. only include products they are able to enable/disable) but because this is a change in behaviour and not the interface, it might not be obvious for them to do that.

Workaround?

So the second challenge I just mentioned isn't ideal and could be argued that it would require a new major version, because we'd otherwise have customers suddenly see changes in their plans and it would be unclear why that is suddenly happening (and if our "solution" is for the customer to have to manually make a change to their config, then although we've not changed the exposed interface, we have still broken their build).

To workaround that concern, I've implemented in this PR something we do already in the 'Delete' method, which is to check the error returned by the API and if it's a message that indicates the user isn't entitled to enable/disable the product, we'll just ignore the error and not return it. This way we'll allow the customer's terraform apply to finish successfully.

Yes, they'll still have things like image_optimizer = false unnecessarily set in their config (and it'll look confusing if they know that they actually have the product enabled manually by Fastly support), and of course they can delete those products from the Terraform config whenever they like once they upgrade to this version of the provider, but at least they won't see an error and they should hopefully not experience any breaks in user flows.

@Integralist Integralist merged commit 79c6bfb into main Oct 17, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cannot enable brotli without impacting image optimization
2 participants