Skip to content
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.

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

[DependencyInjection] Reset env vars when resetting the container #54666

Merged
merged 1 commit into from May 2, 2024

Conversation

faizanakram99
Copy link
Contributor

@faizanakram99 faizanakram99 commented Apr 18, 2024

Q A
Branch? 7.1
Bug fix? no
New feature? yes
Deprecations? no
Issues Fix #54627
License MIT

@carsonbot carsonbot added this to the 7.1 milestone Apr 18, 2024
@carsonbot carsonbot changed the title [DI] Reset env vars on kernel.reset [DependencyInjection] Reset env vars on kernel.reset Apr 18, 2024
@smnandre
Copy link
Contributor

So all env vars would be reset ? Without option / configuration ? Or am i reading this wrong ?

@faizanakram99
Copy link
Contributor Author

So all env vars would be reset ? Without option / configuration ? Or am i reading this wrong ?

Yes (for now), I wasn't sure how to best make it opt-in (config based). Do you think it should be opt in?

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could even consider this as a bugfix IMHO.
In non-worker mode (aka for most pp), env vars are read for every requests.

@GromNaN
Copy link
Member

GromNaN commented Apr 19, 2024

I'm all for the feature.

We could even consider this as a bugfix IMHO.

This will a have a negative impact on performances for messenger applications that uses a slow custom env-var loader or processor.

@nicolas-grekas
Copy link
Member

a slow custom env-var loader or processor.

env loaders/processors are used on web requests, so they have to be fast anyway.
and messenger is not on the hotpath, UX-wise.

@smnandre
Copy link
Contributor

So all env vars would be reset ? Without option / configuration ? Or am i reading this wrong ?

Yes (for now), I wasn't sure how to best make it opt-in (config based). Do you think it should be opt in?

No, but that would probably need some doc on messenger page afterward :)

@GromNaN
Copy link
Member

GromNaN commented Apr 19, 2024

If they process messages at high throughput, they are going to see a rapid impact on their infrastructure, with congested queues. I've been in that situation, I know it's the kind of change I would want to be notified about.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Apr 19, 2024

I'm not sure I understand your concern. As I wrote, env loaders/processors are used on web requests, so they have to be fast anyway. Fast enough for high-throughput.
Note that reading env vars from $_SERVER is already really fast so we're talking about loading them from e.g. consult or similar.
Are you talking about a real-life app where this could have impact?

@GromNaN
Copy link
Member

GromNaN commented Apr 19, 2024

I'm not sure I understand your concern. As I wrote, env loaders/processors are used on web requests, so they have to be fast anyway. Fast enough for high-throughput. Note that reading env vars from $_SERVER is already really fast so we're talking about loading them from e.g. consult or similar. Are you talking about a real-life app where this could have impact?

Yes, I'm talking about an application that fetches certain variables from Hashicorp Vault, without using caches. As these variables were not used for the web, there were no problem. I wrote an article about how I solved this issue and having resetable env var loader is a very great feature for this use-case, but I just wanted to warn that it's a change in behavior that I think is too important to be released as a bugfix.

@faizanakram99
Copy link
Contributor Author

fabbot errors are unrelated.

@faizanakram99
Copy link
Contributor Author

faizanakram99 commented Apr 22, 2024

fabbot errors are unrelated.

fixed fabbot too

reverted as per review

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense to me.
Maybe we could also add a StaticEnvVarLoader that'd be a decorator of EnvVarLoaderInterface and that'd we use to decorate SodiumVault?

@faizanakram99
Copy link
Contributor Author

This makes sense to me. Maybe we could also add a StaticEnvVarLoader that'd be a decorator of EnvVarLoaderInterface and that'd we use to decorate SodiumVault?

What would StaticEnvVarLoader do? cache the env vars of internal EnvVarLoader (SodiumVault) so that subsequent calls to loadEnvVars won't require decrypting secrets again?

@faizanakram99
Copy link
Contributor Author

faizanakram99 commented Apr 26, 2024

Index: src/Symfony/Bundle/FrameworkBundle/Secrets/StaticEnvVarLoader.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/Symfony/Bundle/FrameworkBundle/Secrets/StaticEnvVarLoader.php b/src/Symfony/Bundle/FrameworkBundle/Secrets/StaticEnvVarLoader.php
new file mode 100644
--- /dev/null	(date 1714166958969)
+++ b/src/Symfony/Bundle/FrameworkBundle/Secrets/StaticEnvVarLoader.php	(date 1714166958969)
@@ -0,0 +1,40 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Bundle\FrameworkBundle\Secrets;
+
+use Symfony\Component\DependencyInjection\Attribute\AsDecorator;
+use Symfony\Component\DependencyInjection\Attribute\Autowire;
+use Symfony\Component\DependencyInjection\Attribute\AutowireDecorated;
+use Symfony\Component\DependencyInjection\EnvVarLoaderInterface;
+
+/**
+ * @internal
+ */
+#[AsDecorator('secrets.vault')]
+class StaticEnvVarLoader implements EnvVarLoaderInterface
+{
+    private array $envCache = [];
+
+    public function __construct(
+        #[AutowireDecorated]
+        private EnvVarLoaderInterface $envVarLoader,
+    ) {
+    }
+
+    public function loadEnvVars(): array
+    {
+        if ([] !== $this->envCache) {
+            return $this->envCache;
+        }
+
+        return $this->envCache = $this->envVarLoader->loadEnvVars();
+    }
+}

Like this?

@nicolas-grekas
Copy link
Member

Yes!

@faizanakram99
Copy link
Contributor Author

This makes sense to me. Maybe we could also add a StaticEnvVarLoader that'd be a decorator of EnvVarLoaderInterface and that'd we use to decorate SodiumVault?

Done a888820

@nicolas-grekas nicolas-grekas changed the title [DependencyInjection] Reset env vars on kernel.reset [DependencyInjection] Reset env vars when resetting the container Apr 29, 2024
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍
I just moved StaticEnvVarLoader to the DI component, completed the wiring in FrameworkBundle (we don't use autowiring) and tweaked a bit the messages in changelog.

@fabpot
Copy link
Member

fabpot commented May 2, 2024

Thank you @faizanakram99.

@fabpot fabpot merged commit e6c875e into symfony:7.1 May 2, 2024
3 of 10 checks passed
@fabpot fabpot mentioned this pull request May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DI] Allow reseting of env vars loaded by EnvVarLoaderInterface implementations
7 participants