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

Request for Comments: Include the provider type in creds.json, remove it from dnsconfig.js #1457

Closed
tlimoncelli opened this issue Mar 23, 2022 · 7 comments

Comments

@tlimoncelli
Copy link
Contributor

tlimoncelli commented Mar 23, 2022

Problem:

Many commands and functions require the user to specify the provider name (such as ROUTE53) and the cred-name. This is redundant, error prone, and additional cognitive load on the user.

Proposed solution: Store the provider name in the individual creds.json entries themselves; thus removing the need to specify them in dnsconfig.js or on the command line.

TL;DR:

Should we change creds.json to (A) include the provider name as a field, (B) change cred-names to be name:PROVIDER, (C) change cred-names to be PROVIDER:name? All of this will be done with a smooth transition.

Defintions:

"provider name": The identifier of a service provider, as embedded in the code. For example, CLOUDFLAREAPI, GANDI_V5, BIND, AZURE_DNS, ROUTE53, GCLOUD, and so on. (complete list)

"cred-name": The name given to a set of credentials in creds.json. In the examples below, they are cloudflare_tal, gandi, and inside. There may be multiple cred-names that point to the same provider name. For example, a user with domains in many Azure accounts may have multiple cred-names, one for each account, but all using the AZURE_DNS provider.

Detailed description:

There are a number of commands and functions that require the user to specify both the provider name and the cred-name.

For the purpose of this discussion, assume this is the creds.json file:

{
  "cloudflare_tal": {
    "apikey": "REDACTED1",
    "apiuser": "REDACTED2"
  },
  "gandi": {
    "apikey": "REDACTED3"
  },
  "inside": {
    "directory": "inzones",
    "filenameformat": "db_%T%?_%D"
  }
}

CURRENT SITUATION: Here are some examples where both the provider name and the cred-name must be specified:

dnsconfig.js:

var REG_GANDI = NewRegistrar("gandi", "GANDI_V5");
var DSP_CF = NewDnsProvider("cloudflare_tal", "CLOUDFLAREAPI");

On the command line:

$ dnscontrol get-zones cloudflare_tal CLOUDFLAREAPI

In fact, nearly everywhere a creds.json entry name is specified, the provider name is specified. That is, nearly any time one specifies cloudflare_tal the user must also specify CLOUDFLAREAPI; every time one specifies gandi the user must also specify GANDI_V5; every time one specifies inside the user must also specify BIND.

The one exception is the command line tool's (confusingly named) --providers flag which accepts a list of cred-names and does not require (in fact, it won't accept) provider names.

As you can see from the above examples, this is redundant and inconvenient. In the case of get-zones it is confusing to the users that have to specify two parameters where it seems like one would do (the reason, of course, is that get-zones only accesses creds.json, and never the dnsconfig.js file, which is where the name of the provider is stored).

The goal would be to remove this repetition by placing the provider name in the creds.json file, not the dnsconfig.js file or on the command line.

PROPOSED SITUATION: If we are successful, the above examples would look like this:

var REG_GANDI = NewRegistrar("gandi");
var DSP_CF = NewDnsProvider("cloudflare_tal");

On the command line:

$ dnscontrol get-zones cloudflare_tal

This is cleaner, clearer, and less typing!

BACKWARDS COMPATIBILITY:

It is a hard requirement that users must be able to update their creds.json file to comply with the new format before the new format becomes a requirement. That is, the new format must work on 3.x releases (with new elements silently ignored). In 4.0 (our next opportunity for breaking changes) the new format will be required.

To that end, we will do the following:

  • There will be a 3.x release where:
    • It will use the provider name in creds.json if it exists, but doesn't require it.
    • Anywhere a provider name is required (outside of creds.json) specifying - will mean "refer to the cred-name in creds.json".
    • DNSControl will output helpful warning messages that guide the user to update their configuration in preparation for the new format. These warnings should tell the user exactly what changes to make.
  • There will be a 3.x release that requires the new format, but makes no other breaking changes.
    • The provider name specified in NewRegistrar and NewDnsProvider will be ignored except a warning will be output if it does not match the provider name in creds.json (specifying - will generate no warnings or errors).
    • Command line tools that require a provider name will no longer require one. If one is provided, it will be cross-checked against the provider name in creds.json and mismatches will be a hard error.
    • To maintain backwards compatibility with shell scripts, command line tools that require a provider name as a positional argument will require - as a placeholder, and output a warning if anything else is used (possibly an error if the wrong provider name is specified).
  • In 4.0 (our next chance for breaking changes) command line tools that have a positional parameter that is a provider name will remove that parameter.

Proposal A: provider name as a field

Store the provider name in a field called "PROVIDER" of each entry in creds.json. For example:

