Skip to content

feat(backend): Add the ability to set a proxy for accessing external resources #11771

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

Merged
merged 1 commit into from
Apr 10, 2025

Conversation

hbelmiro
Copy link
Contributor

@hbelmiro hbelmiro commented Mar 24, 2025

Resolves #11744
Resolves #9702

Description of your changes:
This PR adds the ability to set a server-scoped proxy for accessing external resources.

Docs

Notes for reviewers

  • To ensure a "fail fast" approach (preferably in compile time), I:
    • Made the proxy configuration mandatory everywhere but in the entry point, which is in the apiserver main function when we create an instance of proxy.Config. The factory method handles the defaults for the unset values.
    • Created a new constructor for argocompiler.workflowCompiler that receives proxy.Config as a parameter. All existing fields are still optional.
      • Theoretically, argocompiler.workflowCompiler.proxyConfig is also optional since argocompiler.workflowCompiler is a struct. But changing that to make it mandatory at compile time would increase the number of changes in this PR too much. We can do that in a follow-up PR if we think it's necessary
    • Created a new interface for the proxy configuration with a private struct that implements it
    • Didn't leverage other existing "Options" structs
  • Tests
    • config_test.go tests all combinations possible and their default values when needed
    • End to end tests use Squid as a proxy deployed on Kind
      • The test runs a pipeline that reads a proxy variable. The pipeline succeeds if the variable was set. The pipeline should fail when no proxy is set.

Checklist:

Copy link

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@hbelmiro hbelmiro changed the title Issue 11744 WIP: Issue 11744 Mar 25, 2025
@hbelmiro hbelmiro force-pushed the issue-11744 branch 10 times, most recently from 1adedcc to 682e12a Compare March 31, 2025 15:09
@HumairAK HumairAK changed the title WIP: Issue 11744 feat(backend): Add the ability to set a proxy for accessing external resources Mar 31, 2025
@hbelmiro hbelmiro changed the title feat(backend): Add the ability to set a proxy for accessing external resources WIP: feat(backend): Add the ability to set a proxy for accessing external resources Mar 31, 2025
@hbelmiro hbelmiro force-pushed the issue-11744 branch 5 times, most recently from 98b14c8 to b18ebfb Compare March 31, 2025 19:15
@nithin8702
Copy link

This is very useful and waiting for this PR to get merged. Thanks

@hbelmiro
Copy link
Contributor Author

hbelmiro commented Apr 7, 2025

@hbelmiro could you please rebase?

@mprahl done.

Copy link
Contributor

@mprahl mprahl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few minor comments but this overall looks good!

@hbelmiro
Copy link
Contributor Author

hbelmiro commented Apr 8, 2025

/retest

1 similar comment
@hbelmiro
Copy link
Contributor Author

hbelmiro commented Apr 8, 2025

/retest

Copy link
Contributor

@mprahl mprahl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@hbelmiro
Copy link
Contributor Author

hbelmiro commented Apr 8, 2025

@HumairAK can you take another look?

@HumairAK HumairAK added this to the KFP 2.5.0 milestone Apr 9, 2025
@google-oss-prow google-oss-prow bot removed the lgtm label Apr 9, 2025
@nithin8702
Copy link

nithin8702 commented Apr 9, 2025

This is much required feature to us as we heavily involved in using corporate proxy. Thanks for this team. We would like to get this feature as early as possible.

@hbelmiro
Copy link
Contributor Author

hbelmiro commented Apr 9, 2025

/retest

Copy link
Contributor

@mprahl mprahl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve
/lgtm

@google-oss-prow google-oss-prow bot added the lgtm label Apr 10, 2025
@hbelmiro
Copy link
Contributor Author

/retest

1 similar comment
@hbelmiro
Copy link
Contributor Author

/retest

@nithin8702
Copy link

Could someone please approve this PR

@HumairAK @mprahl

@HumairAK
Copy link
Collaborator

/approve

Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: HumairAK, mprahl

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-prow google-oss-prow bot merged commit 6e3548f into kubeflow:master Apr 10, 2025
42 checks passed
@hbelmiro hbelmiro deleted the issue-11744 branch April 10, 2025 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
4 participants