Skip to content

Make IGNORE work with all providers#313

Merged
tlimoncelli merged 1 commit intoStackExchange:masterfrom
masterzen:bugfix/ignore-should-compare-with-namefqdn
Feb 1, 2018
Merged

Make IGNORE work with all providers#313
tlimoncelli merged 1 commit intoStackExchange:masterfrom
masterzen:bugfix/ignore-should-compare-with-namefqdn

Conversation

@masterzen
Copy link
Copy Markdown
Collaborator

the differ.matchIgnored function was comparing only with the Name
field of RecordConfig.

Most providers don't set RecordConfig.Name, only RecordConfig.NameFQDN
and for those IGNORE doesn't work at all.

This patch converts IGNORE to use NameFQDN instead since it seems
all providers provides it.

the `differ.matchIgnored` function was comparing only with the `Name`
field of `RecordConfig`.

Most providers don't set `RecordConfig.Name`, only `RecordConfig.NameFQDN`
and for those `IGNORE` doesn't work at all.

This patch converts `IGNORE` to use `NameFQDN` instead since it seems
all providers provides it.
@tlimoncelli
Copy link
Copy Markdown
Contributor

Which providers don't set .Name?

@tlimoncelli
Copy link
Copy Markdown
Contributor

Try merging in #314 and let me know if any providers now panic. This should find any providers that aren't filling in .Name. Once those are fixed, differ should work without the need for #313 (but i could be wrong).

@captncraig
Copy link
Copy Markdown
Contributor

No Tom, that won't fix it. The problem is providers are inconsistent with how they store name/namefqdn when the create records from the api representations. Since diffing downstream has always relied on fqdn, thats the only field that has been strictly required.

The validation phase won't help this, since it is the internal representation from the drivers, not the user input that is inconsistent.

I think this pr is probably the easiest way to fix the issue.

@masterzen
Copy link
Copy Markdown
Collaborator Author

@tlimoncelli the quick look I had shows that those providers don't set Name:

  • Route53
  • Cloudflare
  • dnsimple
  • digitalocean
  • gcloud
  • linode
  • namecheap
  • namedotcom
  • vultr

Most of them I think use an API that returns only FQDN, so they set the NameFQDN.

@captncraig, what would you think of computing Name from NameFQDN during PostProcessRecords ?

That way all providers would fill Name automatically.

@tlimoncelli
Copy link
Copy Markdown
Contributor

I have to admit that I'm quite surprised. I looked at that list and said "That can't be true! I wrote some of those providers!" but, yes, you are absolutely correct. They don't set .Name.

I don't understand how they could have been working all these years, other than to think that code just doesn't use the .Name field very often and our integration tests caught any problems.

What I also don't understand is that when I add that validation step to the top of the Differ, it still works. That tells me that .Name is getting set somewhere.

@masterzen
Copy link
Copy Markdown
Collaborator Author

@tlimoncelli,

The thing that tricked me into using Name for the ignore stuff is that the integration test sets Name, not the fqdn variant :)

My coworker discovered the issue by using IGNORE with the Route53 provider (which I didn't try beforehand). No records were ignored, and looking at the Name we saw that it was empty.

I think the reason your code doesn't panic is that you're testing the dc.Records which are the records you want to manage, not the ones coming from the existing provider. You need to add the test to differ.IncrementalDiff on existing. I believe all that is coming from the javascript file is properly set with Name and NameFQDN, it's only the records coming from the various providers API that lack the Name.

@tlimoncelli
Copy link
Copy Markdown
Contributor

Yup, I agree with your analysis.

Some day (maybe later this year) I want to unexport Name/NameFQDN and force everyone to use Setters/Gettings that do the right thing. However, that's going to be a big refactoring. In the meanwhile I'm going to add the Setters/Getters so that new code can use them.

@tlimoncelli tlimoncelli merged commit b7c6efa into StackExchange:master Feb 1, 2018
@masterzen masterzen deleted the bugfix/ignore-should-compare-with-namefqdn branch February 2, 2018 09:54
rblenkinsopp pushed a commit to rblenkinsopp/dnscontrol that referenced this pull request Aug 21, 2020
the `differ.matchIgnored` function was comparing only with the `Name`
field of `RecordConfig`.

Most providers don't set `RecordConfig.Name`, only `RecordConfig.NameFQDN`
and for those `IGNORE` doesn't work at all.

This patch converts `IGNORE` to use `NameFQDN` instead since it seems
all providers provides it.
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

Successfully merging this pull request may close these issues.

3 participants