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

[ORCA-4811] Add support for dynamic_route_to action to pagerduty_event_orchestration_router #1

Conversation

alenapan
Copy link
Owner

@alenapan alenapan commented Jun 12, 2024

Context

This PR adds support for the dynamic_route_to rule action for the pagerduty_event_orchestration_router resource. Config validation is out of scope for this ticket and will be added as part of https://pagerduty.atlassian.net/browse/ORCA-4813.

The action can be configured as follows:

resource "pagerduty_event_orchestration_router" "my_router" {
	event_orchestration = pagerduty_event_orchestration.my_orch.id

	catch_all {
		actions {
			route_to = "unrouted"
		}
	}
	set {
		id = "start"
		rule {
			disabled = false
			label = "dynamic routing rule"
			actions {
				dynamic_route_to {
					lookup_by = "service_name" # or "service_id"
					regex = ".*"
					source = "event.custom_details.pd_service"
				}
			}
		}
		rule {
			label = "static routing rule"
			actions {
				route_to = pagerduty_service.my_service.id
			}
		}
	}
}

Testing

Acceptance Tests Passing

image

⚠️ Note 1: I couldn't point my environment to a local copy of the current TF Provider branch using dev_overrides so I couldn't test these changes locally. But to verify that acceptance tests indeed do what we expect them to do, I did the following:

  1. Temporarily commented out all but the first five steps so we don't have a lot of JSON file versions to comb through in S3: image
  2. Re-ran the test: image
  3. Grabbed the orchestration ID from the logs and looked it up in S3: https://us-west-2.console.aws.amazon.com/s3/object/pd-event-rule-engine?region=us-west-2&bucketType=general&prefix=orchestrations%2Frouter%2Faccounts%2F342110%2F03b6bd4d-d2ab-4620-be10-4bc30a020027.json&tab=versions image
  4. Went through the file versions and confirmed that they match the expected order of changes: image
  • 1: the Router is created when a parent Orchestration is created
  • 2: Test Step 1 - the Router is updated but it has the same config - no rules
  • 3: Test Step 2 - the Router is updated with one static routing rule
  • 4: Test Step 3 - a dynamic routing rule by service name is added
  • 5: Test Step 4 - the dynamic routing rule is updated to look up by service ID
  • 6: Test Step 5 - the dynamic routing rule is deleted and the static routing rule is updated to have a condition
  • 7: the cleanup step where the router gets reset to an empty state

⚠️ Note 2: I noticed that the TestAccPagerDutyEventOrchestrationPathRouter_import test is failing, but import tests for unrouted and service orchestrations are also failing on the master branch because of the same error where the ImportStateVerify function finds differences in rule IDs in case with Router/Unrouted EOs, and the enable_event_orchestration_for_service for Service EO. I added some temporary logs in various spots in the code to confirm that rule IDs are mapped to the resource data when a GET call is made, so I suspect it could be an update to the testing library or something similar that caused the checks to get stricter or run at a different time. I will look into it a bit more but I want to do it outside of the scope of this PR.

Note 2 Update: I tested Router import with the latest version of PD TF Provider (3.13.1) locally and confirmed that it's working as expected. So I added the ImportStateVerifyIgnore: []string{"set.0.rule.0.id", "set.0.rule.1.id"} property to the router import test so it doesn't compare rule IDs. All Router tests are passing now but I also asked in the #terraform-provider channel to make sure we are not hiding any import issues: image

Local testing steps for posterity:

  1. I referenced an EO from my pdt- account as a data source in my local main.tf:
data "pagerduty_event_orchestration" "tf_orch_test" {
 name = "TF Router Import Test EO"
}
  1. Added a terraform config for a Router:
resource "pagerduty_event_orchestration_router" "tf_import_test_router" {
  event_orchestration = data.pagerduty_event_orchestration.tf_orch_test.id

  catch_all {
    actions {
      route_to = "unrouted"
    }
  }
  set {
    id = "start"
    rule {
      actions {
        route_to = "PZ73WUB"
      }
    }
  }
}
  1. Ran an import command:
terraform import pagerduty_event_orchestration_router.tf_import_test_router e0a9048f-2a9d-4375-8d79-2d153fe3b730
  1. Verified in the state file that the rule Id matches the rule Id returned by the API:
  • .tfstate: image
  • API response: image
  1. Updated the service ID of the rule and ran terraform plan and terraform apply:
resource "pagerduty_event_orchestration_router" "tf_import_test_router" {
  event_orchestration = data.pagerduty_event_orchestration.tf_orch_test.id

  catch_all {
    actions {
      route_to = "unrouted"
    }
  }
  set {
    id = "start"
    rule {
      actions {
        route_to = "PM0AV05"
      }
    }
  }
}
  1. Verified that the rule still has the same ID in the state file and in the API response:
  • .tfstate: image
  • API response: image

Comment on lines +318 to +322
if !isNilFunc(dra) && len(dra.([]interface{})) > 0 {
actions.DynamicRouteTo = expandRouterDynamicRouteToAction(dra)
} else {
actions.RouteTo = am["route_to"].(string)
}
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Annotation: The dynamic_route_to action takes precedence over route_to but we can hopefully add validation for these actions to be mutually exclusive in the subsequent ticket (https://pagerduty.atlassian.net/browse/ORCA-4813).

@@ -76,3 +76,5 @@ require (
google.golang.org/protobuf v1.33.0 // indirect
gopkg.in/ini.v1 v1.67.0 // indirect
)

replace github.com/heimweh/go-pagerduty => github.com/alexzakabluk/go-pagerduty v0.0.0-20240607142119-ac9a64bba6da
Copy link
Owner Author

@alenapan alenapan Jun 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Annotation: This change as well as the changes in go.sum, vendor/github.com/heimweh/go-pagerduty/pagerduty/event_orchestration_path.go, and vendor/modules.txt is the result of temporarily pointing the provider to a feature branch of github.com/heimweh/go-pagerduty:

 $ go mod edit -replace "github.com/heimweh/go-pagerduty=github.com/alexzakabluk/go-pagerduty@rule-actions-updates"
 $ go mod tidy
 $ go mod vendor

We can merge this change into the base branch and upgrade to the latest go-pagerduty version before merging into https://github.com/PagerDuty/terraform-provider-pagerduty. If we don't merge this change the base branch won't compile since the DynamicRouteTo property will be missing in the API Client.

"route_to": {
Type: schema.TypeString,
Required: true,
Optional: true,
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Annotation: We can't have either dynamic_route_to or route_to required because they are mutually exclusive and a rule will have either one. Hopefully in the subsequent PR we can add a check for a rule to have one or the other.

Comment on lines +27 to +36
dynamicRouteToByNameInput := &pagerduty.EventOrchestrationPathDynamicRouteTo{
LookupBy: "service_name",
Regex: ".*",
Source: "event.custom_details.pd_service_name",
}
dynamicRouteToByIDInput := &pagerduty.EventOrchestrationPathDynamicRouteTo{
LookupBy: "service_id",
Regex: "ID:(.*)",
Source: "event.custom_details.pd_service_id",
}
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Annotation: I'm using these variables to 1. build the config and 2. use in the verification functions

Copy link

@metavida metavida left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still feel unfamiliar enough with Go to be tentative in my ability to spot issues. As usual your through QA gives me confidence, and I don't see anything immediately worrying about the code in this PR 🚀

This reverts commit e42694d.
@alenapan
Copy link
Owner Author

I reverted the commit that fixes the import Router test because there is another PR addressing the issue: PagerDuty#883

@alenapan alenapan merged commit 36a750d into event-orchestration-dynamic-routing-and-esc-policy-support Jun 14, 2024
@alenapan alenapan deleted the ORCA-4811-dynamic-routing branch June 14, 2024 14:41
c-kaieong pushed a commit that referenced this pull request Jul 22, 2024
…vent_orchestration_router` (#1)

* temporarily replace go-client with a feature branch

* ORCA-4811 - add support for  action

* fix import tests

* Revert "fix import tests"

This reverts commit e42694d.
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.

2 participants