search

kubernetes-sigs / gateway-api

Repository for the next iteration of composite service (e.g. Ingress) and load balancing APIs.
https://gateway-api.sigs.k8s.io
Apache License 2.0
1.68k stars 439 forks source link

Conformance Helpers Should be Cleaned Up #1728

Open robscott opened 1 year ago

robscott commented 1 year ago

What would you like to be added: We currently have some duplicative methods that really should be unified before we add conformance tests for more types. For example, the following functions have significant overlap:

  1. GatewayAndHTTPRoutesMustBeAccepted + GatewayAndTLSRoutesMustBeAccepted https://github.com/kubernetes-sigs/gateway-api/blob/bca1f48180ad394c004bb2d4aa4896d5c6d7bae6/conformance/utils/kubernetes/helpers.go#L231 https://github.com/kubernetes-sigs/gateway-api/blob/bca1f48180ad394c004bb2d4aa4896d5c6d7bae6/conformance/utils/kubernetes/helpers.go#L569

  2. HTTPRouteMustHaveParents + TLSRouteMustHaveParents https://github.com/kubernetes-sigs/gateway-api/blob/bca1f48180ad394c004bb2d4aa4896d5c6d7bae6/conformance/utils/kubernetes/helpers.go#L396 https://github.com/kubernetes-sigs/gateway-api/blob/bca1f48180ad394c004bb2d4aa4896d5c6d7bae6/conformance/utils/kubernetes/helpers.go#L426

Why this is needed: Our conformance tests have grown organically over the years. Although each change made sense in isolation, these changes have resulted in a high likelihood that we'll start to have unexpected variations between tests for different route types.

Note: This is going to be complex to resolve, it would likely be good for someone that already has experience writing and/or running conformance tests to work on this. It may also be good to agree on a direction in this issue before investing too much time in writing code for this.

robscott commented 1 year ago

/area conformance /kind cleanup /remove-kind feature

mmamczur commented 1 year ago

I looked at this code a bit yesterday. I'm guessing it would be best if instead GatewayAndHTTPRoutesMustBeAccepted() one could use GatewayAndRoutesMustBeAccepted() and it works for all types of connected routes. So the problem in general is that we have types that have similar structure (at least in the part relevant here), as these all have route.Status.Parents. One options is that these could be read as unstructured (which doesn't look great). Another is to have some adapter per type implementing a function like GetStatus() . The problem is that you need to know what type you need to read upfront https://github.com/kubernetes-sigs/gateway-api/blob/bca1f48180ad394c004bb2d4aa4896d5c6d7bae6/conformance/utils/kubernetes/helpers.go#L404-L405 so you still don't know which adapter to use

mmamczur commented 1 year ago

actually it is not possible to get a route without providing it's kind before. So you do need to know what type it is upfront.

mmamczur commented 1 year ago

one way of doing this without writing code per route type is to use reflection to get RouteStatus, which all routes must have. Seems to work https://github.com/kubernetes-sigs/gateway-api/commit/dfc708c65b07a564c5995262f07d039af58545b6

youngnick commented 1 year ago

Neat. I've seen some similar stuff done before using the Unstructured type, and then deserializing it into a duck-type that only contains the bits you're interested in. This uses the json/yaml Go deserialization property that it drops unknown fields. (I've also been playing around with this for some tooling to read anything with status.Conditions :) )

k8s-triage-robot commented 5 months ago

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

youngnick commented 5 months ago

/remove-lifecycle stale

k8s-triage-robot commented 2 months ago

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot commented 1 month ago

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

youngnick commented 1 month ago

Seems like this would still be useful, so I'll unstale for now.

/remove-lifecycle rotten

@mlavacca @shaneutt for conformance cleanup task tracking.

mlavacca commented 4 weeks ago

Yep, this is definitely still useful 👍