hashicorp / terraform-provider-consul

Terraform Consul provider
https://www.terraform.io/docs/providers/consul/
Mozilla Public License 2.0
124 stars 112 forks source link

consul_service requires time out for HTTP #291

Closed john-delivuk closed 1 year ago

john-delivuk commented 2 years ago

Hi there,

Terraform Version

1.0.11

Affected Resource(s)

Please list the resources as a list, for example:

Terraform Configuration Files

resource "consul_service" "this" {
  name    = local.consul_name
  node    =  "node1"
  address = aws_db_instance.this.address
  port    = aws_db_instance.this.port

  tags = ["appliance"]
  meta = {
    "scheme" : "postgres"
  }

  check {
    check_id                       = "service:${local.consul_name}"
    name                             = local.consul_name
    status                            = "passing"
    tcp                                 = aws_db_instance.this.endpoint
    tls_skip_verify              = false
    interval                          = "60s"
    deregister_critical_service_after = "30s"
  }
}

Debug Output

Please provider a link to a GitHub Gist containing the complete debug output:

│ Error: Missing required argument
│ 
│   on main.tf line 119, in resource "consul_service" "this":
│  119:   check {
│ 
│ The argument "timeout" is required, but no definition was found.
╵

Expected Behavior

According to the docs, timeout is only used for HTTP checks. If this is true, timeout should not be required because TCP checks are an option. Otherwise, docs should be updated to state that TCP checks also need timeout, and respect it.

Actual Behavior

Required argument missing error is shown.

Steps to Reproduce

Please list the steps required to reproduce the issue, for example:

  1. terraform apply
blake commented 2 years ago

According to the documentation at https://www.consul.io/docs/discovery/checks, all of the supported check types have a default timeout value that can optionally be overridden via the timeout parameter.

Unless I'm overlooking something, this section of code for the consul_service resource should be updated to reflect that timeout is an optional parameter.

https://github.com/hashicorp/terraform-provider-consul/blob/b56b18d724458e1699a3be2feac3cdeb295b7986/consul/resource_consul_service.go#L190-L193

remilapeyre commented 2 years ago

Hi @john-delivuk, I don't remember precisely why I made the timeout parameter required when I first implemented the support for health-checks. I think it was because the default timeout depends on the type of the health-check: we would have to reproduce the logic inthe Consul server to know whether the timeout sent back by the server on read is the actual default or if Terraform needs to override it. Things can also get complicated if the default timeout for a given health-check type changes between 2 Consul versions or if a new type is added.

IIRC this is why I had made the timeout parameter required at the time. There is a few places were the dynamic behavior of Consul can make things tricky and I could not make guesses in the provider and had to force the user to be explicit. I will try to look for other ways to improve this, it may need some changes in Consul to make it more predictable thought.