Skip to content

Add proxy support for oci/auth login#808

Merged
stefanprodan merged 1 commit intofluxcd:mainfrom
matheuscscp:oci-auth-proxy
Sep 10, 2024
Merged

Add proxy support for oci/auth login#808
stefanprodan merged 1 commit intofluxcd:mainfrom
matheuscscp:oci-auth-proxy

Conversation

@matheuscscp
Copy link
Copy Markdown
Member

Signed-off-by: Matheus Pimenta <matheuscscp@gmail.com>
Copy link
Copy Markdown
Contributor

@darkowlzz darkowlzz left a comment

Choose a reason for hiding this comment

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

LGTM!

For testing the proxy usage, we can add a proxy based test in the OCI integration tests later. I believe we need to figure out some way to test it.

Comment thread oci/auth/aws/auth.go
if c.proxyURL != nil {
transport := http.DefaultTransport.(*http.Transport).Clone()
transport.Proxy = http.ProxyURL(c.proxyURL)
confOpts = append(confOpts, config.WithHTTPClient(&http.Client{Transport: transport}))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This made me wonder if the explicitly set proxy should be respected when the aws.Config is configured externally. It looks like we can modify the given aws.Config to add proxy to it. But the same can't be done in case of Azure. It'll work for GCP easily as there's no external client in that case.
I believe the ability to provide external config was added just to make testing easier. I don't think anyone uses it for anything else.
I think for consistency across all the clients, it's better to not try to add proxy if an external client config is provided. So, it's good as it is.

@stefanprodan stefanprodan added the area/oci OCI related issues and pull requests label Sep 10, 2024
Copy link
Copy Markdown
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @matheuscscp 🏅

PS. I will do an oci release so we can move forward with image reflector.

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

Labels

area/oci OCI related issues and pull requests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants