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.

Sign up for GitHub

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

Jump to bottom

Plugins: Remove secure socks proxy feature toggle #66611

Merged
merged 4 commits into from
Apr 18, 2023
Merged

Plugins: Remove secure socks proxy feature toggle #66611

merged 4 commits into from
Apr 18, 2023

Conversation

stephaniehingtgen
Copy link
Contributor

What is this feature?

This PR removes the feature toggle secureSocksDSProxyEnabled, which was added in this PR. This will still not be enabled unless it is enabled in the config.ini, via secure_socks_datasource_proxy.enabled.

It also cleans up repeated UI code by moving the toggle to the DataSourceHttpSettings, if the feature is enabled on the instance. This is not available for the alertmanager, so it is false for that datasource.

Which issue(s) does this PR fix?:

Fixes https://github.com/grafana/hosted-grafana/issues/3764

Special notes for your reviewer:

@stephaniehingtgen stephaniehingtgen added this to the 10.0.0 milestone Apr 14, 2023
@stephaniehingtgen stephaniehingtgen self-assigned this Apr 14, 2023
@stephaniehingtgen stephaniehingtgen requested review from a team as code owners April 14, 2023 18:02
@stephaniehingtgen stephaniehingtgen requested a review from a team April 14, 2023 18:02
@stephaniehingtgen stephaniehingtgen requested review from asimpson, bossinc, codeincarnate, oscarkilhed, sakjur, papagian and yangkb09 and removed request for a team April 14, 2023 18:02
@stephaniehingtgen stephaniehingtgen changed the title Remove secure socks proxy feature toggle Plugins: Remove secure socks proxy feature toggle Apr 14, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Apr 14, 2023 •
edited

⚠️   Possible breaking changes

(Open the links below in a new tab to go to the correct steps)

grafana-data has possible breaking changes (more info)

Console output
Read our guideline

@grafanabot grafanabot added the levitate breaking change A label indicating a breaking change and assigned by Levitate. label Apr 14, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Apr 14, 2023 •
edited

Backend code coverage report for PR #66611

Plugin Main PR Difference
cloudwatch 82.6% 82.6% 0%

@github-actions
Copy link
Contributor

github-actions bot commented Apr 14, 2023 •
edited

Frontend code coverage report for PR #66611

Plugin Main PR Difference
azuremonitor 82.91% 82.91% 0%
cloud-monitoring 72.73% 72.73% 0%
cloudwatch 83.03% 83.03% 0%

wbrowne
wbrowne approved these changes Apr 17, 2023
View reviewed changes
Copy link
Member

@wbrowne wbrowne left a comment

Choose a reason for hiding this comment

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

LGTM

marefr
marefr approved these changes Apr 17, 2023
View reviewed changes
Copy link
Member

@marefr marefr left a comment

Choose a reason for hiding this comment

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

LGTM with some questions/comments

public/app/plugins/datasource/graphite/configuration/ConfigEditor.tsx Outdated Show resolved Hide resolved
packages/grafana-ui/src/components/DataSourceSettings/DataSourceHttpSettings.tsx
@@ -308,6 +310,7 @@ export const DataSourceHttpSettings = (props: HttpSettingsProps) => {
<CustomHeadersSettings dataSourceConfig={dataSourceConfig} onChange={onChange} />
)}
</>
{secureSocksDSProxyEnabled && <SecureSocksProxySettings options={dataSourceConfig} onOptionsChange={onChange} />}
Copy link
Member

Choose a reason for hiding this comment

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

In regards to external plugins there's possibly a problem I see with this approach, but maybe external plugins is not a concern for now? If a plugin author would provide secureSocksDSProxyEnabled=true to the DataSourceHttpSettings component it would always render even if the secure socks configuration is not enabled in Grafana config, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct ☝🏻 But I don't think this is a blocker. We should probably try to break this UI into smaller components in the future which might mitigate this problem.

aangelisc
aangelisc approved these changes Apr 17, 2023
View reviewed changes
Copy link
Contributor

@aangelisc aangelisc left a comment

Choose a reason for hiding this comment

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

LGTM

mckn
mckn approved these changes Apr 18, 2023
View reviewed changes
Copy link
Contributor

@mckn mckn left a comment