{
  "cloudflare_tal": {
    "PROVIDER": "CLOUDFLAREAPI",      << NEW
    "apikey": "REDACTED1",
    "apiuser": "REDACTED2"
  },
  "gandi": {
    "PROVIDER": "GANDI_V5",           << NEW
    "apikey": "REDACTED3"
  },
  "inside": {
    "PROVIDER": "BIND_V5",            << NEW
    "directory": "inzones",
    "filenameformat": "db_%T%?_%D"
  }
}

Open issues:

  • Is PROVIDER the right field name? Would _PROVIDER be better? Something else?

Backwards compatibility concerns:

  • Good news: No existing systems use the field "PROVIDER" as a field name, thus this will not break existing creds.json files.
  • Good news: Users can upgrade to this format before it is required since DNSControl silently ignores unused fields.

Proposal B: name:PROVIDER

Append the provider name to the end of cred-name (i.e. name:PROVIDER). For example:

{
  "cloudflare_tal:CLOUDFLAREAPI": {      << NEW
    "apikey": "REDACTED1",
    "apiuser": "REDACTED2"
  },
  "gandi:GANDI_V5": {                    << NEW
    "apikey": "REDACTED3"
  },
  "inside:BIND": {                       << NEW
    "directory": "inzones",
    "filenameformat": "db_%T%?_%D"
  }
}

Notes:

  • Tools will accept name or name:PROVIDER.
  • The "name" portion must be unique. That is, the system would reject if there are cred-name foo:ROUTE53 and foo:GANDI_V5. (I had considered permitting such duplicates and implementing some kind of "longest match" system but... why add so much complexity?)

Backwards compatibility concerns:

  • Problem: What if a legacy creds.json file includes a : in a cred-name?
    • Solution: I've never seen this in the field. However, as a precaution soon dnscontrol will output a warning suggesting that : may become significant in the future and suggest using _ instead.
  • Good news: Users can adopt this new format ahead of when it will be required, though the provider name will be superfluous.
  • Problem: What if someone was using one legacy-style cred-name with multiple providers?
    • Response: AFAIKT this can't exist. No two providers require the same field names. Plus, OMG that's not something we should support.

Proposal C: PROVIDER:name

This is the same as Proposal B, but the order is reversed. i.e. the credential keys would be PROVIDER:name instead of name:PROVIDER.

For example:

{
  "CLOUDFLAREAPI:cloudflare_tal": {
    "apikey": "REDACTED1",
    "apiuser": "REDACTED2"
  },
  "GANDI_V5:gandi": {
    "apikey": "REDACTED3"
  },
  "BIND:inside": {
    "directory": "inzones",
    "filenameformat": "db_%T%?_%D"
  }
}

With this format change, the examples, open issues, and backwards compatibility issues are the same as Proposal B.

Personal option: I think this makes the creds.json file look better. The key looks nicer and sorting the keys will group all the keys of a single provider. However when someone specifies a key on the command line, it looks ugly to me because normally abbreviations chop off the end, not the beginning.

@tlimoncelli tlimoncelli changed the title Add provider type to creds.json Request for Comments: Include the provider type in creds.json, remove it from dnsconfig.js Mar 30, 2022
@tlimoncelli tlimoncelli pinned this issue Mar 30, 2022
@Olivetti
Copy link
Contributor

Olivetti commented Apr 4, 2022

For me, proposal C would fit best.

@philpennock
Copy link
Contributor

I prefer A, because JSON shouldn't be imposing structure inside keys using colon-delimited fields; C is my second choice, B makes little sense to me. I can adapt to whatever.

My preference for A is by quite a wide margin, because whenever I'm using JSON I end up doing ad-hoc queries with tools such as jq, and being able to look up an entry by name and then query the provider as a field makes much more sense than string-matching with wildcards on the key field and then doing substring extraction.

@tlimoncelli
Copy link
Contributor Author

@philpennock That an interesting point about wanting to be able to easily manipulate creds.json using something like jq.
What if PROVIDER:name and name were both permitted? jq scripts could refer to GANDI_V5:gandi but hand-written code might just refer to gandi. Of course, name would have to be unique across all entries, but that's easy to enforce.

Thoughts?

@philpennock
Copy link
Contributor

Duplicate possibilities mean you have to detect both and handle both, or so it's more work and more fragility, with less testing that scenarios are fully handled. Pick one and work with it.

@tlimoncelli
Copy link
Contributor Author

I'm taking a look at how Plan A might be implemented. One thing I've noticed is that it might be more clear to specify "TYPE" instead of "PROVIDER". The code and other docs refer to that field as the type.

@tlimoncelli
Copy link
Contributor Author

I have a draft PR of "Plan A" in #1485

Feedback appreciated!

@tlimoncelli
Copy link
Contributor Author

Thanks to everyone for their feedback! I went with "Plan A" as described.

It turns out Plan A wasn't as difficult to implement as I had hoped. Thanks to philpennock for pushing in that direction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants