-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support multipart/form-data #1115
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Arash Hatami <[email protected]>
Signed-off-by: Arash Hatami <[email protected]>
Signed-off-by: Arash Hatami <[email protected]>
Signed-off-by: Arash Hatami <[email protected]>
any update? |
7de228d
to
e76f9c3
Compare
Signed-off-by: Arash Hatami <[email protected]>
Signed-off-by: Arash Hatami <[email protected]>
Signed-off-by: Arash Hatami <[email protected]>
Signed-off-by: Arash Hatami <[email protected]>
Signed-off-by: Arash Hatami <[email protected]>
Hello Do we have an update for this PR? The latest changes have been synced and new tests have been added for multipart. Unfortunately, the IPv6 tests did not pass, and #1342 will solve the problem. Otherwise, a separate update is needed. |
@@ -424,6 +426,60 @@ func ProbeHTTP(ctx context.Context, target string, module config.Module, registr | |||
body = body_file | |||
} | |||
|
|||
// If a multipart form is configured, add it to the request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this implementation and configuration feels a little awkward, can we make it simple and implement this with just one bool flag called multipart
?
If the flag is set, you can just make the check multi-part. for text and data, you can use the existing Body
and BodyFile
config values?
this would make it easy to configure and simplify this implementation as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a multipart request, submitting multiple files (not just one file or text) should be possible. This is not possible using Body
and BodyFile
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read up some literature more on the multipart, and I still don't understand this code.
type Multipart struct {
Type string `yaml:"type,omitempty"`
Key string `yaml:"key,omitempty"`
Value string `yaml:"value,omitempty"`
}
the part I am having hard time understanding is the why behind Multipart
struct and the Type
, and the Key
and Value
in it.
do you need multiple text fields? if yes, can you share more info on it?
I think it should be possible to have it something like. MultipartFiles []string
, and pass multiple filenames for the multi-part check, and append them to the Body and use the Body
or BodyText for the body text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imagine a scenario where two files and two texts are to be sent. Consider the following curl example:
curl --location 'https://domain.com' \
--form 'text-key-1="text-value-1"' \
--form 'text-key-2="text-value-2"' \
--form 'file-key-1=@"/files/file-1.jpeg"' \
--form 'file-key-2=@"/files/file-2.jpeg"'
If we want to implement something like this, we need a separate structure to set the Type
(text/file), Key
, and Value
.
http_post_body_multipart:
prober: http
timeout: 5s
http:
method: POST
body_multipart:
- type: "text-key-1"
key: "name"
value: "text-value-1"
- type: "text-key-2"
key: "name"
value: "text-value-2"
- type: "file"
key: "file-key-1"
value: "/files/file-1.jpeg"
- type: "file"
key: "file-key-2"
value: "/files/file-2.jpeg"
@@ -1554,3 +1555,108 @@ func TestBody(t *testing.T) { | |||
} | |||
} | |||
} | |||
|
|||
// Test that the HTTP probe module can handle a multipart form request |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this test can also be simplified after the implementation is updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As explained in the previous comment, we need to be able to POST different files/text. For this purpose, the bodyMultipart
variable has been defined and used in this test.
for CI failure: #1342 is merged, please rebase. |
Rebased. |
Description
After this PR, we can send POST requests in
multipart/form-data
format. This allows users to test the upload process quickly.We can define
text
orfile
fields here.Changes
multipart
for HTTP probe