Service binding global and service envVarPrefix doesn't behave as expected #475
Comments
|
@isutton I can provide a PR for this issue. |
|
@qibobo I believe we've implemented all the logic required from #428 (it had to be re-implemented in #407 due to the changes of internals), but I might've been wrong and instead misunderstood this table; so thanks for offering to contribute this one 👍. Here is the function responsible for deciding how to prefix an environment variable according to the combination of global prefix and service prefix, according to the table found in #428: Here you can find the test cases: Thanks a lot! |
|
@isutton |
isutton
changed the title
I'll reply to each point in a different comment, if you don't mind. Regarding point 1 I believe we should split an environment variable to find out where does each part come from in the environment variable name
This, in my point of view, is not really an issue but a side-effect of allowing multiple services to be declared and at the same time using the service Let's take a look ath the following example, where different services are disambiguated by the usage of apiVersion: apps.openshift.io/v1alpha1
kind: ServiceBindingRequest
metadata:
name: sbr5
spec:
envVarPrefix: globalprefix
applicationSelector:
resourceRef: mytranslator-web-vcap5
group: apps
version: v1
resource: deployments
backingServiceSelectors:
- group: ibmcloud.ibm.com
version: v1alpha1
kind: Binding
resourceRef: binding1
envVarPrefix: prefix1
- group: ibmcloud.ibm.com
version: v1alpha1
kind: Binding
resourceRef: binding2
envVarPrefix: prefix2
customEnvVar:
- name: ALL_SECRET
value: '{{json .}}'The configuration above should yield the following environment variables: Of course, this scheme can be modified to better serve us, it was used as a start point :) What do you think? |
There are two points I'd like to clarify with you, regarding the
What do you think? |
|
@isutton 2 For the custom env var, based on the example below, once we add the |
|
Ideally, we should not have the My proposed next steps:
As part of the same fix/effort, we could address
|
|
@isutton @sbose78 The result before PR407: The result after PR407: The differences between before and after. I prefer we make current behavior the same as before for the following 3 ones. |
|
@qibobo can you share more information about the scene, such as the |
|
@isutton |
|
@qibobo Thanks for your thorough verification, really appreciated!
Given this information and the In the app used in the internal examples, a reference to the service kind in environment variables can be seen here and excerpted below: const serviceHost = process.env.DATABASE_DBCONNECTIONIP || 'localhost';
const servicePort = process.env.DATABASE_DBCONNECTIONPORT || '5432';
const user = process.env.DATABASE_SECRET_USER || 'user';
const password = process.env.DATABASE_SECRET_PASSWORD || 'password';
const dbName = process.env.DATABASE_DBNAME || 'my_data'In the apiVersion: apps.openshift.io/v1alpha1
kind: ServiceBindingRequest
metadata:
name: binding-request
namespace: service-binding-demo
spec:
applicationSelector:
resourceRef: nodejs-rest-http-crud
group: apps
version: v1
resource: deployments
backingServiceSelector:
group: postgresql.baiju.dev
version: v1alpha1
kind: Database
resourceRef: db-demoGiven that #407 haven't changed that behavior, what has been experienced is correct (according to the examples and tests existing prior #407) otherwise this example should've been changed. @sbose78 what is your take on this?
That was an interpretation that Shall we track this on a separate issue?
To reference a As can be seen in the test expected value: Please observe the other existing And the expected value: In my opinion, until the Service Binding Spec annotations are implemented, a possibility is to whitelist the fields that should be collected from that secret as CRD annotations: annotations:
servicebindingoperator.redhat.io/status.secretName-key1: binding:env:object:secret
servicebindingoperator.redhat.io/status.secretName-key2: binding:env:object:secret@qibobo can you live with this until we have the Service Binding Spec annotations implemented? |
|
@isutton @sbose78 I think there are regression issues with PR407. First of all, I patched the backingService CRD "Bindings.ibmcloud.ibm.com " with required annotations. After the patch: Refer to the last comment of #475 (comment), @isutton, do you mean the annotaion above doesn't work at all with PR #407? Let me assume the annotation still works, then #Issue 1, the syntax of custom env showed in https://github.com/redhat-developer/service-binding-operator/tree/master/examples/knative_postgresql_customvar doesn't work at all. I put a SBR as below The result before PR407: Then, when I applied the SBR after PR407, the reconcilation was failed with the error See full stacktrace below for refernce: Furthermore, I did a little bit further investigation here by using below customEnvVar With above JSON template, before 407, I can get the But after 407, I only got Obviously, the Given the basic example case https://github.com/redhat-developer/service-binding-operator/tree/master/examples/knative_postgresql_customvar is broken now, I think it is a regression issue, and need to be fixed. #Issue2: The naming of the envprefix I rework my SBR with env prefix in different layer: a. For the default envs, besides global-level envVarPrefix and service-level envVarPrefix, there is an extra string before 407 (EXPECTED result) after 407 b. For custom env vars, after 407 applied, an extra before 407 (EXPECTED result) after 407 |
|
@qibobo thanks for your extensive description, that's really useful!
It should work, according to the tests mentioned in that comment.
This happens due to the introduction of In the example given above, the full value name to be used in customEnvVar would be > - name: LANGUAGE_TRANSLATOR_URL
> value: '{{ index .v1alpha1.ibmcloud_ibm_com.Binding.thetranslator5.status.secretName "url" }}'
Can you confirm
I understand the problem, but I'm not sure it is a regression since the behavior is transitioning from single to multiple services and the implications should be discussed.
The annotations and tests have been implemented in 407 according to the annotations present in here: apiVersion: apiextensions.k8s.io/v1beta1
kind: CustomResourceDefinition
metadata:
name: databases.postgresql.baiju.dev
annotations:
servicebindingoperator.redhat.io/status.dbConfigMap-db.host: binding:env:object:configmap
servicebindingoperator.redhat.io/status.dbConfigMap-db.name: binding:env:object:configmap
servicebindingoperator.redhat.io/status.dbConfigMap-db.password: binding:env:object:configmap
servicebindingoperator.redhat.io/status.dbConfigMap-db.port: binding:env:object:configmap
servicebindingoperator.redhat.io/status.dbConfigMap-db.username: binding:env:object:configmap
servicebindingoperator.redhat.io/status.dbConnectionIP: binding:env:attribute
servicebindingoperator.redhat.io/status.dbConnectionPort: binding:env:attribute
servicebindingoperator.redhat.io/status.dbCredentials-password: binding:env:object:secret
servicebindingoperator.redhat.io/status.dbCredentials-user: binding:env:object:secret
servicebindingoperator.redhat.io/status.dbName: binding:env:attributeI've searched in the repository for As you can see, there isn't a The following test shows the behavior you're describing: In other words, to solve this issue the expected value in the test above should be modified to the following and then adjusted accordingly in
That's indeed a bug, and can be fixed in here: service-binding-operator/pkg/controller/servicebindingrequest/retriever.go Lines 128 to 136 in 179c2fb |
|
@isutton The VCAP_SERVICES is: |
@qibobo just to make sure I understood you correctly: this example refers to the Did I get it right? :) |
|
I really appreciate the detailed conversation above. In @qibobo 's comment, @qibobo mentions that when both global env var prefix and service specific env var prefix is provided, the secret key name looks like this: PR's comment can be used as a reference. I haven't seen this addressed in @isutton 's comment here. The example in that comment is devoid of global or service level prefixes. I might be missing something, could you please tell me if the above issue is being address this @qibobo @isutton ? |
|
@isutton |
|
@isutton
After adding the annotation, the CRD is as the following and we can see that the annotation has beeb added. Then I created the following SBR: Then the operator showed the error log:
The whole error log: And I confirmed that the url is in the secret: |
|
@isutton And with the SBR above I got the some error log that may help: And in https://github.com/qibobo/service-binding-operator/blob/47c3e07c8da3459c4120459c3cbfd8f57d128797/pkg/controller/servicebindingrequest/retriever.go#L125 |
|
@isutton |
|
@isutton , what is the next step to move this forward? Will you raise a new PR to resolve this? |
|
@isutton |
|
@qibobo here it is, right from the oven :) |
This has been discussed in #475 (comment):
And here #475 (comment):
I'm inclined to use the annotations that has been tested so far, and wait for the finalization of the new Service Binding annotation syntax. |
|
@isutton But now we need the definition as below: We can find the whole content of data to do custom_env_var template parsing as below, I think the And the the test steps: 1 add annotation: 2 create the SBR: And the result: |
|
@cdlliuy this inference is made in Then it is consumed in The change could be make It is also necessary to adjust |
According to our latest test, there are some features broken after #407 is merged.
Below is the SBR and the environment result.
Env var result:
There are 3 issues I have found:
1 There are prefixes
BINDING_SECRET_STATUSfor all env vars, but previously we do not have them.2 With the custom env var
ALL_SECRET, there is only secretName in the result content, but previously the content of the secret should be in the content.The text was updated successfully, but these errors were encountered: