Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] IPaddr2: Proposal patch to support the dual stack of IPv4 and IPv6. #97

Closed
wants to merge 7 commits into from

Conversation

kskmori
Copy link

@kskmori kskmori commented May 31, 2012

This is a proposal enhancement of IPaddr2 to support IPv6 as well as IPv4.

I would appreciate your comments and suggestions for merging this into the upstream.

NOTE: This pull request is meant for reviewing the code and discussions, and not intended to be merged as is at this moment.

Benefits:

  • Unify the usage, behavior and the code maintenance between IPv4 and IPv6 on Linux.

    The usage of IPaddr2 and IPv6addr are similar but they have different parameters and different behaviors. In particular, they may choose a different interface depending on your configuration even if you provided similar parameters in the past.

    IPv6addr is written in C and rather hard to make improvements. As /bin/ip already supports both IPv4 and IPv6, we can share the most of the code of IPaddr2 written in bash.

  • usable for LVS on IPv6.

    IPv6addr does not support lvs_support=true and unfortunately there is no possible way to use LVS on IPv6 right now.

    IPaddr2(/bin/ip) works for LVS configurations without enabling lvs_support both for IPv4 and IPv6.

    (You don't have to remove an address on the loopback interface if the virtual address is assigned by using /bin/ip.)

    See also:
    http://www.gossamer-threads.com/lists/linuxha/dev/76429#76429

  • retire the old 'findif' binary.

    'findif' binary is replaced by a shell script version of findif, originally developed by lge.
    See findifcould be rewritten in shell #53 : findif could be rewritten in shell

  • easier support for other pending issues

    These pending issues can be fix based on this new IPaddr2.

Notes / Changes:

  • findif semantics changes

    There are some incompatibility in deciding which interface to be used when your configuration is ambiguous. But in reality it should not be a problem as long as it's configured properly.

    The changes mostly came from fixing a bug in the findif binary (returns a wrong broadcast) or merging the difference between (old)IPaddr2 and IPv6addr.
     See the ofct test cases for details. (case No.6, No.9, No.10, No.12, No.15 in IPaddr2v4 test cases)

    Other notable changes are described below.

  • "broadcast" parameter for IPv4

    "broadcast" parameter may be required along with "cidr_netmask" when you want use a different subnet mask from the static IP address. It's because doing such calculation is difficult in the shell
    script version of findif.

    See the ofct test cases for details. (case No.11, No.14, No.16, No.17 in IPaddr2v4 test cases)

    This limitation may be eliminated if we would remove brd options from the /bin/ip command line.

  • loopback(lo) now requires cidr_netmask or broadcast.

    See the ofct test case in the IPaddr2 ocft script. The reason is similar to the previous one.

  • loose error check for "nic" for a IPv6 link-local address.

    IPv6addr was able to check this, but in the shell script it is hard to determine a link-local address (requires bitmask calculation). I do not think it's worth to implement it in shell.

  • send_ua: a new binary

    We need one new binary as a replacement of send_arp for IPv6 support. IPv6addr.c is reused to make this command.

Note that IPv6addr RA is still there and you can continue to use it for the backward compatibility.

Acknowledgement

Thanks to Tomo Nozawa-san for his hard work for writing and testing this patch.

Thanks to Lars Ellenberg for the first findif.sh implementation.

…Pv6.

This patch is for reviewing and for discussions by the community
and not intended to be merged as is at this moment.
@ghost ghost assigned kskmori May 31, 2012
@fghaas
Copy link
Member

fghaas commented Jun 1, 2012

Thanks Mori-san!

One quick thought here:

 "broadcast" parameter may be required along with "cidr_netmask" when you want use a different subnet mask from the static IP address. It's because doing such calculation is difficult in the shell
 script version of findif.

Can ipcalc help there? Or in case that is Debian/Ubuntu specific, is
there some other (more common) binary that could work out the
broadcast address?

@kskmori
Copy link
Author

kskmori commented Jun 4, 2012

Thank you for your comments, Florian!

I found that ipcalc is available on Red Hat and CentOS (and probably Fedora too), but openSUSE does not seem to have ipcalc by the default installation as long as I can see. (I tried openSUSE 11.2 and 12.1)
A rpm package for SUSE can be found on the net, but I'm not familiar with SUSE very much and not sure if it's available on the standard installation. Do you happen to know that we can safely use ipcalc on SUSE?

Anyway I think that we can just remove brd option to /bin/ip and let /bin/ip calculate a broadcast. I will look into this way.

@m0n5t3r
Copy link
Contributor

m0n5t3r commented Jun 4, 2012

ipcalc is probably available in all Linux distros in one way or another, and probably on *BSD as well, but it's not in the default installation in some of them, so you'll need to add it to the list of dependencies if you use it

@kskmori
Copy link
Author

kskmori commented Jun 6, 2012

Hi,

ipcalc is probably available in all Linux distros in one way or another, and probably on *BSD as well, but it's not in the default installation in some of them, so you'll need to add it to the list of dependencies if you use it

I also found that there exists a several different implementations of ipcalc, and they have a different usage and a different output.

The one in Debian is written in Perl, the output is good for human but requires parsing in shell scripting. Another one in
Red Hat is a binary program but handy for shell scripting because the output can be directly eval'ed to set shell variables.

I think we should avoid to depend on ipcalc.

  • Red Hat
$ ipcalc -b 192.168.1.1 255.255.255.0
BROADCAST=192.168.1.255
  • Debian
$ ./ipcalc  192.168.1.1 255.255.255.0
Address:   192.168.1.1          11000000.10101000.00000001. 00000001
Netmask:   255.255.255.0 = 24   11111111.11111111.11111111. 00000000
Wildcard:  0.0.0.255            00000000.00000000.00000000. 11111111
=>
Network:   192.168.1.0/24       11000000.10101000.00000001. 00000000
HostMin:   192.168.1.1          11000000.10101000.00000001. 00000001
HostMax:   192.168.1.254        11000000.10101000.00000001. 11111110
Broadcast: 192.168.1.255        11000000.10101000.00000001. 11111111
Hosts/Net: 254                   Class C, Private Internet

@kskmori
Copy link
Author

kskmori commented Jun 14, 2012

I've updated the patch.

  • "broadcast" parameter for IPv4
    "broadcast" parameter may be required along with "cidr_netmask" when you want use a different subnet mask from the static IP address.

This is no longer required.
If broadcast is missing then a virtual IP address is assigned without broadcast(brd) option, and if you really need it you can always specify it as an optional broadcast parameter.
I think this is more simple and better rather than trying to calculate it (like the previous findif binary did) because /bin/ip allows both configurations.

  • loopback(lo) now requires cidr_netmask or broadcast.

This limitation is also removed with the same reason as above.

  • loose error check for "nic" for a IPv6 link-local address.

This error check is added.

I'd appreciate for your further comments.
Thanks,

@aborrero
Copy link

aborrero commented Jul 6, 2012

Hi there!

@kskmori I appreciate your work as I'm using custom RAs to manage IPv6 on loopback (#77) and I would like to use one community sollution.

As soon as I have some time I will test your idea and give back comments.

@dmuhamedagic
Copy link
Contributor

It'd be good to retain the same usage in findif. The reasoning: there could be IPaddr2 clones out there and those resources would break then. I know that it's not our responsibility, but if we can prevent it, why not. Of course, if there's a good reason for the change, then we can go that route too.

@kskmori
Copy link
Author

kskmori commented Sep 13, 2012

Sorry for responding so late...

It'd be good to retain the same usage in findif. The reasoning: there could be IPaddr2 clones out there and those resources would break then. I know that it's not our responsibility, but if we can prevent it, why not. Of course, if there's a good reason for the change, then we can go that route too.

Yes, I totally agree with you, and I believe that the changes in the findif semantics are considered a bug (test case No.6, No.9 for example) and nobody has been expecting this behavior. If you have any particular concerns about a change please let me know so that we will figure out them.

Also we can keep including the old findif binary to the package so that such clones can continue to use it and they can rewrite to use find.sh any time they are convenient.

@dmuhamedagic
Copy link
Contributor

On Wed, Sep 12, 2012 at 10:36:26PM -0700, Keisuke MORI wrote:

Sorry for responding so late...

No problem.

It'd be good to retain the same usage in findif. The reasoning: there could be IPaddr2 clones out there and those resources would break then. I know that it's not our responsibility, but if we can prevent it, why not. Of course, if there's a good reason for the change, then we can go that route too.

Yes, I totally agree with you, and I believe that the changes in the findif semantics are considered a bug (test case No.6, No.9 for example) and nobody has been expecting this behavior. If you have any particular concerns about a change please let me know so that we will figure out them.

Also we can keep including the old findif binary to the package so that such clones can continue to use it and they can rewrite to use find.sh any time they are convenient.

Oh, I missed that the new script would have a different name.
Yes, I guess that we can keep the old binary for a while.

@dmuhamedagic
Copy link
Contributor

Keisuke-san, I think that we can commit this. No other way it'll get properly tested :) Would you be so kind.

@kskmori
Copy link
Author

kskmori commented Oct 5, 2012

OK, I will rework the patches appropriately to be merged into the upstream, and submit another pull request for that.

@lge
Copy link
Member

lge commented Oct 16, 2012

Again, apollogies for not having this sent out when I wrote it,
I'm unsure why it "hibernated" in my Draft folder for five month,
but it was not the only one :(

I realize the pull request has meanwhile been closed,
and we do have a findif.sh implementation in current git.

Still I'll just send these comments as I wrote them back then,
maybe some of them still apply.

At the end, there is a bit of comment how to maybe re-implement
the ipcheck and ifcheck functions without grep and awk.

Feel free to ignore for now, though.
I'll try to review again whats in git now,
and send a proper git diff, once I find the time

;-)

On Wed, May 30, 2012 at 11:20:03PM -0700, Keisuke MORI wrote:

This is a proposal enhancement of IPaddr2 to support IPv6 as well as IPv4.

I would appreciate your comments and suggestions for merging this into
the upstream.

NOTE: This pull request is meant for reviewing the code and
discussions, and not intended to be merged as is at this moment.

Github pull request comments are IMO not the best place to discuss these
things, so I send to the linux-ha-dev mailing list as well.

Benefits:

  • Unify the usage, behavior and the code maintenance between IPv4 and IPv6 on Linux.

    The usage of IPaddr2 and IPv6addr are similar but they have
    different parameters and different behaviors. In particular, they
    may choose a different interface depending on your configuration
    even if you provided similar parameters in the past.

    IPv6addr is written in C and rather hard to make improvements. As
    /bin/ip already supports both IPv4 and IPv6, we can share the most
    of the code of IPaddr2 written in bash.

IPv6addr is supposed to run on non-Linux as well.
So we better not deprecate it, as long as all the world is not Linux.

Notes / Changes:

  • findif semantics changes

    There are some incompatibility in deciding which interface to be
    used when your configuration is ambiguous. But in reality it should
    not be a problem as long as it's configured properly.

    The changes mostly came from fixing a bug in the findif binary
    (returns a wrong broadcast) or merging the difference between
    (old)IPaddr2 and IPv6addr.  See the ofct test cases for details.
    (case No.6, No.9, No.10, No.12, No.15 in IPaddr2v4 test cases)

    Other notable changes are described below.

  • "broadcast" parameter for IPv4

    "broadcast" parameter may be required along with "cidr_netmask" when
    you want use a different subnet mask from the static IP address.
    It's because doing such calculation is difficult in the shell script
    version of findif.

    See the ofct test cases for details. (case No.11, No.14, No.16,
    No.17 in IPaddr2v4 test cases)

    This limitation may be eliminated if we would remove brd options
    from the /bin/ip command line.

If we do not specify the broadcast at all, ip will do the "right thing"
by default, I think. We should only use it on the ip command line, if
it is in the input parameters.
I don't really have a use case for the broadcast address to not be the
"default", so I would be ok with dropping it completely.

  • loopback(lo) now requires cidr_netmask or broadcast.

    See the ofct test case in the IPaddr2 ocft script. The reason is
    similar to the previous one.

We really need to avoid breaking existing configurations.
So we need to fix this.
If we find nothing better, with some heuristic.

  • loose error check for "nic" for a IPv6 link-local address.

    IPv6addr was able to check this, but in the shell script it is hard
    to determine a link-local address (requires bitmask calculation). I
    do not think it's worth to implement it in shell.

There may even be use cases for link-local addresses.
So maybe that is a feature, not a bug ;-)

