SetRecords: preserve RRSets not set in the input #7
Conversation
|
Thanks for the PR. I am going to find some time to review it soon, but I can't promise and particular time frame. |
|
You're definitely correct that the current behavior is incorrect. Thank you for spotting this and sending a fix. If you don't mind, I would like to go a bit further than what you proposed here and clean up the complexity I introduced with this bug. IIUC, we no longer need to read the current state, instead we can immediately call putRRSets for records we receive as arguments. Not only will that reduce complexity, it will also make concurrent modifications of the same zone behave a lot better. |
|
Of course, I don't mind abandoning this PR in favor of a better solution, as long as the incorrect behavior is fixed :) Feel free to close this PR. |
|
I was hoping you take that on, but I can do that too :) |
|
I can certainly try! I'll see if I can prepare a new version by Monday, let's just make sure not to duplicate the effort. |
|
SG, happy to help if you have any questions. Let me leave a few comments on this change to give some guidance. |
|
Thanks for your comments @znkr! PTAL at the new version :) |
znkr
left a comment
znkr
left a comment
There was a problem hiding this comment.
This looks good, thank you very much. I had a few minor comments but I am happy to merge this PR once they are addressed.
According to the libdns specification for SetRecords [1]: > SetRecords updates the zone so that the records described in the input > are reflected in the output. It may create or update records > or—depending on the record type—delete records to maintain parity with > the input. No other records are affected. It returns the records which > were set. > > For any (name, type) pair in the input, SetRecords ensures that the > only records in the output zone with that (name, type) pair are those > that were provided in the input. From this description, especially the "No other records are affected" part, I believe that RRSets which are not mentioned in the input should not be modified or deleted by SetRecords. This is also made clear by one of the examples. Note that the TXT record is preserved: > Example 1: > > ;; Original zone > example.com. 3600 IN A 192.0.2.1 > example.com. 3600 IN A 192.0.2.2 > example.com. 3600 IN TXT "hello world" > > ;; Input > example.com. 3600 IN A 192.0.2.3 > > ;; Resultant zone > example.com. 3600 IN A 192.0.2.3 > example.com. 3600 IN TXT "hello world" The current implementation of SetRecords deletes all RRSets from the zone which are not mentioned in the input. This commit changes the behavior to match the spec. [1] https://github.com/libdns/libdns/blob/5ab1d4de259f1eb914085c61784ad9176ea8e803/libdns.go#L108
|
Thank you :) |
|
I'll tag and release a patch version in the evening (only have access via web right now) |
2 participants
According to the libdns specification for SetRecords [1]:
From this description, especially the "No other records are affected" part, I believe that RRSets which are not mentioned in the input should not be modified or deleted by SetRecords.
This is also made clear by one of the examples.
Note that the TXT record is preserved:
The current implementation of SetRecords deletes all RRSets from the zone which are not mentioned in the input. This PR changes the behavior to match the spec.
Tested the change by running the integration test.
[1] https://github.com/libdns/libdns/blob/5ab1d4de259f1eb914085c61784ad9176ea8e803/libdns.go#L108