Adding support for a layer-7 (HTTP(S)) status endpoint #365

mccurdyc · 2022-05-10T19:56:27Z

Signed-off-by: Colton J. McCurdy mccurdyc22@gmail.com

Addresses #364

Testing

GO_VERSION=1.17 make docker-test

mccurdyc · 2022-05-10T19:57:34Z

status_test.go

+
+	response := httptest.NewRecorder()
+	handler := newStatusHandler(func() (net.Conn, error) {
+		u, _ := url.Parse(statusTarget.URL) // NOTE: I tried using statusTarget.Config.Addr instead, but it wasn't set.


Just wanted to call this out because I really don't like this level of complexity in the test.

mccurdyc · 2022-05-10T20:05:46Z

GO_VERSION=1.17 make docker-test
...
=== RUN   TestStatusHandlerListening                                                              
    status_test.go:92: status should return 200 once listening                                    
--- FAIL: TestStatusHandlerListening (0.00s)                                                      
=== RUN   TestStatusHandlerListeningBackendDown                                                   
--- PASS: TestStatusHandlerListeningBackendDown (0.00s)                                           
=== RUN   TestStatusHandlerReloading                                                              
    status_test.go:119: status should return 200 during reload                                    
--- FAIL: TestStatusHandlerReloading (0.00s)

Need to address ☝️

So actually we will address this by updating these tests to be "TCP-specific" as to test the exist functionality of the status handler today I.e., 200 response code from a listening TCP backend. These tests will be renamed to TestStatusHandlerListeningTCP and TestStatusHandlerReloadingTCP.

We will add a new test TestStatusHandlerListeningHTTP and TestStatusHandlerReloadingHTTP that will NOT pass unless the layer-7 path responds with a 200 status code.

In other words, updating the checks to ensure that layer-7 checks only pass if the path they are checking returns a 200 status code.

Signed-off-by: Colton J. McCurdy <mccurdyc22@gmail.com>

…unction Signed-off-by: Colton J. McCurdy <mccurdyc22@gmail.com>

mccurdyc · 2022-05-10T20:58:22Z

Let me know if you want me to squash commits.

mccurdyc · 2022-05-10T21:00:43Z

@csstaub Thanks for your help! ❤️ Let me know what you think about this implementation!

mccurdyc · 2022-05-11T13:42:17Z

@csstaub I've also manually tested via the following steps:

Create a status page

cat <<EOF > status.html 
<html><body><h1>status page</h1></body></html>
EOF

Serve the status page

python -m http.server --bind 0.0.0.0 --directory $(pwd) 8000

Create certs in $(pwd)/out

docker run -it --rm \
--volume $(pwd)/out:/out \
squareup/certstrap \
init \
--expires "10 days" \
--common-name deleteme

Start ghostunnel in server mode pointed at our simple http server serving the status page. Specify the new --status-target flag pointed at http://172.17.0.1:8000/status.html (note: 172.17.0.1 is my docker network interface since inside the container should have access to that and we are binding the simple server to all network interfaces).

docker run --rm -v $(pwd)/out:/out \
-p 8001:8001 \
-p 8002:8002 \
mccurdyc/ghostunnel:latest server \
--unsafe-target \
--target 172.17.0.1:8000 \
--listen 0.0.0.0:8001 \
--status 0.0.0.0:8002 \
--status-target http://172.17.0.1:8000/status.html \
--cert /out/self-sign-ca.crt \
--key /out/self-sign-ca.key \
--cacert /out/self-sign-ca.crt \
--disable-authentication

Curl ghostunnel status endpoint

curl -k https://localhost:8002/_status
{"ok":true,"status":"ok","backend_ok":true,"backend_status":"ok","time":"2022-05-11T13:41:48.86274434Z","hostname":"4fe01a5d2f62","message":"listening","revision":"v1.6.0-18-ga5feb34","compiler":"go1.17.10"}%

mccurdyc · 2022-05-11T13:47:44Z

Just to check existing behavior i.e., a TCP healthcheck by not providing an http status target behaves the same as before:

docker run --rm -v $(pwd)/out:/out -p 8001:8001 -p 8002:8002 mccurdyc/ghostunnel:latest server --unsafe-target --target 172.17.0.1:8000 --listen 0.0.0.0:8001 --status 0.0.0.0:8002 --cert /out/self-sign-ca.crt --key /out/self-sign-ca.key --cacert /out/self-sign-ca.crt --disable-authentication

curl -k https://localhost:8002/_status
{"ok":true,"status":"ok","backend_ok":true,"backend_status":"ok","time":"2022-05-11T13:46:04.134934151Z","hostname":"70d38973ac9c","message":"listening","revision":"v1.6.0-18-ga5feb34","compiler":"go1.17.10"}%

mccurdyc · 2022-05-11T13:49:22Z

What about explicitly providing a TCP target?

docker run --rm -v $(pwd)/out:/out -p 8001:8001 -p 8002:8002 mccurdyc/ghostunnel:latest server --unsafe-target --target 172.17.0.1:8000 --listen 0.0.0.0:8001 --status 0.0.0.0:8002 --status-target 172.17.0.1:8000 --cert /out/self-sign-ca.crt --key /out/self-sign-ca.key --cacert /out/self-sign-ca.crt --disable-authentication

It serves fine, but when you hit the ghostunnel /_status, you get the following response

curl -k https://localhost:8002/_status
{"ok":false,"status":"critical","backend_ok":false,"backend_status":"critical","backend_error":"parse \"172.17.0.1:8000\": first path segment in URL cannot contain colon","time":"2022-05-11T13:47:33.404354207Z","hostname":"5eb47051739a","message":"listening","revision":"v1.6.0-18-ga5feb34","compiler":"go1.17.10"}%

This got me thinking, do we name the flag --status-target-http and do validation to ensure the target is http? Thoughts @csstaub?

Signed-off-by: Colton J. McCurdy <mccurdyc22@gmail.com>

mccurdyc · 2022-05-16T19:12:27Z

I've also pushed the built container image to dockerhub coltonmccurdy/ghostunnel:bf711ee5961b895ad8697041e8b9fa3bc7c7d926.

csstaub · 2022-05-16T21:08:16Z

@mccurdyc Sorry, I will try to get to this this week

mccurdyc · 2022-05-18T10:59:10Z

@csstaub No worries ❤️ ! Let me know if there is anything that I can do to make it easier to review! Thanks again for your help!

codecov · 2022-05-18T14:28:24Z

Codecov Report

Merging #365 (30b07dc) into master (02651e8) will increase coverage by 0.09%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##           master     #365      +/-   ##
==========================================
+ Coverage   79.53%   79.63%   +0.09%     
==========================================
  Files          25       25              
  Lines        1124     1139      +15     
==========================================
+ Hits          894      907      +13     
- Misses        181      182       +1     
- Partials       49       50       +1

Flag	Coverage Δ
darwin	`78.34% <91.66%> (+0.11%)`	⬆️
linux	`82.90% <91.66%> (+0.05%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
status.go	`96.42% <90.00%> (-3.58%)`	⬇️
main.go	`79.05% <100.00%> (+0.10%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 02651e8...30b07dc. Read the comment docs.

csstaub

Left some comments. Overall looks good to me, thank you for this pull request!

main.go

main_test.go

… moving to server flags Signed-off-by: Colton J. McCurdy <mccurdyc22@gmail.com>

mccurdyc · 2022-05-21T01:33:08Z

@csstaub Thank you so much for the great review comments! I've addressed them in 30b07dc and re-ran GO_VERSION=1.17 make docker-test. 🤞 CI passes 😄 .

Thanks again!

csstaub · 2022-05-21T02:55:16Z

Thanks again @mccurdyc!

mccurdyc · 2022-05-31T13:30:20Z

@csstaub Hey Friend! When you get the chance, can you cut a release that includes this change! Thanks a ton! ❤️

Let me know if I can prepare release notes or some way to help!

mccurdyc · 2022-06-01T12:42:04Z

@mcpherrinm Hey Friend! Could you possibly help by cutting a release with this change, we'd like to start using this new functionality? Thanks!

mccurdyc · 2022-06-06T12:28:23Z

@csstaub @mcpherrinm I'm really sorry to keep bothering you, but could I please trouble you to cut a release? ❤️

mccurdyc · 2022-06-13T13:16:12Z

@csstaub I'm sorry to keep bugging, but is there any way I could make it less work for you to cut a release (e.g., draft release notes, etc.)?

Thanks! ❤️

mccurdyc commented May 10, 2022

View reviewed changes

mccurdyc mentioned this pull request May 10, 2022

Adding support for a layer-7 (HTTP(S)) status endpoint #364

Closed

mccurdyc marked this pull request as ready for review May 10, 2022 20:56

mccurdyc added 4 commits May 10, 2022 16:57

http status endpoint

078698d

Signed-off-by: Colton J. McCurdy <mccurdyc22@gmail.com>

fix failing tests

8d85323

Signed-off-by: Colton J. McCurdy <mccurdyc22@gmail.com>

validate --status-target cannot be set in client mode

745d2fd

Signed-off-by: Colton J. McCurdy <mccurdyc22@gmail.com>

adds test boilerplate to statusTargetWithResponseStatusCode utility f…

a5feb34

…unction Signed-off-by: Colton J. McCurdy <mccurdyc22@gmail.com>

mccurdyc force-pushed the mccurdyc/issue-364/http-status-checker branch from 2876b76 to a5feb34 Compare May 10, 2022 20:57

change to --status-target-http

bf711ee

Signed-off-by: Colton J. McCurdy <mccurdyc22@gmail.com>

csstaub reviewed May 20, 2022

View reviewed changes

main.go Outdated Show resolved Hide resolved

main.go Outdated Show resolved Hide resolved

main.go Show resolved Hide resolved

main_test.go Outdated Show resolved Hide resolved

address @csstaub's comments regarding flag naming --target-status and…

30b07dc

… moving to server flags Signed-off-by: Colton J. McCurdy <mccurdyc22@gmail.com>

mccurdyc requested a review from csstaub May 21, 2022 01:34

csstaub approved these changes May 21, 2022

View reviewed changes

csstaub merged commit 374acef into ghostunnel:master May 21, 2022

mccurdyc deleted the mccurdyc/issue-364/http-status-checker branch May 21, 2022 12:09

phamann mentioned this pull request Oct 12, 2022

Fix memory leak in HTTP status checking #379

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support for a layer-7 (HTTP(S)) status endpoint #365

Adding support for a layer-7 (HTTP(S)) status endpoint #365

mccurdyc commented May 10, 2022 •

edited

mccurdyc May 10, 2022

mccurdyc commented May 10, 2022 •

edited

mccurdyc commented May 10, 2022

mccurdyc commented May 10, 2022

mccurdyc commented May 11, 2022 •

edited

mccurdyc commented May 11, 2022

mccurdyc commented May 11, 2022

mccurdyc commented May 16, 2022

csstaub commented May 16, 2022

mccurdyc commented May 18, 2022

codecov bot commented May 18, 2022 •

edited

csstaub left a comment

mccurdyc commented May 21, 2022

csstaub commented May 21, 2022

mccurdyc commented May 31, 2022

mccurdyc commented Jun 1, 2022

mccurdyc commented Jun 6, 2022 •

edited

mccurdyc commented Jun 13, 2022

Adding support for a layer-7 (HTTP(S)) status endpoint #365

Adding support for a layer-7 (HTTP(S)) status endpoint #365

Conversation

mccurdyc commented May 10, 2022 • edited

Testing

mccurdyc May 10, 2022

Choose a reason for hiding this comment

mccurdyc commented May 10, 2022 • edited

mccurdyc commented May 10, 2022

mccurdyc commented May 10, 2022

mccurdyc commented May 11, 2022 • edited

mccurdyc commented May 11, 2022

mccurdyc commented May 11, 2022

mccurdyc commented May 16, 2022

csstaub commented May 16, 2022

mccurdyc commented May 18, 2022

codecov bot commented May 18, 2022 • edited

Codecov Report

csstaub left a comment

Choose a reason for hiding this comment

mccurdyc commented May 21, 2022

csstaub commented May 21, 2022

mccurdyc commented May 31, 2022

mccurdyc commented Jun 1, 2022

mccurdyc commented Jun 6, 2022 • edited

mccurdyc commented Jun 13, 2022

mccurdyc commented May 10, 2022 •

edited

mccurdyc commented May 10, 2022 •

edited

mccurdyc commented May 11, 2022 •

edited

codecov bot commented May 18, 2022 •

edited

mccurdyc commented Jun 6, 2022 •

edited