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

node: Remove the IPv6 prefix /96 constraint #9777

Merged
merged 3 commits into from Dec 18, 2019

Conversation

brb
Copy link
Member

@brb brb commented Dec 17, 2019

This PR removes the IPv6 /96 prefix constraint which allows cilium to be used with k8s in the dual-stack mode.

Previously, the DSR for IPv6 (aka the --lb feature) required the --ipv6-range to be /96. This was because the 113-128 bits of an IPv6 addr were used to store a global reverse NAT ID, and the rest was a unique Pod IPv6 addr.

As the --lb feature was removed, the constraint is no longer needed (the new DSR implementation leverages IPv6 extension field to store a SVC IP addr and port).

Tested manually with kubeadm (kubeadm init --skip-phases=addon/kube-proxy --pod-network-cidr=10.217.0.0/16,fc22::/48 --service-cidr=10.96.0.0/16,fc44::/112 --feature-gates IPv6DualStack=true --apiserver-advertise-address=192.168.34.11).

Remove /96 IPv6 CIDR constraint which makes cilium to work in k8s dual-stack mode

This change is Reviewable

@brb
Copy link
Member Author

brb commented Dec 17, 2019

test-me-please

@coveralls
Copy link

coveralls commented Dec 17, 2019

Coverage Status

Coverage increased (+0.01%) to 45.952% when pulling 03b9025 on pr/brb/rm-ipv6-prefix-constraint into 131fb89 on master.

Previously, the DSR for IPv6 (aka the "--lb" feature) required
the "--ipv6-range" to be /96. This was because the 113-128 bits of
an IPv6 addr were used to store a global reverse NAT ID.

As the "--lb" feature was removed, the constraint is no longer needed,
thus it can be removed.

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb brb force-pushed the pr/brb/rm-ipv6-prefix-constraint branch from 3578324 to abb8e33 Compare December 17, 2019 16:16
@brb brb changed the title WIP: remove /112 ipv6 contraint node: Remove the IPv6 prefix /96 constraint Dec 17, 2019
Neither Node or Endpoint IDs are stored in an IPv6 address.

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb brb added pending-review area/daemon Impacts operation of the Cilium daemon. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Dec 17, 2019
@brb brb marked this pull request as ready for review December 17, 2019 16:21
@brb brb requested a review from a team December 17, 2019 16:21
@brb brb requested review from a team as code owners December 17, 2019 16:21
@brb brb requested a review from a team December 17, 2019 16:21
@brb
Copy link
Member Author

brb commented Dec 17, 2019

test-me-please

@brb brb requested a review from aanm December 17, 2019 16:22
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

If we still have (and use) IPv6NodePrefixLen then I'm assuming that the prefix must still be /96 or shorter?

// The IPv6 allocation should be derived from the IPv4 allocation.
ip := ipv4AllocRange.IP
v6range := fmt.Sprintf("%s%02x%02x:%02x%02x:0:0/%d",
option.Config.IPv6ClusterAllocCIDRBase, ip[0], ip[1], ip[2], ip[3],
defaults.IPv6NodePrefixLen)

@aanm
Copy link
Member

aanm commented Dec 17, 2019

test-me-please

@brb
Copy link
Member Author

brb commented Dec 18, 2019

If we still have (and use) IPv6NodePrefixLen then I'm assuming that the prefix must still be /96 or shorter?

@joestringer Good question. This is for a different case when an IPv6 alloc range is derived from --ipv4-range and--ipv6-cluster-alloc-cidr (/64). The case is not used in the dual-stack mode.

I'm going to remove defaults.IPv6NodePrefixLen and hard-code the 96 prefix instead to make it less confusing.

Remove the "IPv6NodePrefixLen" const (=96), as from now we can
allocate IPv6 addrs from any size CIDR.

The const is only used by a case when an IPv6 allocation range
is derived from "ipv6-cluster-alloc-cidr" and "ipv4-range"

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb
Copy link
Member Author

brb commented Dec 18, 2019

test-me-please

@brb brb requested a review from joestringer December 18, 2019 10:00
@borkmann borkmann merged commit 36af58d into master Dec 18, 2019
@borkmann borkmann deleted the pr/brb/rm-ipv6-prefix-constraint branch December 18, 2019 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/daemon Impacts operation of the Cilium daemon. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants