reset provider creds on every invocation #401
Conversation
| @@ -151,10 +151,14 @@ public AbstractWrapper(final CredentialsProvider providerCredentialsProvider, | |||
| // Both are required parameters when LoggingConfig (optional) is provided when | |||
| // 'RegisterType'. | |||
| if (providerCredentials != null) { | |||
There was a problem hiding this comment.
the only concern i think is, if we want to reset it to null, we will not allow it right?
I can see three scenarios and curios on the third one
- initially it was null, second request we got a non null provider credential we refresh it - it will work
- initially it was non-null provider-credential, second request we got a new non-null provider credential - it will work for this case too
- initially it was non-null provider-credenial, second request we got a null provider credential - We will still use the old one?
aygold92
changed the title
| if (providerCredentials == null) { | ||
| // reset provider credentials back to null to avoid reusing stale credentials | ||
| if (this.providerCredentialsProvider != null) { | ||
| this.providerCredentialsProvider.resetCredentials(); | ||
| } | ||
| this.providerMetricsPublisher = null; | ||
| this.providerEventsLogger = null; | ||
| this.cloudWatchLogHelper = null; | ||
| } else { |
There was a problem hiding this comment.
Why null check providerCredentials and providerCredentialsProvider before resetting the credentials? Cant we do it for every request? The next section will set it if credentials are provided anyway.
There was a problem hiding this comment.
because it does get refreshed when creds are non-null
There was a problem hiding this comment.
Actually that null check on providerCredentialsProvider seemed unnecessary to me. It's a final variable and instantiated in the constructor, but it was already there and I was just afraid to remove it and potentially cause an NPE for something I couldn't see.
There was a problem hiding this comment.
Ok. What exactly are providerCredentials and providerCredentialsProvider? Do the credentials not need to be reset if both providerCredentials and providerCredentialsProvider are null?
There was a problem hiding this comment.
providerCredentials come directly from the request sent in by CloudFormation (or Cloud Control API). So if the credentials passed by CFN are null, then we want to reset everything.
ProviderCredentialsProvider is just a wrapper class which holds a reference to a credentials object. Like I said I don't think it's possible to be null, but if it was then it wouldn't hold any credentials anyway, which is what we want in this case. And even then, we still reset the other related objects metricsPublisher, eventsLogger, and cloudWatchLogHelper
Issue #, if available:
Description of changes:
Change
This change resets provider credentials during runtime initialization even when passed in provider creds are null. Previously, when a lambda container was reused and passed in credentials were null, we would use the same credentials from the previous request.
In the extensive logging I added to verify the issue and test the fix, I also found a bug whereby every time we invoke, we add the lambda logger to the platformProxyLogger, which causes us to log any messages on that logger X number of times, where X is the number of the times the lambda container was reused. I made the 1 line change to ensure that we only log messages once.
Testing
Verified issue and tested by uploading the jar to a function on the lambda console and logging the credentials used (those log statements have been removed from this PR of course).
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.