SpaceRole External-Name Handling

.github/workflows/e2e_test.yaml

@@ -66,10 +66,14 @@ jobs:
       - name: Install gettext for envsubst
         run: sudo apt-get update && sudo apt-get install -y gettext
 
+      - name: Set BUILD_ID
+        run: echo "BUILD_ID=${{ github.run_number }}" >> $GITHUB_ENV
+
       - name: Run build and e2e test
         run: make test-acceptance
         env:
           CF_CREDENTIALS: ${{ secrets.CF_CREDENTIALS }}
           CF_ENVIRONMENT: ${{ secrets.CF_ENVIRONMENT }}
+          BUILD_ID: ${{ env.BUILD_ID }}
           # USER_WHERE_ROLES_GET_ASSIGNED_EMAIL: ${{ secrets.USER_WHERE_ROLES_GET_ASSIGNED_EMAIL }}
           # TECHNICAL_USER_EMAIL: ${{ secrets.TECHNICAL_USER_EMAIL }}

internal/clients/role/spacerole.go

@@ -11,7 +11,6 @@ import (
 	"github.com/cloudfoundry/go-cfclient/v3/resource"
 
 	"github.com/SAP/crossplane-provider-cloudfoundry/apis/resources/v1alpha1"
-	"github.com/SAP/crossplane-provider-cloudfoundry/internal/clients"
 )
 
 const ErrSpaceNotSpecified = "space is not specified"
@@ -33,15 +32,12 @@ func SpaceRoleType(roleType string) resource.SpaceRoleType {
 }
 
 // GetSpaceRole returns the role of a user in a space by guid or by matching the spec
-func GetSpaceRole(ctx context.Context, client Role, guid string, spec v1alpha1.SpaceRoleParameters) (*resource.Role, error) {
-	if clients.IsValidGUID(guid) {
-		return client.Get(ctx, guid)
-	}
-	return findSpaceRole(ctx, client, spec)
+func GetSpaceRole(ctx context.Context, client Role, guid string) (*resource.Role, error) {
+	return client.Get(ctx, guid)
 }
 
 // searchSpaceRole returns the role of a user in a space if the role matches the spec
-func findSpaceRole(ctx context.Context, client Role, spec v1alpha1.SpaceRoleParameters) (*resource.Role, error) {
+func FindSpaceRole(ctx context.Context, client Role, spec v1alpha1.SpaceRoleParameters) (*resource.Role, error) {
 
 	opt, err := newSpaceRoleListOptions(spec)
 	if err != nil {

internal/controller/spacerole/controller.go

@@ -2,6 +2,7 @@ package spacerole
 
 import (
 	"context"
+	"fmt"
 
 	"github.com/pkg/errors"
 
@@ -125,9 +126,33 @@ func (c *external) Observe(ctx context.Context, mg resource.Managed) (managed.Ex
 		return managed.ExternalObservation{}, errors.New(errWrongKind)
 	}
 
-	// Fetch the role object using the CloudFoundry API by guid or according to the specified parameters
+	resourceLateInitialized := false
+
+	// ADR Step 1: Check if external-name is empty
+	if meta.GetExternalName(cr) == "" {
+		// Backwards compatibility required as previously lookup by spec was supported
+		r, err := role.FindSpaceRole(ctx, c.role, cr.Spec.ForProvider)
+		if err != nil {
+			if clients.ErrorIsNotFound(err) {
+				return managed.ExternalObservation{ResourceExists: false}, nil
+			}
+			return managed.ExternalObservation{}, errors.Wrap(err, errGet)
+		}
+		if r == nil {
+			return managed.ExternalObservation{ResourceExists: false}, nil
+		}
+		meta.SetExternalName(cr, r.GUID)
+		resourceLateInitialized = true
+	}
+
 	guid := meta.GetExternalName(cr)
-	r, err := role.GetSpaceRole(ctx, c.role, guid, cr.Spec.ForProvider)
+
+	// ADR Step 2: External-name is set, check its format (must be valid GUID)
+	if !clients.IsValidGUID(guid) {
+		return managed.ExternalObservation{}, errors.New(fmt.Sprintf("external-name '%s' is not a valid GUID format", guid))
+	}
+	// ADR Step 3: Build the Get API Request from the external-name (using GUID directly)
+	r, err := role.GetSpaceRole(ctx, c.role, guid)
 
 	if err != nil {
 		if clients.ErrorIsNotFound(err) {
@@ -140,17 +165,11 @@ func (c *external) Observe(ctx context.Context, mg resource.Managed) (managed.Ex
 		return managed.ExternalObservation{ResourceExists: false}, nil
 	}
 
-	resourceLateInitialized := false
-	if guid != r.GUID {
-		meta.SetExternalName(cr, r.GUID)
-		resourceLateInitialized = true
-	}
-
 	cr.Status.AtProvider = role.GenerateSpaceRoleObservation(r)
 	cr.Status.SetConditions(xpv1.Available())
 
 	return managed.ExternalObservation{
-		ResourceExists:          cr.Status.AtProvider.ID != nil,
+		ResourceExists:          meta.GetExternalName(cr) != "",
 		ResourceUpToDate:        true,
 		ResourceLateInitialized: resourceLateInitialized,
 	}, nil
@@ -207,13 +226,17 @@ func (c *external) Delete(ctx context.Context, mg resource.Managed) (managed.Ext
 	// TODO
 
 	cr.SetConditions(xpv1.Deleting())
-	if cr.Status.AtProvider.ID == nil {
+	if meta.GetExternalName(cr) == "" {
 		return managed.ExternalDelete{}, nil
 	}
 
 	// Delete is async and we need to implement wait for deletion
-	jobGUID, err := c.role.Delete(ctx, *cr.Status.AtProvider.ID)
+	jobGUID, err := c.role.Delete(ctx, meta.GetExternalName(cr))
 	if err != nil {
+		// ADR: 404 not found means already deleted - not considered as error case
+		if clients.ErrorIsNotFound(err) {
+			return managed.ExternalDelete{}, nil
+		}
 		return managed.ExternalDelete{}, errors.Wrap(err, errDelete)
 	}
 

internal/controller/spacerole/controller_test.go

@@ -5,6 +5,7 @@ import (
 	"testing"
 
 	cfresource "github.com/cloudfoundry/go-cfclient/v3/resource"
+	xpv1 "github.com/crossplane/crossplane-runtime/apis/common/v1"
 	"github.com/crossplane/crossplane-runtime/pkg/resource"
 	"github.com/google/go-cmp/cmp"
 	"github.com/pkg/errors"
@@ -88,6 +89,16 @@ func withExternalName(name string) modifier {
 	}
 }
 
+func withID(id string) modifier {
+	return func(r *v1alpha1.SpaceRole) {
+		r.Status.AtProvider.ID = ptr.To(id)
+	}
+}
+
+func withConditions(c ...xpv1.Condition) modifier {
+	return func(i *v1alpha1.SpaceRole) { i.Status.SetConditions(c...) }
+}
+
 func fakeSpaceRole(m ...modifier) *v1alpha1.SpaceRole {
 	r := &v1alpha1.SpaceRole{
 		ObjectMeta: metav1.ObjectMeta{
@@ -183,46 +194,49 @@ func TestObserve(t *testing.T) {
 				return m
 			},
 		},
-		"NotFound by uuid is not found": {
+		"UnsetExternalNameSuccesful": {
 			args: args{
-				mg: fakeSpaceRole(withSpace("my-space"), withExternalName(guidRole)),
+				mg: fakeSpaceRole(withSpace("my-space"), withUsername("user1"), withType(v1alpha1.SpaceManager)),
 			},
 			want: want{
-				mg:  fakeSpaceRole(withSpace("my-space"), withExternalName(guidRole)),
-				obs: managed.ExternalObservation{ResourceExists: false, ResourceUpToDate: false, ResourceLateInitialized: false},
+				mg:  fakeSpaceRole(withSpace("my-space"), withUsername("user1"), withType(v1alpha1.SpaceManager), withExternalName(guidRole)),
+				obs: managed.ExternalObservation{ResourceExists: true, ResourceUpToDate: true, ResourceLateInitialized: true},
 				err: nil,
 			},
 			service: func() *fake.MockSpaceRole {
 				m := &fake.MockSpaceRole{}
 
+				m.On("ListIncludeUsersAll").Return(
+					[]*cfresource.Role{healthyRole},
+					[]*cfresource.User{healthyUser},
+					nil,
+				)
+
 				m.On("Get", guidRole).Return(
-					fake.SpaceRoleNil,
+					healthyRole,
 					nil,
 				)
 				return m
 			},
 		},
-		"Successful": {
+		"UnsetExternalNameNotFound": {
 			args: args{
-				mg: fakeSpaceRole(withSpace("my-space"), withUsername("user1"), withType(v1alpha1.SpaceManager)),
+				mg: fakeSpaceRole(withSpace(guidSpace), withUsername("user1"), withType(v1alpha1.SpaceManager)),
 			},
 			want: want{
-				mg:  fakeSpaceRole(withSpace("my-space"), withUsername("user1"), withType(v1alpha1.SpaceManager), withExternalName(guidRole)),
-				obs: managed.ExternalObservation{ResourceExists: true, ResourceUpToDate: true, ResourceLateInitialized: true},
+				mg:  fakeSpaceRole(withSpace(guidSpace), withUsername("user1"), withType(v1alpha1.SpaceManager)),
+				obs: managed.ExternalObservation{ResourceExists: false},
 				err: nil,
 			},
 			service: func() *fake.MockSpaceRole {
 				m := &fake.MockSpaceRole{}
-
-				m.On("ListIncludeUsersAll").Return(
-					[]*cfresource.Role{healthyRole},
-					[]*cfresource.User{healthyUser},
-					nil,
-				)
+				var emptyRoles []*cfresource.Role
+				var emptyUsers []*cfresource.User
+				m.On("ListIncludeUsersAll").Return(emptyRoles, emptyUsers, nil)
 				return m
 			},
 		},
-		"Successful when SpaceRole guid is found": {
+		"SetExternalNameSuccesful": {
 			args: args{
 				mg: fakeSpaceRole(withSpace("my-space"), withUsername("user1"), withType(v1alpha1.SpaceManager), withExternalName(guidRole)),
 			},
@@ -241,6 +255,37 @@ func TestObserve(t *testing.T) {
 				return m
 			},
 		},
+		"SetExternalNameNotFound": {
+			args: args{
+				mg: fakeSpaceRole(withExternalName(guidRole)),
+			},
+			want: want{
+				mg:  fakeSpaceRole(withExternalName(guidRole)),
+				obs: managed.ExternalObservation{ResourceExists: false},
+				err: nil,
+			},
+			service: func() *fake.MockSpaceRole {
+				m := &fake.MockSpaceRole{}
+				m.On("Get", guidRole).Return(
+					fake.SpaceRoleNil,
+					errors.New("CF-ResourceNotFound: The role was not found"),
+				)
+				return m
+			},
+		},
+		"SetExternalNameInvalidFormat": {
+			args: args{
+				mg: fakeSpaceRole(withExternalName("not-a-valid-guid")),
+			},
+			want: want{
+				mg:  fakeSpaceRole(withExternalName("not-a-valid-guid")),
+				obs: managed.ExternalObservation{},
+				err: errors.New("external-name 'not-a-valid-guid' is not a valid GUID format"),
+			},
+			service: func() *fake.MockSpaceRole {
+				return &fake.MockSpaceRole{}
+			},
+		},
 	}
 
 	for n, tc := range cases {
@@ -454,15 +499,124 @@ func TestCreate(t *testing.T) {
 			if tc.want.err != nil && err != nil {
 				// the case where our mock server returns error.
 				if diff := cmp.Diff(tc.want.err.Error(), err.Error()); diff != "" {
-					t.Errorf("Observe(...): want error string != got error string:\n%s", diff)
+					t.Errorf("Create(...): want error string != got error string:\n%s", diff)
 				}
 			} else {
 				if diff := cmp.Diff(tc.want.err, err); diff != "" {
-					t.Errorf("Observe(...): want error != got error:\n%s", diff)
+					t.Errorf("Create(...): want error != got error:\n%s", diff)
 				}
 			}
 			if diff := cmp.Diff(tc.want.obs, obs); diff != "" {
-				t.Errorf("Observe(...): -want, +got:\n%s", diff)
+				t.Errorf("Create(...): -want, +got:\n%s", diff)
+			}
+		})
+	}
+}
+
+func TestDelete(t *testing.T) {
+	type service func() *fake.MockSpaceRole
+	type args struct {
+		mg resource.Managed
+	}
+
+	type want struct {
+		mg  resource.Managed
+		obs managed.ExternalDelete
+		err error
+	}
+
+	cases := map[string]struct {
+		args    args
+		want    want
+		service service
+		kube    k8s.Client
+	}{
+		"SuccessfulDelete": {
+			args: args{
+				mg: fakeSpaceRole(withExternalName(guidRole), withID("resource-id")),
+			},
+			want: want{
+				mg:  fakeSpaceRole(withExternalName(guidRole), withID("resource-id"), withConditions(xpv1.Deleting())),
+				obs: managed.ExternalDelete{},
+				err: nil,
+			},
+			service: func() *fake.MockSpaceRole {
+				m := &fake.MockSpaceRole{}
+				m.On("Delete").Return(
+					"job-guid-123",
+					nil,
+				)
+				return m
+			},
+		},
+		"404NotFound": {
+			args: args{
+				mg: fakeSpaceRole(withExternalName(guidRole), withID("resource-id")),
+			},
+			want: want{
+				mg:  fakeSpaceRole(withExternalName(guidRole), withID("resource-id"), withConditions(xpv1.Deleting())),
+				obs: managed.ExternalDelete{},
+				err: nil,
+			},
+			service: func() *fake.MockSpaceRole {
+				m := &fake.MockSpaceRole{}
+				m.On("Delete").Return(
+					"",
+					errors.New("CF-ResourceNotFound: The role was not found"),
+				)
+				return m
+			},
+		},
+		"Error": {
+			args: args{
+				mg: fakeSpaceRole(withExternalName(guidRole), withID("resource-id")),
+			},
+			want: want{
+				mg:  fakeSpaceRole(withExternalName(guidRole), withID("resource-id"), withConditions(xpv1.Deleting())),
+				obs: managed.ExternalDelete{},
+				err: errors.Wrap(errors.New("CF-ResourceNotDeleted: The role could not be deleted"), errDelete),
+			},
+			service: func() *fake.MockSpaceRole {
+				m := &fake.MockSpaceRole{}
+				m.On("Delete").Return(
+					"",
+					errors.New("CF-ResourceNotDeleted: The role could not be deleted"),
+				)
+				return m
+			},
+		},
+	}
+	for n, tc := range cases {
+		t.Run(n, func(t *testing.T) {
+			t.Logf("Testing: %s", t.Name())
+			mockJob := &fake.MockJob{}
+			mockJob.On("PollComplete").Return(nil)
+
+			c := &external{
+				kube: &test.MockClient{
+					MockDelete: test.NewMockDeleteFn(nil),
+				},
+				job:  mockJob,
+				role: tc.service(),
+			}
+
+			obs, err := c.Delete(context.Background(), tc.args.mg)
+
+			if tc.want.err != nil && err != nil {
+				// the case where our mock server returns error.
+				if diff := cmp.Diff(tc.want.err.Error(), err.Error()); diff != "" {
+					t.Errorf("Delete(...): want error string != got error string:\n%s", diff)
+				}
+			} else {
+				if diff := cmp.Diff(tc.want.err, err); diff != "" {
+					t.Errorf("Delete(...): want error != got error:\n%s", diff)
+				}
+			}
+			if diff := cmp.Diff(tc.want.obs, obs); diff != "" {
+				t.Errorf("Delete(...): -want, +got:\n%s", diff)
+			}
+			if diff := cmp.Diff(tc.want.mg, tc.args.mg); diff != "" {
+				t.Errorf("Delete(...): -want, +got:\n%s", diff)
 			}
 		})
 	}