Docs: external name definition and handling #374

enrico-kaack-comp · 2025-10-17T14:29:25Z

This should document the meaning of external name and its implementation behavior.

Goal is that we have a clear understanding when implementing it, unify the handling of all our MR resources and giving stakeholders a more expectable and repeatable behavior when using external-name especially in import scenarios.

christophrj · 2025-10-17T15:35:36Z

docs/development/external-name-handling.md

+## Background
+
+### Definitions
+Common interpretations of the external name:


I find both of these definitions/interpretations somewhat misleading. The external-name (string) identifies the external resource in the external-system. That is the definition imo (even though i couldn't find it in the docs right now 😄 ). It may be generated by the external system or chosen by the API consumer if the external systems allows specifying the identifier.

https://github.com/crossplane/crossplane/blob/main/design/one-pager-managed-resource-api-design.md#external-resource-name

I agree with your definition, but I could not read it out of any source like the one pager you also referenced.

I would agree with @christophrj here. Definition for me would be:

The external-name (string) identifies the external resource in the external-system.

The two points mentioned are common assumptions associated with that core definition. A third one might be "Ideally represents a unique identifier across the external system" (which isn't always true depending on the API available).

SatabdiG

Great effort documenting this! 🚀 A lot of thought was put into the structure and lifecycle flows.

I've flagged some areas where I think we need more discussion: compound key semantics, format validation trade-offs, and clarifying the empty external-name behavior.

Looking forward to further discussion!!!

SatabdiG · 2025-10-17T19:03:26Z

docs/development/external-name-handling.md

+     - If resource does not exist, it needsCreation.  
+   - Else no backwards compatibility needed, return resourceExist: false.
+2. External-name is set, we check its format.  
+   - If format is not correct (not a valid GUID or not a valid compound key): return error.


Do we need to verify the format for the External Name? Unlike k8, an external name isn't bound to rigid DNS requirements. It can be any identifier that the provider chooses, including ARN. Checking for the sanctity of the external name is hard, in my opinion. What if btp updates their apis in the future?

We could treat external-name as an opaque string. Pass it to the provider's API without validation. The provider will return proper errors if the identifier is invalid.

that sounds reasonable, no need for another layer if the API has to validate this anyway. For the compound key (and we know which MR have compound key), we should check the format since we define it and might wanna use it later for API requests (although in discussion in your other comment https://github.com/SAP/crossplane-provider-btp/pull/374/files#r2441160793)

SatabdiG · 2025-10-17T20:26:44Z

docs/development/external-name-handling.md

+4. Copy the fields from `status.atProvider` to `spec.forProvider` and change `managementPolicies` to `*` to gain full control.
+
+## Our Approach
+


I have some concerns regarding the compound key concept (line 17-24)
The document proposes that when no unique identifier exists, Crossplane should
construct a compound key from spec.forProvider fields (e.g., "org-123|space-prod")
and store it in external-name.

We control the format but it's fragile. (The open questions acknowledges this)

If external-name is derived from spec.forProvider, then updating spec fields
means updating external-name (per Update() section line 88). This makes
external-name mutable and tightly coupled to spec, contradicting it being an
"identifier."

In Update() and Delete(), we must deconstruct the compound key back into its
parts. This parsing logic is error-prone and provider-specific. What if tomorrow btp decides to update it's spec?

I have a concern regarding us constructing our custom external name, The external-name annotation is meant to bridge Crossplane and the provider API.
If we construct our own compound key format (e.g., "org-123|space-prod"), we're
creating an abstraction layer that exists only in Crossplane, not in the provider. This has numerous issues including a potential debuging nightmare:
When something goes wrong, is it a provider issue or our compound key parsing issue? We've introduced a layer that obscures the actual provider interaction.

Fundamentally I think external-name should represent something the provider
understands, not an internal Crossplane encoding scheme. If a provider requires
multiple fields for identification, those should remain explicit in spec.forProvider
and be used directly in API calls.

Great remark, thank you. I fully agree that the compound key is another abstraction layer that open the gates for bugs with parsing etc. Following your thought, instead of compound keys, we would have to leave out the external-name then for resources that dont have an explicit uid. This is definitely closer to the original API but further away from stakeholders way of using our crossplane provider (since some resources work, some resources dont).
But in general, it wouldnt contradict the idea of external-name being unique identifier as the Managed Resource would be marked as no external name support and the stakeholder has to explicitly use the spec. Huge advantage for implmentation.

it is tightly coupled to Update but if the external system does not give a stable identifier, that is the best identifier that we can build I think.

Fully agree with buggy parsing logic. If BTP changes it spec we have to change our spec of the Managed Resource accordingly. But I think we have to do this even if we use the spec entries for identification, since in both case we would need to update to new btp spec.

Thank you for the great conversation! You raise a really valid UX concern about consistency.

1. Provider-specific behavior is already normal
Users already encounter differences across BTP resources (some are immutable, some async, different deletion behaviors). I think "some use external-name, others use spec.forProvider" could fit naturally as another resource characteristic we document.

2. Import might actually be simpler

ID-based: "Here's the BTP ID, put it in external-name" ✅

No-ID: "Fill in these spec fields that identify it" ✅

3. Drift could be tricky
Compound keys mean two places with the same info:

spec.forProvider.spaceName = "staging-space"

external-name = "org-123|staging-space"

If they drift apart (someone edits the annotation, partial update, etc.), how do we handle it? Spec-based keeps one source of truth.

Your points make sense:

"best identifier we can build"

Fair! Though I wonder if we could skip building one and just use spec.forProvider directly?

"BTP changes require spec changes either way"

True! Though I think spec-based keeps changes localized, while compound keys also need parsing logic updates.

Would it help to try both? Maybe prototype CloudManagement with each approach and see which feels better in practice? Happy to help with that!

I think we are landing on two different possible solutions to deal with cases that don't offer a unique ID here:
1. compound keys in external name

closer to crossplane standard

more clear for user to express his intent

parsing logic feels fragile

2. no external-name, lookup using spec fields

no additional implementation effort for existing resources (because implicitly used already)

import capabilities less clear for the user

I just realized there we have some compound keys already that are not actually part of the spec. In case of ServiceManager and Cloudmanagement we aggregated the GUID of the instance and the binding, because those are kind of an helper instance to manage both on behalf of the user.

SatabdiG · 2025-10-17T21:29:22Z

docs/development/external-name-handling.md

+- If we set any kind of initializer ourselves later, we don’t get the default initializer automatically and therefore might run into bugs if we don’t check for empty initializer
+- At this moment, the finalizer would break our definition as the metadata.name is only an identifier within the Kubernetes cluster, not external system. 
+
+**TODO: how do we handle this?**


In my opinion, this is crucial to answer before we consider actual implementation. How we handle this problem should be the baseline for the rest of the document.

When external-name is unset/empty, the controller cannot perform any CRUD operations
because it has no identifier to use in API calls to the provider.

I think we need to answer the following before we can safely implement the pattern:

What should Observe() do when external-name is empty?

Return an error and block reconciliation?

Assume resource doesn't exist and proceed to Create()?

Attempt to use spec.forProvider fields (but this is problematic - described in another comment)?

How do we prevent resource leaks?
Scenario:

Create() succeeds, provider returns ID "vpc-abc123"

Network failure/restart occurs before the external-name annotation is written

Next reconcile: external-name is still empty

If Observe() proceeds to Create() again → duplicate resource created

Reference: Crossplane Issue #2350 documents this exact problem
Composite reconciler initializes with empty string for external name crossplane/crossplane#2350

Before finalizing this design we need to explicitly specify:

The behavior when external-name is empty

Safeguards against the Create() success / annotation write failure scenario

Whether there are any cases where empty external-name is acceptable vs. should error

We definitely need to specify this behavior once decided how to handle the defaulting of external name.

Exactly this is what I hope we get out of this document :)

That is a good scenario: Since we can not guarantee the write operation of external name (nor can guarantee any kind of state), if the external name is empty we have to try to observe based on the spec to potentially match the resource that was created before the network failure. So we have to design our observe in every case to try to observe even if external-name is empty?

Missing external-name means that the resource has been created from scratch. In this case we need to return a notFound and create the resource. Some providers even blindly send this empty or default external-name to the api and rely on it returning 404.

I don't think we can or should prevent such kind of leaks. At least not in a generic way. The identifier should be obtained from the create response and saved into the external-name. If it gets lost the provide attempts another create in the next loop and the results depend on how the APIs deal with that. I'd say leaks like that would indicate problems that need to be solved on another infrastructural layer, rather then and API level.

SatabdiG · 2025-10-17T21:33:24Z

docs/development/external-name-handling.md

+
+Since the matching between MR and external resource is done based on external-name, a wrongly filled spec would result in Update() calls to the resource changing it. To prevent this, the user would not set the managementPolicy to Update. If there is then a missmatch, the Observe() method would determines this, sets the externalObservation.Diff and it would be visible in the debug log. TODO: this should be put as a warning event on the resource maybe, because crossplane-runtime only does a log.Debug with the Diff. As a user adopting existing landscapes it would be good to know if the current spec WOULD trigger an update/diff for confidence.
+
+## Open Questions, Things not considered


General comment About Provider-BTP Implementation:
I had a few general questions:

1. Does BTP actually lack unique identifiers?
Even if BTP's API requires multiple fields to query a resource, the platform
likely assigns GUIDs internally.

2. Can BTP change identifier formats?
If we hardcode compound key construction logic (e.g., "org-123|space-prod"),
what happens when:

BTP adds GUID fields to resources in a future API version?

BTP changes the required identification parameters?

BTP introduces new identifier schemes?

Hardcoded compound key logic couples our provider to the current BTP API structure.
If BTP evolves its APIs, our parsing/construction logic breaks.

Alternatively, we could consider using spec.forProvider directly
Instead of encoding fields into external-name, use spec.forProvider fields
directly in API calls:

More resilient to API changes

Clearer mapping to provider API

No parsing/delimiter collision issues

external-name only populated if BTP returns a true unique identifier

unfortunately yes. No identifier available in the API at least for several resources such as: CloudManagment, Entitlement (no id and we have an additional logic that we map multiple Entitlement MR to one BTP Entitlement), KymaModule (also some kind of different abstraction on top of the API)

could happen of course, but if they change it we need to change our coding anyway to follow the btp api changes I think.

Your overview against using compound external-name and only id external-names is quite good, I will collect it with for/against in a separate comment below :)

Good discussion here, I like it.
Generally speaking we have to deal with whatever the API gives us and our guidelines have to be flexible enough to deal with those different scenarios.
We can't foresee changes that BTP is going to do in the future, but so far they have been quite stable and rolled out only additions that were able to adopt over time. I am hoping they won't just kill existing APIs and if they do, I guess we have other problems then external-name formats ;)

Thank you for providing that clarity! Based on our conversation about Observe(), maybe we have a pattern emerging

We could think about

Resources WITH IDs: external-name = provider ID (simple, opaque)

Resources WITHOUT IDs:

external-name empty or not used for identification

Observe() queries using spec.forProvider fields directly - as discussed

SatabdiG · 2025-10-17T21:35:51Z

docs/development/external-name-handling.md

+
+#### Observe()
+1. Check if external-name is empty.  
+   - If we need backwards compatibility, we create our API query from the spec.forProvider fields.  


When do we "need backwards compatibility"?

How would the controller determine this?

Isn't querying by spec.forProvider exactly what we're trying to avoid with the compound key approach?

Basically for the resources we have right now that dont have external-name support (see #301).
If we add external-name requirement here, we need a migration logic, at least for several versions.

sdischer-sap

Thanks everyone for participating in this, especially @enrico-kaack-comp for drafting this ADR.
I guess the biggest choice we would have to agree on is about having compound key external-names or spec fields.

sdischer-sap · 2025-10-21T09:05:20Z

docs/development/external-name-handling.md

+## Background
+
+### Definitions
+Common interpretations of the external name:


I would agree with @christophrj here. Definition for me would be:

The external-name (string) identifies the external resource in the external-system.

The two points mentioned are common assumptions associated with that core definition. A third one might be "Ideally represents a unique identifier across the external system" (which isn't always true depending on the API available).

sdischer-sap · 2025-10-21T09:07:51Z

docs/development/external-name-handling.md

+
+### Importing Existing External Resources (Official Flow) [3]
+1. Create an MR manifest with `spec.managementPolicies: Observe`.
+2. Add the external-name annotation with the external resource identifier. If not globally unique, also supply distinguishing `spec.forProvider` fields.


forProvider fields should not identify the resource.
They sometimes need to be set to fullfill schema mandatory requirements.

sdischer-sap · 2025-10-21T10:55:34Z

docs/development/external-name-handling.md

+
+Perform the UPDATE request, return error if it returns error.
+
+If the external-name is a compound key, update the compound key. This is necessary if parts of the compound key were updated.


Agree in theory, in practice I don't see a use case for that. Compound keys are essentially identifiers and the API usually should not allow for changing those.

sdischer-sap · 2025-10-21T10:56:37Z

docs/development/external-name-handling.md

+If the response is creation failed – resource already exist, we treat it as an error. In the next reconcile Observe(), the resource will be shown as resourceExist: true.
+
+#### Update()
+If the diff contains a non-updateable field, we return error without executing the update, staying in an error loop until the user fixes the wrong state. This would put the resource into Ready: false, representing the inability to correct the drift.


Thats an intersting though. Up until I think we mostly either ignore drifts from none updatable fields or prevent them from changing on a schema level.

sdischer-sap · 2025-10-21T12:26:04Z

docs/development/external-name-handling.md

+
+Since the matching between MR and external resource is done based on external-name, a wrongly filled spec would result in Update() calls to the resource changing it. To prevent this, the user would not set the managementPolicy to Update. If there is then a missmatch, the Observe() method would determines this, sets the externalObservation.Diff and it would be visible in the debug log. TODO: this should be put as a warning event on the resource maybe, because crossplane-runtime only does a log.Debug with the Diff. As a user adopting existing landscapes it would be good to know if the current spec WOULD trigger an update/diff for confidence.
+
+## Open Questions, Things not considered


Good discussion here, I like it.
Generally speaking we have to deal with whatever the API gives us and our guidelines have to be flexible enough to deal with those different scenarios.
We can't foresee changes that BTP is going to do in the future, but so far they have been quite stable and rolled out only additions that were able to adopt over time. I am hoping they won't just kill existing APIs and if they do, I guess we have other problems then external-name formats ;)

sdischer-sap · 2025-10-21T12:39:25Z

docs/development/external-name-handling.md

+- If we set any kind of initializer ourselves later, we don’t get the default initializer automatically and therefore might run into bugs if we don’t check for empty initializer
+- At this moment, the finalizer would break our definition as the metadata.name is only an identifier within the Kubernetes cluster, not external system. 
+
+**TODO: how do we handle this?**


Missing external-name means that the resource has been created from scratch. In this case we need to return a notFound and create the resource. Some providers even blindly send this empty or default external-name to the api and rely on it returning 404.

I don't think we can or should prevent such kind of leaks. At least not in a generic way. The identifier should be obtained from the create response and saved into the external-name. If it gets lost the provide attempts another create in the next loop and the results depend on how the APIs deal with that. I'd say leaks like that would indicate problems that need to be solved on another infrastructural layer, rather then and API level.

sdischer-sap · 2025-10-21T12:46:20Z

docs/development/external-name-handling.md

+4. Copy the fields from `status.atProvider` to `spec.forProvider` and change `managementPolicies` to `*` to gain full control.
+
+## Our Approach
+


I think we are landing on two different possible solutions to deal with cases that don't offer a unique ID here:
1. compound keys in external name

closer to crossplane standard

more clear for user to express his intent

parsing logic feels fragile

2. no external-name, lookup using spec fields

no additional implementation effort for existing resources (because implicitly used already)

import capabilities less clear for the user

sdischer-sap · 2025-10-21T12:52:04Z

docs/development/external-name-handling.md

+4. Copy the fields from `status.atProvider` to `spec.forProvider` and change `managementPolicies` to `*` to gain full control.
+
+## Our Approach
+


I just realized there we have some compound keys already that are not actually part of the spec. In case of ServiceManager and Cloudmanagement we aggregated the GUID of the instance and the binding, because those are kind of an helper instance to manage both on behalf of the user.

enrico-kaack-comp added 3 commits October 17, 2025 15:06

first draft

77e1f52

add open questions

68b45ff

formating/language

ba530b8

enrico-kaack-comp requested a deployment to pr-e2e-approval October 17, 2025 14:29 — with GitHub Actions Waiting

enrico-kaack-comp added the do-not-merge On PRs, this prevents merging through the status checks label Oct 17, 2025

enrico-kaack-comp self-assigned this Oct 17, 2025

christophrj reviewed Oct 17, 2025

View reviewed changes

SatabdiG requested changes Oct 17, 2025

View reviewed changes

whitespace treatment + export tool considerations

920ea25

gergely-szabo-sap requested a deployment to pr-e2e-approval October 20, 2025 07:46 — with GitHub Actions Waiting

dattito mentioned this pull request Oct 20, 2025

(ExternalName) Entitlement Add full external-name support #315

Open

4 tasks

sdischer-sap reviewed Oct 21, 2025

View reviewed changes

		4. Copy the fields from `status.atProvider` to `spec.forProvider` and change `managementPolicies` to `*` to gain full control.

		## Our Approach


		Since the matching between MR and external resource is done based on external-name, a wrongly filled spec would result in Update() calls to the resource changing it. To prevent this, the user would not set the managementPolicy to Update. If there is then a missmatch, the Observe() method would determines this, sets the externalObservation.Diff and it would be visible in the debug log. TODO: this should be put as a warning event on the resource maybe, because crossplane-runtime only does a log.Debug with the Diff. As a user adopting existing landscapes it would be good to know if the current spec WOULD trigger an update/diff for confidence.

		## Open Questions, Things not considered


		Perform the UPDATE request, return error if it returns error.

		If the external-name is a compound key, update the compound key. This is necessary if parts of the compound key were updated.

Docs: external name definition and handling #374

Are you sure you want to change the base?

Docs: external name definition and handling #374

Uh oh!

Conversation

enrico-kaack-comp commented Oct 17, 2025

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!

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!

SatabdiG Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

SatabdiG Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

sdischer-sap 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

SatabdiG Oct 21, 2025 •

edited

Loading

SatabdiG Oct 21, 2025 •

edited

Loading