Initial IPv6 support for GCE #12170
Initial IPv6 support for GCE #12170
Conversation
k8s-ci-robot
added
cncf-cla: yes
pkg/cidr/cidrset.go
Outdated
| func (s Set) WhereIPV4() Set { | ||
| var matching []net.IPNet | ||
| for i := range s.cidrs { | ||
| if s.cidrs[i].IP.To4() != nil { |
There was a problem hiding this comment.
Probably needs to exclude IPv4-mapped IPv6 addresses. See validation.validateCIDR().
There was a problem hiding this comment.
I didn't understand the reference to validateCIDR - was it checking that ip == cidr.IP?
IIUC these IPv4-mapped IPv6 addresses are not publicly routable, and are probably going to be special cases for most networking operations on clouds. We can add them if people start using them, but I think right now they will be an error, which I think is the correct thing to do until we/I understand better what we should do here.
I could change validateCIDR (and friends) to reject them in the validation phase though?
There was a problem hiding this comment.
validateCIDR uses ip.To4() != nil && !strings.Contains(cidr, ":") to exclude IPv4-mapped IPv6 addresses from the IPv4 code path.
There was a problem hiding this comment.
Gotcha - makes sense - thanks! Added!
pkg/model/gcemodel/firewall.go
Outdated
| func ipv6SourceRange(cidrs cidr.Set) []string { | ||
| ipv6s := cidrs.WhereIPV6().ToStrings() | ||
| if len(ipv6s) == 0 { | ||
| ipv6s = append(ipv6s, "::/128") |
There was a problem hiding this comment.
So we have to let at least one address through? No way to express "block all IPv6?"
There was a problem hiding this comment.
Actually I think I found one! We can set disabled=true on the rule - I think that's clearer than both the odd IPs and deleting the rules. (I think!)
pkg/model/gcemodel/firewall.go
Outdated
| t := &gcetasks.FirewallRule{ | ||
| Name: s(b.SafeObjectName("cidr-to-node")), | ||
| Lifecycle: b.Lifecycle, | ||
| Network: b.LinkToNetwork(), | ||
| SourceRanges: []string{b.Cluster.Spec.NonMasqueradeCIDR}, | ||
| SourceRanges: ipv4SourceRange(nonMasqueradeCIDR), | ||
| TargetTags: []string{b.GCETagForRole(kops.InstanceGroupRoleNode)}, | ||
| Allowed: []string{"tcp", "udp", "icmp", "esp", "ah", "sctp"}, | ||
| } |
There was a problem hiding this comment.
Why aren't we building IPv6 firewall rules here?
There was a problem hiding this comment.
It was because they weren't set by default in kops create. However, now we're using a helper function, we do now build the rules for ipv4 and ipv6.
justinsb
force-pushed
the
gce_ipv6
branch
5 times, most recently
from
7d5ddb5
to
88813c0
Compare
Supporting IPv6 values where they can be set by the user, and ensuring that IPv4 and IPv6 firewall rules are split because on GCP they cannot be in the same rule.
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: johngmyers The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
Supporting IPv6 values where they can be set by the user, and ensuring
that IPv4 and IPv6 firewall rules are split because on GCP they cannot
be in the same rule.