Replace ObjectReference, TypedLocalObjectReference

[Miciah]

Define well focused reference types and use them instead of ObjectReference and TypedLocalObjectReference, which should be avoided for all the reasons described in the ObjectReference godoc.

• api/v1alpha1/gateway_types.go (GatewaySpec): Replace TypedLocalObjectReference with RouteObjectReference.
  (Listener): Replace TypedLocalObjectReference with ListenerExtensionObjectReference.
  (ListenerTLS): Replace TypedLocalObjectReference with CertificateObjectReference.
  (LocalObjectReference): New type. Identify an API object within a known namespace.
  (CertificateObjectReference): New type alias for LocalObjectReference. Identify a certificate object.
  (ListenerExtensionObjectReference): New type alias for LocalObjectReference. Identify a listener extension object.
  (RouteObjectReference): New type alias for LocalObjectReference. Identify a route object.
• api/v1alpha1/gatewayclass_types.go (GatewayClassSpec): Replace ObjectReference with GatewayClassParametersObjectReference.
  (GatewayClassParametersObjectReference): New type alias for LocalObjectReference. Identify a parameters object for a GatewayClass object.
• api/v1alpha1/httproute_types.go (HTTPRouteHost): Replace TypedLocalObjectReference with RouteHostExtensionObjectReference.
  (HTTPRouteMatch): Replace TypedLocalObjectReference with RouteMatchExtensionObjectReference.
  (RouteMatchExtensionObjectReference): New type alias for LocalObjectReference. Identify a route-match extension object.
  (HTTPRouteFilter): Replace TypedLocalObjectReference with RouteFilterExtensionObjectReference.
  (RouteFilterExtensionObjectReference): New type alias for LocalObjectReference. Identify a route-filter extension object.
  (HTTPHeaderFilter): Replace TypedLocalObjectReference with RouteActionTargetObjectReference and RouteActionExtensionObjectReference.
  (RouteActionTargetObjectReference): New type alias for LocalObjectReference. Identify a target object for a route action.
  (RouteActionExtensionObjectReference): New type alias for LocalObjectReference. Identify a route-action extension object.
  (RouteHostExtensionObjectReference): New type alias for LocalObjectReference. Identify a route-host extension object.
  (HTTPRouteStatus): Replace ObjectReference with GatewayObjectReference.
  (GatewayObjectReference): New type. Identify a Gateway object.
• api/v1alpha1/zz_generated.deepcopy.go:
• config/crd/bases/networking.x.k8s.io_gatewayclasses.yaml:
• config/crd/bases/networking.x.k8s.io_gateways.yaml:
• config/crd/bases/networking.x.k8s.io_httproutes.yaml: Regenerate.

[k8s-ci-robot]

Welcome @Miciah!

It looks like this is your first PR to kubernetes-sigs/service-apis 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/service-apis has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @Miciah. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[Miciah]

Can we assume that the referenced object is a Kubernetes object?

[bowei]

Let's keep the assumption for now, we can discuss in a separate issue/PR

[Miciah]

Should this definition be in gateway_types.go?

[bowei]

Yes, will be used by more than http imagine

[bowei]

Put that in a follow on PR.

[robscott]

Hey @Miciah, thanks for all the work on this! The concept of using entirely new types for each object reference is new to me, though the comments you linked to do make sense. This does seem like it adds a lot of potential overhead though with all the new types. It seems like each one of these new object reference types are identical and parallel the core TypedLocalObjectReference type. The comments you referenced seemed to be covering the core ObjectReference type specifically, though some of the bullet points there would also apply to TypedLocalObjectReference.

Do you know if there are any existing examples of this kind of pattern of defining custom reference types is being used in place of the core types? Is there any more official documentation providing more clarity here? If not, it might be good to first PR some kind of clarification to the contributor guide describing this approach to object references in more detail and getting a wider audience on this.

It looks like @deads2k originally added the warning on the ObjectReference type, is this how we should be implementing object references going forward?

[bowei]

/assign @robscott

[bowei]

/ok-to-test

[bowei]

If the struct is basically the same as the type in core, can we use type aliases to keep the field names consistent (Group vs APIGroup) the same until the time comes for the structs to diverge.
e.g.

// FooReference references blah.
type FooReference = core.TypeLocalObjectReference

[Miciah]

@robscott,

Hey @Miciah, thanks for all the work on this! The concept of using entirely new types for each object reference is new to me, though the comments you linked to do make sense. This does seem like it adds a lot of potential overhead though with all the new types. It seems like each one of these new object reference types are identical and parallel the core TypedLocalObjectReference type. The comments you referenced seemed to be covering the core ObjectReference type specifically, though some of the bullet points there would also apply to TypedLocalObjectReference.

Right, most of the reasons against using ObjectReference apply to TypedLocalObjectReference: using kind instead of resource, overly general godoc, unneeded fields (for example, GatewayObjectReference does not need kind or resource), and lack of flexibility.

By "overhead", do you mean lines of code? I agree it is verbose. The flip side of that argument is that we get much clearer output in generated API references and kubectl explain output. Using custom types affords us the opportunity to present an API without redundant fields or vague documentation.

Do you know if there are any existing examples of this kind of pattern of defining custom reference types is being used in place of the core types? Is there any more official documentation providing more clarity here? If not, it might be good to first PR some kind of clarification to the contributor guide describing this approach to object references in more detail and getting a wider audience on this.

The ObjectReference godoc mentions admission registration as an example: https://github.com/kubernetes/api/blob/release-1.17/admissionregistration/v1/types.go#L532-L551

It looks like @deads2k originally added the warning on the ObjectReference type, is this how we should be implementing object references going forward?

I think so. Migrating old APIs may not be possible, and we will be among the first to adopt the new API.

