Skip to content
This repository has been archived by the owner on Jan 7, 2018. It is now read-only.

Commit

Permalink
Fix handling of long hostnames.
Browse files Browse the repository at this point in the history
It was possible that nginx would bomb if an API backend was added with a
long hostname. The exact length that would trigger this error varied,
since nginx's default lengths actually varied depending on the CPU type.
But this should fix things so that we should properly support hostnames
up to 110 characters long regardless of CPU defaults.

Fixes NREL/api-umbrella#168
  • Loading branch information
GUI committed Sep 22, 2015
1 parent c42c274 commit c44ddf0
Show file tree
Hide file tree
Showing 4 changed files with 55 additions and 0 deletions.
25 changes: 25 additions & 0 deletions lib/config_reloader/worker.js
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,8 @@ _.extend(Worker.prototype, {
hosts['*'].hostname = '_';
}

var longestHostname = 0;

// Now that we know each of the hosts and each of the URL prefixes those
// hosts are responsible for, we can assemble the regex for matching the
// prefixes on each host which we'll then pass to nginx.
Expand All @@ -271,10 +273,33 @@ _.extend(Worker.prototype, {
hostConfig.api_url_prefixes_matcher = '^(' + hostConfig.api_url_prefixes.join('|') + ')';
delete hostConfig.api_url_prefixes;
}

if(hostConfig.hostname.length > longestHostname) {
longestHostname = hostConfig.hostname.length;
}
});

// Choose the nginx host bucket size based on the longest hostname that's
// in the system. Try to pick among the nginx default values, since I think
// there may be benefits with sticking with these sizes (to align with CPU
// cache sizes): http://nginx.org/en/docs/hash.html Max out at 128. We can
// revisit this max if needs be.
//
// For reasons I don't currently understand, a setting of 128 only supports
// a hostname 110 characters long. So add in a buffer so that we increase
// this setting's length even for shorter hostnames.
var hostnameBucketSize = 32;
var hostnameBucketBuffer = 18;
if(longestHostname >= 32 - hostnameBucketBuffer && longestHostname < 64 - hostnameBucketBuffer) {
hostnameBucketSize = 64;
} else if(longestHostname >= 64 - hostnameBucketBuffer) {
hostnameBucketSize = 128;
}

var templateConfig = _.extend({}, config.getAll(), {
hosts: _.values(hosts),

_server_names_hash_bucket_size: hostnameBucketSize,
});

var newContent = this.nginxFrontendTemplate(templateConfig);
Expand Down
2 changes: 2 additions & 0 deletions templates/etc/nginx/frontend_hosts.conf.hbs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
server_names_hash_bucket_size {{_server_names_hash_bucket_size}};

{{#each hosts}}
server {
listen {{../http_port}}{{#if default}} default_server{{/if}};
Expand Down
9 changes: 9 additions & 0 deletions test/config/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,15 @@ apis:
url_matches:
- frontend_prefix: /wildcard-dot-info/
backend_prefix: /info/
- _id: long-frontend-host
frontend_host: abcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghij
backend_host: abcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghij
servers:
- host: 127.0.0.1
port: 9444
url_matches:
- frontend_prefix: /long-host-info/
backend_prefix: /info/
- _id: wildcard-frontend-host
frontend_host: "*"
backend_host: localhost
Expand Down
19 changes: 19 additions & 0 deletions test/integration/proxying.js
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,25 @@ describe('proxying', function() {
done();
});
});

// I'm not exactly sure why, but when we set server_names_hash_bucket_size
// to 128, it actually only allows hostnames 110 characters long. Perhaps
// something to investigate or better understand, but in the meantime
// documenting current behavior.
it('supports hostname lengths up to 110 characters', function(done) {
var options = _.merge({}, this.options, {
headers: {
'Host': 'abcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghij',
},
});

request.get('http://localhost:9080/long-host-info/', options, function(error, response, body) {
response.statusCode.should.eql(200);
var data = JSON.parse(body);
data.headers.host.length.should.eql(110);
done();
});
});
});

// Ensure basic HTTP requests of all HTTP methods work with the entire stack
Expand Down

0 comments on commit c44ddf0

Please sign in to comment.