oke--30296-fix-multi-nodeport-bug

oracle · Apr 3, 2024 · 4b92a5c · 4b92a5c
1 parent c687e3e
commit 4b92a5c
Show file tree

Hide file tree

Showing 4 changed files with 126 additions and 28 deletions.
diff --git a/pkg/controllers/backend/backend.go b/pkg/controllers/backend/backend.go
@@ -146,41 +146,38 @@ func (c *Controller) ensureBackends(ingressClass *networkingv1.IngressClass, lbI
 	for _, ingress := range ingresses {
 		for _, rule := range ingress.Spec.Rules {
 			for _, path := range rule.HTTP.Paths {
-				svcName, svcPort, targetPort, _, err := util.PathToServiceAndTargetPort(c.serviceLister, ingress.Namespace, path)
+				pSvc, svc, err := util.ExtractServices(path, c.serviceLister, ingress)
+				if err != nil {
+					return err
+				}
+				svcName, svcPort, targetPort, err := util.PathToServiceAndTargetPort(svc, pSvc, ingress.Namespace, false)
 				if err != nil {
 					return err
 				}
-
 				epAddrs, err := util.GetEndpoints(c.endpointLister, ingress.Namespace, svcName)
 				if err != nil {
 					return fmt.Errorf("unable to fetch endpoints for %s/%s/%d: %w", ingress.Namespace, svcName, targetPort, err)
 				}
-
 				backends := []ociloadbalancer.BackendDetails{}
 				for _, epAddr := range epAddrs {
 					backends = append(backends, util.NewBackend(epAddr.IP, targetPort))
 				}
-
 				backendSetName := util.GenerateBackendSetName(ingress.Namespace, svcName, svcPort)
 				err = c.client.GetLbClient().UpdateBackends(context.TODO(), lbID, backendSetName, backends)
 				if err != nil {
 					return fmt.Errorf("unable to update backends for %s/%s: %w", ingressClass.Name, backendSetName, err)
 				}
-
 				backendSetHealth, err := c.client.GetLbClient().GetBackendSetHealth(context.TODO(), lbID, backendSetName)
 				if err != nil {
 					return fmt.Errorf("unable to fetch backendset health: %w", err)
 				}
-
 				for _, epAddr := range epAddrs {
 					pod, err := c.podLister.Pods(ingress.Namespace).Get(epAddr.TargetRef.Name)
 					if err != nil {
 						return fmt.Errorf("failed to fetch pod %s/%s: %w", ingress.Namespace, epAddr.TargetRef.Name, err)
 					}
-
 					backendName := fmt.Sprintf("%s:%d", epAddr.IP, targetPort)
 					readinessCondition := util.GetPodReadinessCondition(ingress.Name, rule.Host, path)
-
 					err = c.ensurePodReadinessCondition(pod, readinessCondition, backendSetHealth, backendName)
 					if err != nil {
 						return fmt.Errorf("%w", err)

diff --git a/pkg/controllers/nodeBackend/nodeBackend.go b/pkg/controllers/nodeBackend/nodeBackend.go
@@ -169,17 +169,21 @@ func (c *Controller) ensureBackends(ingressClass *networkingv1.IngressClass, lbI
 	for _, ingress := range ingresses {
 		for _, rule := range ingress.Spec.Rules {
 			for _, path := range rule.HTTP.Paths {
-				svcName, svcPort, _, svc, err := util.PathToServiceAndTargetPort(c.serviceLister, ingress.Namespace, path)
+
+				pSvc, svc, err := util.ExtractServices(path, c.serviceLister, ingress)
 				if err != nil {
 					return err
 				}
 
-				if svc == nil || svc.Spec.Ports == nil || svc.Spec.Ports[0].NodePort == 0 {
+				svcName, svcPort, nodePort, err := util.PathToServiceAndTargetPort(svc, pSvc, ingress.Namespace, true)
+				if err != nil {
+					return err
+				}
+				if svc == nil || svc.Spec.Ports == nil || nodePort == 0 {
 					continue
 				}
 
 				var backends []ociloadbalancer.BackendDetails
-				nodePort := svc.Spec.Ports[0].NodePort
 				trafficPolicy := svc.Spec.ExternalTrafficPolicy
 				if trafficPolicy == corev1.ServiceExternalTrafficPolicyTypeCluster {
 					for _, node := range nodes {

diff --git a/pkg/util/util.go b/pkg/util/util.go
@@ -460,37 +460,56 @@ func PatchIngressClassWithAnnotation(client kubernetes.Interface, ic *networking
 	return nil, false
 }
 
-func GetTargetPortForService(lister corelisters.ServiceLister, namespace string, name string, port int32, portName string) (int32, int32, *corev1.Service, error) {
-	svc, err := lister.Services(namespace).Get(name)
-	if err != nil {
-		return 0, 0, nil, err
-	}
-
+func GetTargetPortForService(svc *corev1.Service, namespace string, name string, port int32, portName string, isNodeportRequired bool) (int32, int32, error) {
 	for _, p := range svc.Spec.Ports {
 		if (p.Port != 0 && p.Port == port) || p.Name == portName {
-			if p.TargetPort.Type != intstr.Int {
-				return 0, 0, nil, fmt.Errorf("service %s/%s has non-integer ports: %s", namespace, name, p.Name)
+
+			if !isNodeportRequired {
+				if p.TargetPort.Type != intstr.Int {
+					return 0, 0, fmt.Errorf("service %s/%s has non-integer ports: %s", namespace, name, p.Name)
+				}
+				return p.Port, p.TargetPort.IntVal, nil
+			} else {
+				if p.NodePort == 0 {
+					return 0, 0, fmt.Errorf("service %s/%s has no nodeports: %s", namespace, name, p.Name)
+				}
+				return p.Port, p.NodePort, nil
 			}
-			return p.Port, p.TargetPort.IntVal, svc, nil
+
 		}
 	}
 
-	return 0, 0, nil, fmt.Errorf("service %s/%s does not have port: %s (%d)", namespace, name, portName, port)
+	return 0, 0, fmt.Errorf("service %s/%s does not have port: %s (%d)", namespace, name, portName, port)
 }
 
-func PathToServiceAndTargetPort(lister corelisters.ServiceLister, ingressNamespace string, path networkingv1.HTTPIngressPath) (string, int32, int32, *corev1.Service, error) {
-	if path.Backend.Service == nil {
-		return "", 0, 0, nil, fmt.Errorf("backend service is not defined for ingress")
+func PathToServiceAndTargetPort(svc *corev1.Service, svcBackend networkingv1.IngressServiceBackend, namespace string, isNodePortRequired bool) (string, int32, int32, error) {
+
+	svcPort, targetPort, err := GetTargetPortForService(svc, namespace, svcBackend.Name, svcBackend.Port.Number, svcBackend.Port.Name, isNodePortRequired)
+	if err != nil {
+		return "", 0, 0, err
 	}
+	return svcBackend.Name, svcPort, targetPort, nil
+}
 
-	pSvc := *path.Backend.Service
+func ExtractServices(path networkingv1.HTTPIngressPath, svcLister corelisters.ServiceLister, ingress *networkingv1.Ingress) (networkingv1.IngressServiceBackend, *corev1.Service, error) {
+	pSvc, err := getIngressBackend(path)
+	if err != nil {
+		return networkingv1.IngressServiceBackend{}, nil, err
+	}
 
-	svcPort, targetPort, svc, err := GetTargetPortForService(lister, ingressNamespace, pSvc.Name, pSvc.Port.Number, pSvc.Port.Name)
+	svc, err := svcLister.Services(ingress.Namespace).Get(pSvc.Name)
 	if err != nil {
-		return "", 0, 0, nil, err
+		return networkingv1.IngressServiceBackend{}, nil, err
 	}
+	return pSvc, svc, nil
+}
 
-	return pSvc.Name, svcPort, targetPort, svc, nil
+func getIngressBackend(path networkingv1.HTTPIngressPath) (networkingv1.IngressServiceBackend, error) {
+	if path.Backend.Service == nil {
+		return networkingv1.IngressServiceBackend{}, fmt.Errorf("backend service is not defined for ingress")
+	}
+	pSvc := *path.Backend.Service
+	return pSvc, nil
 }
 
 func GetEndpoints(lister corelisters.EndpointsLister, namespace string, service string) ([]corev1.EndpointAddress, error) {

diff --git a/pkg/util/util_test.go b/pkg/util/util_test.go
@@ -11,6 +11,7 @@ package util
 import (
 	"context"
 	"fmt"
+	"strconv"
 	"strings"
 	"testing"
 
@@ -21,6 +22,7 @@ import (
 	networkingv1 "k8s.io/api/networking/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/client-go/informers"
 	fakeclientset "k8s.io/client-go/kubernetes/fake"
 	fake2 "k8s.io/client-go/kubernetes/typed/networking/v1/fake"
@@ -562,6 +564,82 @@ func TestIsIngressDeleting(t *testing.T) {
 	Expect(IsIngressDeleting(&i)).Should(Equal(false))
 }
 
+func TestPathToServiceAndTargetPort(t *testing.T) {
+	RegisterTestingT(t)
+	namespace := "test"
+	svc := &v1.Service{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "TestService",
+			Namespace: namespace,
+		},
+		Spec: v1.ServiceSpec{
+			Selector: map[string]string{"app": "multi-test"},
+			Ports: []v1.ServicePort{
+				{
+					Name:     "abcd",
+					Protocol: v1.ProtocolTCP,
+					Port:     443,
+					TargetPort: intstr.IntOrString{
+						IntVal: 8080,
+					},
+					NodePort: 30224,
+				},
+				{
+					Name:     "efgh",
+					Protocol: v1.ProtocolTCP,
+					Port:     444,
+					TargetPort: intstr.IntOrString{
+						IntVal: 8080,
+					},
+					NodePort: 30225,
+				},
+				{
+					Name:     "ijkl",
+					Protocol: v1.ProtocolTCP,
+					Port:     445,
+					TargetPort: intstr.IntOrString{
+						IntVal: 8080,
+					},
+					NodePort: 30226,
+				},
+			},
+			ExternalTrafficPolicy: v1.ServiceExternalTrafficPolicyTypeLocal,
+		},
+	}
+	ingBackend1 := networkingv1.IngressServiceBackend{
+		Name: "TestService1",
+		Port: networkingv1.ServiceBackendPort{
+			Number: 443,
+			Name:   "abcd",
+		},
+	}
+	ingBackend2 := networkingv1.IngressServiceBackend{
+		Name: "TestService2",
+		Port: networkingv1.ServiceBackendPort{
+			Number: 444,
+			Name:   "efgh",
+		},
+	}
+	ingBackend3 := networkingv1.IngressServiceBackend{
+		Name: "TestService3",
+		Port: networkingv1.ServiceBackendPort{
+			Number: 445,
+		},
+	}
+
+	runTests(svc, ingBackend1, namespace, "TestService1", "443", "30224")
+	runTests(svc, ingBackend2, namespace, "TestService2", "444", "30225")
+	runTests(svc, ingBackend3, namespace, "TestService3", "445", "30226")
+}
+
+func runTests(svc *v1.Service, ingBackend networkingv1.IngressServiceBackend, namespace string, expectedSvcName string, expectedPort string, np string) {
+	svcName, svcPort, nodePort, err := PathToServiceAndTargetPort(svc, ingBackend, namespace, true)
+	Expect(err == nil).Should(Equal(true))
+	Expect(svcName).Should(Equal(expectedSvcName))
+	Expect(strconv.Itoa(int(svcPort))).Should(Equal(expectedPort))
+	Expect(strconv.Itoa(int(nodePort))).Should(Equal(np))
+}
+
 func getIngressClassList() *networkingv1.IngressClassList {
 	defaultIngressClass := getIngressClassResource("default-ingress-class", true, "oci.oraclecloud.com/native-ingress-controller")
 	ingressClass := getIngressClassResource("ingress-class", false, "oci.oraclecloud.com/native-ingress-controller")