[Handler] Integrate the watcher's cache into request handler#78
Conversation
weilerN
changed the title
TomerShor
left a comment
TomerShor
left a comment
There was a problem hiding this comment.
Nice, added some comments and suggestions
pkg/dlx/handler.go
Outdated
| proxy.ServeHTTP(res, req) | ||
| } | ||
|
|
||
| func (h *Handler) getResourceNameAndPath(req *http.Request) (string, []string, error) { |
There was a problem hiding this comment.
align to the return order to make it clearer
| func (h *Handler) getResourceNameAndPath(req *http.Request) (string, []string, error) { | |
| func (h *Handler) getPathAndResourceNames(req *http.Request) (string, []string, error) { |
There was a problem hiding this comment.
Fixed here - CR comment - align namings
pkg/dlx/handler.go
Outdated
| return path, resourceNames, nil | ||
| } | ||
|
|
||
| func (h *Handler) extractValuesFromIngress(req *http.Request) (string, []string, error) { |
There was a problem hiding this comment.
| func (h *Handler) extractValuesFromIngress(req *http.Request) (string, []string, error) { | |
| func (h *Handler) getValuesFromCache(req *http.Request) (string, []string, error) { |
There was a problem hiding this comment.
Fixed here - CR comment - align namings
pkg/dlx/handler.go
Outdated
| return path, resourceNames, nil | ||
| } | ||
|
|
||
| h.logger.DebugWith("Failed to get target name from ingress cache, try to extract from the request headers", |
There was a problem hiding this comment.
| h.logger.DebugWith("Failed to get target name from ingress cache, try to extract from the request headers", | |
| h.logger.DebugWith("Failed to get target from ingress cache, trying to extract from the request headers", |
There was a problem hiding this comment.
Fixed here - CR comment - align namings
pkg/dlx/handler.go
Outdated
| path := req.URL.Path | ||
| resourceNames, err := h.ingressCache.Get(host, path) | ||
| if err != nil { | ||
| return "", nil, errors.New("Failed to get target name from ingress cache") |
There was a problem hiding this comment.
| return "", nil, errors.New("Failed to get target name from ingress cache") | |
| return "", nil, errors.New("Failed to get target names from ingress cache") |
There was a problem hiding this comment.
Fixed here - CR comment - align namings
pkg/dlx/handler.go
Outdated
| resourceNames, err := h.ingressCache.Get(host, path) | ||
| if err != nil { | ||
| return "", nil, errors.New("Failed to get target name from ingress cache") |
There was a problem hiding this comment.
You mix resourceNames and target name in the variables and logs - choose one and stick with it.
There was a problem hiding this comment.
Right. I’ll keep using resourceNames to stay consistent with the naming convention used in the handler context.
There was a problem hiding this comment.
Fixed here - CR comment - align namings
pkg/dlx/handler_test.go
Outdated
| } | ||
|
|
||
| func (suite *HandlerTestSuite) TestHandleRequest() { | ||
| for _, tc := range []struct { |
There was a problem hiding this comment.
| for _, tc := range []struct { | |
| for _, testCase := range []struct { |
There was a problem hiding this comment.
Fixed here - CR comment - unit test comments
pkg/dlx/handler_test.go
Outdated
| reqPath: "/test/path", | ||
| expectedStatus: http.StatusOK, | ||
| }, { | ||
| name: "No ingress headers,multiple targets found in ingress cache", |
There was a problem hiding this comment.
| name: "No ingress headers,multiple targets found in ingress cache", | |
| name: "No ingress headers, multiple targets found in ingress cache", |
There was a problem hiding this comment.
Fixed here - CR comment - unit test comments
pkg/dlx/handler_test.go
Outdated
| } | ||
|
|
||
| func (suite *HandlerTestSuite) TestGetResourceNameAndPath() { | ||
| for _, tc := range []struct { |
There was a problem hiding this comment.
| for _, tc := range []struct { | |
| for _, testCase := range []struct { |
There was a problem hiding this comment.
Fixed here - CR comment - unit test comments
| }, { | ||
| name: "Missing both ingress headers and host and path did not found in ingress cache", | ||
| reqHost: "www.example.com", | ||
| reqPath: "/test/path", | ||
| expectErr: true, | ||
| errMsg: "No target name header found", | ||
| }, | ||
| } { |
There was a problem hiding this comment.
Add a test case for existing both cache and headers - cache should take
There was a problem hiding this comment.
Good catch of missing test case, adding
There was a problem hiding this comment.
Fixed here - CR comment - unit test comments
pkg/dlx/handler_test.go
Outdated
| testIngressCache := ingresscache.NewIngressCache(suite.logger) | ||
| if tc.initialCachedData != nil { | ||
| err := testIngressCache.Set(tc.initialCachedData.host, tc.initialCachedData.path, tc.initialCachedData.targets) | ||
| suite.Require().NoError(err) | ||
| } | ||
|
|
||
| testHandler := suite.createTestHandler(suite.backendPort, testIngressCache) | ||
| testRequest := suite.createTestHTTPRequest(tc.reqHeaders, tc.reqHost, tc.reqPath) |
There was a problem hiding this comment.
Duplicate from previous test - create a helper function
There was a problem hiding this comment.
I replaced suite.createTestHandler with suite.createTestHandlerAndInitTestCache to reduce the duplication.
Refactoring the rest of the variables is a bit tedious, so I prefer to keep the test as-is for now.
Done here - CR comment - unit test comments
pkg/dlx/handler.go
Outdated
| path := req.URL.Path | ||
| resourceNames, err := h.ingressCache.Get(host, path) | ||
| if err != nil { | ||
| return "", nil, errors.New("Failed to get resourceNames from ingress cache") |
There was a problem hiding this comment.
| return "", nil, errors.New("Failed to get resourceNames from ingress cache") | |
| return "", nil, errors.New("Failed to get resource names from ingress cache") |
There was a problem hiding this comment.
There was a problem hiding this comment.
| name string | ||
| resolveServiceNameErr error | ||
| initialCachedData *kube.IngressValue | ||
| reqHeaders map[string]string |
There was a problem hiding this comment.
You have no test case for existing request headers
There was a problem hiding this comment.
Right, I didn't add coverage for the existing flow.
There was a problem hiding this comment.
There was a problem hiding this comment.
Added coverage here - CR comments - Unit tests - add request header flow coverage
pkg/dlx/handler_test.go
Outdated
| expectedStatus int | ||
| }{ | ||
| { | ||
| name: "No ingress headers, host and path found in ingress cache", |
There was a problem hiding this comment.
| name: "No ingress headers, host and path found in ingress cache", | |
| name: "No request headers, host and path found in ingress cache", |
Same comment for all test case names
There was a problem hiding this comment.
pkg/dlx/handler_test.go
Outdated
| expectedResourceNames []string | ||
| }{ | ||
| { | ||
| name: "No ingress headers, host and path found in ingress cache", |
There was a problem hiding this comment.
Same for the cases in this test:
| name: "No ingress headers, host and path found in ingress cache", | |
| name: "No request headers, host and path found in ingress cache", |
There was a problem hiding this comment.
pkg/dlx/handler.go
Outdated
| h.logger.WarnWith("Failed to get resource names and path from request", | ||
| "error", err.Error(), | ||
| "host", req.Host, | ||
| "path", req.URL.Path) |
There was a problem hiding this comment.
I see that req.URL is pointer, so might technically be nil, do we check it anywhere?
There was a problem hiding this comment.
Good catch. I will cover it
There was a problem hiding this comment.
Fixed here - CR comments - handler req.URL is nil
### Description This PR integrates the new DLX from the Scaler package, which introduces a revised internal flow. The changes align the DLX initialization with the new Scaler initialization and configuration patterns by updating the `dlxOptions` and related logic. ### Changes Made - Extracted hardcoded configuration variables - Implemented the `scalertype.ResolveTargetsFromIngressCallback` for extracting Nuclio targets - Added changes to the DLX init (as described in the Jira) ### Testing - Unit tests for the new `resolveCallback` - Manual integration tests to verify the functionalities ### References - Jira ticket link - https://iguazio.atlassian.net/browse/NUC-509 - External links - the related scaler PRs -- v3io/scaler#76 -- v3io/scaler#78 ### Additional Notes - Before merging this PR, the `go.mod` must be modified since it currently relies on a feature branch --------- Co-authored-by: Katerina Molchanova <35141662+rokatyy@users.noreply.github.com>
3 participants
Description
This PR integrates the cache into the handler in order to resolve the request target based on the new cache.
Changes Made
IngressHostCacheReader(the cache read-only access) to the handlerTesting
References
Additional Notes
targetURL, the handler appends the path twice. This fix is relatively minor and has been tested manually, but since the current code still works, it's worth deciding whether we want to address it now.