feat(resources/servicevcl): handle state drift

docs/resources/fastly_service_vcl.md

@@ -36,7 +36,9 @@ The Service resource requires a domain name configured to direct traffic to the
 
 ### Read-Only
 
+- `force_refresh` (Boolean) Used internally by the provider to temporarily indicate if all resources should call their associated API to update the local state. This is for scenarios where the service version has been reverted outside of Terraform (e.g. via the Fastly UI) and the provider needs to resync the state for a different active version (this is only if `activate` is `true`)
 - `id` (String) Alphanumeric string identifying the service
+- `imported` (Boolean) Used internally by the provider to temporarily indicate if the service is being imported, and is reset to false once the import is finished
 - `last_active` (Number) The last 'active' service version (typically in-sync with `version` but not if `activate` is `false`)
 - `version` (Number) The latest version that the provider will clone from (typically in-sync with `last_active` but not if `activate` is `false`)
 

internal/helpers/errors.go

@@ -7,6 +7,8 @@ const (
 	ErrorAPIClient = "API Client Error"
 	// ErrorProvider indicates a Provider error.
 	ErrorProvider = "Provider Error"
+	// ErrorUnknown indicates an error incompatible with any other known scenario.
+	ErrorUnknown = "Unknown Error"
 	// ErrorUser indicates a User error.
 	ErrorUser = "User Error"
 )

internal/provider/models/service_vcl.go

@@ -18,8 +18,12 @@ type ServiceVCL struct {
 	Domains map[string]Domain `tfsdk:"domains"`
 	// ForceDestroy ensures a service will be fully deleted upon `terraform destroy`.
 	ForceDestroy types.Bool `tfsdk:"force_destroy"`
+	// ForceRefresh ensures all nested resources will have their state refreshed.
+	ForceRefresh types.Bool `tfsdk:"force_refresh"`
 	// ID is a unique ID for the service.
 	ID types.String `tfsdk:"id"`
+	// Imported indicates the resource is being imported.
+	Imported types.Bool `tfsdk:"imported"`
 	// LastActive is the last known active service version.
 	LastActive types.Int64 `tfsdk:"last_active"`
 	// Name is the service name.

internal/provider/resources/servicevcl/process_delete.go

@@ -25,7 +25,7 @@ func (r *Resource) Delete(ctx context.Context, req resource.DeleteRequest, resp
 		return
 	}
 
-	if (state.ForceDestroy.ValueBool() || state.Reuse.ValueBool()) && state.Activate.ValueBool() {
+	if state.ForceDestroy.ValueBool() || state.Reuse.ValueBool() {
 		clientReq := r.client.ServiceAPI.GetServiceDetail(r.clientCtx, state.ID.ValueString())
 		clientResp, httpResp, err := clientReq.Execute()
 		if err != nil {
@@ -40,14 +40,17 @@ func (r *Resource) Delete(ctx context.Context, req resource.DeleteRequest, resp
 			return
 		}
 
-		version := *clientResp.GetActiveVersion().Number
+		var activeVersion int32
+		if clientResp.GetActiveVersion().Number != nil {
+			activeVersion = *clientResp.GetActiveVersion().Number
+		}
 
-		if version != 0 {
-			clientReq := r.client.VersionAPI.DeactivateServiceVersion(r.clientCtx, state.ID.ValueString(), version)
+		if activeVersion != 0 {
+			clientReq := r.client.VersionAPI.DeactivateServiceVersion(r.clientCtx, state.ID.ValueString(), activeVersion)
 			_, httpResp, err := clientReq.Execute()
 			if err != nil {
 				tflog.Trace(ctx, "Fastly VersionAPI.DeactivateServiceVersion error", map[string]any{"http_resp": httpResp})
-				resp.Diagnostics.AddError(helpers.ErrorAPIClient, fmt.Sprintf("Unable to deactivate service version %d, got error: %s", version, err))
+				resp.Diagnostics.AddError(helpers.ErrorAPIClient, fmt.Sprintf("Unable to deactivate service version %d, got error: %s", activeVersion, err))
 				return
 			}
 			defer httpResp.Body.Close()

internal/provider/resources/servicevcl/process_read.go

@@ -28,8 +28,8 @@ func (r *Resource) Read(ctx context.Context, req resource.ReadRequest, resp *res
 		return
 	}
 
-	clientReq := r.client.ServiceAPI.GetServiceDetail(r.clientCtx, state.ID.ValueString())
-	clientResp, httpResp, err := clientReq.Execute()
+	serviceDetailsReq := r.client.ServiceAPI.GetServiceDetail(r.clientCtx, state.ID.ValueString())
+	serviceDetailsResp, httpResp, err := serviceDetailsReq.Execute()
 	if err != nil {
 		tflog.Trace(ctx, "Fastly ServiceAPI.GetServiceDetail error", map[string]any{"http_resp": httpResp})
 		resp.Diagnostics.AddError(helpers.ErrorAPIClient, fmt.Sprintf("Unable to retrieve service details, got error: %s", err))
@@ -39,22 +39,41 @@ func (r *Resource) Read(ctx context.Context, req resource.ReadRequest, resp *res
 
 	// Check if the service has been deleted outside of Terraform.
 	// And if so we'll just return.
-	if t, ok := clientResp.GetDeletedAtOk(); ok && t != nil {
+	if t, ok := serviceDetailsResp.GetDeletedAtOk(); ok && t != nil {
 		tflog.Trace(ctx, "Fastly ServiceAPI.GetDeletedAtOk", map[string]any{"deleted_at": t, "state": state})
 		resp.State.RemoveResource(ctx)
 		return
 	}
 
 	// Avoid issue with service type mismatch (only relevant when importing).
-	serviceType := clientResp.GetType()
+	serviceType := serviceDetailsResp.GetType()
 	vclServiceType := helpers.ServiceTypeVCL.String()
 	if serviceType != vclServiceType {
 		tflog.Trace(ctx, "Fastly service type error", map[string]any{"http_resp": httpResp, "type": serviceType})
 		resp.Diagnostics.AddError(helpers.ErrorUser, fmt.Sprintf("Expected service type %s, got: %s", vclServiceType, serviceType))
 		return
 	}
 
-	serviceVersion := readServiceVersion(state, clientResp)
+	remoteServiceVersion, err := readServiceVersion(state, serviceDetailsResp)
+	if err != nil {
+		tflog.Trace(ctx, "Fastly service version identification error", map[string]any{"state": state, "service_details": serviceDetailsResp, "error": err})
+		resp.Diagnostics.AddError(helpers.ErrorUnknown, err.Error())
+		return
+	}
+
+	// If the user has indicated they want their service to be 'active', then we
+	// presume when refreshing the state that we should be dealing with a service
+	// version that is active. If the prior state has a `version` field that
+	// doesn't match the current latest active version, then this suggests that
+	// the service versions have drifted outside of Terraform.
+	//
+	// e.g. a user has reverted the service version to another version via the UI.
+	//
+	// In this scenario, we'll set `force_refresh=true` so that the nested
+	// resources will call the Fastly API to get updated state information.
+	if state.Activate.ValueBool() && state.Version != types.Int64Value(remoteServiceVersion) {
+		state.ForceRefresh = types.BoolValue(true)
+	}
 
 	api := helpers.API{
 		Client:    r.client,
@@ -71,8 +90,8 @@ func (r *Resource) Read(ctx context.Context, req resource.ReadRequest, resp *res
 	// See `readSettings()` for an example of directly modifying `state`.
 	for _, nestedResource := range r.nestedResources {
 		serviceData := helpers.Service{
-			ID:      clientResp.GetID(),
-			Version: int32(serviceVersion),
+			ID:      serviceDetailsResp.GetID(),
+			Version: int32(remoteServiceVersion),
 		}
 		if err := nestedResource.Read(ctx, &req, resp, api, &serviceData); err != nil {
 			return
@@ -86,17 +105,37 @@ func (r *Resource) Read(ctx context.Context, req resource.ReadRequest, resp *res
 		return
 	}
 
-	state.Comment = types.StringValue(clientResp.GetComment())
-	state.ID = types.StringValue(clientResp.GetID())
-	state.Name = types.StringValue(clientResp.GetName())
-	state.Version = types.Int64Value(serviceVersion)
-	state.LastActive = types.Int64Value(serviceVersion)
+	state.Comment = types.StringValue(serviceDetailsResp.GetComment())
+	state.ID = types.StringValue(serviceDetailsResp.GetID())
+	state.Name = types.StringValue(serviceDetailsResp.GetName())
+	state.Version = types.Int64Value(remoteServiceVersion)
 
-	err = readSettings(ctx, state, resp, api)
+	// We set `last_active` to align with `version` only if `activate = true` or
+	// if we're importing (i.e. `activate` will be null). This is because we only
+	// expect `version` to drift from `last_active` if `activate = false`.
+	if state.Activate.ValueBool() || state.Activate.IsNull() {
+		state.LastActive = types.Int64Value(remoteServiceVersion)
+	}
+
+	err = readServiceSettings(ctx, remoteServiceVersion, state, resp, api)
 	if err != nil {
 		return
 	}
 
+	// To ensure nested resources don't continue to call the Fastly API to
+	// refresh the internal Terraform state, we set `imported`/`force_refresh`
+	// back to false.
+	//
+	// `force_refresh` is set to true earlier in this method.
+	// `imported` is set to true when `ImportState()` is called in ./resource.go
+	//
+	// We do this because it's slow and expensive to refresh the state for every
+	// nested resource if they've not even been defined in the user's TF config.
+	// But during an import we DO want to refresh all the state because we can't
+	// know up front what nested resources should exist.
+	state.ForceRefresh = types.BoolValue(false)
+	state.Imported = types.BoolValue(false)
+
 	// Save the final `state` data back into Terraform state.
 	resp.Diagnostics.Append(resp.State.Set(ctx, &state)...)
 
@@ -105,62 +144,90 @@ func (r *Resource) Read(ctx context.Context, req resource.ReadRequest, resp *res
 
 // readServiceVersion returns the service version.
 //
-// The returned value depends on if we're in an import scenario.
+// The returned values depends on if we're in an import scenario.
 //
 // When importing a service there might be no prior 'serviceVersion' in state.
 // If the user imports using the `ID@VERSION` syntax, then there will be.
 // This is because `ImportState()` in ./resource.go makes sure it's set.
 //
-// So we check if the attribute is null or not.
-//
-// If it's null, then we'll presume the user wants the last active version.
-// Which we retrieve from the GetServiceDetail call.
-// We fallback to the latest version if there is no prior active version.
+// So we check if `imported` is set and if the `version` attribute is not null.
+// If these conditions are true we'll check the specified version exists.
+// (see `versionFromImport()` for details).
 //
-// Otherwise we'll use whatever version they specified in their import.
-func readServiceVersion(state *models.ServiceVCL, clientResp *fastly.ServiceDetail) int64 {
-	var serviceVersion int64
+// If the conditions aren't met, then we'll call the Fastly API to get all
+// available service versions, and then we'll figure out which version we want
+// to return (see `versionFromRemote()` for details).
+func readServiceVersion(state *models.ServiceVCL, serviceDetailsResp *fastly.ServiceDetail) (serviceVersion int64, err error) {
+	if state.Imported.ValueBool() && !state.Version.IsNull() {
+		serviceVersion, err = versionFromImport(state, serviceDetailsResp)
+	} else {
+		serviceVersion, err = versionFromAttr(state, serviceDetailsResp)
+	}
+	return serviceVersion, err
+}
 
-	// This is the 'import without version' scenario.
-	//
-	// The Read() flow only gets executed after resources exist in the state
-	// (and we know a service resource will always have a version) or if the user
-	// is importing the resource (where upon there is no prior state and so the
-	// version field will be null).
-	//
-	// The only caveat to that is if the user specifies a version when importing.
-	// In that case, they'll end up in the `else` block below.
-	if state.Version.IsNull() {
-		var foundActive bool
-		versions := clientResp.GetVersions()
+// versionFromImport returns import specified service version.
+// It will validate the version specified actually exists remotely.
+func versionFromImport(state *models.ServiceVCL, serviceDetailsResp *fastly.ServiceDetail) (serviceVersion int64, err error) {
+	serviceVersion = state.Version.ValueInt64() // whatever version the user specified in their import
+	versions := serviceDetailsResp.GetVersions()
+	var foundVersion bool
+	for _, version := range versions {
+		if int64(version.GetNumber()) == serviceVersion {
+			foundVersion = true
+			break
+		}
+	}
+	if !foundVersion {
+		err = fmt.Errorf("failed to find version '%d' remotely", serviceVersion)
+	}
+	return serviceVersion, err
+}
+
+// versionFromAttr returns the service version based on `activate` attribute.
+// If `activate = true`, then we return the latest 'active' service version.
+// If `activate = false` we return the latest version. This allows state drift.
+func versionFromAttr(state *models.ServiceVCL, serviceDetailsResp *fastly.ServiceDetail) (serviceVersion int64, err error) {
+	versions := serviceDetailsResp.GetVersions()
+	size := len(versions)
+	switch {
+	case size == 0:
+		err = errors.New("failed to find any service versions remotely")
+	case state.Activate.IsNull():
+		fallthrough // when importing `activate` doesn't have its default value set so we default to importing the latest 'active' version.
+	case state.Activate.ValueBool():
+		var foundVersion bool
 		for _, version := range versions {
 			if version.GetActive() {
 				serviceVersion = int64(version.GetNumber())
-				foundActive = true
+				foundVersion = true
 				break
 			}
 		}
-		if !foundActive {
-			// Use latest version if the user imports a service with no active versions.
-			serviceVersion = int64(versions[0].GetNumber())
+		if !foundVersion {
+			// If we're importing a service, then we don't have `activate` value.
+			// So if there's no active version to use, fallback the latest version.
+			if state.Imported.ValueBool() {
+				serviceVersion = getLatestServiceVersion(size-1, versions)
+			} else {
+				err = errors.New("failed to find active version remotely")
+			}
 		}
-	} else {
-		// This is the standard flow which is reached either when the user has a
-		// plan and there is already this resource in the state (and we know a
-		// service resource will always have a version) or if the user is importing
-		// the resource WITH a service version specified.
-		serviceVersion = state.Version.ValueInt64()
+	default:
+		// If `activate = false` then we expect state drift and will pull in the
+		// latest version available (regardless of if it's active or not).
+		serviceVersion = getLatestServiceVersion(size-1, versions)
 	}
+	return serviceVersion, err
+}
 
-	return serviceVersion
+func getLatestServiceVersion(i int, versions []fastly.SchemasVersionResponse) int64 {
+	return int64(versions[i].GetNumber())
 }
 
-func readSettings(ctx context.Context, state *models.ServiceVCL, resp *resource.ReadResponse, api helpers.API) error {
+func readServiceSettings(ctx context.Context, serviceVersion int64, state *models.ServiceVCL, resp *resource.ReadResponse, api helpers.API) error {
 	serviceID := state.ID.ValueString()
-	serviceVersion := int32(state.Version.ValueInt64())
-
-	clientReq := api.Client.SettingsAPI.GetServiceSettings(api.ClientCtx, serviceID, serviceVersion)
-
+	clientReq := api.Client.SettingsAPI.GetServiceSettings(api.ClientCtx, serviceID, int32(serviceVersion))
 	readErr := errors.New("failed to read service settings")
 
 	clientResp, httpResp, err := clientReq.Execute()

internal/provider/resources/servicevcl/process_update.go

@@ -78,7 +78,7 @@ func (r *Resource) Update(ctx context.Context, req resource.UpdateRequest, resp
 		return
 	}
 
-	if nestedResourcesChanged {
+	if nestedResourcesChanged && plan.Activate.ValueBool() {
 		clientReq := r.client.VersionAPI.ActivateServiceVersion(r.clientCtx, plan.ID.ValueString(), serviceVersion)
 		_, httpResp, err := clientReq.Execute()
 		if err != nil {

internal/provider/resources/servicevcl/resource.go

@@ -142,6 +142,18 @@ func (r *Resource) Configure(_ context.Context, req resource.ConfigureRequest, r
 // The service resource then iterates over all nested resources populating the
 // state for each nested resource.
 func (r *Resource) ImportState(ctx context.Context, req resource.ImportStateRequest, resp *resource.ImportStateResponse) {
+	// FIXME: Make sure we validate this in a test.
+	//
+	// To ensure nested resources don't continue to call the Fastly API to
+	// refresh the internal Terraform state, we set `imported` to true.
+	// It's set back to false in ./process_read.go
+	//
+	// We do this because it's slow and expensive to refresh the state for every
+	// nested resource if they've not even been defined in the user's TF config.
+	// But during an import we DO want to refresh all the state because we can't
+	// know up front what nested resources should exist.
+	resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root("imported"), true)...)
+
 	id, version, found := strings.Cut(req.ID, "@")
 	if found {
 		v, err := strconv.ParseInt(version, 10, 64)

internal/provider/schemas/service.go

@@ -10,8 +10,12 @@ import (
 
 // Service returns the common schema attributes between VCL/Compute services.
 //
-// NOTE: Some optional attributes are also 'computed' so we can set a default.
+// NOTE: Some 'optional' attributes are also 'computed' so we can set a default.
 // This is a requirement enforced on us by Terraform.
+//
+// NOTE: Some 'computed' attributes require a default to avoid test errors.
+// If we don't set a default, the Create/Update methods have to explicitly set a
+// value for the computed attributes. It's cleaner/easier to just set defaults.
 func Service() map[string]schema.Attribute {
 	return map[string]schema.Attribute{
 		"activate": schema.BoolAttribute{
@@ -46,6 +50,11 @@ func Service() map[string]schema.Attribute {
 			MarkdownDescription: "Services that are active cannot be destroyed. In order to destroy the service, set `force_destroy` to `true`. Default `false`",
 			Optional:            true,
 		},
+		"force_refresh": schema.BoolAttribute{
+			Computed:            true,
+			Default:             booldefault.StaticBool(false),
+			MarkdownDescription: "Used internally by the provider to temporarily indicate if all resources should call their associated API to update the local state. This is for scenarios where the service version has been reverted outside of Terraform (e.g. via the Fastly UI) and the provider needs to resync the state for a different active version (this is only if `activate` is `true`)",
+		},
 		"id": schema.StringAttribute{
 			Computed:            true,
 			MarkdownDescription: "Alphanumeric string identifying the service",
@@ -55,6 +64,11 @@ func Service() map[string]schema.Attribute {
 				stringplanmodifier.UseStateForUnknown(),
 			},
 		},
+		"imported": schema.BoolAttribute{
+			Computed:            true,
+			Default:             booldefault.StaticBool(false),
+			MarkdownDescription: "Used internally by the provider to temporarily indicate if the service is being imported, and is reset to false once the import is finished",
+		},
 		"last_active": schema.Int64Attribute{
 			Computed:            true,
 			MarkdownDescription: "The last 'active' service version (typically in-sync with `version` but not if `activate` is `false`)",