Promote `DisableDNSProviderManagement` to GA #6341

MartinWeindel · 2022-07-18T07:00:05Z

How to categorize this PR?

/area open-source
/kind cleanup

What this PR does / why we need it:
The DisableDNSProviderManagement feature gate is promoted to GA and is now unconditionally enabled.
Code has been cleaned up regarding usage of DNSEntry and DNSProvider which is not used anymore since UseDNSRecord was promoted to GA or now with promotion of DisableDNSProviderManagement.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:
Follow-up of #6142
Part of #5270

Release note:

The `DisableDNSProviderManagement` feature gate has been promoted to GA and is now unconditionally enabled. If the `shoot-dns-service` extension is deployed, please make sure following prerequistes are given for a smoothly transition:
 - The `shoot-dns-service` extension must be installed in a version >= `v1.20.0`.
 - The controller deployment of the `shoot-dns-service` sets `providerConfig.values.dnsProviderManagement.enabled=true`
 - Its admission controller (`gardener-extension-admission-shoot-dns-service`) is deployed on the garden cluster
 - the `dns-external` extension must still be installed

vpnachev · 2022-07-18T07:52:50Z

/uncc

ary1992 · 2022-07-19T04:52:07Z

/assign

rfranzke · 2022-07-19T06:26:19Z

There are a few more occurrences of the external-dns-management dependency. What about them? Can they be dropped as well now?

MartinWeindel · 2022-07-19T06:36:40Z

I still have problems to run the e2e tests locally to understand the failure of the managed seed test.

There are a few more occurrences of the external-dns-management dependency. What about them? Can they be dropped as well now?

I will take a look at them after I fixed the e2e test. Any hint is welcome.

ary1992

Thank you for the PR!.
While doing joint review with @acumino, we have the following comments:

gardener/example/20-componentconfig-gardenlet.yaml

Line 124 in d19de58

DisableDNSProviderManagement: false

DisableDNSProviderManagement should be set to true or this can be removed.

gardener/pkg/features/features.go

Line 113 in b7d9ffa

// beta: v1.50

// GA: v1.52.0 needs to be added after this line.

gardener/docs/extensions/dns.md

Lines 76 to 102 in 788e942

    
           ```yaml 
        
           apiVersion: dns.gardener.cloud/v1alpha1 
        
           kind: DNSProvider 
        
           ... 
        
           status: 
        
             state: Ready 
        
             message: everything ok 
        
           ``` 
        
           Other possible states are `Pending`, `Error`, and `Invalid`. 
        
           The DNS controller may provide an explanation of the `.status.state` in the `.status.message` field. 
        
           Now Gardener may create `DNSEntry` objects that represent the ask to create an actual external DNS record: 
        
           ```yaml 
        
           --- 
        
           apiVersion: dns.gardener.cloud/v1alpha1 
        
           kind: DNSEntry 
        
           metadata: 
        
             name: dns 
        
             namespace: default 
        
           spec: 
        
             dnsName: apiserver.cluster1.dev.my-fancy-domain.com 
        
             ttl: 600 
        
             targets: 
        
             - 8.8.8.8 
        
           ```

Do we still need these in the documents ?

pkg/operation/botanist/addons_test.go

MartinWeindel · 2022-07-19T11:39:27Z

Thank you for the PR!. While doing joint review with @acumino, we have the following comments:

...

@ary1992 Thanks for your review. All requested changes have been addressed.
The failing e2e test ManagedSeeds worked for me locally, let's wait and see if Prow gets the same result.

MartinWeindel · 2022-07-19T12:17:47Z

There are a few more occurrences of the external-dns-management dependency. What about them? Can they be dropped as well now?

@rfranzke

The remaining external-dns-management imports are related to the calculation of the required extensions.
The DNSProvider related parts cannot be dropped without loosing the ControllerInstallations for the dns-external extension. It is still needed for the shoot-dns-service and shoot-cert-service.
I see three alternatives:

Provide some dependency mechanism for extensions to other extensions. I.e if shoot-dns-service is installed, also install the dns-external extension.
Change the deployment policy to Always in its ControllerRegistration
The dns-controller-manager could be deployed by the shoot-dns-service. But this would cause substantial migration, configuration and testing efforts.

We need also to discuss which component should be responsible for deploying the CRDs for DNSProvider, DNSEntry, and DNSOwner.

rfranzke · 2022-07-19T14:33:13Z

@MartinWeindel I think we should drop support for registering external-dns-management via ControllerRegistration since it's no longer a supported extension for Gardener (only indirectly via shoot-dns-service).

I think the clean solution would be that shoot-dns-service itself deploys external-dns-management. Introducing a dependency mechanism or using Always policy are workarounds FMPOV. If you think this is too much of an effort for now, then I propose to go ahead with Always deployment policy of external-dns-management but remove all .spec.resources from the ControllerRegistration (Gardener shall not understand DNSProvider any longer).

We need also to discuss which component should be responsible for deploying the CRDs for DNSProvider, DNSEntry, and DNSOwner.

Same like above, it should be shoot-dns-service.

MartinWeindel · 2022-07-19T15:54:48Z

@rfranzke
I suggest to move the cleanup work for the DNSProvider registration to a separate PR, after either the ControllerRegistration has been changed to policy Always or the deployment has moved completely to the shoot-dns-service extension.

rfranzke · 2022-07-19T16:23:30Z

@rfranzke I suggest to move the cleanup work for the DNSProvider registration to a separate PR, after either the ControllerRegistration has been changed to policy Always or the deployment has moved completely to the shoot-dns-service extension.

Sure. For now, probably it makes sense to still document (or at least announce via release note) that external-dns-management must still be registered as an extension via dedicated ControllerRegistration in case shoot-dns-service is used.

ary1992 · 2022-07-25T02:44:50Z

Thank you for the PR!. While doing joint review with @acumino, we have the following comments:
...

@ary1992 Thanks for your review. All requested changes have been addressed. The failing e2e test ManagedSeeds worked for me locally, let's wait and see if Prow gets the same result.

Thank you for the changes. I don't have any further comments. Please rebase the PR.
/lgtm

rfranzke · 2022-07-26T07:17:45Z

ping @MartinWeindel, please rebase so that we can go ahead with this PR

rfranzke · 2022-07-26T07:31:45Z

/assign

docs/deployment/feature_gates.md

extensions/README.md

pkg/features/features.go

rfranzke · 2022-07-26T11:58:48Z

pkg/operation/seed/types.go

@@ -45,7 +45,5 @@ type Components struct {

 // DNS contains all necessary DNS components for the Seed cluster.
 type DNS struct {


We don't need this struct anymore now, or?

The struct still has one field record used to create the managed Ingress DNSRecord.
See https://github.com/gardener/gardener/blob/master/pkg/operation/seed/seed.go#L1615

Making it more clear: My point was about dropping

gardener/pkg/operation/seed/types.go

Lines 46 to 51 in 5ac7715

// DNS contains all necessary DNS components for the Seed cluster.

type DNS struct {

entry component.DeployWaiter

owner component.DeployWaiter

record component.DeployMigrateWaiter

}

and changing

gardener/pkg/operation/seed/types.go

Lines 41 to 44 in 5ac7715

// Components contains different components deployed in the Seed cluster.

type Components struct {

dns *DNS

}

to

// Components contains different components deployed in the Seed cluster.[29 days ago • Tim Usner [Add CA rotation support for multi-node etcd c…]](https://sourcegraph.com/github.com/gardener/gardener/-/commit/bdd5621e4e882be510d08ae01bc1f7774803efbb) type Components struct { dnsRecord component. DeployMigrateWaiter }

rfranzke

/lgtm
/approve

Thank you @MartinWeindel !

gardener-prow · 2022-07-26T13:22:27Z

[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

rfranzke · 2022-07-26T13:24:06Z

/milestone v1.52

gardener-prow bot added area/open-source Open Source (community, enablement, contributions, conferences, CNCF, etc.) related kind/cleanup Something that is not needed anymore and can be cleaned up labels Jul 18, 2022

gardener-prow bot requested review from Kristian-ZH and vpnachev July 18, 2022 07:00

gardener-prow bot added cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 18, 2022

gardener-prow bot removed the request for review from vpnachev July 18, 2022 07:52

ialidzhikov mentioned this pull request Jul 18, 2022

Move DNSProvider capabilities out of g/g #5270

Closed

10 tasks

gardener-prow bot assigned ary1992 Jul 19, 2022

ary1992 reviewed Jul 19, 2022

View reviewed changes

pkg/operation/botanist/addons_test.go Show resolved Hide resolved

pkg/operation/botanist/addons_test.go Show resolved Hide resolved

pkg/operation/botanist/addons_test.go Show resolved Hide resolved

gardener-prow bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 19, 2022

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

gardener-prow bot assigned rfranzke Jul 26, 2022

MartinWeindel added 5 commits July 26, 2022 09:40

DisableDNSProviderManagement as GA, code cleanup

bba02d0

review related changes

50af34f

drop dnsprovider controller from local provider

2d9d9fb

drop DNSOwnerList from test

85dfd39

fix provider-local deployment

46a4408

MartinWeindel force-pushed the disablednsprovidermanagement-ga branch from ec04ccf to 46a4408 Compare July 26, 2022 07:40

gardener-prow bot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 26, 2022

rfranzke reviewed Jul 26, 2022

View reviewed changes

MartinWeindel added 2 commits July 26, 2022 15:03

drop DNS struct

2cc1259

remove dead docu links

6c3e56a

rfranzke reviewed Jul 26, 2022

View reviewed changes

gardener-prow bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 26, 2022

gardener-prow bot added this to the v1.52 milestone Jul 26, 2022

rfranzke added the component/gardener Gardener label Jul 26, 2022

gardener-prow bot merged commit c840b5b into gardener:master Jul 26, 2022

MartinWeindel deleted the disablednsprovidermanagement-ga branch December 7, 2022 09:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Promote `DisableDNSProviderManagement` to GA #6341

Promote `DisableDNSProviderManagement` to GA #6341

MartinWeindel commented Jul 18, 2022 •

edited by rfranzke

vpnachev commented Jul 18, 2022

ary1992 commented Jul 19, 2022

rfranzke commented Jul 19, 2022

MartinWeindel commented Jul 19, 2022

ary1992 left a comment

MartinWeindel commented Jul 19, 2022

MartinWeindel commented Jul 19, 2022

rfranzke commented Jul 19, 2022

MartinWeindel commented Jul 19, 2022

rfranzke commented Jul 19, 2022

ary1992 commented Jul 25, 2022 •

edited

rfranzke commented Jul 26, 2022

rfranzke commented Jul 26, 2022

rfranzke Jul 26, 2022

MartinWeindel Jul 26, 2022

rfranzke Jul 26, 2022

MartinWeindel Jul 26, 2022

rfranzke left a comment

gardener-prow bot commented Jul 26, 2022

rfranzke commented Jul 26, 2022

	```yaml
	apiVersion: dns.gardener.cloud/v1alpha1
	kind: DNSProvider
	...
	status:
	state: Ready
	message: everything ok
	```

	Other possible states are `Pending`, `Error`, and `Invalid`.
	The DNS controller may provide an explanation of the `.status.state` in the `.status.message` field.

	Now Gardener may create `DNSEntry` objects that represent the ask to create an actual external DNS record:

	```yaml
	---
	apiVersion: dns.gardener.cloud/v1alpha1
	kind: DNSEntry
	metadata:
	name: dns
	namespace: default
	spec:
	dnsName: apiserver.cluster1.dev.my-fancy-domain.com
	ttl: 600
	targets:
	- 8.8.8.8
	```

		@@ -45,7 +45,5 @@ type Components struct {

		// DNS contains all necessary DNS components for the Seed cluster.
		type DNS struct {

	// DNS contains all necessary DNS components for the Seed cluster.
	type DNS struct {
	entry component.DeployWaiter
	owner component.DeployWaiter
	record component.DeployMigrateWaiter
	}

	// Components contains different components deployed in the Seed cluster.
	type Components struct {
	dns *DNS
	}

Promote DisableDNSProviderManagement to GA #6341

Promote DisableDNSProviderManagement to GA #6341

Conversation

MartinWeindel commented Jul 18, 2022 • edited by rfranzke

vpnachev commented Jul 18, 2022

ary1992 commented Jul 19, 2022

rfranzke commented Jul 19, 2022

MartinWeindel commented Jul 19, 2022

ary1992 left a comment

Choose a reason for hiding this comment

MartinWeindel commented Jul 19, 2022

MartinWeindel commented Jul 19, 2022

rfranzke commented Jul 19, 2022

MartinWeindel commented Jul 19, 2022

rfranzke commented Jul 19, 2022

ary1992 commented Jul 25, 2022 • edited

rfranzke commented Jul 26, 2022

rfranzke commented Jul 26, 2022

rfranzke Jul 26, 2022

Choose a reason for hiding this comment

MartinWeindel Jul 26, 2022

Choose a reason for hiding this comment

rfranzke Jul 26, 2022

Choose a reason for hiding this comment

MartinWeindel Jul 26, 2022

Choose a reason for hiding this comment

rfranzke left a comment

Choose a reason for hiding this comment

gardener-prow bot commented Jul 26, 2022

rfranzke commented Jul 26, 2022

Promote `DisableDNSProviderManagement` to GA #6341

Promote `DisableDNSProviderManagement` to GA #6341

MartinWeindel commented Jul 18, 2022 •

edited by rfranzke

ary1992 commented Jul 25, 2022 •

edited