CloudWatch: Remove dependencies on grafana/pkg/setting #81208

iwysiu · 2024-01-24T21:33:59Z

What is this feature?

Removes references to the grafana/pkg/setting package from the CloudWatch plugin. Currently, CloudWatch is passed the settings when the service is started up and stores the settings on the executor, and after this change it will get the settings from the instance context and store them on the instance. It also passes those settings to GetSession in the grafana-aws-sdk so it doesn't need to use environment variables for its session settings. Depends on grafana/grafana-aws-sdk#118

I also found that AWSListMetricsPageLimit was only in settings.Cfg and not in config.Cfg so I copied it over. In the opposite direction, I found that there was an AWS Session Duration that was only being set through an environment variable and added it to the aws config settings

Why do we need this feature?

It's required as part of the decoupling work for CloudWatch

Which issue(s) does this PR fix?:

Fixes #79324

Special notes for your reviewer:

Please check that:

It works as expected from a user's perspective.
If this is a pre-GA feature, it is behind a feature toggle.
The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

iwysiu · 2024-01-29T21:33:27Z

conf/defaults.ini

@@ -874,6 +874,10 @@ list_metrics_page_limit = 500
 # Experimental, for use in Grafana Cloud only. Please do not set.
 external_id =

+# Set the plugins that will receive AWS settings for each request (via plugin context)
+# By default this will include all Grafana Labs owned AWS plugins, or those that make use of AWS settings (ElasticSearch, Prometheus).
+forward_settings_to_plugins = cloudwatch, grafana-athena-datasource, grafana-redshift-datasource, grafana-x-ray-datasource, grafana-timestream-datasource, grafana-iot-sitewise-datasource, grafana-iot-twinmaker-app, grafana-opensearch-datasource, aws-datasource-provisioner, elasticsearch, prometheus


I added all the external AWS plugins (even though they're not using the context yet), the AWS provisioner app, and ElasticSearch and Prometheus (which support sigv4) to the list along with CloudWatch. I think that's all of them, unless something else uses sigv4?

iwysiu · 2024-01-31T20:30:12Z

pkg/plugins/config/config.go

+	AWSAllowedAuthProviders   []string
+	AWSAssumeRoleEnabled      bool
+	AWSExternalId             string
+	AWSSessionDuration        string


grafana-aws-sdk had session duration settable as a secret environment variable setting before this, and the AWSListMetricsPageLimit was settable but not propagated to Cfg.

iwysiu · 2024-01-31T20:31:46Z

pkg/services/pluginsintegration/config/config.go

@@ -35,6 +35,9 @@ func ProvideConfig(settingProvider setting.Provider, grafanaCfg *setting.Cfg, fe
 		allowedAuth,
 		aws.KeyValue("assume_role_enabled").MustBool(grafanaCfg.AWSAssumeRoleEnabled),
 		aws.KeyValue("external_id").Value(),
+		aws.KeyValue("session_duration").Value(),
+		grafanaCfg.AWSListMetricsPageLimit,


Is there a reason that the old aws variables get values off settingsProvider instead of grafanaCfg? I wasn't sure what the difference was

settingsProvider can be backed by a database instead of a file - which I believe AWS originally wanted so that config could be reloaded on the fly. When not used, the settingsProvider just falls back to the file anyway. That was the original motivation there!

Ah, I see! Should I update the new settings to be gotten off settingsProvider? It's a little weird because allowedAuth is actually being gotten from grafanaCfg

Yeah TBH I'm not sure we've done a great job here of making it clear what should be happening (and I don't think many know the actual details of settingsProvider). I just remember needing to do this for plugin settings originally - but I think any config that people would like to potentially be updated via API/backend by the DB/can change at runtime, should be fetched from the settingsProvider!

I made it so that the aws settings (including allowedAuth) are gotten from the settingsProvider and added a comment so hopefully we remember

iwysiu · 2024-01-31T20:32:35Z

pkg/setting/setting.go

@@ -1333,11 +1339,20 @@ func (cfg *Cfg) handleAWSConfig() {
 		cfg.Logger.Error(fmt.Sprintf("could not set environment variable '%s'", awsds.AllowedAuthProvidersEnvVarKeyName), err)
 	}

-	cfg.AWSExternalId = awsPluginSec.Key("external_id").Value()


I moved this to group all the cfg setting together

iwysiu · 2024-01-31T20:33:39Z

conf/defaults.ini

@@ -874,6 +874,14 @@ list_metrics_page_limit = 500
 # Experimental, for use in Grafana Cloud only. Please do not set.
 external_id =

+# Sets the expiry duration of an assumed role.
+# This setting should be expressed as a duration. Examples: 6h (hours), 10d (days), 2w (weeks), 1M (month).
+session_duration = "15m"


This was a secret/experimental setting for grafana-aws-sdk that was being fetched from the environment variables

iwysiu · 2024-01-31T22:39:12Z

pkg/tsdb/cloudwatch/models/settings_test.go

 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )

 func Test_Settings_LoadCloudWatchSettings(t *testing.T) {
+	settingCtx := backend.WithGrafanaConfig(context.Background(), backend.NewGrafanaCfg(map[string]string{


This context just exists so that the tests not testing the context settings don't produce a bunch of warning messages

andresmgot

The changes from the plugins management perspective LGTM 👍

iwysiu

After double checking, I confirmed that if it's not set to forward the setting to cloudwatch we use the environment variables. Note that forward_settings_to_plugins has to be set to something for it to overwrite the default, and it can't just be an empty string.

iwysiu · 2024-02-01T18:29:01Z

pkg/services/pluginsintegration/config/config.go

@@ -35,6 +35,9 @@ func ProvideConfig(settingProvider setting.Provider, grafanaCfg *setting.Cfg, fe
 		allowedAuth,
 		aws.KeyValue("assume_role_enabled").MustBool(grafanaCfg.AWSAssumeRoleEnabled),
 		aws.KeyValue("external_id").Value(),
+		aws.KeyValue("session_duration").Value(),
+		grafanaCfg.AWSListMetricsPageLimit,


Ah, I see! Should I update the new settings to be gotten off settingsProvider? It's a little weird because allowedAuth is actually being gotten from grafanaCfg

iwysiu · 2024-02-01T18:30:07Z

pkg/plugins/envvars/envvars.go

-	// }
+
+	if slices.Contains[[]string, string](s.cfg.AWSForwardSettingsPlugins, pluginID) {
+		if !s.cfg.AWSAssumeRoleEnabled {


This value defaults to true, so it actually should be added if its false

kevinwcyu

Nice work! Did a quick test with CloudWatch as well as Athena.

…nv var

sarahzinger

Nice work! Also I manually tested this with grafana assume role and it worked for me!

grafana-delivery-bot bot added this to the 10.4.x milestone Jan 24, 2024

iwysiu mentioned this pull request Jan 24, 2024

Add ReadSettings to get config settings from context grafana/grafana-aws-sdk#118

Merged

grafana-pr-automation bot added area/backend datasource/CloudWatch labels Jan 24, 2024

iwysiu force-pushed the iwysiu/79324 branch from 8ab508f to 3c5a484 Compare January 29, 2024 20:20

iwysiu commented Jan 29, 2024

View reviewed changes

iwysiu mentioned this pull request Jan 30, 2024

Deprecate using environment variables for auth settings in sessions grafana/grafana-aws-sdk#121

Merged

iwysiu force-pushed the iwysiu/79324 branch 3 times, most recently from 138eb14 to 1b66b90 Compare January 31, 2024 20:10

iwysiu added 7 commits January 31, 2024 15:18

CloudWatch: remove dependencies on grafana/pkg/setting

d43095f

cleanup

24ab99c

update go.mod

76ac6c7

update grafana-aws-sdk (again) and rebase

d39f9a4

fix imports

025f9ec

fix tests

4974573

actually fix tests

497e2b6

iwysiu force-pushed the iwysiu/79324 branch from 1b66b90 to 497e2b6 Compare January 31, 2024 20:19

tidy go.mod and add tests

ba04926

iwysiu commented Jan 31, 2024

View reviewed changes

iwysiu added the no-backport Skip backport of PR label Jan 31, 2024

iwysiu changed the title ~~CloudWatch: remove dependencies on grafana/pkg/setting~~ CloudWatch: Remove dependencies on grafana/pkg/setting Jan 31, 2024

iwysiu added the add to changelog label Jan 31, 2024

iwysiu marked this pull request as ready for review January 31, 2024 23:02

iwysiu requested review from torkelo and a team as code owners January 31, 2024 23:02

iwysiu requested review from wbrowne and removed request for a team January 31, 2024 23:02

iwysiu requested review from marefr, oshirohugo, sarahzinger, idastambuk, undef1nd, suntala and idafurjes and removed request for a team January 31, 2024 23:02

andresmgot reviewed Feb 1, 2024

View reviewed changes

iwysiu added 2 commits February 1, 2024 12:23

fix passing assumeRoleEnabled

16398b1

Merge branch 'main' into iwysiu/79324

a3ad8aa

iwysiu commented Feb 1, 2024

View reviewed changes

kevinwcyu approved these changes Feb 1, 2024

View reviewed changes

get config settings from SessionProvider and set secureProxyEnabled e…

8642ec4

…nv var

sarahzinger approved these changes Feb 2, 2024

View reviewed changes

iwysiu added 3 commits February 2, 2024 13:28

actually don't set the proxy env var

a46c043

fix tests for default true AWSEnableAssumeRole

230ebde

Merge branch 'main' into iwysiu/79324

bff2208

iwysiu merged commit 81da3ff into main Feb 5, 2024
12 checks passed

iwysiu deleted the iwysiu/79324 branch February 5, 2024 18:59

iwysiu mentioned this pull request Feb 9, 2024

AWS: Add session duration to aws config options #81414

Closed

aangelisc modified the milestones: 10.4.x, 10.4.0 Mar 6, 2024

BrewTestBot mentioned this pull request Mar 6, 2024

grafana 10.4.0 Homebrew/homebrew-core#165251

Closed

BrewTestBot mentioned this pull request Mar 14, 2024

grafana 10.4.0 Homebrew/homebrew-core#166070

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CloudWatch: Remove dependencies on grafana/pkg/setting #81208

CloudWatch: Remove dependencies on grafana/pkg/setting #81208

iwysiu commented Jan 24, 2024 •

edited

Loading

iwysiu Jan 29, 2024 •

edited

Loading

iwysiu Jan 31, 2024

iwysiu Jan 31, 2024

wbrowne Feb 1, 2024

iwysiu Feb 1, 2024

wbrowne Feb 1, 2024

iwysiu Feb 2, 2024

iwysiu Jan 31, 2024

iwysiu Jan 31, 2024

iwysiu Jan 31, 2024

andresmgot left a comment

iwysiu left a comment

iwysiu Feb 1, 2024

iwysiu Feb 1, 2024

kevinwcyu left a comment

sarahzinger left a comment

CloudWatch: Remove dependencies on grafana/pkg/setting #81208

CloudWatch: Remove dependencies on grafana/pkg/setting #81208

Conversation

iwysiu commented Jan 24, 2024 • edited Loading

iwysiu Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresmgot left a comment

Choose a reason for hiding this comment

iwysiu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinwcyu left a comment

Choose a reason for hiding this comment

sarahzinger left a comment

Choose a reason for hiding this comment

iwysiu commented Jan 24, 2024 •

edited

Loading

iwysiu Jan 29, 2024 •

edited

Loading