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

Root NS record support #590

Open
wants to merge 27 commits into
base: master
from
Open

Root NS record support #590

wants to merge 27 commits into from

Conversation

@acm1
Copy link
Contributor

acm1 commented Jul 23, 2020

This is built on #434. I'm opening this PR to test mergeability, but I've also opened joschi36#6 so that @joschi36 can merge it there if desired.

joschi36 and others added 27 commits Dec 11, 2019
@acm1 acm1 mentioned this pull request Jul 23, 2020
@gildegoma
Copy link

gildegoma commented Jul 23, 2020

Hi @acm1 @ross @joschi36 !
This is great! We were also working on reactivating this change request "ASAP" 😄.

In the meantime, we (@swisstxt) have rebased onto the upstream master and integrated Joschi's changes with some Ross' comments on #434. See https://github.com/swisstxt/octodns-2/tree/feature/root-nameservers.


PS: don't worry about the odd octodns-2 fork name (this is temporary, until we can switch from a legacy repo, and then it will be renamed).

@gildegoma
Copy link

gildegoma commented Jul 23, 2020

Sorry, I updated the above comment.

@ross please let us know if you're okay to review+merge #434 or #590, or if you need/appreciate a cleaner+rebased feature branch.

@istr
Copy link

istr commented Aug 2, 2020

@acm1 you could set SUPPORTS_ROOT_NS = True for digitalocean.py provider.
I tried it and it works out-of-the-box with this PR.

joschi36 added a commit to joschi36/octodns that referenced this pull request Aug 3, 2020
@ross
Copy link
Contributor

ross commented Aug 3, 2020

@ross please let us know if you're okay to review+merge #434 or #590, or if you need/appreciate a cleaner+rebased feature branch.

Swamped at the moment and haven't been able to string together enough time to look at them.

@acm1 you could set SUPPORTS_ROOT_NS = True for digitalocean.py provider.
I tried it and it works out-of-the-box with this PR.

I'd like to see the ability to manage ROOT NS records and Dyn was always one of the problematic providers (and it's 💀 soon) so 🤞 we can find a path here. SUPPORTS_ROOT_NS is probably part of that.

@istr
Copy link

istr commented Aug 3, 2020

Another finding with the ns1 provider that I am currently unable to find a good fix for is this:
NS handling works, but not right after creation of new zones.
This is what happens

  1. the plan to create a new zone is added
  2. the plan to insert a new NS entry is added
  3. the provider ns1 creates the zone
  4. the provider ns1 automatically creates a new NS entry with the default servers
  5. now the plan fails, because we need an update of the NS entries instead of a create

There are two possible ways to remedy the situation, both would need to re-scan / re-check the provider's state:

  1. always make zone creations the first items in a plan; then re-check the state (re-scan the inventory); then build the plan for new/changed entries
  2. after each zone creation that is followed by NS manipulation re-check immediately before the planned NS items and change a create plan to a update plan when necessary

Another option that would need more research would be to create new zones at the ns1 provider always as secondary zones (this suppresses the automatic creation of the NS entries, as per documentation). But I am unsure if this could be done for zones that are meant to be primary.

I could dig deeper into this, but I am not yet deeply acquainted with the workflow within the provider logic (plan building, hooks, actions etc.), so I could not tell which of these options is the better one (less code / easier to implement / better to read).

@ramnes
Copy link
Contributor

ramnes commented Aug 28, 2020

Just a quick word here to say that we also needed to modify Route 53 root NS for a couple of zones, and this branch worked just as expected.

Edit: Oops, I spoke too fast, the plan worked but not the actual change: botocore.errorfactory.InvalidChangeBatch: An error occurred (InvalidChangeBatch) when calling the ChangeResourceRecordSets operation: [Tried to create resource record set [name='myzone.com.', type='NS'] but it already exists]

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

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.