Addressed more reviewer comments

* limited collection name to 63 chars max, allowing us to that name as the memebership label, without need to handle names longer than 63 chars * remove `.status.objectRefs` fields * addressed nits Signed-off-by: Predrag Knezevic <[email protected]>
crossplane-contrib · Apr 3, 2024 · a13774a · a13774a
1 parent 78bf7cb
commit a13774a
Show file tree

Hide file tree

Showing 5 changed files with 40 additions and 99 deletions.
diff --git a/apis/observedobjectcollection/v1alpha1/types.go b/apis/observedobjectcollection/v1alpha1/types.go
@@ -33,6 +33,7 @@ import (
 // +kubebuilder:printcolumn:name="READY",type="string",JSONPath=".status.conditions[?(@.type=='Ready')].status"
 // +kubebuilder:printcolumn:name="AGE",type="date",JSONPath=".metadata.creationTimestamp"
 // +kubebuilder:resource:scope=Cluster,categories={crossplane,managed,kubernetes}
+// +kubebuilder:validation:XValidation:rule="size(self.metadata.name) < 64",message="metadata.name max length is 63"
 type ObservedObjectCollection struct {
 	v1.TypeMeta   `json:",inline"`
 	v1.ObjectMeta `json:"metadata,omitempty"`
@@ -109,14 +110,9 @@ type ObservedObjectTemplateMetadata struct {
 type ObservedObjectCollectionStatus struct {
 	v12.ResourceStatus `json:",inline"`
 
-	// List of object references mathing the given selector
-	// +optional
-	// +listType=map
-	// +listMapKey=name
-	ObjectRefs []ObservedObjectReference `json:"objectRefs,omitempty"`
-
 	// MembershipLabel is the label set on each member of this collection
 	// and can be used for fetching them.
+	// +optional
 	MembershipLabel map[string]string `json:"membershipLabel,omitempty"`
 }
 

diff --git a/apis/observedobjectcollection/v1alpha1/zz_generated.deepcopy.go b/apis/observedobjectcollection/v1alpha1/zz_generated.deepcopy.go
diff --git a/internal/controller/observedobjectcollection/reconciler.go b/internal/controller/observedobjectcollection/reconciler.go
@@ -51,7 +51,7 @@ import (
 const (
 	errNewKubernetesClient = "cannot create new Kubernetes client"
 	errStatusUpdate        = "cannot update status"
-	fieldOwner             = client.FieldOwner("observed-object-collection-controller")
+	fieldOwner             = client.FieldOwner("kubernetes.crossplane.io/observed-object-collection-controller")
 	membershipLabelKey     = "kubernetes.crossplane.io/owned-by-collection"
 )
 
@@ -125,14 +125,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (result ct
 
 	log.Info("Reconciling")
 
-	mlv, err := membershipLabelValue(c)
-	if err != nil {
-		werr := errors.Wrapf(err, "error getting value for label %v", membershipLabelKey)
-		c.Status.SetConditions(xpv1.ReconcileError(werr))
-		_ = r.client.Status().Update(ctx, c)
-		return ctrl.Result{}, werr
-	}
-
 	// Get client for the referenced provider config.
 	clusterClient, err := r.clientForProvider(ctx, r.client, c.Spec.ProviderConfigReference.Name)
 	if err != nil {
@@ -143,9 +135,9 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (result ct
 	}
 
 	// Fetch objects based on the set GVK and selector.
-	objects := &unstructured.UnstructuredList{}
-	objects.SetAPIVersion(c.Spec.ObserveObjects.APIVersion)
-	objects.SetKind(c.Spec.ObserveObjects.Kind)
+	k8sobjects := &unstructured.UnstructuredList{}
+	k8sobjects.SetAPIVersion(c.Spec.ObserveObjects.APIVersion)
+	k8sobjects.SetKind(c.Spec.ObserveObjects.Kind)
 	selector, err := metav1.LabelSelectorAsSelector(&c.Spec.ObserveObjects.Selector)
 
 	if err != nil {
@@ -155,17 +147,28 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (result ct
 		return ctrl.Result{}, werr
 	}
 
-	if err := clusterClient.List(ctx, objects, &client.ListOptions{LabelSelector: selector, Namespace: c.Spec.ObserveObjects.Namespace}); err != nil {
-		werr := errors.Wrap(err, "error listing objects")
+	lo := client.ListOptions{LabelSelector: selector, Namespace: c.Spec.ObserveObjects.Namespace}
+	if err := clusterClient.List(ctx, k8sobjects, &lo); err != nil {
+		werr := errors.Wrapf(err, "error fetching objects for GVK %v and options %v", k8sobjects.GetObjectKind().GroupVersionKind(), lo)
+		c.Status.SetConditions(xpv1.ReconcileError(werr))
+		_ = r.client.Status().Update(ctx, c)
+		return ctrl.Result{}, werr
+	}
+
+	// Fetch any existing counter-part observe only Objects by collection label.
+	ml := map[string]string{membershipLabelKey: c.Name}
+	ol := &v1alpha2.ObjectList{}
+	if err := r.client.List(ctx, ol, client.MatchingLabels(ml)); err != nil {
+		werr := errors.Wrapf(err, "cannot list members matching labels %v", ml)
 		c.Status.SetConditions(xpv1.ReconcileError(werr))
 		_ = r.client.Status().Update(ctx, c)
 		return ctrl.Result{}, werr
 	}
 
-	// Create observed-only Objects for all found items.
+	// Create/update observed-only Objects for all found items.
 	refs := sets.New[v1alpha1.ObservedObjectReference]()
-	for i := range objects.Items {
-		o := objects.Items[i]
+	for i := range k8sobjects.Items {
+		o := k8sobjects.Items[i]
 		log.Debug("creating observed object for the matched item", "gvk", o.GroupVersionKind(), "name", o.GetName())
 		name, err := r.observedObjectName(c, &o)
 		if err != nil {
@@ -183,12 +186,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (result ct
 			_ = r.client.Status().Update(ctx, c)
 			return ctrl.Result{}, werr
 		}
-		if l := po.GetLabels(); l != nil {
-			l[membershipLabelKey] = mlv
-			po.SetLabels(l)
-		} else {
-			po.SetLabels(map[string]string{membershipLabelKey: mlv})
-		}
 		if err := r.client.Patch(ctx, po, client.Apply, fieldOwner, client.ForceOwnership); err != nil {
 			werr := errors.Wrap(err, "cannot create observed object")
 			c.Status.SetConditions(xpv1.ReconcileError(werr))
@@ -201,35 +198,22 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (result ct
 	}
 
 	// Remove collection members that either do not exist anymore or are no match.
-	ol := &v1alpha2.ObjectList{}
-	if err := r.client.List(ctx, ol, client.MatchingLabels(map[string]string{membershipLabelKey: mlv})); err != nil {
-		werr := errors.Wrapf(err, "cannot list members matching label %v=%v", membershipLabelKey, mlv)
-		c.Status.SetConditions(xpv1.ReconcileError(werr))
-		_ = r.client.Status().Update(ctx, c)
-		return ctrl.Result{}, werr
-	}
-
-	oldRefs := sets.New[v1alpha1.ObservedObjectReference](c.Status.ObjectRefs...)
 	for i := range ol.Items {
-		name := ol.Items[i].Name
-		if refs.Has(v1alpha1.ObservedObjectReference{Name: name}) {
+		o := ol.Items[i]
+		if refs.Has(v1alpha1.ObservedObjectReference{Name: o.Name}) {
 			continue
 		}
-		log.Debug("Removing", "name", name)
+		log.Debug("Removing", "name", o.Name)
 		if err := r.client.Delete(ctx, &ol.Items[i]); err != nil {
-			werr := errors.Wrapf(err, "cannot delete observed object %s", name)
+			werr := errors.Wrapf(err, "cannot delete observed object %v", o)
 			c.Status.SetConditions(xpv1.ReconcileError(werr))
 			_ = r.client.Status().Update(ctx, c)
 			return ctrl.Result{}, werr
 		}
 	}
 	c.Status.SetConditions(xpv1.ReconcileSuccess(), xpv1.Available())
 
-	// Update references only if more or less objects got matched since the last reconciliation.
-	if !refs.Equal(oldRefs) {
-		c.Status.ObjectRefs = refs.UnsortedList()
-	}
-	c.Status.MembershipLabel = map[string]string{membershipLabelKey: mlv}
+	c.Status.MembershipLabel = ml
 
 	return ctrl.Result{RequeueAfter: r.pollInterval()}, r.client.Status().Update(ctx, c)
 }
@@ -252,19 +236,6 @@ func observedObjectName(collection client.Object, matchedObject client.Object) (
 	return id.String(), err
 }
 
-func membershipLabelValue(collection *v1alpha1.ObservedObjectCollection) (string, error) {
-	if len(collection.GetName()) <= 63 {
-		return collection.GetName(), nil
-	}
-	// Otherwise, compute md5 hash of it, to reduce it length.
-	h := md5.New() //#nosec G401 -- used only for unique name generation
-	if _, err := h.Write([]byte(collection.GetName())); err != nil {
-		return "", err
-	}
-	id, err := uuid.FromBytes(h.Sum(nil))
-	return id.String(), err
-}
-
 func observedObjectPatch(name string, matchedObject unstructured.Unstructured, collection *v1alpha1.ObservedObjectCollection) (*unstructured.Unstructured, error) {
 	objectManifestTemplate := `{
 "kind": "%s",
@@ -299,14 +270,18 @@ func observedObjectPatch(name string, matchedObject unstructured.Unstructured, c
 			},
 		},
 	}
+	labels := map[string]string{
+		membershipLabelKey: collection.Name,
+	}
 	if t := collection.Spec.Template; t != nil {
-		if len(t.Metadata.Labels) > 0 {
-			observedObject.SetLabels(t.Metadata.Labels)
+		for k, v := range t.Metadata.Labels {
+			labels[k] = v
 		}
 		if len(t.Metadata.Annotations) > 0 {
 			observedObject.SetAnnotations(t.Metadata.Annotations)
 		}
 	}
+	observedObject.SetLabels(labels)
 	v, err := runtime.DefaultUnstructuredConverter.ToUnstructured(observedObject)
 	if err != nil {
 		return nil, errors.Wrap(err, "cannot convert to unstructured")

diff --git a/internal/controller/observedobjectcollection/reconciler_test.go b/internal/controller/observedobjectcollection/reconciler_test.go
@@ -163,18 +163,13 @@ func TestReconciler(t *testing.T) {
 					MockStatusUpdate: func(ctx context.Context, obj client.Object, opts ...client.SubResourceUpdateOption) error {
 						c := obj.(*v1alpha1.ObservedObjectCollection)
 						if cnd := c.Status.GetCondition(xpv1.TypeSynced); cnd.Status != corev1.ConditionTrue {
-							return fmt.Errorf("Object sync condition not true: %v", cnd.Message)
+							panic(fmt.Sprintf("Object sync condition not true: %v", cnd.Message))
 						}
 						if cnd := c.Status.GetCondition(xpv1.TypeReady); cnd.Status != corev1.ConditionTrue {
-							return fmt.Errorf("Object ready condition not true: %v", cnd.Message)
-						}
-
-						if l := len(c.Status.ObjectRefs); l != 2 {
-							return fmt.Errorf("Expected 2 objects refs, but got %v", l)
+							panic(fmt.Sprintf("Object ready condition not true: %v", cnd.Message))
 						}
-
 						if v := c.Status.MembershipLabel[membershipLabelKey]; v != collectionName.Name {
-							return fmt.Errorf("Expected membership label %v but got %v", collectionName.Name, v)
+							panic(fmt.Sprintf("Expected membership label %v but got %v", collectionName.Name, v))
 						}
 						return nil
 					},
@@ -270,11 +265,6 @@ func TestReconciler(t *testing.T) {
 						if cnd := c.Status.GetCondition(xpv1.TypeReady); cnd.Status != corev1.ConditionTrue {
 							return fmt.Errorf("Object ready condition not true: %v", cnd.Message)
 						}
-
-						if l := len(c.Status.ObjectRefs); l != 2 {
-							return fmt.Errorf("Expected 2 objects refs, but got %v", l)
-						}
-
 						if v := c.Status.MembershipLabel[membershipLabelKey]; v != collectionName.Name {
 							return fmt.Errorf("Expected membership label %v but got %v", collectionName.Name, v)
 						}
@@ -402,7 +392,7 @@ func TestReconciler(t *testing.T) {
 			}
 			got, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: collectionName})
 			if !errors.Is(err, tc.want.err) {
-				t.Errorf("\n%s\nr.Reconcile(...): want error %v, got error %v", tc.reason, tc.want.err, err)
+				t.Errorf("\n%s\nr.Reconcile(...): want error: %v, got error: %v", tc.reason, tc.want.err, err)
 			}
 			if diff := cmp.Diff(tc.want.r, got, test.EquateErrors()); diff != "" {
 				t.Errorf("\n%s\nr.Reconcile(...): -want, +got:\n%s", tc.reason, diff)

diff --git a/package/crds/kubernetes.crossplane.io_observedobjectcollections.yaml b/package/crds/kubernetes.crossplane.io_observedobjectcollections.yaml
@@ -244,28 +244,13 @@ spec:
                   MembershipLabel is the label set on each member of this collection
                   and can be used for fetching them.
                 type: object
-              objectRefs:
-                description: List of object references mathing the given selector
-                items:
-                  description: ObservedObjectReference represents a reference to Object
-                    with ObserveOnly management policy
-                  properties:
-                    name:
-                      description: Name of the observed object
-                      maxLength: 253
-                      minLength: 1
-                      type: string
-                  required:
-                  - name
-                  type: object
-                type: array
-                x-kubernetes-list-map-keys:
-                - name
-                x-kubernetes-list-type: map
             type: object
         required:
         - spec
         type: object
+        x-kubernetes-validations:
+        - message: metadata.name max length is 63
+          rule: size(self.metadata.name) < 64
     served: true
     storage: true
     subresources: