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

Incorrect validation of alert_type in notification policy #1750

Closed
2 tasks done
weastur opened this issue Jul 4, 2022 · 15 comments · Fixed by #1830
Closed
2 tasks done

Incorrect validation of alert_type in notification policy #1750

weastur opened this issue Jul 4, 2022 · 15 comments · Fixed by #1830
Labels
kind/enhancement Categorizes issue or PR as related to improving an existing feature. service/notifications Categorizes issue or PR as related to the notification service. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Milestone

Comments

@weastur
Copy link

weastur commented Jul 4, 2022

Confirmation

  • My issue isn't already found on the issue tracker.
  • I have replicated my issue using the latest version of the provider and it is still present.

Terraform and Cloudflare provider version

Terraform v1.2.4
on darwin_amd64
+ provider bitbucket.org/easybrain/godaddy v2.0.1
+ provider registry.terraform.io/cloudflare/cloudflare v3.18.0
+ provider registry.terraform.io/grafana/grafana v1.24.0
+ provider registry.terraform.io/hashicorp/aws v4.21.0
+ provider registry.terraform.io/hashicorp/vault v3.7.0
+ provider registry.terraform.io/ibm-cloud/ibm v1.43.0
+ provider registry.terraform.io/phillbaker/elasticsearch v2.0.2

Affected resource(s)

cloudflare_notification_policy

Terraform configuration files

resource "cloudflare_notification_policy" "example_notifications" {
  account_id  = "<<REDACTED>>"
  name        = "Expiring Service Token Alert"
  description = "Cloudflare Access service token expiration notice, sent 7 days before token is set to expire"
  alert_type  = "expiring_service_token_alert"
  enabled     = true

  webhooks_integration {
    ...
  }
}

Debug output

too many sensitive information

Panic output

No response

Expected output

Acquiring state lock. This may take a few moments...
...
Releasing state lock. This may take a few moments...

Actual output

Acquiring state lock. This may take a few moments...
...
╷
│ Error: expected alert_type to be one of [billing_usage_alert health_check_status_notification g6_pool_toggle_alert real_origin_monitoring universal_ssl_event_type bgp_hijack_notification http_alert_origin_error workers_alert weekly_account_overview], got expiring_service_token_alert
│ 
│   with module.cloudflare.cloudflare_notification_policy.easybrain_notifications["Expiring Service Token Alert"],
│   on modules/cloudflare/easybrain.tf line 225, in resource "cloudflare_notification_policy" "easybrain_notifications":
│  225:   alert_type  = lookup(each.value, "alert_type", null)
│ 
╵
╷
│ Error: expected alert_type to be one of [billing_usage_alert health_check_status_notification g6_pool_toggle_alert real_origin_monitoring universal_ssl_event_type bgp_hijack_notification http_alert_origin_error workers_alert weekly_account_overview], got dos_attack_l7
│ 
│   with module.cloudflare.cloudflare_notification_policy.easybrain_notifications["HTTP DDoS Attack Alert"],
│   on modules/cloudflare/easybrain.tf line 225, in resource "cloudflare_notification_policy" "easybrain_notifications":
│  225:   alert_type  = lookup(each.value, "alert_type", null)
│ 
╵
╷
│ Error: expected alert_type to be one of [billing_usage_alert health_check_status_notification g6_pool_toggle_alert real_origin_monitoring universal_ssl_event_type bgp_hijack_notification http_alert_origin_error workers_alert weekly_account_overview], got g6_health_alert
│ 
│   with module.cloudflare.cloudflare_notification_policy.easybrain_notifications["Load Balancing Health Alert"],
│   on modules/cloudflare/easybrain.tf line 225, in resource "cloudflare_notification_policy" "easybrain_notifications":
│  225:   alert_type  = lookup(each.value, "alert_type", null)
│ 
╵
╷
│ Error: expected alert_type to be one of [billing_usage_alert health_check_status_notification g6_pool_toggle_alert real_origin_monitoring universal_ssl_event_type bgp_hijack_notification http_alert_origin_error workers_alert weekly_account_overview], got dedicated_ssl_certificate_event_type
│ 
│   with module.cloudflare.cloudflare_notification_policy.easybrain_notifications["Dedicated SSL Alert"],
│   on modules/cloudflare/easybrain.tf line 225, in resource "cloudflare_notification_policy" "easybrain_notifications":
│  225:   alert_type  = lookup(each.value, "alert_type", null)
│ 
╵
Releasing state lock. This may take a few moments...

Steps to reproduce

  1. Create cloudflare_notification_policy with non-supported by provider (since 3.18.0), but correct alert_type, for example expiring_service_token_alert

Additional factoids

Not each Cloudflare account has a list of alert types exactly the same you've introduced in the commit below.

References

c424ccd

@weastur weastur added kind/bug Categorizes issue or PR as related to a bug. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jul 4, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jul 4, 2022

Thank you for reporting this issue! For maintainers to dig into issues it is required that all issues include the entirety of TF_LOG=DEBUG output to be provided. The only parts that should be redacted are your user credentials in the X-Auth-Key, X-Auth-Email and Authorization HTTP headers. Details such as zone or account identifiers are not considered sensitive but can be redacted if you are very cautious. This log file provides additional context from Terraform, the provider and the Cloudflare API that helps in debugging issues. Without it, maintainers are very limited in what they can do and may hamper diagnosis efforts.

This issue has been marked with triage/needs-information and is unlikely to receive maintainer attention until the log file is provided making this a complete bug report.

@github-actions github-actions bot added triage/needs-information Indicates an issue needs more information in order to work on it. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jul 4, 2022
@billxinli
Copy link

+1

We are seeing the same issue.

│Error: expected alert_type to be one of [billing_usage_alert health_check_status_notification g6_pool_toggle_alert real_origin_monitoring universal_ssl_event_type bgp_hijack_notification http_alert_origin_error workers_alert weekly_account_overview], got dos_attack_l7

Where we are using dos_attack_l7 as the alert_type

The API returns this value as dos_attack_l7.

@jacobbednarz
Copy link
Member

this isn't a bug but an enhancement; we don't currently have support for these values.

@jacobbednarz jacobbednarz added kind/enhancement Categorizes issue or PR as related to improving an existing feature. triage/accepted Indicates an issue or PR is ready to be actively worked on. service/notifications Categorizes issue or PR as related to the notification service. and removed kind/bug Categorizes issue or PR as related to a bug. triage/needs-information Indicates an issue needs more information in order to work on it. labels Jul 4, 2022
@weastur
Copy link
Author

weastur commented Jul 5, 2022

@jacobbednarz Would you please explain what kind of support you meant?
It worked on 3.17.0, but now it doesn't.

@jacobbednarz
Copy link
Member

3.18.0 added validation for publicly documented alert types. if it was working prior to this change, it was because the validation wasn't restricted to only the documented types and allowed anything (including invalid values).

if you can find public documentation for the alert types you want to use, send over a PR with them added and we can extend the validation to cater for them.

@weastur
Copy link
Author

weastur commented Jul 5, 2022

Why are you asking public documentation? I thought it's official terraform provider, so it shouldn't suffer from lack of knowledge about API.

In the API doc I can only see the method to get all available notifications for a particular account. Maybe it can be used for a future validation.
https://api.cloudflare.com/#notification-alert-types-get-alert-types

Also, there is a definition of each alert type here, but there is no API types.
https://developers.cloudflare.com/fundamentals/notifications/notification-available/

@jacobbednarz
Copy link
Member

Why are you asking public documentation? I thought it's official terraform provider, so it shouldn't suffer from lack of knowledge about API.

for functionality to be added to the provider (and other SDKs), it needs to be publicly documented and considered stable by the service team. no public documentation means there is no definition of the expected values and endpoints. there are plenty of APIs at Cloudflare and not all of them are intended or ready for public consumption.

@weastur
Copy link
Author

weastur commented Jul 6, 2022

Could you please point me to the documented alert types, so I can track changes? Because I only see the Get Alert Types method, as I mentioned above.

@weastur
Copy link
Author

weastur commented Jul 7, 2022

@jacobbednarz

@zestysoft
Copy link

Can we get a flag we can set to disable the forced validation added in 3.18?

@billxinli
Copy link

for functionality to be added to the provider (and other SDKs), it needs to be publicly documented and considered stable by the service team. no public documentation means there is no definition of the expected values and endpoints. there are plenty of APIs at Cloudflare and not all of them are intended or ready for public consumption.

@jacobbednarz Can we see where billing_usage_alert, health_check_status_notification, g6_pool_toggle_alert, real_origin_monitoring, universal_ssl_event_type, bgp_hijack_notification, http_alert_origin_error, workers_alert, weekly_account_overview are documented?

Cloudflare documents a list of available notifications: https://developers.cloudflare.com/fundamentals/notifications/notification-available/ which is also configurable in the admin panel. So these features are used by people in production environments, then why can't we use these notification settings in terraform?

@ruxandrafed
Copy link

@jacobbednarz Would this be good enough for considering dos_attack_l7 as valid? 😄

https://developers.cloudflare.com/ddos-protection/reference/alerts/ + this is the API payload if you follow the instructions in the official docs

Screen_Shot_2022-07-18_at_9_44_49_AM

@kaplanmaxe
Copy link
Contributor

This looks to be a regression actually in 3.19. Seems to be working fine in 3.18.

Especially considering that this even appears to be a regression, adding my +1 here in hopes this can be fixed. I'll be downgrading in necessary places until such time.

@kaplanmaxe
Copy link
Contributor

My apologies. 3.18 broke this. 3.17 is the last version where this works

@github-actions
Copy link
Contributor

This functionality has been released in v3.22.0 of the Terraform Cloudflare Provider.

Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Categorizes issue or PR as related to improving an existing feature. service/notifications Categorizes issue or PR as related to the notification service. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
6 participants