Add utilvalidation.GetWarningsForIP and .GetWarningsForCIDR

(And port the existing Service warnings to use them.)
This commit is contained in:
Dan Winship 2024-02-16 12:07:56 -05:00
parent 34717000da
commit 610adebdb7
4 changed files with 232 additions and 155 deletions

View File

@ -18,8 +18,8 @@ package service
import (
"fmt"
"net/netip"
utilvalidation "k8s.io/apimachinery/pkg/util/validation"
"k8s.io/apimachinery/pkg/util/validation/field"
api "k8s.io/kubernetes/pkg/apis/core"
"k8s.io/kubernetes/pkg/apis/core/helper"
@ -37,20 +37,20 @@ func GetWarningsForService(service, oldService *api.Service) []string {
if helper.IsServiceIPSet(service) {
for i, clusterIP := range service.Spec.ClusterIPs {
warnings = append(warnings, getWarningsForIP(field.NewPath("spec").Child("clusterIPs").Index(i), clusterIP)...)
warnings = append(warnings, utilvalidation.GetWarningsForIP(field.NewPath("spec").Child("clusterIPs").Index(i), clusterIP)...)
}
}
for i, externalIP := range service.Spec.ExternalIPs {
warnings = append(warnings, getWarningsForIP(field.NewPath("spec").Child("externalIPs").Index(i), externalIP)...)
warnings = append(warnings, utilvalidation.GetWarningsForIP(field.NewPath("spec").Child("externalIPs").Index(i), externalIP)...)
}
if len(service.Spec.LoadBalancerIP) > 0 {
warnings = append(warnings, getWarningsForIP(field.NewPath("spec").Child("loadBalancerIP"), service.Spec.LoadBalancerIP)...)
warnings = append(warnings, utilvalidation.GetWarningsForIP(field.NewPath("spec").Child("loadBalancerIP"), service.Spec.LoadBalancerIP)...)
}
for i, cidr := range service.Spec.LoadBalancerSourceRanges {
warnings = append(warnings, getWarningsForCIDR(field.NewPath("spec").Child("loadBalancerSourceRanges").Index(i), cidr)...)
warnings = append(warnings, utilvalidation.GetWarningsForCIDR(field.NewPath("spec").Child("loadBalancerSourceRanges").Index(i), cidr)...)
}
if service.Spec.Type == api.ServiceTypeExternalName && len(service.Spec.ExternalIPs) > 0 {
@ -62,45 +62,3 @@ func GetWarningsForService(service, oldService *api.Service) []string {
return warnings
}
func getWarningsForIP(fieldPath *field.Path, address string) []string {
// IPv4 addresses with leading zeros CVE-2021-29923 are not valid in golang since 1.17
// This will also warn about possible future changes on the golang std library
// xref: https://issues.k8s.io/108074
ip, err := netip.ParseAddr(address)
if err != nil {
return []string{fmt.Sprintf("%s: IP address was accepted, but will be invalid in a future Kubernetes release: %v", fieldPath, err)}
}
// A Recommendation for IPv6 Address Text Representation
//
// "All of the above examples represent the same IPv6 address. This
// flexibility has caused many problems for operators, systems
// engineers, and customers.
// ..."
// https://datatracker.ietf.org/doc/rfc5952/
if ip.Is6() && ip.String() != address {
return []string{fmt.Sprintf("%s: IPv6 address %q is not in RFC 5952 canonical format (%q), which may cause controller apply-loops", fieldPath, address, ip.String())}
}
return []string{}
}
func getWarningsForCIDR(fieldPath *field.Path, cidr string) []string {
// IPv4 addresses with leading zeros CVE-2021-29923 are not valid in golang since 1.17
// This will also warn about possible future changes on the golang std library
// xref: https://issues.k8s.io/108074
prefix, err := netip.ParsePrefix(cidr)
if err != nil {
return []string{fmt.Sprintf("%s: IP prefix was accepted, but will be invalid in a future Kubernetes release: %v", fieldPath, err)}
}
// A Recommendation for IPv6 Address Text Representation
//
// "All of the above examples represent the same IPv6 address. This
// flexibility has caused many problems for operators, systems
// engineers, and customers.
// ..."
// https://datatracker.ietf.org/doc/rfc5952/
if prefix.Addr().Is6() && prefix.String() != cidr {
return []string{fmt.Sprintf("%s: IPv6 prefix %q is not in RFC 5952 canonical format (%q), which may cause controller apply-loops", fieldPath, cidr, prefix.String())}
}
return []string{}
}

View File

@ -22,7 +22,6 @@ import (
"github.com/google/go-cmp/cmp"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/validation/field"
api "k8s.io/kubernetes/pkg/apis/core"
)
@ -108,58 +107,58 @@ func TestGetWarningsForServiceClusterIPs(t *testing.T) {
name: "IPv4 with leading zeros",
service: service([]string{"192.012.2.2"}),
want: []string{
`spec.clusterIPs[0]: IP address was accepted, but will be invalid in a future Kubernetes release: ParseAddr("192.012.2.2"): IPv4 field has octet with leading zero`,
`spec.clusterIPs[0]: non-standard IP address "192.012.2.2" will be considered invalid in a future Kubernetes release: use "192.12.2.2"`,
},
},
{
name: "Dual Stack IPv4-IPv6 and IPv4 with leading zeros",
service: service([]string{"192.012.2.2", "2001:db8::2"}),
want: []string{
`spec.clusterIPs[0]: IP address was accepted, but will be invalid in a future Kubernetes release: ParseAddr("192.012.2.2"): IPv4 field has octet with leading zero`,
`spec.clusterIPs[0]: non-standard IP address "192.012.2.2" will be considered invalid in a future Kubernetes release: use "192.12.2.2"`,
},
},
{
name: "Dual Stack IPv6-IPv4 and IPv4 with leading zeros",
service: service([]string{"2001:db8::2", "192.012.2.2"}),
want: []string{
`spec.clusterIPs[1]: IP address was accepted, but will be invalid in a future Kubernetes release: ParseAddr("192.012.2.2"): IPv4 field has octet with leading zero`,
`spec.clusterIPs[1]: non-standard IP address "192.012.2.2" will be considered invalid in a future Kubernetes release: use "192.12.2.2"`,
},
},
{
name: "IPv6 non canonical format",
service: service([]string{"2001:db8:0:0::2"}),
want: []string{
`spec.clusterIPs[0]: IPv6 address "2001:db8:0:0::2" is not in RFC 5952 canonical format ("2001:db8::2"), which may cause controller apply-loops`,
`spec.clusterIPs[0]: IPv6 address "2001:db8:0:0::2" should be in RFC 5952 canonical format ("2001:db8::2")`,
},
},
{
name: "Dual Stack IPv4-IPv6 and IPv6 non-canonical format",
service: service([]string{"192.12.2.2", "2001:db8:0:0::2"}),
want: []string{
`spec.clusterIPs[1]: IPv6 address "2001:db8:0:0::2" is not in RFC 5952 canonical format ("2001:db8::2"), which may cause controller apply-loops`,
`spec.clusterIPs[1]: IPv6 address "2001:db8:0:0::2" should be in RFC 5952 canonical format ("2001:db8::2")`,
},
},
{
name: "Dual Stack IPv6-IPv4 and IPv6 non-canonical formats",
service: service([]string{"2001:db8:0:0::2", "192.12.2.2"}),
want: []string{
`spec.clusterIPs[0]: IPv6 address "2001:db8:0:0::2" is not in RFC 5952 canonical format ("2001:db8::2"), which may cause controller apply-loops`,
`spec.clusterIPs[0]: IPv6 address "2001:db8:0:0::2" should be in RFC 5952 canonical format ("2001:db8::2")`,
},
},
{
name: "Dual Stack IPv4-IPv6 and IPv4 with leading zeros and IPv6 non-canonical format",
service: service([]string{"192.012.2.2", "2001:db8:0:0::2"}),
want: []string{
`spec.clusterIPs[0]: IP address was accepted, but will be invalid in a future Kubernetes release: ParseAddr("192.012.2.2"): IPv4 field has octet with leading zero`,
`spec.clusterIPs[1]: IPv6 address "2001:db8:0:0::2" is not in RFC 5952 canonical format ("2001:db8::2"), which may cause controller apply-loops`,
`spec.clusterIPs[0]: non-standard IP address "192.012.2.2" will be considered invalid in a future Kubernetes release: use "192.12.2.2"`,
`spec.clusterIPs[1]: IPv6 address "2001:db8:0:0::2" should be in RFC 5952 canonical format ("2001:db8::2")`,
},
},
{
name: "Dual Stack IPv6-IPv4 and IPv4 with leading zeros and IPv6 non-canonical format",
service: service([]string{"2001:db8:0:0::2", "192.012.2.2"}),
want: []string{
`spec.clusterIPs[0]: IPv6 address "2001:db8:0:0::2" is not in RFC 5952 canonical format ("2001:db8::2"), which may cause controller apply-loops`,
`spec.clusterIPs[1]: IP address was accepted, but will be invalid in a future Kubernetes release: ParseAddr("192.012.2.2"): IPv4 field has octet with leading zero`,
`spec.clusterIPs[0]: IPv6 address "2001:db8:0:0::2" should be in RFC 5952 canonical format ("2001:db8::2")`,
`spec.clusterIPs[1]: non-standard IP address "192.012.2.2" will be considered invalid in a future Kubernetes release: use "192.12.2.2"`,
},
},
{
@ -179,13 +178,13 @@ func TestGetWarningsForServiceClusterIPs(t *testing.T) {
},
},
want: []string{
`spec.clusterIPs[0]: IPv6 address "2001:db8:0:0::2" is not in RFC 5952 canonical format ("2001:db8::2"), which may cause controller apply-loops`,
`spec.clusterIPs[1]: IP address was accepted, but will be invalid in a future Kubernetes release: ParseAddr("192.012.2.2"): IPv4 field has octet with leading zero`,
`spec.externalIPs[0]: IPv6 address "2001:db8:1:0::2" is not in RFC 5952 canonical format ("2001:db8:1::2"), which may cause controller apply-loops`,
`spec.externalIPs[1]: IP address was accepted, but will be invalid in a future Kubernetes release: ParseAddr("10.012.2.2"): IPv4 field has octet with leading zero`,
`spec.loadBalancerIP: IP address was accepted, but will be invalid in a future Kubernetes release: ParseAddr("10.001.1.1"): IPv4 field has octet with leading zero`,
`spec.loadBalancerSourceRanges[0]: IPv6 prefix "2001:db8:1:0::/64" is not in RFC 5952 canonical format ("2001:db8:1::/64"), which may cause controller apply-loops`,
`spec.loadBalancerSourceRanges[1]: IP prefix was accepted, but will be invalid in a future Kubernetes release: netip.ParsePrefix("10.012.2.0/24"): ParseAddr("10.012.2.0"): IPv4 field has octet with leading zero`,
`spec.clusterIPs[0]: IPv6 address "2001:db8:0:0::2" should be in RFC 5952 canonical format ("2001:db8::2")`,
`spec.clusterIPs[1]: non-standard IP address "192.012.2.2" will be considered invalid in a future Kubernetes release: use "192.12.2.2"`,
`spec.externalIPs[0]: IPv6 address "2001:db8:1:0::2" should be in RFC 5952 canonical format ("2001:db8:1::2")`,
`spec.externalIPs[1]: non-standard IP address "10.012.2.2" will be considered invalid in a future Kubernetes release: use "10.12.2.2"`,
`spec.loadBalancerIP: non-standard IP address "10.001.1.1" will be considered invalid in a future Kubernetes release: use "10.1.1.1"`,
`spec.loadBalancerSourceRanges[0]: IPv6 CIDR value "2001:db8:1:0::/64" should be in RFC 5952 canonical format ("2001:db8:1::/64")`,
`spec.loadBalancerSourceRanges[1]: non-standard CIDR value "10.012.2.0/24" will be considered invalid in a future Kubernetes release: use "10.12.2.0/24"`,
},
},
}
@ -197,93 +196,3 @@ func TestGetWarningsForServiceClusterIPs(t *testing.T) {
})
}
}
func Test_getWarningsForIP(t *testing.T) {
tests := []struct {
name string
fieldPath *field.Path
address string
want []string
}{
{
name: "IPv4 No failures",
address: "192.12.2.2",
fieldPath: field.NewPath("spec").Child("clusterIPs").Index(0),
want: []string{},
},
{
name: "IPv6 No failures",
address: "2001:db8::2",
fieldPath: field.NewPath("spec").Child("clusterIPs").Index(0),
want: []string{},
},
{
name: "IPv4 with leading zeros",
address: "192.012.2.2",
fieldPath: field.NewPath("spec").Child("clusterIPs").Index(0),
want: []string{
`spec.clusterIPs[0]: IP address was accepted, but will be invalid in a future Kubernetes release: ParseAddr("192.012.2.2"): IPv4 field has octet with leading zero`,
},
},
{
name: "IPv6 non-canonical format",
address: "2001:db8:0:0::2",
fieldPath: field.NewPath("spec").Child("loadBalancerIP"),
want: []string{
`spec.loadBalancerIP: IPv6 address "2001:db8:0:0::2" is not in RFC 5952 canonical format ("2001:db8::2"), which may cause controller apply-loops`,
},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if got := getWarningsForIP(tt.fieldPath, tt.address); !reflect.DeepEqual(got, tt.want) {
t.Errorf("getWarningsForIP() = %v, want %v", got, tt.want)
}
})
}
}
func Test_getWarningsForCIDR(t *testing.T) {
tests := []struct {
name string
fieldPath *field.Path
cidr string
want []string
}{
{
name: "IPv4 No failures",
cidr: "192.12.2.0/24",
fieldPath: field.NewPath("spec").Child("loadBalancerSourceRanges").Index(0),
want: []string{},
},
{
name: "IPv6 No failures",
cidr: "2001:db8::/64",
fieldPath: field.NewPath("spec").Child("loadBalancerSourceRanges").Index(0),
want: []string{},
},
{
name: "IPv4 with leading zeros",
cidr: "192.012.2.0/24",
fieldPath: field.NewPath("spec").Child("loadBalancerSourceRanges").Index(0),
want: []string{
`spec.loadBalancerSourceRanges[0]: IP prefix was accepted, but will be invalid in a future Kubernetes release: netip.ParsePrefix("192.012.2.0/24"): ParseAddr("192.012.2.0"): IPv4 field has octet with leading zero`,
},
},
{
name: "IPv6 non-canonical format",
cidr: "2001:db8:0:0::/64",
fieldPath: field.NewPath("spec").Child("loadBalancerSourceRanges").Index(0),
want: []string{
`spec.loadBalancerSourceRanges[0]: IPv6 prefix "2001:db8:0:0::/64" is not in RFC 5952 canonical format ("2001:db8::/64"), which may cause controller apply-loops`,
},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if got := getWarningsForCIDR(tt.fieldPath, tt.cidr); !reflect.DeepEqual(got, tt.want) {
t.Errorf("getWarningsForCIDR() = %v, want %v", got, tt.want)
}
})
}
}