@bowei,

If the struct is basically the same as the type in core, can we use type aliases to keep the field names consistent (Group vs APIGroup) the same until the time comes for the structs to diverge.
e.g.

// FooReference references blah.
type FooReference = core.TypeLocalObjectReference

I think we should avoid TypedLocalObjectReference entirely, for the reasons I have outlined. I did consider defining a smaller number of re-usable reference types. I'm not sure that would be clearer, and it would sacrifice our ability to provide specific documentation (such as, "The resource is typically a configmap"), but it could be a reasonable compromise as we could still provide context-appropriate godoc on the aliased type ("FooReference idenfities a foo object", possibly with context on how the object will be used). So something like the following:

// LocalObjectReference identifies an object within a known namespace.
type LocalObjectReference struct {
	// Group is the group of the referent.
	//
	// +kubebuilder:validation:Optional
	// +optional
	Group string `json:"group,omitempty"`
	// Resource is the resource of the referent.
	//
	// +kubebuilder:validation:Required
	// +required
	Resource string `json:"resource"`
	// Name is the name of the referent.
	//
	// +kubebuilder:validation:Required
	// +required
	Name string `json:"name"`
}

// CertificateObjectReference identifies a certificate object within a known
// namespace.  Typically, the resource is a secret.  [Insert further verbiage
// specific to certificates here.]
type CertificateObjectReference = LocalObjectReference

Would that be acceptable?

[bowei]

The gist of the type alias is two things:

• make sure our field names are consistent with those in use across Kubernetes. For instance, you have Group where core has APIGroup.
• reduce surface area right now since things are not different. once they start evolving separately, break the relationship at that point (reduce amount of boilerplate changes needed for field renames, comments etc)

I'm ok with decoupling from core, but we should make sure things that are the same are the same.

[deads2k]

The gist of the type alias is two things:

• make sure our field names are consistent with those in use across Kubernetes. For instance, you have Group where core has APIGroup.
• reduce surface area right now since things are not different. once they start evolving separately, break the relationship at that point (reduce amount of boilerplate changes needed for field renames, comments etc)

I'm ok with decoupling from core, but we should make sure things that are the same are the same.

Given a that this is a reference to an API object, having API in the field name is redundant. With 20/20 hindsight on an APIReference type, you wouldn't have APIGroup, APIVersion, APIResource as the fields.

[Miciah]

The gist of the type alias is two things:

• make sure our field names are consistent with those in use across Kubernetes. For instance, you have Group where core has APIGroup.

The argument for using "Group" instead of "APIGroup" is that the referent is necessarily an API object, so "API" is redundant (and the other fields do not have "API" in their names even though they all identify an API object). That said, "APIGroup" is not wrong (in the way that "Kind" is wrong), so per your request, I have changed the field name in the new types to "APIGroup" for consistency with the deprecated type.

• reduce surface area right now since things are not different. once they start evolving separately, break the relationship at that point (reduce amount of boilerplate changes needed for field renames, comments etc)

Some things (such as changing "Kind" to "Resource") we cannot reasonably change once the API is GA, but point taken.

I'm ok with decoupling from core, but we should make sure things that are the same are the same.

All right. I have updated the PR to use type aliases. Because the CRD generation omits godoc on types, it is not useful to document fields in types' godoc, so I have moved some godoc to the reference fields. Here is an example of how this looks, from spec.hosts[*].rules[*].action.forwardTo/spec.default.rules[*].action.forwardTo in HTTPRoute:

                           forwardTo:
                             description: ForwardTo sends requests to the referenced
-                              object.
+                              object.  The resource may be "service" (use the empty
+                              string for the group), or an implementation may support
+                              other resources (for example, resource "myroutetarget"
+                              in group "networking.acme.io").
                             properties:
-                              group:
-                                description: Group is the group of the referent.  For
-                                  example, "networking.acme.io".  The empty string
-                                  represents the core API group.
+                              apiGroup:
+                                description: APIGroup is the group of the referent.  The
+                                  empty string represents the core API group.
                                 type: string
                               name:
                                 description: Name is the name of the referent.
                                 type: string
                               resource:
-                                description: Resource is the resource of the referent.  For
-                                  example, "service" or "myroutetarget".
+                                description: Resource is the resource of the referent.
                                 type: string

Thanks for the feedback!

[Miciah]

Rebased.

[Miciah]

Sorry, I just saw David's comment. I am happy to change "APIGroup" back to "Group" if we can reach a consensus.

[bowei]

I'm ok with Group, as long as it is going to be consistent going forward.

[bowei]

Thanks, this looks great.
Minor comments above.

/lgtm
/approve

[Miciah]

Looks like the deepcopy generation does not like type aliases:

api/v1alpha1/zz_generated.deepcopy.go:157:50: (*LocalObjectReference).DeepCopyInto redeclared in this block
	previous declaration at api/v1alpha1/zz_generated.deepcopy.go:27:6

The definition on line 27 is the following:

// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
func (in *CertificateObjectReference) DeepCopyInto(out *CertificateObjectReference) {
	*out = *in
}

The definition on line 157 is the following:

// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
func (in *GatewayClassParametersObjectReference) DeepCopyInto(out *GatewayClassParametersObjectReference) {
	*out = *in
}

Both types (CertificateObjectReference and GatewayClassParametersObjectReference) are aliases of LocalObjectReference.

[Miciah]

I think I need to add // +k8s:deepcopy-gen=false to each type alias definition.

[Miciah]

Latest push changes "APIGroup" to "Group", adds // +k8s:deepcopy-gen=false to type alias definitions, and fixes some whitespace for gofmt.

[bowei]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bowei, Miciah

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [bowei]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment