Switch HTTP client to not use external memory for header injection

[grcevski]

I'm making a draft pull request for an alternative implementation of header injection, which directly writes in the outgoing request buffer at the beginning when the request headers are serialized. At that point, the outgoing buffer is 4K in size, while typically only handful of bytes are used to write the request method and URL.

This approach has a limitation that the injection will not work if the URL (and the method) are > 4027 bytes and <= 4096 bytes. URLs larger than 2048 are unsupported by various programs, but technically they can be larger. It's possible to overcome this limitation with more work if it's considered a blocker.

TODO:

• Remove the debug statements
• Add tests for the manifests changes

[edeNFed]

Nice chnage, I liked the fallback using -DNO_HEADER_PROPAGATION.

A couple of questions:

• I am concerned about the performance impact of adding additional uprobe
  for the writeSubset() function. Do you know how often writeSubset() is called per request?
• I wonder what are the advantages of using an additional uprobe over the existing mechanism. This is very specific to the HTTP instrumentation and in general, I am not sure we will be able to find such a function for every instrumentation in the future.
• The current implementation assumes io.Writer is implemented by bufio.Writer, I think this function can also be called by users with different io.Writer implementations by invoking Header.WriteSubset() (the exported function). This may cause the application to crash due to writing to the wrong memory address. A possible solution may be to look at the itab struct to verify the concrete type of the `io.Writer.

[grcevski]

Nice chnage, I liked the fallback using -DNO_HEADER_PROPAGATION.

A couple of questions:

• I am concerned about the performance impact of adding additional uprobe
  for the writeSubset() function. Do you know how often writeSubset() is called per request?

So it's called once per client request, but it does get called also when headers are sent in a response of a server request. There will be one extra probe that will run when the application is also a server, but we'll end very quickly since no header will be found, so it's mostly the probe overhead, around 100ns. We can time it with this new tool by Netflix bpftop vs the old instrumentation to get some actual overhead data.

• I wonder what are the advantages of using an additional uprobe over the existing mechanism. This is very specific to the HTTP instrumentation and in general, I am not sure we will be able to find such a function for every instrumentation in the future.

It's mostly around memory safety, since we'll actually use only memory that the Go garbage collector manages. No rewriting of map spines needs to be done with non-managed memory, which theoretically can cause the GC to temporarily lose reference to the underlying map key values. I've found a way how to do the same for gRPC outgoing calls and HTTP2. My current gRPC implementation is inefficient, but I think I can do the same as for HTTP2 and it will be similar to how we handle HTTP.

If we are able to fully work without external memory segments, other things become simpler. OTel Go Auto will be able to run without privileged: true, I expect CAP_SYS_ADMIN to be sufficient, or maybe even CAP_BPF. Implementing multi-process instrumentation also becomes a lot simpler and leaner in terms of memory consumption.

• The current implementation assumes io.Writer is implemented by bufio.Writer, I think this function can also be called by users with different io.Writer implementations by invoking Header.WriteSubset() (the exported function). This may cause the application to crash due to writing to the wrong memory address. A possible solution may be to look at the itab struct to verify the concrete type of the `io.Writer.

I believe we are safe from that scenario, because the code that assumes bufio.Writer is guarded by the check that verifies that we have recorded the header in the RoundTrip start, and we also verify it's actually an active http client request. I think, we will only assume bufio when it's actually a bufio, but maybe I'm missing an edge case when during RoundTrip this can be another io.Writer type.

To validate bufio I looked at the caller of Header.writeSubset in the client HTTP scenario, which is

func (r *Request) write(w io.Writer, usingProxy bool, extraHeaders Header, waitForContinue func() bool) (err error) {

In there, they explicitly make a bufio if it wasn't, before the writeSubset function is called:

	// Wrap the writer in a bufio Writer if it's not already buffered.
	// Don't always call NewWriter, as that forces a bytes.Buffer
	// and other small bufio Writers to have a minimum 4k buffer
	// size.
	var bw *bufio.Writer
	if _, ok := w.(io.ByteWriter); !ok {
		bw = bufio.NewWriter(w)
		w = bw
	}

I believe we are safe, but I can add extra code to confirm the type if you think it's still a problem.

[edeNFed]

OK, got it on the performance overhead and io.Writer implementation. This makes sense I don't think we need to add an additional type check then.

Regarding memory safety, I think this is something we need to consider on a per-instrumentation basis.
I do see the point in reducing the required permissions, however, I think that finding a pre-allocated space will not work for every library. We (Keyval) have many instrumentations that we are planning to gradually merge and I think that finding that specific buffer on every library is going to be extremely hard, inefficient, and probably impossible for some libraries.

This will tie us to mostly having gRPC/HTTP instrumentations as opposed to achieving full coverage (like otel-go-contrib)

[grcevski]

Regarding memory safety, I think this is something we need to consider on a per-instrumentation basis.
I do see the point in reducing the required permissions, however, I think that finding a pre-allocated space will not work for every library. We (Keyval) have many instrumentations that we are planning to gradually merge and I think that finding that specific buffer on every library is going to be extremely hard, inefficient, and probably impossible for some libraries.

Agreed. As discussed at the SIG, let's try to implement the instrumentations we can without the external memory segment. If the application is only using instrumentations that don't require an external memory segment, we can avoid the ptrace call and the shared memory segment injection. We won't remove the functionality, we'll tag which probes require it and after filtering we can determine if injecting the foreign memory segment is required or not.

[RonFed]

Added two small comments

[grcevski]

Thanks so much @edeNFed, @RonFed, @MrAlias and @damemi!