Metadata annotation keys are lacking K8S prefix conventions

[gberche-orange]

Issue

Documentation (api and user manual) and CC do not expect nor enforce that annotations conform to K8S specifications on annotations keys:

Valid annotation keys have two segments: an optional prefix and name, separated by a slash (/). The name segment is required and must be 63 characters or less, beginning and ending with an alphanumeric character ([a-z0-9A-Z]) with dashes (-), underscores (_), dots (.), and alphanumerics between. The prefix is optional. If specified, the prefix must be a DNS subdomain: a series of DNS labels separated by dots (.), not longer than 253 characters in total, followed by a slash (/).
If the prefix is omitted, the annotation Key is presumed to be private to the user.

Context

Metadata design doc mentions:

The CF V3 API will match as closely as possible the kubernetes Annotations API. API resources will gain a “metadata” object with a valid key of “annotations”. The structure of “annotations” will be an object of key-value pairs. These values will not be queryable.

The OSB API is planning to publish "public annotations" to service brokers in openservicebrokerapi/servicebroker#658 in a consistent way between K8S and CF.

The lack of consistency on annotation keys between CF and K8S makes this harder. In particular to be able to skip "private annotations" from being shared with service brokers. See related openservicebrokerapi/servicebroker#654 (comment)

How do you think we can find out whether or not sharing all resource metadata with service providers could be a security issue for some users? Do we need to make this an opt-in feature?

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)

Steps to Reproduce

Documentation (api and user manual) do not mention annotation key conformance to the K8S specifications

Suspecting CC code not to enforce that the annotation label keys are conformant to the K8S specifications unlike what was done for label keys in 279ea4a

Expected result

CC enforces that label keys are conformant to the K8S specifications, e.g. reject prefix that are not formatted like a DNS domain.

Also it might make sense to reserve the cloudfoundry.org prefix in annotations also.

Annotations might be useful in the future for CF control plane for things like recording verbose audit logs (which would not be wise to store as indexed labels for performance reasons)

/CC @mattmcneeney

[cf-gitbot]

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/165640281

The labels on this github issue will be updated when the story is started.

[williammartin]

@tcdowney @ssisil Although the annotations in question here are services related, the overarching request seems to be broader than just services as annotation conventions should be consistent across all CC endpoints. What do you think about this issue?

[tcdowney]

Hi @gberche-orange, regarding the following:

Annotations might be useful in the future for CF control plane for things like recording verbose audit logs (which would not be wise to store as indexed labels for performance reasons)

Metadata in Cloud Controller was never intended to be used directly by the control plane, hence this part of the docs:

Metadata allows you to tag API Resources with information that does not directly affect its functionality.

Given the above (I feel like it could be worded clearer, though), I feel like actually using the existing metadata for this OSBAPI interaction violates that goal. I wonder if some services-specific construct should be used instead. Tagging @zrob since he sent out the original proposal and has some more context on the product goals of metadata from that time.

Also, to answer your specific question about why Annotation prefixes are different from Labels I'm not actually sure why that decision was made. To be honest, Annotations in Cloud Foundry don't support prefixes at all. It's all just one giant name field for them. Changing that might be difficult since there are annotations out in the wild at this point that might rely on this behavior.

[gberche-orange]

@tcdowney

Metadata in Cloud Controller was never intended to be used directly by the control plane, hence this part of the docs:

Metadata allows you to tag API Resources with information that does not directly affect its functionality.

Given the above (I feel like it could be worded clearer, though), I feel like actually using the existing metadata for this OSBAPI interaction violates that goal.

Can you please be more specific about the goal violation ?

Design specs for example mention precisely the use-case that openservicebrokerapi/servicebroker#658 aims to address: Publishing user-provided information attached to resources so that 3rd party service brokers can alert on failures.

Users want to annotate applications with contact information that can be used if the application experiences problems.

Changing that might be difficult since there are annotations out in the wild at this point that might rely on this behavior.

Given the metadata feature was only recently made available in CC API, and I understand CLI support is not yet available (see related cli epic, usage in the field is likely to be limited, and this might be a good time to consider such CFAR/CFCR consistency fixes that CF is aiming at.

The cases where a breaking change would occur would be very rare: i.e. annotations that indeed have a prefix (i.e. contain a '/') and this prefix is not a FQDN.

[tcdowney]

Given the metadata feature was only recently made available in CC API, and I understand CLI support is not yet available (see related cli epic, usage in the field is likely to be limited, and this might be a good time to consider such CFAR/CFCR consistency fixes that CF is aiming at.

Yeah this is true, if we are going to make the field more restrictive the sooner the better.

Tagging our PM @ssisil again for awareness of this request.

[gberche-orange]

@ssisil @tcdowney were you able to review and consider this issue ?

[ssisil]

@gberche-orange - Yes - this is something we will prioritizing. You should see something for this in one of the upcoming releases.

[tcdowney]

@gberche-orange

This change shipped in capi-release 1.83.0. You restrictions on prefix/name should be what you expect, but you can read more about them in our docs:

• https://v3-apidocs.cloudfoundry.org/version/3.74.0/index.html#annotations

We also documented our "migration" strategy for existing data in this ADR:
https://github.com/cloudfoundry/cloud_controller_ng/blob/d3d80eccb1ea39f6f6872a84791ff2da0ea283a0/decisions/0004-adding-key-prefix-to-annotations.md

Most notably, we won't be attempting to migrate existing annotations that don't conform to these stricter requirements and they will continue to be readable. When a user tries to update an annotation that's non-conformant then they will get a validation error and have to make changes.

[gberche-orange]

great, thanks a lot @tcdowney !

I've submitted cloudfoundry/docs-cf-admin#161 to also cover annotations prefixes in doc.