fix(product_enablement): improve error handling for various scenarios (#651)

* fix(product_enablement): set defaults so no unexpected plan diff when block is defined with no attributes set

* fix(product_enablement): improve error handling for various scenarios

* fix(tests): update name computed value

Author: Integralist

fastly/block_fastly_service_product_enablement.go

@@ -2,7 +2,10 @@ package fastly
 
 import (
 	"context"
+	"fmt"
 	"log"
+	"net/http"
+	"strings"
 
 	gofastly "github.com/fastly/go-fastly/v7/fastly"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
@@ -42,6 +45,7 @@ func (h *ProductEnablementServiceAttributeHandler) GetSchema() *schema.Schema {
 		blockAttributes["fanout"] = &schema.Schema{
 			Type:        schema.TypeBool,
 			Optional:    true,
+			Default:     false,
 			Description: "Enable Fanout support",
 		}
 	}
@@ -50,21 +54,25 @@ func (h *ProductEnablementServiceAttributeHandler) GetSchema() *schema.Schema {
 		blockAttributes["brotli_compression"] = &schema.Schema{
 			Type:        schema.TypeBool,
 			Optional:    true,
+			Default:     false,
 			Description: "Enable Brotli Compression support",
 		}
 		blockAttributes["domain_inspector"] = &schema.Schema{
 			Type:        schema.TypeBool,
 			Optional:    true,
+			Default:     false,
 			Description: "Enable Domain Inspector support",
 		}
 		blockAttributes["image_optimizer"] = &schema.Schema{
 			Type:        schema.TypeBool,
 			Optional:    true,
+			Default:     false,
 			Description: "Enable Image Optimizer support (requires at least one backend with a `shield` attribute)",
 		}
 		blockAttributes["origin_inspector"] = &schema.Schema{
 			Type:        schema.TypeBool,
 			Optional:    true,
+			Default:     false,
 			Description: "Enable Origin Inspector support",
 		}
 	}
@@ -73,6 +81,7 @@ func (h *ProductEnablementServiceAttributeHandler) GetSchema() *schema.Schema {
 	blockAttributes["websockets"] = &schema.Schema{
 		Type:        schema.TypeBool,
 		Optional:    true,
+		Default:     false,
 		Description: "Enable WebSockets support",
 	}
 
@@ -101,7 +110,7 @@ func (h *ProductEnablementServiceAttributeHandler) Create(_ context.Context, d *
 				ServiceID: serviceID,
 			})
 			if err != nil {
-				return err
+				return fmt.Errorf("failed to enable fanout: %w", err)
 			}
 		}
 	}
@@ -114,7 +123,7 @@ func (h *ProductEnablementServiceAttributeHandler) Create(_ context.Context, d *
 				ServiceID: serviceID,
 			})
 			if err != nil {
-				return err
+				return fmt.Errorf("failed to enable brotli_compression: %w", err)
 			}
 		}
 
@@ -125,7 +134,7 @@ func (h *ProductEnablementServiceAttributeHandler) Create(_ context.Context, d *
 				ServiceID: serviceID,
 			})
 			if err != nil {
-				return err
+				return fmt.Errorf("failed to enable domain_inspector: %w", err)
 			}
 		}
 
@@ -136,7 +145,7 @@ func (h *ProductEnablementServiceAttributeHandler) Create(_ context.Context, d *
 				ServiceID: serviceID,
 			})
 			if err != nil {
-				return err
+				return fmt.Errorf("failed to enable image_optimizer: %w", err)
 			}
 		}
 
@@ -147,7 +156,7 @@ func (h *ProductEnablementServiceAttributeHandler) Create(_ context.Context, d *
 				ServiceID: serviceID,
 			})
 			if err != nil {
-				return err
+				return fmt.Errorf("failed to enable origin_inspector: %w", err)
 			}
 		}
 	}
@@ -159,7 +168,7 @@ func (h *ProductEnablementServiceAttributeHandler) Create(_ context.Context, d *
 			ServiceID: serviceID,
 		})
 		if err != nil {
-			return err
+			return fmt.Errorf("failed to enable websockets: %w", err)
 		}
 	}
 
@@ -191,7 +200,7 @@ func (h *ProductEnablementServiceAttributeHandler) Read(_ context.Context, d *sc
 		// This is done so we can benefit from the 'modified' map data passed to the
 		// 'update' CRUD method.
 		result := map[string]any{
-			"name": "product_enablement",
+			"name": "products",
 		}
 
 		if h.GetServiceMetadata().serviceType == ServiceTypeCompute {
@@ -282,7 +291,7 @@ func (h *ProductEnablementServiceAttributeHandler) Update(_ context.Context, d *
 					ServiceID: serviceID,
 				})
 				if err != nil {
-					return err
+					return fmt.Errorf("failed to enable fanout: %w", err)
 				}
 			} else {
 				log.Println("[DEBUG] fanout not set")
@@ -291,13 +300,15 @@ func (h *ProductEnablementServiceAttributeHandler) Update(_ context.Context, d *
 					ServiceID: serviceID,
 				})
 				if err != nil {
-					return err
+					return fmt.Errorf("failed to disable fanout: %w", err)
 				}
 			}
 		}
 	}
 
 	if h.GetServiceMetadata().serviceType == ServiceTypeVCL {
+		// FIXME: Looks like `modified` contains products that haven't been updated.
+		// The only practical issue here is that an unnecessary API request is made.
 		if v, ok := modified["brotli_compression"]; ok {
 			if v.(bool) {
 				log.Println("[DEBUG] brotli_compression set")
@@ -306,7 +317,7 @@ func (h *ProductEnablementServiceAttributeHandler) Update(_ context.Context, d *
 					ServiceID: serviceID,
 				})
 				if err != nil {
-					return err
+					return fmt.Errorf("failed to enable brotli_compression: %w", err)
 				}
 			} else {
 				log.Println("[DEBUG] brotli_compression not set")
@@ -315,7 +326,7 @@ func (h *ProductEnablementServiceAttributeHandler) Update(_ context.Context, d *
 					ServiceID: serviceID,
 				})
 				if err != nil {
-					return err
+					return fmt.Errorf("failed to disable brotli_compression: %w", err)
 				}
 			}
 		}
@@ -328,7 +339,7 @@ func (h *ProductEnablementServiceAttributeHandler) Update(_ context.Context, d *
 					ServiceID: serviceID,
 				})
 				if err != nil {
-					return err
+					return fmt.Errorf("failed to enable domain_inspector: %w", err)
 				}
 			} else {
 				log.Println("[DEBUG] domain_inspector not set")
@@ -337,7 +348,7 @@ func (h *ProductEnablementServiceAttributeHandler) Update(_ context.Context, d *
 					ServiceID: serviceID,
 				})
 				if err != nil {
-					return err
+					return fmt.Errorf("failed to disable domain_inspector: %w", err)
 				}
 			}
 		}
@@ -350,7 +361,7 @@ func (h *ProductEnablementServiceAttributeHandler) Update(_ context.Context, d *
 					ServiceID: serviceID,
 				})
 				if err != nil {
-					return err
+					return fmt.Errorf("failed to enable image_optimizer: %w", err)
 				}
 			} else {
 				log.Println("[DEBUG] image_optimizer not set")
@@ -359,7 +370,7 @@ func (h *ProductEnablementServiceAttributeHandler) Update(_ context.Context, d *
 					ServiceID: serviceID,
 				})
 				if err != nil {
-					return err
+					return fmt.Errorf("failed to disable image_optimizer: %w", err)
 				}
 			}
 		}
@@ -372,7 +383,7 @@ func (h *ProductEnablementServiceAttributeHandler) Update(_ context.Context, d *
 					ServiceID: serviceID,
 				})
 				if err != nil {
-					return err
+					return fmt.Errorf("failed to enable origin_inspector: %w", err)
 				}
 			} else {
 				log.Println("[DEBUG] origin_inspector not set")
@@ -381,7 +392,7 @@ func (h *ProductEnablementServiceAttributeHandler) Update(_ context.Context, d *
 					ServiceID: serviceID,
 				})
 				if err != nil {
-					return err
+					return fmt.Errorf("failed to disable origin_inspector: %w", err)
 				}
 			}
 		}
@@ -395,7 +406,7 @@ func (h *ProductEnablementServiceAttributeHandler) Update(_ context.Context, d *
 				ServiceID: serviceID,
 			})
 			if err != nil {
-				return err
+				return fmt.Errorf("failed to enable websockets: %w", err)
 			}
 		} else {
 			log.Println("[DEBUG] websockets not set")
@@ -404,7 +415,7 @@ func (h *ProductEnablementServiceAttributeHandler) Update(_ context.Context, d *
 				ServiceID: serviceID,
 			})
 			if err != nil {
-				return err
+				return fmt.Errorf("failed to disable websockets: %w", err)
 			}
 		}
 	}
