bug: avoid failing when not finding status expressions

controllers/authzctrl/controller.go

@@ -21,6 +21,7 @@ import (
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/builder"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/predicate"
 )
 
 const name = "authorization"
@@ -115,8 +116,8 @@ func (r *Controller) SetupWithManager(mgr ctrl.Manager) error {
 				Kind:       r.protectedResource.ResourceReference.Kind,
 			},
 		}, builder.OnlyMetadata).
-		Owns(&authorinov1beta2.AuthConfig{}).
-		Owns(&istiosecurityv1beta1.AuthorizationPolicy{}).
+		Owns(&authorinov1beta2.AuthConfig{}, builder.WithPredicates(predicate.GenerationChangedPredicate{})).
+		Owns(&istiosecurityv1beta1.AuthorizationPolicy{}, builder.WithPredicates(predicate.GenerationChangedPredicate{})).
 		Complete(r)
 }
 

controllers/authzctrl/reconcile_authpolicy.go

@@ -69,6 +69,13 @@ func (r *Controller) reconcileAuthPolicy(ctx context.Context, target *unstructur
 }
 
 func createAuthzPolicy(ports []string, workloadSelector map[string]string, providerName string, target *unstructured.Unstructured) *istiosecurityv1beta1.AuthorizationPolicy {
+	excludePaths := []string{ // TODO: part of AuthRule?
+		"/healthz",
+		"/debug/pprof/",
+		"/metrics",
+		"/wait-for-drain",
+	}
+
 	policy := &istiosecurityv1beta1.AuthorizationPolicy{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:        target.GetName(),
@@ -91,23 +98,31 @@ func createAuthzPolicy(ports []string, workloadSelector map[string]string, provi
 		},
 	}
 
-	for _, port := range ports {
+	if len(ports) == 0 {
 		rule := v1beta1.Rule{
 			To: []*v1beta1.Rule_To{
 				{
 					Operation: &v1beta1.Operation{
-						Ports: []string{port},
-						NotPaths: []string{ // TODO: part of AuthRule?
-							"/healthz",
-							"/debug/pprof/",
-							"/metrics",
-							"/wait-for-drain",
-						},
+						NotPaths: excludePaths,
 					},
 				},
 			},
 		}
 		policy.Spec.Rules = append(policy.Spec.Rules, &rule)
+	} else {
+		for _, port := range ports {
+			rule := v1beta1.Rule{
+				To: []*v1beta1.Rule_To{
+					{
+						Operation: &v1beta1.Operation{
+							Ports:    []string{port},
+							NotPaths: excludePaths,
+						},
+					},
+				},
+			}
+			policy.Spec.Rules = append(policy.Spec.Rules, &rule)
+		}
 	}
 
 	metadata.ApplyMetaOptions(policy, labels.StandardLabelsFrom(target)...)

controllers/routingctrl/controller.go

@@ -98,8 +98,6 @@ func (r *Controller) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 		errs = append(errs, reconciler(ctx, sourceRes))
 	}
 
-	errs = append(errs, unstruct.Patch(ctx, r.Client, sourceRes))
-
 	return ctrl.Result{}, errors.Join(errs...)
 }
 

controllers/routingctrl/delete_resources.go

@@ -6,6 +6,7 @@ import (
 
 	"github.com/opendatahub-io/odh-platform/pkg/metadata/labels"
 	"github.com/opendatahub-io/odh-platform/pkg/routing"
+	"github.com/opendatahub-io/odh-platform/pkg/unstruct"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	k8slabels "k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime/schema"
@@ -31,12 +32,12 @@ func (r *Controller) removeUnusedRoutingResources(ctx context.Context, target *u
 func (r *Controller) handleResourceDeletion(ctx context.Context, sourceRes *unstructured.Unstructured) error {
 	exportModes := r.extractExportModes(sourceRes)
 	if len(exportModes) == 0 {
-		r.log.Info("No export modes found, skipping deletion logic", "sourceRes", sourceRes)
+		r.log.Info("no export modes found, skipping deletion logic", "sourceRes", sourceRes)
 
 		return nil
 	}
 
-	r.log.Info("Handling deletion of dependent resources", "sourceRes", sourceRes)
+	r.log.Info("handling deletion of dependent resources", "sourceRes", sourceRes)
 
 	gvks := routingResourceGVKs(exportModes...)
 
@@ -88,10 +89,13 @@ func (r *Controller) deleteOwnedResources(ctx context.Context,
 // removeFinalizer is called after a successful cleanup, it removes the finalizer from the resource in the cluster.
 func removeFinalizer(ctx context.Context, cli client.Client, sourceRes *unstructured.Unstructured) error {
 	if controllerutil.ContainsFinalizer(sourceRes, finalizerName) {
-		controllerutil.RemoveFinalizer(sourceRes, finalizerName)
+		if err := unstruct.PatchWithRetry(ctx, cli, sourceRes, func() error {
+			controllerutil.RemoveFinalizer(sourceRes, finalizerName)
 
-		if err := cli.Update(ctx, sourceRes); err != nil {
-			return fmt.Errorf("failed to remove finalizer: %w", err)
+			return nil
+		}); err != nil {
+			return fmt.Errorf("failed to remove finalizer from %s (in %s): %w",
+				sourceRes.GroupVersionKind().String(), sourceRes.GetNamespace(), err)
 		}
 	}
 

controllers/routingctrl/exported_svc_locator.go

@@ -48,6 +48,11 @@
 	return fmt.Sprintf("no exported services found for target %s/%s (%s)", e.target.GetNamespace(), e.target.GetName(), e.target.GetObjectKind().GroupVersionKind().String())
 }
 
+func (e *ExportedServiceNotFoundError) Is(target error) bool {
+	_, ok := target.(*ExportedServiceNotFoundError)
+	return ok
+}
+
 func isExportedServiceNotFoundError(err error) bool {
 	return errors.Is(err, &ExportedServiceNotFoundError{})
 }

controllers/routingctrl/reconcile_resources.go

@@ -23,15 +23,11 @@ func (r *Controller) createRoutingResources(ctx context.Context, target *unstruc
 
 	if len(exportModes) == 0 {
 		r.log.Info("No export mode found for target")
-		metadata.ApplyMetaOptions(target,
-			annotations.Remove(annotations.RoutingAddressesExternal("")),
-			annotations.Remove(annotations.RoutingAddressesPublic("")),
-		)
 
-		return nil
+		return r.propagateHostsToWatchedCR(ctx, target, nil, nil)
 	}
 
-	r.log.Info("Reconciling resources for target", "target", target)
+	r.log.Info("reconciling resources for target", "target", target)
 
 	renderedSelectors, errLables := config.ResolveSelectors(r.component.ServiceSelector, target)
 	if errLables != nil {
@@ -56,21 +52,34 @@ func (r *Controller) createRoutingResources(ctx context.Context, target *unstruc
 
 	var errSvcExport []error
 
+	var targetPublicHosts []string
+
+	var targetExternalHosts []string
+
 	for i := range exportedServices {
-		if errExport := r.exportService(ctx, target, &exportedServices[i], domain); errExport != nil {
+		servicePublicHosts, serviceExternalHosts, errExport := r.exportService(ctx, target, &exportedServices[i], domain)
+		if errExport != nil {
 			errSvcExport = append(errSvcExport, errExport)
+
+			continue
 		}
+
+		targetPublicHosts = append(targetPublicHosts, servicePublicHosts...)
+		targetExternalHosts = append(targetExternalHosts, serviceExternalHosts...)
 	}
 
-	return errors.Join(errSvcExport...)
+	if errSvcExportCombined := errors.Join(errSvcExport...); errSvcExportCombined != nil {
+		return errSvcExportCombined
+	}
+
+	return r.propagateHostsToWatchedCR(ctx, target, targetPublicHosts, targetExternalHosts)
 }
 
-func (r *Controller) exportService(ctx context.Context, target *unstructured.Unstructured, exportedSvc *corev1.Service, domain string) error {
+//nolint:nonamedreturns //reason make up your mind, nonamedreturns vs gocritic
+func (r *Controller) exportService(ctx context.Context, target *unstructured.Unstructured,
+	exportedSvc *corev1.Service, domain string) (publicHosts, externalHosts []string, err error) {
 	exportModes := r.extractExportModes(target)
 
-	externalHosts := []string{}
-	publicHosts := []string{}
-
 	// To establish ownership for watched component
 	ownershipLabels := append(labels.AsOwner(target), labels.AppManagedBy("odh-routing-controller"))
 
@@ -80,12 +89,12 @@ func (r *Controller) exportService(ctx context.Context, target *unstructured.Uns
 		for _, exportMode := range exportModes {
 			resources, err := r.templateLoader.Load(templateData, exportMode)
 			if err != nil {
-				return fmt.Errorf("could not load templates for type %s: %w", exportMode, err)
+				return nil, nil, fmt.Errorf("could not load templates for type %s: %w", exportMode, err)
 			}
 
 			ownershipLabels = append(ownershipLabels, labels.ExportType(exportMode))
 			if errApply := unstruct.Apply(ctx, r.Client, resources, ownershipLabels...); errApply != nil {
-				return fmt.Errorf("could not apply routing resources for type %s: %w", exportMode, errApply)
+				return nil, nil, fmt.Errorf("could not apply routing resources for type %s: %w", exportMode, errApply)
 			}
 
 			switch exportMode {
@@ -97,33 +106,49 @@ func (r *Controller) exportService(ctx context.Context, target *unstructured.Uns
 		}
 	}
 
-	return r.propagateHostsToWatchedCR(target, publicHosts, externalHosts)
+	return publicHosts, externalHosts, nil
 }
 
-func (r *Controller) propagateHostsToWatchedCR(target *unstructured.Unstructured, publicHosts, externalHosts []string) error {
-	// Remove all existing routing addresses
-	metaOptions := []metadata.Option{
-		annotations.Remove(annotations.RoutingAddressesExternal("")),
-		annotations.Remove(annotations.RoutingAddressesPublic("")),
+func (r *Controller) propagateHostsToWatchedCR(ctx context.Context, target *unstructured.Unstructured, publicHosts, externalHosts []string) error {
+	errPatch := unstruct.PatchWithRetry(ctx, r.Client, target, r.updateAddressAnnotations(target, publicHosts, externalHosts))
+	if errPatch != nil {
+		return fmt.Errorf("failed to propagate hosts to watched CR %s/%s: %w", target.GetNamespace(), target.GetName(), errPatch)
 	}
 
-	if len(publicHosts) > 0 {
-		metaOptions = append(metaOptions, annotations.RoutingAddressesPublic(strings.Join(publicHosts, ";")))
-	}
+	return nil
+}
 
-	if len(externalHosts) > 0 {
-		metaOptions = append(metaOptions, annotations.RoutingAddressesExternal(strings.Join(externalHosts, ";")))
-	}
+func (r *Controller) updateAddressAnnotations(target *unstructured.Unstructured, publicHosts, externalHosts []string) func() error {
+	return func() error {
+		// Remove all existing routing addresses
+		metaOptions := []metadata.Option{
+			annotations.Remove(annotations.RoutingAddressesExternal("")),
+			annotations.Remove(annotations.RoutingAddressesPublic("")),
+		}
 
-	metadata.ApplyMetaOptions(target, metaOptions...)
+		if len(publicHosts) > 0 {
+			metaOptions = append(metaOptions, annotations.RoutingAddressesPublic(strings.Join(publicHosts, ";")))
+		}
 
-	return nil
+		if len(externalHosts) > 0 {
+			metaOptions = append(metaOptions, annotations.RoutingAddressesExternal(strings.Join(externalHosts, ";")))
+		}
+
+		metadata.ApplyMetaOptions(target, metaOptions...)
+
+		return nil
+	}
 }
 
 func (r *Controller) ensureResourceHasFinalizer(ctx context.Context, target *unstructured.Unstructured) error {
-	if controllerutil.AddFinalizer(target, finalizerName) {
-		if err := unstruct.Patch(ctx, r.Client, target); err != nil {
-			return fmt.Errorf("failed to patch finalizer to resource %s/%s: %w", target.GetNamespace(), target.GetName(), err)
+	if !controllerutil.ContainsFinalizer(target, finalizerName) {
+		if err := unstruct.PatchWithRetry(ctx, r.Client, target, func() error {
+			controllerutil.AddFinalizer(target, finalizerName)
+
+			return nil
+		}); err != nil {
+			return fmt.Errorf("failed to patch finalizer to %s (in %s): %w",
+				target.GroupVersionKind().String(), target.GetNamespace(), err)
 		}
 	}
 
@@ -144,11 +169,6 @@ func (r *Controller) extractExportModes(target *unstructured.Unstructured) []rou
 			routeType, valid := routing.IsValidRouteType(key)
 			if valid {
 				validRouteTypes = append(validRouteTypes, routeType)
-			} else {
-				r.log.Info("Invalid route type found",
-					"invalidRouteType", routeType,
-					"resourceName", target.GetName(),
-					"resourceNamespace", target.GetNamespace())
 			}
 		}
 	}

pkg/spi/types.go

@@ -94,7 +94,7 @@ func UnifiedHostExtractor(extractors ...HostExtractor) HostExtractor { //nolint:
 }
 
 func NewPathExpressionExtractor(paths []string) HostExtractor {
-	extractHosts := func(target *unstructured.Unstructured, splitPath []string) ([]string, error) {
+	extractHosts := func(target *unstructured.Unstructured, splitPath []string) ([]string, error) { //nolint:unparam //reason Part of HostExtractor interface
 		// extracting as string
 		if foundHost, found, err := unstructured.NestedString(target.Object, splitPath...); err == nil && found {
 			return []string{foundHost}, nil
@@ -105,7 +105,8 @@ func NewPathExpressionExtractor(paths []string) HostExtractor {
 			return foundHosts, nil
 		}
 
-		return nil, fmt.Errorf("neither string nor slice of strings found at path %v", splitPath)
+		// TODO: Nothing found yet, move on no error?
+		return []string{}, nil
 	}
 
 	return func(target *unstructured.Unstructured) ([]string, error) {

pkg/unstruct/funcs.go

@@ -10,6 +10,7 @@ import (
 	k8stypes "k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/util/retry"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 )
 
 func Apply(ctx context.Context, cli client.Client, objects []*unstructured.Unstructured, metaOptions ...metadata.Option) error {
@@ -63,23 +64,13 @@ func IsMarkedForDeletion(target *unstructured.Unstructured) bool {
 	return !target.GetDeletionTimestamp().IsZero()
 }
 
-// Patch updates the specified Kubernetes resource by applying changes from the provided target object.
-// In case of conflicts, it will retry using default strategy.
-func Patch(ctx context.Context, cli client.Client, target *unstructured.Unstructured) error {
-	err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
-		currentRes := &unstructured.Unstructured{}
-		currentRes.SetGroupVersionKind(target.GroupVersionKind())
+// PatchWithRetry applies changes to the specified resource using the provided mutate function.
+// It uses a retry mechanism to handle potential conflicts during the update process.
+func PatchWithRetry(ctx context.Context, cli client.Client, target *unstructured.Unstructured, mutate controllerutil.MutateFn) error {
+	err := retry.RetryOnConflict(retry.DefaultBackoff, func() error {
+		_, err := controllerutil.CreateOrPatch(ctx, cli, target, mutate)
 
-		if err := cli.Get(ctx, client.ObjectKeyFromObject(target), currentRes); err != nil {
-			return fmt.Errorf("failed re-fetching resource: %w", err)
-		}
-
-		patch := client.MergeFrom(currentRes)
-		if errPatch := cli.Patch(ctx, target, patch); errPatch != nil {
-			return fmt.Errorf("failed to patch: %w", errPatch)
-		}
-
-		return nil
+		return err //nolint:wrapcheck // Return unwrapped error per RetryOnConflict godoc
 	})
 
 	if err != nil {