Skip to content

Fix: Limit grpcio version in flytekit-identity-aware-proxy due to regression#2156

Merged
pingsutw merged 2 commits into
masterfrom
fg91/fix/identity-aware-proxy-grpcio-regression
Feb 5, 2024
Merged

Fix: Limit grpcio version in flytekit-identity-aware-proxy due to regression#2156
pingsutw merged 2 commits into
masterfrom
fg91/fix/identity-aware-proxy-grpcio-regression

Conversation

@fg91

@fg91 fg91 commented Feb 2, 2024

Copy link
Copy Markdown
Member

Why are the changes needed?

This only affects Flyte users who protect flyteadmin with GCP Identity Aware Proxy.

In this PR I limit the grpcio version required by flytekit-identity-aware-proxy.

There is a regression in grpcio where a channel breaks if the server doesn't respond with the content-type application/grpc.

The same problem was fixed for the nginx ingress recently by including the accept=application/grpc header in requests to flyteadmin from flytekit (see #1841). (The grpc go client doesn't have this issue so flytectl is not affected.)

When flyteadmin is protected with GCP Identity Aware Proxy, this accept header doesn't help - unauthenticated requests still get a text/html response by the proxy.

Lazy authentication works with grpcio<1.55 even if the first 401 response is of content type text/html. I described the details in this issue in the grpcio repository. This issue is closed because the grpcio maintainers acknowledged the problem and reopened the original issue.


For now, let's restrict the grpcio version users can use when protecting flyteadmin with GCP Identity Aware Proxy.


Prospect:

If this regression wouldn't be fixed in grpcio (despite the fact that the maintainers reopened the issue), the following would allow us to circumvent this problem for GCP Identity Aware Proxy Users:

  • When upgrading a channel to proxy authenticated, not use the AuthUnaryInterceptor that is also used for flytekit's normal authentication but introduce a new interceptor for proxy authentication.

  • This new interceptor for proxy auth wouldn't do

    try rpc
    if 401:
       refresh creds
       retry rpc
    

    but

    if creds is None or creds is expired:  (via exp claim in jwt)
        refresh creds
    try rpc
    if 401:
        refresh creds
        retry-rpc
    

This is my backup plan in case the regression persists.

Signed-off-by: Fabio Grätz <fabiogratz@googlemail.com>
Signed-off-by: Fabio Grätz <fabiogratz@googlemail.com>
@codecov

codecov Bot commented Feb 2, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f721eef) 82.78% compared to head (bfd0300) 84.56%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2156      +/-   ##
==========================================
+ Coverage   82.78%   84.56%   +1.78%     
==========================================
  Files         313      313              
  Lines       23630    23630              
  Branches     3541     3541              
==========================================
+ Hits        19561    19982     +421     
+ Misses       3481     3024     -457     
- Partials      588      624      +36     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants