[#369] Request reproducer #443

Merged
nzinkevich merged 1 commit from nzinkevich/frostfs-s3-gw:feature/playback into feature/369 2024-09-04 19:51:14 +00:00
Member

Signed-off-by: Nikita Zinkevich n.zinkevich@yadro.com

Signed-off-by: Nikita Zinkevich <n.zinkevich@yadro.com>
nzinkevich added 1 commit 2024-07-24 11:55:45 +00:00
[#369] Request reproducer
All checks were successful
/ DCO (pull_request) Successful in 56s
/ Vulncheck (pull_request) Successful in 1m8s
/ Builds (1.20) (pull_request) Successful in 1m50s
/ Builds (1.21) (pull_request) Successful in 1m38s
/ Lint (pull_request) Successful in 2m7s
/ Tests (1.20) (pull_request) Successful in 2m0s
/ Tests (1.21) (pull_request) Successful in 1m57s
bcaee6b499
Signed-off-by: Nikita Zinkevich <n.zinkevich@yadro.com>
nzinkevich requested review from pogpp 2024-07-24 11:56:04 +00:00
pogpp reviewed 2024-07-24 13:34:17 +00:00
@ -0,0 +90,4 @@
return authinfo, nil
}
/*func getHexSHA256(r *http.Request) (string, error) {
Member

do we need this?

do we need this?
Author
Member

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.

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

Then we should add this function when we will actually use it rather than keep commented code.

Then we should add this function when we will actually use it rather than keep commented code.
dkirillov marked this conversation as resolved
nzinkevich requested review from alexvanin 2024-07-25 11:13:03 +00:00
nzinkevich requested review from dkirillov 2024-07-25 11:13:03 +00:00
Member

we'll also need playback.md file in docs dir, similar to authmate.md

we'll also need playback.md file in docs dir, similar to [authmate.md](https://git.frostfs.info/TrueCloudLab/frostfs-s3-gw/src/branch/master/docs/authmate.md)
pogpp requested changes 2024-07-25 11:44:11 +00:00
@ -0,0 +17,4 @@
cmd.PrintErrf("Run '%v --help' for usage.\n", cmd.CommandPath())
os.Exit(1)
}
}
Member

blank line

blank line
@ -0,0 +46,4 @@
rootCmd.PersistentFlags().String(configFlag, "", "Configuration file")
_ = rootCmd.MarkFlagRequired(configFlag)
rootCmd.AddCommand(runCmd)
}
Member

blank line

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

credentials.access_key & credentials.secret_key
should be constants

credentials.access_key & credentials.secret_key should be constants
@ -0,0 +120,4 @@
return nil, err
}
return r, nil
}
Member

blank line

blank line
@ -0,0 +1,6 @@
package request
type Credentials struct {
Member

Think we don't need separate file for Credentials struct

Think we don't need separate file for Credentials struct
@ -0,0 +3,4 @@
type Credentials struct {
AccessKey string
SecretKey string
}
Member

blank line

blank line
@ -0,0 +49,4 @@
}
return nil
}
Member

blank line

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

we have own signer v4

we have own [signer](https://git.frostfs.info/TrueCloudLab/frostfs-s3-gw/src/branch/master/api/auth/signer/v4/v4.go#L167) v4
Author
Member

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

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

no usages

no usages
dkirillov reviewed 2024-07-26 09:42:59 +00:00
@ -0,0 +25,4 @@
Short: "Reads network log file and executes requests to specified URL",
SilenceUsage: true,
RunE: run,
}
Member

Please fill Example field

Please fill `Example` field
dkirillov marked this conversation as resolved
@ -0,0 +32,4 @@
runCmd.Flags().String(endpointFlag, "", "Endpoint URL")
}
var requestID = 1
Member

Try to not use global varialbe

Try to not use global varialbe
dkirillov marked this conversation as resolved
@ -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)
Member

It's better to use Println function from cmd *cobra.Command

It's better to use `Println` function from `cmd *cobra.Command`
dkirillov marked this conversation as resolved
@ -0,0 +47,4 @@
}
func run(_ *cobra.Command, _ []string) error {
var logdata request.LoggedRequest
Member

Let's move this near to the place where it's used. (e.g. before or after line for sc.Scan() {)

Let's move this near to the place where it's used. (e.g. before or after line `for sc.Scan() {`)
dkirillov marked this conversation as resolved
@ -0,0 +48,4 @@
func run(_ *cobra.Command, _ []string) error {
var logdata request.LoggedRequest
var creds = request.Credentials{
Member

We can omit var:

creds := request.Credentials{/*...*/}
We can omit `var`: ``` creds := request.Credentials{/*...*/} ```
dkirillov marked this conversation as resolved
@ -0,0 +52,4 @@
AccessKey: viper.GetString("credentials.access_key"),
SecretKey: viper.GetString("credentials.secret_key"),
}
ctx := context.TODO()
Member

