terraform-provider-consul icon indicating copy to clipboard operation
terraform-provider-consul copied to clipboard

consul_service requires time out for HTTP

Open john-delivuk opened this issue 3 years ago • 2 comments

Hi there,

Terraform Version

1.0.11

Affected Resource(s)

Please list the resources as a list, for example:

  • consul_service

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

john-delivuk avatar Jan 03 '22 18:01 john-delivuk

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

blake avatar Jan 10 '22 07:01 blake

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.

remilapeyre avatar Jan 13 '22 20:01 remilapeyre