Skip to content

Commit 4e44dbc

Browse files
authored
[Scaler] Fix path duplication for request http (#82)
### 📝 Description The changes simplify the handler logic by removing redundant path handling and ensuring that path additions are only applied when explicitly provided via headers. --- ### 🛠️ Changes Made - Replaced `getPathAndResourceNames` in favor of a simplified `getResourceNames`. - Updated `handleRequest` to extract `pathAddition` only from the pre-configured handler's `targetPathHeader`. - Adjusted cache lookup functions to return only `resourceNames` instead of `(path, resourceNames)`. --- ### ✅ Checklist - [x] I have tested the changes in this PR --- ### 🧪 Testing - Dev QA - Created a function that prints the `event.path` - Triggered the function with this command: `curl print-path.default-tenant.app.vmdev5.lab.iguazeng.com/a/b` - **Before changes:** `"name":"processor.http.w0.python.logger.connection-manager.event","message":"Request path","more":{"path":"/a/b/a/b"}}` - **After changes:** `"name":"processor.http.w0.python.logger.connection-manager.event","message":"Request path","more":{"path":"/a/b"}}` - Updated UTs and added more cases coverage --- ### 🔗 References - Ticket link: https://iguazio.atlassian.net/browse/NUC-602 - Design docs links: - External links: The second path append happens [here](https://cs.opensource.google/go/go/+/refs/tags/go1.25.1:src/net/http/httputil/reverseproxy.go;l=275) --- ### 🚨 Breaking Changes? - [ ] Yes (explain below) - [x] No <!-- If yes, describe what needs to be changed downstream: --> --- ### 🔍️ Additional Notes - Need to discuss weather we still want to support `targetPathHeader` [[link](https://github.com/v3io/scaler/blob/development/pkg/dlx/handler.go#L47)]
1 parent 01b3274 commit 4e44dbc

File tree

2 files changed

+42
-24
lines changed

2 files changed

+42
-24
lines changed

pkg/dlx/handler.go

Lines changed: 15 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,6 @@ func NewHandler(parentLogger logger.Logger,
7979
}
8080

8181
func (h *Handler) handleRequest(res http.ResponseWriter, req *http.Request) {
82-
var path string
8382
var err error
8483
var resourceNames []string
8584

@@ -100,7 +99,7 @@ func (h *Handler) handleRequest(res http.ResponseWriter, req *http.Request) {
10099
resourceNames = append(resourceNames, resourceName)
101100
resourceTargetURLMap[resourceName] = targetURL
102101
} else {
103-
path, resourceNames, err = h.getPathAndResourceNames(req)
102+
resourceNames, err = h.getResourceNames(req)
104103
if err != nil {
105104
h.logger.WarnWith("Failed to get resource names and path from request",
106105
"error", err.Error(),
@@ -109,8 +108,11 @@ func (h *Handler) handleRequest(res http.ResponseWriter, req *http.Request) {
109108
res.WriteHeader(http.StatusBadRequest)
110109
return
111110
}
111+
// add the path addition to the path if the header exists
112+
// this is used when the ingress controller needs to add a suffix to the path request
113+
pathAddition := req.Header.Get(h.targetPathHeader)
112114
for _, resourceName := range resourceNames {
113-
targetURL, status := h.parseTargetURL(resourceName, path)
115+
targetURL, status := h.parseTargetURL(resourceName, pathAddition)
114116
if targetURL == nil {
115117
res.WriteHeader(status)
116118
return
@@ -166,11 +168,11 @@ func (h *Handler) handleRequest(res http.ResponseWriter, req *http.Request) {
166168
proxy.ServeHTTP(res, req)
167169
}
168170

169-
func (h *Handler) getPathAndResourceNames(req *http.Request) (string, []string, error) {
170-
// first try to get the resource names and path from the ingress cache
171-
path, resourceNames, err := h.getValuesFromCache(req)
171+
func (h *Handler) getResourceNames(req *http.Request) ([]string, error) {
172+
// first try to get the resource names from the ingress cache
173+
resourceNames, err := h.getValuesFromCache(req)
172174
if err == nil {
173-
return path, resourceNames, nil
175+
return resourceNames, nil
174176
}
175177

176178
h.logger.DebugWith("Failed to get resource names from ingress cache, trying to extract from the request headers",
@@ -180,27 +182,26 @@ func (h *Handler) getPathAndResourceNames(req *http.Request) (string, []string,
180182

181183
// old implementation for backward compatibility
182184
targetNameHeaderValue := req.Header.Get(h.targetNameHeader)
183-
path = req.Header.Get(h.targetPathHeader)
184185
if targetNameHeaderValue == "" {
185-
return "", nil, errors.New("No target name header found")
186+
return nil, errors.New("No target name header found")
186187
}
187188
resourceNames = strings.Split(targetNameHeaderValue, ",")
188-
return path, resourceNames, nil
189+
return resourceNames, nil
189190
}
190191

191-
func (h *Handler) getValuesFromCache(req *http.Request) (string, []string, error) {
192+
func (h *Handler) getValuesFromCache(req *http.Request) ([]string, error) {
192193
host := req.Host
193194
path := h.getRequestURLPath(req)
194195
resourceNames, err := h.ingressCache.Get(host, path)
195196
if err != nil {
196-
return "", nil, errors.New("Failed to get resource names from ingress cache")
197+
return nil, errors.New("Failed to get resource names from ingress cache")
197198
}
198199

199200
if len(resourceNames) == 0 {
200-
return "", nil, errors.New("No resources found in ingress cache")
201+
return nil, errors.New("No resources found in ingress cache")
201202
}
202203

203-
return path, resourceNames, nil
204+
return resourceNames, nil
204205
}
205206

206207
func (h *Handler) parseTargetURL(resourceName, path string) (*url.URL, int) {

pkg/dlx/handler_test.go

Lines changed: 27 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,8 @@ func (suite *HandlerTestSuite) SetupTest() {
4444
resourceReadinessTimeout: 3 * time.Second,
4545
}
4646
allowedPaths := map[string]struct{}{
47-
"/test/path/test/path": {},
48-
"/test/path/to/multiple/test/path/to/multiple": {},
47+
"/test/path": {},
48+
"/test/path/to/multiple": {},
4949
}
5050
// Start a test server that always returns 200
5151
suite.httpServer = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
@@ -130,13 +130,34 @@ func (suite *HandlerTestSuite) TestHandleRequest() {
130130
name: "Request headers flow",
131131
reqHeaders: map[string]string{
132132
"X-Forwarded-Host": "127.0.0.1",
133-
"X-Original-Uri": "test/path",
133+
"X-Original-Uri": "",
134134
"X-Resource-Name": "test-targets-name-1",
135135
},
136136
reqHost: "www.example.com",
137137
reqPath: "test/path",
138138
expectedStatus: http.StatusOK,
139139
},
140+
{
141+
name: "Request headers negative flow - duplicate request path should fail",
142+
reqHeaders: map[string]string{
143+
"X-Forwarded-Host": "127.0.0.1",
144+
"X-Original-Uri": "test/path",
145+
"X-Resource-Name": "test-targets-name-1",
146+
},
147+
reqHost: "www.example.com",
148+
reqPath: "test/path",
149+
expectedStatus: http.StatusBadRequest,
150+
},
151+
{
152+
name: "Request headers flow with resource path header- should fail because path is duplicated",
153+
reqHeaders: map[string]string{
154+
"X-Resource-Name": "test-targets-name-1",
155+
"X-Resource-Path": "test/path",
156+
},
157+
reqHost: "www.example.com",
158+
reqPath: "test/path",
159+
expectedStatus: http.StatusBadRequest,
160+
},
140161
} {
141162
suite.Run(testCase.name, func() {
142163
// test case setup
@@ -166,7 +187,6 @@ func (suite *HandlerTestSuite) TestGetPathAndResourceNames() {
166187
reqHost string
167188
reqPath string
168189
expectErr bool
169-
expectedPath string
170190
expectedResourceNames []string
171191
}{
172192
{
@@ -178,13 +198,11 @@ func (suite *HandlerTestSuite) TestGetPathAndResourceNames() {
178198
},
179199
reqHost: "www.example.com",
180200
reqPath: "test/path",
181-
expectedPath: "test/path",
182201
expectedResourceNames: []string{"test-targets-name-1"},
183202
}, {
184203
name: "request headers, host and path did not found in ingress cache",
185204
reqHost: "www.example.com",
186205
reqPath: "test/path",
187-
expectedPath: "test/path",
188206
expectedResourceNames: []string{"test-targets-name-1"},
189207
reqHeaders: map[string]string{
190208
"X-Resource-Name": "test-targets-name-1",
@@ -209,7 +227,6 @@ func (suite *HandlerTestSuite) TestGetPathAndResourceNames() {
209227
"X-Resource-Name": "test-targets-from-headers",
210228
"X-Resource-Path": "test/path",
211229
},
212-
expectedPath: "test/path",
213230
expectedResourceNames: []string{"test-targets-from-cache"},
214231
},
215232
{
@@ -230,15 +247,14 @@ func (suite *HandlerTestSuite) TestGetPathAndResourceNames() {
230247
testHandler, err := suite.createTestHandlerAndInitTestCache(suite.backendPort, testCase.initialCachedData)
231248
suite.Require().NoError(err)
232249
testRequest := suite.createTestHTTPRequest(testCase.name, testCase.reqHeaders, testCase.reqHost, testCase.reqPath)
233-
resultPath, resultResourceNames, err := testHandler.getPathAndResourceNames(testRequest)
250+
resultResourceNames, err := testHandler.getResourceNames(testRequest)
234251

235252
// validate the result
236253
if testCase.expectErr {
237254
suite.Require().Error(err)
238255
suite.Require().ErrorContains(err, testCase.errMsg)
239256
} else {
240257
suite.Require().NoError(err)
241-
suite.Require().Equal(testCase.expectedPath, resultPath)
242258
suite.Require().Equal(testCase.expectedResourceNames, resultResourceNames)
243259
}
244260
})
@@ -308,7 +324,8 @@ func (suite *HandlerTestSuite) setScalerMocksBasedOnTestCase(
308324
case "No request headers, scaler fails":
309325
suite.scaler.On("ResolveServiceName", mock.Anything).Return(suite.backendHost, resolveServiceNameErr)
310326
case "No request headers, not found in ingress cache":
311-
case "Request headers flow":
327+
case "Request headers flow",
328+
"Request headers negative flow - duplicate request path should fail":
312329
suite.scaler.On("SetScaleCtx", mock.Anything, mock.Anything, mock.Anything).Return(nil)
313330
default:
314331
suite.scaler.On("ResolveServiceName", mock.Anything).Return(suite.backendHost, resolveServiceNameErr)

0 commit comments

Comments
 (0)