Change hsConn to a map

[Ryanfsdf]

Previously, hsConn was a single value. If an application specifies 2 different handshaker service addresses, this would result in strange behavior where the first handshaker address that gets called into Dial would always be used. By using a map, we allow a handshaker connection per handshaker service address.

---

This change is 

[matthewstevenson88]

Thanks for the PR Ryan! I left a couple of comments.

Reviewable status: 0 of 2 files reviewed, 4 unresolved discussions (waiting on @davisgu, @matthewstevenson88, and @Ryanfsdf)

---

security/s2a/internal/handshaker/service/service.go, line 29 at r1 (raw file):

var (
	// hsConnMap represents a mapping from an S2A handshaker service address
	// to a corresponding S2A handshaker service connection.

nit: "corresponding connection to an S2A handshaker service instance."

---

security/s2a/internal/handshaker/service/service.go, line 31 at r1 (raw file):

	// to a corresponding S2A handshaker service connection.
	hsConnMap = make(map[string]*grpc.ClientConn)
	// mu guards hsDialer.

This precedes this PR, but doesn't mu also guard hsConnMap?

If that's correct, should we move mu to the top of the list of variables and change its comment?

---

security/s2a/internal/handshaker/service/service_test.go, line 30 at r1 (raw file):

	// The address is irrelevant in this test.
	testAddress  = "some_address"
	testAddress2 = "some_address2"

nit: Please add an underscore between address and 2.

(Also, I know this already existed in the code, but could you change some to test?)

---

security/s2a/internal/handshaker/service/service_test.go, line 80 at r1 (raw file):

		t.Fatalf("hsConnmap[%v] = %v, want %v", testAddress2, got, want)
	}
	if got, want := conn2 == conn3, false; got != want {

It might be simpler to write if got, want := conn2, conn3; got == want { ... }?

[Ryanfsdf]

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @davisgu and @matthewstevenson88)

---

security/s2a/internal/handshaker/service/service.go, line 31 at r1 (raw file):

Previously, matthewstevenson88 wrote…

This precedes this PR, but doesn't mu also guard hsConnMap?

If that's correct, should we move mu to the top of the list of variables and change its comment?

Done.

---

security/s2a/internal/handshaker/service/service_test.go, line 80 at r1 (raw file):

Previously, matthewstevenson88 wrote…

It might be simpler to write if got, want := conn2, conn3; got == want { ... }?

I don't think that would be correct, since want would be equal to conn3 and that would be implying that we want conn3. But we don't want conn3, we want conn2 to be equal to conn3.

[Ryanfsdf]

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @davisgu and @matthewstevenson88)

---

security/s2a/internal/handshaker/service/service_test.go, line 80 at r1 (raw file):

Previously, Ryanfsdf (Ryan Kim) wrote…

I don't think that would be correct, since want would be equal to conn3 and that would be implying that we want conn3. But we don't want conn3, we want conn2 to be equal to conn3.

we want conn2 not to be equal to conn3*

[matthewstevenson88]

Approved, with 2 small nits. Please get approval from @cesarghali for Go readability.

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @davisgu and @Ryanfsdf)

---

security/s2a/internal/handshaker/service/service_test.go, line 28 at r2 (raw file):

const (
	// The address is irrelevant in this test.

nit: Can we remove this comment? Now that we have a map, the address is relevant (in that they are distinct addresses).

---

security/s2a/internal/handshaker/service/service_test.go, line 44 at r2 (raw file):

	}()

	// First call to Dial, it should create a connection for the given address.

nit: s/for/to the server running at

[cesarghali]

This seems to be a bug in ALTS code as well, do you mind sending a PR to fix that?

Reviewed 1 of 2 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @davisgu and @Ryanfsdf)

---

security/s2a/internal/handshaker/service/service_test.go, line 28 at r3 (raw file):

const (
	testAddress  = "test_address"

testAddress1 = "test_address_1"

[Ryanfsdf]

Sure, will do!

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @cesarghali and @davisgu)

---

security/s2a/internal/handshaker/service/service_test.go, line 28 at r3 (raw file):

Previously, cesarghali (Cesar Ghali) wrote…

testAddress1 = "test_address_1"

Done.