Skip to content

API conventions: Discourage use of ObjectReference #4705

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

API conventions: Discourage use of ObjectReference #4705

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 33 additions & 8 deletions contributors/devel/sig-architecture/api-conventions.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -427,11 +427,36 @@ ensure that GETs of individual objects remain bounded in time and space, these
sets may be queried via separate API queries, but will not be expanded in the
referring object's status.

References to specific objects, especially specific resource versions and/or
specific fields of those objects, are specified using the `ObjectReference` type
(or other types representing strict subsets of it). Unlike partial URLs, the
ObjectReference type facilitates flexible defaulting of fields from the
referring object or other contextual information.
References to specific objects are specified using object reference types that
uniquely identify their referents. Object reference types may be defined along
with the referring object types. An object reference type must have a required
`name` field. In addition, the type may have a `namespace` field to accommodate
references to non-local objects (that is, namespaced referents that are not in
the same namespace as the referring object). If the reference needs to
accommodate referents of different resource types, the object reference type may
also have `group` and `resource` fields. For example:

```go
// FooReference holds a reference to an API object that can be used as a foo.
type FooReference struct {
// group is the group of the referent. The empty string represents
// the core API group.
Group string
// resource is the resource of the referent.
Resource string
// namespace is the namespace of the referent.
Namespace string
// name is the name of the referent.
Name string
}
```

Unlike partial URLs, object reference types facilitate flexible defaulting of
fields from the referring object or other contextual information.

Some existing APIs use the generic `ObjectReference`, `LocalObjectReference`,
Copy link
Member

Choose a reason for hiding this comment

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

ObjectReference has lots of rarely-used, poorly defined fields, so I agree it is not great to recommend. Are LocalObjectReference and TypedLocalObjectReference not recommended? LocalObjectReference seems reasonable to use

Copy link
Member

@robscott robscott Sep 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@liggitt We're having trouble understanding the current recommendations here. The guidance added to ObjectReference by @deads2k suggests the following:

Instead of using this type, create a locally provided and used type that is well-focused on your reference.

At the same time, it sounds like you're suggesting that TypedLocalObjectReference might still be worth using here?

This is in the context of a new set of Service APIs that use lots of object references. So far we've relied on the guidance from that comment on ObjectReference and created custom object reference types for all the things. Would it be preferable to reuse the core type?

We're also having trouble understanding the suggestion added in that same PR to use Resource instead of Kind in object references. That seems like it would be especially confusing for users that are used to Kind in object refs.

Right now we're in a confusing state where the godocs seem to disagree with the API conventions and it's hard to know which to follow.

Copy link
Member

Choose a reason for hiding this comment

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

So far we've relied on the guidance from that comment on ObjectReference and created custom object reference types for all the things. Would it be preferable to reuse the core type?

A scoped type with exactly the fields you need is a good idea. LocalObjectReference is so simplistic as to make reuse almost more trouble than it's worth. TypedLocalObjectReference is more constrained than ObjectReference, but reusing the shared struct means you can't put any context-specific documentation/descriptions on the generated API doc.

We're also having trouble understanding the suggestion added in that same PR to use Resource instead of Kind in object references. That seems like it would be especially confusing for users that are used to Kind in object refs.

Typically, references are used to look up other objects via API calls. To make those API calls, you always need to resolve to a resource (which shows up in the API request URL path). You can use discovery to resolve a Kind to a resource, but since multiple resources could accept the same Kind, you would have to have a convention of taking the first resource associated with the Kind. Using the resource directly in the reference is less ambiguous.

Copy link

@hiddeco hiddeco Oct 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

You can use discovery to resolve a Kind to a resource, but since multiple resources could accept the same Kind, you would have to have a convention of taking the first resource associated with the Kind. Using the resource directly in the reference is less ambiguous.

Isn't this solved by the introduction of the Group? The convention I am used to as a controller developer is that a Resource translates to <kind>/<name>, but that does not seem to apply here as there also is a Name field in the example structure. Given this, I think convention like the following would be more intuitive to users and developers:

// FooReference holds a reference to an API object that can be used as a foo.
type FooReference struct {
   // Group is the group of the referent. The empty string represents
   // the core API group.
   Group string
   // Kind is the kind of of the referent, and must exist in the referenced Group.
   Kind string
   // Namespace is the namespace of the referent.
   Namespace string
   // Name is the name of the referent.
   Name string
}

This would also match what is currently defined in CustomResourceDefinition resources:

apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
  name: secrets.secret.example.com
spec:
  group: secret.example.com
  names:
    kind: Secret
    listKind: SecretList
    plural: secrets
    singular: secret

which would translate to:

&FooReference{
	Group:     "secret.example.com",
	Kind:      "Secret",
	Namespace: "default",
	Name:      "foo",
}

while not causing conflicts with the core Secret resource kind.

and `TypedLocalObjectReference` types. Use of these types is discouraged when
defining new APIs.

References in the status of the referee to the referrer may be permitted, when
the references are one-to-one and do not need to be frequently updated,
Expand Down Expand Up @@ -803,8 +828,8 @@ Examples:

Object references should either be called `fooName` if referring to an object of
kind `Foo` by just the name (within the current namespace, if a namespaced
resource), or should be called `fooRef`, and should contain a subset of the
fields of the `ObjectReference` type.
resource), or should be called `fooRef`, and should contain the fields of the
corresponding object reference type (see [References to related objects](#references-to-related-objects)).


TODO: Plugins, extensions, nested kinds, headers
Expand Down Expand Up @@ -1173,7 +1198,7 @@ be all lowercase, such as "httpGet". Where used as a constant, all letters
should be uppercase, such as "TCP" or "UDP".
* The name of a field referring to another resource of kind `Foo` by name should
be called `fooName`. The name of a field referring to another resource of kind
`Foo` by ObjectReference (or subset thereof) should be called `fooRef`.
`Foo` using an object reference type should be called `fooRef`.
* More generally, include the units and/or type in the field name if they could
be ambiguous and they are not specified by the value or value type.
* The name of a field expressing a boolean property called 'fooable' should be
Expand Down