alts: Read max number of concurrent ALTS handshakes from environment variable. #6267

matthewstevenson88 · 2023-05-10T16:28:05Z

This mimics the existing functionality for Java (grpc/grpc-java#10016) and for C++ (grpc/grpc#32672). When the environment variable is not set, the functionality is unchanged.

Along the way, we remove 2 obsolete TODOs in handshaker.go.

RELEASE NOTES:

alts: add support for GRPC_ALTS_MAX_CONCURRENT_HANDSHAKES env var

matthewstevenson88 · 2023-05-10T16:50:02Z

@easwars Would you be able to take a look at this PR when you have a chance? :)

easwars · 2023-05-10T17:24:27Z

credentials/alts/internal/handshaker/handshaker.go

 	// handshakes.
-	maxPendingHandshakes = 100
+	defaultMaxPendingHandshakes        = 100
+	maxConcurrentHandshakesEnvVariable = "GRPC_ALTS_MAX_CONCURRENT_HANDSHAKES"


Please take a look at the following files to get an idea of how we handle env vars in grpc-go:
https://github.com/grpc/grpc-go/blob/master/internal/envconfig/envconfig.go
https://github.com/grpc/grpc-go/blob/master/internal/envconfig/xds.go

Thanks for the pointers! I've cleaned up the PR accordingly. :)

easwars · 2023-05-10T22:08:47Z

internal/envconfig/alts.go

+	// maximum number of concurrent ALTS handshakes that can be performed.
+	// Its value is read and kept in the variable
+	// ALTSMaxConcurrentHandshakes.
+	ALTSMaxConcurrentHandshakesEnv     = "GRPC_ALTS_MAX_CONCURRENT_HANDSHAKES"


Is there a reason to export this const? If not, it can be inline into the var definition on line 33.

easwars · 2023-05-10T22:09:53Z

internal/envconfig/alts.go

+	// Its value is read and kept in the variable
+	// ALTSMaxConcurrentHandshakes.
+	ALTSMaxConcurrentHandshakesEnv     = "GRPC_ALTS_MAX_CONCURRENT_HANDSHAKES"
+	altsDefaultMaxConcurrentHandshakes = uint64(100)


Same with this one. This can be inlined as well.

easwars · 2023-05-10T22:10:52Z

internal/envconfig/alts.go

+var (
+	// ALTSMaxConcurrentHandshakes is the maximum number of concurrent ALTS
+	// handshakes that can be performed.
+	ALTSMaxConcurrentHandshakes = uint64FromEnv(ALTSMaxConcurrentHandshakesEnv, altsDefaultMaxConcurrentHandshakes, uint64(1), altsDefaultMaxConcurrentHandshakes)


Are these type conversions to uint64 required? uint64FromEnv is defined as:

func uint64FromEnv(envVar string, def, min, max uint64) uint64 { ... }

Good call, fixed.

easwars · 2023-05-10T22:12:08Z

credentials/alts/internal/handshaker/handshaker.go

-	if concurrentHandshakes < 0 {
-		mu.Unlock()
-		panic("bad release")
-	}
 	mu.Unlock()
 }


Is it possible to use a weighted semaphore type provided by the sync package, which does exactly what you want in here. See: https://pkg.go.dev/golang.org/x/sync/semaphore.

Great suggestion! Done.

github-actions · 2023-05-16T22:45:49Z

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

matthewstevenson88 · 2023-05-22T00:44:26Z

Waiting on #6300 before coming back to this.

…into alts-read-envvar

matthewstevenson88 · 2023-06-01T05:12:40Z

Thanks for the great suggestions @easwars and apologies for the delay: your suggestion to use a weight semaphore was a great one, but I wanted to add another e2e test to ensure correctness and it took a little while to get it working as expected. :)

matthewstevenson88 · 2023-06-01T22:34:19Z

@dfawley It seems @easwars is OOO, would you mind if I re-assign to you to finish the review?

dfawley

Why are all these go.mod changes needed? That seems unexpected, given that all the code changes are in the main module.

dfawley · 2023-06-06T16:32:29Z

credentials/alts/alts_test.go

+	// handshake begins. If vmOnGCP is not reset and this test is run
+	// anywhere except for a GCP VM, then the ALTS handshake will
+	// immediately fail.
+	once.Do(func() {})


Should this just go into an init()? Then it doesn't need to be repeated.

Good call, done.

dfawley · 2023-06-06T16:38:57Z

internal/envconfig/alts.go

