Bug #8198

[API] Consider X-Forwarded-For when setting Node ip_address on first ping

Added by Brett Smith over 3 years ago. Updated over 3 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Radhika Chippada
Category:
API
Target version:
Start date:
01/12/2016
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
1.0

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.

Subtasks

Task #8271: Review branch: 8198-node-ip-addressResolvedPeter Amstutz

Associated revisions

Revision cc79feaa
Added by Radhika Chippada over 3 years ago

closes #8198
Merge branch '8198-node-ip-address'

History

#1 Updated by Tom Clegg over 3 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

#2 Updated by Brett Smith over 3 years ago

  • Target version set to Arvados Future Sprints

#3 Updated by Brett Smith over 3 years ago

  • Story points set to 0.5

Command sounds good, as long as it works on CentOS 6.

#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

#5 Updated by Brett Smith over 3 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.

#6 Updated by Tom Clegg over 3 years ago

  • Description updated (diff)

#7 Updated by Brett Smith over 3 years ago

  • Assigned To set to Radhika Chippada

#8 Updated by Brett Smith over 3 years ago

  • Target version changed from Arvados Future Sprints to 2016-02-03 Sprint

#9 Updated by Radhika Chippada over 3 years ago

  • Status changed from New to In Progress

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

#13 Updated by Peter Amstutz over 3 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.

#14 Updated by Radhika Chippada over 3 years ago

  • Status changed from In Progress to Resolved
  • % Done changed from 0 to 100

Applied in changeset arvados|commit:cc79feaa11ec7436bf5f94ac9e3db4ed76bdc4c9.

Also available in: Atom PDF