Add K8S and CF annotations to context object and CF bind resource #658

gberche-orange · 2019-04-24T13:23:33Z

What is the problem this PR solves?

Addresses #654

Checklist:

The swagger.yaml doc has been updated with any required changes (no changes required: the Context object is not strongly typed)

Lines 676 to 681 in cc11bc5

    
             Context: 
        
               description: >- 
        
                 See [Context 
        
                 Conventions](https://github.com/openservicebrokerapi/servicebroker/blob/master/profile.md#context-object) 
        
                 for more details. 
        
               type: object

The openapi.yaml doc has been updated with any required changes (no changes required: the Context object is not strongly typed)

servicebroker/openapi.yaml

Lines 854 to 856 in d1e1273

    
           Context: 
        
             description: "See [Context Conventions](https://github.com/openservicebrokerapi/servicebroker/blob/master/profile.md#context-object) for more details." 
        
             type: object

cfdreddbot · 2019-04-24T13:23:34Z

✅ Hey gberche-orange! The commit authors and yourself have already signed the CLA.

profile.md

waterlink · 2019-04-24T16:21:15Z

@gberche-orange

I’m concerned about: what will happen if a service-broker wants to support a 3rd platform (not CF and not K8S), and that platform has a slightly different concept? Will we have to add one or two more *_annotation? Should then all service broker authors make changes to support these? Or should service brokers be able to handle any_annotation as long as they can recognise the keys inside (like your example send-email-alert-to)?

Another thing that I’ve noticed: instance_annotations and serviceinstance_annotations seem to be equivalent to me (one comes from CF, and another from K8S). Do you think it makes sense to merge them into a single key?

gberche-orange · 2019-04-25T15:14:08Z

thanks @waterlink for your review

I’m concerned about: what will happen if a service-broker wants to support a 3rd platform (not CF and not K8S), and that platform has a slightly different concept? Will we have to add one or two more *_annotation?

The extension of the profile.md to additional platforms beyond CF and K8S does not seem specific to annotations. The current specs do not state whether new plaforms names can be sent without changing the specs to register their platform name.

Should then all service broker authors make changes to support these?

I would expect service broker to only parse/handle context objects for platforms they are aware of, and either ignore or reject unsupported platform types as they would not be sure of what context keys to expect.

Or should service brokers be able to handle any_annotation as long as they can recognise the keys inside (like your example send-email-alert-to)?

Yes, generally brokers can't make any assumptions about the user provided annotations that they will receive in the context objects. They may choose to document the annotations they support/expect/require (e.g. in their documentation page url specified in catalog), and ignore annotations they are unaware of.

I can imagine that if a broker absolutely requires a mandatory inputs, it may offer its users to specify them at a higher level through annotations or explicitly through parameters that are documented in the catalog through associated Json schema.

We'll have to see in the future if there is demand for brokers to publish their supported annotations directly in the catalog through additional json schemas (e.g. using a key annotations in place of the existing parameters)

gberche-orange · 2019-04-25T15:17:12Z

@mattmcneeney the travis-ci build is apparently failing following your commit 275d5fa with the following message,

tools/../.github/pull_request_template.md: Can't find: tools/../.github/swagger.yaml
tools/../.github/pull_request_template.md: Can't find: tools/../.github/openapi.yaml

See build 524511401 for full traces.

mattmcneeney · 2019-04-25T15:27:35Z

Ah! Thanks @gberche-orange - I'm on the case now...

mattmcneeney · 2019-04-25T15:35:05Z

I believe this is fixed @gberche-orange if you can rebase this!

Close openservicebrokerapi#654

@waterlink

Fixed typos spotted by travis-ci build & @waterlink review

waterlink · 2019-04-25T16:51:09Z

@gberche-orange Thank you for addressing my concerns!

I still feel a bit weird about:

CF instance_annotations, and
K8S serviceinstance_annotations.

These seem to be terms for the same thing. So as a reader, my question: “Why they are not called the same thing (e.g. just instance_annotations)?”

Then my next thought is: “Oh, one comes from CF and another comes from K8S!” after reading a bit deeper.

And then my further line of thinking is: “Okay if we need to distinguish between CF and K8S annotations, then why not call them cf_instance_annotations and k8s_instance_annotations? This seems to be more clear, and deliver intent better.”

And when I’m asking this question to myself, I can come up with one hypothesis with my current knowledge: “Maybe we don’t want to mention cf and k8s in the JSON too much…”

Can you please help me understand if my line of thought is correct or not, and where did I take the wrong turn?

Other than that, the PR looks good to me! 👍

gberche-orange · 2019-04-25T17:13:10Z

thanks @waterlink for your review, sorry I missed this question in my previous answers

My rationales for keeping CF instance_annotations, and K8S serviceinstance_annotations keys distinct and not prefixed by their related platforms were:

I named CF instance_annotations to remain consistent with the instance_name in the Cloud Foundry Context Object and K8S serviceinstance_annotations to remain consistent with the ServiceInstance service catalog resource as I felt instance was ambiguous. I'm happy to revert that to instance_annotations if no one feels the same ambiguity and if consistency among K8S and CF context is felt more important.
The Context object contains a property platform which will be set as either cloudfoundry or kubernetes. I don't believe we need to disambiguate other properties in the context object since the platform key plays this role.

waterlink · 2019-04-25T20:48:57Z

@gberche-orange if we do make this key the same, then serviceinstance_annotations for both will make more sense because:

this is less ambiguous, and
even in CF Services API (sapi) team we call them "service instances" in our spoken language, user story writing, and the code: both backend and CLI.

gberche-orange · 2019-04-26T14:13:42Z

@mattmcneeney any thoughts on CF instance_annotations key consistency with the instance_name key that you seem to have authored recently in 0430220#diff-3b9a1a89335a911c0c69b58b043febbeR227 ?

mattmcneeney · 2019-04-26T14:43:49Z

I'm not sure I've quite followed this. Are we proposing that the context object has both instance_annotations and serviceinstance_annotations?

Refined annotations examples to be compliant with K8S annotations keys

gberche-orange · 2019-04-26T15:23:19Z

Are we proposing that the context object has both instance_annotations and serviceinstance_annotations?

No.

The CF context in this PR has an instance_annotations key to remain consistent with the instance_name you authored in the Cloud Foundry Context Object and the K8S context has a serviceinstance_annotations to remain consistent with the ServiceInstance service catalog resource.

@waterlink is suggesting in #658 (comment) to instead name the both keys in CF context and K8S context as serviceinstance_annotations

This creates an inconsistency in the CF context between instance_annotations and instance_name keys.

I was keen to get your opinion on whether you prefer :

a) consistency across K8S and CF contexts (ie both keys named serviceinstance_annotations)
b) consistency in the CF context itself (as currently in this PR).

Fix typo in Bind Resource Object annotation

mattmcneeney · 2019-04-26T15:42:30Z

Ah I've got it, thank you!

If possible, it would be good to use instance_annotations for both CF and K8S, to be consistent with the existing instance_name field.

However, if this isn't possible, I'd probably vote for consistency across K8S and CF contexts, and call them both serviceinstance_annotations. We would leave instance_name as it is.

Specify that When present the annotations must be non-empty. The rationale for this is to: - optimize the payload by avoiding that platforms send empty annotations keys - simplify service authors work that don't need to handle inconsistent platform behaviors The specs however let platform choose the criteria for sending annotations (e.g. sensible defaults or user opt-in), and therefore annotations keys are still optional.

Use instance_annotations keys for both CF and K8S, to be consistent with the existing instance_name field.

gberche-orange · 2019-04-26T15:52:49Z

thanks @mattmcneeney and @waterlink for inputs

If possible, it would be good to use instance_annotations for both CF and K8S, to be consistent with the existing instance_name field.

I've applied this in 12b46c7

@waterlink

please note that I also pushed further commits to:

make example consistent with K8S annotations format, that will be hopefully soon also enforced by CF (see Metadata annotation keys are lacking K8S prefix conventions cloudfoundry/cloud_controller_ng#1335)
fix typo on Bind Resource Object annotation key example
specify that when present the annotations must be non-empty. The rationale for this is to:
- optimize the payload by avoiding that platforms send empty annotations keys
- simplify service authors work that don't need to handle inconsistent platform behaviors

You may therefore need to approve again the PR with these changes.

gberche-orange · 2019-04-26T16:18:10Z

Note that the following CF resources that do not yet have annotation support implemented might need to be exposed in a future annotation-related PR:

route (see story). A new route_annotations key would be added in the CF Bind Resource object: annotations attached to the route that a Service Binding is associated with (i.e. the Binding is of type Route Services). Sample associated use-case: define implicit alerting target per route in a route service instance.
isolation segment (see story). A new isolation_segment_annotations would be added to the CF context. Sample associated use-case: define an implicit alerting target for all orgs in an isolation segments.

waterlink

@gberche-orange From CF SAPI perspective, everything looks good to me.

@mattmcneeney, are there any other comments from you that are not addressed?

I feel like this has lingered for already 2 weeks without updates. I think the only thing that’s left is to get review approvals from OSBAPI team. Or am I missing something?

waterlink · 2019-05-06T13:06:01Z

Also, Travis failure seem to be unrelated and seems to be an internet connectivity problem that Travis’ VM/container had. Someone who has permissions should re-trigger that.

waterlink · 2019-05-07T16:20:01Z

profile.md

+  This OPTIONAL property holds an object with the annotations key/value pairs. If present, this property MUST be an object, with one or more properties as follows:
+
+  ```
+  "organization_annotations": { "prefix-here.org/name-here":"value-here" }


@gberche-orange In the OSBAPI call we’ve realised that CF won’t have the prefixes here.

Is there expectation that the annotation object will always need such prefixes?, or is it only for K8S example?

@waterlink

I was hoping that the CF CAPI team would handle cloudfoundry/cloud_controller_ng#1335 bringing planned Cf/K8S consistency and that prefixes could be used to filter out private annotation keys as mentionned in #654 (comment) in a consistent way between CF and K8S:

As a 1st step, we can leverage K8S annotations naming conventions reproduced below to allow users to distinguish between public annotations (shared with all brokers) and private annotations (not shared with brokers):

I'm still waiting for CF CAPI response on this. The CF SAPI team could consider other alternative options for filtering private annotations though, and we could relax the CF examples in OSB API PR

fmui · 2019-05-23T10:53:49Z

Not sure if it fits into this PR or should be another one, but if we add instance annotations for K8S, I think we should also have the instance name in the context object.

jberkhahn · 2019-05-28T18:13:39Z

Looks like we're still waiting for the CF CAPI team to get back to us on this one?

georgi-lozev · 2019-05-29T05:19:06Z

It looks like that to me.

cloudfoundry/cloud_controller_ng#1335 (comment)

gberche-orange · 2019-06-05T07:32:45Z

CF CAPI team just confirmed they will be prioritizing enforcing the K8S prefix conventions to CF annotations, see cloudfoundry/cloud_controller_ng#1335 (comment)

mattmcneeney · 2019-06-11T15:48:02Z

@gberche-orange can this PR be merged before the associated CC change? @waterlink has kindly offered to take a look into this

gberche-orange · 2019-06-12T08:22:43Z

@mattmcneeney Yes, I believe this PR can be merged before user input validation by CF CC enforces the K8S prefix into the user-provided annotations. The examples in this PR will conform to future enforced model by both CF and K8S.

thanks @waterlink !

waterlink · 2019-06-14T13:56:23Z

@mattmcneeney @gberche-orange Yes, this can go forward, since this RFC was accepted in CAPI.

mattmcneeney · 2019-06-25T15:54:50Z

@zrob Are you happy for us to merge this into the spec, knowing that the associated CC change is in the backlog somewhere?

zrob · 2019-06-25T17:03:29Z

Sorry I'm behind the 8-ball on this proposal. Is the intent of this PR that ALL annotations of the various resources org,space,app be sent to a broker for every bind request?

profile.md

waterlink · 2019-06-26T17:44:21Z

User story in SAPI backlog to track the implementation of this on CF side: https://www.pivotaltracker.com/n/projects/2105761/stories/166935990

gberche-orange · 2019-06-27T07:24:31Z

User story in SAPI backlog to track the implementation of this on CF side: https://www.pivotaltracker.com/n/projects/2105761/stories/166935990

thanks @waterlink !

Related updating scenarios 3 and 4, I wonder whether in the future changes to annotations (org/space/service-instance) should automatically trigger a PATCH /v2/service_instances/:instance_id request with updated annotations (i.e. without user triggering a cf update-service, similar to https://www.pivotaltracker.com/story/show/163861852 (in which PATCH request was triggered by a cf rename-service and not an explicit cf update-service).

Currently with cf cli 6.45 a cf update-service command without argument seems to not trigger calls to cloudcontroller to update the instance:

cf update-service mysql
OK
No changes were made

CF_TRACE=true cf update-service mysql | grep -A1 REQUEST
REQUEST: [2019-06-27T09:23:16+02:00]
GET /v2/info HTTP/1.1
--
REQUEST: [2019-06-27T09:23:16+02:00]
GET /login HTTP/1.1

Refine cardinality of annotations object to accept updates clearing annotations Update version into which the feature was introduced from 2.15 to 2.16

jberkhahn

LGTM

mattmcneeney · 2019-07-25T10:00:12Z

@blgm Do you know if there is a story in the SAPI backlog for this? Happy to approve but just want to make sure this work is tracked somewhere

Edit: Sorry @blgm I just spotted this above: https://www.pivotaltracker.com/n/projects/2105761/stories/166935990

gberche-orange · 2019-09-20T06:38:37Z

@mattmcneeney thanks for merging this PR!

I notice this PR does not appear in the roadmap project for 2.16 https://github.com/openservicebrokerapi/servicebroker/projects/1

Would it make sense to associate this PR to the 2.16 milestone in the roadmap?

mattmcneeney · 2019-09-20T08:17:43Z

Good spot, thanks @gberche-orange - project updated!

gberche-orange · 2019-09-20T13:07:30Z

thanks @mattmcneeney !

gberche-orange mentioned this pull request Apr 24, 2019

Add K8S and CF annotations to context object and CF bind resource #654

Closed

waterlink reviewed Apr 24, 2019

View reviewed changes

profile.md Outdated Show resolved Hide resolved

profile.md Outdated Show resolved Hide resolved

gberche-orange added 2 commits April 25, 2019 17:51

Add K8S and CF annotations to context object and CF bind resource

1d9f007

Close openservicebrokerapi#654

Add K8S and CF annotations to context object and CF bind resource

d4d072f

Fixed typos spotted by travis-ci build & @waterlink review

gberche-orange force-pushed the annotations branch from 41362b1 to d4d072f Compare April 25, 2019 15:51

waterlink previously approved these changes Apr 25, 2019

View reviewed changes

gberche-orange mentioned this pull request Apr 26, 2019

Metadata annotation keys are lacking K8S prefix conventions cloudfoundry/cloud_controller_ng#1335

Closed

Add K8S and CF annotations to context object and CF bind resource

b93d59b

Refined annotations examples to be compliant with K8S annotations keys

gberche-orange dismissed waterlink’s stale review via b93d59b April 26, 2019 15:10

Add K8S and CF annotations to context object and CF bind resource

5362f2f

Fix typo in Bind Resource Object annotation

gberche-orange added 2 commits April 26, 2019 17:42

Add K8S and CF annotations to context object and CF bind resource

12b46c7

Use instance_annotations keys for both CF and K8S, to be consistent with the existing instance_name field.

waterlink previously approved these changes May 6, 2019

View reviewed changes

waterlink reviewed May 7, 2019

View reviewed changes

mattmcneeney added this to the 2.16 milestone Jun 24, 2019

fmui previously approved these changes Jun 25, 2019

View reviewed changes

waterlink reviewed Jun 26, 2019

View reviewed changes

profile.md Outdated Show resolved Hide resolved

waterlink reviewed Jun 26, 2019

View reviewed changes

profile.md Outdated Show resolved Hide resolved

Add K8S and CF annotations to context object and CF bind resource

80268a5

Refine cardinality of annotations object to accept updates clearing annotations Update version into which the feature was introduced from 2.15 to 2.16

gberche-orange dismissed stale reviews from fmui and waterlink via 80268a5 July 1, 2019 13:48

jberkhahn approved these changes Jul 1, 2019

View reviewed changes

fmui approved these changes Jul 9, 2019

View reviewed changes

mattmcneeney approved these changes Jul 25, 2019

View reviewed changes

mattmcneeney merged commit 896f342 into openservicebrokerapi:master Aug 16, 2019

gberche-orange mentioned this pull request May 13, 2020

Whitelisted annotations cloudfoundry/eirini#82

Closed

	Context:
	description: >-
	See [Context
	Conventions](https://github.com/openservicebrokerapi/servicebroker/blob/master/profile.md#context-object)
	for more details.
	type: object

	Context:
	description: "See [Context Conventions](https://github.com/openservicebrokerapi/servicebroker/blob/master/profile.md#context-object) for more details."
	type: object

Add K8S and CF annotations to context object and CF bind resource #658

Add K8S and CF annotations to context object and CF bind resource #658

Conversation

gberche-orange commented Apr 24, 2019

cfdreddbot commented Apr 24, 2019

waterlink commented Apr 24, 2019 • edited Loading

gberche-orange commented Apr 25, 2019

gberche-orange commented Apr 25, 2019

mattmcneeney commented Apr 25, 2019

mattmcneeney commented Apr 25, 2019

waterlink commented Apr 25, 2019

gberche-orange commented Apr 25, 2019 • edited Loading

waterlink commented Apr 25, 2019

gberche-orange commented Apr 26, 2019

mattmcneeney commented Apr 26, 2019

gberche-orange commented Apr 26, 2019

mattmcneeney commented Apr 26, 2019

gberche-orange commented Apr 26, 2019 • edited Loading

gberche-orange commented Apr 26, 2019

waterlink left a comment • edited Loading

Choose a reason for hiding this comment

waterlink commented May 6, 2019

waterlink May 7, 2019

Choose a reason for hiding this comment

gberche-orange May 7, 2019

Choose a reason for hiding this comment

fmui commented May 23, 2019

jberkhahn commented May 28, 2019

georgi-lozev commented May 29, 2019

gberche-orange commented Jun 5, 2019

mattmcneeney commented Jun 11, 2019

gberche-orange commented Jun 12, 2019

waterlink commented Jun 14, 2019

mattmcneeney commented Jun 25, 2019

zrob commented Jun 25, 2019

waterlink commented Jun 26, 2019

gberche-orange commented Jun 27, 2019

jberkhahn left a comment

Choose a reason for hiding this comment

mattmcneeney commented Jul 25, 2019 • edited Loading

gberche-orange commented Sep 20, 2019

mattmcneeney commented Sep 20, 2019

gberche-orange commented Sep 20, 2019

waterlink commented Apr 24, 2019 •

edited

Loading

gberche-orange commented Apr 25, 2019 •

edited

Loading

gberche-orange commented Apr 26, 2019 •

edited

Loading

waterlink left a comment •

edited

Loading

mattmcneeney commented Jul 25, 2019 •

edited

Loading