lexicon
lexicon copied to clipboard
AnalogJ
A list of issues present in lexicon
I'll create a list of issues that I've come across.
- Issue with update_record mentioned in #200
- No TTL implementation in some providers
- No method to specify accepted value of TTL by different providers
- No policy to handle illegal TTL value in providers
- Unpredictable provider behavior in
create_record,list_record,update_recordanddelete_record - No easier way to specify advanced options in case of MX, SRV, etc records
- No test to ensure autopagination is enabled when retrieving domain/record lists. (test with large number of domains & records, approx 100?)
- Tests needed for delegated domains and delegated accounts (provide specific suites?)
- we do not specify which Record Types are supported
- Should lexicon wrap/provide comprehensive list of custom error types for error handling downstream?
- SPEC should specify that records should be autopaginated when possible.
TTL Issue
Providers should be able to tell the user about the range/list of TTL values accepted by it. A policy should be defined on how to handle invalid TTL values supplied by the user.
Unpredictable behavior
Different provider implementations behave differently in certain situations at the moment. The behavior for the methods create_record, list_record, update_record and delete_record should be specified for most/all possible situations.
This includes record sets and the like.
Easier reading of advanced options
This included TTL and extra options that MX,SRV, etc records require. Instead of self.options.get, we could try passing these in an argument if possible.
I'll update this as I come across other issues.
Edited by @analogj to add additional issues.
this is great, I started something like this in #201 but I'll probably close that out in favor of this issue.
If you could, could you include links/numbers for any existing issues in your description?
And yes please, I'd like to get a comprehensive list of provider inconsistencies before I start extending the test suite again and contacting developers to make recordings. Updates would be appreciated!
A lot of the exceptions raised are stock Python exceptions. It would make sense to take the approach SqlAlchemy does - a set of custom lexicon exceptions are created, and then underlying provider exceptions are mapped to them or raised as needed.
Using the TTL issue above, we might see:
class LexiconError(Exception):
pass
class InvalidTTL(LexiconError):
pass
If the provider raises a TTL error (or the logic demands one is raised), then InvalidTTL is invoked (stashing the original exception if needed).
People would then be able to switch DNS providers and not have to adapt their code.
I was thinking more along the lines of matching the provided ttl to the closest one supported by the provider. For instance, godaddy takes a minimum of 600, so providing anything less, the provider will use 600. Something like that. Let's not have the user worry about exceptions on invalid values. Additionally, provide a list/range of accepted ttl values per provider so the user can choose between those.
But I do agree that lexicon exceptions should be implemented for various error outcomes instead of relying on python exceptions.
@AnalogJ is it possible to change method signatures or do we have to go with the existing signatures?
@jvanasco I originally looked at wrapping all exceptions by a set of lexicon specific ones, however I decided against it at the time for 3 reasons:
- while there's a finite amount of exception types that we might handle, there's incredible diversity in how they are generated by providers. Also, some providers do not distinguish between various error types, they just return a single error code.
- most people will be using lexicon as a CLI, where exception types are not as useful
- laziness :) - it was alot of work, and I didnt have time to handle all the error cases for all the various providers I was adding
I'm open to reconsidering though. If you want to throw together a proposal or a quick PR with 3 or so providers, that could give us an idea of how it could work for the rest of them.
@trinopoty That's actually an interesting idea. Setting a floor & ceiling value dynamically on any TTL value passed in. That would also allow us to re-enable tests with a standard TTL test value.
I know I'll have to modify the update_record signature to fix the issue for record sets, but what else are you considering?
@AnalogJ I'm considering adding an options parameter to all the methods. Instead of depending on self.options.get. This will get us more thread safety and less ambiguity.
This options parameter can contain all the extra stuff like ttl, mx priority, srv parameters, etc.
-1 on the TTL fix. You'll end up with a user expecting to set a TTL to one value, thinking it is set to that value because everything succeeded, and then having it set to a completely different value with no notice. It's usually best to raise the error so users can fix their systems.
For example, My main use of this library now is for managing some cloud services for a multi domain application. If i hit a typo and set a TTL for 30 (invalid on my provider) instead of 300, I need to know. There is also a chance the API will change and a previously valid value will no longer work - suppressing/bypassing that error in the client and not letting me know also does me a disservice.
The automated tests could still be handled up giving the provider attributes like "max_ttl", "min_ttl".
@jvanasco We'll have to find a provider agnostic way of letting the user know about legal TTL values. We can't expect the user to have to change their implementation too much with provider changes.
Tests needed for delegated domains and delegated accounts (provide specific suites?)
To handle this in the Namecheap client, I just added a second testclass to the file with the delegated domain. That runs all the tests.
We'll have to find a provider agnostic way of letting the user know about legal TTL values. We can't expect the user to have to change their implementation too much with provider changes.
If the user changes to a provider that does not accept the same TTL, the correct action is to raise an Exception alerting them to the valid values, and stop. Coercing their input to the nearest acceptable value is not something this, or any other, library should do.
I've tried to create a specification for providers at #211 Review it and suggest necessary changes on the PR.
Since we've got the specs more or less figured out, we should try to figure out the exceptions next.
How are we handling providers that support staging of changes? For instance, Route53 has a mechanism where a set of changes is first staged and then pushed in one go. At the moment, there is no mechanism for this in lexicon. Each change is standalone. A provision for staged changes should be added.
I'm not sure if that's relevant to enough providers. Personality I'd say that's out of scope for the problem that lexicon is trying to solve. On Fri, May 4, 2018 at 8:41 AM Trinopoty Biswas [email protected] wrote:
How are we handling providers that support staging of changes? For instance, Route53 has a mechanism where a set of changes is first staged and then pushed in one go. At the moment, there is no mechanism for this in lexicon. Each change is standalone. A provision for staged changes should be added.
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/AnalogJ/lexicon/issues/204#issuecomment-386641004, or mute the thread https://github.com/notifications/unsubscribe-auth/ANLxg85L70dJOxosN8zrdOM9I5JHu5W4ks5tvHaOgaJpZM4S6jwl .
I'm not sure if that's relevant to enough providers. Personality I'd say that's out of scope for the problem that lexicon is trying to solve.
On a similar note, it might be nice to have a --dry-run functionality, which would print the computed changes for review, without pushing them.
Why? Some APIs operate on a delta (add/delete a record). Others operate by downloading, editing, and replacing the entire record set.
As far as I can see, most providers will just delete all the existing records when you call delete_record() with no arguments.
It looks extremely dangerous.
As far as I can see, most providers will just delete all the existing records when you call delete_record() with no arguments. It looks extremely dangerous.
Looks like my linode providers are guilty of this too.
What about to extend exception handling to at least cover some real use cases? Like one in issue #619, where certbot is trying to detect the base domain by simple try-and-fail approach, while checking the exception text to see if it was a domain-not-found or other error?
