Skip to content

Commit ad22c0d

Browse files
danwinshipdanwinship
committed
Fix IP/CIDR validation to allow updates to existing invalid objects
Ignore pre-existing bad IP/CIDR values in: - pod.spec.podIP(s) - pod.spec.hostIP(s) - service.spec.externalIPs - service.spec.clusterIP(s) - service.spec.loadBalancerSourceRanges (and corresponding annotation) - service.status.loadBalancer.ingress[].ip - endpoints.subsets - endpointslice.endpoints - networkpolicy.spec.{ingress[].from[],egress[].to[]}.ipBlock - ingress.status.loadBalancer.ingress[].ip In the Endpoints and EndpointSlice case, if *any* endpoint IP is changed, then the entire object must be valid; invalid IPs are only allowed to remain in place for updates that don't change any IPs. (e.g., changing the labels or annotations). In most of the other cases, when the invalid IP is part of an array, it can be moved around within the array without triggering revalidation.
1 parent 692785d commit ad22c0d

File tree

9 files changed

+693
-99
lines changed

9 files changed

+693
-99
lines changed

‎pkg/apis/core/validation/validation.go‎

Lines changed: 97 additions & 39 deletions
Large diffs are not rendered by default.

‎pkg/apis/core/validation/validation_test.go‎

Lines changed: 310 additions & 31 deletions
Large diffs are not rendered by default.

‎pkg/apis/discovery/validation/validation.go‎

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import (
2020
"fmt"
2121

2222
corev1 "k8s.io/api/core/v1"
23+
apiequality "k8s.io/apimachinery/pkg/api/equality"
2324
apimachineryvalidation "k8s.io/apimachinery/pkg/api/validation"
2425
metavalidation "k8s.io/apimachinery/pkg/apis/meta/v1/validation"
2526
"k8s.io/apimachinery/pkg/util/sets"
@@ -55,23 +56,34 @@ var (
5556
var ValidateEndpointSliceName = apimachineryvalidation.NameIsDNSSubdomain
5657

5758
// ValidateEndpointSlice validates an EndpointSlice.
58-
func ValidateEndpointSlice(endpointSlice *discovery.EndpointSlice) field.ErrorList {
59+
func ValidateEndpointSlice(endpointSlice, oldEndpointSlice *discovery.EndpointSlice) field.ErrorList {
5960
allErrs := apivalidation.ValidateObjectMeta(&endpointSlice.ObjectMeta, true, ValidateEndpointSliceName, field.NewPath("metadata"))
6061
allErrs = append(allErrs, validateAddressType(endpointSlice.AddressType)...)
61-
allErrs = append(allErrs, validateEndpoints(endpointSlice.Endpoints, endpointSlice.AddressType, field.NewPath("endpoints"))...)
6262
allErrs = append(allErrs, validatePorts(endpointSlice.Ports, field.NewPath("ports"))...)
6363

64+
endpointsErrs := validateEndpoints(endpointSlice.Endpoints, endpointSlice.AddressType, field.NewPath("endpoints"))
65+
if len(endpointsErrs) != 0 {
66+
// If this is an update, and Endpoints was unchanged, then ignore the
67+
// validation errors, since apparently older versions of Kubernetes
68+
// considered the data valid. (We only check this after getting a
69+
// validation error since Endpoints may be large and DeepEqual is slow.)
70+
if oldEndpointSlice != nil && apiequality.Semantic.DeepEqual(oldEndpointSlice.Endpoints, endpointSlice.Endpoints) {
71+
endpointsErrs = nil
72+
}
73+
}
74+
allErrs = append(allErrs, endpointsErrs...)
75+
6476
return allErrs
6577
}
6678

6779
// ValidateEndpointSliceCreate validates an EndpointSlice when it is created.
6880
func ValidateEndpointSliceCreate(endpointSlice *discovery.EndpointSlice) field.ErrorList {
69-
return ValidateEndpointSlice(endpointSlice)
81+
return ValidateEndpointSlice(endpointSlice, nil)
7082
}
7183

7284
// ValidateEndpointSliceUpdate validates an EndpointSlice when it is updated.
7385
func ValidateEndpointSliceUpdate(newEndpointSlice, oldEndpointSlice *discovery.EndpointSlice) field.ErrorList {
74-
allErrs := ValidateEndpointSlice(newEndpointSlice)
86+
allErrs := ValidateEndpointSlice(newEndpointSlice, oldEndpointSlice)
7587
allErrs = append(allErrs, apivalidation.ValidateImmutableField(newEndpointSlice.AddressType, oldEndpointSlice.AddressType, field.NewPath("addressType"))...)
7688

7789
return allErrs
@@ -100,7 +112,7 @@ func validateEndpoints(endpoints []discovery.Endpoint, addrType discovery.Addres
100112
// and do not get validated.
101113
switch addrType {
102114
case discovery.AddressTypeIPv4:
103-
ipErrs := apivalidation.IsValidIPForLegacyField(addressPath.Index(i), address)
115+
ipErrs := apivalidation.IsValidIPForLegacyField(addressPath.Index(i), address, nil)
104116
if len(ipErrs) > 0 {
105117
allErrs = append(allErrs, ipErrs...)
106118
} else {

‎pkg/apis/discovery/validation/validation_test.go‎

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -637,7 +637,7 @@ func TestValidateEndpointSlice(t *testing.T) {
637637
for name, testCase := range testCases {
638638
t.Run(name, func(t *testing.T) {
639639
featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, !testCase.legacyIPs)
640-
errs := ValidateEndpointSlice(testCase.endpointSlice)
640+
errs := ValidateEndpointSlice(testCase.endpointSlice, nil)
641641
if len(errs) != testCase.expectedErrors {
642642
t.Errorf("Expected %d errors, got %d errors: %v", testCase.expectedErrors, len(errs), errs)
643643
}
@@ -737,6 +737,7 @@ func TestValidateEndpointSliceCreate(t *testing.T) {
737737

738738
for name, testCase := range testCases {
739739
t.Run(name, func(t *testing.T) {
740+
featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, true)
740741
errs := ValidateEndpointSliceCreate(testCase.endpointSlice)
741742
if len(errs) != testCase.expectedErrors {
742743
t.Errorf("Expected %d errors, got %d errors: %v", testCase.expectedErrors, len(errs), errs)
@@ -765,6 +766,23 @@ func TestValidateEndpointSliceUpdate(t *testing.T) {
765766
},
766767
expectedErrors: 0,
767768
},
769+
"unchanged IPs are not revalidated": {
770+
oldEndpointSlice: &discovery.EndpointSlice{
771+
ObjectMeta: standardMeta,
772+
AddressType: discovery.AddressTypeIPv4,
773+
Endpoints: []discovery.Endpoint{{
774+
Addresses: []string{"10.1.2.04"},
775+
}},
776+
},
777+
newEndpointSlice: &discovery.EndpointSlice{
778+
ObjectMeta: standardMeta,
779+
AddressType: discovery.AddressTypeIPv4,
780+
Endpoints: []discovery.Endpoint{{
781+
Addresses: []string{"10.1.2.04"},
782+
}},
783+
},
784+
expectedErrors: 0,
785+
},
768786

769787
// expected errors
770788
"invalid node name set": {
@@ -823,10 +841,28 @@ func TestValidateEndpointSliceUpdate(t *testing.T) {
823841
},
824842
expectedErrors: 1,
825843
},
844+
"changed IPs are revalidated": {
845+
oldEndpointSlice: &discovery.EndpointSlice{
846+
ObjectMeta: standardMeta,
847+
AddressType: discovery.AddressTypeIPv4,
848+
Endpoints: []discovery.Endpoint{{
849+
Addresses: []string{"10.1.2.3"},
850+
}},
851+
},
852+
newEndpointSlice: &discovery.EndpointSlice{
853+
ObjectMeta: standardMeta,
854+
AddressType: discovery.AddressTypeIPv4,
855+
Endpoints: []discovery.Endpoint{{
856+
Addresses: []string{"10.1.2.03"},
857+
}},
858+
},
859+
expectedErrors: 1,
860+
},
826861
}
827862

828863
for name, testCase := range testCases {
829864
t.Run(name, func(t *testing.T) {
865+
featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, true)
830866
errs := ValidateEndpointSliceUpdate(testCase.newEndpointSlice, testCase.oldEndpointSlice)
831867
if len(errs) != testCase.expectedErrors {
832868
t.Errorf("Expected %d errors, got %d errors: %v", testCase.expectedErrors, len(errs), errs)

‎pkg/apis/networking/validation/validation.go‎

Lines changed: 47 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,7 @@ var (
5656

5757
type NetworkPolicyValidationOptions struct {
5858
AllowInvalidLabelValueInSelector bool
59+
AllowCIDRsEvenIfInvalid []string
5960
}
6061

6162
// ValidateNetworkPolicyName can be used to check whether the given networkpolicy
@@ -118,7 +119,7 @@ func ValidateNetworkPolicyPeer(peer *networking.NetworkPolicyPeer, opts NetworkP
118119
}
119120
if peer.IPBlock != nil {
120121
numPeers++
121-
allErrs = append(allErrs, ValidateIPBlock(peer.IPBlock, peerPath.Child("ipBlock"))...)
122+
allErrs = append(allErrs, ValidateIPBlock(peer.IPBlock, peerPath.Child("ipBlock"), opts)...)
122123
}
123124

124125
if numPeers == 0 {
@@ -206,20 +207,47 @@ func ValidationOptionsForNetworking(new, old *networking.NetworkPolicy) NetworkP
206207

207208
// ValidateNetworkPolicyUpdate tests if an update to a NetworkPolicy is valid.
208209
func ValidateNetworkPolicyUpdate(update, old *networking.NetworkPolicy, opts NetworkPolicyValidationOptions) field.ErrorList {
210+
// Record all existing CIDRs in the policy; see ValidateIPBlock.
211+
var existingCIDRs []string
212+
for _, ingress := range old.Spec.Ingress {
213+
for _, peer := range ingress.From {
214+
if peer.IPBlock != nil {
215+
existingCIDRs = append(existingCIDRs, peer.IPBlock.CIDR)
216+
existingCIDRs = append(existingCIDRs, peer.IPBlock.Except...)
217+
}
218+
}
219+
}
220+
for _, egress := range old.Spec.Egress {
221+
for _, peer := range egress.To {
222+
if peer.IPBlock != nil {
223+
existingCIDRs = append(existingCIDRs, peer.IPBlock.CIDR)
224+
existingCIDRs = append(existingCIDRs, peer.IPBlock.Except...)
225+
}
226+
}
227+
}
228+
opts.AllowCIDRsEvenIfInvalid = existingCIDRs
229+
209230
allErrs := field.ErrorList{}
210231
allErrs = append(allErrs, apivalidation.ValidateObjectMetaUpdate(&update.ObjectMeta, &old.ObjectMeta, field.NewPath("metadata"))...)
211232
allErrs = append(allErrs, ValidateNetworkPolicySpec(&update.Spec, opts, field.NewPath("spec"))...)
212233
return allErrs
213234
}
214235

215-
// ValidateIPBlock validates a cidr and the except fields of an IpBlock NetworkPolicyPeer
216-
func ValidateIPBlock(ipb *networking.IPBlock, fldPath *field.Path) field.ErrorList {
236+
// ValidateIPBlock validates a cidr and the except fields of an IPBlock NetworkPolicyPeer.
237+
//
238+
// If a pre-existing CIDR is invalid/insecure, but it is not being changed by this update,
239+
// then we have to continue allowing it. But since the user may have changed the policy in
240+
// arbitrary ways (adding/removing rules, adding/removing peers, adding/removing
241+
// ipBlock.except values, etc), we can't reliably determine whether a CIDR value we see
242+
// here is a new value or a pre-existing one. So we just allow any CIDR value that
243+
// appeared in the old NetworkPolicy to be used here without revalidation.
244+
func ValidateIPBlock(ipb *networking.IPBlock, fldPath *field.Path, opts NetworkPolicyValidationOptions) field.ErrorList {
217245
allErrs := field.ErrorList{}
218246
if ipb.CIDR == "" {
219247
allErrs = append(allErrs, field.Required(fldPath.Child("cidr"), ""))
220248
return allErrs
221249
}
222-
allErrs = append(allErrs, apivalidation.IsValidCIDRForLegacyField(fldPath.Child("cidr"), ipb.CIDR)...)
250+
allErrs = append(allErrs, apivalidation.IsValidCIDRForLegacyField(fldPath.Child("cidr"), ipb.CIDR, opts.AllowCIDRsEvenIfInvalid)...)
223251
_, cidrIPNet, err := netutils.ParseCIDRSloppy(ipb.CIDR)
224252
if err != nil {
225253
// Implies validation would have failed so we already added errors for it.
@@ -228,7 +256,7 @@ func ValidateIPBlock(ipb *networking.IPBlock, fldPath *field.Path) field.ErrorLi
228256

229257
for i, exceptCIDRStr := range ipb.Except {
230258
exceptPath := fldPath.Child("except").Index(i)
231-
allErrs = append(allErrs, apivalidation.IsValidCIDRForLegacyField(exceptPath, exceptCIDRStr)...)
259+
allErrs = append(allErrs, apivalidation.IsValidCIDRForLegacyField(exceptPath, exceptCIDRStr, opts.AllowCIDRsEvenIfInvalid)...)
232260
_, exceptCIDR, err := netutils.ParseCIDRSloppy(exceptCIDRStr)
233261
if err != nil {
234262
// Implies validation would have failed so we already added errors for it.
@@ -346,17 +374,28 @@ func ValidateIngressSpec(spec *networking.IngressSpec, fldPath *field.Path, opts
346374
// ValidateIngressStatusUpdate tests if required fields in the Ingress are set when updating status.
347375
func ValidateIngressStatusUpdate(ingress, oldIngress *networking.Ingress) field.ErrorList {
348376
allErrs := apivalidation.ValidateObjectMetaUpdate(&ingress.ObjectMeta, &oldIngress.ObjectMeta, field.NewPath("metadata"))
349-
allErrs = append(allErrs, ValidateIngressLoadBalancerStatus(&ingress.Status.LoadBalancer, field.NewPath("status", "loadBalancer"))...)
377+
allErrs = append(allErrs, ValidateIngressLoadBalancerStatus(&ingress.Status.LoadBalancer, &oldIngress.Status.LoadBalancer, field.NewPath("status", "loadBalancer"))...)
350378
return allErrs
351379
}
352380

353381
// ValidateIngressLoadBalancerStatus validates required fields on an IngressLoadBalancerStatus
354-
func ValidateIngressLoadBalancerStatus(status *networking.IngressLoadBalancerStatus, fldPath *field.Path) field.ErrorList {
382+
func ValidateIngressLoadBalancerStatus(status, oldStatus *networking.IngressLoadBalancerStatus, fldPath *field.Path) field.ErrorList {
355383
allErrs := field.ErrorList{}
384+
385+
var existingIngressIPs []string
386+
if oldStatus != nil {
387+
existingIngressIPs = make([]string, 0, len(oldStatus.Ingress))
388+
for _, ingress := range oldStatus.Ingress {
389+
if len(ingress.IP) > 0 {
390+
existingIngressIPs = append(existingIngressIPs, ingress.IP)
391+
}
392+
}
393+
}
394+
356395
for i, ingress := range status.Ingress {
357396
idxPath := fldPath.Child("ingress").Index(i)
358397
if len(ingress.IP) > 0 {
359-
allErrs = append(allErrs, apivalidation.IsValidIPForLegacyField(idxPath.Child("ip"), ingress.IP)...)
398+
allErrs = append(allErrs, apivalidation.IsValidIPForLegacyField(idxPath.Child("ip"), ingress.IP, existingIngressIPs)...)
360399
}
361400
if len(ingress.Hostname) > 0 {
362401
for _, msg := range validation.IsDNS1123Subdomain(ingress.Hostname) {

‎pkg/apis/networking/validation/validation_test.go‎

Lines changed: 130 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -442,13 +442,95 @@ func TestValidateNetworkPolicyUpdate(t *testing.T) {
442442
},
443443
},
444444
},
445+
"fix pre-existing invalid CIDR": {
446+
old: networking.NetworkPolicy{
447+
ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "bar"},
448+
Spec: networking.NetworkPolicySpec{
449+
PodSelector: metav1.LabelSelector{
450+
MatchLabels: map[string]string{"a": "b"},
451+
},
452+
Ingress: []networking.NetworkPolicyIngressRule{{
453+
From: []networking.NetworkPolicyPeer{{
454+
IPBlock: &networking.IPBlock{
455+
CIDR: "192.168.0.0/16",
456+
Except: []string{
457+
"192.168.2.0/24",
458+
"192.168.3.1/24",
459+
},
460+
},
461+
}},
462+
}},
463+
},
464+
},
465+
update: networking.NetworkPolicy{
466+
ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "bar"},
467+
Spec: networking.NetworkPolicySpec{
468+
PodSelector: metav1.LabelSelector{
469+
MatchLabels: map[string]string{"a": "b"},
470+
},
471+
Ingress: []networking.NetworkPolicyIngressRule{{
472+
From: []networking.NetworkPolicyPeer{{
473+
IPBlock: &networking.IPBlock{
474+
CIDR: "192.168.0.0/16",
475+
Except: []string{
476+
"192.168.2.0/24",
477+
"192.168.3.0/24",
478+
},
479+
},
480+
}},
481+
}},
482+
},
483+
},
484+
},
485+
"fail to fix pre-existing invalid CIDR": {
486+
old: networking.NetworkPolicy{
487+
ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "bar"},
488+
Spec: networking.NetworkPolicySpec{
489+
PodSelector: metav1.LabelSelector{
490+
MatchLabels: map[string]string{"a": "b"},
491+
},
492+
Ingress: []networking.NetworkPolicyIngressRule{{
493+
From: []networking.NetworkPolicyPeer{{
494+
IPBlock: &networking.IPBlock{
495+
CIDR: "192.168.0.0/16",
496+
Except: []string{
497+
"192.168.2.0/24",
498+
"192.168.3.1/24",
499+
},
500+
},
501+
}},
502+
}},
503+
},
504+
},
505+
update: networking.NetworkPolicy{
506+
ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "bar"},
507+
Spec: networking.NetworkPolicySpec{
508+
PodSelector: metav1.LabelSelector{
509+
MatchLabels: map[string]string{"a": "b"},
510+
},
511+
Ingress: []networking.NetworkPolicyIngressRule{{
512+
From: []networking.NetworkPolicyPeer{{
513+
IPBlock: &networking.IPBlock{
514+
CIDR: "192.168.0.0/16",
515+
Except: []string{
516+
"192.168.1.0/24",
517+
"192.168.2.0/24",
518+
"192.168.3.1/24",
519+
},
520+
},
521+
}},
522+
}},
523+
},
524+
},
525+
},
445526
}
446527

447528
for testName, successCase := range successCases {
448529
t.Run(testName, func(t *testing.T) {
530+
featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, true)
449531
successCase.old.ObjectMeta.ResourceVersion = "1"
450532
successCase.update.ObjectMeta.ResourceVersion = "1"
451-
if errs := ValidateNetworkPolicyUpdate(&successCase.update, &successCase.old, NetworkPolicyValidationOptions{false}); len(errs) != 0 {
533+
if errs := ValidateNetworkPolicyUpdate(&successCase.update, &successCase.old, NetworkPolicyValidationOptions{}); len(errs) != 0 {
452534
t.Errorf("expected success, but got %v", errs)
453535
}
454536
})
@@ -471,13 +553,55 @@ func TestValidateNetworkPolicyUpdate(t *testing.T) {
471553
},
472554
},
473555
},
556+
"add new invalid CIDR": {
557+
old: networking.NetworkPolicy{
558+
ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "bar"},
559+
Spec: networking.NetworkPolicySpec{
560+
PodSelector: metav1.LabelSelector{
561+
MatchLabels: map[string]string{"a": "b"},
562+
},
563+
Ingress: []networking.NetworkPolicyIngressRule{{
564+
From: []networking.NetworkPolicyPeer{{
565+
IPBlock: &networking.IPBlock{
566+
CIDR: "192.168.0.0/16",
567+
Except: []string{
568+
"192.168.2.0/24",
569+
"192.168.3.0/24",
570+
},
571+
},
572+
}},
573+
}},
574+
},
575+
},
576+
update: networking.NetworkPolicy{
577+
ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "bar"},
578+
Spec: networking.NetworkPolicySpec{
579+
PodSelector: metav1.LabelSelector{
580+
MatchLabels: map[string]string{"a": "b"},
581+
},
582+
Ingress: []networking.NetworkPolicyIngressRule{{
583+
From: []networking.NetworkPolicyPeer{{
584+
IPBlock: &networking.IPBlock{
585+
CIDR: "192.168.0.0/16",
586+
Except: []string{
587+
"192.168.1.1/24",
588+
"192.168.2.0/24",
589+
"192.168.3.0/24",
590+
},
591+
},
592+
}},
593+
}},
594+
},
595+
},
596+
},
474597
}
475598

476599
for testName, errorCase := range errorCases {
477600
t.Run(testName, func(t *testing.T) {
601+
featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StrictIPCIDRValidation, true)
478602
errorCase.old.ObjectMeta.ResourceVersion = "1"
479603
errorCase.update.ObjectMeta.ResourceVersion = "1"
480-
if errs := ValidateNetworkPolicyUpdate(&errorCase.update, &errorCase.old, NetworkPolicyValidationOptions{false}); len(errs) == 0 {
604+
if errs := ValidateNetworkPolicyUpdate(&errorCase.update, &errorCase.old, NetworkPolicyValidationOptions{}); len(errs) == 0 {
481605
t.Errorf("expected failure")
482606
}
483607
})
@@ -1865,6 +1989,10 @@ func TestValidateIngressStatusUpdate(t *testing.T) {
18651989
newValue: legacyIP,
18661990
legacyIPs: true,
18671991
},
1992+
"legacy IPs unchanged in update": {
1993+
oldValue: legacyIP,
1994+
newValue: legacyIP,
1995+
},
18681996
}
18691997
for k, tc := range successCases {
18701998
t.Run(k, func(t *testing.T) {

0 commit comments

Comments
 (0)