diff --git a/api/v1alpha1/azureasomanagedcluster_types.go b/api/v1alpha1/azureasomanagedcluster_types.go index b0a5f15139b..5d96edbcfff 100644 --- a/api/v1alpha1/azureasomanagedcluster_types.go +++ b/api/v1alpha1/azureasomanagedcluster_types.go @@ -79,11 +79,6 @@ type AzureASOManagedCluster struct { Status AzureASOManagedClusterStatus `json:"status,omitempty"` } -// GetResourceStatuses returns the status of resources. -func (a *AzureASOManagedCluster) GetResourceStatuses() []ResourceStatus { - return a.Status.Resources -} - // SetResourceStatuses sets the status of resources. func (a *AzureASOManagedCluster) SetResourceStatuses(r []ResourceStatus) { a.Status.Resources = r diff --git a/api/v1alpha1/azureasomanagedcontrolplane_types.go b/api/v1alpha1/azureasomanagedcontrolplane_types.go index 41d75a89fa0..7e3a7210bee 100644 --- a/api/v1alpha1/azureasomanagedcontrolplane_types.go +++ b/api/v1alpha1/azureasomanagedcontrolplane_types.go @@ -67,11 +67,6 @@ type AzureASOManagedControlPlane struct { Status AzureASOManagedControlPlaneStatus `json:"status,omitempty"` } -// GetResourceStatuses returns the status of resources. -func (a *AzureASOManagedControlPlane) GetResourceStatuses() []ResourceStatus { - return a.Status.Resources -} - // SetResourceStatuses sets the status of resources. func (a *AzureASOManagedControlPlane) SetResourceStatuses(r []ResourceStatus) { a.Status.Resources = r diff --git a/api/v1alpha1/azureasomanagedmachinepool_types.go b/api/v1alpha1/azureasomanagedmachinepool_types.go index ea3316e9cfc..a6bf8e83d8d 100644 --- a/api/v1alpha1/azureasomanagedmachinepool_types.go +++ b/api/v1alpha1/azureasomanagedmachinepool_types.go @@ -62,11 +62,6 @@ type AzureASOManagedMachinePool struct { Status AzureASOManagedMachinePoolStatus `json:"status,omitempty"` } -// GetResourceStatuses returns the status of resources. -func (a *AzureASOManagedMachinePool) GetResourceStatuses() []ResourceStatus { - return a.Status.Resources -} - // SetResourceStatuses sets the status of resources. func (a *AzureASOManagedMachinePool) SetResourceStatuses(r []ResourceStatus) { a.Status.Resources = r diff --git a/controllers/azureasomanagedcluster_controller.go b/controllers/azureasomanagedcluster_controller.go index caa41645bd9..e7eb41f7807 100644 --- a/controllers/azureasomanagedcluster_controller.go +++ b/controllers/azureasomanagedcluster_controller.go @@ -300,19 +300,15 @@ func (r *AzureASOManagedClusterReconciler) reconcileDelete(ctx context.Context, defer done() log.V(4).Info("reconciling delete") - resources, err := mutators.ToUnstructured(ctx, asoManagedCluster.Spec.Resources) - if err != nil { - return ctrl.Result{}, err - } - resourceReconciler := r.newResourceReconciler(asoManagedCluster, resources) - err = resourceReconciler.Delete(ctx) + resourceReconciler := r.newResourceReconciler(asoManagedCluster, nil) + err := resourceReconciler.Delete(ctx) if err != nil { return ctrl.Result{}, fmt.Errorf("failed to reconcile resources: %w", err) } - if len(asoManagedCluster.Status.Resources) > 0 { - return ctrl.Result{}, nil + + if len(asoManagedCluster.Status.Resources) == 0 { + controllerutil.RemoveFinalizer(asoManagedCluster, clusterv1.ClusterFinalizer) } - controllerutil.RemoveFinalizer(asoManagedCluster, clusterv1.ClusterFinalizer) return ctrl.Result{}, nil } diff --git a/controllers/azureasomanagedcontrolplane_controller.go b/controllers/azureasomanagedcontrolplane_controller.go index 5063ae025d4..25b8843e303 100644 --- a/controllers/azureasomanagedcontrolplane_controller.go +++ b/controllers/azureasomanagedcontrolplane_controller.go @@ -379,20 +379,16 @@ func (r *AzureASOManagedControlPlaneReconciler) reconcileDelete(ctx context.Cont defer done() log.V(4).Info("reconciling delete") - resources, err := mutators.ToUnstructured(ctx, asoManagedControlPlane.Spec.Resources) - if err != nil { - return ctrl.Result{}, err - } - resourceReconciler := r.newResourceReconciler(asoManagedControlPlane, resources) - err = resourceReconciler.Delete(ctx) + resourceReconciler := r.newResourceReconciler(asoManagedControlPlane, nil) + err := resourceReconciler.Delete(ctx) if err != nil { return ctrl.Result{}, fmt.Errorf("failed to reconcile resources: %w", err) } - if len(asoManagedControlPlane.Status.Resources) > 0 { - return ctrl.Result{}, nil + + if len(asoManagedControlPlane.Status.Resources) == 0 { + controllerutil.RemoveFinalizer(asoManagedControlPlane, infrav1.AzureASOManagedControlPlaneFinalizer) } - controllerutil.RemoveFinalizer(asoManagedControlPlane, infrav1.AzureASOManagedControlPlaneFinalizer) return ctrl.Result{}, nil } diff --git a/controllers/azureasomanagedcontrolplane_controller_test.go b/controllers/azureasomanagedcontrolplane_controller_test.go index 013ef163f71..4d3e14a7118 100644 --- a/controllers/azureasomanagedcontrolplane_controller_test.go +++ b/controllers/azureasomanagedcontrolplane_controller_test.go @@ -435,7 +435,10 @@ func TestAzureASOManagedControlPlaneReconcile(t *testing.T) { Client: &FakeClient{ Client: c, patchFunc: func(_ context.Context, obj client.Object, _ client.Patch, _ ...client.PatchOption) error { - kubeconfigSecret := obj.(*corev1.Secret) + kubeconfigSecret, ok := obj.(*corev1.Secret) + if !ok { + return nil + } g.Expect(kubeconfigSecret.Data[secret.KubeconfigDataName]).NotTo(BeEmpty()) kubeConfigPatched = true @@ -571,7 +574,10 @@ func TestAzureASOManagedControlPlaneReconcile(t *testing.T) { Client: &FakeClient{ Client: c, patchFunc: func(_ context.Context, obj client.Object, _ client.Patch, _ ...client.PatchOption) error { - kubeconfigSecret := obj.(*corev1.Secret) + kubeconfigSecret, ok := obj.(*corev1.Secret) + if !ok { + return nil + } g.Expect(kubeconfigSecret.Data[secret.KubeconfigDataName]).NotTo(BeEmpty()) kubeConfigPatched = true diff --git a/controllers/azureasomanagedmachinepool_controller.go b/controllers/azureasomanagedmachinepool_controller.go index cca0bfb2358..87156d77d12 100644 --- a/controllers/azureasomanagedmachinepool_controller.go +++ b/controllers/azureasomanagedmachinepool_controller.go @@ -333,12 +333,8 @@ func (r *AzureASOManagedMachinePoolReconciler) reconcileDelete(ctx context.Conte // If the entire cluster is being deleted, this ASO ManagedClustersAgentPool will be deleted with the rest // of the ManagedCluster. if cluster.DeletionTimestamp.IsZero() { - resources, err := mutators.ToUnstructured(ctx, asoManagedMachinePool.Spec.Resources) - if err != nil { - return ctrl.Result{}, err - } - resourceReconciler := r.newResourceReconciler(asoManagedMachinePool, resources) - err = resourceReconciler.Delete(ctx) + resourceReconciler := r.newResourceReconciler(asoManagedMachinePool, nil) + err := resourceReconciler.Delete(ctx) if err != nil { return ctrl.Result{}, fmt.Errorf("failed to reconcile resources: %w", err) } diff --git a/controllers/resource_reconciler.go b/controllers/resource_reconciler.go index bd5e8ee0b16..d0b11002c02 100644 --- a/controllers/resource_reconciler.go +++ b/controllers/resource_reconciler.go @@ -20,13 +20,17 @@ import ( "context" "errors" "fmt" + "slices" + "strings" "github.com/Azure/azure-service-operator/v2/pkg/genruntime/conditions" "github.com/go-logr/logr" apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/klog/v2" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -39,6 +43,11 @@ import ( "sigs.k8s.io/cluster-api-provider-azure/util/tele" ) +const ( + ownedKindsAnnotation = "sigs.k8s.io/cluster-api-provider-azure-owned-aso-kinds" + ownedKindsSep = ";" +) + // ResourceReconciler reconciles a set of arbitrary ASO resources. type ResourceReconciler struct { client.Client @@ -53,7 +62,6 @@ type watcher interface { type resourceStatusObject interface { client.Object - GetResourceStatuses() []infrav1.ResourceStatus SetResourceStatuses([]infrav1.ResourceStatus) } @@ -62,69 +70,19 @@ func (r *ResourceReconciler) Reconcile(ctx context.Context) error { ctx, log, done := tele.StartSpanWithLogger(ctx, "controllers.ResourceReconciler.Reconcile") defer done() log.V(4).Info("reconciling resources") + return r.reconcile(ctx) +} - var newResourceStatuses []infrav1.ResourceStatus - - for _, spec := range r.resources { - gvk := spec.GroupVersionKind() - spec.SetNamespace(r.owner.GetNamespace()) - - log := log.WithValues("resource", klog.KObj(spec), "resourceVersion", gvk.GroupVersion(), "resourceKind", gvk.Kind) - - if err := controllerutil.SetControllerReference(r.owner, spec, r.Scheme()); err != nil { - return fmt.Errorf("failed to set owner reference: %w", err) - } - - if err := r.watcher.Watch(log, spec, handler.EnqueueRequestForOwner(r.Client.Scheme(), r.Client.RESTMapper(), r.owner)); err != nil { - return fmt.Errorf("failed to watch resource: %w", err) - } - - log.V(4).Info("applying resource") - err := r.Patch(ctx, spec, client.Apply, client.FieldOwner("capz-manager"), client.ForceOwnership) - if err != nil { - return fmt.Errorf("failed to apply resource: %w", err) - } - - ready, err := readyStatus(ctx, spec) - if err != nil { - return fmt.Errorf("failed to get ready status: %w", err) - } - newResourceStatuses = append(newResourceStatuses, infrav1.ResourceStatus{ - Resource: infrav1.StatusResource{ - Group: gvk.Group, - Version: gvk.Version, - Kind: gvk.Kind, - Name: spec.GetName(), - }, - Ready: ready, - }) - } - - for _, oldStatus := range r.owner.GetResourceStatuses() { - needsDelete := true - for _, newStatus := range newResourceStatuses { - if oldStatus.Resource.Group == newStatus.Resource.Group && - oldStatus.Resource.Kind == newStatus.Resource.Kind && - oldStatus.Resource.Name == newStatus.Resource.Name { - needsDelete = false - break - } - } - - if needsDelete { - updatedStatus, err := r.deleteResource(ctx, oldStatus.Resource) - if err != nil { - return err - } - if updatedStatus != nil { - newResourceStatuses = append(newResourceStatuses, *updatedStatus) - } - } - } - - r.owner.SetResourceStatuses(newResourceStatuses) +// Delete deletes the specified resources. +func (r *ResourceReconciler) Delete(ctx context.Context) error { + ctx, log, done := tele.StartSpanWithLogger(ctx, "controllers.ResourceReconciler.Delete") + defer done() + log.V(4).Info("deleting resources") - return nil + // Delete is a special case of a normal reconciliation which is equivalent to all resources from spec + // being deleted. + r.resources = nil + return r.reconcile(ctx) } // Pause pauses reconciliation of the specified resources. @@ -142,14 +100,11 @@ func (r *ResourceReconciler) Pause(ctx context.Context) error { } for _, spec := range r.resources { - gvk := spec.GroupVersionKind() spec.SetNamespace(r.owner.GetNamespace()) - - log := log.WithValues("resource", klog.KObj(spec), "resourceVersion", gvk.GroupVersion(), "resourceKind", gvk.Kind) - - log.V(4).Info("pausing resource") + gvk := spec.GroupVersionKind() + log.V(4).Info("pausing resource", "resource", klog.KObj(spec), "resourceVersion", gvk.GroupVersion(), "resourceKind", gvk.Kind) err := r.Patch(ctx, spec, client.Apply, client.FieldOwner("capz-manager")) - if err != nil { + if client.IgnoreNotFound(err) != nil { return fmt.Errorf("failed to patch resource: %w", err) } } @@ -157,42 +112,110 @@ func (r *ResourceReconciler) Pause(ctx context.Context) error { return nil } -// Delete deletes the specified resources. -func (r *ResourceReconciler) Delete(ctx context.Context) error { - ctx, log, done := tele.StartSpanWithLogger(ctx, "controllers.ResourceReconciler.Delete") +func (r *ResourceReconciler) reconcile(ctx context.Context) error { + ctx, log, done := tele.StartSpanWithLogger(ctx, "controllers.ResourceReconciler.reconcile") defer done() - log.V(4).Info("deleting resources") var newResourceStatuses []infrav1.ResourceStatus - for _, spec := range r.owner.GetResourceStatuses() { - newStatus, err := r.deleteResource(ctx, spec.Resource) + ownedKindsValue := r.owner.GetAnnotations()[ownedKindsAnnotation] + ownedKinds, err := parseOwnedKinds(ownedKindsValue) + if err != nil { + return fmt.Errorf("failed to parse %s annotation: %s", ownedKindsAnnotation, ownedKindsValue) + } + + ownedObjs, err := r.ownedObjs(ctx, ownedKinds) + if err != nil { + return fmt.Errorf("failed to get owned objects: %w", err) + } + + unrecordedTypeResources, recordedTypeResources, toBeDeletedResources := partitionResources(ownedKinds, r.resources, ownedObjs) + + // Newly-defined types in the CAPZ spec are first recorded in the annotation without performing a + // patch that would create resources of that type. CAPZ only patches resources whose kinds have + // already been recorded to ensure no resources are orphaned. + for _, spec := range unrecordedTypeResources { + newResourceStatuses = append(newResourceStatuses, infrav1.ResourceStatus{ + Resource: statusResource(spec), + Ready: false, + }) + } + + for _, spec := range recordedTypeResources { + spec.SetNamespace(r.owner.GetNamespace()) + + if err := controllerutil.SetControllerReference(r.owner, spec, r.Scheme()); err != nil { + return fmt.Errorf("failed to set owner reference: %w", err) + } + + toWatch := meta.AsPartialObjectMetadata(spec) + toWatch.APIVersion = spec.GetAPIVersion() + toWatch.Kind = spec.GetKind() + if err := r.watcher.Watch(log, toWatch, handler.EnqueueRequestForOwner(r.Client.Scheme(), r.Client.RESTMapper(), r.owner)); err != nil { + return fmt.Errorf("failed to watch resource: %w", err) + } + + gvk := spec.GroupVersionKind() + log.V(4).Info("applying resource", "resource", klog.KObj(spec), "resourceVersion", gvk.GroupVersion(), "resourceKind", gvk.Kind) + err := r.Patch(ctx, spec, client.Apply, client.FieldOwner("capz-manager"), client.ForceOwnership) + if err != nil { + return fmt.Errorf("failed to apply resource: %w", err) + } + + ready, err := readyStatus(ctx, spec) if err != nil { - return err + return fmt.Errorf("failed to get ready status: %w", err) + } + newResourceStatuses = append(newResourceStatuses, infrav1.ResourceStatus{ + Resource: statusResource(spec), + Ready: ready, + }) + } + + for _, obj := range toBeDeletedResources { + newStatus, err := r.deleteResource(ctx, obj) + if err != nil { + return fmt.Errorf("failed to delete %s %s/%s", obj.GroupVersionKind(), obj.Namespace, obj.Name) } if newStatus != nil { newResourceStatuses = append(newResourceStatuses, *newStatus) } } + newOwnedKinds := []schema.GroupVersionKind{} + for _, status := range newResourceStatuses { + gvk := schema.GroupVersionKind{ + Group: status.Resource.Group, + Version: status.Resource.Version, + Kind: status.Resource.Kind, + } + if !slices.Contains(newOwnedKinds, gvk) { + newOwnedKinds = append(newOwnedKinds, gvk) + } + } + annotations := r.owner.GetAnnotations() + if annotations == nil { + annotations = make(map[string]string) + } + annotations[ownedKindsAnnotation] = getOwnedKindsValue(newOwnedKinds) + if annotations[ownedKindsAnnotation] == "" { + delete(annotations, ownedKindsAnnotation) + } + r.owner.SetAnnotations(annotations) + r.owner.SetResourceStatuses(newResourceStatuses) return nil } -func (r *ResourceReconciler) deleteResource(ctx context.Context, resource infrav1.StatusResource) (*infrav1.ResourceStatus, error) { +func (r *ResourceReconciler) deleteResource(ctx context.Context, resource *metav1.PartialObjectMetadata) (*infrav1.ResourceStatus, error) { ctx, log, done := tele.StartSpanWithLogger(ctx, "controllers.ResourceReconciler.deleteResource") defer done() - spec := &unstructured.Unstructured{} - spec.SetGroupVersionKind(schema.GroupVersionKind{Group: resource.Group, Version: resource.Version, Kind: resource.Kind}) - spec.SetNamespace(r.owner.GetNamespace()) - spec.SetName(resource.Name) - - log = log.WithValues("resource", klog.KObj(spec), "resourceVersion", spec.GroupVersionKind().GroupVersion(), "resourceKind", spec.GetKind()) + log = log.WithValues("resource", klog.KObj(resource), "resourceVersion", resource.APIVersion, "resourceKind", resource.Kind) log.V(4).Info("deleting resource") - err := r.Client.Delete(ctx, spec) + err := r.Client.Delete(ctx, resource) if apierrors.IsNotFound(err) { log.V(4).Info("resource has been deleted") return nil, nil @@ -201,7 +224,7 @@ func (r *ResourceReconciler) deleteResource(ctx context.Context, resource infrav return nil, fmt.Errorf("failed to delete resource: %w", err) } - err = r.Client.Get(ctx, client.ObjectKeyFromObject(spec), spec) + err = r.Client.Get(ctx, client.ObjectKeyFromObject(resource), resource) if apierrors.IsNotFound(err) { log.V(4).Info("resource has been deleted") return nil, nil @@ -209,17 +232,40 @@ func (r *ResourceReconciler) deleteResource(ctx context.Context, resource infrav if err != nil { return nil, fmt.Errorf("failed to get resource: %w", err) } - ready, err := readyStatus(ctx, spec) - if err != nil { - return nil, fmt.Errorf("failed to get ready status: %w", err) - } + gvk := resource.GroupVersionKind() return &infrav1.ResourceStatus{ - Resource: resource, - Ready: ready, + Resource: infrav1.StatusResource{ + Group: gvk.Group, + Version: gvk.Version, + Kind: gvk.Kind, + Name: resource.Name, + }, + Ready: false, }, nil } +func (r *ResourceReconciler) ownedObjs(ctx context.Context, ownedTypes sets.Set[metav1.TypeMeta]) ([]*metav1.PartialObjectMetadata, error) { + var ownedObjs []*metav1.PartialObjectMetadata + + for typeMeta := range ownedTypes { + objs := &metav1.PartialObjectMetadataList{TypeMeta: typeMeta} + err := r.List(ctx, objs, client.InNamespace(r.owner.GetNamespace())) + if err != nil { + return nil, fmt.Errorf("failed to list %s %s: %w", typeMeta.APIVersion, typeMeta.Kind, err) + } + + for _, obj := range objs.Items { + controller := metav1.GetControllerOfNoCopy(&obj) + if controller != nil && controller.UID == r.owner.GetUID() { + ownedObjs = append(ownedObjs, &obj) + } + } + } + + return ownedObjs, nil +} + func readyStatus(ctx context.Context, u *unstructured.Unstructured) (bool, error) { _, log, done := tele.StartSpanWithLogger(ctx, "controllers.ResourceReconciler.readyStatus") defer done() @@ -261,3 +307,82 @@ func readyStatus(ctx context.Context, u *unstructured.Unstructured) (bool, error // no ready condition is set return false, nil } + +// partitionResources splits the sets of resources in spec and the current set +// of owned, existing resources into three groups: +// - unrecordedTypeResources are of a type not yet known to this owning CAPZ resource. +// - recordedTypeResources are of a type already known to this owning CAPZ resource. +// - toBeDeletedResources exist but are not defined in spec. +func partitionResources( + ownedKinds sets.Set[metav1.TypeMeta], + specs []*unstructured.Unstructured, + ownedObjs []*metav1.PartialObjectMetadata, +) ( + unrecordedTypeResources []*unstructured.Unstructured, + recordedTypeResources []*unstructured.Unstructured, + toBeDeletedResources []*metav1.PartialObjectMetadata, +) { + for _, spec := range specs { + typeMeta := metav1.TypeMeta{ + APIVersion: spec.GetAPIVersion(), + Kind: spec.GetKind(), + } + if ownedKinds.Has(typeMeta) { + recordedTypeResources = append(recordedTypeResources, spec) + } else { + unrecordedTypeResources = append(unrecordedTypeResources, spec) + } + } + + for _, owned := range ownedObjs { + if !slices.ContainsFunc(specs, metadataRefersToResource(owned)) { + toBeDeletedResources = append(toBeDeletedResources, owned) + } + } + return +} + +func statusResource(resource *unstructured.Unstructured) infrav1.StatusResource { + gvk := resource.GroupVersionKind() + return infrav1.StatusResource{ + Group: gvk.Group, + Version: gvk.Version, + Kind: gvk.Kind, + Name: resource.GetName(), + } +} + +func metadataRefersToResource(metadata *metav1.PartialObjectMetadata) func(*unstructured.Unstructured) bool { + return func(u *unstructured.Unstructured) bool { + // Version is not a stable property of a particular resource. The API version of an ASO resource may + // change in the CAPZ spec from v1 to v2 but that still represents the same underlying resource. + return metadata.GroupVersionKind().GroupKind() == u.GroupVersionKind().GroupKind() && + metadata.Name == u.GetName() + } +} + +func parseOwnedKinds(value string) (sets.Set[metav1.TypeMeta], error) { + ownedKinds := sets.Set[metav1.TypeMeta]{} + if value == "" { + return nil, nil + } + for _, ownedKind := range strings.Split(value, ownedKindsSep) { + gvk, _ := schema.ParseKindArg(ownedKind) + if gvk == nil { + return nil, fmt.Errorf("invalid field %q: expected Kind.version.group", ownedKind) + } + ownedKinds.Insert(metav1.TypeMeta{ + APIVersion: gvk.GroupVersion().Identifier(), + Kind: gvk.Kind, + }) + } + return ownedKinds, nil +} + +func getOwnedKindsValue(ownedKinds []schema.GroupVersionKind) string { + fields := make([]string, 0, len(ownedKinds)) + for _, gvk := range ownedKinds { + fields = append(fields, strings.Join([]string{gvk.Kind, gvk.Version, gvk.Group}, ".")) + } + return strings.Join(fields, ownedKindsSep) +} diff --git a/controllers/resource_reconciler_test.go b/controllers/resource_reconciler_test.go index 1135ff5c6cf..94816bddca9 100644 --- a/controllers/resource_reconciler_test.go +++ b/controllers/resource_reconciler_test.go @@ -29,7 +29,9 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/apiutil" fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/predicate" @@ -87,11 +89,11 @@ func TestResourceReconcilerReconcile(t *testing.T) { resources: []*unstructured.Unstructured{}, owner: &infrav1.AzureASOManagedCluster{}, } - - g.Expect(r.Reconcile(ctx)).To(Succeed()) + err := r.Reconcile(ctx) + g.Expect(err).NotTo(HaveOccurred()) }) - t.Run("reconcile several resources", func(t *testing.T) { + t.Run("acknowledge new types", func(t *testing.T) { g := NewGomegaWithT(t) w := &FakeWatcher{} @@ -100,6 +102,61 @@ func TestResourceReconcilerReconcile(t *testing.T) { asoManagedCluster := &infrav1.AzureASOManagedCluster{} + unpatchedRGs := map[string]struct{}{} + r := &ResourceReconciler{ + Client: &FakeClient{ + Client: c, + patchFunc: func(ctx context.Context, o client.Object, p client.Patch, po ...client.PatchOption) error { + g.Expect(unpatchedRGs).To(HaveKey(o.GetName())) + delete(unpatchedRGs, o.GetName()) + return nil + }, + }, + resources: []*unstructured.Unstructured{ + rgJSON(g, s, &asoresourcesv1.ResourceGroup{ + ObjectMeta: metav1.ObjectMeta{ + Name: "rg1", + }, + }), + rgJSON(g, s, &asoresourcesv1.ResourceGroup{ + ObjectMeta: metav1.ObjectMeta{ + Name: "rg2", + }, + }), + }, + owner: asoManagedCluster, + watcher: w, + } + + err := r.Reconcile(ctx) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(w.watching).To(BeEmpty()) + g.Expect(unpatchedRGs).To(BeEmpty()) // all expected resources were patched + g.Expect(asoManagedCluster.Annotations).To(HaveKeyWithValue(ownedKindsAnnotation, getOwnedKindsValue([]schema.GroupVersionKind{asoresourcesv1.GroupVersion.WithKind("ResourceGroup")}))) + + resourcesStatuses := asoManagedCluster.Status.Resources + g.Expect(resourcesStatuses).To(HaveLen(2)) + g.Expect(resourcesStatuses[0].Resource.Name).To(Equal("rg1")) + g.Expect(resourcesStatuses[0].Ready).To(BeFalse()) + g.Expect(resourcesStatuses[1].Resource.Name).To(Equal("rg2")) + g.Expect(resourcesStatuses[1].Ready).To(BeFalse()) + }) + + t.Run("create resources with acknowledged types", func(t *testing.T) { + g := NewGomegaWithT(t) + + asoManagedCluster := &infrav1.AzureASOManagedCluster{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + ownedKindsAnnotation: getOwnedKindsValue([]schema.GroupVersionKind{asoresourcesv1.GroupVersion.WithKind("ResourceGroup")}), + }, + }, + } + + w := &FakeWatcher{} + c := fakeClientBuilder(). + Build() + unpatchedRGs := map[string]struct{}{ "rg1": {}, "rg2": {}, @@ -132,15 +189,25 @@ func TestResourceReconcilerReconcile(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "rg2", }, + Status: asoresourcesv1.ResourceGroup_STATUS{ + Conditions: []conditions.Condition{ + { + Type: conditions.ConditionTypeReady, + Status: metav1.ConditionFalse, + }, + }, + }, }), }, owner: asoManagedCluster, watcher: w, } - g.Expect(r.Reconcile(ctx)).To(Succeed()) + err := r.Reconcile(ctx) + g.Expect(err).NotTo(HaveOccurred()) g.Expect(w.watching).To(HaveKey("ResourceGroup.resources.azure.com")) g.Expect(unpatchedRGs).To(BeEmpty()) // all expected resources were patched + g.Expect(asoManagedCluster.Annotations).To(HaveKeyWithValue(ownedKindsAnnotation, getOwnedKindsValue([]schema.GroupVersionKind{asoresourcesv1.GroupVersion.WithKind("ResourceGroup")}))) resourcesStatuses := asoManagedCluster.Status.Resources g.Expect(resourcesStatuses).To(HaveLen(2)) @@ -154,40 +221,51 @@ func TestResourceReconcilerReconcile(t *testing.T) { g := NewGomegaWithT(t) owner := &infrav1.AzureASOManagedCluster{ - Status: infrav1.AzureASOManagedClusterStatus{ - Resources: []infrav1.ResourceStatus{ - rgStatus("rg0"), - rgStatus("rg1"), - rgStatus("rg2"), - rgStatus("rg3"), + ObjectMeta: metav1.ObjectMeta{ + Namespace: namespace, + Annotations: map[string]string{ + ownedKindsAnnotation: getOwnedKindsValue([]schema.GroupVersionKind{asoresourcesv1.GroupVersion.WithKind("ResourceGroup")}), }, }, } + ownerGVK, err := apiutil.GVKForObject(owner, s) + g.Expect(err).NotTo(HaveOccurred()) + controlledByOwner := []metav1.OwnerReference{ + *metav1.NewControllerRef(owner, ownerGVK), + } objs := []client.Object{ &asoresourcesv1.ResourceGroup{ + TypeMeta: rgTypeMeta, ObjectMeta: metav1.ObjectMeta{ - Name: "rg0", - Namespace: owner.Namespace, + Name: "rg0", + Namespace: owner.Namespace, + OwnerReferences: controlledBy(g, s, owner), }, }, &asoresourcesv1.ResourceGroup{ + TypeMeta: rgTypeMeta, ObjectMeta: metav1.ObjectMeta{ - Name: "rg1", - Namespace: owner.Namespace, + Name: "rg1", + Namespace: owner.Namespace, + OwnerReferences: controlledBy(g, s, owner), }, }, &asoresourcesv1.ResourceGroup{ + TypeMeta: rgTypeMeta, ObjectMeta: metav1.ObjectMeta{ - Name: "rg2", - Namespace: owner.Namespace, + Name: "rg2", + Namespace: owner.Namespace, + OwnerReferences: controlledBy(g, s, owner), }, }, &asoresourcesv1.ResourceGroup{ + TypeMeta: rgTypeMeta, ObjectMeta: metav1.ObjectMeta{ - Name: "rg3", - Namespace: owner.Namespace, - Finalizers: []string{"still deleting"}, + Name: "rg3", + Namespace: owner.Namespace, + OwnerReferences: controlledByOwner, + Finalizers: []string{"still deleting"}, }, }, } @@ -219,7 +297,9 @@ func TestResourceReconcilerReconcile(t *testing.T) { watcher: &FakeWatcher{}, } - g.Expect(r.Reconcile(ctx)).To(Succeed()) + err = r.Reconcile(ctx) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(owner.Annotations).To(HaveKeyWithValue(ownedKindsAnnotation, getOwnedKindsValue([]schema.GroupVersionKind{asoresourcesv1.GroupVersion.WithKind("ResourceGroup")}))) resourcesStatuses := owner.Status.Resources g.Expect(resourcesStatuses).To(HaveLen(3)) @@ -227,6 +307,20 @@ func TestResourceReconcilerReconcile(t *testing.T) { g.Expect(resourcesStatuses[0].Resource.Name).To(Equal("rg1")) g.Expect(resourcesStatuses[1].Resource.Name).To(Equal("rg2")) g.Expect(resourcesStatuses[2].Resource.Name).To(Equal("rg3")) + + err = r.Get(ctx, client.ObjectKey{Namespace: namespace, Name: "rg0"}, &asoresourcesv1.ResourceGroup{}) + g.Expect(apierrors.IsNotFound(err)).To(BeTrue(), "err is not a NotFound error") + + err = r.Get(ctx, client.ObjectKey{Namespace: namespace, Name: "rg1"}, &asoresourcesv1.ResourceGroup{}) + g.Expect(err).NotTo(HaveOccurred()) + + err = r.Get(ctx, client.ObjectKey{Namespace: namespace, Name: "rg2"}, &asoresourcesv1.ResourceGroup{}) + g.Expect(err).NotTo(HaveOccurred()) + + rg3 := &asoresourcesv1.ResourceGroup{} + err = r.Get(ctx, client.ObjectKey{Namespace: namespace, Name: "rg3"}, rg3) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(rg3.GetDeletionTimestamp().IsZero()).To(BeFalse(), "resource does not have expected deletion timestamp") }) } @@ -260,17 +354,57 @@ func TestResourceReconcilerPause(t *testing.T) { t.Run("pause several resources", func(t *testing.T) { g := NewGomegaWithT(t) + owner := &infrav1.AzureASOManagedCluster{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: namespace, + Annotations: map[string]string{ + ownedKindsAnnotation: getOwnedKindsValue([]schema.GroupVersionKind{asoresourcesv1.GroupVersion.WithKind("ResourceGroup")}), + }, + }, + } + + objs := []client.Object{ + &asoresourcesv1.ResourceGroup{ + TypeMeta: rgTypeMeta, + ObjectMeta: metav1.ObjectMeta{ + Name: "rg1", + Namespace: owner.Namespace, + OwnerReferences: controlledBy(g, s, owner), + }, + }, + &asoresourcesv1.ResourceGroup{ + TypeMeta: rgTypeMeta, + ObjectMeta: metav1.ObjectMeta{ + Name: "rg2", + Namespace: owner.Namespace, + OwnerReferences: controlledBy(g, s, owner), + }, + }, + &asoresourcesv1.ResourceGroup{ + TypeMeta: rgTypeMeta, + ObjectMeta: metav1.ObjectMeta{ + Name: "deleted from spec", + Namespace: owner.Namespace, + OwnerReferences: controlledBy(g, s, owner), + Finalizers: []string{"still deleting"}, + }, + }, + } + c := fakeClientBuilder(). + WithObjects(objs...). Build() - asoManagedCluster := &infrav1.AzureASOManagedCluster{} - var patchedRGs []string r := &ResourceReconciler{ Client: &FakeClient{ Client: c, patchFunc: func(ctx context.Context, o client.Object, p client.Patch, po ...client.PatchOption) error { g.Expect(o.GetAnnotations()).To(HaveKeyWithValue(annotations.ReconcilePolicy, string(annotations.ReconcilePolicySkip))) + if err := c.Get(ctx, client.ObjectKeyFromObject(o), &asoresourcesv1.ResourceGroup{}); err != nil { + // propagate errors like "NotFound" + return err + } patchedRGs = append(patchedRGs, o.GetName()) return nil }, @@ -286,8 +420,13 @@ func TestResourceReconcilerPause(t *testing.T) { Name: "rg2", }, }), + rgJSON(g, s, &asoresourcesv1.ResourceGroup{ + ObjectMeta: metav1.ObjectMeta{ + Name: "not-yet-created", + }, + }), }, - owner: asoManagedCluster, + owner: owner, } g.Expect(r.Pause(ctx)).To(Succeed()) @@ -328,25 +467,32 @@ func TestResourceReconcilerDelete(t *testing.T) { owner := &infrav1.AzureASOManagedCluster{ ObjectMeta: metav1.ObjectMeta{ Namespace: "ns", - }, - Status: infrav1.AzureASOManagedClusterStatus{ - Resources: []infrav1.ResourceStatus{ - rgStatus("still-deleting"), - rgStatus("already-gone"), + Annotations: map[string]string{ + ownedKindsAnnotation: getOwnedKindsValue([]schema.GroupVersionKind{asoresourcesv1.GroupVersion.WithKind("ResourceGroup")}), }, }, } objs := []client.Object{ &asoresourcesv1.ResourceGroup{ + TypeMeta: rgTypeMeta, ObjectMeta: metav1.ObjectMeta{ - Name: "still-deleting", - Namespace: owner.Namespace, + Name: "still-deleting", + Namespace: owner.Namespace, + OwnerReferences: controlledBy(g, s, owner), Finalizers: []string{ "ASO finalizer", }, }, }, + &asoresourcesv1.ResourceGroup{ + TypeMeta: rgTypeMeta, + ObjectMeta: metav1.ObjectMeta{ + Name: "just-deleted", + Namespace: owner.Namespace, + OwnerReferences: controlledBy(g, s, owner), + }, + }, } c := fakeClientBuilder(). @@ -361,7 +507,8 @@ func TestResourceReconcilerDelete(t *testing.T) { } g.Expect(r.Delete(ctx)).To(Succeed()) - g.Expect(apierrors.IsNotFound(r.Client.Get(ctx, client.ObjectKey{Namespace: owner.Namespace, Name: "already-gone"}, &asoresourcesv1.ResourceGroup{}))).To(BeTrue()) + g.Expect(owner.Annotations).To(HaveKeyWithValue(ownedKindsAnnotation, getOwnedKindsValue([]schema.GroupVersionKind{asoresourcesv1.GroupVersion.WithKind("ResourceGroup")}))) + g.Expect(apierrors.IsNotFound(r.Client.Get(ctx, client.ObjectKey{Namespace: owner.Namespace, Name: "just-deleted"}, &asoresourcesv1.ResourceGroup{}))).To(BeTrue()) stillDeleting := &asoresourcesv1.ResourceGroup{} g.Expect(r.Client.Get(ctx, client.ObjectKey{Namespace: owner.Namespace, Name: "still-deleting"}, stillDeleting)).To(Succeed()) g.Expect(stillDeleting.GetDeletionTimestamp().IsZero()).To(BeFalse()) @@ -370,6 +517,34 @@ func TestResourceReconcilerDelete(t *testing.T) { g.Expect(owner.Status.Resources[0].Resource.Name).To(Equal("still-deleting")) g.Expect(owner.Status.Resources[0].Ready).To(BeFalse()) }) + + t.Run("done deleting", func(t *testing.T) { + g := NewGomegaWithT(t) + + owner := &infrav1.AzureASOManagedCluster{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "ns", + Annotations: map[string]string{ + ownedKindsAnnotation: getOwnedKindsValue([]schema.GroupVersionKind{asoresourcesv1.GroupVersion.WithKind("ResourceGroup")}), + }, + }, + } + + c := fakeClientBuilder(). + Build() + + r := &ResourceReconciler{ + Client: &FakeClient{ + Client: c, + }, + owner: owner, + } + + g.Expect(r.Delete(ctx)).To(Succeed()) + + g.Expect(owner.Annotations).NotTo(HaveKey(ownedKindsAnnotation)) + g.Expect(owner.Status.Resources).To(BeEmpty()) + }) } func TestReadyStatus(t *testing.T) { @@ -575,20 +750,22 @@ func TestReadyStatus(t *testing.T) { }) } +func controlledBy(g *GomegaWithT, s *runtime.Scheme, owner client.Object) []metav1.OwnerReference { + ownerGVK, err := apiutil.GVKForObject(owner, s) + g.Expect(err).NotTo(HaveOccurred()) + return []metav1.OwnerReference{ + *metav1.NewControllerRef(owner, ownerGVK), + } +} + +var rgTypeMeta = metav1.TypeMeta{ + APIVersion: asoresourcesv1.GroupVersion.Identifier(), + Kind: "ResourceGroup", +} + func rgJSON(g Gomega, scheme *runtime.Scheme, rg *asoresourcesv1.ResourceGroup) *unstructured.Unstructured { rg.SetGroupVersionKind(asoresourcesv1.GroupVersion.WithKind("ResourceGroup")) u := &unstructured.Unstructured{} g.Expect(scheme.Convert(rg, u, nil)).To(Succeed()) return u } - -func rgStatus(name string) infrav1.ResourceStatus { - return infrav1.ResourceStatus{ - Resource: infrav1.StatusResource{ - Group: asoresourcesv1.GroupVersion.Group, - Version: asoresourcesv1.GroupVersion.Version, - Kind: "ResourceGroup", - Name: name, - }, - } -}