We could always have a few helpers in C, still.
Or see if we can use existing helpers, if present at runtime.
(ipcalc, sipcalc, there may be more).

  • send_ua: a new binary

    We need one new binary as a replacement of send_arp for IPv6
    support. IPv6addr.c is reused to make this command.

Note that IPv6addr RA is still there and you can continue to use it
for the backward compatibility.

Acknowledgement

Thanks to Tomo Nozawa-san for his hard work for writing and testing
this patch.

Thanks to Lars Ellenberg for the first findif.sh implementation.

You can merge this Pull Request by running:

git pull https://github.com/kskmori/resource-agents
IPaddr2-dualstack-devel

Or you can view, comment on it, or merge it online at:

#97

-- Commit Summary --

  • [RFC] IPaddr2: Proposal patch to support the dual stack of IPv4 and
    IPv6.

-- File Changes --

M heartbeat/IPaddr2 (59) M heartbeat/IPv6addr.c (89) M
heartbeat/Makefile.am (9) A heartbeat/findif.sh (184) M
tools/ocft/IPaddr2 (24) A tools/ocft/IPaddr2v4 (315) A
tools/ocft/IPaddr2v6 (257) M tools/ocft/Makefile.am (2)

-- Patch Links --

https://github.com/ClusterLabs/resource-agents/pull/97.patch

