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

use default broadcast source address for add-node hint #4260

Merged
merged 2 commits into from
Oct 26, 2023

Conversation

nihr43
Copy link
Contributor

@nihr43 nihr43 commented Oct 20, 2023

Summary

This simplifies the address detection a bit for the 'add-node' command hint, enabling a use case where FRR or similar is installing the default route via multiple 'unnumbered' interfaces:

# microk8s add-node
From the node you wish to join to this cluster, run the following:
microk8s join none:25000/........

# ip route show default
default nhid 151 proto bgp metric 20 
	nexthop via inet6 fe80::290:bff:fea5:e2d3 dev bgpenp36s0 weight 1 
	nexthop via inet6 fe80::290:bff:fea5:deab dev bgpenp35s0 weight 1 

# ip route get 255.255.255.255
broadcast 255.255.255.255 dev bgpenp35s0 src 10.0.200.1 uid 0 
    cache <local,brd> 

Changes

Instead of fetching the default route interface and then its address, the suggested change simply finds the system-wide default source address for 255.255.255.255. In a typical dhcp or static ip scenario, this will be the expected interface; in my 'L3 to the hypervisor' scenario, this will be a loopback /32 ipv4 address.

Testing

I've tested this by simply running snapcraft build and installing the artifact. First on a machine running bgp, with 10.0.200.255/32 on lo:

$ snapcraft build
$ sudo snap install microk8s_v1.28.2_amd64.snap --dangerous --classic
$ microk8s add-node
From the node you wish to join to this cluster, run the following:
microk8s join 10.0.200.255:25000/e6f7f77b060d7b53bba6c97db29bfc47/aab0f29f6056

Then on an lxd vm with basic networking:

$ lxc file push microk8s_v1.28.2_amd64.snap microk8s/root/
$ lxc shell microk8s
root@microk8s:~# snap install microk8s_v1.28.2_amd64.snap --dangerous --classic
root@microk8s:~# microk8s add-node
From the node you wish to join to this cluster, run the following:
microk8s join 10.11.228.100:25000/7bf37091dd4c3f0abad312f16beb23ee/e369d5eb414d

I need guidance on running the legitimate test suite, it seems the testing documentation is outdated.

Possible Regressions

Checklist

  • [x ] Read the contributions page.
  • [x ] Submitted the CLA form, if you are a first time contributor.
  • The introduced changes are covered by unit and/or integration tests.

Notes

@neoaggelos neoaggelos self-requested a review October 20, 2023 06:38
Copy link
Contributor

@neoaggelos neoaggelos left a comment

Choose a reason for hiding this comment

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

Hi @nihr43, thanks for the PR, I see why this is useful.

However, I am afraid that this would break airgap environments where a route to 255.255.255.255 might not be available:

root@u3:~# ip r
10.0.0.0/16 dev eth0 proto kernel scope link src 10.0.0.41 
10.0.0.1 dev eth0 proto dhcp scope link src 10.0.0.41 metric 100 
root@u3:~# ip route get 255.255.255.255
RTNETLINK answers: Network is unreachable

However, I see the value in the change that you propose. Would you be willing to fallback to your method if the first does not find an IP?

e.g. something like

local IP_ADDR="$(as is)"
if [[ -z "$IP_ADDR" ]]; then
  local IP_ADDR="$($SNAP/sbin/ip route get 255.255.255.255 | $SNAP/usr/bin/gawk '{for(i=1; i<NF; i++) if($i=="src") print$(i+1)}' | head -1)"
fi

if [[ -z "$IP_ADDR" ]]; then
        echo "none"
fi

@nihr43
Copy link
Contributor Author

nihr43 commented Oct 21, 2023

yep that works.

about the CLA check that failed earlier; I think the check ran before my CLA submission was officially "accepted". I expect it to work the next time around.

@nihr43 nihr43 requested a review from neoaggelos October 23, 2023 19:07
Copy link
Contributor

@neoaggelos neoaggelos left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @nihr43!

nihr43 and others added 2 commits October 26, 2023 17:37
allows detection of loopback address in the event interfaces are unnumbered
Co-authored-by: Angelos Kolaitis <angelos.kolaitis@canonical.com>
@neoaggelos neoaggelos force-pushed the detect_loopback_address branch from 97f8116 to 75d60f8 Compare October 26, 2023 14:37
@neoaggelos
Copy link
Contributor

@nihr43 I rebased against latest master to get a CI run. After it's green, we're good to merge. Thanks again for the PR!

@neoaggelos neoaggelos merged commit d46eb82 into canonical:master Oct 26, 2023
14 checks passed
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