Repository: honeycombio/terraform-provider-honeycombio
Pull request #433 (2024-02-15 17:11:30)

fix(recipients): better handle the case where a recipient is modified outside of Terraform
# Summary

If a recipient was updated outside of Terraform (i.e. the UI or via the API in some other way) the provider would get rather cranky and spit out an error like:

> Could not find Recipient xysef83ryH in state

This adjusts that behaviour to instead accept that the recipient is defined remotely and allow the rest of Terraform's diffing process to handle the difference and present that plan to the user.

Effects `r/burn_alert` and `r/trigger`

---
- To see the specific tasks where the Asana app for GitHub is being used, see below:
  - https://app.asana.com/0/0/1206583960682330


Patch:
diff --git a/internal/provider/burn_alert_resource.go b/internal/provider/burn_alert_resource.go
index 20e6b29..665df9c 100644
--- a/internal/provider/burn_alert_resource.go
+++ b/internal/provider/burn_alert_resource.go
@@ -5,8 +5,6 @@ import (
 	"errors"
 	"strings"
 
-	"golang.org/x/exp/slices"
-
 	"github.com/honeycombio/terraform-provider-honeycombio/internal/helper/validation"
 
 	"github.com/hashicorp/terraform-plugin-framework-validators/float64validator"
@@ -316,6 +314,7 @@ func (r *burnAlertResource) Read(ctx context.Context, req resource.ReadRequest,
 	state.ID = types.StringValue(burnAlert.ID)
 	state.AlertType = types.StringValue(string(burnAlert.AlertType))
 	state.SLOID = types.StringValue(burnAlert.SLO.ID)
+	state.Recipients = reconcileReadNotificationRecipientState(burnAlert.Recipients, state.Recipients)
 
 	// Process any attributes that could be nil and add them to the state values
 	if burnAlert.ExhaustionMinutes != nil {
@@ -330,31 +329,6 @@ func (r *burnAlertResource) Read(ctx context.Context, req resource.ReadRequest,
 		state.BudgetRateWindowMinutes = types.Int64Value(int64(*burnAlert.BudgetRateWindowMinutes))
 	}
 
-	recipients := make([]models.NotificationRecipientModel, len(burnAlert.Recipients))
-	if state.Recipients != nil {
-		// match the recipients to those in the state sorting out type+target vs ID
-		for i, r := range burnAlert.Recipients {
-			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.ValueString() == r.Target
-			})
-			if idx < 0 {
-				// this should never happen?! But if it does, we'll just skip it and hope to get a reproducible case
-				resp.Diagnostics.AddError(
-					"Error Reading Honeycomb Burn Alert",
-					"Could not find Recipient "+r.ID+" in state",
-				)
-				continue
-			}
-			recipients[i] = state.Recipients[idx]
-		}
-	} else {
-		recipients = flattenNotificationRecipients(burnAlert.Recipients)
-	}
-	state.Recipients = recipients
-
 	// Set the burn alert's attributes in state
 	resp.Diagnostics.Append(resp.State.Set(ctx, state)...)
 }
diff --git a/internal/provider/notification_recipients.go b/internal/provider/notification_recipients.go
index 9036546..e07597e 100644
--- a/internal/provider/notification_recipients.go
+++ b/internal/provider/notification_recipients.go
@@ -11,6 +11,7 @@ import (
 	"github.com/hashicorp/terraform-plugin-framework/resource/schema/stringplanmodifier"
 	"github.com/hashicorp/terraform-plugin-framework/schema/validator"
 	"github.com/hashicorp/terraform-plugin-framework/types"
+	"golang.org/x/exp/slices"
 
 	"github.com/honeycombio/terraform-provider-honeycombio/client"
 	"github.com/honeycombio/terraform-provider-honeycombio/internal/helper"
@@ -84,6 +85,34 @@ func notificationRecipientSchema(allowedTypes []client.RecipientType) schema.Set
 	}
 }
 