We should use context from command.

func run (cmd *cobra.Command, _ []string) error {
    // ...
    ctx := context.WithValue(cmd.Context(), ...)
    // ...
}
We should use context from command. ``` func run (cmd *cobra.Command, _ []string) error { // ... ctx := context.WithValue(cmd.Context(), ...) // ... } ```
dkirillov marked this conversation as resolved
@ -0,0 +69,4 @@
defer logfile.Close()
sc := bufio.NewScanner(logfile)
sc.Buffer(buf, bufSize)
Member

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

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).
dkirillov marked this conversation as resolved
@ -0,0 +73,4 @@
for sc.Scan() {
if err = json.Unmarshal(sc.Bytes(), &logdata); err != nil {
fmt.Println("Error parsing log file:", err)
Member

Please, use cmd.Println

Please, use `cmd.Println`
dkirillov marked this conversation as resolved
@ -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)
Member

It seems message should be failed to prepare request:

It seems message should be `failed to prepare request:`
dkirillov marked this conversation as resolved
@ -0,0 +89,4 @@
logResponse(logContent.Method, logContent.URI, "", nil, "failed to prepare response:", err)
return
}
resp, err := http.DefaultClient.Do(r)
Member

Don't use default client. It has no timeout so in theory it can hang out

Don't use default client. It has no timeout so in theory it can hang out
dkirillov marked this conversation as resolved
@ -0,0 +107,4 @@
var r *http.Request
var err error
r, err = request.NewRequest(viper.GetString(endpointFlag), data)
Member

Use:

r, err := request.NewRequest(viper.GetString(endpointFlag), data)
Use: ``` r, err := request.NewRequest(viper.GetString(endpointFlag), data) ```
dkirillov marked this conversation as resolved
@ -0,0 +115,4 @@
if err != nil {
return nil, err
}
err = request.Sign(ctx, r)
Member

Why don't we invoke request.SwapUploadID and request.Sign inside request.NewRequest?

Why don't we invoke `request.SwapUploadID` and `request.Sign` inside `request.NewRequest`?
Author
Member

request.NewRequest was removed, so considered to leave these functions in the prepareRequest

request.NewRequest was removed, so considered to leave these functions in the prepareRequest
@ -0,0 +8,4 @@
)
var (
multiparts = make(map[string]MultipartUpload)
Member

Try to not use global varialbe

Try to not use global varialbe
dkirillov marked this conversation as resolved
@ -0,0 +18,4 @@
UploadID string `json:"uploadId" xml:"UploadId"`
}
func RegisterMultipart(r *http.Request, resp []byte, loggedResponse []byte) error {
Member

It's better to rename this function to something like HandleResponse

It's better to rename this function to something like `HandleResponse`
dkirillov marked this conversation as resolved
@ -0,0 +33,4 @@
Header http.Header `json:"headers"`
Response []byte `json:"response"`
}
ContextKey string
Member

Let's write something like

type credKeyType struct{}

func SetCredentials(ctx context.Context, creds Credentials) context.Context {
	return context.WithValue(ctx, credKeyType{}, creds)
}

