Add template.openshift.io/expose annotation for use with tsb bind #14486

jim-minter · 2017-06-06T12:15:08Z

jim-minter · 2017-06-06T12:15:30Z

[test][testextended][extended:core(templates)]

jim-minter · 2017-06-06T12:22:59Z

@openshift/api-review @pmorie @bparees @jwforres ptal - addition of annotation "template.openshift.io/expose" (name and semantics) as well as template service broker bind response. A sample tsb bind response follows:

{
  "credentials": {
   "Route.route.openshift.io.cakephp-mysql-example.host": "cakephp-mysql-example-demo.router.default.svc.cluster.local",
   "Route.route.openshift.io.cakephp-mysql-example.path": "",
   "Secret.cakephp-mysql-example.database-password": "TkJXRWtUVmdRc1N3ZklKUQ==",
   "Secret.cakephp-mysql-example.database-user": "Y2FrZXBocA==",
   "Service.cakephp-mysql-example.clusterIP": "172.30.113.152",
   "Service.cakephp-mysql-example.port": 8080,
   "Service.cakephp-mysql-example.port.web": 8080
  }
}

smarterclayton · 2017-06-06T13:12:45Z

Separation between resource and name is questionable. If that's our whole API surface we have a problem of both expressiveness and future proofing. This looks like we are abusing the credentials api On Jun 6, 2017, at 8:23 AM, Jim Minter <notifications@github.com> wrote: @openshift/api-review <https://github.com/orgs/openshift/teams/api-review> @pmorie <https://github.com/pmorie> @bparees <https://github.com/bparees> @jwforres <https://github.com/jwforres> ptal - addition of annotation " template.openshift.io/expose" (name and semantics) as well as template service broker bind response. A sample tsb bind response follows: { "credentials": { "Route.route.openshift.io.cakephp-mysql-example.host": "cakephp-mysql-example-demo.router.default.svc.cluster.local", "Route.route.openshift.io.cakephp-mysql-example.path": "", "Secret.cakephp-mysql-example.database-password": "TkJXRWtUVmdRc1N3ZklKUQ==", "Secret.cakephp-mysql-example.database-user": "Y2FrZXBocA==", "Service.cakephp-mysql-example.clusterIP": "172.30.113.152", "Service.cakephp-mysql-example.port": 8080, "Service.cakephp-mysql-example.port.web": 8080 } } — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub <#14486 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p7bpxAtBB_hJUILgzl5_R_riyw_Fks5sBUSmgaJpZM4NxQgx> .

bparees · 2017-06-06T20:07:05Z

Separation between resource and name is questionable. If that's our whole API surface we have a problem of both expressiveness and future proofing.

you mean the lack of separation? the way we concatenated the resource type and the key name?

Our problem is the limitations of the service broker bind credentials api. If we want our credential object to be easily consumed by applications, we really need to stick to flat key/value structures.

Otherwise applications are going to be forced to parse json to extract configs/credentials/service-addresses

This looks like we are abusing the credentials api

how so? because these things are not all credentials? because we're not creating a more sophisticated structure?

@pmorie do we have examples of what other brokers are returning for bind credential objects?

deads2k · 2017-06-07T13:28:40Z

you mean the lack of separation? the way we concatenated the resource type and the key name?

That's how I read it and mirrors the comment I was going to post about the key-space. You have no reliable way to know where the end of the group is (dots and dashes are legal in may resource names) so generic parsing is impossible (ambiguous) leading to specific whitelists.

Another issue is that you're choosing your favorite part of the API object and returning back a format tailored for that. If you used a generic format (something like json path from the root for instance), you'd face fewer problems if you later wanted to allow pulling a different value back and the behavior would be generic across any future resource type.

bparees · 2017-06-07T13:45:15Z

You have no reliable way to know where the end of the group is (dots and dashes are legal in may resource names) so generic parsing is impossible (ambiguous) leading to specific whitelists.

