Skip to content

Commit

Permalink
Review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@ingvagabund
ingvagabund committed Aug 11, 2023
1 parent 7113150 commit 2cc5428
Show file tree
Hide file tree
Showing 3 changed files with 110 additions and 66 deletions.
10 changes: 5 additions & 5 deletions pkg/operator/starter.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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",
Expand All @@ -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(),
Expand Down Expand Up @@ -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
}
Expand Down
41 changes: 22 additions & 19 deletions pkg/operator/workload/workload_openshiftapiserver_v311_00_sync.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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
Expand All @@ -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,
Expand All @@ -96,7 +96,7 @@ func NewOpenShiftAPIServerWorkload(
operatorClient: operatorClient,
operatorConfigClient: operatorConfigClient,
openshiftConfigClient: openshiftConfigClient,
clusterVersionInformer: clusterVersionInformer,
clusterVersionLister: clusterVersionLister,
countNodes: countNodes,
ensureAtMostOnePodPerNode: ensureAtMostOnePodPerNode,
targetNamespace: targetNamespace,
Expand Down Expand Up @@ -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))
}
Expand Down Expand Up @@ -279,35 +279,38 @@ 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
}

knownCaps := sets.New[configv1.ClusterVersionCapability](clusterVersion.Status.Capabilities.KnownCapabilities...)
capsEnabled := sets.New[configv1.ClusterVersionCapability](clusterVersion.Status.Capabilities.EnabledCapabilities...)

apiServersConfig := openshiftcontrolplanev1.OpenShiftAPIServerConfig{
APIServers: openshiftcontrolplanev1.APIServers{
PerGroupOptions: []openshiftcontrolplanev1.PerGroupOptions{},
},
apiServers := openshiftcontrolplanev1.APIServers{
PerGroupOptions: []openshiftcontrolplanev1.PerGroupOptions{},
}

if knownCaps.Has(configv1.ClusterVersionCapabilityBuild) && !capsEnabled.Has(configv1.ClusterVersionCapabilityBuild) {
klog.V(4).Infof("Capability %q not enabled, disabling 'openshift.io/build' controller", configv1.ClusterVersionCapabilityBuild)
apiServersConfig.APIServers.PerGroupOptions = append(apiServersConfig.APIServers.PerGroupOptions, openshiftcontrolplanev1.PerGroupOptions{Name: openshiftcontrolplanev1.OpenShiftBuildAPIserver, DisabledVersions: []string{"v1"}})
apiServers.PerGroupOptions = append(apiServers.PerGroupOptions, openshiftcontrolplanev1.PerGroupOptions{Name: openshiftcontrolplanev1.OpenShiftBuildAPIserver, DisabledVersions: []string{"v1"}})
}

if knownCaps.Has(configv1.ClusterVersionCapabilityDeploymentConfig) && !capsEnabled.Has(configv1.ClusterVersionCapabilityDeploymentConfig) {
klog.V(4).Infof("Capability %q not enabled, disabling 'openshift.io/apps' controller", configv1.ClusterVersionCapabilityDeploymentConfig)
apiServersConfig.APIServers.PerGroupOptions = append(apiServersConfig.APIServers.PerGroupOptions, openshiftcontrolplanev1.PerGroupOptions{Name: openshiftcontrolplanev1.OpenShiftAppsAPIserver, DisabledVersions: []string{"v1"}})
apiServers.PerGroupOptions = append(apiServers.PerGroupOptions, openshiftcontrolplanev1.PerGroupOptions{Name: openshiftcontrolplanev1.OpenShiftAppsAPIserver, DisabledVersions: []string{"v1"}})
}

bytes, err := json.Marshal(apiServers)
if err != nil {
return nil, false, fmt.Errorf("unable to marshal APIServers struct: %v", err)
}

bytes, err := json.Marshal(apiServersConfig)
configYaml, err := yaml.JSONToYAML([]byte(fmt.Sprintf("{\"apiVersion\": \"openshiftcontrolplane.config.openshift.io/v1\", \"kind\": \"OpenShiftAPIServerConfig\", \"apiServers\": %v}\n", string(bytes))))
if err != nil {
return nil, false, fmt.Errorf("unable to marshal OpenShiftAPIServerConfig struct: %v", err)
}
Expand All @@ -318,7 +321,7 @@ func manageOpenShiftAPIServerConfigMap_v311_00_to_latest(ctx context.Context, cl
"config.yaml",
nil,
defaultConfig,
bytes,
configYaml,
operatorConfig.Spec.ObservedConfig.Raw,
operatorConfig.Spec.UnsupportedConfigOverrides.Raw,
)
Expand Down
125 changes: 83 additions & 42 deletions pkg/operator/workload/workload_openshiftapiserver_v311_00_sync_test.go
View file
Original file line number Diff line number Diff line change
@@ -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"
Expand All @@ -24,8 +21,12 @@ 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"
clientgotesting "k8s.io/client-go/testing"
"k8s.io/client-go/tools/cache"
)

func fakeCountNodes(_ map[string]string) (*int32, error) {
Expand Down Expand Up @@ -70,7 +71,7 @@ func TestOperatorConfigProgressingCondition(t *testing.T) {
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {

kubeClient := fake.NewSimpleClientset(
fakeKubeClient := fake.NewSimpleClientset(
&corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "serving-cert", Namespace: "openshift-apiserver"}},
&corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "etcd-client", Namespace: operatorclient.GlobalUserSpecifiedConfigNamespace}},
&appsv1.Deployment{
Expand Down Expand Up @@ -105,27 +106,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,
kubeClient: fakeKubeClient,
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)
}

Expand Down Expand Up @@ -306,7 +301,7 @@ func TestCapabilities(t *testing.T) {
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {

kubeClient := fake.NewSimpleClientset(
fakeKubeClient := fake.NewSimpleClientset(
&corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "serving-cert", Namespace: "openshift-apiserver"}},
&corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "etcd-client", Namespace: operatorclient.GlobalUserSpecifiedConfigNamespace}},
&appsv1.Deployment{
Expand Down Expand Up @@ -335,56 +330,102 @@ 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,
kubeClient: fakeKubeClient,
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)
}

cm, err := kubeClient.CoreV1().ConfigMaps("openshift-apiserver").Get(ctx, "config", metav1.GetOptions{})
if err != nil {
t.Fatalf("Unable to get 'config' configmap: %v", err)
expecteOpenShiftAPIServerConfig := &openshiftcontrolplanev1.OpenShiftAPIServerConfig{
TypeMeta: metav1.TypeMeta{
Kind: "OpenShiftAPIServerConfig",
APIVersion: "openshiftcontrolplane.config.openshift.io/v1",
},
GenericAPIServerConfig: configv1.GenericAPIServerConfig{
ServingInfo: configv1.HTTPServingInfo{
ServingInfo: configv1.ServingInfo{
BindNetwork: "tcp",
},
},
StorageConfig: configv1.EtcdStorageConfig{
EtcdConnectionInfo: configv1.EtcdConnectionInfo{
URLs: []string{"https://etcd.openshift-etcd.svc:2379"},
},
},
},
APIServerArguments: map[string][]string{
"audit-log-format": {"json"},
"audit-log-maxbackup": {"10"},
"audit-log-maxsize": {"100"},
"audit-log-path": {"/var/log/openshift-apiserver/audit.log"},
"audit-policy-file": {"/var/run/configmaps/audit/policy.yaml"},
"etcd-healthcheck-timeout": {"9s"},
"etcd-readycheck-timeout": {"9s"},
"shutdown-delay-duration": {"15s"},
"shutdown-send-retry-after": {"true"},
},
APIServers: openshiftcontrolplanev1.APIServers{
PerGroupOptions: tc.expectedPerGroupOptions,
},
}

var createConfigCM *corev1.ConfigMap
for _, action := range fakeKubeClient.Actions() {
if action.GetResource().Resource != "configmaps" || action.GetVerb() != "create" {
continue
}
cm := action.(clientgotesting.CreateActionImpl).GetObject().(*corev1.ConfigMap)
if cm.Name != "config" {
continue
}
createConfigCM = cm
break
}

config := openshiftcontrolplanev1.OpenShiftAPIServerConfig{}
if err := json.NewDecoder(bytes.NewBuffer([]byte(cm.Data["config.yaml"]))).Decode(&config); err != nil {
if createConfigCM == nil {
t.Fatalf("Expected client action for create configmaps/config")
}

scheme := runtime.NewScheme()
utilruntime.Must(openshiftcontrolplanev1.Install(scheme))
codecs := serializer.NewCodecFactory(scheme)
obj, err := runtime.Decode(codecs.UniversalDecoder(openshiftcontrolplanev1.GroupVersion, configv1.GroupVersion), []byte(createConfigCM.Data["config.yaml"]))
if err != nil {
t.Fatalf("Unable to decode OpenShiftAPIServerConfig: %v", err)
}

if !equality.Semantic.DeepEqual(config.APIServers.PerGroupOptions, tc.expectedPerGroupOptions) {
t.Errorf("generation status mismatch, diff = %s", diff.ObjectDiff(config.APIServers.PerGroupOptions, tc.expectedPerGroupOptions))
config := obj.(*openshiftcontrolplanev1.OpenShiftAPIServerConfig)
if !equality.Semantic.DeepEqual(config, expecteOpenShiftAPIServerConfig) {
t.Errorf("generation status mismatch, diff = %s", diff.ObjectDiff(config, expecteOpenShiftAPIServerConfig))
}
})
}
Expand Down

0 comments on commit 2cc5428

Please sign in to comment.