Repository: honeycombio/terraform-provider-honeycombio (language: python)

Issue:
PagerDuty notification recipients don't handle default computed value
**Versions**

0.15.0 or newer

**Steps to reproduce**

Effects `honeycombio_trigger` resources.

If a configured recipient (by ID or Type) is a PagerDuty recipient and doesn't provide a severity in the optional `notification_details` block, 

```hcl
data "honeycombio_query_specification" "test" {
  calculation {
    op     = "AVG"
    column = "duration_ms"
  }
  time_range = 1200
}

resource "honeycombio_query" "test" {
  dataset    = var.dataset
  query_json = data.honeycombio_query_specification.test.json
}

resource "honeycombio_pagerduty_recipient" "test" {
  integration_key  = var.pagerduty_integration_key
  integration_name = "severity-repro"
}

resource "honeycombio_trigger" "test" {
  name    = "Test trigger from terraform-provider-honeycombio"
  dataset = var.dataset

  description = "My nice description"

  query_id = honeycombio_query.test.id

  threshold {
    op    = ">"
    value = 100
  }

  frequency = data.honeycombio_query_specification.test.time_range / 2

  recipient {
    id = honeycombio_pagerduty_recipient.test.id
  }
}
```

Running the code above produces the following error on `apply`:

```
Error: Provider produced inconsistent result after apply
        
When applying changes to honeycombio_trigger.test, provider
"provider[\"registry.terraform.io/hashicorp/honeycombio\"]" produced an
unexpected new value: .recipient: planned set element
cty.ObjectVal(map[string]cty.Value{"id":cty.StringVal("FTh7K9xqTgS"),
"notification_details":cty.ListValEmpty(cty.Object(map[string]cty.Type{"pagerduty_severity":cty.String})),
"target":cty.UnknownVal(cty.String), "type":cty.UnknownVal(cty.String)}) does
not correlate with any element in actual.
```

**Workaround**

Until this is resolved, all PagerDuty `recipient` blocks should have `notification_details` provided -- even if you're relying on the default severity level of `critical`.

```hcl
  recipient {
    id = honeycombio_pagerduty_recipient.test.id

    notification_details {
      pagerduty_severity = "critical"
    }
  }
```

**Additional context**

Introduced by #306

