Skip to content

Commit

Permalink
Add service controller unit tests (#195)
Browse files Browse the repository at this point in the history 
* Add unit tests for service controller
* Add more tests for CF services
* Add "make test-fast" for quicker local testing

Signed-off-by: John Starich <[email protected]>
Signed-off-by: Art Berger <[email protected]>
  • Loading branch information
@JohnStarich
JohnStarich authored and Art Berger committed Sep 23, 2020
1 parent 21b43fe commit 309f91f
Show file tree
Hide file tree
Showing 7 changed files with 2,058 additions and 21 deletions.
4 changes: 4 additions & 0 deletions Makefile
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,10 @@ cache/bin/kustomize: cache/bin
curl -s "https://raw.githubusercontent.com/kubernetes-sigs/kustomize/master/hack/install_kustomize.sh" | bash
[[ "$$(which kustomize)" =~ cache/bin/kustomize ]]

.PHONY: test-fast
test-fast: generate manifests kubebuilder
go test -short -coverprofile cover.out ./...

.PHONY: test
test: generate manifests kubebuilder
go test -race -coverprofile cover.out ./...
Expand Down
41 changes: 23 additions & 18 deletions controllers/service_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ import (

ibmcloudv1beta1 "github.com/ibm/cloud-operators/api/v1beta1"
"github.com/ibm/cloud-operators/internal/config"
"github.com/ibm/cloud-operators/internal/ibmcloud"
"github.com/ibm/cloud-operators/internal/ibmcloud/cfservice"
"github.com/ibm/cloud-operators/internal/ibmcloud/resource"
)
Expand Down Expand Up @@ -64,6 +63,7 @@ type ServiceReconciler struct {
DeleteCFServiceInstance cfservice.InstanceDeleter
DeleteResourceServiceInstance resource.ServiceInstanceDeleter
GetCFServiceInstance cfservice.InstanceGetter
GetIBMCloudInfo IBMCloudInfoGetter
GetResourceServiceAliasInstance resource.ServiceAliasInstanceGetter
GetResourceServiceInstanceState resource.ServiceInstanceStatusGetter
UpdateResourceServiceInstance resource.ServiceInstanceUpdater
Expand Down Expand Up @@ -121,7 +121,7 @@ func (r *ServiceReconciler) Reconcile(request ctrl.Request) (ctrl.Result, error)
targetCRN string
)
{
ibmCloudInfo, err := ibmcloud.GetInfo(logt, r.Client, instance)
ibmCloudInfo, err := r.GetIBMCloudInfo(logt, r.Client, instance)
if err != nil {
// If secrets have already been deleted and we are in a deletion flow, just delete the finalizers
// to not prevent object from finalizing. This would cause orphaned service in IBM Cloud.
Expand All @@ -130,11 +130,13 @@ func (r *ServiceReconciler) Reconcile(request ctrl.Request) (ctrl.Result, error)
logt.Info("Cannot get IBMCloud related secrets and configmaps, just remove finalizers", "in deletion", err.Error())
instance.ObjectMeta.Finalizers = deleteServiceFinalizer(instance)
if err := r.Update(ctx, instance); err != nil {
logt.Info("Error removing finalizers", "in deletion", err.Error())
logt.Error(err, "Error removing finalizers in deletion")
// TODO(johnstarich): Shouldn't this be a failure so it can be requeued?
// Also, should the status be updated to include this failure message?
}
return ctrl.Result{}, nil
}
logt.Info(err.Error())
logt.Error(err, "Failed to get IBM Cloud info for service")
return r.updateStatusError(instance, serviceStateFailed, err)
}
resourceContext = ibmCloudInfo.Context
Expand Down Expand Up @@ -170,7 +172,8 @@ func (r *ServiceReconciler) Reconcile(request ctrl.Request) (ctrl.Result, error)
if !containsServiceFinalizer(instance) {
instance.ObjectMeta.Finalizers = append(instance.ObjectMeta.Finalizers, serviceFinalizer)
if err := r.Update(ctx, instance); err != nil {
logt.Info("Error adding finalizer", instance.ObjectMeta.Name, err.Error())
logt.Error(err, "Error adding finalizer", "service", instance.ObjectMeta.Name)
// TODO(johnstarich): Shouldn't this update the status with the failure message?
return ctrl.Result{}, err
}
}
Expand All @@ -179,14 +182,16 @@ func (r *ServiceReconciler) Reconcile(request ctrl.Request) (ctrl.Result, error)
if containsServiceFinalizer(instance) {
err := r.deleteService(session, logt, instance, serviceClassType)
if err != nil {
logt.Info("Error deleting resource", instance.ObjectMeta.Name, err.Error())
logt.Error(err, "Error deleting resource", "service", instance.ObjectMeta.Name)
// TODO(johnstarich): Shouldn't this return the error so it will be logged?
return ctrl.Result{Requeue: true, RequeueAfter: time.Second * 10}, nil
}

// remove our finalizer from the list and update it.
instance.ObjectMeta.Finalizers = deleteServiceFinalizer(instance)
if err := r.Update(ctx, instance); err != nil {
logt.Info("Error removing finalizers", "in deletion", err.Error())
err = r.Update(ctx, instance)
if err != nil {
logt.Error(err, "Error removing finalizers")
}
return ctrl.Result{}, err
}
Expand Down Expand Up @@ -222,14 +227,14 @@ func (r *ServiceReconciler) Reconcile(request ctrl.Request) (ctrl.Result, error)
externalName := getExternalName(instance)
params, err := r.getParams(ctx, instance)
if err != nil {
logt.Error(err, "Instance ", instance.ObjectMeta.Name, " has problems with its parameters")
logt.Error(err, "Instance has problems with its parameters", "service", instance.ObjectMeta.Name)
return r.updateStatusError(instance, serviceStateFailed, err)
}
tags := getTags(instance)
logt.Info("ServiceInstance ", "name", externalName, "tags", tags)

if serviceClassType == "CF" {
logt.Info("ServiceInstance ", "is CF", instance.ObjectMeta.Name)
logt.Info("ServiceInstance is CF", "instance", instance.ObjectMeta.Name)
if instance.Status.InstanceID == "" { // ServiceInstance has not been created on Bluemix
// check if using the alias plan, in that case we need to use the existing instance
if isAlias(instance) {
Expand All @@ -243,7 +248,7 @@ func (r *ServiceReconciler) Reconcile(request ctrl.Request) (ctrl.Result, error)
return r.updateStatus(session, logt, instance, resourceContext, instanceID, serviceStateOnline, serviceClassType)
}
// Service is not Alias
logt.Info("Creating ", instance.ObjectMeta.Name, instance.Spec.ServiceClass)
logt.Info("Creating", "instance", instance.ObjectMeta.Name, "service class", instance.Spec.ServiceClass)
guid, state, err := r.CreateCFServiceInstance(session, externalName, servicePlanID, spaceID, params, tags)
if err != nil {
return r.updateStatusError(instance, serviceStateFailed, err)
Expand Down Expand Up @@ -285,13 +290,13 @@ func (r *ServiceReconciler) Reconcile(request ctrl.Request) (ctrl.Result, error)
if instance.Status.InstanceID == "" { // ServiceInstance has not been created on Bluemix
// check if using the alias plan, in that case we need to use the existing instance
if isAlias(instance) {
logt := logt.WithValues("Name", instance.ObjectMeta.Name)
logt.Info("Using `Alias` plan, checking if instance exists")

// check if there is an annotation for service ID
instanceID := instance.ObjectMeta.GetAnnotations()[instanceIDKey]

logger := logt.WithValues("Name", instance.ObjectMeta.Name)
id, state, err := r.GetResourceServiceAliasInstance(session, instanceID, resourceGroupID, servicePlanID, externalName, logger)
id, state, err := r.GetResourceServiceAliasInstance(session, instanceID, resourceGroupID, servicePlanID, externalName, logt)
if _, notFound := err.(resource.NotFoundError); notFound {
return r.updateStatusError(instance, serviceStateFailed, errors.Wrapf(err, "no service instances with name %s found for alias plan", instance.ObjectMeta.Name))
}
Expand Down Expand Up @@ -323,7 +328,7 @@ func (r *ServiceReconciler) Reconcile(request ctrl.Request) (ctrl.Result, error)
if _, ok := err.(resource.NotFoundError); ok { // Need to recreate it!
if !isAlias(instance) {
logt.Info("Recreating ", instance.ObjectMeta.Name, instance.Spec.ServiceClass)
instance.Status.InstanceID = "IN PROGRESS"
instance.Status.InstanceID = inProgress
if err := r.Status().Update(ctx, instance); err != nil {
logt.Info("Error updating instanceID to be in progress", "Error", err.Error())
return ctrl.Result{}, err
Expand Down Expand Up @@ -476,7 +481,7 @@ func (r *ServiceReconciler) getParams(ctx context.Context, instance *ibmcloudv1b
// paramToJSON converts variable value to JSON value
func (r *ServiceReconciler) paramToJSON(ctx context.Context, p ibmcloudv1beta1.Param, namespace string) (interface{}, error) {
if p.Value != nil && p.ValueFrom != nil {
return nil, fmt.Errorf("value and ValueFrom properties are mutually exclusive (for %s variable)", p.Name)
return nil, fmt.Errorf("Value and ValueFrom properties are mutually exclusive (for %s variable)", p.Name)
}

valueFrom := p.ValueFrom
Expand All @@ -496,18 +501,18 @@ func (r *ServiceReconciler) paramValueToJSON(ctx context.Context, valueFrom ibmc
data, err := getKubeSecretValue(ctx, r, r.Log, valueFrom.SecretKeyRef.Name, valueFrom.SecretKeyRef.Key, true, namespace)
if err != nil {
// Recoverable
return nil, fmt.Errorf("missing secret %s", valueFrom.SecretKeyRef.Name)
return nil, fmt.Errorf("Missing secret %s", valueFrom.SecretKeyRef.Name)
}
return paramToJSONFromString(string(data))
} else if valueFrom.ConfigMapKeyRef != nil {
data, err := getConfigMapValue(ctx, r, r.Log, valueFrom.ConfigMapKeyRef.Name, valueFrom.ConfigMapKeyRef.Key, true, namespace)
if err != nil {
// Recoverable
return nil, fmt.Errorf("missing configmap %s", valueFrom.ConfigMapKeyRef.Name)
return nil, fmt.Errorf("Missing configmap %s", valueFrom.ConfigMapKeyRef.Name)
}
return paramToJSONFromString(data)
}
return nil, fmt.Errorf("missing secretKeyRef or configMapKeyRef")
return nil, fmt.Errorf("Missing secretKeyRef or configMapKeyRef")
}

func getTags(instance *ibmcloudv1beta1.Service) []string {
Expand Down
Loading

0 comments on commit 309f91f

Please sign in to comment.