Remove global & service prefixes from customEnvVar

[Avni-Sharma]

Fix #486

Steps to confirm

• Create a postgres db

apiVersion: postgresql.baiju.dev/v1alpha1
kind: Database
metadata:
  name: db-demo
spec:
  image: docker.io/postgres
  imageName: postgres
  dbName: db-demo

• Create a SBR

apiVersion: apps.openshift.io/v1alpha1
kind: ServiceBindingRequest
metadata:
  name: binding-request
spec:
  backingServiceSelector:
    group: postgresql.baiju.dev
    version: v1alpha1
    kind: Database
    resourceRef: db-demo
    id: postgresDB
  envVarPrefix: HELLO
  customEnvVar:
    - name: JDBC_URL
      value: 'jdbc:postgresql://{{ .postgresDB.status.dbConnectionIP }}:{{ .postgresDB.status.dbConnectionPort }}/{{ .postgresDB.status.dbName }}'
    - name: DB_USER
      value: '{{ .postgresDB.status.dbCredentials.user }}'
    - name: DB_PASSWORD
      value: '{{ .postgresDB.status.dbCredentials.password }}'
    - name: TAGS
      value: '{{ .postgresDB.spec.tags }}'
    - name: ARCHIVE_USERLABEL
      value: '{{ .postgresDB.spec.userLabels.archive }}'
    - name: SECONDARY_SECRETNAME
      value: '{{ .postgresDB.spec.secretName }}'

• check the secret binding-request

data:
  ARCHIVE_USERLABEL: PG5vIHZhbHVlPg==
  DB_PASSWORD: cGFzc3dvcmQ=
  DB_USER: cG9zdGdyZXM=
  HELLO_DATABASE_CONFIGMAP_DB_HOST: MTcyLjMwLjE2OS4xOA==
  HELLO_DATABASE_CONFIGMAP_DB_NAME: ZGItZGVtbw==
  HELLO_DATABASE_CONFIGMAP_DB_PASSWORD: cGFzc3dvcmQ=
  HELLO_DATABASE_CONFIGMAP_DB_PORT: NTQzMg==
  HELLO_DATABASE_DBCONNECTIONIP: MTcyLjMwLjE2OS4xOA==
  HELLO_DATABASE_DBCONNECTIONPORT: NTQzMg==
  HELLO_DATABASE_DBNAME: ZGItZGVtbw==
  HELLO_DATABASE_SECRET_PASSWORD: cGFzc3dvcmQ=
  HELLO_DATABASE_SECRET_USER: cG9zdGdyZXM=
  JDBC_URL: amRiYzpwb3N0Z3Jlc3FsOi8vMTcyLjMwLjE2OS4xODo1NDMyL2RiLWRlbW8=
  SECONDARY_SECRETNAME: PG5vIHZhbHVlPg==
  TAGS: PG5vIHZhbHVlPg==

Prefixes should not be included for customEnvVar. The names for customEnVar is mentioned in customEnvVar.Name

[Avni-Sharma]

Further discussion is going on #486 (comment)

[DhritiShikhar]

@Avni-Sharma Is this PR ready for review?

[DhritiShikhar]

@avni

I am trying your PR's proposed changes locally.

This is my SBR:

apiVersion: apps.openshift.io/v1alpha1
kind: ServiceBindingRequest
metadata:
  name: binding-request
  namespace: sb-1
spec:
  envVarPrefix: REDHAT
  applicationSelector:
    resourceRef: nodejs-rest-http-crud
    group: apps
    version: v1
    resource: deployments
  backingServiceSelectors:
        - group: postgresql.baiju.dev
          version: v1alpha1
          kind: Database
          resourceRef: db-demo
          id: postgresDB
          envVarPrefix: DEVTOOLS
  customEnvVar:
    - name: JDBC_URL
      value: 'jdbc:postgresql://{{ .postgresDB.status.dbConnectionIP }}:{{ .postgresDB.status.dbConnectionPort }}/{{ .postgresDB.status.dbName }}'
    - name: DB_USER
      value: '{{ .postgresDB.status.dbCredentials.user }}'
    - name: DB_PASSWORD
      value: '{{ .postgresDB.status.dbCredentials.password }}'
    - name: TAGS
      value: '{{ .postgresDB.spec.tags }}'

These are the contents of the intermediate secret:

  DB_PASSWORD: cGFzc3dvcmQ=
  DB_USER: cG9zdGdyZXM=
  JDBC_URL: amRiYzpwb3N0Z3Jlc3FsOi8vMTcyLjMwLjEzNS4zOTo1NDMyL2RiLWRlbW8=
  REDHAT_DEVTOOLS_CONFIGMAP_DB_HOST: MTcyLjMwLjEzNS4zOQ==
  REDHAT_DEVTOOLS_CONFIGMAP_DB_NAME: ZGItZGVtbw==
  REDHAT_DEVTOOLS_CONFIGMAP_DB_PASSWORD: cGFzc3dvcmQ=
  REDHAT_DEVTOOLS_CONFIGMAP_DB_PORT: NTQzMg==
  REDHAT_DEVTOOLS_CONFIGMAP_DB_USERNAME: cG9zdGdyZXM=
  REDHAT_DEVTOOLS_DBCONNECTIONIP: MTcyLjMwLjEzNS4zOQ==
  REDHAT_DEVTOOLS_DBCONNECTIONPORT: NTQzMg==
  REDHAT_DEVTOOLS_DBNAME: ZGItZGVtbw==    
  REDHAT_DEVTOOLS_SECRET_PASSWORD: cGFzc3dvcmQ= 
  REDHAT_DEVTOOLS_SECRET_USER: cG9zdGdyZXM=
  TAGS: W2NlbnRvczctMTIuMyAxMjNd

As proposed in your PR, the customEnvVar variables are not prefixed by GlobalEnvVarPrefix or service EnvVarPrefix.

[DhritiShikhar]

Also, there should be a check for this in the tests.

[sbose78]

Let's get a review from @qibobo too.

[qibobo]

@Avni-Sharma @DhritiShikhar @sbose78
Just a bit curious, from the code change, I can see that the global env prefix is remove for custom env var, but seems it is not relevant to service level env prefix. But the PR title Remove Global and service prefixes for customEnvVar names means both global and service level prefix are removed. Do I have a misunderstanding?

[pedjak]

Acceptances tests need to be added within this PR, demonstrating the expected behaviour. PR description contains already enough details that can be used for writing such tests.

[Avni-Sharma]

@Avni-Sharma @DhritiShikhar @sbose78
Just a bit curious, from the code change, I can see that the global env prefix is remove for custom env var, but seems it is not relevant to service level env prefix. But the PR title Remove Global and service prefixes for customEnvVar names means both global and service level prefix are removed. Do I have a misunderstanding?

Hi @qibobo Thanks for calling that out. Indeed it is for global prefixes. Got a confirmation in the same regard here as well. #486 (comment)

[Avni-Sharma]

The acceptance test for this is dependent on #597 since the function for fetching of custom_env_var key and value is mentioned there and that can be reused here. Feature file's scenario is ready for this PR cc @pedjak

[Avni-Sharma]

@pedjak what would be a suitable scenario name for this ? I am planning to add it to https://github.com/redhat-developer/service-binding-operator/blob/master/test/acceptance/features/bindAppToService.feature

[pedjak]

Looks, proposed rephrased scenario description for improved readability.

[pedjak]

/lgtm
/approve

[baijum]

/retest

[baijum]

/retest

[baijum]

/retest

[pedjak]

/lgtm
/approve

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pedjak

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [pedjak]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[baijum]

/retest

[baijum]

/retest

[baijum]

/test 4.5-unit

[baijum]

/retest

[baijum]

/retest

[openshift-ci-robot]

@Avni-Sharma: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/4.5-e2e	4e09b00	link	/test 4.5-e2e

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[baijum]

/retest