Use local aws config in cli to get account and regions #7758

vishwahiremat · 2024-07-23T21:16:29Z

Description

Today for rad init --full command while configuring aws provider, we are using accesskey and secretkey provided by user to get accountid and regions, but for irsa we cannot get the account id and region information.

So instead of using accesskey and secretkey to authenticate , updated it to use the local aws config to authenticate and get account id and regions informaton.

Type of change

This pull request fixes a bug in Radius and has an approved issue (issue link required).
This pull request adds or changes features of Radius and has an approved issue (issue link required).
This pull request is a minor refactor, code cleanup, test improvement, or other maintenance task and doesn't change the functionality of Radius (issue link optional).

Fixes: #issue_number

Signed-off-by: Vishwanath Hiremath <vhiremath@microsoft.com>

codecov · 2024-07-23T21:26:37Z

Codecov Report

Attention: Patch coverage is 44.73684% with 21 lines in your changes missing coverage. Please review.

Project coverage is 61.04%. Comparing base (63490d6) to head (8c95e50).

Files	Patch %	Lines
pkg/cli/aws/client.go	0.00%	10 Missing ⚠️
pkg/cli/cmd/radinit/aws.go	56.25%	5 Missing and 2 partials ⚠️
pkg/cli/aws/client_mock.go	66.66%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7758      +/-   ##
==========================================
- Coverage   61.05%   61.04%   -0.02%     
==========================================
  Files         523      523              
  Lines       27361    27367       +6     
==========================================
  Hits        16706    16706              
- Misses       9184     9187       +3     
- Partials     1471     1474       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

nithyatsu · 2024-07-23T21:26:51Z

pkg/cli/aws/client.go

+func (c *client) GetCallerIdentity(ctx context.Context, region string) (*sts.GetCallerIdentityOutput, error) {
+	// Load the AWS SDK config and credentials
+	cfg, err := config.LoadDefaultConfig(ctx, config.WithSharedConfigProfile("default"))
+	if err != nil {


should "default" be a const?

removed the config.WithSharedConfigProfile

pkg/cli/aws/client.go

nithyatsu · 2024-07-23T21:37:10Z

pkg/cli/cmd/radinit/aws.go

-func (r *Runner) getAccountId(ctx context.Context, accessKeyID, secretAccessKey string) (string, error) {
-	callerIdentityOutput, err := r.awsClient.GetCallerIdentity(ctx, QueryRegion, accessKeyID, secretAccessKey)
+func (r *Runner) getAccountId(ctx context.Context) (string, error) {
+	callerIdentityOutput, err := r.awsClient.GetCallerIdentity(ctx, QueryRegion)
 	if err != nil {
 		return "", clierrors.MessageWithCause(err, "AWS credential verification failed.")
 	}


here, would it help user if we add some detail, like "AWS credential verification failed. Please use aws configure to configure credentials and then try again " ? cc @Reshrahim

pkg/cli/aws/client.go

kachawla · 2024-07-24T01:13:25Z

pkg/cli/aws/client.go

+	svc := ec2.NewFromConfig(cfg)
+
+	// Call DescribeRegions
+	input := &ec2.DescribeRegionsInput{}


What's the motivation for initializing empty input?

the update doesn't seem to be reflected here

pkg/cli/aws/client.go

kachawla · 2024-07-24T01:19:13Z

pkg/cli/aws/client.go

+func (c *client) GetCallerIdentity(ctx context.Context, region string) (*sts.GetCallerIdentityOutput, error) {
+	// Load the AWS SDK config and credentials
+	cfg, err := config.LoadDefaultConfig(ctx, config.WithSharedConfigProfile("default"))
+	if err != nil {


Switching this to use local configuration is more nuanced than the call to list regions. The credentials registered with the environment do not necessarily need to belong to the same account as the account the user has setup locally, and it will certainly be different for all production scenarios. So I'd suggest making this a user input. Please bounce it by @Reshrahim @willtsai to confirm if there are any concerns with updating this user experience.

Chatted offline with @vishwahiremat and @willtsai, agreed to go forward with the following approach -

User provides irsa role to register

Radius fetches aws account id for the credentials configured in users local aws config. This account could be different from the account id that the irsa role belongs to.
- Ask user if this account id should be configured with the environment, or do they want to provide another account id.

List region and selection experience stays the same, internally we will fetch it using locally configured aws credentials. It will fail if no local aws credentials are configured.

Here's a refresher on the current state:

User provides aws access key and secret key

Radius fetches corresponding account id using the provides access key and configures that on the environment

Radius lists regions and asks the user to pick a region

pkg/cli/aws/client.go

radius-functional-tests · 2024-07-24T01:35:45Z

Radius functional test overview

🔍 Go to test action run

Name	Value
Repository	vishwahiremat/radius
Commit ref	`94d35f7`
Unique ID	func314350b3cf
Image tag	pr-func314350b3cf

Click here to see the list of tools in the current test run

gotestsum 1.12.0
KinD: v0.20.0
Dapr: 1.12.0
Azure KeyVault CSI driver: 1.4.2
Azure Workload identity webhook: 1.1.0
Bicep recipe location ghcr.io/radius-project/dev/test/testrecipes/test-bicep-recipes/<name>:pr-func314350b3cf
Terraform recipe location http://tf-module-server.radius-test-tf-module-server.svc.cluster.local/<name>.zip (in cluster)
applications-rp test image location: ghcr.io/radius-project/dev/applications-rp:pr-func314350b3cf
controller test image location: ghcr.io/radius-project/dev/controller:pr-func314350b3cf
ucp test image location: ghcr.io/radius-project/dev/ucpd:pr-func314350b3cf
deployment-engine test image location: ghcr.io/radius-project/deployment-engine:latest

Test Status

⌛ Building Radius and pushing container images for functional tests...
✅ Container images build succeeded
⌛ Publishing Bicep Recipes for functional tests...
✅ Recipe publishing succeeded
⌛ Starting ucp-cloud functional tests...
⌛ Starting corerp-cloud functional tests...
⌛ Starting datastoresrp-cloud functional tests...
✅ ucp-cloud functional tests succeeded
✅ datastoresrp-cloud functional tests succeeded
✅ corerp-cloud functional tests succeeded

ytimocin · 2024-07-24T16:01:04Z

pkg/cli/aws/client.go

-	result, err := stsClient.GetCallerIdentity(ctx, &sts.GetCallerIdentityInput{})
+func (c *client) GetCallerIdentity(ctx context.Context, region string) (*sts.GetCallerIdentityOutput, error) {
+	// Load the AWS SDK config and credentials
+	cfg, err := config.LoadDefaultConfig(ctx, config.WithSharedConfigProfile("default"))


Can we ever use anything other than default?

removed the config.WithSharedConfigProfile

ytimocin · 2024-07-24T16:01:36Z

pkg/cli/aws/client.go

+		return nil, err
+	}
+
+	// Create an STS client


What is an STS Client?

Security Token Service Client and it is generally used when short-term access is needed to privileged AWS resources

pkg/cli/aws/client.go

Signed-off-by: Vishwanath Hiremath <vhiremath@microsoft.com>

kachawla · 2024-07-30T00:34:04Z

pkg/cli/cmd/radinit/aws.go

+	enterAWSAccountIDPrompt               = "Enter the account ID you want to use:"
+	enterAWSAccountIDPlaceholder          = "Enter the account ID you want to use.."


Could you please check that the format/phrasing is consistent with rest of the prompts?

Suggested change

enterAWSAccountIDPrompt = "Enter the account ID you want to use:"

enterAWSAccountIDPlaceholder = "Enter the account ID you want to use.."

enterAWSAccountIDPrompt = "Enter the account ID:"

enterAWSAccountIDPlaceholder = "Enter the account ID you want to use..."

kachawla · 2024-07-30T00:40:57Z

pkg/cli/cmd/radinit/aws.go

 	if err != nil {
 		return nil, err
 	}

-	region, err := r.selectAWSRegion(ctx, QueryRegion, accessKeyID, secretAccessKey)
+	addingCloudProvider, err := prompt.YesOrNoPrompt(fmt.Sprintf(confirmAWSAccountIDPromptFmt, accountId), prompt.ConfirmYes, r.Prompter)


this code is a bit difficult to understand, what is the intended usage of addingCloudProvider? Isn't this decided based on the inputs for confirmCloudProviderPrompt and selectCloudProviderPrompt prompts?

i missed to change the variable name here, updated it. changed it from addingCloudProvider -> addingAccountID

kachawla · 2024-07-30T00:41:32Z

pkg/cli/cmd/radinit/aws.go

-	region, err := r.selectAWSRegion(ctx, QueryRegion, accessKeyID, secretAccessKey)
+	addingCloudProvider, err := prompt.YesOrNoPrompt(fmt.Sprintf(confirmAWSAccountIDPromptFmt, accountId), prompt.ConfirmYes, r.Prompter)
+	if err != nil {
+		return nil, err


what would cause this to error out?

error could be caused when creating a new Program in GetListInput function.

radius/pkg/cli/prompt/prompt.go

Lines 74 to 77 in c62434f

model, err := tea.NewProgram(tm).Run()

if err != nil {

return "", err

}

kachawla · 2024-07-30T00:42:15Z

pkg/cli/cmd/radinit/aws.go

+	}
+
+	if !addingCloudProvider {
+		accountId, err = r.Prompter.GetTextInput("Enter the account ID you want to use: ", prompt.TextInputOptions{Placeholder: enterAWSAccountIDPlaceholder})


use the constant defined above for the prompt text

kachawla · 2024-07-30T00:44:44Z

pkg/cli/cmd/radinit/aws.go

@@ -84,8 +97,8 @@ func (r *Runner) getAccountId(ctx context.Context, accessKeyID, secretAccessKey
 	return *callerIdentityOutput.Account, nil
 }

-func (r *Runner) selectAWSRegion(ctx context.Context, region, accessKeyID, secretAccessKey string) (string, error) {
-	listRegionsOutput, err := r.awsClient.ListRegions(ctx, region, accessKeyID, secretAccessKey)
+func (r *Runner) selectAWSRegion(ctx context.Context) (string, error) {


just for easier context, can you please add a comment here that it uses local aws config to list regions?

kachawla · 2024-07-30T00:45:31Z

pkg/cli/cmd/radinit/aws.go

+	}
+
+	if !addingCloudProvider {
+		accountId, err = r.Prompter.GetTextInput("Enter the account ID you want to use: ", prompt.TextInputOptions{Placeholder: enterAWSAccountIDPlaceholder})


can all of the accountID related code be abstracted out into getAccountId?

moved accoutID related code into getAccountID

radius-functional-tests · 2024-07-30T00:48:03Z

Radius functional test overview

🔍 Go to test action run

Name	Value
Repository	vishwahiremat/radius
Commit ref	`b9ecbfe`
Unique ID	func941a852dd7
Image tag	pr-func941a852dd7

Click here to see the list of tools in the current test run

gotestsum 1.12.0
KinD: v0.20.0
Dapr: 1.12.0
Azure KeyVault CSI driver: 1.4.2
Azure Workload identity webhook: 1.1.0
Bicep recipe location ghcr.io/radius-project/dev/test/testrecipes/test-bicep-recipes/<name>:pr-func941a852dd7
Terraform recipe location http://tf-module-server.radius-test-tf-module-server.svc.cluster.local/<name>.zip (in cluster)
applications-rp test image location: ghcr.io/radius-project/dev/applications-rp:pr-func941a852dd7
controller test image location: ghcr.io/radius-project/dev/controller:pr-func941a852dd7
ucp test image location: ghcr.io/radius-project/dev/ucpd:pr-func941a852dd7
deployment-engine test image location: ghcr.io/radius-project/deployment-engine:latest

Test Status

⌛ Building Radius and pushing container images for functional tests...
✅ Container images build succeeded
⌛ Publishing Bicep Recipes for functional tests...
✅ Recipe publishing succeeded
⌛ Starting ucp-cloud functional tests...
⌛ Starting datastoresrp-cloud functional tests...
✅ ucp-cloud functional tests succeeded
✅ datastoresrp-cloud functional tests succeeded
⌛ Starting corerp-cloud functional tests...
✅ corerp-cloud functional tests succeeded

Signed-off-by: Vishwanath Hiremath <vhiremath@microsoft.com>

ytimocin · 2024-08-05T20:27:55Z

pkg/cli/aws/client.go

+		return nil, err
+	}
+
+	// Create an EC2 client


Not sure if we need these comments.

ytimocin · 2024-08-05T20:28:32Z

pkg/cli/cmd/radinit/aws.go

+	// addAccountID, err := prompt.YesOrNoPrompt(fmt.Sprintf(confirmAWSAccountIDPromptFmt, accountId), prompt.ConfirmYes, r.Prompter)
+	// if err != nil {
+	// 	return nil, err
+	// }
+
+	// if !addAccountID {
+	// 	accountId, err = r.Prompter.GetTextInput(enterAWSAccountIDPrompt, prompt.TextInputOptions{Placeholder: enterAWSAccountIDPlaceholder})
+	// 	if err != nil {
+	// 		return nil, err
+	// 	}
+	// }


Is this supposed to be uncommented?

missed to removed thess, updated in the pr

ytimocin · 2024-08-05T20:43:47Z

pkg/cli/cmd/radinit/aws.go

+		return "", err
+	}
+
+	if !addAccountID {


Shouldn't this be true so that the user can enter account id? Like the logic here is that if addAccountID is false then ask the user to enter AWS account id? Am I missing something here?

we retrieve the account id using the local config , and in the yes or no prompt ask the user if they want to use the account id from local config, if user selects no then we propmpt to provide the account id.

We can have a more meaningful name instead of addAccountID then I guess.

ytimocin · 2024-08-05T20:44:27Z

pkg/cli/cmd/radinit/aws.go

-func (r *Runner) selectAWSRegion(ctx context.Context, region, accessKeyID, secretAccessKey string) (string, error) {
-	listRegionsOutput, err := r.awsClient.ListRegions(ctx, region, accessKeyID, secretAccessKey)
+// selectAWSRegion prompts the user to select an AWS region from a list of available regions.
+// regions list is retrieved using the locally configured AWS account.


Suggested change

// regions list is retrieved using the locally configured AWS account.

// Region list is retrieved using the locally configured AWS account.

Signed-off-by: Vishwanath Hiremath <vhiremath@microsoft.com>

ytimocin · 2024-08-05T21:52:13Z

pkg/cli/cmd/radinit/aws.go

+		return "", err
+	}
+
+	if !addAccountID {


We can have a more meaningful name instead of addAccountID then I guess.

radius-functional-tests · 2024-08-05T22:03:22Z

Radius functional test overview

🔍 Go to test action run

Name	Value
Repository	vishwahiremat/radius
Commit ref	`77004be`
Unique ID	func79236b5f64
Image tag	pr-func79236b5f64

Click here to see the list of tools in the current test run

gotestsum 1.12.0
KinD: v0.20.0
Dapr: 1.12.0
Azure KeyVault CSI driver: 1.4.2
Azure Workload identity webhook: 1.1.0
Bicep recipe location ghcr.io/radius-project/dev/test/testrecipes/test-bicep-recipes/<name>:pr-func79236b5f64
Terraform recipe location http://tf-module-server.radius-test-tf-module-server.svc.cluster.local/<name>.zip (in cluster)
applications-rp test image location: ghcr.io/radius-project/dev/applications-rp:pr-func79236b5f64
controller test image location: ghcr.io/radius-project/dev/controller:pr-func79236b5f64
ucp test image location: ghcr.io/radius-project/dev/ucpd:pr-func79236b5f64
deployment-engine test image location: ghcr.io/radius-project/deployment-engine:latest

Test Status

⌛ Building Radius and pushing container images for functional tests...
✅ Container images build succeeded
⌛ Publishing Bicep Recipes for functional tests...
✅ Recipe publishing succeeded
⌛ Starting ucp-cloud functional tests...
⌛ Starting datastoresrp-cloud functional tests...
⌛ Starting corerp-cloud functional tests...
✅ datastoresrp-cloud functional tests succeeded
✅ corerp-cloud functional tests succeeded
❌ Test tool installation for ucp-cloud failed. Please check the logs for more details
❌ Failed to install Radius for ucp-cloud functional test. Please check the logs for more details
❌ ucp-cloud functional test failed. Please check the logs for more details

Signed-off-by: Vishwanath Hiremath <vhiremath@microsoft.com>

radius-functional-tests · 2024-08-05T22:39:30Z

Radius functional test overview

🔍 Go to test action run

Name	Value
Repository	vishwahiremat/radius
Commit ref	`8c95e50`
Unique ID	func1af5eef32b
Image tag	pr-func1af5eef32b

Click here to see the list of tools in the current test run

gotestsum 1.12.0
KinD: v0.20.0
Dapr: 1.12.0
Azure KeyVault CSI driver: 1.4.2
Azure Workload identity webhook: 1.1.0
Bicep recipe location ghcr.io/radius-project/dev/test/testrecipes/test-bicep-recipes/<name>:pr-func1af5eef32b
Terraform recipe location http://tf-module-server.radius-test-tf-module-server.svc.cluster.local/<name>.zip (in cluster)
applications-rp test image location: ghcr.io/radius-project/dev/applications-rp:pr-func1af5eef32b
controller test image location: ghcr.io/radius-project/dev/controller:pr-func1af5eef32b
ucp test image location: ghcr.io/radius-project/dev/ucpd:pr-func1af5eef32b
deployment-engine test image location: ghcr.io/radius-project/deployment-engine:latest

Test Status

⌛ Building Radius and pushing container images for functional tests...
✅ Container images build succeeded
⌛ Publishing Bicep Recipes for functional tests...
✅ Recipe publishing succeeded
⌛ Starting datastoresrp-cloud functional tests...
⌛ Starting corerp-cloud functional tests...
✅ datastoresrp-cloud functional tests succeeded
⌛ Starting ucp-cloud functional tests...
✅ corerp-cloud functional tests succeeded
✅ ucp-cloud functional tests succeeded

…t#7758) # Description Today for `rad init --full` command while configuring `aws` provider, we are using `accesskey` and `secretkey` provided by user to get accountid and regions, but for `irsa` we cannot get the account id and region information. So instead of using `accesskey` and `secretkey` to authenticate , updated it to use the local aws config to authenticate and get account id and regions informaton. ## Type of change  - This pull request fixes a bug in Radius and has an approved issue (issue link required). - This pull request adds or changes features of Radius and has an approved issue (issue link required). - This pull request is a minor refactor, code cleanup, test improvement, or other maintenance task and doesn't change the functionality of Radius (issue link optional).  Fixes: #issue_number --------- Signed-off-by: Vishwanath Hiremath <vhiremath@microsoft.com> Co-authored-by: Karishma Chawla <kachawla@microsoft.com>

…t#7758) # Description Today for `rad init --full` command while configuring `aws` provider, we are using `accesskey` and `secretkey` provided by user to get accountid and regions, but for `irsa` we cannot get the account id and region information. So instead of using `accesskey` and `secretkey` to authenticate , updated it to use the local aws config to authenticate and get account id and regions informaton. ## Type of change  - This pull request fixes a bug in Radius and has an approved issue (issue link required). - This pull request adds or changes features of Radius and has an approved issue (issue link required). - This pull request is a minor refactor, code cleanup, test improvement, or other maintenance task and doesn't change the functionality of Radius (issue link optional).  Fixes: #issue_number --------- Signed-off-by: Vishwanath Hiremath <vhiremath@microsoft.com> Co-authored-by: Karishma Chawla <kachawla@microsoft.com> Signed-off-by: Reshma Abdul Rahim <reshmarahim.abdul@microsoft.com>

using aws local config to get account id and regions

94d35f7

Signed-off-by: Vishwanath Hiremath <vhiremath@microsoft.com>

vishwahiremat requested review from a team as code owners July 23, 2024 21:16

nithyatsu reviewed Jul 23, 2024

View reviewed changes