Make use of unstructured when using SSA to prevent default values causing issues

controllers/cluster_controller.go

@@ -24,7 +24,10 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/fields"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/serializer/yaml"
 	"k8s.io/apimachinery/pkg/types"
 	"regexp"
 	"sigs.k8s.io/controller-runtime/pkg/builder"
@@ -41,6 +44,7 @@ import (
 
 	"github.com/FoundationDB/fdb-kubernetes-operator/internal"
 	"sigs.k8s.io/controller-runtime/pkg/predicate"
+	sigyaml "sigs.k8s.io/yaml"
 
 	fdbv1beta2 "github.com/FoundationDB/fdb-kubernetes-operator/api/v1beta2"
 	"github.com/FoundationDB/fdb-kubernetes-operator/pkg/podclient"
@@ -87,6 +91,7 @@ type FoundationDBClusterReconciler struct {
 	// ClusterLabelKeyForNodeTrigger if set will trigger a reconciliation for all FoundationDBClusters that host a Pod
 	// on the affected node.
 	ClusterLabelKeyForNodeTrigger string
+	decodingSerializer            runtime.Serializer
 }
 
 // NewFoundationDBClusterReconciler creates a new FoundationDBClusterReconciler with defaults.
@@ -95,6 +100,7 @@ func NewFoundationDBClusterReconciler(podLifecycleManager podmanager.PodLifecycl
 		PodLifecycleManager: podLifecycleManager,
 	}
 	r.PodClientProvider = r.newFdbPodClient
+	r.decodingSerializer = yaml.NewDecodingSerializer(unstructured.UnstructuredJSONScheme)
 
 	return r
 }
@@ -508,7 +514,7 @@ func (r *FoundationDBClusterReconciler) newFdbPodClient(cluster *fdbv1beta2.Foun
 // updateOrApply updates the status either with server-side apply or if disabled with the normal update call.
 func (r *FoundationDBClusterReconciler) updateOrApply(ctx context.Context, cluster *fdbv1beta2.FoundationDBCluster) error {
 	if r.ServerSideApply {
-		// TODO(johscheuer): We have to set the TypeMeta otherwise the Patch command will fail. This is the rudimentary
+		// We have to set the TypeMeta otherwise the Patch command will fail. This is the rudimentary
 		// support for server side apply which should be enough for the status use case. The controller runtime will
 		// add some additional support in the future: https://github.com/kubernetes-sigs/controller-runtime/issues/347.
 		patch := &fdbv1beta2.FoundationDBCluster{
@@ -523,7 +529,22 @@ func (r *FoundationDBClusterReconciler) updateOrApply(ctx context.Context, clust
 			Status: cluster.Status,
 		}
 
-		return r.Status().Patch(ctx, patch, client.Apply, client.FieldOwner("fdb-operator")) //, client.ForceOwnership)
+		// We are converting the patch into an *unstructured.Unstructured to remove fields that use a default value.
+		// If we are not doing this, empty (nil) fields will be evaluated as if they were set by the default value.
+		// In some previous testing we discovered some issues with that behaviour. With the *unstructured.Unstructured
+		// we make sure that only fields that are actually set will be applied.
+		outBytes, err := sigyaml.Marshal(patch)
+		if err != nil {
+			return err
+		}
+
+		unstructuredPatch := &unstructured.Unstructured{}
+		_, _, err = r.decodingSerializer.Decode(outBytes, nil, unstructuredPatch)
+		if err != nil {
+			return err
+		}
+
+		return r.Status().Patch(ctx, unstructuredPatch, client.Apply, client.FieldOwner("fdb-operator")) //, client.ForceOwnership)
 	}
 
 	return r.Status().Update(ctx, cluster)

e2e/Makefile

@@ -38,6 +38,13 @@ FEATURE_DNS?=false
 FEATURE_LOCALITIES?=false
 # Allows to specify the tag for a specific FDB version. Format is 7.1.57:7.1.57-testing,7.3.35:7.3.35-debugging
 FDB_VERSION_TAG_MAPPING?=
+# If the FEATURE_SERVER_SIDE_APPLY environment variable is not defined the test suite will be randomly enable (1) or disable (0)
+# the server side apply feature.
+ifndef FEATURE_SERVER_SIDE_APPLY
+  FEATURE_SERVER_SIDE_APPLY=$(shell shuf -i 0-1 -n 1)
+else
+  FEATURE_SERVER_SIDE_APPLY=false
+endif
 
 # Make bash pickier about errors.
 SHELL=/bin/bash -euo pipefail
@@ -136,4 +143,5 @@ nightly-tests: run
 	  --storage-engine=$(STORAGE_ENGINE) \
 	  --fdb-version-tag-mapping=$(FDB_VERSION_TAG_MAPPING) \
 	  --unified-fdb-image=$(UNIFIED_FDB_IMAGE) \
+	  --feature-server-side-apply=$(FEATURE_SERVER_SIDE_APPLY) \
 	  | grep -v 'constructing many client instances from the same exec auth config can cause performance problems during cert rotation' &> $(BASE_DIR)/../logs/$<.log

e2e/fixtures/fdb_operator_client.go

@@ -485,7 +485,9 @@ spec:
           - --max-concurrent-reconciles=5
           - --zap-log-level=debug
           - --minimum-required-uptime-for-cc-bounce=60s
-          #- --server-side-apply
+{{ if .EnableServerSideApply }}
+          - --server-side-apply
+{{ end }}
           # We are setting low values here as the e2e test are taking down processes multiple times
           # and having a high wait time between recoveries will increase the reliability of the cluster but also
           # increase the time our e2e test take.
@@ -518,6 +520,8 @@ type operatorConfig struct {
 	MemoryRequests string
 	// Defines the user that runs the current e2e tests.
 	User string
+	// EnableServerSideApply if true, the operator will make use of server side apply.
+	EnableServerSideApply bool
 }
 
 // SidecarConfig represents the configuration for a sidecar. This can be used for templating.
@@ -603,15 +607,16 @@ func (factory *Factory) getOperatorConfig(namespace string) *operatorConfig {
 	}
 
 	return &operatorConfig{
-		OperatorImage:    factory.GetOperatorImage(),
-		SecretName:       factory.GetSecretName(),
-		BackupSecretName: factory.GetBackupSecretName(),
-		Namespace:        namespace,
-		SidecarVersions:  factory.GetSidecarConfigs(),
-		ImagePullPolicy:  factory.getImagePullPolicy(),
-		CPURequests:      cpuRequests,
-		MemoryRequests:   MemoryRequests,
-		User:             factory.options.username,
+		OperatorImage:         factory.GetOperatorImage(),
+		SecretName:            factory.GetSecretName(),
+		BackupSecretName:      factory.GetBackupSecretName(),
+		Namespace:             namespace,
+		SidecarVersions:       factory.GetSidecarConfigs(),
+		ImagePullPolicy:       factory.getImagePullPolicy(),
+		CPURequests:           cpuRequests,
+		MemoryRequests:        MemoryRequests,
+		User:                  factory.options.username,
+		EnableServerSideApply: factory.options.featureOperatorServerSideApply,
 	}
 }
 

e2e/fixtures/options.go

@@ -31,30 +31,31 @@ import (
 
 // FactoryOptions defines the (command line) options that are support for the e2e test cases.
 type FactoryOptions struct {
-	namespace                   string
-	chaosNamespace              string
-	context                     string
-	fdbImage                    string
-	unifiedFDBImage             string
-	sidecarImage                string
-	operatorImage               string
-	dataLoaderImage             string
-	registry                    string
-	fdbVersion                  string
-	username                    string
-	storageClass                string
-	upgradeString               string
-	cloudProvider               string
-	clusterName                 string
-	storageEngine               string
-	fdbVersionTagMapping        string
-	enableChaosTests            bool
-	enableDataLoading           bool
-	cleanup                     bool
-	featureOperatorDNS          bool
-	featureOperatorLocalities   bool
-	featureOperatorUnifiedImage bool
-	dumpOperatorState           bool
+	namespace                      string
+	chaosNamespace                 string
+	context                        string
+	fdbImage                       string
+	unifiedFDBImage                string
+	sidecarImage                   string
+	operatorImage                  string
+	dataLoaderImage                string
+	registry                       string
+	fdbVersion                     string
+	username                       string
+	storageClass                   string
+	upgradeString                  string
+	cloudProvider                  string
+	clusterName                    string
+	storageEngine                  string
+	fdbVersionTagMapping           string
+	enableChaosTests               bool
+	enableDataLoading              bool
+	cleanup                        bool
+	featureOperatorDNS             bool
+	featureOperatorLocalities      bool
+	featureOperatorUnifiedImage    bool
+	featureOperatorServerSideApply bool
+	dumpOperatorState              bool
 }
 
 // BindFlags binds the FactoryOptions flags to the provided FlagSet. This can be used to extend the current test setup
@@ -192,6 +193,12 @@ func (options *FactoryOptions) BindFlags(fs *flag.FlagSet) {
 		true,
 		"defines if the operator tests should print the state of the cluster and the according Pods for better debugging.",
 	)
+	fs.BoolVar(
+		&options.featureOperatorServerSideApply,
+		"feature-server-side-apply",
+		true, // TODO change to false.
+		"defines if the operator should make use of server side apply.",
+	)
 	fs.StringVar(&options.clusterName,
 		"cluster-name",
 		"",