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

New Annotations/descriptors for all use-cases #25

Merged
merged 11 commits into from
Apr 9, 2020

Conversation

sbose78
Copy link
Contributor

@sbose78 sbose78 commented Apr 8, 2020

Based on extensive discussions in #21 , I've drafted how the annotations and descriptors need to look like.

  • Details of reasonable defaults in the data model
  • Update primary readme with a summarization on the annotations.

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
annotations.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
annotations.md Outdated Show resolved Hide resolved
Copy link
Member

@arthurdm arthurdm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome job @sbose78, thanks. I think this is simple yet flexible enough to cater for complex cases as well.

I put some minor comments for more clarity. Another thing if you wanted to do a global search/replace is there were a few instances of configmap vs ConfigMap and secret vs Secret - just for the syntax police out there. :)

Let me know when it's ready to merge. 👍

@sbose78
Copy link
Contributor Author

sbose78 commented Apr 8, 2020

All Secret and configmap vs ConfigMap textual changes done 👍 . Thanks for catching that. I'm good to have this merged.


During a binding operation, annotations from relevant Kubernetes resources are extracted to gather information about what is interesting for binding. This information is eventually used to bind the application with the backing service by populating the binding Secret.

### Requirements for specifying binding information in a backing service CRD / Kubernetes resource
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just trying to see if any of these scenarios can extract binding info from Kafka CR e.g.

status:
  listeners:
    - type: plain
      addresses:
        - host: my-cluster-kafka-bootstrap.service-binding-demo.svc
          port: 9092
        - host: my-cluster-kafka-bootstrap.service-binding-demo.svc
          port: 9093
    - type: tls
      addresses:
        - host: my-cluster-kafka-bootstrap.service-binding-demo.svc
          port: 9094

I'm wondering if supporting templating would cover cases we haven't thought of. Maybe something like:

    - path: listeners
      x-descriptors:
        - servicebinding:prefix:elementType=template:source:{{ GO_TEMPLATE }}:bindAs=volume

And the Go templates generates the following:

plain_0=my-cluster-kafka-bootstrap.service-binding-demo.svc:9092
plain_1=my-cluster-kafka-bootstrap.service-binding-demo.svc:9093
tls_0=my-cluster-kafka-bootstrap.service-binding-demo.svc:9094

After adding the prefix, the secret would look like this:

PREFIX_PLAIN_0: my-cluster-kafka-bootstrap.service-binding-demo.svc:9092
...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Escape hatch! :) Let me think.

Copy link
Member

@arthurdm arthurdm Apr 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ya - I think that's a great point @navidsh. This essentially allows the Operator author to define these complex mappings without changing their controller code and without requiring each consumer to have to deal with that complexity in the dataMappings portion of their ServiceBinding.

@sbose78 - I believe if we introduced Navid's proposal then we could close this issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@navidsh could you help me with a complete example please? I will work on introducing this in a separate PR today.

@sbose78
Copy link
Contributor Author

sbose78 commented Apr 8, 2020

On a second look, this does look like a very complex structure - we can realistically support only top-level items in a slice of maps and slice of strings.

Therefore, we need a "customEnvVar"-styled thing which you've suggested - and it's a good escape hatch.

annotations.md Outdated
* `sourceValue`: Specifies the key in the slice of maps whose value would be used as the value, corresponding to the value of the `sourceKey` which is added as the key, in the binding Secret. Mandatory only if `elementType` is `sliceOfMaps`.


### A Sample CR : The Kubernetes resource that the appliaction would bind to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo on application

annotations.md Outdated
- path: data.dbConfiguration
x-descriptors:
- urn:alm:descriptor:io.kubernetes:ConfigMap
- servicebinding:certificate:bindAs=volume
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably meant bindAs=envVar or omit it?

annotations.md Outdated
```
- path: data.uri
x-descriptors:
- servicebinding:connectionURL
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps this is reversed? should be data.connectionURL and servicebinding:uri

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup!

- servicebinding:timeout:sourceKey=db_timeout
```

6. #### Use the attribute “status.data.url”
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we used both url and uri in this example. I think you meant to use uri in all cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup!

@sbose78
Copy link
Contributor Author

sbose78 commented Apr 9, 2020

Comments addressed in 6942689 . Thank you for the excellent review.

@sbose78
Copy link
Contributor Author

sbose78 commented Apr 9, 2020

.. and 0eae110

@arthurdm
Copy link
Member

arthurdm commented Apr 9, 2020

Awesome. Let's merge this now, and @navidsh will work with @sbose78 to get a subsequent PR going with the complex templating addition. Thanks guys!

@arthurdm arthurdm merged commit 8d32ba7 into servicebinding:master Apr 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants