targetconfigcontroller: add special merges for admission plugins

The resource merge does not merge slice contents, it replaces them;
therefore we need a special merge for the --enable-admission-plugins
and --disable-admission-plugins arguments.

Author: liouk

pkg/operator/targetconfigcontroller/targetconfigcontroller.go

@@ -34,6 +34,7 @@ import (
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/client-go/informers"
 	"k8s.io/client-go/kubernetes"
 	coreclientv1 "k8s.io/client-go/kubernetes/typed/core/v1"
@@ -266,7 +267,13 @@ func manageKubeAPIServerConfig(ctx context.Context, client coreclientv1.ConfigMa
 	configMap := resourceread.ReadConfigMapV1OrDie(bindata.MustAsset("assets/kube-apiserver/cm.yaml"))
 	defaultConfig := bindata.MustAsset("assets/config/defaultconfig.yaml")
 	configOverrides := bindata.MustAsset("assets/config/config-overrides.yaml")
-	specialMergeRules := map[string]resourcemerge.MergeFunc{}
+
+	// resourcemerge will not merge slice contents; therefore we must set up special merge rules for
+	// enable-admission-plugins/disable-admission-plugins in order to merge the config from the various sources
+	specialMergeRules := map[string]resourcemerge.MergeFunc{
+		".apiServerArguments.enable-admission-plugins":  mergeStringSlices,
+		".apiServerArguments.disable-admission-plugins": mergeStringSlices,
+	}
 
 	// Guarantee the authorization-mode will be present in the base config, regardless of whether the observer is running
 	authModeOverride := map[string]interface{}{}
@@ -618,3 +625,42 @@ func manageTemplate(rawTemplate string, imagePullSpec string, operatorImagePullS
 	}
 	return buf.String(), nil
 }
+
+func mergeStringSlices(dst, src any, _ string) (any, error) {
+	if src == nil {
+		return dst, nil
+	}
+
+	if dst == nil {
+		return src, nil
+	}
+
+	dstSlice, dstIsSlice := dst.([]any)
+	if !dstIsSlice {
+		return nil, fmt.Errorf("destination is not a slice (type: %T)", dst)
+	}
+
+	srcSlice, srcIsSlice := src.([]any)
+	if !srcIsSlice {
+		return nil, fmt.Errorf("source is not a slice (type: %T)", dst)
+	}
+
+	values := sets.NewString()
+	for _, elem := range dstSlice {
+		s, ok := elem.(string)
+		if !ok {
+			return nil, fmt.Errorf("destination slice element is not a string (type: %T)", elem)
+		}
+		values.Insert(s)
+	}
+
+	for _, elem := range srcSlice {
+		s, ok := elem.(string)
+		if !ok {
+			return nil, fmt.Errorf("source slice element is not a string (type: %T)", elem)
+		}
+		values.Insert(s)
+	}
+
+	return values.List(), nil
+}

pkg/operator/targetconfigcontroller/targetconfigcontroller_test.go

@@ -7,14 +7,21 @@ import (
 	"strings"
 	"testing"
 
-	operatorv1 "github.com/openshift/api/operator/v1"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/equality"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/util/diff"
 	"k8s.io/client-go/kubernetes/fake"
 	"k8s.io/client-go/kubernetes/scheme"
 	corev1listers "k8s.io/client-go/listers/core/v1"
+
+	"github.com/ghodss/yaml"
+	kubecontrolplanev1 "github.com/openshift/api/kubecontrolplane/v1"
+	operatorv1 "github.com/openshift/api/operator/v1"
+	"github.com/openshift/library-go/pkg/operator/resource/resourcemerge"
+	"github.com/stretchr/testify/require"
 )
 
 var codec = scheme.Codecs.LegacyCodec(scheme.Scheme.PrioritizedVersionsAllGroups()...)
@@ -417,6 +424,142 @@ func TestIsRequiredConfigPresentEtcdEndpoints(t *testing.T) {
 	}
 }
 
