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

Add conformance report for Application Gateway for Containers #3021

Merged
merged 2 commits into from
May 1, 2024

Conversation

snehachhabria
Copy link
Contributor

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

@k8s-ci-robot k8s-ci-robot added area/conformance do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 24, 2024
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 24, 2024
@k8s-ci-robot
Copy link
Contributor

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.

@mikemorris
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 24, 2024
@snehachhabria
Copy link
Contributor Author

snehachhabria commented Apr 24, 2024

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.

@mikemorris
Copy link
Contributor

@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?

@robscott
Copy link
Member

robscott commented Apr 24, 2024

@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:

features.SupportGatewayPort8080,

extendedUnsupportedFeatures map[ConformanceProfileName]sets.Set[features.SupportedFeature]

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

@snehachhabria
Copy link
Contributor Author

@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:

features.SupportGatewayPort8080,

extendedUnsupportedFeatures map[ConformanceProfileName]sets.Set[features.SupportedFeature]

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
Copy link
Contributor Author

/retest

Copy link
Member

@robscott robscott left a 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)|


Copy link
Member

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?

https://github.com/kubernetes-sigs/gateway-api/tree/main/conformance/reports/v1.0.0/kong-kubernetes-ingress-controller#prerequisites

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
Copy link
Member

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:

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.

/cc @mlavacca @shaneutt

Copy link
Member

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

- 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.

Copy link
Contributor

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.

/cc @robscott @shaneutt

@snehachhabria
Copy link
Contributor Author

@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.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 30, 2024
@youngnick
Copy link
Contributor

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.

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 30, 2024
Co-authored-by: Rob Scott <rob.scott87@gmail.com>
@robscott
Copy link
Member

Thanks @snehachhabria!

/approve

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

@k8s-ci-robot
Copy link
Contributor

[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 k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 30, 2024
@youngnick
Copy link
Contributor

Agreed, let's get this in.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 1, 2024
@robscott
Copy link
Member

robscott commented May 1, 2024

/release-note-none

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels May 1, 2024
@k8s-ci-robot k8s-ci-robot merged commit 8372d20 into kubernetes-sigs:main May 1, 2024
8 checks passed
@snehachhabria snehachhabria deleted the conformanceReport branch May 7, 2024 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/conformance cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants