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

Dual Stack Additions #1386

Merged
merged 59 commits into from
Jan 23, 2023
Merged

Conversation

aauren
Copy link
Collaborator

@aauren aauren commented Oct 25, 2022

@mrueg @murali-reddy

This is PR is primarily the work of @vadorovsky and @thomasferrandiz done in #1280 and #1343 to make the network policy controller (NPC) dual-stack compatible. This was done by keeping track of two different iptables and ipset handlers within the NPC. For rules/sets that are IP agnostic, rules/sets are written using both handlers. For rules/sets that are IP specific, they check which family the IP belongs to and then use the appropriate handlers. This PR replaces both of the previous PRs.

In addition to the NPC changes, in order to test this well, I found that I required dual-stack capabilities in the network routes controller (NRC). This had previously been IPv6 compatible, but not dual-stack compatible (meaning that you could only use IPv4 or IPv6). I refactored this to make the controller truly dual stack.

I expect that this will have a marginal impact on NPC performance as the initial section of the NPC's logic that execs out to ensure that default rules exist and are valid still uses the old style List / Insert logic. However, all subsequent network policy rules are written using the newer iptables-save / iptables-restore which greatly reduces processing time. It still requires 2 such calls as IPv4 and IPv6 are managed using different iptables commands (iptables vs ip6tables). But this is the best we can do here without switching to nftables or BPF.

My goal with this PR is to:

  1. Provide maintainers a chance to review the work, while acknowledging that this is a humongous PR and there is little chance of a decent review given its size.
  2. Give any other members of the community a chance to review or add thoughts
  3. Begin a conversation about my intent with this work (see below)
  4. Give anyone who is interested a chance to test the work before it gets merged

You'll notice that this PR does not target the master branch. This is an incredibly large change with over 5k lines of code having been modified. Additionally, there are a few nuances and options that are not fully backward compatible. As such, I would like to eventually incorporate this into a kube-router 2.0 release and release several release candidates over the next month or two to allow community members to test.