func GetCredentials(ctx context.Context) (Credentials, error) {
	creds, ok := ctx.Value(credKeyType{}).(Credentials)
	if !ok {
		return Credentials{}, errors.New("credentials is missing in contex")
	}

	return creds, nil
}
Let's write something like ``` type credKeyType struct{} func SetCredentials(ctx context.Context, creds Credentials) context.Context { return context.WithValue(ctx, credKeyType{}, creds) } func GetCredentials(ctx context.Context) (Credentials, error) { creds, ok := ctx.Value(credKeyType{}).(Credentials) if !ok { return Credentials{}, errors.New("credentials is missing in contex") } return creds, nil } ```
dkirillov marked this conversation as resolved
nzinkevich force-pushed feature/playback from bcaee6b499 to 407dd4b63c 2024-07-30 06:54:45 +00:00 Compare
nzinkevich force-pushed feature/playback from 407dd4b63c to 924f3c0b75 2024-07-30 06:57:46 +00:00 Compare
nzinkevich requested review from pogpp 2024-07-30 07:20:07 +00:00
nzinkevich force-pushed feature/playback from 924f3c0b75 to 71abf7089d 2024-07-30 07:43:33 +00:00 Compare
nzinkevich force-pushed feature/playback from 71abf7089d to c8d0e7f855 2024-07-30 08:10:32 +00:00 Compare
nzinkevich force-pushed feature/playback from c8d0e7f855 to 4d6da10eda 2024-07-30 11:36:56 +00:00 Compare
alexvanin requested changes 2024-07-30 13:29:04 +00:00
Makefile Outdated
@ -40,3 +49,4 @@
CGO_ENABLED=0 \
go build -v -trimpath \
-ldflags "-X $(REPO)/internal/version.Version=$(VERSION)" \
-tags "$(BUILD_TAGS)" \
Owner

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 remove loghttp and image-loghttp and update docker/% target:

docker/%:
	$(if $(filter $*,all $(BINS)), \
		@echo "=> Running 'make $*' in clean Docker environment" && \
		docker run --rm -t \
		  -v `pwd`:/src \
		  golang:$(GO_VERSION) make $* BUILD_TAGS=$(BUILD_TAGS),\
	  	@echo "supported docker targets: all $(BINS) lint")
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 remove `loghttp` and `image-loghttp` and update `docker/%` target: ``` docker/%: $(if $(filter $*,all $(BINS)), \ @echo "=> Running 'make $*' in clean Docker environment" && \ docker run --rm -t \ -v `pwd`:/src \ golang:$(GO_VERSION) make $* BUILD_TAGS=$(BUILD_TAGS),\ @echo "supported docker targets: all $(BINS) lint") ```
alexvanin marked this conversation as resolved
@ -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"
Owner

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.

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

We definitely should use http.NewRequestWithContext here. Network request may take some time, we don't want to wait in case of SIGTERM or any other context cancellation.

We definitely should use `http.NewRequestWithContext` here. Network request may take some time, we don't want to wait in case of `SIGTERM` or any other context cancellation.
alexvanin marked this conversation as resolved
@ -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>.+)`)
Owner

We need some unit-tests to validate this regexp, just in case.

We need some unit-tests to validate this regexp, just in case.
Member

Maybe we can also reuse this regexp from api/auth/center.go

Maybe we can also reuse this regexp from `api/auth/center.go`
alexvanin marked this conversation as resolved
dkirillov reviewed 2024-07-31 09:51:08 +00:00
@ -0,0 +64,4 @@
viper.GetString(awsAccessKey),
viper.GetString(awsSecretKey),
)
ctx = request2.WithMultiparts(ctx)
Member

I think this isn't necessary. We can create new map in SetMultipartUpload if it doesn't exist yet

I think this isn't necessary. We can create new map in `SetMultipartUpload` if it doesn't exist yet
Author
Member

But 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.

But 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.
dkirillov marked this conversation as resolved
@ -0,0 +78,4 @@
client := &http.Client{
Transport: http.DefaultTransport,
Timeout: time.Second * time.Duration(viper.GetInt64(httpTimeout)),
Member

We can use viper.GetDuration

We can use `viper.GetDuration`
nzinkevich force-pushed feature/playback from 4d6da10eda to ac0fe42d29 2024-07-31 10:17:37 +00:00 Compare
nzinkevich force-pushed feature/playback from ac0fe42d29 to 963a378b9e 2024-07-31 10:30:25 +00:00 Compare
nzinkevich changed title from WIP: [#369] Request reproducer to [#369] Request reproducer 2024-07-31 10:34:04 +00:00
nzinkevich force-pushed feature/playback from 963a378b9e to 6a064d524c 2024-07-31 11:07:25 +00:00 Compare
nzinkevich requested review from alexvanin 2024-07-31 11:47:49 +00:00
dkirillov reviewed 2024-07-31 12:49:36 +00:00
@ -0,0 +65,4 @@
)
ctx = request.WithMultiparts(ctx)
cmd.SetContext(ctx)
Member

Why do we do this?
It seems we can pass only context to playback function

Why do we do this? It seems we can pass only context to `playback` function
dkirillov marked this conversation as resolved
nzinkevich force-pushed feature/playback from 6a064d524c to 8d830aec5f 2024-07-31 13:15:40 +00:00 Compare
dkirillov reviewed 2024-07-31 13:32:34 +00:00
@ -0,0 +41,4 @@
func init() {
rootCmd.PersistentFlags().StringP(configPathFlag, "c", "",
"configuration filepath")
Member

Let's keep statement in one line

Let's keep statement in one line
dkirillov reviewed 2024-07-31 13:33:35 +00:00
@ -0,0 +46,4 @@
_ = rootCmd.MarkPersistentFlagFilename(configPathFlag)
_ = viper.BindPFlag(configPath, rootCmd.PersistentFlags().Lookup(configPathFlag))
rootCmd.PersistentFlags().String(httpTimeoutFlag, "60",
Member
rootCmd.PersistentFlags().Duration(httpTimeoutFlag, time.Minute, "http request timeout")
```golang rootCmd.PersistentFlags().Duration(httpTimeoutFlag, time.Minute, "http request timeout") ````
dkirillov reviewed 2024-07-31 13:40:17 +00:00
@ -0,0 +52,4 @@
rootCmd.PersistentFlags().Bool(skipVerifyTLSFlag, false,
"skip TLS certificate verification")
_ = viper.BindPFlag(skipVerifyTLS, rootCmd.PersistentFlags().Lookup(skipVerifyTLSFlag))
Member

Maybe we can move flag binding into PersistentPreRunE of rootCmd?

Maybe we can move flag binding into `PersistentPreRunE` of `rootCmd`?
fyrchik reviewed 2024-08-01 06:24:30 +00:00
Makefile Outdated
@ -40,6 +44,7 @@ $(BINS): $(BINDIR) dep
CGO_ENABLED=0 \
go build -v -trimpath \
-ldflags "-X $(REPO)/internal/version.Version=$(VERSION)" \
-tags "$(BUILD_TAGS)" \
Owner

There is a more generic solution called GOFLAGS, see here, for example

test: GOFLAGS ?= "-cover -count=1"

It may play nicely with build systems, because GOFLAGS is a well-known env variable.

There is a more generic solution called `GOFLAGS`, see here, for example https://git.frostfs.info/TrueCloudLab/frostfs-sdk-go/src/commit/9da46f566fec219c0f52450c887c47fc0205340a/Makefile#L11 It may play nicely with build systems, because GOFLAGS is a well-known env variable.
nzinkevich force-pushed feature/playback from 8d830aec5f to 982ea1f44c 2024-08-01 08:26:15 +00:00 Compare
nzinkevich force-pushed feature/playback from 982ea1f44c to 7532afd237 2024-08-01 08:29:48 +00:00 Compare
nzinkevich force-pushed feature/playback from 7532afd237 to e5494f4383 2024-08-01 08:37:38 +00:00 Compare
alexvanin approved these changes 2024-08-01 10:35:56 +00:00
@ -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.
Owner

Needs to be updated with new make command.

Needs to be updated with new make command.
help.mk Outdated
@ -13,3 +13,3 @@
# Show help for docker/% IGNORE
help.docker/%:
$(eval TARGETS:=$(notdir all lint) ${BINS})
$(eval TARGETS:=$(notdir all lint loghttp) ${BINS})
Owner

Doesn't need anymore.

Doesn't need anymore.
nzinkevich force-pushed feature/playback from e5494f4383 to a17c0d701b 2024-08-01 11:31:52 +00:00 Compare
nzinkevich requested review from alexvanin 2024-08-01 11:35:26 +00:00
alexvanin approved these changes 2024-08-01 12:08:19 +00:00
nzinkevich merged commit a17c0d701b into feature/369 2024-08-01 12:47:22 +00:00
nzinkevich deleted branch feature/playback 2024-08-01 12:47:24 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
5 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#443
No description provided.