yeah i anticipated this concern(at least that we might have to choose a different separator), but do you have an alternate suggestion? Again we strongly prefer a flat structure because otherwise we're putting a high burden on the applications that consume this content.

Another issue is that you're choosing your favorite part of the API object and returning back a format tailored for that.

if i'm understanding you correctly, this is a complaint about the annotation scheme used for selecting the keys, not the format of the key/value pairs being returned in the credential object.

it's a fair point, but it's also going to make it even harder to sensibly define a key for the content the jsonpath extracts (and of course make life harder for the template authors who have to craft jsonpath expressions now).

bparees

couple nits otherwise lgtm, @smarterclayton and @deads2k's concerns about the annotation syntax and credential key/value format notwithstanding.

bparees · 2017-06-07T17:23:55Z

examples/quickstarts/cakephp-mysql-persistent.json

+        "name": "${NAME}",
+        "annotations": {
+          "template.openshift.io/expose": ""
+        }


I clearly need to find a way to make this readme more visible :(

https://github.com/openshift/origin/blob/master/examples/quickstarts/README.md

Files in this directory are automatically pulled down, do not modify/add files to this directory.

Snort. How about a file called CONTENT_IN_THIS_DIRECTORY_IS_GENERATED_ELSEWHERE (or whatever)?

OK, I'm going to leave these changes in the PR for now as one of them is required for the extended test to work. After api-review and before merge I'll send PRs to the upstream repos and run hack/update-external-examples.sh. Can't wait.

sounds good.

bparees · 2017-06-07T17:41:45Z

pkg/template/servicebroker/bind_test.go

+				"Route.route.openshift.io.myroute.host": route.Spec.Host,
+				"Route.route.openshift.io.myroute.path": route.Spec.Path,
+			},
+		},


no test for "bad" on this one?

no, because as things stand the value of expose is currently ignored on routes.

jim-minter · 2017-06-08T12:44:43Z

@bparees @openshift/api-review thanks for the feedback, I was psyched that no-one took issue with the annotation name. Anyway, please take another look. I've reworked this to use JSON path and I think the whole thing is better as a result. Sample input:

On a route: "annotations": { "template.openshift.io/expose-uri": "http://{.spec.host}{.spec.path}" }
On a secret: "annotations": { "template.openshift.io/expose-username": "{.data['username']}", "template.openshift.io/expose-password": "{.data['password']}" }

Sample bind result:

{
  "credentials": {
    "uri": "http://cakephp-mysql-example-demo.router.default.svc.cluster.local",
    "username": "user",
    "password": password"
  }
}

