Skip to content
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

Feat/serviceinstances #103

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from
Draft

Feat/serviceinstances #103

wants to merge 15 commits into from

Conversation

sdischer-sap
Copy link
Member

@sdischer-sap sdischer-sap commented Feb 7, 2025

related ticket: #54

Summary

Adds 3 new CRDs:

To be more precise the first two were already used internally within ServiceManager and CloudManagement before, but are exposed now as a user feature.

Challenges

Sensitive Parameters field

Users need to be able to pass sensitive parameters like passwords in ServiceInstances. This can be easily supported by upjet, but it breaks the existing CloudManagement implementation. Here we need to pass a static parameter for grantType. Since its hybridity called from a native controller we can’t model that data as secrets here.

Implementation approach

  • resolving sensitive field secrets is done on the flow in the upjet reconciler
  • found a way to inject a mock for the kubeclient that is used here
    • this mock will return the static grantype value in case of a very specific namespace and name for the requested secret ([[CO_INTERNAL_PARAMETERS_SECRET]], [[CO_INTERNAL_PARAMETERS_NAMESPACE]])
    • otherwise it will resolve using a regular kubeclient

Asynchronous Creates

Some serviceinstance creations might take minutes to complete. Again upjet provides an easy configuration called useAsync for that and again we can’t just easily use it to create any instance of that controller, because it will break the existing ´ServiceManagerandCloudManagementimplementation. The native controller that delegates to the serviceinstance internally in this case would not receive the created ID ifuseAsync` is used here. This would break future reconcilation attempts.

Implementation approach

Luckily I found a way to configure this useAsync in the different instance initializations. This way we will be able to use false in for internal hybrid usage like in the CloudManagement and ServiceManager case, but true in case of a manager based initialization.

Remarks on behaviour / limitations

Binding Structure

Creating a SubaccountServiceBinding allows to publish Binding as Secret with this structure:

data:
  attribute.credentials: '<binding json>'

As far as I know this is upjets default behaviour for such cases. If it gets relevant we can follow up to see if we can find a way to change that structure.

Imports

Imports generally work as expected with setting annotation crossplane.io/external-name: <guid> with Managementpolicies other then ObserveOnly. Fields not being specified are being imported into the spec via late initialize.
For ObserveOnly upjet seems to use a different flow with calling an explicit tf import, which in this case will fail, since it requires multiple ids as per the implementation in the tf provider. We have to follow up if thats something we can fix from our end.

BTP service operator:
Its worth noting that instances and bindings created via btp-service-operator can be imported as ObserveOnly. Any attempt to update or delete the instance will be blocked by the API though, so I guess the use is limited in this case. This is not a limitation of this approach though. You can't even delete the instance in BTP Cockpit. It will get cleaned up when you delete the ServiceManager instance though.

Other

  • instances are created in environment "Others", which is different from the btp service operator (which uses "Kyma")
  • deleting instances is blocked until bindings are cleaned up
  • in case of ambiguous service plans the ID can be directly set, unfortunately there is no way to lookup the serviceplan more precisely with a unique plan name (like implemented for the entitlements, see feat: add servicePlanUniqueIdentifier to entitlement resource #29)

@sdischer-sap sdischer-sap deployed to pr-e2e-no-approval February 7, 2025 13:50 — with GitHub Actions Active
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant