-
Notifications
You must be signed in to change notification settings - Fork 688
conformance: Move Inference Extension conformance helper functions to gateway-api/conformance #4602
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
663bcb3
f1ce5bb
aa028b2
6644614
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,8 @@ import ( | |
| v1 "k8s.io/api/core/v1" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/apimachinery/pkg/runtime/schema" | ||
| "k8s.io/apimachinery/pkg/runtime" | ||
|
|
||
| "k8s.io/apimachinery/pkg/types" | ||
| "k8s.io/apimachinery/pkg/util/wait" | ||
| "k8s.io/utils/ptr" | ||
|
|
@@ -984,6 +986,26 @@ func HTTPRouteMustHaveResolvedRefsConditionsTrue(t *testing.T, client client.Cli | |
| }) | ||
| } | ||
|
|
||
| // HTTPRouteMustHaveResolvedRefsMustHaveBackendsNotFound checks that the supplied HTTPRoute has the resolvedRefsCondition | ||
| // set to false due to RouteReasonBackendNotFound. | ||
| func HTTPRouteMustHaveResolvedRefsMustHaveBackendsNotFound(t *testing.T, client client.Client, timeoutConfig config.TimeoutConfig, routeNN types.NamespacedName, gwNN types.NamespacedName) { | ||
| HTTPRouteMustHaveCondition(t, client, timeoutConfig, routeNN, gwNN, metav1.Condition{ | ||
| Type: string(gatewayv1.RouteConditionResolvedRefs), | ||
| Status: metav1.ConditionFalse, | ||
| Reason: string(gatewayv1.RouteReasonBackendNotFound), | ||
| }) | ||
| } | ||
|
|
||
| // HTTPRouteMustHaveRouteAcceptedConditionsTrue checks that the supplied HTTPRoute has RouteConditionAccepted | ||
| // set to true. | ||
| func HTTPRouteMustHaveRouteAcceptedConditionsTrue(t *testing.T, client client.Client, timeoutConfig config.TimeoutConfig, routeNN types.NamespacedName, gwNN types.NamespacedName) { | ||
| HTTPRouteMustHaveCondition(t, client, timeoutConfig, routeNN, gwNN, metav1.Condition{ | ||
| Type: string(gatewayv1.RouteConditionAccepted), | ||
| Status: metav1.ConditionTrue, | ||
| Reason: string(gatewayv1.RouteReasonAccepted), | ||
| }) | ||
| } | ||
|
|
||
| func parentRefToString(p gatewayv1.ParentReference) string { | ||
| if p.Namespace != nil && *p.Namespace != "" { | ||
| return fmt.Sprintf("%v/%v", p.Namespace, p.Name) | ||
|
|
@@ -1467,3 +1489,73 @@ func GetConfigMapData(client client.Client, timeoutConfig config.TimeoutConfig, | |
|
|
||
| return configMap.Data, nil | ||
| } | ||
|
|
||
| // HTTPRouteMustBeAcceptedAndResolved waits for the specified HTTPRoute | ||
| // to be Accepted and have its references resolved by the specified Gateway. | ||
| // It uses the upstream Gateway API's HTTPRouteMustHaveCondition helper. | ||
| // NOTE: Used in Gateway API Inference Extension conformance. | ||
| func HTTPRouteMustBeAcceptedAndResolved(t *testing.T, c client.Client, timeoutConfig config.TimeoutConfig, routeNN, gatewayNN types.NamespacedName) { | ||
| t.Helper() | ||
|
|
||
| acceptedCondition := metav1.Condition{ | ||
| Type: string(gatewayv1.RouteConditionAccepted), | ||
| Status: metav1.ConditionTrue, | ||
| Reason: string(gatewayv1.RouteReasonAccepted), | ||
| } | ||
|
|
||
| resolvedRefsCondition := metav1.Condition{ | ||
| Type: string(gatewayv1.RouteConditionResolvedRefs), | ||
| Status: metav1.ConditionTrue, | ||
| Reason: string(gatewayv1.RouteReasonResolvedRefs), | ||
| } | ||
|
|
||
| t.Logf("Waiting for HTTPRoute %s to be Accepted by Gateway %s", routeNN.String(), gatewayNN.String()) | ||
| HTTPRouteMustHaveCondition(t, c, timeoutConfig, routeNN, gatewayNN, acceptedCondition) | ||
|
|
||
| t.Logf("Waiting for HTTPRoute %s to have ResolvedRefs by Gateway %s", routeNN.String(), gatewayNN.String()) | ||
| HTTPRouteMustHaveCondition(t, c, timeoutConfig, routeNN, gatewayNN, resolvedRefsCondition) | ||
|
|
||
| t.Logf("HTTPRoute %s is now Accepted and has ResolvedRefs by Gateway %s", routeNN.String(), gatewayNN.String()) | ||
| } | ||
|
|
||
| // GetAcceptedByParentGatewayCondition retrieves the 'Accepted' condition with | ||
| // status True and reason 'Accepted' that would be returned in from an input Gateway. | ||
| // NOTE: Used in Gateway API Inference Extension conformance. | ||
| func GetAcceptedByParentGatewayCondition() metav1.Condition { | ||
| return metav1.Condition{ | ||
ericdbishop marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| Type: string(gatewayv1.GatewayConditionAccepted), | ||
| Status: metav1.ConditionTrue, | ||
| Reason: string(gatewayv1.GatewayReasonAccepted), // The standard "Accepted" reason | ||
| } | ||
| } | ||
|
|
||
| // NOTE: Used in Gateway API Inference Extension conformance. | ||
| func GetGatewayAcceptedCondition() metav1.Condition { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ericdbishop Could you clarify why this function needs to be part of this repository if it’s only used for Inference Extension conformance? I don’t quite see how this helps eliminate the circular dependencies.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So my understanding was that any helper functions to check Gateway API CRD statuses/usage of the gatewayv1 apis in gateway-api-inference-extension could be moved here for general use, and this is to make sure that any changes in gateway-api wouldn't break gateway-api-inference-extension/conformance. Maybe @rikatz could also help clarify what a good end state here would be, as we discussed this in the past few weeks.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also I realize this PR lacks testing, I will work on that if we agree these changes are useful.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cc @zetxqx if you have additional thoughts.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @snorwin mostly we are trying to avoid GIE importing Gateway API core just for its own assertions, so during a GIE meeting we have discussed about bringing these helper functions to the repository. Eventually this will change, but for now what we want here is that GIE conformance has no dependency on Gateway API, just on Gateway API conformance
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@rikatz what I don’t quite understand is, what’s the practical difference between having an indirect dependency on the Gateway API (which you already have anyway, since it’s a dependency of Gateway API conformance) versus having a direct dependency on it? To be honest, functions like this don’t seem to provide much value: Especially if they were introduced just to avoid importing
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @snorwin basically depending on Gateway API core broke their release workflow, and we agreed that their conformance should depend as less as possible on Gateway API internals (they need a statement like "provision a gateway and wait to be ready") There was an agreement during the GIE meeting and we that it was a low effort approach to get some helper functions on our side that would reduce their dependency, and in fact this is a very low effort :) I am assuming we will come back to the conformance machinery this quarter to do some proper cleanups, and we may revisit all of this, but for now this is not really a change that will break anything on our side
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rikatz sounds good to me. I was just trying to understand the issue.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd be happy to consolidate some of these helper functions as a follow up, I have other conformance tasks I'm tracking here: #4478. |
||
| return metav1.Condition{ | ||
| Type: string(gatewayv1.GatewayConditionAccepted), | ||
| Status: metav1.ConditionTrue, | ||
| } | ||
| } | ||
|
|
||
| // NOTE: Used in Gateway API Inference Extension conformance. | ||
| func GetGatewayProgrammedCondition() metav1.Condition { | ||
| return metav1.Condition{ | ||
| Type: string(gatewayv1.GatewayConditionProgrammed), | ||
| Status: metav1.ConditionTrue, | ||
| } | ||
| } | ||
|
|
||
| // NOTE: Used in Gateway API Inference Extension conformance. | ||
| func DeleteHTTPRoute(t *testing.T, c client.Client, routeNN types.NamespacedName) { | ||
| httpRoute := &gatewayv1.HTTPRoute{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: routeNN.Name, Namespace: routeNN.Namespace}, | ||
| } | ||
| t.Logf("Deleting HTTPRoute %s", routeNN.String()) | ||
| require.NoError(t, c.Delete(context.TODO(), httpRoute), "failed to delete httproute %s", routeNN.String()) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what if this is not found? should it still error?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see what you're saying, but I'm tempted to leave this as-is, since it's the behavior they had in gateway-api-inference-extension and the only reason I could see it being 'not found' is if the user running the tests deletes the HTTPRoute somehow? |
||
| } | ||
|
|
||
| // Install gatewayv1 types into scheme. | ||
| // NOTE: Used in Gateway API Inference Extension conformance. | ||
| func InstallGatewayV1(scheme *runtime.Scheme) error { | ||
| return gatewayv1.Install(scheme) | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.