[#369] Enhanced log recording and playback #477

Merged
alexvanin merged 2 commits from nzinkevich/frostfs-s3-gw:feature/369 into master 2024-09-11 12:30:34 +00:00
Member

Add LogHTTP middleware for writing logs to file
Add s3-playback util for log reproducing

Add LogHTTP middleware for writing logs to file Add s3-playback util for log reproducing
nzinkevich added 8 commits 2024-08-29 08:53:14 +00:00
Signed-off-by: Nikita Zinkevich <n.zinkevich@yadro.com>
Signed-off-by: Nikita Zinkevich <n.zinkevich@yadro.com>
Build tag 'loghttp' for log_http.go. Created log_http_stub.go for default response without tag.

Signed-off-by: Nikita Zinkevich <n.zinkevich@yadro.com>
Declared fileLogger struct in log_http.go. Add ReloadFileLogger method

Signed-off-by: Nikita Zinkevich <n.zinkevich@yadro.com>
Signed-off-by: Nikita Zinkevich <n.zinkevich@yadro.com>
Signed-off-by: Nikita Zinkevich <n.zinkevich@yadro.com>
Signed-off-by: Nikita Zinkevich <n.zinkevich@yadro.com>
[#369] Add env GOFLAGS passing in Make targets
Some checks reported warnings
/ DCO (pull_request) Successful in 1m15s
/ Vulncheck (pull_request) Successful in 1m28s
/ Lint (pull_request) Successful in 3m7s
/ Tests (1.22) (pull_request) Successful in 1m52s
/ Tests (1.23) (pull_request) Successful in 1m56s
/ Builds (1.22) (pull_request) Has been cancelled
/ Builds (1.23) (pull_request) Has been cancelled
76990b7575
Signed-off-by: Nikita Zinkevich <n.zinkevich@yadro.com>
nzinkevich force-pushed feature/369 from 76990b7575 to f419924f60 2024-08-29 09:01:10 +00:00 Compare
nzinkevich requested review from alexvanin 2024-08-29 09:04:02 +00:00
nzinkevich requested review from dkirillov 2024-08-29 09:04:03 +00:00
alexvanin requested review from pogpp 2024-08-29 09:16:07 +00:00
alexvanin requested review from mbiryukova 2024-08-29 09:16:07 +00:00
alexvanin requested review from r.loginov 2024-08-29 09:16:07 +00:00
pogpp reviewed 2024-08-29 12:57:49 +00:00
@ -55,0 +57,4 @@
# max body size to log
S3_GW_HTTP_LOGGING_MAX_BODY=1024
# max log size in Mb
S3_GW_HTTP_LOGGING_MAX_LOG_SIZE: 20
Member

= ?

= ?
alexvanin requested changes 2024-08-29 13:17:07 +00:00
Dismissed
alexvanin left a comment
Owner

Check comments.

Check comments.
@ -0,0 +154,4 @@
var resp = bytes.Buffer{}
ww := &responseReadWriter{ResponseWriter: w, response: &resp}
h.ServeHTTP(ww, r)
if int64(resp.Len()) <= config.MaxBody {
Owner

I think this is not a latest version of this code.
We've change it it a bit and also modified detector.go to check and print payload correspondingly.

I think this is not a latest version of this code. We've change it it a bit and also modified `detector.go` to check and print payload correspondingly.
alexvanin marked this conversation as resolved
cmd/s3-gw/app.go Outdated
@ -643,1 +646,4 @@
func (s *appSettings) updateHTTPLoggingSettings(cfg *viper.Viper, log *zap.Logger) {
s.mu.Lock()
defer s.mu.Unlock()
Owner

After #463 we've moved synchronization out of update* functions, so it's being used only in update().

Therefore ReloadFileLogger should be invoked in update() function as well.

After https://git.frostfs.info/TrueCloudLab/frostfs-s3-gw/pulls/463 we've moved synchronization out of `update*` functions, so it's being used only in `update()`. Therefore `ReloadFileLogger` should be invoked in `update()` function as well.
alexvanin marked this conversation as resolved
Owner

@nzinkevich by the way, you can open PR from feature/369 in this repo. You can freely rebase and force-push into it.

@nzinkevich by the way, you can open PR from `feature/369` in this repo. You can freely rebase and force-push into it.
nzinkevich force-pushed feature/369 from f419924f60 to 9733f0c6b1 2024-08-29 13:33:03 +00:00 Compare
nzinkevich force-pushed feature/369 from 9733f0c6b1 to ecb3d47ab4 2024-08-29 13:34:08 +00:00 Compare
nzinkevich requested review from alexvanin 2024-08-29 13:41:52 +00:00
nzinkevich force-pushed feature/369 from ecb3d47ab4 to bd206938b4 2024-08-30 06:41:16 +00:00 Compare
nzinkevich force-pushed feature/369 from bd206938b4 to d3832af5ba 2024-08-30 07:09:54 +00:00 Compare
dkirillov reviewed 2024-09-02 14:59:08 +00:00
@ -0,0 +22,4 @@
var (
// authorizationFieldRegexp -- is regexp for credentials with Base58 encoded cid and oid and '0' (zero) as delimiter.
authorizationFieldRegexp = regexp.MustCompile(`AWS4-HMAC-SHA256 Credential=(?P<access_key_id>[^/]+)/(?P<date>[^/]+)/(?P<region>[^/]*)/(?P<service>[^/]+)/aws4_request,\s*SignedHeaders=(?P<signed_header_fields>.+),\s*Signature=(?P<v4_signature>.+)`)
Member

Why don't we use the same regexp from api/auth/center.go file?

Why don't we use the same regexp from `api/auth/center.go` file?
Author
Member

Made regex from center.go public

Made regex from center.go public
@ -0,0 +88,4 @@
return errors.New("failed to get credentials")
}
credProvider, err := credentials.NewStaticCredentialsProvider(creds.AccessKey,
creds.SecretKey, "").Retrieve(ctx)
Member

Please use one line here

Please use one line here
@ -0,0 +97,4 @@
if err != nil {
return err
}
r.Header[auth.AuthorizationHdr][0] = strings.Replace(r.Header[auth.AuthorizationHdr][0],
Member

Are we sure if auth.AuthorizationHdr always presents?

Are we sure if `auth.AuthorizationHdr` always presents?
Author
Member

Add safe header setting

Add safe header setting
@ -0,0 +108,4 @@
}
err = signer.SignHTTP(ctx, credProvider, r, r.Header.Get(api.AmzContentSha256),
authInfo["service"], authInfo["region"], signatureDateTime)
Member

Please use one line here

Please use one line here
@ -0,0 +110,4 @@
err = signer.SignHTTP(ctx, credProvider, r, r.Header.Get(api.AmzContentSha256),
authInfo["service"], authInfo["region"], signatureDateTime)
if err != nil {
return err
Member

If we don't add any context to error, we can write just

return signer.SignHTTP(ctx, credProvider, r, r.Header.Get(api.AmzContentSha256), authInfo["service"], authInfo["region"], signatureDateTime)
If we don't add any context to error, we can write just ```golang return signer.SignHTTP(ctx, credProvider, r, r.Header.Get(api.AmzContentSha256), authInfo["service"], authInfo["region"], signatureDateTime) ```
@ -0,0 +119,4 @@
func parseAuthHeader(authHeader string) (map[string]string, error) {
authInfo := auth.NewRegexpMatcher(authorizationFieldRegexp).GetSubmatches(authHeader)
if len(authInfo) == 0 {
return nil, ErrNoMatches
Member

Why do we return exported error. It seems there is no code that check it

Why do we return exported error. It seems there is no code that check it
Author
Member

Fixed

Fixed
@ -0,0 +17,4 @@
const (
nonXML = "nonXML"
typeXML = "application/XML"
Member

Let's use application/xml

Let's use `application/xml`
@ -0,0 +36,4 @@
func ChooseWriter(dataType string, bodyWriter io.Writer) io.WriteCloser {
writeCloser := nopCloseWriter{bodyWriter}
if dataType == typeXML {
return writeCloser
Member

We can write

return nopCloseWriter{bodyWriter}
We can write ```golang return nopCloseWriter{bodyWriter} ```
dkirillov reviewed 2024-09-03 06:44:47 +00:00
@ -0,0 +29,4 @@
return nil
}
func GetMultipart(ctx context.Context, oldUploadID string) (MultipartUpload, error) {
Member

This can be unexported

This can be unexported
dkirillov reviewed 2024-09-03 07:52:52 +00:00
cmd/s3-gw/app.go Outdated
@ -86,6 +86,7 @@ type (
appSettings struct {
logLevel zap.AtomicLevel
httpLogging *s3middleware.LogHTTPConfig
Member

Reloadable params should be placed under mu field

Reloadable params should be placed under `mu` field
cmd/s3-gw/app.go Outdated
@ -255,6 +257,7 @@ func (s *appSettings) update(v *viper.Viper, log *zap.Logger) {
s.mu.Lock()
defer s.mu.Unlock()
s.updateHTTPLoggingSettings(v, log)
Member

Please, keep critical section as small as possible. So we shouldn't invoke cfg.GetBool and others there

Please, keep critical section as small as possible. So we shouldn't invoke `cfg.GetBool` and others there
cmd/s3-gw/app.go Outdated
@ -588,0 +594,4 @@
s.httpLogging.MaxLogSize = cfg.GetInt(cfgHTTPLoggingMaxLogSize)
s.httpLogging.OutputPath = cfg.GetString(cfgHTTPLoggingDestination)
s.httpLogging.UseGzip = cfg.GetBool(cfgHTTPLoggingGzip)
if err := s3middleware.ReloadFileLogger(s.httpLogging); err != nil {
Member

I would expect that this function be method on LogHTTPConfig struct

I would expect that this function be method on `LogHTTPConfig` struct
cmd/s3-gw/app.go Outdated
@ -705,6 +719,7 @@ func (a *App) Serve(ctx context.Context) {
Handler: a.api,
Center: a.ctr,
Log: a.log,
LogHTTP: a.settings.httpLogging,
Member

If this setting (httpLogging) is reloadable by SIGHUP. In LogHTTP midleware we must read such settings under mutex

If this setting (httpLogging) is reloadable by SIGHUP. In `LogHTTP` midleware we must read such settings under mutex
Author
Member

Made safe config getter in middleware

Made safe config getter in middleware
@ -176,6 +176,7 @@ There are some custom types used for brevity:
| `placement_policy` | [Placement policy configuration](#placement_policy-section) |
| `server` | [Server configuration](#server-section) |
| `logger` | [Logger configuration](#logger-section) |
| `http_logging` | [HTTP Request logger configuration](#http_logging-section) |
Member

Please reformat this table

Please reformat this table
@ -379,0 +396,4 @@
| Parameter | Type | SIGHUP reload | Default value | Description |
|----------------|--------|---------------|---------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `enabled` | bool | yes | false | Flag to enable the logger. |
Member

Please be consistent with other sections and write

| `enabled`      | `bool`   | yes           | `false`         | Flag to enable the logger. |
Please be consistent with other sections and write ```golang | `enabled` | `bool` | yes | `false` | Flag to enable the logger. | ```
Member

Actually this param isn't reloadable. If this flag is false on application startap we don't add LogHTTP middleware to router at all (see api.NewRouter func)

Actually this param isn't reloadable. If this flag is `false` on application startap we don't add `LogHTTP` middleware to router at all (see `api.NewRouter` func)
Author
Member

Disabled this condition check in router, it was redundant because there are enabled condition check in the body of middleware func

Disabled this condition check in router, it was redundant because there are `enabled` condition check in the body of middleware func
docs/playback.md Outdated
@ -0,0 +35,4 @@
| # | Config parameter name | Flag name | Type | Default value | Description |
|---|-----------------------|-----------------|----------|---------------|-------------------------------------------------------------------------------|
| 1 | - | config | string | - | config file path (e.g. `./config/playback.yaml`) |
| 2 | http_timeout | http-timeout | duration | 60s | http request timeout |
Member

Consider using "`" for param names

| 2   | `http_timeout`          | `http-timeout`    | `duration` | `60s`           | http request timeout                                                          |

Do we really need order number?

Consider using "`" for param names ```golang | 2 | `http_timeout` | `http-timeout` | `duration` | `60s` | http request timeout | ``` Do we really need order number?
docs/playback.md Outdated
@ -0,0 +45,4 @@
| # | Config parameter name | Flag name | Type | Default value | Description |
|---|-----------------------|-----------|--------|---------------|--------------------------------------------------------|
| 1 | endpoint | endpoint | string | - | s3-gw endpoint URL |
| 2 | log | log | string | ./request.log | path to log file, could be either absolute or relative |
Member

Please reformat these tables

Please reformat these tables
@ -0,0 +44,4 @@
err1 := xml.Unmarshal(resp, &mpart)
err2 := xml.Unmarshal(logResponse, &mpartOld)
if err1 != nil || err2 != nil {
return errors.New("xml unmarshal error")
Member

It's better to wrap original error

It's better to wrap original error
dkirillov reviewed 2024-09-03 08:38:09 +00:00
@ -0,0 +45,4 @@
responseLabel = "response"
)
var filelog = fileLogger{}
Member

Do we really need global variable?

Do we really need global variable?
Author
Member

put logger var in LogHTTPConfig and added methods to initialize it

put logger var in LogHTTPConfig and added methods to initialize it
@ -0,0 +82,4 @@
MaxSize: conf.MaxLogSize,
Compress: conf.UseGzip,
}
err := zap.RegisterSink(SinkName, func(_ *url.URL) (zap.Sink, error) {
Member

Use

err := zap.RegisterSink(SinkName, func(*url.URL) (zap.Sink, error) {
Use ```golang err := zap.RegisterSink(SinkName, func(*url.URL) (zap.Sink, error) { ```
Author
Member

reimplemented zapcore.WriteSyncer appending to log because there was issue during config reload

reimplemented zapcore.WriteSyncer appending to log because there was issue during config reload
@ -0,0 +16,4 @@
httpTimeout = "http_timeout"
httpTimeoutFlag = "http-timeout"
skipVerifyTLS = "skip_verify_tls"
skipVerifyTLSFlag = "skip-verify-tls"
Member

I believe we can keep only one version skip-verify-tls (the same for other params)

I believe we can keep only one version `skip-verify-tls` (the same for other params)
Member

Then we can just invoke viper.BindPFlags

Then we can just invoke `viper.BindPFlags`
Author
Member

fixed

fixed
@ -0,0 +33,4 @@
SilenceErrors: true,
PersistentPreRunE: func(cmd *cobra.Command, _ []string) error {
_ = viper.BindPFlag(configPath, cmd.PersistentFlags().Lookup(configPathFlag))
Member

Why do we need empty line here?

By the way, let's check error and return it if it isn't nil

Why do we need empty line here? By the way, let's check error and return it if it isn't `nil`
@ -0,0 +64,4 @@
_ = rootCmd.MarkPersistentFlagFilename(configPathFlag)
rootCmd.PersistentFlags().Duration(httpTimeoutFlag, time.Minute, "http request timeout")
rootCmd.PersistentFlags().Bool(skipVerifyTLSFlag, false, "skip TLS certificate verification")
Member

Please add

rootCmd.SetOut(os.Stdout)
Please add ```golang rootCmd.SetOut(os.Stdout) ```
@ -0,0 +38,4 @@
Long: "Reads the network log file and sends each request to the specified URL",
Example: "frostfs-s3-playback --config <config_path> run [--endpoint=<endpoint>] [--log=<log_path>]",
PersistentPreRunE: func(cmd *cobra.Command, args []string) error {
if rootCmd.PersistentPreRunE != nil {
Member

We can drop this if we update cobra to v1.8.1 and will use cobra.EnableTraverseRunHooks = true

We can drop this if we update cobra to `v1.8.1` and will use `cobra.EnableTraverseRunHooks = true`
@ -0,0 +45,4 @@
}
_ = viper.BindPFlag(logPathFlag, cmd.Flags().Lookup(logPathFlag))
_ = viper.BindPFlag(endpointFlag, cmd.Flags().Lookup(endpointFlag))
Member

It seems we can use viper.BindPFlags(cmd.Flags()) on parent cmd to achieve the same result

It seems we can use `viper.BindPFlags(cmd.Flags())` on parent cmd to achieve the same result
@ -0,0 +52,4 @@
RunE: run,
}
func init() {
Member

Please rename this to iniRunCmd and invoke this in init function in root.go

Please rename this to `iniRunCmd` and invoke this in `init` function in `root.go`
@ -0,0 +66,4 @@
detect := detector.NewDetector(resp.Body, utils.DetectXML)
dataType, err := detect.Detect()
if err != nil {
cmd.Println(err.Error())
Member

If we already use PrintErrln, let's use it here too

If we already use `PrintErrln`, let's use it here too
@ -0,0 +91,4 @@
viper.GetString(awsAccessKey),
viper.GetString(awsSecretKey),
)
ctx = playback.WithMultiparts(ctx)
Member

By the way, why do we use context to store creds and multipart info? Can we pass this as parameters to functions that require this?

By the way, why do we use context to store creds and multipart info? Can we pass this as parameters to functions that require this?
Author
Member

added Settings struct passing instead of storing in context

added Settings struct passing instead of storing in context
@ -0,0 +183,4 @@
// prepareRequest creates request from logs and modifies its signature and uploadId (if presents).
func prepareRequest(ctx context.Context, logReq playback.LoggedRequest) (*http.Request, error) {
r, err := http.NewRequestWithContext(ctx, logReq.Method, viper.GetString(endpointFlag)+logReq.URI,
Member

It's better to accept endpoint as params (to not compute in on every request)

It's better to accept endpoint as params (to not compute in on every request)
@ -0,0 +184,4 @@
// prepareRequest creates request from logs and modifies its signature and uploadId (if presents).
func prepareRequest(ctx context.Context, logReq playback.LoggedRequest) (*http.Request, error) {
r, err := http.NewRequestWithContext(ctx, logReq.Method, viper.GetString(endpointFlag)+logReq.URI,
bytes.NewReader(logReq.Payload))
Member

Please don't split function invocation for several lines

Please don't split function invocation for several lines
@ -0,0 +204,4 @@
md5hash.Write(logReq.Payload)
r.Header.Set(api.ContentMD5, base64.StdEncoding.EncodeToString(md5hash.Sum(nil)))
}
err = playback.Sign(ctx, r)
Member

Can we support unauthorized requests?

Can we support unauthorized requests?
Author
Member

Add check whether request has auth header. No Sign() call if auth header is not present

Add check whether request has auth header. No Sign() call if auth header is not present
@ -0,0 +1,74 @@
package playback
Member

Let's move playback package to internal directory

Let's move `playback` package to `internal` directory
Author
Member

Fixed, but I remained xmlutils package with detect function in pkg/ because it's used both in s3-playback and logging middleware

Fixed, but I remained xmlutils package with detect function in `pkg/` because it's used both in s3-playback and logging middleware
Owner

playback/utils package still in this PR, we should drop it.

`playback/utils` package still in this PR, we should drop it.
nzinkevich force-pushed feature/369 from d3832af5ba to 6da59eccfc 2024-09-04 13:42:06 +00:00 Compare
nzinkevich force-pushed feature/369 from 6da59eccfc to 6c5cef9205 2024-09-04 13:43:45 +00:00 Compare
nzinkevich force-pushed feature/369 from 6c5cef9205 to a53e8d0604 2024-09-05 07:15:01 +00:00 Compare
alexvanin approved these changes 2024-09-11 11:42:06 +00:00
Dismissed
alexvanin left a comment
Owner

Let's merge it after #477 (comment)

Let's merge it after https://git.frostfs.info/TrueCloudLab/frostfs-s3-gw/pulls/477#issuecomment-51234
nzinkevich force-pushed feature/369 from a53e8d0604 to 0fb2168418 2024-09-11 12:23:39 +00:00 Compare
nzinkevich dismissed alexvanin's review 2024-09-11 12:23:39 +00:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

nzinkevich force-pushed feature/369 from 0fb2168418 to 62615d7ab7 2024-09-11 12:26:39 +00:00 Compare
alexvanin approved these changes 2024-09-11 12:30:29 +00:00
alexvanin merged commit 62615d7ab7 into master 2024-09-11 12:30:34 +00:00
alexvanin deleted branch feature/369 2024-09-11 12:30:35 +00:00
Sign in to join this conversation.
No milestone
No project
No assignees
4 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#477
No description provided.