Choose a reason for hiding this comment

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

LGTM!

packages/grafana-ui/src/components/DataSourceSettings/DataSourceHttpSettings.tsx
@@ -308,6 +310,7 @@ export const DataSourceHttpSettings = (props: HttpSettingsProps) => {
<CustomHeadersSettings dataSourceConfig={dataSourceConfig} onChange={onChange} />
)}
</>
{secureSocksDSProxyEnabled && <SecureSocksProxySettings options={dataSourceConfig} onOptionsChange={onChange} />}
Copy link
Contributor

Choose a reason for hiding this comment

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

Correct ☝🏻 But I don't think this is a blocker. We should probably try to break this UI into smaller components in the future which might mitigate this problem.

public/app/plugins/datasource/graphite/configuration/ConfigEditor.tsx Outdated Show resolved Hide resolved
@stephaniehingtgen stephaniehingtgen removed the levitate breaking change A label indicating a breaking change and assigned by Levitate. label Apr 18, 2023
@grafanabot grafanabot added the levitate breaking change A label indicating a breaking change and assigned by Levitate. label Apr 18, 2023
@stephaniehingtgen stephaniehingtgen removed the levitate breaking change A label indicating a breaking change and assigned by Levitate. label Apr 18, 2023
@stephaniehingtgen stephaniehingtgen merged commit 255d8f3 into main Apr 18, 2023
21 of 22 checks passed
@stephaniehingtgen stephaniehingtgen deleted the secure-socks-remove-feature-toggle branch April 18, 2023 17:11
@stephaniehingtgen stephaniehingtgen mentioned this pull request Apr 18, 2023
Merged
@zerok zerok modified the milestones: 10.0.0, 10.0.0-preview May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mckn mckn mckn approved these changes

@wbrowne wbrowne wbrowne approved these changes

@marefr marefr marefr approved these changes

@aangelisc aangelisc aangelisc approved these changes

@torkelo torkelo Awaiting requested review from torkelo torkelo is a code owner

@chri2547 chri2547 Awaiting requested review from chri2547

@asimpson asimpson Awaiting requested review from asimpson asimpson is a code owner automatically assigned from grafana/partner-datasources

@bossinc bossinc Awaiting requested review from bossinc bossinc is a code owner automatically assigned from grafana/partner-datasources

@codeincarnate codeincarnate Awaiting requested review from codeincarnate

@oscarkilhed oscarkilhed Awaiting requested review from oscarkilhed

@sakjur sakjur Awaiting requested review from sakjur sakjur was automatically assigned from grafana/backend-platform

@papagian papagian Awaiting requested review from papagian papagian was automatically assigned from grafana/backend-platform

@yangkb09 yangkb09 Awaiting requested review from yangkb09 yangkb09 was automatically assigned from grafana/backend-platform

@iwysiu iwysiu Awaiting requested review from iwysiu iwysiu is a code owner automatically assigned from grafana/aws-datasources

@idastambuk idastambuk Awaiting requested review from idastambuk idastambuk is a code owner automatically assigned from grafana/aws-datasources

@ashharrison90 ashharrison90 Awaiting requested review from ashharrison90 ashharrison90 is a code owner automatically assigned from grafana/grafana-frontend-platform

@JoaoSilvaGrafana JoaoSilvaGrafana Awaiting requested review from JoaoSilvaGrafana JoaoSilvaGrafana is a code owner automatically assigned from grafana/grafana-frontend-platform

@grafanabot grafanabot Awaiting requested review from grafanabot grafanabot is a code owner

Assignees

@stephaniehingtgen stephaniehingtgen

Labels
add to changelog area/backend area/frontend datasource/Azure Azure Monitor Datasource datasource/CloudWatch datasource/Elasticsearch datasource/GoogleCloudMonitoring datasource/Graphite datasource/InfluxDB datasource/Loki datasource/MSSQL Microsoft SQL Server Data Source datasource/MySQL datasource/OpenTSDB datasource/Postgres datasource/Prometheus datasource/Tempo datasource/Zipkin no-backport Skip backport of PR type/docs
Projects
None yet
Milestone
10.0.0-preview
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@stephaniehingtgen @mckn @wbrowne @marefr @aangelisc @zerok @grafanabot