Skip to content

Upgrading to symfony 6.4 from 6.3 causes major CPU spike (100x load) because of unnecessary secret decryption #53429

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.

Sign up for GitHub

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

Jump to bottom
Closed
719media opened this issue Jan 4, 2024 · 8 comments
Closed

Upgrading to symfony 6.4 from 6.3 causes major CPU spike (100x load) because of unnecessary secret decryption #53429

719media opened this issue Jan 4, 2024 · 8 comments
Labels
Bug Status: Needs Review

Comments

@719media
Copy link

719media commented Jan 4, 2024 •
edited
Loading

Symfony version(s) affected

6.4.x, 7.x

Description

Secrets are being decrypted even though .env.local.* exists. Same symptom as #52146, but for different reasons.

How to reproduce

Can reproduce it easily by following the same steps, only actually more easily reproducible:

  1. composer create-project symfony/skeleton new_project/
  2. ./bin/console secrets:generate-keys
  3. ./bin/console secrets:set BLACK
  4. ./bin/console secrets:decrypt-to-local
  5. var_dump or error_log inside of the file config/secrets/dev/dev.decrypt.private.php (this file should not be used because the secrets have been decrypted to local)
  6. run ./bin/console and note that the error_log or var_dump is echo to screen. This should not happen.

Possible Solution

It appears related to the introduction of APP_RUNTIME_MODE. If you explicitly define this in .env, you can avoid the secret decrypting. However, if you want the default behavior of APP_RUNTIME_MODE to be null, so that runtime_mode is dynamically calculated, this gets a little confusing on how to accomplish this. Shouldn't be required to understand this parameter just to get secrets to work properly.

Additional Context

Seems like secret decryption is rather "fragile", perhaps it should be made less brittle in that any ENV var not found will force the entire secrets list to be decrypted...
@719media 719media added the Bug label Jan 4, 2024
@719media 719media changed the title Upgrading to symfony 6.4 from 6.3 causes major CPU spike because of bad secret decryption Upgrading to symfony 6.4 from 6.3 causes major CPU spike (100x load) because of unnecessary secret decryption Jan 5, 2024
@OskarStark
Copy link
Contributor

Can you create a small reproducer app?

@719media
Copy link
Author

719media commented Jan 8, 2024 •
edited
Loading

I mean you literally can follow the steps above, but here you go in repo form:
https://github.com/719media/symfony_bug

You should run ./bin/console secrets:decrypt-to-local after cloning so you have the secrets file decrypted.

Then just run ./bin/console and see the var_dump happen from the secrets file

@719media
Copy link
Author

Any other information needed? For me, this is a 6.4 upgrade blocker, and it seems like it would be for anyone using symfony secrets? Thanks!

@Smuzzies
Copy link

Experiencing the same issue. Ram hits 64G limit, CPU at 100%, OOM Killer invokes, systems freeze.
Reverted to 6.3 until fix provided.

Screenshot from 2024-01-17 14-33-18

@719media
Copy link
Author

719media commented Jan 18, 2024 •
edited
Loading

I got as far as seeing that the introduction of APP_RUNTIME_MODE into the codebase causes loadEnvVars method to be called on Symfony\Bundle\FrameworkBundle\Secrets\SodiumVault, which in turn decrypts all the secrets (causing sever load) to happen on every request.
Even if you are NOT using secrets, seems like this would be something to avoid (calling loadEnvVars on SodiumVault) as well (not as bad because without any secrets, you won't have a horrible performance penalty, but still seems not ideal)... but idk how it all is supposed to work.

@xabbuh
Copy link
Member

xabbuh commented Jan 19, 2024

I don't know if there is much we can do here. The container will always fall back to try reading from the vault if an environment variable is not found if I read the related correctly.

If you define the APP_RUNTIME_MODE env var yourself, the issue probably vanishes, right?

@719media
Copy link
Author

719media commented Jan 21, 2024 •
edited
Loading

@xabbuh Yes, according to what I was testing, it works. Honestly, I can't even figure out what the "default" of APP_RUNTIME_MODE is SUPPOSED to be in order to even set it the same. I guess that's the problem here. If you do a regular, default install of symfony 6.4 or 7.x, and add any secret at all, you instantly suffer really bad performance. To me, that should be unacceptable by any standard. So, what solutions are there? Well, I'm too ignorant of symfony standards to know for sure, but some ideas:

  1. Have APP_RUNTIME_MODE have a default value on base install so it doesn't have this problem
  2. Have a way to disable falling back on decryption of secrets cache in APP_MODE prod... I can't think of a scenario where you would intentionally have requests decrypting secrets on every request, it would be much better to just have it logged as missing?
  3. A rewrite of the decryption process, perhaps so that it only attempts to load all secrets if the var being searched for even exists in the list? (e.g. secrets decryption would only happen if the env var was actually in the secrets list)
  4. Perhaps generation of .env.local.php could INCLUDE APP_RUNTIME_MODE so that it doesn't get searched for. I'm not sure how the "default" of APP_RUNTIME_MODE is set, but presumably it's done outside of the .env.local.php generation, so composer dump-env doesn't handle it?

Again, I'm not very familiar with this part of symfony, so I don't know the reasoning behind all the choices. I just know that, as is, it definitely seems wrong to me.

@nicolas-grekas nicolas-grekas mentioned this issue Jan 24, 2024
Merged
@nicolas-grekas
Copy link
Member

This should be fixed by #53631; can you please give it a try?

nicolas-grekas added a commit that referenced this issue Jan 29, 2024
@nicolas-grekas
bug #53631 [DependencyInjection] Fix loading all env vars from secret…
84f5bdc 
…s when only a subset is needed (nicolas-grekas)

This PR was merged into the 6.4 branch.

Discussion
----------

[DependencyInjection] Fix loading all env vars from secrets when only a subset is needed

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | Fix #53429
| License       | MIT

As spotted in the linked issue, we have a significant performance issue when loading env vars.
The issue is that when one env var is needed but not defined, we still decrypt all existing env vars.
This also means we have a scalability issue when the number of env vars increases.
Since this happens usually on every single requests, this can burn the CPU.

The solution implemented in this PR is to use lazy strings to load env vars so that we don't decrypt them unless they are actually needed.

Commits
-------

1d6f795 [DependencyInjection] Fix loading all env vars from secrets when only a subset is needed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@nicolas-grekas @OskarStark @xabbuh @719media @carsonbot @Smuzzies