Skip to content
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

Fix hostname blocking #1723

Merged
merged 2 commits into from
Nov 17, 2020
Merged

Fix hostname blocking #1723

merged 2 commits into from
Nov 17, 2020

Conversation

na--
Copy link
Member

@na-- na-- commented Nov 16, 2020

This should close #1721 (cc @curvedriver). There were 2 separate bugs... 😞 Wildcards didn't catch 0-length matches, and when you blocked both a domain and a sub.domain, the HostnameTrie stopped blocking the domain because the len(t.children) == 0 check was no longer true (wrong assumption that you can't block both a top-level and a sub-domain). This script more succinctly demonstrates the issue, running it with k6 0.29.0 will fail all checks, whereas with this PR they all pass:

import http from 'k6/http';
import { check, group } from "k6";

export const options = {
    blockHostnames: ['*httpbin.org', 'test.k6.io', 'k6.io']
}

export default function getFromBlockedHostname() {
    [
        'https://httpbin.org/get?my_param=my_value',
        'https://k6.io/',
    ].forEach(url =>
        group(url, () => {
            check(http.get(url, { redirects: 0 }), {
                "status is 0": (r) => r.status === 0,
                "error_code is 1111": (r) => r.error_code === 1111,
            });
        }));
}

@na-- na-- added this to the v0.30.0 milestone Nov 16, 2020
@na-- na-- requested review from mstoykov and imiric November 16, 2020 12:04
if len(s) == 0 {
t.isLeaf = true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this become ... false if it goes beyond this ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand - the next line is return nil, so it can't go beyond this 😕

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant that if on sequential call to insert on a node that is already a leaf ... we won't unleaf it ... which I now understand is exactly what this is fixing (but I needed to read the whole code ...).

I am somewhat of the opinion that if there is "*valid.test" and "test.valid,test" we should probably not add the second one to tree ... and/or give out a warning/error that there is a more general glob already.

But this can be done as an optimization in later PR and I am not particularly opinioanted about it ...

Copy link
Contributor

@imiric imiric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nice work! 👍

@na-- na-- merged commit 6081e6a into master Nov 17, 2020
@na-- na-- deleted the fix-hostname-blocking branch November 17, 2020 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

blockHostnames- Can't block hostname with and without subdomain at the same time
3 participants