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.

Sign up for GitHub

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

Jump to bottom

[GEP-1911] h2c backend protocol conformance #2456

Merged
merged 4 commits into from
Oct 24, 2023
Merged

[GEP-1911] h2c backend protocol conformance #2456

merged 4 commits into from
Oct 24, 2023

Conversation

dprotaso
Copy link
Contributor

@dprotaso dprotaso commented Oct 5, 2023 •
edited
Loading

/kind test
/area conformance

What this PR does / why we need it:

This PR adds a conformance test validating implementations support h2c when the target Kubernetes Service Service Port has a kubernetes.io/h2c appProtocol

Depends on echo-basic changes which are being bumped here: #2456

Which issue(s) this PR fixes:

Related #1911

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/test area/conformance cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 5, 2023
@shaneutt shaneutt marked this pull request as draft October 9, 2023 15:11
@dprotaso dprotaso mentioned this pull request Oct 11, 2023
Merged
@dprotaso dprotaso force-pushed the gep-1911-h2c branch 2 times, most recently from aed554d to afc0295 Compare October 13, 2023 21:04
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 14, 2023
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 14, 2023
@dprotaso dprotaso changed the title [WIP][GEP-1911] h2c backend protocol conformance [GEP-1911] h2c backend protocol conformance Oct 15, 2023
@dprotaso dprotaso marked this pull request as ready for review October 15, 2023 15:34
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 15, 2023
@robscott
Copy link
Member

Thanks @dprotaso! This looks reasonable to me, but will defer to conformance approvers on this one.

/cc @arkodg @sunjayBhatia @mlavacca

@dprotaso dprotaso mentioned this pull request Oct 19, 2023
Closed
@dprotaso
Copy link
Contributor Author

/assign @arkodg @mlavacca @sunjayBhatia

arkodg
arkodg reviewed Oct 20, 2023
View reviewed changes
conformance/tests/httproute-backend-protocol-h2c.go
gwNN := types.NamespacedName{Name: "same-namespace", Namespace: ns}
gwAddr := kubernetes.GatewayAndHTTPRoutesMustBeAccepted(t, suite.Client, suite.TimeoutConfig, suite.ControllerName, kubernetes.NewGatewayRef(gwNN), routeNN)

// We are not testing the h2c HTTP upgrade mechanism as it is deprecated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ha good to know

arkodg
arkodg approved these changes Oct 20, 2023
View reviewed changes
Copy link
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good !
/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 20, 2023
@arkodg arkodg mentioned this pull request Oct 20, 2023
Merged
sunjayBhatia
sunjayBhatia reviewed Oct 23, 2023
View reviewed changes
Copy link
Member

@sunjayBhatia sunjayBhatia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like

gateway-api/conformance/conformance_test.go

Lines 49 to 50 in 0cc3e0e

v1alpha2.AddToScheme(client.Scheme())
v1beta1.AddToScheme(client.Scheme())
needs to add the v1 api as well, otherwise tests don't run (same for main so not exclusive to this PR)

conformance/base/manifests.yaml Show resolved Hide resolved
sunjayBhatia
sunjayBhatia approved these changes Oct 23, 2023
View reviewed changes
Copy link
Member

@sunjayBhatia sunjayBhatia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

other than the previous comment, everything seems to work, (I cheated a little with Contour and added the required annotation contour supports for h2c to get the tests to pass/fail accordingly since we don't support the appprotocol field yet)

sunjayBhatia
sunjayBhatia suggested changes Oct 23, 2023
View reviewed changes
Copy link
Member

@sunjayBhatia sunjayBhatia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I'll request changes officially still since I have some outstanding requests and there is a prerequisite change that needs to happen before these tests actually run successfully

@dprotaso
Copy link
Contributor Author

I guess I'll request changes officially still since I have some outstanding requests and there is a prerequisite change that needs to happen before these tests actually run successfully

what do you mean?

@sunjayBhatia
Copy link
Member

I guess I'll request changes officially still since I have some outstanding requests and there is a prerequisite change that needs to happen before these tests actually run successfully

what do you mean?

#2456 (review)

the v1 api group/version needs to be added to the scheme otherwise the tests don't run

otherwise everything looks good

@dprotaso
Copy link
Contributor Author

Made a separate PR: #2511

@dprotaso dprotaso mentioned this pull request Oct 23, 2023
Merged
mlavacca
mlavacca approved these changes Oct 24, 2023
View reviewed changes
Copy link
Member

@mlavacca mlavacca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from a minor nitpick, lgtm 👍

conformance/utils/suite/features.go Outdated Show resolved Hide resolved
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arkodg, dprotaso, mlavacca

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

@mlavacca
Copy link
Member

Since @sunjayBhatia's requested changes have been addressed in a separate PR, I think we can merge this one, as no other concern popped up.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 24, 2023
@k8s-ci-robot k8s-ci-robot merged commit e2912b0 into kubernetes-sigs:main Oct 24, 2023
@dprotaso dprotaso deleted the gep-1911-h2c branch October 24, 2023 14:05
@dprotaso dprotaso mentioned this pull request Oct 26, 2023
Closed
@dprotaso dprotaso mentioned this pull request Apr 2, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sunjayBhatia sunjayBhatia sunjayBhatia requested changes

@mlavacca mlavacca mlavacca approved these changes

@arkodg arkodg arkodg approved these changes

@LiorLieberman LiorLieberman Awaiting requested review from LiorLieberman

@michaelbeaumont michaelbeaumont Awaiting requested review from michaelbeaumont

@kflynn kflynn Awaiting requested review from kflynn

Assignees

@mlavacca mlavacca

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. kind/test lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@dprotaso @robscott @sunjayBhatia @k8s-ci-robot @mlavacca @arkodg