search

puma / puma-dev

A tool to manage rack apps in development with puma
BSD 3-Clause "New" or "Revised" License
1.73k stars 106 forks source link

Confirm, test, and document subdomain support #286

Closed nonrational closed 2 years ago

nonrational commented 2 years ago

foo.hipuma.test, bar.hipuma.test, and hipuma.test should all resolve to hipuma.

The functionality described by 3f601b0 and tested in 2aed6ee actually works pretty reliably for me and is important for my use case.

FWIW, anecdotally, I've found that the behavior works as described in the README when using symlinks to point to directories, but does not work when using the "proxy" support by writing a file with a host/port.

https://github.com/puma/puma-dev/pull/285#issuecomment-956315897

cjlarose commented 2 years ago

Ok so I started looking into this briefly and I realized that in my previous comment I was wrong about the functionality described by the README in 3f601b07d762074b2d9c21f303309cfc183354fd being related to the test in 2aed6eeb372a2223a3f5d9cf49cbc11e3e98ac41 Edit: After revisiting this I think I was right to conclude originally that those commits are related. It's important to be able to strip the TLD off of Host headers that use subdomains as a prerequisite for being able to use subdomains at all (whether the subdomain points to a distinct app from the main domain or not)

The subdomain functionality as described by 3f601b07d762074b2d9c21f303309cfc183354fd doesn't appear to have any test coverage. Its implementation is in Http.findApp here: https://github.com/puma/puma-dev/blob/06bb09ed93920e9b593314c4c36323fc62a42310/dev/http.go#L82-L94

My WIP branch is here: https://github.com/puma/puma-dev/compare/master...cjlarose:test-subdomain-support

I basically just moved the function over to AppPool since that seemed like a more appropriate place for it to live (open to feedback on this), but also tried to make it easier to add unit tests. That's what I'll try to look into adding next.

cjlarose commented 2 years ago

Opened #289 to add tests for the behavior described in 3f601b07d762074b2d9c21f303309cfc183354fd

I took a look also at the test from 2aed6eeb372a2223a3f5d9cf49cbc11e3e98ac41, but it's not clear to me actually how it passed previously. My expectation would be that, given the configured TLD .co.test, the domain subdomain.confusing-riddle.co.test should be stripped to subdomain.confusing-riddle, not just confusing-riddle.

nonrational commented 2 years ago

Thanks @cjlarose.

That test never passed. I added it to try to ensure we didn't introduce a regression with the new TLD pruning. But, wasn't able to get it to pass and just removed it, incorrectly assuming that the behavior was broken. Turns out the behavior is just fine, but exists elsewhere.

cjlarose commented 2 years ago

Ok that makes sense! Thanks for clarifying @nonrational