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

s3: handle 307 redirects #716

Closed
wants to merge 1 commit into from
Closed
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
2 changes: 2 additions & 0 deletions .github/workflows/test-nginx.yml
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,5 @@ jobs:
run: docker logs test-service-1
- if: always()
run: docker logs test-enketo-1
- if: always()
run: docker logs test-mock_s3-1
alxndrsn marked this conversation as resolved.
Show resolved Hide resolved
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
9 changes: 9 additions & 0 deletions test/mock-http-server/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,15 @@ app.get('/reset', withStdLogging((req, res) => {
res.json('OK');
}));

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
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