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

[Monorepo] Enable proxy-authorization in admin client #4189

Merged
merged 19 commits into from
Oct 11, 2023

Conversation

eapolinario
Copy link
Contributor

@eapolinario eapolinario commented Oct 9, 2023

Describe your changes

This PR imports flyteorg/flyteidl#437 into the monorepo.

Original description

Part of an effort to integrate Flyte with GCP Identity Aware Proxy, see #3965.

Allows the flyteidl admin client to sent a "proxy-authorization" header with every request to flyteadmin. Tokens for this header are created using an external command which is configured via the new proxyCommand config entry.

Same logic as was introduced in flytekit in flyteorg/flytekit#1787.

I replicated the existing logic in MaterializeCredentials and NewAuthInterceptor for the second token. The external command is called a single time per flytectl call.

Tracking Issue

Closes #3965

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Screenshots

Note to reviewers

Fabio Grätz and others added 18 commits September 22, 2023 18:33
Signed-off-by: Fabio Grätz <[email protected]>
Signed-off-by: Fabio Grätz <[email protected]>
Signed-off-by: Fabio Grätz <[email protected]>
Signed-off-by: Fabio Grätz <[email protected]>
…uthorization' into monorepo--importing-flyteidl-437

Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
@codecov
Copy link

codecov bot commented Oct 9, 2023

Codecov Report

Attention: 18 lines in your changes are missing coverage. Please review.

Comparison is base (4a780c3) 59.45% compared to head (85a819a) 59.94%.
Report is 4 commits behind head on master.

❗ Current head 85a819a differs from pull request most recent head d2013e9. Consider uploading reports for the commit d2013e9 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4189      +/-   ##
==========================================
+ Coverage   59.45%   59.94%   +0.49%     
==========================================
  Files         638      569      -69     
  Lines       54134    41049   -13085     
==========================================
- Hits        32183    24607    -7576     
+ Misses      19404    14053    -5351     
+ Partials     2547     2389     -158     
Flag Coverage Δ
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
flyteidl/clients/go/admin/config.go 50.00% <ø> (ø)
flyteidl/clients/go/admin/config_flags.go 62.00% <100.00%> (+0.77%) ⬆️
...eidl/clients/go/admin/pkce/handle_app_call_back.go 94.11% <100.00%> (+0.17%) ⬆️
...lyteplugins/go/tasks/pluginmachinery/core/phase.go 21.50% <ø> (-1.27%) ⬇️
...yteplugins/go/tasks/plugins/webapi/agent/plugin.go 71.82% <100.00%> (+5.92%) ⬆️
...dl/clients/go/admin/pkce/auth_flow_orchestrator.go 51.02% <75.00%> (-12.03%) ⬇️
flytepropeller/pkg/controller/controller.go 11.56% <0.00%> (-0.19%) ⬇️
flyteidl/clients/go/admin/client.go 84.68% <70.00%> (-2.24%) ⬇️
flyteidl/clients/go/admin/auth_interceptor.go 73.75% <71.73%> (+2.32%) ⬆️

... and 555 files with indirect coverage changes

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

@eapolinario eapolinario changed the title [Monorepo] Import flyteidl 437 [Monorepo] Enable proxy-authorization in admin client Oct 9, 2023
@eapolinario
Copy link
Contributor Author

@fg91 , can you fix the error in single-binary propeller? It repros locally, just run make compile from the root of the repo.

@fg91 fg91 force-pushed the monorepo--importing-flyteidl-437 branch from 952d2f5 to d2013e9 Compare October 11, 2023 00:02
@fg91 fg91 self-assigned this Oct 11, 2023
@fg91 fg91 added flytectl Issues related to flytectl -Flytes CLI flyteidl labels Oct 11, 2023
@eapolinario eapolinario merged commit b9f6e8c into master Oct 11, 2023
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flytectl Issues related to flytectl -Flytes CLI flyteidl
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Core feature] Make Flyte work with GCP Identity Aware Proxy (IAP)
3 participants