From 0e8ef765e2bc440dbbff138231bb16dd5e837cfd Mon Sep 17 00:00:00 2001 From: Martin Weindel Date: Fri, 13 Dec 2024 13:51:17 +0100 Subject: [PATCH 1/3] ensure record for DNSEntries are left untouched during migration --- go.mod | 3 +- go.sum | 6 +- pkg/controller/common/utils.go | 14 ++++- pkg/controller/lifecycle/actuator.go | 82 +++++++++++++++++++++++++++- 4 files changed, 98 insertions(+), 7 deletions(-) diff --git a/go.mod b/go.mod index b2906683..4b478178 100644 --- a/go.mod +++ b/go.mod @@ -4,7 +4,7 @@ go 1.23.0 require ( github.com/ahmetb/gen-crd-api-reference-docs v0.3.1-0.20241014194617-ffc4efda75d4 - github.com/gardener/external-dns-management v0.22.1 + github.com/gardener/external-dns-management v0.22.2 github.com/gardener/gardener v1.110.0 github.com/go-logr/logr v1.4.2 github.com/hashicorp/go-multierror v1.1.1 @@ -43,6 +43,7 @@ require ( github.com/fsnotify/fsnotify v1.7.0 // indirect github.com/fxamacker/cbor/v2 v2.7.0 // indirect github.com/gardener/cert-management v0.17.1 // indirect + github.com/gardener/controller-manager-library v0.2.1-0.20241206090116-9fadce45689c // indirect github.com/gardener/etcd-druid v0.25.0 // indirect github.com/gardener/machine-controller-manager v0.55.1 // indirect github.com/go-logr/zapr v1.3.0 // indirect diff --git a/go.sum b/go.sum index edcfe219..803afae4 100644 --- a/go.sum +++ b/go.sum @@ -95,10 +95,12 @@ github.com/fxamacker/cbor/v2 v2.7.0 h1:iM5WgngdRBanHcxugY4JySA0nk1wZorNOpTgCMedv github.com/fxamacker/cbor/v2 v2.7.0/go.mod h1:pxXPTn3joSm21Gbwsv0w9OSA2y1HFR9qXEeXQVeNoDQ= github.com/gardener/cert-management v0.17.1 h1:vawZGN+rsCRMviacnnMSWELbuIJsNXHaqaLbZ4hYADw= github.com/gardener/cert-management v0.17.1/go.mod h1:cwSsyN935017HojKVuWqw2TBhiaxSisX132D9Tn+n9I= +github.com/gardener/controller-manager-library v0.2.1-0.20241206090116-9fadce45689c h1:im/eYurY6+CzlRtxYzes6IfqMPIuzQ7Cy3sA8D/t528= +github.com/gardener/controller-manager-library v0.2.1-0.20241206090116-9fadce45689c/go.mod h1:fyLOrcaKtGno4McZKW21b6QtwNghCF0IemTLKcwKZlM= github.com/gardener/etcd-druid v0.25.0 h1:mR9/x5r27pO+I+XzpNcN2DDenam+7ITrhc7qKt9rbsI= github.com/gardener/etcd-druid v0.25.0/go.mod h1:6C0eyfdlw6CowLm/l4ZiKwrvkc+5NHrnc/rY2wCUwys= -github.com/gardener/external-dns-management v0.22.1 h1:WEwCDOersJ7ezeDJelbGVac1BTmEveJuds3JlJc84Xg= -github.com/gardener/external-dns-management v0.22.1/go.mod h1:2P7PamBPMKIOZMYRhl/VFhxZEBn4VUTdjESjKPxvOXA= +github.com/gardener/external-dns-management v0.22.2 h1:caSPJBLFHv9Y95IAwk1HvarIUCjDccLcyuyjW1qqwhM= +github.com/gardener/external-dns-management v0.22.2/go.mod h1:adBY3qQ39Fvc2PvihP4xzEE5Y2//GuurMXQpKylMOJ4= github.com/gardener/gardener v1.110.0 h1:Ix/NeYJyYIIDRHqO0126JYPGNVKy2kDEco7RyXuCYwo= github.com/gardener/gardener v1.110.0/go.mod h1:Ge2wQMWm0NmQZP3L/WMejpfXsnGbfTFBEZud819P3vU= github.com/gardener/machine-controller-manager v0.55.1 h1:d6mTnuYko+jWeIi7tAFWgWnL1nR5hGcI6pRCDcH0TGY= diff --git a/pkg/controller/common/utils.go b/pkg/controller/common/utils.go index a0131755..5a4e240c 100644 --- a/pkg/controller/common/utils.go +++ b/pkg/controller/common/utils.go @@ -20,9 +20,10 @@ import ( ) const ( - ANNOTATION_OPERATION = constants.GardenerOperation - ANNOTATION_OPERATION_MIGRATE = constants.GardenerOperationMigrate - ANNOTATION_OPERATION_RESTORE = constants.GardenerOperationRestore + ANNOTATION_OPERATION = constants.GardenerOperation + ANNOTATION_OPERATION_MIGRATE = constants.GardenerOperationMigrate + ANNOTATION_OPERATION_RESTORE = constants.GardenerOperationRestore + ANNOTATION_OPERATION_RESTORE_STEP1 = "restore/step1" ) func CopyMap(m map[string]string) map[string]string { @@ -67,6 +68,13 @@ func IsRestoring(ex *extensionsv1alpha1.Extension) bool { return ex.Annotations[ANNOTATION_OPERATION] == ANNOTATION_OPERATION_RESTORE } +func IsRestoringStep1(ex *extensionsv1alpha1.Extension) bool { + if ex.Annotations == nil { + return false + } + return ex.Annotations[ANNOTATION_OPERATION] == ANNOTATION_OPERATION_RESTORE_STEP1 +} + // ShortenID shortens an identifier longer than maxlen characters by cutting the string // and adding a hash suffix so that total length is maxlen. Identifiers are preserved // if length < maxlen. diff --git a/pkg/controller/lifecycle/actuator.go b/pkg/controller/lifecycle/actuator.go index a678e0a3..027bfffd 100644 --- a/pkg/controller/lifecycle/actuator.go +++ b/pkg/controller/lifecycle/actuator.go @@ -13,6 +13,7 @@ import ( "time" dnsv1alpha1 "github.com/gardener/external-dns-management/pkg/apis/dns/v1alpha1" + "github.com/gardener/external-dns-management/pkg/dns" "github.com/gardener/gardener/extensions/pkg/controller" "github.com/gardener/gardener/extensions/pkg/controller/extension" "github.com/gardener/gardener/extensions/pkg/util" @@ -258,6 +259,21 @@ func (a *actuator) delete(ctx context.Context, log logr.Logger, ex *extensionsv1 // Restore the Extension resource. func (a *actuator) Restore(ctx context.Context, log logr.Logger, ex *extensionsv1alpha1.Extension) error { + // First run extension reconciliation with deactivated DNSOwner to avoid + // zone reconciliation before all entries are reconciled. + // Premature zone reconciliation can lead to DNS entries being deleted temporarily. + exCopy := ex.DeepCopy() + if exCopy.Annotations == nil { + exCopy.Annotations = map[string]string{} + } + exCopy.Annotations[common.ANNOTATION_OPERATION] = common.ANNOTATION_OPERATION_RESTORE_STEP1 + if err := a.Reconcile(ctx, log, exCopy); err != nil { + return err + } + + if err := a.waitForEntryReconciliation(ctx, log, ex); err != nil { + return err + } return a.Reconcile(ctx, log, ex) } @@ -268,9 +284,73 @@ func (a *actuator) Migrate(ctx context.Context, log logr.Logger, ex *extensionsv return err } + if err := a.ignoreDNSEntriesForMigration(ctx, ex); err != nil { + return err + } + return a.delete(ctx, log, ex, true) } +func (a *actuator) ignoreDNSEntriesForMigration(ctx context.Context, ex *extensionsv1alpha1.Extension) error { + entriesHelper := common.NewShootDNSEntriesHelper(ctx, a.Client(), ex) + list, err := entriesHelper.List() + if err != nil { + return err + } + for _, entry := range list { + patch := client.MergeFrom(entry.DeepCopy()) + if entry.Annotations == nil { + entry.Annotations = map[string]string{} + } + entry.Annotations[dns.AnnotationHardIgnore] = "true" + if err := client.IgnoreNotFound(a.Client().Patch(ctx, &entry, patch)); err != nil { + return fmt.Errorf("failed to ignore DNS entry %q: %w", entry.Name, err) + } + } + return nil +} + +func (a *actuator) waitForEntryReconciliation(ctx context.Context, log logr.Logger, ex *extensionsv1alpha1.Extension) error { + entriesHelper := common.NewShootDNSEntriesHelper(ctx, a.Client(), ex) + list, err := entriesHelper.List() + if err != nil { + return err + } + + // annotate all entries with gardener.cloud/operation=reconcile + for _, entry := range list { + patch := client.MergeFrom(entry.DeepCopy()) + if entry.Annotations == nil { + entry.Annotations = map[string]string{} + } + entry.Annotations[v1beta1constants.GardenerOperation] = v1beta1constants.GardenerOperationReconcile + delete(entry.Annotations, dns.AnnotationHardIgnore) // should not be needed as the DNSEntries have been recreated, but just to be sure + if err := client.IgnoreNotFound(a.Client().Patch(ctx, &entry, patch)); err != nil { + return fmt.Errorf("failed to revert ignore DNS entry %q: %w", entry.Name, err) + } + } + + // wait for all entries to be reconciled, i.e. gardener.cloud/operation annotation is removed + start := time.Now() + for _, entry := range list { + for { + if err := a.Client().Get(ctx, client.ObjectKeyFromObject(&entry), &entry); err != nil { + return err + } + if _, ok := entry.Annotations[v1beta1constants.GardenerOperation]; !ok { + log.Info("DNS entry reconciled", "entry", entry.Name) + break + } + if time.Since(start) > 3*time.Minute { + return fmt.Errorf("timeout waiting for DNS entry %q to be reconciled", entry.Name) + } + time.Sleep(1 * time.Second) + } + } + + return nil +} + func (a *actuator) isManagingDNSProviders(dns *gardencorev1beta1.DNS) bool { return a.Config().ManageDNSProviders && dns != nil && dns.Domain != nil } @@ -312,7 +392,7 @@ func (a *actuator) createOrUpdateSeedResources(ctx context.Context, dnsconfig *a if !deploymentEnabled || a.isHibernated(cluster) { replicas = 0 } - shootActive := !common.IsMigrating(ex) + shootActive := !common.IsMigrating(ex) && !common.IsRestoringStep1(ex) chartValues := map[string]interface{}{ "serviceName": service.ServiceName, From 9c45decc1d4012fcf6facb2af8b2e4592517d101 Mon Sep 17 00:00:00 2001 From: Martin Weindel Date: Wed, 18 Dec 2024 08:51:05 +0100 Subject: [PATCH 2/3] fix NPE if extension has been deleted --- pkg/controller/replication/reconciler.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/controller/replication/reconciler.go b/pkg/controller/replication/reconciler.go index ab0cd83a..44cf963f 100644 --- a/pkg/controller/replication/reconciler.go +++ b/pkg/controller/replication/reconciler.go @@ -53,7 +53,7 @@ func (r *reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco if err != nil { return result, err } - if common.IsMigrating(ext) { + if ext == nil || common.IsMigrating(ext) { return result, nil } statehandler, err := common.NewStateHandler(ctx, r.Env, ext, false) From 2545dfc140c6545db0b4f37950cc02c584f6f436 Mon Sep 17 00:00:00 2001 From: Martin Weindel Date: Wed, 18 Dec 2024 10:30:24 +0100 Subject: [PATCH 3/3] drop indirection for gardener operator constants --- pkg/controller/common/utils.go | 23 +++++++++++++---------- pkg/controller/lifecycle/actuator.go | 10 ++++------ 2 files changed, 17 insertions(+), 16 deletions(-) diff --git a/pkg/controller/common/utils.go b/pkg/controller/common/utils.go index 5a4e240c..7e37c487 100644 --- a/pkg/controller/common/utils.go +++ b/pkg/controller/common/utils.go @@ -19,12 +19,8 @@ import ( "github.com/gardener/gardener-extension-shoot-dns-service/pkg/service" ) -const ( - ANNOTATION_OPERATION = constants.GardenerOperation - ANNOTATION_OPERATION_MIGRATE = constants.GardenerOperationMigrate - ANNOTATION_OPERATION_RESTORE = constants.GardenerOperationRestore - ANNOTATION_OPERATION_RESTORE_STEP1 = "restore/step1" -) +// gardenerOperationRestorePrepare is an internal value for the restore prepare operation +const gardenerOperationRestorePrepare = "restore/prepare" func CopyMap(m map[string]string) map[string]string { if m == nil { @@ -58,21 +54,28 @@ func IsMigrating(ex *extensionsv1alpha1.Extension) bool { if ex.Annotations == nil { return false } - return ex.Annotations[ANNOTATION_OPERATION] == ANNOTATION_OPERATION_MIGRATE + return ex.Annotations[constants.GardenerOperation] == constants.GardenerOperationMigrate } func IsRestoring(ex *extensionsv1alpha1.Extension) bool { if ex.Annotations == nil { return false } - return ex.Annotations[ANNOTATION_OPERATION] == ANNOTATION_OPERATION_RESTORE + return ex.Annotations[constants.GardenerOperation] == constants.GardenerOperationRestore } -func IsRestoringStep1(ex *extensionsv1alpha1.Extension) bool { +func IsPreparingRestore(ex *extensionsv1alpha1.Extension) bool { if ex.Annotations == nil { return false } - return ex.Annotations[ANNOTATION_OPERATION] == ANNOTATION_OPERATION_RESTORE_STEP1 + return ex.Annotations[constants.GardenerOperation] == gardenerOperationRestorePrepare +} + +func SetRestorePrepareAnnotation(ex *extensionsv1alpha1.Extension) { + if ex.Annotations == nil { + ex.Annotations = map[string]string{} + } + ex.Annotations[constants.GardenerOperation] = gardenerOperationRestorePrepare } // ShortenID shortens an identifier longer than maxlen characters by cutting the string diff --git a/pkg/controller/lifecycle/actuator.go b/pkg/controller/lifecycle/actuator.go index 027bfffd..9cdcb1a5 100644 --- a/pkg/controller/lifecycle/actuator.go +++ b/pkg/controller/lifecycle/actuator.go @@ -259,14 +259,12 @@ func (a *actuator) delete(ctx context.Context, log logr.Logger, ex *extensionsv1 // Restore the Extension resource. func (a *actuator) Restore(ctx context.Context, log logr.Logger, ex *extensionsv1alpha1.Extension) error { - // First run extension reconciliation with deactivated DNSOwner to avoid + // TODO(martinweindel): Drop this section once the DNS owner has been removed for all seeds on all landscapes. + // First, run extension reconciliation with deactivated DNSOwner to avoid // zone reconciliation before all entries are reconciled. // Premature zone reconciliation can lead to DNS entries being deleted temporarily. exCopy := ex.DeepCopy() - if exCopy.Annotations == nil { - exCopy.Annotations = map[string]string{} - } - exCopy.Annotations[common.ANNOTATION_OPERATION] = common.ANNOTATION_OPERATION_RESTORE_STEP1 + common.SetRestorePrepareAnnotation(exCopy) if err := a.Reconcile(ctx, log, exCopy); err != nil { return err } @@ -392,7 +390,7 @@ func (a *actuator) createOrUpdateSeedResources(ctx context.Context, dnsconfig *a if !deploymentEnabled || a.isHibernated(cluster) { replicas = 0 } - shootActive := !common.IsMigrating(ex) && !common.IsRestoringStep1(ex) + shootActive := !common.IsMigrating(ex) && !common.IsPreparingRestore(ex) chartValues := map[string]interface{}{ "serviceName": service.ServiceName,