[#369] Request reproducer #443
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
5 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: TrueCloudLab/frostfs-s3-gw#443
Loading…
Reference in a new issue
No description provided.
Delete branch "nzinkevich/frostfs-s3-gw:feature/playback"
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?
Signed-off-by: Nikita Zinkevich n.zinkevich@yadro.com
@ -0,0 +90,4 @@
return authinfo, nil
}
/*func getHexSHA256(r *http.Request) (string, error) {
do we need this?
I will use this later to replace the 'X-Amz-Content-SHA256' header. This is useful to ensure that the hash relates to the actual body of the request, which can sometimes be incomplete when saved in logs.
Then we should add this function when we will actually use it rather than keep commented code.
we'll also need playback.md file in docs dir, similar to authmate.md
@ -0,0 +17,4 @@
cmd.PrintErrf("Run '%v --help' for usage.\n", cmd.CommandPath())
os.Exit(1)
}
}
blank line
@ -0,0 +46,4 @@
rootCmd.PersistentFlags().String(configFlag, "", "Configuration file")
_ = rootCmd.MarkFlagRequired(configFlag)
rootCmd.AddCommand(runCmd)
}
blank line
@ -0,0 +49,4 @@
func run(_ *cobra.Command, _ []string) error {
var logdata request.LoggedRequest
var creds = request.Credentials{
AccessKey: viper.GetString("credentials.access_key"),
credentials.access_key & credentials.secret_key
should be constants
@ -0,0 +120,4 @@
return nil, err
}
return r, nil
}
blank line
@ -0,0 +1,6 @@
package request
type Credentials struct {
Think we don't need separate file for Credentials struct
@ -0,0 +3,4 @@
type Credentials struct {
AccessKey string
SecretKey string
}
blank line
@ -0,0 +49,4 @@
}
return nil
}
blank line
@ -0,0 +12,4 @@
"git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api"
"git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/auth"
v4 "github.com/aws/aws-sdk-go-v2/aws/signer/v4"
we have own signer v4
I chose AWS SDK v2 because it has the Sign HTTP method, which is convenient for updating the S3 request signature. SDK v2 is already set in the go.mod file, but I also added the "github.com/aws/aws-sdk-go-v2/aws/credentials" package. @dkirillov
@ -0,0 +17,4 @@
)
const (
EmptyStringHash = "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"
no usages
@ -0,0 +25,4 @@
Short: "Reads network log file and executes requests to specified URL",
SilenceUsage: true,
RunE: run,
}
Please fill
Example
field@ -0,0 +32,4 @@
runCmd.Flags().String(endpointFlag, "", "Endpoint URL")
}
var requestID = 1
Try to not use global varialbe
@ -0,0 +35,4 @@
var requestID = 1
func logResponse(method, url, status string, body []byte, values ...any) {
fmt.Println(strconv.Itoa(requestID)+")", method, url, status)
It's better to use
Println
function fromcmd *cobra.Command
@ -0,0 +47,4 @@
}
func run(_ *cobra.Command, _ []string) error {
var logdata request.LoggedRequest
Let's move this near to the place where it's used. (e.g. before or after line
for sc.Scan() {
)@ -0,0 +48,4 @@
func run(_ *cobra.Command, _ []string) error {
var logdata request.LoggedRequest
var creds = request.Credentials{
We can omit
var
:@ -0,0 +52,4 @@
AccessKey: viper.GetString("credentials.access_key"),
SecretKey: viper.GetString("credentials.secret_key"),
}
ctx := context.TODO()
We should use context from command.
@ -0,0 +69,4 @@
defer logfile.Close()
sc := bufio.NewScanner(logfile)
sc.Buffer(buf, bufSize)
It's quite odd to set buffer that has size of full file to read. I believe we can skip setting this param at all (or we can configure this buffer size).
@ -0,0 +73,4 @@
for sc.Scan() {
if err = json.Unmarshal(sc.Bytes(), &logdata); err != nil {
fmt.Println("Error parsing log file:", err)
Please, use
cmd.Println
@ -0,0 +86,4 @@
func playback(ctx context.Context, logContent request.LoggedRequest) {
r, err := prepareRequest(ctx, logContent)
if err != nil {
logResponse(logContent.Method, logContent.URI, "", nil, "failed to prepare response:", err)
It seems message should be
failed to prepare request:
@ -0,0 +89,4 @@
logResponse(logContent.Method, logContent.URI, "", nil, "failed to prepare response:", err)
return
}
resp, err := http.DefaultClient.Do(r)
Don't use default client. It has no timeout so in theory it can hang out
@ -0,0 +107,4 @@
var r *http.Request
var err error
r, err = request.NewRequest(viper.GetString(endpointFlag), data)
Use:
@ -0,0 +115,4 @@
if err != nil {
return nil, err
}
err = request.Sign(ctx, r)
Why don't we invoke
request.SwapUploadID
andrequest.Sign
insiderequest.NewRequest
?request.NewRequest was removed, so considered to leave these functions in the prepareRequest
@ -0,0 +8,4 @@
)
var (
multiparts = make(map[string]MultipartUpload)
Try to not use global varialbe
@ -0,0 +18,4 @@
UploadID string `json:"uploadId" xml:"UploadId"`
}
func RegisterMultipart(r *http.Request, resp []byte, loggedResponse []byte) error {
It's better to rename this function to something like
HandleResponse
@ -0,0 +33,4 @@
Header http.Header `json:"headers"`
Response []byte `json:"response"`
}
ContextKey string
Let's write something like
bcaee6b499
to407dd4b63c
407dd4b63c
to924f3c0b75
924f3c0b75
to71abf7089d
71abf7089d
toc8d0e7f855
c8d0e7f855
to4d6da10eda
@ -40,3 +49,4 @@
CGO_ENABLED=0 \
go build -v -trimpath \
-ldflags "-X $(REPO)/internal/version.Version=$(VERSION)" \
-tags "$(BUILD_TAGS)" \
Let's keep it and use it directly like
make BUILD_TAGS=loghttp
. I think this is quite robust and can be reused for any number of build-tags that can be introduced later. Therefore removeloghttp
andimage-loghttp
and updatedocker/%
target:@ -0,0 +18,4 @@
"git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api"
"git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/auth"
request2 "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/cmd/s3-playback/request"
In case of package name collision, try to provide context to an alias name instead of putting 2 in the end.
In this case we can name it
s3request
.@ -0,0 +148,4 @@
// prepareRequest creates request from logs and modifies its signature and uploadId (if presents).
func prepareRequest(cmd *cobra.Command, logReq request2.LoggedRequest) (*http.Request, error) {
r, err := http.NewRequest(logReq.Method, viper.GetString(endpointFlag)+logReq.URI,
We definitely should use
http.NewRequestWithContext
here. Network request may take some time, we don't want to wait in case ofSIGTERM
or any other context cancellation.@ -0,0 +17,4 @@
)
// authorizationFieldRegexp -- is regexp for credentials with Base58 encoded cid and oid and '0' (zero) as delimiter.
var 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>.+)`)
We need some unit-tests to validate this regexp, just in case.
Maybe we can also reuse this regexp from
api/auth/center.go
@ -0,0 +64,4 @@
viper.GetString(awsAccessKey),
viper.GetString(awsSecretKey),
)
ctx = request2.WithMultiparts(ctx)
I think this isn't necessary. We can create new map in
SetMultipartUpload
if it doesn't exist yetBut then SetMultipartUpload should also return an updated context or I should pass a *cobra.Command argument to it for SetContext(ctx). I think both ways look worse.
@ -0,0 +78,4 @@
client := &http.Client{
Transport: http.DefaultTransport,
Timeout: time.Second * time.Duration(viper.GetInt64(httpTimeout)),
We can use
viper.GetDuration
4d6da10eda
toac0fe42d29
ac0fe42d29
to963a378b9e
WIP: [#369] Request reproducerto [#369] Request reproducer963a378b9e
to6a064d524c
@ -0,0 +65,4 @@
)
ctx = request.WithMultiparts(ctx)
cmd.SetContext(ctx)
Why do we do this?
It seems we can pass only context to
playback
function6a064d524c
to8d830aec5f
@ -0,0 +41,4 @@
func init() {
rootCmd.PersistentFlags().StringP(configPathFlag, "c", "",
"configuration filepath")
Let's keep statement in one line
@ -0,0 +46,4 @@
_ = rootCmd.MarkPersistentFlagFilename(configPathFlag)
_ = viper.BindPFlag(configPath, rootCmd.PersistentFlags().Lookup(configPathFlag))
rootCmd.PersistentFlags().String(httpTimeoutFlag, "60",
@ -0,0 +52,4 @@
rootCmd.PersistentFlags().Bool(skipVerifyTLSFlag, false,
"skip TLS certificate verification")
_ = viper.BindPFlag(skipVerifyTLS, rootCmd.PersistentFlags().Lookup(skipVerifyTLSFlag))
Maybe we can move flag binding into
PersistentPreRunE
ofrootCmd
?@ -40,6 +44,7 @@ $(BINS): $(BINDIR) dep
CGO_ENABLED=0 \
go build -v -trimpath \
-ldflags "-X $(REPO)/internal/version.Version=$(VERSION)" \
-tags "$(BUILD_TAGS)" \
There is a more generic solution called
GOFLAGS
, see here, for exampletest: GOFLAGS ?= "-cover -count=1"
It may play nicely with build systems, because GOFLAGS is a well-known env variable.
8d830aec5f
to982ea1f44c
982ea1f44c
to7532afd237
7532afd237
toe5494f4383
@ -379,1 +379,4 @@
Could be enabled only in builds with `loghttp` build tag. This could be done via `loghttp`,
`docker/loghttp` or `image-loghttp` Make targets.
Needs to be updated with new make command.
@ -13,3 +13,3 @@
# Show help for docker/% IGNORE
help.docker/%:
$(eval TARGETS:=$(notdir all lint) ${BINS})
$(eval TARGETS:=$(notdir all lint loghttp) ${BINS})
Doesn't need anymore.
e5494f4383
toa17c0d701b