Skip to content

Commit

Permalink
s3: handle 307 redirects
Browse files Browse the repository at this point in the history
  • Loading branch information
alxndrsn authored and alxndrsn committed Sep 20, 2024
1 parent b8572ab commit ec65b68
Show file tree
Hide file tree
Showing 6 changed files with 67 additions and 3 deletions.
9 changes: 9 additions & 0 deletions .github/workflows/test-nginx.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,12 @@ jobs:
node-version: 20
- run: cd test && npm i
- run: cd test && ./run-tests.sh

- if: always()
run: docker logs test-nginx-1
- if: always()
run: docker logs test-service-1
- if: always()
run: docker logs test-enketo-1
- if: always()
run: docker logs test-mock_s3-1
16 changes: 16 additions & 0 deletions files/nginx/odk.conf.template
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,22 @@ server {
proxy_request_buffering on;
proxy_buffering off;
proxy_read_timeout 2m;

# transparently follow 307 redirects
# See: https://serverfault.com/a/792035
proxy_intercept_errors on;
error_page 307 = @follow_redirect_transparently;
}

location @follow_redirect_transparently {
# TODO 127.0.0.11 is docker DNS, and may not be required in production
# TODO 8.8.8.8 is google DNS, and may not be palatable to some users
# TODO resolver config will need testing IRL
# See: https://nginx.org/en/docs/http/ngx_http_core_module.html#resolver
# See: https://stackoverflow.com/a/40331256
resolver 127.0.0.11 8.8.8.8;
set $hdr_location '$upstream_http_location';
proxy_pass '$hdr_location';
}

location / {
Expand Down
15 changes: 12 additions & 3 deletions test/mock-http-server/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,17 @@ app.get('/reset', withStdLogging((req, res) => {
res.json('OK');
}));

app.get('/*', logRequestType('GET'));
app.post('/*', logRequestType('POST'));
app.get('/blob-server/*', withStdLogging((req, res) => {
res.send(`blob:${req.path.replace('/blob-server/', '')}`);
}));

app.get('/*blob/*', withStdLogging((req, res) => {
// NOTE this may require tweaking when reality of using real nginx server is understood.
res.redirect(307, 'http://mock_s3:33333/blob-server/' + req.path.replace(/.*blob\//, ''));
}));

app.get('/*', ok('GET'));
app.post('/*', ok('POST'));
// TODO add more methods as required

app.listen(port, () => {
Expand All @@ -29,7 +38,7 @@ function withStdLogging(fn) {
};
}

function logRequestType(method) {
function ok(method) {
return withStdLogging((req, res) => {
requests.push({ method, path:req.path });
res.send('OK');
Expand Down
8 changes: 8 additions & 0 deletions test/nginx.test.docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,21 @@ services:
- "8383:8383"
environment:
- PORT=8383
mock_s3:
build:
dockerfile: mock-http-service.dockerfile
ports:
- "33333:33333"
environment:
- PORT=33333
nginx:
build:
context: ..
dockerfile: ./test/nginx-test.dockerfile
depends_on:
- service
- enketo
- mock_s3
environment:
- DOMAIN=odk-nginx.example.test
- SENTRY_ORG_SUBDOMAIN=example
Expand Down
2 changes: 2 additions & 0 deletions test/run-tests.sh
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ log "Waiting for mock backend..."
wait_for_http_response 5 localhost:8383/health 200
log "Waiting for mock enketo..."
wait_for_http_response 5 localhost:8005/health 200
log "Waiting for mock s3..."
wait_for_http_response 5 localhost:33333/health 200
log "Waiting for nginx..."
wait_for_http_response 90 localhost:9000 301

Expand Down
20 changes: 20 additions & 0 deletions test/test-nginx.js
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,26 @@ describe('nginx config', () => {
{ method:'GET', path:'/v1/some/central-backend/path' },
);
});

it('should transparently follow backend 307 redirects', async () => {
// when
const res = await fetchHttps('/v1/blob/some-bucket/some-id');

// then
assert.isTrue(res.ok);
assert.equal(res.status, 200);
assert.equal(await res.text(), 'blob:some-bucket/some-id');
});

it('should not modify enketo 307 redirects', async () => {
// when
const res = await fetchHttps('/-/blob/some-bucket/some-id');

// then
assert.isFalse(res.ok);
assert.equal(res.status, 307);
assert.equal(await res.headers.get('location'), 'http://mock_s3:33333/blob-server/some-bucket/some-id');
});
});

function fetchHttp(path, options) {
Expand Down

0 comments on commit ec65b68

Please sign in to comment.