PUBAPI-1291: primaryIp's logic could be better

Details

Issue Type:Bug
Priority:3 - Elevated
Status:Resolved
Created at:2016-05-10T04:37:33.000Z
Updated at:2017-02-25T01:10:06.000Z

People

Created by:Michael Zeller
Reported by:Michael Zeller
Assigned to:Marsell Kukuljevic

Resolution

Fixed: A fix for this issue is checked into the tree and tested.
(Resolution Date: 2017-02-25T01:10:06.000Z)

Fix Versions

2017-03-02 KENYA OPTION (Release Date: 2017-03-02)

Related Issues

Description

On my home SDC standup I noticed 'triton ssh alias' wasn't working because the primaryIp is not set.

I managed to dig up this thread from the mailing list:
https://www.listbox.com/member/archive/247449/2016/01/sort/subj/page/4/entry/4:85/20160127170534:1458CF52-C542-11E5-BFC2-B5E6F010038B/

Looking at the code it seems like we should check nic.primary rather than nic_tag. https://github.com/joyent/sdc-cloudapi/blob/master/lib/machines.js#L168

Making this change locally makes cloudapi/triton work as expected. What I am not sure about is previous versions of napi reporting the primary field.

Comments

Comment by trent.mick
Created at 2016-05-12T22:42:26.000Z

Discussed with Cody. Just using the "primary:true" NIC isn't what we want.

tl;dr: The "primaryIp" spec should be: If there is a NIC with primary: true, and it has a non-RFC1918 (private) IPv4 address, select that. Otherwise, pick an IPv4 address from the first NIC that has non-RFC1918 addresses.

Some other notes:


Comment by trent.mick
Created at 2016-05-12T22:42:51.000Z

@mike.zeller Want to volunteer to poke at a patch for this? Else you can drop yourself as the assignee.


Comment by trent.mick
Created at 2016-06-08T17:24:33.000Z

Discussion on #joyent:

bdha
10:14
Finally get back to this bug where primaryIp is returning a non-primary interface:
10:14
https://gist.github.com/bdha/952c8fc7395b4151cfc4b3bcc74f178f
10:15
Anyone have suggestions on what to kick?
trentm
10:19
bdha: any of PUBAPI-1238, PUBAPI-1286, or PUBAPI-1291 :|
bdha
10:22
hm.
10:22
What if all of my NICs are RFC1918?
10:22
(re 1291 specifically)
10:23
I think you should rely on the NIC tagged as primary, personally, but maybe my use case is weird.
trentm
10:24
given the name "primaryIp", I can't disagree with you
bdha
10:24
"The "primaryIp" spec should be: If there is a NIC with primary: true, and it has a non-RFC1918 (private) IPv4 address, select that. Otherwise, pick an IPv4 address from the first NIC that has non-RFC1918 addresses."
10:24
^-- Then I disagree with that.
trentm
10:24
yah


Comment by trent.mick
Created at 2016-09-02T19:16:22.000Z
Updated at 2016-11-25T18:00:36.000Z

Long discussion today on this: https://jabber.joyent.com/logs/mib@conference.joyent.com/2016/09/02.html#18:24:02.139839

An attempted summary of ^^:


Comment by trent.mick
Created at 2016-11-25T18:01:54.000Z

Another hit of this on sdc-discuss: https://www.listbox.com/member/archive/247449/2016/11/sort/time_rev/page/1/entry/2:91/20161124223923:BF3E5B1E-B2C0-11E6-B8B2-8A430559514B/


Comment by Marsell Kukuljevic
Created at 2016-11-29T10:51:49.000Z

A proper fix seems like it could get a bit involved. This is a possible short-term improvement: https://cr.joyent.us/#/c/965/

It still checks for the external nic_tag first, since I'd rather not break anything that currently works using that, but then next checks for a nic set as primary.


Comment by trent.mick
Created at 2016-11-29T17:39:26.000Z

^^^ I think we should just do the nic.primary check as we've discussed, instead of a middle ground, more complex change.

It would be nice to take a look at all the current VMs in JPC to see if/how many would get a changed 'primaryIp' by just changing to using nic.primary -- to get a feel for potential compat.


Comment by trent.mick
Created at 2016-11-29T19:26:22.000Z

However, thanks for pushing on this Marsell!

I discussed with Cody a bit more to try to cut to a concensus. Notes and a proposal:

History: 'primaryIp' was added to the cloudapi machine object waaay back (see PUBAPI-400, PROV-1333 for the original, and then PUBAPI-548 to revive it in the re-written cloudapi for SDC 7) to enable the user to tell which IP to use for ssh'ing in. Commonly in JPC one would get two IPs for a zone (one on an 'internal' DC-only network and one on the 'external', i.e. a public IP). The machine object would return both IPs in an 'ips' array as simple strings, so there wasn't a way to distinguish internal from external from just that data. Both then and now, it isn't documented whether there is a stable and logical order of the IPs in the 'ips' array. For reasons not given on the tickets above a 'primary_ip' (later "primaryIp") field was added to give the "primary" IP to cloudapi users (notably at the time, the portal).

Now, coming ipv6 support (RFD 11, RFD 32) and CNS (RFD 1) provide wrinkles. Providing a simple 'primaryIp' attribute and having tooling (e.g. command examples in portal, 'triton ssh ...') use that is problematic: Should 'primaryIp' return an IPv6 address if the primary NIC has one? If the primary NIC has both IPv4 and IPv6 address, which should it prefer? A problem with preferring IPv6 is that it could mean the tooling breaks for a user on an IPv4-only host. With CNS a potentially better answer is that tooling use the CNS hostname. However, there are multiple 'dns_names' provided and (at least currently, IIUC the comments from Alex) some can be for non-primary networks and not typically routable and the order in 'dns_names' isn't specified.

Proposal:

tl;dr: We should change "MACHINE.primaryIp" to be the IPv4 address of the NIC with primary=true. We should look at backward compatibility. Everything else is a separate ticket.


Comment by Marsell Kukuljevic
Created at 2016-11-29T20:52:27.000Z

I would love if we could just treat primaryIp = primary, but I am leery about removing the prior 'external' check. While looking at JPC is a good idea, that covers one installation of many. Part of cloudapi's job is to contain all the gungy crap so that internal systems can evolve freely while we present a fairly stable external API.

If we go with the primaryIp = primary in all cases, then I'd prefer we bumped the API version, keep CR 965 for version 8.0.0 and older, and use primaryIp = primary for anything newer.


Comment by Marsell Kukuljevic
Created at 2016-11-30T11:15:40.000Z

Nice research, thanks! Updated CR.


Comment by bot
Created at 2016-12-08T01:09:05.000Z

sdc-cloudapi commit 2a296b0 (branch master, by Marsell Kukuljevic)

PUBAPI-1291: primaryIp's logic could be better
Reviewed by: Trent Mick <trent.mick@joyent.com>


Comment by trent.mick
Created at 2017-02-25T00:29:32.000Z

@marsell Any reason not to close this now?