EcsCredentialProvider - Replace getenv() with $_SERVER#2155
EcsCredentialProvider - Replace getenv() with $_SERVER#2155SamRemis merged 6 commits intoaws:masterfrom
Conversation
marek-pietrzak-tg
changed the title
|
Hi @mheki , |
Codecov Report
@@ Coverage Diff @@
## master #2155 +/- ##
============================================
- Coverage 93.08% 92.98% -0.10%
- Complexity 4003 4023 +20
============================================
Files 220 222 +2
Lines 10799 10855 +56
============================================
+ Hits 10052 10094 +42
- Misses 747 761 +14
Continue to review full report at Codecov.
|
|
You're absolutely right @SamRemis |
| $shouldUseEcsCredentialsProvider = getenv(EcsCredentialProvider::ENV_URI); | ||
| // getenv() is not thread safe - fall back to $_SERVER | ||
| if ($shouldUseEcsCredentialsProvider === false) { | ||
| $shouldUseEcsCredentialsProvider = isset($_SERVER[EcsCredentialProvider::ENV_URI]) ? $_SERVER[EcsCredentialProvider::ENV_URI] : false; |
There was a problem hiding this comment.
For following PSR-2 guidelines, please separate this into 3 lines:
$shouldUseEcsCredentialsProvider = isset($_SERVER[EcsCredentialProvider::ENV_URI])
? $_SERVER[EcsCredentialProvider::ENV_URI]
: false;
| $timeout = getenv(self::ENV_TIMEOUT); | ||
|
|
||
| if (!$timeout) { | ||
| $timeout = isset($_SERVER[self::ENV_TIMEOUT]) ? $_SERVER[self::ENV_TIMEOUT] : (isset($config['timeout']) ? $config['timeout'] : 1.0); |
| $shouldUseEcsCredentialsProvider = getenv(EcsCredentialProvider::ENV_URI); | ||
| // getenv() is not thread safe - fall back to $_SERVER | ||
| if ($shouldUseEcsCredentialsProvider === false) { | ||
| $shouldUseEcsCredentialsProvider = isset($_SERVER[EcsCredentialProvider::ENV_URI]) ? $_SERVER[EcsCredentialProvider::ENV_URI] : false; |
There was a problem hiding this comment.
Depending on who is running this and on what platform $_ENV may be worth checking too- that can be a later addition though
There was a problem hiding this comment.
In both the CGI and FastCGI SAPIs, $_SERVER is also populated by values from the environment;
https://www.php.net/manual/en/ini.core.php#ini.variables-order
I think using $_SERVER should be fine as a fallback.
Is it possible that getenv() and $_SERVER return nothing but $_ENVcontains values?
The only example I can think of is explicitly set with $_ENV['foo'] = 'bar', but that's a rear edge case.
There was a problem hiding this comment.
agreed, and thank you for getting those fixes in so quickly as well as for your contribution :) I will merge soon
|
Hi @mheki, |
|
Just to update: I found out what was going on, the $_SERVER variables needed to be cleared out before certain tests ran on the credential provider's default chain. This issue only showed up on certain operating systems. Will work on a fix ASAP |
|
Redone in #2176 |
4 participants
*Issue #2154
Description of changes:
Changes how env variable is taken for
EcsCredentialProvider(non-thread safegetenv()to$_SERVER)This fixed the issue we had on PHP script triggered by a cronjob on ECS task where
AWS_CONTAINER_CREDENTIALS_RELATIVE_URIwas available only in$_SERVERand$_ENVbut not bygetenv().This resulted with a different CredentialsProvider being used and failing with an error.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.