Skip to content

Commit

Permalink
fix: potential nil deference bugs (nilaway)
Browse files Browse the repository at this point in the history
  • Loading branch information
Integralist committed Dec 13, 2023
1 parent 71b132f commit a3185c7
Show file tree
Hide file tree
Showing 5 changed files with 77 additions and 18 deletions.
2 changes: 2 additions & 0 deletions internal/helpers/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
11 changes: 11 additions & 0 deletions internal/provider/resources/servicevcl/process_create.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down Expand Up @@ -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())
Expand Down
5 changes: 5 additions & 0 deletions internal/provider/resources/servicevcl/process_delete.go
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down
30 changes: 20 additions & 10 deletions internal/provider/resources/servicevcl/process_read.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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))
Expand Down
47 changes: 39 additions & 8 deletions internal/provider/resources/servicevcl/process_update.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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'.
Expand All @@ -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())

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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) {
Expand Down

0 comments on commit a3185c7

Please sign in to comment.