Skip to content

Commit 0266d3b

Browse files
aojeaaojea
committed
Allow single-to-dual-stack reconfiguration for ServiceCIDR
This change modifies the validation logic for ServiceCIDR updates (`ValidateServiceCIDRUpdate`) to specifically permit upgrading a single-stack ServiceCIDR (either IPv4 or IPv6) to a dual-stack configuration. This reconfiguration path is considered safe because it only involves adding a new CIDR range without altering the existing primary CIDR. This ensures that existing Service IP allocations are not disrupted. Other modifications, such as: - Downgrading from dual-stack to single-stack - Reordering CIDRs in a dual-stack configuration - Changing the primary CIDR during a single-to-dual-stack reconfiguration remain disallowed by the validation. These operations carry a higher risk of breaking existing Services or cluster networking configurations. Preventing these updates automatically encourages administrators to perform such changes manually after carefully assessing the potential impact on their specific cluster environment. The validation errors and controller logs provide guidance when such disallowed changes are attempted. Change-Id: I41dc09dfddb05f277925da2262f8114d6accbd1d
1 parent b15dfce commit 0266d3b

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)