@@ -414,60 +425,121 @@ func (h *ProductEnablementServiceAttributeHandler) Update(_ context.Context, d *
 
 // Delete deletes the resource.
 //
-// FIXME: This implementation causes unnecessary API calls.
-// We should check if the specific 'products' have been disabled.
+// IMPORTANT: We must allow a user to clean-up their state.
+// If a user doesn't have self-enablement for a particular product and they add
+// the product_enablement block to their config, then they'll have errors trying
+// to enable that product. The problem now is if they remove the block from
+// their config completely, they still won't be able to successfully apply the
+// deletion because the API will error telling them they're not entitled to
+// disable a product. So if that's the error we're getting back from the API,
+// then we'll skip the error as we want the `terraform apply` to be successful
+// and for the user to end up with a clean state.
+//
+// NOTE: We don't return the nil error, ensuring all products are processed.
+// We don't want to return the nil error (e.g. when a user is trying to clean-up
+// their state as they're not entitled to enable/disable the product) because
+// returning nil will short-circuit the `Delete` method and we'll not process
+// the disabling of other products they might be entitled to disable!
+//
+// FIXME: Looks like the use of a TypeSet means unnecessary API calls.
+// In a scenario where a new product is set to `true` (e.g. to be enabled) the
+// set hash changes and so the set 'as a whole' is deleted (causing all the
+// products to be disabled) and then all the APIs are called again to re-enable
+// the products (even though they might not have actually been set to `false` in
+// the first place). The solution would be to swap TypeSet for TypeList but then
+// that means we'd lose the 'modified' data diff abstraction that was built into
+// the Fastly provider. Look at the `package` block as an example of a TypeList,
+// and you'll see it doesn't implement standard CRUD methods but has a single
+// `Process` method that handles both CREATE and UPDATE stages and doesn't get
+// passed a data structure that indicates what has changed like we do with the
+// TypeSet data type. So it'll be a trade-off.
 func (h *ProductEnablementServiceAttributeHandler) Delete(_ context.Context, d *schema.ResourceData, resource map[string]any, serviceVersion int, conn *gofastly.Client) error {
 	if h.GetServiceMetadata().serviceType == ServiceTypeCompute {
+		log.Println("[DEBUG] disable fanout")
 		err := conn.DisableProduct(&gofastly.ProductEnablementInput{
 			ProductID: gofastly.ProductFanout,
 			ServiceID: d.Id(),
 		})
 		if err != nil {
-			return err
+			if e := h.checkAPIError(err); e != nil {
+				return e
+			}
 		}
 	}
 
 	if h.GetServiceMetadata().serviceType == ServiceTypeVCL {
+		log.Println("[DEBUG] disable brotli_compression")
 		err := conn.DisableProduct(&gofastly.ProductEnablementInput{
 			ProductID: gofastly.ProductBrotliCompression,
 			ServiceID: d.Id(),
 		})
 		if err != nil {
-			return err
+			if e := h.checkAPIError(err); e != nil {
+				return e
+			}
 		}
 
+		log.Println("[DEBUG] disable domain_inspector")
 		err = conn.DisableProduct(&gofastly.ProductEnablementInput{
 			ProductID: gofastly.ProductDomainInspector,
 			ServiceID: d.Id(),
 		})
 		if err != nil {
-			return err
+			if e := h.checkAPIError(err); e != nil {
+				return e
+			}
 		}
 
+		log.Println("[DEBUG] disable image_optimizer")
 		err = conn.DisableProduct(&gofastly.ProductEnablementInput{
 			ProductID: gofastly.ProductImageOptimizer,
 			ServiceID: d.Id(),
 		})
 		if err != nil {
-			return err
+			if e := h.checkAPIError(err); e != nil {
+				return e
+			}
 		}
 
+		log.Println("[DEBUG] disable origin_inspector")
 		err = conn.DisableProduct(&gofastly.ProductEnablementInput{
 			ProductID: gofastly.ProductOriginInspector,
 			ServiceID: d.Id(),
 		})
 		if err != nil {
-			return err
+			if e := h.checkAPIError(err); e != nil {
+				return e
+			}
 		}
 	}
 
+	log.Println("[DEBUG] disable websockets")
 	err := conn.DisableProduct(&gofastly.ProductEnablementInput{
 		ProductID: gofastly.ProductWebSockets,
 		ServiceID: d.Id(),
 	})
 	if err != nil {
-		return err
+		if e := h.checkAPIError(err); e != nil {
+			return e
+		}
 	}
 
 	return nil
 }
+
+// checkAPIError inspects the error type for a title that has a message
+// indicating the user cannot call the API because they are not entitled. For
+// these users we want to skip the error so that we can allow them to clean up
+// their Terraform state.
+func (h *ProductEnablementServiceAttributeHandler) checkAPIError(err error) error {
+	if he, ok := err.(*gofastly.HTTPError); ok {
+		if he.StatusCode == http.StatusBadRequest {
+			for _, e := range he.Errors {
+				if strings.Contains(e.Title, "is not entitled to disable product") {
+					return nil
+				}
+			}
+		}
+	}
+	return err
+}

fastly/resource_fastly_service_compute_test.go

@@ -60,8 +60,8 @@ func TestAccFastlyServiceCompute_basic(t *testing.T) {
 					if s[0].Attributes["product_enablement.0.websockets"] != "false" {
 						return fmt.Errorf("expected websockets to be false")
 					}
-					if s[0].Attributes["product_enablement.0.name"] != "product_enablement" {
-						return fmt.Errorf("expected the generated 'name' key to be 'product_enablement'")
+					if s[0].Attributes["product_enablement.0.name"] != "products" {
+						return fmt.Errorf("expected the generated 'name' key to be 'products'")
 					}
 					return nil
 				},

fastly/resource_fastly_service_vcl_test.go

@@ -377,8 +377,8 @@ func TestAccFastlyServiceVCL_basic(t *testing.T) {
 					if s[0].Attributes["product_enablement.0.websockets"] != "false" {
 						return fmt.Errorf("expected websockets to be false")
 					}
-					if s[0].Attributes["product_enablement.0.name"] != "product_enablement" {
-						return fmt.Errorf("expected the generated 'name' key to be 'product_enablement'")
+					if s[0].Attributes["product_enablement.0.name"] != "products" {
+						return fmt.Errorf("expected the generated 'name' key to be 'products'")
 					}
 					return nil
 				},