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.

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

[Housekeeping] flytekit's AuthUnaryInterceptor should not have to try to refresh credentials on grpc.StatusCode.UNKNOWN #5213

Open
2 tasks done
fg91 opened this issue Apr 10, 2024 · 3 comments
Assignees
Labels
housekeeping Issues that help maintain flyte and keep it tech-debt free

Comments

@fg91
Copy link
Member

fg91 commented Apr 10, 2024

Describe the issue

Currently, flytekit's AuthUnaryInterceptor lazily tries to refresh credentials, not only when a call to flyteadmin fails with 401 or grpc status 16 (UNAUTHENTICATED) but also grpc status 2 (UNKOWN):

class AuthUnaryInterceptor(grpc.UnaryUnaryClientInterceptor, grpc.UnaryStreamClientInterceptor):

    def intercept_unary_unary(
        ...
    ):
            if e.code() == grpc.StatusCode.UNAUTHENTICATED or e.code() == grpc.StatusCode.UNKNOWN:
                self._authenticator.refresh_credentials()

The reason is that the python grpc client does not correctly map status codes if the server responds with a content-type other than application/grpc.

We reported the issue here.

What if we do not do this?

Uncleanliness. This is a housekeeping note that this can be removed in the future if the underlying issue is fixed in the grpc python client.

Related component(s)

flytekit

Are you sure this issue hasn't been raised already?

  • Yes

Have you read the Code of Conduct?

  • Yes
@fg91 fg91 added the housekeeping Issues that help maintain flyte and keep it tech-debt free label Apr 10, 2024
@fg91 fg91 self-assigned this Apr 10, 2024
@fg91 fg91 changed the title [Housekeeping] flytekit's AuthUnaryInterceptor should not try to refresh credentials on grpc.StatusCode.UNKNOWN [Housekeeping] flytekit's AuthUnaryInterceptor should not have to try to refresh credentials on grpc.StatusCode.UNKNOWN Apr 10, 2024
@fg91
Copy link
Member Author

fg91 commented Apr 10, 2024

@wild-endeavor fyi

@wild-endeavor
Copy link
Contributor

Wait @fg91 I thought you tried this again and said it still wasn't working?

@wild-endeavor
Copy link
Contributor

Had a brief discussion, thank you again @fg91 for staying on top of this. Unfortunately the underlying issue is still not resolved so there's nothing to do at this time (circa Q2 2024).

Basically the backstory to this is that between grpcio 1.53.0 and 1.61.x the grpc channel completely crashed when the server responded with content-type != application/grpc. Then, after 1.62.0 the channel doesn’t crash and you can try to refresh credentials and try again with the same channel. But even with 1.62.0 and onwards is that the content-type is not correct, the error is grpc status 2 (unknown) even if it actually was 401/status-code 16.

So we currently still need to || e.code() == grpc.StatusCode.UNKNOWN in the auth interceptor.

Will keep this ticket open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
housekeeping Issues that help maintain flyte and keep it tech-debt free
Projects
None yet
Development

No branches or pull requests

2 participants