Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Maybe resolve a supplier in the DynamicSecurityHeaderCallCredentials in an executor to avoid blocking the thread #958

Open
XAMeLeOH opened this issue Sep 7, 2023 · 2 comments · May be fixed by #995
Open

Maybe resolve a supplier in the DynamicSecurityHeaderCallCredentials in an executor to avoid blocking the thread #958

XAMeLeOH opened this issue Sep 7, 2023 · 2 comments · May be fixed by #995
Assignees
@ST-DDT
Labels
enhancement A feature request or improvement

Comments

@XAMeLeOH
Copy link

XAMeLeOH commented Sep 7, 2023

CallCredentials documentation suggests using asynchronous way of setting headers in case there is a blocking function in the code (e.g. a network call).

I would change this

        @Override
        public void applyRequestMetadata(final RequestInfo requestInfo, final Executor appExecutor,
                final MetadataApplier applier) {
            applier.apply(this.extraHeadersSupplier.get());
        }

To something like in this example

        @Override
        public void applyRequestMetadata(final RequestInfo requestInfo, final Executor appExecutor,
                final MetadataApplier applier) {
            executor.execute(new Runnable() {
                @Override
                public void run() {
                    try {
                        applier.apply(this.extraHeadersSupplier.get());
                    } catch (Throwable t) {
                        applier.fail(Status.UNAUTHENTICATED.withCause(e));
                    }
                }
            });
        }
@fengli79
Copy link
Collaborator

Yes, this is the right approach. Setting the header is trivial, but the cost of getting the credential header's content could be a longer process, such as generate a token from a remote security provider, which should be offloaded to an async task without blocking the data plane. Would you like to contribute a PR?

@ST-DDT ST-DDT linked a pull request Nov 25, 2023 that will close this issue
Open
@ST-DDT ST-DDT self-assigned this Nov 25, 2023
@ST-DDT
Copy link
Collaborator

ST-DDT commented Nov 25, 2023

I created a PR to fix this.

@ST-DDT ST-DDT added the enhancement A feature request or improvement label Nov 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ST-DDT ST-DDT

Labels
enhancement A feature request or improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: run dynamic call credentials via executor grpc-ecosystem/grpc-spring
3 participants
@ST-DDT @XAMeLeOH @fengli79