-
Notifications
You must be signed in to change notification settings - Fork 36
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
GCP auth failure logging and testing #942
Conversation
AOP around advice is applied to managed instances of GoogleCredentials to warn when there are failures that will be silently handled by the GCP libraries as an infinite series of retry requests. A thorough set of tests are added to validate the algorithm in GoogleCredentialsFactory for locating GCP credentials and ensure that the new AOP interceptor works with the different subclass variations of GoogleCredentials.
@@ -0,0 +1,3 @@ | |||
plugins { | |||
id 'java-test-fixtures' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure it is worth creating a new file to apply a single plugin, we could just apply the plugin in gcp-common/build.gradle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ambivalent about that. This is a result of following the way it is done in micronaut-test-resources
as I was initially having some odd compilation issues when adding the plugin and following what was done there got me past that. That turned out to be more an issue with how dependencies are managed for test-fixtures so I'm fine with getting rid of this file and moving the plugin configuration.
@@ -83,11 +83,11 @@ public GoogleCredentialsFactory(@NonNull GoogleCredentialsConfiguration configur | |||
* @return The {@link GoogleCredentials} | |||
* @throws IOException An exception if an error occurs | |||
*/ | |||
@Requires(missingBeans = GoogleCredentials.class) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why removing this requirement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's somewhat redundant and odd that it was ever added here in the first place, and it was preventing the AOP interceptor from working (which may be a separate issue worth investigating). The documented and intended way to replace the framework-supplied GoogleCredentials
being created by this factory is to use gcp.credentials.enabled=false
.
Note that I can't think of any good reason for a user to ever supply their own version of GoogleCredentials
.
@Documented | ||
@Retention(RUNTIME) | ||
@Target({ElementType.METHOD, ElementType.TYPE}) | ||
@Around(proxyTargetMode = Around.ProxyTargetConstructorMode.ALLOW) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use?
@Around(proxyTargetMode = Around.ProxyTargetConstructorMode.ALLOW, proxyTarget=true)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just verified via test that it seems to already be behaving as though proxyTarget=true
in that the returned instance implements InterceptedProxy
. Explicitly setting proxyTarget=true
gives the same result.
import io.micronaut.context.exceptions.NoSuchBeanException | ||
import io.micronaut.core.reflect.ReflectionUtils |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have to be careful. we are coping GCP tests to an internal class. We can leave it, but in general coupling modules to an internal class is bad because it maybe changed without warning and break the module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood. This was absolutely used as a last resort. It is a workaround for the fact that GoogleCredentials
is internally caching the credentials loaded from a given .json file in a static instance of DefaultCredentialsProvider
the first time the .json is loaded. Using this hack in cleanup()
is unfortunately the only good way to make these test methods free of side-effects and repeatable.
This tight coupling to the internal details of GoogleCredentials
already exists here and is one of the things that motivated me to add these tests in the first place. Better to have GoogleCredentials
change and break these tests than to have that change in behavior make the module stop working correctly in a deployed app. :)
...tFixtures/java/io/micronaut/gcp/credentials/fixture/ServiceAccountCredentialsTestHelper.java
Outdated
Show resolved
Hide resolved
gcp-common/src/main/java/io/micronaut/gcp/credentials/GoogleCredentialsFactory.java
Show resolved
Hide resolved
gcp-common/src/main/java/io/micronaut/gcp/credentials/GoogleCredentialsFactory.java
Outdated
Show resolved
Hide resolved
gcp-common/src/main/java/io/micronaut/gcp/credentials/AuthenticationLoggingInterceptor.java
Show resolved
Hide resolved
gcp-common/src/main/java/io/micronaut/gcp/credentials/AuthenticationLoggingInterceptor.java
Show resolved
Hide resolved
Co-authored-by: Tim Yates <tim.yates@gmail.com>
Co-authored-by: Tim Yates <tim.yates@gmail.com>
@Requires(property = GoogleCredentialsConfiguration.PREFIX + ".enabled", value = StringUtils.TRUE, defaultValue = StringUtils.TRUE) | ||
@Primary | ||
@Singleton | ||
@LogAuthenticationFailures | ||
protected GoogleCredentials defaultGoogleCredentials() throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need to expose GoogleCredentials? Can at be some interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, unfortunately it needs to be GoogleCredentials (and what actually gets instantiated and returned here is a subclass of GoogleCredentials, depending on which method of authentication is being used - developer credentials, impersonated service account credentials, and proper service account credentials being the variations). This ultimately gets passed to the various higher-level GCP library's service abstractions and is used to facilitate their GRPC-based communication.
The error that we are interested in detecting and reporting to the users of our module happens deep within the GRPC service flow and is never propagated beyond the getRequestMetadata
method that is being intercepted here.
Kudos, SonarCloud Quality Gate passed! |
AOP around advice is applied to managed instances of GoogleCredentials
to warn when there are failures that will be silently handled by the
GCP libraries as an infinite series of retry requests, such as when
subscribing to a PubSub topic.
A thorough set of tests are added to validate the algorithm in
GoogleCredentialsFactory for locating GCP credentials and ensure that
the new AOP interceptor works with the different subclass variations of
GoogleCredentials.
Fixes #687