Add SourceAddr support #82
Conversation
|
Hi @zetaab thanks for opening this. I have a few things to mention here:
I will dig into the assignment of the |
Remote addr is based on TCP connection information, not headers, so it wouldn't make sense to translate it and it should probably be a native abi. X-forwarded headers might be what you are thinking of, some frameworks may merge them into their exposed remote addr accessor, but I don't believe Go does, not sure though. Even in such frameworks I believe they would then always have a "originalIP" type of field. To maintain being "raw", we would ideally have a ABI to get the true remote addr and ensure any x-forwarded headers that came in the input are preserved in the headers passed to wasm. |
|
the difference between Host and RemoteAddr is that Host is really header in packet level. RemoteAddr is not header. |
|
Thanks for the feedback @zetaab and @anuraaga. I was looking into this in golang source code and stumbled upon a couple of things:
That said I think it makes sense to add it to the ABI as @anuraaga suggested. Would you be up to open first a PR into https://github.com/http-wasm/http-wasm/blob/main/content/http-handler-abi.md adding the functions you already added to the host/guest? |
|
@zetaab do you mind adding a test for getSourceAddress in https://github.com/http-wasm/http-wasm-host-go/blob/main/tck/run.go#L44 ? (is that all regarding TCK @anuraaga ?) |
|
A tck test requires both the host and guest side, which is here https://github.com/http-wasm/http-wasm-host-go/blob/main/tck/guest/main.go So we have to merge to both before writing the test. We would update the guest sdk version in that module to the unreleased code when doing so. So we shouldn't block any merges on tck tests. But it reminded me I think we need a test in handlertest, which is this repo's unit tests. https://github.com/http-wasm/http-wasm-host-go/blob/main/testing/handlertest/testhandler.go |
|
@anuraaga @jcchavezs tests added. One minor issue is that the whole test pipeline does not listen for ipv6, so I could not add tests for ipv6 case. |
| @@ -2,4 +2,4 @@ module github.com/anuraaga/http-wasm-tck/guest | |||
|
|
|||
| go 1.19 | |||
|
|
|||
| require github.com/http-wasm/http-wasm-guest-tinygo v0.1.1 | |||
| require github.com/http-wasm/http-wasm-guest-tinygo v0.3.1-0.20231031134125-487a6e2eec5e | |||
There was a problem hiding this comment.
not binding to release, it uses current latest commit
| @@ -214,6 +216,24 @@ func (h *handler) testReadBody(req api.Request, resp api.Response, expectedBody | |||
| return true, 0 | |||
| } | |||
|
|
|||
| func (h *handler) testGetSourceAddr(req api.Request, resp api.Response, expectedAddr string) (next bool, reqCtx uint32) { | |||
| addr := req.GetSourceAddr() | |||
| raw := strings.Split(addr, ":") | |||
There was a problem hiding this comment.
port number is always something random in this case. So its difficult to verify
supports both ipv4 and ipv6.
The format is [::1]:51608 or 127.0.0.1:51701, so it will map golang net/http RemoteAddr field.