feat(resources/servicevcl): handle state drift (#74)

* doc(resources/servicevcl): explain reason for mutating req state in nested resources

* doc(helpers/data): detail issue with int32 support in fastly-go

* doc(resources/servicevcl): clarify different read scenarios

* refactor(tests): rename standard behaviours test function

* 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/data.go

@@ -6,6 +6,16 @@ type Service struct {
 	ID string
 	// Version is the current version for the Fastly service.
 	Version int32
+
+	// TODO: Consider updating the fastly-go API client to use int64.
+	//
+	// This is because Terraform doesn't support int32 (https://github.com/hashicorp/terraform-plugin-framework/issues/801).
+	// The change would be more to help the readability of the Terraform provider code.
+	// As we wouldn't need the visual noise of constantly converting between 32 and 64 types.
+	//
+	// Although, strictly speaking, downsizing an int64 to int32 could cause data
+	// loss, the reality is an int32 largest value is 2,147,483,647 and it's
+	// doubtful that a user service will contain that many versions (in practice).
 }
 
 // ServiceType is a base for the different service variants.

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_create.go

@@ -58,8 +58,6 @@ func (r *Resource) Create(ctx context.Context, req resource.CreateRequest, resp
 	}
 
 	if plan.Activate.ValueBool() {
-		plan.LastActive = plan.Version
-
 		clientReq := r.client.VersionAPI.ActivateServiceVersion(r.clientCtx, serviceID, serviceVersion)
 		_, httpResp, err := clientReq.Execute()
 		if err != nil {
@@ -68,6 +66,9 @@ func (r *Resource) Create(ctx context.Context, req resource.CreateRequest, resp
 			return
 		}
 		defer httpResp.Body.Close()
+
+		// Only set LastActive to Version if we successfully activate the service.
+		plan.LastActive = plan.Version
 	}
 
 	// Save the planned changes into Terraform state.

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

@@ -54,26 +54,52 @@ func (r *Resource) Read(ctx context.Context, req resource.ReadRequest, resp *res
 		return
 	}
 
-	serviceVersion := readServiceVersion(state, clientResp)
+	remoteServiceVersion, err := readServiceVersion(state, clientResp)
+	if err != nil {
+		tflog.Trace(ctx, "Fastly service version identification error", map[string]any{"state": state, "service_details": clientResp, "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,
 		ClientCtx: r.clientCtx,
 	}
 
-	// IMPORTANT: nestedResources are expected to mutate the plan data.
+	// IMPORTANT: nestedResources are expected to mutate the `req` plan data.
+	//
+	// We really should modify the `state` variable instead.
+	// The reason we don't do this is for interface consistency.
+	// i.e. The interfaces.Resource.Read() can have a consistent type.
+	// This is because the `state` variable type can change based on the resource.
+	// e.g. `models.ServiceVCL` or `models.ServiceCompute`.
+	// See `readSettings()` for an example of directly modifying `state`.
 	for _, nestedResource := range r.nestedResources {
 		serviceData := helpers.Service{
 			ID:      clientResp.GetID(),
-			Version: int32(serviceVersion),
+			Version: int32(remoteServiceVersion),
 		}
 		if err := nestedResource.Read(ctx, &req, resp, api, &serviceData); err != nil {
 			return
 		}
 	}
 
-	// Refresh the Terraform state data inside the model.
-	// As the state is expected to be mutated by nested resources.
+	// Sync the Terraform `state` data.
+	// As the `req` state is expected to be mutated by nested resources.
 	resp.Diagnostics.Append(req.State.Get(ctx, &state)...)
 	if resp.Diagnostics.HasError() {
 		return
@@ -82,65 +108,126 @@ func (r *Resource) Read(ctx context.Context, req resource.ReadRequest, resp *res
 	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.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.LastActive = types.Int64Value(remoteServiceVersion)
+	}
+
+	err = readServiceSettings(ctx, remoteServiceVersion, state, resp, api)
 	if err != nil {
 		return
 	}
 
-	// Save the updated state data back into Terraform state.
+	// 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)...)
 
 	tflog.Debug(ctx, "Read", map[string]any{"state": fmt.Sprintf("%#v", state)})
 }
 
 // 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.
+// When importing a service there might be no prior `version` 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
+}
+
+// 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
+}
 
-	if state.Version.IsNull() {
-		var foundActive bool
-		versions := clientResp.GetVersions()
+// 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 {
-		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`)",