-
Notifications
You must be signed in to change notification settings - Fork 472
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
Add conformance report for Application Gateway for Containers #3021
Add conformance report for Application Gateway for Containers #3021
Conversation
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 Once the patch is verified, the new status will be reflected by the 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. |
/ok-to-test |
343ce99
to
6401341
Compare
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. |
@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 @snehachhabria I think if you mark
gateway-api/conformance/utils/suite/suite.go Line 114 in 2645468
If that doesn't work, we're probably missing something in our conformance test runner. |
6401341
to
a416c22
Compare
thanks for the suggestion @robscott. I was able to do this and regenerated our conformance report. PTAL |
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @snehachhabria! Congrats on reaching this milestone for conformance!
|-----------|----------------------|----|------| | ||
|x|[v1.0.0](https://learn.microsoft.com/azure/application-gateway/for-containers/alb-controller-release-notes#latest-release-recommended)|x|[link](./v1.0.0-report.yaml)| | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've started to require reproduction steps for all new conformance reports, do you mind filling out something like this?
It may also be useful to look at AKS upstream conformance here because this will probably be the first conformance report that requires spinning up a cluster on a specific cloud provider.
Failed: 0 | ||
summary: "" | ||
skippedTests: | ||
- GatewayClassObservedGenerationBump |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't really have a lot of prior art for how we represent the idea of conformance status when it comes to skipped tests. Apparently we actually have another implementation that has a skipped test in their report, but I think we just missed that:
- MeshFrontendHostname |
This doesn't need to block this PR, but I'll create a follow up issue to track how we can represent this kind of partial conformance on our implementations page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have rules in our rulebook about partial results
gateway-api/conformance/reports/README.md
Lines 123 to 127 in 7320257
- Test result: in order to have a report valid to be accepted, all the profiles | |
need to have the `result` fields (core and extended) set to `success`. It means | |
that all the core conformance tests have been successfully run as well as all | |
the tests related to the supported extended features. No reports with partial | |
or failing results can be accepted. |
The rule here should be applied starting from 1.1.0, but I honestly think it makes sense to begin enforcing it here from now on, even considering the skipped test is a core one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I share the concerns expressed over in #3025 (comment) and would propose that it's valuable to both end users and implementers to allow acceptance of conformance reports with "partial" conformance.
For users, having a clear centralized reference to see where current behavior of an implementation may differ from the spec, and to see implementation progress toward full conformance can help users make an educated decision and know where to look if some functionality is missing or does not work as expected.
For implementers, being able to publicly document incremental progress toward conformance can be motivating and demonstrate a commitment to reaching conformance as a project goal.
I do think we should promote or highlight fully-conformant implementations as a reward of sorts, but allowing acceptance of partial conformance reports would be a marked improvement over the current list of implementations which would benefit substantially from this additional context.
@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. |
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. |
a416c22
to
b3b40e0
Compare
b3b40e0
to
a5c10cf
Compare
Co-authored-by: Rob Scott <rob.scott87@gmail.com>
Thanks @snehachhabria! /approve Will defer to @youngnick or @mlavacca for final LGTM. |
[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 |
Agreed, let's get this in. /lgtm |
/release-note-none |
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