Bug #8198
closed
[API] Consider X-Forwarded-For when setting Node ip_address on first ping
Description
Problem: There's some Python at the bottom of the compute node install guide that creates the node record and sets up pings. It has some logic to detect the node's IP address. That logic does the wrong thing on nodes with a standard Debian-ish hosts setup: it'll get the IP address as 127.0.1.1 (from /etc/hosts), and create the node record with that. The script needs to be made smarter to choose a public IP address.
Solution:
- The API server already has code to set the node's IP address on first ping, if it's not already set. Extend this code to account for the X-Forwarded-For header so it works behind the Nginx proxy. (This might be as simple as changing
ENV['REMOTE_ADDR']→request.remote_ip; rails has built-in support for X-Forwarded-For, including a configurable list of trusted proxies that includes 127.0.0.1 by default.) - Add a controller test to match.
- Update the compute node install guide to remove the IP address detection and setting code from the Python script at the bottom.
Updated by Tom Clegg over 8 years ago
"Which interface would I use to send packets to the API server?" might be a reasonable way to choose.
$ ip route get `getent hosts "$ARVADOS_API_HOST" | awk '{ print $1 ; exit }'`| awk '$(NF - 1) == "src" { print $NF; exit }'
10.26.64.14
Updated by Brett Smith over 8 years ago
- Target version set to Arvados Future Sprints
Updated by Brett Smith over 8 years ago
- Story points set to 0.5
Command sounds good, as long as it works on CentOS 6.
Updated by Brett Smith over 8 years ago
- Subject changed from [Documentation] Compute node setup script doesn't choose right IP address from multiple to [API] Consider X-Forwarded-For when setting Node ip_address on first ping
- Description updated (diff)
- Category changed from Documentation to API
- Story points changed from 0.5 to 1.0
Updated by Brett Smith over 8 years ago
We got away from making this a docs-only change because it's very difficult to find a solution that's both portable and robust on the compute node side, with minimal dependencies.
Updated by Brett Smith over 8 years ago
- Target version changed from Arvados Future Sprints to 2016-02-03 Sprint
Updated by Radhika Chippada over 8 years ago
- Status changed from New to In Progress
Updated by Radhika Chippada over 8 years ago
Updated by Peter Amstutz about 8 years ago
Looks good to me, although the behavior tested by "future pings should not change previous ip address" seems strange that it would return success but not change the IP address. A better behavior would be one of:
a) trust that it is the same node, and update the IP address in the record
b) assume something is wrong, return an error and reject the ping entirely
Updated by Tom Clegg about 8 years ago
"Ignore reported/discovered IP address if IP address is already known" is the existing behavior, right? I agree there are potential pitfalls with that approach, but changing it seems out of scope here. (Is there any particular reason to reconsider it now, other than that it's now being tested?)
The current behavior does have some redeeming features, by the way:- If two nodes somehow end up fighting over the same record/secret, we don't flap
- If the node starts talking to us through a different interface, we don't erroneously mark the node "down / no recent ping"
Updated by Peter Amstutz about 8 years ago
Ok, since it's not currently causing problems (although I suspect that has more to do with the fact that addresses don't change in practice than the correctness of the current behavior) then we don't need to tinker with it. Rest of the branch LGTM.
Updated by Radhika Chippada about 8 years ago
- Status changed from In Progress to Resolved
- % Done changed from 0 to 100
Applied in changeset arvados|commit:cc79feaa11ec7436bf5f94ac9e3db4ed76bdc4c9.