Review comments

openshift · Aug 10, 2023 · 5361ad6 · 5361ad6
1 parent 7113150
commit 5361ad6
Show file tree

Hide file tree

Showing 3 changed files with 45 additions and 50 deletions.
diff --git a/pkg/operator/starter.go b/pkg/operator/starter.go
@@ -10,7 +10,7 @@ import (
 	operatorv1 "github.com/openshift/api/operator/v1"
 	configv1client "github.com/openshift/client-go/config/clientset/versioned"
 	configinformers "github.com/openshift/client-go/config/informers/externalversions"
-	configinformersconfigv1 "github.com/openshift/client-go/config/informers/externalversions/config/v1"
+	configlisterv1 "github.com/openshift/client-go/config/listers/config/v1"
 	operatorv1client "github.com/openshift/client-go/operator/clientset/versioned"
 	operatorv1informers "github.com/openshift/client-go/operator/informers/externalversions"
 	operatorcontrolplaneclient "github.com/openshift/client-go/operatorcontrolplane/clientset/versioned"
@@ -171,7 +171,7 @@ func RunOperator(ctx context.Context, controllerConfig *controllercmd.Controller
 		operatorClient,
 		operatorConfigClient.OperatorV1(),
 		configClient.ConfigV1(),
-		configInformers.Config().V1().ClusterVersions(),
+		configInformers.Config().V1().ClusterVersions().Lister(),
 		workloadcontroller.CountNodesFuncWrapper(kubeInformersForNamespaces.InformersFor("").Core().V1().Nodes().Lister()),
 		workloadcontroller.EnsureAtMostOnePodPerNode,
 		"openshift-apiserver",
@@ -197,7 +197,7 @@ func RunOperator(ctx context.Context, controllerConfig *controllercmd.Controller
 	).WithAPIServiceController(
 		"openshift-apiserver",
 		func() ([]*apiregistrationv1.APIService, []*apiregistrationv1.APIService, error) {
-			return apiServices(configInformers.Config().V1().ClusterVersions())
+			return apiServices(configInformers.Config().V1().ClusterVersions().Lister())
 		},
 		apiregistrationInformers,
 		apiregistrationv1Client.ApiregistrationV1(),
@@ -401,8 +401,8 @@ func RunOperator(ctx context.Context, controllerConfig *controllercmd.Controller
 	return nil
 }
 
-func apiServices(clusterVersionInformer configinformersconfigv1.ClusterVersionInformer) ([]*apiregistrationv1.APIService, []*apiregistrationv1.APIService, error) {
-	clusterVersion, err := clusterVersionInformer.Lister().Get("version")
+func apiServices(clusterVersionLister configlisterv1.ClusterVersionLister) ([]*apiregistrationv1.APIService, []*apiregistrationv1.APIService, error) {
+	clusterVersion, err := clusterVersionLister.Get("version")
 	if err != nil {
 		return nil, nil, err
 	}

diff --git a/pkg/operator/workload/workload_openshiftapiserver_v311_00_sync.go b/pkg/operator/workload/workload_openshiftapiserver_v311_00_sync.go
@@ -31,7 +31,7 @@ import (
 	openshiftcontrolplanev1 "github.com/openshift/api/openshiftcontrolplane/v1"
 	operatorv1 "github.com/openshift/api/operator/v1"
 	openshiftconfigclientv1 "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1"
-	configinformersconfigv1 "github.com/openshift/client-go/config/informers/externalversions/config/v1"
+	configlisterv1 "github.com/openshift/client-go/config/listers/config/v1"
 	operatorv1client "github.com/openshift/client-go/operator/clientset/versioned/typed/operator/v1"
 	"github.com/openshift/cluster-openshift-apiserver-operator/pkg/operator/operatorclient"
 	"github.com/openshift/cluster-openshift-apiserver-operator/pkg/operator/v311_00_assets"
@@ -58,11 +58,11 @@ type ensureAtMostOnePodPerNodeFunc func(spec *appsv1.DeploymentSpec, component s
 
 // OpenShiftAPIServerWorkload is a struct that holds necessary data to install OpenShiftAPIServer
 type OpenShiftAPIServerWorkload struct {
-	operatorClient         v1helpers.OperatorClient
-	operatorConfigClient   operatorv1client.OpenShiftAPIServersGetter
-	openshiftConfigClient  openshiftconfigclientv1.ConfigV1Interface
-	clusterVersionInformer configinformersconfigv1.ClusterVersionInformer
-	kubeClient             kubernetes.Interface
+	operatorClient        v1helpers.OperatorClient
+	operatorConfigClient  operatorv1client.OpenShiftAPIServersGetter
+	openshiftConfigClient openshiftconfigclientv1.ConfigV1Interface
+	clusterVersionLister  configlisterv1.ClusterVersionLister
+	kubeClient            kubernetes.Interface
 
 	// countNodes a function to return count of nodes on which the workload will be installed
 	countNodes nodeCountFunc
@@ -83,7 +83,7 @@ func NewOpenShiftAPIServerWorkload(
 	operatorClient v1helpers.OperatorClient,
 	operatorConfigClient operatorv1client.OpenShiftAPIServersGetter,
 	openshiftConfigClient openshiftconfigclientv1.ConfigV1Interface,
-	clusterVersionInformer configinformersconfigv1.ClusterVersionInformer,
+	clusterVersionLister configlisterv1.ClusterVersionLister,
 	countNodes nodeCountFunc,
 	ensureAtMostOnePodPerNode ensureAtMostOnePodPerNodeFunc,
 	targetNamespace string,
@@ -96,7 +96,7 @@ func NewOpenShiftAPIServerWorkload(
 		operatorClient:            operatorClient,
 		operatorConfigClient:      operatorConfigClient,
 		openshiftConfigClient:     openshiftConfigClient,
-		clusterVersionInformer:    clusterVersionInformer,
+		clusterVersionLister:      clusterVersionLister,
 		countNodes:                countNodes,
 		ensureAtMostOnePodPerNode: ensureAtMostOnePodPerNode,
 		targetNamespace:           targetNamespace,
@@ -153,7 +153,7 @@ func (c *OpenShiftAPIServerWorkload) Sync(ctx context.Context, syncContext facto
 	}
 	operatorConfig := originalOperatorConfig.DeepCopy()
 
-	_, _, err = manageOpenShiftAPIServerConfigMap_v311_00_to_latest(ctx, c.kubeClient.CoreV1(), c.clusterVersionInformer, syncContext.Recorder(), operatorConfig)
+	_, _, err = manageOpenShiftAPIServerConfigMap_v311_00_to_latest(ctx, c.kubeClient.CoreV1(), c.clusterVersionLister, syncContext.Recorder(), operatorConfig)
 	if err != nil {
 		errors = append(errors, fmt.Errorf("%q: %v", "configmap", err))
 	}
@@ -279,11 +279,11 @@ func manageOpenShiftAPIServerImageImportCA_v311_00_to_latest(ctx context.Context
 	return resourceapply.ApplyConfigMap(ctx, client, recorder, requiredConfigMap)
 }
 
-func manageOpenShiftAPIServerConfigMap_v311_00_to_latest(ctx context.Context, client coreclientv1.ConfigMapsGetter, clusterVersionInformer configinformersconfigv1.ClusterVersionInformer, recorder events.Recorder, operatorConfig *operatorv1.OpenShiftAPIServer) (*corev1.ConfigMap, bool, error) {
+func manageOpenShiftAPIServerConfigMap_v311_00_to_latest(ctx context.Context, client coreclientv1.ConfigMapsGetter, clusterVersionLister configlisterv1.ClusterVersionLister, recorder events.Recorder, operatorConfig *operatorv1.OpenShiftAPIServer) (*corev1.ConfigMap, bool, error) {
 	configMap := resourceread.ReadConfigMapV1OrDie(v311_00_assets.MustAsset("v3.11.0/openshift-apiserver/cm.yaml"))
 	defaultConfig := v311_00_assets.MustAsset("v3.11.0/config/defaultconfig.yaml")
 
-	clusterVersion, err := clusterVersionInformer.Lister().Get("version")
+	clusterVersion, err := clusterVersionLister.Get("version")
 	if err != nil {
 		return nil, false, err
 	}
@@ -307,7 +307,7 @@ func manageOpenShiftAPIServerConfigMap_v311_00_to_latest(ctx context.Context, cl
 		apiServersConfig.APIServers.PerGroupOptions = append(apiServersConfig.APIServers.PerGroupOptions, openshiftcontrolplanev1.PerGroupOptions{Name: openshiftcontrolplanev1.OpenShiftAppsAPIserver, DisabledVersions: []string{"v1"}})
 	}
 
-	bytes, err := json.Marshal(apiServersConfig)
+	bytes, err := yaml.Marshal(apiServersConfig)
 	if err != nil {
 		return nil, false, fmt.Errorf("unable to marshal OpenShiftAPIServerConfig struct: %v", err)
 	}

diff --git a/pkg/operator/workload/workload_openshiftapiserver_v311_00_sync_test.go b/pkg/operator/workload/workload_openshiftapiserver_v311_00_sync_test.go
@@ -1,17 +1,14 @@
 package workload
 
 import (
-	"bytes"
 	"context"
-	"encoding/json"
 	"testing"
-	"time"
 
 	configv1 "github.com/openshift/api/config/v1"
 	openshiftcontrolplanev1 "github.com/openshift/api/openshiftcontrolplane/v1"
 	operatorv1 "github.com/openshift/api/operator/v1"
 	configfake "github.com/openshift/client-go/config/clientset/versioned/fake"
-	configinformers "github.com/openshift/client-go/config/informers/externalversions"
+	configlistersv1 "github.com/openshift/client-go/config/listers/config/v1"
 	operatorfake "github.com/openshift/client-go/operator/clientset/versioned/fake"
 	"github.com/openshift/cluster-openshift-apiserver-operator/pkg/operator/operatorclient"
 	"github.com/openshift/library-go/pkg/controller/factory"
@@ -24,8 +21,11 @@ import (
 	"k8s.io/apimachinery/pkg/api/equality"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/serializer"
 	"k8s.io/apimachinery/pkg/util/diff"
+	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	"k8s.io/client-go/kubernetes/fake"
+	"k8s.io/client-go/tools/cache"
 )
 
 func fakeCountNodes(_ map[string]string) (*int32, error) {
@@ -105,27 +105,21 @@ func TestOperatorConfigProgressingCondition(t *testing.T) {
 			)
 
 			fakeOperatorClient := operatorv1helpers.NewFakeOperatorClient(&operatorv1.OperatorSpec{ManagementState: operatorv1.Managed}, &operatorv1.OperatorStatus{}, nil)
-			configInformers := configinformers.NewSharedInformerFactory(openshiftConfigClient, 10*time.Minute)
+			indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{})
+			indexer.Add(&configv1.ClusterVersion{ObjectMeta: metav1.ObjectMeta{Name: "version"}})
 
 			target := OpenShiftAPIServerWorkload{
 				kubeClient:                kubeClient,
 				operatorClient:            fakeOperatorClient,
 				operatorConfigClient:      apiServiceOperatorClient.OperatorV1(),
 				openshiftConfigClient:     openshiftConfigClient.ConfigV1(),
-				clusterVersionInformer:    configInformers.Config().V1().ClusterVersions(),
+				clusterVersionLister:      configlistersv1.NewClusterVersionLister(indexer),
 				versionRecorder:           status.NewVersionGetter(),
 				countNodes:                fakeCountNodes,
 				ensureAtMostOnePodPerNode: func(spec *appsv1.DeploymentSpec, componentName string) error { return nil },
 			}
 
-			// register the informer
-			configInformers.Config().V1().ClusterVersions().Informer()
-
-			ctx := context.Background()
-			configInformers.Start(ctx.Done())
-			configInformers.WaitForCacheSync(ctx.Done())
-
-			if _, _, err := target.Sync(ctx, factory.NewSyncContext("TestSyncCOntext", events.NewInMemoryRecorder(""))); len(err) > 0 {
+			if _, _, err := target.Sync(context.Background(), factory.NewSyncContext("TestSyncCOntext", events.NewInMemoryRecorder(""))); len(err) > 0 {
 				t.Fatal(err)
 			}
 
@@ -335,40 +329,37 @@ func TestCapabilities(t *testing.T) {
 				},
 			}
 			apiServiceOperatorClient := operatorfake.NewSimpleClientset(operatorConfig)
-			openshiftConfigClient := configfake.NewSimpleClientset(
-				&configv1.Image{ObjectMeta: metav1.ObjectMeta{Name: "cluster"}},
-				&configv1.ClusterVersion{
-					ObjectMeta: metav1.ObjectMeta{Name: "version"},
-					Status: configv1.ClusterVersionStatus{
-						Capabilities: configv1.ClusterVersionCapabilitiesStatus{
-							EnabledCapabilities: tc.enabledCapabilities,
-							KnownCapabilities:   tc.knownCapabilities,
-						},
+			clusterVersion := &configv1.ClusterVersion{
+				ObjectMeta: metav1.ObjectMeta{Name: "version"},
+				Status: configv1.ClusterVersionStatus{
+					Capabilities: configv1.ClusterVersionCapabilitiesStatus{
+						EnabledCapabilities: tc.enabledCapabilities,
+						KnownCapabilities:   tc.knownCapabilities,
 					},
 				},
+			}
+			openshiftConfigClient := configfake.NewSimpleClientset(
+				&configv1.Image{ObjectMeta: metav1.ObjectMeta{Name: "cluster"}},
+				clusterVersion,
 			)
 
 			fakeOperatorClient := operatorv1helpers.NewFakeOperatorClient(&operatorv1.OperatorSpec{ManagementState: operatorv1.Managed}, &operatorv1.OperatorStatus{}, nil)
-			configInformers := configinformers.NewSharedInformerFactory(openshiftConfigClient, 10*time.Minute)
+
+			indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{})
+			indexer.Add(clusterVersion)
 
 			target := OpenShiftAPIServerWorkload{
 				kubeClient:                kubeClient,
 				operatorClient:            fakeOperatorClient,
 				operatorConfigClient:      apiServiceOperatorClient.OperatorV1(),
 				openshiftConfigClient:     openshiftConfigClient.ConfigV1(),
-				clusterVersionInformer:    configInformers.Config().V1().ClusterVersions(),
+				clusterVersionLister:      configlistersv1.NewClusterVersionLister(indexer),
 				versionRecorder:           status.NewVersionGetter(),
 				countNodes:                fakeCountNodes,
 				ensureAtMostOnePodPerNode: func(spec *appsv1.DeploymentSpec, componentName string) error { return nil },
 			}
 
-			// register the informer
-			configInformers.Config().V1().ClusterVersions().Informer()
-
 			ctx := context.Background()
-			configInformers.Start(ctx.Done())
-			configInformers.WaitForCacheSync(ctx.Done())
-
 			if _, _, err := target.Sync(ctx, factory.NewSyncContext("TestSyncCOntext", events.NewInMemoryRecorder(""))); len(err) > 0 {
 				t.Fatal(err)
 			}
@@ -378,10 +369,14 @@ func TestCapabilities(t *testing.T) {
 				t.Fatalf("Unable to get 'config' configmap: %v", err)
 			}
 
-			config := openshiftcontrolplanev1.OpenShiftAPIServerConfig{}
-			if err := json.NewDecoder(bytes.NewBuffer([]byte(cm.Data["config.yaml"]))).Decode(&config); err != nil {
+			scheme := runtime.NewScheme()
+			utilruntime.Must(openshiftcontrolplanev1.Install(scheme))
+			codecs := serializer.NewCodecFactory(scheme)
+			obj, err := runtime.Decode(codecs.UniversalDecoder(openshiftcontrolplanev1.GroupVersion, configv1.GroupVersion), []byte(cm.Data["config.yaml"]))
+			if err != nil {
 				t.Fatalf("Unable to decode OpenShiftAPIServerConfig: %v", err)
 			}
+			config := obj.(*openshiftcontrolplanev1.OpenShiftAPIServerConfig)
 
 			if !equality.Semantic.DeepEqual(config.APIServers.PerGroupOptions, tc.expectedPerGroupOptions) {
 				t.Errorf("generation status mismatch, diff = %s", diff.ObjectDiff(config.APIServers.PerGroupOptions, tc.expectedPerGroupOptions))