Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert annotation handling mechanism to not use the original config map or secret resource name location #522

Merged
merged 6 commits into from
Jul 16, 2020

Conversation

isutton
Copy link
Contributor

@isutton isutton commented Jun 26, 2020

Motivation

After #407 an undocumented behavior regarding annotations meant to extract
config map and secret values has been unwillingly changed and needs to be
reverted.

The annotations handling mechanism after #407 extracts the whole contents of
the config map or secret to the same path where the config map or secret name
has been stored (resulting in, for example, .status.secretName.password if
.data.password is present in the associated resource) to workaround
situations where two services might have a password field within different
paths and might get shadowed, whereas before #407 it used to extract the
contents to the root of the resulting object (for example, .password),
imposing no issue whatsoever because there were only one service being
declared in the binding.

Closes #485

Changes

This change reverts the behavior where the source path isn't expected in the
resulting environment variable name; for example, the excerpt below shows
annotations present in a resource CRD, as a JSON object:

{
  "servicebindingoperator.redhat.io/status.connectionArguments.databaseName": "binding:env:attribute",
  "servicebindingoperator.redhat.io/status.connectionArguments.host": "binding:env:attribute",
  "servicebindingoperator.redhat.io/status.connectionArguments.port": "binding:env:attribute",
  "servicebindingoperator.redhat.io/status.connectionCredentials-password": "binding:env:object:secret",
  "servicebindingoperator.redhat.io/status.connectionCredentials-username": "binding:env:object:secret",
  "servicebindingoperator.redhat.io/status.externalConnectionArguments-databaseName": "binding:env:object:configmap",
  "servicebindingoperator.redhat.io/status.externalConnectionArguments-host": "binding:env:object:configmap",
  "servicebindingoperator.redhat.io/status.externalConnectionArguments-port": "binding:env:object:configmap"
}

Before the changes introduced in this PR, the following environment variables
are generated and encoded in the service binding intermediate secret, where
the issue can be seen where the string STATUS_EXTERNALCONNECTIONARGUMENTS
and STATUS_CONNECTIONCREDENTIALS can be read:

NAME            TYPE    KEY                                                                     VALUE
service-binding Opaque  DATABASE_CONFIGMAP_STATUS_EXTERNALCONNECTIONARGUMENTS_DATABASENAME      "db-name"
service-binding Opaque  DATABASE_CONFIGMAP_STATUS_EXTERNALCONNECTIONARGUMENTS_HOST              "database.prod.example.com"
service-binding Opaque  DATABASE_CONFIGMAP_STATUS_EXTERNALCONNECTIONARGUMENTS_PORT              "5432"
service-binding Opaque  DATABASE_CONNECTIONARGUMENTS_DATABASENAME                               "db-demo"
service-binding Opaque  DATABASE_CONNECTIONARGUMENTS_HOST                                       "192.168.1.100"
service-binding Opaque  DATABASE_CONNECTIONARGUMENTS_PORT                                       "5432"
service-binding Opaque  DATABASE_SECRET_STATUS_CONNECTIONCREDENTIALS_PASSWORD                   "hunter2"
service-binding Opaque  DATABASE_SECRET_STATUS_CONNECTIONCREDENTIALS_USERNAME                   "AzureDiamond"

After the changes introduced in this PR, the following environment variables
are produced (please observe the absence of the strings mentioned above):

NAME            TYPE    KEY                                             VALUE
service-binding Opaque  DATABASE_CONFIGMAP_DATABASENAME                 "db-name"
service-binding Opaque  DATABASE_CONFIGMAP_HOST                         "database.prod.example.com"
service-binding Opaque  DATABASE_CONFIGMAP_PORT                         "5432"
service-binding Opaque  DATABASE_CONNECTIONARGUMENTS_DATABASENAME       "db-demo"
service-binding Opaque  DATABASE_CONNECTIONARGUMENTS_HOST               "192.168.1.100"
service-binding Opaque  DATABASE_CONNECTIONARGUMENTS_PORT               "5432"
service-binding Opaque  DATABASE_SECRET_PASSWORD                        "hunter2"
service-binding Opaque  DATABASE_SECRET_USERNAME                        "AzureDiamond"

The same behavior can be seen when changing the annotations to include the entire object, secret or config map as well; for example, the three annotations below should produce the same environment variables as above:

{
  "servicebindingoperator.redhat.io/status.connectionArguments": "binding:env:attribute",
  "servicebindingoperator.redhat.io/status.connectionCredentials": "binding:env:object:secret",
  "servicebindingoperator.redhat.io/status.externalConnectionArguments": "binding:env:object:configmap"
}

As can be observed above, the same extra information is present in the resulting secret:

