Resolve review comments in Usage

Signed-off-by: Hasan Turken <turkenh@gmail.com>

apis/apiextensions/v1alpha1/usage_types.go

@@ -59,9 +59,11 @@ type Resource struct {
 // UsageSpec defines the desired state of Usage.
 type UsageSpec struct {
 	// Of is the resource that is "being used".
+	// +kubebuilder:validation:XValidation:rule="has(self.resourceRef) || has(self.resourceSelector)",message="either a resource reference or a resource selector should be set."
 	Of Resource `json:"of"`
 	// By is the resource that is "using the other resource".
 	// +optional
+	// +kubebuilder:validation:XValidation:rule="has(self.resourceRef) || has(self.resourceSelector)",message="either a resource reference or a resource selector should be set."
 	By *Resource `json:"by,omitempty"`
 	// Reason is the reason for blocking deletion of the resource.
 	// +optional

cluster/crds/apiextensions.crossplane.io_usages.yaml

@@ -2,7 +2,7 @@ apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
   annotations:
-    controller-gen.kubebuilder.io/version: v0.12.1
+    controller-gen.kubebuilder.io/version: v0.13.0
   name: usages.apiextensions.crossplane.io
 spec:
   group: apiextensions.crossplane.io
@@ -80,6 +80,10 @@ spec:
                         type: object
                     type: object
                 type: object
+                x-kubernetes-validations:
+                - message: either a resource reference or a resource selector should
+                    be set.
+                  rule: has(self.resourceRef) || has(self.resourceSelector)
               of:
                 description: Of is the resource that is "being used".
                 properties:
@@ -114,6 +118,10 @@ spec:
                         type: object
                     type: object
                 type: object
+                x-kubernetes-validations:
+                - message: either a resource reference or a resource selector should
+                    be set.
+                  rule: has(self.resourceRef) || has(self.resourceSelector)
               reason:
                 description: Reason is the reason for blocking deletion of the resource.
                 type: string

cmd/crossplane/core/core.go

@@ -275,8 +275,10 @@ func (c *startCommand) Run(s *runtime.Scheme, log logging.Logger) error { //noli
 		if err := composition.SetupWebhookWithManager(mgr, o); err != nil {
 			return errors.Wrap(err, "cannot setup webhook for compositions")
 		}
-		if err := usage.SetupWebhookWithManager(mgr, o); err != nil {
-			return errors.Wrap(err, "cannot setup webhook for usages")
+		if o.Features.Enabled(features.EnableAlphaUsages) {
+			if err := usage.SetupWebhookWithManager(mgr, o); err != nil {
+				return errors.Wrap(err, "cannot setup webhook for usages")
+			}
 		}
 	}
 

internal/controller/apiextensions/composite/api.go

@@ -214,7 +214,7 @@ func (r *CompositionSelectorChain) SelectComposition(ctx context.Context, cp res
 	return nil
 }
 
-// NewAPILabelSelectorResolver returns a selectorResolver for composite resource.
+// NewAPILabelSelectorResolver returns a SelectorResolver for composite resource.
 func NewAPILabelSelectorResolver(c client.Client) *APILabelSelectorResolver {
 	return &APILabelSelectorResolver{client: c}
 }

internal/controller/apiextensions/usage/reconciler.go

@@ -47,8 +47,9 @@ import (
 )
 
 const (
-	timeout   = 2 * time.Minute
-	finalizer = "usage.apiextensions.crossplane.io"
+	reconcileTimeout = 1 * time.Minute
+	waitPollInterval = 30 * time.Second
+	finalizer        = "usage.apiextensions.crossplane.io"
 	// Note(turkenh): In-use label enables the "DELETE" requests on resources
 	// with this label to be intercepted by the webhook and rejected if the
 	// resource is in use.
@@ -96,7 +97,8 @@ func Setup(mgr ctrl.Manager, o apiextensionscontroller.Options) error {
 	name := "usage/" + strings.ToLower(v1alpha1.UsageGroupKind)
 	r := NewReconciler(mgr,
 		WithLogger(o.Logger.WithValues("controller", name)),
-		WithRecorder(event.NewAPIRecorder(mgr.GetEventRecorderFor(name))))
+		WithRecorder(event.NewAPIRecorder(mgr.GetEventRecorderFor(name))),
+		WithPollInterval(o.PollInterval))
 
 	return ctrl.NewControllerManagedBy(mgr).
 		Named(name).
@@ -146,6 +148,17 @@ func WithSelectorResolver(sr selectorResolver) ReconcilerOption {
 	}
 }
 
+// WithPollInterval specifies how long the Reconciler should wait before queueing
+// a new reconciliation after a successful reconcile. The Reconciler requeues
+// after a specified duration when it is not actively waiting for an external
+// operation, but wishes to check whether resources it does not have a watch on
+// (i.e. used/using resources) need to be reconciled.
+func WithPollInterval(after time.Duration) ReconcilerOption {
+	return func(r *Reconciler) {
+		r.pollInterval = after
+	}
+}
+
 type usageResource struct {
 	xpresource.Finalizer
 	selectorResolver
@@ -168,8 +181,6 @@ func NewReconciler(mgr manager.Manager, opts ...ReconcilerOption) *Reconciler {
 
 		log:    logging.NewNopLogger(),
 		record: event.NewNopRecorder(),
-
-		pollInterval: 30 * time.Second,
 	}
 
 	for _, f := range opts {
@@ -194,7 +205,7 @@ type Reconciler struct {
 // relationship, adding a finalizer and handling proper deletion.
 func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reconcile.Result, error) { //nolint:gocyclo // Reconcilers are typically complex.
 	log := r.log.WithValues("request", req)
-	ctx, cancel := context.WithTimeout(ctx, timeout)
+	ctx, cancel := context.WithTimeout(ctx, reconcileTimeout)
 	defer cancel()
 
 	// Get the usageResource resource for this request.
@@ -242,12 +253,18 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco
 
 			if err == nil {
 				// Using resource is still there, so we need to wait for it to be deleted.
-				msg := "Waiting for using resource to be deleted."
+				msg := fmt.Sprintf("Waiting for the using resource (which is a %q named %q) to be deleted.", by.Kind, by.ResourceRef.Name)
 				log.Debug(msg)
 				r.record.Event(u, event.Normal(reasonWaitUsing, msg))
-				return reconcile.Result{RequeueAfter: 30 * time.Second}, nil
+				// We are using a waitPollInterval which is shorter than the
+				// pollInterval to make sure we delete the usage as soon as
+				// possible after the using resource is deleted. This is
+				// to add minimal delay to the overall deletion process which is
+				// usually extended by backoff intervals.
+				return reconcile.Result{RequeueAfter: waitPollInterval}, nil
 			}
 		}
+
 		// At this point using resource is either:
 		// - not defined
 		// - not found (e.g. deleted)
@@ -371,7 +388,10 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco
 
 	u.Status.SetConditions(xpv1.Available())
 	r.record.Event(u, event.Normal(reasonUsageConfigured, "Usage configured successfully."))
-	return reconcile.Result{}, errors.Wrap(r.client.Status().Update(ctx, u), errUpdateStatus)
+	// We are only watching the Usage itself but not using or used resources.
+	// So, we need to reconcile the Usage periodically to check if the using
+	// or used resources are still there.
+	return reconcile.Result{RequeueAfter: r.pollInterval}, errors.Wrap(r.client.Status().Update(ctx, u), errUpdateStatus)
 }
 
 func detailsAnnotation(u *v1alpha1.Usage) string {

internal/controller/apiextensions/usage/selector.go

@@ -33,11 +33,10 @@ const (
 	errUpdateAfterResolveSelector            = "cannot update usage after resolving selector"
 	errResolveSelectorForUsingResource       = "cannot resolve selector at \"spec.by.resourceSelector\""
 	errResolveSelectorForUsedResource        = "cannot resolve selector at \"spec.of.resourceSelector\""
+	errNoSelectorToResolve                   = "no selector defined for resolving"
 	errListResourceMatchingLabels            = "cannot list resources matching labels"
 	errFmtResourcesNotFound                  = "no %q found matching labels: %q"
 	errFmtResourcesNotFoundWithControllerRef = "no %q found matching labels: %q and with same controller reference"
-	errIdentifyUsedResource                  = "cannot identify used resource, neither \"spec.of.resourceRef\" nor \"spec.of.resourceSelector\" is set"
-	errIdentifyUsingResource                 = "cannot identify using resource, neither \"spec.by.resourceRef\" nor \"spec.by.resourceSelector\" is set"
 )
 
 type apiSelectorResolver struct {
@@ -48,14 +47,11 @@ func newAPISelectorResolver(c client.Client) *apiSelectorResolver {
 	return &apiSelectorResolver{client: c}
 }
 
-func (r *apiSelectorResolver) resolveSelectors(ctx context.Context, u *v1alpha1.Usage) error { //nolint:gocyclo // we need to resolve both selectors so no real complexity rather a duplication
+func (r *apiSelectorResolver) resolveSelectors(ctx context.Context, u *v1alpha1.Usage) error {
 	of := u.Spec.Of
 	by := u.Spec.By
 
 	if of.ResourceRef == nil || len(of.ResourceRef.Name) == 0 {
-		if of.ResourceSelector == nil {
-			return errors.New(errIdentifyUsedResource)
-		}
 		if err := r.resolveSelector(ctx, u, &of); err != nil {
 			return errors.Wrap(err, errResolveSelectorForUsedResource)
 		}
@@ -70,9 +66,6 @@ func (r *apiSelectorResolver) resolveSelectors(ctx context.Context, u *v1alpha1.
 	}
 
 	if by.ResourceRef == nil || len(by.ResourceRef.Name) == 0 {
-		if by.ResourceSelector == nil {
-			return errors.New(errIdentifyUsingResource)
-		}
 		if err := r.resolveSelector(ctx, u, by); err != nil {
 			return errors.Wrap(err, errResolveSelectorForUsingResource)
 		}
@@ -91,6 +84,9 @@ func (r *apiSelectorResolver) resolveSelector(ctx context.Context, u *v1alpha1.U
 		Kind:       rs.Kind,
 	}))
 
+	if rs.ResourceSelector == nil {
+		return errors.New(errNoSelectorToResolve)
+	}
 	if err := r.client.List(ctx, l, client.MatchingLabels(rs.ResourceSelector.MatchLabels)); err != nil {
 		return errors.Wrap(err, errListResourceMatchingLabels)
 	}

internal/controller/apiextensions/usage/selector_test.go

@@ -337,7 +337,7 @@ func TestResolveSelectors(t *testing.T) {
 		},
 
 		"CannotResolveNoMatchingResourcesWithControllerRef": {
-			reason: "If selectors defined for both \"of\" and \"by\", both should be resolved.",
+			reason: "We should return error if there are no matching resources with controller ref.",
 			args: args{
 				client: &test.MockClient{
 					MockList: func(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error {
@@ -387,10 +387,16 @@ func TestResolveSelectors(t *testing.T) {
 			reason: "If selectors defined for both \"of\" and \"by\", both should be resolved.",
 			args: args{
 				client: &test.MockClient{
-					MockList: test.NewMockListFn(nil, func(list client.ObjectList) error {
+					MockList: func(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error {
 						l := list.(*composed.UnstructuredList)
+						if v := l.GroupVersionKind().Version; v != "v1" {
+							t.Errorf("unexpected list version: %s", v)
+						}
 						switch l.GetKind() {
 						case "SomeKindList":
+							if len(opts) != 1 && opts[0].(client.MatchingLabels)["foo"] != "bar" {
+								t.Errorf("unexpected list options: %v", opts)
+							}
 							l.Items = []unstructured.Unstructured{
 								{
 									Object: map[string]interface{}{
@@ -412,6 +418,9 @@ func TestResolveSelectors(t *testing.T) {
 								},
 							}
 						case "AnotherKindList":
+							if len(opts) != 1 && opts[0].(client.MatchingLabels)["baz"] != "qux" {
+								t.Errorf("unexpected list options: %v", opts)
+							}
 							l.Items = []unstructured.Unstructured{
 								{
 									Object: map[string]interface{}{
@@ -427,7 +436,7 @@ func TestResolveSelectors(t *testing.T) {
 							t.Errorf("unexpected list kind: %s", l.GetKind())
 						}
 						return nil
-					}),
+					},
 					MockUpdate: func(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error {
 						return nil
 					},

internal/initializer/webhook_configurations.go

@@ -111,8 +111,8 @@ func (c *WebhookConfigurations) Run(ctx context.Context, kube client.Client) err
 				conf.Webhooks[i].ClientConfig.Service.Namespace = c.ServiceReference.Namespace
 				conf.Webhooks[i].ClientConfig.Service.Port = c.ServiceReference.Port
 			}
-			// Note(turkenh): We have webhook configurations other than the
-			// ones defined with kubebuilder/controller-tools, and we
+			// Note(turkenh): We have validating webhook configurations other
+			// than the ones defined with kubebuilder/controller-tools, and we
 			// name them as we want. So, we need to apply workaround for the
 			// linked issue below only for the one generated by controller-tools.
 			if conf.GetName() == "validating-webhook-configuration" {
@@ -126,14 +126,8 @@ func (c *WebhookConfigurations) Run(ctx context.Context, kube client.Client) err
 				conf.Webhooks[i].ClientConfig.Service.Namespace = c.ServiceReference.Namespace
 				conf.Webhooks[i].ClientConfig.Service.Port = c.ServiceReference.Port
 			}
-			// Note(turkenh): We have webhook configurations other than the
-			// ones defined with kubebuilder/controller-tools, and we
-			// name them as we want. So, we need to apply workaround for the
-			// linked issue below only for the one generated by controller-tools.
-			if conf.GetName() == "mutating-webhook-configuration" {
-				// See https://github.com/kubernetes-sigs/controller-tools/issues/658
-				conf.SetName("crossplane")
-			}
+			// See https://github.com/kubernetes-sigs/controller-tools/issues/658
+			conf.SetName("crossplane")
 		default:
 			return errors.Errorf("only MutatingWebhookConfiguration and ValidatingWebhookConfiguration kinds are accepted, got %T", obj)
 		}

internal/usage/handler.go

@@ -36,7 +36,6 @@ import (
 	xpunstructured "github.com/crossplane/crossplane-runtime/pkg/resource/unstructured"
 
 	"github.com/crossplane/crossplane/apis/apiextensions/v1alpha1"
-	"github.com/crossplane/crossplane/internal/features"
 )
 
 const (
@@ -50,21 +49,22 @@ const (
 
 // IndexValueForObject returns the index value for the given object.
 func IndexValueForObject(u *unstructured.Unstructured) string {
-	return fmt.Sprintf("%s.%s.%s", u.GetAPIVersion(), u.GetKind(), u.GetName())
+	return indexValue(u.GetAPIVersion(), u.GetKind(), u.GetName())
+}
+
+func indexValue(apiVersion, kind, name string) string {
+	return fmt.Sprintf("%s.%s.%s", apiVersion, kind, name)
 }
 
 // SetupWebhookWithManager sets up the webhook with the manager.
 func SetupWebhookWithManager(mgr ctrl.Manager, options controller.Options) error {
-	if !options.Features.Enabled(features.EnableAlphaUsages) {
-		return nil
-	}
 	indexer := mgr.GetFieldIndexer()
 	if err := indexer.IndexField(context.Background(), &v1alpha1.Usage{}, InUseIndexKey, func(obj client.Object) []string {
 		u := obj.(*v1alpha1.Usage)
 		if u.Spec.Of.ResourceRef == nil || len(u.Spec.Of.ResourceRef.Name) == 0 {
 			return []string{}
 		}
-		return []string{fmt.Sprintf("%s.%s.%s", u.Spec.Of.APIVersion, u.Spec.Of.Kind, u.Spec.Of.ResourceRef.Name)}
+		return []string{indexValue(u.Spec.Of.APIVersion, u.Spec.Of.Kind, u.Spec.Of.ResourceRef.Name)}
 	}); err != nil {
 		return err
 	}

test/e2e/funcs/feature.go

@@ -535,7 +535,7 @@ func ListedResourcesValidatedWithin(d time.Duration, list k8s.ObjectList, min in
 			return ctx
 		}
 
-		t.Logf("%d resource(s) have desired conditions", min)
+		t.Logf("at least %d resource(s) have desired conditions", min)
 		return ctx
 	}
 }
@@ -607,6 +607,7 @@ func DeletionBlockedByUsageWebhook(dir, pattern string) features.Func {
 			t.Fatal("expected the usage webhook to deny the request but deletion succeeded")
 			return ctx
 		}
+
 		if !strings.HasPrefix(err.Error(), "admission webhook \"nousages.apiextensions.crossplane.io\" denied the request") {
 			t.Fatalf("expected the usage webhook to deny the request but it failed with err: %s", err.Error())
 			return ctx