From 9f154bc6a95c0fe72ec50539fe952a72e2882a06 Mon Sep 17 00:00:00 2001 From: Danil-Grigorev Date: Sun, 16 May 2021 15:45:07 +0200 Subject: [PATCH] Separate config, substitution and cloud package from controllers - Refactor tests to use assert and ensure full test coverage --- controllers/clusteroperator_controller.go | 26 ++---- .../clusteroperator_controller_test.go | 23 ++--- pkg/cloud/aws.go | 43 --------- pkg/cloud/aws/aws.go | 34 +++++++ pkg/cloud/aws/aws_test.go | 21 +++++ pkg/cloud/cloud.go | 21 +++++ pkg/cloud/cloud_test.go | 73 +++++++++++++++ .../common/_testdata/assets/deployment.yaml | 22 +++++ pkg/cloud/common/_testdata/foo | 1 + pkg/cloud/common/sources.go | 37 ++++++++ pkg/cloud/common/sources_test.go | 80 ++++++++++++++++ pkg/cloud/openstack/openstack.go | 50 ++++------ pkg/cloud/openstack/openstack_test.go | 29 ++---- {controllers => pkg/config}/config.go | 24 ++--- {controllers => pkg/config}/config_test.go | 92 ++++++++----------- .../substitution}/substitution.go | 7 +- .../substitution}/substitution_test.go | 39 ++++---- 17 files changed, 411 insertions(+), 211 deletions(-) delete mode 100644 pkg/cloud/aws.go create mode 100644 pkg/cloud/aws/aws.go create mode 100644 pkg/cloud/aws/aws_test.go create mode 100644 pkg/cloud/cloud.go create mode 100644 pkg/cloud/cloud_test.go create mode 100644 pkg/cloud/common/_testdata/assets/deployment.yaml create mode 100644 pkg/cloud/common/_testdata/foo create mode 100644 pkg/cloud/common/sources.go create mode 100644 pkg/cloud/common/sources_test.go rename {controllers => pkg/config}/config.go (65%) rename {controllers => pkg/config}/config_test.go (79%) rename {controllers => pkg/substitution}/substitution.go (82%) rename {controllers => pkg/substitution}/substitution_test.go (82%) diff --git a/controllers/clusteroperator_controller.go b/controllers/clusteroperator_controller.go index 9c62a19dc..7181823f1 100644 --- a/controllers/clusteroperator_controller.go +++ b/controllers/clusteroperator_controller.go @@ -22,7 +22,8 @@ import ( configv1 "github.com/openshift/api/config/v1" "github.com/openshift/cluster-cloud-controller-manager-operator/pkg/cloud" - "github.com/openshift/cluster-cloud-controller-manager-operator/pkg/cloud/openstack" + "github.com/openshift/cluster-cloud-controller-manager-operator/pkg/config" + "github.com/openshift/cluster-cloud-controller-manager-operator/pkg/substitution" "github.com/openshift/library-go/pkg/cloudprovider" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" @@ -100,7 +101,7 @@ func (r *CloudOperatorReconciler) Reconcile(ctx context.Context, _ ctrl.Request) return ctrl.Result{}, err } - platform, err := getProviderFromInfrastructure(infra) + platform, err := config.GetProviderFromInfrastructure(infra) if err != nil { klog.Errorf("Unable to determine platform from infrastructure: %s", err) // Ignoring error here as infrastructure resource needs to be reconciled externally @@ -133,7 +134,7 @@ func (r *CloudOperatorReconciler) Reconcile(ctx context.Context, _ ctrl.Request) return ctrl.Result{}, nil } - config, err := r.composeConfig(platform) + config, err := config.ComposeConfig(platform, r.ImagesFile, r.ManagedNamespace) if err != nil { klog.Errorf("Unable to build operator config %s", err) if err := r.setStatusDegraded(ctx, err); err != nil { @@ -160,10 +161,10 @@ func (r *CloudOperatorReconciler) Reconcile(ctx context.Context, _ ctrl.Request) return ctrl.Result{}, nil } -func (r *CloudOperatorReconciler) sync(ctx context.Context, config operatorConfig) error { +func (r *CloudOperatorReconciler) sync(ctx context.Context, config config.OperatorConfig) error { // Deploy resources for platform - templates := getResources(config.Platform) - resources := fillConfigValues(config, templates) + templates := cloud.GetResources(config.Platform) + resources := substitution.FillConfigValues(config, templates) updated, err := r.applyResources(ctx, resources) if err != nil { @@ -220,19 +221,6 @@ func (r *CloudOperatorReconciler) applyResources(ctx context.Context, resources return updated, nil } -func getResources(platform configv1.PlatformType) []client.Object { - switch platform { - case configv1.AWSPlatformType: - return cloud.GetAWSResources() - case configv1.OpenStackPlatformType: - return openstack.GetResources() - default: - klog.Warning("No recognized cloud provider platform found in infrastructure") - } - - return nil -} - // SetupWithManager sets up the controller with the Manager. func (r *CloudOperatorReconciler) SetupWithManager(mgr ctrl.Manager) error { watcher, err := NewObjectWatcher(WatcherOptions{ diff --git a/controllers/clusteroperator_controller_test.go b/controllers/clusteroperator_controller_test.go index 5cb6bedb7..ed24048fe 100644 --- a/controllers/clusteroperator_controller_test.go +++ b/controllers/clusteroperator_controller_test.go @@ -9,7 +9,8 @@ import ( . "github.com/onsi/gomega" configv1 "github.com/openshift/api/config/v1" "github.com/openshift/cluster-cloud-controller-manager-operator/pkg/cloud" - openstack "github.com/openshift/cluster-cloud-controller-manager-operator/pkg/cloud/openstack" + "github.com/openshift/cluster-cloud-controller-manager-operator/pkg/config" + "github.com/openshift/cluster-cloud-controller-manager-operator/pkg/substitution" "github.com/openshift/library-go/pkg/cloudprovider" "github.com/openshift/library-go/pkg/config/clusteroperator/v1helpers" appsv1 "k8s.io/api/apps/v1" @@ -269,7 +270,7 @@ var _ = Describe("Component sync controller", func() { type testCase struct { status *configv1.InfrastructureStatus featureGateSpec *configv1.FeatureGateSpec - config operatorConfig + config config.OperatorConfig expected []client.Object } @@ -290,7 +291,7 @@ var _ = Describe("Component sync controller", func() { watchMap := watcher.getWatchedResources() - operands = fillConfigValues(tc.config, tc.expected) + operands = substitution.FillConfigValues(tc.config, tc.expected) for _, obj := range operands { Expect(watchMap[obj.GetName()]).ToNot(BeNil()) @@ -317,11 +318,11 @@ var _ = Describe("Component sync controller", func() { }, }, featureGateSpec: externalFeatureGateSpec, - config: operatorConfig{ + config: config.OperatorConfig{ ManagedNamespace: testManagementNamespace, ControllerImage: "registry.ci.openshift.org/openshift:aws-cloud-controller-manager", }, - expected: cloud.GetAWSResources(), + expected: cloud.GetResources(configv1.AWSPlatformType), }), Entry("Should provision OpenStack resources", testCase{ status: &configv1.InfrastructureStatus{ @@ -330,12 +331,12 @@ var _ = Describe("Component sync controller", func() { Type: configv1.OpenStackPlatformType, }, }, - config: operatorConfig{ + config: config.OperatorConfig{ ManagedNamespace: testManagementNamespace, ControllerImage: "registry.ci.openshift.org/openshift:openstack-cloud-controller-manager", }, featureGateSpec: externalFeatureGateSpec, - expected: openstack.GetResources(), + expected: cloud.GetResources(configv1.OpenStackPlatformType), }), Entry("Should not provision resources for currently unsupported platform", testCase{ status: &configv1.InfrastructureStatus{ @@ -395,7 +396,7 @@ var _ = Describe("Apply resources should", func() { }) It("Expect update when resources are not found", func() { - resources = append(resources, cloud.GetAWSResources()...) + resources = append(resources, cloud.GetResources(configv1.AWSPlatformType)...) updated, err := reconciler.applyResources(context.TODO(), resources) Expect(err).ShouldNot(HaveOccurred()) @@ -405,7 +406,7 @@ var _ = Describe("Apply resources should", func() { It("Expect update when deployment generation have changed", func() { var dep *appsv1.Deployment - for _, res := range cloud.GetAWSResources() { + for _, res := range cloud.GetResources(configv1.AWSPlatformType) { if deployment, ok := res.(*appsv1.Deployment); ok { dep = deployment break @@ -432,7 +433,7 @@ var _ = Describe("Apply resources should", func() { }) It("Expect error when object requsted is incorrect", func() { - objects := cloud.GetAWSResources() + objects := cloud.GetResources(configv1.AWSPlatformType) objects[0].SetNamespace("non-existent") updated, err := reconciler.applyResources(context.TODO(), objects) @@ -442,7 +443,7 @@ var _ = Describe("Apply resources should", func() { }) It("Expect no update when resources are applied twice", func() { - resources = append(resources, openstack.GetResources()...) + resources = append(resources, cloud.GetResources(configv1.OpenStackPlatformType)...) updated, err := reconciler.applyResources(context.TODO(), resources) Expect(err).ShouldNot(HaveOccurred()) diff --git a/pkg/cloud/aws.go b/pkg/cloud/aws.go deleted file mode 100644 index 036e4fc37..000000000 --- a/pkg/cloud/aws.go +++ /dev/null @@ -1,43 +0,0 @@ -package cloud - -import ( - "bytes" - "embed" - - appsv1 "k8s.io/api/apps/v1" - k8sYaml "k8s.io/apimachinery/pkg/util/yaml" - - "k8s.io/klog/v2" - "sigs.k8s.io/controller-runtime/pkg/client" -) - -//go:embed aws/assets/* -var f embed.FS - -func GetAWSResources() []client.Object { - resources := []struct { - obj client.Object - asset string - }{ - {&appsv1.Deployment{}, "aws/assets/deployment.yaml"}, - } - ret := make([]client.Object, 0, len(resources)) - - for _, resource := range resources { - data, err := f.ReadFile(resource.asset) - if err != nil { - klog.Errorf("Cannot read embedded resource %v: %v", resource.asset, err) - return nil - } - - dec := k8sYaml.NewYAMLOrJSONDecoder(bytes.NewReader(data), 1000) - if err := dec.Decode(resource.obj); err != nil { - klog.Errorf("Cannot decode data from embedded resource %v: %v", resource.asset, err) - return nil - } - - ret = append(ret, resource.obj) - } - - return ret -} diff --git a/pkg/cloud/aws/aws.go b/pkg/cloud/aws/aws.go new file mode 100644 index 000000000..f9b04bfa9 --- /dev/null +++ b/pkg/cloud/aws/aws.go @@ -0,0 +1,34 @@ +package aws + +import ( + "embed" + + "github.com/openshift/cluster-cloud-controller-manager-operator/pkg/cloud/common" + appsv1 "k8s.io/api/apps/v1" + utilruntime "k8s.io/apimachinery/pkg/util/runtime" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +var ( + //go:embed assets/* + awsFS embed.FS + awsSources = []common.ObjectSource{ + {Object: &appsv1.Deployment{}, Path: "assets/deployment.yaml"}, + } + awsResources []client.Object +) + +func init() { + var err error + awsResources, err = common.ReadResources(awsFS, awsSources) + utilruntime.Must(err) +} + +func GetResources() []client.Object { + resources := make([]client.Object, len(awsResources)) + for i := range awsResources { + resources[i] = awsResources[i].DeepCopyObject().(client.Object) + } + + return resources +} diff --git a/pkg/cloud/aws/aws_test.go b/pkg/cloud/aws/aws_test.go new file mode 100644 index 000000000..c41055307 --- /dev/null +++ b/pkg/cloud/aws/aws_test.go @@ -0,0 +1,21 @@ +package aws + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestGetResources(t *testing.T) { + resources := GetResources() + assert.Len(t, resources, 1) + + var names, kinds []string + for _, r := range resources { + names = append(names, r.GetName()) + kinds = append(kinds, r.GetObjectKind().GroupVersionKind().Kind) + } + + assert.Contains(t, names, "aws-cloud-controller-manager") + assert.Contains(t, kinds, "Deployment") +} diff --git a/pkg/cloud/cloud.go b/pkg/cloud/cloud.go new file mode 100644 index 000000000..76a92a42e --- /dev/null +++ b/pkg/cloud/cloud.go @@ -0,0 +1,21 @@ +package cloud + +import ( + configv1 "github.com/openshift/api/config/v1" + "github.com/openshift/cluster-cloud-controller-manager-operator/pkg/cloud/aws" + "github.com/openshift/cluster-cloud-controller-manager-operator/pkg/cloud/openstack" + "k8s.io/klog" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +func GetResources(platform configv1.PlatformType) []client.Object { + switch platform { + case configv1.AWSPlatformType: + return aws.GetResources() + case configv1.OpenStackPlatformType: + return openstack.GetResources() + default: + klog.Warningf("Unrecognized platform type %q found in infrastructure", platform) + return nil + } +} diff --git a/pkg/cloud/cloud_test.go b/pkg/cloud/cloud_test.go new file mode 100644 index 000000000..31a95ed35 --- /dev/null +++ b/pkg/cloud/cloud_test.go @@ -0,0 +1,73 @@ +package cloud + +import ( + "testing" + + configv1 "github.com/openshift/api/config/v1" + "github.com/openshift/cluster-cloud-controller-manager-operator/pkg/cloud/aws" + "github.com/openshift/cluster-cloud-controller-manager-operator/pkg/cloud/openstack" + "github.com/stretchr/testify/assert" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +func TestGetResources(t *testing.T) { + tc := []struct { + name string + platform configv1.PlatformType + expected []client.Object + }{{ + name: "AWS resources returned as expected", + platform: configv1.AWSPlatformType, + expected: aws.GetResources(), + }, { + name: "OpenStack resources returned as expected", + platform: configv1.OpenStackPlatformType, + expected: openstack.GetResources(), + }, { + name: "GCP resources are empty, as the platform is not yet supported", + platform: configv1.GCPPlatformType, + }, { + name: "Azure resources are empty, as the platform is not yet supported", + platform: configv1.AzurePlatformType, + }, { + name: "VSphere resources are empty, as the platform is not yet supported", + platform: configv1.VSpherePlatformType, + }, { + name: "OVirt resources are empty, as the platform is not yet supported", + platform: configv1.OvirtPlatformType, + }, { + name: "IBMCloud resources are empty, as the platform is not yet supported", + platform: configv1.IBMCloudPlatformType, + }, { + name: "Libvirt resources are empty", + platform: configv1.LibvirtPlatformType, + }, { + name: "Kubevirt resources are empty", + platform: configv1.KubevirtPlatformType, + }, { + name: "BareMetal resources are empty", + platform: configv1.BareMetalPlatformType, + }, { + name: "None platform resources are empty", + platform: configv1.NonePlatformType, + }} + + for _, tc := range tc { + t.Run(tc.name, func(t *testing.T) { + resources := GetResources(tc.platform) + + assert.Equal(t, len(tc.expected), len(resources)) + assert.EqualValues(t, tc.expected, resources) + + // Edit and repeat procedure to ensure modification in place is not present + if len(resources) > 0 { + resources[0].SetName("different") + newResources := GetResources(tc.platform) + + assert.Equal(t, len(tc.expected), len(newResources)) + assert.EqualValues(t, tc.expected, newResources) + assert.NotEqualValues(t, resources, newResources) + } + }) + } +} diff --git a/pkg/cloud/common/_testdata/assets/deployment.yaml b/pkg/cloud/common/_testdata/assets/deployment.yaml new file mode 100644 index 000000000..682cbe0c8 --- /dev/null +++ b/pkg/cloud/common/_testdata/assets/deployment.yaml @@ -0,0 +1,22 @@ +apiVersion: apps/v1 +kind: Deployment +metadata: + name: sample + namespace: sample +spec: + selector: + matchLabels: + k8s-app: aws-cloud-controller-manager + template: + metadata: + annotations: + target.workload.openshift.io/management: '{"effect": "PreferredDuringScheduling"}' + labels: + k8s-app: aws-cloud-controller-manager + spec: + containers: + - args: + - -v=2 + image: test:image + imagePullPolicy: IfNotPresent + name: cloud-controller-manager diff --git a/pkg/cloud/common/_testdata/foo b/pkg/cloud/common/_testdata/foo new file mode 100644 index 000000000..a24e5b48e --- /dev/null +++ b/pkg/cloud/common/_testdata/foo @@ -0,0 +1 @@ +Sample file undecodable as yaml or json diff --git a/pkg/cloud/common/sources.go b/pkg/cloud/common/sources.go new file mode 100644 index 000000000..0ab1f9d7b --- /dev/null +++ b/pkg/cloud/common/sources.go @@ -0,0 +1,37 @@ +package common + +import ( + "bytes" + "embed" + + "k8s.io/apimachinery/pkg/util/yaml" + "k8s.io/klog" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +type ObjectSource struct { + Object client.Object + Path string +} + +func ReadResources(f embed.FS, sources []ObjectSource) ([]client.Object, error) { + ret := []client.Object{} + for _, source := range sources { + data, err := f.ReadFile(source.Path) + if err != nil { + klog.Errorf("Cannot read embedded resource %v: %v", source.Path, err) + return nil, err + } + + object := source.Object.DeepCopyObject().(client.Object) + dec := yaml.NewYAMLOrJSONDecoder(bytes.NewReader(data), 1000) + if err := dec.Decode(object); err != nil { + klog.Errorf("Cannot decode data from embedded resource %v: %v", source.Path, err) + return nil, err + } + + ret = append(ret, object) + } + + return ret, nil +} diff --git a/pkg/cloud/common/sources_test.go b/pkg/cloud/common/sources_test.go new file mode 100644 index 000000000..e330677e4 --- /dev/null +++ b/pkg/cloud/common/sources_test.go @@ -0,0 +1,80 @@ +package common + +import ( + "embed" + "testing" + + "github.com/stretchr/testify/assert" + appsv1 "k8s.io/api/apps/v1" +) + +//go:embed _testdata/* +var badPath embed.FS + +//go:embed _testdata/assets/* +var assetPath embed.FS + +func TestReadResources(t *testing.T) { + + type resourcemeta struct { + kind string + name string + namespace string + } + + tc := []struct { + name string + fs embed.FS + sources []ObjectSource + expected []resourcemeta + expectedError string + }{{ + name: "Reading corect sources cause no error", + fs: assetPath, + sources: []ObjectSource{ + {Object: &appsv1.Deployment{}, Path: "_testdata/assets/deployment.yaml"}, + }, + expected: []resourcemeta{ + {"Deployment", "sample", "sample"}, + }, + }, { + name: "Error opening a non-existent source", + fs: badPath, + sources: []ObjectSource{ + {Object: &appsv1.Deployment{}, Path: "wrong-path"}, + }, + expectedError: "open wrong-path: file does not exist", + }, { + name: "Error during unmarshaling into wrong resource type", + fs: badPath, + sources: []ObjectSource{ + {Object: &appsv1.Deployment{}, Path: "_testdata/foo"}, + }, + expectedError: "error unmarshaling JSON: while decoding JSON: json: cannot unmarshal string into Go value of type v1.Deployment", + }} + + for _, tc := range tc { + t.Run(tc.name, func(t *testing.T) { + initialSources := tc.sources + resources, err := ReadResources(tc.fs, tc.sources) + if tc.expectedError != "" { + assert.EqualError(t, err, tc.expectedError) + } else { + assert.NoError(t, err) + } + + // Check that the returned resources contain a named set of objects + assert.Equal(t, len(tc.expected), len(resources)) + + for i := 0; i < len(resources); i++ { + assert.Contains(t, tc.expected, resourcemeta{ + resources[i].GetObjectKind().GroupVersionKind().Kind, + resources[i].GetName(), + resources[i].GetNamespace()}) + } + + // No modification in place happened + assert.EqualValues(t, initialSources, tc.sources) + }) + } +} diff --git a/pkg/cloud/openstack/openstack.go b/pkg/cloud/openstack/openstack.go index 03096d1c3..21fb51517 100644 --- a/pkg/cloud/openstack/openstack.go +++ b/pkg/cloud/openstack/openstack.go @@ -1,46 +1,36 @@ package openstack import ( - "bytes" "embed" + "github.com/openshift/cluster-cloud-controller-manager-operator/pkg/cloud/common" appsv1 "k8s.io/api/apps/v1" v1 "k8s.io/api/core/v1" - k8sYaml "k8s.io/apimachinery/pkg/util/yaml" - - "k8s.io/klog/v2" + utilruntime "k8s.io/apimachinery/pkg/util/runtime" "sigs.k8s.io/controller-runtime/pkg/client" ) -//go:embed assets/* -var f embed.FS - -func GetResources() []client.Object { - resources := []struct { - obj client.Object - asset string - }{ - {&v1.ConfigMap{}, "assets/config.yaml"}, - {&appsv1.Deployment{}, "assets/deployment.yaml"}, +var ( + //go:embed assets/* + openStackFS embed.FS + openStackSources = []common.ObjectSource{ + {Object: &v1.ConfigMap{}, Path: "assets/config.yaml"}, + {Object: &appsv1.Deployment{}, Path: "assets/deployment.yaml"}, } - ret := make([]client.Object, 0, len(resources)) - - for i := 0; i < len(resources); i++ { - resource := &resources[i] - data, err := f.ReadFile(resource.asset) - if err != nil { - klog.Errorf("Cannot read embedded resource %v: %v", resource.asset, err) - return nil - } + openStackResources []client.Object +) - dec := k8sYaml.NewYAMLOrJSONDecoder(bytes.NewReader(data), 1000) - if err := dec.Decode(resource.obj); err != nil { - klog.Errorf("Cannot decode data from embedded resource %v: %v", resource.asset, err) - return nil - } +func init() { + var err error + openStackResources, err = common.ReadResources(openStackFS, openStackSources) + utilruntime.Must(err) +} - ret = append(ret, resource.obj) +func GetResources() []client.Object { + resources := make([]client.Object, len(openStackResources)) + for i := range openStackResources { + resources[i] = openStackResources[i].DeepCopyObject().(client.Object) } - return ret + return resources } diff --git a/pkg/cloud/openstack/openstack_test.go b/pkg/cloud/openstack/openstack_test.go index ad09dc6a1..aeaa5a0de 100644 --- a/pkg/cloud/openstack/openstack_test.go +++ b/pkg/cloud/openstack/openstack_test.go @@ -6,29 +6,20 @@ import ( "github.com/stretchr/testify/assert" ) -func TestOpenStackResources(t *testing.T) { +func TestGetResources(t *testing.T) { resources := GetResources() - assert.NotNil(t, resources) + assert.Len(t, resources, 2) - type resourcemeta struct { - kind string - name string - namespace string + var names, kinds []string + for _, r := range resources { + names = append(names, r.GetName()) + kinds = append(kinds, r.GetObjectKind().GroupVersionKind().Kind) } - const ccmo_ns = "openshift-cloud-controller-manager" + assert.Contains(t, names, "openstack-cloud-controller-manager") + assert.Contains(t, names, "openstack-cloud-controller-manager-config") - // Check that the returned resources contain a named set of objects - expected := []resourcemeta{ - {"ConfigMap", "openstack-cloud-controller-manager-config", ccmo_ns}, - {"Deployment", "openstack-cloud-controller-manager", ccmo_ns}, - } - assert.Equal(t, len(expected), len(resources)) + assert.Contains(t, kinds, "Deployment") + assert.Contains(t, kinds, "ConfigMap") - for i := 0; i < len(resources); i++ { - assert.Contains(t, expected, resourcemeta{ - resources[i].GetObjectKind().GroupVersionKind().Kind, - resources[i].GetName(), - resources[i].GetNamespace()}) - } } diff --git a/controllers/config.go b/pkg/config/config.go similarity index 65% rename from controllers/config.go rename to pkg/config/config.go index daad3a399..6ea5bda6b 100644 --- a/controllers/config.go +++ b/pkg/config/config.go @@ -1,4 +1,4 @@ -package controllers +package config import ( "encoding/json" @@ -16,16 +16,16 @@ type imagesReference struct { CloudControllerManagerOpenStack string `json:"cloudControllerManagerOpenStack"` } -// operatorConfig contains configuration values for templating resources -type operatorConfig struct { +// OperatorConfig contains configuration values for templating resources +type OperatorConfig struct { ManagedNamespace string ControllerImage string Platform configv1.PlatformType } -func getProviderFromInfrastructure(infra *configv1.Infrastructure) (configv1.PlatformType, error) { +func GetProviderFromInfrastructure(infra *configv1.Infrastructure) (configv1.PlatformType, error) { if infra == nil || infra.Status.PlatformStatus == nil { - return "", fmt.Errorf("platform status is not pupulated on infrastructure") + return "", fmt.Errorf("platform status is not populated on infrastructure") } if infra.Status.PlatformStatus.Type == "" { return "", fmt.Errorf("no platform provider found on infrastructure") @@ -47,7 +47,7 @@ func getImagesFromJSONFile(filePath string) (imagesReference, error) { return i, nil } -func getProviderControllerFromImages(platform configv1.PlatformType, images imagesReference) string { +func getCloudControllerManagerFromImages(platform configv1.PlatformType, images imagesReference) string { switch platform { case configv1.AWSPlatformType: return images.CloudControllerManagerAWS @@ -58,19 +58,19 @@ func getProviderControllerFromImages(platform configv1.PlatformType, images imag } } -func (r *CloudOperatorReconciler) composeConfig(platform configv1.PlatformType) (operatorConfig, error) { - config := operatorConfig{ +func ComposeConfig(platform configv1.PlatformType, imagesFile, managedNamespace string) (OperatorConfig, error) { + config := OperatorConfig{ Platform: platform, - ManagedNamespace: r.ManagedNamespace, + ManagedNamespace: managedNamespace, } - images, err := getImagesFromJSONFile(r.ImagesFile) + images, err := getImagesFromJSONFile(imagesFile) if err != nil { - klog.Errorf("Unable to decode images file from location %s", r.ImagesFile, err) + klog.Errorf("Unable to decode images file from location %s", imagesFile, err) return config, err } - config.ControllerImage = getProviderControllerFromImages(platform, images) + config.ControllerImage = getCloudControllerManagerFromImages(platform, images) return config, nil } diff --git a/controllers/config_test.go b/pkg/config/config_test.go similarity index 79% rename from controllers/config_test.go rename to pkg/config/config_test.go index e33d25c20..15ee2e0a0 100644 --- a/controllers/config_test.go +++ b/pkg/config/config_test.go @@ -1,4 +1,4 @@ -package controllers +package config import ( "io/ioutil" @@ -6,8 +6,7 @@ import ( "testing" configv1 "github.com/openshift/api/config/v1" - "k8s.io/apimachinery/pkg/api/equality" - "k8s.io/client-go/tools/record" + "github.com/stretchr/testify/assert" ) func TestGetImagesFromJSONFile(t *testing.T) { @@ -16,7 +15,7 @@ func TestGetImagesFromJSONFile(t *testing.T) { path string imagesContent string expectedImages imagesReference - expectError bool + expectError string }{{ name: "Unmarshal images from file", path: "images_file", @@ -30,7 +29,7 @@ func TestGetImagesFromJSONFile(t *testing.T) { }, }, { name: "Error on non present file", - expectError: true, + expectError: "open not_found: no such file or directory", }, { name: "Partial content is accepted", path: "images_file", @@ -69,7 +68,7 @@ func TestGetImagesFromJSONFile(t *testing.T) { imagesContent: `{ "cloudControllerManagerAWS": BAD, }`, - expectError: true, + expectError: "invalid character 'B' looking for beginning of value", }, } @@ -79,25 +78,21 @@ func TestGetImagesFromJSONFile(t *testing.T) { if tc.path != "" { file, err := ioutil.TempFile(os.TempDir(), tc.path) path = file.Name() - if err != nil { - t.Fatal(err) - } + assert.NoError(t, err) defer file.Close() _, err = file.WriteString(tc.imagesContent) - if err != nil { - t.Fatal(err) - } + assert.NoError(t, err) } images, err := getImagesFromJSONFile(path) - if isErr := err != nil; isErr != tc.expectError { - t.Fatalf("Unexpected error result: %v", err) + if tc.expectError != "" { + assert.EqualError(t, err, tc.expectError) + } else { + assert.NoError(t, err) } - if !equality.Semantic.DeepEqual(images, tc.expectedImages) { - t.Errorf("Images are not set correctly:\n%v\nexpected\n%v", images, tc.expectedImages) - } + assert.EqualValues(t, tc.expectedImages, images) }) } } @@ -107,11 +102,11 @@ func TestGetProviderFromInfrastructure(t *testing.T) { name string infra *configv1.Infrastructure expectPlatform configv1.PlatformType - expectErr bool + expectErr string }{{ name: "Passing empty infra causes error", infra: nil, - expectErr: true, + expectErr: "platform status is not populated on infrastructure", }, { name: "No platform type causes error", infra: &configv1.Infrastructure{ @@ -119,7 +114,7 @@ func TestGetProviderFromInfrastructure(t *testing.T) { PlatformStatus: &configv1.PlatformStatus{}, }, }, - expectErr: true, + expectErr: "no platform provider found on infrastructure", }, { name: "All good", infra: &configv1.Infrastructure{ @@ -134,13 +129,13 @@ func TestGetProviderFromInfrastructure(t *testing.T) { for _, tc := range tc { t.Run(tc.name, func(t *testing.T) { - platform, err := getProviderFromInfrastructure(tc.infra) - if isErr := (err != nil); tc.expectErr != isErr { - t.Fatalf("Unexpected error result: %v", err) - } - if platform != tc.expectPlatform { - t.Errorf("Unexpected platform %s, expected %s", platform, tc.expectPlatform) + platform, err := GetProviderFromInfrastructure(tc.infra) + if tc.expectErr != "" { + assert.EqualError(t, err, tc.expectErr) + } else { + assert.NoError(t, err) } + assert.Equal(t, platform, tc.expectPlatform) }) } } @@ -175,22 +170,22 @@ func TestGetProviderControllerFromImages(t *testing.T) { for _, tc := range tc { t.Run(tc.name, func(t *testing.T) { - image := getProviderControllerFromImages(tc.platformType, images) - if image != tc.expectedImage { - t.Errorf("Unexpected image %s, expected %s", image, tc.expectedImage) - } + image := getCloudControllerManagerFromImages(tc.platformType, images) + assert.Equal(t, tc.expectedImage, image) }) } } func TestComposeConfig(t *testing.T) { + defaultManagementNamespace := "test-namespace" + tc := []struct { name string namespace string platform configv1.PlatformType imagesContent string - expectConfig operatorConfig - expectError bool + expectConfig OperatorConfig + expectError string }{{ name: "Unmarshal images from file for AWS", namespace: defaultManagementNamespace, @@ -199,7 +194,7 @@ func TestComposeConfig(t *testing.T) { "cloudControllerManagerAWS": "registry.ci.openshift.org/openshift:aws-cloud-controller-manager", "cloudControllerManagerOpenStack": "registry.ci.openshift.org/openshift:openstack-cloud-controller-manager" }`, - expectConfig: operatorConfig{ + expectConfig: OperatorConfig{ ControllerImage: "registry.ci.openshift.org/openshift:aws-cloud-controller-manager", ManagedNamespace: defaultManagementNamespace, Platform: configv1.AWSPlatformType, @@ -212,7 +207,7 @@ func TestComposeConfig(t *testing.T) { "cloudControllerManagerAWS": "registry.ci.openshift.org/openshift:aws-cloud-controller-manager", "cloudControllerManagerOpenStack": "registry.ci.openshift.org/openshift:openstack-cloud-controller-manager" }`, - expectConfig: operatorConfig{ + expectConfig: OperatorConfig{ ControllerImage: "registry.ci.openshift.org/openshift:openstack-cloud-controller-manager", ManagedNamespace: defaultManagementNamespace, Platform: configv1.OpenStackPlatformType, @@ -225,7 +220,7 @@ func TestComposeConfig(t *testing.T) { "cloudControllerManagerAWS": "registry.ci.openshift.org/openshift:aws-cloud-controller-manager", "cloudControllerManagerOpenStack": "registry.ci.openshift.org/openshift:openstack-cloud-controller-manager" }`, - expectConfig: operatorConfig{ + expectConfig: OperatorConfig{ ControllerImage: "", ManagedNamespace: "otherNamespace", Platform: configv1.NonePlatformType, @@ -235,36 +230,27 @@ func TestComposeConfig(t *testing.T) { imagesContent: `{ "cloudControllerManagerAWS": BAD, }`, - expectError: true, + expectError: "invalid character 'B' looking for beginning of value", }} for _, tc := range tc { t.Run(tc.name, func(t *testing.T) { file, err := ioutil.TempFile(os.TempDir(), "images") path := file.Name() - if err != nil { - t.Fatal(err) - } + assert.NoError(t, err) defer file.Close() _, err = file.WriteString(tc.imagesContent) - if err != nil { - t.Fatal(err) - } + assert.NoError(t, err) - r := &CloudOperatorReconciler{ - ImagesFile: path, - ManagedNamespace: tc.namespace, - Recorder: record.NewFakeRecorder(32), - } - config, err := r.composeConfig(tc.platform) - if isErr := err != nil; isErr != tc.expectError { - t.Fatalf("Unexpected error result: %v", err) + config, err := ComposeConfig(tc.platform, path, tc.namespace) + if tc.expectError != "" { + assert.EqualError(t, err, tc.expectError) + } else { + assert.NoError(t, err) } - if !equality.Semantic.DeepEqual(config, tc.expectConfig) { - t.Errorf("Config is not equal:\n%v\nexpected\n%v", config, tc.expectConfig) - } + assert.EqualValues(t, config, tc.expectConfig) }) } } diff --git a/controllers/substitution.go b/pkg/substitution/substitution.go similarity index 82% rename from controllers/substitution.go rename to pkg/substitution/substitution.go index b46833f11..a899a8441 100644 --- a/controllers/substitution.go +++ b/pkg/substitution/substitution.go @@ -1,6 +1,7 @@ -package controllers +package substitution import ( + "github.com/openshift/cluster-cloud-controller-manager-operator/pkg/config" v1 "k8s.io/api/apps/v1" "k8s.io/klog/v2" "sigs.k8s.io/controller-runtime/pkg/client" @@ -13,7 +14,7 @@ const ( ) // setDeploymentImages substitutes controller containers in Deployment with correct image -func setDeploymentImages(config operatorConfig, d *v1.Deployment) { +func setDeploymentImages(config config.OperatorConfig, d *v1.Deployment) { for i, container := range d.Spec.Template.Spec.Containers { if container.Name != cloudControllerManagerName { continue @@ -24,7 +25,7 @@ func setDeploymentImages(config operatorConfig, d *v1.Deployment) { } } -func fillConfigValues(config operatorConfig, templates []client.Object) []client.Object { +func FillConfigValues(config config.OperatorConfig, templates []client.Object) []client.Object { objects := make([]client.Object, len(templates)) for i, objectTemplate := range templates { templateCopy := objectTemplate.DeepCopyObject().(client.Object) diff --git a/controllers/substitution_test.go b/pkg/substitution/substitution_test.go similarity index 82% rename from controllers/substitution_test.go rename to pkg/substitution/substitution_test.go index eb3daeb73..487054478 100644 --- a/controllers/substitution_test.go +++ b/pkg/substitution/substitution_test.go @@ -1,11 +1,12 @@ -package controllers +package substitution import ( "testing" + "github.com/openshift/cluster-cloud-controller-manager-operator/pkg/config" + "github.com/stretchr/testify/assert" v1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/equality" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -14,7 +15,7 @@ func TestSetDeploymentImages(t *testing.T) { tc := []struct { name string containers []corev1.Container - config operatorConfig + config config.OperatorConfig expectedContainers []corev1.Container }{{ name: "Unknown container name", @@ -26,7 +27,7 @@ func TestSetDeploymentImages(t *testing.T) { Name: "different_name", Image: "no_change", }}, - config: operatorConfig{ + config: config.OperatorConfig{ ControllerImage: "correct_image:tag", }, }, { @@ -39,7 +40,7 @@ func TestSetDeploymentImages(t *testing.T) { Name: cloudControllerManagerName, Image: "correct_image:tag", }}, - config: operatorConfig{ + config: config.OperatorConfig{ ControllerImage: "correct_image:tag", }, }, { @@ -58,7 +59,7 @@ func TestSetDeploymentImages(t *testing.T) { Name: "node-controller-manager", Image: "no_change", }}, - config: operatorConfig{ + config: config.OperatorConfig{ ControllerImage: "correct_image:tag", }, }} @@ -77,19 +78,19 @@ func TestSetDeploymentImages(t *testing.T) { setDeploymentImages(tc.config, deploy) - if !equality.Semantic.DeepEqual(deploy.Spec.Template.Spec.Containers, tc.expectedContainers) { - t.Errorf("Container images are not set correctly:\n%v\nexpected\n%v", deploy.Spec.Template.Spec.Containers, tc.expectedContainers) - } + assert.EqualValues(t, deploy.Spec.Template.Spec.Containers, tc.expectedContainers) }) } } func TestFillConfigValues(t *testing.T) { + testManagementNamespace := "test-namespace" + tc := []struct { name string objects []client.Object - config operatorConfig + config config.OperatorConfig expectedObjects []client.Object }{{ name: "Substitute object namespace", @@ -99,7 +100,7 @@ func TestFillConfigValues(t *testing.T) { Namespace: testManagementNamespace, }, }}, - config: operatorConfig{ + config: config.OperatorConfig{ ManagedNamespace: testManagementNamespace, }, }, { @@ -131,7 +132,7 @@ func TestFillConfigValues(t *testing.T) { }, }, }}, - config: operatorConfig{ + config: config.OperatorConfig{ ControllerImage: "correct_image:tag", ManagedNamespace: testManagementNamespace, }, @@ -169,7 +170,7 @@ func TestFillConfigValues(t *testing.T) { }, }, }}, - config: operatorConfig{ + config: config.OperatorConfig{ ControllerImage: "correct_image:tag", ManagedNamespace: testManagementNamespace, }, @@ -177,16 +178,12 @@ func TestFillConfigValues(t *testing.T) { for _, tc := range tc { t.Run(tc.name, func(t *testing.T) { - initialObjects := tc.objects - updatedObjects := fillConfigValues(tc.config, tc.objects) + updatedObjects := FillConfigValues(tc.config, tc.objects) - if !equality.Semantic.DeepEqual(updatedObjects, tc.expectedObjects) { - t.Errorf("Objects are not equal expected: \n%v, \n\nexpected %v", updatedObjects, tc.expectedObjects) - } - if !equality.Semantic.DeepEqual(initialObjects, tc.objects) { - t.Errorf("Objects were mutated in place unexpectingly") - } + assert.EqualValues(t, updatedObjects, tc.expectedObjects) + // Ensure there is no mutation in place + assert.EqualValues(t, initialObjects, tc.objects) }) } }