Skip to content

Commit 30469e1

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request kubernetes#131263 from aojea/dualstack_upgrade
Allow to convert clusters Service CIDRs from single to dual stack
2 parents cfed8c7 + 0266d3b commit 30469e1

File tree

8 files changed

+837
-59
lines changed

8 files changed

+837
-59
lines changed

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

Lines changed: 37 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -765,39 +765,62 @@ var ValidateServiceCIDRName = apimachineryvalidation.NameIsDNSSubdomain
765765

766766
func ValidateServiceCIDR(cidrConfig *networking.ServiceCIDR) field.ErrorList {
767767
allErrs := apivalidation.ValidateObjectMeta(&cidrConfig.ObjectMeta, false, ValidateServiceCIDRName, field.NewPath("metadata"))
768-
fieldPath := field.NewPath("spec", "cidrs")
768+
allErrs = append(allErrs, validateServiceCIDRSpec(&cidrConfig.Spec, field.NewPath("spec", "cidrs"))...)
769+
return allErrs
770+
}
769771

770-
if len(cidrConfig.Spec.CIDRs) == 0 {
772+
func validateServiceCIDRSpec(cidrConfigSpec *networking.ServiceCIDRSpec, fieldPath *field.Path) field.ErrorList {
773+
var allErrs field.ErrorList
774+
if len(cidrConfigSpec.CIDRs) == 0 {
771775
allErrs = append(allErrs, field.Required(fieldPath, "at least one CIDR required"))
772776
return allErrs
773777
}
774778

775-
if len(cidrConfig.Spec.CIDRs) > 2 {
776-
allErrs = append(allErrs, field.Invalid(fieldPath, cidrConfig.Spec, "may only hold up to 2 values"))
779+
if len(cidrConfigSpec.CIDRs) > 2 {
780+
allErrs = append(allErrs, field.Invalid(fieldPath, cidrConfigSpec, "may only hold up to 2 values"))
777781
return allErrs
778782
}
779-
// validate cidrs are dual stack, one of each IP family
780-
if len(cidrConfig.Spec.CIDRs) == 2 {
781-
isDual, err := netutils.IsDualStackCIDRStrings(cidrConfig.Spec.CIDRs)
782-
if err != nil || !isDual {
783-
allErrs = append(allErrs, field.Invalid(fieldPath, cidrConfig.Spec, "may specify no more than one IP for each IP family, i.e 192.168.0.0/24 and 2001:db8::/64"))
784-
return allErrs
785-
}
786-
}
787783

788-
for i, cidr := range cidrConfig.Spec.CIDRs {
784+
for i, cidr := range cidrConfigSpec.CIDRs {
789785
allErrs = append(allErrs, validation.IsValidCIDR(fieldPath.Index(i), cidr)...)
790786
}
791787

788+
// validate cidrs are dual stack, one of each IP family
789+
if len(cidrConfigSpec.CIDRs) == 2 &&
790+
netutils.IPFamilyOfCIDRString(cidrConfigSpec.CIDRs[0]) == netutils.IPFamilyOfCIDRString(cidrConfigSpec.CIDRs[1]) &&
791+
netutils.IPFamilyOfCIDRString(cidrConfigSpec.CIDRs[0]) != netutils.IPFamilyUnknown {
792+
allErrs = append(allErrs, field.Invalid(fieldPath, cidrConfigSpec.CIDRs, "may specify no more than one IP for each IP family, i.e 192.168.0.0/24 and 2001:db8::/64"))
793+
}
794+
792795
return allErrs
793796
}
794797

795798
// ValidateServiceCIDRUpdate tests if an update to a ServiceCIDR is valid.
796799
func ValidateServiceCIDRUpdate(update, old *networking.ServiceCIDR) field.ErrorList {
797800
var allErrs field.ErrorList
798801
allErrs = append(allErrs, apivalidation.ValidateObjectMetaUpdate(&update.ObjectMeta, &old.ObjectMeta, field.NewPath("metadata"))...)
799-
allErrs = append(allErrs, apivalidation.ValidateImmutableField(update.Spec.CIDRs, old.Spec.CIDRs, field.NewPath("spec").Child("cidrs"))...)
802+
switch {
803+
// no change in Spec.CIDRs lengths fields must not change
804+
case len(old.Spec.CIDRs) == len(update.Spec.CIDRs):
805+
for i, ip := range old.Spec.CIDRs {
806+
if ip != update.Spec.CIDRs[i] {
807+
allErrs = append(allErrs, field.Invalid(field.NewPath("spec").Child("cidrs").Index(i), update.Spec.CIDRs[i], apimachineryvalidation.FieldImmutableErrorMsg))
808+
}
809+
}
810+
// added a new CIDR is allowed to convert to Dual Stack
811+
// ref: https://issues.k8s.io/131261
812+
case len(old.Spec.CIDRs) == 1 && len(update.Spec.CIDRs) == 2:
813+
// existing CIDR can not change
814+
if update.Spec.CIDRs[0] != old.Spec.CIDRs[0] {
815+
allErrs = append(allErrs, field.Invalid(field.NewPath("spec").Child("cidrs").Index(0), update.Spec.CIDRs[0], apimachineryvalidation.FieldImmutableErrorMsg))
816+
}
817+
// validate the new added CIDR
818+
allErrs = append(allErrs, validateServiceCIDRSpec(&update.Spec, field.NewPath("spec", "cidrs"))...)
800819

820+
// no other changes allowed
821+
default:
822+
allErrs = append(allErrs, field.Invalid(field.NewPath("spec").Child("cidrs"), update.Spec.CIDRs, apimachineryvalidation.FieldImmutableErrorMsg))
823+
}
801824
return allErrs
802825
}
803826

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

Lines changed: 206 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -2373,6 +2373,17 @@ func TestValidateServiceCIDR(t *testing.T) {
23732373
},
23742374
},
23752375
},
2376+
"bad-iprange-ipv6-bad-ipv4": {
2377+
expectedErrors: 2,
2378+
ipRange: &networking.ServiceCIDR{
2379+
ObjectMeta: metav1.ObjectMeta{
2380+
Name: "test-name",
2381+
},
2382+
Spec: networking.ServiceCIDRSpec{
2383+
CIDRs: []string{"192.168.007.0/24", "MN00:1234::/64"},
2384+
},
2385+
},
2386+
},
23762387
}
23772388

23782389
for name, testCase := range testCases {
@@ -2386,55 +2397,224 @@ func TestValidateServiceCIDR(t *testing.T) {
23862397
}
23872398

23882399
func TestValidateServiceCIDRUpdate(t *testing.T) {
2389-
oldServiceCIDR := &networking.ServiceCIDR{
2400+
oldServiceCIDRv4 := &networking.ServiceCIDR{
2401+
ObjectMeta: metav1.ObjectMeta{
2402+
Name: "mysvc-v4",
2403+
ResourceVersion: "1",
2404+
},
2405+
Spec: networking.ServiceCIDRSpec{
2406+
CIDRs: []string{"192.168.0.0/24"},
2407+
},
2408+
}
2409+
oldServiceCIDRv6 := &networking.ServiceCIDR{
23902410
ObjectMeta: metav1.ObjectMeta{
2391-
Name: "mysvc",
2411+
Name: "mysvc-v6",
2412+
ResourceVersion: "1",
2413+
},
2414+
Spec: networking.ServiceCIDRSpec{
2415+
CIDRs: []string{"fd00:1234::/64"},
2416+
},
2417+
}
2418+
oldServiceCIDRDual := &networking.ServiceCIDR{
2419+
ObjectMeta: metav1.ObjectMeta{
2420+
Name: "mysvc-dual",
23922421
ResourceVersion: "1",
23932422
},
23942423
Spec: networking.ServiceCIDRSpec{
23952424
CIDRs: []string{"192.168.0.0/24", "fd00:1234::/64"},
23962425
},
23972426
}
23982427

2428+
// Define expected immutable field error for convenience
2429+
cidrsPath := field.NewPath("spec").Child("cidrs")
2430+
cidr0Path := cidrsPath.Index(0)
2431+
cidr1Path := cidrsPath.Index(1)
2432+
23992433
testCases := []struct {
2400-
name string
2401-
svc func(svc *networking.ServiceCIDR) *networking.ServiceCIDR
2402-
expectErr bool
2434+
name string
2435+
old *networking.ServiceCIDR
2436+
new *networking.ServiceCIDR
2437+
expectedErrs field.ErrorList
24032438
}{
24042439
{
2405-
name: "Successful update, no changes",
2406-
svc: func(svc *networking.ServiceCIDR) *networking.ServiceCIDR {
2407-
out := svc.DeepCopy()
2440+
name: "Successful update, no changes (dual)",
2441+
old: oldServiceCIDRDual,
2442+
new: oldServiceCIDRDual.DeepCopy(),
2443+
},
2444+
{
2445+
name: "Successful update, no changes (v4)",
2446+
old: oldServiceCIDRv4,
2447+
new: oldServiceCIDRv4.DeepCopy(),
2448+
},
2449+
{
2450+
name: "Successful update, single IPv4 to dual stack upgrade",
2451+
old: oldServiceCIDRv4,
2452+
new: func() *networking.ServiceCIDR {
2453+
out := oldServiceCIDRv4.DeepCopy()
2454+
out.Spec.CIDRs = []string{"192.168.0.0/24", "fd00:1234::/64"} // Add IPv6
2455+
return out
2456+
}(),
2457+
},
2458+
{
2459+
name: "Successful update, single IPv6 to dual stack upgrade",
2460+
old: oldServiceCIDRv6,
2461+
new: func() *networking.ServiceCIDR {
2462+
out := oldServiceCIDRv6.DeepCopy()
2463+
out.Spec.CIDRs = []string{"fd00:1234::/64", "192.168.0.0/24"} // Add IPv4
24082464
return out
2465+
}(),
2466+
},
2467+
{
2468+
name: "Failed update, change CIDRs (dual)",
2469+
old: oldServiceCIDRDual,
2470+
new: func() *networking.ServiceCIDR {
2471+
out := oldServiceCIDRDual.DeepCopy()
2472+
out.Spec.CIDRs = []string{"10.0.0.0/16", "fd00:abcd::/64"}
2473+
return out
2474+
}(),
2475+
expectedErrs: field.ErrorList{
2476+
field.Invalid(cidr0Path, "10.0.0.0/16", apimachineryvalidation.FieldImmutableErrorMsg),
2477+
field.Invalid(cidr1Path, "fd00:abcd::/64", apimachineryvalidation.FieldImmutableErrorMsg),
24092478
},
2410-
expectErr: false,
24112479
},
2412-
24132480
{
2414-
name: "Failed update, update spec.CIDRs single stack",
2415-
svc: func(svc *networking.ServiceCIDR) *networking.ServiceCIDR {
2416-
out := svc.DeepCopy()
2481+
name: "Failed update, change CIDRs (single)",
2482+
old: oldServiceCIDRv4,
2483+
new: func() *networking.ServiceCIDR {
2484+
out := oldServiceCIDRv4.DeepCopy()
24172485
out.Spec.CIDRs = []string{"10.0.0.0/16"}
24182486
return out
2419-
}, expectErr: true,
2487+
}(),
2488+
expectedErrs: field.ErrorList{field.Invalid(cidr0Path, "10.0.0.0/16", apimachineryvalidation.FieldImmutableErrorMsg)},
24202489
},
24212490
{
2422-
name: "Failed update, update spec.CIDRs dual stack",
2423-
svc: func(svc *networking.ServiceCIDR) *networking.ServiceCIDR {
2424-
out := svc.DeepCopy()
2425-
out.Spec.CIDRs = []string{"10.0.0.0/24", "fd00:1234::/64"}
2491+
name: "Failed update, single IPv4 to dual stack upgrade with primary change",
2492+
old: oldServiceCIDRv4,
2493+
new: func() *networking.ServiceCIDR {
2494+
out := oldServiceCIDRv4.DeepCopy()
2495+
// Change primary CIDR during upgrade
2496+
out.Spec.CIDRs = []string{"10.0.0.0/16", "fd00:1234::/64"}
24262497
return out
2427-
}, expectErr: true,
2498+
}(),
2499+
expectedErrs: field.ErrorList{field.Invalid(cidr0Path, "10.0.0.0/16", apimachineryvalidation.FieldImmutableErrorMsg)},
2500+
},
2501+
{
2502+
name: "Failed update, single IPv6 to dual stack upgrade with primary change",
2503+
old: oldServiceCIDRv6,
2504+
new: func() *networking.ServiceCIDR {
2505+
out := oldServiceCIDRv6.DeepCopy()
2506+
// Change primary CIDR during upgrade
2507+
out.Spec.CIDRs = []string{"fd00:abcd::/64", "192.168.0.0/24"}
2508+
return out
2509+
}(),
2510+
expectedErrs: field.ErrorList{field.Invalid(cidr0Path, "fd00:abcd::/64", apimachineryvalidation.FieldImmutableErrorMsg)},
2511+
},
2512+
{
2513+
name: "Failed update, dual stack downgrade to single",
2514+
old: oldServiceCIDRDual,
2515+
new: func() *networking.ServiceCIDR {
2516+
out := oldServiceCIDRDual.DeepCopy()
2517+
out.Spec.CIDRs = []string{"192.168.0.0/24"} // Remove IPv6
2518+
return out
2519+
}(),
2520+
expectedErrs: field.ErrorList{field.Invalid(cidrsPath, []string{"192.168.0.0/24"}, apimachineryvalidation.FieldImmutableErrorMsg)},
2521+
},
2522+
{
2523+
name: "Failed update, dual stack reorder",
2524+
old: oldServiceCIDRDual,
2525+
new: func() *networking.ServiceCIDR {
2526+
out := oldServiceCIDRDual.DeepCopy()
2527+
// Swap order
2528+
out.Spec.CIDRs = []string{"fd00:1234::/64", "192.168.0.0/24"}
2529+
return out
2530+
}(),
2531+
expectedErrs: field.ErrorList{
2532+
field.Invalid(cidr0Path, "fd00:1234::/64", apimachineryvalidation.FieldImmutableErrorMsg),
2533+
field.Invalid(cidr1Path, "192.168.0.0/24", apimachineryvalidation.FieldImmutableErrorMsg),
2534+
},
2535+
},
2536+
{
2537+
name: "Failed update, add invalid CIDR during upgrade",
2538+
old: oldServiceCIDRv4,
2539+
new: func() *networking.ServiceCIDR {
2540+
out := oldServiceCIDRv4.DeepCopy()
2541+
out.Spec.CIDRs = []string{"192.168.0.0/24", "invalid-cidr"}
2542+
return out
2543+
}(),
2544+
expectedErrs: field.ErrorList{field.Invalid(cidrsPath.Index(1), "invalid-cidr", "must be a valid CIDR value, (e.g. 10.9.8.0/24 or 2001:db8::/64)")},
2545+
},
2546+
{
2547+
name: "Failed update, add duplicate family CIDR during upgrade",
2548+
old: oldServiceCIDRv4,
2549+
new: func() *networking.ServiceCIDR {
2550+
out := oldServiceCIDRv4.DeepCopy()
2551+
out.Spec.CIDRs = []string{"192.168.0.0/24", "10.0.0.0/16"}
2552+
return out
2553+
}(),
2554+
expectedErrs: field.ErrorList{field.Invalid(cidrsPath, []string{"192.168.0.0/24", "10.0.0.0/16"}, "may specify no more than one IP for each IP family, i.e 192.168.0.0/24 and 2001:db8::/64")},
2555+
},
2556+
{
2557+
name: "Failed update, dual stack remove one cidr",
2558+
old: oldServiceCIDRDual,
2559+
new: func() *networking.ServiceCIDR {
2560+
out := oldServiceCIDRDual.DeepCopy()
2561+
out.Spec.CIDRs = out.Spec.CIDRs[0:1]
2562+
return out
2563+
}(),
2564+
expectedErrs: field.ErrorList{
2565+
field.Invalid(cidrsPath, []string{"192.168.0.0/24"}, apimachineryvalidation.FieldImmutableErrorMsg),
2566+
},
2567+
},
2568+
{
2569+
name: "Failed update, dual stack remove all cidrs",
2570+
old: oldServiceCIDRDual,
2571+
new: func() *networking.ServiceCIDR {
2572+
out := oldServiceCIDRDual.DeepCopy()
2573+
out.Spec.CIDRs = []string{}
2574+
return out
2575+
}(),
2576+
expectedErrs: field.ErrorList{
2577+
field.Invalid(cidrsPath, []string{}, apimachineryvalidation.FieldImmutableErrorMsg),
2578+
},
2579+
},
2580+
{
2581+
name: "Failed update, single stack remove cidr",
2582+
old: oldServiceCIDRv4,
2583+
new: func() *networking.ServiceCIDR {
2584+
out := oldServiceCIDRv4.DeepCopy()
2585+
out.Spec.CIDRs = []string{}
2586+
return out
2587+
}(),
2588+
expectedErrs: field.ErrorList{
2589+
field.Invalid(cidrsPath, []string{}, apimachineryvalidation.FieldImmutableErrorMsg),
2590+
},
2591+
},
2592+
{
2593+
name: "Failed update, add additional cidrs",
2594+
old: oldServiceCIDRDual,
2595+
new: func() *networking.ServiceCIDR {
2596+
out := oldServiceCIDRDual.DeepCopy()
2597+
out.Spec.CIDRs = append(out.Spec.CIDRs, "172.16.0.0/24")
2598+
return out
2599+
}(),
2600+
expectedErrs: field.ErrorList{
2601+
field.Invalid(cidrsPath, []string{"192.168.0.0/24", "fd00:1234::/64", "172.16.0.0/24"}, apimachineryvalidation.FieldImmutableErrorMsg),
2602+
},
24282603
},
24292604
}
2430-
for _, testCase := range testCases {
2431-
t.Run(testCase.name, func(t *testing.T) {
2432-
err := ValidateServiceCIDRUpdate(testCase.svc(oldServiceCIDR), oldServiceCIDR)
2433-
if !testCase.expectErr && err != nil {
2434-
t.Errorf("ValidateServiceCIDRUpdate must be successful for test '%s', got %v", testCase.name, err)
2605+
for _, tc := range testCases {
2606+
t.Run(tc.name, func(t *testing.T) {
2607+
// Ensure ResourceVersion is set for update validation
2608+
tc.new.ResourceVersion = tc.old.ResourceVersion
2609+
errs := ValidateServiceCIDRUpdate(tc.new, tc.old)
2610+
2611+
if len(errs) != len(tc.expectedErrs) {
2612+
t.Fatalf("Expected %d errors, got %d errors: %v", len(tc.expectedErrs), len(errs), errs)
24352613
}
2436-
if testCase.expectErr && err == nil {
2437-
t.Errorf("ValidateServiceCIDRUpdate must return error for test: %s, but got nil", testCase.name)
2614+
for i, expectedErr := range tc.expectedErrs {
2615+
if errs[i].Error() != expectedErr.Error() {
2616+
t.Errorf("Expected error %d: %v, got: %v", i, expectedErr, errs[i])
2617+
}
24382618
}
24392619
})
24402620
}

0 commit comments

Comments
 (0)