Missing return after AddError

[punkeel]

Affected resource(s)

• internal/framework/service/list_item/resource.go
• internal/framework/service/hyperdrive_config/resource.go

Words

We found a bug in the Turnstile resource: AddError is called without a return. The State is then saved, and not properly validated by Terraform... leading to corrupted state.

I wrote a quick (and dirty) Semgrep rule to try to catch it:

rules:
  - id: go-resp-diagnostics-add-error-no-return
    languages:
      - go
    severity: ERROR
    message: Calls to resp.Diagnostics.AddError should be directly followed by a
      return statement
    patterns:
      - pattern: |-
          if ... {
            ...
            resp.Diagnostics.AddError(...)
            ...
          }
      - pattern-not: |-
          if ... {
            ...
            resp.Diagnostics.AddError(...)
            ...
            return
          }

It's not perfect, but it did find the following two files:

    internal/framework/service/hyperdrive_config/resource.go
   ❯❯❱ go-resp-diagnostics-add-error-no-return
          Calls to resp.Diagnostics.AddError should be directly followed by a return statement

          168┆ if err != nil {
          169┆   resp.Diagnostics.AddError("Error deleting hyperdrive config", err.Error())
          170┆ }
            ⋮┆----------------------------------------
          175┆ if len(idParts) != 2 {
          176┆   resp.Diagnostics.AddError("Error importing hyperdrive config", "Invalid ID specified.
               Please specify the ID as \"accounts_id/hyperdrive_config_id\"")
          177┆ }

    internal/framework/service/list_item/resource.go
   ❯❯❱ go-resp-diagnostics-add-error-no-return
          Calls to resp.Diagnostics.AddError should be directly followed by a return statement

           87┆ if err != nil {
           88┆   resp.Diagnostics.AddError(fmt.Sprintf("failed reading List Item with ID: %q", data.ID),
               err.Error())
           89┆ }
            ⋮┆----------------------------------------
          110┆ if err != nil {
          111┆   resp.Diagnostics.AddError(fmt.Sprintf("failed reading List Item with ID: %q", data.ID),
               err.Error())
          112┆ }

Hyperdrive #1

terraform-provider-cloudflare/internal/framework/service/hyperdrive_config/resource.go

Lines 168 to 170 in ba46db6

	if err != nil {
	resp.Diagnostics.AddError("Error deleting hyperdrive config", err.Error())
	}

This looks like a false positive to me.

Hyperdrive #2

terraform-provider-cloudflare/internal/framework/service/hyperdrive_config/resource.go

Lines 175 to 177 in ba46db6

	if len(idParts) != 2 {
	resp.Diagnostics.AddError("Error importing hyperdrive config", "Invalid ID specified. Please specify the ID as \"accounts_id/hyperdrive_config_id\"")
	}

Real finding?

"List item??" #1

terraform-provider-cloudflare/internal/framework/service/list_item/resource.go

Lines 87 to 90 in ba46db6

	if err != nil {
	resp.Diagnostics.AddError(fmt.Sprintf("failed reading List Item with ID: %q", data.ID), err.Error())
	}
	resp.Diagnostics.Append(resp.State.Set(ctx, listItem)...)

Real finding?

"List item??" #2

terraform-provider-cloudflare/internal/framework/service/list_item/resource.go

Lines 110 to 112 in ba46db6

	if err != nil {
	resp.Diagnostics.AddError(fmt.Sprintf("failed reading List Item with ID: %q", data.ID), err.Error())
	}

Real finding?

I don't own these files (and I'm really no tf expert) - could someone take a look if these are worth fixing?

[github-actions]

Community Note

Voting for Prioritization

• Please vote on this issue by adding a 👍 reaction to the original post to help the community and maintainers prioritize this request.
• Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request.

Volunteering to Work on This Issue

• If you are interested in working on this issue, please leave a comment.
• If this would be your first contribution, please review the contribution guide.

[github-actions]

Thank you for reporting this issue! For maintainers to dig into issues it is required that all issues include the entirety of TF_LOG=DEBUG output to be provided. The only parts that should be redacted are your user credentials in the X-Auth-Key, X-Auth-Email and Authorization HTTP headers. Details such as zone or account identifiers are not considered sensitive but can be redacted if you are very cautious. This log file provides additional context from Terraform, the provider and the Cloudflare API that helps in debugging issues. Without it, maintainers are very limited in what they can do and may hamper diagnosis efforts.

This issue has been marked with triage/needs-information and is unlikely to receive maintainer attention until the log file is provided making this a complete bug report.

[github-actions]

Thank you for reporting this issue! For maintainers to dig into issues it is required that all issues include the entirety of TF_LOG=DEBUG output to be provided. The only parts that should be redacted are your user credentials in the X-Auth-Key, X-Auth-Email and Authorization HTTP headers. Details such as zone or account identifiers are not considered sensitive but can be redacted if you are very cautious. This log file provides additional context from Terraform, the provider and the Cloudflare API that helps in debugging issues. Without it, maintainers are very limited in what they can do and may hamper diagnosis efforts.

This issue has been marked with triage/needs-information and is unlikely to receive maintainer attention until the log file is provided making this a complete bug report.

[jacobbednarz]

i touched on this in #3284 but i'm not sure if we want this to be universally applied. should these be somewhat recoverable, we should instead be setting the correct state values to be saved instead of halting execution. for me, it really comes down to how important these failures are and whether we can handle them more gracefully or if we need to totally just drop and bail completely.