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

TTL for NS-records is "stuck" #74

Closed
joachimtingvold opened this issue Jul 5, 2016 · 13 comments
Closed

TTL for NS-records is "stuck" #74

joachimtingvold opened this issue Jul 5, 2016 · 13 comments

Comments

@joachimtingvold
Copy link
Contributor

joachimtingvold commented Jul 5, 2016

Hi,

Trying to change the NS-records for my domains. They are defaulted to 3600 (1 hour). Trying to change these to 86400 (24 hours). I set the value (for all the NS-records), hit "Save", then "Apply". Update successful. Refresh domain/page, old values are displayed (and is also what resides in the DB).

PowerDNS 4.0.0rc2 + PostgreSQL DB backend.

@joachimtingvold
Copy link
Contributor Author

  1. "Apply"-button calls for /domain/$domain/apply, which hits record_apply().
  2. Records sent to Record.apply() has new TTL value (86400).
  3. Records in for r in new_records: have new TTL value.
  4. final_records does not have new TTL value (it's now been replaced with the old value).

@joachimtingvold
Copy link
Contributor Author

Culprit seems to be related to NEW_SCHEMA, and in turn changes done in #5.

@joachimtingvold joachimtingvold changed the title TTL for NS-records is "stuck" at 3600 TTL for NS-records is "stuck" Jul 5, 2016
@joachimtingvold
Copy link
Contributor Author

joachimtingvold commented Jul 5, 2016

if NEW_SCHEMA:
            records = sorted(records, key = lambda item: (item["name"], item["type"]))
            for key, group in itertools.groupby(records, lambda item: (item["name"], item["type"])):
                new_record = {
                        "name": key[0],
                        "type": key[1],
                        "ttl": records[0]['ttl'],
                        "changetype": "REPLACE",
                        "records": []
                    }

This sets the same TTL to all records in the RRset. Probably not what we want.

@joachimtingvold
Copy link
Contributor Author

joachimtingvold commented Jul 5, 2016

I've patched it, and it seems to work so far. However, there needs to be more logic in Record.apply(). The API in 4.0.0 seems to limit the TTL to be set per RRset-entry, which means that the logic should handle each TTL for each type of RRset.

https://doc.powerdns.com/md/httpapi/api_spec/#url-apiv1serversserver95idzoneszone95id

E.g. if we have one NS-record with TTL 3600 and another NS-record with TTL 86400, they should be put into two RRset's.

NS-records are probably not the best example (usually you want them to have the same TTL), but this applies for A- and AAAA-records as well.

@joachimtingvold
Copy link
Contributor Author

joachimtingvold commented Jul 5, 2016

How is the API supposed to handle different TTL's?

According to https://doc.powerdns.com/md/httpapi/api_spec/#url-apiv1serversserver95idzoneszone95id;

changetype Must be REPLACE or DELETE. With DELETE, all existing RRs matching name and type will be deleted, including all comments. With REPLACE: when records is present, all existing RRs matching name and type will be deleted, and then new records given in records will be created. If no records are left, any existing comments will be deleted as well. When comments is present, all existing comments for the RRs matching name and type will be deleted, and then new comments given in comments will be created.

So, since changetype is REPLACE, all records with same name/type are deleted, and then re-created. Since TTL now is set per RRset, this means records not matching the TTL, would be deleted. For good.

@joachimtingvold
Copy link
Contributor Author

TIL; TTL's for same name and type has to be the same. Having two NS-records with different TTL is not supposed to happen, so having one of them deleted I guess is "fair". Should add more error-handling of this, though (e.g. trying to "Apply" when two records of same name and type has different TTL --> give warning).

@joachimtingvold
Copy link
Contributor Author

I guess the PR (#75) could be more a bit more "nice" and/or smart about it. That is, don't delete, but set both to the same TTL value (either from DB, or the new TTL). I'll take a look at it. Then the warning-suggestion (#76) would be "obsolete".

@joachimtingvold
Copy link
Contributor Author

Okay, after a bit of testing, PR #95 should do the trick. It fixes so that it doesn't use the same TTL for the entire RRset (only the same name and type), which fixes the "stuck" issue I had. It also address the issue when you try setting different TTL-values for the same name+type.

If you have the following two entries;

@ 3600 NS ns1.foo.bar
@ 3600 NS ns2.foo.bar

...and you try changing one of them to TTL=1800, it would not delete any of them, and it would set the TTL to the same for both entries, using whatever entry is topmost in the record-table when you hit "Apply Changes".

@joachimtingvold
Copy link
Contributor Author

#76 is no longer relevant imho, and I closed it.

@joachimtingvold
Copy link
Contributor Author

@ngoduykhanh -- food for you (-:

@ngoduykhanh
Copy link
Contributor

If you have the following two entries;

@ 3600 NS ns1.foo.bar
@ 3600 NS ns2.foo.bar
...and you try changing one of them to TTL=1800, it would not delete any of them, and it would > set the TTL to the same for both entries, using whatever entry is topmost in the record-table when you hit "Apply Changes".

@jallakim : I tried but it didn't work. I have a comment at your PR :)

@joachimtingvold
Copy link
Contributor Author

@ngoduykhanh - Ah, yes, my bad. Tested with the wrong branch. Should work now (-:

@joachimtingvold
Copy link
Contributor Author

Fixed in PR #95.

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

2 participants