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

Working reproduction of EP Management problem + Fix #898

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

lorrrrrrrenzo
Copy link

Problem: Escalation policies are wrongly reassigned when a pagerduty_team_membership resource is deleted.

This pull request adds a test, TestAccPagerDutyTeamMembership_basic, reproducing a bug in the code that manages the deletion of pagerduty_team_membership objects.

First message to PD Support:

To be specific, the problem seems to be in how the provider handles the deletion of a pagerduty_team_membership resource. Based on the behavior I observed ( for example, seeing the Terraform provider manipulate resources it is emphatically NOT managing, and seeing the Terraform Provider attach unmanaged EPs to incorrect Teams) and reading through the source code, it seems that the Terraform Provider is, upon deletion of a pagerduty_team_membership resource, removing all EPs associated with the user named in the membership resource from the team, and subsequently re-adding the EPs to the team. There seems to be a bug in how this is calculated, and EPs associated with a user but NOT associated with a team are getting attached to the team as part of the membership deletion process. I’ve linked the specific function HERE and HERE – the latter link is where the problem likely lies: there is no validation performed concerning whether an EP belongs to team whose membership is being modified.


When running the below test, the following error message is generated:

go test -v -timeout 300s -run ^TestAccPagerDutyTeamMembership_basic$ github.com/PagerDuty/terraform-provider-pagerduty/pagerduty



=== RUN   TestAccPagerDutyTeamMembership_basic
starting tests
starting tests

checkpoint1-1

checkpoint1-2

checkpoint1-3

checkpoint1-4

    resource_pagerduty_team_membership_test.go:311: Step 2/2 error: Error running apply: exit status 1
        
        Error: PUT API call to https://api.eu.pagerduty.com/teams/REDACTED/escalation_policies/REDACTED failed 400 Bad Request. Code: 2001, Errors: <nil>, Message: Escalation Policy has already been taken; Error while trying to associate back team "REDACTED" to Escalation Policy "REDACTED". Resource succesfully deleted, but some team association couldn't be completed, so you need to run "terraform plan -refresh-only" and again "terraform apply/destroy" in order to remediate the drift
        
    testing_new.go:91: Error running post-test destroy, there may be dangling resources: exit status 1
        
        Error: PUT API call to https://api.eu.pagerduty.com/teams/REDACTED/escalation_policies/REDACTED failed 400 Bad Request. Code: 2001, Errors: <nil>, Message: Escalation Policy has already been taken; Error while trying to associate back team "PJHYT8V" to Escalation Policy "PV3RGAT". Resource succesfully deleted, but some team association couldn't be completed, so you need to run "terraform plan -refresh-only" and again "terraform apply/destroy" in order to remediate the drift
        
        
        Error: DELETE API call to https://api.eu.pagerduty.com/teams/REDACTED/users/REDACTED failed 404 Not Found. Code: 2013, Errors: <nil>, Message: User not found
        
--- FAIL: TestAccPagerDutyTeamMembership_basic (255.57s)
FAIL
FAIL    github.com/PagerDuty/terraform-provider-pagerduty/pagerduty     256.594s
FAIL

Key is looking at the PUT call: the Provider is attempting to attach an unrelated and already-attached EP to a team, which results from the logic around how EPs are managed on team_membership deletion, as described above. I've redacted the team IDs here, but I will provide them to support. Running these Acceptance Tests yourself should replicate the error. Please let me know if it doesn't.


I found the following PRs that attempt to address the same issue. It seems like I’m not far off the mark with my original assessment.

I am using version 3.14.4, which incorporates PR 838, which looks like an attempt to address this exact problem.

@lorrrrrrrenzo
Copy link
Author

The additional updates modify the Provider code to correct the issue (as far as I understand it). Worth highlighting is the update to testAccCheckPagerDutyTeamExists, which corrects it from a function with (apparently) no way of producing a passing test.

@lorrrrrrrenzo lorrrrrrrenzo force-pushed the lorrrrrrrenzo/update-resourcePagerDutyTeamMembershipDelete branch from d379233 to 50f96ba Compare July 5, 2024 08:16
@lorrrrrrrenzo lorrrrrrrenzo changed the title Working reproduction of EP Management problem Working reproduction of EP Management problem + Fix Jul 5, 2024
@lorrrrrrrenzo
Copy link
Author

I just tested the compiled provider with my codebase and it is performing as expected, correcting the bug I identified in my opening message. Please let me know how I can help get this PR merged -- I have the bandwidth to dedicate some time to this.

Thanks,
Lorenzo

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.

1 participant