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

Fixes #29384 - Connect by IP: Prefer IPv6 over IPv4 #577

Merged
merged 1 commit into from Aug 24, 2021

Conversation

UXabre
Copy link
Member

@UXabre UXabre commented Apr 13, 2021

No description provided.

@adamruzicka
Copy link
Contributor

Is this safe? Interfaces often have more than 1 ipv6 address, but foreman doesn't model this well (or at all to be precise) so just a single one of them is stored. I don't know from the top of my head how the address-to-be-stored is picked from all the addresses, but it is quite possible it will be a link-local address or something that can't really be used to reach the host.

@UXabre
Copy link
Member Author

UXabre commented Apr 13, 2021

Hi @adamruzicka
As far as my mileage goes and afaik, I don't see any subsytem storing link-local IPv6 addresses in the host; only the global ones being configured via the interfaces tab (as this, indeed, only supports a single IPv6 address).
Is there any module you know that would actually store the link-local address or, even worse, overwrite this IPv6 address? My first thought was that the facts scraping module would perhaps overwrite this, but it doesn't seem to be like this.

@adamruzicka
Copy link
Contributor

I thought we just use whatever facter (in case of puppet) gives us, which returns link local addresses if there are no other ipv6 addresses on a given interface. But apparently there's some additional filtering going on somewhere because I don't see ll addresses in foreman.

@ares Do you recall if there was something else that made us not implement connect by ip for ipv6?

@UXabre
Copy link
Member Author

UXabre commented Apr 13, 2021

Well, this file: https://github.com/theforeman/foreman/blob/3347fa49d500964f0209122d8d36c920d1feafcc/app/models/host/base.rb
seems to filter specifically on link-local addresses when set; which is probably the worst-case that we want to avoid. I'm not done unraveling the entire flow from fact to set tough...

@lzap
Copy link
Member

lzap commented Apr 15, 2021

I was actually solving the very same problem in provisioning and my approach is a host parameter:

theforeman/foreman#8447

Just saying.

@UXabre
Copy link
Member Author

UXabre commented Apr 19, 2021

I've been thinking and was wondering if it would make sense to do some sort of "TCP Ping" to check if the device can be reached on any of the IP addresses instead of just taking IPv6 as the preferred address?
On the other hand, I try to mimic the proposed default of starting with IPv6, as, by now, probably most OS'es, by default, abide to? (I know, this can be circumvented and configured to behave in IPv4 being preferred)
I'm probably just curious to learn who uses IPv6 on a REX-interface without it being actually reachable from foreman? The only thing I can imagine is because somewhere is a router in between which can't IPv6?

@lzap
Copy link
Member

lzap commented Apr 23, 2021

I would stay away from ping checks, from time to time desperately confused users come to us saying that the ping/echo ICMP/TCP checks in our DHCP workflow made some huge pain to them until they figured it out.

@UXabre
Copy link
Member Author

UXabre commented Apr 26, 2021

I completely undestand your point and agree that this would indeed be an less then suboptimal solution.
I think it might be a good idea that I draft up an RFC on the subject on the community page first

@lzap
Copy link
Member

lzap commented Apr 27, 2021

You could turn the setting into IPv4, IPv6 or no (three states). Plus a host parameter could do the trick on a per-host basis.

@UXabre
Copy link
Member Author

UXabre commented Apr 28, 2021

Oh yes, I actually like that idea a lot! Will implement this accordingly.

@adamruzicka
Copy link
Contributor

@UXabre any progress here?

@UXabre
Copy link
Member Author

UXabre commented Jun 15, 2021

Hi @adamruzicka , no further progress yet, I'm kinda flooded at this point. My next window of opportunity is coming saturday, it's not a lot of work really, so sorry for the delay.

@adamruzicka
Copy link
Contributor

Gentle reminder we'd still be interested in this :)

@UXabre
Copy link
Member Author

UXabre commented Jul 29, 2021

Hi, so sorry for the huge delay, now I'm in my holidays I did find the time!
I've added a setting to configure preference for IPv6 (or not). Not sure if the name isn't too straighforward; any proposal is welcome at this point.

@UXabre UXabre force-pushed the master branch 3 times, most recently from 55548d9 to e8e0afa Compare July 29, 2021 12:43
Copy link
Contributor

@adamruzicka adamruzicka left a comment

Choose a reason for hiding this comment

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

Could be made a bit DRYer, but in general +1

app/models/remote_execution_provider.rb Outdated Show resolved Hide resolved
@UXabre
Copy link
Member Author

UXabre commented Aug 17, 2021

Allright, I've made a tiny adjustment to actually work as a preference setting, meaning, if IPv6 preference is enabled, it should try an IPv6 address, if none exists, it should fall back to IPv4; and vice versa if the IPv6 preference is disabled.
I've also added a test for this behavior.

Copy link
Contributor

@adamruzicka adamruzicka left a comment

Choose a reason for hiding this comment

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

Works nicely, let's get this in

@adamruzicka adamruzicka merged commit a49fa19 into theforeman:master Aug 24, 2021
@adamruzicka
Copy link
Contributor

Thank you @UXabre !

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