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

Temporary Credentials: Add Grafana Assume role authentication option #49

Conversation

idastambuk
Copy link
Contributor

@idastambuk idastambuk requested a review from a team as a code owner July 6, 2023 13:57
@idastambuk idastambuk requested review from fridgepoet and removed request for a team July 6, 2023 13:57
src/ConnectionConfig.tsx Outdated Show resolved Hide resolved
src/ConnectionConfig.tsx Outdated Show resolved Hide resolved
src/ConnectionConfig.tsx Outdated Show resolved Hide resolved
Copy link
Member

@sarahzinger sarahzinger left a comment

Choose a reason for hiding this comment

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

few more minor suggestions!

src/ConnectionConfig.test.tsx Outdated Show resolved Hide resolved
AwsAuthType.Default,
AwsAuthType.Keys,
AwsAuthType.Credentials,
AwsAuthType.GrafanaAssumeRole,
Copy link
Member

Choose a reason for hiding this comment

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

imo this line is not necessary because we don't want this setting in oss/on prem only in cloud, and in cloud I think we define it in awsAllowedAuthProviders

Copy link
Contributor

Choose a reason for hiding this comment

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

on that note, do either of you think it needs to be made more obvious that this option is meant only for cloud? (since this would be the first time an AWS auth option is meant just for cloud, right?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Im ok with not putting it in the config for now, since we will probably add it in the docs under AWS authentication once it goes GA. @sarahzinger what do you think?

src/ConnectionConfig.test.tsx Show resolved Hide resolved
@idastambuk idastambuk force-pushed the 205-temporary-credentials-update-connectionconfig-in-grafana-aws-sdk-to-have-a-new-authentication-provider branch from 3bc45da to 30c516c Compare July 13, 2023 12:20
@@ -8,15 +8,19 @@ import {config} from '@grafana/runtime'
jest.mock('@grafana/runtime', () => ({
...jest.requireActual('@grafana/runtime'),
config: {
awsAllowedAuthProviders: [AwsAuthType.Credentials, AwsAuthType.Keys],
awsAllowedAuthProviders: [AwsAuthType.Credentials, AwsAuthType.Keys, AwsAuthType.Credentials],
Copy link
Contributor

Choose a reason for hiding this comment

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

Is AwsAuthType.Credentials meant to be included twice? Same question for line 21 below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, good catch!

@idastambuk idastambuk merged commit f219da7 into main Jul 14, 2023
3 checks passed
@idastambuk idastambuk deleted the 205-temporary-credentials-update-connectionconfig-in-grafana-aws-sdk-to-have-a-new-authentication-provider branch July 14, 2023 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants