Adopting Orphan Instances via Kubernetes Annotation

[santiago-ventura]

Motivation

We introduce the new annotation service-operator.cf.cs.sap.com/adopt-instances to facilitate the adoption of orphan instances.
Currently, the only way is via a call to the Cloud Foundry API, as explained in the documentation "Adopt existing resources".

This allows updating the Orphaned Cloud Foundry instances with the label owner and the annotations generation and parameter-hash.

[RalfHammer]

message "found multiple service bindings with owner" is only correct in case of empty bindingName
Either use a more generic message or distinguish both cases in this message.

[bKiralyWdf]

The runtime check is a little ugly. How about taking the listOpts as an argument?

[bKiralyWdf]

Adding a parameter to the binding in a method called GetBinding is a bit unexpected for me. Consider moving this to the reconcile function.

[bKiralyWdf]

See comment above

[bKiralyWdf]

See comment above

[bKiralyWdf]

We are already in an if exists && cfbinding != nil block, and we just set bindingName to serviceBinding.Name (which is guaranteed to exist in the type). This if statement can be simplified to if true, therefore removed.

[bKiralyWdf]

I was mistaken here, the cfbinding variable is shadowed, so we still need to check for cfbinding != nil

[bKiralyWdf]

See above for bindings

[bKiralyWdf]

See above, cfinstance is shadowed, so we need the check for cfinstance != nil

[bKiralyWdf]

How about replacing this with

func (c *spaceClient) GetBinding(ctx context.Context, listOpts cfclient.ServiceCredentialBindingListOptions) (*facade.Binding, error) {

And then just creating the listOpts at the servicebinding_controller level?

[santiago-ventura]

The suggested change is not viable. ServiceCredentialBindingListOptions is part of the go-cfclient library's client package.

The package controller does not have direct access to the go-cfclient library's client package. Instead, it has an interface called SpaceClient, which wraps any interaction with the go-client library.

That is a problem because changing the function GetBinding to receive listOpts will break the designed separation between the controller and the go-cfclient library.

Also, we will have to create the LabelSelector in the controller:
listOpts.LabelSelector.EqualTo(fmt.Sprintf("%s/%s=%s", labelPrefix, labelKeyOwner, mapBindingOpts["owner"]))

labelPrefix: is only visible within the cf package, not the controller package.
labelKeyOwner: is only visible within the cf package, not the controller package.

Again, this breaks the current design where those variables are only visible within the cf package.

Change this:
const (
labelPrefix = "service-operator.cf.cs.sap.com"
labelKeyOwner = "owner"
labelOwner = labelPrefix + "/" + labelKeyOwner
annotationPrefix = "service-operator.cf.cs.sap.com"
annotationKeyGeneration = "generation"
annotationGeneration = annotationPrefix + "/" + annotationKeyGeneration
annotationKeyParameterHash = "parameter-hash"
annotationParameterHash = annotationPrefix + "/" + annotationKeyParameterHash
)

to

const (
LabelPrefix = "service-operator.cf.cs.sap.com"
LabelKeyOwner = "owner"
LabelOwner = labelPrefix + "/" + labelKeyOwner
AnnotationPrefix = "service-operator.cf.cs.sap.com"
AnnotationKeyGeneration = "generation"
AnnotationGeneration = annotationPrefix + "/" + annotationKeyGeneration
AnnotationKeyParameterHash = "parameter-hash"
AnnotationParameterHash = annotationPrefix + "/" + annotationKeyParameterHash
)

This change is not going to be accepted because we will have to break the separation of concerns between the controller and the cf client.

[bKiralyWdf]

You're right!

I thought about including the gocfclient in the controller might not be very nice, I guess it is worse than I thought since you have to also expose the constants.

Maybe add a new type instead, to pass the owner or name? Like this:

type BindingFilter interface {
	GetListOptions() *cfclient.ServiceCredentialBindingListOptions
}

type BindingFilterName struct {
	name string
}
type BindingFilterOwner struct {
	owner string
}

func (bn *BindingFilterName) GetListOptions() *cfclient.ServiceCredentialBindingListOptions {
	listOpts := cfclient.NewServiceCredentialBindingListOptions()
	listOpts.Names.EqualTo(bn.name)
	return listOpts
}

func (bo *BindingFilterOwner) GetListOptions() *cfclient.ServiceCredentialBindingListOptions {
	listOpts := cfclient.NewServiceCredentialBindingListOptions()
	listOpts.LabelSelector.EqualTo(fmt.Sprintf("%s/%s=%s", labelPrefix, labelKeyOwner, bo.owner))
	return listOpts
}

func (c *spaceClient) GetBinding(ctx context.Context, filter BindindingFilter) (*facade.Binding, error) {
  listOpts := filter.GetListOptions()
  //...

This way above in the Reconcile, you could just call the function like so GetBinding(ctx, BindingFilterOwner{name: "foo"}) or GetBinding(BindingFilterOwner{owner: "bar"}. This is nice, since the type is checked at compile time, so we don't need the runtime check for the string. It also makes the function signature nicer and removes the need to initialize a complex string map.