Skip to content

Commit 7f10b2d

Browse files
openshift-merge-robotopenshift-merge-robot
authored
Merge pull request #16807 from danwinship/validate-ipv4
Automatic merge from submit-queue. Require network API objects to have IPv4 addresses https://bugzilla.redhat.com/show_bug.cgi?id=1500664 points out problems that occur if you specify an IPv6 EgressIP. In fact, we shouldn't be allowing IPv6 addresses for any network API objects: - `ClusterNetwork.Network` / `ClusterNetwork.ClusterNetworks[].CIDR` must be IPv4 because we only support an IPv4 SDN right now, for multiple reasons throughout kube and the SDN code. (Among other things, the HostSubnet allocator implicitly assumes IPv4, so while it's currently possible to start up a master with an IPv6 `clusterNetworkCIDR` value, it won't ever succeed in allocating any HostSubnets, so no nodes will ever successfully start up.) - `ClusterNetwork.ServiceNetwork` must be IPv4 because kube-proxy only supports all-IPv4 or all-IPv6, and service rules must be able to refer to pod IPs, and since those are IPv4, then service IPs must be IPv4 too. - `HostSubnet.HostIP` must be IPv4 because the node's IP address is used in kube-proxy rules, which per the above must be IPv4 only - `HostSubnet.Subnet` must be IPv4 because it's a subset of the cluster network, which is IPv4 - `HostSubnet.EgressIPs` must be IPv4 because they are used for NATting in iptables rules. - `NetNamespace.EgressIPs` must be IPv4 because they have to match some hostsubnet's `EgressIPs`. - `EgressNetworkPolicyPeer.CIDRSelector` ... well, ... actually I guess these don't *need* to be IPv4; it's pointless to specify an IPv6 value here since pods don't have IPv6 connectivity, but nothing will break if you do. So this PR requires all of the above except `EgressNetworkPolicyPeer.CIDRSelector` to be IPv4.
2 parents 1f88b28 + f98ead5 commit 7f10b2d

File tree

2 files changed

+100
-10
lines changed

2 files changed

+100
-10
lines changed

Diff for: ‎pkg/network/apis/network/validation/validation.go

+31-8
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,28 @@ import (
1616
"github.com/openshift/origin/pkg/util/netutils"
1717
)
1818

19+
func validateCIDRv4(cidr string) (*net.IPNet, error) {
20+
ipnet, err := netutils.ParseCIDRMask(cidr)
21+
if err != nil {
22+
return nil, err
23+
}
24+
if ipnet.IP.To4() == nil {
25+
return nil, fmt.Errorf("must be an IPv4 network")
26+
}
27+
return ipnet, nil
28+
}
29+
30+
func validateIPv4(ip string) (net.IP, error) {
31+
bytes := net.ParseIP(ip)
32+
if bytes == nil {
33+
return nil, fmt.Errorf("invalid IP address")
34+
}
35+
if bytes.To4() == nil {
36+
return nil, fmt.Errorf("must be an IPv4 address")
37+
}
38+
return bytes, nil
39+
}
40+
1941
var defaultClusterNetwork *networkapi.ClusterNetwork
2042

2143
// SetDefaultClusterNetwork sets the expected value of the default ClusterNetwork record
@@ -31,7 +53,7 @@ func ValidateClusterNetwork(clusterNet *networkapi.ClusterNetwork) field.ErrorLi
3153
// This check is mainly for ensuring backward compatibility
3254
if len(clusterNet.Network) != 0 || clusterNet.HostSubnetLength != 0 {
3355
//In the case that a user manually makes a clusterNetwork object with clusterNet.Network and clusterNet.HostubnetLength at least make sure they are valid values
34-
clusterIPNet, err := netutils.ParseCIDRMask(clusterNet.Network)
56+
clusterIPNet, err := validateCIDRv4(clusterNet.Network)
3557
if err != nil {
3658
allErrs = append(allErrs, field.Invalid(field.NewPath("network"), clusterNet.Network, err.Error()))
3759
} else {
@@ -47,12 +69,12 @@ func ValidateClusterNetwork(clusterNet *networkapi.ClusterNetwork) field.ErrorLi
4769
if len(clusterNet.ClusterNetworks) == 0 && len(clusterNet.Network) == 0 {
4870
allErrs = append(allErrs, field.Invalid(field.NewPath("clusterNetworks"), clusterNet.ClusterNetworks, "must have at least one cluster network CIDR"))
4971
}
50-
serviceIPNet, err := netutils.ParseCIDRMask(clusterNet.ServiceNetwork)
72+
serviceIPNet, err := validateCIDRv4(clusterNet.ServiceNetwork)
5173
if err != nil {
5274
allErrs = append(allErrs, field.Invalid(field.NewPath("serviceNetwork"), clusterNet.ServiceNetwork, err.Error()))
5375
}
5476
for i, cn := range clusterNet.ClusterNetworks {
55-
clusterIPNet, err := netutils.ParseCIDRMask(cn.CIDR)
77+
clusterIPNet, err := validateCIDRv4(cn.CIDR)
5678
if err != nil {
5779
allErrs = append(allErrs, field.Invalid(field.NewPath("clusterNetworks").Index(i).Child("cidr"), cn.CIDR, err.Error()))
5880
continue
@@ -116,18 +138,19 @@ func ValidateHostSubnet(hs *networkapi.HostSubnet) field.ErrorList {
116138
allErrs = append(allErrs, field.Invalid(field.NewPath("subnet"), hs.Subnet, "field cannot be empty"))
117139
}
118140
} else {
119-
_, err := netutils.ParseCIDRMask(hs.Subnet)
141+
_, err := validateCIDRv4(hs.Subnet)
120142
if err != nil {
121143
allErrs = append(allErrs, field.Invalid(field.NewPath("subnet"), hs.Subnet, err.Error()))
122144
}
123145
}
146+
// In theory this has to be IPv4, but it's possible some clusters might be limping along with IPv6 values?
124147
if net.ParseIP(hs.HostIP) == nil {
125148
allErrs = append(allErrs, field.Invalid(field.NewPath("hostIP"), hs.HostIP, "invalid IP address"))
126149
}
127150

128151
for i, egressIP := range hs.EgressIPs {
129-
if net.ParseIP(egressIP) == nil {
130-
allErrs = append(allErrs, field.Invalid(field.NewPath("egressIPs").Index(i), egressIP, "invalid IP address"))
152+
if _, err := validateIPv4(egressIP); err != nil {
153+
allErrs = append(allErrs, field.Invalid(field.NewPath("egressIPs").Index(i), egressIP, err.Error()))
131154
}
132155
}
133156

@@ -158,8 +181,8 @@ func ValidateNetNamespace(netnamespace *networkapi.NetNamespace) field.ErrorList
158181
}
159182

160183
for i, ip := range netnamespace.EgressIPs {
161-
if net.ParseIP(ip) == nil {
162-
allErrs = append(allErrs, field.Invalid(field.NewPath("egressIPs").Index(i), ip, "invalid IP address"))
184+
if _, err := validateIPv4(ip); err != nil {
185+
allErrs = append(allErrs, field.Invalid(field.NewPath("egressIPs").Index(i), ip, err.Error()))
163186
}
164187
}
165188

Diff for: ‎pkg/network/apis/network/validation/validation_test.go

+69-2
Original file line numberDiff line numberDiff line change
@@ -154,6 +154,24 @@ func TestValidateClusterNetwork(t *testing.T) {
154154
},
155155
expectedErrors: 1,
156156
},
157+
{
158+
name: "IPv6 ClusterNetwork",
159+
cn: &networkapi.ClusterNetwork{
160+
ObjectMeta: metav1.ObjectMeta{Name: "any"},
161+
ClusterNetworks: []networkapi.ClusterNetworkEntry{{CIDR: "fe80:1234::/64", HostSubnetLength: 8}},
162+
ServiceNetwork: "172.30.0.0/16",
163+
},
164+
expectedErrors: 1,
165+
},
166+
{
167+
name: "IPv6 ServiceNetwork",
168+
cn: &networkapi.ClusterNetwork{
169+
ObjectMeta: metav1.ObjectMeta{Name: "any"},
170+
ClusterNetworks: []networkapi.ClusterNetworkEntry{{CIDR: "10.20.0.0/16", HostSubnetLength: 8}},
171+
ServiceNetwork: "fe80:1234::/64",
172+
},
173+
expectedErrors: 1,
174+
},
157175
}
158176

159177
for _, tc := range tests {
@@ -331,6 +349,34 @@ func TestValidateHostSubnet(t *testing.T) {
331349
},
332350
expectedErrors: 2,
333351
},
352+
{
353+
name: "IPv6 subnet",
354+
hs: &networkapi.HostSubnet{
355+
ObjectMeta: metav1.ObjectMeta{
356+
Name: "abc.def.com",
357+
},
358+
Host: "abc.def.com",
359+
HostIP: "10.20.30.40",
360+
Subnet: "fe80:1234::/64",
361+
},
362+
expectedErrors: 1,
363+
},
364+
{
365+
name: "IPv6 EgressIP",
366+
hs: &networkapi.HostSubnet{
367+
ObjectMeta: metav1.ObjectMeta{
368+
Name: "abc.def.com",
369+
},
370+
Host: "abc.def.com",
371+
HostIP: "10.20.30.40",
372+
Subnet: "8.8.8.0/24",
373+
EgressIPs: []string{
374+
"192.168.1.99",
375+
"1234::5678",
376+
},
377+
},
378+
expectedErrors: 1,
379+
},
334380
}
335381

336382
for _, tc := range tests {
@@ -412,9 +458,9 @@ func TestValidateNetNamespace(t *testing.T) {
412458
},
413459
NetName: "abc",
414460
NetID: 12345,
415-
EgressIPs: []string{"example.com", ""},
461+
EgressIPs: []string{"example.com", "", "1234::5678"},
416462
},
417-
expectedErrors: 2,
463+
expectedErrors: 3,
418464
},
419465
}
420466

@@ -615,6 +661,27 @@ func TestValidateEgressNetworkPolicy(t *testing.T) {
615661
},
616662
expectedErrors: 1,
617663
},
664+
{
665+
// IPv6 CIDRSelectors are currently useless, but they don't break anything
666+
name: "IPv6 CIDR",
667+
fw: &networkapi.EgressNetworkPolicy{
668+
ObjectMeta: metav1.ObjectMeta{
669+
Name: "default",
670+
Namespace: "testing",
671+
},
672+
Spec: networkapi.EgressNetworkPolicySpec{
673+
Egress: []networkapi.EgressNetworkPolicyRule{
674+
{
675+
Type: networkapi.EgressNetworkPolicyRuleAllow,
676+
To: networkapi.EgressNetworkPolicyPeer{
677+
CIDRSelector: "fe80::/64",
678+
},
679+
},
680+
},
681+
},
682+
},
683+
expectedErrors: 0,
684+
},
618685
}
619686

620687
for _, tc := range tests {

0 commit comments

Comments
 (0)