Update sync package to support creating objects it doesn't recognize

Update the sync package to support it attempting to create potentially
arbitrary Kubernetes objects, as there are no restrictions (outside of
RBAC) on what can be included in Kubernetes/OpenShift components.

Signed-off-by: Angel Misevski <[email protected]>

Author: amisevsk

pkg/provision/sync/cluster_api.go

@@ -67,3 +67,17 @@ type UnrecoverableSyncError struct {
 func (e *UnrecoverableSyncError) Error() string {
 	return e.Cause.Error()
 }
+
+// WarningError is returned when syncing is successful and can continue but there is a warning
+// regarding the objects synced to the cluster (e.g. they will not be updated)
+type WarningError struct {
+	Message string
+	Err     error
+}
+
+func (e *WarningError) Error() string {
+	if e.Err != nil {
+		return fmt.Sprintf("%s: %s", e.Message, e.Err)
+	}
+	return e.Message
+}

pkg/provision/sync/diff.go

@@ -36,17 +36,17 @@ import (
 type diffFunc func(spec crclient.Object, cluster crclient.Object) (delete, update bool)
 
 var diffFuncs = map[reflect.Type]diffFunc{
-	reflect.TypeOf(rbacv1.Role{}):                  allDiffFuncs(labelsAndAnnotationsDiffFunc, basicDiffFunc(roleDiffOpts)),
-	reflect.TypeOf(rbacv1.RoleBinding{}):           allDiffFuncs(labelsAndAnnotationsDiffFunc, basicDiffFunc(rolebindingDiffOpts)),
-	reflect.TypeOf(corev1.ServiceAccount{}):        allDiffFuncs(labelsAndAnnotationsDiffFunc, ownerrefsDiffFunc),
-	reflect.TypeOf(appsv1.Deployment{}):            allDiffFuncs(deploymentDiffFunc, labelsAndAnnotationsDiffFunc, basicDiffFunc(deploymentDiffOpts)),
-	reflect.TypeOf(corev1.ConfigMap{}):             allDiffFuncs(labelsAndAnnotationsDiffFunc, basicDiffFunc(configmapDiffOpts)),
-	reflect.TypeOf(corev1.Secret{}):                allDiffFuncs(labelsAndAnnotationsDiffFunc, basicDiffFunc(secretDiffOpts)),
-	reflect.TypeOf(v1alpha1.DevWorkspaceRouting{}): allDiffFuncs(routingDiffFunc, labelsAndAnnotationsDiffFunc, basicDiffFunc(routingDiffOpts)),
-	reflect.TypeOf(batchv1.Job{}):                  allDiffFuncs(labelsAndAnnotationsDiffFunc, jobDiffFunc),
-	reflect.TypeOf(corev1.Service{}):               allDiffFuncs(labelsAndAnnotationsDiffFunc, serviceDiffFunc),
-	reflect.TypeOf(networkingv1.Ingress{}):         allDiffFuncs(labelsAndAnnotationsDiffFunc, basicDiffFunc(ingressDiffOpts)),
-	reflect.TypeOf(routev1.Route{}):                allDiffFuncs(labelsAndAnnotationsDiffFunc, basicDiffFunc(routeDiffOpts)),
+	reflect.TypeOf(rbacv1.Role{}):                  allDiffFuncs(metadataDiffFunc, basicDiffFunc(roleDiffOpts)),
+	reflect.TypeOf(rbacv1.RoleBinding{}):           allDiffFuncs(metadataDiffFunc, basicDiffFunc(rolebindingDiffOpts)),
+	reflect.TypeOf(corev1.ServiceAccount{}):        metadataDiffFunc,
+	reflect.TypeOf(appsv1.Deployment{}):            allDiffFuncs(deploymentDiffFunc, metadataDiffFunc, basicDiffFunc(deploymentDiffOpts)),
+	reflect.TypeOf(corev1.ConfigMap{}):             allDiffFuncs(metadataDiffFunc, basicDiffFunc(configmapDiffOpts)),
+	reflect.TypeOf(corev1.Secret{}):                allDiffFuncs(metadataDiffFunc, basicDiffFunc(secretDiffOpts)),
+	reflect.TypeOf(v1alpha1.DevWorkspaceRouting{}): allDiffFuncs(routingDiffFunc, metadataDiffFunc, basicDiffFunc(routingDiffOpts)),
+	reflect.TypeOf(batchv1.Job{}):                  allDiffFuncs(metadataDiffFunc, jobDiffFunc),
+	reflect.TypeOf(corev1.Service{}):               allDiffFuncs(metadataDiffFunc, serviceDiffFunc),
+	reflect.TypeOf(networkingv1.Ingress{}):         allDiffFuncs(metadataDiffFunc, basicDiffFunc(ingressDiffOpts)),
+	reflect.TypeOf(routev1.Route{}):                allDiffFuncs(metadataDiffFunc, basicDiffFunc(routeDiffOpts)),
 }
 
 // basicDiffFunc returns a diffFunc that specifies an object needs an update if cmp.Equal fails
@@ -56,9 +56,9 @@ func basicDiffFunc(diffOpt cmp.Options) diffFunc {
 	}
 }
 
-// labelsAndAnnotationsDiffFunc requires an object to be updated if any label or annotation present in the spec
+// metadataDiffFunc requires an object to be updated if any label or annotation present in the spec
 // object is not present in the cluster object.
-func labelsAndAnnotationsDiffFunc(spec, cluster crclient.Object) (delete, update bool) {
+func metadataDiffFunc(spec, cluster crclient.Object) (delete, update bool) {
 	clusterAnnotations := cluster.GetAnnotations()
 	for k, v := range spec.GetAnnotations() {
 		if clusterAnnotations[k] != v {
@@ -71,10 +71,6 @@ func labelsAndAnnotationsDiffFunc(spec, cluster crclient.Object) (delete, update
 			return false, true
 		}
 	}
-	return false, false
-}
-
-func ownerrefsDiffFunc(spec, cluster crclient.Object) (delete, update bool) {
 	clusterRefs := cluster.GetOwnerReferences()
 	for _, ownerref := range spec.GetOwnerReferences() {
 		if !containsOwnerRef(ownerref, clusterRefs) {
@@ -147,6 +143,10 @@ func serviceDiffFunc(spec, cluster crclient.Object) (delete, update bool) {
 	return false, specCopy.Spec.Type != clusterCopy.Spec.Type
 }
 
+func unrecognizedObjectDiffFunc(spec, cluster crclient.Object) (delete, update bool) {
+	return metadataDiffFunc(spec, cluster)
+}
+
 func containsOwnerRef(toCheck metav1.OwnerReference, listRefs []metav1.OwnerReference) bool {
 	boolPtrsEqual := func(a, b *bool) bool {
 		// If either is nil, assume check other is nil or false; otherwise, compare actual values

pkg/provision/sync/sync.go

@@ -31,6 +31,15 @@ import (
 	crclient "sigs.k8s.io/controller-runtime/pkg/client"
 )
 
+// IsRecognizedObject returns whether the provided object kind is recognized by the sync package to support updating
+// existing objects on the cluster. If the object is not recognized, passing it to SyncObjectWithCluster will fail.
+// Instead, SyncUnrecognizedObjectWithCluster should be used.
+func IsRecognizedObject(specObj crclient.Object) bool {
+	objType := reflect.TypeOf(specObj).Elem()
+	_, ok := diffFuncs[objType]
+	return ok
+}
+
 // SyncObjectWithCluster synchronises the state of specObj to the cluster, creating or updating the cluster object
 // as required. If specObj is in sync with the cluster, returns the object as it exists on the cluster. Returns a
 // NotInSyncError if an update is required, UnrecoverableSyncError if object provided is invalid, or generic error
@@ -72,6 +81,59 @@ func SyncObjectWithCluster(specObj crclient.Object, api ClusterAPI) (crclient.Ob
 	return clusterObj, nil
 }
 
+// SyncUnrecognizedObjectWithCluster allows syncing objects not supported by SyncObjectWithCluster. As there is
+// no generic way of deciding if an object needs to be updated, a WarningError is returned if the object exists
+// on the cluster. The only object updating performed by this function is to ensure labels/annotations and
+// ownerReferences in specObj are synced to the cluster.
+// The reason arbitrary updates are not supported is 1) certain objects have defaulted fields that can always
+// trigger naive diff checks (causing reconciles to get stuck), and 2) it's unknown which fields are unmodifiable,
+// (e.g. services must keep ClusterIP once set; pod fields cannot be changed after creation)
+func SyncUnrecognizedObjectWithCluster(specObj crclient.Object, api ClusterAPI) (crclient.Object, error) {
+	objType := reflect.TypeOf(specObj).Elem()
+	clusterObj := reflect.New(objType).Interface().(crclient.Object)
+
+	err := api.Client.Get(api.Ctx, types.NamespacedName{Name: specObj.GetName(), Namespace: specObj.GetNamespace()}, clusterObj)
+	if err != nil {
+		if k8sErrors.IsNotFound(err) {
+			return nil, createObjectGeneric(specObj, api)
+		}
+		return nil, err
+	}
+	update, delete := unrecognizedObjectDiffFunc(specObj, clusterObj)
+	if update {
+		toUpdate, err := unrecognizedObjectUpdateFunc(specObj, clusterObj)
+		if err != nil {
+			return nil, &UnrecoverableSyncError{err}
+		}
+		err = api.Client.Update(api.Ctx, toUpdate)
+		switch {
+		case err == nil:
+			api.Logger.Info("Updated object", "kind", reflect.TypeOf(specObj).Elem().String(), "name", specObj.GetName())
+			return nil, NewNotInSync(specObj, UpdatedObjectReason)
+		case k8sErrors.IsConflict(err):
+			return nil, NewNotInSync(specObj, NeedRetryReason)
+		case k8sErrors.IsInvalid(err), k8sErrors.IsForbidden(err):
+			return nil, &UnrecoverableSyncError{err}
+		default:
+			return nil, err
+		}
+	}
+	if delete {
+		if err := api.Client.Delete(api.Ctx, clusterObj); err != nil {
+			if k8sErrors.IsForbidden(err) {
+				return nil, &UnrecoverableSyncError{fmt.Errorf("failed to delete object %s: %w", clusterObj.GetName(), err)}
+			}
+			return nil, err
+		}
+		api.Logger.Info("Deleted object", "kind", reflect.TypeOf(specObj).Elem().String(), "name", specObj.GetName())
+		return nil, NewNotInSync(specObj, DeletedObjectReason)
+	}
+	kind := specObj.GetObjectKind().GroupVersionKind().GroupKind().String()
+	return clusterObj, &WarningError{
+		Message: fmt.Sprintf("Unrecognized object kind %s. Object will not be updated on cluster", kind),
+	}
+}
+
 func createObjectGeneric(specObj crclient.Object, api ClusterAPI) error {
 	err := api.Client.Create(api.Ctx, specObj)
 	switch {

pkg/provision/sync/update.go

@@ -18,6 +18,7 @@ import (
 	"reflect"
 
 	corev1 "k8s.io/api/core/v1"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 	crclient "sigs.k8s.io/controller-runtime/pkg/client"
 )
 
@@ -77,3 +78,40 @@ func getUpdateFunc(obj crclient.Object) updateFunc {
 		return defaultUpdateFunc
 	}
 }
+
+func unrecognizedObjectUpdateFunc(spec, cluster crclient.Object) (crclient.Object, error) {
+	if cluster == nil {
+		return nil, errors.New("updating unrecognized object requires the cluster instance")
+	}
+	clusterCopy := cluster.DeepCopyObject().(client.Object)
+	newLabels := clusterCopy.GetLabels()
+	for k, v := range spec.GetLabels() {
+		newLabels[k] = v
+	}
+	clusterCopy.SetLabels(newLabels)
+
+	newAnnotations := clusterCopy.GetAnnotations()
+	for k, v := range spec.GetAnnotations() {
+		newAnnotations[k] = v
+	}
+	clusterCopy.SetAnnotations(newAnnotations)
+
+	newOwnerrefs := clusterCopy.GetOwnerReferences()
+	for _, specOwnerref := range spec.GetOwnerReferences() {
+		found := false
+		for _, ownerref := range cluster.GetOwnerReferences() {
+			if specOwnerref.Kind == ownerref.Kind &&
+				specOwnerref.Name == ownerref.Name &&
+				specOwnerref.UID == ownerref.UID {
+				found = true
+				break
+			}
+		}
+		if !found {
+			newOwnerrefs = append(newOwnerrefs, specOwnerref)
+		}
+	}
+	clusterCopy.SetOwnerReferences(newOwnerrefs)
+
+	return clusterCopy, nil
+}