feat!: namesys refactor, ipns TTL bubbled up to gateway #459

hacdias · 2023-09-04T15:19:13Z

Turns out, we already had the TTL for IPNS Records inside this package. We were just not bubbling it up. This, for IPNS Records. We can manage this without doing it for domains, but I think it would be great if we did. Closes #329.

Based on feat(path)!: consolidated path libraries #334
Spec clarification: gateway: clean Cache-Control and Last-Modified specs#436
Gateway Conformance: Tests for /ipns paths and Cache-Control HTTP header gateway-conformance#173
Kubo PR: refactor: namesys cleanup, gateway /ipns/ ttl kubo#10115

codecov · 2023-09-04T15:26:14Z

Codecov Report

Merging #459 (d4411ba) into main (0a566c9) will increase coverage by 0.09%.
The diff coverage is 62.02%.

@@            Coverage Diff             @@
##             main     #459      +/-   ##
==========================================
+ Coverage   65.78%   65.87%   +0.09%     
==========================================
  Files         207      205       -2     
  Lines       25156    25134      -22     
==========================================
+ Hits        16549    16558       +9     
+ Misses       7147     7112      -35     
- Partials     1460     1464       +4

Files	Coverage Δ
gateway/gateway.go	`85.60% <ø> (ø)`
gateway/handler_block.go	`69.76% <100.00%> (ø)`
gateway/handler_car.go	`75.42% <100.00%> (ø)`
gateway/handler_codec.go	`62.08% <100.00%> (+1.42%)`	⬆️
gateway/handler_tar.go	`79.36% <100.00%> (ø)`
gateway/handler_unixfs_file.go	`85.71% <100.00%> (+12.69%)`	⬆️
gateway/metrics.go	`79.23% <100.00%> (ø)`
ipns/name.go	`79.68% <100.00%> (ø)`
namesys/republisher/repub.go	`58.65% <100.00%> (+0.81%)`	⬆️
gateway/handler_unixfs__redirects.go	`48.54% <0.00%> (ø)`
... and 11 more

... and 13 files with indirect coverage changes

hacdias · 2023-09-04T15:40:12Z

@lidel IPNS TTL is easy. We already have it inside namesys, it's just a matter of returning it. DNS not so much. Our DNS resolution is 100% tied to go-multiaddr-dns, where it was explicitly decided not to return the TTL. My suggestion is to set ETag and Max-Age for IPNS requests where we can return the TTL. If we can't, we just set the Last-Modified as we do now. What do you think?

lidel

@hacdias sounds like a sensible compromise. quick thoughts:

update specs for Cache-Control and Last-Modified to reflect that acting on TTL is SHOULD, but implementation that have no access to TTL information SHOULD return Last-Modified instead (replace language from legacy/new etc) [https://github.com/gateway: clean Cache-Control and Last-Modified specs#436]
testing this will need something like AnyOf from Support MUST / MAY / SHOULD responses gateway-conformance#153 i think?

namesys/dns_resolver.go

namesys/ipns_publisher.go

namesys/mpns.go

namesys/namesys.go

namesys/mpns.go

namesys/interface.go

namesys/ipns_publisher.go

hacdias · 2023-09-05T11:28:56Z

@lidel thanks for the preliminary review. Here's some notes and further questions:

Specification update is in: gateway: clean Cache-Control and Last-Modified specs#436
Kubo update is in: refactor: namesys cleanup, gateway /ipns/ ttl kubo#10115, failing at the moment because gateway conformance expects no Cache-Control in IPNS land.
Do you think we should pass this TTL the way we did before for IPNS Records? That is, either the TTL from the record, the time to EOL, or the default. Or do you think it's best to pass strictly the given TTL? So far this was only being used for caching purposes, so I may need to update this if we really only want to return the TTL defined in the record.

boxo/namesys/ipns_resolver.go

Lines 114 to 129 in d213160

    
           ttl := DefaultResolverCacheTTL 
        
           if recordTTL, err := rec.TTL(); err == nil { 
        
           	ttl = recordTTL 
        
           } 
        
           switch eol, err := rec.Validity(); err { 
        
           case ipns.ErrUnrecognizedValidity: 
        
           	// No EOL. 
        
           case nil: 
        
           	ttEol := time.Until(eol) 
        
           	if ttEol < 0 { 
        
           		// It *was* valid when we first resolved it. 
        
           		ttl = 0 
        
           	} else if ttEol < ttl { 
        
           		ttl = ttEol 
        
           	}

Regarding testing, I think it would be useful to have AnyOf, but I can probably get around without it for now.
Should we add Cache-Control in the following situations (important before tackling conformance):
a. Generated directory listings under /ipns?
b. Directory listings that serve index.html?
Other comments inline.

namesys/interface.go

lidel · 2023-09-07T01:46:09Z

re:

(3) Hm.. the TTL is still used for caching purposes, we reuse it for HTTP caching. I think the logic we had was sensible: it was respecting TTL, but if EOL of IPNS Record happened to hit before TTL expires, we picked the lower value (EOL). Does not matter much today (when we have low ttl), but might be safer to keep this in case people set short expiration and high TTL (being stuck with old value against their will).
(4) mark these places with // TODO: switch to AnyOf from https://github.com/ipfs/gateway-conformance/issues/153, so we dont forget 🙏
(5) Yes to both?
- (5.a) Should be ok to do so: TTL will usually expire before there is a new release of Kubo
- (5.b) This is the most important change we make, as it will allow IPNS publisher to use TTL value from the record to control how long websites are cached on gateways and on user's browsers. Currently its Last-Modified-based heuristic which is a blunt instrument. Returning Cache-Control when we have TTL will be value added.

ps. We probably want to KEEP Last-Modified for /ipns and base it on how long namesys resolution entry has been on the cache, and add Cache-Control based on TTL in addition to it (when we know TTL). See ipfs/specs#436 (comment)

hacdias · 2023-09-07T13:00:12Z

ps. We probably want to KEEP Last-Modified for /ipns and base it on how long namesys resolution entry has been on the cache, and add Cache-Control based on TTL in addition to it (when we know TTL).

Not sure how we're supposed to achieve that. Somehow we'd have to tap into the resolver cache to get that information out of it. Besides, the time it is in cache is, at most TTL or "time until EOL" if TTL is smaller.

lidel · 2023-09-08T00:46:00Z

Namesys has own cache. The idea here is to leverage that cache and store (value, lastmodified, ttl) instead of (value, ttl).

When ttl expires namesys knows it should check if there is a new updated version of the record.
Once a record with higher sequence number appears, and the value in the new record is the same, namesys keeps the same value in the cache, lastmodified timestamp also remains the same (indicating the first time the value was resolved), and only bumps TTL. IF the value changed, we bump lastmodified to the current timestamp (in the future, we could use value from signed IPNS record)

This way, we could have a more meaningful Last-Modified than time.now().
I think the main ask here is to future-proof the interface and return lastModified timestamp in addition to the ttl that is already added by this PR (this way we break API only once).

hacdias · 2023-09-08T07:52:00Z

When ttl expires namesys knows it should check if there is a new updated version of the record.

Btw, I want to note that that is not the case at the time. Namesys cache keeps the record value until EOL:

boxo/namesys/cache.go

Lines 33 to 35 in 574df96

    
           if time.Now().Before(entry.eol) { 
        
           	return entry.val, true 
        
           }

I can definitely change it, because it would make more sense, but just a note.

hacdias · 2023-10-09T11:38:10Z

This PR is ready for review:

Kubo passing PR is at refactor: namesys cleanup, gateway /ipns/ ttl kubo#10115 (conformance see below)
Conformance is at Tests for /ipns paths and Cache-Control HTTP header gateway-conformance#173.

namesys/ipns_publisher.go

lidel

@hacdias thank you for cleaning the coreifrace and path leftovers and being patient with reviews on this.

I blocked some time today for review and pushed cosmetic refactors that unify implicit behaviors and docs for changes we should make to the IPNS publishing defaults.

Please check my changes and CHANGELOG I've added, namely the changes to defaults:
We had to increase TTL, now that it impacts caching in multiple places (namesys, gateway responses) 1 minute was too low. While at it, I also increased Lifetime and used it for publishing and reproviding to benefit from longer persistence window in Amino DHT.

Only remaining questions:

why CI did not run? (it is in pendign for me)
inline if we need separate ResolveResult and ResolveAsyncResult or can have one, but not a blocker.

If you are ok with my changes, and CI runs and is green, feel free to merge and bubble up – we want to include this in Kubo 0.24.

namesys/interface.go

namesys/republisher/repub.go

namesys/interface.go

lidel · 2023-10-17T16:21:14Z

namesys/interface.go

+// ResolveResult is the return type for [Resolver.Resolve].
+type ResolveResult struct {
+	Path    path.Path
+	TTL     time.Duration
+	LastMod time.Time
+}
+
+// ResolveAsyncResult is the return type for [Resolver.ResolveAsync].
+type ResolveAsyncResult struct {
+	Path    path.Path
+	TTL     time.Duration
+	LastMod time.Time
+	Err     error
 }


💭 ⚠️

@hacdias this feels like overkill, do we need both? can we have only one ResolveResult (or even shorter Result) with Err, like we did before?

@lidel the issue is that I don't think it makes sense for Resolve (non-async) to return a struct that contains an Err field. We can always add a comment saying that Err is always nil for .Resolve, but that's a bit beating around the bush. I would however suggest:

// Result is the return type for [Resolver.Resolve]. type Result struct { Path path.Path TTL time.Duration LastMod time.Time } // AsyncResult is the return type for [Resolver.ResolveAsync]. type AsyncResult struct { Result Result Err error }

@lidel I have simplified the names, but I did not incorporate Result inside AsyncResult. I am going to merge the PR as-is. We can change this in a separate PR if we find a better of doing it. As long as it's before final release, it should be fine.

namesys/interface.go

ipns/name.go

hacdias · 2023-10-18T07:13:41Z

@lidel I think CI did not run because it has conflicts with the main branch. I will take care of the feedback in Kubo, and bubble this up.

I made ipns.Name.src a string in order to make the struct comparable. It can now be used as a map key, but it's still protected from being converted around.

Since we've started using TTLs, we should adjust default to make them more useful by default. 1h comes from https://specs.ipfs.tech/ipns/ipns-record/#ipns-record and is a safer default than 1m.

Moving defaults that are not specific to (re)publisher

This defines the implicit defaults used for every 'ipns name resolve' DefaultResolverDhtTimeout DefaultResolverDhtRecordCount

This applies changes necessary after ipfs/boxo#459 we need this to unblock ipfs/kubo#10161

hacdias force-pushed the ns-cleanup-fix-329 branch from c790a66 to b335bde Compare September 4, 2023 15:47

lidel requested changes Sep 4, 2023

View reviewed changes

hacdias commented Sep 4, 2023

View reviewed changes

namesys/ipns_publisher.go Outdated Show resolved Hide resolved

hacdias commented Sep 4, 2023

View reviewed changes

namesys/ipns_publisher.go Outdated Show resolved Hide resolved

hacdias force-pushed the ns-cleanup-fix-329 branch 5 times, most recently from 6779eb2 to 4daa75d Compare September 5, 2023 08:25

hacdias mentioned this pull request Sep 5, 2023

refactor: namesys cleanup, gateway /ipns/ ttl ipfs/kubo#10115

Merged

4 tasks

hacdias self-assigned this Sep 5, 2023

hacdias mentioned this pull request Sep 5, 2023

gateway: clean Cache-Control and Last-Modified ipfs/specs#436

Merged

hacdias requested a review from lidel September 5, 2023 11:32

hacdias commented Sep 5, 2023

View reviewed changes

namesys/interface.go Outdated Show resolved Hide resolved

hacdias commented Sep 5, 2023

View reviewed changes

namesys/interface.go Outdated Show resolved Hide resolved

hacdias force-pushed the ns-cleanup-fix-329 branch from 4daa75d to fb409c2 Compare September 5, 2023 13:15

hacdias changed the base branch from main to issue/198 September 5, 2023 13:15

hacdias force-pushed the issue/198 branch from 1bf3f67 to bf1947a Compare September 5, 2023 13:23

hacdias force-pushed the ns-cleanup-fix-329 branch 2 times, most recently from c5a7219 to 17cd5dd Compare September 6, 2023 10:21

hacdias force-pushed the issue/198 branch from c4cf7d4 to 4276346 Compare September 7, 2023 09:18

hacdias force-pushed the ns-cleanup-fix-329 branch 2 times, most recently from e29ffd2 to 4e023b4 Compare September 7, 2023 13:15

hacdias force-pushed the issue/198 branch from 48118e1 to 968b014 Compare October 5, 2023 10:32

Base automatically changed from issue/198 to main October 6, 2023 14:04

hacdias mentioned this pull request Oct 9, 2023

Gateway: ensure consistent caching of DNS records #480

Open

hacdias force-pushed the ns-cleanup-fix-329 branch from 42af4f8 to 7610773 Compare October 9, 2023 11:21

hacdias changed the title ~~refactor!: clean namesys and return TTL~~ feat!: namesys refactor, ipns TTL bubbled up to gateway Oct 9, 2023

hacdias force-pushed the ns-cleanup-fix-329 branch from 7610773 to 310943f Compare October 9, 2023 11:22

lidel reviewed Oct 10, 2023

View reviewed changes

namesys/ipns_publisher.go Outdated Show resolved Hide resolved

hacdias requested a review from lidel October 10, 2023 12:10

hacdias force-pushed the ns-cleanup-fix-329 branch 3 times, most recently from 3d4e275 to 18aab37 Compare October 16, 2023 08:56

lidel approved these changes Oct 17, 2023

View reviewed changes

hacdias and others added 8 commits October 18, 2023 09:19

feat!: namesys refactor, ipns TTL bubbled up to gateway

429b016

refactor: make span names consistent

a046446

refactor: use ipns.Name instead of peer.ID

d25d673

I made ipns.Name.src a string in order to make the struct comparable. It can now be used as a map key, but it's still protected from being converted around.

fix: set DefaultIPNSRecordTTL to 1h

0d12f2a

Since we've started using TTLs, we should adjust default to make them more useful by default. 1h comes from https://specs.ipfs.tech/ipns/ipns-record/#ipns-record and is a safer default than 1m.

refactor: DefaultRecordTTL DefaultRecordLifetime

88f0702

Moving defaults that are not specific to (re)publisher

refactor: dht params to consts

2776fd3

This defines the implicit defaults used for every 'ipns name resolve' DefaultResolverDhtTimeout DefaultResolverDhtRecordCount

chore: cosmetics

27c8e55

refactor(ipns) Name.src → Name.multihash

8d4e7dd

hacdias force-pushed the ns-cleanup-fix-329 branch from 8c1d32a to 8d4e7dd Compare October 18, 2023 07:19

refactor: rename Resolve*Result to *Result

d4411ba

hacdias merged commit a50f784 into main Oct 18, 2023
13 of 14 checks passed

hacdias deleted the ns-cleanup-fix-329 branch October 18, 2023 08:12

lidel added a commit to ipfs-shipyard/nopfs that referenced this pull request Oct 19, 2023

refactor: new boxo/namesys

ca629a8

This applies changes necessary after ipfs/boxo#459 we need this to unblock ipfs/kubo#10161

lidel mentioned this pull request Oct 19, 2023

refactor: new boxo/namesys ipfs-shipyard/nopfs#25

Merged

BigLep mentioned this pull request Nov 9, 2023

Release 0.24 ipfs/kubo#10043

Closed

11 tasks

lidel mentioned this pull request Mar 13, 2024

Gateway: Cache-Control, max-age=ttl, ETag for /ipns requests #329

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: namesys refactor, ipns TTL bubbled up to gateway #459

feat!: namesys refactor, ipns TTL bubbled up to gateway #459

hacdias commented Sep 4, 2023 •

edited

Loading

codecov bot commented Sep 4, 2023 •

edited

Loading

hacdias commented Sep 4, 2023

lidel left a comment •

edited by hacdias

Loading

hacdias commented Sep 5, 2023 •

edited

Loading

lidel commented Sep 7, 2023 •

edited

Loading

hacdias commented Sep 7, 2023

lidel commented Sep 8, 2023

hacdias commented Sep 8, 2023

hacdias commented Oct 9, 2023

lidel left a comment •

edited

Loading

lidel Oct 17, 2023 •

edited

Loading

hacdias Oct 18, 2023 •

edited

Loading

hacdias Oct 18, 2023

hacdias commented Oct 18, 2023

feat!: namesys refactor, ipns TTL bubbled up to gateway #459

feat!: namesys refactor, ipns TTL bubbled up to gateway #459

Conversation

hacdias commented Sep 4, 2023 • edited Loading

codecov bot commented Sep 4, 2023 • edited Loading

Codecov Report

hacdias commented Sep 4, 2023

lidel left a comment • edited by hacdias Loading

Choose a reason for hiding this comment

hacdias commented Sep 5, 2023 • edited Loading

lidel commented Sep 7, 2023 • edited Loading

hacdias commented Sep 7, 2023

lidel commented Sep 8, 2023

hacdias commented Sep 8, 2023

hacdias commented Oct 9, 2023

lidel left a comment • edited Loading

Choose a reason for hiding this comment

lidel Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

hacdias Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

hacdias Oct 18, 2023

Choose a reason for hiding this comment

hacdias commented Oct 18, 2023

hacdias commented Sep 4, 2023 •

edited

Loading

codecov bot commented Sep 4, 2023 •

edited

Loading

lidel left a comment •

edited by hacdias

Loading

hacdias commented Sep 5, 2023 •

edited

Loading

lidel commented Sep 7, 2023 •

edited

Loading

lidel left a comment •

edited

Loading

lidel Oct 17, 2023 •

edited

Loading

hacdias Oct 18, 2023 •

edited

Loading