View File

@ -17,7 +17,11 @@ limitations under the License.
package validation
import (
"fmt"
"net/netip"
"k8s.io/apimachinery/pkg/util/validation/field"
"k8s.io/klog/v2"
netutils "k8s.io/utils/net"
)
@ -30,6 +34,36 @@ func IsValidIP(fldPath *field.Path, value string) field.ErrorList {
return allErrors
}
// GetWarningsForIP returns warnings for IP address values in non-standard forms. This
// should only be used with fields that are validated with IsValidIP().
func GetWarningsForIP(fldPath *field.Path, value string) []string {
ip := netutils.ParseIPSloppy(value)
if ip == nil {
klog.ErrorS(nil, "GetWarningsForIP called on value that was not validated with IsValidIP", "field", fldPath, "value", value)
return nil
}
addr, _ := netip.ParseAddr(value)
if !addr.IsValid() || addr.Is4In6() {
// This catches 2 cases: leading 0s (if ParseIPSloppy() accepted it but
// ParseAddr() doesn't) or IPv4-mapped IPv6 (.Is4In6()). Either way,
// re-stringifying the net.IP value will give the preferred form.
return []string{
fmt.Sprintf("%s: non-standard IP address %q will be considered invalid in a future Kubernetes release: use %q", fldPath, value, ip.String()),
}
}
// If ParseIPSloppy() and ParseAddr() both accept it then it's fully valid, though
// it may be non-canonical.
if addr.Is6() && addr.String() != value {
return []string{
fmt.Sprintf("%s: IPv6 address %q should be in RFC 5952 canonical format (%q)", fldPath, value, addr.String()),
}
}
return nil
}
// IsValidIPv4Address tests that the argument is a valid IPv4 address.
func IsValidIPv4Address(fldPath *field.Path, value string) field.ErrorList {
var allErrors field.ErrorList
@ -59,3 +93,47 @@ func IsValidCIDR(fldPath *field.Path, value string) field.ErrorList {
}
return allErrors
}
// GetWarningsForCIDR returns warnings for CIDR values in non-standard forms. This should
// only be used with fields that are validated with IsValidCIDR().
func GetWarningsForCIDR(fldPath *field.Path, value string) []string {
ip, ipnet, err := netutils.ParseCIDRSloppy(value)
if err != nil {
klog.ErrorS(err, "GetWarningsForCIDR called on value that was not validated with IsValidCIDR", "field", fldPath, "value", value)
return nil
}
var warnings []string
// Check for bits set after prefix length
if !ip.Equal(ipnet.IP) {
_, addrlen := ipnet.Mask.Size()
singleIPCIDR := fmt.Sprintf("%s/%d", ip.String(), addrlen)
warnings = append(warnings,
fmt.Sprintf("%s: CIDR value %q is ambiguous in this context (should be %q or %q?)", fldPath, value, ipnet.String(), singleIPCIDR),
)
}
prefix, _ := netip.ParsePrefix(value)
addr := prefix.Addr()
if !prefix.IsValid() || addr.Is4In6() {
// This catches 2 cases: leading 0s (if ParseCIDRSloppy() accepted it but
// ParsePrefix() doesn't) or IPv4-mapped IPv6 (.Is4In6()). Either way,
// re-stringifying the net.IPNet value will give the preferred form.
warnings = append(warnings,
fmt.Sprintf("%s: non-standard CIDR value %q will be considered invalid in a future Kubernetes release: use %q", fldPath, value, ipnet.String()),
)
}
// If ParseCIDRSloppy() and ParsePrefix() both accept it then it's fully valid,
// though it may be non-canonical. But only check this if there are no other
// warnings, since either of the other warnings would also cause a round-trip
// failure.
if len(warnings) == 0 && addr.Is6() && prefix.String() != value {
warnings = append(warnings,
fmt.Sprintf("%s: IPv6 CIDR value %q should be in RFC 5952 canonical format (%q)", fldPath, value, prefix.String()),
)
}
return warnings
}

View File

@ -17,6 +17,7 @@ limitations under the License.
package validation
import (
"reflect"
"strings"
"testing"
@ -197,6 +198,59 @@ func TestIsValidIP(t *testing.T) {
}
}
func TestGetWarningsForIP(t *testing.T) {
tests := []struct {
name string
fieldPath *field.Path
address string
want []string
}{
{
name: "IPv4 No failures",
address: "192.12.2.2",
fieldPath: field.NewPath("spec").Child("clusterIPs").Index(0),
want: nil,
},
{
name: "IPv6 No failures",
address: "2001:db8::2",
fieldPath: field.NewPath("spec").Child("clusterIPs").Index(0),
want: nil,
},
{
name: "IPv4 with leading zeros",
address: "192.012.2.2",
fieldPath: field.NewPath("spec").Child("clusterIPs").Index(0),
want: []string{
`spec.clusterIPs[0]: non-standard IP address "192.012.2.2" will be considered invalid in a future Kubernetes release: use "192.12.2.2"`,
},
},
{
name: "IPv4-mapped IPv6",
address: "::ffff:192.12.2.2",
fieldPath: field.NewPath("spec").Child("clusterIPs").Index(0),
want: []string{
`spec.clusterIPs[0]: non-standard IP address "::ffff:192.12.2.2" will be considered invalid in a future Kubernetes release: use "192.12.2.2"`,
},
},
{
name: "IPv6 non-canonical format",
address: "2001:db8:0:0::2",
fieldPath: field.NewPath("spec").Child("loadBalancerIP"),
want: []string{
`spec.loadBalancerIP: IPv6 address "2001:db8:0:0::2" should be in RFC 5952 canonical format ("2001:db8::2")`,
},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if got := GetWarningsForIP(tt.fieldPath, tt.address); !reflect.DeepEqual(got, tt.want) {
t.Errorf("getWarningsForIP() = %v, want %v", got, tt.want)
}
})
}
}
func TestIsValidCIDR(t *testing.T) {
for _, tc := range []struct {
name string
@ -318,3 +372,81 @@ func TestIsValidCIDR(t *testing.T) {
})
}
}
func TestGetWarningsForCIDR(t *testing.T) {
tests := []struct {
name string
fieldPath *field.Path
cidr string
want []string
}{
{
name: "IPv4 No failures",
cidr: "192.12.2.0/24",
fieldPath: field.NewPath("spec").Child("loadBalancerSourceRanges").Index(0),
want: nil,
},
{
name: "IPv6 No failures",
cidr: "2001:db8::/64",
fieldPath: field.NewPath("spec").Child("loadBalancerSourceRanges").Index(0),
want: nil,
},
{
name: "IPv4 with leading zeros",
cidr: "192.012.2.0/24",
fieldPath: field.NewPath("spec").Child("loadBalancerSourceRanges").Index(0),
want: []string{
`spec.loadBalancerSourceRanges[0]: non-standard CIDR value "192.012.2.0/24" will be considered invalid in a future Kubernetes release: use "192.12.2.0/24"`,
},
},
{
name: "leading zeros in prefix length",
cidr: "192.12.2.0/024",
fieldPath: field.NewPath("spec").Child("loadBalancerSourceRanges").Index(0),
want: []string{
`spec.loadBalancerSourceRanges[0]: non-standard CIDR value "192.12.2.0/024" will be considered invalid in a future Kubernetes release: use "192.12.2.0/24"`,
},
},
{
name: "IPv4-mapped IPv6",
cidr: "::ffff:192.12.2.0/120",
fieldPath: field.NewPath("spec").Child("loadBalancerSourceRanges").Index(0),
want: []string{
`spec.loadBalancerSourceRanges[0]: non-standard CIDR value "::ffff:192.12.2.0/120" will be considered invalid in a future Kubernetes release: use "192.12.2.0/24"`,
},
},
{
name: "bits after prefix length",
cidr: "192.12.2.8/24",
fieldPath: field.NewPath("spec").Child("loadBalancerSourceRanges").Index(0),
want: []string{
`spec.loadBalancerSourceRanges[0]: CIDR value "192.12.2.8/24" is ambiguous in this context (should be "192.12.2.0/24" or "192.12.2.8/32"?)`,
},
},
{
name: "multiple problems",
cidr: "192.012.2.8/24",
fieldPath: field.NewPath("spec").Child("loadBalancerSourceRanges").Index(0),
want: []string{
`spec.loadBalancerSourceRanges[0]: CIDR value "192.012.2.8/24" is ambiguous in this context (should be "192.12.2.0/24" or "192.12.2.8/32"?)`,
`spec.loadBalancerSourceRanges[0]: non-standard CIDR value "192.012.2.8/24" will be considered invalid in a future Kubernetes release: use "192.12.2.0/24"`,
},
},
{
name: "IPv6 non-canonical format",
cidr: "2001:db8:0:0::/64",
fieldPath: field.NewPath("spec").Child("loadBalancerSourceRanges").Index(0),
want: []string{
`spec.loadBalancerSourceRanges[0]: IPv6 CIDR value "2001:db8:0:0::/64" should be in RFC 5952 canonical format ("2001:db8::/64")`,
},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if got := GetWarningsForCIDR(tt.fieldPath, tt.cidr); !reflect.DeepEqual(got, tt.want) {
t.Errorf("getWarningsForCIDR() = %v, want %v", got, tt.want)
}
})
}
}