+func TestSpecialMergeRules(t *testing.T) {
+	mergeRules := map[string]resourcemerge.MergeFunc{
+		".apiServerArguments.enable-admission-plugins":  mergeStringSlices,
+		".apiServerArguments.disable-admission-plugins": mergeStringSlices,
+	}
+
+	configsToMerge := []*kubecontrolplanev1.KubeAPIServerConfig{
+		{
+			APIServerArguments: map[string]kubecontrolplanev1.Arguments{
+				"audit-log-format":          []string{"json"},
+				"enable-admission-plugins":  []string{"enabled0"},
+				"disable-admission-plugins": []string{"disabled0"},
+			},
+		},
+		{
+			APIServerArguments: map[string]kubecontrolplanev1.Arguments{
+				"audit-log-format":          []string{"yaml"},
+				"enable-admission-plugins":  []string{"enabled1"},
+				"disable-admission-plugins": []string{"disabled1"},
+			},
+		},
+		{
+			APIServerArguments: map[string]kubecontrolplanev1.Arguments{
+				"enable-admission-plugins":  []string{"enabled2"},
+				"disable-admission-plugins": []string{"disabled2"},
+			},
+		},
+	}
+
+	configs := make([][]byte, 0, len(configsToMerge))
+	for _, cfg := range configsToMerge {
+		cfgBytes, err := yaml.Marshal(cfg)
+		require.NoError(t, err)
+		configs = append(configs, cfgBytes)
+	}
+
+	result, _, err := resourcemerge.MergePrunedConfigMap(
+		&kubecontrolplanev1.KubeAPIServerConfig{},
+		&corev1.ConfigMap{Data: map[string]string{"config.yaml": ""}},
+		"config.yaml",
+		mergeRules,
+		configs...,
+	)
+	require.NoError(t, err)
+
+	config := &kubecontrolplanev1.KubeAPIServerConfig{}
+	err = yaml.Unmarshal([]byte(result.Data["config.yaml"]), config)
+	require.NoError(t, err)
+
+	// plugins have special merge rules, therefore slices must be merged
+	require.ElementsMatch(t, config.APIServerArguments["enable-admission-plugins"], []string{"enabled0", "enabled1", "enabled2"})
+	require.ElementsMatch(t, config.APIServerArguments["disable-admission-plugins"], []string{"disabled0", "disabled1", "disabled2"})
+
+	// audit-log-format does not have any special merge rules, therefore value gets replaced
+	require.ElementsMatch(t, config.APIServerArguments["audit-log-format"], []string{"yaml"})
+}
+
+func TestMergeStringSlices(t *testing.T) {
+	for _, tt := range []struct {
+		name        string
+		dst         any
+		src         any
+		expected    any
+		expectError bool
+	}{
+		{
+			name:        "dst and src empty",
+			dst:         nil,
+			src:         nil,
+			expected:    nil,
+			expectError: false,
+		},
+		{
+			name:        "src empty",
+			dst:         []any{"value"},
+			src:         nil,
+			expected:    []any{"value"},
+			expectError: false,
+		},
+		{
+			name:        "dst empty",
+			dst:         nil,
+			src:         []any{"value"},
+			expected:    []any{"value"},
+			expectError: false,
+		},
+		{
+			name:        "dst not a slice",
+			dst:         "not-a-slice",
+			src:         []any{"new-item"},
+			expected:    nil,
+			expectError: true,
+		},
+		{
+			name:        "src not a slice",
+			dst:         []any{"existing-item"},
+			src:         "not-a-slice",
+			expected:    nil,
+			expectError: true,
+		},
+		{
+			name:        "dst not a string slice",
+			dst:         []any{1, 2, 3},
+			src:         []any{"new-item"},
+			expected:    nil,
+			expectError: true,
+		},
+		{
+			name:        "src not a string slice",
+			dst:         []any{"existing-item"},
+			src:         []any{1, 2, 3},
+			expected:    nil,
+			expectError: true,
+		},
+		{
+			name:        "dst and src merged",
+			dst:         []any{"existing-item"},
+			src:         []any{"new-item"},
+			expected:    []string{"existing-item", "new-item"},
+			expectError: false,
+		},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			merged, err := mergeStringSlices(tt.dst, tt.src, "")
+			if tt.expectError != (err != nil) {
+				t.Errorf("expected error: %v; got %v", tt.expectError, err)
+			}
+
+			if !equality.Semantic.DeepEqual(tt.expected, merged) {
+				t.Errorf("unexpected merged slice: %s", diff.ObjectReflectDiff(tt.expected, merged))
+			}
+
+		})
+	}
+}
+
 func makeEtcdEndpointsCM(endpoints ...string) *corev1.ConfigMap {
 	cm := &corev1.ConfigMap{}
 	cm.Name = etcdEndpointName