-
Notifications
You must be signed in to change notification settings - Fork 3
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
sdischer-sap
wants to merge
15
commits into
main
Choose a base branch
from
feat/serviceinstances
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
related ticket: #54
Summary
Adds 3 new CRDs:
SubaccountServiceInstance
- upjeted based on btp_subaccount_service_instanceSubaccountServiceBinding
- upjeted based on btp_subaccount_service_bindingServicePlan
- acts as datasource forSubaccountServiceInstance
To be more precise the first two were already used internally within
ServiceManager
andCloudManagement
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
[[CO_INTERNAL_PARAMETERS_SECRET]]
,[[CO_INTERNAL_PARAMETERS_NAMESPACE]]
)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 ´ServiceManagerand
CloudManagementimplementation. The native controller that delegates to the serviceinstance internally in this case would not receive the created ID if
useAsync` 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 usefalse
in for internal hybrid usage like in theCloudManagement
andServiceManager
case, buttrue
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: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
servicePlanUniqueIdentifier
to entitlement resource #29)