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

Conformance tests cannot run on v1.2.1 #3499

Open
snehachhabria opened this issue Dec 13, 2024 · 10 comments
Open

Conformance tests cannot run on v1.2.1 #3499

snehachhabria opened this issue Dec 13, 2024 · 10 comments
Labels
area/conformance kind/bug Categorizes issue or PR as related to a bug.

Comments

@snehachhabria
Copy link
Contributor

This is caused by #3211 which overrides the passed in Client as mentioned here: #3343 (comment). Due to this change Microsoft Azure's implementation of the conformance tests is broken because we had a custom Client that was passed in while setting up the conformance suite.

What would you like to be added: Ensure that the Client is still configurable for implementors to override if needed

Why this is needed: Unable to upgrade to v1.2.1 because it breaks conformance tests.

cc @howardjohn @BobyMCbobs

@robscott
Copy link
Member

Thanks for reporting this @snehachhabria! To clarify, is this a regression between v1.2.0 and v1.2.1, or is it a problem with all v1.2 releases?

/cc @mlavacca

@snehachhabria
Copy link
Contributor Author

snehachhabria commented Dec 13, 2024

Thanks for reporting this @snehachhabria! To clarify, is this a regression between v1.2.0 and v1.2.1, or is it a problem with all v1.2 releases?

/cc @mlavacca
it's a problem with all v1.2 releases

@robscott
Copy link
Member

/kind bug

@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Dec 13, 2024
@mlavacca
Copy link
Member

Thanks for reporting this issue, @snehachhabria! With #3343 we introduced the possibility to customize the client options to be able to pass scheme, mapper, etc. Isn't the ClientOptions field enough to fit your needs?

@snehachhabria
Copy link
Contributor Author

Thanks for reporting this issue, @snehachhabria! With #3343 we introduced the possibility to customize the client options to be able to pass scheme, mapper, etc. Isn't the ClientOptions field enough to fit your needs?

Unfortunately not, exposing the Client allows us to modify the writer interface for the Create/Update function to add some custom annotations while creating gateway objects

@youngnick
Copy link
Contributor

Are there any more details you can share about what you need to add? Maybe that might be useful for other folks as well, and we should build support directly into the tests?

@snehachhabria
Copy link
Contributor Author

snehachhabria commented Dec 17, 2024

Our implementation requires custom annotations on the gateway object, sample can be found here: https://learn.microsoft.com/en-us/azure/application-gateway/for-containers/how-to-traffic-splitting-gateway-api?tabs=alb-managed#deploy-the-required-gateway-api-resources . In order to add these annotations to the gateway objects created by the conformance suite we have modified the Create/Update function exposed via the client.

It would be great if support for annotations on Gateway objects can be supported directly via the test suite, similar to how namespace annotations are exposed:

NamespaceAnnotations map[string]string
. That way it could potentially be used by other implementors as well

@youngnick
Copy link
Contributor

To be honest, it's really intended that what's described in that doc is implemented by having a GatewayClass that sets all Gateways associated to it to use the configured ALB.

I really don't like the approach of using annotations on Gateway to do this - it's totally against one of the major design goals of Gateway API, which is to pull that kind of config out of annotations and into structured fields and more specific resources.

@robscott
Copy link
Member

+1 to @youngnick's comment here. In my mind, one of the most important purposes of conformance tests is ensuring that users will get the same shared experience out of the box with our portable features. Obviously implementations should feel welcome to add features on top of the core API, but we really want the base API and examples to work without any changes (other than setting a GatewayClass name on a Gateway).

If I'm understanding this specific modification correctly, it seems like users would need to manually configure some annotations on each Gateway, which goes against the API experience we're trying to guarantee with conformance tests and status.

@snehachhabria
Copy link
Contributor Author

@youngnick and @robscott thanks a lot of sharing your thoughts. I completely understand the primary design goal of Gateway API is not to have any configurability via annotations. Given the limitations that existed at the time we were implementing Gateway API and our infrastructure/design needs we had to go down the route of using an annotation on the Gateway object.

I understand if providing support for annotations on the gateway object in the conformance is against the experience desired and shouldn't be exposed. But if we could go back to having the Client exposed, like it was prior to release v1.2, we would still be able to run the conformance tests and generate the reports. We are still conformant with all of Gateway API's requirements and would like to continue doing so, the conformance tests aid this and it would be great if we can run them as is without too many modifications from our end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/conformance kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

No branches or pull requests

5 participants