(c.f. contents of "credentials" in spec example bind result: https://github.com/openservicebrokerapi/servicebroker/blob/master/spec.md#response-4)

jim-minter · 2017-06-09T11:47:54Z

ping @openshift/api-review is there any chance of feedback on this today?

deads2k · 2017-06-09T17:37:54Z

Can you update the result of the bind call in your first comment? It still looks like you may be suffering from an inconsistent parse problem in those keys. kind.group/name/key would be guaranteed to parse predictably.

deads2k · 2017-06-09T17:39:19Z

Can you update the result of the bind call in your first comment? It still looks like you may be suffering from an inconsistent parse problem in those keys. kind.group/name/key would be guaranteed to parse predictably.

Ah, I see you did it in a later comment. That did come out quite a bit nicer.

jim-minter · 2017-06-09T19:13:16Z

@deads2k @smarterclayton but can it get the approved label? The two points I'd highlight on my side:

The same key could be exposed twice, either in the same object or across multiple objects. It might be easiest to doc the results of doing this as being undefined, but if required I could also sort by object type / object name / annotation name to make it well defined which value wins in this circumstance. Please advise if the latter is wanted/a requirement.
Given this is a completely different API to k8s, as a nod to usability I would quite like to straight convert any exposed []byte data (e.g. secret data) to string to hand it back to the service broker without base64 encoding it. The conversion would just be the Golang default one - i.e. interpret the []byte as UTF-8 and insert U+FFFD for any undecodable rune. I think @jwforres also expressed a preference for this. I don't fundamentally mind which way this goes, I just want the nod to get the PR moved on.

smarterclayton · 2017-06-09T19:33:55Z

For 2, wouldn't that mean that we lose data? UTF -> JSON -> raw is lossy.

bparees · 2017-06-09T19:34:20Z

Given this is a completely different API to k8s, as a nod to usability I would quite like to straight convert any exposed []byte data (e.g. secret data) to string to hand it back to the service broker without base64 encoding it.

there's no guarantee the byte data can be represented as a non-base64 encoded string. I can put a binary secret into my secret object. I don't see how we can expect to reliably decode the field.

from the Secret api doc:

	// Data contains the secret data.  Each key must be a valid DNS_SUBDOMAIN
	// or leading dot followed by valid DNS_SUBDOMAIN.
	// The serialized form of the secret data is a base64 encoded string,
	// representing the arbitrary (possibly non-string) data value here.
	// +optional
	Data map[string][]byte

smarterclayton · 2017-06-09T19:35:00Z

I don't even think you can safely encode \x00 for instance.

jim-minter · 2017-06-09T19:53:07Z

I'm just putting out on the table the idea of losing the ability to pass through arbitrary raw binary, for the benefit of people not worrying about their password xyzzy coming through in base64, in what is somewhat an entry level technology anyway. Feel free to say no.

The loss is when the binary can't be decoded into a UTF-8 string. @smarterclayton I think anything UTF-8 is then representable in json including \x00 - Go marshals it to "\u0000", right?

smarterclayton · 2017-06-09T20:04:37Z

https://stackoverflow.com/questions/1443158/binary-data-in-json-string-something-better-than-base64

bparees · 2017-06-09T20:06:42Z

I'm just putting out on the table the idea of losing the ability to pass through arbitrary raw binary, for the benefit of people not worrying about their password xyzzy coming through in base64, in what is somewhat an entry level technology anyway. Feel free to say no.

i don't suppose there's a way to encode into the jsonpath extraction, whether we want the base64 encoded form, or the string form. Potentially it could be encoded into the annotation key itself. (default to strings, but if the annotation key is something like "template.openshift.io/expose-binary-mysecret", then we do a base64 encode instead?)

Barring that, both choices feel bad. Making app developers base64 decode their passwords is bad. Letting people create binary secrets and then not supporting them in the template broker is bad.

jim-minter · 2017-06-09T20:36:19Z

Right now, I'm hearing "base64-encode anything thats []byte under the covers because it's the least worst option that offers fidelity."

BTW, did I say I'd love to have api approval on this before Monday UK time so that this has a chance to merge? 😄

bparees · 2017-06-09T20:40:39Z

Right now, I'm hearing "base64-encode anything thats []byte under the covers because it's the least worst option that offers fidelity."

i like my secondary annotation option that allows users to choose base64 or string encoding.

bparees · 2017-06-09T20:42:08Z

@openshift/api-review please sign off on the concept of this annotation(with key suffix)+jsonpath extraction scheme. We can iterate on:

naming
base64 encoding or not (or having 2 annotations, one for base64 and one for string extraction)

as a follow up.

smarterclayton · 2017-06-09T21:35:46Z

Approved.

bparees · 2017-06-09T22:00:51Z

@jim-minter squash it and i'll merge.

we can negotiate the binary-content behavior on monday.

jim-minter · 2017-06-13T11:36:34Z

@bparees updated, ptal.

jim-minter · 2017-06-13T11:52:42Z

The following PRs have been opened which should hopefully mean that this PR is mergeable as is, at least from an examples/quickstarts point of view:

sclorg/cakephp-ex#72
sclorg/dancer-ex#61
sclorg/django-ex#89
sclorg/nodejs-ex#125
sclorg/rails-ex#87

bparees

@jim-minter two nits and then lgtm.

bparees · 2017-06-13T12:07:52Z

pkg/template/servicebroker/bind.go

+	for _, r := range results {
+		switch len(r) {
+		case 0:
+			// shouldn't happen


do we not want to at least log an error indicating what the jsonpath was and the object we searched, if this happens then?

bparees · 2017-06-13T12:08:41Z

pkg/template/servicebroker/bind.go

+
+		default:
+			// we don't permit individual JSONPath expressions which return
+			// multiple objects as we haven't decided how these should be output


this should be part of the api doc for the annotation. (that the jsonpath expression must select non-compound fields)

jim-minter · 2017-06-13T16:30:57Z

@bparees nits addressed, please merge

bparees · 2017-06-13T16:45:28Z

[merge]

@jim-minter we need follow up discussion with @openshift/api-review to finalize agreement on:

the annotation name(s)
the binary vs string mechanism
the restriction to primitive value extraction only.

Probably best to open that as a fresh issue and have the discussion there.

bparees · 2017-06-13T18:58:13Z

at least one of these failures is legit:

--- FAIL: TestEvaluateJSONPathExpression (0.00s)
	bind_test.go:118: 2: error "3 JSONPath results found on annotation a"

jim-minter · 2017-06-14T08:49:19Z

oops, forgot to update error message in unit test - fixed and pushed
[test]

jim-minter · 2017-06-14T10:57:21Z

@bparees tests passed please rehit merge

bparees · 2017-06-14T13:00:39Z

[merge][severity:blocker]

openshift-bot · 2017-06-16T00:55:40Z

continuous-integration/openshift-jenkins/merge Waiting: You are in the build queue at position: 10

smarterclayton · 2017-06-16T01:36:29Z

Weird [test]

jim-minter · 2017-06-16T13:38:38Z

rebased. [test]

openshift-bot · 2017-06-16T13:40:05Z

Evaluated for origin testextended up to a8b2dba

bparees · 2017-06-16T13:51:59Z

re[merge][severity:blocker]

openshift-bot · 2017-06-16T13:56:10Z

Evaluated for origin merge up to a8b2dba

openshift-bot · 2017-06-16T14:32:17Z

continuous-integration/openshift-jenkins/testextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended/638/) (Base Commit: 967d057) (PR Branch Commit: a8b2dba) (Extended Tests: core(templates))

bparees · 2017-06-16T15:43:23Z

[test]

…

On Fri, Jun 16, 2017 at 11:40 AM, OpenShift Bot ***@***.***> wrote: continuous-integration/openshift-jenkins/test FAILURE ( https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2307/) (Base Commit: 967d057 <967d057>) (PR Branch Commit: a8b2dba <a8b2dba> ) — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#14486 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEvl3hSepvmjeUcEzaDbeVmTXCXbzaXHks5sEqHngaJpZM4NxQgx> .

-- Ben Parees | OpenShift

bparees · 2017-06-16T18:18:34Z

[test]

…

On Fri, Jun 16, 2017 at 2:08 PM, OpenShift Bot ***@***.***> wrote: continuous-integration/openshift-jenkins/test FAILURE ( https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2316/) (Base Commit: 4aa3720 <4aa3720>) (PR Branch Commit: a8b2dba <a8b2dba> ) — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#14486 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEvl3mIPaYYj5BqI6FkRGP_AE1M0FNoiks5sEsSagaJpZM4NxQgx> .

-- Ben Parees | OpenShift

openshift-bot · 2017-06-16T18:24:01Z

Evaluated for origin test up to a8b2dba

openshift-bot · 2017-06-16T20:18:37Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2326/) (Base Commit: d40783e) (PR Branch Commit: a8b2dba)

jim-minter added the needs-api-review label Jun 6, 2017

jim-minter self-assigned this Jun 6, 2017

jim-minter force-pushed the trello133-tsb-bind branch 2 times, most recently from e96f64c to 2f22b2d Compare June 6, 2017 17:57

bparees requested changes Jun 7, 2017

View reviewed changes

smarterclayton added api-approved and removed needs-api-review labels Jun 9, 2017

jim-minter force-pushed the trello133-tsb-bind branch from 6d95eba to 51abb59 Compare June 13, 2017 11:34

bparees requested changes Jun 13, 2017

View reviewed changes

jim-minter force-pushed the trello133-tsb-bind branch from 51abb59 to 56917d4 Compare June 13, 2017 16:30

jim-minter force-pushed the trello133-tsb-bind branch from 56917d4 to 6db9d38 Compare June 14, 2017 08:49

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 15, 2017

jim-minter mentioned this pull request Jun 16, 2017

update templates to use template.openshift.io/expose-uri sclorg/httpd-ex#3

Merged

Add template.openshift.io/expose annotation for use with tsb bind

a8b2dba

jim-minter force-pushed the trello133-tsb-bind branch from 6db9d38 to a8b2dba Compare June 16, 2017 13:38

jim-minter removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 16, 2017

bparees self-assigned this Jun 16, 2017

smarterclayton merged commit 5891474 into openshift:master Jun 16, 2017

Add template.openshift.io/expose annotation for use with tsb bind #14486

Add template.openshift.io/expose annotation for use with tsb bind #14486

Conversation

jim-minter commented Jun 6, 2017

jim-minter commented Jun 6, 2017

jim-minter commented Jun 6, 2017

smarterclayton commented Jun 6, 2017 via email

bparees commented Jun 6, 2017

deads2k commented Jun 7, 2017

bparees commented Jun 7, 2017

bparees left a comment

Choose a reason for hiding this comment

bparees Jun 7, 2017

Choose a reason for hiding this comment

jim-minter Jun 8, 2017 • edited

Choose a reason for hiding this comment

jim-minter Jun 8, 2017

Choose a reason for hiding this comment

bparees Jun 8, 2017

Choose a reason for hiding this comment

bparees Jun 7, 2017

Choose a reason for hiding this comment

jim-minter Jun 8, 2017

Choose a reason for hiding this comment

jim-minter commented Jun 8, 2017 • edited

jim-minter commented Jun 9, 2017

deads2k commented Jun 9, 2017

deads2k commented Jun 9, 2017

jim-minter commented Jun 9, 2017

smarterclayton commented Jun 9, 2017

bparees commented Jun 9, 2017

smarterclayton commented Jun 9, 2017

jim-minter commented Jun 9, 2017

smarterclayton commented Jun 9, 2017

bparees commented Jun 9, 2017

jim-minter commented Jun 9, 2017

bparees commented Jun 9, 2017

bparees commented Jun 9, 2017

smarterclayton commented Jun 9, 2017

bparees commented Jun 9, 2017

jim-minter commented Jun 13, 2017

jim-minter commented Jun 13, 2017

bparees left a comment

Choose a reason for hiding this comment

bparees Jun 13, 2017

Choose a reason for hiding this comment

bparees Jun 13, 2017

Choose a reason for hiding this comment

jim-minter commented Jun 13, 2017

bparees commented Jun 13, 2017

bparees commented Jun 13, 2017

jim-minter commented Jun 14, 2017

jim-minter commented Jun 14, 2017

bparees commented Jun 14, 2017 • edited

openshift-bot commented Jun 16, 2017 • edited

smarterclayton commented Jun 16, 2017

jim-minter commented Jun 16, 2017

openshift-bot commented Jun 16, 2017

bparees commented Jun 16, 2017

openshift-bot commented Jun 16, 2017

openshift-bot commented Jun 16, 2017

bparees commented Jun 16, 2017 via email

bparees commented Jun 16, 2017 via email

openshift-bot commented Jun 16, 2017

openshift-bot commented Jun 16, 2017

jim-minter Jun 8, 2017 •

edited

jim-minter commented Jun 8, 2017 •

edited

bparees commented Jun 14, 2017 •

edited

openshift-bot commented Jun 16, 2017 •

edited