feat(resources/servicevcl): handle state drift

docs/resources/fastly_service_vcl.md

@@ -37,6 +37,7 @@ The Service resource requires a domain name configured to direct traffic to the
 ### Read-Only
 
 - `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

@@ -20,6 +20,8 @@ type ServiceVCL struct {
 	ForceDestroy types.Bool `tfsdk:"force_destroy"`
 	// 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 {

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,27 @@ 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)
+	serviceVersion, 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
+	}
 
 	api := helpers.API{
 		Client:    r.client,
@@ -71,7 +76,7 @@ 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(),
+			ID:      serviceDetailsResp.GetID(),
 			Version: int32(serviceVersion),
 		}
 		if err := nestedResource.Read(ctx, &req, resp, api, &serviceData); err != nil {
@@ -86,17 +91,33 @@ 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.Comment = types.StringValue(serviceDetailsResp.GetComment())
+	state.ID = types.StringValue(serviceDetailsResp.GetID())
+	state.Name = types.StringValue(serviceDetailsResp.GetName())
 	state.Version = types.Int64Value(serviceVersion)
-	state.LastActive = types.Int64Value(serviceVersion)
 
-	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(serviceVersion)
+	}
+
+	err = readServiceSettings(ctx, serviceVersion, 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` back to false.
+	// It's 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.Imported = types.BoolValue(false)
+
 	// Save the final `state` data back into Terraform state.
 	resp.Diagnostics.Append(resp.State.Set(ctx, &state)...)
 
@@ -105,7 +126,7 @@ 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.
@@ -117,50 +138,66 @@ func (r *Resource) Read(ctx context.Context, req resource.ReadRequest, resp *res
 // Which we retrieve from the GetServiceDetail call.
 // We fallback to the latest version if there is no prior active version.
 //
-// Otherwise we'll use whatever version they specified in their import.
-func readServiceVersion(state *models.ServiceVCL, clientResp *fastly.ServiceDetail) int64 {
-	var serviceVersion int64
+// Otherwise we'll use whatever version the user specified in their import.
+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 = versionFromRemote(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
+}
+
+// versionFromRemote 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 versionFromRemote(state *models.ServiceVCL, serviceDetailsResp *fastly.ServiceDetail) (serviceVersion int64, err error) {
+	versions := serviceDetailsResp.GetVersions()
+	size := len(versions)
+	switch {
+	case state.Activate.IsNull():
+		fallthrough // when importing `activate` doesn't have its default value set
+	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 {
+			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()
+	case size == 0:
+		err = fmt.Errorf("failed to find version '%d' remotely", serviceVersion)
+	default:
+		serviceVersion = int64(versions[size-1].GetNumber()) // use latest version
 	}
-
-	return serviceVersion
+	return serviceVersion, err
 }
 
-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

@@ -55,6 +55,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`)",

internal/provider/tests/resources/service_vcl_test.go

@@ -325,6 +325,94 @@ func TestAccResourceServiceVCLImportServiceVersion(t *testing.T) {
 	})
 }
 
+// The following test validates that when we have more than one service, where
+// the latter is not 'active' that we return and track that version instead of
+// any prior active version.
+func TestAccResourceServiceVCLLatestNonActiveVersion(t *testing.T) {
+	serviceName := fmt.Sprintf("tf-test-%s", acctest.RandString(10))
+	domain1Name := fmt.Sprintf("%s-tpff-1.integralist.co.uk", serviceName)
+	domain1CommentAdded := "a random updated comment"
+	domain2Name := fmt.Sprintf("%s-tpff-2.integralist.co.uk", serviceName)
+
+	configCreate := configServiceVCLCreate(configServiceVCLCreateOpts{
+		activate:     true,
+		forceDestroy: false,
+		serviceName:  serviceName,
+		domain1Name:  domain1Name,
+		domain2Name:  domain2Name,
+	})
+
+	// Update the first domain's comment + set `activate = false`.
+	//
+	// IMPORTANT: Must set `force_destroy` to `true` so we can delete the service.
+	configUpdate := fmt.Sprintf(`
+    resource "fastly_service_vcl" "test" {
+      activate = false
+      name = "%s"
+      force_destroy = true
+
+      domains = {
+        "example-1" = {
+          name = "%s"
+          comment = "%s"
+        },
+        "example-2" = {
+          name = "%s"
+        },
+      }
+    }
+    `, serviceName, domain1Name, domain1CommentAdded, domain2Name)
+
+	resource.ParallelTest(t, resource.TestCase{
+		PreCheck:                 func() { provider.TestAccPreCheck(t) },
+		ProtoV6ProviderFactories: provider.TestAccProtoV6ProviderFactories,
+		Steps: []resource.TestStep{
+			// Create and Read testing
+			{
+				Config: configCreate,
+			},
+			// Update and Read testing
+			{
+				Config: configUpdate,
+				Check: resource.ComposeAggregateTestCheckFunc(
+					resource.TestCheckResourceAttr("fastly_service_vcl.test", "last_active", "1"), // we expect `version` to drift from `last_active`
+					resource.TestCheckResourceAttr("fastly_service_vcl.test", "version", "2"),
+				),
+			},
+			// ImportState testing
+			{
+				ResourceName:            "fastly_service_vcl.test",
+				ImportState:             true,
+				ImportStateVerify:       true,
+				ImportStateVerifyIgnore: []string{"activate", "domain", "force_destroy", "version"},
+				ImportStateCheck: func(is []*terraform.InstanceState) error {
+					for _, s := range is {
+						// In this test case, when we're importing a service we expect
+						// `activate = true` and so we expect to pull in the last active
+						// service version.
+						//
+						// FIXME: this ^^ isn't accurate! We might well want to import a
+						// service that doesn't have an active version. So we need to handle
+						// this and make sure if when importing, we can't find an active
+						// version that we fallback to whatever the latest version is and
+						// also have a test to validate that behaviour.
+						if version, ok := s.Attributes["version"]; ok && version != "1" {
+							return fmt.Errorf("import failed: expected service version 1 (the last active): got %s", version)
+						}
+						if numDomains, ok := s.Attributes["domains.%"]; ok {
+							if numDomains != "2" {
+								return fmt.Errorf("import failed: unexpected number of domains found: got %s, want 2", numDomains)
+							}
+						}
+					}
+					return nil
+				},
+			},
+			// Delete testing automatically occurs in TestCase
+		},
+	})
+}
+
 type configServiceVCLCreateOpts struct {
 	activate, forceDestroy                bool
 	serviceName, domain1Name, domain2Name string