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

Ipv6; Support ipset with "family inet6" #538

Merged
merged 2 commits into from
Sep 23, 2018

Conversation

uablrek
Copy link
Contributor

@uablrek uablrek commented Sep 17, 2018

Handle ipset for ipv4, ipv6 and dual-stack

For ipv6 and dual-stack support the IPSet is altered. An effort is made to be compatible with existing ipv4-only code while allowing an easy switch to ipv6-only. Dual-stack is considered in the sense that instances of IPSet for ipv4 and ipv6 can exist in paralell.

This PR is preliminary and intended for discussions on how to introduce ipv6-only and dual-stack, but it is a serious effort and my hope is that it will be accepted eventually.

NewIPSet

An isIpv6 flag is added to IPSet and is passed on creation;

nrc.ipSetHandler, err = utils.NewIPSet(false)

All other calls to functions in IPSet will remain the same, so to switch to ipv6-only should be simple.

For dual-stack all functions calls must go to both an ipv4 and ipv6 instance of IPSet. So perhaps it would be better to pass an enum; ipv4/ipv6/dual-stack to NewIPSet(operation). But then again, it is hard for me to decide at this early stage, another abstraction layer may be better(?). The current flag is sufficient for ipv6-only and can easily be changed for dual-stack if needed.

The family options becomes invalid for any set operation to the IPSet (since it is implicit). I do not consider that as a problem.

Ipset name-space

Unlike iptables the same ipset is used for both ipv4 and ipv6. This means that the sets must have different names for ipv4 and ipv6. This makes it impossible to intantiate two IPSet for dual-stack with the same set names. For coding in higher levels this becomes a mess so instead a prefix inet6: is silently added to all set names if the IPSet is of ipv6 type.

Save/Restore/Flush

These operations seem to be imported from other code (the "file" comments) and not fully adapted. It works on all ipsets. For instance a IPSet.Save() operation will "slurp-in" all ipset's on the host to the IPSet (example).

This must be fixed in some way, but this is a larger job and not really necessary for ipv6-only. So I postpone this for now.

An idea may be to pass a regexp to NewIPSet() that filters the ipset to only names allowed.

Test

For now the ipv6 tests are manual. IMO the most important is that ipv4-only still works in the same way as before, including upgrades, so regression tests are very important.

For the future ipv6, and ultimately dual-stack, tests are needed. There are updates in this area initiated for Kubernetes that hopefully can be used.

The current status is that I have temporary hard-coded NewIPSet(true) to get the CNI part of kube-router up and running for ipv6-only. Ipset's are created ok;

# ipset list
Name: inet6:kube-router-pod-subnets
Type: hash:net
Revision: 6
Header: family inet6 hashsize 1024 maxelem 65536 timeout 0
Size in memory: 1128
References: 0
Number of entries: 0
Members:

Name: inet6:kube-router-node-ips
Type: hash:ip
Revision: 4
Header: family inet6 hashsize 1024 maxelem 65536 timeout 0
Size in memory: 104
References: 0
Number of entries: 0
Members:

But the gobgp can't be started;

E0917 09:37:13.528009     400 network_routes_controller.go:210] Failed to start node BGP server: Failed to start BGP server due to : listen tcp6 [2000::4]:179: bind: address already in use

and re-tries continue forever. This indicates that port is not properly closed on cleanup of some earlier failure. But I will take that in a later PR.

Copy link
Member

@murali-reddy murali-reddy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got a chance to test this PR. I do not see any regression as its all ipv4 only.

if err != nil {
return err
}
entry.Set.Parent.Save()
entry.Set.Parent.Save() // <-- EKM: Implications??
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this comment required?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. Sorry I forgot to remove it, sorry. It was a personal note.
The Save function will add all sets to the IPSet object. Eventually all IPSet objects may inlcude all sets. Something must be done eventually but it obviously works and I coudn't make an analyze fast so I put the reminder there.

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.

2 participants