Deprecate using environment variables for auth settings in sessions #121

iwysiu · 2024-01-30T16:42:13Z

Previously, GetSessions would use the environment variables to get the grafana aws settings. This updates it to use settings passed in from the datasource and fallback to using the environment variables if they aren't set.

I also updated the changelog so that doesn't have to be a separate commit

for grafana/grafana#81208

iwysiu · 2024-01-30T16:46:30Z

pkg/awsds/sessions.go

@@ -106,8 +105,12 @@ func (sc *SessionCache) GetSession(c SessionConfig) (*session.Session, error) {
 		// DefaultRegion is deprecated, Region should be used instead
 		c.Settings.Region = c.Settings.DefaultRegion
 	}
+	if c.AuthSettings == nil {


If the datasource calling GetSession is getting the settings from the contexts, they'll pass the values through AuthSettings. Otherwise, we'll need to get them from the env variables.

Maybe this comment should be in the code?

njvrzm

Looks good! Only the tiniest of nits in comments.

njvrzm · 2024-01-31T10:55:40Z

pkg/awsds/authSettings_test.go

-	expectedDuration, err := time.ParseDuration("20m")
-	require.NoError(t, err)
+	var ctxDuration time.Duration = 600000000000  // 10 minutes in nanoseconds count
+	var envDuration time.Duration = 1200000000000 // 20 minutes in nanoseconds count


tiny nit: these could be expressed as 10 * time.Minute and 20 * time.Minute to be a bit more readable

njvrzm · 2024-01-31T13:08:19Z

pkg/awsds/sessions_test.go

-		expCreds := credentials.NewCredentials(&stscreds.AssumeRoleProvider{
-			RoleARN:  roleARN,
-			Duration: 1200000000000, //20 minutes in nanoseconds count
+		var expectedDuration time.Duration = 1200000000000 //20 minutes in nanoseconds count


same tiny nit here

sarahzinger

Is there a general plan to someday remove support for this entirely? Maybe we need to make a ticket and put it in out backlog to remove this from the env and then link that throughout the code so we know when we can remove this?

sarahzinger · 2024-01-31T13:03:04Z

pkg/awsds/authSettings.go

@@ -91,6 +91,15 @@ func ReadAuthSettingsFromContext(ctx context.Context) (*AuthSettings, bool) {
 		hasSettings = true
 	}

+	if v := cfg.Get(SessionDurationEnvVarKeyName); v != "" {


Should the const still be called SessionsDurationEnvVarKeyName?

Maybe not, but it's better practice for library maintenance to avoid renaming exported consts/functions. Technically this library is still unreleased so we could change it if we want, but the name was close enough that I thought it was fine to leave. I can rename it if we think it'd be better

sarahzinger · 2024-01-31T13:07:34Z

pkg/awsds/sessions.go

@@ -106,8 +105,12 @@ func (sc *SessionCache) GetSession(c SessionConfig) (*session.Session, error) {
 		// DefaultRegion is deprecated, Region should be used instead
 		c.Settings.Region = c.Settings.DefaultRegion
 	}
+	if c.AuthSettings == nil {


Maybe this comment should be in the code?

sarahzinger · 2024-01-31T13:13:49Z

pkg/awsds/sessions.go

 	authTypeAllowed := false
-	for _, provider := range sc.authSettings.AllowedAuthProviders {


this change of going over the config vs whats in the session cache seems like it could have unexpected changes? Is it related to this pr? Sorry if I missed it!

It's related! To stop relying on the environment variables, we need to get the GrafanaConfig off the context, and we don't have access to the context at when we create the SessionCache in the datasource. Instead, the datasource gets the config off the context when we create a new instance, and if it doesn't it gets the config off the environment variables (what currently happens)
The only unexpected thing that I can think of is if they were setting ExternalID (which is experimental and grafana only) or SessionDuration (which isn't a documented and is basically an old experimental feature) through env variables, in which case those will be ignored. Everything else gets overwritten with defaults when Grafana starts up if they weren't set in the ini file.

iwysiu

Is there a general plan to someday remove support for this entirely? Maybe we need to make a ticket and put it in out backlog to remove this from the env and then link that throughout the code so we know when we can remove this?

For multitenancy, we want plugins to stop relying on env variables. Yeah, made an issue #122

iwysiu · 2024-01-31T14:48:23Z

pkg/awsds/authSettings_test.go

-	expectedDuration, err := time.ParseDuration("20m")
-	require.NoError(t, err)
+	var ctxDuration time.Duration = 600000000000  // 10 minutes in nanoseconds count
+	var envDuration time.Duration = 1200000000000 // 20 minutes in nanoseconds count


iwysiu · 2024-01-31T14:49:25Z

pkg/awsds/authSettings.go

@@ -91,6 +91,15 @@ func ReadAuthSettingsFromContext(ctx context.Context) (*AuthSettings, bool) {
 		hasSettings = true
 	}

+	if v := cfg.Get(SessionDurationEnvVarKeyName); v != "" {


Maybe not, but it's better practice for library maintenance to avoid renaming exported consts/functions. Technically this library is still unreleased so we could change it if we want, but the name was close enough that I thought it was fine to leave. I can rename it if we think it'd be better

iwysiu · 2024-01-31T14:51:33Z

pkg/awsds/sessions.go

@@ -106,8 +105,12 @@ func (sc *SessionCache) GetSession(c SessionConfig) (*session.Session, error) {
 		// DefaultRegion is deprecated, Region should be used instead
 		c.Settings.Region = c.Settings.DefaultRegion
 	}
+	if c.AuthSettings == nil {


iwysiu · 2024-01-31T15:08:35Z

pkg/awsds/sessions_test.go

-		expCreds := credentials.NewCredentials(&stscreds.AssumeRoleProvider{
-			RoleARN:  roleARN,
-			Duration: 1200000000000, //20 minutes in nanoseconds count
+		var expectedDuration time.Duration = 1200000000000 //20 minutes in nanoseconds count


iwysiu · 2024-01-31T16:02:11Z

pkg/awsds/sessions.go

 	authTypeAllowed := false
-	for _, provider := range sc.authSettings.AllowedAuthProviders {


It's related! To stop relying on the environment variables, we need to get the GrafanaConfig off the context, and we don't have access to the context at when we create the SessionCache in the datasource. Instead, the datasource gets the config off the context when we create a new instance, and if it doesn't it gets the config off the environment variables (what currently happens)
The only unexpected thing that I can think of is if they were setting ExternalID (which is experimental and grafana only) or SessionDuration (which isn't a documented and is basically an old experimental feature) through env variables, in which case those will be ignored. Everything else gets overwritten with defaults when Grafana starts up if they weren't set in the ini file.

iwysiu · 2024-01-31T16:19:32Z

pkg/awsds/authSettings.go

-	// GrafanaListMetricsPageLimit is the string literal for the cloudwatch list metrics page limit key name
-	GrafanaListMetricsPageLimit = "AWS_CW_LIST_METRICS_PAGE_LIMIT"
+	// ListMetricsPageLimitKeyName is the string literal for the cloudwatch list metrics page limit key name
+	ListMetricsPageLimitKeyName = "AWS_CW_LIST_METRICS_PAGE_LIMIT"


Discussing the other variable names made me realize that I wanted to rename this one to be more in line with the others before anyone's using it

iwysiu added 2 commits January 30, 2024 11:31

Deprecate env variables

091a600

update changelog

66583e3

iwysiu commented Jan 30, 2024

View reviewed changes

update tests

a0b7605

iwysiu marked this pull request as ready for review January 30, 2024 20:02

iwysiu requested a review from a team as a code owner January 30, 2024 20:02

iwysiu requested review from sarahzinger and kevinwcyu January 30, 2024 20:02

njvrzm approved these changes Jan 31, 2024

View reviewed changes

sarahzinger approved these changes Jan 31, 2024

View reviewed changes

comments

697fba4

iwysiu commented Jan 31, 2024

View reviewed changes

iwysiu merged commit 2cf8fad into main Jan 31, 2024
2 checks passed

iwysiu deleted the deprecate-env branch January 31, 2024 16:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate using environment variables for auth settings in sessions #121

Deprecate using environment variables for auth settings in sessions #121

iwysiu commented Jan 30, 2024 •

edited

iwysiu Jan 30, 2024 •

edited

sarahzinger Jan 31, 2024

iwysiu Jan 31, 2024

njvrzm left a comment

njvrzm Jan 31, 2024

iwysiu Jan 31, 2024

njvrzm Jan 31, 2024

iwysiu Jan 31, 2024

sarahzinger left a comment

sarahzinger Jan 31, 2024

iwysiu Jan 31, 2024

sarahzinger Jan 31, 2024

sarahzinger Jan 31, 2024

iwysiu Jan 31, 2024

iwysiu left a comment

iwysiu Jan 31, 2024

iwysiu Jan 31, 2024

iwysiu Jan 31, 2024

iwysiu Jan 31, 2024

iwysiu Jan 31, 2024

iwysiu Jan 31, 2024

		authTypeAllowed := false
		for _, provider := range sc.authSettings.AllowedAuthProviders {

Deprecate using environment variables for auth settings in sessions #121

Deprecate using environment variables for auth settings in sessions #121

Conversation

iwysiu commented Jan 30, 2024 • edited

iwysiu Jan 30, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njvrzm left a 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

sarahzinger left a 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

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

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

iwysiu commented Jan 30, 2024 •

edited

iwysiu Jan 30, 2024 •

edited