xds: Implement GcpAuthenticationFilter #11638
Conversation
xds/src/main/java/io/grpc/xds/internal/GcpAuthenticationFilter.java
Outdated
Show resolved
Hide resolved
|
@ejona86 I see there's one more mistake in this PR,
I wanted to put |
|
I suggest putting both in the same package ( (Personally, I'd generally put LruCache as a nested class of the Filter. Yes, you can think it can be reused, but reusable code is reusable when it is reused. Until then "reusable" is a lie and can be harmful if you make the wrong thing complex to make some code generic. We can always move the code out of the class. Even in this case, the |
|
(Although I'll also mention that in cases like this where it doesn't matter, it's mostly up to the author of the code to decide these sorts of things.) |
Alright, so we do check for <=0 values in our FYI, the highlighted message in this comment is from grpc/proposal#438 (comment) |
|
Well, There's no real downside to our limit. A thousand would be a heck of a lot and a million is outrageous. A billion is well beyond the realm of legitimate. |
| static final String TYPE_URL = | ||
| "type.googleapis.com/envoy.extensions.filters.http.gcp_authn.v3.GcpAuthnFilterConfig"; | ||
|
|
||
| public GcpAuthenticationFilter() { |
There was a problem hiding this comment.
No need to have this constructor. public qualifier in a package private class is also not useful.
| cacheSize = cacheConfig.getCacheSize().getValue(); | ||
| if (cacheSize == 0) { | ||
| return ConfigOrError.fromError( | ||
| "cache_config.cache_size must be in the range (0, INT64_MAX)"); |
There was a problem hiding this comment.
Remove the part about int64_max. That's out-of-date.
| @Test | ||
| public void testParseFilterConfig_withInvalidMessageType() { | ||
| GcpAuthenticationFilter filter = new GcpAuthenticationFilter(); | ||
| Message invalidMessage = Mockito.mock(Message.class); |
There was a problem hiding this comment.
Don't mock a Message. That won't behave correctly in many cases. A well-known type would be convenient for this test, like Empty.getInstance().
| } | ||
|
|
||
| @Test | ||
| public void testBuildClientInterceptor() { |
There was a problem hiding this comment.
This test could use a better name. It seems to do more than the name suggests.
|
|
||
| assertNull(result.config); | ||
| assertNotNull(result.errorDetail); | ||
| assertTrue(result.errorDetail.contains("Invalid config type")); |
There was a problem hiding this comment.
assertTrue() produces particularly unhelpful error messages. Prefer Truth. assertThat(result.errorDetail).contains("Invalid config type") produces much better results. You also then don't need the assertNotNull() check.
| @Test | ||
| public void testParseFilterConfig_withValidConfig() { | ||
| GcpAuthnFilterConfig config = GcpAuthnFilterConfig.newBuilder() | ||
| .setCacheConfig(TokenCacheConfig.newBuilder().setCacheSize(UInt64Value.of(10))) |
There was a problem hiding this comment.
Use a value other than the default? How else do you know the config was used?
| assertNull(result.errorDetail); | ||
| assertEquals(10L, | ||
| ((GcpAuthenticationFilter.GcpAuthenticationConfig) | ||
| result.config).getCacheSize()); |
There was a problem hiding this comment.
Unwrap some? It seems this will fit on the line above just fine, and then would be more readable.
This is a step towards gRFC A83.
When enabled for a listener, GcpAuthenticationFilter fetches JWT tokens from GCE Metadata Server for the cluster audience, creates Google Cloud CallCredentials using the token and attaches them to outgoing RPCs.