feat(servicebinding): ensure External Name ADR compliance for ServiceBinding

[gergely-szabo-sap]

Description

This issue tracks the implementation work needed to ensure the Directory resource fully complies with the External Name Handling ADR. The goal is to verify and implement proper external name handling across all CRUD operations according to the established guidelines.

Tasks

Please note, for all tasks the External Name Handling ADR acts as a source of truth, the details here are for just for reference. If deviating from the ADR, please note why.

1. Implement Proper External Name Handling

Acceptance Criteria:

• Observe() Method Implementation

• Create() Method Implementation

• Update() Method Implementation

• Delete() Method Implementation

• Review Migration Logic:

  • Evaluate existing migration logic (introduced in v1.4.0) against ADR requirements
  • Update or remove as needed to ensure ADR compliance
  • Update documentation for any breaking changes

2. Unit Test Coverage for External Name Handling

The code is is using upjetted resource with upjet-specific importing mechanism. I haven't touched the test codebase.

Acceptance Criteria:

• Observe() Tests:

  • Test empty external-name scenarios (witch backwards compatibility)
  • Test invalid GUID format in external-name
  • Test valid GUID that doesn't exist (404 response) → should return resourceExists: false
  • Test valid GUID that exists → normal observation flow
  • Test drift detection and diff reporting

• Create() Tests:

  • Test successful creation sets external-name to returned GUID
  • Test "resource already exists" error handling (external-name should not be set)
  • Test other creation errors (external-name should not be set)

• Update() Tests:

  • Test update with valid existing external-name
  • Test validation of immutable fields (if any)

• Delete() Tests:

  • Test deletion with valid external-name
  • Test 404 response handling (should not be treated as error)
  • Test async deletion scenarios with deletion states
  • Test deletion of externally removed resource

3. E2E Import Test Coverage

Acceptance Criteria:

• Import Flow Test:
  • Test importing existing ServiceBinding resources using the external-name annotation
  • Verify imported resources transition to healthy state with correct external-name
  • Test that imported resources can be observed and updated properly
  • Take the resuable import test in test/e2e/import_utils.go, already used in test/e2e/subaccount_test.go .

4. Upgrade Test Coverage for External Name Behavior

This PR adds only tests and documentation. No need for upgrade testing.

If this part is blocked by adding the upgrade tests to btp-provider, move this to a follow up issue.

Acceptance Criteria:

• External Name Format Migration Test:

• Backward Compatibility Validation:

  • Test that existing Subaccount resources continue to function after provider upgrade
  • Verify external-name migration logic (if implemented) works correctly
  • Ensure imported resources maintain proper external-name format post-upgrade

Implementation Notes

1. ADR Compliance: Ensure all implementation follows the patterns and requirements defined in the External Name Handling ADR.

2. GUID Validation: Implement proper validation for external-name format checking (GUID format expected).

3. Error Handling: Ensure API errors are properly wrapped and provide clear messages to users about external-name related issues.

4. Backward Compatibility: Evaluate existing migration logic and determine appropriate approach for maintaining or updating compatibility. This includes the already existing migration logic introduced in v1.4.0.

Related Resources

• External Name Handling ADR
• Crossplane Import Guide

[sdischer-sap]

@gergely-szabo-sap did you test that logic with both observe-only and regular resources?
Why is it required to change the external-name format?

I mean I am not totally against changing the external-name format, but we would need extensive testing (probably upgrade testing) for making sure existing resources stay the same and I wouldn't be too happy to now introduce this new "," notation if we do not do the same in all cases that have compound keys.

I had a similar discussion with @jn-av the other day regarding serviceinstances. (Sorry we didn't include you here. I missed that you are facing the same problem in bindings as well)

And at least there my assumption was that its not required to change the external-name. Upjet has some weird specific logic to call an explicit Import command in case of observe only resources (I mean this). Unfortunately due to the way how the btp tf provider is implemented the lookup is totally different there and that is why this confusing error message came up.
However my assumption was that there should be a very simple solution to that. We do not need to pass the the observe only management policy to the embeded upjetted resource here. Since we are entirely in control of when what CRUD method is called we can just pass on a * as management policy. It should lead to upjet using a regular lookup (like in any other case) rather then an import statement.

However now looking at the binding code I see that there we already do not pass that management policies, so I am wondering about all of that ;)

So is the import not working right now?

[gergely-szabo-sap]

@gergely-szabo-sap did you test that logic with both observe-only and regular resources? Why is it required to change the external-name format?

I mean I am not totally against changing the external-name format, but we would need extensive testing (probably upgrade testing) for making sure existing resources stay the same and I wouldn't be too happy to now introduce this new "," notation if we do not do the same in all cases that have compound keys.

I had a similar discussion with @jn-av the other day regarding serviceinstances. (Sorry we didn't include you here. I missed that you are facing the same problem in bindings as well)

And at least there my assumption was that its not required to change the external-name. Upjet has some weird specific logic to call an explicit Import command in case of observe only resources (I mean this). Unfortunately due to the way how the btp tf provider is implemented the lookup is totally different there and that is why this confusing error message came up. However my assumption was that there should be a very simple solution to that. We do not need to pass the the observe only management policy to the embeded upjetted resource here. Since we are entirely in control of when what CRUD method is called we can just pass on a * as management policy. It should lead to upjet using a regular lookup (like in any other case) rather then an import statement.

However now looking at the binding code I see that there we already do not pass that management policies, so I am wondering about all of that ;)

So is the import not working right now?

@sdischer-sap, it was a transient commit, I did not mean to review it yet.

Importing of ServiceBinding did not work in my tests back then. It was due to buggy tests. I managed to fix my test environment, and the importing is working as expected. I added an import test to prove this.

I removed most of the extra code. It become much simpler now.

[enrico-kaack-comp]

Looks great, thank you.
Also thank you for fixing some small things in the import_test_utils I missed when introducing it :)

One thing though: did you check the Observe/Delete/Create Logic against the ADR? E.g. 404 in delete is not handled as error etc.

Another question: what about rotation of serviceBindings, does it work with servicebinding roation?