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

feat: DNS.MaxCacheTTL for DNS-over-HTTPS resolvers #8615

Merged
merged 6 commits into from
Feb 10, 2022
Merged

feat: DNS.MaxCacheTTL for DNS-over-HTTPS resolvers #8615

merged 6 commits into from
Feb 10, 2022

Conversation

thibmeu
Copy link
Contributor

@thibmeu thibmeu commented Dec 16, 2021

Expose WithMaxCacheTTL option of go-doh-resolver within IPFS node config.
This allows to cap the Time-To-Live suggested by the DNS response. This includes resolution of DNSLink captured by the custom resolver URLs.

This change relies on go-ipfs.config change. The change of go.mod and go.sum would have to be updated accordingly at this time.

Before merging, both go-doh-resolver and go-ipfs-config have to be updated.

Tagging @vyzo and @marten-seemann , as they reviewed the go-doh-resolver change.

@welcome

This comment has been minimized.

go.mod Outdated Show resolved Hide resolved
core/node/dns.go Show resolved Hide resolved
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.

@thibmeu mind elaborating why you want to cap the Time-To-Live suggested by the DNS response? What about being able to set the minimum, or override TTL for IPNS records as well?

FYSA I am looking into adding a proper cache-control header to all HTTP responses under /ipns/ namespace (https://github.com/ipfs/go-ipfs/issues/1818#issuecomment-1015849462) – Gateway is not utilizing TTL from DNS at all right now. This means your PR will not have much impact on gateway responses, but maybe you have a different goal?

My understanding is we want to fix cache control in Gateway responses for /ipns/, and need to wait for #1818. After we wire up TTLs, and have both cache-control and etag in Gateway responses for /ipns/ content paths, then we can pick this PR up and tweak cache-control / based on TTL minimum and maximum for both IPNS and DNSLink records.

I believe we need to have config option for both min and max, because there are people setting records with very small TTL such as 60 seconds, and Gateway operators should be able to override the minimum to be higher, to leverage the cache more.

If we want to control this at the record cache level, then we need four OptionalDurations:

  • min/max for DNSLink (and multiaddrs which resolve DNS)
    • DNS.MaxCacheTTL
    • DNS.MinCacheTTL
  • min/max for IPNS records
    • Ipns.MaxCacheTTL
    • Ipns.MinCacheTTL

Instead, we could introduce min/max specific to Gateway.
It would be applied at the very end, when cache-control header is calculated for both DNSLink and IPNS records:

  • Gateway.MaxCacheTTL
  • Gateway.MinCacheTTL

Of course, /ipfs/ would remain immutable,
min/max would be applied only to mutable responses under /ipns/ (including DNSLink websites).

I am leaning towards adding Gateway.* ones for now, but lmk your thoughts and use case, perhaps there is a better/simpler way for controlling this?

@thibmeu
Copy link
Contributor Author

thibmeu commented Jan 20, 2022

@lidel The goal with this PR is to allow update to be propagated faster to the gateway. At the moment, DNS requests are cached for the duration of their TTL. There is no option to customise this. As a node operator, I would like to decide how long these DNS responses are cached for. Then, when a DNSLink is updated, it is seen by the gateway within a bounded timeframe (up to MaxCacheTTL). This comes from RFC 2131 Section 8 which defines how DNS implementations can use records TTL.

Regarding having cache-control header, I agree this would be good to propagate the resolution cache time information to gateway users. However, I think this gateway setting is different from the DNS setting the present change tries to introduce.

thibmeu and others added 4 commits January 20, 2022 19:49
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.

Thank you @thibmeu, in this context it makes sense to move forward with this PR: it is not directly related to https://github.com/ipfs/go-ipfs/issues/1818 and brings value on its own.

Pushed some cosmetic changes:

LGTM and ready to merge, but parking until we tag a new release of go-ipfs-config.

@aschmahmann are you aware of any other config changes in 0.13 that we should wait for?
If not, I'd like to tag and merge this to clear up the queue.

go.mod Outdated Show resolved Hide resolved
@aschmahmann
Copy link
Contributor

@aschmahmann are you aware of any other config changes in 0.13

Yes, there will likely be some related to the next go-libp2p release

that we should wait for?

🤷. It doesn't cost us anything to cut more go-ipfs-config releases so we can go ahead and merge + release. I don't know how many people are running builds from master though/if it really makes a difference though. Certainly if you think that we'll have to worry about rebasing the PRs and dealing with conflicts here then definitely just tag + merge.

@lidel lidel changed the title Add MaxCacheTTL option for DNS-over-HTTPS resolvers feat: DNS.MaxCacheTTL for DNS-over-HTTPS resolvers Jan 25, 2022
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.

Switched to go-ipfs-config v0.19.0, this is ready for merge.

@lidel lidel added the status/ready Ready to be worked label Jan 25, 2022
@BigLep
Copy link
Contributor

BigLep commented Feb 1, 2022

Next step: review the help text.

docs/config.md Outdated Show resolved Hide resolved
docs/config.md Outdated Show resolved Hide resolved
docs/config.md Outdated Show resolved Hide resolved

Default: Respect DNS Response TTL

Type: `optionalDuration`
Copy link
Contributor

@guseggert guseggert Feb 3, 2022

Choose a reason for hiding this comment

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

For completeness, can we add optionalDuration to the Types section?

Copy link
Member

Choose a reason for hiding this comment

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

Good idea, but opened #8729 to not block this PR.

@lidel lidel merged commit a93907a into ipfs:master Feb 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/ready Ready to be worked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants