-
Notifications
You must be signed in to change notification settings - Fork 13
Docs: external name definition and handling #374
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,110 @@ | ||
# External Name Handling | ||
|
||
## Context | ||
`crossplane.io/external-name` is an annotation for Managed Resources. Across providers we lack a unified definition and implementation guideline. This ADR proposes a consistent definition, expected behavior, and implementation approach. | ||
|
||
## Background | ||
|
||
### Definitions | ||
Common interpretations of the external name: | ||
1. Human-facing name of the resource shown in the external provider UI. [1] | ||
2. Unique identifier generated by the external system (GUID). [2] | ||
|
||
### Default Behavior in Crossplane | ||
- By default, the external name is initialized (by crossplane-runtime) to the Kubernetes `metadata.name` of the Managed Resource (MR) if not explicitly set. | ||
- This default conflicts with cases where the external system uses a different unique identifier. | ||
|
||
### 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
3. Apply the MR. Controller populates `status.atProvider`. | ||
4. Copy the fields from `status.atProvider` to `spec.forProvider` and change `managementPolicies` to `*` to gain full control. | ||
|
||
## Our Approach | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have some concerns regarding the compound key concept (line 17-24)
I have a concern regarding us constructing our custom external name, The external-name annotation is meant to bridge Crossplane and the provider API. Fundamentally I think external-name should represent something the provider There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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).
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for the great conversation! You raise a really valid UX concern about consistency. 1. Provider-specific behavior is already normal 2. Import might actually be simpler
3. Drift could be tricky
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:
Fair! Though I wonder if we could skip building one and just use spec.forProvider directly?
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! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we are landing on two different possible solutions to deal with cases that don't offer a unique ID here:
2. no external-name, lookup using spec fields
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would like to understand why compound names are closer to the crossplane standard. " My reading of Crossplane docs suggests:
For the concern about import being less clear: we could address this by:
|
||
### Definition | ||
External-name represents the unique identifier given by the external-system. If this does not exist, it is a compound key of values from spec.forProvider that uniquely identify the external resource. This annotation is used for API Get requests (or filter List requests). | ||
|
||
In Create(), the external name is set based on the response of the API (if possible). Observe() use the external-name to match the external resource, Update() and Delete() also use the external-name. [4] | ||
This is easy in the case of a unique identifier. If we have a compound key, we deconstruct the compound key to perform the API requests. | ||
|
||
### Special Case: defaulting of external-name | ||
If external-name is unset, crossplane-runtime will run a default initializer that will set the external name to metadata.name before the first Observe() is called. | ||
|
||
This poses two problems: | ||
- 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?** | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 I think we need to answer the following before we can safely implement the pattern:
Before finalizing this design we need to explicitly specify:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We definitely need to specify this behavior once decided how to handle the defaulting of external name.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did look Crossplane's official guidance on this.
So your position aligns with Crossplane's documented behavior. :) |
||
|
||
### Implementation guideline | ||
|
||
#### Observe() | ||
1. Check if external-name is empty. | ||
- If we need backwards compatibility, we create our API query from the spec.forProvider fields. | ||
- If resource exists we perform the normal needsCreation/needsUpdate and set the external name to the correct value. | ||
- 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) |
||
3. Build the Get API Request from the external-name (by either parsing the compound key or using the GUID directly, do not use the spec.forProvider). | ||
- If resource is not found, that is a drift, not an error case, we therefore return resourceExist: false to trigger a Create() call. | ||
- Else, updateObservation and perform needsCreate()/needsUpdate() check as usual. | ||
|
||
#### Create() | ||
We know that the resource does not exist so we perform the Create API Request and store the necessary GUID or compound key from the API response/spec.ForProvider. The case that we don’t have all the necessary information to set the key can not happen because than we would not be able to check the resource for being created. | ||
|
||
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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
#### Delete() | ||
If our external resource has a field indicating it is in deletion state, we return. | ||
|
||
We use external-name to perform DELETE request. | ||
|
||
If the response is 404 – not found (or the equivalent API response for this API), we do NOT treat this as an error. This only happens when the resource was externally removed in the meantime since after Observe(), Delete() is not called if the resource does not exist. Technically we have a drift in here but it can be safely ignored since the drift already covers the desired state. | ||
|
||
If our delete request is successful, we return EVEN IF the deletion operation is performed async by the external system (in the next reconcile Observe(), the resourceExist field will be checked by crossplane-runtime and Delete() is called again if the resource still exists). Should the resource still exist in next reconcile, Delete() is triggered again but we check for the deletion state in the external system (= status.observation being something like `DELETING` or so) so we don’t perform another Delete(). If this is not possible, we have to perform another DELETE API call, treat a possible error response as error and return it. Once the resource is deleted in the external system, the Observe() function will determine resourceExist: false and crossplane-runtime removed the MR. | ||
|
||
Else, if we have an error in our request, return with error. | ||
|
||
### Importing of existing external resources | ||
For importing, setting the right external-name will match the external resource no matter the values in `spec.ForProvider`. | ||
|
||
If we set the `spec.forProvider` as required, the user can not create managed resources without those values and therefore must also set the required fields. The official import guide [3] talks about setting fields that are required for unique matching but the validation rules might require more fields. On the other hand, setting it as optional would allow to Observe an external resource and deleting them (using the managementPolicies: Observe, Delete) without having to fill out the spec. | ||
|
||
This is not super important for us right now since we plan to have an export cli to create the spec for us but might help to have a more common behaviour accross our providers. | ||
|
||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. General comment About Provider-BTP Implementation: 1. Does BTP actually lack unique identifiers? 2. Can BTP change identifier formats?
Hardcoded compound key logic couples our provider to the current BTP API structure. Alternatively, we could consider using spec.forProvider directly
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good discussion here, I like it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for providing that clarity
|
||
These might not be important for our APIs if we don’t have this scenarios. | ||
|
||
### ID can’t be determined in Create() | ||
Maybe the ID can not be determined in Create() after the API request, maybe it only has a job id/temporal id and later gets a real GUID, the external name needs to be swapped. | ||
|
||
### external-name compound key | ||
The delimiter can collide with the field values if they can be in there too. Case sensitive/insensitive matching, leading/trailing spaces. Length limit of annotation, maybe base64 encoded parts of the compound key to avoid delimiter collisions (or escaping of the delimiter character). | ||
|
||
Also error case if the compound key matches more than one external resource needs to be handled if a List API is used. | ||
|
||
### External-name annotation manually edited | ||
Observe() would return a not found and the resource would be created again with the same `spec.forProvider`. This would probably lead to a creation error. Is this what we want? | ||
|
||
### Export tool considerations | ||
|
||
The export tool must generate managed resource definitions based on external resource state retrieved through the external system API. To ensure these definitions can be imported successfully, the tool must generate proper `crossplane.io/external-name` annotations. | ||
|
||
The export tool **must** be able to derive the `crossplane.io/external-name` annotation value from information obtained through standard API operations (GET, LIST) on external resources. | ||
|
||
## References | ||
[1] https://github.com/crossplane/crossplane/blob/main/design/one-pager-managed-resource-api-design.md?plain=1#L151 | ||
[2] https://github.com/crossplane/crossplane/issues/1640 | ||
[3] https://docs.crossplane.io/latest/guides/import-existing-resources/ | ||
[4] https://github.com/SAP/crossplane-provider-cloudfoundry/blob/main/docs/resource-names.md#crossplaneioexternal-name-annotation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/crossplane/crossplane/blob/main/design/one-pager-managed-resource-api-design.md#external-resource-name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with your definition, but I could not read it out of any source like the one pager you also referenced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would agree with @christophrj here. Definition for me would be:
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).