[API] Consider X-Forwarded-For when setting Node ip_address on first ping
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.
- 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
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.
#4 Updated by Brett Smith over 3 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
#10 Updated by Radhika Chippada over 3 years ago
#11 Updated by Peter Amstutz over 3 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
#12 Updated by Tom Clegg over 3 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"