From a3185c734f372a65576b5e68e7382e482f17c243 Mon Sep 17 00:00:00 2001 From: Integralist Date: Wed, 13 Dec 2023 17:13:18 +0000 Subject: [PATCH] fix: potential nil deference bugs (nilaway) --- internal/helpers/errors.go | 2 + .../resources/servicevcl/process_create.go | 11 +++++ .../resources/servicevcl/process_delete.go | 5 ++ .../resources/servicevcl/process_read.go | 30 ++++++++---- .../resources/servicevcl/process_update.go | 47 +++++++++++++++---- 5 files changed, 77 insertions(+), 18 deletions(-) diff --git a/internal/helpers/errors.go b/internal/helpers/errors.go index 16c3db7..17860a2 100644 --- a/internal/helpers/errors.go +++ b/internal/helpers/errors.go @@ -7,6 +7,8 @@ const ( ErrorAPIClient = "API Client Error" // ErrorProvider indicates a Provider error. ErrorProvider = "Provider Error" + // ErrorTerraformPointer indicates a Terraform error populating a pointer. + ErrorTerraformPointer = "Terraform Pointer Error" // ErrorUnknown indicates an error incompatible with any other known scenario. ErrorUnknown = "Unknown Error" // ErrorUser indicates a User error. diff --git a/internal/provider/resources/servicevcl/process_create.go b/internal/provider/resources/servicevcl/process_create.go index cf11e39..7841ce4 100644 --- a/internal/provider/resources/servicevcl/process_create.go +++ b/internal/provider/resources/servicevcl/process_create.go @@ -45,6 +45,11 @@ func (r *Resource) Create(ctx context.Context, req resource.CreateRequest, resp if resp.Diagnostics.HasError() { return } + if plan == nil { + tflog.Trace(ctx, helpers.ErrorTerraformPointer, map[string]any{"req": req, "resp": resp}) + resp.Diagnostics.AddError(helpers.ErrorTerraformPointer, "nil pointer after plan population") + return + } plan.ID = types.StringValue(serviceID) plan.Version = types.Int64Value(int64(serviceVersion)) @@ -90,6 +95,12 @@ func createService( if resp.Diagnostics.HasError() { return "", 0, errors.New("failed to read Terraform plan") } + if plan == nil { + tflog.Trace(ctx, helpers.ErrorTerraformPointer, map[string]any{"req": req, "resp": resp}) + msg := "nil pointer after plan population" + resp.Diagnostics.AddError(helpers.ErrorTerraformPointer, msg) + return "", 0, fmt.Errorf("%s: %s", helpers.ErrorTerraformPointer, msg) + } clientReq := api.Client.ServiceAPI.CreateService(api.ClientCtx) clientReq.Comment(plan.Comment.ValueString()) diff --git a/internal/provider/resources/servicevcl/process_delete.go b/internal/provider/resources/servicevcl/process_delete.go index 7ab198c..ebc536d 100644 --- a/internal/provider/resources/servicevcl/process_delete.go +++ b/internal/provider/resources/servicevcl/process_delete.go @@ -24,6 +24,11 @@ func (r *Resource) Delete(ctx context.Context, req resource.DeleteRequest, resp if resp.Diagnostics.HasError() { return } + if state == nil { + tflog.Trace(ctx, helpers.ErrorTerraformPointer, map[string]any{"req": req, "resp": resp}) + resp.Diagnostics.AddError(helpers.ErrorTerraformPointer, "nil pointer after state population") + return + } if state.ForceDestroy.ValueBool() || state.Reuse.ValueBool() { clientReq := r.client.ServiceAPI.GetServiceDetail(r.clientCtx, state.ID.ValueString()) diff --git a/internal/provider/resources/servicevcl/process_read.go b/internal/provider/resources/servicevcl/process_read.go index 4cd310e..3fd5a71 100644 --- a/internal/provider/resources/servicevcl/process_read.go +++ b/internal/provider/resources/servicevcl/process_read.go @@ -27,6 +27,11 @@ func (r *Resource) Read(ctx context.Context, req resource.ReadRequest, resp *res if resp.Diagnostics.HasError() { return } + if state == nil { + tflog.Trace(ctx, helpers.ErrorTerraformPointer, map[string]any{"req": req, "resp": resp}) + resp.Diagnostics.AddError(helpers.ErrorTerraformPointer, "nil pointer after state population") + return + } clientReq := r.client.ServiceAPI.GetServiceDetail(r.clientCtx, state.ID.ValueString()) clientResp, httpResp, err := clientReq.Execute() @@ -105,16 +110,7 @@ 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(remoteServiceVersion) - - // We set `last_active` to align with `version` only if `activate=true`. - // We only expect `version` to drift from `last_active` if `activate=false`. - if state.Activate.ValueBool() { - state.LastActive = types.Int64Value(remoteServiceVersion) - } + setServiceState(state, clientResp, remoteServiceVersion) err = readServiceSettings(ctx, remoteServiceVersion, state, resp, api) if err != nil { @@ -224,6 +220,20 @@ func getLatestServiceVersion(i int, versions []fastly.SchemasVersionResponse) in return int64(versions[i].GetNumber()) } +// setServiceState mutates the resource state with service data from the API. +func setServiceState(state *models.ServiceVCL, clientResp *fastly.ServiceDetail, remoteServiceVersion int64) { + state.Comment = types.StringValue(clientResp.GetComment()) + state.ID = types.StringValue(clientResp.GetID()) + state.Name = types.StringValue(clientResp.GetName()) + state.Version = types.Int64Value(remoteServiceVersion) + + // We set `last_active` to align with `version` only if `activate=true`. + // We only expect `version` to drift from `last_active` if `activate=false`. + if state.Activate.ValueBool() { + state.LastActive = types.Int64Value(remoteServiceVersion) + } +} + func readServiceSettings(ctx context.Context, serviceVersion int64, state *models.ServiceVCL, resp *resource.ReadResponse, api helpers.API) error { serviceID := state.ID.ValueString() clientReq := api.Client.SettingsAPI.GetServiceSettings(api.ClientCtx, serviceID, int32(serviceVersion)) diff --git a/internal/provider/resources/servicevcl/process_update.go b/internal/provider/resources/servicevcl/process_update.go index 1ee529b..564fcc9 100644 --- a/internal/provider/resources/servicevcl/process_update.go +++ b/internal/provider/resources/servicevcl/process_update.go @@ -30,12 +30,22 @@ func (r *Resource) Update(ctx context.Context, req resource.UpdateRequest, resp if resp.Diagnostics.HasError() { return } + if plan == nil { + tflog.Trace(ctx, helpers.ErrorTerraformPointer, map[string]any{"req": req, "resp": resp}) + resp.Diagnostics.AddError(helpers.ErrorTerraformPointer, "nil pointer after plan population") + return + } var state *models.ServiceVCL resp.Diagnostics.Append(req.State.Get(ctx, &state)...) if resp.Diagnostics.HasError() { return } + if state == nil { + tflog.Trace(ctx, helpers.ErrorTerraformPointer, map[string]any{"req": req, "resp": resp}) + resp.Diagnostics.AddError(helpers.ErrorTerraformPointer, "nil pointer after state population") + return + } // NOTE: The plan data doesn't contain computed attributes. // So we need to read it from the current state. @@ -79,17 +89,11 @@ func (r *Resource) Update(ctx context.Context, req resource.UpdateRequest, resp } if nestedResourcesChanged && plan.Activate.ValueBool() { - clientReq := r.client.VersionAPI.ActivateServiceVersion(r.clientCtx, plan.ID.ValueString(), serviceVersion) - clientResp, httpResp, err := clientReq.Execute() + latestVersion, err := activateService(ctx, plan.ID.ValueString(), serviceVersion, r, resp) if err != nil { - tflog.Trace(ctx, "Fastly VersionAPI.ActivateServiceVersion error", map[string]any{"http_resp": httpResp}) - resp.Diagnostics.AddError(helpers.ErrorAPIClient, fmt.Sprintf("Unable to activate service version %d, got error: %s", 1, err)) return } - defer httpResp.Body.Close() - - // Only set LastActive if we successfully activate the service. - plan.LastActive = types.Int64Value(int64(clientResp.GetNumber())) + plan.LastActive = types.Int64Value(latestVersion) } // NOTE: The service attributes (Name, Comment) are 'versionless'. @@ -108,6 +112,10 @@ func (r *Resource) Update(ctx context.Context, req resource.UpdateRequest, resp } func updateServiceSettings(ctx context.Context, plan *models.ServiceVCL, diags diag.Diagnostics, api helpers.API) error { + if plan == nil { + return fmt.Errorf("unexpected nil for pointer argument type: %T", plan) + } + serviceID := plan.ID.ValueString() serviceVersion := int32(plan.Version.ValueInt64()) @@ -145,6 +153,25 @@ func updateServiceSettings(ctx context.Context, plan *models.ServiceVCL, diags d return nil } +// activateService activates the service and updates the plan's LastActive. +func activateService( + ctx context.Context, + serviceID string, + serviceVersion int32, + r *Resource, + resp *resource.UpdateResponse, +) (int64, error) { + clientReq := r.client.VersionAPI.ActivateServiceVersion(r.clientCtx, serviceID, serviceVersion) + clientResp, httpResp, err := clientReq.Execute() + if err != nil { + tflog.Trace(ctx, "Fastly VersionAPI.ActivateServiceVersion error", map[string]any{"http_resp": httpResp}) + resp.Diagnostics.AddError(helpers.ErrorAPIClient, fmt.Sprintf("Unable to activate service version %d, got error: %s", 1, err)) + return 0, err + } + defer httpResp.Body.Close() + return int64(clientResp.GetNumber()), nil +} + func determineChangesInNestedResources( ctx context.Context, nestedResources []interfaces.Resource, @@ -194,6 +221,10 @@ func updateServiceAttributes( api helpers.API, state *models.ServiceVCL, ) error { + if plan == nil || resp == nil || state == nil { + return errors.New("unexpected nil for pointer argument type") + } + // NOTE: UpdateService doesn't take a version because its attributes are versionless. clientReq := api.Client.ServiceAPI.UpdateService(api.ClientCtx, plan.ID.ValueString()) if !plan.Comment.Equal(state.Comment) {