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

Refactor IPv6 generator to support hosts with smaller than prefix 64 #1595

Merged
merged 1 commit into from
May 30, 2024

Conversation

furkansahin
Copy link
Contributor

@furkansahin furkansahin commented May 14, 2024

Some providers place multiple hosts behind the same /64. Therefore, the
gateway is the same for both servers. We then try to use this whole /64
in Ubicloud and carve out /80 addresses to assign to VMs. However, when
a single server claims the full /64, we cannot add the second host. Here
in this PR we solve this problem by supporting subnets that are smaller
than /64. This way, when there are 2 servers behind the same subnet, we
can selectively assign a (let's say) /68 subnet to one and another /68
to another. After that, we can still carve out /80 addresses from those.

One important change we are making is in the ip6_random_vm_network. That
is needed because, the rounding up causes the fail line to be
triggered. Here is a scenario;

  • The host_prefix is 68.
  • The prefix is 79
    With the rounding up, we produce a lower_bit that is 2 bytes. These 2
    bytes before the /79, doesn't fit together with /68 prefix, we may reach
    an address that is not directly a child address of the host_prefix.
    Therefore, we keep that at minimum 1 and produce a number that is a
    single byte which easily fits under the /68 prefix.

@furkansahin furkansahin requested a review from fdr May 14, 2024 12:16
@furkansahin furkansahin marked this pull request as draft May 14, 2024 12:44
@furkansahin furkansahin marked this pull request as ready for review May 14, 2024 12:53
model/vm_host.rb Show resolved Hide resolved
@furkansahin furkansahin marked this pull request as draft May 16, 2024 16:38
@furkansahin furkansahin requested a review from fdr May 16, 2024 16:44
@furkansahin furkansahin force-pushed the support_smaller_ip6_net branch 3 times, most recently from 9eef4f8 to c046834 Compare May 17, 2024 10:35
@furkansahin furkansahin marked this pull request as ready for review May 17, 2024 10:36
@furkansahin furkansahin force-pushed the support_smaller_ip6_net branch 2 times, most recently from 58e4659 to 527baaa Compare May 17, 2024 15:29
model/vm_host.rb Outdated Show resolved Hide resolved
Copy link
Collaborator

@fdr fdr left a comment

Choose a reason for hiding this comment

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

I'm a bit puzzled by this patch. It appears it changes how many random bits are used in all cases, e.g. a reduction in precision from n to C that is unconditional. Is that what we want?

Also, is bytes_needed = [((subnet_bits - 1) / 8), 1].max fixing a bug?

also, does this patch more or less do everything necessary to support longer prefixes?

@furkansahin
Copy link
Contributor Author

So, when we go to the longer prefixes landscape, we're talking about > 64 and < 80. If we have the example of 68,

bytes_needed = ((subnet_bits - 1) / 8) + 1

would return 2 bytes which is longer than we needed, and in return we hit the line for most of the proposals;

fail "BUG: host should be supernet of randomized subnet" unless net6.rel(proposal) == 1

By adding [].max, we also make sure we won't get nil from SecureRandom.bytes(bytes_needed).unpack1("C") for cases bytes_needed=0.

So, without the changes in this PR, we would either hit nil ref in line 84 or hit the fail in line 92 for hosts with longer prefixes.

@furkansahin furkansahin requested a review from fdr May 21, 2024 07:59
@furkansahin
Copy link
Contributor Author

@fdr waiting for your input here

@fdr fdr force-pushed the support_smaller_ip6_net branch from 527baaa to b568330 Compare May 25, 2024 02:11
Copy link
Collaborator

@fdr fdr left a comment

Choose a reason for hiding this comment

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

I would rewrite the commit message, but I'm out of time. I think it is somehow repetitive and could be condensed.

And, it omits that ALL random subnets, even for hosts with shorter prefixes, will use fewer random bits as a side effect.

Secondarily, that you are fixing a bug, as the previous code would crash if host_prefix was too large, rounding the number of random bytes generated to zero.

But it's better to commit in a timely manner than not, so, here's the approval bit.

@fdr
Copy link
Collaborator

fdr commented May 27, 2024

Actually, now that I think of it, why even bother with the bytes needed with the "C" unpack? Can you avoid all that and generate, always, one byte of random bits? So the code can stop pretending it handles more or less random bytes?

We used to generate 2 bytes long random bits and shift it to the head of
guest prefix, assuming that these random bits will fit in between host's
and the guests' network. It is fine for hosts with /64 prefix. However,
there are providers who would assign the same /64 prefix to multiple
hosts, such as Leaseweb. In that case, this random ipv6 generator fails
to properly allocate an ipv6 prefix from the host's network.

To fix this issue, we reduce the random bit count to a single byte. This
way, if the host has a prefix length of less than 64 but larger than 72,
we can still allocate guest prefixes at the length of /80.

We are also fixing a bug that would cause a crash in case of the host
prefix is too long and bytes_needed returns 0. In that case, the unpack1
call would crash since SecureRandom.random_bytes(0) returns nil. We
simplify the whole function by simply generating a random_number in the
range of [2..255].
@furkansahin
Copy link
Contributor Author

I have updated the commit message, thanks!

@furkansahin furkansahin changed the title Support Hosts with IPv6 subnet smaller than /64 Refactor IPv6 generator to support hosts with smaller than prefix 64 May 28, 2024
@furkansahin furkansahin merged commit 543e912 into main May 30, 2024
7 checks passed
@furkansahin furkansahin deleted the support_smaller_ip6_net branch May 30, 2024 06:54
@github-actions github-actions bot locked and limited conversation to collaborators May 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants