Skip to content

🐛 fix(proxy): eliminate data race in DialContext #12778

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+19 −14
Open

🐛 fix(proxy): eliminate data race in DialContext #12778

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -943,9 +943,16 @@ test-infrastructure: $(SETUP_ENVTEST) ## Run unit and integration tests with rac
# Note: Fuzz tests are not executed with race detector because they would just time out.
# To achieve that, all files with fuzz tests have the "!race" build tag, to still run fuzz tests
# we have an additional `go test` run that focuses on "TestFuzzyConversion".
cd test/infrastructure; KUBEBUILDER_ASSETS="$(KUBEBUILDER_ASSETS)" go test ./... $(TEST_ARGS)
cd test/infrastructure; KUBEBUILDER_ASSETS="$(KUBEBUILDER_ASSETS)" go test -race ./... $(TEST_ARGS)
$(MAKE) test-infrastructure-conversions TEST_ARGS="$(TEST_ARGS)"

.PHONY: test-infrastructure-no-race
test-infrastructure-no-race: $(SETUP_ENVTEST) ## Run unit and integration tests with no race detector for docker infrastructure provider
# Note: Fuzz tests are not executed with race detector because they would just time out.
# To achieve that, all files with fuzz tests have the "!race" build tag, to still run fuzz tests
# we have an additional `go test` run that focuses on "TestFuzzyConversion".
cd test/infrastructure; KUBEBUILDER_ASSETS="$(KUBEBUILDER_ASSETS)" go test ./... $(TEST_ARGS)

.PHONY: test-infrastructure-conversions
test-infrastructure-conversions: $(SETUP_ENVTEST) ## Run conversions test for docker infrastructure provider
cd test/infrastructure; KUBEBUILDER_ASSETS="$(KUBEBUILDER_ASSETS)" go test -run "^TestFuzzyConversion$$" ./... $(TEST_ARGS)
Expand All @@ -956,7 +963,7 @@ test-infrastructure-verbose: ## Run unit and integration tests with race detecto

.PHONY: test-infrastructure-junit
test-infrastructure-junit: $(SETUP_ENVTEST) $(GOTESTSUM) ## Run unit and integration tests with race detector and generate a junit report for docker infrastructure provider
cd test/infrastructure; set +o errexit; (KUBEBUILDER_ASSETS="$(KUBEBUILDER_ASSETS)" go test -json ./... $(TEST_ARGS); echo $$? > $(ARTIFACTS)/junit.infra_docker.exitcode) | tee $(ARTIFACTS)/junit.infra_docker.stdout
cd test/infrastructure; set +o errexit; (KUBEBUILDER_ASSETS="$(KUBEBUILDER_ASSETS)" go test -race -json ./... $(TEST_ARGS); echo $$? > $(ARTIFACTS)/junit.infra_docker.exitcode) | tee $(ARTIFACTS)/junit.infra_docker.stdout
$(GOTESTSUM) --junitfile $(ARTIFACTS)/junit.infra_docker.xml --raw-command cat $(ARTIFACTS)/junit.infra_docker.stdout
exit $$(cat $(ARTIFACTS)/junit.infra_docker.exitcode)
cd test/infrastructure; set +o errexit; (KUBEBUILDER_ASSETS="$(KUBEBUILDER_ASSETS)" go test -run "^TestFuzzyConversion$$" -json ./... $(TEST_ARGS); echo $$? > $(ARTIFACTS)/junit-fuzz.infra_docker.exitcode) | tee $(ARTIFACTS)/junit-fuzz.infra_docker.stdout
Expand Down
22 changes: 10 additions & 12 deletions test/infrastructure/inmemory/pkg/server/proxy/dial.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,9 @@ const defaultTimeout = 10 * time.Second

// Dialer creates connections using Kubernetes API Server port-forwarding.
type Dialer struct {
proxy Proxy
clientset *kubernetes.Clientset
proxyTransport http.RoundTripper
upgrader spdy.Upgrader
timeout time.Duration
proxy Proxy
clientset *kubernetes.Clientset
timeout time.Duration
}

// NewDialer creates a new dialer for a given API server scope.
Expand Down Expand Up @@ -67,12 +65,6 @@ func NewDialer(p Proxy, options ...func(*Dialer) error) (*Dialer, error) {
if err != nil {
return nil, err
}
proxyTransport, upgrader, err := spdy.RoundTripperFor(p.KubeConfig)
if err != nil {
return nil, err
}
dialer.proxyTransport = proxyTransport
dialer.upgrader = upgrader
dialer.clientset = clientset
return dialer, nil
}
Expand All @@ -85,14 +77,20 @@ func (d *Dialer) DialContextWithAddr(ctx context.Context, addr string) (net.Conn
// DialContext creates proxied port-forwarded connections.
// ctx is currently unused, but fulfils the type signature used by GRPC.
func (d *Dialer) DialContext(_ context.Context, _ string, addr string) (net.Conn, error) {
proxyTransport, upgrader, err := spdy.RoundTripperFor(d.proxy.KubeConfig)
if err != nil {
return nil, err
}
httpClient := &http.Client{Transport: proxyTransport}

req := d.clientset.CoreV1().RESTClient().
Post().
Resource(d.proxy.Kind).
Namespace(d.proxy.Namespace).
Name(addr).
SubResource("portforward")

dialer := spdy.NewDialer(d.upgrader, &http.Client{Transport: d.proxyTransport}, "POST", req.URL())
dialer := spdy.NewDialer(upgrader, httpClient, "POST", req.URL())

// Create a new connection from the dialer.
//
Expand Down