Experimental(kyma-service-instance) - Adding a kyma service instance crd#366
Experimental(kyma-service-instance) - Adding a kyma service instance crd#366
Conversation
Lasserich
left a comment
There was a problem hiding this comment.
Great work and super impressive how fast you got this working!! 🥇
I have left some comments which should be addressed. Let us wait however for Tuesday to hear about the ideas from the Kyma folks and our Stakeholders to see the importance. 😄
| // External name for display in BTP cockpit (optional) | ||
| // If not specified, uses the Name field | ||
| // +kubebuilder:validation:Optional | ||
| ExternalName string `json:"externalName,omitempty"` |
There was a problem hiding this comment.
This could lead to confusion from a naming perspective, I already envision the support tickets with confusion about two external-names. Is there another way we could name this? + Optional fields should be pointer.
| Conditions []ServiceInstanceCondition `json:"conditions,omitempty"` | ||
| } | ||
|
|
||
| type ServiceInstanceCondition struct { |
There was a problem hiding this comment.
Could you add a comment with a reference to the original structure in the Kyma environments? This helps us determine the effects of changes from their side better.
|
|
||
| } | ||
|
|
||
| func extractConditions(si *ServiceInstance) []v1alpha1.ServiceInstanceCondition { |
There was a problem hiding this comment.
What is this doing, copying over the Conditions from the BTP Operator? A small comment would be nice here. :D
| ) | ||
|
|
||
| // This matches services.cloud.sap.com/v1/ServiceInstance in Kyma | ||
| type ServiceInstance struct { |
There was a problem hiding this comment.
These are copied over right? Also here some reference link would be very nice!
| return managed.ExternalObservation{}, errors.New(errNotKymaServiceInstance) | ||
| } | ||
|
|
||
| if cr.Spec.KymaEnvironmentBindingRef == nil { |
There was a problem hiding this comment.
I feel this should move into the SecretFetcher logic. Do not like to have this bloating the Observe method too much.
| } | ||
|
|
||
| // Describe the instance in Kyma | ||
| observation, err := e.client.DescribeInstance(ctx, cr.Spec.ForProvider.Namespace, cr.Spec.ForProvider.Name) |
There was a problem hiding this comment.
Is there an option to use the resource metadata.annotation.external-name as a look-up? Even if the BTP Operator does not support such we should use its value as the look-up to stay consistent with our importing scheme.
| // Update status | ||
| cr.Status.AtProvider = *observation | ||
|
|
||
| if hasFailedCondition(observation.Conditions) { |
There was a problem hiding this comment.
Please separate the concerns of conditions into an extra function, either in the controller or client directly.
Also the Messages should go into constants on top. Then the Observe() gets a bit cleaner again.
There was a problem hiding this comment.
We should unify this for KymaModule and ServiceInstance and have it in some common folder, otherwise we are duplicating a lot of code.
There was a problem hiding this comment.
Totally agree. I was holding off touching any parts that would require refactoring any existing code. I would like it do this eventually.
KymaServiceInstance PR
Status
🧟 Living/Dead waiting for revival, aka interest.
Demoed and ready, but the user requirements seemed low.
Issue: #271
Table of Contents
Overview
Implement
KymaServiceInstanceCRD to simplify creating BTP service instances in Kyma runtime.Goal: User-friendly abstraction that hides manual wrapping complexity.
Scope:
Problem Statement
Current State (Painful)
Users must manually wrap BTP Service Operator resources in
kubernetes.crossplane.io/Object:Problems:
Desired State (Simple)
Benefits:
Architecture Investigation
The Complete Resource Hierarchy
Investigation of KymaModule/KymaEnvironment/KymaEnvironmentBinding revealed the dependency chain:
Design Decisions
1. Proposed CRD Structure
File:
apis/environment/v1alpha1/kymaserviceinstance_types.goField Mapping (BTP Crossplane → Kyma Operator):
namespace→metadata.namespacein KymaserviceOfferingName→spec.serviceOfferingNameservicePlanName→spec.servicePlanNameexternalName→spec.externalNameparameters→spec.parameters2. Client Implementation
File:
internal/clients/kymaserviceinstance/client.goPattern (following
kymamodule.NewKymaModuleClient):Target CRD (in Kyma cluster):
Libraries:
sigs.k8s.io/controller-runtime/pkg/client- Kubernetes clientk8s.io/client-go/tools/clientcmd- Kubeconfig parsingk8s.io/apimachinery/pkg/apis/meta/v1/unstructured- Dynamic CRD handlingCode Reference:
internal/clients/kymamodule/module.go:NewKymaModuleClient()3. Controller Pattern
File:
internal/controller/kymaserviceinstance/kymaserviceinstance.goCode Reference:
internal/controller/kymamodule/kymamodule.go4. Re-use SecretFetcher Pattern
File:
internal/controller/kymaserviceinstance/secretfetcher.goCode Reference:
internal/controller/kymamodule/secretfetcher.goImplementation Plan
Phase 1: CRD & API Types
apis/account/v1alpha1/kymaserviceinstance_types.goKymaServiceInstanceParametersKymaServiceInstanceObservationKymaServiceInstanceSpecwithKymaEnvironmentBindingRefPhase 2: Client Implementation
internal/clients/kymaserviceinstance/client.goClientinterfaceNewKymaServiceInstanceClient(kubeconfig []byte)GetServiceInstance(ctx, namespace, name)CreateServiceInstance(ctx, cr)DeleteServiceInstance(ctx, namespace, name)Phase 3: Controller Scaffolding
internal/controller/kymaserviceinstance/controller.gointernal/controller/kymaserviceinstance/secretfetcher.goconnectorstructexternalstructConnect()with secret fetchingPhase 4: Controller Lifecycle
Observe()- check resource existsCreate()- create ServiceInstance in KymaDelete()- remove ServiceInstance from KymaUpdate()- no-op for now (or future)Phase 5: Testing & Validation
Phase 6: Documentation
Testing Strategy
Prerequisites
Required Resources:
Error Scenarios to Test
Open Questions
1. Parameter Handling
Question: How to handle complex/secret parameters?
Options:
Proposal: Start with A, add B if needed
2. Status Fields
Question: What fields from Kyma ServiceInstance status should we expose?
From services.cloud.sap.com/v1/ServiceInstance:
status.ready(bool)status.instanceID(string)status.conditions([]Condition)status.operationURL(string)status.operationType(string)Proposal: Start minimal (ready, instanceID), expand as needed
3. Update Support
Question: Should Update() actually update or be no-op?
Considerations:
Proposal: Start with no-op (like KymaModule), add if needed
4. Multiple Bindings per Instance
Question: Can one ServiceInstance have multiple ServiceBindings?
Impact: Affects how we structure KymaServiceBinding CRD
Decision: Research Kyma operator behavior
Known Limitations
make generatecurrently fails due to schema.json generation not supportingmixed Upjet/custom controller architectures. This is a pre-existing issue
affecting all custom controllers (KymaModule, KymaEnvironmentBinding, etc.).
Workaround: Use
make generate.runfor code generation, which skipsschema.json generation and works correctly for custom controllers.
References
Code References
internal/controller/kymamodule/internal/clients/kymamodule/apis/environment/v1alpha1/kymaenvironment_types.goapis/environment/v1alpha1/kymaenvironmentbinding_types.goExternal Documentation
TODO