Skip to content

Add R53_ZONE as an domain identifier#1241

Merged
tlimoncelli merged 1 commit into
DNSControl:masterfrom
Enrise:R53_ZONE-as-domain-identifier
Sep 2, 2021
Merged

Add R53_ZONE as an domain identifier#1241
tlimoncelli merged 1 commit into
DNSControl:masterfrom
Enrise:R53_ZONE-as-domain-identifier

Conversation

@blackshadev
Copy link
Copy Markdown
Contributor

Using R53_ZONE allows you to differentiate between split horizon domains across zones.

But why?

Well, imagine you have a hosted public zone called enrise.com and a private zone for enrise.com specific to a VPC. different enrise.com zones across different VPCs. In these examples DNSControl doesn't allow you to specify the zone by ID. This MR addresses this by using the R53_ZONE directive inside the D directive

@tlimoncelli
Copy link
Copy Markdown
Contributor

Looks good so far. Please add some tests. It needs a test in pkg/js/parse_tests. I'd like to see a test in integrationTest/integration_test.go but I'm not sure if that's possible without hardcoding a Zone ID.

@blackshadev
Copy link
Copy Markdown
Contributor Author

Looks good so far. Please add some tests. It needs a test in pkg/js/parse_tests. I'd like to see a test in integrationTest/integration_test.go but I'm not sure if that's possible without hardcoding a Zone ID.

Sure, I added some parse_tests. I am not sure how to run them locally. Could you tel me that? for both the pkg/js/parse_tests and the integrationTest/integration_test.go

@blackshadev
Copy link
Copy Markdown
Contributor Author

Looks good so far. Please add some tests. It needs a test in pkg/js/parse_tests. I'd like to see a test in integrationTest/integration_test.go but I'm not sure if that's possible without hardcoding a Zone ID.

Regarding the integration tests, I am unsure how to do this. I do not know the ID beforehand. Which makes it hard to tests. I would need to change the intergration test runner such that we can get zoneId before trying the second time with another domain-name. But that seems pretty fragile to me.

@tlimoncelli
Copy link
Copy Markdown
Contributor

To run the tests locally just run go test ./... in the right subdirectory:

cd ~/git/dnscontrol/pkg/js
go test ./...

My suggestion is to edit parse_tests/040-r53-zone.js so that it has a D() example and a R53_ALIAS example. Then make sure the .json file has the expected output.

@tlimoncelli
Copy link
Copy Markdown
Contributor

Regarding the integration tests, I am unsure how to do this. I do not know the ID beforehand. Which makes it hard to tests. I would need to change the intergration test runner such that we can get zoneId before trying the second time with another domain-name. But that seems pretty fragile to me.

Yeah, sounds like there isn't a generic way to do this. Oh well.

@blackshadev
Copy link
Copy Markdown
Contributor Author

To run the tests locally just run go test ./... in the right subdirectory:

cd ~/git/dnscontrol/pkg/js
go test ./...

My suggestion is to edit parse_tests/040-r53-zone.js so that it has a D() example and a R53_ALIAS example. Then make sure the .json file has the expected output.

Thanks, and Done, I added the R53_alias as well like you suggested. It didn't run at first because of caching, but by adding -count=1 it worked fine.

@tlimoncelli
Copy link
Copy Markdown
Contributor

tlimoncelli commented Aug 27, 2021 via email

@tlimoncelli
Copy link
Copy Markdown
Contributor

  • Please rebase
  • Please integrate my suggested rewrite of the doc.

@blackshadev
Copy link
Copy Markdown
Contributor Author

  • Please rebase
  • Please integrate my suggested rewrite of the doc.

I dont see any comment about the doc rewrite, could you elaborate?

Comment thread docs/_functions/record/R53_ZONE.md Outdated
@tlimoncelli
Copy link
Copy Markdown
Contributor

I guess the suggestion about the docs didn't get saved the first time. Take a look now.

@blackshadev
Copy link
Copy Markdown
Contributor Author

I guess the suggestion about the docs didn't get saved the first time. Take a look now.

Done

@tlimoncelli
Copy link
Copy Markdown
Contributor

Sorry for the delay, things have been busy here with Ida.

I think this PR is ready for merge. If you agree, rebase and confirm that it is ready.

Using R53_ZONE allows you to differentiate between split horizon
domains across zones.
@blackshadev
Copy link
Copy Markdown
Contributor Author

Sorry for the delay, things have been busy here with Ida.

I think this PR is ready for merge. If you agree, rebase and confirm that it is ready.

I rebased it (again). And yes it is ready to merge.

@tlimoncelli
Copy link
Copy Markdown
Contributor

Awesome! Thanks for this new feature! I'm sure everyone that uses AWS will appreciate it!

@tlimoncelli tlimoncelli merged commit 8c5db2e into DNSControl:master Sep 2, 2021
@tlimoncelli tlimoncelli mentioned this pull request Sep 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants