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(gateway) handle NodePort addresses #2827

Merged
merged 2 commits into from
Aug 17, 2022
Merged

fix(gateway) handle NodePort addresses #2827

merged 2 commits into from
Aug 17, 2022

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Aug 16, 2022

What this PR does / why we need it:

Ensure that the controller's desired addresses for Services with no external addresses (such as NodePorts) match the Gateway representation of no addresses. This ensures that the address update check does not attempt to proceed with an unnecessary update every reconcile, which would prevent the Gateway from becoming ready.

Add an E2E test that confirms Gateways work with NodePorts.

Which issue this PR fixes:

Fix #2808
Fix #2823

Special notes for your reviewer:

E2E run: https://github.com/Kong/kubernetes-ingress-controller/actions/runs/2871403290

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

@rainest rainest requested a review from a team as a code owner August 16, 2022 20:05
Ensure that the controller's desired addresses for Services with no
external addresses (such as NodePorts) match the Gateway representation
of no addresses. This ensures that the address update check does not
attempt to proceed with an unnecessary update every reconcile, which
would prevent the Gateway from becoming ready.
@rainest rainest temporarily deployed to Configure ci August 16, 2022 20:51 Inactive
@shaneutt shaneutt added the bug Something isn't working label Aug 16, 2022
@shaneutt shaneutt added this to the Gateway API - Milestone 3 milestone Aug 16, 2022
@rainest
Copy link
Contributor Author

rainest commented Aug 16, 2022

This is currently affected by the failure that #2824 will fix. Draft waiting for that; this needs to be rebased after that's merged.

2022-08-16T20:24:15.5342094Z     helpers_test.go:290: 
2022-08-16T20:24:15.5343441Z         	Error Trace:	/home/runner/work/kubernetes-ingress-controller/kubernetes-ingress-controller/test/e2e/helpers_test.go:290
2022-08-16T20:24:15.5345023Z         	            				/home/runner/work/kubernetes-ingress-controller/kubernetes-ingress-controller/test/e2e/features_test.go:337
2022-08-16T20:24:15.5345626Z         	Error:      	Condition never satisfied
2022-08-16T20:24:15.5346129Z         	Test:       	TestDeployAllInOneDBLESSGateway
2022-08-16T20:24:15.5347559Z     features_test.go:286: cleaning up environment for cluster 14e300ea-8260-41ba-9b37-7493e874583f
2022-08-16T20:24:17.0472065Z --- FAIL: TestDeployAllInOneDBLESSGateway (396.49s)

@rainest rainest marked this pull request as draft August 16, 2022 20:53
@rainest rainest temporarily deployed to Configure ci August 16, 2022 21:15 Inactive
HTTPRoutes don't strip paths by default. We wrote tests that need them
stripped.
@rainest rainest temporarily deployed to Configure ci August 16, 2022 22:10 Inactive
@rainest rainest temporarily deployed to Configure ci August 16, 2022 22:11 Inactive
@rainest
Copy link
Contributor Author

rainest commented Aug 16, 2022

On second thought, looking at that failure, configuring the route to strip paths makes more sense than intentionally causing a 404 and scraping the server header, so I've added that change here.

@rainest rainest temporarily deployed to Configure ci August 16, 2022 22:33 Inactive
@randmonkey
Copy link
Contributor

On second thought, looking at that failure, configuring the route to strip paths makes more sense than intentionally causing a 404 and scraping the server header, so I've added that change here.

OK, I did not find that we have an annotation to do the strip path of HTTPRoute. I think we do not need to put #2824 as a blocker of this.

@randmonkey randmonkey marked this pull request as ready for review August 17, 2022 02:55
@randmonkey randmonkey merged commit ab1db69 into main Aug 17, 2022
@randmonkey randmonkey deleted the fix/nodeport-gw branch August 17, 2022 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants