Fix: Check for missing client config in secure socks proxy check #1179
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kevinwcyu
requested review from
wbrowne,
andresmgot and
oshirohugo
and removed request for
a team
wbrowne
reviewed
Jan 6, 2025
Co-authored-by: Will Browne <wbrowne@users.noreply.github.com>
wbrowne
reviewed
Jan 8, 2025
wbrowne
approved these changes
Jan 8, 2025
There was a problem hiding this comment.
Thanks a lot @kevinwcyu! Feel free to merge as I will cut a release of the SDK soon 😄
andresmgot
approved these changes
Jan 8, 2025
| // it cannot be enabled if it's not enabled on Grafana | ||
| if p.opts == nil { | ||
| // the secure proxy can only be used if it's enabled on both the datasource connection and the client (Grafana server) | ||
| if p.opts == nil || !p.opts.Enabled || p.opts.ClientCfg == nil { |
There was a problem hiding this comment.
nit: you can now change the line 91 to simply return true. Also, I'd move extra comment to the function comment (so it's more discoverable that ClientCfg is also checked)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
When I was adding PDC support for some AWS data sources, it didn't look like
p.optswas evernilwhen checking whether the secure socks proxy should be enabled. It looks like it's always set, at least for the path that our data sources goes through.This PR checks for the presence of a
clientCfgin addition to checking if the proxy options are available as theclientCfgdoesn't get set if secure socks proxy is not enabled in Grafana.I was running into an edge case where I enabled the secure socks proxy in my
custom.inias described here, then enabled the secure socks proxy for my data source, then disabled the secure socks proxy in mycustom.iniagain. Since the data source still had the secure socks proxy setting enabled, I was hitting this error becausep.optswas never nil and the data source's settings still had the secure socks proxy setting enabled.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer: