PagerDuty / terraform-provider-pagerduty

Terraform PagerDuty provider
https://www.terraform.io/docs/providers/pagerduty/
Mozilla Public License 2.0
203 stars 212 forks source link

Error "Phone number should only contain digits" #578

Open JnMik opened 2 years ago

JnMik commented 2 years ago

Hello ! I am having issues applying my user contact method after running a terraform init --upgrade and it was working before. So I have downgraded versions to identify that the bug seems to have appear in 2.4.0

Simply put, these 2 resources seems affected image image

Terraform Version

Any version of the provider 2.4.0 seems affected

Affected Resource(s)

pagerduty_user_contact_method using _phone_contactmethod or _phone_contactmethod

If this issue appears to affect multiple resources, it may be an issue with Terraform's core, so please mention this.

Debug Output

Error: phone numbers should only contain digits

Expected Behavior

User method should be applied properly when respecting the documentation format (country code with +1 and number as a digit wrapped in string double quotes).

Actual Behavior

Can't apply

References

I think the bug has been introduced here : https://github.com/PagerDuty/terraform-provider-pagerduty/pull/475/files

CraigCraig-jpeg commented 2 years ago

whats the var.phone look like? the value

shgawas commented 1 year ago

Phone number should contain maximum 10 digits. Just solved this problem... !

Tzrlk commented 1 year ago

I've got it as well on 2.9.1, with the following config:

resource "pagerduty_user_contact_method" "mobile_text" {
  user_id      = local.user_id
  type         = "sms_contact_method"
  country_code = tonumber(local.mobile_prefix)
  address      = tonumber(local.mobile_number)
  label        = "Mobile"
}

and

{
  "mobile_number": "208230116"
  "mobile_prefix": "64"
}

I've tried varying the length of the number to no effect.

Interestingly, if I put the values in as static inputs, or set the local vars to static inputs, it appears to work fine. It's exactly the same data, and the tonumber call should pick up if there were any issues with the content.

The code I'm using for the input parsing is:

# Mobile number sanitising.
locals {

  # Clean up the input mobile by cleaning up any whitespace.
  mobile = replace(var.mobile, "\\s", "")

  # Extract the mobile prefix if it is possible.
  mobile_prefix = one(regex("^\\+?(${join("|", local.mobile_prefixes)})", local.mobile))

  # Strip the prefix from the mobile number if needed
  mobile_number = trimprefix(one(regex("(\\d+)$", local.mobile)), local.mobile_prefix)

I'll try a few other places to throw static values in to see what part of the process it's breaking on.

Tzrlk commented 1 year ago

I tracked it back to my use of timestamp() to generate the number in the first place. Same thing happens when using the random_integer resource.

I assume that the issue crops up during one of the initial processing runs where the timestamp value hasn't been resolved yet, and there's still a placeholder. Because the field is formatted as a string, you'd get the string placeholder value, breaking the atoi call no matter what you do.

Ideally, the contact methods should be implemented as sub-resource blocks, so they can have the standard input validation applied. At the very least, this particular validation should be removed, and left to the API to fail on and the user to prevent.

Would be super-useful if the error message contained the offending value, of course.

Tzrlk commented 1 year ago

As seen above, I just put together some test cases for this.