`NewClient` performance issues?

[pgavlin]

Benchmark code:

package onepassword

import (
	"context"
	"testing"
)

const serviceAccountToken = "[secret]"

func BenchmarkNewClient(b *testing.B) {
	// Warm up the WASM runtime.
	_, err := NewClient(
		context.Background(),
		WithServiceAccountToken(serviceAccountToken),
		WithIntegrationInfo(DefaultIntegrationName, DefaultIntegrationVersion),
	)
	if err != nil {
		b.Fatal(err)
	}
	b.ResetTimer()

	for i := 0; i < b.N; i++ {
		_, err := NewClient(
			context.Background(),
			WithServiceAccountToken(serviceAccountToken),
			WithIntegrationInfo(DefaultIntegrationName, DefaultIntegrationVersion),
		)
		if err != nil {
			b.Fatal(err)
		}
	}
}

func BenchmarkResolveSecret(b *testing.B) {
	const secretRef = "op://Test Vault/Example login/password"

	// Warm up the WASM runtime.
	client, err := NewClient(
		context.Background(),
		WithServiceAccountToken(serviceAccountToken),
		WithIntegrationInfo(DefaultIntegrationName, DefaultIntegrationVersion),
	)
	if err != nil {
		b.Fatal(err)
	}
	b.ResetTimer()

	for i := 0; i < b.N; i++ {
		_, err = client.Secrets.Resolve(secretRef)
		if err != nil {
			b.Fatal(err)
		}
	}
}

Local results for 100x iterations:

1password-go-sdk ❯ go test . -benchtime 100x -bench "Benchmark.*" -cpuprofile cpu.out
goos: darwin
goarch: arm64
pkg: github.com/1password/onepassword-sdk-go
BenchmarkNewClient-10        	    100	1328848140 ns/op
BenchmarkResolveSecret-10    	    100	351173673 ns/op
PASS
ok  	github.com/1password/onepassword-sdk-go	175.427s

Local results for 1x iteration:

1password-go-sdk ❯ HTTPS_PROXY=http://localhost:8080 go test . -benchtime 1x -bench "Benchmark.*"  
goos: darwin
goarch: arm64
pkg: github.com/1password/onepassword-sdk-go
BenchmarkNewClient-10        	      1	1329156334 ns/op
BenchmarkResolveSecret-10    	      1	403518250 ns/op
PASS
ok  	github.com/1password/onepassword-sdk-go	4.931s

Tracing the HTTP traffic for the last set of benchmark results shows the following calls on the NewClient path:

  13:44:31 HTTPS POST                my.1password.com /api/v3/auth/start?                                                                                                        200   application/json  204b 111ms 
  13:44:32 HTTPS POST                my.1password.com /api/v1/auth?                                                                                                              200   application/json  665b  92ms 
  13:44:32 HTTPS POST                my.1password.com /api/v2/auth/verify?                                                                                                       200   application/json  899b 104ms 
  13:44:32 HTTPS GET                 my.1password.com /api/v2/account/keysets?                                                                                                   200   application/json  4.1k  89ms 

It would be nice to have a way to cache the auth info if possible so that it can be shared between NewClient invocations.

It's also a bit concerning that the HTTP traffic adds up to < 400ms while NewClient takes 1.3s to execute. Unfortunately, the CPU profile is opaque due to the compiled WASM, so it is unclear what is happening during the other 900ms. Note that the benchmarks warm the WASM runtime by calling NewClient once prior to the actual benchmark.

[pgavlin]

For context, our standard use case in ESC involves a request like this:

values:
  op:
    login:
      serviceAccountToken: ...
  alpha:
    fn::open::1password-secrets:
      login: ${op.login}
      get:
        secret-one: op://...
        ...
        secret-n: op://...
  ...
  omega:
    fn::open::1password-secrets:
      login: ${op.login}
      get:
        secret-one: op://...
        ...
        secret-n: op://...

For each fn::open::1password-secrets, we will create a new client using login.serviceAccountToken and then fetch each secret in get. It would be nice to be able to pass something like a cached session rather than the service account token to avoid the overhead of client creation for each provider. For example:

values:
  op:
    login:
      fn::open::1password-login:
        serviceAccountToken: ...
  alpha:
    fn::open::1password-secrets:
      login: ${op.login}
      get:
        secret-one: op://...
        ...
        secret-n: op://...
  ...
  omega:
    fn::open::1password-secrets:
      login: ${op.login}
      get:
        secret-one: op://...
        ...
        secret-n: op://...

In this case, fn::open::1password-login would return some textual representation of the cached credentials that could then be reused by the two calls to fn::open::1password-secrets.

[Marton6]

@pgavlin Thanks for raising this issue and for taking the time to benchmark the SDK!

It's also a bit concerning that the HTTP traffic adds up to < 400ms while NewClient takes 1.3s to execute. Unfortunately, the CPU profile is opaque due to the compiled WASM, so it is unclear what is happening during the other 900ms.

I understand how the longer execution time of the NewClient function can raise questions for you. I suspect that most of the time spent in the NewClient function is spent dealing with cryptographic computations that happen during the authentication process. We will consider investigating and optimizing this further if possible.

Interestingly I also ran a couple of benchmarks on my local device and in my case, most of the time (>75%) was spent making HTTP requests. But of course this depends on the device the SDK is run on and on its network connection. Seeing your results definitely makes me curious to investigate how the speed of the SDK could be improved!

It would be nice to have a way to cache the auth info if possible so that it can be shared between NewClient invocations.

This can be done by the caller of the SDK, by holding onto an instance of onepassword.Client after authentication instead of constructing new ones with the same service account token. Is this something that you could implement for your use-case?

Caching the auth info "inside" of the SDK is an interesting idea, but it does raise some challenges around security and keeping sensitive information in memory for longer than necessary (or longer than the user of the SDK expected). Therefore this will require further consideration on our end and will likely not be added to the SDK unless it is necessary for specific use-cases. So let me know if it is a requirement for you or if you can achieve the same result by caching instances of onepassword.Client!

[pgavlin]

This can be done by the caller of the SDK, by holding onto an instance of onepassword.Client after authentication instead of constructing new ones with the same service account token. Is this something that you could implement for your use-case?

we can, but it's a bit inconvenient. we'll need to keep a cache from service account token to client and be very, very certain that the cache is not accessible to multiple tenants.

Caching the auth info "inside" of the SDK is an interesting idea, but it does raise some challenges around security and keeping sensitive information in memory for longer than necessary (or longer than the user of the SDK expected). Therefore this will require further consideration on our end and will likely not be added to the SDK unless it is necessary for specific use-cases. So let me know if it is a requirement for you or if you can achieve the same result by caching instances of onepassword.Client!

I was thinking of something like this:

// create a JSON-serializable credential bundle. perform initial auth, etc. here
creds, _ := op.Login()

// create a client from the credential bundle
client := op.NewClient(creds)

so the caching isn't inside the SDK--it's under the control of the user.

doesn't the OP CLI perform some sort of credential caching?

[Marton6]

we can, but it's a bit inconvenient. we'll need to keep a cache from service account token to client and be very, very certain that the cache is not accessible to multiple tenants.

I was thinking of something like this:

// create a JSON-serializable credential bundle. perform initial auth, etc. here
creds, _ := op.Login()

// create a client from the credential bundle
client := op.NewClient(creds)

I'm trying to understand you use-case a bit better. Could you clarify why caching the credential bundle would be easier than caching the SDK's client instance? I believe you would still need to implement the same kind of access control (to "be very, very certain that the cache is not accessible to multiple tenants").

Is it just because the credential bundle from your example would be serializable?