Skip to content

Commit e2eb847

Browse files
mkowalskimkowalski
committed
OCPBUGS-23140, OCPBUGS-23305: Soften VIP validations for external load-balancer
Currently for cluster-managed load balancer we have a set of validations ensuring that API and Ingress VIP are placed in a correct subnet as well as they do not overlap. For user-managed load balancer those validations are too strong as customers may have external load balancers placed outside of networks where cluster nodes live. Similarly, they may reuse the same IP for API and Ingress VIP, and distribute traffic via other means (e.g. using the port for the connection). With this PR we are softening the validations if the configuration used is indicating that external load balancer is used. Closes: OCPBUGS-23140 Closes: OCPBUGS-23305
1 parent d64216d commit e2eb847

File tree

2 files changed

+80
-16
lines changed

2 files changed

+80
-16
lines changed

‎pkg/types/validation/installconfig.go‎

Lines changed: 47 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -642,14 +642,21 @@ func validateVIPsForPlatform(network *types.Networking, platform *types.Platform
642642
APIVIPs: "apiVIPs",
643643
IngressVIPs: "ingressVIPs",
644644
}
645+
646+
var lbType configv1.PlatformLoadBalancerType
647+
645648
switch {
646649
case platform.BareMetal != nil:
647650
virtualIPs = vips{
648651
API: platform.BareMetal.APIVIPs,
649652
Ingress: platform.BareMetal.IngressVIPs,
650653
}
651654

652-
allErrs = append(allErrs, validateAPIAndIngressVIPs(virtualIPs, newVIPsFields, true, true, network, fldPath.Child(baremetal.Name))...)
655+
if platform.BareMetal.LoadBalancer != nil {
656+
lbType = platform.BareMetal.LoadBalancer.Type
657+
}
658+
659+
allErrs = append(allErrs, validateAPIAndIngressVIPs(virtualIPs, newVIPsFields, true, true, lbType, network, fldPath.Child(baremetal.Name))...)
653660
case platform.Nutanix != nil:
654661
allErrs = append(allErrs, ensureIPv4IsFirstInDualStackSlice(&platform.Nutanix.APIVIPs, fldPath.Child(nutanix.Name, newVIPsFields.APIVIPs))...)
655662
allErrs = append(allErrs, ensureIPv4IsFirstInDualStackSlice(&platform.Nutanix.IngressVIPs, fldPath.Child(nutanix.Name, newVIPsFields.IngressVIPs))...)
@@ -659,21 +666,33 @@ func validateVIPsForPlatform(network *types.Networking, platform *types.Platform
659666
Ingress: platform.Nutanix.IngressVIPs,
660667
}
661668

662-
allErrs = append(allErrs, validateAPIAndIngressVIPs(virtualIPs, newVIPsFields, false, false, network, fldPath.Child(nutanix.Name))...)
669+
if platform.Nutanix.LoadBalancer != nil {
670+
lbType = platform.Nutanix.LoadBalancer.Type
671+
}
672+
673+
allErrs = append(allErrs, validateAPIAndIngressVIPs(virtualIPs, newVIPsFields, false, false, lbType, network, fldPath.Child(nutanix.Name))...)
663674
case platform.OpenStack != nil:
664675
virtualIPs = vips{
665676
API: platform.OpenStack.APIVIPs,
666677
Ingress: platform.OpenStack.IngressVIPs,
667678
}
668679

669-
allErrs = append(allErrs, validateAPIAndIngressVIPs(virtualIPs, newVIPsFields, true, true, network, fldPath.Child(openstack.Name))...)
680+
if platform.OpenStack.LoadBalancer != nil {
681+
lbType = platform.OpenStack.LoadBalancer.Type
682+
}
683+
684+
allErrs = append(allErrs, validateAPIAndIngressVIPs(virtualIPs, newVIPsFields, true, true, lbType, network, fldPath.Child(openstack.Name))...)
670685
case platform.VSphere != nil:
671686
virtualIPs = vips{
672687
API: platform.VSphere.APIVIPs,
673688
Ingress: platform.VSphere.IngressVIPs,
674689
}
675690

676-
allErrs = append(allErrs, validateAPIAndIngressVIPs(virtualIPs, newVIPsFields, false, false, network, fldPath.Child(vsphere.Name))...)
691+
if platform.VSphere.LoadBalancer != nil {
692+
lbType = platform.VSphere.LoadBalancer.Type
693+
}
694+
695+
allErrs = append(allErrs, validateAPIAndIngressVIPs(virtualIPs, newVIPsFields, false, false, lbType, network, fldPath.Child(vsphere.Name))...)
677696
case platform.Ovirt != nil:
678697
allErrs = append(allErrs, ensureIPv4IsFirstInDualStackSlice(&platform.Ovirt.APIVIPs, fldPath.Child(ovirt.Name, newVIPsFields.APIVIPs))...)
679698
allErrs = append(allErrs, ensureIPv4IsFirstInDualStackSlice(&platform.Ovirt.IngressVIPs, fldPath.Child(ovirt.Name, newVIPsFields.IngressVIPs))...)
@@ -687,7 +706,11 @@ func validateVIPsForPlatform(network *types.Networking, platform *types.Platform
687706
Ingress: platform.Ovirt.IngressVIPs,
688707
}
689708

690-
allErrs = append(allErrs, validateAPIAndIngressVIPs(virtualIPs, newVIPsFields, true, true, network, fldPath.Child(ovirt.Name))...)
709+
if platform.Ovirt.LoadBalancer != nil {
710+
lbType = platform.Ovirt.LoadBalancer.Type
711+
}
712+
713+
allErrs = append(allErrs, validateAPIAndIngressVIPs(virtualIPs, newVIPsFields, true, true, lbType, network, fldPath.Child(ovirt.Name))...)
691714
default:
692715
//no vips to validate on this platform
693716
}
@@ -720,7 +743,7 @@ func ensureIPv4IsFirstInDualStackSlice(vips *[]string, fldPath *field.Path) fiel
720743
// validateAPIAndIngressVIPs validates the API and Ingress VIPs
721744
//
722745
//nolint:gocyclo
723-
func validateAPIAndIngressVIPs(vips vips, fieldNames vipFields, vipIsRequired, reqVIPinMachineCIDR bool, n *types.Networking, fldPath *field.Path) field.ErrorList {
746+
func validateAPIAndIngressVIPs(vips vips, fieldNames vipFields, vipIsRequired, reqVIPinMachineCIDR bool, lbType configv1.PlatformLoadBalancerType, n *types.Networking, fldPath *field.Path) field.ErrorList {
724747
allErrs := field.ErrorList{}
725748

726749
if len(vips.API) == 0 {
@@ -733,17 +756,21 @@ func validateAPIAndIngressVIPs(vips vips, fieldNames vipFields, vipIsRequired, r
733756
allErrs = append(allErrs, field.Invalid(fldPath.Child(fieldNames.APIVIPs), vip, err.Error()))
734757
}
735758

736-
for _, ingressVIP := range vips.Ingress {
737-
apiVIPNet := net.ParseIP(vip)
738-
ingressVIPNet := net.ParseIP(ingressVIP)
759+
// When using user-managed loadbalancer we do not require API and Ingress VIP to be different as well as
760+
// we allow them to be from outside the machine network CIDR.
761+
if lbType != configv1.LoadBalancerTypeUserManaged {
762+
for _, ingressVIP := range vips.Ingress {
763+
apiVIPNet := net.ParseIP(vip)
764+
ingressVIPNet := net.ParseIP(ingressVIP)
739765

740-
if apiVIPNet.Equal(ingressVIPNet) {
741-
allErrs = append(allErrs, field.Invalid(fldPath.Child(fieldNames.APIVIPs), vip, "VIP for API must not be one of the Ingress VIPs"))
766+
if apiVIPNet.Equal(ingressVIPNet) {
767+
allErrs = append(allErrs, field.Invalid(fldPath.Child(fieldNames.APIVIPs), vip, "VIP for API must not be one of the Ingress VIPs"))
768+
}
742769
}
743-
}
744770

745-
if err := ValidateIPinMachineCIDR(vip, n); reqVIPinMachineCIDR && err != nil {
746-
allErrs = append(allErrs, field.Invalid(fldPath.Child(fieldNames.APIVIPs), vip, err.Error()))
771+
if err := ValidateIPinMachineCIDR(vip, n); reqVIPinMachineCIDR && err != nil {
772+
allErrs = append(allErrs, field.Invalid(fldPath.Child(fieldNames.APIVIPs), vip, err.Error()))
773+
}
747774
}
748775

749776
if utilsnet.IsIPv6String(vip) && n.NetworkType == string(operv1.NetworkTypeOpenShiftSDN) {
@@ -785,8 +812,12 @@ func validateAPIAndIngressVIPs(vips vips, fieldNames vipFields, vipIsRequired, r
785812
allErrs = append(allErrs, field.Invalid(fldPath.Child(fieldNames.IngressVIPs), vip, err.Error()))
786813
}
787814

788-
if err := ValidateIPinMachineCIDR(vip, n); reqVIPinMachineCIDR && err != nil {
789-
allErrs = append(allErrs, field.Invalid(fldPath.Child(fieldNames.IngressVIPs), vip, err.Error()))
815+
// When using user-managed loadbalancer we do not require API and Ingress VIP to be different as well as
816+
// we allow them to be from outside the machine network CIDR.
817+
if lbType != configv1.LoadBalancerTypeUserManaged {
818+
if err := ValidateIPinMachineCIDR(vip, n); reqVIPinMachineCIDR && err != nil {
819+
allErrs = append(allErrs, field.Invalid(fldPath.Child(fieldNames.IngressVIPs), vip, err.Error()))
820+
}
790821
}
791822

792823
if utilsnet.IsIPv6String(vip) && n.NetworkType == string(operv1.NetworkTypeOpenShiftSDN) {

‎pkg/types/validation/installconfig_test.go‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1678,6 +1678,24 @@ func TestValidateInstallConfig(t *testing.T) {
16781678
}(),
16791679
expectedError: "platform.baremetal.apiVIPs: Invalid value: \"192.168.222.1\": IP expected to be in one of the machine networks: 10.0.0.0/16,fe80::/10",
16801680
},
1681+
{
1682+
name: "apivip_v4_not_in_machinenetwork_cidr_usermanaged_loadbalancer",
1683+
installConfig: func() *types.InstallConfig {
1684+
c := validInstallConfig()
1685+
c.FeatureSet = configv1.TechPreviewNoUpgrade
1686+
c.Networking.MachineNetwork = []types.MachineNetworkEntry{
1687+
{CIDR: *ipnet.MustParseCIDR("10.0.0.0/16")},
1688+
{CIDR: *ipnet.MustParseCIDR("fe80::/10")},
1689+
}
1690+
c.Platform = types.Platform{
1691+
BareMetal: validBareMetalPlatform(),
1692+
}
1693+
c.Platform.BareMetal.LoadBalancer = &configv1.BareMetalPlatformLoadBalancer{Type: configv1.LoadBalancerTypeUserManaged}
1694+
c.Platform.BareMetal.APIVIPs = []string{"192.168.222.1"}
1695+
1696+
return c
1697+
}(),
1698+
},
16811699
{
16821700
name: "apivip_v6_not_in_machinenetwork_cidr",
16831701
installConfig: func() *types.InstallConfig {
@@ -1961,6 +1979,21 @@ func TestValidateInstallConfig(t *testing.T) {
19611979
}(),
19621980
expectedError: "platform.baremetal.apiVIPs: Invalid value: \"fe80::1\": VIP for API must not be one of the Ingress VIPs",
19631981
},
1982+
{
1983+
name: "identical_apivip_ingressvip_usermanaged_loadbalancer",
1984+
installConfig: func() *types.InstallConfig {
1985+
c := validInstallConfig()
1986+
c.FeatureSet = configv1.TechPreviewNoUpgrade
1987+
c.Platform = types.Platform{
1988+
BareMetal: validBareMetalPlatform(),
1989+
}
1990+
c.Platform.BareMetal.LoadBalancer = &configv1.BareMetalPlatformLoadBalancer{Type: configv1.LoadBalancerTypeUserManaged}
1991+
c.Platform.BareMetal.APIVIPs = []string{"fe80::1"}
1992+
c.Platform.BareMetal.IngressVIPs = []string{"fe80::1"}
1993+
1994+
return c
1995+
}(),
1996+
},
19641997
{
19651998
name: "identical_apivips_ingressvips_multiple_ips",
19661999
installConfig: func() *types.InstallConfig {

0 commit comments

Comments
 (0)