diff --git a/heartbeat/findif.sh b/heartbeat/findif.sh
new file mode 100644
index 0000000..6a2807c
--- /dev/null
+++ b/heartbeat/findif.sh
@@ -0,0 +1,184 @@
+#!/bin/sh
+ipcheck_ipv4() {
local r1_to_255="([1-9][0-9]?|1[0-9][0-9]|2[0-4][0-9]|25[0-5])"
local r0_to_255="([0-9][0-9]?|1[0-9][0-9]|2[0-4][0-9]|25[0-5])"
local r_ipv4="^$r1_to_255.$r0_to_255.$r0_to_255.$r0_to_255$"
echo "$1" | grep -q -Ee "$r_ipv4"
}

+ipcheck_ipv6() {
! echo "$1" | grep -qs "[^0-9:a-fA-F]"
}

+ifcheck_ipv4() {

  • local ifcheck=$1
  • local ifstr
  • local counter=0
  • local procfile="/proc/net/dev"
  • while read LINE
  • do
  • if [ $counter -ge 2 ] ; then
  •  ifstr=`echo $LINE | cut -d ':' -f 1`
    
  •  if [ "$ifstr" = "$ifcheck" ] ; then
    
  •    return 0
    
  •  fi
    
  • fi
  • counter=expr $counter + 1
  • done < $procfile

local ifstr rest
while read ifstr rest ; do
[ "$ifstr" = "$ifcheck:" ] && return 0
done
return 1

  • return 1
    +}
    +ifcheck_ipv6() {
  • local ifcheck="$1"
  • local ifstr
  • local procfile="/proc/net/if_inet6"
  • while read LINE
  • do
  • ifstr=echo $LINE | awk -F ' ' '{print $6}'
  • if [ "$ifstr" = "$ifcheck" ] ; then
  •  return 0
    
  • fi
  • done < $procfile

btw, in bash, I tend to use _ as dummy

much more readable

local tmp ifstr
while read tmp tmp tmp tmp tmp ifstr ; do
[ "$ifstr" = "$ifcheck" ] && return 0
done

  • return 1
    +}

