search

octodns / octodns-ns1

Ns1Provider provider for octoDNS
MIT License
4 stars 13 forks source link

Use HTTP 1.1 in for NS1 dynamic DNS HTTPS monitors #29

Closed bkane-msft closed 1 year ago

bkane-msft commented 1 year ago

== Problem ==

Currently NS1 monitors generated by octodns-ns1 use HTTP 1.0 (smuggled over a TCP connection). This does not work when pointing to Azure Storage accounts. This ticket is to create a PR to change these monitors to use HTTP 1.1

== Example ==

NS1 doesn't actually create HTTP monitors, instead prefering to create TCP monitors and manually constructing HTTP messages on top. We can emulate sending a monitor ping to an Azure storage blob using ncat:

$ printf 'GET /admin HTTP/1.0\r\nHost: <storageaccountnamehere>.z5.web.core.windows.net\r\nUser-agent: NS1\r\n\r\n' | ncat --ssl <storageaccountnamehere>.z5.web.core.windows.net 443
HTTP/1.1 505 The HTTP version specified is not supported for this operation by the server.
Content-Length: 369
Content-Type: text/html
Server: Windows-Azure-Web/1.0 Microsoft-HTTPAPI/2.0
x-ms-error-code: UnsupportedHttpVersion
x-ms-request-id: 400f2f1b-101e-0013-1eb5-26566a000000
x-ms-version: 2018-03-28
Date: Thu, 12 Jan 2023 18:43:30 GMT
Connection: close

<!DOCTYPE html><html><head><title>UnsupportedHttpVersion</title></head><body><h1>The HTTP version specified is not supported for this operation by the server.</h1><p><ul><li>HttpStatusCode: 505</li><li>ErrorCode: UnsupportedHttpVersion</li><li>RequestId : 400f2f1b-101e-0013-1eb5-26566a000000</li><li>TimeStamp : 2023-01-12T18:43:30.9213214Z</li></ul></p></body></html>

This returns a 505 because the storage account doesn't accept HTTP 1.0

We can easily fix this by using HTTP 1.1:

$ printf 'GET /admin HTTP/1.1\r\nHost: <storageaccountnamehere>.z5.web.core.windows.net\r\nUser-agent: NS1\r\n\r\n' | ncat --ssl <storageaccountnamehere>.z5.web.core.windows.net 443
HTTP/1.1 200 OK
Content-Length: 5
Content-Type: application/octet-stream
Content-MD5: +dncK6slcrqVz9Z7WWptGg==
Last-Modified: Tue, 10 Jan 2023 23:42:26 GMT
Accept-Ranges: bytes
ETag: "0x8DAF3645475F653"
Server: Windows-Azure-Web/1.0 Microsoft-HTTPAPI/2.0
x-ms-request-id: d2b0f662-601e-007b-24b6-2630fa000000
x-ms-version: 2018-03-28
Date: Thu, 12 Jan 2023 18:45:47 GMT

GOOD

I've also tested this using actual NS1 monitors (you can't see the actual HTTP response from them, they just show "failed" becasue "200 OK" is not in the response).

== Will this change break anything? ==

This change requires any existing endpoints to understand HTTP 1.1, instead of the current requirement to only understand HTTP 1.0.

Here's why I think that's ok:

In any event, octodns generated monitors NOT supporting HTTP 1.1 is breaking things for us, hence this PR. If there are better ways to handle this, happy to change this approach.

== Testing ==

In addition to the unit tests, I also made a test dynamic record on NS1 and verified the created monitor successfully HTTP GETs the /admin path from the Azure Storage Account

bkane-msft commented 1 year ago

I've sent NS1 support an email asking for their HTTP probe versions and whether they use "Connection: Close" headers. I figure we should match their defaults. I cc'd you on the email using your commit-log email address. I hope that's ok. If not, I'll remove you from the chain on further replies.

bkane-msft commented 1 year ago

Email text for the record :)

cc. Ross (octoDNS maintainer)

