Fix missing host header in http check

[SamMousa]

Fixes #15308

[hashicorp-cla]

All committers have signed the CLA.

[SamMousa]

Disclaimer: I have only learned go ~30 minutes ago. So check this properly.

One architectural issue with this solution is that by using a host header on our side the model doesn't match cleanly with the Go net/http client.

This means that to support this use case, on setting headers we need to create an exception for the Host header. This exception also needs to be applied to the test to make sure that we don't expect the Host header in the request headers. (Since it handles it differently and doesn't expose it in headers)

I'd argue that a cleaner solution would be to adopt the same model as net/http and handle it seperately from headers; although it feels like an arbitrary thing to make an exception for this specific header, I don't have enough knowledge of the ecosystem to decide of it makes sense to abstract from that (ie are there other http clients that have different models and should we want our model to not depend on implementation details in other libraries).

[tgross]

Thanks for this PR @SamMousa! I think you're probably right that this isn't quite the right place to handle this, and I've left a comment for an approach you might want to try.

[SamMousa]

After the rewrite and finding out how http.Header is implemented, it does not feel like a very strong contract to me.
The http.Header type is internally defined as type Header map[string][]string and does not give us any stronger guarantees; all it does it add some helper functions.

[SamMousa]

We still test sending in weird data although we don't expect our code to ever send us this data since everything is normalized at the edge.

[tgross]

Hi @SamMousa! Thanks for digging into this further. I think we're getting closer, but something about the change in nomad/structs/services.go tickled that paranoid part of my brain and I checked the encoding round trip. We'll need to back out that part unfortunately.

Once that's done I think we're in good shape here. You can run make cl and that'll walk you through creating a changelog entry file in .changelog/15337.txt that you can add to this PR.

[tgross]

You'd rightfully think that encoding shouldn't care about a type alias. But between Go and optimized MessagePack encoding we can't have nice things. 😀 Changing this value breaks the MessagePack encoding we use to serialize these structs to raft. If you run make generate-structs and then:

func Test_EncodeServiceCheck(t *testing.T) {

	check := &structs.ServiceCheck{
		Name:   uuid.Generate(),
		Type:   "http",
		Header: map[string][]string{"Host": {"example.com"}},
	}

	data := []byte{}
	buf := bytes.NewBuffer(data)
	encoder := codec.NewEncoder(buf, structs.MsgpackHandle)

	err := encoder.Encode(check)
	must.NoError(t, err)

	err = os.WriteFile("encoded.msgpack", buf.Bytes(), 0o700)
	must.NoError(t, err)
}

Then make this change and run make generate-structs again the following test will fail!

func Test_DecodeServiceCheck(t *testing.T) {

	data, err := os.ReadFile("encoded.msgpack")
	must.NoError(t, err)

	buf := bytes.NewBuffer(data)
	decoder := codec.NewDecoder(buf, structs.MsgpackHandle)

	var check *structs.ServiceCheck
	err = decoder.Decode(check)
	must.NoError(t, err)
	must.NotNil(t, check)
	must.Eq(t, "example.com", check.Header.Get("Host"))
}

$ go test -v -count=1 ./nomad -run Test_DecodeServiceCheck
# github.com/hashicorp/nomad/nomad.test
ld: warning: -no_pie is deprecated when targeting new OS versions
=== RUN   Test_DecodeServiceCheck
    assert.go:24:
        service_registration_endpoint_test.go:1547: expected nil error
        ↪ error: msgpack decode error [pos 1]: cannot decode into value of kind: ptr, type: *structs.ServiceCheck, <nil>
--- FAIL: Test_DecodeServiceCheck (0.00s)
FAIL
FAIL    github.com/hashicorp/nomad/nomad        0.525s
FAIL

So unfortunately for backwards compatibility with existing raft entries, we'll need to leave this field untouched and only fix it up in client/serviceregistration/checks/client.go

[shoenig]

nit: this is the second test case named "host header"; making them unique helps quickly tracking down failures

[SamMousa]

Once that's done I think we're in good shape here. You can run make cl and that'll walk you through creating a changelog entry file in .changelog/15337.txt that you can add to this PR.

Done! Couldn't this script get the metadata directly from the PR? It asks for the PR number and the type is derivable from the tag you add to the PR. (https://api.github.com/repos/hashicorp/nomad/pulls/15337).

[tgross]

Note for future readers of this PR: this is safe without checking for it being empty because:

// For client requests, Host optionally overrides the Host
// header to send. If empty, the Request.Write method uses
// the value of URL.Host. Host may contain an international
// domain name.

[tgross]

Couldn't this script get the metadata directly from the PR? It asks for the PR number and the type is derivable from the tag you add to the PR.

Oh that's a neat idea. I think we'd just need to make sure we have labels that correspond to each of the states. Some PRs actually end up having multiple changelog entries as well (ex. breaking change + bug fix), and folks would still need to write the changelog itself (which often shouldn't be 1:1 with the commit message).

[tgross]

LGTM!

[tgross]

Thanks @SamMousa! This will ship in the next regular version of Nomad (likely 1.4.4)

[SamMousa]

In other CI ideas: there are bots that will post to an issue when the fix has been released:

I'll stop spamming here, create a new issue and tag me if you're interested to discuss those things further.

[github-actions]

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.