@lge
Copy link
Member

lge commented Oct 18, 2012

Hi Lars,

Thank you for your comments,
I'm going to answer to your comments below,
and if you have further comments I would greatly appreciate it.

2012/10/16 Lars Ellenberg lars@linbit.com:

Again, apollogies for not having this sent out when I wrote it,
I'm unsure why it "hibernated" in my Draft folder for five month,
but it was not the only one :(

I realize the pull request has meanwhile been closed,
and we do have a findif.sh implementation in current git.

Still I'll just send these comments as I wrote them back then,
maybe some of them still apply.

At the end, there is a bit of comment how to maybe re-implement
the ipcheck and ifcheck functions without grep and awk.

Feel free to ignore for now, though.
I'll try to review again whats in git now,
and send a proper git diff, once I find the time

;-)

On Wed, May 30, 2012 at 11:20:03PM -0700, Keisuke MORI wrote:

This is a proposal enhancement of IPaddr2 to support IPv6 as well as IPv4.

I would appreciate your comments and suggestions for merging this into
the upstream.

NOTE: This pull request is meant for reviewing the code and
discussions, and not intended to be merged as is at this moment.

Github pull request comments are IMO not the best place to discuss these
things, so I send to the linux-ha-dev mailing list as well.

Benefits:

  • Unify the usage, behavior and the code maintenance between IPv4 and IPv6 on Linux.

    The usage of IPaddr2 and IPv6addr are similar but they have
    different parameters and different behaviors.�$B!!�(BIn particular, they
    may choose a different interface depending on your configuration
    even if you provided similar parameters in the past.

    IPv6addr is written in C and rather hard to make improvements. As
    /bin/ip already supports both IPv4 and IPv6, we can share the most
    of the code of IPaddr2 written in bash.

IPv6addr is supposed to run on non-Linux as well.
So we better not deprecate it, as long as all the world is not Linux.

Agreed.

Notes / Changes:

  • findif semantics changes

    There are some incompatibility in deciding which interface to be
    used when your configuration is ambiguous. But in reality it should
    not be a problem as long as it's configured properly.

    The changes mostly came from fixing a bug in the findif binary
    (returns a wrong broadcast) or merging the difference between
    (old)IPaddr2 and IPv6addr.�$B!!�(B See the ofct test cases for details.
    (case No.6, No.9, No.10, No.12, No.15 in IPaddr2v4 test cases)

    Other notable changes are described below.

  • "broadcast" parameter for IPv4

    "broadcast" parameter may be required along with "cidr_netmask" when
    you want use a different subnet mask from the static IP address.
    It's because doing such calculation is difficult in the shell script
    version of findif.

    See the ofct test cases for details. (case No.11, No.14, No.16,
    No.17 in IPaddr2v4 test cases)

    This limitation may be eliminated if we would remove brd options
    from the /bin/ip command line.

If we do not specify the broadcast at all, ip will do the "right thing"
by default, I think. We should only use it on the ip command line, if
it is in the input parameters.
I don't really have a use case for the broadcast address to not be the
"default", so I would be ok with dropping it completely.

It has been fixed and the latest code in the repo now should work like you said.

  • loopback(lo) now requires cidr_netmask or broadcast.

    See the ofct test case in the IPaddr2 ocft script. The reason is
    similar to the previous one.

We really need to avoid breaking existing configurations.
So we need to fix this.
If we find nothing better, with some heuristic.

It has also been fixed now and loopback can be used as same as before.

  • loose error check for "nic" for a IPv6 link-local address.

    IPv6addr was able to check this, but in the shell script it is hard
    to determine a link-local address (requires bitmask calculation). I
    do not think it's worth to implement it in shell.

There may even be use cases for link-local addresses.
So maybe that is a feature, not a bug ;-)

This check is done by simply matching fe80:: prefix as Alan's suggestion
and I think it is just enough.

We could always have a few helpers in C, still.
Or see if we can use existing helpers, if present at runtime.
(ipcalc, sipcalc, there may be more).

  • send_ua: a new binary

    We need one new binary as a replacement of send_arp for IPv6
    support. IPv6addr.c is reused to make this command.

Note that IPv6addr RA is still there and you can continue to use it
for the backward compatibility.

Acknowledgement

Thanks to Tomo Nozawa-san for his hard work for writing and testing
this patch.

Thanks to Lars Ellenberg for the first findif.sh implementation.

You can merge this Pull Request by running:

git pull https://github.com/kskmori/resource-agents
IPaddr2-dualstack-devel

Or you can view, comment on it, or merge it online at:

#97

-- Commit Summary --

  • [RFC] IPaddr2: Proposal patch to support the dual stack of IPv4 and
    IPv6.

-- File Changes --

M heartbeat/IPaddr2 (59) M heartbeat/IPv6addr.c (89) M
heartbeat/Makefile.am (9) A heartbeat/findif.sh (184) M
tools/ocft/IPaddr2 (24) A tools/ocft/IPaddr2v4 (315) A
tools/ocft/IPaddr2v6 (257) M tools/ocft/Makefile.am (2)

-- Patch Links --

https://github.com/ClusterLabs/resource-agents/pull/97.patch

diff --git a/heartbeat/findif.sh b/heartbeat/findif.sh
new file mode 100644
index 0000000..6a2807c
--- /dev/null
+++ b/heartbeat/findif.sh
@@ -0,0 +1,184 @@
+#!/bin/sh
+ipcheck_ipv4() {
local r1_to_255="([1-9][0-9]?|1[0-9][0-9]|2[0-4][0-9]|25[0-5])"
local r0_to_255="([0-9][0-9]?|1[0-9][0-9]|2[0-4][0-9]|25[0-5])"
local r_ipv4="^$r1_to_255.$r0_to_255.$r0_to_255.$r0_to_255$"
echo "$1" | grep -q -Ee "$r_ipv4"
}

+ipcheck_ipv6() {
! echo "$1" | grep -qs "[^0-9:a-fA-F]"
}

+ifcheck_ipv4() {

  • local ifcheck=$1
  • local ifstr
  • local counter=0
  • local procfile="/proc/net/dev"
  • while read LINE
  • do
  • if [ $counter -ge 2 ] ; then
  •  ifstr=`echo $LINE | cut -d ':' -f 1`
    
  •  if [ "$ifstr" = "$ifcheck" ] ; then
    
  •    return 0
    
  •  fi
    
  • fi
  • counter=expr $counter + 1
  • done < $procfile

local ifstr rest
while read ifstr rest ; do
[ "$ifstr" = "$ifcheck:" ] && return 0
done
return 1

  • return 1
    +}
    +ifcheck_ipv6() {
  • local ifcheck="$1"
  • local ifstr
  • local procfile="/proc/net/if_inet6"
  • while read LINE
  • do
  • ifstr=echo $LINE | awk -F ' ' '{print $6}'
  • if [ "$ifstr" = "$ifcheck" ] ; then
  •  return 0
    
  • fi
  • done < $procfile

btw, in bash, I tend to use _ as dummy

much more readable

local tmp ifstr
while read tmp tmp tmp tmp tmp ifstr ; do
[ "$ifstr" = "$ifcheck" ] && return 0
done

  • return 1
    +}

Linux-HA-Dev: Linux-HA-Dev@lists.linux-ha.org
http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev
Home Page: http://linux-ha.org/

Regards,

Keisuke MORI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants