Clean up DNS handling #8199

timuthy · 2023-06-30T15:25:50Z

How to categorize this PR?

/area open-source
/kind cleanup

What this PR does / why we need it:
This PR cleans up some leftovers regarding DNS handling, please see individual commits.

It mainly drops support for the nip.io domains - A functionality that's been broken for a long time, is untested and likely not used by the community.

Special notes for your reviewer:
/cc @rfranzke

Release note:

Support for `nip.io` shoot domains is discontinued.

Shoot fields `.spec.dns.providers[].domains` and `.spec.dns.providers[].zones` are now deprecated and expected to be removed in version `v1.87`. Please use the extensions' configuration to configure providers with this ability.

Shoot fields `.spec.dns.providers[].domains` and `.spec.dns.providers[].zones` are now deprecated and expected to be removed in version `v1.87`. Please plan ahead to drop using those fields in extensions.

`IncludeZones` and `ExcludeZones` are a leftover when Gardenlet created `DNSEntries` instead of `DNSRecords`.

`IncludeDomains` and `ExcludeDomains` are obsolete and not supported by `DNSRecord`s.

Earlier, we used `!useSNI` which was later replaced by `!useDNS` (v1.73) but at those places a 1:1 replacement is not possible. We don't support non-SNI and thus won't deploy Kube-Apiserver Services of Type `LoadBalancer`. Now we can always deploy the service, also when the cluster is hibernated.

Support for `nip.io` is obsolete: - It's been broken for a long time (shoot fails with `Could not initialize a new operation for Shoot cluster: unable to figure out which secret should be used for dns`) - It's not documented at all

shafeeqes

Thanks for the PR, Only one doubt.

pkg/apis/core/v1beta1/types_shoot.go

pkg/utils/gardener/dns.go

rfranzke · 2023-07-04T08:18:52Z

pkg/apis/core/types_shoot.go

+// Note: This struct is not used by Gardener anymore. However, providers might still be synced by the DNS extension which
+// is mainly why we can't deprecate or remove it, see https://github.com/gardener/gardener-extension-shoot-dns-service/blob/f599bec786ab6e27e6fd41d423d4e73dd4952f4f/pkg/admission/mutator/shoot_mutator.go#L110.


What is the plan forward to drop it? Probably this DNS extension should adapt to use its provider config? cc @MartinWeindel
Is there an issue which we can track and link here?

The extension already uses its provider config. Due to compatibility reasons there is a provider sync functionality that copies dns.providers to the extension's provider config.
I wondered as well if we plan to drop this functionality. OTHO, we probably won't remove the domains and zones field anytime soon as it's part of the shoot API and would rather consider removing it when we promote the API to v1? 😄
We can however already add an deprecation note now?!

What is the plan forward to drop it? Probably this DNS extension should adapt to use its provider config? cc @MartinWeindel Is there an issue which we can track and link here?

Thanks @timuthy for your comments. I have not much to add to that. I see the Gardener core team in the driver seat for further planning to define what fields to drop with API v1 as they are partially used by the DNSRecord controller, too.

We can also drop fields in v1beta1 after a proper deprecation period so that users have time to adopt. We already did so many times in the past.
Hence, I suggest to deprecate it know with the plan to finally remove after v1.87 has been released (first release in 2024).
Then people have enough time to switch to providerConfig only.

Fair enough, I adjusted the doc strings and release notes.

FYI: gardener/gardener-extension-shoot-dns-service#220

gardener-prow · 2023-07-05T09:41:33Z

@timuthy: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-gardener-apidiff	`3bc1627`	link	false	`/test pull-gardener-apidiff`

Full PR test history. Your PR dashboard. Command help for this repository.
Please help us cut down on flakes by linking this test failure to an open flake report or filing a new flake report if you can't find an existing one. Also see our testing guideline for how to avoid and hunt flakes.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

rfranzke

/lgtm
/approve

Thank you @timuthy

gardener-prow · 2023-07-05T14:51:46Z

LGTM label has been added.

Git tree hash: 32f66d1aa5c21f860d358f317daae84545806651

gardener-prow · 2023-07-05T14:51:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rfranzke

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [rfranzke]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

* Drop `IncludeZones` and `ExcludeZones` `IncludeZones` and `ExcludeZones` are a leftover when Gardenlet created `DNSEntries` instead of `DNSRecords`. * Drop `IncludeDomains` and `ExcludeDomains` `IncludeDomains` and `ExcludeDomains` are obsolete and not supported by `DNSRecord`s. * Drop irrelevant condition in shoot flow Earlier, we used `!useSNI` which was later replaced by `!useDNS` (v1.73) but at those places a 1:1 replacement is not possible. We don't support non-SNI and thus won't deploy Kube-Apiserver Services of Type `LoadBalancer`. Now we can always deploy the service, also when the cluster is hibernated. * Drop `nip.io` support Support for `nip.io` is obsolete: - It's been broken for a long time (shoot fails with `Could not initialize a new operation for Shoot cluster: unable to figure out which secret should be used for dns`) - It's not documented at all * [make generate] * Address review comment * Address review comments * Deprecate 'Domains' and 'Zones'

timuthy added 4 commits June 30, 2023 17:08

Drop IncludeZones and ExcludeZones

d39c813

`IncludeZones` and `ExcludeZones` are a leftover when Gardenlet created `DNSEntries` instead of `DNSRecords`.

Drop IncludeDomains and ExcludeDomains

81996d2

`IncludeDomains` and `ExcludeDomains` are obsolete and not supported by `DNSRecord`s.

Drop nip.io support

fbd8b1e

Support for `nip.io` is obsolete: - It's been broken for a long time (shoot fails with `Could not initialize a new operation for Shoot cluster: unable to figure out which secret should be used for dns`) - It's not documented at all

gardener-prow bot requested a review from rfranzke June 30, 2023 15:25

[make generate]

ca72bc2

shafeeqes reviewed Jul 3, 2023

View reviewed changes

pkg/apis/core/v1beta1/types_shoot.go Outdated Show resolved Hide resolved

rfranzke reviewed Jul 4, 2023

View reviewed changes

Address review comment

1450ff7

timuthy force-pushed the cleanup.dns-handling branch from 7c58e8a to 1450ff7 Compare July 4, 2023 08:32

timuthy added 2 commits July 5, 2023 11:39

Address review comments

b25c11e

Deprecate 'Domains' and 'Zones'

3bc1627

timuthy force-pushed the cleanup.dns-handling branch from f023093 to 3bc1627 Compare July 5, 2023 09:39

timuthy requested a review from rfranzke July 5, 2023 10:30

timuthy mentioned this pull request Jul 5, 2023

Discontinue support for SyncProvidersFromShootSpecDNS gardener/gardener-extension-shoot-dns-service#220

Open

rfranzke reviewed Jul 5, 2023

View reviewed changes

gardener-prow bot assigned rfranzke Jul 5, 2023

gardener-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jul 5, 2023

gardener-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 5, 2023

gardener-prow bot merged commit 1549014 into gardener:master Jul 5, 2023

timuthy deleted the cleanup.dns-handling branch July 7, 2023 06:32

rfranzke mentioned this pull request Feb 8, 2024

🧹 Cleanup a few TODOs #9145

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up DNS handling #8199

Clean up DNS handling #8199

timuthy commented Jun 30, 2023 •

edited by rfranzke

Loading

shafeeqes left a comment •

edited

Loading

rfranzke Jul 4, 2023

timuthy Jul 4, 2023

MartinWeindel Jul 4, 2023

rfranzke Jul 4, 2023

timuthy Jul 5, 2023

timuthy Jul 5, 2023

gardener-prow bot commented Jul 5, 2023 •

edited

Loading

rfranzke left a comment

gardener-prow bot commented Jul 5, 2023

gardener-prow bot commented Jul 5, 2023

		// Note: This struct is not used by Gardener anymore. However, providers might still be synced by the DNS extension which
		// is mainly why we can't deprecate or remove it, see https://github.com/gardener/gardener-extension-shoot-dns-service/blob/f599bec786ab6e27e6fd41d423d4e73dd4952f4f/pkg/admission/mutator/shoot_mutator.go#L110.

Clean up DNS handling #8199

Clean up DNS handling #8199

Conversation

timuthy commented Jun 30, 2023 • edited by rfranzke Loading

shafeeqes left a comment • edited Loading

Choose a reason for hiding this comment

rfranzke Jul 4, 2023

Choose a reason for hiding this comment

timuthy Jul 4, 2023

Choose a reason for hiding this comment

MartinWeindel Jul 4, 2023

Choose a reason for hiding this comment

rfranzke Jul 4, 2023

Choose a reason for hiding this comment

timuthy Jul 5, 2023

Choose a reason for hiding this comment

timuthy Jul 5, 2023

Choose a reason for hiding this comment

gardener-prow bot commented Jul 5, 2023 • edited Loading

rfranzke left a comment

Choose a reason for hiding this comment

gardener-prow bot commented Jul 5, 2023

gardener-prow bot commented Jul 5, 2023

timuthy commented Jun 30, 2023 •

edited by rfranzke

Loading

shafeeqes left a comment •

edited

Loading

gardener-prow bot commented Jul 5, 2023 •

edited

Loading