Potential memory leak in login logic

[skwair]

Describe the bug

Hello! I'm using Bitwarden Secrets Manager as a secrets provider for External Secrets Operator, deployed via Helm with:

bitwarden-sdk-server:
  enabled: true

Looking at memory consumption of this pod, it looks like there's a memory leak:

Checking quickly the code, it might be related to the Login function that creates a Bitwarden client, but never closes it.

This function call seems to be wrapped in a middleware, executed on all requests which could explain the leak?

To Reproduce

Steps to reproduce the behavior:

1. Install ESO with Bitwarden Secrets Manager enabled
2. Let it run for a while, check memory usage of sdk-server

Expected behavior

Memory usage should be proportional to HTTP traffic, not scale indefinitely.

[Skarlso]

Hello!

Thank you for reporting!!

Hmmm the client is used in a context that should get destroyed once the request is done.

But maybe my assumption is incorrect. I will investigate this. Thank you!

[skwair]

Thanks for the quick reply!

It’s indeed destroyed on each request, but unless I’m misunderstanding the SDK internals, Bitwarden wraps some C-level resources. If Close isn’t called, those may not be freed, potentially leaving resources dangling?

[Skarlso]

Ah yes that might be it. I think it's possible to put a close method into the context or a struct with the client and a close method that is called on defer. That should do the trick.

[Skarlso]

Ahh... Actually, I am closing the client:

func (s *Server) getSecretHandler(w http.ResponseWriter, r *http.Request) {
	request := &sdk.SecretGetRequest{}
	c, err := s.getClient(r, &request)
	if err != nil {
		http.Error(w, err.Error(), http.StatusInternalServerError)

		return
	}
	defer c.Close()

	secretResponse, err := c.Secrets().Get(request.ID)
	if err != nil {
		http.Error(w, "failed to get secret: "+err.Error(), http.StatusBadRequest)

		return
	}

	s.handleResponse(secretResponse, w)
}

Every time there is a new request, once the secret is retrieved, I'm closing the client on all functions. So that's not it.

[Skarlso]

Technically, there might be a chance that it never closes on an error because the defer is after. Did you see errors?

[Skarlso]

Merged something that might fix this. :) Will release it soon.

[Skarlso]

Alright, you can try v0.5.3. That has the client closing.