Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

disable-last-modified-behaviour-ipns-routes #8074

Closed
wants to merge 2 commits into from
Closed

disable-last-modified-behaviour-ipns-routes #8074

wants to merge 2 commits into from

Conversation

cong-or
Copy link
Contributor

@cong-or cong-or commented Apr 14, 2021

7968 Webgateway: sets 'last-modified' header to now()

@RubenKelevra @Stebalien

As per the documentation:

If modtime is not zero time, ServeContent includes it in the Last-Modified header in the response. If the request includes an If-Modified-Since header, ServeContent uses modtime to determine whether the content needs to be sent at all.

To disable this behaviour, pass a "zero" date to ServeContent.

This change only effects ipns routes not ipfs routes.

@RubenKelevra
Copy link
Contributor

Question: Don't we already cache the ipns record and "know" when we last updated it? 🤔

Shouldn't we use this value instead of sending a "0"?

@cong-or
Copy link
Contributor Author

cong-or commented Apr 14, 2021

@RubenKelevra

Do you know where the ipns record is cached?

If so, it would make sense to use this value instead of "0".

However, if it's hard to extract, it might be best to just omit the header with "0" to avoid confusion.

@RubenKelevra
Copy link
Contributor

@mycorrhizas nope. I only remember the setting in the config to specify the size. Would have to look it up where the value is used.

@aschmahmann aschmahmann added need/analysis Needs further analysis before proceeding need/maintainer-input Needs input from the current maintainer(s) labels Apr 26, 2021
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added sharness tests for HTTP headers related to caching but think this PR needs more work before can be safely merged.

Namely, I am worried that removal of Last-Modified without replacing it with an explicit Cache-Control will introduce performance regression around HTTP caching. See my comment about browsers using Last-Modified as weak signal for how long to cache page: https://github.com/ipfs/go-ipfs/issues/1818#issuecomment-450866591

Cache-Control based on /ipns/ TTL

Ideally, /ipns/* should return Cache-Control based on TTL from IPNS record or DNSLink (TTL of a TXT record on DNS).

@aschmahmann to resolve this, we need to extend the namesys interface to return TTL of both record types in addition to Path.

I think we decided that is out of scope for #8068, so we may want to park this PR until 0.9 ships and aim at 0.10+, but we need to do this at some point: otherwise every IPNS website will be a bit slower due to unnecessary requests than it should.

UPDATE: now tracked in https://github.com/ipfs/go-ipfs/issues/1818#issuecomment-1015849462

Related:

@BigLep BigLep added this to In Review in Maintenance Priorities - Go via automation May 17, 2021
@BigLep BigLep added the status/blocked Unable to be worked further until needs are met label May 17, 2021
@BigLep
Copy link
Contributor

BigLep commented May 17, 2021

This is blocked until TTLs are respected in DNSLink and IPNS. (https://github.com/ipfs/go-ipfs/issues/1818#issuecomment-1015849462)

@cong-or
Copy link
Contributor Author

cong-or commented May 25, 2021

This is blocked until TTLs are respected in DNSLink and IPNS.

👍

@lidel lidel removed need/analysis Needs further analysis before proceeding need/maintainer-input Needs input from the current maintainer(s) labels May 25, 2021
@aschmahmann aschmahmann moved this from In Review to Weekly Candidates in Maintenance Priorities - Go Jun 14, 2021
@aschmahmann aschmahmann moved this from Weekly Candidates to Backlog in Maintenance Priorities - Go Jun 14, 2021
@aschmahmann aschmahmann added P3 Low: Not priority right now topic/ipns Topic ipns need/analysis Needs further analysis before proceeding labels Jun 14, 2021
@BigLep BigLep removed this from Backlog in Maintenance Priorities - Go Mar 10, 2022
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/analysis Needs further analysis before proceeding P3 Low: Not priority right now status/blocked Unable to be worked further until needs are met topic/ipns Topic ipns
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants