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

resolve circular dep problem by making trigger recipients lists their own type #262

Open
jmhodges-color opened this issue Jan 19, 2023 · 7 comments
Labels
feature This issue wants to add new functionality. needs design We need some clarification before we can start working on this.

Comments

@jmhodges-color
Copy link
Contributor

jmhodges-color commented Jan 19, 2023

Is your feature request related to a problem? Please describe.

We had to rename the Slack channel a set of alerts went to today in terraform. That caused the original Slack channel recipient (call it channelA) to be deleted before the triggers had it removed from their list of recipients and that threw an error:

409 Conflict: Recipient [REDACTED] is being used by a trigger or burn alert and cannot be deleted

Now, one non-working, terraform-only solution would be to require the triggers to be updated before the recipients with an explicit depends_on. But that depends_on can't be added because when you're creating a brand new recipient you need it to exist before adding it to a trigger. A very common kind of circular dependency problem.

So, you end up having to hunt down which triggers are using this recipient and manually remove them. (and, unfortunately, there's no search functionality for this and the web UX listing triggers truncates what recipients are included, so you have to get an admin to get you an API key to use the right API to find them. Doubly unfortunate: that API key will have write access because there's no way to get read-only recipients API access.)

Describe the solution you'd like

To solve this problem with only automated terraform work, it would be nice for the list of recipients of a trigger to be modeled as a separate resource akin to okta_group_memberships in okta-land (n.b.: I'm def talking about the okta_group_memberships one, not okta_group_membership).

I've seen this work as a way to cut the knot of circular dependencies because the memberships object never needs to be deleted, just updated to have zero members associated with it.

Describe alternatives you've considered

Manually futzing with things when the terraform apply breaks.

There may be some other terraform magicks that could be considered, but it would depend on the internal architecture of the provider (and I'm not super familiar with it)

@jharley
Copy link
Collaborator

jharley commented Jan 23, 2023

Thanks for opening this issue, @jmhodges-color! We'll ponder this one a bit.

In the interim, assuming you're on a new-enough version of Terraform the new-ish replace_triggered_by lifecycle argument might help?

@jmhodges-color
Copy link
Contributor Author

jmhodges-color commented Jan 24, 2023

Unfortunately, replace_triggered_by doesn't work for this problem and I just tried a few different refactorings that failed. While we're on terraform 1.3, replace_triggered_by has limitations that make it unsuitable for this problem.

Specifically, replace_triggered_by can only use each.key (or count.index) inside of it when in a for_each. You can't even use a locals look up to find the resource in question with the each.key.

In our specific case, we're iterating over is a map of query ids -> trigger configurations, which meant each.value calls. But refactoring to use each.key as the look up in a map of recipients or recipient ids will get you failures because of the meta-ness of lifecycle means it really needs to know the exact resource referenced.

And, of course, for_each can't iterate over a list of objects.

The error from terraform is: Only resources, count.index, and each.key may be used in replace_triggered_by.

@jmhodges-color
Copy link
Contributor Author

Oh you know what, I just looked at our config and the API docs more closely and we're specifying target and type on the recipient block as well as the id.

I wonder if that's causing some of these problems?

@jharley
Copy link
Collaborator

jharley commented Jan 24, 2023

Oh you know what, I just looked at our config and the API docs more closely and we're specifying target and type on the recipient block as well as the id.

I wonder if that's causing some of these problems?

Ah, yes: the provider should do a better job of marking these (id vs type+target) as mutually exclusive.

I'm curious if you just switch to IDs (assuming you have them?) if the issue goes away?

@jmhodges-color
Copy link
Contributor Author

jmhodges-color commented Jan 26, 2023

Yeah, so, my attempts to do that got a comment from a colleague saying we'd had to add the target because of a bug in the provider back on Sept 13, 2022. Maybe it got fixed in the mean time, though?

They said in their PR, when trying to change the recipient of a trigger:

Basically without providing both, the HC provider tries the change the name
but not the ID, and then freaks out because the ID doesn't match the new name.

Worse, the terraform apply would look correct because the Honeycomb API would return 200 but actually be wrong

@jmhodges-color
Copy link
Contributor Author

I suspect given the same issues are cropping up with with SLOs and columns, that the problem is not the name addition.

@jmhodges-color
Copy link
Contributor Author

I've made a separate ticket with a repro and an acceptance test because it seems that using just IDs doesn't make the problem go away. I believe there'll be cyclic dependency problems after we solve the id update problem described in #267, but I believe that's where we need to start.

@jharley jharley added the feature This issue wants to add new functionality. label Jun 2, 2023
@jharley jharley added the needs design We need some clarification before we can start working on this. label Sep 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature This issue wants to add new functionality. needs design We need some clarification before we can start working on this.
Projects
None yet
Development

No branches or pull requests

2 participants