Merge pull request #762 from cttttt/reject-invalid-compound-resource-ids

More gracefully reject colonCompoundIDs that aren't compound IDs
PagerDuty · Dec 7, 2023 · 8201be5 · 8201be5
2 parents 6f39184 + 4f90ecf
commit 8201be5
Show file tree

Hide file tree

Showing 10 changed files with 121 additions and 17 deletions.
diff --git a/pagerduty/resource_pagerduty_automation_actions_action_service_association.go b/pagerduty/resource_pagerduty_automation_actions_action_service_association.go
@@ -71,7 +71,11 @@ func fetchPagerDutyAutomationActionsActionServiceAssociation(d *schema.ResourceD
 		return err
 	}
 
-	actionID, serviceID := resourcePagerDutyParseColonCompoundID(d.Id())
+	actionID, serviceID, err := resourcePagerDutyParseColonCompoundID(d.Id())
+	if err != nil {
+		return err
+	}
+
 	return resource.Retry(30*time.Second, func() *resource.RetryError {
 		resp, _, err := client.AutomationActionsAction.GetAssociationToService(actionID, serviceID)
 		if err != nil {
@@ -111,7 +115,11 @@ func resourcePagerDutyAutomationActionsActionServiceAssociationDelete(d *schema.
 		return err
 	}
 
-	actionID, serviceID := resourcePagerDutyParseColonCompoundID(d.Id())
+	actionID, serviceID, err := resourcePagerDutyParseColonCompoundID(d.Id())
+	if err != nil {
+		return err
+	}
+
 	log.Printf("[INFO] Deleting PagerDuty AutomationActionsActionServiceAssociation %s", d.Id())
 
 	retryErr := resource.Retry(2*time.Minute, func() *resource.RetryError {

diff --git a/pagerduty/resource_pagerduty_automation_actions_action_service_association_test.go b/pagerduty/resource_pagerduty_automation_actions_action_service_association_test.go
@@ -50,7 +50,11 @@ func testAccCheckPagerDutyAutomationActionsActionServiceAssociationDestroy(s *te
 		if r.Type != "pagerduty_automation_actions_action_service_association" {
 			continue
 		}
-		actionID, serviceID := resourcePagerDutyParseColonCompoundID(r.Primary.ID)
+		actionID, serviceID, err := resourcePagerDutyParseColonCompoundID(r.Primary.ID)
+		if err != nil {
+			return err
+		}
+
 		if _, _, err := client.AutomationActionsAction.GetAssociationToService(actionID, serviceID); err == nil {
 			return fmt.Errorf("Automation Actions Runner still exists")
 		}
@@ -69,7 +73,11 @@ func testAccCheckPagerDutyAutomationActionsActionServiceAssociationExists(n stri
 		}
 
 		client, _ := testAccProvider.Meta().(*Config).Client()
-		actionID, serviceID := resourcePagerDutyParseColonCompoundID(rs.Primary.ID)
+		actionID, serviceID, err := resourcePagerDutyParseColonCompoundID(rs.Primary.ID)
+		if err != nil {
+			return err
+		}
+
 		found, _, err := client.AutomationActionsAction.GetAssociationToService(actionID, serviceID)
 		if err != nil {
 			return err

diff --git a/pagerduty/resource_pagerduty_automation_actions_action_team_association.go b/pagerduty/resource_pagerduty_automation_actions_action_team_association.go
@@ -75,7 +75,11 @@ func fetchPagerDutyAutomationActionsActionTeamAssociation(d *schema.ResourceData
 		return err
 	}
 
-	actionID, teamID := resourcePagerDutyParseColonCompoundID(d.Id())
+	actionID, teamID, err := resourcePagerDutyParseColonCompoundID(d.Id())
+	if err != nil {
+		return err
+	}
+
 	return resource.Retry(30*time.Second, func() *resource.RetryError {
 		resp, _, err := client.AutomationActionsAction.GetAssociationToTeam(actionID, teamID)
 		if err != nil {
@@ -115,7 +119,11 @@ func resourcePagerDutyAutomationActionsActionTeamAssociationDelete(d *schema.Res
 		return err
 	}
 
-	actionID, teamID := resourcePagerDutyParseColonCompoundID(d.Id())
+	actionID, teamID, err := resourcePagerDutyParseColonCompoundID(d.Id())
+	if err != nil {
+		return err
+	}
+
 	log.Printf("[INFO] Deleting PagerDuty AutomationActionsActionTeamAssociation %s", d.Id())
 
 	retryErr := resource.Retry(2*time.Minute, func() *resource.RetryError {

diff --git a/pagerduty/resource_pagerduty_automation_actions_action_team_association_test.go b/pagerduty/resource_pagerduty_automation_actions_action_team_association_test.go
@@ -47,7 +47,11 @@ func testAccCheckPagerDutyAutomationActionsActionTeamAssociationDestroy(s *terra
 		if r.Type != "pagerduty_automation_actions_action_team_association" {
 			continue
 		}
-		actionID, teamID := resourcePagerDutyParseColonCompoundID(r.Primary.ID)
+		actionID, teamID, err := resourcePagerDutyParseColonCompoundID(r.Primary.ID)
+		if err != nil {
+			return err
+		}
+
 		if _, _, err := client.AutomationActionsAction.GetAssociationToTeam(actionID, teamID); err == nil {
 			return fmt.Errorf("Automation Actions Runner still exists")
 		}
@@ -66,7 +70,11 @@ func testAccCheckPagerDutyAutomationActionsActionTeamAssociationExists(n string)
 		}
 
 		client, _ := testAccProvider.Meta().(*Config).Client()
-		actionID, teamID := resourcePagerDutyParseColonCompoundID(rs.Primary.ID)
+		actionID, teamID, err := resourcePagerDutyParseColonCompoundID(rs.Primary.ID)
+		if err != nil {
+			return err
+		}
+
 		found, _, err := client.AutomationActionsAction.GetAssociationToTeam(actionID, teamID)
 		if err != nil {
 			return err

diff --git a/pagerduty/resource_pagerduty_automation_actions_runner_team_association.go b/pagerduty/resource_pagerduty_automation_actions_runner_team_association.go
@@ -71,7 +71,11 @@ func fetchPagerDutyAutomationActionsRunnerTeamAssociation(d *schema.ResourceData
 		return err
 	}
 
-	runnerID, teamID := resourcePagerDutyParseColonCompoundID(d.Id())
+	runnerID, teamID, err := resourcePagerDutyParseColonCompoundID(d.Id())
+	if err != nil {
+		return err
+	}
+
 	return resource.Retry(30*time.Second, func() *resource.RetryError {
 		resp, _, err := client.AutomationActionsRunner.GetAssociationToTeam(runnerID, teamID)
 		if err != nil {
@@ -111,7 +115,11 @@ func resourcePagerDutyAutomationActionsRunnerTeamAssociationDelete(d *schema.Res
 		return err
 	}
 
-	runnerID, teamID := resourcePagerDutyParseColonCompoundID(d.Id())
+	runnerID, teamID, err := resourcePagerDutyParseColonCompoundID(d.Id())
+	if err != nil {
+		return err
+	}
+
 	log.Printf("[INFO] Deleting PagerDuty AutomationActionsRunnerTeamAssociation %s", d.Id())
 
 	retryErr := resource.Retry(2*time.Minute, func() *resource.RetryError {

diff --git a/pagerduty/resource_pagerduty_automation_actions_runner_team_association_test.go b/pagerduty/resource_pagerduty_automation_actions_runner_team_association_test.go
@@ -47,7 +47,11 @@ func testAccCheckPagerDutyAutomationActionsRunnerTeamAssociationDestroy(s *terra
 		if r.Type != "pagerduty_automation_actions_runner_team_association" {
 			continue
 		}
-		runnerID, teamID := resourcePagerDutyParseColonCompoundID(r.Primary.ID)
+		runnerID, teamID, err := resourcePagerDutyParseColonCompoundID(r.Primary.ID)
+		if err != nil {
+			return err
+		}
+
 		if _, _, err := client.AutomationActionsRunner.GetAssociationToTeam(runnerID, teamID); err == nil {
 			return fmt.Errorf("Automation Actions Runner Team association still exists")
 		}
@@ -66,7 +70,11 @@ func testAccCheckPagerDutyAutomationActionsRunnerTeamAssociationExists(n string)
 		}
 
 		client, _ := testAccProvider.Meta().(*Config).Client()
-		runnerID, teamID := resourcePagerDutyParseColonCompoundID(rs.Primary.ID)
+		runnerID, teamID, err := resourcePagerDutyParseColonCompoundID(rs.Primary.ID)
+		if err != nil {
+			return err
+		}
+
 		found, _, err := client.AutomationActionsRunner.GetAssociationToTeam(runnerID, teamID)
 		if err != nil {
 			return err

diff --git a/pagerduty/resource_pagerduty_event_orchestration_integration.go b/pagerduty/resource_pagerduty_event_orchestration_integration.go
@@ -281,7 +281,10 @@ func resourcePagerDutyEventOrchestrationIntegrationDelete(ctx context.Context, d
 }
 
 func resourcePagerDutyEventOrchestrationIntegrationImport(ctx context.Context, d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) {
-	oid, id := resourcePagerDutyParseColonCompoundID(d.Id())
+	oid, id, err := resourcePagerDutyParseColonCompoundID(d.Id())
+	if err != nil {
+		return []*schema.ResourceData{}, err
+	}
 
 	if oid == "" || id == "" {
 		return []*schema.ResourceData{}, fmt.Errorf("Error importing pagerduty_event_orchestration_integration. Expected import ID format: <orchestration_id>:<integration_id>")

diff --git a/pagerduty/resource_pagerduty_team_membership.go b/pagerduty/resource_pagerduty_team_membership.go
@@ -82,7 +82,11 @@ func fetchPagerDutyTeamMembership(d *schema.ResourceData, meta interface{}, errC
 		return err
 	}
 
-	userID, teamID := resourcePagerDutyParseColonCompoundID(d.Id())
+	userID, teamID, err := resourcePagerDutyParseColonCompoundID(d.Id())
+	if err != nil {
+		return err
+	}
+
 	log.Printf("[DEBUG] Reading user: %s from team: %s", userID, teamID)
 	return resource.Retry(2*time.Minute, func() *resource.RetryError {
 		resp, _, err := client.Teams.GetMembers(teamID, &pagerduty.GetMembersOptions{})
@@ -191,7 +195,10 @@ func resourcePagerDutyTeamMembershipDelete(d *schema.ResourceData, meta interfac
 		return err
 	}
 
-	userID, teamID := resourcePagerDutyParseColonCompoundID(d.Id())
+	userID, teamID, err := resourcePagerDutyParseColonCompoundID(d.Id())
+	if err != nil {
+		return err
+	}
 
 	log.Printf("[DEBUG] Removing user: %s from team: %s", userID, teamID)
 

diff --git a/pagerduty/util.go b/pagerduty/util.go
@@ -262,7 +262,12 @@ func unique(s []string) []string {
 	return result
 }
 
-func resourcePagerDutyParseColonCompoundID(id string) (string, string) {
+func resourcePagerDutyParseColonCompoundID(id string) (string, string, error) {
 	parts := strings.Split(id, ":")
-	return parts[0], parts[1]
+
+	if len(parts) < 2 {
+		return "", "", fmt.Errorf("%s: expected colon compound ID to have at least two components", id)
+	}
+
+	return parts[0], parts[1], nil
 }
diff --git a/pagerduty/util_test.go b/pagerduty/util_test.go
@@ -0,0 +1,41 @@
+package pagerduty
+
+import (
+	"strings"
+	"testing"
+)
+
+func TestResourcePagerDutyParseColonCompoundID(t *testing.T) {
+	resourceIDComponents := []string{"PABC12A", "PABC12B"}
+	validCompoundResourceID := strings.Join(resourceIDComponents, ":")
+
+	first, second, err := resourcePagerDutyParseColonCompoundID(validCompoundResourceID)
+
+	if err != nil {
+		t.Fatalf("%s: expected success while parsing invalid compound resource id: got %s", validCompoundResourceID, err)
+	}
+
+	for i, component := range []string{first, second} {
+		expectedResourceIDComponent := resourceIDComponents[i]
+
+		if expectedResourceIDComponent != component {
+			t.Errorf(
+				"%s: expected component %d of a valid compound resource ID to be %s: got %s",
+				validCompoundResourceID,
+				i+1,
+				expectedResourceIDComponent,
+				component,
+			)
+		}
+	}
+}
+
+func TestResourcePagerDutyParseColonCompoundIDFailsForInvalidCompoundIDs(t *testing.T) {
+	invalidCompoundResourceID := "PABC12APABC12B"
+
+	_, _, err := resourcePagerDutyParseColonCompoundID(invalidCompoundResourceID)
+
+	if err == nil {
+		t.Fatalf("%s: expected errors while parsing invalid compound resource id: got success", invalidCompoundResourceID)
+	}
+}