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

Add alternate_hosts #10166

Merged
merged 11 commits into from May 1, 2018

Conversation

Projects
None yet
6 participants
@gsnedders
Copy link
Contributor

gsnedders commented Mar 24, 2018

Fixes #2669.

Note this doesn't yet do #8581, but adding a new host at this point is adding a string to the config file, and adding new subdomains is just editing the list in tools/serve/serve.py.

@mikewest: This gives us alt_domains[0] and alt_domains[1], which isn't quite what we were talking about in #8581 (given there's nowhere to put 'this-is-mikes-domain-go-away'). I think giving them names is going to be a fair bit of extra complexity, but let me know what you think?

Also, FWIW, I'm pretty sure this will fail CI. But opening the PR so people can look at it if they want before I touch it again on Monday.


This change is Reviewable

@wpt-pr-bot wpt-pr-bot added the infra label Mar 24, 2018

@wpt-pr-bot wpt-pr-bot requested a review from jgraham Mar 24, 2018

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Mar 24, 2018

Build BROKEN

Started: 2018-04-02 12:00:02
Finished: 2018-04-02 12:13:04

Failing Jobs

  • tools_unittest in py27
  • tools_unittest in py36
  • tools_unittest in pypy
  • wptrunner_infrastructure

View more information about this build on:

@mikewest

This comment has been minimized.

Copy link
Contributor

mikewest commented Mar 26, 2018

@mikewest: This gives us alt_domains[0] and alt_domains[1], which isn't quite what we were talking about in #8581 (given there's nowhere to put 'this-is-mikes-domain-go-away'). I think giving them names is going to be a fair bit of extra complexity, but let me know what you think?

I guess I'd be a little surprised when writing tests if I had to shift over to an entirely different semantic in order to use a different domain. Subdomains of web-platform.test are referenced as {{domains[www]}}. I'd expect this alternate domain to work similarly. That is, {{alt_domains[www]}} would work as a subdomain on the alternate root, or {{domains[alt_www]}} would be the www subdomain on the alternate root.

{{alt_domains[0]}} feels pretty different. Can you help me understand the complexity hiding behind the simple-seeming approaches above?

(Also: I don't actually care about this-is-mikes-domain-go-away. I care about the technical capability to have subdomains of subdomains. The scary name seems interesting from an ergonomic perspective, but that's the least important bit of the request, and I'm totally happy to drop it entirely.)

@gsnedders

This comment has been minimized.

Copy link
Contributor Author

gsnedders commented Mar 26, 2018

@mikewest

So I think there's a few questions here:

  1. How do we refer to the new alternate hosts from the original one (esp. given all tests are loaded from web-platform.test:8000)?
  2. How do we refer to a subdomain on the current top-level host (for lack of a better term)?
  3. How do we refer to the original host from the alternate ones?
  4. How do we refer to alternate hosts from each other?

Half of this is complicated by the expectation that we'll have multiple alternate hosts (i.e., we can't just go with alt_domains[www] because which alt host is this referring to?).

The current behaviour in this PR is:

  1. alt_domains[0][]
  2. You don't reasonably (there's no way to find out what alternate host you're on)
  3. domains[]
  4. See 2.

Most of that is driven by what makes implementation simple.

I'm not entirely sure what I want the answers to be; my gut says:

  1. alt_domains[0][]
  2. domains[www]
  3. domains[www] (um, that's impossible along with 2); thoughts?
  4. Not sure? I don't know how much this matters though?
@mikewest

This comment has been minimized.

Copy link
Contributor

mikewest commented Mar 26, 2018

  1. How do we refer to the new alternate hosts from the original one (esp. given all tests are loaded from web-platform.test:8000)?
  2. How do we refer to a subdomain on the current top-level host (for lack of a better term)?
  3. How do we refer to the original host from the alternate ones?

One way to answer this question is to stop thinking of {{domains[...]}} as referencing a subdomain, but as referencing an entire domain (maybe we could rename it "host" or something?). Then {{domains[www]}} => www.web-platform.test, while {{domains[alt_www]}} => www.not-web-platform.test.

  1. How do we refer to alternate hosts from each other?

I think some of this logic needs to be hard-coded in the test. Like, we know that tests open up on web-platform.test, and that any reference to {{domains[...]}} will be "somethign else". I think it's probably reasonable to put the onus on the developer to track where they are/need to be via window.open or navigation, and to load the right {{domains[...]}} accordingly.

I guess it's not clear to me whether we really need another alternate host. I guess having two registrable domains suggests that we might need more than two? But two is different in kind from one in a way that three might not me. That is, I think I can perform all the cookie tests I care about from one registrable domain to another. Adding a third wouldn't help me in the tests I know I need to write today.

@jgraham

This comment has been minimized.

Copy link
Contributor

jgraham commented Mar 26, 2018

So changing the semantics of {{domains[]}} is probably risky given that it's used rather widely; a simple grep returned 230+ matches.

One could do some magic to make a two argument form work like {{domains[alt,www]}} or {{domains[alt][www]}}. Or one could make up a new thing like {{hosts[alt][www]}}. I think I agree that the numbered thing is bad, unless the idea is that all alternate domains are interchangable.

@gsnedders

This comment has been minimized.

Copy link
Contributor Author

gsnedders commented Mar 26, 2018

One way to answer this question is to stop thinking of {{domains[...]}} as referencing a subdomain, but as referencing an entire domain (maybe we could rename it "host" or something?). Then {{domains[www]}} => www.web-platform.test, while {{domains[alt_www]}} => www.not-web-platform.test.

We already have {{ host }} (and it's used all over the place), it's essentially an alias of {{ domains[] }} (i.e., web-platform.test). alt_www is workable if we only have one alt domain or otherwise name them.

I guess it's not clear to me whether we really need another alternate host. I guess having two registrable domains suggests that we might need more than two? But two is different in kind from one in a way that three might not me. That is, I think I can perform all the cookie tests I care about from one registrable domain to another. Adding a third wouldn't help me in the tests I know I need to write today.

My understanding from #2669 and #8581 was that you wanted two new alternate hosts. If you think we might only need one, then that arguably makes this all simpler.

So changing the semantics of {{domains[]}} is probably risky given that it's used rather widely; a simple grep returned 230+ matches.

It's low-risk because any bogus use will currently return 500 Internal Server Error (as with most bogus sub uses).

One could do some magic to make a two argument form work like {{domains[alt,www]}} or {{domains[alt][www]}}. Or one could make up a new thing like {{hosts[alt][www]}}. I think I agree that the numbered thing is bad, unless the idea is that all alternate domains are interchangable.

[alt,www] is no easier/harder to make work than [alt_www]. I'm happy to name them, if that's what people want (presumably all the way to the config file, therefore), if we're going to allow more than one alternate host.

@mikewest

This comment has been minimized.

Copy link
Contributor

mikewest commented Mar 26, 2018

My understanding from #2669 and #8581 was that you wanted two new alternate hosts. If you think we might only need one, then that arguably makes this all simpler.

Sorry for the confusion. I need one. I don't need two. I can imagine features that might need two at some point (if we ever get around to working through the weeds of concepts like site affiliation, for instance, or if folks decide that FIDO's facets were a good idea after all), but the use cases I know of today don't need more than two registrable domains total.

[alt,www] is no easier/harder to make work than [alt_www]. I'm happy to name them, if that's what people want (presumably all the way to the config file, therefore), if we're going to allow more than one alternate host.

If we did that, how about going all the way with something like [www.not-web-platform.test], where the existing names are aliased to [www.web-platform.test]?

@gsnedders

This comment has been minimized.

Copy link
Contributor Author

gsnedders commented Mar 26, 2018

Sorry for the confusion. I need one. I don't need two. I can imagine features that might need two at some point (if we ever get around to working through the weeds of concepts like site affiliation, for instance, or if folks decide that FIDO's facets were a good idea after all), but the use cases I know of today don't need more than two registrable domains total.

To me, that sounds like there's a reasonable chance of needing more than two in the future, and we probably shouldn't therefore go with something that makes ergonomics worse when we add more.

If we did that, how about going all the way with something like [www.not-web-platform.test], where the existing names are aliased to [www.web-platform.test]?

I did consider something like that; I'm still not opposed to it, though it does make linting for web-platform.test harder. 🙂

@jgraham

This comment has been minimized.

Copy link
Contributor

jgraham commented Mar 26, 2018

I wouldn't mind [www] == [www.host] and [www.alternate] but you would need to spell the bare alternate as [.alternate] or have clashing syntax.

I object to putting the full wpt domain in because that's just wrong in some scenarios, and therefore misleading.

@gsnedders gsnedders force-pushed the gsnedders:new-hosts branch from 42db5d8 to 202e318 Mar 28, 2018

@gsnedders gsnedders force-pushed the gsnedders:new-hosts branch from 1689cba to 5d4c659 Apr 2, 2018

@gsnedders gsnedders force-pushed the gsnedders:new-hosts branch 3 times, most recently from a2e8388 to 13b5561 Apr 9, 2018

@mikewest

This comment has been minimized.

Copy link
Contributor

mikewest commented Apr 17, 2018

Any progress on this? There's increased interested in SameSite from some browsers, and it would be really nice if we could share tests rather than rewriting things in our own internal testing frameworks a few times over. :)

@jgraham

This comment has been minimized.

Copy link
Contributor

jgraham commented Apr 17, 2018

The only funtional change I would like here is moving from domains[main_foo] to hosts[][foo] or hosts[main][foo] and hosts[alt][foo]. Apart from that it just needs to pass tests.

I agree this is high priority.

@gsnedders

This comment has been minimized.

Copy link
Contributor Author

gsnedders commented Apr 17, 2018

@mikewest tl;dr: I got ill, and then I was at the CSS WG F2F and am now out in CA for BlinkOn. Still a priority for when I actually write any code.

@gsnedders

This comment has been minimized.

Copy link
Contributor Author

gsnedders commented Apr 25, 2018

The only funtional change I would like here is moving from domains[main_foo] to hosts[][foo] or hosts[main][foo] and hosts[alt][foo]. Apart from that it just needs to pass tests.

And have domains[foo] be equal to hosts[][foo]?

@jgraham

This comment has been minimized.

Copy link
Contributor

jgraham commented Apr 30, 2018

Yes.

@gsnedders gsnedders force-pushed the gsnedders:new-hosts branch from b2bc9bc to cd5bb89 Apr 30, 2018

@gsnedders

This comment has been minimized.

Copy link
Contributor Author

gsnedders commented Apr 30, 2018

@jgraham that's now done

@jgraham

This comment has been minimized.

Copy link
Contributor

jgraham commented Apr 30, 2018

Reviewed 2 of 9 files at r1, 1 of 1 files at r2, 3 of 4 files at r3, 4 of 4 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.


tools/wptserve/wptserve/config.py, line 185 at r4 (raw file):

                        for subdomain in self.subdomains}
            rv[name][""] = host
        return rv

I am a bit scared that changing the return type here is going to break stuff (again).


tools/wptserve/wptserve/pipes.py, line 445 at r4 (raw file):

        elif field == "GET":
            value = FirstWrapper(request.GET)
        elif field == "hosts":

This should be added to the docs.


tools/wptserve/wptserve/pipes.py, line 448 at r4 (raw file):

            value = request.server.config.all_domains
        elif field == "domains":
            value = request.server.config.all_domains[""]

Should we just support "" for the default domain, or should we have a named alias like "default"?


Comments from Reviewable

@gsnedders

This comment has been minimized.

Copy link
Contributor Author

gsnedders commented Apr 30, 2018

Review status: 7 of 11 files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.


tools/wptserve/wptserve/config.py, line 185 at r4 (raw file):

Previously, jgraham wrote…

I am a bit scared that changing the return type here is going to break stuff (again).

There isn't that much that uses domains (or not_domains, etc.); you can quite easily grep through all of it and check.


tools/wptserve/wptserve/pipes.py, line 448 at r4 (raw file):

Previously, jgraham wrote…

Should we just support "" for the default domain, or should we have a named alias like "default"?

I have no strong preference.


Comments from Reviewable

@jgraham

This comment has been minimized.

Copy link
Contributor

jgraham commented Apr 30, 2018

Reviewed 1 of 4 files at r5, 6 of 6 files at r6.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.


tools/wptserve/wptserve/pipes.py, line 448 at r4 (raw file):

Previously, gsnedders (Geoffrey Sneddon) wrote…

I have no strong preference.

I think I err in the direction of having an alias, if it's not too much trouble. But don't worry if it is.


Comments from Reviewable

@wpt-pr-bot wpt-pr-bot added the docs label Apr 30, 2018

gsnedders added some commits Apr 30, 2018

@gsnedders

This comment has been minimized.

Copy link
Contributor Author

gsnedders commented Apr 30, 2018

Review status: 10 of 12 files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.


tools/wptserve/wptserve/pipes.py, line 445 at r4 (raw file):

Previously, jgraham wrote…

This should be added to the docs.

Done.


tools/wptserve/wptserve/pipes.py, line 448 at r4 (raw file):

Previously, jgraham wrote…

I think I err in the direction of having an alias, if it's not too much trouble. But don't worry if it is.

I think I'm now erring against it, given it could easily lead to bugs with code expecting each thing in domains to be unique.


Comments from Reviewable

@jgraham

This comment has been minimized.

Copy link
Contributor

jgraham commented May 1, 2018

Reviewed 2 of 2 files at r7.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


tools/wptserve/wptserve/pipes.py, line 448 at r4 (raw file):

Previously, gsnedders (Geoffrey Sneddon) wrote…

I think I'm now erring against it, given it could easily lead to bugs with code expecting each thing in domains to be unique.

OK, we can do it in a followup if there are issues.


Comments from Reviewable

@jgraham

jgraham approved these changes May 1, 2018

Copy link
Contributor

jgraham left a comment

This looks good; happy for you to land it. Thanks!

@gsnedders gsnedders merged commit dfeea56 into web-platform-tests:master May 1, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gsnedders gsnedders deleted the gsnedders:new-hosts branch May 1, 2018

@mozmark mozmark referenced this pull request May 3, 2018

Merged

Rfc6265bis tests #10822

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.