Updating the custom [plan modifier](https://developer.hashicorp.com/terraform/plugin/framework/migrating/resources/plan-modification) for the `recipient` block should solve resolve this "computed default sometimes" case.

Fix missing recipient panic
It looks like these missing returns are an oversight from #311?

<!--
Thank you for contributing to the project! 💜
Please see our [OSS process document](https://github.com/honeycombio/home/blob/main/honeycomb-oss-lifecycle-and-practices.md#) to get an idea of how we operate.
-->

## Which problem is this PR solving?

No bug, but two panics we've observed:

```text
panic: runtime error: index out of range [-1]

goroutine 123 [running]:
github.com/honeycombio/terraform-provider-honeycombio/internal/provider.(*triggerResource).Update(0xc000120f30, {0xeb5980, 0xc0001f6630}, {{{{0xeba508, 0xc000726e10}, {0xc679a0, 0xc000329a70}}, {0xebbd28, 0xc0002da7d0}}, {{{0xeba508, ...}, ...}, ...}, ...}, ...)
	github.com/honeycombio/terraform-provider-honeycombio/internal/provider/trigger_resource.go:477 +0xe1b
github.com/hashicorp/terraform-plugin-framework/internal/fwserver.(*Server).UpdateResource(0xc000161b80, {0xeb5980, 0xc0001f6630}, 0xc000543188, 0xc000775098)
	github.com/hashicorp/terraform-plugin-framework@v1.2.0/internal/fwserver/server_updateresource.go:118 +0x731
github.com/hashicorp/terraform-plugin-framework/internal/fwserver.(*Server).ApplyResourceChange(0xeb5980?, {0xeb5980, 0xc0001f6630}, 0xc000537720, 0xc0007752b8)
	github.com/hashicorp/terraform-plugin-framework@v1.2.0/internal/fwserver/server_applyresourcechange.go:99 +0x1a5
github.com/hashicorp/terraform-plugin-framework/internal/proto5server.(*Server).ApplyResourceChange(0xc000161b80, {0xeb5980?, 0xc0001f64e0?}, 0xc0005376d0)
	github.com/hashicorp/terraform-plugin-framework@v1.2.0/internal/proto5server/server_applyresourcechange.go:52 +0x27b
github.com/hashicorp/terraform-plugin-mux/tf5muxserver.muxServer.ApplyResourceChange({0xc000239f50, 0xc000239fb0, {0xc000133c20, 0x2, 0x2}, {0x0, 0x0, 0x0}, {0x0, 0x0, ...}, ...}, ...)
	github.com/hashicorp/terraform-plugin-mux@v0.10.0/tf5muxserver/mux_server_ApplyResourceChange.go:27 +0x102

Error: The terraform-provider-honeycombio_v0.15.1 plugin crashed!
```

```text
panic: runtime error: index out of range [-1]

goroutine 1257 [running]:
github.com/honeycombio/terraform-provider-honeycombio/internal/provider.(*triggerResource).Read(0xc000ac4108, {0x1ab5a60, 0xc0006465a0}, {{{{0x1aba668, 0xc0007c6780}, {0x1867800, 0xc0001c1860}}, {0x1abbe88, 0xc0003be500}}, 0xc000ac4158, ...}, ...)
        github.com/honeycombio/terraform-provider-honeycombio/internal/provider/trigger_resource.go:397 +0x9af
github.com/hashicorp/terraform-plugin-framework/internal/fwserver.(*Server).ReadResource(0xc00012f760, {0x1ab5a60, 0xc0006465a0}, 0xc000646690, 0xc0006e73f8)
        github.com/hashicorp/terraform-plugin-framework@v1.2.0/internal/fwserver/server_readresource.go:97 +0x617
github.com/hashicorp/terraform-plugin-framework/internal/proto5server.(*Server).ReadResource(0xc00012f760, {0x1ab5a60?, 0xc000646450?}, 0xc0005adec0)
        github.com/hashicorp/terraform-plugin-framework@v1.2.0/internal/proto5server/server_readresource.go:53 +0x27b
github.com/hashicorp/terraform-plugin-mux/tf5muxserver.muxServer.ReadResource({0xc000311d10, 0xc000311d70, {0xc000117760, 0x2, 0x2}, {0x0, 0x0, 0x0}, {0x0, 0x0, ...}, ...}, ...)
        github.com/hashicorp/terraform-plugin-mux@v0.10.0/tf5muxserver/mux_server_ReadResource.go:26 +0x102
github.com/hashicorp/terraform-plugin-go/tfprotov5/tf5server.(*server).ReadResource(0xc0004800a0, {0x1ab5a60?, 0xc0009bd830?}, 0xc000870de0)
        github.com/hashicorp/terraform-plugin-go@v0.15.0/tfprotov5/tf5server/server.go:748 +0x4b1
github.com/hashicorp/terraform-plugin-go/tfprotov5/internal/tfplugin5._Provider_ReadResource_Handler({0x19273c0?, 0xc0004800a0}, {0x1ab5a60, 0xc0009bd830}, 0xc00031e230, 0x0)
        github.com/hashicorp/terraform-plugin-go@v0.15.0/tfprotov5/internal/tfplugin5/tfplugin5_grpc.pb.go:383 +0x170
google.golang.org/grpc.(*Server).processUnaryRPC(0xc0002921e0, {0x1abaa80, 0xc0002144e0}, 0xc00094e900, 0xc00048c810, 0x1fd3a90, 0x0)
        google.golang.org/grpc@v1.54.0/server.go:1345 +0xdf0
google.golang.org/grpc.(*Server).handleStream(0xc0002921e0, {0x1abaa80, 0xc0002144e0}, 0xc00094e900, 0x0)
        google.golang.org/grpc@v1.54.0/server.go:1722 +0xa2f
google.golang.org/grpc.(*Server).serveStreams.func1.2()
        google.golang.org/grpc@v1.54.0/server.go:966 +0x98
created by google.golang.org/grpc.(*Server).serveStreams.func1
        google.golang.org/grpc@v1.54.0/server.go:964 +0x28a

Error: The terraform-provider-honeycombio_v0.15.1 plugin crashed!
```

## Short description of the changes

When we identify a missing recipient, bail before we try to dereference a known-invalid index.

## How to verify that this has the expected result

Observe errors like the following in place of panics:

```text
╷
│ Error: Error Reading Honeycomb Trigger
│
│   with module.microservice_group.module.this["consumer_api"].module.response_time[0].honeycombio_trigger.this[0],
│   on ../platform-component-hc-alert/alert/alert.tf line 63, in resource "honeycombio_trigger" "this":
│   63: resource "honeycombio_trigger" "this" {
│
│ Could not find Recipient :snip in state
```


Discussion / hints:
We've ran into this too.

```
│ Error: Provider produced inconsistent result after apply
│ 
│ When applying changes to
│ module.wrathofcron.module.wrathofcron.honeycombio_trigger.triggers["critical_errors"],
│ provider "provider[\"registry.terraform.io/honeycombio/honeycombio\"]"
│ produced an unexpected new value: .recipient: planned set element
│ cty.ObjectVal(map[string]cty.Value{"id":cty.StringVal("9KiCzVXtxaj"),
│ "notification_details":cty.ListValEmpty(cty.Object(map[string]cty.Type{"pagerduty_severity":cty.String})),
│ "target":cty.UnknownVal(cty.String), "type":cty.UnknownVal(cty.String)})
│ does not correlate with any element in actual.
│ 
│ This is a bug in the provider, which should be reported in the provider's
│ own issue tracker.
```
We've ran into this too.

```
Error: Provider produced inconsistent result after apply
When applying changes to honeycombio_trigger.routing_canary_cedar_request_latency_warning["production"], provider "provider[\"registry.terraform.io/honeycombio/honeycombio\"]" produced an unexpected new value: .recipient: planned set element cty.ObjectVal(map[string]cty.Value{"id":cty.StringVal("r9skJ6ZBv1C"), "notification_details":cty.ListValEmpty(cty.Object(map[string]cty.Type{"pagerduty_severity":cty.String})), "target":cty.UnknownVal(cty.String), "type":cty.UnknownVal(cty.String)}) does not correlate with any element in actual.
This is a bug in the provider, which should be reported in the provider's own issue tracker.
```
Thanks @LukeCarrier! This looks pretty obviously correct, but we'd like to see a test for this as well. If you're interested in putting that in, that'd be great. If not, we'll get one added shortly and then get this merged.
Hey @dstrelau, I'm not really sure how to best catch this case, so feel free to add commits :+1:
(continuing a discussion from #335)

@jharley 

> As for the panic, that is another bug that we are aware of but I've yet to be given a reproducible case to trigger it. If you have such a thing I'd very much appreciate you sharing it so we can get that one squashed as well!

For me, this was a flaky thing, since it popped up with the exact same configuration I was using before to generate the original bug (#335). The panic appeared the next day, and is gone now, with no changes made to our recipients or anything.

Looking through the stacktrace, though, there's two issues:
* https://github.com/honeycombio/terraform-provider-honeycombio/blob/957dbdd7bbe957c6aa5967797717712827e6322b/internal/provider/trigger_resource.go#L397 should be skipped with `idx < 0`. I was about to PR this, but noticed #331 already caught it
* I dunno why exactly the recipient wasn't found, since it was using the same one as before. It could be something from my side, but I don't know what would've happened, since that recipient is actively used by other triggers, I was just adding a new trigger using an existing recipient. And additionally, we only use recipient IDs in the triggers. 

One question though: in `(s.Target.IsNull() || s.Target.ValueString() == r.Target)`, shouldn't the first condition be dropped? If the `ID` is null, the `Target` _must_ be specified, right? What if `r.Target` is specified, but `s.Target` is null?

If I come across the panic again, I'll check the state immediately to see what's going on, and report if it's relevant.

Fix patch:
diff --git a/internal/provider/trigger_resource.go b/internal/provider/trigger_resource.go
index b19c9b1a..ba8f46b8 100644
--- a/internal/provider/trigger_resource.go
+++ b/internal/provider/trigger_resource.go
@@ -258,7 +258,8 @@ func (r *triggerResource) Configure(_ context.Context, req resource.ConfigureReq
 }
 
 func (r *triggerResource) Create(ctx context.Context, req resource.CreateRequest, resp *resource.CreateResponse) {
-	var plan models.TriggerResourceModel
+	var plan, config models.TriggerResourceModel
+	resp.Diagnostics.Append(req.Config.Get(ctx, &config)...)
 	resp.Diagnostics.Append(req.Plan.Get(ctx, &plan)...)
 	if resp.Diagnostics.HasError() {
 		return
@@ -299,51 +300,8 @@ func (r *triggerResource) Create(ctx context.Context, req resource.CreateRequest
 	state.Threshold = flattenTriggerThreshold(trigger.Threshold)
 	state.Frequency = types.Int64Value(int64(trigger.Frequency))
 	state.EvaluationSchedule = flattenTriggerEvaluationSchedule(trigger)
-
-	recipients := make([]models.NotificationRecipientModel, len(trigger.Recipients))
-	for i, r := range trigger.Recipients {
-		var rcpt models.NotificationRecipientModel
-
-		// match the trigger's recipient to that in the plan
-		idx := slices.IndexFunc(plan.Recipients, func(s models.NotificationRecipientModel) bool {
-			if !s.ID.IsUnknown() {
-				return s.ID.ValueString() == r.ID
-			}
-			return s.Type.ValueString() == string(r.Type) && (s.Target.IsNull() || s.Target.ValueString() == r.Target)
-		})
-		if idx < 0 {
-			resp.Diagnostics.AddError(
-				"Error Creating Honeycomb Trigger",
-				"Could not find Recipient "+r.ID+" in plan",
-			)
-		}
-		rcpt = plan.Recipients[idx]
-
-		// TODO: can we move this to the planmodifier by adding a create state?
-		if !rcpt.ID.IsUnknown() {
-			// recipient provided by ID
-			rcpt.ID = types.StringValue(r.ID)
-			rcpt.Type = types.StringNull()
-			rcpt.Target = types.StringNull()
-		} else {
-			// recipient provided by type+target
-			rcpt.ID = types.StringNull()
-			rcpt.Type = types.StringValue(string(r.Type))
-			if rcpt.Type.ValueString() == string(client.RecipientTypePagerDuty) {
-				// PagerDuty recipients don't have a target
-				rcpt.Target = types.StringNull()
-			} else {
-				rcpt.Target = types.StringValue(r.Target)
-			}
-		}
-
-		if r.Type == client.RecipientTypePagerDuty && r.Details != nil {
-			rcpt.Details = make([]models.NotificationRecipientDetailsModel, 1)
-			rcpt.Details[0].PDSeverity = types.StringValue(string(r.Details.PDSeverity))
-		}
-		recipients[i] = rcpt
-	}
-	state.Recipients = recipients
+	// we created them as authored so to avoid matching type-target or ID we can just use the same value
+	state.Recipients = config.Recipients
 
 	resp.Diagnostics.Append(resp.State.Set(ctx, state)...)
 }
@@ -379,21 +337,22 @@ func (r *triggerResource) Read(ctx context.Context, req resource.ReadRequest, re
 
 	recipients := make([]models.NotificationRecipientModel, len(trigger.Recipients))
 	if state.Recipients != nil {
+		// match the Trigger's recipients to those in the state sorting out type+target vs ID
 		for i, r := range trigger.Recipients {
-			// match the Trigger's recipient to that in state
 			idx := slices.IndexFunc(state.Recipients, func(s models.NotificationRecipientModel) bool {
 				if !s.ID.IsNull() {
 					return s.ID.ValueString() == r.ID
 				}
-				return s.Type.ValueString() == string(r.Type) && (s.Target.IsNull() || s.Target.ValueString() == r.Target)
+				return s.Type.ValueString() == string(r.Type) && s.Target.ValueString() == r.Target
 			})
 			if idx < 0 {
+				// this should never happen?! But if it does, we'll just skip it and hope to get a reproducable case
 				resp.Diagnostics.AddError(
 					"Error Reading Honeycomb Trigger",
 					"Could not find Recipient "+r.ID+" in state",
 				)
+				continue
 			}
-
 			recipients[i] = state.Recipients[idx]
 		}
 	} else {
@@ -405,7 +364,8 @@ func (r *triggerResource) Read(ctx context.Context, req resource.ReadRequest, re
 }
 
 func (r *triggerResource) Update(ctx context.Context, req resource.UpdateRequest, resp *resource.UpdateResponse) {
-	var plan models.TriggerResourceModel
+	var plan, config models.TriggerResourceModel
+	resp.Diagnostics.Append(req.Config.Get(ctx, &config)...)
 	resp.Diagnostics.Append(req.Plan.Get(ctx, &plan)...)
 	if resp.Diagnostics.HasError() {
 		return
@@ -458,25 +418,8 @@ func (r *triggerResource) Update(ctx context.Context, req resource.UpdateRequest
 	state.Frequency = types.Int64Value(int64(trigger.Frequency))
 	state.Threshold = flattenTriggerThreshold(trigger.Threshold)
 	state.EvaluationSchedule = flattenTriggerEvaluationSchedule(trigger)
-
-	recipients := make([]models.NotificationRecipientModel, len(trigger.Recipients))
-	for i, r := range trigger.Recipients {
-		// match the Trigger's recipient to that in the plan
-		idx := slices.IndexFunc(plan.Recipients, func(s models.NotificationRecipientModel) bool {
-			if !s.ID.IsNull() {
-				return s.ID.ValueString() == r.ID
-			}
-			return s.Type.ValueString() == string(r.Type) && (s.Target.IsNull() || s.Target.ValueString() == r.Target)
-		})
-		if idx < 0 {
-			resp.Diagnostics.AddError(
-				"Error Updating Honeycomb Trigger",
-				"Could not find Recipient "+r.ID+" in plan",
-			)
-		}
-		recipients[i] = plan.Recipients[idx]
-	}
-	state.Recipients = recipients
+	// we created them as authored so to avoid matching type-target or ID we can just use the same value
+	state.Recipients = config.Recipients
 
 	resp.Diagnostics.Append(resp.State.Set(ctx, state)...)
 }


Test patch:
diff --git a/internal/provider/trigger_resource_test.go b/internal/provider/trigger_resource_test.go
index 9a4e0931..94929358 100644
--- a/internal/provider/trigger_resource_test.go
+++ b/internal/provider/trigger_resource_test.go
@@ -69,6 +69,7 @@ func TestAcc_TriggerResourceUpgradeFromVersion014(t *testing.T) {
 			{
 				ProtoV5ProviderFactories: testAccProtoV5MuxServerFactory,
 				Config:                   config,
+				PlanOnly:                 true,
 			},
 		},
 	})
@@ -352,8 +353,6 @@ resource "honeycombio_trigger" "test" {
 }
 
 func TestAcc_TriggerResourcePagerDutyUnsetSeverity(t *testing.T) {
-	t.Skip("known issue: see https://github.com/honeycombio/terraform-provider-honeycombio/issues/309")
-
 	dataset := testAccDataset()
 
 	resource.Test(t, resource.TestCase{
@@ -390,7 +389,7 @@ resource "honeycombio_trigger" "test" {
 
   threshold {
     op    = ">"
-    value = 100
+    value = 1 - 0.99
   }
 
   frequency = data.honeycombio_query_specification.test.time_range / 2
@@ -405,49 +404,6 @@ resource "honeycombio_trigger" "test" {
 	})
 }
 
-func TestAcc_TriggerResourceThresholdFloatingPointBug(t *testing.T) {
-	// reported as https://github.com/honeycombio/terraform-provider-honeycombio/issues/335
-	dataset := testAccDataset()
-
-	resource.Test(t, resource.TestCase{
-		PreCheck:                 testAccPreCheck(t),
-		ProtoV5ProviderFactories: testAccProtoV5MuxServerFactory,
-		Steps: []resource.TestStep{
-			{
-				Config: fmt.Sprintf(`
-data "honeycombio_query_specification" "test" {
-  calculation {
-    op     = "COUNT"
-  }
-
-  time_range = 1200
-}
-
-resource "honeycombio_query" "test" {
-  dataset    = "%[1]s"
-  query_json = data.honeycombio_query_specification.test.json
-}
-
-resource "honeycombio_trigger" "test" {
-  name = "floating point test"
-
-  dataset  = "%[1]s"
-  query_id = honeycombio_query.test.id
-
-  threshold {
-    op    = ">"
-    value = 1 - 0.99
-  }
-}
-`, dataset),
-				Check: resource.ComposeTestCheckFunc(
-					testAccEnsureTriggerExists(t, "honeycombio_trigger.test"),
-				),
-			},
-		},
-	})
-}
-
 func testAccConfigBasicTriggerTest(dataset, pdseverity string) string {
 	return fmt.Sprintf(`
 data "honeycombio_query_specification" "test" {