fix reviews

Signed-off-by: Patrik Cyvoct <patrik@ptrk.io>

pkg/apis/core/types.go

@@ -3509,8 +3509,12 @@ type LoadBalancerIngress struct {
 	// +optional
 	Hostname string
 
-	// IPMode specifies the IP mode to use for this ingress
+	// IPMode specifies how the load-balancer's IP behaves.
-	// Defaults to `VIP` if IP is set, null otherwise
+	// Setting this to "VIP" indicates that the traffic passing through
+	// this load-balancer is delivered with the destination IP and port set to the load-balancer's IP and port.
+	// Setting this to "Proxy" indicates that the load-balancer acts like a proxy,
+	// delivering traffic with the destination IP and port set to the node's IP and nodePort or to the pod's IP and targetPort.
+	// This field can only be set when the ip field is also set, and defaults to "VIP" if not specified.
 	// +optional
 	IPMode *LoadBalancerIPMode
 }
@@ -3528,7 +3532,7 @@ const (
 	// is delivered with the destination IP set to the specified LoadBalancer IP
 	LoadBalancerIPModeVIP LoadBalancerIPMode = "VIP"
 	// LoadBalancerIPModeProxy indicates that the specified LoadBalancer acts like a proxy,
-	// changing the destination IP to the node IP and the source IP to the LoadBalaner (mostly private) IP
+	// changing the destination IP to the node IP and the source IP to the LoadBalancer (mostly private) IP
 	LoadBalancerIPModeProxy LoadBalancerIPMode = "Proxy"
 )
 

pkg/apis/core/validation/validation.go

@@ -5989,18 +5989,22 @@ func ValidateLoadBalancerStatus(status *core.LoadBalancerStatus, fldPath *field.
 		}
 
 		if utilfeature.DefaultFeatureGate.Enabled(features.LoadBalancerIPMode) {
-			if len(ingress.IP) > 0 && ingress.IPMode != nil {
+			if len(ingress.IP) > 0 && ingress.IPMode == nil {
-				switch *ingress.IPMode {
-				case core.LoadBalancerIPModeVIP, core.LoadBalancerIPModeProxy:
-					break
-				default:
-					allErrs = append(allErrs, field.NotSupported(idxPath.Child("ipMode"), ingress.IPMode, supportedLoadBalancerIPMode.List()))
-				}
-			} else if len(ingress.IP) > 0 && ingress.IPMode == nil {
 				allErrs = append(allErrs, field.Required(idxPath.Child("ipMode"), "must be specified when `ip` is set"))
-			} else if len(ingress.IP) == 0 && ingress.IPMode != nil {
+			}
+		}
+
+		if ingress.IPMode != nil {
+			if len(ingress.IP) == 0 {
 				allErrs = append(allErrs, field.Forbidden(idxPath.Child("ipMode"), "may not be used when `ip` is not set"))
 			}
+
+			switch *ingress.IPMode {
+			case core.LoadBalancerIPModeVIP, core.LoadBalancerIPModeProxy:
+				break
+			default:
+				allErrs = append(allErrs, field.NotSupported(idxPath.Child("ipMode"), ingress.IPMode, supportedLoadBalancerIPMode.List()))
+			}
 		}
 
 		if len(ingress.Hostname) > 0 {

pkg/apis/core/validation/validation_test.go

@@ -11071,6 +11071,90 @@ func TestValidateServiceCreate(t *testing.T) {
 	}
 }
 
+func TestValidateLoadBalancerStatus(t *testing.T) {
+	defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.LoadBalancerIPMode, true)()
+
+	ipModeVIP := core.LoadBalancerIPModeVIP
+	ipModeProxy := core.LoadBalancerIPModeProxy
+	ipModeDummy := core.LoadBalancerIPMode("dummy")
+
+	testCases := []struct {
+		name          string
+		tweakLBStatus func(s *core.LoadBalancerStatus)
+		numErrs       int
+	}{
+		/* LoadBalancerIPMode*/
+		{
+			name: `valid vip ipMode`,
+			tweakLBStatus: func(s *core.LoadBalancerStatus) {
+				s.Ingress = []core.LoadBalancerIngress{
+					{
+						IP:     "1.2.3.4",
+						IPMode: &ipModeVIP,
+					},
+				}
+			},
+			numErrs: 0,
+		},
+		{
+			name: `valid proxy ipMode`,
+			tweakLBStatus: func(s *core.LoadBalancerStatus) {
+				s.Ingress = []core.LoadBalancerIngress{
+					{
+						IP:     "1.2.3.4",
+						IPMode: &ipModeProxy,
+					},
+				}
+			},
+			numErrs: 0,
+		},
+		{
+			name: `invalid ipMode`,
+			tweakLBStatus: func(s *core.LoadBalancerStatus) {
+				s.Ingress = []core.LoadBalancerIngress{
+					{
+						IP:     "1.2.3.4",
+						IPMode: &ipModeDummy,
+					},
+				}
+			},
+			numErrs: 1,
+		},
+		{
+			name: `missing ipMode`,
+			tweakLBStatus: func(s *core.LoadBalancerStatus) {
+				s.Ingress = []core.LoadBalancerIngress{
+					{
+						IP: "1.2.3.4",
+					},
+				}
+			},
+			numErrs: 1,
+		},
+		{
+			name: `missing ip with ipMode present`,
+			tweakLBStatus: func(s *core.LoadBalancerStatus) {
+				s.Ingress = []core.LoadBalancerIngress{
+					{
+						IPMode: &ipModeProxy,
+					},
+				}
+			},
+			numErrs: 1,
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			s := core.LoadBalancerStatus{}
+			tc.tweakLBStatus(&s)
+			errs := ValidateLoadBalancerStatus(&s, field.NewPath("status"))
+			if len(errs) != tc.numErrs {
+				t.Errorf("Unexpected error list for case %q(expected:%v got %v) - Errors:\n %v", tc.name, tc.numErrs, len(errs), errs.ToAggregate())
+			}
+		})
+	}
+}
+
 func TestValidateServiceExternalTrafficFieldsCombination(t *testing.T) {
 	testCases := []struct {
 		name     string

pkg/proxy/iptables/proxier.go

@@ -1175,7 +1175,7 @@ func (proxier *Proxier) syncProxyRules() {
 					// This currently works for loadbalancers that preserves source ips.
 					// For loadbalancers which direct traffic to service NodePort, the firewall rules will not apply.
 
-					if !utilfeature.DefaultFeatureGate.Enabled(features.LoadBalancerIPMode) || ingress.IPMode == nil || *ingress.IPMode == v1.LoadBalancerIPModeVIP {
+					if !utilfeature.DefaultFeatureGate.Enabled(features.LoadBalancerIPMode) || *ingress.IPMode == v1.LoadBalancerIPModeVIP {
 						args = append(args[:0],
 							"-A", string(kubeServicesChain),
 							"-m", "comment", "--comment", fmt.Sprintf(`"%s loadbalancer IP"`, svcNameString),

pkg/proxy/ipvs/proxier.go

@@ -1333,7 +1333,7 @@ func (proxier *Proxier) syncProxyRules() {
 
 		// Capture load-balancer ingress.
 		for _, ingress := range svcInfo.LoadBalancerIngress() {
-			if ingress.IP != "" && (!utilfeature.DefaultFeatureGate.Enabled(features.LoadBalancerIPMode) || ingress.IPMode == nil || *ingress.IPMode == v1.LoadBalancerIPModeVIP) {
+			if ingress.IP != "" && (!utilfeature.DefaultFeatureGate.Enabled(features.LoadBalancerIPMode) || *ingress.IPMode == v1.LoadBalancerIPModeVIP) {
 				// ipset call
 				entry = &utilipset.Entry{
 					IP:       ingress.IP,

pkg/proxy/service.go

@@ -163,26 +163,16 @@ func (sct *ServiceChangeTracker) newBaseServiceInfo(port *v1.ServicePort, servic
 		klog.V(4).Infof("service change tracker(%v) ignored the following load balancer source ranges(%s) for service %v/%v as they don't match IPFamily", sct.ipFamily, strings.Join(incorrectIPs, ","), service.Namespace, service.Name)
 	}
 
-	// Obtain Load Balancer Ingress IPs
+	correctIngresses, incorrectIngresses := utilproxy.FilterIncorrectLoadBalancerIngress(service.Status.LoadBalancer.Ingress, sct.ipFamily)
-	var allIncorrectIPs []string
-	for _, ing := range service.Status.LoadBalancer.Ingress {
-		// []string{ing.IP} have a len of 1, so len(correctIPs) + len(incorrectIPs) == 1
-		correctIPs, incorrectIPs := utilproxy.FilterIncorrectIPVersion([]string{ing.IP}, sct.ipFamily)
 
-		// len is either 1 or 0
+	info.loadBalancerStatus.Ingress = correctIngresses
-		if len(correctIPs) == 1 {
+
-			// Update the LoadBalancerStatus with the filtered IP
+	if len(incorrectIngresses) > 0 {
-			info.loadBalancerStatus.Ingress = append(info.loadBalancerStatus.Ingress, ing)
+		var incorrectIPs []string
-			continue
+		for _, incorrectIng := range incorrectIngresses {
+			incorrectIPs = append(incorrectIPs, incorrectIng.IP)
 		}
-
-		// here len(incorrectIPs) == 1 since len(correctIPs) == 0
-		allIncorrectIPs = append(allIncorrectIPs, incorrectIPs[0])
-	}
-
-	if len(allIncorrectIPs) > 0 {
 		klog.V(4).Infof("service change tracker(%v) ignored the following load balancer(%s) ingress ips for service %v/%v as they don't match IPFamily", sct.ipFamily, strings.Join(incorrectIPs, ","), service.Namespace, service.Name)
-
 	}
 
 	if apiservice.NeedsHealthCheck(service) {

pkg/proxy/util/utils.go

@@ -403,3 +403,26 @@ func GetClusterIPByFamily(ipFamily v1.IPFamily, service *v1.Service) string {
 
 	return ""
 }
+
+// FilterIncorrectLoadBalancerIngress filters out the ingresses with an IP version different from the given one
+func FilterIncorrectLoadBalancerIngress(ingresses []v1.LoadBalancerIngress, ipFamily v1.IPFamily) ([]v1.LoadBalancerIngress, []v1.LoadBalancerIngress) {
+	var validIngresses []v1.LoadBalancerIngress
+	var invalidIngresses []v1.LoadBalancerIngress
+
+	for _, ing := range ingresses {
+		// []string{ing.IP} have a len of 1, so len(correctIPs) + len(incorrectIPs) == 1
+		correctIPs, _ := FilterIncorrectIPVersion([]string{ing.IP}, ipFamily)
+
+		// len is either 1 or 0
+		if len(correctIPs) == 1 {
+			// Update the LoadBalancerStatus with the filtered IP
+			validIngresses = append(validIngresses, ing)
+			continue
+		}
+
+		// here len(incorrectIPs) == 1 since len(correctIPs) == 0
+		invalidIngresses = append(invalidIngresses, ing)
+	}
+
+	return validIngresses, invalidIngresses
+}

pkg/proxy/util/utils_test.go

@@ -822,3 +822,179 @@ func TestGetClusterIPByFamily(t *testing.T) {
 	}
 
 }
+
+func TestFilterIncorrectLoadBalancerIngress(t *testing.T) {
+	ipModeVIP := v1.LoadBalancerIPModeVIP
+	testCases := []struct {
+		name              string
+		ingresses         []v1.LoadBalancerIngress
+		ipFamily          v1.IPFamily
+		expectedCorrect   []v1.LoadBalancerIngress
+		expectedIncorrect []v1.LoadBalancerIngress
+	}{
+		{
+			name:     "IPv4 only valid ingresses",
+			ipFamily: v1.IPv4Protocol,
+			ingresses: []v1.LoadBalancerIngress{
+				{
+					IP:     "1.2.3.4",
+					IPMode: &ipModeVIP,
+				},
+				{
+					IP: "1.2.3.5",
+				},
+			},
+			expectedCorrect: []v1.LoadBalancerIngress{
+				{
+					IP:     "1.2.3.4",
+					IPMode: &ipModeVIP,
+				},
+				{
+					IP: "1.2.3.5",
+				},
+			},
+			expectedIncorrect: nil,
+		},
+		{
+			name:     "IPv4 some invalid ingresses",
+			ipFamily: v1.IPv4Protocol,
+			ingresses: []v1.LoadBalancerIngress{
+				{
+					IP:     "1.2.3.4",
+					IPMode: &ipModeVIP,
+				},
+				{
+					IP: "2000::1",
+				},
+				{
+					Hostname: "dummy",
+				},
+			},
+			expectedCorrect: []v1.LoadBalancerIngress{
+				{
+					IP:     "1.2.3.4",
+					IPMode: &ipModeVIP,
+				},
+				{
+					Hostname: "dummy", // weirdly no IP is a valid IPv4 but invalid IPv6
+				},
+			},
+			expectedIncorrect: []v1.LoadBalancerIngress{
+				{
+					IP: "2000::1",
+				},
+			},
+		},
+		{
+			name:     "IPv4 only invalid ingresses",
+			ipFamily: v1.IPv4Protocol,
+			ingresses: []v1.LoadBalancerIngress{
+				{
+					IP: "2000::1",
+				},
+				{
+					IP:     "2000::1",
+					IPMode: &ipModeVIP,
+				},
+			},
+			expectedCorrect: nil,
+			expectedIncorrect: []v1.LoadBalancerIngress{
+				{
+					IP: "2000::1",
+				},
+				{
+					IP:     "2000::1",
+					IPMode: &ipModeVIP,
+				},
+			},
+		},
+		{
+			name:     "IPv6 only valid ingresses",
+			ipFamily: v1.IPv6Protocol,
+			ingresses: []v1.LoadBalancerIngress{
+				{
+					IP:     "2000::1",
+					IPMode: &ipModeVIP,
+				},
+				{
+					IP: "2000::2",
+				},
+			},
+			expectedCorrect: []v1.LoadBalancerIngress{
+				{
+					IP:     "2000::1",
+					IPMode: &ipModeVIP,
+				},
+				{
+					IP: "2000::2",
+				},
+			},
+			expectedIncorrect: nil,
+		},
+		{
+			name:     "IPv6 some invalid ingresses",
+			ipFamily: v1.IPv6Protocol,
+			ingresses: []v1.LoadBalancerIngress{
+				{
+					IP:     "2000::1",
+					IPMode: &ipModeVIP,
+				},
+				{
+					IP: "1.2.3.4",
+				},
+				{
+					Hostname: "dummy",
+				},
+			},
+			expectedCorrect: []v1.LoadBalancerIngress{
+				{
+					IP:     "2000::1",
+					IPMode: &ipModeVIP,
+				},
+			},
+			expectedIncorrect: []v1.LoadBalancerIngress{
+				{
+					IP: "1.2.3.4",
+				},
+				{
+					Hostname: "dummy", // weirdly no IP is a valid IPv4 but invalid IPv6
+				},
+			},
+		},
+		{
+			name:     "IPv6 only invalid ingresses",
+			ipFamily: v1.IPv6Protocol,
+			ingresses: []v1.LoadBalancerIngress{
+				{
+					IP: "1.2.3.4",
+				},
+				{
+					IP:     "1.2.3.5",
+					IPMode: &ipModeVIP,
+				},
+			},
+			expectedCorrect: nil,
+			expectedIncorrect: []v1.LoadBalancerIngress{
+				{
+					IP: "1.2.3.4",
+				},
+				{
+					IP:     "1.2.3.5",
+					IPMode: &ipModeVIP,
+				},
+			},
+		},
+	}
+
+	for _, testCase := range testCases {
+		t.Run(testCase.name, func(t *testing.T) {
+			correctIngresses, incorrectIngresses := FilterIncorrectLoadBalancerIngress(testCase.ingresses, testCase.ipFamily)
+			if !reflect.DeepEqual(correctIngresses, testCase.expectedCorrect) {
+				t.Errorf("Test %v failed: expected %v, got %v", testCase.name, testCase.expectedCorrect, correctIngresses)
+			}
+			if !reflect.DeepEqual(incorrectIngresses, testCase.expectedIncorrect) {
+				t.Errorf("Test %v failed: expected %v, got %v", testCase.name, testCase.expectedIncorrect, incorrectIngresses)
+			}
+		})
+	}
+}

staging/src/k8s.io/api/core/v1/types.go

@@ -3972,8 +3972,12 @@ type LoadBalancerIngress struct {
 	// +optional
 	Hostname string `json:"hostname,omitempty" protobuf:"bytes,2,opt,name=hostname"`
 
-	// IPMode specifies the IP mode to use for this ingress
+	// IPMode specifies how the load-balancer's IP behaves.
-	// Defaults to `VIP` if IP is set, null otherwise
+	// Setting this to "VIP" indicates that the traffic passing through
+	// this load-balancer is delivered with the destination IP and port set to the load-balancer's IP and port.
+	// Setting this to "Proxy" indicates that the load-balancer acts like a proxy,
+	// delivering traffic with the destination IP and port set to the node's IP and nodePort or to the pod's IP and targetPort.
+	// This field can only be set when the ip field is also set, and defaults to "VIP" if not specified.
 	// +optional
 	IPMode *LoadBalancerIPMode `json:"ipMode,omitempty" protobuf:"bytes,3,opt,name=ipMode"`
 }
@@ -3991,7 +3995,7 @@ const (
 	// is delivered with the destination IP set to the specified LoadBalancer IP
 	LoadBalancerIPModeVIP LoadBalancerIPMode = "VIP"
 	// LoadBalancerIPModeProxy indicates that the specified LoadBalancer acts like a proxy,
-	// changing the destination IP to the node IP and the source IP to the LoadBalaner (mostly private) IP
+	// changing the destination IP to the node IP and the source IP to the LoadBalancer (mostly private) IP
 	LoadBalancerIPModeProxy LoadBalancerIPMode = "Proxy"
 )