dryrun before update as reconciliation option

Makefile

@@ -263,7 +263,7 @@ test: test-unit test-integration ## Run all tests
 
 test-integration: clean-cov generate fmt vet envtest ## Run Integration tests.
 	mkdir -p coverage/integration
-	KUBEBUILDER_ASSETS="$(shell $(ENVTEST) $(ARCH_PARAM) use $(ENVTEST_K8S_VERSION) -p path)" USE_EXISTING_CLUSTER=true go test $(INTEGRATION_DIRS) -coverprofile $(PROJECT_PATH)/coverage/integration/cover.out -tags integration -ginkgo.v -ginkgo.progress -v -timeout 0
+	KUBEBUILDER_ASSETS="$(shell $(ENVTEST) $(ARCH_PARAM) use $(ENVTEST_K8S_VERSION) -p path)" go test $(INTEGRATION_DIRS) -coverprofile $(PROJECT_PATH)/coverage/integration/cover.out -tags integration -ginkgo.v -ginkgo.progress -v -timeout 0
 
 ifdef TEST_NAME
 test-unit: TEST_PATTERN := --run $(TEST_NAME)
@@ -382,7 +382,7 @@ install-metallb: $(KUSTOMIZE) ## Installs the metallb load balancer allowing use
 	./utils/docker-network-ipaddresspool.sh kind | kubectl apply -n metallb-system -f -
 
 .PHONY: uninstall-metallb
-uninstall-metallb: $(KUSTOMIZE) 
+uninstall-metallb: $(KUSTOMIZE)
 	$(KUSTOMIZE) build config/metallb | kubectl delete -f -
 
 .PHONY: install-olm

controllers/authpolicy_authconfig.go

@@ -16,6 +16,7 @@ import (
 	authorinoapi "github.com/kuadrant/authorino/api/v1beta2"
 	api "github.com/kuadrant/kuadrant-operator/api/v1beta2"
 	"github.com/kuadrant/kuadrant-operator/pkg/common"
+	"github.com/kuadrant/kuadrant-operator/pkg/reconcilers"
 )
 
 func (r *AuthPolicyReconciler) reconcileAuthConfigs(ctx context.Context, ap *api.AuthPolicy, targetNetworkObject client.Object) error {
@@ -34,7 +35,9 @@ func (r *AuthPolicyReconciler) reconcileAuthConfigs(ctx context.Context, ap *api
 		return err
 	}
 
-	err = r.ReconcileResource(ctx, &authorinoapi.AuthConfig{}, authConfig, alwaysUpdateAuthConfig)
+	// Authconfig objects have server side defaults and webhook mutations, reconciliation should
+	// run dry run first for a consistent reconciliation (does not trigger update all the times)
+	err = r.ReconcileResource(ctx, &authorinoapi.AuthConfig{}, authConfig, alwaysUpdateAuthConfig, reconcilers.DryRunFirst)
 	if err != nil && !apierrors.IsAlreadyExists(err) {
 		logger.Error(err, "ReconcileResource failed to create/update AuthConfig resource")
 		return err

controllers/suite_test.go

@@ -34,6 +34,7 @@ import (
 	istioapis "istio.io/istio/operator/pkg/apis"
 	"k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/client-go/rest"
+	"k8s.io/utils/ptr"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/envtest"
@@ -71,6 +72,7 @@ var _ = BeforeSuite(func() {
 	testEnv = &envtest.Environment{
 		CRDDirectoryPaths:     []string{filepath.Join("..", "config", "crd", "bases")},
 		ErrorIfCRDPathMissing: true,
+		UseExistingCluster:    ptr.To(true),
 	}
 
 	cfg, err := testEnv.Start()

pkg/reconcilers/base_reconciler.go

@@ -26,6 +26,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/tools/record"
+	"k8s.io/utils/ptr"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
@@ -92,6 +93,41 @@ func (b *BaseReconciler) EventRecorder() record.EventRecorder {
 	return b.recorder
 }
 
+// ReconcileOptions contains options for reconcile objects.
+type ReconcileOptions struct {
+	// Ensure defaults and webhook mutations are considered before comparing
+	// existing vs desired state of resources when reconciling updates
+	// by dry-running the updates before
+	DryRunFirst *bool
+
+	GetOptions []client.GetOption
+
+	CreateOptions []client.CreateOption
+
+	DeleteOptions []client.DeleteOption
+
+	UpdateOptions []client.UpdateOption
+}
+
+// DryRunFirst sets the "dry run first" option to "true", ensuring all
+// validation, mutations etc first without persisting the change to storage.
+var DryRunFirst = dryRunFirst{}
+
+type dryRunFirst struct{}
+
+// ApplyToList applies this configuration to the given list options.
+func (dryRunFirst) ApplyToReconcile(opts *ReconcileOptions) {
+	opts.DryRunFirst = ptr.To(true)
+}
+
+var _ ReconcileOption = &dryRunFirst{}
+
+// ReconcileOption adds some customization for the reconciliation process.
+type ReconcileOption interface {
+	// ApplyToUpdate applies this configuration to the given update options.
+	ApplyToReconcile(*ReconcileOptions)
+}
+
 // ReconcileResource attempts to mutate the existing state
 // in order to match the desired state. The object's desired state must be reconciled
 // with the existing state inside the passed in callback MutateFn.
@@ -104,17 +140,23 @@ func (b *BaseReconciler) EventRecorder() record.EventRecorder {
 // desired: Object representing the desired state
 //
 // It returns an error.
-func (b *BaseReconciler) ReconcileResource(ctx context.Context, obj, desired client.Object, mutateFn MutateFn) error {
+func (b *BaseReconciler) ReconcileResource(ctx context.Context, obj, desired client.Object, mutateFn MutateFn, opts ...ReconcileOption) error {
+	// Capture options
+	reconcileOpts := &ReconcileOptions{}
+	for _, opt := range opts {
+		opt.ApplyToReconcile(reconcileOpts)
+	}
+
 	key := client.ObjectKeyFromObject(desired)
 
-	if err := b.Client().Get(ctx, key, obj); err != nil {
+	if err := b.GetResource(ctx, key, obj, reconcileOpts.GetOptions...); err != nil {
 		if !errors.IsNotFound(err) {
 			return err
 		}
 
 		// Not found
 		if !common.IsObjectTaggedToDelete(desired) {
-			return b.CreateResource(ctx, desired)
+			return b.CreateResource(ctx, desired, reconcileOpts.CreateOptions...)
 		}
 
 		// Marked for deletion and not found. Nothing to do.
@@ -123,48 +165,53 @@ func (b *BaseReconciler) ReconcileResource(ctx context.Context, obj, desired cli
 
 	// item found successfully
 	if common.IsObjectTaggedToDelete(desired) {
-		return b.DeleteResource(ctx, desired)
+		return b.DeleteResource(ctx, desired, reconcileOpts.DeleteOptions...)
 	}
 
-	desired.SetResourceVersion(obj.GetResourceVersion())
-	if err := b.Client().Update(ctx, desired, client.DryRunAll); err != nil {
-		return err
+	var desiredMutated client.Object = desired
+
+	if ptr.Deref(reconcileOpts.DryRunFirst, false) {
+		desiredMutated = desired.DeepCopyObject().(client.Object)
+		desiredMutated.SetResourceVersion(obj.GetResourceVersion())
+		if err := b.UpdateResource(ctx, desiredMutated, client.DryRunAll); err != nil {
+			return err
+		}
 	}
 
-	update, err := mutateFn(obj, desired)
+	update, err := mutateFn(obj, desiredMutated)
 	if err != nil {
 		return err
 	}
 
 	if update {
-		return b.UpdateResource(ctx, obj)
+		return b.UpdateResource(ctx, obj, reconcileOpts.UpdateOptions...)
 	}
 
 	return nil
 }
 
-func (b *BaseReconciler) GetResource(ctx context.Context, objKey types.NamespacedName, obj client.Object) error {
+func (b *BaseReconciler) GetResource(ctx context.Context, objKey types.NamespacedName, obj client.Object, opts ...client.GetOption) error {
 	logger, _ := logr.FromContext(ctx)
 	logger.Info("get object", "kind", strings.Replace(fmt.Sprintf("%T", obj), "*", "", 1), "name", objKey.Name, "namespace", objKey.Namespace)
-	return b.Client().Get(ctx, objKey, obj)
+	return b.Client().Get(ctx, objKey, obj, opts...)
 }
 
-func (b *BaseReconciler) CreateResource(ctx context.Context, obj client.Object) error {
+func (b *BaseReconciler) CreateResource(ctx context.Context, obj client.Object, opts ...client.CreateOption) error {
 	logger, _ := logr.FromContext(ctx)
 	logger.Info("create object", "kind", strings.Replace(fmt.Sprintf("%T", obj), "*", "", 1), "name", obj.GetName(), "namespace", obj.GetNamespace())
-	return b.Client().Create(ctx, obj)
+	return b.Client().Create(ctx, obj, opts...)
 }
 
-func (b *BaseReconciler) UpdateResource(ctx context.Context, obj client.Object) error {
+func (b *BaseReconciler) UpdateResource(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error {
 	logger, _ := logr.FromContext(ctx)
 	logger.Info("update object", "kind", strings.Replace(fmt.Sprintf("%T", obj), "*", "", 1), "name", obj.GetName(), "namespace", obj.GetNamespace())
-	return b.Client().Update(ctx, obj)
+	return b.Client().Update(ctx, obj, opts...)
 }
 
-func (b *BaseReconciler) DeleteResource(ctx context.Context, obj client.Object, options ...client.DeleteOption) error {
+func (b *BaseReconciler) DeleteResource(ctx context.Context, obj client.Object, opts ...client.DeleteOption) error {
 	logger, _ := logr.FromContext(ctx)
 	logger.Info("delete object", "kind", strings.Replace(fmt.Sprintf("%T", obj), "*", "", 1), "name", obj.GetName(), "namespace", obj.GetNamespace())
-	return b.Client().Delete(ctx, obj, options...)
+	return b.Client().Delete(ctx, obj, opts...)
 }
 
 func (b *BaseReconciler) UpdateResourceStatus(ctx context.Context, obj client.Object) error {

pkg/reconcilers/base_reconciler_test.go

@@ -262,3 +262,92 @@ func TestBaseReconcilerDelete(t *testing.T) {
 		t.Fatal(err)
 	}
 }
+
+type FakeClient struct {
+	client.Client
+
+	UpdateCalledWithDryRun bool
+}
+
+func (f *FakeClient) Update(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error {
+	for _, opt := range opts {
+		if opt == client.DryRunAll {
+			f.UpdateCalledWithDryRun = true
+		}
+	}
+
+	return f.Client.Update(ctx, obj, opts...)
+}
+
+func TestBaseReconcilerWithDryRunFirst(t *testing.T) {
+	// Test that update with dry run first is done
+	var (
+		name      = "myConfigmap"
+		namespace = "operator-unittest"
+	)
+	baseCtx := context.Background()
+	ctx := logr.NewContext(baseCtx, log.Log)
+
+	s := runtime.NewScheme()
+	err := v1.AddToScheme(s)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	existingConfigmap := &v1.ConfigMap{
+		TypeMeta: metav1.TypeMeta{
+			APIVersion: "v1",
+			Kind:       "ConfigMap",
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      name,
+			Namespace: namespace,
+		},
+		Data: map[string]string{
+			"somekey": "somevalue",
+		},
+	}
+
+	// Objects to track in the fake client.
+	objs := []runtime.Object{existingConfigmap}
+
+	// Create a fake client to mock API calls.
+	fakeCL := fake.NewClientBuilder().WithScheme(s).WithRuntimeObjects(objs...).Build()
+	cl := &FakeClient{Client: fakeCL}
+	clientAPIReader := fake.NewClientBuilder().WithScheme(s).WithRuntimeObjects(objs...).Build()
+	recorder := record.NewFakeRecorder(10000)
+
+	baseReconciler := NewBaseReconciler(cl, s, clientAPIReader, log.Log, recorder)
+
+	desiredConfigmap := &v1.ConfigMap{
+		TypeMeta: metav1.TypeMeta{
+			APIVersion: "v1",
+			Kind:       "ConfigMap",
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      name,
+			Namespace: namespace,
+		},
+	}
+
+	customMutator := func(existingObj, desiredObj client.Object) (bool, error) {
+		existing, ok := existingObj.(*v1.ConfigMap)
+		if !ok {
+			return false, fmt.Errorf("%T is not a *v1.ConfigMap", existingObj)
+		}
+		if existing.Data == nil {
+			existing.Data = map[string]string{}
+		}
+		existing.Data["customKey"] = "customValue"
+		return true, nil
+	}
+
+	err = baseReconciler.ReconcileResource(ctx, &v1.ConfigMap{}, desiredConfigmap, customMutator, DryRunFirst)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if !cl.UpdateCalledWithDryRun {
+		t.Fatal("Dry run not called")
+	}
+}