gateway-api icon indicating copy to clipboard operation
gateway-api copied to clipboard

Add conformance report for Application Gateway for Containers

Open snehachhabria opened this issue 1 year ago • 12 comments

What type of PR is this? /area conformance

What this PR does / why we need it: Adds conformance report and updates implementation page for Azure Application Gateway for Containers v1.0.0

Which issue(s) this PR fixes: N/A

Does this PR introduce a user-facing change?: None

snehachhabria avatar Apr 24 '24 20:04 snehachhabria

Hi @snehachhabria. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot avatar Apr 24 '24 20:04 k8s-ci-robot

/ok-to-test

mikemorris avatar Apr 24 '24 20:04 mikemorris

As FYI, the tests skipped in the core implementation are because they use port 8080 and Application Gateway For Containers currently supports only port 80 and 443.

snehachhabria avatar Apr 24 '24 21:04 snehachhabria

@robscott Are the tests in https://github.com/kubernetes-sigs/gateway-api/blob/264546815534e45a777c7ae48ecd3c493d7aaa86/conformance/tests/gateway-with-attached-routes-with-port-8080.yaml something we could consider scoping differently so they could be skipped more granularly without skipping the rest of that (relatively large and important) test?

mikemorris avatar Apr 24 '24 21:04 mikemorris

@mikemorris @snehachhabria I think if you mark SupportGatewayPort8080 as an unsupported feature when generating this test report, any tests dependent on that should automatically be skipped:

https://github.com/kubernetes-sigs/gateway-api/blob/264546815534e45a777c7ae48ecd3c493d7aaa86/conformance/tests/gateway-with-attached-routes.go#L138 https://github.com/kubernetes-sigs/gateway-api/blob/264546815534e45a777c7ae48ecd3c493d7aaa86/conformance/utils/suite/suite.go#L114

If that doesn't work, we're probably missing something in our conformance test runner.

robscott avatar Apr 24 '24 21:04 robscott

@mikemorris @snehachhabria I think if you mark SupportGatewayPort8080 as an unsupported feature when generating this test report, any tests dependent on that should automatically be skipped:

https://github.com/kubernetes-sigs/gateway-api/blob/264546815534e45a777c7ae48ecd3c493d7aaa86/conformance/tests/gateway-with-attached-routes.go#L138

https://github.com/kubernetes-sigs/gateway-api/blob/264546815534e45a777c7ae48ecd3c493d7aaa86/conformance/utils/suite/suite.go#L114

If that doesn't work, we're probably missing something in our conformance test runner.

thanks for the suggestion @robscott. I was able to do this and regenerated our conformance report. PTAL

snehachhabria avatar Apr 24 '24 23:04 snehachhabria

/retest

snehachhabria avatar Apr 25 '24 19:04 snehachhabria

@robscott , unfortunately we have an implementation specific annotation that is required for the gateway objects in order for our controller to process it. An example for the annotation 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-t[…]esources

Internally in our codebase, we have modified the kube client used by the conformance to inject this annotation on the fly for all gateway objects created by the conformance and that's how we run the conformance tests in our repo. Since our project is a managed offering from a cloud provider, we have not publicly exposed anything. There might be a few solutions such as creating a cli tool for our project and adding a command to run conformance tests, but it would take time. We would like to think a bit on what the best way to expose reproducing the conformance report would be.

In the meantime would it be possible to merge the current PR? and I can take an actionable item on our end to come up with a way to reproduce the report generation that would work for both Gateway API and our project.

snehachhabria avatar Apr 26 '24 18:04 snehachhabria

I think that, in the spirit of encouraging folks to submit conformance sooner rather than later, and accept partial conformance reports, we should accept this as-is. This means that we have some time to sort out the details of how we handle problems with conformance that lie outside the actual suite (which #3036 is discussing), while the Azure folks look at how to pass the ObservedGenerationBump GatewayClass test.

youngnick avatar Apr 30 '24 07:04 youngnick

Thanks @snehachhabria!

/approve

Will defer to @youngnick or @mlavacca for final LGTM.

robscott avatar Apr 30 '24 22:04 robscott

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: robscott, snehachhabria

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot avatar Apr 30 '24 22:04 k8s-ci-robot

Agreed, let's get this in.

/lgtm

youngnick avatar May 01 '24 03:05 youngnick

/release-note-none

robscott avatar May 01 '24 05:05 robscott