Merge pull request openshift#878 from zeeke/us/OCPBUGS-53346

controllers: Use patch to add finalizers

Author: SchSeba

controllers/sriovoperatorconfig_controller.go

@@ -85,7 +85,7 @@ func (r *SriovOperatorConfigReconciler) Reconcile(ctx context.Context, req ctrl.
 	if err != nil {
 		if apierrors.IsNotFound(err) {
 			logger.Info("default SriovOperatorConfig object not found. waiting for creation.")
-			return reconcile.Result{}, err
+			return reconcile.Result{}, nil
 		}
 		// Error reading the object - requeue the request.
 		logger.Error(err, "Failed to get default SriovOperatorConfig object")
@@ -99,12 +99,9 @@ func (r *SriovOperatorConfigReconciler) Reconcile(ctx context.Context, req ctrl.
 		// The object is being deleted
 		return r.handleSriovOperatorConfigDeletion(ctx, defaultConfig, logger)
 	}
-	// add finalizer if needed
-	if !sriovnetworkv1.StringInArray(sriovnetworkv1.OPERATORCONFIGFINALIZERNAME, defaultConfig.ObjectMeta.Finalizers) {
-		defaultConfig.ObjectMeta.Finalizers = append(defaultConfig.ObjectMeta.Finalizers, sriovnetworkv1.OPERATORCONFIGFINALIZERNAME)
-		if err := r.Update(ctx, defaultConfig); err != nil {
-			return reconcile.Result{}, err
-		}
+
+	if err = r.syncOperatorConfigFinalizers(ctx, defaultConfig, logger); err != nil {
+		return reconcile.Result{}, err
 	}
 
 	r.FeatureGate.Init(defaultConfig.Spec.FeatureGates)
@@ -448,6 +445,29 @@ func (r *SriovOperatorConfigReconciler) syncOpenShiftSystemdService(ctx context.
 	return r.setLabelInsideObject(ctx, cr, objs)
 }
 
+func (r SriovOperatorConfigReconciler) syncOperatorConfigFinalizers(ctx context.Context, defaultConfig *sriovnetworkv1.SriovOperatorConfig, logger logr.Logger) error {
+	if sriovnetworkv1.StringInArray(sriovnetworkv1.OPERATORCONFIGFINALIZERNAME, defaultConfig.ObjectMeta.Finalizers) {
+		return nil
+	}
+
+	newObj := defaultConfig.DeepCopyObject().(client.Object)
+	newObj.SetFinalizers(
+		append(newObj.GetFinalizers(), sriovnetworkv1.OPERATORCONFIGFINALIZERNAME),
+	)
+
+	logger.WithName("syncOperatorConfigFinalizers").
+		Info("Adding finalizer", "key", sriovnetworkv1.OPERATORCONFIGFINALIZERNAME)
+
+	patch := client.MergeFrom(defaultConfig)
+	err := r.Patch(ctx, newObj, patch)
+	if err != nil {
+		return fmt.Errorf("can't patch SriovOperatorConfig to add finalizer [%s]: %w", sriovnetworkv1.OPERATORCONFIGFINALIZERNAME, err)
+	}
+
+	// Refresh the defaultConfig object with the latest changes
+	return r.Get(ctx, types.NamespacedName{Namespace: defaultConfig.Namespace, Name: defaultConfig.Name}, defaultConfig)
+}
+
 func (r *SriovOperatorConfigReconciler) handleSriovOperatorConfigDeletion(ctx context.Context,
 	defaultConfig *sriovnetworkv1.SriovOperatorConfig, logger logr.Logger) (ctrl.Result, error) {
 	var err error

controllers/sriovoperatorconfig_controller_test.go

@@ -30,6 +30,8 @@ import (
 	"github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/platforms/openshift"
 	"github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/vars"
 	"github.com/k8snetworkplumbingwg/sriov-network-operator/test/util"
+
+	uns "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 )
 
 var _ = Describe("SriovOperatorConfig controller", Ordered, func() {
@@ -117,21 +119,23 @@ var _ = Describe("SriovOperatorConfig controller", Ordered, func() {
 		BeforeEach(func() {
 			var err error
 			config := &sriovnetworkv1.SriovOperatorConfig{}
-			err = util.WaitForNamespacedObject(config, k8sClient, testNamespace, "default", util.RetryInterval, util.APITimeout)
-			Expect(err).NotTo(HaveOccurred())
-			// in case controller yet to add object's finalizer (e.g whenever test deferCleanup is creating new 'default' config object)
-			if len(config.Finalizers) == 0 {
+
+			Eventually(func(g Gomega) {
 				err = util.WaitForNamespacedObject(config, k8sClient, testNamespace, "default", util.RetryInterval, util.APITimeout)
+				g.Expect(err).NotTo(HaveOccurred())
+				// in case controller yet to add object's finalizer (e.g whenever test deferCleanup is creating new 'default' config object)
+				g.Expect(config.Finalizers).ToNot(BeEmpty())
+
+				config.Spec = sriovnetworkv1.SriovOperatorConfigSpec{
+					EnableInjector:        true,
+					EnableOperatorWebhook: true,
+					LogLevel:              2,
+					FeatureGates:          map[string]bool{},
+				}
+				err = k8sClient.Update(ctx, config)
 				Expect(err).NotTo(HaveOccurred())
-			}
-			config.Spec = sriovnetworkv1.SriovOperatorConfigSpec{
-				EnableInjector:        true,
-				EnableOperatorWebhook: true,
-				LogLevel:              2,
-				FeatureGates:          map[string]bool{},
-			}
-			err = k8sClient.Update(ctx, config)
-			Expect(err).NotTo(HaveOccurred())
+			}).Should(Succeed())
+
 		})
 
 		It("should have webhook enable", func() {
@@ -291,6 +295,46 @@ var _ = Describe("SriovOperatorConfig controller", Ordered, func() {
 			}, util.APITimeout, util.RetryInterval).Should(Equal(empty))
 		})
 
+		It("should not remove fields with default values when SriovOperatorConfig is created", func() {
+			err := k8sClient.Delete(context.Background(), &sriovnetworkv1.SriovOperatorConfig{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: consts.DefaultConfigName},
+			})
+			Expect(err).NotTo(HaveOccurred())
+
+			config := &uns.Unstructured{}
+			config.SetGroupVersionKind(sriovnetworkv1.GroupVersion.WithKind("SriovOperatorConfig"))
+			config.SetName(consts.DefaultConfigName)
+			config.SetNamespace(testNamespace)
+			config.Object["spec"] = map[string]interface{}{
+				"enableInjector":        false,
+				"enableOperatorWebhook": false,
+				"logLevel":              0,
+				"disableDrain":          false,
+			}
+
+			Eventually(func() error {
+				return k8sClient.Create(context.Background(), config)
+			}).Should(Succeed())
+
+			By("Wait for the operator to reconcile the object")
+			Eventually(func(g Gomega) {
+				err := k8sClient.Get(context.Background(), types.NamespacedName{Namespace: testNamespace, Name: consts.DefaultConfigName}, config)
+				g.Expect(err).ToNot(HaveOccurred())
+				g.Expect(config.GetFinalizers()).To(ContainElement(sriovnetworkv1.OPERATORCONFIGFINALIZERNAME))
+			}, util.APITimeout, util.RetryInterval).Should(Succeed())
+
+			By("Verify default values have not been omitted")
+			obj := &uns.Unstructured{}
+			obj.SetGroupVersionKind(sriovnetworkv1.GroupVersion.WithKind("SriovOperatorConfig"))
+			err = k8sClient.Get(context.Background(), types.NamespacedName{Namespace: testNamespace, Name: consts.DefaultConfigName}, obj)
+			Expect(err).NotTo(HaveOccurred())
+
+			Expect(obj.Object["spec"]).To(HaveKeyWithValue("enableInjector", false))
+			Expect(obj.Object["spec"]).To(HaveKeyWithValue("enableOperatorWebhook", false))
+			Expect(obj.Object["spec"]).To(HaveKeyWithValue("logLevel", int64(0)))
+			Expect(obj.Object["spec"]).To(HaveKeyWithValue("disableDrain", false))
+		})
+
 		It("should be able to update the node selector of sriov-network-config-daemon", func() {
 			By("specify the configDaemonNodeSelector")
 			nodeSelector := map[string]string{"node-role.kubernetes.io/worker": ""}