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

updating recipient ids in honeycombio_trigger doesn't update recipient ids #267

Closed
jmhodges-color opened this issue Feb 3, 2023 · 12 comments
Assignees
Labels

Comments

@jmhodges-color
Copy link
Contributor

jmhodges-color commented Feb 3, 2023

Versions
0.13.0

Steps to reproduce

  1. Write this module like this (copied from the docs on honeycombio_trigger, substituting the channel values for Slack channels in your own Slack instance, using a honeycombio_dataset that has traces with child spans in it so the trace.parent_id column exists, and updating time_range to a working value):
resource "honeycombio_dataset" "test-yourdataset" {
  name              = "test-yourdataset"
}

resource "honeycombio_slack_recipient" "frameworks-alerts-test1" {
  channel = "#test1"
}

resource "honeycombio_slack_recipient" "frameworks-alerts-test2" {
  channel = "#test2"
}

data "honeycombio_query_specification" "example" {
  calculation {
    op     = "P90"
    column = "duration_ms"
  }

  filter {
    column = "trace.parent_id"
    op     = "does-not-exist"
  }

  time_range = 2000
}

resource "honeycombio_query" "example3" {
  dataset    = honeycombio_dataset.test-yourdataset.name
  query_json = data.honeycombio_query_specification.example.json
}

resource "honeycombio_trigger" "example" {
  name        = "Requests are slower than usual"
  description = "Average duration of all requests for the last 10 minutes."

  query_id = honeycombio_query.example3.id
  dataset  = honeycombio_dataset.test-yourdataset.name

  frequency = 600 // in seconds, 10 minutes

  alert_type = "on_change" // on_change is default, on_true can refers to the "Alert on True" checkbox in the UI

  threshold {
    op    = ">"
    value = 4000000 # FIXME an absurdly high number so it doesn't fire.
  }

  recipient {
    id = honeycombio_slack_recipient.frameworks-alerts-test1.id
  }
  1. Run terraform plan
  2. Run terraform apply
  3. Update the recipient block to:
   recipient {
    id = honeycombio_slack_recipient.frameworks-alerts-test1.id
  }
  1. Run terraform plan and notice that says:
Terraform will perform the following actions:

  # honeycombio_trigger.example will be updated in-place
  ~ resource "honeycombio_trigger" "example" {
        id          = "J8ri22RPZ5d"
        name        = "Requests are slower than usual"
        # (6 unchanged attributes hidden)

      ~ recipient {
          ~ id     = "Bj7FG65MoWU" -> "fTwUjLv3GtS"
            # (2 unchanged attributes hidden)
        }

        # (1 unchanged block hidden)
    }
  1. Run terraform apply and see it finishes successfully
  2. Run terraform plan and see it still says that the recipient id needs to be changed in the exact same way:
Terraform will perform the following actions:

  # honeycombio_trigger.example will be updated in-place
  ~ resource "honeycombio_trigger" "example" {
        id          = "J8ri22RPZ5d"
        name        = "Requests are slower than usual"
        # (6 unchanged attributes hidden)

      ~ recipient {
          ~ id     = "Bj7FG65MoWU" -> "fTwUjLv3GtS"
            # (2 unchanged attributes hidden)
        }

        # (1 unchanged block hidden)
    }

Additional context

I believe a colleague worked around this problem previously by setting type and target in these recipient blocks as discussed in the comments of #262. That workaround possibly opened us up to #262 or some of the difficulties in #262 are another expression of this bug

@jmhodges-color jmhodges-color changed the title updating recipients in honeycombio_trigger doesn't update recipients updating recipient ids in honeycombio_trigger doesn't update recipient ids Feb 3, 2023
jmhodges-color added a commit to jmhodges-color/terraform-provider-honeycombio that referenced this issue Feb 3, 2023
A quick attempt to add an acceptance test for changing the recipient ids
of a `honeycombio_trigger` to reproduce honeycombio#267.

Since I don't have a fully clean slate to work with (recipients and
other global objects are shared in my org's Honeycomb API access), I
can't be sure if this failure is accidental or demonstrates the problem
in honeycombio#267.

It currently fails on my machine in a special environment and dataset just for it with:

```
$  TF_ACC=1 go test -v -run=TestAccHoneycombIOTrigger_changingRecipientID ./...
?   	github.com/honeycombio/terraform-provider-honeycombio	[no test files]
testing: warning: no tests to run
PASS
ok  	github.com/honeycombio/terraform-provider-honeycombio/client	(cached) [no tests to run]
?   	github.com/honeycombio/terraform-provider-honeycombio/client/internal/httputil	[no test files]
=== RUN   TestAccHoneycombIOTrigger_changingRecipientID
    resource_trigger_test.go:213: Step 2/2 error: Check failed: honeycombio_trigger.test: Attribute 'recipient.0.id' expected "xmbNbyhSY91", got "rX2sUPHwbjn"
    testing_new.go:84: Error running post-test destroy, there may be dangling resources: exit status 1

        Error: 409 Conflict: Recipient rX2sUPHwbjn is being used by a trigger or burn alert and cannot be deleted

--- FAIL: TestAccHoneycombIOTrigger_changingRecipientID (4.38s)
FAIL
FAIL	github.com/honeycombio/terraform-provider-honeycombio/honeycombio	4.703s
?   	github.com/honeycombio/terraform-provider-honeycombio/honeycombio/internal/hashcode	[no test files]
?   	github.com/honeycombio/terraform-provider-honeycombio/honeycombio/internal/verify	[no test files]
FAIL

```
@jmhodges-color
Copy link
Contributor Author

I've attempted to write an acceptance test in a branch on my fork: jeff.hodges/debug-trigger-recipient-update

Since recipients are global objects across environments and I have only access to my org's set of environments, I wasn't able to get full isolation like y'all could.

But with its own environment and dataset set, I was able to get a failure that might be a reproduction of this issue. I'm open, though, to see it fixed up by y'all or take feedback to make it correct.

$  TF_ACC=1 go test -v -run=TestAccHoneycombIOTrigger_changingRecipientID ./...
?   	github.com/honeycombio/terraform-provider-honeycombio	[no test files]
testing: warning: no tests to run
PASS
ok  	github.com/honeycombio/terraform-provider-honeycombio/client	(cached) [no tests to run]
?   	github.com/honeycombio/terraform-provider-honeycombio/client/internal/httputil	[no test files]
=== RUN   TestAccHoneycombIOTrigger_changingRecipientID
    resource_trigger_test.go:213: Step 2/2 error: Check failed: honeycombio_trigger.test: Attribute 'recipient.0.id' expected "xmbNbyhSY91", got "rX2sUPHwbjn"
    testing_new.go:84: Error running post-test destroy, there may be dangling resources: exit status 1

        Error: 409 Conflict: Recipient rX2sUPHwbjn is being used by a trigger or burn alert and cannot be deleted

--- FAIL: TestAccHoneycombIOTrigger_changingRecipientID (4.38s)
FAIL
FAIL	github.com/honeycombio/terraform-provider-honeycombio/honeycombio	4.703s
?   	github.com/honeycombio/terraform-provider-honeycombio/honeycombio/internal/hashcode	[no test files]
?   	github.com/honeycombio/terraform-provider-honeycombio/honeycombio/internal/verify	[no test files]
FAIL

@jmhodges-color
Copy link
Contributor Author

This is also happening on honeycombio_burn_alerts with recipient ids.

Calling terraform refresh, oddly, fixes the honeycombio_triggers to the right state, but honeycombio_burn_alerts both in the terraform state and in the Honeycomb API will have the old recipients.

@jmhodges-color
Copy link
Contributor Author

jmhodges-color commented Feb 15, 2023

Any updates on this one? We're still seeing it in production when triggers need to go to a new Slack channel or pager duty or whatever

@jmhodges-color
Copy link
Contributor Author

Also, I seem to be wrong about terraform refresh fixing it.

@jharley
Copy link
Collaborator

jharley commented Feb 15, 2023

@jmhodges-color sorry for the delay here! We'll pick this up today and see what we can sort out.

@jmhodges-color
Copy link
Contributor Author

Hitting this again on dozens of triggers I'm having to fix manually

@jmhodges-color
Copy link
Contributor Author

Just hit this again for another 6 triggers (and it's not even my usual job to manage these)

@jmhodges-color
Copy link
Contributor Author

Any news on this one?

@jharley
Copy link
Collaborator

jharley commented May 3, 2023

@jmhodges-color work is underway (#305) to migrate honeycombio_trigger to the ~new Terraform Plugin Framework as I'm hopeful it will give us the control required -- which the current Plugin SDK makes rather challenging/hacky -- to squash this collection of related bugs .

@jmhodges-color
Copy link
Contributor Author

Any news?

@jharley
Copy link
Collaborator

jharley commented May 24, 2023

The trigger-recipient-fixes branch has a working solution but only for Terraform Core (CLI) 1.4.x.

Bit of a gut bunch as I was going to get it merged. Working on a solve for Terraform <1.4 as requiring everyone to update doesn't really feel like a path forward unless we're ready to cut version 1.0 of the provider

jharley added a commit that referenced this issue May 31, 2023
This completes the move of the `honeycombio_trigger` resource to the
Terraform Plugin Framework (started in #306).

In order to support versions of Terraform core older than 1.4.0 (which,
is rather "fresh") there is a bit of logic to handle `Null` or `Unknown`
values of these awkwardly-shaped notification recipients.

This also 'bumps' the version of Terraform core used in CI to 1.0.11
(from the old 0.14.x series).

- Closes #303, #267, #200
@jharley
Copy link
Collaborator

jharley commented Jun 2, 2023

Fix released in v0.15.0

@jharley jharley closed this as completed Jun 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants