Repository: skupperproject/skupper
Pull request #1730 (2024-10-17 12:05:25)

improve handling of nodeport service
* only return nodeport endpoints when port has been assigned
* fix reconciliation of ports for nodeport services

grs: @c-kruse I was a little tunnel minded on my return yesterday after some days away from this, and was focused purely on the issue #1720 where the endpoints in the SecuredAccess status (and from there copied into site status) have a 0 port. This change is really just dealing with that. If the nodePort is zero for a nodeport service then there is no valid endpoint to report.

A separate potential issue is the one that your patch addresses, which is whether the current reconciliation of a nodeport service results in thrashing/conflict as it does not consider the system assigned nodePort field. Do you actually see such conflict? So far I have not but will investigate a little more on that. We may end up wanting both changes. 
grs: @c-kruse I have added an alternative fix to the issue your PR addresses, namely fixing the reconciliation of the service ports when the service is of type nodeport.
c-kruse: @grs thank you! Much more confidence inspiring.

I am able to reliably reproduce that conflict, although it is difficult to observe without some printf debugging - it does not manifest itself quite like I would expect with "thrashing". Maybe the kube api server (kind v1.27) I am using kindly disregards the update zeroing out NodePort but the client returns the caller's value?

In case you're curious and haven't observed for yourself:
```
diff --git a/pkg/kube/securedaccess/access.go b/pkg/kube/securedaccess/access.go
index 19dc7989..5e48f585 100644
--- a/pkg/kube/securedaccess/access.go
+++ b/pkg/kube/securedaccess/access.go
@@ -2,6 +2,7 @@ package securedaccess

 import (
        "context"
+       "encoding/json"
        "errors"
        "fmt"
        "log"
@@ -154,6 +155,16 @@ func (m *SecuredAccessManager) reconcile(sa *skupperv1alpha1.SecuredAccess) erro
                return nil
        }
        updated := false
+       defer func() {
+               pb, _ := json.Marshal(svc.Spec.Ports)
+               fmt.Printf(
+                       "ck/debug: SecuredAccess.reconcile type: %q updated %v svc/%s --namespace %s :: %s\n",
+                       sa.Spec.AccessType,
+                       updated,
+                       svc.Name,
+                       svc.Namespace,
+                       string(pb))
+       }()
        endpoints, resourceErr := m.accessType(sa).RealiseAndResolve(sa, svc)
```
Anyways, this looks correct to me now.

Patch:
diff --git a/pkg/kube/securedaccess/access.go b/pkg/kube/securedaccess/access.go
index 6fc56b27..19dc7989 100644
--- a/pkg/kube/securedaccess/access.go
+++ b/pkg/kube/securedaccess/access.go
@@ -424,11 +424,13 @@ func updatePorts(spec *corev1.ServiceSpec, desired []skupperv1alpha1.SecuredAcce
 	var ports []corev1.ServicePort
 	for _, actual := range spec.Ports {
 		if port, ok := expected[actual.Name]; ok {
-			ports = append(ports, port)
-			delete(expected, actual.Name)
-			if actual != port {
+			if equivalentPorts(port, actual) {
+				ports = append(ports, actual)
+			} else {
+				ports = append(ports, port)
 				changed = true
 			}
+			delete(expected, actual.Name)
 		} else {
 			changed = true
 		}
@@ -443,6 +445,18 @@ func updatePorts(spec *corev1.ServiceSpec, desired []skupperv1alpha1.SecuredAcce
 	return changed
 }
 
+func equivalentPorts(desired corev1.ServicePort, actual corev1.ServicePort) bool {
+	return desired.Name == actual.Name && desired.Port == actual.Port && equivalentTargetPorts(desired, actual) && equivalentProtocols(desired.Protocol, actual.Protocol)
+}
+
+func equivalentTargetPorts(desired corev1.ServicePort, actual corev1.ServicePort) bool {
+	return desired.TargetPort == actual.TargetPort || (desired.TargetPort.IntVal == 0 && actual.TargetPort.IntVal == desired.Port)
+}
+
+func equivalentProtocols(desired corev1.Protocol, actual corev1.Protocol) bool {
+	return desired == actual || (desired == "" && actual == corev1.ProtocolTCP)
+}
+
 func toServicePorts(desired map[string]skupperv1alpha1.SecuredAccessPort) map[string]corev1.ServicePort {
 	results := map[string]corev1.ServicePort{}
 	for name, details := range desired {
diff --git a/pkg/kube/securedaccess/nodeport.go b/pkg/kube/securedaccess/nodeport.go
index fcb5e96d..162f685d 100644
--- a/pkg/kube/securedaccess/nodeport.go
+++ b/pkg/kube/securedaccess/nodeport.go
@@ -23,11 +23,13 @@ func newNodeportAccess(m *SecuredAccessManager, clusterHost string) AccessType {
 func (o *NodeportAccessType) RealiseAndResolve(access *skupperv1alpha1.SecuredAccess, svc *corev1.Service) ([]skupperv1alpha1.Endpoint, error) {
 	var endpoints []skupperv1alpha1.Endpoint
 	for _, p := range svc.Spec.Ports {
-		endpoints = append(endpoints, skupperv1alpha1.Endpoint{
-			Name: p.Name,
-			Host: o.clusterHost,
-			Port: strconv.Itoa(int(p.NodePort)),
-		})
+		if p.NodePort != 0 {
+			endpoints = append(endpoints, skupperv1alpha1.Endpoint{
+				Name: p.Name,
+				Host: o.clusterHost,
+				Port: strconv.Itoa(int(p.NodePort)),
+			})
+		}
 	}
 	return endpoints, nil
 }


Test patch:
diff --git a/pkg/kube/securedaccess/access_test.go b/pkg/kube/securedaccess/access_test.go
index 348efc48..07b65c75 100644
--- a/pkg/kube/securedaccess/access_test.go
+++ b/pkg/kube/securedaccess/access_test.go
@@ -47,6 +47,7 @@ func TestSecuredAccessGeneral(t *testing.T) {
 		expectedEndpoints    []skupperv1alpha1.Endpoint
 		expectedError        string
 		defaultDomain        string
+		nodePorts            map[string]int32
 	}{
 		{
 			name: "loadbalancer",
@@ -1111,9 +1112,12 @@ func TestSecuredAccessGeneral(t *testing.T) {
 				{
 					Name: "port1",
 					Host: "mycluster.com",
-					Port: "0", //FIXME
+					Port: "33333",
 				},
 			},
+			nodePorts: map[string]int32{
+				"port1": 33333,
+			},
 		},
 		{
 			name: "local",
@@ -1843,6 +1847,20 @@ func TestSecuredAccessGeneral(t *testing.T) {
 						actual, err = m.clients.GetKubeClient().CoreV1().Services(desired.Namespace).UpdateStatus(context.Background(), actual, metav1.UpdateOptions{})
 						assert.Assert(t, err)
 					}
+					if tt.nodePorts != nil {
+						var updatedPorts []corev1.ServicePort
+						for _, port := range actual.Spec.Ports {
+							if nodePort, ok := tt.nodePorts[port.Name]; ok {
+								port.NodePort = nodePort
+							}
+							updatedPorts = append(updatedPorts, port)
+						}
+						if !reflect.DeepEqual(updatedPorts, actual.Spec.Ports) {
+							actual.Spec.Ports = updatedPorts
+							actual, err = m.clients.GetKubeClient().CoreV1().Services(desired.Namespace).Update(context.Background(), actual, metav1.UpdateOptions{})
+							assert.Assert(t, err)
+						}
+					}
 					err = m.CheckService(serviceKey(actual), actual)
 					assert.Assert(t, err)
 				}