Spurious HTTP Check updates

[Foxlik]

I do have an external service defined in Consul:

[
  {
    "ID": "",
    "Node": "external-node",
    "Address": "an-address-different-from-http-host.local",
    "Datacenter": "ams",
    "TaggedAddresses": null,
    "NodeMeta": {
      "external-node": "true",
      "external-probe": "false"
    },
    "ServiceKind": "",
    "ServiceID": "someservice",
    "ServiceName": "someservice",
    "ServiceTags": [
      "tag"
    ],
    "ServiceAddress": "an-address-different-from-http-host.local",
    "ServiceWeights": {
      "Passing": 1,
      "Warning": 1
    },
    "ServiceMeta": {
      "external-source": "terraform"
    },
    "ServicePort": 443,
    "ServiceSocketPath": "",
    "ServiceEnableTagOverride": false,
    "ServiceProxy": {
      "Mode": "",
      "MeshGateway": {},
      "Expose": {}
    },
    "ServiceConnect": {},
    "CreateIndex": 8788307,
    "ModifyIndex": 8788307
  }
]

with following HTTP check:

  {
    "Node": "an-address-different-from-http-host.local",
    "CheckID": "someservice-https",
    "Name": "someservice-https",
    "Status": "passing",
    "Notes": "",
    "Output": "<truncated>",
    "ServiceID": "someservice",
    "ServiceName": "someserivce",
    "ServiceTags": [
       "tag"
    ],
    "Type": "",
    "Interval": "",
    "Timeout": "",
    "ExposedPort": 0,
    "Definition": {
      "Interval": "10s",
      "Timeout": "2s",
      "HTTP": "https://an-address-different-from-http-host.local",
      "TLSSkipVerify": true,
      "Header": {
        "Host": [
          "someservice.local"
        ]
      },
      "Method": "GET"
    },
    "CreateIndex": 8788307,
    "ModifyIndex": 8789505
  }

And every time there is any change to Consul database the HTTP Check in ESM gets updated because of Headers mismatch on

consul-esm/check.go

Line 119 in 93a8ff1

reflect.DeepEqual(httpCheck.Header, http.Header) &&

because

httpCheck.Header= "map[Accept:[text/plain, text/*, */*] Host:[infradb-k8s.rfiserve.net] User-Agent:[Consul Health Check]]"

and

http.Header= map[Host:[infradb-k8s.rfiserve.net]]

So it seems that Consul is adding some Headers of its own causing the mistmatch. The check gets updated to what's in the definition and next time there is any change in Consul database (that causes check to be compared) this happens again.

Thanks to this happening so often, I've also found a deadlock in the update itself. Another issue to follow.

[cbroglie]

Thanks for blazing the trail @Foxlik, we're hitting this issue too as part of an attempted change to provide values for Header for the first time.

The issue appears to be this code: https://github.com/hashicorp/consul/blob/6d9df7aedbc28bc5531ed9812ee86319c5848648/agent/checks/check.go#L466-L482. When c.Header is not provided, check creates a new http.Header instance for each request and adds the default headers to it. But when c.Header is non-nil, the default headers are added to it directly instead of a copy.

I believe the correct thing to do is to create a new instance of http.Header with the default headers in each request, and then overlay any values of c.Header on top of it, thus never modifying c.Header.

A workaround is to include the same default header values check is injecting in the values we configure Header with so the check's modifications are a no-op.

[eikenb]

Thanks for filing the issue @Foxlik and the followup @cbroglie!

Sorry for the delay in communications. I'm looking into this now.

[Foxlik]

Thank you @eikenb. Let me know if I can help in any way.

[eikenb]

I'm trying to reproduce this and can't seem to get any content in the HTTP headers. Could you expand a bit on..

And every time there is any change to Consul database the HTTP Check in ESM gets updated [..]

Maybe give a couple examples of "any change" to be sure I'm hitting the same paths as you?

Sorry for asking about basics... I'm still getting up to speed on ESM and have little experience reproducing errors with it as of yet.

Thanks.

[eikenb]

NM... I was missing the Header section in my external service definition. :P

[eikenb]

@Foxlik, @cbroglie ... any issue with just skipping comparing the User-Agent and Accept headers in the comparison? Seems like the simplest fix.

[eikenb]

Pushed up a PR of what I have in mind. #171

[cbroglie]

I think that approach should work well enough