[#369] Conditional HTTP-body encode & decode #453

Merged
alexvanin merged 1 commit from nzinkevich/frostfs-s3-gw:feature/log-http-body into feature/369 2024-09-04 19:51:14 +00:00
Member
No description provided.
nzinkevich added 3 commits 2024-08-02 14:01:37 +00:00
Signed-off-by: Nikita Zinkevich <n.zinkevich@yadro.com>
Move git.frostfs.info/TrueCloudLab/frostfs-s3-gw/cmd/frostfs-s3-playback/request package to git.frostfs.info/TrueCloudLab/frostfs-s3-gw/playback.
Move playback.yaml example config to root config folder.

Signed-off-by: Nikita Zinkevich <n.zinkevich@yadro.com>
[#369] S3-playback http-body decoding
All checks were successful
/ DCO (pull_request) Successful in 1m4s
/ Vulncheck (pull_request) Successful in 1m15s
/ Builds (1.20) (pull_request) Successful in 1m34s
/ Builds (1.21) (pull_request) Successful in 1m36s
/ Lint (pull_request) Successful in 1m58s
/ Tests (1.20) (pull_request) Successful in 1m40s
/ Tests (1.21) (pull_request) Successful in 1m38s
d04ae2b47c
Signed-off-by: Nikita Zinkevich <n.zinkevich@yadro.com>
nzinkevich changed title from [#369] Conditional HTTP-body encode & decode to WIP: [#369] Conditional HTTP-body encode & decode 2024-08-02 14:01:42 +00:00
nzinkevich changed title from WIP: [#369] Conditional HTTP-body encode & decode to [#369] Conditional HTTP-body encode & decode 2024-08-02 14:01:56 +00:00
nzinkevich requested review from alexvanin 2024-08-02 14:07:26 +00:00
nzinkevich requested review from dkirillov 2024-08-02 14:07:27 +00:00
nzinkevich requested review from pogpp 2024-08-02 14:07:27 +00:00
nzinkevich force-pushed feature/log-http-body from d04ae2b47c to 88486bb0d5 2024-08-07 05:33:42 +00:00 Compare
dkirillov reviewed 2024-08-07 12:43:54 +00:00
@ -110,2 +115,4 @@
}
func (ww *responseReadWriter) WriteHeader(code int) {
ww.Do(func() {
Member

Why do we need this?

Why do we need this?
dkirillov marked this conversation as resolved
@ -160,2 +149,2 @@
}
}
httplog := filelog.getHTTPLogger(r).
withFieldIfExist("query", r.URL.Query()).
Member

Why do we log query? We have already logged URI and it's used in playback (query isn't used).

Why do we log `query`? We have already logged `URI` and it's used in playback (query isn't used).
Author
Member

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 it

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 it
Member

I 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

I 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) {
Member

Why is this exported?

Why is this exported?
Author
Member

It's not needed for now. Renamed.

It's not needed for now. Renamed.
dkirillov marked this conversation as resolved
@ -189,0 +234,4 @@
return nil, err
}
writer := utils.ChooseWriter(isXML, resultBody)
writer.Write(checkedPart)
Member

Don't ignore error

Don't ignore error
Author
Member

Fixed

Fixed
dkirillov marked this conversation as resolved
@ -0,0 +10,4 @@
const BodyRecognizeLimit int64 = 128
func DetectXML(reader io.Reader) (bool, []byte, error) {
Member

Can we use http.DetectContentType?

Can we use `http.DetectContentType`?
Author
Member

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

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)
Member

We must invoke Close after writing data into base64.Encoder. See docs

// NewEncoder returns a new base64 stream encoder. Data written to
// the returned writer will be encoded using enc and then written to w.
// Base64 encodings operate in 4-byte blocks; when finished
// writing, the caller must Close the returned encoder to flush any
// partially written blocks.
func NewEncoder(enc *Encoding, w io.Writer) io.WriteCloser {

otherwise in logs we will see e.g. eyJqc29uIjoidmFs that refers to {"json":"val rather than eyJqc29uIjoidmFsIn0= that refers to {"json":"val"} (that was actual body)

We must invoke `Close` after writing data into `base64.Encoder`. See docs ``` // NewEncoder returns a new base64 stream encoder. Data written to // the returned writer will be encoded using enc and then written to w. // Base64 encodings operate in 4-byte blocks; when finished // writing, the caller must Close the returned encoder to flush any // partially written blocks. func NewEncoder(enc *Encoding, w io.Writer) io.WriteCloser { ``` otherwise in logs we will see e.g. `eyJqc29uIjoidmFs` that refers to `{"json":"val` rather than `eyJqc29uIjoidmFsIn0=` that refers to `{"json":"val"}` (that was actual body)
Author
Member

Fixed

Fixed
Member

It seems we forget to invoke Close in logResponse function. We also should handle Close errors

It seems we forget to invoke `Close` in `logResponse` function. We also should handle `Close` errors
Author
Member

Fixed

Fixed
dkirillov marked this conversation as resolved
nzinkevich force-pushed feature/log-http-body from 88486bb0d5 to 57e93b2137 2024-08-09 10:07:04 +00:00 Compare
nzinkevich force-pushed feature/log-http-body from 57e93b2137 to a5b38537e6 2024-08-14 10:22:55 +00:00 Compare
dkirillov reviewed 2024-08-19 07:51:58 +00:00
@ -0,0 +16,4 @@
return nil
}
const BodyRecognizeLimit int64 = 128
Member

Should be unexported

Should be unexported
dkirillov marked this conversation as resolved
dkirillov approved these changes 2024-08-19 07:52:11 +00:00
dkirillov left a comment
Member

LGTM

LGTM
alexvanin reviewed 2024-08-21 13:17:59 +00:00
@ -162,0 +156,4 @@
payload := getBody(r.Body, l)
r.Body = io.NopCloser(bytes.NewReader(payload))
payloadReader := io.LimitReader(bytes.NewReader(payload), config.MaxBody)
Owner

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.

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.
Author
Member

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

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
alexvanin marked this conversation as resolved
@ -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)
Owner

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:

  • reuse it and analyze returned MIME type, I think it should detect XML as well
  • or make DetectXML similar by having a wrapper and calling this function on a wrapper.

This way we won't need to store checkedBytes and call writer.Write on it.

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: * reuse it and analyze returned MIME type, I think it should detect XML as well * or make `DetectXML` similar by having a wrapper and calling this function on a wrapper. This way we won't need to store `checkedBytes` and call `writer.Write` on it.
Owner

Oh I see, it might not detect XML.

I tried doing this way, but http.DetectContentType can't detect base64 encoding - it simply returns "text/plain".

Then we can just simply reorganize it as a wrapper similar to detector.go.

Oh I see, it might not detect XML. > I tried doing this way, but http.DetectContentType can't detect base64 encoding - it simply returns "text/plain". Then we can just simply reorganize it as a wrapper similar to `detector.go`.
alexvanin marked this conversation as resolved
nzinkevich added 1 commit 2024-08-23 09:15:22 +00:00
[#369] Modify data type detector
All checks were successful
/ Vulncheck (pull_request) Successful in 1m14s
/ Builds (1.20) (pull_request) Successful in 1m39s
/ Builds (1.21) (pull_request) Successful in 1m37s
/ Lint (pull_request) Successful in 2m47s
/ Tests (1.20) (pull_request) Successful in 1m39s
/ Tests (1.21) (pull_request) Successful in 1m40s
/ DCO (pull_request) Successful in 31s
2b2de00bd8
Signed-off-by: Nikita Zinkevich <n.zinkevich@yadro.com>
alexvanin approved these changes 2024-08-23 11:04:19 +00:00
alexvanin merged commit 2b2de00bd8 into feature/369 2024-08-23 11:04:23 +00:00
alexvanin deleted branch feature/log-http-body 2024-08-23 11:04:26 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-s3-gw#453
No description provided.