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

Check if we are on ipv4, ipv6 or dualStack when doing tailscale #7838

Merged
merged 1 commit into from
Jul 6, 2023

Conversation

manuelbuil
Copy link
Contributor

@manuelbuil manuelbuil commented Jun 30, 2023

Proposed Changes

Instead of listing the IPs of the tailscale interface in the variable IPs []net.IP, separate the ipv4 and ipv6 address that are always assigned to the tailscale interface:

	IPv4Address  net.IP
	IPv6Address  net.IP

That way we can easily control what address to use in kube-api depending on the configured mode in k3s.

Also a bug in utils/net.go was fixed:

elem.To16() == nil

is not a correct way to check if an IP is ipv6. It works for both ipv4 and ipv6!

Types of Changes

Bugfix

Verification

Verify that this works in:
Deploy k3s in three modes:

  • ipv6-only
  • ipv4-only
  • dualStack

And check what IP is used for kubeapi-server in the flag advertised-ip. It should be ipv4 for ipv4-only and dual-stack mode. Whereas it should be ipv6 for ipv6-only mode.

Testing

Linked Issues

#7831

User-Facing Change


Further Comments

@manuelbuil manuelbuil requested a review from a team as a code owner June 30, 2023 10:19
@manuelbuil manuelbuil force-pushed the ipv4ipv6tailscale branch 2 times, most recently from d9141bf to 28de47b Compare June 30, 2023 13:25
@manuelbuil manuelbuil marked this pull request as draft June 30, 2023 13:30
@manuelbuil manuelbuil force-pushed the ipv4ipv6tailscale branch 2 times, most recently from 7849673 to bbd49f6 Compare June 30, 2023 14:06
@manuelbuil manuelbuil marked this pull request as ready for review June 30, 2023 14:27
@codecov
Copy link

codecov bot commented Jun 30, 2023

Codecov Report

Patch coverage has no change and project coverage change: +4.27 🎉

Comparison is base (2215870) 47.18% compared to head (f21a014) 51.45%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7838      +/-   ##
==========================================
+ Coverage   47.18%   51.45%   +4.27%     
==========================================
  Files         143      143              
  Lines       14509    14532      +23     
==========================================
+ Hits         6846     7478     +632     
+ Misses       6576     5865     -711     
- Partials     1087     1189     +102     
Flag Coverage Δ
e2etests 49.34% <0.00%> (?)
inttests 44.40% <0.00%> (-0.14%) ⬇️
unittests 19.85% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/agent/config/config.go 53.63% <0.00%> (+2.87%) ⬆️
pkg/cli/server/server.go 58.35% <0.00%> (-0.17%) ⬇️
pkg/util/net.go 56.88% <0.00%> (+8.88%) ⬆️
pkg/vpn/vpn.go 0.00% <0.00%> (ø)

... and 42 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

pkg/util/net.go Outdated
@@ -97,10 +97,9 @@ func GetFirst6(elems []net.IP) (net.IP, error) {
// If no IPv6 addresses are found, an error is raised.
func GetFirst6Net(elems []*net.IPNet) (*net.IPNet, error) {
for _, elem := range elems {
if elem == nil || elem.IP.To16() == nil {
continue
if elem != nil || netutils.IsIPv6(elem.IP) {
Copy link
Member

@brandond brandond Jun 30, 2023

Choose a reason for hiding this comment

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

This is wrong, you'll now return any non-nil element! The previous code was skipping non-ipv6 addresses; I'm not sure how this is functionally different?

Suggested change
if elem != nil || netutils.IsIPv6(elem.IP) {
if elem != nil && netutils.IsIPv6(elem.IP) {

Also, note that the code you're calling effectively does the same thing - checks for non-nil .To16():
https://github.com/kubernetes/utils/blob/9f6742963106fb1fadd7ec2493d6d6cd689e5318/net/ipfamily.go#L106

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch! I missed that 🤦

It is slightly different in the ipfamily.go code because by the time to code reaches case ip.To16() != nil:, ipv4 ips were already returned in the previous line, i.e. only ipv6 addresses can reach case ip.To16() != nil:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Run this gist and you'll see the difference: https://gist.github.com/manuelbuil/73d7e0ff32a69eb045602aeee45896a6

Signed-off-by: Manuel Buil <mbuil@suse.com>
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.

3 participants