From 79f36d1e56139cb3de1f18b62323ce7458efaa98 Mon Sep 17 00:00:00 2001 From: Pengyuan Zhao Date: Wed, 24 Jan 2024 11:09:36 -0500 Subject: [PATCH] chore: update user flow and validation for using existing CloudTrail (#1521) --- cli/cmd/generate_aws.go | 86 +++++++++++++++++++----------- integration/aws_generation_test.go | 76 +++++++------------------- lwgenerate/aws/aws.go | 44 ++++++++++----- lwgenerate/aws/aws_test.go | 2 +- 4 files changed, 106 insertions(+), 102 deletions(-) diff --git a/cli/cmd/generate_aws.go b/cli/cmd/generate_aws.go index 00c2851ec..9ac57d32e 100644 --- a/cli/cmd/generate_aws.go +++ b/cli/cmd/generate_aws.go @@ -65,7 +65,7 @@ var ( // CloudTrail questions QuestionEnableCloudtrail = "Enable CloudTrail integration?" - QuestionCloudtrailName = "Name of cloudtrail integration (optional):" + QuestionCloudtrailName = "Existing trail name:" QuestionCloudtrailAdvanced = "Configure advanced options?" // CloudTrail Control Tower questions @@ -97,7 +97,7 @@ var ( // CloudTrail S3 Bucket Questions QuestionCloudtrailUseConsolidated = "Use consolidated CloudTrail?" - QuestionCloudtrailUseExistingS3 = "Use an existing CloudTrail?" + QuestionCloudtrailUseExistingTrail = "Use an existing CloudTrail?" QuestionCloudtrailS3ExistingBucketArn = "Existing S3 bucket ARN used for CloudTrail logs:" QuestionCloudtrailS3BucketEnableEncryption = "Enable S3 bucket encryption" @@ -106,7 +106,7 @@ var ( QuestionCloudtrailS3BucketNotification = "Enable S3 bucket notifications" // CloudTrail SNS Topic Questions - QuestionCloudtrailUseExistingSNSTopic = "Use an existing SNS topic?" + QuestionCloudtrailUseExistingSNSTopic = "Use an existing SNS topic? (If not, S3 notification will be used)" QuestionCloudtrailSnsExistingTopicArn = "Existing SNS topic arn:" QuestionCloudtrailSnsEnableEncryption = "Enable encryption on SNS topic?" QuestionCloudtrailSnsEncryptionKeyArn = "Existing KMS encryption key arn for SNS topic (optional):" @@ -204,7 +204,7 @@ See help output for more details on the parameter value(s) required for Terrafor aws.WithControlTowerLogArchiveAccount(GenerateAwsCommandState.ControlTowerLogArchiveAccount), aws.WithControlTowerKmsKeyArn(GenerateAwsCommandState.ControlTowerKmsKeyArn), aws.WithConsolidatedCloudtrail(GenerateAwsCommandState.ConsolidatedCloudtrail), - aws.WithCloudtrailUseExistingS3(GenerateAwsCommandState.CloudtrailUseExistingS3), + aws.WithCloudtrailUseExistingTrail(GenerateAwsCommandState.CloudtrailUseExistingTrail), aws.WithCloudtrailUseExistingSNSTopic(GenerateAwsCommandState.CloudtrailUseExistingSNSTopic), aws.WithExistingCloudtrailBucketArn(GenerateAwsCommandState.ExistingCloudtrailBucketArn), aws.WithExistingSnsTopicArn(GenerateAwsCommandState.ExistingSnsTopicArn), @@ -326,7 +326,7 @@ See help output for more details on the parameter value(s) required for Terrafor return err } if arn != "" { - GenerateAwsCommandState.CloudtrailUseExistingS3 = true + GenerateAwsCommandState.CloudtrailUseExistingTrail = true } // Validate SNS Topic Arn if passed @@ -1027,6 +1027,10 @@ func promptCloudtrailQuestions( return err } + if err := promptCloudtrailExistingTrailQuestions(config); err != nil { + return err + } + // Find out if the customer wants to specify more advanced features if err := SurveyQuestionInteractiveOnly(SurveyQuestionWithValidationArgs{ Icon: IconCloudTrail, @@ -1046,6 +1050,13 @@ func promptCloudtrailQuestions( OptCloudtrailIAM, OptCloudtrailDone, } + if config.CloudtrailUseExistingTrail { + options = []string{ + OptCloudtrailSQS, + OptCloudtrailIAM, + OptCloudtrailDone, + } + } if config.AwsOrganization { if config.ControlTower { options = []string{ @@ -1288,22 +1299,31 @@ func promptCloudtrailKmsKeyQuestions(config *aws.GenerateAwsTfConfigurationArgs) return nil } -func promptCloudtrailS3Questions(config *aws.GenerateAwsTfConfigurationArgs) error { +func promptCloudtrailExistingTrailQuestions(config *aws.GenerateAwsTfConfigurationArgs) error { + if config.ControlTower { + return nil + } + if err := SurveyMultipleQuestionWithValidation([]SurveyQuestionWithValidationArgs{ { Icon: IconCloudTrail, - Prompt: &survey.Confirm{Message: QuestionCloudtrailUseExistingS3, Default: config.CloudtrailUseExistingS3}, - Response: &config.CloudtrailUseExistingS3, + Prompt: &survey.Confirm{Message: QuestionCloudtrailUseExistingTrail, Default: config.CloudtrailUseExistingTrail}, + Response: &config.CloudtrailUseExistingTrail, }, }); err != nil { return err } + if !config.CloudtrailUseExistingTrail { + return nil + } + if err := SurveyMultipleQuestionWithValidation([]SurveyQuestionWithValidationArgs{ { Icon: IconCloudTrail, Prompt: &survey.Input{Message: QuestionCloudtrailName, Default: config.CloudtrailName}, Response: &config.CloudtrailName, + Required: true, }, { Icon: IconCloudTrail, @@ -1315,10 +1335,35 @@ func promptCloudtrailS3Questions(config *aws.GenerateAwsTfConfigurationArgs) err Opts: []survey.AskOpt{survey.WithValidator(validateAwsArnFormat)}, Response: &config.ExistingCloudtrailBucketArn, }, - }, config.CloudtrailUseExistingS3); err != nil { + { + Icon: IconCloudTrail, + Prompt: &survey.Confirm{ + Message: QuestionCloudtrailUseExistingSNSTopic, + Default: config.CloudtrailUseExistingSNSTopic, + }, + Response: &config.CloudtrailUseExistingSNSTopic, + }, + { + Icon: IconCloudTrail, + Prompt: &survey.Input{Message: QuestionCloudtrailSnsExistingTopicArn, Default: config.ExistingSnsTopicArn}, + Checks: []*bool{&config.CloudtrailUseExistingSNSTopic}, + Required: true, + Opts: []survey.AskOpt{survey.WithValidator(validateAwsArnFormat)}, + Response: &config.ExistingSnsTopicArn, + }, + }); err != nil { return err } + // If no SNS topic is provided for the existing trail, fallback to use S3 notification + if !config.CloudtrailUseExistingSNSTopic { + config.S3BucketNotification = true + } + + return nil +} + +func promptCloudtrailS3Questions(config *aws.GenerateAwsTfConfigurationArgs) error { if err := SurveyMultipleQuestionWithValidation([]SurveyQuestionWithValidationArgs{ { Icon: IconCloudTrail, @@ -1345,7 +1390,7 @@ func promptCloudtrailS3Questions(config *aws.GenerateAwsTfConfigurationArgs) err Prompt: &survey.Confirm{Message: QuestionCloudtrailS3BucketNotification, Default: config.S3BucketNotification}, Response: &config.S3BucketNotification, }, - }, !config.CloudtrailUseExistingS3); err != nil { + }, !config.CloudtrailUseExistingTrail); err != nil { return err } @@ -1353,27 +1398,6 @@ func promptCloudtrailS3Questions(config *aws.GenerateAwsTfConfigurationArgs) err } func promptCloudtrailSNSQuestions(config *aws.GenerateAwsTfConfigurationArgs) error { - if err := SurveyMultipleQuestionWithValidation([]SurveyQuestionWithValidationArgs{ - { - Icon: IconCloudTrail, - Prompt: &survey.Confirm{ - Message: QuestionCloudtrailUseExistingSNSTopic, - Default: config.CloudtrailUseExistingSNSTopic, - }, - Response: &config.CloudtrailUseExistingSNSTopic, - }, - { - Icon: IconCloudTrail, - Prompt: &survey.Input{Message: QuestionCloudtrailSnsExistingTopicArn, Default: config.ExistingSnsTopicArn}, - Checks: []*bool{&config.CloudtrailUseExistingSNSTopic}, - Required: true, - Opts: []survey.AskOpt{survey.WithValidator(validateAwsArnFormat)}, - Response: &config.ExistingSnsTopicArn, - }, - }); err != nil { - return err - } - if err := SurveyMultipleQuestionWithValidation([]SurveyQuestionWithValidationArgs{ { Icon: IconCloudTrail, diff --git a/integration/aws_generation_test.go b/integration/aws_generation_test.go index d0eb26579..911b0906f 100644 --- a/integration/aws_generation_test.go +++ b/integration/aws_generation_test.go @@ -156,7 +156,7 @@ func TestGenerationAwsNoninteractive(t *testing.T) { aws.WithConfigOrgCfResourcePrefix(cfResourcePrefix), aws.WithConsolidatedCloudtrail(true), aws.WithOrgAccountMappings(orgAccountMappings), - aws.WithCloudtrailUseExistingS3(true), + aws.WithCloudtrailUseExistingTrail(true), aws.WithCloudtrailName(cloudtrailName), aws.WithExistingCloudtrailBucketArn(s3BucketArn), aws.WithCloudtrailUseExistingSNSTopic(true), @@ -397,6 +397,7 @@ func TestGenerationAwsCloudtrail(t *testing.T) { MsgRsp{cmd.QuestionEnableConfig, "n"}, MsgRsp{cmd.QuestionEnableCloudtrail, "y"}, MsgRsp{cmd.QuestionCloudtrailUseConsolidated, "y"}, + MsgRsp{cmd.QuestionCloudtrailUseExistingTrail, "n"}, MsgRsp{cmd.QuestionCloudtrailAdvanced, "n"}, MsgRsp{cmd.QuestionAwsOutputLocation, ""}, MsgRsp{cmd.QuestionRunTfPlan, "n"}, @@ -438,6 +439,7 @@ func TestGenerationAwsCloudtrailOrganization(t *testing.T) { MsgRsp{cmd.QuestionEnableCloudtrail, "y"}, MsgRsp{cmd.QuestionControlTower, "n"}, MsgRsp{cmd.QuestionCloudtrailUseConsolidated, "y"}, + MsgRsp{cmd.QuestionCloudtrailUseExistingTrail, "n"}, MsgRsp{cmd.QuestionCloudtrailAdvanced, "y"}, MsgMenu{cmd.OptCloudtrailOrg, 0}, MsgRsp{cmd.QuestionCloudtrailOrgAccountMappingsDefaultLWAccount, "main"}, @@ -551,14 +553,15 @@ func TestGenerationAwsCloudtrailControlTower(t *testing.T) { assert.Equal(t, buildTf, string(tfResult)) } -// Test CloudTrail integration with existing S3 bucket -func TestGenerationAwsCloudtrailWithExistingS3Bucket(t *testing.T) { +// Test CloudTrail integration with existing trail +func TestGenerationAwsCloudtrailWithExistingTrail(t *testing.T) { os.Setenv("LW_NOCACHE", "true") defer os.Setenv("LW_NOCACHE", "") var final string cloudtrailName := "cloudtrail-integration-name" s3BucketArn := "arn:aws:s3:::bucket-name" + snsTopicArn := "arn:aws:sns:us-east-2:249446771485:topic-name" // Run CLI tfResult := runGenerateTest(t, @@ -571,12 +574,12 @@ func TestGenerationAwsCloudtrailWithExistingS3Bucket(t *testing.T) { MsgRsp{cmd.QuestionEnableConfig, "n"}, MsgRsp{cmd.QuestionEnableCloudtrail, "y"}, MsgRsp{cmd.QuestionCloudtrailUseConsolidated, "n"}, - MsgRsp{cmd.QuestionCloudtrailAdvanced, "y"}, - MsgMenu{cmd.OptCloudtrailS3, 0}, - MsgRsp{cmd.QuestionCloudtrailUseExistingS3, "y"}, + MsgRsp{cmd.QuestionCloudtrailUseExistingTrail, "y"}, MsgRsp{cmd.QuestionCloudtrailName, cloudtrailName}, MsgRsp{cmd.QuestionCloudtrailS3ExistingBucketArn, s3BucketArn}, - MsgMenu{cmd.OptCloudtrailDone, 4}, + MsgRsp{cmd.QuestionCloudtrailUseExistingSNSTopic, "y"}, + MsgRsp{cmd.QuestionCloudtrailSnsExistingTopicArn, snsTopicArn}, + MsgRsp{cmd.QuestionCloudtrailAdvanced, "n"}, MsgRsp{cmd.QuestionAwsOutputLocation, ""}, MsgRsp{cmd.QuestionRunTfPlan, "n"}, }) @@ -594,9 +597,11 @@ func TestGenerationAwsCloudtrailWithExistingS3Bucket(t *testing.T) { buildTf, _ := aws.NewTerraform(false, false, false, true, aws.WithAwsProfile("main"), aws.WithAwsRegion("us-east-2"), - aws.WithCloudtrailUseExistingS3(true), + aws.WithCloudtrailUseExistingTrail(true), aws.WithCloudtrailName(cloudtrailName), aws.WithExistingCloudtrailBucketArn(s3BucketArn), + aws.WithCloudtrailUseExistingSNSTopic(true), + aws.WithExistingSnsTopicArn(snsTopicArn), ).Generate() assert.Equal(t, buildTf, tfResult) } @@ -621,9 +626,9 @@ func TestGenerationAwsCloudtrailWithNewS3Bucket(t *testing.T) { MsgRsp{cmd.QuestionEnableConfig, "n"}, MsgRsp{cmd.QuestionEnableCloudtrail, "y"}, MsgRsp{cmd.QuestionCloudtrailUseConsolidated, "n"}, + MsgRsp{cmd.QuestionCloudtrailUseExistingTrail, "n"}, MsgRsp{cmd.QuestionCloudtrailAdvanced, "y"}, MsgMenu{cmd.OptCloudtrailS3, 0}, - MsgRsp{cmd.QuestionCloudtrailUseExistingS3, "n"}, MsgRsp{cmd.QuestionCloudtrailS3BucketName, s3BucketName}, MsgRsp{cmd.QuestionCloudtrailS3BucketEnableEncryption, "y"}, MsgRsp{cmd.QuestionCloudtrailS3BucketSseKeyArn, kmsArn}, @@ -646,7 +651,7 @@ func TestGenerationAwsCloudtrailWithNewS3Bucket(t *testing.T) { buildTf, _ := aws.NewTerraform(false, false, false, true, aws.WithAwsProfile("main"), aws.WithAwsRegion("us-east-2"), - aws.WithCloudtrailUseExistingS3(false), + aws.WithCloudtrailUseExistingTrail(false), aws.WithBucketName(s3BucketName), aws.WithBucketEncryptionEnabled(true), aws.WithBucketSSEKeyArn(kmsArn), @@ -655,53 +660,6 @@ func TestGenerationAwsCloudtrailWithNewS3Bucket(t *testing.T) { assert.Equal(t, buildTf, tfResult) } -// Test CloudTrail integration with existing SNS topic -func TestGenerationAwsCloudtrailWithExistingSnsTopic(t *testing.T) { - os.Setenv("LW_NOCACHE", "true") - defer os.Setenv("LW_NOCACHE", "") - var final string - - snsTopicArn := "arn:aws:sns:us-east-2:249446771485:topic-name" - - // Run CLI - tfResult := runGenerateTest(t, - func(c *expect.Console) { - expectsCliOutput(t, c, []MsgRspHandler{ - MsgRsp{cmd.QuestionEnableAwsOrganization, "n"}, - MsgRsp{cmd.QuestionMainAwsProfile, "main"}, - MsgRsp{cmd.QuestionMainAwsRegion, "us-east-2"}, - MsgRsp{cmd.QuestionEnableAgentless, "n"}, - MsgRsp{cmd.QuestionEnableConfig, "n"}, - MsgRsp{cmd.QuestionEnableCloudtrail, "y"}, - MsgRsp{cmd.QuestionCloudtrailUseConsolidated, "n"}, - MsgRsp{cmd.QuestionCloudtrailAdvanced, "y"}, - MsgMenu{cmd.OptCloudtrailSNS, 1}, - MsgRsp{cmd.QuestionCloudtrailUseExistingSNSTopic, "y"}, - MsgRsp{cmd.QuestionCloudtrailSnsExistingTopicArn, snsTopicArn}, - MsgMenu{cmd.OptCloudtrailDone, 4}, - MsgRsp{cmd.QuestionAwsOutputLocation, ""}, - MsgRsp{cmd.QuestionRunTfPlan, "n"}, - }) - final, _ = c.ExpectEOF() - }, - "generate", - "cloud-account", - "aws", - ) - - // Ensure CLI ran correctly - assert.Contains(t, final, "Terraform code saved in") - - // Create the TF directly with lwgenerate and validate same result via CLI - buildTf, _ := aws.NewTerraform(false, false, false, true, - aws.WithAwsProfile("main"), - aws.WithAwsRegion("us-east-2"), - aws.WithCloudtrailUseExistingSNSTopic(true), - aws.WithExistingSnsTopicArn(snsTopicArn), - ).Generate() - assert.Equal(t, buildTf, tfResult) -} - // Test CloudTrail integration with new SNS topic func TestGenerationAwsCloudtrailWithNewSNSTopic(t *testing.T) { os.Setenv("LW_NOCACHE", "true") @@ -722,9 +680,9 @@ func TestGenerationAwsCloudtrailWithNewSNSTopic(t *testing.T) { MsgRsp{cmd.QuestionEnableConfig, "n"}, MsgRsp{cmd.QuestionEnableCloudtrail, "y"}, MsgRsp{cmd.QuestionCloudtrailUseConsolidated, "n"}, + MsgRsp{cmd.QuestionCloudtrailUseExistingTrail, "n"}, MsgRsp{cmd.QuestionCloudtrailAdvanced, "y"}, MsgMenu{cmd.OptCloudtrailSNS, 1}, - MsgRsp{cmd.QuestionCloudtrailUseExistingSNSTopic, "n"}, MsgRsp{cmd.QuestionCloudtrailSnsTopicName, snsTopicName}, MsgRsp{cmd.QuestionCloudtrailSnsEnableEncryption, "y"}, MsgRsp{cmd.QuestionCloudtrailSnsEncryptionKeyArn, kmsArn}, @@ -774,6 +732,7 @@ func TestGenerationAwsCloudtrailWithNewSQSQueue(t *testing.T) { MsgRsp{cmd.QuestionEnableConfig, "n"}, MsgRsp{cmd.QuestionEnableCloudtrail, "y"}, MsgRsp{cmd.QuestionCloudtrailUseConsolidated, "n"}, + MsgRsp{cmd.QuestionCloudtrailUseExistingTrail, "n"}, MsgRsp{cmd.QuestionCloudtrailAdvanced, "y"}, MsgMenu{cmd.OptCloudtrailSQS, 2}, MsgRsp{cmd.QuestionCloudtrailSqsQueueName, sqsQueueName}, @@ -825,6 +784,7 @@ func TestGenerationAwsCloudtrailWithExistingIamRole(t *testing.T) { MsgRsp{cmd.QuestionEnableConfig, "n"}, MsgRsp{cmd.QuestionEnableCloudtrail, "y"}, MsgRsp{cmd.QuestionCloudtrailUseConsolidated, "n"}, + MsgRsp{cmd.QuestionCloudtrailUseExistingTrail, "n"}, MsgRsp{cmd.QuestionCloudtrailAdvanced, "y"}, MsgMenu{cmd.OptCloudtrailIAM, 3}, MsgRsp{cmd.QuestionCloudtrailExistingIamRoleName, roleName}, diff --git a/lwgenerate/aws/aws.go b/lwgenerate/aws/aws.go index 0f6c5c8b1..99b89ef8e 100644 --- a/lwgenerate/aws/aws.go +++ b/lwgenerate/aws/aws.go @@ -171,8 +171,8 @@ type GenerateAwsTfConfigurationArgs struct { // OrgAccountMapping json used for flag input OrgAccountMappingsJson string - // Use exisiting CloudTrail S3 bucket - CloudtrailUseExistingS3 bool + // Use exisiting CloudTrail + CloudtrailUseExistingTrail bool // Use exisiting CloudTrail SNS topic CloudtrailUseExistingSNSTopic bool @@ -352,6 +352,30 @@ func (args *GenerateAwsTfConfigurationArgs) Validate() error { } } + if args.Cloudtrail { + if args.CloudtrailUseExistingTrail { + if args.CloudtrailName == "" { + return errors.New("must specify the existing trail name when integrating with existing CloudTrail") + } + if args.ExistingCloudtrailBucketArn == "" { + return errors.New( + fmt.Sprintf( + "must specify the S3 bucket ARN associated with the trail %s "+ + " when integrating with existing CloudTrail", args.CloudtrailName, + ), + ) + } + if args.ExistingSnsTopicArn == "" && !args.S3BucketNotification { + return errors.New( + fmt.Sprintf( + "must either specify the SNS topic ARN associated with the trail %s "+ + "or enable S3 notification when integrating with existing CloudTrail", args.CloudtrailName, + ), + ) + } + } + } + return nil } @@ -549,10 +573,10 @@ func WithControlTowerKmsKeyArn(kmsKeyArn string) AwsTerraformModifier { } } -// WithCloudtrailUseExistingS3 Use the existing Cloudtrail S3 bucket -func WithCloudtrailUseExistingS3(useExistingS3 bool) AwsTerraformModifier { +// WithCloudtrailUseExistingTrail Use the existing Cloudtrail S3 bucket +func WithCloudtrailUseExistingTrail(useExistingS3 bool) AwsTerraformModifier { return func(c *GenerateAwsTfConfigurationArgs) { - c.CloudtrailUseExistingS3 = useExistingS3 + c.CloudtrailUseExistingTrail = useExistingS3 } } @@ -944,14 +968,10 @@ func createCloudtrail(args *GenerateAwsTfConfigurationArgs) (*hclwrite.Block, er attributes["consolidated_trail"] = true } // S3 Bucket attributes - if args.CloudtrailUseExistingS3 { + if args.CloudtrailUseExistingTrail { attributes["use_existing_cloudtrail"] = true - if args.CloudtrailName != "" { - attributes["cloudtrail_name"] = args.CloudtrailName - } - if args.ExistingCloudtrailBucketArn != "" { - attributes["bucket_arn"] = args.ExistingCloudtrailBucketArn - } + attributes["cloudtrail_name"] = args.CloudtrailName + attributes["bucket_arn"] = args.ExistingCloudtrailBucketArn } else { if args.BucketName != "" { attributes["bucket_name"] = args.BucketName diff --git a/lwgenerate/aws/aws_test.go b/lwgenerate/aws/aws_test.go index 3450c916c..ea424a07c 100644 --- a/lwgenerate/aws/aws_test.go +++ b/lwgenerate/aws/aws_test.go @@ -257,7 +257,7 @@ func TestGenerationCloudtrailExistingBucket(t *testing.T) { existingBucketArn := "arn:aws:s3:::test-bucket-12345" data, err := createCloudtrail(&GenerateAwsTfConfigurationArgs{ Cloudtrail: true, - CloudtrailUseExistingS3: true, + CloudtrailUseExistingTrail: true, ExistingCloudtrailBucketArn: existingBucketArn, }) assert.Nil(t, err)