search

bgp / stayrtr

RPKI-To-Router server implementation in Go
BSD 3-Clause "New" or "Revised" License
97 stars 15 forks source link

Add last modified support #8

Closed ties closed 3 years ago

ties commented 3 years ago

Adds Last-Modified header support and enables this (and ETags) by default for rtrmon.

I'm using rtrmon with a backend that suports If-Modified-Since but not If-None-Match. To poll this often (sub-minute interval) it is better to use conditional requests.

I am not sure if it makes sense to enable this by default for stayrtr. My understanding is that the combination of both headers should be handled properly by back-ends (and are sent by browsers) - so it should be safe to do.

Furthermore I am not a golang expert, so this change is a bit mechanical, let me know what you think.

job commented 3 years ago

@ties thanks! Conditional requests are very useful indeed!

You indicate that your backend does not support eTags, which probably means the change is harder to test. Perhaps it makes sense to split this into two aspects?

  1. introduce support for if-modified-since: (which is a simpler mechanism, and you can test)
  2. introduce support for eTags
ties commented 3 years ago

@job I agree that is is good to be careful about this.

The current status is:

Because it is already used in stayrtr, I think enabling ETags on rtrmon is safe. The HTTP(S) endpoints that connects to are the same as stayrtr already connects to.

I can test rtrmon against endpoints that support only if-modified-since:. How about this: If the code looks good, both headers are enabled in rtrmon by default (since I think two toggles is not needed). if-modified-since: gets disabled in stayrtr by default for now (since I won't be actively running that), and may be set to default-enabled later.

job commented 3 years ago

Hi @ties

You wrote:

if-modified-since: gets disabled in stayrtr by default

Did you mean to say "If-none-match gets disabled in stayrtr by default" ?

ties commented 3 years ago

Hi @job

Did you mean to say "If-none-match gets disabled in stayrtr by default" ?

I think it is already enabled by default. The new part (if-modified-since) would be disabled by default (and the flag is intentionally named last.modified after the server-side header for consistency).

randomthingsandstuff commented 3 years ago

Nice clean commits. Thanks for the contrib!

ties commented 3 years ago

I think it was safe to merge. Thanks for handling this quickly :).