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

fix: set accepted even if gateway could not be programmed and attach route to listener even if listener not ready #4987

Merged
merged 12 commits into from
Oct 31, 2023

Conversation

randmonkey
Copy link
Contributor

@randmonkey randmonkey commented Oct 26, 2023

What this PR does / why we need it:

To pass the conformance test GatewayWithAttachedRoutes, the PR:

  • Set Accepted of listener to true even if the listener could not be Programmed
  • Attach routes to listeners even if the listener is not ready

Which issue this PR fixes:

fixes #4982

Special notes for your reviewer:

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

  • the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

@randmonkey randmonkey requested a review from a team as a code owner October 26, 2023 10:28
@randmonkey randmonkey marked this pull request as draft October 26, 2023 10:28
@codecov
Copy link

codecov bot commented Oct 26, 2023

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Comparison is base (0422f3b) 75.3% compared to head (533a4b6) 73.1%.
Report is 4 commits behind head on main.

❗ Current head 533a4b6 differs from pull request most recent head e39fd2c. Consider uploading reports for the commit e39fd2c to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #4987     +/-   ##
=======================================
- Coverage   75.3%   73.1%   -2.2%     
=======================================
  Files        167     167             
  Lines      18721   18843    +122     
=======================================
- Hits       14104   13788    -316     
- Misses      3790    4227    +437     
- Partials     827     828      +1     
Files Coverage Δ
...ternal/controllers/gateway/grpcroute_controller.go 71.6% <100.0%> (ø)
...ternal/controllers/gateway/httproute_controller.go 67.0% <100.0%> (-10.2%) ⬇️
...nternal/controllers/gateway/tlsroute_controller.go 75.7% <100.0%> (ø)
...nternal/controllers/gateway/tcproute_controller.go 16.0% <0.0%> (ø)
...nternal/controllers/gateway/udproute_controller.go 16.0% <0.0%> (ø)
internal/controllers/gateway/route_utils.go 82.1% <91.1%> (-3.3%) ⬇️
internal/controllers/gateway/gateway_utils.go 83.4% <86.1%> (-0.8%) ⬇️

... and 28 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@randmonkey randmonkey force-pushed the fix/attach_route_unresolved_refs branch from b0806c9 to a144f68 Compare October 27, 2023 03:36
@randmonkey randmonkey changed the title fix: set accepted even if gateway could not be programmed for conformance test GatewayWithAttachedRoutes fix: set accepted even if gateway could not be programmed and attach route to listener even if listener not ready Oct 27, 2023
@randmonkey randmonkey marked this pull request as ready for review October 27, 2023 03:39
@randmonkey randmonkey force-pushed the fix/attach_route_unresolved_refs branch from a144f68 to e80ed59 Compare October 30, 2023 06:26
@randmonkey
Copy link
Contributor Author

The current main branch of gateway API has merged PR kubernetes-sigs/gateway-api#2535 that expect Accepted of HTTPRoute is false, and does not check the Accepted status of the listener.

@pmalek pmalek added the area/gateway-api Relating to upstream Kubernetes SIG Networking Gateway API label Oct 30, 2023
pmalek
pmalek previously approved these changes Oct 30, 2023
@pmalek pmalek requested a review from czeslavo October 30, 2023 13:00
@pmalek pmalek added this to the KIC v3.0.0 milestone Oct 30, 2023
internal/controllers/gateway/gateway_utils.go Outdated Show resolved Hide resolved
internal/controllers/gateway/route_utils.go Outdated Show resolved Hide resolved
internal/controllers/gateway/route_utils.go Outdated Show resolved Hide resolved
internal/controllers/gateway/route_utils.go Outdated Show resolved Hide resolved
Copy link
Member

@programmer04 programmer04 left a comment

Choose a reason for hiding this comment

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

typos

@randmonkey randmonkey added the do not merge let the author merge this, don't merge for them. label Oct 30, 2023
@randmonkey
Copy link
Contributor Author

marked this as "do not merge" since the latest version of gateway API changed expected "Accepted" condition of tested HTTPRoute to false and does not check the "Accepted" condition of the listener. This change is brought by the PR kubernetes-sigs/gateway-api#2535.

@randmonkey randmonkey removed the do not merge let the author merge this, don't merge for them. label Oct 31, 2023
go.mod Show resolved Hide resolved
test/conformance/gateway_conformance_test.go Show resolved Hide resolved
internal/controllers/gateway/route_utils.go Outdated Show resolved Hide resolved
internal/controllers/gateway/route_utils.go Outdated Show resolved Hide resolved
internal/controllers/gateway/route_utils.go Outdated Show resolved Hide resolved
internal/controllers/gateway/gateway_utils_test.go Outdated Show resolved Hide resolved
internal/controllers/gateway/route_utils.go Show resolved Hide resolved
internal/controllers/gateway/route_utils.go Outdated Show resolved Hide resolved
internal/controllers/gateway/route_utils.go Outdated Show resolved Hide resolved
@programmer04 programmer04 requested review from programmer04 and removed request for programmer04 October 31, 2023 11:49
programmer04
programmer04 previously approved these changes Oct 31, 2023
Makefile Show resolved Hide resolved
@pmalek pmalek enabled auto-merge (squash) October 31, 2023 12:40
@pmalek pmalek merged commit dce791d into main Oct 31, 2023
33 checks passed
@pmalek pmalek deleted the fix/attach_route_unresolved_refs branch October 31, 2023 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/gateway-api Relating to upstream Kubernetes SIG Networking Gateway API size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support conformance test GatewayWithAttachedRoutes
4 participants