Hello! I'm making a monitor for NS1, and it doesn't say whether the HTTP probes use HTTP 1.0, HTTP 1.1, or something else. 

image

I also couldn't find this information in a quick search of the docs ( https://help.ns1.com/hc/en-us/articles/4413752114579-Creating-an-HTTP-or-HTTPS-monitor ) .

Which HTTP version do NS1 monitors use to send requests? If the monitors use HTTP 1.1, do they send a "Connection: Close" header as well or try to keep the connection alive for further pings?

For context, I'm modifying octoDNS's monitor code ( see https://github.com/octodns/octodns-ns1/pull/29#pullrequestreview-1246606745 ) . When octoDNS creates an "HTTP" monitor, it doesn't actually use the dedicated HTTP probe, but instead speaks the HTTP protocol over a TCP connection. I'm asking for these low-level details, so we can set matching defaults when creating monitors using octoDNS.

Also, if you see fit, feel free to comment on that GitHub PR. I'm watching that too

Thank you!

viranch commented 1 year ago

Probably better off using native HTTP monitors so we don't have to remember to not violate HTTP RFC in TCP monitors.

ross commented 1 year ago

Probably better off using native HTTP monitors so we don't have to remember to not violate HTTP RFC in TCP monitors.

There were specific reasons it's using TCP for HTTP that unfortunately don't appear to have made it into a comment, they may be in a PR/issue from back then, but I don't have the bandwidth to dig into that atm. My fuzzy memory says it was something that other providers, Dyn, Route53, etc, supported that NS1 didn't and the work around was to fake http checks with sending a simple request and searching the body for the expected 200 status code.

It's even possible NS1 has since updated things to support whatever was missing. Messing with healthchecks is fairly serious though and any changes to their behavior requires a lot of care and orchestration around the process and backwards compatibility.

bkane-msft commented 1 year ago

From NS1 Support:

ross commented 1 year ago

From NS1 Support:

  • NS1 HTTP monitors use HTTP 1.1
  • NS1 HTTP monitors do not send a Connection: Close header for each request. They do send user-configured headers (that octoDNS doesn't currently support).

:cool: I think the safest/simplest course is to add a provider config parameter for the http version and then just send the requested version with the TCP check as-is.

If we take on the ~major change of switching to HTTP checks that's probably best done in a separate PR. Personally I don't see anything that would make it worth the trouble it'd take to do it carefully/make sure it wouldn't break anyone, but if someone else takes on the details of sorting that out I'm not opposed.

bkane-msft commented 1 year ago

@ross would you prefer the default for new records to stay HTTP1.0 or would you prefer we change the default to HTTP1.1 to match NS1's "native HTTP" monitors?

ross commented 1 year ago

@ross would you prefer the default for new records to stay HTTP1.0 or would you prefer we change the default to HTTP1.1 to match NS1's "native HTTP" monitors?

I think it's safer to avoid chaning the behavior. It's hard to imagine it'd be a big enough difference to break something, but HTTP/1.0 should be fine for anything that will support it and if that runs anyone into problems as it did you all they could then make the switch.

Sorry for the delayed response, been afk a lot lately.

bkane-msft commented 1 year ago

Sorry for the delayed response, been afk a lot lately.

I'm grateful for your help here whenever you can find the time to spare :)

I've parameterized the HTTP version now. Unfortunately, by default my editor auto-sorts imports and I accidentally committed those as part of this PR. If that's a problem, I'll work to revert those hunks.

bkane-msft commented 1 year ago

I've implemented all of your comments. What do you think?

In regards to the import statement ordering, isort can auto-group imports in any order you configure - see here. isort could be added into script/format (I think we can configure isort to work withblack but would need to test).

Something to consider if you get time, or, if you do like the idea but don't have time, I'll play with isort if I get some time (depends on how smoothly work projects are going 😃 ).

ross commented 1 year ago

Something to consider if you get time, or, if you do like the idea but don't have time, I'll play with isort if I get some time (depends on how smoothly work projects are going 😃 ).

Will look into that. Definitely a fan of outsourcing that sort of cleanup/formatting.