Resolve robscott feedback

Signed-off-by: Daneyon Hansen <[email protected]>

Author: danehans

.golangci-kal.yml

@@ -32,6 +32,8 @@ linters:
                 policy: SuggestFix # SuggestFix | Warn # The policy for pointers in optional fields. Defaults to `SuggestFix`. 
               omitempty:
                 policy: SuggestFix # SuggestFix | Warn | Ignore # The policy for omitempty in optional fields. Defaults to `SuggestFix`.
+              omitzero:
+                policy: Forbid # Enforce the `omitempty` route with a pointer for all optional structs.
   exclusions:
     generated: strict
     paths:

api/v1/inferencepool_types.go

@@ -43,11 +43,10 @@ type InferencePool struct {
 	// Status defines the observed state of the InferencePool.
 	//
 	// +optional
-	//nolint:kubeapilinter // ignore kubeapilinter to follow K8s conventions of optional but non-pointer.
 	Status InferencePoolStatus `json:"status,omitempty"`
 }
 
-// InferencePoolList contains a list InferencePools.
+// InferencePoolList contains a list of InferencePools.
 //
 // +kubebuilder:object:root=true
 type InferencePoolList struct {
@@ -67,8 +66,7 @@ type InferencePoolSpec struct {
 	// this configuration into a Service resource.
 	//
 	// +required
-	//nolint:kubeapilinter // ignore kubeapilinter here as we don't want to use pointer here.
-	Selector LabelSelector `json:"selector"`
+	Selector LabelSelector `json:"selector,omitzero"`
 
 	// TargetPorts defines a list of ports that are exposed by this InferencePool.
 	// Currently, the list may only include a single port definition.
@@ -77,8 +75,7 @@ type InferencePoolSpec struct {
 	// +kubebuilder:validation:MaxItems=1
 	// +listType=atomic
 	// +required
-	//nolint:kubeapilinter // ignore kubeapilinter here since the field is required and we don't want omitempty tag.
-	TargetPorts []Port `json:"targetPorts"`
+	TargetPorts []Port `json:"targetPorts,omitzero"`
 
 	// EndpointPickerRef is a reference to the Endpoint Picker extension and its
 	// associated configuration.
@@ -93,8 +90,7 @@ type Port struct {
 	// The number must be in the range 1 to 65535.
 	//
 	// +required
-	//nolint:kubeapilinter // ignore kubeapilinter here since the field is required and we don't want omitempty tag.
-	Number PortNumber `json:"number"`
+	Number PortNumber `json:"number,omitzero"`
 }
 
 // EndpointPickerRef specifies a reference to an Endpoint Picker extension and its
@@ -126,23 +122,20 @@ type EndpointPickerRef struct {
 	// Name is the name of the referent API object.
 	//
 	// +required
-	//nolint:kubeapilinter // ignore kubeapilinter here as we want to use pointer since empty means default value.
-	Name ObjectName `json:"name"`
+	Name ObjectName `json:"name,omitzero"`
 
 	// PortNumber is the port number of the Endpoint Picker extension service. When unspecified,
 	// implementations SHOULD infer a default value of 9002 when the kind field is "Service" or
 	// unspecified (defaults to "Service").
 	//
 	// +optional
-	//nolint:kubeapilinter // ignore kubeapilinter here as we want to use pointer here as 0 usually means all ports.
 	PortNumber *PortNumber `json:"portNumber,omitempty"`
 
 	// FailureMode configures how the parent handles the case when the Endpoint Picker extension
 	// is non-responsive. When unspecified, defaults to "FailClose".
 	//
 	// +optional
 	// +kubebuilder:default="FailClose"
-	//nolint:kubeapilinter // ignore kubeapilinter here as we want to use pointer since empty means default value.
 	FailureMode *EndpointPickerFailureMode `json:"failureMode,omitempty"`
 }
 
@@ -185,18 +178,18 @@ type ParentStatus struct {
 	// state of the InferencePool. This field is required to be set by the controller that
 	// manages the InferencePool.
 	//
-	// Known condition types are:
+	// Supported condition types are:
 	//
 	// * "Accepted"
 	// * "ResolvedRefs"
 	//
-	// +required
+	// +optional
 	// +listType=map
 	// +listMapKey=type
-	// +kubebuilder:validation:MinItems=1
+	// +patchStrategy=merge
+	// +patchMergeKey=type
 	// +kubebuilder:validation:MaxItems=8
-	//nolint:kubeapilinter // ignore kubeapilinter here as we want conditions to be required.
-	Conditions []metav1.Condition `json:"conditions"`
+	Conditions []metav1.Condition `json:"conditions,omitempty"`
 
 	// ParentRef is used to identify the parent resource that this status
 	// is associated with. It is used to match the InferencePool with the parent
@@ -222,7 +215,7 @@ const (
 	//
 	// Possible reasons for this condition to be False are:
 	//
-	// * "NotSupportedByParent"
+	// * "Accepted"
 	// * "HTTPRouteNotAccepted"
 	//
 	// Possible reasons for this condition to be Unknown are:
@@ -236,22 +229,18 @@ const (
 	// InferencePoolReasonAccepted is a reason used with the "Accepted" condition
 	// when the InferencePool is accepted by a Parent because the Parent supports
 	// InferencePool as a backend.
-	InferencePoolReasonAccepted InferencePoolReason = "SupportedByParent"
+	InferencePoolReasonAccepted InferencePoolReason = "Accepted"
 
 	// InferencePoolReasonNotSupportedByParent is a reason used with the "Accepted"
 	// condition when the InferencePool has not been accepted by a Parent because
 	// the Parent does not support InferencePool as a backend.
 	InferencePoolReasonNotSupportedByParent InferencePoolReason = "NotSupportedByParent"
 
-	// InferencePoolReasonHTTPRouteNotAccepted is a reason used with the "Accepted"
-	// condition when the InferencePool is referenced by an HTTPRoute that has been
-	// rejected by the Parent. The user should inspect the status of the referring
-	// HTTPRoute for the specific reason.
+	// InferencePoolReasonHTTPRouteNotAccepted is an optional reason used with the
+	// "Accepted" condition when the InferencePool is referenced by an HTTPRoute that
+	// has been rejected by the Parent. The user should inspect the status of the
+	// referring HTTPRoute for the specific reason.
 	InferencePoolReasonHTTPRouteNotAccepted InferencePoolReason = "HTTPRouteNotAccepted"
-
-	// This reason is used with the "Accepted" when a controller has not yet
-	// reconciled the InferencePool.
-	InferencePoolReasonPending InferencePoolReason = "Pending"
 )
 
 const (
@@ -295,19 +284,21 @@ type ParentReference struct {
 	//
 	// +optional
 	// +kubebuilder:default=Gateway
-	//nolint:kubeapilinter // ignore kubeapilinter here as we want to use pointer here as empty means default value.
 	Kind *Kind `json:"kind,omitempty"`
 
 	// Name is the name of the referent API object.
 	//
 	// +required
-	Name ObjectName `json:"name,omitempty"`
+	Name ObjectName `json:"name,omitzero"`
 
-	// Namespace is the namespace of the referent API object. When unspecified,
-	// the namespace of the referent is assumed to be the same as the namespace
-	// of the referring object.
+	// Namespace is the namespace of the referenced object. When unspecified, the local
+	// namespace is inferred.
+	//
+	// Note that when a namespace different than the local namespace is specified,
+	// a ReferenceGrant object is required in the referent namespace to allow that
+	// namespace's owner to accept the reference. See the ReferenceGrant
+	// documentation for details: https://gateway-api.sigs.k8s.io/api-types/referencegrant/
 	//
 	// +optional
-	//nolint:kubeapilinter // ignore kubeapilinter here as we want to use pointer here as empty means same namespace.
 	Namespace *Namespace `json:"namespace,omitempty"`
 }

api/v1/shared_types.go

@@ -139,5 +139,5 @@ type LabelSelector struct {
 	// +required
 	// +kubebuilder:validation:MinItems=1
 	// +kubebuilder:validation:MaxItems=64
-	MatchLabels map[LabelKey]LabelValue `json:"matchLabels,omitempty"`
+	MatchLabels map[LabelKey]LabelValue `json:"matchLabels,omitzero"`
 }

apix/v1alpha2/inferencepool_conversion.go

@@ -18,12 +18,8 @@ package v1alpha2
 
 import (
 	"errors"
-	"fmt"
-	"time"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-	runtime "k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/utils/ptr"
 
 	v1 "sigs.k8s.io/gateway-api-inference-extension/api/v1"
@@ -43,9 +39,6 @@ func (src *InferencePool) ConvertTo(dst *v1.InferencePool) error {
 		return err
 	}
 
-	// v1 requires at least one condition per parent.
-	ensureV1StatusConditionsMinimum(v1Status)
-
 	meta := metav1.TypeMeta{
 		Kind:       src.Kind,
 		APIVersion: v1.GroupVersion.String(), // Ensure the API version is set correctly.
@@ -102,31 +95,24 @@ func convertStatusToV1(src *InferencePoolStatus) (*v1.InferencePoolStatus, error
 	if src == nil {
 		return nil, errors.New("src cannot be nil")
 	}
-	u, err := toUnstructured(src)
-	if err != nil {
-		return nil, err
-	}
-
-	// v1alpha2 used json:"parent" (singular) v1 uses json:"parents"
-	if v, ok := u.Object["parent"]; ok {
-		u.Object["parents"] = v
-		delete(u.Object, "parent")
-	}
-
-	out, err := convert[v1.InferencePoolStatus](u)
-	if err != nil {
-		return nil, err
+	out := &v1.InferencePoolStatus{
+		Parents: make([]v1.ParentStatus, 0, len(src.Parents)),
 	}
-
-	// Map condition reason string: "Accepted" (v1alpha2) -> "SupportedByParent" (v1)
-	for i := range out.Parents {
-		for j := range out.Parents[i].Conditions {
-			c := &out.Parents[i].Conditions[j]
-			if c.Type == string(v1.InferencePoolConditionAccepted) &&
-				c.Reason == string(InferencePoolReasonAccepted) {
-				c.Reason = string(v1.InferencePoolReasonAccepted)
+	for _, p := range src.Parents {
+		ps := v1.ParentStatus{
+			ParentRef:  toV1ParentRef(p.GatewayRef),
+			Conditions: make([]metav1.Condition, 0, len(p.Conditions)),
+		}
+		for _, c := range p.Conditions {
+			cc := c
+			// v1alpha2: "Accepted" -> v1: "SupportedByParent"
+			if cc.Type == string(v1.InferencePoolConditionAccepted) &&
+				cc.Reason == string(InferencePoolReasonAccepted) {
+				cc.Reason = string(v1.InferencePoolReasonAccepted)
 			}
+			ps.Conditions = append(ps.Conditions, cc)
 		}
+		out.Parents = append(out.Parents, ps)
 	}
 	return out, nil
 }
@@ -135,33 +121,64 @@ func convertStatusFromV1(src *v1.InferencePoolStatus) (*InferencePoolStatus, err
 	if src == nil {
 		return nil, errors.New("src cannot be nil")
 	}
-	u, err := toUnstructured(src)
-	if err != nil {
-		return nil, err
+	out := &InferencePoolStatus{
+		Parents: make([]PoolStatus, 0, len(src.Parents)),
 	}
-
-	// v1 uses json:"parents"; v1alpha2 used json:"parent" (singular)
-	if v, ok := u.Object["parents"]; ok {
-		u.Object["parent"] = v
-		delete(u.Object, "parents")
+	for _, p := range src.Parents {
+		ps := PoolStatus{
+			GatewayRef: fromV1ParentRef(p.ParentRef),
+			Conditions: make([]metav1.Condition, 0, len(p.Conditions)),
+		}
+		for _, c := range p.Conditions {
+			cc := c
+			// v1: "SupportedByParent" -> v1alpha2: "Accepted"
+			if cc.Type == string(v1.InferencePoolConditionAccepted) &&
+				cc.Reason == string(v1.InferencePoolReasonAccepted) {
+				cc.Reason = string(InferencePoolReasonAccepted)
+			}
+			ps.Conditions = append(ps.Conditions, cc)
+		}
+		out.Parents = append(out.Parents, ps)
 	}
+	return out, nil
+}
 
-	out, err := convert[InferencePoolStatus](u)
-	if err != nil {
-		return nil, err
+func toV1ParentRef(in ParentGatewayReference) v1.ParentReference {
+	out := v1.ParentReference{
+		Name: v1.ObjectName(in.Name),
+	}
+	if in.Group != nil {
+		g := v1.Group(*in.Group)
+		out.Group = &g
+	}
+	if in.Kind != nil {
+		k := v1.Kind(*in.Kind)
+		out.Kind = &k
 	}
+	if in.Namespace != nil {
+		ns := v1.Namespace(*in.Namespace)
+		out.Namespace = &ns
+	}
+	return out
+}
 
-	// Map condition reason string: "SupportedByParent" (v1) -> "Accepted" (v1alpha2)
-	for i := range out.Parents {
-		for j := range out.Parents[i].Conditions {
-			c := &out.Parents[i].Conditions[j]
-			if c.Type == string(v1.InferencePoolConditionAccepted) &&
-				c.Reason == string(v1.InferencePoolReasonAccepted) {
-				c.Reason = string(InferencePoolReasonAccepted)
-			}
-		}
+func fromV1ParentRef(in v1.ParentReference) ParentGatewayReference {
+	out := ParentGatewayReference{
+		Name: ObjectName(in.Name),
 	}
-	return out, nil
+	if in.Group != nil {
+		g := Group(*in.Group)
+		out.Group = &g
+	}
+	if in.Kind != nil {
+		k := Kind(*in.Kind)
+		out.Kind = &k
+	}
+	if in.Namespace != nil {
+		ns := Namespace(*in.Namespace)
+		out.Namespace = &ns
+	}
+	return out
 }
 
 func convertExtensionRefToV1(src *Extension) (v1.EndpointPickerRef, error) {
@@ -206,39 +223,3 @@ func convertEndpointPickerRefFromV1(src *v1.EndpointPickerRef) (Extension, error
 	}
 	return extension, nil
 }
-
-func toUnstructured(obj any) (*unstructured.Unstructured, error) {
-	u, err := runtime.DefaultUnstructuredConverter.ToUnstructured(obj)
-	if err != nil {
-		return nil, err
-	}
-	return &unstructured.Unstructured{Object: u}, nil
-}
-
-func convert[T any](u *unstructured.Unstructured) (*T, error) {
-	var res T
-	if err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, &res); err != nil {
-		return nil, fmt.Errorf("error converting unstructured to T: %v", err)
-	}
-	return &res, nil
-}
-
-// Ensure v1's PoolStatus.Conditions has at least one entry.
-func ensureV1StatusConditionsMinimum(s *v1.InferencePoolStatus) {
-	if s == nil {
-		return
-	}
-	epoch := metav1.NewTime(time.Unix(0, 0))
-	for i := range s.Parents {
-		ps := &s.Parents[i]
-		if len(ps.Conditions) == 0 {
-			ps.Conditions = []metav1.Condition{{
-				Type:               string(v1.InferencePoolConditionAccepted),
-				Status:             metav1.ConditionUnknown,
-				Reason:             string(v1.InferencePoolReasonPending),
-				Message:            "Waiting for controller",
-				LastTransitionTime: epoch,
-			}}
-		}
-	}
-}