From 955b037fce26aa0d3e1f714b3de219633a6f29dc Mon Sep 17 00:00:00 2001 From: Konstanty Karagiorgis Date: Tue, 28 May 2024 14:33:18 +0200 Subject: [PATCH 1/7] Limit interfaces listened on by NGINX, fixes #54 --- README.md | 8 ++++ pkg/cmd/client.go | 2 + pkg/cmd/server.go | 2 + pkg/nginx/exposer.go | 25 ++++++++---- pkg/nginx/listener.go | 93 +++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 123 insertions(+), 7 deletions(-) create mode 100644 pkg/nginx/listener.go diff --git a/README.md b/README.md index 8240417..9af3a3d 100644 --- a/README.md +++ b/README.md @@ -115,6 +115,14 @@ tilt up First start of wormhole will be really slow - it compiles the go code inside the container. Subsequent starts will be faster, as the go build cache is preserved in PVC. +The development environment deploys a server, two clients and a mock service, that you can use to test the tunnels. + +``` +kubectl annotate --overwrite svc --namespace nginx nginx wormhole.glothriel.github.com/exposed=yes +``` + +The additional services should be immediately created. Please note, that all three workloads are deployed on the same (and by extension are monitoring the same services for annotations), so the nginx will be exposed 4 times - client1 to server, client2 to server, server to client1 and server to client2. + ### Integration tests ``` diff --git a/pkg/cmd/client.go b/pkg/cmd/client.go index f8c770a..7b8602f 100644 --- a/pkg/cmd/client.go +++ b/pkg/cmd/client.go @@ -50,6 +50,7 @@ var joinCommand *cli.Command = &cli.Command{ "local", nginx.NewDefaultReloader(), nginx.NewRangePortAllocator(20000, 25000), + nginx.NewOnlyWireguardListener(), )) remoteNginxExposer := nginx.NewNginxExposer( @@ -57,6 +58,7 @@ var joinCommand *cli.Command = &cli.Command{ "remote", nginx.NewDefaultReloader(), nginx.NewRangePortAllocator(25001, 30000), + nginx.NewAllAcceptWireguardListener(), ) var effectiveExposer listeners.Exposer = remoteNginxExposer diff --git a/pkg/cmd/server.go b/pkg/cmd/server.go index 3b9d88e..f5176ed 100644 --- a/pkg/cmd/server.go +++ b/pkg/cmd/server.go @@ -81,6 +81,7 @@ var listenCommand *cli.Command = &cli.Command{ "local", nginx.NewDefaultReloader(), nginx.NewRangePortAllocator(20000, 25000), + nginx.NewOnlyWireguardListener(), )) remoteNginxExposer := nginx.NewNginxExposer( @@ -88,6 +89,7 @@ var listenCommand *cli.Command = &cli.Command{ "remote", nginx.NewDefaultReloader(), nginx.NewRangePortAllocator(25001, 30000), + nginx.NewAllAcceptWireguardListener(), ) var effectiveExposer listeners.Exposer = remoteNginxExposer diff --git a/pkg/nginx/exposer.go b/pkg/nginx/exposer.go index 6946197..1614638 100644 --- a/pkg/nginx/exposer.go +++ b/pkg/nginx/exposer.go @@ -15,9 +15,10 @@ import ( // Exposer is an Exposer implementation that uses NGINX as a proxy server type Exposer struct { - prefix string - path string - fs afero.Fs + prefix string + path string + fs afero.Fs + listener Listener reloader Reloader ports PortAllocator @@ -35,17 +36,24 @@ func (n *Exposer) Add(app peers.App) (peers.App, error) { File: nginxConfigPath(n.prefix, app), App: app, } - + listen := "" + listenAddrs, listenerErr := n.listener.Addrs(port) + if listenerErr != nil || len(listenAddrs) == 0 { + logrus.Errorf("Could not get listener addresses: %v", listenerErr) + } + for _, addr := range listenAddrs { + listen += fmt.Sprintf(" listen %s;\n", addr) + } if writeErr := afero.WriteFile(n.fs, path.Join(n.path, server.File), []byte(fmt.Sprintf(` # [%s] %s server { - listen %d; +%s proxy_pass %s; } `, server.App.Peer, server.App.Name, - server.ListenPort, + listen, server.ProxyPass, )), 0644); writeErr != nil { logrus.Errorf("Could not write NGINX config file: %v", writeErr) @@ -117,7 +125,9 @@ func (n *Exposer) WithdrawAll() error { } // NewNginxExposer creates a new NGINX exposer -func NewNginxExposer(path, confPrefix string, reloader Reloader, allocator PortAllocator) listeners.Exposer { +func NewNginxExposer( + path, confPrefix string, reloader Reloader, allocator PortAllocator, listener Listener, +) listeners.Exposer { fs := afero.NewOsFs() cg := &Exposer{ path: path, @@ -126,6 +136,7 @@ func NewNginxExposer(path, confPrefix string, reloader Reloader, allocator PortA reloader: reloader, ports: allocator, + listener: listener, } createErr := fs.MkdirAll(path, 0755) if createErr != nil && createErr != afero.ErrDestinationExists { diff --git a/pkg/nginx/listener.go b/pkg/nginx/listener.go new file mode 100644 index 0000000..0e38e7d --- /dev/null +++ b/pkg/nginx/listener.go @@ -0,0 +1,93 @@ +package nginx + +import ( + "fmt" + "net" + "strings" +) + +// Listener is an interface for NGINX listeners +type Listener interface { + // Addrs returns a list of addresses that the listener is listening on + Addrs(portNumber int) ([]string, error) +} + +type portOnlyListener struct { +} + +// Addrs implements Listener +func (p *portOnlyListener) Addrs(portNumber int) ([]string, error) { + return []string{fmt.Sprintf("%d", portNumber)}, nil +} + +// NewPortOnlyListener creates a new Listener that listens on a single port +func NewPortOnlyListener() Listener { + return &portOnlyListener{} +} + +type wg0FilteringListener struct { + includeWg0 bool +} + +// Addrs implements Listener +func (a *wg0FilteringListener) Addrs(portNumber int) ([]string, error) { + interfaces, interfacesErr := net.Interfaces() + if interfacesErr != nil { + return []string{}, interfacesErr + } + + var allAddrs []string + + for _, iface := range interfaces { + if iface.Name == "wg0" && !a.includeWg0 { + continue + } + if iface.Name != "wg0" && a.includeWg0 { + continue + } + + addrs, err := iface.Addrs() + if err != nil { + fmt.Println("Error retrieving addresses for interface:", iface.Name, err) + continue + } + + for _, addr := range addrs { + switch v := addr.(type) { + // Ignore ipv6 + case *net.IPNet: + if strings.Contains(v.IP.String(), ":") { + continue + } else { + allAddrs = append(allAddrs, formatIP(v.IP, portNumber)) + } + + case *net.IPAddr: + if strings.Contains(v.IP.String(), ":") { + continue + } else { + allAddrs = append(allAddrs, formatIP(v.IP, portNumber)) + } + } + } + } + return allAddrs, nil +} + +func formatIP(ip fmt.Stringer, portNumber int) string { + return fmt.Sprintf("%s:%d", ip.String(), portNumber) +} + +// NewAllAcceptWireguardListener creates a new Listener that listens on all interfaces accept wg0 +func NewAllAcceptWireguardListener() Listener { + return &wg0FilteringListener{ + includeWg0: false, + } +} + +// NewOnlyWireguardListener creates a new Listener that listens on all interfaces +func NewOnlyWireguardListener() Listener { + return &wg0FilteringListener{ + includeWg0: true, + } +} From fe25db286ff942617804f7c61df0fbf2d40f24d2 Mon Sep 17 00:00:00 2001 From: Konstanty Karagiorgis Date: Tue, 28 May 2024 15:14:23 +0200 Subject: [PATCH 2/7] 54: Added tests --- pkg/nginx/exposer.go | 6 +-- pkg/nginx/listener.go | 80 ++++++++++++++++++++++++-------------- pkg/nginx/listener_test.go | 68 ++++++++++++++++++++++++++++++++ 3 files changed, 122 insertions(+), 32 deletions(-) create mode 100644 pkg/nginx/listener_test.go diff --git a/pkg/nginx/exposer.go b/pkg/nginx/exposer.go index 1614638..7e48b91 100644 --- a/pkg/nginx/exposer.go +++ b/pkg/nginx/exposer.go @@ -36,13 +36,13 @@ func (n *Exposer) Add(app peers.App) (peers.App, error) { File: nginxConfigPath(n.prefix, app), App: app, } - listen := "" + listenBlock := "" listenAddrs, listenerErr := n.listener.Addrs(port) if listenerErr != nil || len(listenAddrs) == 0 { logrus.Errorf("Could not get listener addresses: %v", listenerErr) } for _, addr := range listenAddrs { - listen += fmt.Sprintf(" listen %s;\n", addr) + listenBlock += fmt.Sprintf(" listen %s;\n", addr) } if writeErr := afero.WriteFile(n.fs, path.Join(n.path, server.File), []byte(fmt.Sprintf(` # [%s] %s @@ -53,7 +53,7 @@ server { `, server.App.Peer, server.App.Name, - listen, + listenBlock, server.ProxyPass, )), 0644); writeErr != nil { logrus.Errorf("Could not write NGINX config file: %v", writeErr) diff --git a/pkg/nginx/listener.go b/pkg/nginx/listener.go index 0e38e7d..fbdc9e5 100644 --- a/pkg/nginx/listener.go +++ b/pkg/nginx/listener.go @@ -12,6 +12,15 @@ type Listener interface { Addrs(portNumber int) ([]string, error) } +type networkInterface struct { + name string + addresses []string +} + +type networkInterfaceLister interface { + Interfaces() ([]networkInterface, error) +} + type portOnlyListener struct { } @@ -26,12 +35,13 @@ func NewPortOnlyListener() Listener { } type wg0FilteringListener struct { + lister networkInterfaceLister includeWg0 bool } // Addrs implements Listener func (a *wg0FilteringListener) Addrs(portNumber int) ([]string, error) { - interfaces, interfacesErr := net.Interfaces() + interfaces, interfacesErr := a.lister.Interfaces() if interfacesErr != nil { return []string{}, interfacesErr } @@ -39,49 +49,28 @@ func (a *wg0FilteringListener) Addrs(portNumber int) ([]string, error) { var allAddrs []string for _, iface := range interfaces { - if iface.Name == "wg0" && !a.includeWg0 { + if iface.name == "wg0" && !a.includeWg0 { continue } - if iface.Name != "wg0" && a.includeWg0 { - continue - } - - addrs, err := iface.Addrs() - if err != nil { - fmt.Println("Error retrieving addresses for interface:", iface.Name, err) + if iface.name != "wg0" && a.includeWg0 { continue } - for _, addr := range addrs { - switch v := addr.(type) { - // Ignore ipv6 - case *net.IPNet: - if strings.Contains(v.IP.String(), ":") { - continue - } else { - allAddrs = append(allAddrs, formatIP(v.IP, portNumber)) - } - - case *net.IPAddr: - if strings.Contains(v.IP.String(), ":") { - continue - } else { - allAddrs = append(allAddrs, formatIP(v.IP, portNumber)) - } + for _, addr := range iface.addresses { + // Ignore IPv6 + if !strings.Contains(addr, "::") { + allAddrs = append(allAddrs, fmt.Sprintf("%s:%d", addr, portNumber)) } } } return allAddrs, nil } -func formatIP(ip fmt.Stringer, portNumber int) string { - return fmt.Sprintf("%s:%d", ip.String(), portNumber) -} - // NewAllAcceptWireguardListener creates a new Listener that listens on all interfaces accept wg0 func NewAllAcceptWireguardListener() Listener { return &wg0FilteringListener{ includeWg0: false, + lister: &defaultNetworkInterfaceLister{}, } } @@ -89,5 +78,38 @@ func NewAllAcceptWireguardListener() Listener { func NewOnlyWireguardListener() Listener { return &wg0FilteringListener{ includeWg0: true, + lister: &defaultNetworkInterfaceLister{}, + } +} + +type defaultNetworkInterfaceLister struct { +} + +// Interfaces implements networkinterfacelister +func (l *defaultNetworkInterfaceLister) Interfaces() ([]networkInterface, error) { + ifaces, err := net.Interfaces() + if err != nil { + return nil, err + } + var result []networkInterface + for _, iface := range ifaces { + netAddrs, err := iface.Addrs() + if err != nil { + return nil, err + } + var addrs []string + for _, addr := range netAddrs { + switch v := addr.(type) { + case *net.IPNet: + addrs = append(addrs, v.IP.String()) + case *net.IPAddr: + addrs = append(addrs, v.IP.String()) + } + } + result = append(result, networkInterface{ + name: iface.Name, + addresses: addrs, + }) } + return result, nil } diff --git a/pkg/nginx/listener_test.go b/pkg/nginx/listener_test.go new file mode 100644 index 0000000..e0cce0f --- /dev/null +++ b/pkg/nginx/listener_test.go @@ -0,0 +1,68 @@ +package nginx + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +type mockLister struct { + interfaces []networkInterface + err error +} + +func (m *mockLister) Interfaces() ([]networkInterface, error) { + return m.interfaces, m.err +} + +func TestOnlyWgListener(t *testing.T) { + // given + lister := &mockLister{ + interfaces: []networkInterface{ + { + name: "eth0", + addresses: []string{"127.0.0.1"}, + }, + { + name: "wg0", + addresses: []string{"10.178.2.1"}, + }, + }, + } + listenerIf := NewOnlyWireguardListener() + listener := listenerIf.(*wg0FilteringListener) + listener.lister = lister + + // when + addrs, err := listener.Addrs(80) + + // then + assert.NoError(t, err) + assert.ElementsMatch(t, []string{"10.178.2.1:80"}, addrs) +} + +func TestAllAcceptWgListener(t *testing.T) { + // given + lister := &mockLister{ + interfaces: []networkInterface{ + { + name: "eth0", + addresses: []string{"127.0.0.1"}, + }, + { + name: "wg0", + addresses: []string{"10.178.2.1"}, + }, + }, + } + listenerIf := NewAllAcceptWireguardListener() + listener := listenerIf.(*wg0FilteringListener) + listener.lister = lister + + // when + addrs, err := listener.Addrs(80) + + // then + assert.NoError(t, err) + assert.ElementsMatch(t, []string{"127.0.0.1:80"}, addrs) +} From a149157af11482b29f815a1ed3926d8f4d372629 Mon Sep 17 00:00:00 2001 From: Konstanty Karagiorgis Date: Wed, 29 May 2024 11:48:44 +0200 Subject: [PATCH 3/7] 54: Added extra listener --- pkg/nginx/exposer.go | 12 ++++++++--- pkg/nginx/listener.go | 43 ++++++++++++++++++++++++-------------- pkg/nginx/listener_test.go | 28 +------------------------ 3 files changed, 37 insertions(+), 46 deletions(-) diff --git a/pkg/nginx/exposer.go b/pkg/nginx/exposer.go index 7e48b91..e2a73b5 100644 --- a/pkg/nginx/exposer.go +++ b/pkg/nginx/exposer.go @@ -6,7 +6,9 @@ import ( "os" "path" "strings" + "time" + "github.com/avast/retry-go/v4" "github.com/glothriel/wormhole/pkg/listeners" "github.com/glothriel/wormhole/pkg/peers" "github.com/sirupsen/logrus" @@ -37,9 +39,13 @@ func (n *Exposer) Add(app peers.App) (peers.App, error) { App: app, } listenBlock := "" - listenAddrs, listenerErr := n.listener.Addrs(port) - if listenerErr != nil || len(listenAddrs) == 0 { - logrus.Errorf("Could not get listener addresses: %v", listenerErr) + listenAddrs := []string{} + if listenerErr := retry.Do(func() error { + var addrsErr error + listenAddrs, addrsErr = n.listener.Addrs(port) + return addrsErr + }, retry.Attempts(5), retry.Delay(time.Second)); listenerErr != nil { + return peers.App{}, fmt.Errorf("Could not get listener addresses: %v", listenerErr) } for _, addr := range listenAddrs { listenBlock += fmt.Sprintf(" listen %s;\n", addr) diff --git a/pkg/nginx/listener.go b/pkg/nginx/listener.go index fbdc9e5..681fd77 100644 --- a/pkg/nginx/listener.go +++ b/pkg/nginx/listener.go @@ -1,6 +1,7 @@ package nginx import ( + "errors" "fmt" "net" "strings" @@ -34,13 +35,12 @@ func NewPortOnlyListener() Listener { return &portOnlyListener{} } -type wg0FilteringListener struct { - lister networkInterfaceLister - includeWg0 bool +type allAcceptWg0Listener struct { + lister networkInterfaceLister } // Addrs implements Listener -func (a *wg0FilteringListener) Addrs(portNumber int) ([]string, error) { +func (a *allAcceptWg0Listener) Addrs(portNumber int) ([]string, error) { interfaces, interfacesErr := a.lister.Interfaces() if interfacesErr != nil { return []string{}, interfacesErr @@ -49,10 +49,7 @@ func (a *wg0FilteringListener) Addrs(portNumber int) ([]string, error) { var allAddrs []string for _, iface := range interfaces { - if iface.name == "wg0" && !a.includeWg0 { - continue - } - if iface.name != "wg0" && a.includeWg0 { + if iface.name == "wg0" { continue } @@ -63,22 +60,36 @@ func (a *wg0FilteringListener) Addrs(portNumber int) ([]string, error) { } } } + + if len(allAddrs) == 0 { + return []string{}, errors.New("No network interfaces matching conditions found") + } + return allAddrs, nil } +type givenAddressOnlyListener struct { + address string +} + +// Addrs implements Listener +func (l *givenAddressOnlyListener) Addrs(portNumber int) ([]string, error) { + return []string{ + fmt.Sprintf("%s:%d", l.address, portNumber), + }, nil +} + // NewAllAcceptWireguardListener creates a new Listener that listens on all interfaces accept wg0 func NewAllAcceptWireguardListener() Listener { - return &wg0FilteringListener{ - includeWg0: false, - lister: &defaultNetworkInterfaceLister{}, + return &allAcceptWg0Listener{ + lister: &defaultNetworkInterfaceLister{}, } } -// NewOnlyWireguardListener creates a new Listener that listens on all interfaces -func NewOnlyWireguardListener() Listener { - return &wg0FilteringListener{ - includeWg0: true, - lister: &defaultNetworkInterfaceLister{}, +// NewOnlyGivenAddressListener creates a new Listener that listens only on given address +func NewOnlyGivenAddressListener(address string) Listener { + return &givenAddressOnlyListener{ + address: address, } } diff --git a/pkg/nginx/listener_test.go b/pkg/nginx/listener_test.go index e0cce0f..6cbe9fb 100644 --- a/pkg/nginx/listener_test.go +++ b/pkg/nginx/listener_test.go @@ -15,32 +15,6 @@ func (m *mockLister) Interfaces() ([]networkInterface, error) { return m.interfaces, m.err } -func TestOnlyWgListener(t *testing.T) { - // given - lister := &mockLister{ - interfaces: []networkInterface{ - { - name: "eth0", - addresses: []string{"127.0.0.1"}, - }, - { - name: "wg0", - addresses: []string{"10.178.2.1"}, - }, - }, - } - listenerIf := NewOnlyWireguardListener() - listener := listenerIf.(*wg0FilteringListener) - listener.lister = lister - - // when - addrs, err := listener.Addrs(80) - - // then - assert.NoError(t, err) - assert.ElementsMatch(t, []string{"10.178.2.1:80"}, addrs) -} - func TestAllAcceptWgListener(t *testing.T) { // given lister := &mockLister{ @@ -56,7 +30,7 @@ func TestAllAcceptWgListener(t *testing.T) { }, } listenerIf := NewAllAcceptWireguardListener() - listener := listenerIf.(*wg0FilteringListener) + listener := listenerIf.(*allAcceptWg0Listener) listener.lister = lister // when From 4cc9fbc9186648f7f405e3126bbac2960317604d Mon Sep 17 00:00:00 2001 From: Konstanty Karagiorgis Date: Wed, 29 May 2024 11:50:03 +0200 Subject: [PATCH 4/7] 54: Modified the CLI --- pkg/cmd/client.go | 15 +++++++-------- pkg/cmd/server.go | 2 +- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/pkg/cmd/client.go b/pkg/cmd/client.go index 7b8602f..066c648 100644 --- a/pkg/cmd/client.go +++ b/pkg/cmd/client.go @@ -45,14 +45,6 @@ var joinCommand *cli.Command = &cli.Command{ } startPrometheusServer(c) - localListenerRegistry := listeners.NewApps(nginx.NewNginxExposer( - c.String(nginxExposerConfdPathFlag.Name), - "local", - nginx.NewDefaultReloader(), - nginx.NewRangePortAllocator(20000, 25000), - nginx.NewOnlyWireguardListener(), - )) - remoteNginxExposer := nginx.NewNginxExposer( c.String(nginxExposerConfdPathFlag.Name), "remote", @@ -115,6 +107,13 @@ var joinCommand *cli.Command = &cli.Command{ } break } + localListenerRegistry := listeners.NewApps(nginx.NewNginxExposer( + c.String(nginxExposerConfdPathFlag.Name), + "local", + nginx.NewDefaultReloader(), + nginx.NewRangePortAllocator(20000, 25000), + nginx.NewOnlyGivenAddressListener(pairingResponse.AssignedIP), + )) logrus.Infof("Paired with server, assigned IP: %s", pairingResponse.AssignedIP) go localListenerRegistry.Watch(getAppStateChangeGenerator(c).Changes(), make(chan bool)) diff --git a/pkg/cmd/server.go b/pkg/cmd/server.go index f5176ed..8859cf9 100644 --- a/pkg/cmd/server.go +++ b/pkg/cmd/server.go @@ -81,7 +81,7 @@ var listenCommand *cli.Command = &cli.Command{ "local", nginx.NewDefaultReloader(), nginx.NewRangePortAllocator(20000, 25000), - nginx.NewOnlyWireguardListener(), + nginx.NewOnlyGivenAddressListener(c.String(wgAddressFlag.Name)), )) remoteNginxExposer := nginx.NewNginxExposer( From 638910be18be1ab2c06ec14a2ae1a25674c81e6a Mon Sep 17 00:00:00 2001 From: Konstanty Karagiorgis Date: Wed, 29 May 2024 11:55:59 +0200 Subject: [PATCH 5/7] 54: Added integration tests --- tests/conftest.py | 1 + tests/test_kubernetes.py | 42 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/tests/conftest.py b/tests/conftest.py index 7cfb1c5..96a2df2 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -235,6 +235,7 @@ def k8s_server( { "server.enabled": True, "server.wg.publicHost": "wormhole-server-server.server.svc.cluster.local", + "server.service.type": "ClusterIP", "docker.image": wormhole_image.split(":")[0], "docker.version": wormhole_image.split(":")[1], "docker.wgImage": wireguard_image.split(":")[0], diff --git a/tests/test_kubernetes.py b/tests/test_kubernetes.py index 1996aa9..faa4469 100644 --- a/tests/test_kubernetes.py +++ b/tests/test_kubernetes.py @@ -201,4 +201,44 @@ def _ensure_that_proxied_service_is_deleted(): if 'client-custom' in [ svc['metadata']['name'] for svc in kubectl.json(["-n", "server", "get", "svc"])["items"] ]: - pytest.skip("The orphaned service should be removed, but it's not critical, so skipping for now") + pytest.skip( + "The orphaned service should be removed, but it's not critical, so skipping for now" + ) + + +def test_connection_via_the_tunnel( + kubectl, + k8s_server, + k8s_client, + mock_server, +): + + annotator = Annotator(mock_server, kubectl) + amount_of_services_before_annotation = len( + kubectl.json(["-n", "server", "get", "svc"])["items"] + ) + annotator.do("wormhole.glothriel.github.com/exposed", "yes") + + @retry(tries=60, delay=1) + def _ensure_that_proxied_service_is_created(): + assert ( + len(kubectl.json(["-n", "server", "get", "svc"])["items"]) + == amount_of_services_before_annotation + 1 + ) + _ensure_that_proxied_service_is_created() + + @retry(tries=60, delay=1) + def _ensure_that_proxied_service_is_reachable(): + kubectl.run( + [ + '-n', + 'nginx', + 'exec', + 'deployment/nginx', + '--', + 'curl', + 'server-nginx-nginx.client.svc.cluster.local' + ] + ) + + _ensure_that_proxied_service_is_reachable() From 3d2e0b0b17216d6858bbae2392038694ca64be2f Mon Sep 17 00:00:00 2001 From: Konstanty Karagiorgis Date: Wed, 29 May 2024 12:09:03 +0200 Subject: [PATCH 6/7] #54: Removed redundant retries --- pkg/nginx/exposer.go | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/pkg/nginx/exposer.go b/pkg/nginx/exposer.go index e2a73b5..81f17c7 100644 --- a/pkg/nginx/exposer.go +++ b/pkg/nginx/exposer.go @@ -6,9 +6,7 @@ import ( "os" "path" "strings" - "time" - "github.com/avast/retry-go/v4" "github.com/glothriel/wormhole/pkg/listeners" "github.com/glothriel/wormhole/pkg/peers" "github.com/sirupsen/logrus" @@ -39,13 +37,9 @@ func (n *Exposer) Add(app peers.App) (peers.App, error) { App: app, } listenBlock := "" - listenAddrs := []string{} - if listenerErr := retry.Do(func() error { - var addrsErr error - listenAddrs, addrsErr = n.listener.Addrs(port) - return addrsErr - }, retry.Attempts(5), retry.Delay(time.Second)); listenerErr != nil { - return peers.App{}, fmt.Errorf("Could not get listener addresses: %v", listenerErr) + listenAddrs, addrsErr := n.listener.Addrs(port) + if addrsErr != nil { + return peers.App{}, fmt.Errorf("Could not get listener addresses: %v", addrsErr) } for _, addr := range listenAddrs { listenBlock += fmt.Sprintf(" listen %s;\n", addr) From 3aad9ab5862959c8da56ad82f3a8e92073275957 Mon Sep 17 00:00:00 2001 From: Konstanty Karagiorgis Date: Mon, 3 Jun 2024 12:11:43 +0200 Subject: [PATCH 7/7] #54: Code review --- pkg/nginx/listener.go | 17 ++--------- pkg/nginx/listener_test.go | 60 ++++++++++++++++++++++++++++++++++++++ tests/fixtures.py | 11 +++++++ tests/test_kubernetes.py | 32 +++++++------------- 4 files changed, 84 insertions(+), 36 deletions(-) diff --git a/pkg/nginx/listener.go b/pkg/nginx/listener.go index 681fd77..111a6d1 100644 --- a/pkg/nginx/listener.go +++ b/pkg/nginx/listener.go @@ -7,6 +7,8 @@ import ( "strings" ) +const wg0InterfaceName = "wg0" + // Listener is an interface for NGINX listeners type Listener interface { // Addrs returns a list of addresses that the listener is listening on @@ -22,19 +24,6 @@ type networkInterfaceLister interface { Interfaces() ([]networkInterface, error) } -type portOnlyListener struct { -} - -// Addrs implements Listener -func (p *portOnlyListener) Addrs(portNumber int) ([]string, error) { - return []string{fmt.Sprintf("%d", portNumber)}, nil -} - -// NewPortOnlyListener creates a new Listener that listens on a single port -func NewPortOnlyListener() Listener { - return &portOnlyListener{} -} - type allAcceptWg0Listener struct { lister networkInterfaceLister } @@ -49,7 +38,7 @@ func (a *allAcceptWg0Listener) Addrs(portNumber int) ([]string, error) { var allAddrs []string for _, iface := range interfaces { - if iface.name == "wg0" { + if iface.name == wg0InterfaceName { continue } diff --git a/pkg/nginx/listener_test.go b/pkg/nginx/listener_test.go index 6cbe9fb..a4d7dd9 100644 --- a/pkg/nginx/listener_test.go +++ b/pkg/nginx/listener_test.go @@ -1,6 +1,7 @@ package nginx import ( + "errors" "testing" "github.com/stretchr/testify/assert" @@ -40,3 +41,62 @@ func TestAllAcceptWgListener(t *testing.T) { assert.NoError(t, err) assert.ElementsMatch(t, []string{"127.0.0.1:80"}, addrs) } +func TestAllAcceptWgListenerErrors(t *testing.T) { + tests := []struct { + name string + interfaces []networkInterface + err error + expected []string + }{ + { + name: "Nonempty interface list with error", + interfaces: []networkInterface{ + + { + name: "eth0", + addresses: []string{"127.0.0.1"}, + }, + { + name: "wg0", + addresses: []string{"10.178.2.1"}, + }, + }, + err: errors.New("Blabla"), + expected: nil, + }, + { + name: "Empty interface list without error", + interfaces: []networkInterface{}, + err: nil, + expected: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + lister := &mockLister{ + interfaces: tt.interfaces, + err: tt.err, + } + listenerIf := NewAllAcceptWireguardListener() + listener := listenerIf.(*allAcceptWg0Listener) + listener.lister = lister + + _, err := listener.Addrs(80) + + assert.Error(t, err) + }) + } +} + +func TestGivenAddressOnlyListener(t *testing.T) { + // given + listener := NewOnlyGivenAddressListener("127.0.0.1") + + // when + addrs, err := listener.Addrs(80) + + // then + assert.NoError(t, err) + assert.ElementsMatch(t, []string{"127.0.0.1:80"}, addrs) +} diff --git a/tests/fixtures.py b/tests/fixtures.py index 9633a8a..4501b4d 100644 --- a/tests/fixtures.py +++ b/tests/fixtures.py @@ -465,3 +465,14 @@ def do(self, key, value): "--overwrite" ] ) + + +class Services: + + @classmethod + def count(cls, kubectl, namespace): + return len(kubectl.json(["get", "svc", "-n", namespace])["items"]) + + @classmethod + def names(cls, kubectl, namespace): + return [item["metadata"]["name"] for item in kubectl.json(["get", "svc", "-n", namespace])["items"]] diff --git a/tests/test_kubernetes.py b/tests/test_kubernetes.py index faa4469..364c360 100644 --- a/tests/test_kubernetes.py +++ b/tests/test_kubernetes.py @@ -1,7 +1,7 @@ import pytest from retry import retry -from .fixtures import Annotator +from .fixtures import Annotator, Services def test_changing_annotation_causes_creating_proxy_service( @@ -12,15 +12,13 @@ def test_changing_annotation_causes_creating_proxy_service( ): annotator = Annotator(mock_server, kubectl) - amount_of_services_before_annotation = len( - kubectl.json(["-n", "server", "get", "svc"])["items"] - ) + amount_of_services_before_annotation = Services.count(kubectl, "server") annotator.do("wormhole.glothriel.github.com/exposed", "yes") @retry(tries=60, delay=1) def _ensure_that_proxied_service_is_created(): assert ( - len(kubectl.json(["-n", "server", "get", "svc"])["items"]) + Services.count(kubectl, "server") == amount_of_services_before_annotation + 1 ) _ensure_that_proxied_service_is_created() @@ -29,7 +27,7 @@ def _ensure_that_proxied_service_is_created(): @retry(tries=60, delay=1) def _ensure_that_proxied_service_is_deleted(): assert ( - len(kubectl.json(["-n", "server", "get", "svc"])["items"]) + Services.count(kubectl, "server") == amount_of_services_before_annotation ) @@ -48,12 +46,8 @@ def test_annotating_with_custom_name_correctly_sets_remote_name( @retry(tries=60, delay=1) def _ensure_that_proxied_service_is_created(): - assert 'client-huehue-one-two-three' in [ - svc['metadata']['name'] for svc in kubectl.json(["-n", "server", "get", "svc"])["items"] - ] - assert 'server-huehue-one-two-three' in [ - svc['metadata']['name'] for svc in kubectl.json(["-n", "client", "get", "svc"])["items"] - ] + assert 'client-huehue-one-two-three' in Services.names(kubectl, namespace="server") + assert 'server-huehue-one-two-three' in Services.names(kubectl, namespace="client") _ensure_that_proxied_service_is_created() @@ -61,12 +55,8 @@ def _ensure_that_proxied_service_is_created(): @retry(tries=60, delay=1) def _ensure_that_proxied_service_is_deleted(): - assert 'client-huehue-one-two-three' not in [ - svc['metadata']['name'] for svc in kubectl.json(["-n", "server", "get", "svc"])["items"] - ] - assert 'server-huehue-one-two-three' not in [ - svc['metadata']['name'] for svc in kubectl.json(["-n", "client", "get", "svc"])["items"] - ] + assert 'client-huehue-one-two-three' not in Services.names(kubectl, namespace="server") + assert 'server-huehue-one-two-three' not in Services.names(kubectl, namespace="client") _ensure_that_proxied_service_is_deleted() @@ -214,15 +204,13 @@ def test_connection_via_the_tunnel( ): annotator = Annotator(mock_server, kubectl) - amount_of_services_before_annotation = len( - kubectl.json(["-n", "server", "get", "svc"])["items"] - ) + amount_of_services_before_annotation = Services.count(kubectl, "server") annotator.do("wormhole.glothriel.github.com/exposed", "yes") @retry(tries=60, delay=1) def _ensure_that_proxied_service_is_created(): assert ( - len(kubectl.json(["-n", "server", "get", "svc"])["items"]) + Services.count(kubectl, "server") == amount_of_services_before_annotation + 1 ) _ensure_that_proxied_service_is_created()