[#369] Enhanced log recording and playback #477
No reviewers
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
4 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: TrueCloudLab/frostfs-s3-gw#477
Loading…
Reference in a new issue
No description provided.
Delete branch "nzinkevich/frostfs-s3-gw:feature/369"
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?
Add LogHTTP middleware for writing logs to file
Add s3-playback util for log reproducing
76990b7575
tof419924f60
@ -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
= ?
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 {
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.@ -643,1 +646,4 @@
func (s *appSettings) updateHTTPLoggingSettings(cfg *viper.Viper, log *zap.Logger) {
s.mu.Lock()
defer s.mu.Unlock()
After #463 we've moved synchronization out of
update*
functions, so it's being used only inupdate()
.Therefore
ReloadFileLogger
should be invoked inupdate()
function as well.@nzinkevich by the way, you can open PR from
feature/369
in this repo. You can freely rebase and force-push into it.f419924f60
to9733f0c6b1
9733f0c6b1
toecb3d47ab4
ecb3d47ab4
tobd206938b4
bd206938b4
tod3832af5ba
@ -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>.+)`)
Why don't we use the same regexp from
api/auth/center.go
file?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)
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],
Are we sure if
auth.AuthorizationHdr
always presents?Add safe header setting
@ -0,0 +108,4 @@
}
err = signer.SignHTTP(ctx, credProvider, r, r.Header.Get(api.AmzContentSha256),
authInfo["service"], authInfo["region"], signatureDateTime)
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
If we don't add any context to error, we can write just
@ -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
Why do we return exported error. It seems there is no code that check it
Fixed
@ -0,0 +17,4 @@
const (
nonXML = "nonXML"
typeXML = "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
We can write
@ -0,0 +29,4 @@
return nil
}
func GetMultipart(ctx context.Context, oldUploadID string) (MultipartUpload, error) {
This can be unexported
@ -86,6 +86,7 @@ type (
appSettings struct {
logLevel zap.AtomicLevel
httpLogging *s3middleware.LogHTTPConfig
Reloadable params should be placed under
mu
field@ -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)
Please, keep critical section as small as possible. So we shouldn't invoke
cfg.GetBool
and others there@ -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 {
I would expect that this function be method on
LogHTTPConfig
struct@ -705,6 +719,7 @@ func (a *App) Serve(ctx context.Context) {
Handler: a.api,
Center: a.ctr,
Log: a.log,
LogHTTP: a.settings.httpLogging,
If this setting (httpLogging) is reloadable by SIGHUP. In
LogHTTP
midleware we must read such settings under mutexMade 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) |
Please reformat this table
@ -379,0 +396,4 @@
| Parameter | Type | SIGHUP reload | Default value | Description |
|----------------|--------|---------------|---------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `enabled` | bool | yes | false | Flag to enable the logger. |
Please be consistent with other sections and write
Actually this param isn't reloadable. If this flag is
false
on application startap we don't addLogHTTP
middleware to router at all (seeapi.NewRouter
func)Disabled this condition check in router, it was redundant because there are
enabled
condition check in the body of middleware func@ -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 |
Consider using "`" for param names
Do we really need order number?
@ -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 |
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")
It's better to wrap original error
@ -0,0 +45,4 @@
responseLabel = "response"
)
var filelog = fileLogger{}
Do we really need global variable?
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) {
Use
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"
I believe we can keep only one version
skip-verify-tls
(the same for other params)Then we can just invoke
viper.BindPFlags
fixed
@ -0,0 +33,4 @@
SilenceErrors: true,
PersistentPreRunE: func(cmd *cobra.Command, _ []string) error {
_ = viper.BindPFlag(configPath, cmd.PersistentFlags().Lookup(configPathFlag))
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")
Please add
@ -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 {
We can drop this if we update cobra to
v1.8.1
and will usecobra.EnableTraverseRunHooks = true
@ -0,0 +45,4 @@
}
_ = viper.BindPFlag(logPathFlag, cmd.Flags().Lookup(logPathFlag))
_ = viper.BindPFlag(endpointFlag, cmd.Flags().Lookup(endpointFlag))
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() {
Please rename this to
iniRunCmd
and invoke this ininit
function inroot.go
@ -0,0 +66,4 @@
detect := detector.NewDetector(resp.Body, utils.DetectXML)
dataType, err := detect.Detect()
if err != nil {
cmd.Println(err.Error())
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)
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?
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,
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))
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)
Can we support unauthorized requests?
Add check whether request has auth header. No Sign() call if auth header is not present
@ -0,0 +1,74 @@
package playback
Let's move
playback
package tointernal
directoryFixed, but I remained xmlutils package with detect function in
pkg/
because it's used both in s3-playback and logging middlewareplayback/utils
package still in this PR, we should drop it.d3832af5ba
to6da59eccfc
6da59eccfc
to6c5cef9205
6c5cef9205
toa53e8d0604
Let's merge it after #477 (comment)
a53e8d0604
to0fb2168418
New commits pushed, approval review dismissed automatically according to repository settings
0fb2168418
to62615d7ab7