Merge pull request #103 from SAP/fix/si-importing

fix: fixing crash for external-name imported ServiceInstances

Author: Lasserich

internal/clients/serviceinstance/serviceinstance.go

@@ -311,13 +311,13 @@ func UpdateObservation(in *v1alpha1.ServiceInstanceObservation, r *resource.Serv
 
 // IsUpToDate checks if the managed resource is in sync with CR.
 func IsUpToDate(in *v1alpha1.ServiceInstanceParameters, observed *resource.ServiceInstance) bool {
-	if *in.Name != observed.Name {
+	if in.Name != nil && *in.Name != observed.Name {
 		return false
 	}
 
 	switch in.Type {
 	case v1alpha1.ManagedService:
-		if in.ServicePlan.ID != nil && observed.Relationships.ServicePlan.Data.GUID != *in.ServicePlan.ID {
+		if in.ServicePlan != nil && in.ServicePlan.ID != nil && observed.Relationships.ServicePlan.Data.GUID != *in.ServicePlan.ID {
 			return false
 		}
 	case v1alpha1.UserProvidedService:

internal/controller/serviceinstance/controller.go

@@ -4,7 +4,6 @@ import (
 	"bytes"
 	"context"
 	"crypto/sha256"
-	"encoding/json"
 	"time"
 
 	"github.com/cloudfoundry/go-cfclient/v3/client"
@@ -16,6 +15,7 @@ import (
 	"github.com/crossplane/crossplane-runtime/pkg/ratelimiter"
 	"github.com/crossplane/crossplane-runtime/pkg/reconciler/managed"
 	"github.com/crossplane/crossplane-runtime/pkg/resource"
+	"github.com/google/uuid"
 	"github.com/nsf/jsondiff"
 	"github.com/pkg/errors"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -31,19 +31,20 @@ import (
 )
 
 const (
-	resourceType     = "ServiceInstance"
-	externalSystem   = "Cloud Foundry"
-	errTrackPCUsage  = "cannot track ProviderConfig usage"
-	errNewClient     = "cannot create a client for " + externalSystem
-	errWrongCRType   = "managed resource is not a " + resourceType
-	errUpdateCR      = "cannot update the managed resource"
-	errGet           = "cannot get " + resourceType + " in " + externalSystem
-	errCreate        = "cannot create " + resourceType + " in " + externalSystem
-	errUpdate        = "cannot update " + resourceType + " in " + externalSystem
-	errDelete        = "cannot delete " + resourceType + " in " + externalSystem
-	errCleanFailed   = "cannot delete failed service instance"
-	errSecret        = "cannot resolve secret reference"
-	errGetParameters = "cannot get parameters of the service instance for drift detection. Please check this is supported or set enableParameterDriftDetection to false."
+	resourceType          = "ServiceInstance"
+	externalSystem        = "Cloud Foundry"
+	errTrackPCUsage       = "cannot track ProviderConfig usage"
+	errNewClient          = "cannot create a client for " + externalSystem
+	errWrongCRType        = "managed resource is not a " + resourceType
+	errUpdateCR           = "cannot update the managed resource"
+	errGet                = "cannot get " + resourceType + " in " + externalSystem
+	errCreate             = "cannot create " + resourceType + " in " + externalSystem
+	errUpdate             = "cannot update " + resourceType + " in " + externalSystem
+	errDelete             = "cannot delete " + resourceType + " in " + externalSystem
+	errCleanFailed        = "cannot delete failed service instance"
+	errSecret             = "cannot resolve secret reference"
+	errGetParameters      = "cannot get parameters of the service instance for drift detection. Please check this is supported or set enableParameterDriftDetection to false."
+	errMissingServicePlan = "managed resource service instance requires a service plan"
 )
 
 // Setup adds a controller that reconciles ServiceInstance CR.
@@ -374,37 +375,41 @@ func (s servicePlanInitializer) Initialize(ctx context.Context, mg resource.Mana
 		return nil
 	}
 
-	if cr.Spec.ForProvider.ServicePlan == nil {
-		// fallback on crossplane.io/external-data annotation for backward compatibility
-		sp := struct {
-			ServicePlan *v1alpha1.ServicePlanParameters `json:"service_plan"`
-		}{ServicePlan: &v1alpha1.ServicePlanParameters{}}
+	if cr.Spec.ForProvider.ServicePlan != nil {
+		// When ServicePlan is set we populate the service
+		// plan ID based on the external resource GUID.
+		cf, err := clients.ClientFnBuilder(ctx, s.kube)(mg)
+		if err != nil {
+			return errors.Wrapf(err, errNewClient)
+		}
 
-		if data, ok := mg.GetAnnotations()["crossplane.io/external-data"]; ok {
-			if err := json.Unmarshal([]byte(data), &sp); err == nil {
-				cr.Spec.ForProvider.ServicePlan = sp.ServicePlan
-			}
+		opt := client.NewServicePlanListOptions()
+		if cr.Spec.ForProvider.ServicePlan.Offering != nil {
+			opt.ServiceOfferingNames.EqualTo(*cr.Spec.ForProvider.ServicePlan.Offering)
+		}
+		if cr.Spec.ForProvider.ServicePlan.Plan != nil {
+			opt.Names.EqualTo(*cr.Spec.ForProvider.ServicePlan.Plan)
+		}
+		sp, err := cf.ServicePlans.Single(ctx, opt)
+		if err != nil {
+			return errors.Wrapf(err, "Cannot initialize service plan using serviceName/servicePlanName: %s:%s`", *cr.Spec.ForProvider.ServicePlan.Offering, *cr.Spec.ForProvider.ServicePlan.Plan)
 		}
-	}
 
-	//  Lookup service plan during every reconciliation to detect an update service_plan of existing service.
-	cf, err := clients.ClientFnBuilder(ctx, s.kube)(mg)
-	if err != nil {
-		return errors.Wrapf(err, errNewClient)
-	}
-	opt := client.NewServicePlanListOptions()
-	opt.ServiceOfferingNames.EqualTo(*cr.Spec.ForProvider.ServicePlan.Offering)
-	opt.Names.EqualTo(*cr.Spec.ForProvider.ServicePlan.Plan)
+		cr.Spec.ForProvider.ServicePlan.ID = &sp.GUID
 
-	// There must be exactly one matching service plan
-	sp, err := cf.ServicePlans.Single(ctx, opt)
-	if err != nil {
-		return errors.Wrapf(err, "Cannot initialize service plan using serviceName/servicePlanName: %s:%s`", *cr.Spec.ForProvider.ServicePlan.Offering, *cr.Spec.ForProvider.ServicePlan.Plan)
+		return s.kube.Update(ctx, cr)
 	}
 
-	cr.Spec.ForProvider.ServicePlan.ID = &sp.GUID
+	// Service plan is not set
+	guid := meta.GetExternalName(cr)
+
+	if _, err := uuid.Parse(guid); err == nil {
+		// We have a valid external-name annotation
+		return nil
+	}
 
-	return s.kube.Update(ctx, cr)
+	// No valid external-name annotation
+	return errors.New(errMissingServicePlan)
 }
 
 // Small wrapper around sha256.Sum256()