Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add blocking by hostname patterns (--block-hostname) #1532
Add blocking by hostname patterns (--block-hostname) #1532
Changes from 6 commits
99dae50
8c6f433
9d761f2
9dee57f
9b59100
0e96344
a86dd3e
372f140
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
now that you don't have other properties in this struct, you might be able to have something like
type HostnameTrie map[rune]HostnameTrie
, which might resolve the envconfig issue I posted below. Though I'm not sure if this wouldn't cause other issues 🤷♂️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 tried doing something similar when I was working on 372f140 and ran into some issues. Will try again in the next commit 👍
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.
Unfortunately, you should also implement the
MarshalJSON()
method 😞 k6 needs to be able to marshal all of the options in a JSON format when it's making an archive bundle.Try running
k6 archive --archive-out test.tar --block-hostname="*" github.com/loadimpact/k6/samples/http_get.js
and thenk6 run test.tar
. You'd get an error like this:this is because the
metadata.json
in the archive will have"blockHostnames": {},
in it, which is not the array this code expects.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.
Ah got it! Will go ahead and implement this. Is there anything else I've missed in my changes?
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 don't think so, having
MarshalJSON()
that produces an array should take care of the issue, since your code can already deal with arrays (after the envconfig issue is worked around). I think you don't need to usek6 archive
every time though,k6 inspect --block-hostname="*" github.com/loadimpact/k6/samples/http_get.js
should mostly do the same things but give you faster feedback.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.
Or a unit test that makes an archive on the fly and runs it, for example take a look at https://github.com/loadimpact/k6/blob/4c39bcccf606aa3b4324ec5d4f3adfdfcf7d2fa5/js/runner_test.go#L179-L227
Or some of the other tests that make use of
Runner.MakeArchive()
and run the result: https://github.com/loadimpact/k6/blob/4c39bcccf606aa3b4324ec5d4f3adfdfcf7d2fa5/js/runner.go#L110-L112There 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.
Ah, this is somewhat problematic. Because of a bug in the
envconfig
library, it initializesBlockedHostnames
, so theif opts.BlockedHostnames != nil
check inApply()
will always be true. So, you're never able to set the blocked hostnames in the JS script, becauseApply
will always overwrite them, even if it's with an empty list.Try running this script with k6:
k6 will not block the domain and the request will happen! 😞 The proper fix for this would be to replace the envconfig library with something saner, but that's a huge refactoring task we've been postponing for a long time... 😭 For now, maybe we shouldn't use a pointer here, and instead have the pointer
HostnameTrie
struct? Or, maybe removeenvconfig:"K6_BLOCK_HOSTNAMES"
and add support for environment variables whenever we fix envconfig?See kelseyhightower/envconfig#113 and #1560 and other envconfig issues.
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.
envconfig strikes again. I will experiment with best options here since having configuration through environment variables is desirable for this sort of thing. Worst comes to worst, I will remove the envconfig support.