Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More gracefully reject colonCompoundIDs that aren't compound IDs #762

Merged

Conversation

cttttt
Copy link
Contributor

@cttttt cttttt commented Oct 30, 2023

What's the problem?

Attempts to plan a resource import fail with a panic where the provider expects special colon delimited resource IDs ... that don't contain a colon (whoops). An example of such a resource is a team_membership, whose ID is a colon separated userID and teamID.

These changes help the provider more gracefully fail on these user errors instead of causing the provider to panic.

Specifically

A plan with the following Terraform configuration (IDs redacted):

terraform {
  required_providers {
    pagerduty = {
      source  = "pagerduty/pagerduty"
      version = "~> 3.0.1"
    }
  }
}

resource "pagerduty_team_membership" "this" {
  user_id = "PUUSERH"
  team_id = "PTEAMNP"
}

import {
  id = "PUUSERHPTEAMP"
  to = pagerduty_team_membership.this
}

...results in the following panic:

$ terraform plan
pagerduty_team_membership.this: Preparing import... [id=PUSERHPTEAMNP]
pagerduty_team_membership.this: Refreshing state... [id=PUSERHPTEAMNP]

Planning failed. Terraform encountered an error while generating this plan.

╷
│ Error: Plugin did not respond
│ 
│ The plugin encountered an error, and failed to respond to the plugin.(*GRPCProvider).ReadResource
│ call. The plugin logs may contain more details.
╵

Stack trace from the terraform-provider-pagerduty_v3.0.3 plugin:

panic: runtime error: index out of range [1] with length 1

goroutine 39 [running]:
github.com/terraform-providers/terraform-provider-pagerduty/pagerduty.resourcePagerDutyParseColonCompound
ID(...)
        github.com/terraform-providers/terraform-provider-pagerduty/pagerduty/util.go:267
github.com/terraform-providers/terraform-provider-pagerduty/pagerduty.fetchPagerDutyTeamMembership(0x1400
0149980, {0x1054ad060, 0x1400042ff40}, 0x1055bf298)
        github.com/terraform-providers/terraform-provider-pagerduty/pagerduty/resource_pagerduty_team_mem
bership.go:85 +0x324
github.com/terraform-providers/terraform-provider-pagerduty/pagerduty.resourcePagerDutyTeamMembershipRead
(0x14000149980, {0x1054ad060, 0x1400042ff40})
        github.com/terraform-providers/terraform-provider-pagerduty/pagerduty/resource_pagerduty_team_mem
bership.go:155 +0x44
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*Resource).read(0x14000480540, {0x1055e8300, 
0x1400020b3b0}, 0x14000149980, {0x1054ad060, 0x1400042ff40})
        github.com/hashicorp/terraform-plugin-sdk/v2@v2.11.0/helper/schema/resource.go:347 +0x170
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*Resource).RefreshWithoutUpgrade(0x1400048054
0, {0x1055e8300, 0x1400020b3b0}, 0x14000210dd0, {0x1054ad060, 0x1400042ff40})
        github.com/hashicorp/terraform-plugin-sdk/v2@v2.11.0/helper/schema/resource.go:650 +0x478
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*GRPCProviderServer).ReadResource(0x14000123a
b8, {0x1055e8258, 0x1400058aac0}, 0x1400058ab40)
        github.com/hashicorp/terraform-plugin-sdk/v2@v2.11.0/helper/schema/grpc_provider.go:613 +0x618
github.com/hashicorp/terraform-plugin-go/tfprotov5/tf5server.(*server).ReadResource(0x14000439400, {0x105
5e8300, 0x1400020ab70}, 0x14000512a80)
        github.com/hashicorp/terraform-plugin-go@v0.8.0/tfprotov5/tf5server/server.go:746 +0x4a0
github.com/hashicorp/terraform-plugin-go/tfprotov5/internal/tfplugin5._Provider_ReadResource_Handler({0x1
055843e0, 0x14000439400}, {0x1055e8300, 0x1400020ab70}, 0x14000512a20, 0x0)
        github.com/hashicorp/terraform-plugin-go@v0.8.0/tfprotov5/internal/tfplugin5/tfplugin5_grpc.pb.go
:349 +0x1c0
google.golang.org/grpc.(*Server).processUnaryRPC(0x140001348c0, {0x1055f8a58, 0x14000469ba0}, 0x140004119
e0, 0x14000492fc0, 0x105c448b0, 0x0)
        google.golang.org/grpc@v1.45.0/server.go:1282 +0xc5c
google.golang.org/grpc.(*Server).handleStream(0x140001348c0, {0x1055f8a58, 0x14000469ba0}, 0x140004119e0,
 0x0)
        google.golang.org/grpc@v1.45.0/server.go:1619 +0xa34
google.golang.org/grpc.(*Server).serveStreams.func1.2(0x140004c83b0, 0x140001348c0, {0x1055f8a58, 0x14000
469ba0}, 0x140004119e0)
        google.golang.org/grpc@v1.45.0/server.go:921 +0x94
created by google.golang.org/grpc.(*Server).serveStreams.func1
        google.golang.org/grpc@v1.45.0/server.go:919 +0x1f0

Error: The terraform-provider-pagerduty_v3.0.3 plugin crashed!

This is always indicative of a bug within the plugin. It would be immensely
helpful if you could report the crash with the plugin's maintainers so that it
can be fixed. The output above should help diagnose the issue.

How does this fix it?

These changes have the parser for compound resource IDs surface a descriptive error instead of causing panic.

The resulting plan becomes:

$ terraform plan
╷
│ Warning: Provider development overrides are in effect
│ 
│ The following provider development overrides are set in the CLI configuration:
│  - pagerduty/pagerduty in <provider path redacted>
│ 
│ The behavior may therefore not match any released version of the provider and applying changes may
│ cause the state to become incompatible with published releases.
╵
pagerduty_team_membership.this: Preparing import... [id=PUSERHPTEAMNP]
pagerduty_team_membership.this: Refreshing state... [id=PUSERHPTEAMNP]

Planning failed. Terraform encountered an error while generating this plan.

╷
│ Error: PUSERHPTEAMNP: expected colon compound ID to have at least two components
│ 
│ 
╵

Note to reviewers

Note that I attempt to maintain behaviour where >2 component IDs are passed in, even so we should technically fail for component IDs that don't have exactly two components. Not sure how much you want to focus the behaviour changes from bugfixes like this, but we can discuss.

@cttttt cttttt marked this pull request as ready for review October 31, 2023 11:39
Avoids an array out of bounds (panic) resulting from colon compound
resource IDs that have only one component (no colon).
@imjaroiswebdev imjaroiswebdev force-pushed the reject-invalid-compound-resource-ids branch from 5023f43 to 4f90ecf Compare December 7, 2023 22:16
@imjaroiswebdev imjaroiswebdev self-requested a review December 7, 2023 22:17
Copy link
Contributor

@imjaroiswebdev imjaroiswebdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @cttttt awesome! Thanks a lot for this valuable contribution 🎉 I took the liberty of rebasing for a clean merge. Good job! 💪🏽

@imjaroiswebdev imjaroiswebdev merged commit 8201be5 into PagerDuty:master Dec 7, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants