grpc-ecosystem / grpc-spring

Spring Boot starter module for gRPC framework.
https://grpc-ecosystem.github.io/grpc-spring/
Apache License 2.0
3.46k stars 811 forks source link

fix: run dynamic call credentials via executor #995

Open ST-DDT opened 9 months ago

ST-DDT commented 9 months ago

Fixes #958

fengli79 commented 9 months ago

Caveat: Better to offload the credential fetching when it's non-cached one. Assuming most of the request can reuse a cached the credential, this may pay a small cost for most of the rpcs.

ST-DDT commented 9 months ago

So would it be better to revert the change and just add a hint to the javadocs?

Or maybe introduce a new variant that it is explicitly called "async" that takes a (executor) -> CompletableFuture<String>? That way users can return CompletableFuture.completedFuture() if nothing async needs to be done.

private final Function<Executor, CompletableFuture<String>> extraHeadersFutureSupplier;

@Override
        public void applyRequestMetadata(final RequestInfo requestInfo, final Executor appExecutor,
                final MetadataApplier applier) {

            extraHeadersFutureSupplier.apply(executor).whenComplete((header, e) -> {
                if (e == null) {
                    final Metadata extraHeaders = new Metadata();
                    extraHeaders.put(AUTHORIZATION_HEADER, header);
                    applier.apply( extraHeaders);
                } else {
                    applier.fail(Status.UNAUTHENTICATED.withCause(e));
                }
            });
        }

On the other hand we could just let the user implement that temselves.

fengli79 commented 9 months ago

Yes, I feel the dynamic credential doesn't provide too much value to the end users. This is an important enough detail (cached/non-cached/async/blocking) which should be exposed to the implementer of the credential. Either way works, but I personally like the ideal to just let the user implement that themselves.