dnslink-std / test Goto Github PK

The implementations should be forwarding the TTL for the txt entries returned by the server.

The API should probably look like:

{
  "links": {
    "ipfs": "QmTg....yomU"
  },
  "path": [],
  "log": [],
  "ttl": {
    "ipfs": 100
  }
}

This way the ttl can be optional, and users can choose if they implement ttl or not.

Concern

getting original content path is what DNSLink users want, and re-assembling it from multiple chunks is extremely tedious, at least from IPFS perspective.

Ask

Make it easier to get to the original value in TXT record.

MVP would be to add result.txtRecords array with original content paths (pre-sorted in alphabetical order)

result = {
  txtRecords: [
    { value: '/foo/bar/buzz', ttl: 60 },
    { value: '/ipfs/QmTg....yomU/some/path', ttl: 60 }
  ],
  links: 
...

While dnslink-test does come with a on-the-fly dns server, that contains all the dns settings required to run the tests, it would also be important to setup the actual DNS settings of the domains used in the test in case an implementation is not able to configure the dns settins to use the on-the-fly server.

All dns settings required are defined in dns methods of integration-tests.js, for example:

't02: A domain without a _dnslink subdomain, containing one valid ipfs link, should return that link.': {
    dns: domain => ({
      [domain]: ['dnslink=/ipfs/ABCD']
    }),

Here domain is the t02.dnslink.dev endpoint meaning: to run this test we need the TXT entry of t02.dnslink.dev TXT dnslink=/ipfs/ABCD.

@lidel How can we set the entries best?

Concern

CI seems to be broken, unable to apply DNS changes: https://github.com/dnslink-std/test/runs/3224210180?check_suite_focus=true

Ask

Given we want to switch to fake domain (#12) it makes sense to just remove this CI job.

The set up is too brittle to provide any value – we should always use built-in DNS Server.

Concern

DNSLink should not make any decisions based on TTL, just read it (if available) and only return it to user land (if requested)

Ask

Remove this

Concern

_dnslink._dnslink.t04.dnslink.example.com producing RECURSIVE_DNSLINK_PREFIX

Ask

Is this necessary? Feels like an arbitrary choice to mark this as error. Can we remove this?

If the goal was to improve UX when someone introduced misconfiguration, then DUPLICATE_DNSLINK_PREFIX makes more sense, but I'd rather have us remove this entirely and reduce complexity.

nit: rename RCODE_N to DNS_RCODE_N to indicate that code comes from DNS system itself

Concern

PoC implementation special-cased redirect to take precendent over entries. This goes against the general simplicity of having records returned in alphabetical order. /dnslink should not have any special treatment here, so I looked deeper into our redirect story.

After analysis of test examples and real life usage I came to the conclusion that we do not need dnslink=/dnslink/other.example.com because this functionality is already provided by DNS system itself.

DNS provides us with primitives for redirecting (delegating) one FQDN to another:

• CNAME can be used for delegating _dnslink subdomain to other FQDN
• ALIAS can be used for delegating all record types (eg. A for HTTP gateway)

This is already used in production:

• We have ALIAS and CNAME setup for some of PL Websites
• Fleek: to delegate DNSLink resolution to different domain name, set ANAME/ALIAS records for your main root domain, and CNAME record for your subdomains. (docs)

Ask

Remove tests (like t14) and special-casing of /dnslink/ redirects.

We should return this as just another content path type, and leave it up to user land consumer app to decide if it wants to act on them.

Concern

Tests like t17, t19, t20 are mixing concerns and responsibilities: it is not our job to do DNS record validation, this adds unnecessary complexity that implementers need to do to be "compliant"

Ask

Temove this type of DNS validation, leave it up to DNS resolver to return an error

Concern

• confusing description
• trimming is absolute no – DNSLink should not mutate content paths in any way.

Ask

• rename it to "Multiple valid entries for the same key should be returned in alphabetical order"
• remove trimming operation from tests and libraries and have tests that confirm whitespace characters are not trimmed

Looking at https://multiformats.io/multiaddr/ it may actually be relevant for implementations to use multiple entries per key/value. As long as the entries are correctly sorted, each system should be able to decide itself how to resolve the issues.

This should make the whole standard implementations a lot simpler.

Concern

Allowing arbitrary strings is leaky: mixes DNS names with protocol/application-specific paths.
We should be more strict here.

Ask

return "invalid FQDN" error when invalid FQDN is passed to resolve methods

Another common behavior is how to act with a unqualified domain (i.e.: "google")

If a target TTL value is set to "never expire" (=long TTL) but the redirect is set to expire soon, then this means that target values for the domain should not be cached as a change in redirect might change all values.

rejecting non-ascii strings is 👍

Concern

Introducing %encoding support at the level of DNSLink is beyond its scope: too opinionated, forces protocols to implement support forthis specific encoding and introduces potential conflicts with use cases where userland expect original %encoded string.

Ask

• Remove %encoding from libraries, return string as-is
• Document ASCII requirement on the website + suggest %encoding in user land when support for non-ascii characters is desired

nit: use full form parameters in --help examples (eg. —format=json instead of -f=json) so reader does need to jump up and down to know what each one does

Concern

CLI and library uses key for the first segment of content paths, and value for the second, and everything after it seems to fall under path thing.

Those are extremely ambiguous and making decision how to parse things after the namespace is arbitrary – not something DNSLink dtandard should do.

Ask

• Remove parsing of sub-paths (#17) and keep a single identifier value per namespace
• Replace those three terms with less ambiguous ones:
  • key→ namespace
  • value+ path →identifier

Concern

The lack of documentation makes some code names really confusing. Both JS and GO repos have a subset of errors documented in their READMEs, but they don't have things like EMPTY_PART or RCODE_*, which are part of this test suite.

Ask

• Document all log codes in dnslink-std/test repo in separate file named LOG_CODES.md
• Update readmes in GO and JS repos: remove code listings and link to LOG_CODES.md in this repo instead

Concern

When we switch to fake domain (#12) and built-in server, we will be fully independent from any DNS system.
We should not special-case any system, ENS included.

Ask

Remove --enable=eth.

Pointing test suite at arbitrary DNS server is better approach: it builds proper mental model without "built-in magic" and decreases maintenance costs (we don't want other systems to PR support for their own magical TLDs).

Concern

TTL is noise unless user cares about it and wants to use it for cache-control.

Ask

• Default CLI output should not have TTL. (look like current -f=reduced)
• Add opt-in --ttl parameter to include it in text output.

When implementing DNSLink for golang, I noticed that golang's dnsResolver combines multiple returned TXT entries making it necessary to adjust the DNS output for that case: 661a2ec Obviously this is different between the implementations. My current belief, after reading the golang issue, is that they made a mistand and having them separate is the correct way to go about, but in any case there should be a test whether or not an implementation supports multiple dns entries or not. That test needs to be optional or there needs to be a switch that tests either/or.

There should be a test how the implementation is supposed to be behaved when being asked for an unknown domain.

Concern

iiuc the only reason for parsing sub-paths was special-casing during redirects.

Given that we will be removing that special-casing (#16) there is no reason for DNSLink libraries to care about the value after dnslink=/foo/..

Additionally, the way path is parsed in dnslink-std/test v0.9.0 forces everyone to use URL abstractions (pathname, search are URL components that could conflict with characters used as content identifiers)

Ask

Remove sub-path parsing and keep any sub-path as part of the identifier:

dnslink=/<namespace>/<identifier>

## Example
dnslink=/ipfs/QmX2KTMXiUb6tHTMS1JynhyQaL9qoj6uf1SLzHPhzGNnDv/go-ipfs/v0.9.1
namespace=ipfs
identifier=QmX2KTMXiUb6tHTMS1JynhyQaL9qoj6uf1SLzHPhzGNnDv/go-ipfs/v0.9.1

While it should probably just work as-is it may be a good idea to also add a test that uses a domain ALIAS both for _dnslink.<fqdn> and <fqdn>

Concern

CLI tools have --help which includes examples of real or non-existent domains.
Some examples from .dnslink.dev are broken and do not resolve, which is a bad UX.

Ask

• Switch --help texts to use *.dnslink.example.com (#12) so it is clear to user that they should not expect them to work
• Always include link to https://dnslink.dev for "more docs and examples"

Concern

Any reason why we can't reuse TOO_MANY_REDIRECTS for ENDLESS_REDIRECT edge case?
This is essentially the same error and collapsing this simplifies implementations and error handling in userland.

Ask

remove ENDLESS_REDIRECT, reuse TOO_MANY_REDIRECTS for handling infinite loop

All tests are using ResolveN at the moment, it would be good to also have a test for Resolve to make sure that the implementations are aware of both concepts.

General ask for entire test suite: do not use invalid IPFS paths. We have a policy that every CID or /ipfs/ content path published in specs, docs, repos or tests should resolve, unless we test failure scenario.

• either use valid IPFS CIDs and paths like /ipfs/bafkqae2xmvwgg33nmuqhi3zajfiemuzahiwss or /ipfs/bafybeiaysi4s6lnjev27ln5icwm6tueaw2vdykrtjkwiphwekaywqhcjze/wiki/Ancient_Greece
• or some fake namespaces like /foo/bar /bar/buz/zzz

We just realized it causes issues at PL infra because we have two sources of truth for this domain.

README states:

dnslink-test comes automatically with a dns server that is run on the localhost's udp and tcp port

@martinheidegger what is the use case for setting those *.t*.dnslink.dev records via dnsimple-update-script.js?
Ideally, we would not be dependent on real world DNS and have tests self-contained – using local/embedded DNS server.

dnslink-test does start with a on-the-fly dns server which is passed in as options. This needs some documentation.

Technically the /dns/ is an implementation mistake by the go-dnslink module, while the documentation on https://dnslink.dev points to /dnslink/. To be backwards compatible the libraries should support /dnslink/ as well as /dns/ with preference for /dnslink/.