BetterStackHQ / terraform-provider-better-uptime

Apache License 2.0
45 stars 12 forks source link

Provider crash due to conversion panic #104

Open fancybear-dev opened 3 weeks ago

fancybear-dev commented 3 weeks ago

Hi,

We're running into what seems to be a provider bug, but we're unsure as of yet.

Resources it seems to affect;

I have not been able to reproduce it outside of our production environment yet, but also cannot share those details. These kinds of errors can happen in case of a lack of memory, we have not validated this yet. It would be unusual, given we run on a 32GB RAM environment that is very lean. I'm aware this is limited information to do anything with, but wanted to report it regardless and keep you in the loop in case we find a root cause / fix. If this is something that someone else has seen / if this is a known issue - I'd eagerly would want to know.

2024-06-25 15:16:18.162 CEST
The plugin.(*GRPCProvider).ValidateResourceConfig request was cancelled.
2024-06-25 15:16:18.186 CEST
Error: Request cancelled

2024-06-25 15:16:18.327 CEST
The plugin encountered an error, and failed to respond to the
2024-06-25 15:16:18.327 CEST
plugin.(*GRPCProvider).ApplyResourceChange call. The plugin logs may contain
2024-06-25 15:16:18.327 CEST
more details.
2024-06-25 15:16:18.328 CEST
2024-06-25 15:16:18.328 CEST
Error: Plugin did not respond

2024-06-25 15:16:18.384 CEST
panic: interface conversion: interface {} is nil, not map[string]interface {}
2024-06-25 15:16:18.384 CEST
2024-06-25 15:16:18.384 CEST
goroutine 92 [running]:
2024-06-25 15:16:18.384 CEST
github.com/BetterStackHQ/terraform-provider-better-uptime/internal/provider.loadRequestHeaders(0xc0000da680, 0xc0001cc650)
2024-06-25 15:16:18.384 CEST
    github.com/BetterStackHQ/terraform-provider-better-uptime/internal/provider/resource_monitor.go:574 +0x99f
2024-06-25 15:16:18.384 CEST
github.com/BetterStackHQ/terraform-provider-better-uptime/internal/provider.monitorCreate(0xda3c88, 0xc000756120, 0xc0000da680, 0xc08e00, 0xc000272bc0, 0xc00065c6d0, 0x834e2a, 0xc000600ba0)
2024-06-25 15:16:18.384 CEST
    github.com/BetterStackHQ/terraform-provider-better-uptime/internal/provider/resource_monitor.go:514 +0x117
2024-06-25 15:16:18.384 CEST
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*Resource).create(0xc00043c9a0, 0xda3c18, 0xc0002735c0, 0xc0000da680, 0xc08e00, 0xc000272bc0, 0x0, 0x0, 0x0)
2024-06-25 15:16:18.384 CEST
    github.com/hashicorp/terraform-plugin-sdk/v2@v2.7.0/helper/schema/resource.go:330 +0x17f
2024-06-25 15:16:18.384 CEST
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*Resource).Apply(0xc00043c9a0, 0xda3c18, 0xc0002735c0, 0xc0008181c0, 0xc000600ba0, 0xc08e00, 0xc000272bc0, 0x0, 0x0, 0x0, ...)
2024-06-25 15:16:18.384 CEST
    github.com/hashicorp/terraform-plugin-sdk/v2@v2.7.0/helper/schema/resource.go:456 +0x67b
2024-06-25 15:16:18.384 CEST
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*GRPCProviderServer).ApplyResourceChange(0xc00000c0c0, 0xda3c18, 0xc0002735c0, 0xc0003dd8b0, 0xc0002735c0, 0xc70f00, 0xc0006b8300)
2024-06-25 15:16:18.384 CEST
    github.com/hashicorp/terraform-plugin-sdk/v2@v2.7.0/helper/schema/grpc_provider.go:955 +0x8ef
2024-06-25 15:16:18.384 CEST
github.com/hashicorp/terraform-plugin-go/tfprotov5/server.(*server).ApplyResourceChange(0xc000600220, 0xda3cc0, 0xc0002735c0, 0xc000818070, 0xc000600220, 0xc0006b8390, 0xc00075eba0)
2024-06-25 15:16:18.384 CEST
    github.com/hashicorp/terraform-plugin-go@v0.3.0/tfprotov5/server/server.go:332 +0xb5
2024-06-25 15:16:18.384 CEST
github.com/hashicorp/terraform-plugin-go/tfprotov5/internal/tfplugin5._Provider_ApplyResourceChange_Handler(0xc70f00, 0xc000600220, 0xda3cc0, 0xc0006b8390, 0xc00038a3c0, 0x0, 0xda3cc0, 0xc0006b8390, 0xc00016ce00, 0x6db)
2024-06-25 15:16:18.384 CEST
    github.com/hashicorp/terraform-plugin-go@v0.3.0/tfprotov5/internal/tfplugin5/tfplugin5_grpc.pb.go:380 +0x214
2024-06-25 15:16:18.384 CEST
google.golang.org/grpc.(*Server).processUnaryRPC(0xc0000f2540, 0xdaabb8, 0xc00038fb00, 0xc00020a000, 0xc0001227b0, 0x11cdc60, 0x0, 0x0, 0x0)
2024-06-25 15:16:18.384 CEST
    google.golang.org/grpc@v1.32.0/server.go:1194 +0x52b
2024-06-25 15:16:18.384 CEST
google.golang.org/grpc.(*Server).handleStream(0xc0000f2540, 0xdaabb8, 0xc00038fb00, 0xc00020a000, 0x0)
2024-06-25 15:16:18.384 CEST
    google.golang.org/grpc@v1.32.0/server.go:1517 +0xd0c
2024-06-25 15:16:18.384 CEST
google.golang.org/grpc.(*Server).serveStreams.func1.2(0xc000182410, 0xc0000f2540, 0xdaabb8, 0xc00038fb00, 0xc00020a000)
2024-06-25 15:16:18.384 CEST
    google.golang.org/grpc@v1.32.0/server.go:859 +0xab
2024-06-25 15:16:18.384 CEST
created by google.golang.org/grpc.(*Server).serveStreams.func1
2024-06-25 15:16:18.384 CEST
    google.golang.org/grpc@v1.32.0/server.go:857 +0x1fd
2024-06-25 15:16:18.384 CEST
2024-06-25 15:16:18.384 CEST
Error: The terraform-provider-better-uptime_v0.11.1 plugin crashed!
2024-06-25 15:16:18.384 CEST
2024-06-25 15:16:18.384 CEST
This is always indicative of a bug within the plugin. It would be immensely
2024-06-25 15:16:18.384 CEST
helpful if you could report the crash with the plugin's maintainers so that it
2024-06-25 15:16:18.384 CEST
can be fixed. The output above should help diagnose the issue.
fancybear-dev commented 3 weeks ago

Based on this issue (https://github.com/BetterStackHQ/terraform-provider-better-uptime/issues/19) and API docs (https://betterstack.com/docs/uptime/api/create-a-new-monitor/#example-curl) I suspect the provider does insufficient validation for the input value of request_headers of the betteruptime_monitor resource. We have a condition in place that results in a null value being passed, which is accepted - but seemingly not processed properly by the provider -> resulting in the conversion panic. Testing this now.

I find the API odd in general as well - never seen a data structure like this for this purpose;

"request_headers": [
  {
    "name": "X-Custom-Header",
    "value": "custom header value"
  }
]

This is overly complex to simply pass headers, something like;

"request_headers": {
    header_key: header_value,
    header_key2: header_value2
}

makes more sense to me personally - it also reads better in places like Terraform. I'm sure there are reasons for the existing solution, but thought I'd share regardless.

Right now in Terraform I do the conversion like this (to pass it 1:1 to the resource) - but this conversion in itself is the thing I find odd but it's what it is because the API expects it like this;

  headers = {
    "Authorization" = "whatever"
    "magic"         = "tree"
  }
  request_headers = [
    for header_key, header_value in local.headers: {
      name  = header_key
      value = header_value
    }
  ]
fancybear-dev commented 3 weeks ago

Managed to work around the issue by ensuring the input value is as expected, and am now receiving the error that is correct - but should also be catched imo in the provider. If not catched in the provider at plan time, it creates overhead for developers when it fails at apply time. The provider should reflect what the API is validating on.

image

In any case, solved my issues - but will leave this issue open as I'm curious what your thoughts are.

fancybear-dev commented 3 weeks ago

What I would like most, is if null is accepted to be passed to the provider - to allow for conditional request_headers. Right now it's either a provider panic or the API that does not accept it -> so I'm currently always passing a (bogus) header.

PetrHeinz commented 2 weeks ago

Hello @fancybear-dev, thank you for the detailed report 🙌

While I can't provide any ETA on this, we'd like to fix the provider panicking in this case. The request_headers field should be easier to work with there. Thanks again for reporting it and all the details 👍