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

Normalize object references #2158

Closed
nicolaferraro opened this issue Mar 23, 2021 · 10 comments · Fixed by #2169
Closed

Normalize object references #2158

nicolaferraro opened this issue Mar 23, 2021 · 10 comments · Fixed by #2169
Assignees
Milestone

Comments

@nicolaferraro
Copy link
Member

Also in the context of #2083, we should normalize the way we reference generic Kubernetes resources.

I cannot find a standard for that. I see in the service-binding trait we're using a convention like:

PostgreSQL.v1alpha1.postgres.org/name[/ns1]

Which is not natural... A better way would be to use the Knative CLI convention using colon separators:

postgres.org:v1alpha1:PostgreSQL[:ns1]:name

Which also can simplify to:

PostgreSQL[:ns1]:name # if the kind is known

We can also allow both : and / as separators, to account for something like:

postgres.org/v1alpha1:PostgreSQL:ns1/name

Like the kn CLI, we can also use lowercase prefixes for special types, e.g.

channel:ch1
ksvc:myservice
broker:default
@nicolaferraro nicolaferraro added this to the 1.4.0 milestone Mar 23, 2021
@nicolaferraro
Copy link
Member Author

cc: @johnpoth , @lburgazzoli, @astefanutti

@lburgazzoli
Copy link
Contributor

+1

@astefanutti
Copy link
Member

I like the [[apigroup/]version:]kind:[namespace/]name form, which is also compatible with the lower-cased type shorthands.

I would avoid the apigroup:version and namespace:name forms, which do not appear standard in the Kubernetes eco-system.

@nicolaferraro
Copy link
Member Author

Yes, I think that if we want to make more than 1 field optional, we cannot treat "/" and ":" as interchangeable.. Hoping that there are no other ambiguities..

@astefanutti
Copy link
Member

Right, I'd rather not make / and : interchangeable, both for avoiding possible ambiguities, and sticking to conventions.

@johnpoth
Copy link
Member

Right I remember searching for a "standard" thinking this would be easy to find but came up empty handed ...

If we look at how the API server exposes the resource URLs, in order it goes apigroup then version then namespace then kind then name. Not sure how I feel about putting the namespace before kind...

I think I would prefer not having two different separators but it seems that way in most cases I see.

If we look at kubectl get, which I think I ended copying, its something like KIND[.VERSION][.GROUP]/NAME

At this stage I feel like I would +1 anything :)

@heiko-braun
Copy link

@astefanutti
Copy link
Member

Another case against / and : interchangeability, is that it increases complexity of equality comparison. We used to support it for dependency references, but that led to duplicated entries issues. Sooner or later, that increased complexity causes issues, as it's been forgotten to normalise before comparing or hashing. So I went ahead and removed it for dependencies (#1903).

Generally, : carries the semantic of "logical unit" separator, while / carries path-like or scoped-like semantic in tree-like structures.

For the namespace, it seems there is a consensus it's a path / scope, applied to the resource location "unit", hence, if we follow the above generality, should be followed by /.

For the type, it is less obvious to find a consensus in all the places that re-surface its serialised forms. As @johnpoth pointed out, you have kubectl get integrations.camel.apache.org/name. If we consider the API server REST contract as the source of truth, that command translates to the following request:

GET /apis/camel.apache.org/v1/namespaces/camel-k/integrations/name

Which is a serialised form of object reference! So maybe [group[/version]/][namespace/]kind/name is a solution :)

@nicolaferraro
Copy link
Member Author

nicolaferraro commented Mar 24, 2021

The problem is ambiguity with optional fields:

If the pattern is [group[/version]/][namespace/]kind/name, then in:

apps/deployment/x

Is apps the group or the namespace?

With: [[apigroup/]version:]kind:[namespace/]name you should not have that problem.

@nicolaferraro nicolaferraro self-assigned this Mar 24, 2021
@astefanutti
Copy link
Member

Right, that's possible the namespaces segment is there for that reason, so that it solves ambiguity between namespace-scoped and cluster-scoped resources. So that would lead to [group[/version]/][namespaces/ns/]kind/name.

A couple of examples to see how it compares to [[apigroup/]version:]kind:[namespace/]name:

messaging.knative.dev/v1:channel:acme/invoices
messaging.knative.dev/v1/namespaces/acme/channels/invoices
broker:acme/messages
namespaces/acme/brokers/messages
service:splitter
services/splitter

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 a pull request may close this issue.

5 participants