@@ -0,0 +1,25 @@
+/*


Please don't create a new file for this one new variable.. Just put it in envconfig.go.

Sure thing, done.

…into alts-read-envvar

matthewstevenson88 · 2023-06-07T21:25:24Z

Why are all these go.mod changes needed? That seems unexpected, given that all the code changes are in the main module.

Sorry about that, not sure what happened there. This should all be cleaned up now.

matthewstevenson88 · 2023-06-08T01:54:01Z

Thanks for the review!

Read max number of concurrent ALTS handshakes from environment variable.

1852f83

matthewstevenson88 marked this pull request as ready for review May 10, 2023 16:49

easwars added this to the 1.56 Release milestone May 10, 2023

easwars added the Type: Feature New features or improvements in behavior label May 10, 2023

easwars reviewed May 10, 2023

View reviewed changes

easwars assigned matthewstevenson88 May 10, 2023

easwars added the Status: Requires Reporter Clarification label May 10, 2023

matthewstevenson88 added 2 commits May 10, 2023 13:12

Refactor to use new envconfig file.

6e70e9b

Remove impossible if condition in acquire().

e3f3dcc

matthewstevenson88 assigned easwars and unassigned matthewstevenson88 May 10, 2023

easwars reviewed May 10, 2023

View reviewed changes

easwars assigned matthewstevenson88 and unassigned easwars May 10, 2023

github-actions bot added the stale label May 16, 2023

matthewstevenson88 and others added 3 commits May 17, 2023 09:52

Use weighted semaphore.

f8d609a

Add e2e test for concurrent ALTS handshakes.

6f61a5a

Merge branch 'grpc:master' into alts-read-envvar

6f2edc6

github-actions bot removed the stale label May 22, 2023

arvindbr8 added Status: Blocked and removed Status: Requires Reporter Clarification labels May 23, 2023

matthewstevenson88 added 6 commits May 26, 2023 09:53

Merge branch 'master' of https://github.com/matthewstevenson88/grpc-go …

e41e933

…into alts-read-envvar

Separate into client and server semaphores.

e53683d

Merge branch 'master' of https://github.com/matthewstevenson88/grpc-go …

93e546c

…into alts-read-envvar

Use TryAcquire instead of Acquire.

97177fa

Attempt to fix go.sum error.

ad5e053

Run go mod tidy compat=1.17.

a151bb3

matthewstevenson88 added 2 commits May 31, 2023 16:22

More glog cleanup.

9d5d1dc

Fix glog issue in gcp/observability/go.sum.

ff48d09

matthewstevenson88 removed the Status: Requires Reporter Clarification label Jun 1, 2023

matthewstevenson88 assigned easwars and unassigned matthewstevenson88 Jun 1, 2023

dfawley modified the milestones: 1.56 Release, 1.57 Release Jun 2, 2023

matthewstevenson88 assigned dfawley Jun 5, 2023

dfawley reviewed Jun 6, 2023

View reviewed changes

dfawley assigned matthewstevenson88 and unassigned easwars and dfawley Jun 6, 2023

matthewstevenson88 added 9 commits June 7, 2023 13:06

Move ALTS env var into envconfig.go.

75b0dc0

Merge branch 'master' of https://github.com/matthewstevenson88/grpc-go …

caa6ad8

…into alts-read-envvar

Fix go.mod files.

0fec934

Revert go.sum files.

0ab0b7d

Merge branch 'master' of https://github.com/matthewstevenson88/grpc-go …

f247acb

…into alts-read-envvar

Revert interop/observability/go.mod change.

d046f3a

Run go mod tidy -compat=1.17 on examples/.

ab17f8d

Run gofmt.

f75369c

Add comment describing test init function.

8c5bc34

matthewstevenson88 assigned dfawley and unassigned matthewstevenson88 Jun 7, 2023

dfawley approved these changes Jun 7, 2023

View reviewed changes

matthewstevenson88 merged commit 907bdaa into grpc:master Jun 8, 2023
1 check passed

github-actions bot locked as resolved and limited conversation to collaborators Dec 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

alts: Read max number of concurrent ALTS handshakes from environment variable. #6267

alts: Read max number of concurrent ALTS handshakes from environment variable. #6267

matthewstevenson88 commented May 10, 2023 •

edited

Loading

matthewstevenson88 commented May 10, 2023

easwars May 10, 2023

matthewstevenson88 May 10, 2023

easwars May 10, 2023

matthewstevenson88 Jun 1, 2023

easwars May 10, 2023

matthewstevenson88 Jun 1, 2023

easwars May 10, 2023

matthewstevenson88 Jun 1, 2023

easwars May 10, 2023

matthewstevenson88 Jun 1, 2023

github-actions bot commented May 16, 2023

matthewstevenson88 commented May 22, 2023

matthewstevenson88 commented Jun 1, 2023

matthewstevenson88 commented Jun 1, 2023

dfawley left a comment

dfawley Jun 6, 2023

matthewstevenson88 Jun 7, 2023

dfawley Jun 6, 2023

matthewstevenson88 Jun 7, 2023

matthewstevenson88 commented Jun 7, 2023

matthewstevenson88 commented Jun 8, 2023

alts: Read max number of concurrent ALTS handshakes from environment variable. #6267

alts: Read max number of concurrent ALTS handshakes from environment variable. #6267

Conversation

matthewstevenson88 commented May 10, 2023 • edited Loading

matthewstevenson88 commented May 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 16, 2023

matthewstevenson88 commented May 22, 2023

matthewstevenson88 commented Jun 1, 2023

matthewstevenson88 commented Jun 1, 2023

dfawley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewstevenson88 commented Jun 7, 2023

matthewstevenson88 commented Jun 8, 2023

matthewstevenson88 commented May 10, 2023 •

edited

Loading