[#369] Conditional HTTP-body encode & decode #453
Labels
No labels
P0
P1
P2
P3
good first issue
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
No milestone
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: TrueCloudLab/frostfs-s3-gw#453
Loading…
Reference in a new issue
No description provided.
Delete branch "nzinkevich/frostfs-s3-gw:feature/log-http-body"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
[#369] Conditional HTTP-body encode & decodeto WIP: [#369] Conditional HTTP-body encode & decodeWIP: [#369] Conditional HTTP-body encode & decodeto [#369] Conditional HTTP-body encode & decoded04ae2b47c
to88486bb0d5
@ -110,2 +115,4 @@
}
func (ww *responseReadWriter) WriteHeader(code int) {
ww.Do(func() {
Why do we need this?
@ -160,2 +149,2 @@
}
}
httplog := filelog.getHTTPLogger(r).
withFieldIfExist("query", r.URL.Query()).
Why do we log
query
? We have already loggedURI
and it's used in playback (query isn't used).Collected log is also considered not only for playback, but for reading. That's why separate
query
field presented. I will ask @alexvanin later whether to keep itI suppose it can be usefull only when non-ascii character is presented in URI (so it's url encoded). But I would drop this param.
Let's discuss this when we will open PR into master
@ -187,2 +228,3 @@
httplog = httplog.With(zap.String("body", base64.StdEncoding.EncodeToString(body)))
// ProcessBody reads body and base64 encode it if it's not XML.
func ProcessBody(bodyReader io.Reader) ([]byte, error) {
Why is this exported?
It's not needed for now. Renamed.
@ -189,0 +234,4 @@
return nil, err
}
writer := utils.ChooseWriter(isXML, resultBody)
writer.Write(checkedPart)
Don't ignore error
Fixed
@ -0,0 +10,4 @@
const BodyRecognizeLimit int64 = 128
func DetectXML(reader io.Reader) (bool, []byte, error) {
Can we use
http.DetectContentType
?I tried doing this way, but
http.DetectContentType
can't detect base64 encoding - it simply returns "text/plain". However XML responses have no Processing Instruction header thus they are recognized as "text/plain" too@ -0,0 +30,4 @@
func ChooseWriter(isXML bool, body *bytes.Buffer) io.Writer {
if !isXML {
return base64.NewEncoder(base64.StdEncoding, body)
We must invoke
Close
after writing data intobase64.Encoder
. See docsotherwise in logs we will see e.g.
eyJqc29uIjoidmFs
that refers to{"json":"val
rather thaneyJqc29uIjoidmFsIn0=
that refers to{"json":"val"}
(that was actual body)Fixed
It seems we forget to invoke
Close
inlogResponse
function. We also should handleClose
errorsFixed
88486bb0d5
to57e93b2137
57e93b2137
toa5b38537e6
@ -0,0 +16,4 @@
return nil
}
const BodyRecognizeLimit int64 = 128
Should be unexported
LGTM
@ -162,0 +156,4 @@
payload := getBody(r.Body, l)
r.Body = io.NopCloser(bytes.NewReader(payload))
payloadReader := io.LimitReader(bytes.NewReader(payload), config.MaxBody)
With this change we will always print request payload, however it will be at most
MaxBody
size, right?I think previous implementation just dropped payload printing completely for everything over the limit.
Yes, it will read data until MaxBody limit exceeded. I decided it's a better way. For example, it would be easier to reproduce commands where request body shouldn't be empty (even if it's not complete). And that makes log more informative than whole body absence
@ -189,0 +231,4 @@
// processBody reads body and base64 encode it if it's not XML.
func processBody(bodyReader io.Reader) ([]byte, error) {
resultBody := &bytes.Buffer{}
isXML, checkedBytes, err := utils.DetectXML(bodyReader)
Have you looked at
api/layer/detector.go
? We can reuse the idea from there, maybe it will look a bit better.In detector we wrap reader and provide
Detect()
method to read a part of it to determine payload type.We can:
DetectXML
similar by having a wrapper and calling this function on a wrapper.This way we won't need to store
checkedBytes
and callwriter.Write
on it.Oh I see, it might not detect XML.
Then we can just simply reorganize it as a wrapper similar to
detector.go
.