Secret credentials has an incorrect `instance_name` (when externalName is used) about sap-btp-service-operator HOT 9 CLOSED

@avilupu Thanks for looking into this.

A small typo/issue in your comment above:
"In the created secret, the value of the instance_name key is the BTP name, which is maintained in the external name property of the binding instance (instance_name contains the external service instance name as in BTP when present), not the metadata name."

The change (#338) itself looks good though and there intance (external) name is used in secret instance_name property. 👍

from sap-btp-service-operator.

Hi Pavan,
I don't think it's a bug, external name is the name that is being used outside of the cluster, it will be the instance name in Service Manager and the name that you will see in cockpit and SM api, but within the cluster the instance name is metadata.name
Why do you need to use the external name within the cluster?
Thanks,
Keren

from sap-btp-service-operator.

Hi Keren,
As mentioned in my original description.. when the credentials are consumed say by a Pod within the cluster and an app wants to access the BTP service credentials via instance name - it would do so using the instance name in SM api and/or BTP cockpit and NOT via the k8s cluster metadata.name. Using the cluster specific name in the credential metadata is a bug as consuming apps do not care what the k8s instance name of the service is.. they need to know the actual instance name from SM / BTP cockpit.
The fix I proposed addresses that by updating the OSB credential metadata with the instance_name as present in BTP cockpit.

from sap-btp-service-operator.

some applications might require the k8s name and some the BTP name, I will add another key to the secret "instance_external_name" that will contain the external name

from sap-btp-service-operator.

Hi Keren,
That will not help as all consuming libraries need to adopt and support this new metadata. E.g. CAP, xsenv etc.. How should we/consumers now handle that with your proposal? (we do not want to create multiple mounts/secrets here to work around the issue).
See for instance: https://www.npmjs.com/package/@sap/xsenv#usage-in-kubernetes: as to how one must mount this in k8s.

Shouldn't your guidance instead be that applications that rely on k8s name - do not set any externalName? With that existing apps should work as they did before (i.e. use the same name for k8s and BTP service instance).

Regards,
Pavan

from sap-btp-service-operator.

Hi,
Sorry the new property you introduce does not solve issue (as mentioned above). It instead causes all consumers to adopt and support this new metadata.

We think the instance_name property in the Secret metadata, never worked as intended and hence fixing it would be the proper solution here.

from sap-btp-service-operator.

Actually, changing this would be incompatible, service tags might be a better option in general for selecting service credentials and why would users actually not use the instance name they created?

Adding external name as a property seems to be the best way forward, wouldn't it?

from sap-btp-service-operator.

...why would users actually not use the instance name they created?

Exactly.. but if you try to consider the actual consumers here (who are trying to consume the BTP service credentials (not necessaryily created by this operator)), the instance_name that one would expect to see in the service metadata would be the actual service instance name (from BTP) and not the "k8s instance name".
Especially if you put yourself in the shoes of app developers.. they would wish to use the BTP service instance name (e.g. from BTP cockpit) - just like say they would when they'd create this manually -or- via any other mechanism on CF/K8s.

Adding external name as a property seems to be the best way forward, wouldn't it?

No. As mentioned before.. most of the consumers CAP, @sap/xsenv, any other library/consumers who have their existing way of consuming BTP service credentials - do not know what this "external name" is. To be frank, this would be something that is just invented here by the BTP service operator, which is NOT the only way to create service instances in BTP.

Moreover.. another discrepancy here that we mentioned already to the developers:
if instance_name is really meant for K8s instance name then shouldn’t instance_guid also point to the UId of the K8s instance(?).
In our opinion however it makes no sense do that, instead we should always point to the actual BTP service instance_name and instance_guid when it exists (e.g. when externalName is set). For existing consumers, who do not wish this behaviour they could just (continue to) not set any externalName and get the behaviour you seem to always mention (k8s instance name = BTP instance name = metadata credentials instance_name).

Basically the service metadata added to the secret by this operator was always incorrect and needs to be fixed.. even if it may be incompatible. Or do we stop fixing every issue claiming it's incompatible?

from sap-btp-service-operator.

Hi. After re-evaluation, we accepted your argument that this is a bug.
A fix is delivered with release v0.5.3.
In the created secret, the value of the instance_name key is the BTP name, which is maintained in the external name property of the service instance, not in the metadata name.
external_instance_name key was removed from the secret.

from sap-btp-service-operator.