+func reconcileReadNotificationRecipientState(remote []client.NotificationRecipient, state []models.NotificationRecipientModel) []models.NotificationRecipientModel {
+	if state == nil {
+		// if we don't have any state, we can't reconcile anything so just return the remote recipients
+		return flattenNotificationRecipients(remote)
+	}
+
+	recipients := make([]models.NotificationRecipientModel, len(remote))
+	// match the remote recipients to those in the state
+	// in an effort to preserve the id vs type+target distinction
+	for i, r := range remote {
+		idx := slices.IndexFunc(state, func(s models.NotificationRecipientModel) bool {
+			if !s.ID.IsNull() {
+				return s.ID.ValueString() == r.ID
+			}
+			return s.Type.ValueString() == string(r.Type) && s.Target.ValueString() == r.Target
+		})
+		if idx < 0 {
+			// if we didn't find a match, use the recipient as specified in remote
+			recipients[i] = notificationRecipientToModel(r)
+		} else {
+			// if we found a match, use the stored recipient
+			recipients[i] = state[idx]
+		}
+	}
+
+	return recipients
+}
+
 func expandNotificationRecipients(n []models.NotificationRecipientModel) []client.NotificationRecipient {
 	recipients := make([]client.NotificationRecipient, len(n))
 
@@ -106,19 +135,24 @@ func expandNotificationRecipients(n []models.NotificationRecipientModel) []clien
 
 func flattenNotificationRecipients(n []client.NotificationRecipient) []models.NotificationRecipientModel {
 	recipients := make([]models.NotificationRecipientModel, len(n))
-
 	for i, r := range n {
-		rcpt := models.NotificationRecipientModel{
-			ID:     types.StringValue(r.ID),
-			Type:   types.StringValue(string(r.Type)),
-			Target: types.StringValue(r.Target),
-		}
-		if r.Details != nil {
-			rcpt.Details = make([]models.NotificationRecipientDetailsModel, 1)
-			rcpt.Details[0].PDSeverity = types.StringValue(string(r.Details.PDSeverity))
-		}
-		recipients[i] = rcpt
+		recipients[i] = notificationRecipientToModel(r)
 	}
 
 	return recipients
 }
+
+func notificationRecipientToModel(r client.NotificationRecipient) models.NotificationRecipientModel {
+	rcpt := models.NotificationRecipientModel{
+		ID:     types.StringValue(r.ID),
+		Type:   types.StringValue(string(r.Type)),
+		Target: types.StringValue(r.Target),
+	}
+	if r.Details != nil {
+		rcpt.Details = []models.NotificationRecipientDetailsModel{
+			{PDSeverity: types.StringValue(string(r.Details.PDSeverity))},
+		}
+	}
+
+	return rcpt
+}
diff --git a/internal/provider/trigger_resource.go b/internal/provider/trigger_resource.go
index 3d3963b..d729e5e 100644
--- a/internal/provider/trigger_resource.go
+++ b/internal/provider/trigger_resource.go
@@ -6,8 +6,6 @@ import (
 	"regexp"
 	"strings"
 
-	"golang.org/x/exp/slices"
-
 	"github.com/hashicorp/terraform-plugin-framework-validators/int64validator"
 	"github.com/hashicorp/terraform-plugin-framework-validators/listvalidator"
 	"github.com/hashicorp/terraform-plugin-framework-validators/stringvalidator"
@@ -288,31 +286,7 @@ func (r *triggerResource) Read(ctx context.Context, req resource.ReadRequest, re
 	state.Threshold = flattenTriggerThreshold(trigger.Threshold)
 	state.Frequency = types.Int64Value(int64(trigger.Frequency))
 	state.EvaluationSchedule = flattenTriggerEvaluationSchedule(trigger)
-
-	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 {
-			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.ValueString() == r.Target
-			})
-			if idx < 0 {
-				// this should never happen?! But if it does, we'll just skip it and hope to get a reproducible case
-				resp.Diagnostics.AddError(
-					"Error Reading Honeycomb Trigger",
-					"Could not find Recipient "+r.ID+" in state",
-				)
-				continue
-			}
-			recipients[i] = state.Recipients[idx]
-		}
-	} else {
-		recipients = flattenNotificationRecipients(trigger.Recipients)
-	}
-	state.Recipients = recipients
+	state.Recipients = reconcileReadNotificationRecipientState(trigger.Recipients, state.Recipients)
 
 	resp.Diagnostics.Append(resp.State.Set(ctx, state)...)
 }


Test patch:
diff --git a/internal/provider/burn_alert_resource_test.go b/internal/provider/burn_alert_resource_test.go
index 64e9719..b3115f4 100644
--- a/internal/provider/burn_alert_resource_test.go
+++ b/internal/provider/burn_alert_resource_test.go
@@ -421,6 +421,46 @@ func TestAcc_BurnAlertResource_recreateOnNotFound(t *testing.T) {
 	})
 }
 
+func TestAcc_BurnAlertResource_HandlesRecipientChangedOutsideOfTerraform(t *testing.T) {
+	c := testAccClient(t)
+	ctx := context.Background()
+	dataset, sloID := burnAlertAccTestSetup(t)
+
+	// setup a slack recipient to be used in the burn alert, and modified outside of terraform
+	channel := "#" + acctest.RandString(8)
+	rcpt, err := c.Recipients.Create(ctx, &client.Recipient{
+		Type: client.RecipientTypeSlack,
+		Details: client.RecipientDetails{
+			SlackChannel: channel,
+		},
+	})
+	require.NoError(t, err, "failed to create test recipient")
+	t.Cleanup(func() {
+		//nolint:errcheck
+		c.Recipients.Delete(ctx, rcpt.ID)
+	})
+
+	resource.Test(t, resource.TestCase{
+		PreCheck:                 testAccPreCheck(t),
+		ProtoV5ProviderFactories: testAccProtoV5MuxServerFactory,
+		Steps: []resource.TestStep{
+			{
+				Config: testAccConfigBurnAlertWithSlackRecipient(dataset, sloID, channel),
+			},
+			{
+				PreConfig: func() {
+					// update the channel name outside of Terraform
+					channel += "-1"
+					rcpt.Details.SlackChannel = channel
+					_, err := c.Recipients.Update(ctx, rcpt)
+					require.NoError(t, err, "failed to update test recipient")
+				},
+				Config: testAccConfigBurnAlertWithSlackRecipient(dataset, sloID, channel),
+			},
+		},
+	})
+}
+
 // Checks that the exhaustion time burn alert exists, has the correct attributes, and has the correct state
 func testAccEnsureSuccessExhaustionTimeAlert(t *testing.T, burnAlert *client.BurnAlert, exhaustionMinutes int, pagerdutySeverity, sloID string) resource.TestCheckFunc {
 	return resource.ComposeAggregateTestCheckFunc(
@@ -764,3 +804,18 @@ resource "honeycombio_burn_alert" "test" {
   }
 }`, dataset, sloID)
 }
+
+func testAccConfigBurnAlertWithSlackRecipient(dataset, sloID, channel string) string {
+	return fmt.Sprintf(`
+resource "honeycombio_burn_alert" "test" {
+  exhaustion_minutes = 60
+
+  dataset = "%[1]s"
+  slo_id  = "%[2]s"
+
+  recipient {
+    type   = "slack"
+    target = "%[3]s"
+  }
+}`, dataset, sloID, channel)
+}
diff --git a/internal/provider/notification_recipients_test.go b/internal/provider/notification_recipients_test.go
new file mode 100644
index 0000000..a3e727c
--- /dev/null
+++ b/internal/provider/notification_recipients_test.go
@@ -0,0 +1,140 @@
+package provider
+
+import (
+	"testing"
+
+	"github.com/hashicorp/terraform-plugin-framework/types"
+	"github.com/stretchr/testify/assert"
+
+	"github.com/honeycombio/terraform-provider-honeycombio/client"
+	"github.com/honeycombio/terraform-provider-honeycombio/internal/models"
+)
+
+func Test_reconcileReadNotificationRecipientState(t *testing.T) {
+	type args struct {
+		remote []client.NotificationRecipient
+		state  []models.NotificationRecipientModel
+	}
+	tests := []struct {
+		name string
+		args args
+		want []models.NotificationRecipientModel
+	}{
+		{
+			name: "both empty",
+			args: args{},
+			want: []models.NotificationRecipientModel{},
+		},
+		{
+			name: "empty state",
+			args: args{
+				remote: []client.NotificationRecipient{
+					{ID: "abcd12345", Type: client.RecipientTypeEmail, Target: "test@example.com"},
+				},
+				state: []models.NotificationRecipientModel{},
+			},
+			want: []models.NotificationRecipientModel{
+				{ID: types.StringValue("abcd12345"), Type: types.StringValue("email"), Target: types.StringValue("test@example.com")},
+			},
+		},
+		{
+			name: "empty remote",
+			args: args{
+				remote: []client.NotificationRecipient{},
+				state: []models.NotificationRecipientModel{
+					{ID: types.StringValue("abcd12345"), Type: types.StringValue("email"), Target: types.StringValue("test@example.com")},
+				},
+			},
+			want: []models.NotificationRecipientModel{},
+		},
+		{
+			name: "remote and state reconciled",
+			args: args{
+				remote: []client.NotificationRecipient{
+					{ID: "abcd12345", Type: client.RecipientTypeEmail, Target: "test@example.com"},
+					{ID: "efgh67890", Type: client.RecipientTypeSlack, Target: "#test-channel"},
+				},
+				state: []models.NotificationRecipientModel{
+					{ID: types.StringValue("abcd12345")},                                           // defined by ID
+					{Type: types.StringValue("slack"), Target: types.StringValue("#test-channel")}, // defined by type+target
+				},
+			},
+			want: []models.NotificationRecipientModel{
+				{ID: types.StringValue("abcd12345")},
+				{Type: types.StringValue("slack"), Target: types.StringValue("#test-channel")},
+			},
+		},
+		{
+			name: "remote has additional recipients",
+			args: args{
+				remote: []client.NotificationRecipient{
+					{ID: "abcd12345", Type: client.RecipientTypeEmail, Target: "test@example.com"},
+					{ID: "efgh67890", Type: client.RecipientTypeSlack, Target: "#test-channel"},
+					{ID: "qrsty3847", Type: client.RecipientTypeSlack, Target: "#test-alerts"},
+					{
+						ID:     "ijkl13579",
+						Type:   client.RecipientTypePagerDuty,
+						Target: "test-pagerduty",
+						Details: &client.NotificationRecipientDetails{
+							PDSeverity: client.PDSeverityWARNING,
+						}},
+				},
+				state: []models.NotificationRecipientModel{
+					{ID: types.StringValue("abcd12345")},                                           // defined by ID
+					{Type: types.StringValue("slack"), Target: types.StringValue("#test-channel")}, // defined by type+target
+				},
+			},
+			want: []models.NotificationRecipientModel{
+				{ID: types.StringValue("abcd12345")},
+				{Type: types.StringValue("slack"), Target: types.StringValue("#test-channel")},
+				{ID: types.StringValue("qrsty3847"), Type: types.StringValue("slack"), Target: types.StringValue("#test-alerts")},
+				{
+					ID:     types.StringValue("ijkl13579"),
+					Type:   types.StringValue("pagerduty"),
+					Target: types.StringValue("test-pagerduty"),
+					Details: []models.NotificationRecipientDetailsModel{
+						{PDSeverity: types.StringValue("warning")},
+					},
+				},
+			},
+		},
+		{
+			name: "state has additional recipients",
+			args: args{
+				remote: []client.NotificationRecipient{
+					{ID: "efgh67890", Type: client.RecipientTypeSlack, Target: "#test-foo"},
+				},
+				state: []models.NotificationRecipientModel{
+					{ID: types.StringValue("abcd12345")},
+					{Type: types.StringValue("slack"), Target: types.StringValue("#test-foo")},
+					{ID: types.StringValue("ijkl13579"), Details: []models.NotificationRecipientDetailsModel{{PDSeverity: types.StringValue("warning")}}},
+				},
+			},
+			want: []models.NotificationRecipientModel{
+				{Type: types.StringValue("slack"), Target: types.StringValue("#test-foo")},
+			},
+		},
+		{
+			name: "state has totally unmatched recipients",
+			args: args{
+				remote: []client.NotificationRecipient{
+					{ID: "efgh67890", Type: client.RecipientTypeSlack, Target: "#test-foo"},
+				},
+				state: []models.NotificationRecipientModel{
+					{ID: types.StringValue("abcd12345")},
+					{Type: types.StringValue("slack"), Target: types.StringValue("#test-channel")},
+					{ID: types.StringValue("ijkl13579"), Details: []models.NotificationRecipientDetailsModel{{PDSeverity: types.StringValue("warning")}}},
+				},
+			},
+			want: []models.NotificationRecipientModel{
+				{ID: types.StringValue("efgh67890"), Type: types.StringValue("slack"), Target: types.StringValue("#test-foo")},
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.want, reconcileReadNotificationRecipientState(tt.args.remote, tt.args.state))
+		})
+	}
+}
diff --git a/internal/provider/trigger_resource_test.go b/internal/provider/trigger_resource_test.go
index d23103e..0f19936 100644
--- a/internal/provider/trigger_resource_test.go
+++ b/internal/provider/trigger_resource_test.go
@@ -421,6 +421,46 @@ resource "honeycombio_trigger" "test" {
 	})
 }
 
+func TestAcc_TriggerResourceHandlesRecipientChangedOutsideOfTerraform(t *testing.T) {
+	c := testAccClient(t)
+	ctx := context.Background()
+	dataset := testAccDataset()
+
+	// setup a slack recipient to be used in the trigger, and modified outside of terraform
+	channel := "#" + acctest.RandString(8)
+	rcpt, err := c.Recipients.Create(ctx, &client.Recipient{
+		Type: client.RecipientTypeSlack,
+		Details: client.RecipientDetails{
+			SlackChannel: channel,
+		},
+	})
+	require.NoError(t, err, "failed to create test recipient")
+	t.Cleanup(func() {
+		//nolint:errcheck
+		c.Recipients.Delete(ctx, rcpt.ID)
+	})
+
+	resource.Test(t, resource.TestCase{
+		PreCheck:                 testAccPreCheck(t),
+		ProtoV5ProviderFactories: testAccProtoV5MuxServerFactory,
+		Steps: []resource.TestStep{
+			{
+				Config: testAccConfigTriggerWithSlackRecipient(dataset, channel),
+			},
+			{
+				PreConfig: func() {
+					// update the channel name outside of Terraform
+					channel += "-1"
+					rcpt.Details.SlackChannel = channel
+					_, err := c.Recipients.Update(ctx, rcpt)
+					require.NoError(t, err, "failed to update test recipient")
+				},
+				Config: testAccConfigTriggerWithSlackRecipient(dataset, channel),
+			},
+		},
+	})
+}
+
 func testAccConfigBasicTriggerTest(dataset, pdseverity string) string {
 	return fmt.Sprintf(`
 data "honeycombio_query_specification" "test" {
@@ -504,6 +544,35 @@ resource "honeycombio_trigger" "test" {
 }`, dataset, recipientID, exceededLimit)
 }
 
+func testAccConfigTriggerWithSlackRecipient(dataset, channel string) string {
+	return fmt.Sprintf(`
+data "honeycombio_query_specification" "test" {}
+
+resource "honeycombio_query" "test" {
+  dataset    = "%[1]s"
+  query_json = data.honeycombio_query_specification.test.json
+}
+
+resource "honeycombio_trigger" "test" {
+  name = "my test trigger"
+
+  dataset  = "%[1]s"
+  query_id = honeycombio_query.test.id
+
+  threshold {
+    op    = "<"
+    value = 100
+  }
+
+  frequency = 1800
+
+  recipient {
+    type   = "slack"
+    target = "%[2]s"
+  }
+}`, dataset, channel)
+}
+
 func testAccEnsureTriggerExists(t *testing.T, name string) resource.TestCheckFunc {
 	return func(s *terraform.State) error {
 		resourceState, ok := s.RootModule().Resources[name]