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

Drop retrieval of forwarded domains to avoid synching unused hosted zones #336

Merged
merged 2 commits into from
Nov 29, 2023

Conversation

MartinWeindel
Copy link
Member

@MartinWeindel MartinWeindel commented Nov 22, 2023

What this PR does / why we need it:
The providers do not read anymore all accessible hosted zone to find forwarded subdomains defined by NS record sets, i.e. subdomains which have another authoritative nameserver. Only forwarded subdomains known by the domain names of accessible hosted zones will be used to select the correct zone for the domain of a DNSEntry.
This means if there is a NS record set for a subdomain of a hosted zone which is hosted by a foreign nameserver, the DNS records for the DNSEntry will still be created in the accessible hosted zone. In this case, a lookup via a public DNS server will not return these newly created DNS records. Before this change, the DNSEntry would have shown an error state, as it could not find a suitable DNSProvider.

Example for the changed behaviour:
Let's assume the credentials for a provider allows accessing three public zones: zone1.example.com, zone2.example.com, and zone3.example.com. The zone1 contains a NS record for foo.bar.zone1.example.com which is hosted on a foreign authoritative nameserver. There is only a DNSProvider including the domain bar.zone1.example.com. The dns-controller-manager finds the correct zone zone1.example.com. With the new behaviour it only reads the DNS records of zone1. The other zones are ignored as long as no DNSProvider want to access them. Formerly, also all records of zone2 and zone3 would have been read periodically to know about the forwarded domains.
Creating a DNSEntry for svc.foo.bar.zone1.example.com now results in the creation of a DNS record set in the zone zone1.example.com, which will not be found if looked up by a public DNS server, as the DNS record set should have to been placed into the forwarded hosted zone foo.bar.zone1.example.com. With the former behaviour, applying this DNSEntry would have resulted in an error state without action.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Release note:

`NS` records are not retrieved anymore for all accessible hosted zones to avoid reading all DNS record sets of all hosted zones periodically independently if they are used. Only hosted zones with active `DNSProviders` are synched, but without caring about consequences of `NS` records for subdomains. If there are many large hosted zones accessible for given credentials and there are only  `DNSProviders` using a few of these zones (either by domain or zone include), the period synchronisation of the zone state for all other hosted zones is avoided. This can result in a significant reduction of requests to the provider backend. As a downside of this change, applying a `DNSEntry` for a forwarded subdomain now results in a DNS record set in the parent hosted zone, if the real hosted zone is unknown to the controller. Formerly, applying such a `DNSEnty` resulted in an error state. 
No action is necessary from the users, this is only a "heads up" for the changed behaviour if `NS` records are used for subdomains.

@MartinWeindel MartinWeindel requested a review from a team as a code owner November 22, 2023 15:00
@gardener-robot gardener-robot added needs/review Needs review size/l Size of pull request is large (see gardener-robot robot/bots/size.py) needs/second-opinion Needs second review by someone else labels Nov 22, 2023
@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Nov 22, 2023
@MartinWeindel MartinWeindel changed the title Drop retrieval of foreign domains to avoid synching unused hosted zones Drop retrieval of forwarded domains to avoid synching unused hosted zones Nov 27, 2023
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 28, 2023
@gardener-robot-ci-3 gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 28, 2023
@MartinWeindel MartinWeindel force-pushed the selective-hosted-zone-states branch from 454f209 to 532b215 Compare November 29, 2023 09:49
@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Nov 29, 2023
@MartinWeindel MartinWeindel merged commit 17a2ca0 into master Nov 29, 2023
1 check passed
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Nov 29, 2023
@MartinWeindel MartinWeindel deleted the selective-hosted-zone-states branch November 29, 2023 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/review Needs review needs/second-opinion Needs second review by someone else size/l Size of pull request is large (see gardener-robot robot/bots/size.py) status/closed Issue is closed (either delivered or triaged)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants