-
Notifications
You must be signed in to change notification settings - Fork 593
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: fix refChecker when namespace is specified and is the same as route's namespace #5392
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #5392 +/- ##
=======================================
+ Coverage 0 69.5% +69.5%
=======================================
Files 0 176 +176
Lines 0 22525 +22525
=======================================
+ Hits 0 15673 +15673
- Misses 0 5924 +5924
- Partials 0 928 +928 ☔ View full report in Codecov by Sentry. |
This needs to be updated to account for failing conformance test:
|
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 have checked the failing conformance test: TestGatewayConformance/HTTPRouteInvalidNonExistentBackendRef/HTTP_Request_to_invalid_nonexistent_backend_receive_a_500
. This test case specifies a backendRef
with the same namespace of the HTTPRoute
.
Before the change, we got 0 available backend services, but after the change we got a backend but with no upstreams. So we got 503
but not the expected 500
.
We can see the following message in the output of conformance tests:
http.go:238: Response expectation failed for request: {URL: {Scheme:http Opaque: User: Host:172.18.128.3 Path:/ RawPath: OmitHost:false ForceQuery:false RawQuery: Fragment: RawFragment:}, Host: , Protocol: HTTP, Method: GET, Headers: map[X-Echo-Set-Header:[]], UnfollowRedirect: false, Server: , CertPem: <truncated>, KeyPem: <truncated>} not ready yet: expected status code to be 500, got 503 (after 17.04749077s)
Yes. In order to mitigate that we'd have to pass down a client of sorts (or something that could list namespaces) to either
kubernetes-ingress-controller/internal/dataplane/translator/wrappers_refchecker.go Line 23 in 9b81665
backendRef exists.
Unless there are other ideas that I haven't thought about 🤔 |
Yes it will. It's cross referenced above. |
fa1bf7c
to
946db68
Compare
946db68
to
c386d83
Compare
Co-authored-by: Grzegorz Burzyński <[email protected]>
What this PR does / why we need it:
When route has the same namespace specified as the backendref, then it should be allowed without a
ReferenceGrant
.Additionally this PR adds
Service
lookup inbackendRefsToKongStateBackends()
to verify if the provided namespace exists.The gist of this change is in
internal/dataplane/translator/backendref.go
. The rest is just adapting the tests basically.Which issue this PR fixes:
Fixes: #5388
Special notes for your reviewer:
PR Readiness Checklist:
Complete these before marking the PR as
ready to review
:CHANGELOG.md
release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR