Display facts for aliases in *BSD #731

kudu-rex · 2014-07-16T16:20:53Z

interfaces.rb - aliases on *BSD are iterated over up to their actual count - slightly modified Michelle Sullivan's version
util/ip.rb - added get_alias_no method to return alias count; method get_interface_value modified by Michelle Sullivan to allow obtaining values for specific aliases

Originated here:
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=190796

…l count * util/ip.rb - added get_alias_no method to return alias count; method get_interface_value modified by Michelle Sullivan to allow obtaining values for specific aliases

adrienthebo · 2014-07-16T18:04:31Z

@nulldowntime are you the original author of this code? Looking at the FreeBSD ticket indicates that there was code implemented for this earlier and in order to accept this we need a CLA signed from the original author before we can accept it.

adrienthebo · 2014-07-16T18:07:13Z

@jasperla could you eyeball this pull request and see if it's suitable to OpenBSD as well?

adrienthebo · 2014-07-16T18:08:23Z

lib/facter/util/ip.rb

+  # @return [Integer] representing the requested value.  0 is returned if the
+  # kernel is not supported by the REGEX_MAP constant and on Windows.
+  def self.get_alias_no(interface, label)
+    if Facter.value(:kernel) == 'windows'


Is this code guaranteed to be correct on Linux, Solaris, AIX, and other platforms as well?

Linux, AIX, Solaris alias interfaces are seen as completely new interfaces, this will not affect any platform that sees an interface alias as a new interface.

eg:
Linux sees: ipaddress_eth0:0
Solaris sees: ipaddress_hme0.1
etc..

adrienthebo · 2014-07-16T18:09:54Z

Is there an issue on tickets.puppetlabs.com associated with this pull request?

kudu-rex · 2014-07-16T18:12:12Z

Hi,

I've modified someone else's code, so it's only partially mine (the new
method is "mine"). I've mentioned whose code I modified in the pull request
comment. I don't know this person, but she mentions she has sent a pull
request upstream. I've not been able to find her pull request on github
though, so maybe it was a patch in a ticket?

Regards,
Daniel
On 16 Jul 2014 19:04, "Adrien Thebo" notifications@github.com wrote:

@nulldowntime https://github.com/nulldowntime are you the original
author of this code? Looking at the FreeBSD ticket indicates that there was
code implemented for this earlier and in order to accept this we need a CLA
signed from the original author before we can accept it.

—
Reply to this email directly or view it on GitHub
#731 (comment).

puppetcla · 2014-07-16T19:00:16Z

Waiting for CLA signature by @nulldowntime

@nulldowntime - We require a Contributor License Agreement (CLA) for people who contribute to Puppet, but we have an easy click-through license with instructions, which is available at https://cla.puppetlabs.com/

Note: if your contribution is trivial and you think it may be exempt from the CLA, please post a short reply to this comment with details. http://docs.puppetlabs.com/community/trivial_patch_exemption.html

kudu-rex · 2014-07-16T19:54:30Z

OK, CLA signed.

Regards,
Daniel

On 16 July 2014 20:00, puppetcla notifications@github.com wrote:

Waiting for CLA signature by @nulldowntime
https://github.com/nulldowntime

@nulldowntime https://github.com/nulldowntime - We require a
Contributor License Agreement (CLA) for people who contribute to Puppet,
but we have an easy click-through license with instructions, which is
available at https://cla.puppetlabs.com/

Note: if your contribution is trivial and you think it may be exempt from
the CLA, please post a short reply to this comment with details.
http://docs.puppetlabs.com/community/trivial_patch_exemption.html

—
Reply to this email directly or view it on GitHub
#731 (comment).

puppetcla · 2014-07-16T20:00:16Z

CLA signed by all contributors.

jasperla · 2014-07-16T20:18:56Z

Ill test this on OpenBSD tomorrow morning.

adrienthebo · 2014-07-16T21:21:03Z

Looking at the FreeBSD bug report made me notice this comment:

Patch to add interface alias support for the first 10 aliases (limited to 10 because I can't work out how to abort the loop when it gets to an alias that doesn't exist without writing a huge amount of code - which is more likely to break)

@nulldowntime did you address this in your patches to the original change?

kudu-rex · 2014-07-16T21:41:30Z

My code doesn't iterate from 0 to 9 but uses a new method that returns
actual number of aliases that ifconfig sees. So it won't reach "an alias
that doesn't exist". Unless of course something deletes an alias between
obtaining a number of aliases present and actually trying to read its
values. But that's one of those things that "should never happen", right?

Regards,
Daniel

On 16 July 2014 22:21, Adrien Thebo notifications@github.com wrote:

Looking at the FreeBSD bug report made me notice this comment:

Patch to add interface alias support for the first 10 aliases (limited to
10 because I can't work out how to abort the loop when it gets to an alias
that doesn't exist without writing a huge amount of code - which is more
likely to break)

@nulldowntime https://github.com/nulldowntime did you address this in
your patches to the original change?

—
Reply to this email directly or view it on GitHub
#731 (comment).

adrienthebo · 2014-07-16T21:43:46Z

The sort of race condition where an alias is deleted while interfaces are being enumerated is so unlikely that we don't need to worry about it, thanks for clarifying that!

adrienthebo · 2014-07-16T21:44:38Z

lib/facter/util/ip.rb

  # @api private
  #
  # @return [String] representing the requested value.  An empty array is
  # returned if the kernel is not supported by the REGEX_MAP constant.
-  def self.get_interface_value(interface, label)
+  def self.get_interface_value(interface, label, intalias = -1)


Why is -1 used as the starting index of the interface aliases?

It's not "my piece", but I suppose the reason would be that traditionally aliases in FreeBSD are (and have to be) configured in /etc/rc.conf beginning with 0. So alias0 makes sense and corresponds to something that HAS to exist if you're using aliases. It's only an "associative" as ifconfig doesn't actually list "alias0, alias1, alias2", but for anyone comparing facter output with what's configured via /etc/rc.conf it makes sense.

This is correct, however just for reference - the order of adding ( set in /etc/rc.conf by the numeric suffix of the alias definition ) will result in the alias order (alias 0..n ) .. Whilst specifying _alias0 _alias3 in /etc/rc.conf will work, it's highly frowned on as many scripts reading the config will not process correctly (particularly if they are trying to match to 'live' aliases)

adrienthebo · 2014-07-16T23:35:47Z

The original author has signed the CLA so we can go ahead with this pull request.

jasperla · 2014-07-17T07:26:28Z

Seems to be doing the right thing on OpenBSD too:

ipaddress6_trunk0_alias0 => 2001:981:4571:1:6446:e64a:60ff:f4fa
ipaddress_trunk0_alias0 => 192.168.178.199
netmask_trunk0_alias0 => 255.255.255.0

adrienthebo · 2014-07-17T20:56:11Z

@jasperla thanks for the verification!

bartekrutkowski · 2014-07-21T14:18:19Z

Since I own a FreeBSD's bug that relies on this code being merged, I wonder is there any ETA for merging it and releasing Facter update with that code?

adrienthebo · 2014-07-21T16:12:03Z

@bartekrutkowski before we merge this we need to have unit tests added first; I've been working on tests myself but if anyone else is feeling intrepid some help would be appreciated.

adrienthebo · 2014-07-21T16:37:07Z

@bartekrutkowski I failed to ask this earlier - do you have a specific time that you need this to be handled or are you just making sure this has forward momentum?

bartekrutkowski · 2014-07-21T22:22:06Z

The reason I am asking is becasue of two things mostly - this functionality is needed in Facter for BSD users and the general policy is to not to maintain our own set of patches if they can be accepted by upstream (in this case, you guys). This means my hands are tied until you accept the patch and release new version so I could update our port of Facter, and I am interested if this is happening, and if it does, then when, because if this is going to be merged in version that's far away from us, it might have some sense to maintain the patch in Ports tree unless the version having it is released. Hope that helps!

adrienthebo · 2014-07-22T19:09:11Z

lib/facter/util/ip.rb

+  #
+  # @return [Integer] representing the requested value.  0 is returned if the
+  # kernel is not supported by the REGEX_MAP constant and on Windows.
+  def self.get_alias_no(interface, label)


If this method is simply enumerating the number of addresses defined for a given interface, why do we need to scan for a user specified label? Given output like so:

lo1: flags=8049<UP,LOOPBACK,RUNNING,MULTICAST> metric 0 mtu 16384 >.......options=600003<RXCSUM,TXCSUM,RXCSUM_IPV6,TXCSUM_IPV6> >.......inet 10.0.2.21 netmask 0xffffffff. >.......inet 10.0.2.22 netmask 0xffffffff. >.......inet 10.0.2.23 netmask 0xffffffff. >.......inet 10.0.2.24 netmask 0xffffffff. >.......inet 10.0.2.25 netmask 0xffffffff. >.......inet 10.0.2.26 netmask 0xffffffff. >.......inet 10.0.2.27 netmask 0xffffffff. >.......inet 10.0.2.28 netmask 0xffffffff. >.......inet 10.0.2.29 netmask 0xffffffff. >.......inet 10.0.2.30 netmask 0xffffffff

Can't we just count the number of times inet is specified?

Hi,

There can be a different number of ip and ipv6 aliases, hence the label is
left there.

Regards,
Daniel
On 22 Jul 2014 20:09, "Adrien Thebo" notifications@github.com wrote:

In lib/facter/util/ip.rb:

end end

end

get_alias_no obtains alias count for a specified interface and label

@param interface [String] the interface identifier, e.g. "eth0" or "bond0"

@param label [String] the attribute of the interface to obtain a value for,

e.g. "netmask" or "ipaddress" (note eg. ipaddress and ipaddress6 can have different

number of aliases, so label does make sense here)

@api private

@return [Integer] representing the requested value. 0 is returned if the

kernel is not supported by the REGEX_MAP constant and on Windows.

def self.get_alias_no(interface, label)

If this method is simply enumerating the number of addresses defined for a
given interface, why do we need to scan for a user specified label? Given
output like so:

.......options=600003 .......inet 10.0.2.21 netmask 0xffffffff. .......inet 10.0.2.22 netmask 0xffffffff. .......inet 10.0.2.23 netmask 0xffffffff. .......inet 10.0.2.24 netmask 0xffffffff. .......inet 10.0.2.25 netmask 0xffffffff. .......inet 10.0.2.26 netmask 0xffffffff. .......inet 10.0.2.27 netmask 0xffffffff. .......inet 10.0.2.28 netmask 0xffffffff. .......inet 10.0.2.29 netmask 0xffffffff. .......inet 10.0.2.30 netmask 0xffffffff Can't we just count the number of times `inet` is specified? — Reply to this email directly or view it on GitHub https://github.com/puppetlabs/facter/pull/731/files#r15248979.

Using a regex like /inet/ would match both inet and inet6. My concern is that this method looks like it largely duplicates the functionality of .get_interface_value, and if there's a lot of overlap I would like to know if there's extra code that we can trim from the function.

Hi,

It is a trimmed copy, so duplication is there. I was thinking about moving
common parts out to a separate method, but didn't want to change the file
too much.

As for ipv4 and ipv6 aliases, they are meant to be separate I believe. One
will be ipaddress_alias and the other ipaddress6_alias and they are
enumerated separately. But I didn't really test ipv6. That sounds intuitive
to me, but that's an opinion.

rc.conf manual specifies:

Aliases should be set by ifconfig___<interface>___alias<n> with
``inet6'' keyword. For example: ifconfig_ed0_ipv6="inet6 2001:db8:1::1
prefixlen 64" ifconfig_ed0_alias0="inet6 2001:db8:2::1 prefixlen 64"

which doesn't help in this situation (ipv6 interface is configured
separately to ipv4, but ipv6 aliases use the same keyword as ipv4).

Regards,
Daniel

Regards,
Daniel
On 22 Jul 2014 20:19, "Adrien Thebo" notifications@github.com wrote:

In lib/facter/util/ip.rb:

end end

end

get_alias_no obtains alias count for a specified interface and label

@param interface [String] the interface identifier, e.g. "eth0" or "bond0"

@param label [String] the attribute of the interface to obtain a value for,

e.g. "netmask" or "ipaddress" (note eg. ipaddress and ipaddress6 can have different

number of aliases, so label does make sense here)

@api private

@return [Integer] representing the requested value. 0 is returned if the

kernel is not supported by the REGEX_MAP constant and on Windows.

def self.get_alias_no(interface, label)

Using a regex like /inet/ would match both inet and inet6. My concern is
that this method looks like it largely duplicates the functionality of
.get_interface_value, and if there's a lot of overlap I would like to
know if there's extra code that we can trim from the function.

—
Reply to this email directly or view it on GitHub
https://github.com/puppetlabs/facter/pull/731/files#r15249598.

Alright, those are good points. Refactoring into a common method would be good.

adrienthebo · 2014-07-25T17:35:33Z

@bartekrutkowski this would be released in Facter 2.2.0 which is going to be a couple of months away or so, so it make sense to carry a downstream patch in the meantime. I would like to get test coverage added and reduce the amount of duplication in this change before we pull it in to core, so it's your call as to whether you want to pull in the existing pull request or use the final version that we merge.

@nulldowntime I would love to get this pull request merged in sooner rather than later, but it would be a big help if you could take a swing at some of the comments I've left - do you think you'll have time to work on this in the next week or so?

kudu-rex · 2014-07-25T17:57:21Z

Hi,

I should be able to find some time, yes.

Regards,
Daniel
On 25 Jul 2014 18:35, "Adrien Thebo" notifications@github.com wrote:

@bartekrutkowski https://github.com/bartekrutkowski this would be
released in Facter 2.2.0 which is going to be a couple of months away or
so, so it make sense to carry a downstream patch in the meantime. I would
like to get test coverage added and reduce the amount of duplication in
this change before we pull it in to core, so it's your call as to whether
you want to pull in the existing pull request or use the final version that
we merge.

@nulldowntime https://github.com/nulldowntime I would love to get this
pull request merged in sooner rather than later, but it would be a big help
if you could take a swing at some of the comments I've left - do you think
you'll have time to work on this in the next week or so?

—
Reply to this email directly or view it on GitHub
#731 (comment).

adrienthebo · 2014-08-06T18:13:27Z

@nulldowntime have you had time to do more work on this?

jpartlow · 2014-08-20T19:01:14Z

Since this hasn't seen an update this month, I'm closing this pull request for the time being; if you would like to continue work on this please comment on this pull request and we can reopen it.

misullivan · 2015-01-03T10:11:59Z

Did this ever end up in the main code base?

mmoll · 2015-05-03T21:47:21Z

I would say no

* interfaces.rb - aliases on *BSD are iterated over up to their actua…

7718bd9

…l count * util/ip.rb - added get_alias_no method to return alias count; method get_interface_value modified by Michelle Sullivan to allow obtaining values for specific aliases

adrienthebo reviewed Jul 16, 2014
View reviewed changes

adrienthebo reviewed Jul 22, 2014
View reviewed changes

jpartlow closed this Aug 20, 2014

Display facts for aliases in *BSD #731

Display facts for aliases in *BSD #731

Conversation

kudu-rex commented Jul 16, 2014

adrienthebo commented Jul 16, 2014

adrienthebo commented Jul 16, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrienthebo commented Jul 16, 2014

kudu-rex commented Jul 16, 2014

puppetcla commented Jul 16, 2014

kudu-rex commented Jul 16, 2014

puppetcla commented Jul 16, 2014

jasperla commented Jul 16, 2014

adrienthebo commented Jul 16, 2014

kudu-rex commented Jul 16, 2014

adrienthebo commented Jul 16, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrienthebo commented Jul 16, 2014

jasperla commented Jul 17, 2014

adrienthebo commented Jul 17, 2014

bartekrutkowski commented Jul 21, 2014

adrienthebo commented Jul 21, 2014

adrienthebo commented Jul 21, 2014

bartekrutkowski commented Jul 21, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

get_alias_no obtains alias count for a specified interface and label

@param interface [String] the interface identifier, e.g. "eth0" or "bond0"

@param label [String] the attribute of the interface to obtain a value for,

e.g. "netmask" or "ipaddress" (note eg. ipaddress and ipaddress6 can have different

number of aliases, so label does make sense here)

@api private

@return [Integer] representing the requested value. 0 is returned if the

kernel is not supported by the REGEX_MAP constant and on Windows.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

get_alias_no obtains alias count for a specified interface and label

@param interface [String] the interface identifier, e.g. "eth0" or "bond0"

@param label [String] the attribute of the interface to obtain a value for,

e.g. "netmask" or "ipaddress" (note eg. ipaddress and ipaddress6 can have different

number of aliases, so label does make sense here)

@api private

@return [Integer] representing the requested value. 0 is returned if the

kernel is not supported by the REGEX_MAP constant and on Windows.

Choose a reason for hiding this comment

adrienthebo commented Jul 25, 2014

kudu-rex commented Jul 25, 2014

adrienthebo commented Aug 6, 2014

jpartlow commented Aug 20, 2014

misullivan commented Jan 3, 2015

mmoll commented May 3, 2015