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

Redefined Gateway API Types #115

Open
danehans opened this issue Dec 19, 2024 · 7 comments
Open

Redefined Gateway API Types #115

danehans opened this issue Dec 19, 2024 · 7 comments

Comments

@danehans
Copy link
Contributor

danehans commented Dec 19, 2024

Currently, gateway-api-inference-extension defines API types that exist in Gateway API. For example:

// PoolObjectReference identifies an API object within the namespace of the
// referrer.
type PoolObjectReference struct {
	Group string `json:"group,omitempty"`
	Kind  string `json:"kind,omitempty"`
	Name  string `json:"name,omitempty"`
}

Gateway API defines custom alias types for Group, Kind, and Name. For consistency, should these types be imported and used instead of redefining them as opaque strings? For example:

import gwv1 "sigs.k8s.io/gateway-api/apis/v1"

// PoolObjectReference identifies an API object within the namespace of the
// referrer.
type PoolObjectReference struct {
	Group gwv1.Group `json:"group,omitempty"`
	Kind  gwv1.Kind `json:"kind,omitempty"`
	Name  gwv1.ObjectName `json:"name,omitempty"`
}
@danehans
Copy link
Contributor Author

cc: @robscott

@robscott
Copy link
Member

Good catch, thanks @danehans! The goal here was to add some defaults here so 99% of users would only need to specify a name, similar to how BackendRef works in Gateway API. Those have been added now, so hopefully it makes a bit more sense:

// PoolObjectReference identifies an API object within the namespace of the
// referrer.
type PoolObjectReference struct {
// Group is the group of the referent.
//
// +optional
// +kubebuilder:default="inference.networking.x-k8s.io"
// +kubebuilder:validation:MaxLength=253
// +kubebuilder:validation:Pattern=`^$|^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$`
Group string `json:"group,omitempty"`
// Kind is kind of the referent. For example "InferencePool".
//
// +optional
// +kubebuilder:default="InferencePool"
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=63
// +kubebuilder:validation:Pattern=`^[a-zA-Z]([-a-zA-Z0-9]*[a-zA-Z0-9])?$`
Kind string `json:"kind,omitempty"`

@danehans
Copy link
Contributor Author

danehans commented Jan 1, 2025

@robscott thanks for the feedback. Even with defaults and validation added to the API fields, I'm trying to understand why concrete types are used here. In contrast, alias types are used in Gateway API object reference types. For example Group string here vs Group *Group in Gateway API BackendObjectReference.

@robscott
Copy link
Member

robscott commented Jan 2, 2025

@danehans Got it, somehow I completely missed that part of the difference. Agree that it would make sense to reuse gwv1.Group, Kind, and ObjectName types, thanks for catching this!

@ahg-g
Copy link
Contributor

ahg-g commented Jan 2, 2025

My worry is about creating an unnecessary dependency on the gateway api pkg types. The group/kind of resources are not a gateway concept, they are a k8s one, and there they are just defined as strings: https://github.com/kubernetes/kubernetes/blob/6746df77f2376c6bc1fd0de767d2a94e6bd6cec1/pkg/apis/core/types.go#L5750.

@danehans
Copy link
Contributor Author

danehans commented Jan 3, 2025

I think it's fair to not not create Gateway API dependencies. Maybe this should be documented somewhere to help guide API development for the project. Thoughts?

@robscott
Copy link
Member

robscott commented Jan 3, 2025

Yeah, that's a good idea, I'll try to add that into the developer guide in #129.

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

No branches or pull requests

3 participants