NAME            TYPE    KEY                                                                     VALUE
service-binding Opaque  DATABASE_CONFIGMAP_STATUS_EXTERNALCONNECTIONARGUMENTS_DATABASENAME      "db-name"
service-binding Opaque  DATABASE_CONFIGMAP_STATUS_EXTERNALCONNECTIONARGUMENTS_HOST              "database.prod.example.com"
service-binding Opaque  DATABASE_CONFIGMAP_STATUS_EXTERNALCONNECTIONARGUMENTS_PORT              "5432"
service-binding Opaque  DATABASE_CONNECTIONARGUMENTS_DATABASENAME                               "db-demo"
service-binding Opaque  DATABASE_CONNECTIONARGUMENTS_HOST                                       "192.168.1.100"
service-binding Opaque  DATABASE_CONNECTIONARGUMENTS_PORT                                       "5432"
service-binding Opaque  DATABASE_SECRET_STATUS_CONNECTIONCREDENTIALS_PASSWORD                   "hunter2"
service-binding Opaque  DATABASE_SECRET_STATUS_CONNECTIONCREDENTIALS_USERNAME                   "AzureDiamond"

And, as expected, after the changes in this PR are applied:

NAME            TYPE    KEY                                             VALUE
service-binding Opaque  DATABASE_CONFIGMAP_DATABASENAME                 "db-name"
service-binding Opaque  DATABASE_CONFIGMAP_HOST                         "database.prod.example.com"
service-binding Opaque  DATABASE_CONFIGMAP_PORT                         "5432"
service-binding Opaque  DATABASE_CONNECTIONARGUMENTS_DATABASENAME       "db-demo"
service-binding Opaque  DATABASE_CONNECTIONARGUMENTS_HOST               "192.168.1.100"
service-binding Opaque  DATABASE_CONNECTIONARGUMENTS_PORT               "5432"
service-binding Opaque  DATABASE_SECRET_PASSWORD                        "hunter2"
service-binding Opaque  DATABASE_SECRET_USERNAME                        "AzureDiamond"

For further more details refer the CONTRIBUTING.md

@isutton
Copy link
Contributor Author

isutton commented Jun 26, 2020

/retest

1 similar comment
@isutton
Copy link
Contributor Author

isutton commented Jul 1, 2020

/retest

@qibobo
Copy link
Contributor

qibobo commented Jul 6, 2020

@isutton
I went though this PR and my PR #521. And I think they are not fixing the same issue.

This PR522 is worked for the SBR below after the annotation is added:
servicebindingoperator.redhat.io/status.secretName":"binding:env:object:secret

apiVersion: apps.openshift.io/v1alpha1
kind: ServiceBindingRequest
metadata:
  name: binding-request-vcap
spec:
  applicationSelector:
    resourceRef: knative-vcap
    group: serving.knative.dev 
    version: v1 
    resource: services 
  backingServiceSelectors:
    - group: ibmcloud.ibm.com
      version: v1alpha1
      kind: Binding
      resourceRef: thetranslator5
      envVarPrefix: serviceprefix

But there is error for custom env var as we talked about in issue #475 for the SBR below:

apiVersion: apps.openshift.io/v1alpha1
kind: ServiceBindingRequest
metadata:
  name: binding-request-vcap
spec:
  applicationSelector:
    resourceRef: knative-vcap
    group: serving.knative.dev 
    version: v1 
    resource: services 
  backingServiceSelectors:
    - group: ibmcloud.ibm.com
      version: v1alpha1
      kind: Binding
      resourceRef: thetranslator5
      envVarPrefix: serviceprefix
  customEnvVar:
    - name: VCAP_SERVICES
      value: '{"language-translator":[{"credentials":{{json .v1alpha1.ibmcloud_ibm_com.Binding.thetranslator5.status.secretName}},"name":"thetranslator5","plan":"standard"}]}'
    - name: TRANSLATOR_NAME
      value: "thetranslator5"

So raised the PR #521 to fix it.
So the PR521 and PR522 are not conflicting.

@isutton isutton marked this pull request as ready for review July 10, 2020 09:20
@cdlliuy
Copy link
Contributor

cdlliuy commented Jul 14, 2020

@isutton , as discussed on the meeting, can you help to move forward of this PR and #515, so that we can add #521 on top of them?

@cdlliuy
Copy link
Contributor

cdlliuy commented Jul 14, 2020

@isutton I noticed #515 is updated .
Will you merge #522 first? Then merge #515 ? So that we can do update on #521?

@DhritiShikhar
Copy link
Contributor

/lgtm
/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DhritiShikhar

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:

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Revert annotation handling mechanism to not use the original config map or secret resource name location
6 participants