search

grpc / grpc-java

The Java gRPC implementation. HTTP/2 based RPC
https://grpc.io/docs/languages/java/
Apache License 2.0
11.48k stars 3.85k forks source link

xds: Implement GcpAuthenticationFilter #11638

Closed shivaspeaks closed 2 weeks ago

shivaspeaks commented 4 weeks ago

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.

shivaspeaks commented 4 weeks ago

@ejona86 I see there's one more mistake in this PR,

GcpAuthenticationFilter.java and LruCache.java both are in package io.grpc.xds.internal which is not what I intended to do. Probably it slipped in track-pad while refactoring.

I wanted to put GcpAuthenticationFilter.java in package io.grpc.xds where all other Filters are residing. And LruCache.java in package io.grpc.xds.internal. Makes sense? What do you think?

ejona86 commented 4 weeks ago

I suggest putting both in the same package (io.grpc.xds) and both being package-private. LruCache is small and only used by the Filter, having it close to the filter is helpful.

(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 long of maxSize "pollutes" the LruCache with random Filter logic.)

ejona86 commented 4 weeks ago

(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.)

shivaspeaks commented 4 weeks ago

we no longer need to return-an-error/NAK if the value is too large; we'd only NAK on zero. Other cases we just clamp to our maximum.

Alright, so we do check for <=0 values in our parseFilterConfig() and in LRUCache we do clamp to the minimum of our max or provided size. I think we're good with this implementation. But I want to know more about the downsides of limiting the size to our maximum.

FYI, the highlighted message in this comment is from https://github.com/grpc/proposal/pull/438#discussion_r1817274088

ejona86 commented 4 weeks ago

Well, <0 for a uint is "very large", so those should get clamped instead of fail. You can use UnsignedLongs from Guava to help do the comparisons.

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.