Skip to content
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.

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

potential SIGPIPE if configuring tls client wrong #7807

Closed
hongchaodeng opened this issue Apr 23, 2017 · 5 comments
Closed

potential SIGPIPE if configuring tls client wrong #7807

hongchaodeng opened this issue Apr 23, 2017 · 5 comments
Assignees
Labels

Comments

@hongchaodeng
Copy link
Contributor

hongchaodeng commented Apr 23, 2017

Scenario

When calling clientv3.New() with the wrong scheme -- "http://xxx", "https://xxx", program will receive SIGPIPE. Such user error should not cause program level exit.

Environment

etcd version:

  • server: 3.0.17, 3.1.4, 3.1.7
  • client: 3.1.0, 3.1.7

OS: linux

Reproduce

Updated with simpler reproduce steps without TLS:

...

func main() {
	c := make(chan os.Signal, 1)
	signal.Notify(c)
	go func() {
		log.Printf("received signal: %v", <-c)
		os.Exit(1)
	}()
// 2380 is peer port
	if _, err := CheckHealth("http://localhost:2380", nil); err != nil {
		panic(err)
	}

	fmt.Println("sleeping...")
	time.Sleep(5 * time.Second)
}

func CheckHealth(url string, tc *tls.Config) (bool, error) {
	cfg := clientv3.Config{
		Endpoints:   []string{url},
		DialTimeout: 5 * time.Second,
		TLS:         tc,
	}
	etcdcli, err := clientv3.New(cfg)
	if err != nil {
		return false, fmt.Errorf("failed to create etcd client for %s: %v", url, err)
	}
	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
	_, err = etcdcli.Get(ctx, "/", clientv3.WithSerializable())
	cancel()
	etcdcli.Close()
	if err != nil {
		return false, fmt.Errorf("etcd health probing failed for %s: %v", url, err)
	}
	return true, nil
}
@heyitsanthony heyitsanthony added this to the unplanned milestone Apr 25, 2017
@gyuho gyuho self-assigned this Apr 27, 2017
@gyuho
Copy link
Contributor

gyuho commented Apr 28, 2017

@hongchaodeng Can't reproduce. Tried the same code and etcd server/client versions, with wrong scheme. I do not get SIGPIPE, but this error, which is expected for wrong scheme.

rpc error: code = 13 desc = transport is closing

@gyuho
Copy link
Contributor

gyuho commented May 1, 2017

@hongchaodeng This happens only in release-3.1 branch. So try with 3.2.0-rc.0 or master branch. release-3.1 passes initial endpoint when dialing, while master branch does not.
When cfg.Endpoints[0] is passed to processCreds with http scheme, we overwrite TLS fields with nil, so the client request would just hang.

release-3.1 https://github.com/coreos/etcd/blob/release-3.1/clientv3/client.go#L343-L349

client.balancer = newSimpleBalancer(cfg.Endpoints)
conn, err := client.dial(cfg.Endpoints[0], grpc.WithBalancer(client.balancer))

master https://github.com/coreos/etcd/blob/master/clientv3/client.go#L369-L370

client.balancer = newSimpleBalancer(cfg.Endpoints)
conn, err := client.dial("", grpc.WithBalancer(client.balancer))

c.f. #7733

I don't think we would backport that PR. It was for benchmarking. /cc @heyitsanthony @xiang90

@gyuho
Copy link
Contributor

gyuho commented May 1, 2017

Was able to reproduce with @hongchaodeng

Client version (v3.1.x, master) does not matter.
Only happens when requesting against v3.1.x server.

package main

import (
	"context"
	"crypto/tls"
	"fmt"
	"log"
	"os"
	"os/signal"
	"time"

	"github.com/coreos/etcd/clientv3"
)

func main() {
	c := make(chan os.Signal, 1)
	signal.Notify(c)
	go func() {
		log.Printf("received signal: %v", <-c)
		os.Exit(1)
	}()

	if _, err := CheckHealth("http://localhost:2380", nil); err != nil {
		panic(err)
	}

	fmt.Println("sleeping...")
	time.Sleep(5 * time.Second)
}

func CheckHealth(url string, tc *tls.Config) (bool, error) {
	cfg := clientv3.Config{
		Endpoints:   []string{url},
		DialTimeout: 5 * time.Second,
		TLS:         tc,
	}
	etcdcli, err := clientv3.New(cfg)
	if err != nil {
		return false, fmt.Errorf("failed to create etcd client for %s: %v", url, err)
	}
	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
	_, err = etcdcli.Get(ctx, "/", clientv3.WithSerializable())
	cancel()
	etcdcli.Close()
	if err != nil {
		return false, fmt.Errorf("etcd health probing failed for %s: %v", url, err)
	}
	return true, nil
}

I think this is from Go runtime sending SIGPIPE https://golang.org/pkg/os/signal/#hdr-SIGPIPE
when client tries to write to wrong pipe (wrong port).

Docker has separate Notify routine to ignore this signal https://github.com/moby/moby/blob/master/pkg/signal/trap.go.

Any thoughts? @xiang90 @heyitsanthony

@gyuho
Copy link
Contributor

gyuho commented May 11, 2017

@xiang90 @hongchaodeng I checked that grpc-go code does not trigger SIGPIPE. So it's the process getting the signals (c.f. golang/go#12931).

What other projects do (ignore SIGPIPEs and do not terminate):

@gyuho
Copy link
Contributor

gyuho commented Oct 6, 2017

Closing this for now, since it's more of Go issue, and happens only in 3.1 client.
Please reopen if anything is affected by this.

@gyuho gyuho closed this as completed Oct 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

3 participants