Add support for ETag and Last-Modified headers for summary and summary.sig #2205

pwithnall · 2020-09-29T09:59:45Z

See the commit messages. This adds support (in the soup and curl backends) for the Last-Modified/ETag/If-None-Match/If-Modified-Since HTTP headers and hooks them up for requests for summary and summary.sig so they don’t have to be re-downloaded if unchanged from last time. In particular, this will shave 590B off each response for summary.sig in the cached (common) case.

This PR is related to #2167 in that it’s a network efficiency improvement, but can be reviewed/merged independently.

pwithnall · 2020-09-29T10:09:11Z

One possible improvement to think about after this (if it gets merged) is that the separate HTTP requests to get summary.sig and then (if it’s changed) summary could be combined: previously summary.sig was essentially acting as the ETag for summary, but once HTTP ETags are supported properly, then perhaps that could be rethought. If a client has an ETag for its cached summary, it could request summary from the server immediately (without requesting summary.sig first) and rely on the ETag it sends to make sure it only downloads the full summary file when it’s actually changed.

That would mean a shift to using inline signatures to protect summary, though (otherwise you still need to download summary.sig separately). I can’t remember all of the tradeoffs to know whether that’s helpful, unhelpful, or indifferent.

jlebon

Overall idea makes sense to me. Though worth noting there was a suggestion from @alexlarsson recently to have deltas for summary files as well which I think can affect the discussion here. E.g. if we were to support summary deltas, then presumably they'd be in some form which makes querying for "new summary wrt what I have locally" cheap, which intersects with what's going on here.

Though support for cache semantics makes sense on its own too and is compatible with existing OSTree webservers today.

jlebon · 2020-09-29T21:23:16Z

src/libostree/ostree-fetcher-curl.c

+ *    Wed, 21 Oct 2015 07:28:00 GMT
+ */
+static GDateTime *
+parse_rfc2616_date_time (const char *buf,


Woah, is there really no helper function in any of glib, libsoup, or libcurl for this?

There’s soup_date_new_from_string(), but it’s in libsoup and this code is in the libcurl backend, so I assume libsoup is not an option there. Have I misunderstood?

No you're right, I missed that this is specific to libcurl and not in common with both.

Another big thing to keep in mind is that when updating host systems at least ostree usually runs as unrestricted root (I'd eventually like to fix this) and things like the string parsing we're doing here are an attack surface and need careful auditing.

To be clear did you write this code from scratch or is it inspired by anything else?

Let's at least add some unit tests for this?

We've already copied in some libsoup code for URIs, maybe we could copy in the libsoup code?

To be clear did you write this code from scratch or is it inspired by anything else?

It’s written from scratch.

Let's at least add some unit tests for this?

I’ll have a go.

We've already copied in some libsoup code for URIs, maybe we could copy in the libsoup code?

Maybe, but it was rewritten recently, so is in basically the same position as this code (except more general and has support for multiple RFC date formats tied together in the same code). The older version of the code was tied to SoupDate and also supported multiple date formats in one code path.

I think having a minimal in-house implementation makes sense, given that we only need to support the Last-Modified header, which only uses one specific HTTP date format (RFC 2616). If we were supporting more headers with other formats then maybe things would be different.

OK, I’ve split the RFC 2616 date parsing code out into a separate file so that it can be built into a test program, and added a test program which gives 100% coverage of lines (and, I think, branches — although the lcov setup in ostree doesn’t enable branch coverage). How’s that?

Thanks so much for the added tests!

jlebon · 2020-09-29T21:35:51Z

src/libostree/ostree-repo-pull.c

+      g_autoptr(GBytes) etag_bytes = NULL;
+
+      etag_bytes = glnx_fgetxattr_bytes (fd, "user.etag", NULL);


Minor: (here and elsewhere), coding style is declaration and initialization at the same time (or e.g. in the case of fd above, declaration would be right before the openat).

So declarations not necessarily at the start of code blocks? I thought there were problems with doing that with autocleanup, where the autocleanup would happen but the initialisation might not (if not reached by the time the block is exited)?

As long as you initialize it to the "null" value, it's fine. When mixing it with the goto out pattern it gets tricky, but nowadays compilers will yell at you if you try to do something funky like jumping over an initialization.

OK. Tweaked a couple of declarations to move them to where they’re used, but most declarations in the PR are assigned by pointer so can’t be changed.

src/libostree/ostree-repo-pull.c

jlebon · 2020-09-29T21:50:56Z

src/libostree/ostree-repo-pull-private.h

+  char             *summary_etag;
+  guint64           summary_last_modified;  /* seconds since the epoch */


How necessary do you feel is ETag support? Feels like Last-Modified should be enough for our purposes, and it's nice that we don't have to dabble in user xattrs. But I'm not an expert in that field -- are there webservers out there which support ETag but not Last-Modified? (I'd expect the opposite.)

I think ETag support is actually more important than Last-Modified support because it doesn’t have race conditions around updates/checks in the same second, and doesn’t have issues with clock synchronisation. Both require some level of support on the server (typically, aiui, servers are configured to serve everything as if it’s not cacheable, as that’s the safest option). I would expect that all web servers would support both headers, though. They’ve been around a long time.

If xattr support isn’t available then ETags won’t work, but should degrade nicely, so that shouldn’t cause a problem.

jlebon

Would be nice to have a basic test for this where we check that we avoid pulling the summary. Something like just doing another ostree pull and sanity-checking that the inode for the summary didn't change? (Since we always create a new file when caching.) Hmm, though on the ostree-trivial-httpd side, briefly looking at SoupServer, it seems like we'd have to implement that logic ourselves like this test here.

jlebon · 2020-10-01T20:46:48Z

src/libostree/ostree-fetcher-curl.c

+ *    Wed, 21 Oct 2015 07:28:00 GMT
+ */
+static GDateTime *
+parse_rfc2616_date_time (const char *buf,


No you're right, I missed that this is specific to libcurl and not in common with both.

jlebon · 2020-10-01T20:52:39Z

src/libostree/ostree-repo-pull.c

+      g_autoptr(GBytes) etag_bytes = NULL;
+
+      etag_bytes = glnx_fgetxattr_bytes (fd, "user.etag", NULL);


As long as you initialize it to the "null" value, it's fine. When mixing it with the goto out pattern it gets tricky, but nowadays compilers will yell at you if you try to do something funky like jumping over an initialization.

src/libostree/ostree-repo-pull.c

jlebon · 2020-10-01T20:58:09Z

src/libostree/ostree-fetcher-curl.c

+  const char *end_ptr = NULL;
+  gint saved_errno = 0;
+
+  g_return_val_if_fail (n_digits == 2 || n_digits == 4, FALSE);


I'm confused by the role n_digits plays here. Doesn't look like it's actually used outside of this precondition check?

Whoops, good catch. It was used before I refactored the function a bit, and then I forgot to plumb it back in. Will update.

jlebon · 2020-10-05T18:18:54Z

src/libostree/ostree-fetcher-curl.c

+          if (!is_file && response == 304)
+            {
+              /* Version on the server is unchanged from the version we have
+               * cached locally; report this as an out-argument, a zero-length
+               * response buffer, and no error. */
+              req->out_not_modified = TRUE;
+            }


Optional: should we check here that at least one of if_none_match or if_modified_since was provided? That way, if somehow a HTTP server returns 304 even though we didn't provide either of these, we still error out as before.

Good suggestion, will tweak things to do that (and in the libsoup implementation too).

pwithnall · 2020-10-07T15:58:49Z

Would be nice to have a basic test for this where we check that we avoid pulling the summary

Working on that now; will hopefully have something ready tomorrow.

pwithnall · 2020-10-09T18:20:37Z

Updated. I’ve fixed a couple of bugs and added a unit test (although see its commit message: it doesn’t actually fail with the ETag/Last-Modified changes reverted). All my changes are as fixups for ease of review, and can be squashed later.

cgwalters · 2020-10-13T15:13:48Z

src/libostree/ostree-fetcher-curl.c

+      strncasecmp (buffer, "ETag: ", strlen ("ETag: ")) == 0)
+    {
+      g_clear_pointer (&req->out_etag, g_free);
+      req->out_etag = g_strstrip (g_strdup (buffer + strlen ("ETag: ")));


g_strstrip() is dangerous here because if the buffer has leading whitespace we lose track of the real malloc pointer. I think you just want g_strchomp()?

Are you sure? g_strstrip(x) is just g_strchug(g_strchomp(x)), and g_strchug() doesn’t increment the input pointer, it uses memmove() to shift the string contents down over any leading whitespace. The pointer is unchanged.

cgwalters · 2020-10-13T15:23:43Z

src/libostree/ostree-fetcher-curl.c

+ *    Wed, 21 Oct 2015 07:28:00 GMT
+ */
+static GDateTime *
+parse_rfc2616_date_time (const char *buf,


Another big thing to keep in mind is that when updating host systems at least ostree usually runs as unrestricted root (I'd eventually like to fix this) and things like the string parsing we're doing here are an attack surface and need careful auditing.

To be clear did you write this code from scratch or is it inspired by anything else?

Let's at least add some unit tests for this?

We've already copied in some libsoup code for URIs, maybe we could copy in the libsoup code?

src/libostree/ostree-repo-pull.c

src/libostree/ostree-fetcher-soup.c

src/libostree/ostree-fetcher-curl.c

jlebon

This looks great overall! Really just minor things.

Could you squash the commits? We don't use the Homu bot here anymore which means that there is no autosquashing.

src/libostree/ostree-date-utils.c

src/libostree/ostree-fetcher-curl.c

tests/test-rfc2616-dates.c

Add support in the soup and curl fetchers to send the `If-None-Match` and `If-Modified-Since` request headers, and pass on the `ETag` and `Last-Modified` response headers. This currently introduces no functional changes, but once call sites provide the appropriate integration, this will allow HTTP caching to happen with requests (typically with metadata requests, where the data is not immutable due to being content-addressed). That should reduce bandwidth requirements. Signed-off-by: Philip Withnall <[email protected]>

As `summary` and `summary.sig` aren’t immutable, HTTP requests to download them can be optimised by sending the `If-None-Match` and `If-Modified-Since` headers to avoid unnecessarily re-downloading them if they haven’t changed since last being checked. Hook them up to the new support for that in the fetcher. The `ETag` and `Last-Modified` for each file in the cache are stored as the `user.etag` xattr and the mtime, respectively. For flatpak, for example, this affects the cached files in `~/.local/share/flatpak/repo/tmp/cache/summaries`. If xattrs aren’t supported, or if the server doesn’t support the caching headers, the pull behaviour is unchanged from before. Signed-off-by: Philip Withnall <[email protected]>

This test would have actually passed before the summary file caching changes (in the previous few commits) were added, as the `summary.sig` essentially acted as the ETag for the summary file, and itself wasn’t updated on disk if it didn’t change when querying the server. Actually testing that the HTTP caching headers are working to reduce HTTP traffic would require test hooks into the pull code or the trivial-httpd server, neither of which I have the time to add at the moment. Signed-off-by: Philip Withnall <[email protected]>

This is basic support for the Last-Modified/ETag/If-Modified-Since/If-None-Match headers. It’s not high performance, and doesn’t support all of the related caching features (like the If-Match header, etc.). Signed-off-by: Philip Withnall <[email protected]>

This makes it testable, and increases its test coverage too 100% of lines, as measured by `make coverage`. Signed-off-by: Philip Withnall <[email protected]>

pwithnall · 2020-10-22T20:06:04Z

All review comments addressed (or commented on) and all the fixups squashed.

cgwalters

/lgtm

cgwalters · 2020-10-22T20:39:59Z

src/libostree/ostree-fetcher-curl.c

+ *    Wed, 21 Oct 2015 07:28:00 GMT
+ */
+static GDateTime *
+parse_rfc2616_date_time (const char *buf,


Thanks so much for the added tests!

openshift-ci-robot · 2020-10-22T20:58:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, jlebon, pwithnall

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [cgwalters,jlebon,pwithnall]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

cgwalters · 2020-10-22T21:00:05Z

Looks like Travis is stuck, hmm. Not sure how to retrigger that one without another force push. Let's give it a bit and see if it comes back, otherwise we can override.

openshift-ci-robot requested review from mwleeds and rfairley September 29, 2020 09:59

openshift-ci-robot added the approved label Sep 29, 2020

jlebon reviewed Sep 29, 2020

View reviewed changes

jlebon reviewed Oct 5, 2020

View reviewed changes

cgwalters reviewed Oct 13, 2020

View reviewed changes

jlebon approved these changes Oct 20, 2020

View reviewed changes

pwithnall added 5 commits October 22, 2020 21:03

tests: Split RFC 2616 date parsing code out and add tests

0974a7f

This makes it testable, and increases its test coverage too 100% of lines, as measured by `make coverage`. Signed-off-by: Philip Withnall <[email protected]>

pwithnall force-pushed the etags-and-last-modified branch from 4aafdc1 to 0974a7f Compare October 22, 2020 20:05

cgwalters approved these changes Oct 22, 2020

View reviewed changes

openshift-ci-robot assigned cgwalters Oct 22, 2020

openshift-ci-robot added the lgtm label Oct 22, 2020

openshift-merge-robot merged commit f8f6464 into ostreedev:master Oct 22, 2020

pwithnall deleted the etags-and-last-modified branch October 22, 2020 22:24

cgwalters mentioned this pull request Oct 23, 2020

Add indexes for deltas outside of the summary #2188

Merged

pwithnall mentioned this pull request Oct 26, 2020

T30404 Backport latency fixes endlessm/ostree#171

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for ETag and Last-Modified headers for summary and summary.sig #2205

Add support for ETag and Last-Modified headers for summary and summary.sig #2205

pwithnall commented Sep 29, 2020

pwithnall commented Sep 29, 2020

jlebon left a comment

jlebon Sep 29, 2020

pwithnall Oct 1, 2020

jlebon Oct 1, 2020

cgwalters Oct 13, 2020

pwithnall Oct 16, 2020

pwithnall Oct 16, 2020

cgwalters Oct 22, 2020

jlebon Sep 29, 2020

pwithnall Oct 1, 2020

jlebon Oct 1, 2020

pwithnall Oct 7, 2020

jlebon Sep 29, 2020

pwithnall Oct 1, 2020

jlebon left a comment

jlebon Oct 1, 2020

jlebon Oct 1, 2020

jlebon Oct 1, 2020

pwithnall Oct 7, 2020

jlebon Oct 5, 2020

pwithnall Oct 7, 2020

pwithnall commented Oct 7, 2020

pwithnall commented Oct 9, 2020

cgwalters Oct 13, 2020

pwithnall Oct 16, 2020

cgwalters Oct 13, 2020

jlebon left a comment

pwithnall commented Oct 22, 2020

cgwalters left a comment

cgwalters Oct 22, 2020

openshift-ci-robot commented Oct 22, 2020

cgwalters commented Oct 22, 2020

		g_autoptr(GBytes) etag_bytes = NULL;

		etag_bytes = glnx_fgetxattr_bytes (fd, "user.etag", NULL);

		char *summary_etag;
		guint64 summary_last_modified; /* seconds since the epoch */

Add support for ETag and Last-Modified headers for summary and summary.sig #2205

Add support for ETag and Last-Modified headers for summary and summary.sig #2205

Conversation

pwithnall commented Sep 29, 2020

pwithnall commented Sep 29, 2020

jlebon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlebon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwithnall commented Oct 7, 2020

pwithnall commented Oct 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlebon left a comment

Choose a reason for hiding this comment

pwithnall commented Oct 22, 2020

cgwalters left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci-robot commented Oct 22, 2020

cgwalters commented Oct 22, 2020