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

revert force-set of CORS header on cache misses #85

Merged
merged 1 commit into from
Feb 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,8 @@ up:
.PHONY: down
# stop the service and it's dependencies
down:
rm docker/shared/genesis.json
rm docker/shared/VALIDATOR_NODE_ID
rm docker/shared/genesis.json || echo no shared genesis present
rm docker/shared/VALIDATOR_NODE_ID || echo no shared validator node id present
docker compose down

.PHONY: restart
Expand Down
2 changes: 1 addition & 1 deletion ci.docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ services:
dockerfile: ci.Dockerfile
env_file: .env
environment:
PROXY_HEIGHT_BASED_ROUTING_ENABLED: true
PROXY_HEIGHT_BASED_ROUTING_ENABLED: "true"
# use public testnet as backend origin server to avoid having
# to self-host a beefy Github Action runner
# to build and run a kava node each execution
Expand Down
6 changes: 4 additions & 2 deletions main_batch_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -225,8 +225,10 @@ func TestE2ETest_ValidBatchEvmRequests(t *testing.T) {
// check expected cache status header
require.Equal(t, tc.expectedCacheHeader, resp.Header.Get(cachemdw.CacheHeaderKey))

// verify CORS header
require.Equal(t, resp.Header[accessControlAllowOriginHeaderName], []string{"*"})
// verify CORS header (only on cache hits. cache misses return the backend value, if set)
if tc.expectedCacheHeader == cachemdw.CacheHitHeaderValue {
require.Equal(t, resp.Header[accessControlAllowOriginHeaderName], []string{"*"})
}

// wait for all metrics to be created.
// besides verification, waiting for the metrics ensures future tests don't fail b/c metrics are being processed
Expand Down
2 changes: 1 addition & 1 deletion main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -525,7 +525,7 @@ func TestE2ETestCachingMdwWithBlockNumberParam(t *testing.T) {
expectKeysNum(t, redisClient, tc.keysNum)
expectedKey := "local-chain:evm-request:eth_getBlockByNumber:sha256:d08b426164eacf6646fb1817403ec0af5d37869a0f32a01ebfab3096fa4999be"
containsKey(t, redisClient, expectedKey)
require.Equal(t, cacheMissResp.Header[accessControlAllowOriginHeaderName], []string{"*"})
// don't check CORs because proxy only force-sets header for cache hits.

// eth_getBlockByNumber - cache HIT
cacheHitResp := mkJsonRpcRequest(t, proxyServiceURL, 1, tc.method, tc.params)
Expand Down
2 changes: 1 addition & 1 deletion service/batchmdw/batch_processor.go
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ func (bp *BatchProcessor) applyHeaders(h http.Header) {
// clear content length, will be set by actual Write to client
// must be cleared in order to prevent premature end of client read
bp.header.Del("Content-Length")
// clear cache hit header, will be set by flush()
// clear cache hit header, will be set by RequestAndServe()
bp.header.Del(cachemdw.CacheHeaderKey)
}

Expand Down
16 changes: 9 additions & 7 deletions service/middleware.go
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ func createDecodeRequestMiddleware(next http.HandlerFunc, batchProcessingMiddlew

// skip processing if there is no body content
if r.Body == nil {
serviceLogger.Trace().Msg("no data in request")
serviceLogger.Trace().Msg("no data in request body")
next.ServeHTTP(w, r)
return
}
Expand All @@ -119,6 +119,13 @@ func createDecodeRequestMiddleware(next http.HandlerFunc, batchProcessingMiddlew
// Repopulate the request body for the ultimate consumer of this request
r.Body = io.NopCloser(&rawBodyBuffer)

// skip processing if body is empty
if len(rawBody) == 0 {
serviceLogger.Trace().Msg("no data in request body")
next.ServeHTTP(w, r)
return
}

// attempt to decode as single EVM request
decodedRequest, err := decode.DecodeEVMRPCRequest(rawBody)
if err == nil {
Expand Down Expand Up @@ -263,7 +270,7 @@ func createProxyRequestMiddleware(next http.Handler, config config.Config, servi
w.Header().Set(headerName, headerValue)
}
}
// add CORS headers (if not already added)
// cached requests should have CORs set, but if they don't, default to configured value.
accessControlAllowOriginValue := config.GetAccessControlAllowOriginValue(r.Host)
if w.Header().Get("Access-Control-Allow-Origin") == "" && accessControlAllowOriginValue != "" {
w.Header().Set("Access-Control-Allow-Origin", accessControlAllowOriginValue)
Expand All @@ -281,11 +288,6 @@ func createProxyRequestMiddleware(next http.Handler, config config.Config, servi
Msg("cache miss")

w.Header().Add(cachemdw.CacheHeaderKey, cachemdw.CacheMissHeaderValue)
// add CORS headers (if not already added)
accessControlAllowOriginValue := config.GetAccessControlAllowOriginValue(r.Host)
if w.Header().Get("Access-Control-Allow-Origin") == "" && accessControlAllowOriginValue != "" {
w.Header().Set("Access-Control-Allow-Origin", accessControlAllowOriginValue)
}
Comment on lines -284 to -288
Copy link
Member Author

@pirtleshell pirtleshell Feb 21, 2024

Choose a reason for hiding this comment

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

this addition caused the CORs header to be set twice on live deployments (once here, then again from the backend). removing it gives full power to the backend to set all headers.

this is kept for cache hits (the above if block) because we want to ensure correct CORs headers are set for responses that get cached without them. tests are updated to only require CORs header on cache hit tests (locally they do not get the header, but in live deployments nginx sets it).

proxy.ServeHTTP(lrw, r)
}

Expand Down
Loading