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

Adds the K6_INFLUXDB_PROXY environment variable support, closes #3418 #3423

Merged
merged 1 commit into from Oct 30, 2023

Conversation

IvanovOleg
Copy link
Contributor

@IvanovOleg IvanovOleg commented Oct 26, 2023

What?

This PR closes #3418 by adding the K6_INFLUXDB_PROXY environment variable support.

Why?

This PR fixes the problem when your InfluxDB server is not opened for direct connections and requires a connection through proxy.

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (make lint) and all checks pass.
  • I have run tests locally (make tests) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

Closes #3418

@CLAassistant
Copy link

CLAassistant commented Oct 26, 2023

CLA assistant check
All committers have signed the CLA.

@IvanovOleg
Copy link
Contributor Author

I ran tests and only timeout test has failed, also fails when executed from the master branch so I doubt it has anything to do with my changes. I didn't comment anything since my change is really small and obvious.

@mstoykov mstoykov added the documentation-needed A PR which will need a separate PR for documentation label Oct 26, 2023
@mstoykov mstoykov added this to the v0.48.0 milestone Oct 26, 2023
Copy link
Collaborator

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

LGTM in general, thanks 🙇

But can you change the way it is set currently, and I would prefer if we have at least one config test. And if possible actually test the proxying?

I also saw that you didn't change it to be parsable from the cli directly (see ParseURL in the config.go).

I am not certain how good of an idea is to support this ... given that I think it will need to be escaped quite a lot. So I kind of don't think you need to do it , but let's see what @codebien thinks as well

output/influxdb/util.go Outdated Show resolved Hide resolved
@IvanovOleg IvanovOleg force-pushed the feature/influxdb-output-proxy branch 2 times, most recently from 61b24d7 to edac25a Compare October 26, 2023 21:19
@codecov-commenter
Copy link

codecov-commenter commented Oct 26, 2023

Codecov Report

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

Comparison is base (8e2c27a) 73.33% compared to head (437bc30) 73.27%.
Report is 4 commits behind head on master.

❗ Current head 437bc30 differs from pull request most recent head d8e9b0a. Consider uploading reports for the commit d8e9b0a to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3423      +/-   ##
==========================================
- Coverage   73.33%   73.27%   -0.07%     
==========================================
  Files         258      256       -2     
  Lines       19645    19646       +1     
==========================================
- Hits        14407    14395      -12     
- Misses       4353     4361       +8     
- Partials      885      890       +5     
Flag Coverage Δ
ubuntu 73.27% <87.50%> (-0.01%) ⬇️
windows ?

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

Files Coverage Δ
output/influxdb/config.go 50.96% <100.00%> (+0.96%) ⬆️
output/influxdb/util.go 78.43% <83.33%> (-0.30%) ⬇️

... and 8 files with indirect coverage changes

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

@codebien
Copy link
Collaborator

I also saw that you didn't change it to be parsable from the cli directly (see ParseURL in the config.go).

@mstoykov Yeah, I would prefer if we avoid it. An URL in another URL doesn't sound like the best UX.

Copy link
Collaborator

@codebien codebien left a comment

Choose a reason for hiding this comment

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

As @mstoykov reported, we should add a test for it.

output/influxdb/util.go Outdated Show resolved Hide resolved
@@ -17,6 +17,7 @@ import (
type Config struct {
// Connection.
Addr null.String `json:"addr" envconfig:"K6_INFLUXDB_ADDR"`
Proxy null.String `json:"proxy,omitempty" envconfig:"K6_INFLUXDB_PROXY"`
Copy link
Collaborator

@codebien codebien Oct 27, 2023

Choose a reason for hiding this comment

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

Suggested change
Proxy null.String `json:"proxy,omitempty" envconfig:"K6_INFLUXDB_PROXY"`
Proxy null.String `json:"proxy,omitempty" envconfig:"K6_INFLUXDB_HTTP_PROXY"`

@IvanovOleg @mstoykov WDYT? Should we do it? I like the consistency with the original HTTP_PROXY variable, but on the other side, I'm not sure because it could introduce confusion around the expected HTTP/S value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can change the name as you suggested, no problem. The reason I named it like that because it will work with http and https at the same time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a config test for the K6_INFLUXDB_PROXY variable.

@IvanovOleg IvanovOleg force-pushed the feature/influxdb-output-proxy branch 3 times, most recently from 1fe9a57 to 62910f6 Compare October 27, 2023 14:52
output/influxdb/util.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@codebien codebien left a comment

Choose a reason for hiding this comment

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

@mstoykov I checked the returned UX in case of error and it seems fine to me.

ERRO[0000] could not create the 'influxdb' output: failed to parse the http proxy URL: parse "http://foo\x7f.com/": net/url: invalid control character in URL

output/influxdb/config_test.go Outdated Show resolved Hide resolved
output/influxdb/config_test.go Outdated Show resolved Hide resolved
output/influxdb/config_test.go Outdated Show resolved Hide resolved
@IvanovOleg IvanovOleg force-pushed the feature/influxdb-output-proxy branch 2 times, most recently from 81c0124 to 9ac559e Compare October 30, 2023 13:33
@codebien codebien self-requested a review October 30, 2023 15:18
Copy link
Collaborator

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

Thanks for this change @IvanovOleg

@mstoykov mstoykov merged commit 61db2fe into grafana:master Oct 30, 2023
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation-needed A PR which will need a separate PR for documentation hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Separate proxy configuration for outputs
5 participants