ServiceInstance External-Name Handling by Kukkerem · Pull Request #311 · SAP/crossplane-provider-cloudfoundry

Kukkerem · 2026-06-16T11:08:48Z

Closes: #246

Implemented correct external-name handling in Create and Delete function
Adapted Unit Tests for Create and Delete function
Created E2E Import Test
Created Upgrade Test for External-Name Behavior
Added documentation

Signed-off-by: Zoltan Szabo <zoltan.szabo03@sap.com>

SatabdiG

Good work, Some comments from my side.

SatabdiG · 2026-06-17T08:26:46Z

+//   - Follows Standard: yes
+//   - Format: ServiceInstance GUID (UUID format)
+//   - How to find:
+//   - UI: In the BTP Cockpit, navigate to your service instance; the GUID is the ID in the URL


I think there is an indentation off here, UI and CLI are children of How to find right?

Other resources has the same because gofmt rewrites the comment in case of nesting. We could definitely work on this, but currently gofmt would fail.

SatabdiG · 2026-06-17T08:58:01Z

+
 // MatchSingle retrieves external resource by matching CR's ForProvider spec
 func (c *Client) MatchSingle(ctx context.Context, spec v1alpha1.ServiceInstanceParameters) (*resource.ServiceInstance, error) {
+	// Backward-compat lookup requires at least a name to match on.


I know you didn't touch this part, but reading it together, we might have a bug.

I see pollJobComplete intentionally swallows async timeout errors (comment explains this is by design, deferring to later state observation), and the subsequent MatchSingle call returns (nil, nil) when no match is found yet — treating "not visible yet" the same as "doesn't exist".

So c.serviceinstance.Create can return (nil, nil) for a managed service instance whose creation is still in progress when the poll times out. err is nil, so the guard above doesn't catch it, and r.GUID panics.

Maybe we can safeguard it here, make external name handling more robust, what do you think?

Very true, fixed with a new UT!

SatabdiG · 2026-06-17T09:01:01Z

-		return managed.ExternalDelete{}, errors.New(errDelete)
+	// ADR: the external-name (GUID) identifies the resource to delete; nothing to do if unset.
+	guid := meta.GetExternalName(cr)
+	if guid == "" {


minor: Should we also validate the guid here? Worse case here is CF-side crash but still.

Good point, fixed as well.

SatabdiG · 2026-06-17T09:03:32Z

@@ -140,19 +140,25 @@ func (c *external) Observe(ctx context.Context, mg resource.Managed) (managed.Ex
 		return managed.ExternalObservation{}, errors.New(errWrongCRType)
 	}


nit: You didn;t touch part of this code:

if _, err := uuid.Parse(guid); err == nil { // We have a valid external-name annotation return nil }

but should we replace uuid.Parse(guid) with clients.IsValidGUID for a single source of truth on what counts as a valid external-nam

True, fixed, dropped the now unused uuid.

SatabdiG · 2026-06-17T09:15:05Z

 			},
 			want: want{
-				mg:  serviceInstance("managed", withExternalName(guid)),
+				mg:  serviceInstance("managed", withExternalName(guid), withSpace(spaceGUID), withServicePlan(v1alpha1.ServicePlanParameters{ID: &servicePlan})),


I see TestObserve, TestCreate, TestCreate case structs declare the kube k8s.Client.

But the test runner never reads tc.kube; it always constructs a hardcoded &test.MockClient{MockUpdate: test.NewMockUpdateFn(nil)}. Wouldn't that mean there's currently no way to simulate c.kube.Update failing? So in return the Observe's adoption path (meta.SetExternalName + c.kube.Update) and postCreate (c.kube.Update then c.kube.Status().Update) both have an errUpdateCR error path that would never exercised?

Good catch, fixed!

SatabdiG · 2026-06-17T09:56:37Z

+
+				serviceInstance := &v1alpha1.ServiceInstance{}
+
+				err = r.Get(ctx, serviceInstanceName, cfg.Namespace(), serviceInstance)


Should we also use SchemeBuilder here same as PreUpgrade

AddToScheme already covers PostUpgrade I think, same as in the other resources, but we could mirror App if that's preferred.

Signed-off-by: Zoltan Szabo <zoltan.szabo03@sap.com>

feat(serviceinstance): implement ADR-compliant external-name handling

248ec06

Signed-off-by: Zoltan Szabo <zoltan.szabo03@sap.com>

Kukkerem requested review from SatabdiG and lennardrother June 16, 2026 11:08

Kukkerem self-assigned this Jun 16, 2026

Kukkerem requested a deployment to pr-e2e-approval June 16, 2026 11:08 — with GitHub Actions Waiting

Merge branch 'main' into external-name-comp-serviceinstance

92f8225

Kukkerem requested a deployment to pr-e2e-approval June 17, 2026 05:00 — with GitHub Actions Waiting

lennardrother reviewed Jun 17, 2026

View reviewed changes

Comment thread apis/resources/v1alpha1/serviceinstance_types.go Outdated

Comment thread internal/controller/serviceinstance/controller_test.go

Comment thread test/upgrade/service_instance_external_name_upgrade_test.go Outdated

chore(serviceinstance): address review feedback

1b27a9c

Signed-off-by: Zoltan Szabo <zoltan.szabo03@sap.com>

Kukkerem requested a deployment to pr-e2e-approval June 17, 2026 08:56 — with GitHub Actions Waiting

Kukkerem requested a review from lennardrother June 17, 2026 08:57

SatabdiG reviewed Jun 17, 2026

View reviewed changes

chore(serviceinstance): address review feedback

d04a8f6

Signed-off-by: Zoltan Szabo <zoltan.szabo03@sap.com>

Kukkerem requested a deployment to pr-e2e-approval June 17, 2026 12:56 — with GitHub Actions Waiting

		@@ -140,19 +140,25 @@ func (c *external) Observe(ctx context.Context, mg resource.Managed) (managed.Ex
		return managed.ExternalObservation{}, errors.New(errWrongCRType)
		}


		serviceInstance := &v1alpha1.ServiceInstance{}

		err = r.Get(ctx, serviceInstanceName, cfg.Namespace(), serviceInstance)

Conversation

Kukkerem commented Jun 16, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SatabdiG left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants