skupperproject / skupper

Skupper is an implementation of a Virtual Application Network, enabling rich hybrid cloud communication.
http://skupper.io
Apache License 2.0
595 stars 74 forks source link

improve handling of nodeport service #1730

Closed grs closed 1 month ago

grs commented 1 month ago
grs commented 1 month ago

@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 commented 1 month ago

@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 commented 1 month ago

@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.