Bug #8198
closed
[API] Consider X-Forwarded-For when setting Node ip_address on first ping
Added by Brett Smith about 9 years ago.
Updated about 9 years ago.
Assigned To:
Radhika Chippada
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.
"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
- Target version set to Arvados Future Sprints
Command sounds good, as long as it works on CentOS 6.
- 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
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.
- Description updated (diff)
- Assigned To set to Radhika Chippada
- Target version changed from Arvados Future Sprints to 2016-02-03 Sprint
- Status changed from New to In Progress
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
"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"
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.
- Status changed from In Progress to Resolved
- % Done changed from 0 to 100
Applied in changeset arvados|commit:cc79feaa11ec7436bf5f94ac9e3db4ed76bdc4c9.
Also available in: Atom
PDF
Updated by 
Updated by 
Updated by Radhika Chippada 
Updated by