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

integration: use only digits in unix ports #7106

Merged
merged 1 commit into from
Jan 6, 2017
Merged

Conversation

gyuho
Copy link
Contributor

@gyuho gyuho commented Jan 5, 2017

Fix #6959.

Otherwise Travis keeps failing.

@gyuho gyuho requested a review from heyitsanthony January 5, 2017 18:54
@@ -39,7 +39,7 @@ type bridge struct {

func newBridge(addr string) (*bridge, error) {
b := &bridge{
inaddr: addr + ".bridge",
inaddr: addr + "0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this do something with the address like "bridge-" + addr? The 0 seems like it could conflict with other addresses (e.g., c+basePort = 50, pid=50 => ":100"; c+basePort=5, pid=5, bridge => ":100") . Or will that cause cert name conflicts?

@@ -510,7 +510,7 @@ func mustNewMember(t *testing.T, mcfg memberConfig) *member {
// listenGRPC starts a grpc server over a unix domain socket on the member
func (m *member) listenGRPC() error {
// prefix with localhost so cert has right domain
m.grpcAddr = "localhost:" + m.Name + ".sock"
m.grpcAddr = "localhost:" + m.Name + "0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can probably drop the "0" entirely?

@@ -420,7 +420,7 @@ func isMembersEqual(membs []client.Member, wmembs []client.Member) bool {

func newLocalListener(t *testing.T) net.Listener {
c := atomic.AddInt64(&localListenCount, 1)
addr := fmt.Sprintf("127.0.0.1:%d.%d.sock", c+basePort, os.Getpid())
addr := fmt.Sprintf("127.0.0.1:%d%d", c+basePort, os.Getpid())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can these be 0-padded? otherwise there can be collisions given the right port/pid

@heyitsanthony
Copy link
Contributor

does embed_test.go need to updated too?

@gyuho gyuho force-pushed the go1.8 branch 3 times, most recently from 739cf09 to 0473fb3 Compare January 5, 2017 20:05
@gyuho
Copy link
Contributor Author

gyuho commented Jan 5, 2017

@heyitsanthony All fixed.

"bridge-" + addr doesn't work because our test certs are only for localhost.
Just added rand.Intn.

PTAL.

Thanks.

@@ -39,7 +40,7 @@ type bridge struct {

func newBridge(addr string) (*bridge, error) {
b := &bridge{
inaddr: addr + ".bridge",
inaddr: fmt.Sprint(addr, rand.Intn(30000)),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't help with still collisions:
port=1, pid=1, rand=10 => 1 000001 10
port=10, pid=11, rand=0 => 10 000011 0

I think the 0 is better since it's not introducing more changing parts. Needs some commenting about the schema needing port numbers, though:
// bridge "port" is ("%05d%05d0", port, pid) since go1.8 expects the port to be a number

@@ -420,7 +420,7 @@ func isMembersEqual(membs []client.Member, wmembs []client.Member) bool {

func newLocalListener(t *testing.T) net.Listener {
c := atomic.AddInt64(&localListenCount, 1)
addr := fmt.Sprintf("127.0.0.1:%d.%d.sock", c+basePort, os.Getpid())
addr := fmt.Sprintf("127.0.0.1:%d%06d", c+basePort, os.Getpid())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

%05d%05d so that nested bridges won't collide and comment about 1.8 requiring numbers in ports?

@gyuho gyuho added this to the v3.2.0 milestone Jan 5, 2017
@heyitsanthony
Copy link
Contributor

lgtm

@xiang90 xiang90 merged commit 628e83e into etcd-io:master Jan 6, 2017
@gyuho gyuho deleted the go1.8 branch January 6, 2017 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants