Skip to content

Commit

Permalink
ci: remove logger field from portforwarder to avoid race with gorouti…
Browse files Browse the repository at this point in the history
…ne (#2959)

do not pass testing logger into goroutine to avoid race
  • Loading branch information
QxBytes authored Aug 27, 2024
1 parent 00f42f1 commit afdc2b8
Show file tree
Hide file tree
Showing 4 changed files with 9 additions and 16 deletions.
8 changes: 1 addition & 7 deletions test/e2e/framework/kubernetes/port-forward.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ func (p *PortForward) Run() error {

log.Printf("attempting port forward to pod name \"%s\" with label \"%s\", in namespace \"%s\"...\n", targetPodName, p.LabelSelector, p.Namespace)

p.pf, err = k8s.NewPortForwarder(config, &logger{}, opts)
p.pf, err = k8s.NewPortForwarder(config, opts)
if err != nil {
return fmt.Errorf("could not create port forwarder: %w", err)
}
Expand Down Expand Up @@ -161,9 +161,3 @@ func (p *PortForward) Stop() error {
p.pf.Stop()
return nil
}

type logger struct{}

func (l *logger) Logf(format string, args ...interface{}) {
log.Printf(format, args...)
}
2 changes: 1 addition & 1 deletion test/integration/datapath/datapath_linux_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ func TestDatapathLinux(t *testing.T) {
DestPort: 8080,
}

pf, err := k8s.NewPortForwarder(restConfig, t, pfOpts)
pf, err := k8s.NewPortForwarder(restConfig, pfOpts)
if err != nil {
t.Fatal(err)
}
Expand Down
2 changes: 1 addition & 1 deletion test/integration/k8s_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ func TestPodScaling(t *testing.T) {
}

pingCheckFn := func() error {
pf, err := NewPortForwarder(restConfig, t, pfOpts)
pf, err := NewPortForwarder(restConfig, pfOpts)
if err != nil {
t.Fatalf("could not build port forwarder: %v", err)
}
Expand Down
13 changes: 6 additions & 7 deletions test/integration/portforward.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"fmt"
"io"
"log"
"math/rand"
"net/http"
"sync"
Expand All @@ -26,7 +27,6 @@ type PortForwarder struct {
clientset *kubernetes.Clientset
transport http.RoundTripper
upgrader spdy.Upgrader
logger logger

opts PortForwardingOpts

Expand All @@ -45,7 +45,7 @@ type PortForwardingOpts struct {
}

// NewPortForwarder creates a PortForwarder.
func NewPortForwarder(restConfig *rest.Config, logger logger, opts PortForwardingOpts) (*PortForwarder, error) {
func NewPortForwarder(restConfig *rest.Config, opts PortForwardingOpts) (*PortForwarder, error) {
clientset, err := kubernetes.NewForConfig(restConfig)
if err != nil {
return nil, fmt.Errorf("could not create clientset: %w", err)
Expand All @@ -60,7 +60,6 @@ func NewPortForwarder(restConfig *rest.Config, logger logger, opts PortForwardin
clientset: clientset,
transport: transport,
upgrader: upgrader,
logger: logger,
opts: opts,
stopChan: make(chan struct{}, 1),
}, nil
Expand Down Expand Up @@ -173,7 +172,7 @@ func (p *PortForwarder) KeepAlive(ctx context.Context) {
for {
select {
case <-ctx.Done():
p.logger.Logf("port forwarder: keep alive cancelled: %v", ctx.Err())
log.Printf("port forwarder: keep alive cancelled: %v", ctx.Err())
return
case pfErr := <-p.errChan:
// as of client-go v0.26.1, if the connection is successful at first but then fails,
Expand All @@ -182,14 +181,14 @@ func (p *PortForwarder) KeepAlive(ctx context.Context) {
//
// see https://github.com/kubernetes/client-go/commit/d0842249d3b92ea67c446fe273f84fe74ebaed9f
// for the relevant change.
p.logger.Logf("port forwarder: received error signal: %v. restarting session", pfErr)
log.Printf("port forwarder: received error signal: %v. restarting session", pfErr)
p.Stop()
if err := p.Forward(ctx); err != nil {
p.logger.Logf("port forwarder: could not restart session: %v. retrying", err)
log.Printf("port forwarder: could not restart session: %v. retrying", err)

select {
case <-ctx.Done():
p.logger.Logf("port forwarder: keep alive cancelled: %v", ctx.Err())
log.Printf("port forwarder: keep alive cancelled: %v", ctx.Err())
return
case <-time.After(time.Second): // todo: make configurable?
continue
Expand Down

0 comments on commit afdc2b8

Please sign in to comment.