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

PWX-32011: added https proxy support #1120

Merged
merged 18 commits into from
Jul 18, 2023
Merged

PWX-32011: added https proxy support #1120

merged 18 commits into from
Jul 18, 2023

Conversation

ezhang-px
Copy link
Contributor

What this PR does / why we need it:
Enable HTTPS proxy for telemetry (diags phonehome and callhome)

Which issue(s) this PR fixes (optional)
Closes #

Special notes for your reviewer:

@zoxpx
Copy link
Collaborator

zoxpx commented Jul 10, 2023

Does this code have a support for NO_PROXY environment variable?

  • note that we have a few customers using and relying on NO_PROXY functionality, which is not trivial to "calculate" as it supports subnets and DNS-host wildcards

To address proxy-support in porx -- we have introduced util.ProxyFromPxEnvironment() (@ref) -- so setting up the proxy-connection should be as simple as this:

   client := &http.Client{
      Transport: &http.Transport{
         Proxy: ProxyFromPxEnvironment,
      },
   }
  • note, this is similar to "golang native" http.ProxyFromEnvironment -- except it'll react to PX_HTTP_PROXY, NO_PROXY, etc.. so, PX-specific proxy-vars)

So.. does Envoy/Telemetry react to the environment variables (i.e. env HTTP_PROXY=xxx NO_PROXY=xxx), or do we always have to "package" the http-proxy into special config-files?

  • I think that ideally PX-Operator should react on HTTP_PROXY and PX_HTTP_PROXY env variables (generic and PX-specific versions of the proxy-vars), and
  • forward the proxy-setting environment vars to the components

@ezhang-px
Copy link
Contributor Author

Does this code have a support for NO_PROXY environment variable?

  • note that we have a few customers using and relying on NO_PROXY functionality, which is not trivial to "calculate" as it supports subnets and DNS-host wildcards

To address proxy-support in porx -- we have introduced util.ProxyFromPxEnvironment() (@ref) -- so setting up the proxy-connection should be as simple as this:

   client := &http.Client{
      Transport: &http.Transport{
         Proxy: ProxyFromPxEnvironment,
      },
   }
  • note, this is similar to "golang native" http.ProxyFromEnvironment -- except it'll react to PX_HTTP_PROXY, NO_PROXY, etc.. so, PX-specific proxy-vars)

So.. does Envoy/Telemetry react to the environment variables (i.e. env HTTP_PROXY=xxx NO_PROXY=xxx), or do we always have to "package" the http-proxy into special config-files?

  • I think that ideally PX-Operator should react on HTTP_PROXY and PX_HTTP_PROXY env variables (generic and PX-specific versions of the proxy-vars), and
  • forward the proxy-setting environment vars to the components

That's a good point @zoxpx , thanks for bring it up. Operator currently handles the case where no_proxy is required using this usePxProxy flag like (https://github.com/libopenstorage/operator/blob/master/drivers/storage/portworx/component/telemetry.go#L609) which basically tells which template of envoy config yaml to use.
If no proxy, use config like this (https://github.com/libopenstorage/operator/blob/master/deploy/ccm/envoy-config-register.yaml), otherwise use the one which requires either http or https proxy (https://github.com/libopenstorage/operator/blob/master/deploy/ccm/envoy-config-rest-custom-proxy.yaml)

@codecov
Copy link

codecov bot commented Jul 11, 2023

Codecov Report

Patch coverage: 78.33% and no project coverage change.

Comparison is base (4ebe494) 75.60% compared to head (cffc25b) 75.61%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1120   +/-   ##
=======================================
  Coverage   75.60%   75.61%           
=======================================
  Files          64       64           
  Lines       17871    17902   +31     
=======================================
+ Hits        13512    13536   +24     
- Misses       3400     3406    +6     
- Partials      959      960    +1     
Impacted Files Coverage Δ
drivers/storage/portworx/component/telemetry.go 69.33% <65.62%> (+0.03%) ⬆️
drivers/storage/portworx/util/util.go 71.42% <91.66%> (+0.35%) ⬆️
drivers/storage/portworx/component/grafana.go 77.68% <100.00%> (+0.09%) ⬆️
drivers/storage/portworx/portworx.go 84.98% <100.00%> (-0.03%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@github-actions github-actions bot added size/l and removed size/m labels Jul 12, 2023
@github-actions
Copy link

This PR is stale because it has been in review for 3 days with no activity.

@jrivera-px
Copy link
Contributor

Did we ever get the proxy that Lap setup to work with https?

@github-actions github-actions bot added size/xl and removed size/l labels Jul 17, 2023
@ezhang-px
Copy link
Contributor Author

Did we ever get the proxy that Lap setup to work with https?

No, actually used the squid https proxy set up as instructed here

Copy link
Collaborator

@zoxpx zoxpx left a comment

Choose a reason for hiding this comment

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

This is shocking -- you needed 1'500 lines of configuration files to set up proxy, where most Linux tools can do in one line (env http_proxy=xxx curl ...)

But, I suppose it is what it is.
I did log in a few questions/remarks, leaving it up to you if you want to clean it up before the commit.

// ParsePxProxy trims protocol prefix then splits the proxy address of the form "host:port" with possible basic authentication credential
func ParsePxProxyURL(proxy string) (string, string, string, error) {
if strings.HasPrefix(proxy, HttpsProtocolPrefix) && strings.Contains(proxy, "@") {
proxy := strings.TrimPrefix(proxy, HttpsProtocolPrefix)
Copy link
Collaborator

Choose a reason for hiding this comment

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

i'm surprised you had to parse the URL like this --

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is mainly for retrieving the basic auth (user, password) from the url that customer puts in their STC's env variable and convert into header for envoy to pick up

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm.. that should have worked OK -- the https://pkg.go.dev/net/url#URL does have an optional Userinfo parameter, that should be parsing the user/passwd in URL correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for bring it up. Yes, I should have be using that. Fixed in the latest commit

@ezhang-px ezhang-px merged commit 2c2b3f9 into master Jul 18, 2023
8 checks passed
ezhang-px added a commit that referenced this pull request Jul 19, 2023
ezhang-px added a commit that referenced this pull request Jul 25, 2023
* PWX-32011: added https proxy support (#1120)
@piyush-nimbalkar piyush-nimbalkar deleted the PWX-32011 branch September 15, 2023 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants