feat: reimplement proxy #68
Conversation
evgeniy-scherbina
force-pushed
the
yevhenii/proxy-v2
branch
2 times, most recently
from
4a5fa1e to
741503e
Compare
evgeniy-scherbina
force-pushed
the
yevhenii/proxy-v2
branch
from
741503e to
8de8a23
Compare
evgeniy-scherbina
changed the base branch from
yevhenii/remove_cap_dac_override
to
main
There was a problem hiding this comment.
It's hard to review this much more without more context, I've not been involved in boundary so far and there's PR description to indicate the goal here. I only have the brief 60s description from Garrett of the issues you all were seeing in boundary to go off of.
That said, the code here looks fine, I just have two main comments:
- I would remove all usage of emoji in the code, for logging and otherwise
- I would use the buffered reader
Peakas opposed to this new connectionWrapper type to deal with needing to peak at the first byte of the connection bytes to know if we're using TLS
| p.logger.Error("Error copying response body", "error", copyErr, "bytes_written", bytesWritten) | ||
| http.Error(w, "Failed to copy response", http.StatusBadGateway) | ||
| if isTLS { | ||
| p.logger.Debug("🔒 Detected TLS connection - handling as HTTPS") |
There was a problem hiding this comment.
nit: I think we should avoid usage of emoji in logging
There was a problem hiding this comment.
will do in a follow-up PR
| `, | ||
| r.Method, r.URL.Path, host, host, r.Method, host, r.Method) | ||
| } | ||
| connWrapper := &connectionWrapper{conn, buf, false} |
There was a problem hiding this comment.
imo using buffered reader peak would be nicer than this connectionWrapper type: https://pkg.go.dev/bufio#Reader
There was a problem hiding this comment.
I'll take a look into that. I kept this part from proxy-v1 implementation.
Proxy-v1 had many bugs, but connectionWrapper worked fine.
2 participants
No description provided.