diff --git a/Makefile b/Makefile index c97477d33..313e4d4f0 100644 --- a/Makefile +++ b/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) @@ -383,7 +383,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 diff --git a/controllers/authpolicy_authconfig.go b/controllers/authpolicy_authconfig.go index 0b1ba8338..9c76bad2b 100644 --- a/controllers/authpolicy_authconfig.go +++ b/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, authConfigBasicMutator) + // 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, authConfigBasicMutator, reconcilers.DryRunFirst) if err != nil && !apierrors.IsAlreadyExists(err) { logger.Error(err, "ReconcileResource failed to create/update AuthConfig resource") return err @@ -511,6 +514,5 @@ func authConfigBasicMutator(existingObj, desiredObj client.Object) (bool, error) } existing.Spec = desired.Spec - return true, nil } diff --git a/controllers/suite_test.go b/controllers/suite_test.go index 9f8ac9f30..08d7a8e06 100644 --- a/controllers/suite_test.go +++ b/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" istiov1alpha1 "maistra.io/istio-operator/api/v1alpha1" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -72,6 +73,7 @@ var _ = BeforeSuite(func() { testEnv = &envtest.Environment{ CRDDirectoryPaths: []string{filepath.Join("..", "config", "crd", "bases")}, ErrorIfCRDPathMissing: true, + UseExistingCluster: ptr.To(true), } cfg, err := testEnv.Start() diff --git a/pkg/reconcilers/base_reconciler.go b/pkg/reconcilers/base_reconciler.go index addd9f095..33223ad7c 100644 --- a/pkg/reconcilers/base_reconciler.go +++ b/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 = 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 { diff --git a/pkg/reconcilers/base_reconciler_test.go b/pkg/reconcilers/base_reconciler_test.go index 51d0845cc..b1b35a87b 100644 --- a/pkg/reconcilers/base_reconciler_test.go +++ b/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") + } +}