I see the flow going something like this:

  1. Continue to test PR (so far I've only done basic testing) while giving maintainers and community members a chance to review
  2. Add documentation to the docs about config items that have changed or are no longer backward compatible
  3. When confident, merge the PR and create a v2.0.0-rc1 release candidate (this would not affect the latest tag which could introduce breaking changes to regular clusters)
  4. Encourage community members to try out and test the release candidate
  5. Make an additional PR to convert the network services controller (NSC) to also be dual-stack
  6. Incorporate fixes and NSC dual-stack additions into subsequent v2.0.0-rc's
  7. Continue to incorporate fixes and feedback from the community in subsequent RCs
  8. When confident, merge the prep-v2.0 branch into master and cut an official v2.0.0 release of kube-router

Let me know if anyone has any thoughts here.

@aauren aauren force-pushed the dual-stack-additions branch from 9ea1af2 to ef76ef1 Compare October 27, 2022 16:54
Bumps [github.com/aws/aws-sdk-go](https://github.com/aws/aws-sdk-go) from 1.44.116 to 1.44.124.
- [Release notes](https://github.com/aws/aws-sdk-go/releases)
- [Commits](aws/aws-sdk-go@v1.44.116...v1.44.124)

---
updated-dependencies:
- dependency-name: github.com/aws/aws-sdk-go
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
@aauren aauren force-pushed the dual-stack-additions branch 5 times, most recently from 50ba0f2 to a046f5b Compare October 27, 2022 17:41
Bumps [github.com/docker/docker](https://github.com/docker/docker) from 20.10.19+incompatible to 20.10.21+incompatible.
- [Release notes](https://github.com/docker/docker/releases)
- [Changelog](https://github.com/moby/moby/blob/master/CHANGELOG.md)
- [Commits](moby/moby@v20.10.19...v20.10.21)

---
updated-dependencies:
- dependency-name: github.com/docker/docker
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
@aauren
Copy link
Collaborator Author

aauren commented Oct 31, 2022

This PR also fixes #1023

@aauren aauren force-pushed the dual-stack-additions branch from a046f5b to 1db2294 Compare October 31, 2022 04:26
@aauren
Copy link
Collaborator Author

aauren commented Oct 31, 2022

Bumps [github.com/onsi/gomega](https://github.com/onsi/gomega) from 1.22.1 to 1.23.0.
- [Release notes](https://github.com/onsi/gomega/releases)
- [Changelog](https://github.com/onsi/gomega/blob/master/CHANGELOG.md)
- [Commits](onsi/gomega@v1.22.1...v1.23.0)

---
updated-dependencies:
- dependency-name: github.com/onsi/gomega
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
rkojedzinszky and others added 3 commits November 15, 2022 15:19
…1392)

* Support for kube-router.io/peer.localips annotation

* Fix checking for valid addresses in kube-router.io/peer.localips
Bumps [github.com/aws/aws-sdk-go](https://github.com/aws/aws-sdk-go) from 1.44.124 to 1.44.138.
- [Release notes](https://github.com/aws/aws-sdk-go/releases)
- [Commits](aws/aws-sdk-go@v1.44.124...v1.44.138)

---
updated-dependencies:
- dependency-name: github.com/aws/aws-sdk-go
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
@aauren
Copy link
Collaborator Author

aauren commented Nov 20, 2022

Update, I got a chance to test these changes a bit more, and realized that yet still more testing is needed. Specifically around peering with global peers.

  • I currently have some staged changes that catch the unit tests up to the added BGP changes
  • Now that we have the localips annotation that was added in master, there was some better ways to approach peering that needed to be considered
  • Realized that whether or not to advertise IPv6 routes shouldn't be dependent upon the primary IP address of the kube-router node, but should instead by based upon the command-line switch kube-router was started with. Maybe some day down the road it should be based upon more in-depth peer config, but without a more expressive peering configuration i.e. Restructure node peer annotations #1393 this should be postponed.

Additionally, upon merging this PR, I intend to update the IPv6 documentation on the main branch (https://github.com/cloudnativelabs/kube-router/blob/master/docs/ipv6.md) to give updates on where we stand as a project on IPv6 support when using the functionality introduced with this branch. Hopefully, with a container built and updated docs provided more people will help out with testing these changes.

For now, I plan to keep the staged changes where they are until I finish testing.

dependabot bot added 6 commits November 28, 2022 14:37
Bumps [github.com/aws/aws-sdk-go](https://github.com/aws/aws-sdk-go) from 1.44.138 to 1.44.146.
- [Release notes](https://github.com/aws/aws-sdk-go/releases)
- [Commits](aws/aws-sdk-go@v1.44.138...v1.44.146)

---
updated-dependencies:
- dependency-name: github.com/aws/aws-sdk-go
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [github.com/onsi/gomega](https://github.com/onsi/gomega) from 1.23.0 to 1.24.1.
- [Release notes](https://github.com/onsi/gomega/releases)
- [Changelog](https://github.com/onsi/gomega/blob/master/CHANGELOG.md)
- [Commits](onsi/gomega@v1.23.0...v1.24.1)

---
updated-dependencies:
- dependency-name: github.com/onsi/gomega
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [github.com/aws/aws-sdk-go](https://github.com/aws/aws-sdk-go) from 1.44.146 to 1.44.150.
- [Release notes](https://github.com/aws/aws-sdk-go/releases)
- [Commits](aws/aws-sdk-go@v1.44.146...v1.44.150)

---
updated-dependencies:
- dependency-name: github.com/aws/aws-sdk-go
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [github.com/prometheus/client_golang](https://github.com/prometheus/client_golang) from 1.13.0 to 1.14.0.
- [Release notes](https://github.com/prometheus/client_golang/releases)
- [Changelog](https://github.com/prometheus/client_golang/blob/main/CHANGELOG.md)
- [Commits](prometheus/client_golang@v1.13.0...v1.14.0)

---
updated-dependencies:
- dependency-name: github.com/prometheus/client_golang
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [golang.org/x/net](https://github.com/golang/net) from 0.2.0 to 0.4.0.
- [Release notes](https://github.com/golang/net/releases)
- [Commits](golang/net@v0.2.0...v0.4.0)

---
updated-dependencies:
- dependency-name: golang.org/x/net
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [github.com/osrg/gobgp/v3](https://github.com/osrg/gobgp) from 3.7.0 to 3.9.0.
- [Release notes](https://github.com/osrg/gobgp/releases)
- [Changelog](https://github.com/osrg/gobgp/blob/master/.goreleaser.yml)
- [Commits](osrg/gobgp@v3.7.0...v3.9.0)

---
updated-dependencies:
- dependency-name: github.com/osrg/gobgp/v3
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Without this, kube-router would end up sharing the index between ipv4
and ipv6 which would cause it to error out when one incremented beyond
the number of rules that actually existed in the chain.
Running kube-router without metrics is a perfectly valid way to run
kube-router and as such it shouldn't emit an error message when a user
has not set that flag. Move the message down to Info.
Convert all BGP set names to constants and then refer to them via the
constant across the code base so that we reduce the effect of typos.
@aauren aauren force-pushed the dual-stack-additions branch from 5abc614 to ed2a5dc Compare January 22, 2023 21:28
aauren added 20 commits January 22, 2023 17:36
If we can't find an appropriate IP to add for nextHop to injectRoute or
overlay tunnel, raise error rather than trying to continue.
For configured BGP peers only attempt peering if IP protos match,
otherwise skip and log warning
Advertise IPv4 / IPv6 AfiSafi capability based upon node's capabilities
rather than limiting to the node's configured protocol.
We do a lot of getting defined sets for GoBGP and are planning to do
more of it in the future. This commit centralizes the logic for this and
reduces repetition.
Without this logic, it appears that sometimes GoBGP is inclined to match
unintentional routes in policy because of the MATCHSET_ANY declaration
and the way that it interacts with empty sets.

In my testing, without this logic I found that it often resulted in
various routes not being advertised correctly and not even showing up in
GoBGP itself. My current guess is that policy keeps GoBGP from importing
the route into the RIB even from the Protobuf socket connection that
kube-router establishes directly.
Change return to continue so that both IPv4 and IPv6 are checked for
drop policy not just the first one.
On dual-stack nodes there can still be pods that are single stack. When
this happens there won't be a pod IP for a given family and if
kube-router tries to add rules with a missing pod IP the iptables rules
won't be formatted correctly (because it won't have a valid source or
destination for that family).

So rather than breaking the whole iptables-restore we warn in the logs
and skip the pod policy chains for that family.
Rather than just silently not adding policies for controllers that don't
support a given address family, emit a warning so that it is more
obvious in the logs that kube-router isn't able to add a policy for a
given family when the controller doesn't have that family enabled.
@aauren aauren force-pushed the dual-stack-additions branch from ed2a5dc to e6d64a2 Compare January 23, 2023 00:13
@aauren aauren merged commit a99fdbf into cloudnativelabs:prep-v2.0 Jan 23, 2023
@StevenACoffman StevenACoffman mentioned this pull request Sep 6, 2023
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.

5 participants